Received-SPF: pass (google.com: domain of linux-nfs+bounces-2854-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Lorenzo Bianconi" <lorenzo@kernel.org>
Cc: linux-nfs@vger.kernel.org, lorenzo.bianconi@redhat.com,
 chuck.lever@oracle.com, netdev@vger.kernel.org, kuba@kernel.org,
 jlayton@kernel.org
Subject: Re: [PATCH v8 2/6] NFSD: convert write_threads to netlink command
In-reply-to:
 <4ff777ebb8652e31709bd91c3af50693edf86a26.1713209938.git.lorenzo@kernel.org>
References: <cover.1713209938.git.lorenzo@kernel.org>,
 <4ff777ebb8652e31709bd91c3af50693edf86a26.1713209938.git.lorenzo@kernel.org>
Date: Wed, 17 Apr 2024 08:28:33 +1000
Message-id: <171330651306.17212.2078718779289951086@noble.neil.brown.name>

On Tue, 16 Apr 2024, Lorenzo Bianconi wrote:
> Introduce write_threads netlink command similar to the one available
> through the procfs.
>=20
> Tested-by: Jeff Layton <jlayton@kernel.org>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Co-developed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  Documentation/netlink/specs/nfsd.yaml |  33 ++++++++
>  fs/nfsd/netlink.c                     |  19 +++++
>  fs/nfsd/netlink.h                     |   2 +
>  fs/nfsd/nfsctl.c                      | 104 ++++++++++++++++++++++++++
>  include/uapi/linux/nfsd_netlink.h     |  11 +++
>  5 files changed, 169 insertions(+)
>=20
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/=
specs/nfsd.yaml
> index 05acc73e2e33..cbe6c5fd6c4d 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: server-worker
> +    attributes:
> +      -
> +        name: threads
> +        type: u32
> +      -
> +        name: gracetime
> +        type: u32
> +      -
> +        name: leasetime
> +        type: u32

Another thought: I would be really happy if the "scope" were another
optional argument here.  The mechanism of setting the scope by user the
hostname works but is ugly.  I'm inclined to ignore the hostname
completely when netlink is used, but I'm not completely sure about that.
(aside - I think using the hostname for the default scope was a really
bad idea.  It should have been a fixed string like "Linux").

NeilBrown


> =20
>  operations:
>    list:
> @@ -87,3 +99,24 @@ operations:
>              - sport
>              - dport
>              - compound-ops
> +    -
> +      name: threads-set
> +      doc: set the number of running threads
> +      attribute-set: server-worker
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - threads
> +            - gracetime
> +            - leasetime
> +    -
> +      name: threads-get
> +      doc: get the number of running threads
> +      attribute-set: server-worker
> +      do:
> +        reply:
> +          attributes:
> +            - threads
> +            - gracetime
> +            - leasetime
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0e1d635ec5f9..20a646af0324 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -10,6 +10,13 @@
> =20
>  #include <uapi/linux/nfsd_netlink.h>
> =20
> +/* NFSD_CMD_THREADS_SET - do */
> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WO=
RKER_LEASETIME + 1] =3D {
> +	[NFSD_A_SERVER_WORKER_THREADS] =3D { .type =3D NLA_U32, },
> +	[NFSD_A_SERVER_WORKER_GRACETIME] =3D { .type =3D NLA_U32, },
> +	[NFSD_A_SERVER_WORKER_LEASETIME] =3D { .type =3D NLA_U32, },
> +};
> +
>  /* Ops table for nfsd */
>  static const struct genl_split_ops nfsd_nl_ops[] =3D {
>  	{
> @@ -19,6 +26,18 @@ static const struct genl_split_ops nfsd_nl_ops[] =3D {
>  		.done	=3D nfsd_nl_rpc_status_get_done,
>  		.flags	=3D GENL_CMD_CAP_DUMP,
>  	},
> +	{
> +		.cmd		=3D NFSD_CMD_THREADS_SET,
> +		.doit		=3D nfsd_nl_threads_set_doit,
> +		.policy		=3D nfsd_threads_set_nl_policy,
> +		.maxattr	=3D NFSD_A_SERVER_WORKER_LEASETIME,
> +		.flags		=3D GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	=3D NFSD_CMD_THREADS_GET,
> +		.doit	=3D nfsd_nl_threads_get_doit,
> +		.flags	=3D GENL_CMD_CAP_DO,
> +	},
>  };
> =20
>  struct genl_family nfsd_nl_family __ro_after_init =3D {
> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> index d83dd6bdee92..4137fac477e4 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -16,6 +16,8 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *=
cb);
> =20
>  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_doit(struct sk_buff *skb, struct genl_info *info);
> =20
>  extern struct genl_family nfsd_nl_family;
> =20
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index f2e442d7fe16..38a5df03981b 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1653,6 +1653,110 @@ int nfsd_nl_rpc_status_get_done(struct netlink_call=
back *cb)
>  	return 0;
>  }
> =20
> +/**
> + * 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)
> +{
> +	struct net *net =3D genl_info_net(info);
> +	struct nfsd_net *nn =3D net_generic(net, nfsd_net_id);
> +	int ret =3D -EBUSY;
> +	u32 nthreads;
> +
> +	if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_WORKER_THREADS))
> +		return -EINVAL;
> +
> +	nthreads =3D nla_get_u32(info->attrs[NFSD_A_SERVER_WORKER_THREADS]);
> +
> +	mutex_lock(&nfsd_mutex);
> +	if (info->attrs[NFSD_A_SERVER_WORKER_GRACETIME] ||
> +	    info->attrs[NFSD_A_SERVER_WORKER_LEASETIME]) {
> +		const struct nlattr *attr;
> +
> +		if (nn->nfsd_serv && nn->nfsd_serv->sv_nrthreads)
> +			goto out_unlock;
> +
> +		ret =3D -EINVAL;
> +		attr =3D info->attrs[NFSD_A_SERVER_WORKER_GRACETIME];
> +		if (attr) {
> +			u32 gracetime =3D nla_get_u32(attr);
> +
> +			if (gracetime < 10 || gracetime > 3600)
> +				goto out_unlock;
> +
> +			nn->nfsd4_grace =3D gracetime;
> +		}
> +
> +		attr =3D info->attrs[NFSD_A_SERVER_WORKER_LEASETIME];
> +		if (attr) {
> +			u32 leasetime =3D nla_get_u32(attr);
> +
> +			if (leasetime < 10 || leasetime > 3600)
> +				goto out_unlock;
> +
> +			nn->nfsd4_lease =3D leasetime;
> +		}
> +	}
> +
> +	ret =3D nfsd_svc(nthreads, net, get_current_cred());
> +out_unlock:
> +	mutex_unlock(&nfsd_mutex);
> +
> +	return ret =3D=3D nthreads ? 0 : ret;
> +}
> +
> +/**
> + * nfsd_nl_threads_get_doit - get 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_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct net *net =3D genl_info_net(info);
> +	struct nfsd_net *nn =3D net_generic(net, nfsd_net_id);
> +	void *hdr;
> +	int err;
> +
> +	skb =3D genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	hdr =3D genlmsg_iput(skb, info);
> +	if (!hdr) {
> +		err =3D -EMSGSIZE;
> +		goto err_free_msg;
> +	}
> +
> +	mutex_lock(&nfsd_mutex);
> +	err =3D nla_put_u32(skb, NFSD_A_SERVER_WORKER_GRACETIME,
> +			  nn->nfsd4_grace) ||
> +	      nla_put_u32(skb, NFSD_A_SERVER_WORKER_LEASETIME,
> +			  nn->nfsd4_lease) ||
> +	      nla_put_u32(skb, NFSD_A_SERVER_WORKER_THREADS,
> +			  nn->nfsd_serv ? nn->nfsd_serv->sv_nrthreads : 0);
> +	mutex_unlock(&nfsd_mutex);
> +
> +	if (err) {
> +		err =3D -EINVAL;
> +		goto err_free_msg;
> +	}
> +
> +	genlmsg_end(skb, hdr);
> +
> +	return genlmsg_reply(skb, info);
> +
> +err_free_msg:
> +	nlmsg_free(skb);
> +
> +	return err;
> +}
> +
>  /**
>   * 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_ne=
tlink.h
> index 3cd044edee5d..ccc78a5ee650 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -29,8 +29,19 @@ enum {
>  	NFSD_A_RPC_STATUS_MAX =3D (__NFSD_A_RPC_STATUS_MAX - 1)
>  };
> =20
> +enum {
> +	NFSD_A_SERVER_WORKER_THREADS =3D 1,
> +	NFSD_A_SERVER_WORKER_GRACETIME,
> +	NFSD_A_SERVER_WORKER_LEASETIME,
> +
> +	__NFSD_A_SERVER_WORKER_MAX,
> +	NFSD_A_SERVER_WORKER_MAX =3D (__NFSD_A_SERVER_WORKER_MAX - 1)
> +};
> +
>  enum {
>  	NFSD_CMD_RPC_STATUS_GET =3D 1,
> +	NFSD_CMD_THREADS_SET,
> +	NFSD_CMD_THREADS_GET,
> =20
>  	__NFSD_CMD_MAX,
>  	NFSD_CMD_MAX =3D (__NFSD_CMD_MAX - 1)
> --=20
> 2.44.0
>=20
>=20