From: Chuck Lever <chuck.lever@oracle.com>
To: Jeff Layton <jlayton@kernel.org>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>,
Christian Brauner <brauner@kernel.org>, Jan Kara <jack@suse.cz>,
Alexander Aring <alex.aring@gmail.com>,
Trond Myklebust <trond.myklebust@hammerspace.com>,
Anna Schumaker <anna@kernel.org>,
Steve French <sfrench@samba.org>,
Paulo Alcantara <pc@manguebit.com>,
Ronnie Sahlberg <ronniesahlberg@gmail.com>,
Shyam Prasad N <sprasad@microsoft.com>,
Tom Talpey <tom@talpey.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
David Howells <dhowells@redhat.com>,
Tyler Hicks <code@tyhicks.com>, Neil Brown <neilb@suse.de>,
Olga Kornievskaia <kolga@netapp.com>,
Dai Ngo <Dai.Ngo@oracle.com>, Miklos Szeredi <miklos@szeredi.hu>,
Amir Goldstein <amir73il@gmail.com>,
Namjae Jeon <linkinjeon@kernel.org>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org,
samba-technical@lists.samba.org, netfs@lists.linux.dev,
ecryptfs@vger.kernel.org, linux-unionfs@vger.kernel.org,
netdev@vger.kernel.org
Subject: Re: [PATCH RFC 12/24] nfsd: encoders and decoders for GET_DIR_DELEGATION
Date: Sun, 17 Mar 2024 11:34:31 -0400 [thread overview]
Message-ID: <ZfcNh4O3i19P25h1@manet.1015granger.net> (raw)
In-Reply-To: <20240315-dir-deleg-v1-12-a1d6209a3654@kernel.org>
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@kernel.org>
> ---
> 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
next prev parent reply other threads:[~2024-03-17 15:35 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-15 16:52 [PATCH RFC 00/24] vfs, nfsd, nfs: implement directory delegations Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 01/24] filelock: push the S_ISREG check down to ->setlease handlers Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 02/24] filelock: add a lm_set_conflict lease_manager callback Jeff Layton
2024-03-17 14:56 ` Chuck Lever
2024-03-18 11:07 ` Jeff Layton
2024-03-20 13:13 ` Christian Brauner
2024-03-20 13:17 ` Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 03/24] vfs: add try_break_deleg calls for parents to vfs_{link,rename,unlink} Jeff Layton
2024-03-16 23:57 ` Al Viro
2024-03-17 14:09 ` Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 04/24] vfs: allow mkdir to wait for delegation break on parent Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 05/24] vfs: allow rmdir " Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 06/24] vfs: break parent dir delegations in open(..., O_CREAT) codepath Jeff Layton
2024-03-17 0:19 ` Al Viro
2024-03-17 12:23 ` Jeff Layton
2024-03-18 8:25 ` Stefan Metzmacher
2024-03-18 10:21 ` Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 07/24] vfs: make vfs_create break delegations on parent directory Jeff Layton
2024-03-15 16:52 ` [PATCH RFC 08/24] vfs: make vfs_mknod " Jeff Layton
2024-03-20 13:42 ` Christian Brauner
2024-03-20 20:12 ` Jeff Layton
2024-03-22 14:13 ` Christian Brauner
2024-03-15 16:53 ` [PATCH RFC 09/24] filelock: lift the ban on directory leases in generic_setlease Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 10/24] nfsd: allow filecache to hold S_IFDIR files Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 11/24] nfsd: allow DELEGRETURN on directories Jeff Layton
2024-03-17 15:09 ` Chuck Lever
2024-03-17 16:03 ` Trond Myklebust
2024-03-18 11:22 ` Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 12/24] nfsd: encoders and decoders for GET_DIR_DELEGATION Jeff Layton
2024-03-17 15:34 ` Chuck Lever [this message]
2024-03-18 16:12 ` Jeff Layton
2024-03-18 16:19 ` Chuck Lever
2024-03-15 16:53 ` [PATCH RFC 13/24] nfsd: check for delegation conflicts vs. the same client Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 14/24] nfsd: wire up GET_DIR_DELEGATION handling Jeff Layton
2024-03-17 15:42 ` Chuck Lever
2024-03-15 16:53 ` [PATCH RFC 15/24] nfs: fix nfs_stateid_hash prototype when CONFIG_CRC32 isn't set Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 16/24] nfs: remove unused NFS_CALL macro Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 17/24] nfs: add cache_validity to the nfs_inode_event tracepoints Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 18/24] nfs: add a tracepoint to nfs_inode_detach_delegation_locked Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 19/24] nfs: new tracepoint in nfs_delegation_need_return Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 20/24] nfs: new tracepoint in match_stateid operation Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 21/24] nfs: add a GDD_GETATTR rpc operation Jeff Layton
2024-03-15 20:50 ` Anna Schumaker
2024-03-15 21:29 ` Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 22/24] nfs: skip dentry revalidation when parent dir has a delegation Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 23/24] nfs: optionally request a delegation on GETATTR Jeff Layton
2024-03-15 16:53 ` [PATCH RFC 24/24] nfs: add a module parameter to disable directory delegations Jeff Layton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZfcNh4O3i19P25h1@manet.1015granger.net \
--to=chuck.lever@oracle.com \
--cc=Dai.Ngo@oracle.com \
--cc=alex.aring@gmail.com \
--cc=amir73il@gmail.com \
--cc=anna@kernel.org \
--cc=brauner@kernel.org \
--cc=code@tyhicks.com \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=ecryptfs@vger.kernel.org \
--cc=edumazet@google.com \
--cc=gregkh@linuxfoundation.org \
--cc=jack@suse.cz \
--cc=jlayton@kernel.org \
--cc=kolga@netapp.com \
--cc=kuba@kernel.org \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=neilb@suse.de \
--cc=netdev@vger.kernel.org \
--cc=netfs@lists.linux.dev \
--cc=pabeni@redhat.com \
--cc=pc@manguebit.com \
--cc=rafael@kernel.org \
--cc=ronniesahlberg@gmail.com \
--cc=samba-technical@lists.samba.org \
--cc=senozhatsky@chromium.org \
--cc=sfrench@samba.org \
--cc=sprasad@microsoft.com \
--cc=tom@talpey.com \
--cc=trond.myklebust@hammerspace.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).