[PATCH v1] NFS: Zero-stateid SETATTR should first return delegation

[PATCH v1] NFS: Zero-stateid SETATTR should first return delegation

[Chuck Lever]

If a write delegation isn't available, the Linux NFS client uses
a zero-stateid when performing a SETATTR.

If that client happens to hold a read delegation, the server will
recall it immediately, resulting in a short delay while the
CB_RECALL operation is done. Optimize out this delay by having the
client return any delegation it may hold on a file before issuing a
SETATTR(zero-stateid) on that file.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index dbd01548335b..53a56250cf4b 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -3314,6 +3314,7 @@ static int _nfs4_do_setattr(struct inode *inode,
 			goto zero_stateid;
 	} else {
 zero_stateid:
+		nfs4_inode_return_delegation(inode);
 		nfs4_stateid_copy(&arg->stateid, &zero_stateid);
 	}
 	if (delegation_cred)

Re: [PATCH v1] NFS: Zero-stateid SETATTR should first return delegation

[Trond Myklebust]

On Fri, 2020-08-28 at 15:58 -0400, Chuck Lever wrote:
> If a write delegation isn't available, the Linux NFS client uses
> a zero-stateid when performing a SETATTR.
> 
> If that client happens to hold a read delegation, the server will
> recall it immediately, resulting in a short delay while the
> CB_RECALL operation is done. Optimize out this delay by having the
> client return any delegation it may hold on a file before issuing a
> SETATTR(zero-stateid) on that file.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfs/nfs4proc.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index dbd01548335b..53a56250cf4b 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -3314,6 +3314,7 @@ static int _nfs4_do_setattr(struct inode
> *inode,
>  			goto zero_stateid;
>  	} else {
>  zero_stateid:
> +		nfs4_inode_return_delegation(inode);
>  		nfs4_stateid_copy(&arg->stateid, &zero_stateid);
>  	}
>  	if (delegation_cred)
> 

This should not be needed for NFSv4.1 or greater. Only NFSv4.0 is
incapable of identifying the caller and recognising that it is the
holder of the delegation.

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

Re: [PATCH v1] NFS: Zero-stateid SETATTR should first return delegation

[J. Bruce Fields]

On Fri, Aug 28, 2020 at 09:13:07PM +0000, Trond Myklebust wrote:
> On Fri, 2020-08-28 at 15:58 -0400, Chuck Lever wrote:
> > If a write delegation isn't available, the Linux NFS client uses
> > a zero-stateid when performing a SETATTR.
> > 
> > If that client happens to hold a read delegation, the server will
> > recall it immediately, resulting in a short delay while the
> > CB_RECALL operation is done. Optimize out this delay by having the
> > client return any delegation it may hold on a file before issuing a
> > SETATTR(zero-stateid) on that file.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfs/nfs4proc.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index dbd01548335b..53a56250cf4b 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -3314,6 +3314,7 @@ static int _nfs4_do_setattr(struct inode
> > *inode,
> >  			goto zero_stateid;
> >  	} else {
> >  zero_stateid:
> > +		nfs4_inode_return_delegation(inode);
> >  		nfs4_stateid_copy(&arg->stateid, &zero_stateid);
> >  	}
> >  	if (delegation_cred)
> > 
> 
> This should not be needed for NFSv4.1 or greater. Only NFSv4.0 is
> incapable of identifying the caller and recognising that it is the
> holder of the delegation.

And the server should be getting this right now in the >=4.1 case, so
let me know if you see otherwise.

--b.

Re: [PATCH v1] NFS: Zero-stateid SETATTR should first return delegation

[Chuck Lever]

> On Aug 29, 2020, at 12:17 PM, bfields@fieldses.org wrote:
> 
> On Fri, Aug 28, 2020 at 09:13:07PM +0000, Trond Myklebust wrote:
>> On Fri, 2020-08-28 at 15:58 -0400, Chuck Lever wrote:
>>> If a write delegation isn't available, the Linux NFS client uses
>>> a zero-stateid when performing a SETATTR.
>>> 
>>> If that client happens to hold a read delegation, the server will
>>> recall it immediately, resulting in a short delay while the
>>> CB_RECALL operation is done. Optimize out this delay by having the
>>> client return any delegation it may hold on a file before issuing a
>>> SETATTR(zero-stateid) on that file.
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> fs/nfs/nfs4proc.c |    1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>>> index dbd01548335b..53a56250cf4b 100644
>>> --- a/fs/nfs/nfs4proc.c
>>> +++ b/fs/nfs/nfs4proc.c
>>> @@ -3314,6 +3314,7 @@ static int _nfs4_do_setattr(struct inode
>>> *inode,
>>> 			goto zero_stateid;
>>> 	} else {
>>> zero_stateid:
>>> +		nfs4_inode_return_delegation(inode);
>>> 		nfs4_stateid_copy(&arg->stateid, &zero_stateid);
>>> 	}
>>> 	if (delegation_cred)
>>> 
>> 
>> This should not be needed for NFSv4.1 or greater. Only NFSv4.0 is
>> incapable of identifying the caller and recognising that it is the
>> holder of the delegation.
> 
> And the server should be getting this right now in the >=4.1 case, so
> let me know if you see otherwise.

The v5.9-rc1 server appears to be getting this right for v4.1.

Your earlier patch to avoid offering a delegation on OPEN(CREATE) helps
reduce the extra CB_RECALLs for v4.0, but the above patch is needed to
address the non-open-create cases.


--
Chuck Lever