Re: [PATCH v5 1/1] NFSD: move accumulated callback ops to per-net namespace

["Chuck Lever" <cel@kernel.org>]

On Thu, Feb 26, 2026, at 2:35 PM, Dai Ngo wrote:
> Track accumulated callback operations on a per-network-namespace basis
> instead of globally, ensuring proper isolation and behavior when running
> nfsd in containers.

Where are the consumers of this information? "Subsequent patch"
is an OK answer, but that should be indicated here in your patch
description.


> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
>  fs/nfsd/netns.h        |  5 +++
>  fs/nfsd/nfs4callback.c | 75 ++++++++++++++++++++++--------------------
>  fs/nfsd/nfsctl.c       |  5 +++
>  fs/nfsd/state.h        |  2 ++
>  4 files changed, 52 insertions(+), 35 deletions(-)
>
> v2:
>   . free memory allocated for nn->nfsd_cb_version4.counts in
>     nfsd_net_cb_stats_init() on error in nfsd_net_init().
> v3:
>   . reword commit message.
>   . fix initialization of nn->nfsd_cb_program.nrvers.
> v4:
>   . fix merge conflict in nfsd_net_exit in nfsd-testing branch.
> v5:
>   . restore commit message to the original in v1
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 6ad3fe5d7e12..c101bf2c24c2 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -228,6 +228,11 @@ struct nfsd_net {
>  	struct list_head	local_clients;
>  #endif
>  	siphash_key_t		*fh_key;
> +
> +	struct rpc_version	nfsd_cb_version4;
> +	const struct rpc_version *nfsd_cb_versions[2];

I know this is copy-paste of existing code, but can you find a
proper symbolic constant to use here instead of "2" ?


> +	struct rpc_program	nfsd_cb_program;
> +	struct rpc_stat		nfsd_cb_stat;
>  };
> 
>  /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index aea8bdd2fdc4..759f24657c34 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -1016,7 +1016,7 @@ static int nfs4_xdr_dec_cb_offload(struct rpc_rqst *rqstp,
>  	.p_decode  = nfs4_xdr_dec_##restype,				\
>  	.p_arglen  = NFS4_enc_##argtype##_sz,				\
>  	.p_replen  = NFS4_dec_##restype##_sz,				\
> -	.p_statidx = NFSPROC4_CB_##call,				\
> +	.p_statidx = NFSPROC4_CLNT_##proc,				\
>  	.p_name    = #proc,						\
>  }

Previously all compound-based callbacks mapped to statidx 1
(NFSPROC4_CB_COMPOUND); now each operation gets its own counter
slot (values 0–7). This changes what stats are reported, IIUC.
So bundling it here means a bisect on a stats regression cannot
isolate when accounting changed, and reverting either change
forces reverting both.

IMO this should be a pre-requisite commit with its own
rationale.


> @@ -1032,40 +1032,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
>  	PROC(CB_GETATTR,	COMPOUND,	cb_getattr,	cb_getattr),
>  };
> 
> -static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
> -static const struct rpc_version nfs_cb_version4 = {
> -/*
> - * Note on the callback rpc program version number: despite language in rfc
> - * 5661 section 18.36.3 requiring servers to use 4 in this field, the
> - * official xdr descriptions for both 4.0 and 4.1 specify version 1, and
> - * in practice that appears to be what implementations use.  The section
> - * 18.36.3 language is expected to be fixed in an erratum.
> - */
> -	.number			= 1,
> -	.nrprocs		= ARRAY_SIZE(nfs4_cb_procedures),
> -	.procs			= nfs4_cb_procedures,
> -	.counts			= nfs4_cb_counts,
> -};
> -
> -static const struct rpc_version *nfs_cb_version[2] = {
> -	[1] = &nfs_cb_version4,
> -};
> -
> -static const struct rpc_program cb_program;
> -
> -static struct rpc_stat cb_stats = {
> -	.program		= &cb_program
> -};
> -
>  #define NFS4_CALLBACK 0x40000000
> -static const struct rpc_program cb_program = {
> -	.name			= "nfs4_cb",
> -	.number			= NFS4_CALLBACK,
> -	.nrvers			= ARRAY_SIZE(nfs_cb_version),
> -	.version		= nfs_cb_version,
> -	.stats			= &cb_stats,
> -	.pipe_dir_name		= "nfsd4_cb",
> -};
> 
>  static int max_cb_time(struct net *net)
>  {
> @@ -1152,14 +1119,15 @@ static int setup_callback_client(struct 
> nfs4_client *clp, struct nfs4_cb_conn *c
>  		.addrsize	= conn->cb_addrlen,
>  		.saddress	= (struct sockaddr *) &conn->cb_saddr,
>  		.timeout	= &timeparms,
> -		.program	= &cb_program,
>  		.version	= 1,
>  		.flags		= (RPC_CLNT_CREATE_NOPING | RPC_CLNT_CREATE_QUIET),
>  		.cred		= current_cred(),
>  	};
>  	struct rpc_clnt *client;
>  	const struct cred *cred;
> +	struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);

