Re: [PATCH v2] NFS: Revalidate once when holding a delegation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 2020-01-28 at 10:26 -0500, Benjamin Coddington wrote:
> If we skip lookup revalidataion while holding a delegation, we might
> miss
> that the file has changed directories on the server.  This can happen
> if
> the file is moved in between the client caching a dentry and
> obtaining a
> delegation.  That can be reproduced on a single system with this
> bash:
> 
> mkdir -p /exports/dir{1,2}
> exportfs -o rw localhost:/exports
> mount -t nfs -ov4.1,noac localhost:/exports /mnt/localhost
> 
> touch /exports/dir1/fubar
> 
> cat /mnt/localhost/dir1/fubar
> mv /mnt/localhost/dir{1,2}/fubar
> 
> mv /exports/dir{2,1}/fubar
> 
> cat /mnt/localhost/dir1/fubar
> mv /mnt/localhost/dir{1,2}/fubar
> 
> In this example, the final `mv` will stat source and destination and
> find
> they are the same dentry.
> 
> To fix this without giving up the increased lookup performance that
> holding
> a delegation provides, let's revalidate the dentry only once after
> obtaining a delegation by placing a magic value in the dentry's
> verifier.
> 
> Suggested-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
> Signed-off-by: Benjamin Coddington <bcodding@xxxxxxxxxx>
> ---
>  fs/nfs/dir.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index e180033e35cf..e9d07dcd6d6f 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -949,6 +949,7 @@ static int nfs_fsync_dir(struct file *filp,
> loff_t start, loff_t end,
>  	return 0;
>  }
>  
> +#define NFS_DELEGATION_VERF 0xfeeddeaf
>  /**
>   * nfs_force_lookup_revalidate - Mark the directory as having
> changed
>   * @dir: pointer to directory inode
> @@ -962,6 +963,8 @@ static int nfs_fsync_dir(struct file *filp,
> loff_t start, loff_t end,
>  void nfs_force_lookup_revalidate(struct inode *dir)
>  {
>  	NFS_I(dir)->cache_change_attribute++;
> +	if (NFS_I(dir)->cache_change_attribute == NFS_DELEGATION_VERF)
> +		NFS_I(dir)->cache_change_attribute++;

Actually, I think a slight modification to this can might be
beneficial. If we change to the following:

	if (unlikely(NFS_I(dir)->cache_change_attribute == NFS_DELEGATION_VERF - 1))
		NFS_I(dir)->cache_change_attribute = NFS_DELEGATION_VERF + 1;
 	else
		NFS_I(dir)->cache_change_attribute++;

then that should ensure those readers of cache_change_attribute that do
not use locking will also never see the value NFS_DELEGATION_VERF
(assuming that gcc doesn't optimise the above to something weird).

>  }
>  EXPORT_SYMBOL_GPL(nfs_force_lookup_revalidate);
>  
> @@ -1133,6 +1136,7 @@ nfs_lookup_revalidate_dentry(struct inode *dir,
> struct dentry *dentry,
>  	struct nfs_fh *fhandle;
>  	struct nfs_fattr *fattr;
>  	struct nfs4_label *label;
> +	unsigned long verifier;
>  	int ret;
>  
>  	ret = -ENOMEM;
> @@ -1154,6 +1158,11 @@ nfs_lookup_revalidate_dentry(struct inode
> *dir, struct dentry *dentry,
>  	if (nfs_refresh_inode(inode, fattr) < 0)
>  		goto out;
>  
> +	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> +		verifier = NFS_DELEGATION_VERF;
> +	else
> +		verifier = nfs_save_change_attribute(dir);
> +
>  	nfs_setsecurity(inode, fattr, label);
>  	nfs_set_verifier(dentry, nfs_save_change_attribute(dir));
>  
> @@ -1167,6 +1176,15 @@ nfs_lookup_revalidate_dentry(struct inode
> *dir, struct dentry *dentry,
>  	return nfs_lookup_revalidate_done(dir, dentry, inode, ret);
>  }
>  
> +static int nfs_delegation_matches_dentry(struct inode *dir,
> +			struct dentry *dentry, struct inode *inode)
> +{
> +	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ) &&
> +		dentry->d_time == NFS_DELEGATION_VERF)
> +		return 1;
> +	return 0;
> +}
> +
>  /*
>   * This is called every time the dcache has a lookup hit,
>   * and we should check whether we can really trust that
> @@ -1197,7 +1215,7 @@ nfs_do_lookup_revalidate(struct inode *dir,
> struct dentry *dentry,
>  		goto out_bad;
>  	}
>  
> -	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> +	if (nfs_delegation_matches_dentry(dir, dentry, inode))
>  		return nfs_lookup_revalidate_delegated(dir, dentry,
> inode);
>  
>  	/* Force a full look up iff the parent directory has changed */
> @@ -1635,7 +1653,7 @@ nfs4_do_lookup_revalidate(struct inode *dir,
> struct dentry *dentry,
>  	if (inode == NULL)
>  		goto full_reval;
>  
> -	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> +	if (nfs_delegation_matches_dentry(dir, dentry, inode))
>  		return nfs_lookup_revalidate_delegated(dir, dentry,
> inode);
>  
>  	/* NFS only supports OPEN on regular files */
-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx






[Index of Archives]     [Linux Filesystem Development]     [Linux USB Development]     [Linux Media Development]     [Video for Linux]     [Linux NILFS]     [Linux Audio Users]     [Yosemite Info]     [Linux SCSI]

  Powered by Linux