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

[Tejun Heo]

Hi,

Eric W. Biederman wrote:
>>> The locking is correct, something that is much more difficult to
>>> tell with your version.
>> Why? :-)
> 
> Because mine is all in a single place and there is no optimization
> to get locks I don't need.
> 
> Unless I have misread your patch you are failing to get the
> i_mutex for child directories, if it possible to get it.
> 
> Something that it is trivial to see that I always do correctly.
> Simply because the distance between the lock and where I depend on
> it is so small.

If this patch series works out, we don't need to grab i_mutexes while
manipulating sd's, right?

>>> I would prefer to only remove empty directories.  But when I
>>> instrumented things up I found cases where that does indeed happen.
>> IIRC, my version did the whole thing while holding sysfs_mutex, so
>> it's safe against such races.  I can't really see why ops like this
>> can't be atomic in sysfs.  I don't really care how things are done but
>> please make it atomic.
> 
> Nope.  Holding the sysfs_mutex does not make you safe from such races.
> It actually makes you more prone to someone adding a directory entry to
> a deleted directory and not having it deleted.  I have a chance of
> deleting the added directory entry.
> 
> The problem is that sysfs_add_one takes to sysfs_dirents.  The look up
> of the directory is done before we take the sysfs_mutex.  So the
> sysfs_dirent could be grabbed at any time.

Well, it can be trivially fixed by checking the removed flag.  The
add/rm thing is designed to help additions and removals of multiple
nodes at one go and I'd really like to see it working that way.  Any
chance you can change code toward that direction?

Thanks.

-- 
tejun
--
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/