| Message ID | 20241014-delstid-v1-0-7ce8a2f4dd24@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfsd: update the delstid patches for latest draft changes | expand |
From: Chuck Lever <chuck.lever@oracle.com> On Mon, 14 Oct 2024 15:26:48 -0400, Jeff Layton wrote: > This patchset is an update to the delstid patches that went into Chuck's > nfsd-next branch recently. The original versions of the spec left out > OPEN_DELEGATE_READ_ATTRS_DELEG and OPEN_DELEGATE_WRITE_ATTRS_DELEG. This > set adds proper support for them. > > My suggestion is to drop these two patches from nfsd-next: > > [...] Applied to nfsd-next for v6.13, thanks! [1/6] nfsd: drop inode parameter from nfsd4_change_attribute() commit: aacaab97e76307be16bf147b64dc45c0c9013a8a [2/6] nfsd: switch to autogenerated definitions for open_delegation_type4 commit: c1be156cbb5960cf760409416fddbf300b2500c3 [3/6] nfsd: rename NFS4_SHARE_WANT_* constants to OPEN4_SHARE_ACCESS_WANT_* commit: 466247e4e33b7e43589e5fa00bcd721a67463935 [4/6] nfsd: prepare delegation code for handing out *_ATTRS_DELEG delegations commit: 6dd23bce4068b7ecb26390c81713ab6b617990a3 [5/6] nfsd: add support for delegated timestamps commit: 3d342dc5ffe941fed1f6b32ecf59b9b5769f38be [6/6] nfsd: handle delegated timestamps in SETATTR commit: f6b1cfab609da048eba93210f47bf8ef43068119 -- Chuck Lever
Seeing strangeness in a network trace with this patch series where SETATTR is sent with time_deleg_access and server is returning with EINVAL. Test is open() with read delegation, triggering a cb_recall via a local access. I can see that the client has changed from sending just a delegreturn to sending a setattr+delegreturn. Is there no server support and this is normal to return EINVAL. On Mon, Oct 14, 2024 at 3:27 PM Jeff Layton <jlayton@kernel.org> wrote: > > This patchset is an update to the delstid patches that went into Chuck's > nfsd-next branch recently. The original versions of the spec left out > OPEN_DELEGATE_READ_ATTRS_DELEG and OPEN_DELEGATE_WRITE_ATTRS_DELEG. This > set adds proper support for them. > > My suggestion is to drop these two patches from nfsd-next: > > 544c67cc0f26 nfsd: handle delegated timestamps in SETATTR > eee2c04ca5c1 nfsd: add support for delegated timestamps > > ...and then apply this set on top of the remaining pile. The resulting > set is a bit larger than the original, as I took the liberty of adding > some more symbols to the autogenerated part of the spec. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > Jeff Layton (6): > nfsd: drop inode parameter from nfsd4_change_attribute() > nfsd: switch to autogenerated definitions for open_delegation_type4 > nfsd: rename NFS4_SHARE_WANT_* constants to OPEN4_SHARE_ACCESS_WANT_* > nfsd: prepare delegation code for handing out *_ATTRS_DELEG delegations > nfsd: add support for delegated timestamps > nfsd: handle delegated timestamps in SETATTR > > Documentation/sunrpc/xdr/nfs4_1.x | 22 ++++- > fs/nfsd/nfs4callback.c | 42 ++++++++- > fs/nfsd/nfs4proc.c | 26 ++++- > fs/nfsd/nfs4state.c | 178 ++++++++++++++++++++++++++--------- > fs/nfsd/nfs4xdr.c | 57 ++++++++--- > fs/nfsd/nfs4xdr_gen.c | 19 +++- > fs/nfsd/nfs4xdr_gen.h | 2 +- > fs/nfsd/nfsd.h | 2 + > fs/nfsd/nfsfh.c | 11 +-- > fs/nfsd/nfsfh.h | 3 +- > fs/nfsd/state.h | 18 ++++ > fs/nfsd/xdr4cb.h | 10 +- > include/linux/nfs4.h | 2 +- > include/linux/sunrpc/xdrgen/nfs4_1.h | 35 ++++++- > include/linux/time64.h | 5 + > 15 files changed, 348 insertions(+), 84 deletions(-) > --- > base-commit: 9f8009c5be9367d01cd1627d6a379b4c642d8a28 > change-id: 20241014-delstid-bf05220ad941 > > Best regards, > -- > Jeff Layton <jlayton@kernel.org> > >
On Thu, 2024-10-17 at 18:39 -0400, Olga Kornievskaia wrote: > Seeing strangeness in a network trace with this patch series where > SETATTR is sent with time_deleg_access and server is returning with > EINVAL. Test is open() with read delegation, triggering a cb_recall > via a local access. I can see that the client has changed from sending > just a delegreturn to sending a setattr+delegreturn. Is there no > server support and this is normal to return EINVAL. > No, that's a server bug. I think it's this in nfsd4_setattr: if (deleg_attrs || (setattr->sa_iattr.ia_valid & ATTR_SIZE)) { status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh, &setattr->sa_stateid, WR_STATE, NULL, &st); if (status) return status; } We're asking for a WR_STATE in the nfs4_preprocess_stateid_op, but there isn't one. There is only a read delegation, so we get back BAD_STATEID and that eventually runs into -EINVAL. I'll need to look over this and figure out how to fix it properly. Thanks for the bug report. > On Mon, Oct 14, 2024 at 3:27 PM Jeff Layton <jlayton@kernel.org> wrote: > > > > This patchset is an update to the delstid patches that went into Chuck's > > nfsd-next branch recently. The original versions of the spec left out > > OPEN_DELEGATE_READ_ATTRS_DELEG and OPEN_DELEGATE_WRITE_ATTRS_DELEG. This > > set adds proper support for them. > > > > My suggestion is to drop these two patches from nfsd-next: > > > > 544c67cc0f26 nfsd: handle delegated timestamps in SETATTR > > eee2c04ca5c1 nfsd: add support for delegated timestamps > > > > ...and then apply this set on top of the remaining pile. The resulting > > set is a bit larger than the original, as I took the liberty of adding > > some more symbols to the autogenerated part of the spec. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > Jeff Layton (6): > > nfsd: drop inode parameter from nfsd4_change_attribute() > > nfsd: switch to autogenerated definitions for open_delegation_type4 > > nfsd: rename NFS4_SHARE_WANT_* constants to OPEN4_SHARE_ACCESS_WANT_* > > nfsd: prepare delegation code for handing out *_ATTRS_DELEG delegations > > nfsd: add support for delegated timestamps > > nfsd: handle delegated timestamps in SETATTR > > > > Documentation/sunrpc/xdr/nfs4_1.x | 22 ++++- > > fs/nfsd/nfs4callback.c | 42 ++++++++- > > fs/nfsd/nfs4proc.c | 26 ++++- > > fs/nfsd/nfs4state.c | 178 ++++++++++++++++++++++++++--------- > > fs/nfsd/nfs4xdr.c | 57 ++++++++--- > > fs/nfsd/nfs4xdr_gen.c | 19 +++- > > fs/nfsd/nfs4xdr_gen.h | 2 +- > > fs/nfsd/nfsd.h | 2 + > > fs/nfsd/nfsfh.c | 11 +-- > > fs/nfsd/nfsfh.h | 3 +- > > fs/nfsd/state.h | 18 ++++ > > fs/nfsd/xdr4cb.h | 10 +- > > include/linux/nfs4.h | 2 +- > > include/linux/sunrpc/xdrgen/nfs4_1.h | 35 ++++++- > > include/linux/time64.h | 5 + > > 15 files changed, 348 insertions(+), 84 deletions(-) > > --- > > base-commit: 9f8009c5be9367d01cd1627d6a379b4c642d8a28 > > change-id: 20241014-delstid-bf05220ad941 > > > > Best regards, > > -- > > Jeff Layton <jlayton@kernel.org> > > > >
This patchset is an update to the delstid patches that went into Chuck's nfsd-next branch recently. The original versions of the spec left out OPEN_DELEGATE_READ_ATTRS_DELEG and OPEN_DELEGATE_WRITE_ATTRS_DELEG. This set adds proper support for them. My suggestion is to drop these two patches from nfsd-next: 544c67cc0f26 nfsd: handle delegated timestamps in SETATTR eee2c04ca5c1 nfsd: add support for delegated timestamps ...and then apply this set on top of the remaining pile. The resulting set is a bit larger than the original, as I took the liberty of adding some more symbols to the autogenerated part of the spec. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- Jeff Layton (6): nfsd: drop inode parameter from nfsd4_change_attribute() nfsd: switch to autogenerated definitions for open_delegation_type4 nfsd: rename NFS4_SHARE_WANT_* constants to OPEN4_SHARE_ACCESS_WANT_* nfsd: prepare delegation code for handing out *_ATTRS_DELEG delegations nfsd: add support for delegated timestamps nfsd: handle delegated timestamps in SETATTR Documentation/sunrpc/xdr/nfs4_1.x | 22 ++++- fs/nfsd/nfs4callback.c | 42 ++++++++- fs/nfsd/nfs4proc.c | 26 ++++- fs/nfsd/nfs4state.c | 178 ++++++++++++++++++++++++++--------- fs/nfsd/nfs4xdr.c | 57 ++++++++--- fs/nfsd/nfs4xdr_gen.c | 19 +++- fs/nfsd/nfs4xdr_gen.h | 2 +- fs/nfsd/nfsd.h | 2 + fs/nfsd/nfsfh.c | 11 +-- fs/nfsd/nfsfh.h | 3 +- fs/nfsd/state.h | 18 ++++ fs/nfsd/xdr4cb.h | 10 +- include/linux/nfs4.h | 2 +- include/linux/sunrpc/xdrgen/nfs4_1.h | 35 ++++++- include/linux/time64.h | 5 + 15 files changed, 348 insertions(+), 84 deletions(-) --- base-commit: 9f8009c5be9367d01cd1627d6a379b4c642d8a28 change-id: 20241014-delstid-bf05220ad941 Best regards,