Re: [PATCH RFC] NFSD: fix cannot umounting mount points under pseudo root

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2833 bytes --]

On Fri, 1 May 2015 03:03:24 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote:

> On Fri, May 01, 2015 at 11:53:26AM +1000, NeilBrown wrote:
> > While writing that I began to wonder if lookup_one_len is really the right
> > interface to be used, even though it was introduced (in 2.3.99pre2-4)
> > specifically for nfsd.
> > The problem is that it assumes things about the filesystem.  So it makes
> > perfect sense for various filesystems to use it on themselves, but I'm not
> > sure how *right* it is for nfsd (or cachefiles etc) to use it on some
> > *other* filesystem.
> > The particular issue is that it avoids the d_revalidate call.
> > Both vfat and reiserfs have that call ... I wonder if that could ever be a
> > problem.
> > 
> > So I'm really leaning towards creating a variant of kern_path_mountpoint and
> > using a variant of that which takes a length.
> 
> NAK.  As in, "no way in hell".  And yes, lookup_one_len() *does* revalidate -
> RTFS(lookup_dcache), please.

Damn - I always seems to get lost when I'm following those call paths.
 lookup_one_len -> __lookup_hash -> lookup_dcache -> d_lookup,d_revalidate -> __d_lookup
                                 -> lookup_real -> i_op->lookup

I think I was confusing __lookup_hash with __d_lookup in my thoughts.

> 
> What kind of consistency warranties do callers expect, BTW?  You do realize
> that between iterate_dir() and callbacks an entry might have been removed
> and/or replaced?

For READDIR_PLUS, lookup_one_len is called on each name and it requires
i_mutex, so the code currently holds i_mutex over the whole sequence.
This is triggering a deadlock.

We could just grab/drop i_mutex over each call to lookup_one_len(), but that
sort of thing is usually frowned upon, and we don't really always *need*
i_mutex if the lookup can be served from the d_cache.

So I'm looking for the best way to perform the lookup without holding i_mutex
for too long.

It sounds like you are suggesting something like lookup_one_len_unlocked(),
which .... uhm...

I was going to say uses lookup_dcache, but that needs i_mutex.
It calls d_lookup(), which doesn't seem to really need i_mutex, and
d_revalidate().
Does the later need i_mutex?  I don't think so.
So maybe it is just how d_lookup handles failure that needs i_mutex.

So lookup_one_len_unlocked() could call d_lookup and d_revalidate and if
that all worked nicely, return the result. If it didn't, grab i_mutex and try
again??

Or do we just wear the cost of taking i_mutex for each name in the directory
during READDIR_PLUS?

Thanks,
NeilBrown


> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]