| Message ID | 20240613041136.506908-1-trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | OPEN optimisations and Attribute delegations | expand |
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>
Btw, your patch mail threading is quite broken, it replies to the previous patch instead of the cover letter, leading to really long fake reply chains..
On Fri, Jun 14, 2024 at 08:34:32AM -0400, Jeff Layton wrote: > 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. I don't think there ever was a formal rule, but having a feature like this that affects all of the core nfs code without beeing able to test it against knfsd seems a bit dangerous indeed.
On Fri, 2024-06-14 at 23:25 -0700, Christoph Hellwig wrote: > Btw, your patch mail threading is quite broken, it replies to the > previous patch instead of the cover letter, leading to really long > fake reply chains.. > I'm just using the [sendemail] thread = true git config option. Is that no longer recommended?
On Fri, 2024-06-14 at 23:27 -0700, Christoph Hellwig wrote: > On Fri, Jun 14, 2024 at 08:34:32AM -0400, Jeff Layton wrote: > > 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. > > I don't think there ever was a formal rule, but having a feature like > this that affects all of the core nfs code without beeing able to > test > it against knfsd seems a bit dangerous indeed. > We have been going through the IETF process to ensure there is an agreed upon protocol for this that can be used to test the client code and validate its correctness. That spec can also be used by others to contribute the missing knfsd code. At some point soon, I do hope that Hammerspace will be in the position where we're able to contribute both client and server code for these features; for now, we are unfortunately still resource constrained.
On Mon, Jun 17, 2024 at 01:39:06AM +0000, Trond Myklebust wrote: > We have been going through the IETF process to ensure there is an > agreed upon protocol for this that can be used to test the client code > and validate its correctness. That spec can also be used by others to > contribute the missing knfsd code. I still think it is a bad idea to merge Linux code we can't easily test on pure Linux setups. (saying that as someone touching code only exercised by the flexfiles layout right now, which is a rather annoying)
On Mon, Jun 17, 2024 at 01:28:26AM +0000, Trond Myklebust wrote: > On Fri, 2024-06-14 at 23:25 -0700, Christoph Hellwig wrote: > > Btw, your patch mail threading is quite broken, it replies to the > > previous patch instead of the cover letter, leading to really long > > fake reply chains.. > > > > I'm just using the > > [sendemail] > thread = true > > git config option. Is that no longer recommended? According to the man page thread = true is the same as thread = shallow, which make git always reply to the cover letter, which is the recommended form. Try removing it or replace it with 'thread = shallow'
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(-)