Nit: Reverse Christmas tree ordering -- this new declaration
belongs close to the top.


> +	args.program = &nn->nfsd_cb_program;
>  	if (clp->cl_minorversion == 0) {
>  		if (!clp->cl_cred.cr_principal &&
>  		    (clp->cl_cred.cr_flavor >= RPC_AUTH_GSS_KRB5)) {
> @@ -1786,3 +1754,40 @@ bool nfsd4_run_cb(struct nfsd4_callback *cb)
>  		nfsd41_cb_inflight_end(clp);
>  	return queued;
>  }
> +
> +void nfsd_net_cb_stats_shutdown(struct nfsd_net *nn)
> +{
> +	kfree(nn->nfsd_cb_version4.counts);
> +}
> +
> +int nfsd_net_cb_stats_init(struct nfsd_net *nn)
> +{
> +	nn->nfsd_cb_version4.counts = kzalloc_objs(unsigned int,
> +			ARRAY_SIZE(nfs4_cb_procedures), GFP_KERNEL);
> +	if (!nn->nfsd_cb_version4.counts)
> +		return -ENOMEM;
> +	/*
> +	 * Note on the callback rpc program version number: despite language
> +	 * in rfc 5661 section 18.36.3 requiring servers to use 4 in this
> +	 * field, the official xdr descriptions for both 4.0 and 4.1 specify
> +	 * version 1, and in practice that appears to be what implementations
> +	 * use. The section 18.36.3 language is expected to be fixed in an
> +	 * erratum.
> +	 */
> +	nn->nfsd_cb_version4.number = 1;
> +
> +	nn->nfsd_cb_version4.nrprocs = ARRAY_SIZE(nfs4_cb_procedures);
> +	nn->nfsd_cb_version4.procs = nfs4_cb_procedures;
> +	nn->nfsd_cb_versions[1] = &nn->nfsd_cb_version4;

Could you add a comment explaining that slot 0 is intentionally
NULL and slot 1 corresponds to the CB protocol version number?
The original designated-initializer syntax made this self-
evident; the replacement imperative assignment here does not.


> +
> +	memset(&nn->nfsd_cb_stat, 0, sizeof(nn->nfsd_cb_stat));
> +	nn->nfsd_cb_program.name = "nfs4_cb";
> +	nn->nfsd_cb_program.number = NFS4_CALLBACK;
> +	nn->nfsd_cb_program.nrvers = ARRAY_SIZE(nn->nfsd_cb_versions);
> +	nn->nfsd_cb_program.version = &nn->nfsd_cb_versions[0];
> +	nn->nfsd_cb_program.pipe_dir_name = "nfsd4_cb";
> +	nn->nfsd_cb_program.stats = &nn->nfsd_cb_stat;
> +	nn->nfsd_cb_stat.program = &nn->nfsd_cb_program;
> +
> +	return 0;
> +}

New non-static functions should get kernel-doc comments.


> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 032ab44feb70..5daa647ef0fa 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -2216,6 +2216,9 @@ static __net_init int nfsd_net_init(struct net *net)
>  	int retval;
>  	int i;
> 
> +	retval = nfsd_net_cb_stats_init(nn);
> +	if (retval)
> +		return retval;

Does this build if CONFIG_NFSD_V4 is not enabled?


>  	retval = nfsd_export_init(net);
>  	if (retval)
>  		goto out_export_error;
> @@ -2256,6 +2259,7 @@ static __net_init int nfsd_net_init(struct net *net)
>  out_idmap_error:
>  	nfsd_export_shutdown(net);
>  out_export_error:
> +	nfsd_net_cb_stats_shutdown(nn);
>  	return retval;
>  }
> 
> @@ -2286,6 +2290,7 @@ static __net_exit void nfsd_net_exit(struct net *net)
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> 
>  	kfree_sensitive(nn->fh_key);
> +	nfsd_net_cb_stats_shutdown(nn);
>  	nfsd_proc_stat_shutdown(net);
>  	percpu_counter_destroy_many(nn->counter, NFSD_STATS_COUNTERS_NUM);
>  	nfsd_idmap_shutdown(net);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 9b05462da4cc..490193c1877d 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -895,4 +895,6 @@ struct nfsd4_get_dir_delegation;
>  struct nfs4_delegation *nfsd_get_dir_deleg(struct nfsd4_compound_state *cstate,
>  						struct nfsd4_get_dir_delegation *gdd,
>  						struct nfsd_file *nf);
> +int nfsd_net_cb_stats_init(struct nfsd_net *nn);
> +void nfsd_net_cb_stats_shutdown(struct nfsd_net *nn);
>  #endif   /* NFSD4_STATE_H */
> -- 
> 2.47.3

-- 
Chuck Lever