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-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
