On Sun, 2024-03-17 at 11:34 -0400, Chuck Lever wrote: > On Fri, Mar 15, 2024 at 12:53:03PM -0400, Jeff Layton wrote: > > This adds basic infrastructure for handing GET_DIR_DELEGATION calls from > > clients, including the decoders and encoders. For now, the server side > > always just returns that the delegation is GDDR_UNAVAIL (and that we > > won't call back). > > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx> > > --- > > fs/nfsd/nfs4proc.c | 30 ++++++++++++++++++++++ > > fs/nfsd/nfs4xdr.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++-- > > fs/nfsd/xdr4.h | 8 ++++++ > > include/linux/nfs4.h | 5 ++++ > > 4 files changed, 113 insertions(+), 2 deletions(-) > > Just a handful of style preferences below. > Those comments all make sense. I'll respin along those lines. Also, I may go ahead and send this patch separately from the rest of the series. I think it would be best to have trivial support for GET_DIR_DELEGATION in the kernel server as soon as possible. Eventually, clients may start sending these calls, and it's better if we can just return a non-fatal error instead of sending back NFSERR_NOTSUPP when they do. > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c > > index 2927b1263f08..7973fe17bf3c 100644 > > --- a/fs/nfsd/nfs4proc.c > > +++ b/fs/nfsd/nfs4proc.c > > @@ -2173,6 +2173,18 @@ nfsd4_layout_verify(struct svc_export *exp, unsigned int layout_type) > > return nfsd4_layout_ops[layout_type]; > > } > > > > +static __be32 > > +nfsd4_get_dir_delegation(struct svc_rqst *rqstp, > > + struct nfsd4_compound_state *cstate, > > + union nfsd4_op_u *u) > > +{ > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + > > + /* FIXME: actually return a delegation */ > > + gdd->nf_status = GDD4_UNAVAIL; > > + return nfs_ok; > > +} > > + > > static __be32 > > nfsd4_getdeviceinfo(struct svc_rqst *rqstp, > > struct nfsd4_compound_state *cstate, union nfsd4_op_u *u) > > @@ -3082,6 +3094,18 @@ static u32 nfsd4_copy_notify_rsize(const struct svc_rqst *rqstp, > > * sizeof(__be32); > > } > > > > +static u32 nfsd4_get_dir_delegation_rsize(const struct svc_rqst *rqstp, > > + const struct nfsd4_op *op) > > +{ > > + return (op_encode_hdr_size + > > + 1 /* gddr_status */ + > > + op_encode_verifier_maxsz + > > + op_encode_stateid_maxsz + > > + 2 /* gddr_notification */ + > > + 2 /* gddr_child_attributes */ + > > + 2 /* gddr_dir_attributes */); > > +} > > + > > #ifdef CONFIG_NFSD_PNFS > > static u32 nfsd4_getdeviceinfo_rsize(const struct svc_rqst *rqstp, > > const struct nfsd4_op *op) > > @@ -3470,6 +3494,12 @@ static const struct nfsd4_operation nfsd4_ops[] = { > > .op_get_currentstateid = nfsd4_get_freestateid, > > .op_rsize_bop = nfsd4_only_status_rsize, > > }, > > + [OP_GET_DIR_DELEGATION] = { > > + .op_func = nfsd4_get_dir_delegation, > > + .op_flags = OP_MODIFIES_SOMETHING, > > + .op_name = "OP_GET_DIR_DELEGATION", > > + .op_rsize_bop = nfsd4_get_dir_delegation_rsize, > > + }, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = { > > .op_func = nfsd4_getdeviceinfo, > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c > > index fac938f563ad..3718bef74e9f 100644 > > --- a/fs/nfsd/nfs4xdr.c > > +++ b/fs/nfsd/nfs4xdr.c > > @@ -1732,6 +1732,40 @@ nfsd4_decode_free_stateid(struct nfsd4_compoundargs *argp, > > return nfsd4_decode_stateid4(argp, &free_stateid->fr_stateid); > > } > > > > +static __be32 > > +nfsd4_decode_get_dir_delegation(struct nfsd4_compoundargs *argp, > > + union nfsd4_op_u *u) > > +{ > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + struct timespec64 ts; > > + u32 signal_deleg_avail; > > + u32 attrs[1]; > > I know this isn't how we've done XDR in the past, but I'd rather > see these dummy args as fields in struct nfsd4_get_dir_delegation, > and also move the comments about whether each argument is supported > to the putative nfsd4_proc_get_dir_delegation(). > > The actual implementation of GET_DIR_DELEGATION is in nfs4proc.c, > after all, not here. This is simply a translation function. > > > > + __be32 status; > > + > > + memset(gdd, 0, sizeof(*gdd)); > > + > > + /* No signal_avail support for now (and maybe never) */ > > + if (xdr_stream_decode_bool(argp->xdr, &signal_deleg_avail) < 0) > > + return nfserr_bad_xdr; > > + status = nfsd4_decode_bitmap4(argp, gdd->notification_types, > > + ARRAY_SIZE(gdd->notification_types)); > > + if (status) > > + return status; > > + > > + /* For now, we don't support child or dir attr change notification */ > > + status = nfsd4_decode_nfstime4(argp, &ts); > > + if (status) > > + return status; > > + /* No dir attr notification support yet either */ > > + status = nfsd4_decode_nfstime4(argp, &ts); > > + if (status) > > + return status; > > + status = nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > + if (status) > > + return status; > > + return nfsd4_decode_bitmap4(argp, attrs, ARRAY_SIZE(attrs)); > > +} > > + > > #ifdef CONFIG_NFSD_PNFS > > static __be32 > > nfsd4_decode_getdeviceinfo(struct nfsd4_compoundargs *argp, > > @@ -2370,7 +2404,7 @@ static const nfsd4_dec nfsd4_dec_ops[] = { > > [OP_CREATE_SESSION] = nfsd4_decode_create_session, > > [OP_DESTROY_SESSION] = nfsd4_decode_destroy_session, > > [OP_FREE_STATEID] = nfsd4_decode_free_stateid, > > - [OP_GET_DIR_DELEGATION] = nfsd4_decode_notsupp, > > + [OP_GET_DIR_DELEGATION] = nfsd4_decode_get_dir_delegation, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = nfsd4_decode_getdeviceinfo, > > [OP_GETDEVICELIST] = nfsd4_decode_notsupp, > > @@ -5002,6 +5036,40 @@ nfsd4_encode_device_addr4(struct xdr_stream *xdr, > > return nfserr_toosmall; > > } > > > > +static __be32 > > +nfsd4_encode_get_dir_delegation(struct nfsd4_compoundres *resp, __be32 nfserr, > > + union nfsd4_op_u *u) > > +{ > > + struct xdr_stream *xdr = resp->xdr; > > + struct nfsd4_get_dir_delegation *gdd = &u->get_dir_delegation; > > + > > + /* Encode the GDDR_* status code */ > > In other encoders, I've used simply the name of the field as it is > in the RFC as a documenting comment. That's more clear, and is > easily grep-able. So: > > /* gddrnf_status */ > > > > + if (xdr_stream_encode_u32(xdr, gdd->nf_status) != XDR_UNIT) > > + return nfserr_resource; > > + > > + /* if it's GDD4_UNAVAIL then we're (almost) done */ > > + if (gdd->nf_status == GDD4_UNAVAIL) { > > I prefer using a switch for XDR unions. That makes our > implementation look more like the XDR definition; easier for humans > to audit and modify. > > > > + /* We never call back */ > > + return nfsd4_encode_bool(xdr, false); > > Again, let's move this boolean to struct nfsd4_get_dir_delegation to > enable nfsd4_proc_get_dir_delegation to decide in the future. > > > > + } > > + > > + /* GDD4_OK case */ > > If a switch is used, then this comment becomes a real piece of > self-verifying code: > > case GDD4_OK: > > > > + nfserr = nfsd4_encode_verifier4(xdr, &gdd->cookieverf); > > + if (nfserr) > > + return nfserr; > > + nfserr = nfsd4_encode_stateid4(xdr, &gdd->stateid); > > + if (nfserr) > > + return nfserr; > > + /* No notifications (yet) */ > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > + if (nfserr) > > + return nfserr; > > + nfserr = nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > + if (nfserr) > > + return nfserr; > > + return nfsd4_encode_bitmap4(xdr, 0, 0, 0); > > All these as well can go in struct nfsd4_get_dir_delegation. > > > > +} > > + > > static __be32 > > nfsd4_encode_getdeviceinfo(struct nfsd4_compoundres *resp, __be32 nfserr, > > union nfsd4_op_u *u) > > @@ -5580,7 +5648,7 @@ static const nfsd4_enc nfsd4_enc_ops[] = { > > [OP_CREATE_SESSION] = nfsd4_encode_create_session, > > [OP_DESTROY_SESSION] = nfsd4_encode_noop, > > [OP_FREE_STATEID] = nfsd4_encode_noop, > > - [OP_GET_DIR_DELEGATION] = nfsd4_encode_noop, > > + [OP_GET_DIR_DELEGATION] = nfsd4_encode_get_dir_delegation, > > #ifdef CONFIG_NFSD_PNFS > > [OP_GETDEVICEINFO] = nfsd4_encode_getdeviceinfo, > > [OP_GETDEVICELIST] = nfsd4_encode_noop, > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > > index 415516c1b27e..27de75f32dea 100644 > > --- a/fs/nfsd/xdr4.h > > +++ b/fs/nfsd/xdr4.h > > @@ -518,6 +518,13 @@ struct nfsd4_free_stateid { > > stateid_t fr_stateid; /* request */ > > }; > > > > +struct nfsd4_get_dir_delegation { > > + u32 notification_types[1]; /* request */ > > + u32 nf_status; /* response */ > > + nfs4_verifier cookieverf; /* response */ > > + stateid_t stateid; /* response */ > > +}; > > + > > /* also used for NVERIFY */ > > struct nfsd4_verify { > > u32 ve_bmval[3]; /* request */ > > @@ -797,6 +804,7 @@ struct nfsd4_op { > > struct nfsd4_reclaim_complete reclaim_complete; > > struct nfsd4_test_stateid test_stateid; > > struct nfsd4_free_stateid free_stateid; > > + struct nfsd4_get_dir_delegation get_dir_delegation; > > struct nfsd4_getdeviceinfo getdeviceinfo; > > struct nfsd4_layoutget layoutget; > > struct nfsd4_layoutcommit layoutcommit; > > diff --git a/include/linux/nfs4.h b/include/linux/nfs4.h > > index ef8d2d618d5b..11ad088b411d 100644 > > --- a/include/linux/nfs4.h > > +++ b/include/linux/nfs4.h > > @@ -701,6 +701,11 @@ enum state_protect_how4 { > > SP4_SSV = 2 > > }; > > > > +enum get_dir_delegation_non_fatal_res { > > + GDD4_OK = 0, > > + GDD4_UNAVAIL = 1 > > +}; > > + > > enum pnfs_layouttype { > > LAYOUT_NFSV4_1_FILES = 1, > > LAYOUT_OSD2_OBJECTS = 2, > > > > -- > > 2.44.0 > > > -- Jeff Layton <jlayton@xxxxxxxxxx>