lkml.org 
[lkml]   [2009]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 04/20] sysfs: Handle the general case of removing of directories with subdirectories
    Tejun Heo <tj@kernel.org> writes:

    > Eric W. Biederman wrote:
    >> From: Eric W. Biederman <ebiederm@xmission.com>
    >>
    >> 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@aristanetworks.com>
    > ...
    >> -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


    \
     
     \ /
      Last update: 2009-05-21 09:31    [W:2.227 / U:0.544 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site