[PATCH] NFS: Don't skip lookup when holding a delegation

[PATCH] NFS: Don't skip lookup when holding a delegation

[Benjamin Coddington]

If we skip lookup revalidation while holding a delegation, we might miss
that the file has changed directories on the server.  The directory's
change attribute should still be checked against the dentry's d_time to
perform a complete revalidation.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/dir.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index a71d0b42d160..10cc684dc082 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
 		goto out_bad;
 	}
 
-	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
-		return nfs_lookup_revalidate_delegated(dir, dentry, inode);
-
 	/* Force a full look up iff the parent directory has changed */
 	if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) &&
 	    nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) {
+
+		if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
+			return nfs_lookup_revalidate_delegated(dir, dentry, inode);
+
 		error = nfs_lookup_verify_inode(inode, flags);
 		if (error) {
 			if (error == -ESTALE)
@@ -1707,9 +1708,6 @@ 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))
-		return nfs_lookup_revalidate_delegated(dir, dentry, inode);
-
 	/* NFS only supports OPEN on regular files */
 	if (!S_ISREG(inode->i_mode))
 		goto full_reval;
-- 
2.20.1

Re: [PATCH] NFS: Don't skip lookup when holding a delegation

[J. Bruce Fields]

On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote:
> If we skip lookup revalidation while holding a delegation, we might miss
> that the file has changed directories on the server.

The delegation should prevent the file disappearing from this directory,
so if I've been following the discussion, the bug was due to overlooking
the case where the change happened before we got the delegation.  Given
that history it seems worth calling out that case specifically?

Maybe a comment along the lines of:

		/*
		 * Note that the file can't move while we hold a
		 * delegation.  But this dentry could have been cached
		 * before we got a delegation.  So it's only safe to
		 * skip revalidation when the parent directory is
		 * unchanged:
		 */

But maybe there's a pithier way to say that.

--b.

> The directory's
> change attribute should still be checked against the dentry's d_time to
> perform a complete revalidation.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/dir.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index a71d0b42d160..10cc684dc082 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
>  		goto out_bad;
>  	}
>  
> -	if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> -		return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> -
>  	/* Force a full look up iff the parent directory has changed */
>  	if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) &&
>  	    nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) {
> +
> +		if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> +			return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> +
>  		error = nfs_lookup_verify_inode(inode, flags);
>  		if (error) {
>  			if (error == -ESTALE)
> @@ -1707,9 +1708,6 @@ 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))
> -		return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> -
>  	/* NFS only supports OPEN on regular files */
>  	if (!S_ISREG(inode->i_mode))
>  		goto full_reval;
> -- 
> 2.20.1

Re: [PATCH] NFS: Don't skip lookup when holding a delegation

[Olga Kornievskaia]

On Thu, Jun 13, 2019 at 11:00 AM J. Bruce Fields <bfields@fieldses.org> wrote:
>
> On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote:
> > If we skip lookup revalidation while holding a delegation, we might miss
> > that the file has changed directories on the server.
>
> The delegation should prevent the file disappearing from this directory,
> so if I've been following the discussion, the bug was due to overlooking
> the case where the change happened before we got the delegation.  Given
> that history it seems worth calling out that case specifically?
>
> Maybe a comment along the lines of:
>
>                 /*
>                  * Note that the file can't move while we hold a
>                  * delegation.  But this dentry could have been cached
>                  * before we got a delegation.  So it's only safe to
>                  * skip revalidation when the parent directory is
>                  * unchanged:
>                  */
>
> But maybe there's a pithier way to say that.

What is preventing the file from disappearing from the directory while
holding the delegation: is it the server's responsibility to recall
the delegation when it gets a move or is it client's responsibility
not to rely on the cached attributes?

According to this patch it's client's responsibility, in the case, I
find the working " file can't move" confusing as they imply to me that
client can assume file isn't moved (ie, server will prevent it from
happening).

>
> --b.
>
> > The directory's
> > change attribute should still be checked against the dentry's d_time to
> > perform a complete revalidation.
> >
> > Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> > ---
> >  fs/nfs/dir.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > index a71d0b42d160..10cc684dc082 100644
> > --- a/fs/nfs/dir.c
> > +++ b/fs/nfs/dir.c
> > @@ -1269,12 +1269,13 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry,
> >               goto out_bad;
> >       }
> >
> > -     if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> > -             return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> > -
> >       /* Force a full look up iff the parent directory has changed */
> >       if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) &&
> >           nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) {
> > +
> > +             if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ))
> > +                     return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> > +
> >               error = nfs_lookup_verify_inode(inode, flags);
> >               if (error) {
> >                       if (error == -ESTALE)
> > @@ -1707,9 +1708,6 @@ 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))
> > -             return nfs_lookup_revalidate_delegated(dir, dentry, inode);
> > -
> >       /* NFS only supports OPEN on regular files */
> >       if (!S_ISREG(inode->i_mode))
> >               goto full_reval;
> > --
> > 2.20.1

Re: [PATCH] NFS: Don't skip lookup when holding a delegation

[Benjamin Coddington]

On 13 Jun 2019, at 12:02, Olga Kornievskaia wrote:

> On Thu, Jun 13, 2019 at 11:00 AM J. Bruce Fields 
> <bfields@fieldses.org> wrote:
>>
>> On Wed, Jun 12, 2019 at 10:45:13AM -0400, Benjamin Coddington wrote:
>>> If we skip lookup revalidation while holding a delegation, we might 
>>> miss
>>> that the file has changed directories on the server.
>>
>> The delegation should prevent the file disappearing from this 
>> directory,
>> so if I've been following the discussion, the bug was due to 
>> overlooking
>> the case where the change happened before we got the delegation.  
>> Given
>> that history it seems worth calling out that case specifically?
>>
>> Maybe a comment along the lines of:
>>
>>                 /*
>>                  * Note that the file can't move while we hold a
>>                  * delegation.  But this dentry could have been 
>> cached
>>                  * before we got a delegation.  So it's only safe to
>>                  * skip revalidation when the parent directory is
>>                  * unchanged:
>>                  */
>>
>> But maybe there's a pithier way to say that.

I wish I had pith.  I cannot improve on this comment.. I'm OK with or
without it.

> What is preventing the file from disappearing from the directory while
> holding the delegation: is it the server's responsibility to recall
> the delegation when it gets a move or is it client's responsibility
> not to rely on the cached attributes?

The server should recall the delegation if the file moves, since moving 
it
means that additional calls to OPEN that file at that location should 
fail.
The client shouldn't continue to independently handle those OPENs 
(unless by
filehandle.. but how can the server know how the client intends to 
handle
further delegated OPENs?)

> According to this patch it's client's responsibility, in the case, I
> find the working " file can't move" confusing as they imply to me that
> client can assume file isn't moved (ie, server will prevent it from
> happening).

I think that it looks like its the client's responsibility only because 
the
client lacks an easy way to determine what order delegations were 
acquired
with respect to directory modifications.  We could try to track all of 
that,
but the structures and memory used would be hideous.

Ben