[PATCH] NFS: Revalidate once when holding a delegation

[PATCH] NFS: Revalidate once when holding a delegation

[Benjamin Coddington]

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@hammerspace.com>
Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 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 */
-- 
2.20.1

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

[Trond Myklebust]

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@hammerspace.com>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  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@hammerspace.com

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

[Benjamin Coddington]

On 24 Jan 2020, at 14:24, Trond Myklebust wrote:

> On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote:
>
> This patch needs to fix nfs_force_lookup_revalidate() to avoid the
> value NFS_DELEGATION_VERF.

I don't understand why.  Doesn't nfs_force_lookup_revalidate() just bump the
directory's cache_change_attribute, which is value we don't care about at
all here.  This patch just sets a magic value on the dentry's d_time after a
revalidation occurs for that dentry while holding a delegation.  It doesn't
care at all about the directory's change_attr.

What am I missing?

Ben

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

[Trond Myklebust]

On Fri, 2020-01-24 at 20:33 -0500, Benjamin Coddington wrote:
> On 24 Jan 2020, at 14:24, Trond Myklebust wrote:
> 
> > On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote:
> > 
> > This patch needs to fix nfs_force_lookup_revalidate() to avoid the
> > value NFS_DELEGATION_VERF.
> 
> I don't understand why.  Doesn't nfs_force_lookup_revalidate() just
> bump the
> directory's cache_change_attribute, which is value we don't care
> about at
> all here.  This patch just sets a magic value on the dentry's d_time
> after a
> revalidation occurs for that dentry while holding a delegation.  It
> doesn't
> care at all about the directory's change_attr.
> 
> What am I missing?

Those calls to nfs_set_verifier(dentry, nfs_save_change_attribute(dir))
are storing the parent directory cache_change_attribute() in the d_time
field of the child dentry. That's the normal value for the child dentry
verifiers of that directory when there is no delegation.

So to avoid collisions, you need to ensure that dir-
>cache_change_attribute never takes the value NFS_DELEGATION_VERF.


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

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

[Benjamin Coddington]

On 25 Jan 2020, at 14:36, Trond Myklebust wrote:

> On Fri, 2020-01-24 at 20:33 -0500, Benjamin Coddington wrote:
>> On 24 Jan 2020, at 14:24, Trond Myklebust wrote:
>>
>>> On Fri, 2020-01-24 at 09:09 -0500, Benjamin Coddington wrote:
>>>
>>> This patch needs to fix nfs_force_lookup_revalidate() to avoid the
>>> value NFS_DELEGATION_VERF.
>>
>> I don't understand why.  Doesn't nfs_force_lookup_revalidate() just
>> bump the
>> directory's cache_change_attribute, which is value we don't care
>> about at
>> all here.  This patch just sets a magic value on the dentry's d_time
>> after a
>> revalidation occurs for that dentry while holding a delegation.  It
>> doesn't
>> care at all about the directory's change_attr.
>>
>> What am I missing?
>
> Those calls to nfs_set_verifier(dentry, nfs_save_change_attribute(dir))
> are storing the parent directory cache_change_attribute() in the d_time
> field of the child dentry. That's the normal value for the child dentry
> verifiers of that directory when there is no delegation.
>
> So to avoid collisions, you need to ensure that dir-
> cache_change_attribute never takes the value NFS_DELEGATION_VERF.

Ah - of course.  A _complete_ fix.  Thanks for helping me with the obvious.

Ok, after not feeling good about adding a comparison and jump into that path
- maybe its not as big a deal as I'm thinking, but ugh it seems gross for
  just this single case - I wondered about doing this other ways.

As alternatives, I tried to get d_fsdata freed up for this case (there's
already two users of that, it was ugly), and then playing with only some of
the bits in d_time for flags (every approach I tried is slower than just
checking and skipping NFS_DELEGATION_VERF).  I thought maybe we could grow
another d_flag for this sort of thing, but I doubt that will fly.

Oh well, I'll just send a V2 that skips the magic value.

Thanks again,
Ben