Re: what on earth is going on here? paths above mountpoints turn into "(unreachable)"

[Trond Myklebust <trond.myklebust@primarydata.com>]

On Mon, 2015-02-16 at 15:54 +1100, NeilBrown wrote:
> On Sun, 15 Feb 2015 23:28:12 -0500 Trond Myklebust
> <trond.myklebust@primarydata.com> wrote:
> 
> > On Sun, Feb 15, 2015 at 9:46 PM, NeilBrown <neilb@suse.de> wrote:
> > > On Sat, 14 Feb 2015 13:17:00 +0000 Nix <nix@esperi.org.uk> wrote:
> > >
> > >> On 10 Feb 2015, J. Bruce Fields outgrape:
> > >>
> > >> > It might be interesting to see output from
> > >> >
> > >> >     rpc.debug -m rpc -s cache
> > >> >     cat /proc/net/rpc/nfsd.export/content
> > >> >     cat /proc/net/rpc/nfsd.fh/content
> > >> >
> > >> > especially after the problem manifests.
> > >>
> > >> So the mount has vanished again. I couldn't make it happen with
> > >> nordirplus in the mount options, so that might provide you with a clue.
> > >
> > > Yup.  It does.
> > >
> > > There is definitely something wrong in nfs_prime_dcache.  I cannot quite
> > > trace through from cause to effect, but maybe I don't need to.
> > >
> > > Can you try the following patch and see if that makes the problem disappear?
> > >
> > > When you perform a READDIRPLUS request on a directory that contains
> > > mountpoints, the the Linux NFS server doesn't return a file-handle for
> > > those names which are mountpoints (because doing so is a bit tricky).
> > >
> > > nfs3_decode_dirent notices and decodes as a filehandle with zero length.
> > >
> > > The "nfs_same_file()" check in nfs_prime_dcache() determines that isn't
> > > the same as the filehandle it has, and tries to invalidate it and make a new
> > > one.
> > >
> > > The invalidation should fail (probably does).
> > > The creating of a new one ... might succeed.  Beyond that, it all gets a bit
> > > hazy.
> > >
> > > Anyway, please try:
> > >
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index 9b0c55cb2a2e..a460669dc395 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -541,7 +541,7 @@ int nfs_readdir_page_filler(nfs_readdir_descriptor_t *desc, struct nfs_entry *en
> > >
> > >                 count++;
> > >
> > > -               if (desc->plus != 0)
> > > +               if (desc->plus != 0 && entry->fh.size)
> > >                         nfs_prime_dcache(desc->file->f_path.dentry, entry);
> > >
> > >                 status = nfs_readdir_add_to_array(entry, page);
> > >
> > >
> > > which you might have to apply by hand.
> > 
> > Doesn't that check ultimately belong in nfs_fget()? It would seem to
> > apply to all filehandles, irrespective of provenance.
> > 
> 
> Maybe.  Though I think it also needs to be before nfs_prime_dcache() tries to
> valid the dentry it found.
> e.g.
> 
>  if (dentry != NULL) {
>     if (entry->fh->size == 0)
>        goto out;
>     else if (nfs_same_file(..)) {
> 	....
>     else {
>         d_invalidate();
>         ...
>     }
>   }
> 
> ??
> 
> I'd really like to understand what is actually happening though.
> d_invalidate() shouldn't effect an unmount.
> 
> Maybe the dentry that gets mounted on is the one with the all-zero fh...

Commit 8ed936b5671bf (v3.18+) changes d_invalidate() to unmount the
subtree on a directory being invalidated.

I disagree that the problem here is the zero length filehandle. It is
rather that we need to accommodate situations where the server is
setting us up for a submount or a NFSv4 referral.
In that situation, it is perfectly OK for nfs_prime_dcache() to create
an entry for the mounted-on file. It's just not OK for it to invalidate
the dentry if the submount was already performed.

So how about the following alternative patch?

8<----------------------------------------------------------------
>From 1c8194f2147c10fc7a142eda4f6d7f35ae1f7d4f Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Sun, 22 Feb 2015 16:35:36 -0500
Subject: [PATCH] NFS: Don't invalidate a submounted dentry in
 nfs_prime_dcache()

If we're traversing a directory which contains a submounted filesystem,
or one that has a referral, the NFS server that is processing the READDIR
request will often return information for the underlying (mounted-on)
directory. It may, or may not, also return filehandle information.

If this happens, and the lookup in nfs_prime_dcache() returns the
dentry for the submounted directory, the filehandle comparison will
fail, and we call d_invalidate(). Post-commit 8ed936b5671bf
("vfs: Lazily remove mounts on unlinked files and directories."), this
means the entire subtree is unmounted.

The following minimal patch addresses this problem by punting on
the invalidation if there is a submount.

Cudos to Neil Brown <neilb@suse.de> for having tracked down this
issue (see link).

Reported-by: Nix <nix@esperi.org.uk>
Link: http://lkml.kernel.org/r/87iofju9ht.fsf@spindle.srvr.nix
Fixes: d39ab9de3b80 ("NFS: re-add readdir plus")
Cc: stable@vger.kernel.org # 2.6.27+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/dir.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 43e29e3e3697..c35ff07b7345 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -485,10 +485,10 @@ void nfs_prime_dcache(struct dentry *parent, struct nfs_entry *entry)
 			if (!status)
 				nfs_setsecurity(dentry->d_inode, entry->fattr, entry->label);
 			goto out;
-		} else {
-			d_invalidate(dentry);
-			dput(dentry);
-		}
+		} else if (IS_ROOT(dentry))
+			goto out;
+		d_invalidate(dentry);
+		dput(dentry);
 	}
 
 	dentry = d_alloc(parent, &filename);
-- 
2.1.0


-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com