mbox series

[v3,0/7] Wait for DELEGRETURN before returning NFS4ERR_DELAY

Message ID 165996657035.2637.4745479232455341597.stgit@manet.1015granger.net (mailing list archive)
Headers show
Series Wait for DELEGRETURN before returning NFS4ERR_DELAY | expand

Message

Chuck Lever Aug. 8, 2022, 1:52 p.m. UTC
This RFC series adds logic to the Linux NFS server to make it wait a
few moments for clients to return a delegation before replying with
NFS4ERR_DELAY. There are two main benefits:

- This improves responsiveness when a delegated file is accessed
 from another client
- This removes an unnecessary latency if a client has neglected to
 return a delegation before attempting a RENAME or SETATTR

This series is incomplete:
- There are likely other operations that can benefit (eg. OPEN)

This series applies against v5.19.

Changes since v2:
- Wake immediately after client sends DELEGRETURN
- More tracepoint improvements

Changes since RFC:
- Eliminate spurious console messages on the server
- Wait for DELEGRETURN for RENAME operations
- Add CB done tracepoints
- Rework the retry loop

---

Chuck Lever (7):
      NFSD: Instrument fh_verify()
      NFSD: Replace dprintk() call site in fh_verify()
      NFSD: Trace NFSv4 COMPOUND tags
      NFSD: Add tracepoints to report NFSv4 callback completions
      NFSD: Add a mechanism to wait for a DELEGRETURN
      NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
      NFSD: Make nfsd4_rename() wait before returning NFS4ERR_DELAY


 fs/nfsd/nfs4layouts.c |   2 +-
 fs/nfsd/nfs4proc.c    |  56 +++++++++++---
 fs/nfsd/nfs4state.c   |  35 +++++++++
 fs/nfsd/nfsd.h        |   1 +
 fs/nfsd/nfsfh.c       |  13 +---
 fs/nfsd/trace.h       | 171 ++++++++++++++++++++++++++++++++++++++++--
 fs/nfsd/xdr4.h        |   2 +
 7 files changed, 251 insertions(+), 29 deletions(-)

--
Chuck Lever

Comments

Jeff Layton Aug. 8, 2022, 6:48 p.m. UTC | #1
On Mon, 2022-08-08 at 09:52 -0400, Chuck Lever wrote:
> This RFC series adds logic to the Linux NFS server to make it wait a
> few moments for clients to return a delegation before replying with
> NFS4ERR_DELAY. There are two main benefits:
> 
> - This improves responsiveness when a delegated file is accessed
>  from another client
> - This removes an unnecessary latency if a client has neglected to
>  return a delegation before attempting a RENAME or SETATTR
> 
> This series is incomplete:
> - There are likely other operations that can benefit (eg. OPEN)
> 
> This series applies against v5.19.
> 
> Changes since v2:
> - Wake immediately after client sends DELEGRETURN
> - More tracepoint improvements
> 
> Changes since RFC:
> - Eliminate spurious console messages on the server
> - Wait for DELEGRETURN for RENAME operations
    ^^^^
Does REMOVE need similar treatment? Does the Apple client return a
delegation before attempting to unlink?

> - Add CB done tracepoints
> - Rework the retry loop
> 
> ---
> 
> Chuck Lever (7):
>       NFSD: Instrument fh_verify()
>       NFSD: Replace dprintk() call site in fh_verify()
>       NFSD: Trace NFSv4 COMPOUND tags
>       NFSD: Add tracepoints to report NFSv4 callback completions
>       NFSD: Add a mechanism to wait for a DELEGRETURN
>       NFSD: Make nfsd4_setattr() wait before returning NFS4ERR_DELAY
>       NFSD: Make nfsd4_rename() wait before returning NFS4ERR_DELAY
> 
> 
>  fs/nfsd/nfs4layouts.c |   2 +-
>  fs/nfsd/nfs4proc.c    |  56 +++++++++++---
>  fs/nfsd/nfs4state.c   |  35 +++++++++
>  fs/nfsd/nfsd.h        |   1 +
>  fs/nfsd/nfsfh.c       |  13 +---
>  fs/nfsd/trace.h       | 171 ++++++++++++++++++++++++++++++++++++++++--
>  fs/nfsd/xdr4.h        |   2 +
>  7 files changed, 251 insertions(+), 29 deletions(-)
> 
> --
> Chuck Lever
> 

The new tracepoints are nice and the patchset makes sense. You can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Chuck Lever Aug. 8, 2022, 8:06 p.m. UTC | #2
> On Aug 8, 2022, at 2:48 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Mon, 2022-08-08 at 09:52 -0400, Chuck Lever wrote:
>> This RFC series adds logic to the Linux NFS server to make it wait a
>> few moments for clients to return a delegation before replying with
>> NFS4ERR_DELAY. There are two main benefits:
>> 
>> - This improves responsiveness when a delegated file is accessed
>> from another client
>> - This removes an unnecessary latency if a client has neglected to
>> return a delegation before attempting a RENAME or SETATTR
>> 
>> This series is incomplete:
>> - There are likely other operations that can benefit (eg. OPEN)

> Does REMOVE need similar treatment? Does the Apple client return a
> delegation before attempting to unlink?

It's certainly plausible that REMOVE might trigger a delegation recall,
but I haven't yet seen this happen on the wire.


--
Chuck Lever
Chuck Lever Aug. 8, 2022, 9:47 p.m. UTC | #3
> On Aug 8, 2022, at 4:06 PM, Chuck Lever III <chuck.lever@oracle.com> wrote:
> 
>> On Aug 8, 2022, at 2:48 PM, Jeff Layton <jlayton@kernel.org> wrote:
>> 
>> On Mon, 2022-08-08 at 09:52 -0400, Chuck Lever wrote:
>>> This RFC series adds logic to the Linux NFS server to make it wait a
>>> few moments for clients to return a delegation before replying with
>>> NFS4ERR_DELAY. There are two main benefits:
>>> 
>>> - This improves responsiveness when a delegated file is accessed
>>> from another client
>>> - This removes an unnecessary latency if a client has neglected to
>>> return a delegation before attempting a RENAME or SETATTR
>>> 
>>> This series is incomplete:
>>> - There are likely other operations that can benefit (eg. OPEN)
> 
>> Does REMOVE need similar treatment? Does the Apple client return a
>> delegation before attempting to unlink?
> 
> It's certainly plausible that REMOVE might trigger a delegation recall,
> but I haven't yet seen this happen on the wire.

I started playing with REMOVE and DELEG15a, and ran into an
interesting problem.

REMOVE is passed the filehandle of the parent and the name of
the entry to be removed. nfsd4_remove() doesn't have an inode
or filehandle of the object to be removed. nfsd_unlink() does
acquire the inode of that object, though.

And, I guess if we want to avoid NFSv3 operations returning
JUKEBOX on delegated files too, the "wait for DELEGRETURN"
really ought to be called from the fs/nfsd/vfs.c functions,
not from the NFSv4 specific functions in fs/nfsd/nfs4proc.c.


--
Chuck Lever