Date: Wed, 27 Sep 2023 09:05:10 +1000
From: "NeilBrown" <neilb@...e.de>
To: "Lorenzo Bianconi" <lorenzo@...nel.org>
Cc: linux-nfs@...r.kernel.org, lorenzo.bianconi@...hat.com, jlayton@...nel.org,
 chuck.lever@...cle.com, netdev@...r.kernel.org
Subject: Re: [PATCH v3] NFSD: convert write_threads, write_maxblksize and
 write_maxconn to netlink commands


Hi,
 thanks for doing this.  I had a hunt through the patch largely to help
 improve my own understanding of netlink.  A few things stood out.  Not
 necessarily problems with the patch, but things that seemed worth
 mentioning. 

 Firstly, and rather trivially, the word "convert" in the subject lead
 me to think that the old approach was being converted to a new
 approach.  I see that isn't correct - you are just introducing a new
 option.

On Wed, 27 Sep 2023, Lorenzo Bianconi wrote:
> Introduce write_threads, write_maxblksize and write_maxconn netlink
> commands similar to the ones available through the procfs.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
> Changes since v2:
> - use u32 to store nthreads in nfsd_nl_threads_set_doit
> - rename server-attr in control-plane in nfsd.yaml specs
> Changes since v1:
> - remove write_v4_end_grace command
> - add write_maxblksize and write_maxconn netlink commands
> 
> This patch can be tested with user-space tool reported below:
> https://github.com/LorenzoBianconi/nfsd-netlink.git
> ---
>  Documentation/netlink/specs/nfsd.yaml |  63 ++++++++++++
>  fs/nfsd/netlink.c                     |  51 ++++++++++
>  fs/nfsd/netlink.h                     |   9 ++
>  fs/nfsd/nfsctl.c                      | 141 ++++++++++++++++++++++++++
>  include/uapi/linux/nfsd_netlink.h     |  15 +++
>  5 files changed, 279 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 403d3e3a04f3..c6af1653bc1d 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -62,6 +62,18 @@ attribute-sets:
>          name: compound-ops
>          type: u32
>          multi-attr: true
> +  -
> +    name: control-plane
> +    attributes:
> +      -
> +        name: threads
> +        type: u32
> +      -
> +        name: max-blksize
> +        type: u32
> +      -
> +        name: max-conn
> +        type: u32
>  
>  operations:
>    list:
> @@ -72,3 +84,54 @@ operations:
>        dump:
>          pre: nfsd-nl-rpc-status-get-start
>          post: nfsd-nl-rpc-status-get-done
> +    -
> +      name: threads-set
> +      doc: set the number of running threads
> +      attribute-set: control-plane
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - threads
> +    -
> +      name: threads-get
> +      doc: dump the number of running threads
> +      attribute-set: control-plane
> +      dump:
> +        reply:
> +          attributes:
> +            - threads
> +    -
> +      name: max-blksize-set
> +      doc: set the nfs block size
> +      attribute-set: control-plane
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - max-blksize
> +    -
> +      name: max-blksize-get
> +      doc: dump the nfs block size
> +      attribute-set: control-plane
> +      dump:
> +        reply:
> +          attributes:
> +            - max-blksize
> +    -
> +      name: max-conn-set
> +      doc: set the max number of connections
> +      attribute-set: control-plane
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - max-conn
> +    -
> +      name: max-conn-get
> +      doc: dump the max number of connections
> +      attribute-set: control-plane
> +      dump:
> +        reply:
> +          attributes:
> +            - max-conn
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0e1d635ec5f9..8f7d72ae60d6 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -10,6 +10,21 @@
>  
>  #include <uapi/linux/nfsd_netlink.h>
>  
> +/* NFSD_CMD_THREADS_SET - do */
> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_CONTROL_PLANE_THREADS + 1] = {
> +	[NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, },
> +};

This array, and the following arrays, is sized exactly large enough to
hold the last element.  In that case you don't need to explicitly set
the size:

 +static const struct nla_policy nfsd_threads_set_nl_policy[] = {
 +	[NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, },
 +};

