Re: [PATCH] NFS: nfs4_lookup_revalidate need to report STALE inodes.

[NeilBrown <neilb@suse.de>]

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

On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton <jeff.layton@primarydata.com>
wrote:

> On Mon, 14 Jul 2014 15:14:05 +1000
> NeilBrown <neilb@suse.de> 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??


> 
> 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 <neilb@suse.de>
> > 
> > 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 <jlayton@primarydata.com>

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]