Re: [PATCH 00/19] OPEN optimisations and Attribute delegations

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2024-06-13 at 00:11 -0400, trondmy@gmail.com wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> Now that https://datatracker.ietf.org/doc/draft-ietf-nfsv4-delstid/ is
> mostly done with the review process, it is time to look at pushing the
> client implementation that we've been working on upstream.
> 
> The following patch series therefore adds support for the NFSv4.2
> extension to OP_OPEN to allow the client to request that the server
> return either an open stateid or a delegation instead of always sending
> the open stateid whether or not a delegation is returned.
> This allows us to optimise away CLOSE, and hence makes small or cached
> file access significantly more efficient.
> 
> It also adds support for attribute delegations, which allow the client
> to manage the atime and mtime, and simply inform the server at file
> close time what the values should be. This means that most GETATTR
> operations to retrieve the atime/mtime values while the file is under
> I/O can be optimised away.
> 
> Finally, we also add support for the detection mechanism that allows the
> client to determine whether or not the server supports the above
> functionality.
> 
> Lance Shelton (1):
>   NFS: Add a generic callback to return the delegation
> 
> Trond Myklebust (18):
>   NFSv4: Clean up open delegation return structure
>   NFSv4: Refactor nfs4_opendata_check_deleg()
>   NFSv4: Add new attribute delegation definitions
>   NFSv4: Plumb in XDR support for the new delegation-only setattr op
>   NFSv4: Add CB_GETATTR support for delegated attributes
>   NFSv4: Add a flags argument to the 'have_delegation' callback
>   NFSv4: Add support for delegated atime and mtime attributes
>   NFSv4: Add recovery of attribute delegations
>   NFSv4: Add a capability for delegated attributes
>   NFSv4: Enable attribute delegations
>   NFSv4: Delegreturn must set m/atime when they are delegated
>   NFSv4: Fix up delegated attributes in nfs_setattr
>   NFSv4: Don't request atime/mtime/size if they are delegated to us
>   NFSv4: Add support for the FATTR4_OPEN_ARGUMENTS attribute
>   NFSv4: Detect support for OPEN4_SHARE_ACCESS_WANT_OPEN_XOR_DELEGATION
>   NFSv4: Add support for OPEN4_RESULT_NO_OPEN_STATEID
>   NFSv4: Ask for a delegation or an open stateid in OPEN
>   Return the delegation when deleting the sillyrenamed file
> 
>  fs/nfs/callback.h         |   5 +-
>  fs/nfs/callback_proc.c    |  14 ++-
>  fs/nfs/callback_xdr.c     |  39 ++++++-
>  fs/nfs/delegation.c       |  59 ++++++----
>  fs/nfs/delegation.h       |  45 +++++++-
>  fs/nfs/dir.c              |   2 +-
>  fs/nfs/file.c             |   4 +-
>  fs/nfs/inode.c            | 104 +++++++++++++++--
>  fs/nfs/nfs3proc.c         |  10 +-
>  fs/nfs/nfs4proc.c         | 230 ++++++++++++++++++++++++++++----------
>  fs/nfs/nfs4xdr.c          | 131 +++++++++++++++++-----
>  fs/nfs/proc.c             |  10 +-
>  fs/nfs/read.c             |   3 +
>  fs/nfs/unlink.c           |   2 +
>  fs/nfs/write.c            |  11 +-
>  include/linux/nfs4.h      |  11 ++
>  include/linux/nfs_fs_sb.h |   2 +
>  include/linux/nfs_xdr.h   |  45 +++++++-
>  include/uapi/linux/nfs4.h |   4 +
>  19 files changed, 586 insertions(+), 145 deletions(-)
> 

This all looks pretty reasonable except for the last two patches.
Probably, they should be squashed together since there is no caller of
->return_delegation until the last one. There is also nothing
describing the changes there, and I think it could use some explanation
(though I think I get what you're doing).

Finally, I suppose we need to look at implementing support delstid in
knfsd as well. I'll open a new feature request for that the linux-nfs
project on github.

In any case, you can add:

Reviewed-by: Jeff Layton <jlayton@kernel.org>