On Fri, 2020-01-24 at 09:09 -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 | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index e180033e35cf..81a5a28d0015 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1073,6 +1073,7 @@ int nfs_neg_need_reval(struct inode *dir,
> struct dentry *dentry,
> return !nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU);
> }
>
> +#define NFS_DELEGATION_VERF 0xfeeddeaf
> static int
> nfs_lookup_revalidate_done(struct inode *dir, struct dentry *dentry,
> struct inode *inode, int error)
> @@ -1133,6 +1134,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 +1156,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 +1174,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 +1213,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 +1651,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 */
This patch needs to fix nfs_force_lookup_revalidate() to avoid the
value NFS_DELEGATION_VERF.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
