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 <[email protected]>
---
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(-)
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];
+ __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 */
+ 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) {
+ /* We never call back */
+ return nfsd4_encode_bool(xdr, false);
+ }
+
+ /* GDD4_OK case */
+ 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);
+}
+
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
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 <[email protected]>
> ---
> 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.
> 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
>
--
Chuck Lever
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 <[email protected]>
> > ---
> > 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 <[email protected]>
On Mon, Mar 18, 2024 at 12:12:06PM -0400, Jeff Layton wrote:
> 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 <[email protected]>
> > > ---
> > > 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.
I have no objection to the XDR pieces going into NFSD before the
rest of the series. Thanks for this implementation!
> > > 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 <[email protected]>
--
Chuck Lever