On Fri, Mar 18, 2011 at 11:06:03AM +0000, Al Viro wrote:
> That's incredibly ugly. I agree that the deadlock exists and needs to be
> dealt with, but not that way. _Strongly_ NAKed. I'll see what I can come
> up with, but that variant is not an option.
Actually, why do we hold vfsmount_lock over that loop at all? We already
hold namespace_sem, so ->mnt_parent is protected...
FWIW, there's a thing I really don't like about that area - I would really
prefer to have namespace_sem nest _inside_ i_mutex and have no fs operations
whatsoever done under it. Note that we already take care (mostly) to
keep actual fs shutdowns outside of it.
The real obstacle is follow_down() we do under namespace_sem on several
paths; otherwise we'd be able to grab i_mutex first and be done with that.
Moreover, we have extra ugliness in follow_down(); I really wonder
what is the only instance trying to do here.
Look: we pass mounting_here == true to autofs4_d_manage(). It sees the
flag, then checks if we have DCACHE_MOUNTED set and returns either -EISDIR
or 0. -EISDIR leads to follow_down() returning 0 immediately. 0 leads
to trying to cross the mountpoint. First of all, we could as well return
0 regardless - if DCACHE_MOUNTED is not set, follow_down() will see that
and return 0 since there's nothing left to do.
What's more, we have a funny situation here - we might as well replace
if (managed & DCACHE_MANAGE_TRANSIT) {
with
if (!mounting_here && managed & DCACHE_MANAGE_TRANSIT) {
in follow_down() and lose that argument of ->d_manage().
So let's do this:
lock_mount(path, follow)
retry:
lock path->dentry->d_inode
if unlikely(can't mount)
unlock
fail
grab namespace_sem
if !follow || likely(lookup_mnt() returns NULL)
we are done
drop namespace_sem
unlock
drop path
replace it with result of lookup_mnt() (and its ->mnt_root)
goto retry;
and use that (local to fs/namespace.c) in do_add_mount()/do_move_mount()/
do_loopback() (with follow = 1) and pivot_root() (follow = 0). BTW,
the lack of following in do_loopback() looks like a bug...
Also in do_loopback(): take release_mounts() after unlocking everything.
graft_tree() doesn't grab i_mutex - callers take it.
follow_down() loses mounting_here argument and so does ->d_manage().
cant_mount() calls are all merged into one in lock_mount().
Comments?
--
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
