Re: [PATCH RFC 12/24] nfsd: encoders and decoders for GET_DIR_DELEGATION

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <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.

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 <jlayton@xxxxxxxxxx>

-- 
Chuck Lever




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux