On Mon, 2015-02-16 at 15:54 +1100, NeilBrown wrote:
> On Sun, 15 Feb 2015 23:28:12 -0500 Trond Myklebust
> <trond.myklebust@xxxxxxxxxxxxxxx> wrote:
>
> > On Sun, Feb 15, 2015 at 9:46 PM, NeilBrown <neilb@xxxxxxx> wrote:
> > > On Sat, 14 Feb 2015 13:17:00 +0000 Nix <nix@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
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@xxxxxxx> for having tracked down this
issue (see link).
Reported-by: Nix <nix@xxxxxxxxxxxxx>
Link: http://lkml.kernel.org/r/87iofju9ht.fsf@xxxxxxxxxxxxxxxx
Fixes: d39ab9de3b80 ("NFS: re-add readdir plus")
Cc: stable@xxxxxxxxxxxxxxx # 2.6.27+
Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
