Received-SPF: pass (google.com: domain of linux-nfs+bounces-2362-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 17 Mar 2024 11:34:31 -0400
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
Message-ID: <ZfcNh4O3i19P25h1@manet.1015granger.net>
References: <20240315-dir-deleg-v1-0-a1d6209a3654@kernel.org>
 <20240315-dir-deleg-v1-12-a1d6209a3654@kernel.org>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240315-dir-deleg-v1-12-a1d6209a3654@kernel.org>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?p5Yha6DDVl6ds3xNwhtoO+04Ia+YDeQeOnY4tF0Rrnl0DNXd8oJFDl3ryZJ0?=
 =?us-ascii?Q?UaTLHoRqup1aWzVkf37BSQbD17aY2ifkSU1NZlkqJ8BeNuxyAfcKuFdmOA7E?=
 =?us-ascii?Q?E+4LH2QpHRiiqsjE//Q1/SGbyLsAlUxPEn2VjMIsCloBmm9sZJNyZbDve97Y?=
 =?us-ascii?Q?X3OC7P7YyKOYci46DuR0RvgsIOqP/tRmc4i3T7oHKTLmAvuguZBxzTiJNHMy?=
 =?us-ascii?Q?wXLOR1VN3wTY2Z0Kv1eczoej1m/82yVQBJPJg/k7eyaer3j/njW3qgXuRdS6?=
 =?us-ascii?Q?buFeseVtFFSfUqHMsQQyzeSK+tSvSvrx7pqJsL53tuEoZbTpPq0oG0wGpOiz?=
 =?us-ascii?Q?gziyDanlosOot3hVFR2Q3EHKm74Ic0eOieSM29Xw1a85weDEscN4y/UtVE11?=
 =?us-ascii?Q?B1rF77N3c8wDyzR67Y/xgIIXLDlivNy9DWL1P3jJouvEICqjtAFSg/xfI2/Z?=
 =?us-ascii?Q?dPzaULO2qGVxRrTjvXXdN42rGe8ai+rK3EiCzf0JQTiIY+Amzgw0Bx9W00R3?=
 =?us-ascii?Q?k5/4wvKeR7fPXGf+pEFM7qyKD/gk6vTPkLYcKYpM5cvui5b7FssecyZATgcj?=
 =?us-ascii?Q?w99lknwwONF7fCSlWV4+WnndKoOrqt+E4v9Y6KrrI1nZpQmZSD/baPiB1YKP?=
 =?us-ascii?Q?gUHUDd/YSXqG3U2e/H6IkzPK5eHtJ3AaocotRv6mSuKngSBhE45y2sLama7m?=
 =?us-ascii?Q?hyZANZb0SkW+pR6qE8fgG6hlm1Uj15Lfz+3MxpQ69c3xSG5WLxkni/IV8ZR9?=
 =?us-ascii?Q?tvdrEanHWgVuFfj6jfIxMhqPCtYZV/J+awavNbhHbUuMrr/xFzARPd9Hh7u+?=
 =?us-ascii?Q?G6WsfUqiZOcNLOA4mgZMaeR46H1Q3TXXKHxgRvfXCsT5qwpyQZts/p/RIkcV?=
 =?us-ascii?Q?a1+xurwfE7Q3Wj3mgyw4JVyD62asoqqXwETnWhIh9XA9q8O947AonUCZv9fy?=
 =?us-ascii?Q?w37k5KHICarFCvJwWdtim4pyirMTgtCsCIakWsyFWkikXMUCwSzDyU6Arx+J?=
 =?us-ascii?Q?ovbMFCxo381N4V5Smn+WVIk8YTC0DYTbxLeR5o+pF+OaJrqH3hv9VpG/mw6C?=
 =?us-ascii?Q?NKEWQsSgZpPASRvyd+Lh8EM8BC6VZxlklY8VOHh+ZuD5iYYsXcZTW6lHLPXv?=
 =?us-ascii?Q?5YZMC9meVtFXZuZtZUkitq4ONRKEXCoNJhPvscnB1v1qWpYYrHr+nHL3+LBC?=
 =?us-ascii?Q?600aeJNpJYTFwUTYhwK9fr9MA0z7TY8qxEeUZmuTNeg3mkZlE/AF0bUequsE?=
 =?us-ascii?Q?VXdh7mxBCFlQ5O/2y1kqld9tyC0sxnxDa2Ci07P5TGyfACWMb6MXN0HVDSV/?=
 =?us-ascii?Q?shvMHZNJ4zZNH8o7s3KSP+cwe2a3xfCiadHVdvkj1w20HD4/2dxPitzVeXPZ?=
 =?us-ascii?Q?ampJJ+BDzUqpG8BQZAQcQDDrvlmgOdm53mVSQ/acaiQgJ1BXNFziuNtr0zoR?=
 =?us-ascii?Q?DF0RLz0CooSs/4U7LhjLC6vyk09OEFskst3H3OfFmscNDRInRFsvtgtAlkAN?=
 =?us-ascii?Q?sr/RypbiW6eXG1wXkXbbBeUEwVxvqeyMUbOzzkxiP2Opp8GYyMSapbz6dpQe?=
 =?us-ascii?Q?l+h0kfHwqGBvwoWOZbl8Wv7yaQ3baplk5gNrz8zA2odIas+9J2rR7eWwZkwT?=
 =?us-ascii?Q?Ag=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	reG2LbleKAHuHcYxz8PNl4H3Mo6zRNe0OlhMaDiygMMUQbYUXcOU4ViZPakiWT6HTqM61s4u1VdmfE+mETG6QcC61TDLkTXxtkLtYymeKMQtWvIGZF2KKX+9Zt7fl/14qcFwyi5+1hYzNGLR7QGsnp6fr/XY90s3OguhHDge1SBCMjdLBSkiyHEs3Asd2LRmtteWk+3f9C2z4PaGBNtuVc50/L5Z7bTe5FJWM+H4j983napmSKLe0fQX0wBlP790mbOwGRZo1v8uCMIkQTueXhG3D63Ghs22FKgylciSX3LuC5ShEoQpvbqkGEu92JP1bNx9q4sX4mo4V7ZvvUi5qFo25DcPxn0rXO8NJXBPnH36+6vW+MoyL5akS7WkC2qj1i8bYx109TCDabwadEOtyLBYv8FhevjpBoDjVC+8aLC2SqntOpHwS1KTi1t5u2joGlYJX4NowWKGEiDn6p389JYJPoPq4k4G+nNxngjdkQrlzZIvhNq+9jcskM0J4/Th/A2Wj0BnMhfaqNHhbIrhftWVu3NLOi0sSfHFbAZ1mxgmqtcEPHdwWfgb+sbtPXJsYzadXladoLur/caO4sNHsJi3p6YNkBXKufIB87jg6Qs=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4847dd3a-4170-46f1-7a91-08dc4697c3c6
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2024 15:34:41.9480
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QDlKxGI6gKKBXdqsbbTcDiB6mnGTyxbifdXOcEVecFenjoxhdDV8euSoKo3DyZQNp2c3QV1WwOT2xifswr0uZg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR10MB7133
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-03-17_11,2024-03-15_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 malwarescore=0
 phishscore=0 adultscore=0 bulkscore=0 mlxscore=0 spamscore=0
 mlxlogscore=974 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2403140000 definitions=main-2403170121
X-Proofpoint-GUID: ue0Xg9ZYPiTOLENbWv_d0uPbhRC_LZvV
X-Proofpoint-ORIG-GUID: ue0Xg9ZYPiTOLENbWv_d0uPbhRC_LZvV

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