Re: [PATCH 04/20] sysfs: Handle the general case of removing of directories with subdirectories

[Eric W. Biederman]

Tejun Heo <tj@xxxxxxxxxx> writes:

> Eric W. Biederman wrote:
>> From: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>> 
>> Modify sysfs to properly remove directories containing attributes and
>> subdirectories.  The code is relatively simple and means we don't have
>> to worry about what might use this logic.
>> 
>> In a quick survey I have only found /sys/dev/char and /sys/dev/block that are
>> removing non-enmpty directories today (and they are exclusively filled with symlinks).
>> So only removing empty directories does not appear to be an option.
>> 
>> I don't hold sysfs_mutex across the entire operation as that is unneeded
>> for coherence at the sysfs level and some level of coordination is expected
>> at the upper layers.
>> 
>> Signed-off-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxxxxxxxx>
> ...
>> -void sysfs_remove_subdir(struct sysfs_dirent *sd)
>> -{
>> -	remove_dir(sd);
>> +	struct sysfs_dirent *sd = dir_sd;
>> +	mutex_lock(&sysfs_mutex);
>> +	while ((sysfs_type(sd) == SYSFS_DIR) && sd->s_dir.children)
>> +		sd = sd->s_dir.children;
>> +	if (sd != dir_sd)
>> +		sysfs_get(sd);
>> +	else
>> +		sd = NULL;
>> +	mutex_unlock(&sysfs_mutex);
>> +	return sd;
>>  }
>
> Some blank lines wouldn't hurt, especially after local variable
> declaration.
>
>> -static void __sysfs_remove_dir(struct sysfs_dirent *dir_sd)
>> +static void remove_dir(struct sysfs_dirent *dir_sd)
>>  {
>>  	struct sysfs_addrm_cxt acxt;
>> -	struct sysfs_dirent **pos;
>> -
>> -	if (!dir_sd)
>> -		return;
>> +	struct sysfs_dirent *sd;
>>  
>>  	pr_debug("sysfs %s: removing dir\n", dir_sd->s_name);
>> -	sysfs_addrm_start(&acxt, dir_sd);
>> -	pos = &dir_sd->s_dir.children;
>> -	while (*pos) {
>> -		struct sysfs_dirent *sd = *pos;
>>  
>> -		if (sysfs_type(sd) != SYSFS_DIR)
>> -			sysfs_remove_one(&acxt, sd);
>> -		else
>> -			pos = &(*pos)->s_sibling;
>> +	while ((sd = sysfs_get_one(dir_sd))) {
>> +		sysfs_addrm_start(&acxt, sd->s_parent);
>> +		sysfs_remove_one(&acxt, sd);
>> +		sysfs_addrm_finish(&acxt);
>> +		sysfs_put(sd);
>>  	}
>> +	sysfs_addrm_start(&acxt, dir_sd->s_parent);
>> +	sysfs_remove_one(&acxt, dir_sd);
>>  	sysfs_addrm_finish(&acxt);
>> +}
>
> I agree we should be heading this way but what happens to attributes
> or directories living below the subdirectories?  If it's gonna handle
> recursive case, I think it better do it properly.  I had patches of
> similar effect.

I do handle it properly.  sysfs_get_one finds the deepest child of the
first directory entry.  Then I remove it.  And I repeat until done.

The locking is correct, something that is much more difficult to
tell with your version.

By grabbing and dropping the sysfs_mutex things are simpler, and they
get even simpler in future patches.



Now looking at that code in detail there is a question of what happens if
we add a directory entry while we are recursively deleting a directory.
Neither your patch, my patch, nor the existing code handle that case
(assuming the sysfs_dirent) was looked up before it is removed from it's
parent directory.  I expect another patch is called for to plug that
theoretical gap.  

I expect the way to close that hole is to have an extra flag that says
we are removing a directory entry and refuse to add if that flag is
set.

I would prefer to only remove empty directories.  But when I
instrumented things up I found cases where that does indeed happen.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/