I at first assumed you were following a standard practice, but other
places that declare nla_policy use a variety of different approaches.
I prefer the [] approach, but it is up to you.


> +
> +/* NFSD_CMD_MAX_BLKSIZE_SET - do */
> +static const struct nla_policy nfsd_max_blksize_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE + 1] = {
> +	[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE] = { .type = NLA_U32, },
> +};
> +
> +/* NFSD_CMD_MAX_CONN_SET - do */
> +static const struct nla_policy nfsd_max_conn_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_CONN + 1] = {
> +	[NFSD_A_CONTROL_PLANE_MAX_CONN] = { .type = NLA_U32, },
> +};
> +
>  /* Ops table for nfsd */
>  static const struct genl_split_ops nfsd_nl_ops[] = {
>  	{
> @@ -19,6 +34,42 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
>  		.done	= nfsd_nl_rpc_status_get_done,
>  		.flags	= GENL_CMD_CAP_DUMP,
>  	},
> +	{
> +		.cmd		= NFSD_CMD_THREADS_SET,
> +		.doit		= nfsd_nl_threads_set_doit,
> +		.policy		= nfsd_threads_set_nl_policy,
> +		.maxattr	= NFSD_A_CONTROL_PLANE_THREADS,

maxattr is *always* the largest index for the policy array.
I think it would aid reading to have something like

#define NLA_POLICY(_pol) .policy = _pol, .maxattr = ARRAY_SIZE(_pol) - 1
 
then you could have
		.doit  = nfsd_nl_thread_set_doit,
		NLA_POLICY(nfsd_thread_set_nl_policy),

and be certain that all the maxattr values are correct.

> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	= NFSD_CMD_THREADS_GET,
> +		.dumpit	= nfsd_nl_threads_get_dumpit,
> +		.flags	= GENL_CMD_CAP_DUMP,
> +	},
> +	{
> +		.cmd		= NFSD_CMD_MAX_BLKSIZE_SET,
> +		.doit		= nfsd_nl_max_blksize_set_doit,
> +		.policy		= nfsd_max_blksize_set_nl_policy,
> +		.maxattr	= NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},

it is a little weird that the dumpit sections are indented differently
to the doit section.  But that is probably intentional and I might even
grow to like it...

> +	{
> +		.cmd	= NFSD_CMD_MAX_BLKSIZE_GET,
> +		.dumpit	= nfsd_nl_max_blksize_get_dumpit,
> +		.flags	= GENL_CMD_CAP_DUMP,
> +	},
> +	{
> +		.cmd		= NFSD_CMD_MAX_CONN_SET,
> +		.doit		= nfsd_nl_max_conn_set_doit,
> +		.policy		= nfsd_max_conn_set_nl_policy,
> +		.maxattr	= NFSD_A_CONTROL_PLANE_MAX_CONN,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	= NFSD_CMD_MAX_CONN_GET,
> +		.dumpit	= nfsd_nl_max_conn_get_dumpit,
> +		.flags	= GENL_CMD_CAP_DUMP,
> +	},
>  };
>  
>  struct genl_family nfsd_nl_family __ro_after_init = {
> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> index d83dd6bdee92..41b95651c638 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -16,6 +16,15 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb);
>  
>  int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
>  				  struct netlink_callback *cb);
> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb,
> +			       struct netlink_callback *cb);
> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb,
> +				   struct netlink_callback *cb);
> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb,
> +				struct netlink_callback *cb);
>  
>  extern struct genl_family nfsd_nl_family;
>  
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index b71744e355a8..07e7a09e28e3 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1694,6 +1694,147 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb)
>  	return 0;
>  }
>  
> +/**
> + * nfsd_nl_threads_set_doit - set the number of running threads
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	u32 nthreads;
> +	int ret;
> +
> +	if (!info->attrs[NFSD_A_CONTROL_PLANE_THREADS])
> +		return -EINVAL;
> +
> +	nthreads = nla_get_u32(info->attrs[NFSD_A_CONTROL_PLANE_THREADS]);
> +
> +	ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> +	return ret == nthreads ? 0 : ret;
> +}
> +
> +static int nfsd_nl_get_dump(struct sk_buff *skb, struct netlink_callback *cb,
> +			    int cmd, int attr, u32 val)
> +{
> +	void *hdr;
> +
> +	if (cb->args[0]) /* already consumed */
> +		return 0;
> +
> +	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> +			  &nfsd_nl_family, NLM_F_MULTI, cmd);
> +	if (!hdr)
> +		return -ENOBUFS;
> +
> +	if (nla_put_u32(skb, attr, val))
> +		return -ENOBUFS;
> +
> +	genlmsg_end(skb, hdr);
> +	cb->args[0] = 1;
> +
> +	return skb->len;
> +}
> +
> +/**
> + * nfsd_nl_threads_get_dumpit - dump the number of running threads
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> +	return nfsd_nl_get_dump(skb, cb, NFSD_CMD_THREADS_GET,
> +				NFSD_A_CONTROL_PLANE_THREADS,
> +				nfsd_nrthreads(sock_net(skb->sk)));
> +}
> +
> +/**
> + * nfsd_nl_max_blksize_set_doit - set the nfs block size
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> +	struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE];
> +	int ret = 0;
> +
> +	if (!attr)
> +		return -EINVAL;
> +
> +	mutex_lock(&nfsd_mutex);
> +	if (nn->nfsd_serv) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

This code is wrong... but then the original in write_maxblksize is wrong
to, so you can't be blamed.
nfsd_max_blksize applies to nfsd in ALL network namespaces.  So if we
need to check there are no active services in one namespace, we need to
check the same for *all* namespaces.

I think we should make nfsd_max_blksize a per-namespace value.

> +
> +	nfsd_max_blksize = nla_get_u32(attr);
> +	nfsd_max_blksize = max_t(int, nfsd_max_blksize, 1024);
> +	nfsd_max_blksize = min_t(int, nfsd_max_blksize, NFSSVC_MAXBLKSIZE);
> +	nfsd_max_blksize &= ~1023;
> +out:
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return ret;
> +}
> +
> +/**
> + * nfsd_nl_max_blksize_get_dumpit - dump the nfs block size
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb,
> +				   struct netlink_callback *cb)
> +{
> +	return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_BLKSIZE_GET,
> +				NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> +				nfsd_max_blksize);
> +}
> +
> +/**
> + * nfsd_nl_max_conn_set_doit - set the max number of connections
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> +	struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_CONN];
> +
> +	if (!attr)
> +		return -EINVAL;
> +
> +	nn->max_connections = nla_get_u32(attr);
> +
> +	return 0;
> +}
> +
> +/**
> + * nfsd_nl_max_conn_get_dumpit - dump the max number of connections
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb,
> +				struct netlink_callback *cb)
> +{
> +	struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> +
> +	return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_CONN_GET,
> +				NFSD_A_CONTROL_PLANE_MAX_CONN,
> +				nn->max_connections);
> +}
> +
>  /**
>   * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
>   * @net: a freshly-created network namespace
> diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> index c8ae72466ee6..145f4811f3d9 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -29,8 +29,23 @@ enum {
>  	NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
>  };
>  
> +enum {
> +	NFSD_A_CONTROL_PLANE_THREADS = 1,
> +	NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> +	NFSD_A_CONTROL_PLANE_MAX_CONN,
> +
> +	__NFSD_A_CONTROL_PLANE_MAX,
> +	NFSD_A_CONTROL_PLANE_MAX = (__NFSD_A_CONTROL_PLANE_MAX - 1)

This last value is never used.  Is it needed?


Thanks,
NeilBrown

> +};
> +
>  enum {
>  	NFSD_CMD_RPC_STATUS_GET = 1,
> +	NFSD_CMD_THREADS_SET,
> +	NFSD_CMD_THREADS_GET,
> +	NFSD_CMD_MAX_BLKSIZE_SET,
> +	NFSD_CMD_MAX_BLKSIZE_GET,
> +	NFSD_CMD_MAX_CONN_SET,
> +	NFSD_CMD_MAX_CONN_GET,
>  
>  	__NFSD_CMD_MAX,
>  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> -- 
> 2.41.0
> 
> 

Powered by blists - more mailing lists