NFS: nfs4_lookup_revalidate need to report STALE inodes.

Commit Message

Jeff Layton July 14, 2014, 1 p.m. UTC

On Mon, 14 Jul 2014 22:35:13 +1000
NeilBrown <[e-mail]> wrote:

> On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton <[e-mail]>
> wrote:
> 
> > On Mon, 14 Jul 2014 15:14:05 +1000
> > NeilBrown <[e-mail]> wrote:
> > 
> > > 
> > > If an 'open' of a file in an NFSv4 filesystem finds that the dentry is
> > > in cache, but the inode is stale (on the server), the dentry will not
> > > be re-validated immediately and may cause ESTALE to be returned to
> > > user-space.
> > > 
> > > For a non-create 'open', do_last() calls lookup_fast() and on success
> > > will eventually call may_open() which calls into nfs_permission().
> > > If nfs_permission() makes the ACCESS call to the server it will get
> > > NFS4ERR_STALE, resulting in ESTALE from may_open() and thence from
> > > do_last().
> > > The retry-on-ESTALE in filename_lookup() will repeat exactly the same
> > > process because nothing in this path will invalidate the dentry due to
> > > the inode being stale, so the ESTALE will be returned.
> > > 
> > > lookup_fast() calls ->d_revalidate(), but for an OPEN on an NFSv4
> > > filesystem, that will succeed for regular files:
> > > 	/* Let f_op->open() actually open (and revalidate) the file */
> > > 
> > > Unfortunately in the case of a STALE inode, f_op->open() never gets
> > > called.  If we teach nfs4_lookup_revalidate() to report a failure on
> > > NFS_STALE() inodes, then the dentry will be invalidated and a full
> > > lookup will be attempted.  The ESTALE errors go away.
> > > 
> > > 
> > > While I think this fix is correct, I'm not convinced that it is
> > > sufficient, particularly if lookupcache=none.
> > > The current code will fail an "open" is nfs_permission() fails,
> > > without having performed a LOOKUP. i.e. it will use the cache.
> > > nfs_lookup_revalidate will force a lookup before the permission check
> > > if NFS_MOUNT_LOOKUP_CACHE_NONE, but nfs4_lookup_revalidate will not.
> > > 
> > 
> > This patch should make the code fall through to nfs_lookup_revalidate,
> > which would then force the lookup, right?
> 
> Yes ... though maybe that's not what I really want to do.  I really wanted to
> just return '0', though I would need to check that is right in all cases.
> 
> > 
> > Also, I'm a little unclear...
> > 
> > Why would may_open fail with ESTALE after the v4 OPEN succeeds? The
> > OPEN should be returning a filehandle and attributes for the inode
> > actually opened. It seems like we ought to be doing any permission
> > checks vs. that inode, not anything we had in cache. Presumably the
> > server is then holding it open so it shouldn't be stale.
> 
> may_open is called *before* and v4 OPEN.
> 
> In do_last, if the inode is already in cache, then
>   lookup_fast is called, which calls d_revalidate
>   then may_open (calls ->permission)
>   then finish_open which calls f_op->open
> 
> Yes, we should be doing permission checking against whatever 'open' finds.
> But the VFS is structured to the the permission check after d_revalidate and
> before ->open.  So maybe d_revalidate needs to do the NFS open??
> 

Ok, I see. Ugh, having the revalidate do the open sounds...messy.

A simpler fix might be to fix it so that an -ESTALE return from
may_open triggers a retry. Something like this maybe (probably
whitespace damaged, so just for discussion purposes):

...though might need to convert the ESTALE to EOPENSTALE there too, not
sure...

> 
> > 
> > Are we not properly updating the dcache (and attrcache) after the
> > OPEN reply?
> 
> I think so, yes.  But in the problem case, we don't even send an OPEN
> request.
> 
> 
> > 
> > > 
> > > Signed-off-by: NeilBrown <[e-mail]>
> > > 
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index 4a3d4ef76127..4f7414afca27 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -1563,6 +1563,8 @@ static int nfs4_lookup_revalidate(struct
> > > dentry *dentry, unsigned int flags) /* We cannot do exclusive
> > > creation on a positive dentry */ if (flags & LOOKUP_EXCL)
> > >  		goto no_open_dput;
> > > +	if (NFS_STALE(inode))
> > > +		goto no_open_dput;
> > >  
> > >  	/* Let f_op->open() actually open (and revalidate) the
> > > file */ ret = 1;
> > 
> > Looks legit to me too, but it seems like the inode could go stale
> > w/o us knowing after this point.
> > 
> > Acked-by: Jeff Layton <[e-mail]>
> 
> Thanks,
> NeilBrown

Patch

diff --git a/fs/namei.c b/fs/namei.c
index 985c6f368485..c1657deea52c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3045,8 +3045,13 @@  finish_open:
        }
 finish_open_created:
        error = may_open(&nd->path, acc_mode, open_flag);
-       if (error)
+       if (error) {
+               if (error == -ESTALE)
+                       goto stale_open;
                goto out;
+       }
        file->f_path.mnt = nd->path.mnt;
        error = finish_open(file, nd->path.dentry, NULL, opened);
        if (error) {