diff mbox series

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

Message ID 159864470513.1031951.14868951913532221090.stgit@manet.1015granger.net (mailing list archive)
State New
Headers show
Series [v1] NFS: Zero-stateid SETATTR should first return delegation | expand

Commit Message

Chuck Lever Aug. 28, 2020, 7:58 p.m. UTC
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(+)

Comments

Trond Myklebust Aug. 28, 2020, 9:13 p.m. UTC | #1
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.
J. Bruce Fields Aug. 29, 2020, 4:17 p.m. UTC | #2
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.
Chuck Lever Aug. 29, 2020, 4:37 p.m. UTC | #3
> 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
diff mbox series

Patch

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)