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

[Jeff Layton <jeff.layton@primarydata.com>]

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

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?

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.

Are we not properly updating the dcache (and attrcache) after the OPEN
reply?

> 
> 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>

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