NFSD: convert write_threads and write_v4_end_grace to netlink commands

Commit Message

Lorenzo Bianconi Sept. 22, 2023, 12:44 p.m. UTC

Introduce write_threads and write_v4_end_grace netlink commands similar
to the ones available through the procfs.
Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
report global server metadata.

Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
This patch can be tested with user-space tool reported below:
https://github.com/LorenzoBianconi/nfsd-netlink.git
---
 Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
 fs/nfsd/netlink.c                     | 30 ++++++++
 fs/nfsd/netlink.h                     |  5 ++
 fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
 include/uapi/linux/nfsd_netlink.h     | 11 +++
 5 files changed, 177 insertions(+)

Comments

Jeff Layton Sept. 22, 2023, 4:04 p.m. UTC | #1

On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> Introduce write_threads and write_v4_end_grace netlink commands similar
> to the ones available through the procfs.
> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> report global server metadata.
> 
> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> ---
> This patch can be tested with user-space tool reported below:
> https://github.com/LorenzoBianconi/nfsd-netlink.git
> ---
>  Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>  fs/nfsd/netlink.c                     | 30 ++++++++
>  fs/nfsd/netlink.h                     |  5 ++
>  fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>  include/uapi/linux/nfsd_netlink.h     | 11 +++
>  5 files changed, 177 insertions(+)
> 
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 403d3e3a04f3..fa1204892703 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -62,6 +62,15 @@ attribute-sets:
>          name: compound-ops
>          type: u32
>          multi-attr: true
> +  -
> +    name: server-attr
> +    attributes:
> +      -
> +        name: threads
> +        type: u16

65k threads ought to be enough for anybody!

> +      -
> +        name: v4-grace
> +        type: u8
>  
>  operations:
>    list:
> @@ -72,3 +81,27 @@ 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: server-attr
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - threads
> +    -
> +      name: v4-grace-release
> +      doc: release the grace period for nfsd's v4 lock manager
> +      attribute-set: server-attr
> +      flags: [ admin-perm ]
> +      do:
> +        request:
> +          attributes:
> +            - v4-grace
> +    -
> +      name: server-status-get
> +      doc: dump server status info
> +      attribute-set: server-attr
> +      dump:
> +        pre: nfsd-nl-server-status-get-start
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0e1d635ec5f9..783a34e69354 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -10,6 +10,16 @@
>  
>  #include <uapi/linux/nfsd_netlink.h>
>  
> +/* NFSD_CMD_THREADS_SET - do */
> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
> +	[NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
> +};
> +
> +/* NFSD_CMD_V4_GRACE_RELEASE - do */
> +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
> +	[NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
> +};
> +
>  /* Ops table for nfsd */
>  static const struct genl_split_ops nfsd_nl_ops[] = {
>  	{
> @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd		= NFSD_CMD_V4_GRACE_RELEASE,
> +		.doit		= nfsd_nl_v4_grace_release_doit,
> +		.policy		= nfsd_v4_grace_release_nl_policy,
> +		.maxattr	= NFSD_A_SERVER_ATTR_V4_GRACE,
> +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> +	},
> +	{
> +		.cmd	= NFSD_CMD_SERVER_STATUS_GET,
> +		.start	= nfsd_nl_server_status_get_start,
> +		.dumpit	= nfsd_nl_server_status_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..2e98061fbb0a 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -12,10 +12,15 @@
>  #include <uapi/linux/nfsd_netlink.h>
>  
>  int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
>  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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_server_status_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..c631b59b7a4f 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1694,6 +1694,104 @@ 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)
> +{
> +	u16 nthreads;
> +	int ret;
> +
> +	if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
> +		return -EINVAL;
> +
> +	nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
> +
> +	ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> +	return ret == nthreads ? 0 : ret;
> +}
> +
> +/**
> + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +#ifdef CONFIG_NFSD_V4
> +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> +
> +	if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
> +		return -EINVAL;
> +
> +	if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
> +		nfsd4_end_grace(nn);
> +

To be clear here. Issuing this with anything but 0 will end the grace
period. A value of 0 is ignored. It might be best to make the value not
matter at all. Do we have to send down a value at all?

> +	return 0;
> +#else
> +	return -EOPNOTSUPP;
> +#endif /* CONFIG_NFSD_V4 */
> +}
> +
> +/**
> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> + * @cb: netlink metadata and command arguments
> + *
> + * Return values:
> + *   %0: The server_status_get command may proceed
> + *   %-ENODEV: There is no NFSD running in this namespace
> + */
> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> +{
> +	struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> +
> +	return nn->nfsd_serv ? 0 : -ENODEV;
> +}
> +
> +/**
> + * nfsd_nl_server_status_get_dumpit - dump server status info
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> +				     struct netlink_callback *cb)
> +{
> +	struct net *net = sock_net(skb->sk);
> +#ifdef CONFIG_NFSD_V4
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +#endif /* CONFIG_NFSD_V4 */
> +	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,
> +			  NFSD_CMD_SERVER_STATUS_GET);
> +	if (!hdr)
> +		return -ENOBUFS;
> +
> +	if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> +		return -ENOBUFS;
> +#ifdef CONFIG_NFSD_V4
> +	if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> +		return -ENOBUFS;
> +#endif /* CONFIG_NFSD_V4 */
> +
> +	genlmsg_end(skb, hdr);
> +	cb->args[0] = 1;
> +
> +	return skb->len;
> +}
> +
>  /**
>   * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
>  };
>  
> +enum {
> +	NFSD_A_SERVER_ATTR_THREADS = 1,
> +	NFSD_A_SERVER_ATTR_V4_GRACE,
> +
> +	__NFSD_A_SERVER_ATTR_MAX,
> +	NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> +};
> +
>  enum {
>  	NFSD_CMD_RPC_STATUS_GET = 1,
> +	NFSD_CMD_THREADS_SET,
> +	NFSD_CMD_V4_GRACE_RELEASE,
> +	NFSD_CMD_SERVER_STATUS_GET,
>  
>  	__NFSD_CMD_MAX,
>  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)

Chuck Lever Sept. 22, 2023, 4:06 p.m. UTC | #2

> On Sep 22, 2023, at 12:04 PM, Jeff Layton <[e-mail]> wrote:
> 
> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
>> Introduce write_threads and write_v4_end_grace netlink commands similar
>> to the ones available through the procfs.
>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
>> report global server metadata.
>> 
>> Signed-off-by: Lorenzo Bianconi <[e-mail]>
>> ---
>> This patch can be tested with user-space tool reported below:
>> https://github.com/LorenzoBianconi/nfsd-netlink.git
>> ---
>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>> fs/nfsd/netlink.c                     | 30 ++++++++
>> fs/nfsd/netlink.h                     |  5 ++
>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>> include/uapi/linux/nfsd_netlink.h     | 11 +++
>> 5 files changed, 177 insertions(+)
>> 
>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
>> index 403d3e3a04f3..fa1204892703 100644
>> --- a/Documentation/netlink/specs/nfsd.yaml
>> +++ b/Documentation/netlink/specs/nfsd.yaml
>> @@ -62,6 +62,15 @@ attribute-sets:
>>         name: compound-ops
>>         type: u32
>>         multi-attr: true
>> +  -
>> +    name: server-attr
>> +    attributes:
>> +      -
>> +        name: threads
>> +        type: u16
> 
> 65k threads ought to be enough for anybody!

No argument.

But I thought you could echo a negative number of threads in /proc/fs/nfsd/threads
to reduce the thread count. Maybe this field should be an s32?


>> +      -
>> +        name: v4-grace
>> +        type: u8
>> 
>> operations:
>>   list:
>> @@ -72,3 +81,27 @@ 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: server-attr
>> +      flags: [ admin-perm ]
>> +      do:
>> +        request:
>> +          attributes:
>> +            - threads
>> +    -
>> +      name: v4-grace-release
>> +      doc: release the grace period for nfsd's v4 lock manager
>> +      attribute-set: server-attr
>> +      flags: [ admin-perm ]
>> +      do:
>> +        request:
>> +          attributes:
>> +            - v4-grace
>> +    -
>> +      name: server-status-get
>> +      doc: dump server status info
>> +      attribute-set: server-attr
>> +      dump:
>> +        pre: nfsd-nl-server-status-get-start
>> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
>> index 0e1d635ec5f9..783a34e69354 100644
>> --- a/fs/nfsd/netlink.c
>> +++ b/fs/nfsd/netlink.c
>> @@ -10,6 +10,16 @@
>> 
>> #include <uapi/linux/nfsd_netlink.h>
>> 
>> +/* NFSD_CMD_THREADS_SET - do */
>> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
>> + [NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
>> +};
>> +
>> +/* NFSD_CMD_V4_GRACE_RELEASE - do */
>> +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
>> + [NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
>> +};
>> +
>> /* Ops table for nfsd */
>> static const struct genl_split_ops nfsd_nl_ops[] = {
>> {
>> @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>> + },
>> + {
>> + .cmd = NFSD_CMD_V4_GRACE_RELEASE,
>> + .doit = nfsd_nl_v4_grace_release_doit,
>> + .policy = nfsd_v4_grace_release_nl_policy,
>> + .maxattr = NFSD_A_SERVER_ATTR_V4_GRACE,
>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>> + },
>> + {
>> + .cmd = NFSD_CMD_SERVER_STATUS_GET,
>> + .start = nfsd_nl_server_status_get_start,
>> + .dumpit = nfsd_nl_server_status_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..2e98061fbb0a 100644
>> --- a/fs/nfsd/netlink.h
>> +++ b/fs/nfsd/netlink.h
>> @@ -12,10 +12,15 @@
>> #include <uapi/linux/nfsd_netlink.h>
>> 
>> int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
>> 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
>> +int nfsd_nl_server_status_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..c631b59b7a4f 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -1694,6 +1694,104 @@ 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)
>> +{
>> + u16 nthreads;
>> + int ret;
>> +
>> + if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
>> + return -EINVAL;
>> +
>> + nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
>> +
>> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
>> + return ret == nthreads ? 0 : ret;
>> +}
>> +
>> +/**
>> + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
>> + * @skb: reply buffer
>> + * @info: netlink metadata and command arguments
>> + *
>> + * Return 0 on success or a negative errno.
>> + */
>> +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
>> +{
>> +#ifdef CONFIG_NFSD_V4
>> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
>> +
>> + if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
>> + return -EINVAL;
>> +
>> + if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
>> + nfsd4_end_grace(nn);
>> +
> 
> To be clear here. Issuing this with anything but 0 will end the grace
> period. A value of 0 is ignored. It might be best to make the value not
> matter at all. Do we have to send down a value at all?
> 
>> + return 0;
>> +#else
>> + return -EOPNOTSUPP;
>> +#endif /* CONFIG_NFSD_V4 */
>> +}
>> +
>> +/**
>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
>> + * @cb: netlink metadata and command arguments
>> + *
>> + * Return values:
>> + *   %0: The server_status_get command may proceed
>> + *   %-ENODEV: There is no NFSD running in this namespace
>> + */
>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
>> +{
>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
>> +
>> + return nn->nfsd_serv ? 0 : -ENODEV;
>> +}
>> +
>> +/**
>> + * nfsd_nl_server_status_get_dumpit - dump server status info
>> + * @skb: reply buffer
>> + * @cb: netlink metadata and command arguments
>> + *
>> + * Returns the size of the reply or a negative errno.
>> + */
>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
>> +      struct netlink_callback *cb)
>> +{
>> + struct net *net = sock_net(skb->sk);
>> +#ifdef CONFIG_NFSD_V4
>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>> +#endif /* CONFIG_NFSD_V4 */
>> + 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,
>> +   NFSD_CMD_SERVER_STATUS_GET);
>> + if (!hdr)
>> + return -ENOBUFS;
>> +
>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
>> + return -ENOBUFS;
>> +#ifdef CONFIG_NFSD_V4
>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
>> + return -ENOBUFS;
>> +#endif /* CONFIG_NFSD_V4 */
>> +
>> + genlmsg_end(skb, hdr);
>> + cb->args[0] = 1;
>> +
>> + return skb->len;
>> +}
>> +
>> /**
>>  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
>> };
>> 
>> +enum {
>> + NFSD_A_SERVER_ATTR_THREADS = 1,
>> + NFSD_A_SERVER_ATTR_V4_GRACE,
>> +
>> + __NFSD_A_SERVER_ATTR_MAX,
>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
>> +};
>> +
>> enum {
>> NFSD_CMD_RPC_STATUS_GET = 1,
>> + NFSD_CMD_THREADS_SET,
>> + NFSD_CMD_V4_GRACE_RELEASE,
>> + NFSD_CMD_SERVER_STATUS_GET,
>> 
>> __NFSD_CMD_MAX,
>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> 
> -- 
> Jeff Layton <[e-mail]>


--
Chuck Lever

Lorenzo Bianconi Sept. 22, 2023, 4:20 p.m. UTC | #3

> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> > Introduce write_threads and write_v4_end_grace netlink commands similar
> > to the ones available through the procfs.
> > Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> > report global server metadata.
> > 
> > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > ---
> > This patch can be tested with user-space tool reported below:
> > https://github.com/LorenzoBianconi/nfsd-netlink.git
> > ---
> >  Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
> >  fs/nfsd/netlink.c                     | 30 ++++++++
> >  fs/nfsd/netlink.h                     |  5 ++
> >  fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
> >  include/uapi/linux/nfsd_netlink.h     | 11 +++
> >  5 files changed, 177 insertions(+)
> > 
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index 403d3e3a04f3..fa1204892703 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -62,6 +62,15 @@ attribute-sets:
> >          name: compound-ops
> >          type: u32
> >          multi-attr: true
> > +  -
> > +    name: server-attr
> > +    attributes:
> > +      -
> > +        name: threads
> > +        type: u16
> 
> 65k threads ought to be enough for anybody!

maybe u8 is fine here :)

> 
> > +      -
> > +        name: v4-grace
> > +        type: u8
> >  
> >  operations:
> >    list:
> > @@ -72,3 +81,27 @@ 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: server-attr
> > +      flags: [ admin-perm ]
> > +      do:
> > +        request:
> > +          attributes:
> > +            - threads
> > +    -
> > +      name: v4-grace-release
> > +      doc: release the grace period for nfsd's v4 lock manager
> > +      attribute-set: server-attr
> > +      flags: [ admin-perm ]
> > +      do:
> > +        request:
> > +          attributes:
> > +            - v4-grace
> > +    -
> > +      name: server-status-get
> > +      doc: dump server status info
> > +      attribute-set: server-attr
> > +      dump:
> > +        pre: nfsd-nl-server-status-get-start
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 0e1d635ec5f9..783a34e69354 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -10,6 +10,16 @@
> >  
> >  #include <uapi/linux/nfsd_netlink.h>
> >  
> > +/* NFSD_CMD_THREADS_SET - do */
> > +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
> > +	[NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
> > +};
> > +
> > +/* NFSD_CMD_V4_GRACE_RELEASE - do */
> > +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
> > +	[NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
> > +};
> > +
> >  /* Ops table for nfsd */
> >  static const struct genl_split_ops nfsd_nl_ops[] = {
> >  	{
> > @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
> > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > +	},
> > +	{
> > +		.cmd		= NFSD_CMD_V4_GRACE_RELEASE,
> > +		.doit		= nfsd_nl_v4_grace_release_doit,
> > +		.policy		= nfsd_v4_grace_release_nl_policy,
> > +		.maxattr	= NFSD_A_SERVER_ATTR_V4_GRACE,
> > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > +	},
> > +	{
> > +		.cmd	= NFSD_CMD_SERVER_STATUS_GET,
> > +		.start	= nfsd_nl_server_status_get_start,
> > +		.dumpit	= nfsd_nl_server_status_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..2e98061fbb0a 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -12,10 +12,15 @@
> >  #include <uapi/linux/nfsd_netlink.h>
> >  
> >  int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
> > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
> >  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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_server_status_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..c631b59b7a4f 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1694,6 +1694,104 @@ 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)
> > +{
> > +	u16 nthreads;
> > +	int ret;
> > +
> > +	if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
> > +		return -EINVAL;
> > +
> > +	nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
> > +
> > +	ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > +	return ret == nthreads ? 0 : ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +#ifdef CONFIG_NFSD_V4
> > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > +
> > +	if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
> > +		return -EINVAL;
> > +
> > +	if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
> > +		nfsd4_end_grace(nn);
> > +
> 
> To be clear here. Issuing this with anything but 0 will end the grace
> period. A value of 0 is ignored. It might be best to make the value not

I tried to be aligned with write_v4_end_grace() here but supporting just 1 (or
any other non-zero value) and skipping 'Y/y'. If we send 0 it should skip the
release action.

> matter at all. Do we have to send down a value at all?

I am not sure if ynl supports a doit operation with a request with no parameters.
@Chuck, Jakub: any input here?

Regards,
Lorenzo

> 
> > +	return 0;
> > +#else
> > +	return -EOPNOTSUPP;
> > +#endif /* CONFIG_NFSD_V4 */
> > +}
> > +
> > +/**
> > + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Return values:
> > + *   %0: The server_status_get command may proceed
> > + *   %-ENODEV: There is no NFSD running in this namespace
> > + */
> > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> > +{
> > +	struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> > +
> > +	return nn->nfsd_serv ? 0 : -ENODEV;
> > +}
> > +
> > +/**
> > + * nfsd_nl_server_status_get_dumpit - dump server status info
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> > +				     struct netlink_callback *cb)
> > +{
> > +	struct net *net = sock_net(skb->sk);
> > +#ifdef CONFIG_NFSD_V4
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +#endif /* CONFIG_NFSD_V4 */
> > +	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,
> > +			  NFSD_CMD_SERVER_STATUS_GET);
> > +	if (!hdr)
> > +		return -ENOBUFS;
> > +
> > +	if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> > +		return -ENOBUFS;
> > +#ifdef CONFIG_NFSD_V4
> > +	if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> > +		return -ENOBUFS;
> > +#endif /* CONFIG_NFSD_V4 */
> > +
> > +	genlmsg_end(skb, hdr);
> > +	cb->args[0] = 1;
> > +
> > +	return skb->len;
> > +}
> > +
> >  /**
> >   * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
> >  };
> >  
> > +enum {
> > +	NFSD_A_SERVER_ATTR_THREADS = 1,
> > +	NFSD_A_SERVER_ATTR_V4_GRACE,
> > +
> > +	__NFSD_A_SERVER_ATTR_MAX,
> > +	NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> > +};
> > +
> >  enum {
> >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > +	NFSD_CMD_THREADS_SET,
> > +	NFSD_CMD_V4_GRACE_RELEASE,
> > +	NFSD_CMD_SERVER_STATUS_GET,
> >  
> >  	__NFSD_CMD_MAX,
> >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> 
> -- 
> Jeff Layton <[e-mail]>
>

Jeff Layton Sept. 22, 2023, 4:21 p.m. UTC | #4

On Fri, 2023-09-22 at 16:06 +0000, Chuck Lever III wrote:
> 
> > On Sep 22, 2023, at 12:04 PM, Jeff Layton <[e-mail]> wrote:
> > 
> > On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> > > Introduce write_threads and write_v4_end_grace netlink commands similar
> > > to the ones available through the procfs.
> > > Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> > > report global server metadata.
> > > 
> > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > ---
> > > This patch can be tested with user-space tool reported below:
> > > https://github.com/LorenzoBianconi/nfsd-netlink.git
> > > ---
> > > Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
> > > fs/nfsd/netlink.c                     | 30 ++++++++
> > > fs/nfsd/netlink.h                     |  5 ++
> > > fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
> > > include/uapi/linux/nfsd_netlink.h     | 11 +++
> > > 5 files changed, 177 insertions(+)
> > > 
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index 403d3e3a04f3..fa1204892703 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -62,6 +62,15 @@ attribute-sets:
> > >         name: compound-ops
> > >         type: u32
> > >         multi-attr: true
> > > +  -
> > > +    name: server-attr
> > > +    attributes:
> > > +      -
> > > +        name: threads
> > > +        type: u16
> > 
> > 65k threads ought to be enough for anybody!
> 
> No argument.
> 
> But I thought you could echo a negative number of threads in /proc/fs/nfsd/threads
> to reduce the thread count. Maybe this field should be an s32?
> 

Yuck! I think I'd rather see this implemented as a declarative field.

Let's have this specify an explicit number of threads with 0 meaning
shutdown. If someone wants to reduce the number, they can do the math in
userland. That also jives better with the SERVICE_STATUS_GET...

> 
> > > +      -
> > > +        name: v4-grace
> > > +        type: u8
> > > 
> > > operations:
> > >   list:
> > > @@ -72,3 +81,27 @@ 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: server-attr
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - threads
> > > +    -
> > > +      name: v4-grace-release
> > > +      doc: release the grace period for nfsd's v4 lock manager
> > > +      attribute-set: server-attr
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - v4-grace
> > > +    -
> > > +      name: server-status-get
> > > +      doc: dump server status info
> > > +      attribute-set: server-attr
> > > +      dump:
> > > +        pre: nfsd-nl-server-status-get-start
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 0e1d635ec5f9..783a34e69354 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -10,6 +10,16 @@
> > > 
> > > #include <uapi/linux/nfsd_netlink.h>
> > > 
> > > +/* NFSD_CMD_THREADS_SET - do */
> > > +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
> > > + [NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
> > > +};
> > > +
> > > +/* NFSD_CMD_V4_GRACE_RELEASE - do */
> > > +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
> > > + [NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
> > > +};
> > > +
> > > /* Ops table for nfsd */
> > > static const struct genl_split_ops nfsd_nl_ops[] = {
> > > {
> > > @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
> > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > + },
> > > + {
> > > + .cmd = NFSD_CMD_V4_GRACE_RELEASE,
> > > + .doit = nfsd_nl_v4_grace_release_doit,
> > > + .policy = nfsd_v4_grace_release_nl_policy,
> > > + .maxattr = NFSD_A_SERVER_ATTR_V4_GRACE,
> > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > + },
> > > + {
> > > + .cmd = NFSD_CMD_SERVER_STATUS_GET,
> > > + .start = nfsd_nl_server_status_get_start,
> > > + .dumpit = nfsd_nl_server_status_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..2e98061fbb0a 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -12,10 +12,15 @@
> > > #include <uapi/linux/nfsd_netlink.h>
> > > 
> > > int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
> > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
> > > 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_server_status_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..c631b59b7a4f 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1694,6 +1694,104 @@ 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)
> > > +{
> > > + u16 nthreads;
> > > + int ret;
> > > +
> > > + if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
> > > + return -EINVAL;
> > > +
> > > + nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
> > > +
> > > + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > > + return ret == nthreads ? 0 : ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +#ifdef CONFIG_NFSD_V4
> > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > +
> > > + if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
> > > + return -EINVAL;
> > > +
> > > + if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
> > > + nfsd4_end_grace(nn);
> > > +
> > 
> > To be clear here. Issuing this with anything but 0 will end the grace
> > period. A value of 0 is ignored. It might be best to make the value not
> > matter at all. Do we have to send down a value at all?
> > 
> > > + return 0;
> > > +#else
> > > + return -EOPNOTSUPP;
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Return values:
> > > + *   %0: The server_status_get command may proceed
> > > + *   %-ENODEV: There is no NFSD running in this namespace
> > > + */
> > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> > > +{
> > > + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> > > +
> > > + return nn->nfsd_serv ? 0 : -ENODEV;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_server_status_get_dumpit - dump server status info
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> > > +      struct netlink_callback *cb)
> > > +{
> > > + struct net *net = sock_net(skb->sk);
> > > +#ifdef CONFIG_NFSD_V4
> > > + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > +#endif /* CONFIG_NFSD_V4 */
> > > + 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,
> > > +   NFSD_CMD_SERVER_STATUS_GET);
> > > + if (!hdr)
> > > + return -ENOBUFS;
> > > +
> > > + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> > > + return -ENOBUFS;
> > > +#ifdef CONFIG_NFSD_V4
> > > + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> > > + return -ENOBUFS;
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +
> > > + genlmsg_end(skb, hdr);
> > > + cb->args[0] = 1;
> > > +
> > > + return skb->len;
> > > +}
> > > +
> > > /**
> > >  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
> > > };
> > > 
> > > +enum {
> > > + NFSD_A_SERVER_ATTR_THREADS = 1,
> > > + NFSD_A_SERVER_ATTR_V4_GRACE,
> > > +
> > > + __NFSD_A_SERVER_ATTR_MAX,
> > > + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> > > +};
> > > +
> > > enum {
> > > NFSD_CMD_RPC_STATUS_GET = 1,
> > > + NFSD_CMD_THREADS_SET,
> > > + NFSD_CMD_V4_GRACE_RELEASE,
> > > + NFSD_CMD_SERVER_STATUS_GET,
> > > 
> > > __NFSD_CMD_MAX,
> > > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > 
> > -- 
> > Jeff Layton <[e-mail]>
> 
> 
> --
> Chuck Lever
> 
>

Lorenzo Bianconi Sept. 22, 2023, 4:23 p.m. UTC | #5

> On Fri, 2023-09-22 at 16:06 +0000, Chuck Lever III wrote:
> > 
> > > On Sep 22, 2023, at 12:04 PM, Jeff Layton <[e-mail]> wrote:
> > > 
> > > On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> > > > Introduce write_threads and write_v4_end_grace netlink commands similar
> > > > to the ones available through the procfs.
> > > > Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> > > > report global server metadata.
> > > > 
> > > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > > ---
> > > > This patch can be tested with user-space tool reported below:
> > > > https://github.com/LorenzoBianconi/nfsd-netlink.git
> > > > ---
> > > > Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
> > > > fs/nfsd/netlink.c                     | 30 ++++++++
> > > > fs/nfsd/netlink.h                     |  5 ++
> > > > fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
> > > > include/uapi/linux/nfsd_netlink.h     | 11 +++
> > > > 5 files changed, 177 insertions(+)
> > > > 
> > > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > > index 403d3e3a04f3..fa1204892703 100644
> > > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > > @@ -62,6 +62,15 @@ attribute-sets:
> > > >         name: compound-ops
> > > >         type: u32
> > > >         multi-attr: true
> > > > +  -
> > > > +    name: server-attr
> > > > +    attributes:
> > > > +      -
> > > > +        name: threads
> > > > +        type: u16
> > > 
> > > 65k threads ought to be enough for anybody!
> > 
> > No argument.
> > 
> > But I thought you could echo a negative number of threads in /proc/fs/nfsd/threads
> > to reduce the thread count. Maybe this field should be an s32?
> > 
> 
> Yuck! I think I'd rather see this implemented as a declarative field.
> 
> Let's have this specify an explicit number of threads with 0 meaning
> shutdown. If someone wants to reduce the number, they can do the math in
> userland. That also jives better with the SERVICE_STATUS_GET...

ack, I agree.

Regards,
Lorenzo

> 
> > 
> > > > +      -
> > > > +        name: v4-grace
> > > > +        type: u8
> > > > 
> > > > operations:
> > > >   list:
> > > > @@ -72,3 +81,27 @@ 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: server-attr
> > > > +      flags: [ admin-perm ]
> > > > +      do:
> > > > +        request:
> > > > +          attributes:
> > > > +            - threads
> > > > +    -
> > > > +      name: v4-grace-release
> > > > +      doc: release the grace period for nfsd's v4 lock manager
> > > > +      attribute-set: server-attr
> > > > +      flags: [ admin-perm ]
> > > > +      do:
> > > > +        request:
> > > > +          attributes:
> > > > +            - v4-grace
> > > > +    -
> > > > +      name: server-status-get
> > > > +      doc: dump server status info
> > > > +      attribute-set: server-attr
> > > > +      dump:
> > > > +        pre: nfsd-nl-server-status-get-start
> > > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > > index 0e1d635ec5f9..783a34e69354 100644
> > > > --- a/fs/nfsd/netlink.c
> > > > +++ b/fs/nfsd/netlink.c
> > > > @@ -10,6 +10,16 @@
> > > > 
> > > > #include <uapi/linux/nfsd_netlink.h>
> > > > 
> > > > +/* NFSD_CMD_THREADS_SET - do */
> > > > +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
> > > > + [NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
> > > > +};
> > > > +
> > > > +/* NFSD_CMD_V4_GRACE_RELEASE - do */
> > > > +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
> > > > + [NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
> > > > +};
> > > > +
> > > > /* Ops table for nfsd */
> > > > static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > {
> > > > @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
> > > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > + },
> > > > + {
> > > > + .cmd = NFSD_CMD_V4_GRACE_RELEASE,
> > > > + .doit = nfsd_nl_v4_grace_release_doit,
> > > > + .policy = nfsd_v4_grace_release_nl_policy,
> > > > + .maxattr = NFSD_A_SERVER_ATTR_V4_GRACE,
> > > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > + },
> > > > + {
> > > > + .cmd = NFSD_CMD_SERVER_STATUS_GET,
> > > > + .start = nfsd_nl_server_status_get_start,
> > > > + .dumpit = nfsd_nl_server_status_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..2e98061fbb0a 100644
> > > > --- a/fs/nfsd/netlink.h
> > > > +++ b/fs/nfsd/netlink.h
> > > > @@ -12,10 +12,15 @@
> > > > #include <uapi/linux/nfsd_netlink.h>
> > > > 
> > > > int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
> > > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
> > > > 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
> > > > +int nfsd_nl_server_status_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..c631b59b7a4f 100644
> > > > --- a/fs/nfsd/nfsctl.c
> > > > +++ b/fs/nfsd/nfsctl.c
> > > > @@ -1694,6 +1694,104 @@ 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)
> > > > +{
> > > > + u16 nthreads;
> > > > + int ret;
> > > > +
> > > > + if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
> > > > + return -EINVAL;
> > > > +
> > > > + nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
> > > > +
> > > > + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > > > + return ret == nthreads ? 0 : ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
> > > > + * @skb: reply buffer
> > > > + * @info: netlink metadata and command arguments
> > > > + *
> > > > + * Return 0 on success or a negative errno.
> > > > + */
> > > > +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
> > > > +{
> > > > +#ifdef CONFIG_NFSD_V4
> > > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > > +
> > > > + if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
> > > > + return -EINVAL;
> > > > +
> > > > + if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
> > > > + nfsd4_end_grace(nn);
> > > > +
> > > 
> > > To be clear here. Issuing this with anything but 0 will end the grace
> > > period. A value of 0 is ignored. It might be best to make the value not
> > > matter at all. Do we have to send down a value at all?
> > > 
> > > > + return 0;
> > > > +#else
> > > > + return -EOPNOTSUPP;
> > > > +#endif /* CONFIG_NFSD_V4 */
> > > > +}
> > > > +
> > > > +/**
> > > > + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> > > > + * @cb: netlink metadata and command arguments
> > > > + *
> > > > + * Return values:
> > > > + *   %0: The server_status_get command may proceed
> > > > + *   %-ENODEV: There is no NFSD running in this namespace
> > > > + */
> > > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> > > > +{
> > > > + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> > > > +
> > > > + return nn->nfsd_serv ? 0 : -ENODEV;
> > > > +}
> > > > +
> > > > +/**
> > > > + * nfsd_nl_server_status_get_dumpit - dump server status info
> > > > + * @skb: reply buffer
> > > > + * @cb: netlink metadata and command arguments
> > > > + *
> > > > + * Returns the size of the reply or a negative errno.
> > > > + */
> > > > +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> > > > +      struct netlink_callback *cb)
> > > > +{
> > > > + struct net *net = sock_net(skb->sk);
> > > > +#ifdef CONFIG_NFSD_V4
> > > > + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > > +#endif /* CONFIG_NFSD_V4 */
> > > > + 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,
> > > > +   NFSD_CMD_SERVER_STATUS_GET);
> > > > + if (!hdr)
> > > > + return -ENOBUFS;
> > > > +
> > > > + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> > > > + return -ENOBUFS;
> > > > +#ifdef CONFIG_NFSD_V4
> > > > + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> > > > + return -ENOBUFS;
> > > > +#endif /* CONFIG_NFSD_V4 */
> > > > +
> > > > + genlmsg_end(skb, hdr);
> > > > + cb->args[0] = 1;
> > > > +
> > > > + return skb->len;
> > > > +}
> > > > +
> > > > /**
> > > >  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
> > > > };
> > > > 
> > > > +enum {
> > > > + NFSD_A_SERVER_ATTR_THREADS = 1,
> > > > + NFSD_A_SERVER_ATTR_V4_GRACE,
> > > > +
> > > > + __NFSD_A_SERVER_ATTR_MAX,
> > > > + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> > > > +};
> > > > +
> > > > enum {
> > > > NFSD_CMD_RPC_STATUS_GET = 1,
> > > > + NFSD_CMD_THREADS_SET,
> > > > + NFSD_CMD_V4_GRACE_RELEASE,
> > > > + NFSD_CMD_SERVER_STATUS_GET,
> > > > 
> > > > __NFSD_CMD_MAX,
> > > > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > 
> > > -- 
> > > Jeff Layton <[e-mail]>
> > 
> > 
> > --
> > Chuck Lever
> > 
> > 
> 
> -- 
> Jeff Layton <[e-mail]>
>

Jeff Layton Sept. 22, 2023, 4:53 p.m. UTC | #6

On Fri, 2023-09-22 at 18:20 +0200, Lorenzo Bianconi wrote:
> > On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> > > Introduce write_threads and write_v4_end_grace netlink commands similar
> > > to the ones available through the procfs.
> > > Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> > > report global server metadata.
> > > 
> > > Signed-off-by: Lorenzo Bianconi <[e-mail]>
> > > ---
> > > This patch can be tested with user-space tool reported below:
> > > https://github.com/LorenzoBianconi/nfsd-netlink.git
> > > ---
> > >  Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
> > >  fs/nfsd/netlink.c                     | 30 ++++++++
> > >  fs/nfsd/netlink.h                     |  5 ++
> > >  fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
> > >  include/uapi/linux/nfsd_netlink.h     | 11 +++
> > >  5 files changed, 177 insertions(+)
> > > 
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index 403d3e3a04f3..fa1204892703 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -62,6 +62,15 @@ attribute-sets:
> > >          name: compound-ops
> > >          type: u32
> > >          multi-attr: true
> > > +  -
> > > +    name: server-attr
> > > +    attributes:
> > > +      -
> > > +        name: threads
> > > +        type: u16
> > 
> > 65k threads ought to be enough for anybody!
> 
> maybe u8 is fine here :)
> 

No, I was just kidding. u16 is fine. We should allow for large-scale
machines. Heck, we might even want to just make it u32. Who knows how
many threads we'll be needing in 10 years?

> > 
> > > +      -
> > > +        name: v4-grace
> > > +        type: u8
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  operations:
> > >    list:
> > > @@ -72,3 +81,27 @@ 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: server-attr
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - threads
> > > +    -
> > > +      name: v4-grace-release
> > > +      doc: release the grace period for nfsd's v4 lock manager
> > > +      attribute-set: server-attr
> > > +      flags: [ admin-perm ]
> > > +      do:
> > > +        request:
> > > +          attributes:
> > > +            - v4-grace
> > > +    -
> > > +      name: server-status-get
> > > +      doc: dump server status info
> > > +      attribute-set: server-attr
> > > +      dump:
> > > +        pre: nfsd-nl-server-status-get-start
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 0e1d635ec5f9..783a34e69354 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -10,6 +10,16 @@
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  #include <uapi/linux/nfsd_netlink.h>
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > +/* NFSD_CMD_THREADS_SET - do */
> > > +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
> > > +	[NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
> > > +};
> > > +
> > > +/* NFSD_CMD_V4_GRACE_RELEASE - do */
> > > +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
> > > +	[NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
> > > +};
> > > +
> > >  /* Ops table for nfsd */
> > >  static const struct genl_split_ops nfsd_nl_ops[] = {
> > >  	{
> > > @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
> > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > +	},
> > > +	{
> > > +		.cmd		= NFSD_CMD_V4_GRACE_RELEASE,
> > > +		.doit		= nfsd_nl_v4_grace_release_doit,
> > > +		.policy		= nfsd_v4_grace_release_nl_policy,
> > > +		.maxattr	= NFSD_A_SERVER_ATTR_V4_GRACE,
> > > +		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > +	},
> > > +	{
> > > +		.cmd	= NFSD_CMD_SERVER_STATUS_GET,
> > > +		.start	= nfsd_nl_server_status_get_start,
> > > +		.dumpit	= nfsd_nl_server_status_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..2e98061fbb0a 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -12,10 +12,15 @@
> > >  #include <uapi/linux/nfsd_netlink.h>
> > >  
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > >  int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
> > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
> > >  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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_server_status_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..c631b59b7a4f 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1694,6 +1694,104 @@ 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)
> > > +{
> > > +	u16 nthreads;
> > > +	int ret;
> > > +
> > > +	if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
> > > +		return -EINVAL;
> > > +
> > > +	nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
> > > +
> > > +	ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > > +	return ret == nthreads ? 0 : ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > +#ifdef CONFIG_NFSD_V4
> > > +	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > +
> > > +	if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
> > > +		return -EINVAL;
> > > +
> > > +	if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
> > > +		nfsd4_end_grace(nn);
> > > +
> > 
> > To be clear here. Issuing this with anything but 0 will end the grace
> > period. A value of 0 is ignored. It might be best to make the value not
> 
> I tried to be aligned with write_v4_end_grace() here but supporting just 1 (or
> any other non-zero value) and skipping 'Y/y'. If we send 0 it should skip the
> release action.
> 
> > matter at all. Do we have to send down a value at all?
> 
> I am not sure if ynl supports a doit operation with a request with no parameters.
> @Chuck, Jakub: any input here?
> 
> 
That was the way the original interface worked, but it has turned out to
be somewhat klunky. In fact, the whole nfsdcld upcall scheme is pretty
klunky altogether. It could really use a rework such that it doesn't
require so much upcalling, and it would be _really_ nice to ditch
rpc_pipefs.

I think it might be best not to add this field to the interface just
yet. I think we might want to consider reworking nfsdcld with an upcall
scheme based around netlink instead of rpc.pipefs.

What might be good to add to this interface in the nearer term is stuff
like the listener port configuration, the versions being served, etc.
IOW, I think with this patch, you want to aim to have rpc.nfsd(8) talk
to the kernel via netlink instead of nfsdfs.

> > 
> > > +	return 0;
> > > +#else
> > > +	return -EOPNOTSUPP;
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Return values:
> > > + *   %0: The server_status_get command may proceed
> > > + *   %-ENODEV: There is no NFSD running in this namespace
> > > + */
> > > +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> > > +{
> > > +	struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> > > +
> > > +	return nn->nfsd_serv ? 0 : -ENODEV;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_server_status_get_dumpit - dump server status info
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> > > +				     struct netlink_callback *cb)
> > > +{
> > > +	struct net *net = sock_net(skb->sk);
> > > +#ifdef CONFIG_NFSD_V4
> > > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +	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,
> > > +			  NFSD_CMD_SERVER_STATUS_GET);
> > > +	if (!hdr)
> > > +		return -ENOBUFS;
> > > +
> > > +	if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> > > +		return -ENOBUFS;
> > > +#ifdef CONFIG_NFSD_V4
> > > +	if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> > > +		return -ENOBUFS;
> > > +#endif /* CONFIG_NFSD_V4 */
> > > +
> > > +	genlmsg_end(skb, hdr);
> > > +	cb->args[0] = 1;
> > > +
> > > +	return skb->len;
> > > +}
> > > +
> > >  /**
> > >   * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
> > >  };
> > >  
> > > 
> > > 
> > > 
> > > +enum {
> > > +	NFSD_A_SERVER_ATTR_THREADS = 1,
> > > +	NFSD_A_SERVER_ATTR_V4_GRACE,
> > > +
> > > +	__NFSD_A_SERVER_ATTR_MAX,
> > > +	NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> > > +};
> > > +
> > >  enum {
> > >  	NFSD_CMD_RPC_STATUS_GET = 1,
> > > +	NFSD_CMD_THREADS_SET,
> > > +	NFSD_CMD_V4_GRACE_RELEASE,
> > > +	NFSD_CMD_SERVER_STATUS_GET,
> > >  
> > > 
> > > 
> > > 
> > >  	__NFSD_CMD_MAX,
> > >  	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > 
> > -- 
> > Jeff Layton <[e-mail]>
> >

Chuck Lever Sept. 22, 2023, 5:23 p.m. UTC | #7

> On Sep 22, 2023, at 12:23 PM, Lorenzo Bianconi <[e-mail]> wrote:
> 
>> On Fri, 2023-09-22 at 16:06 +0000, Chuck Lever III wrote:
>>> 
>>>> On Sep 22, 2023, at 12:04 PM, Jeff Layton <[e-mail]> wrote:
>>>> 
>>>> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
>>>>> Introduce write_threads and write_v4_end_grace netlink commands similar
>>>>> to the ones available through the procfs.
>>>>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
>>>>> report global server metadata.
>>>>> 
>>>>> Signed-off-by: Lorenzo Bianconi <[e-mail]>
>>>>> ---
>>>>> This patch can be tested with user-space tool reported below:
>>>>> https://github.com/LorenzoBianconi/nfsd-netlink.git
>>>>> ---
>>>>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>>>>> fs/nfsd/netlink.c                     | 30 ++++++++
>>>>> fs/nfsd/netlink.h                     |  5 ++
>>>>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>>>>> include/uapi/linux/nfsd_netlink.h     | 11 +++
>>>>> 5 files changed, 177 insertions(+)
>>>>> 
>>>>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
>>>>> index 403d3e3a04f3..fa1204892703 100644
>>>>> --- a/Documentation/netlink/specs/nfsd.yaml
>>>>> +++ b/Documentation/netlink/specs/nfsd.yaml
>>>>> @@ -62,6 +62,15 @@ attribute-sets:
>>>>>        name: compound-ops
>>>>>        type: u32
>>>>>        multi-attr: true
>>>>> +  -
>>>>> +    name: server-attr
>>>>> +    attributes:
>>>>> +      -
>>>>> +        name: threads
>>>>> +        type: u16
>>>> 
>>>> 65k threads ought to be enough for anybody!
>>> 
>>> No argument.
>>> 
>>> But I thought you could echo a negative number of threads in /proc/fs/nfsd/threads
>>> to reduce the thread count. Maybe this field should be an s32?
>>> 
>> 
>> Yuck! I think I'd rather see this implemented as a declarative field.
>> 
>> Let's have this specify an explicit number of threads with 0 meaning
>> shutdown. If someone wants to reduce the number, they can do the math in
>> userland. That also jives better with the SERVICE_STATUS_GET...
> 
> ack, I agree.

The positive / negative value is actually nailed into some of the
thread infrastructure. Changing that might be more of schlep.

I'm curious how Neil feels about this.


> Regards,
> Lorenzo
> 
>> 
>>> 
>>>>> +      -
>>>>> +        name: v4-grace
>>>>> +        type: u8
>>>>> 
>>>>> operations:
>>>>>  list:
>>>>> @@ -72,3 +81,27 @@ 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: server-attr
>>>>> +      flags: [ admin-perm ]
>>>>> +      do:
>>>>> +        request:
>>>>> +          attributes:
>>>>> +            - threads
>>>>> +    -
>>>>> +      name: v4-grace-release
>>>>> +      doc: release the grace period for nfsd's v4 lock manager
>>>>> +      attribute-set: server-attr
>>>>> +      flags: [ admin-perm ]
>>>>> +      do:
>>>>> +        request:
>>>>> +          attributes:
>>>>> +            - v4-grace
>>>>> +    -
>>>>> +      name: server-status-get
>>>>> +      doc: dump server status info
>>>>> +      attribute-set: server-attr
>>>>> +      dump:
>>>>> +        pre: nfsd-nl-server-status-get-start
>>>>> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
>>>>> index 0e1d635ec5f9..783a34e69354 100644
>>>>> --- a/fs/nfsd/netlink.c
>>>>> +++ b/fs/nfsd/netlink.c
>>>>> @@ -10,6 +10,16 @@
>>>>> 
>>>>> #include <uapi/linux/nfsd_netlink.h>
>>>>> 
>>>>> +/* NFSD_CMD_THREADS_SET - do */
>>>>> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
>>>>> + [NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
>>>>> +};
>>>>> +
>>>>> +/* NFSD_CMD_V4_GRACE_RELEASE - do */
>>>>> +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
>>>>> + [NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
>>>>> +};
>>>>> +
>>>>> /* Ops table for nfsd */
>>>>> static const struct genl_split_ops nfsd_nl_ops[] = {
>>>>> {
>>>>> @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
>>>>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>>> + },
>>>>> + {
>>>>> + .cmd = NFSD_CMD_V4_GRACE_RELEASE,
>>>>> + .doit = nfsd_nl_v4_grace_release_doit,
>>>>> + .policy = nfsd_v4_grace_release_nl_policy,
>>>>> + .maxattr = NFSD_A_SERVER_ATTR_V4_GRACE,
>>>>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>>>> + },
>>>>> + {
>>>>> + .cmd = NFSD_CMD_SERVER_STATUS_GET,
>>>>> + .start = nfsd_nl_server_status_get_start,
>>>>> + .dumpit = nfsd_nl_server_status_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..2e98061fbb0a 100644
>>>>> --- a/fs/nfsd/netlink.h
>>>>> +++ b/fs/nfsd/netlink.h
>>>>> @@ -12,10 +12,15 @@
>>>>> #include <uapi/linux/nfsd_netlink.h>
>>>>> 
>>>>> int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
>>>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
>>>>> 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
>>>>> +int nfsd_nl_server_status_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..c631b59b7a4f 100644
>>>>> --- a/fs/nfsd/nfsctl.c
>>>>> +++ b/fs/nfsd/nfsctl.c
>>>>> @@ -1694,6 +1694,104 @@ 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)
>>>>> +{
>>>>> + u16 nthreads;
>>>>> + int ret;
>>>>> +
>>>>> + if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
>>>>> + return -EINVAL;
>>>>> +
>>>>> + nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
>>>>> +
>>>>> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
>>>>> + return ret == nthreads ? 0 : ret;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
>>>>> + * @skb: reply buffer
>>>>> + * @info: netlink metadata and command arguments
>>>>> + *
>>>>> + * Return 0 on success or a negative errno.
>>>>> + */
>>>>> +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
>>>>> +{
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
>>>>> +
>>>>> + if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
>>>>> + nfsd4_end_grace(nn);
>>>>> +
>>>> 
>>>> To be clear here. Issuing this with anything but 0 will end the grace
>>>> period. A value of 0 is ignored. It might be best to make the value not
>>>> matter at all. Do we have to send down a value at all?
>>>> 
>>>>> + return 0;
>>>>> +#else
>>>>> + return -EOPNOTSUPP;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Return values:
>>>>> + *   %0: The server_status_get command may proceed
>>>>> + *   %-ENODEV: There is no NFSD running in this namespace
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
>>>>> +{
>>>>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
>>>>> +
>>>>> + return nn->nfsd_serv ? 0 : -ENODEV;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_dumpit - dump server status info
>>>>> + * @skb: reply buffer
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Returns the size of the reply or a negative errno.
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
>>>>> +      struct netlink_callback *cb)
>>>>> +{
>>>>> + struct net *net = sock_net(skb->sk);
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> + 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,
>>>>> +   NFSD_CMD_SERVER_STATUS_GET);
>>>>> + if (!hdr)
>>>>> + return -ENOBUFS;
>>>>> +
>>>>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
>>>>> + return -ENOBUFS;
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
>>>>> + return -ENOBUFS;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +
>>>>> + genlmsg_end(skb, hdr);
>>>>> + cb->args[0] = 1;
>>>>> +
>>>>> + return skb->len;
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
>>>>> };
>>>>> 
>>>>> +enum {
>>>>> + NFSD_A_SERVER_ATTR_THREADS = 1,
>>>>> + NFSD_A_SERVER_ATTR_V4_GRACE,
>>>>> +
>>>>> + __NFSD_A_SERVER_ATTR_MAX,
>>>>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
>>>>> +};
>>>>> +
>>>>> enum {
>>>>> NFSD_CMD_RPC_STATUS_GET = 1,
>>>>> + NFSD_CMD_THREADS_SET,
>>>>> + NFSD_CMD_V4_GRACE_RELEASE,
>>>>> + NFSD_CMD_SERVER_STATUS_GET,
>>>>> 
>>>>> __NFSD_CMD_MAX,
>>>>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
>>>> 
>>>> -- 
>>>> Jeff Layton <[e-mail]>
>>> 
>>> 
>>> --
>>> Chuck Lever
>>> 
>>> 
>> 
>> -- 
>> Jeff Layton <[e-mail]>
>> 

--
Chuck Lever

Chuck Lever Sept. 22, 2023, 5:25 p.m. UTC | #8

> On Sep 22, 2023, at 12:20 PM, Lorenzo Bianconi <[e-mail]> wrote:
> 
>> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
>>> Introduce write_threads and write_v4_end_grace netlink commands similar
>>> to the ones available through the procfs.
>>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
>>> report global server metadata.
>>> 
>>> Signed-off-by: Lorenzo Bianconi <[e-mail]>
>>> ---
>>> This patch can be tested with user-space tool reported below:
>>> https://github.com/LorenzoBianconi/nfsd-netlink.git
>>> ---
>>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>>> fs/nfsd/netlink.c                     | 30 ++++++++
>>> fs/nfsd/netlink.h                     |  5 ++
>>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>>> include/uapi/linux/nfsd_netlink.h     | 11 +++
>>> 5 files changed, 177 insertions(+)
>>> 
>>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
>>> index 403d3e3a04f3..fa1204892703 100644
>>> --- a/Documentation/netlink/specs/nfsd.yaml
>>> +++ b/Documentation/netlink/specs/nfsd.yaml
>>> @@ -62,6 +62,15 @@ attribute-sets:
>>>         name: compound-ops
>>>         type: u32
>>>         multi-attr: true
>>> +  -
>>> +    name: server-attr
>>> +    attributes:
>>> +      -
>>> +        name: threads
>>> +        type: u16
>> 
>> 65k threads ought to be enough for anybody!
> 
> maybe u8 is fine here :)

32-bit is the usual for this kind of interface. I don't think we need to go with 16-bit.


>>> +      -
>>> +        name: v4-grace
>>> +        type: u8
>>> 
>>> operations:
>>>   list:
>>> @@ -72,3 +81,27 @@ 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: server-attr
>>> +      flags: [ admin-perm ]
>>> +      do:
>>> +        request:
>>> +          attributes:
>>> +            - threads
>>> +    -
>>> +      name: v4-grace-release
>>> +      doc: release the grace period for nfsd's v4 lock manager
>>> +      attribute-set: server-attr
>>> +      flags: [ admin-perm ]
>>> +      do:
>>> +        request:
>>> +          attributes:
>>> +            - v4-grace
>>> +    -
>>> +      name: server-status-get
>>> +      doc: dump server status info
>>> +      attribute-set: server-attr
>>> +      dump:
>>> +        pre: nfsd-nl-server-status-get-start
>>> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
>>> index 0e1d635ec5f9..783a34e69354 100644
>>> --- a/fs/nfsd/netlink.c
>>> +++ b/fs/nfsd/netlink.c
>>> @@ -10,6 +10,16 @@
>>> 
>>> #include <uapi/linux/nfsd_netlink.h>
>>> 
>>> +/* NFSD_CMD_THREADS_SET - do */
>>> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
>>> + [NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
>>> +};
>>> +
>>> +/* NFSD_CMD_V4_GRACE_RELEASE - do */
>>> +static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
>>> + [NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
>>> +};
>>> +
>>> /* Ops table for nfsd */
>>> static const struct genl_split_ops nfsd_nl_ops[] = {
>>> {
>>> @@ -19,6 +29,26 @@ 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_SERVER_ATTR_THREADS,
>>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>> + },
>>> + {
>>> + .cmd = NFSD_CMD_V4_GRACE_RELEASE,
>>> + .doit = nfsd_nl_v4_grace_release_doit,
>>> + .policy = nfsd_v4_grace_release_nl_policy,
>>> + .maxattr = NFSD_A_SERVER_ATTR_V4_GRACE,
>>> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
>>> + },
>>> + {
>>> + .cmd = NFSD_CMD_SERVER_STATUS_GET,
>>> + .start = nfsd_nl_server_status_get_start,
>>> + .dumpit = nfsd_nl_server_status_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..2e98061fbb0a 100644
>>> --- a/fs/nfsd/netlink.h
>>> +++ b/fs/nfsd/netlink.h
>>> @@ -12,10 +12,15 @@
>>> #include <uapi/linux/nfsd_netlink.h>
>>> 
>>> int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
>>> 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
>>> +int nfsd_nl_server_status_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..c631b59b7a4f 100644
>>> --- a/fs/nfsd/nfsctl.c
>>> +++ b/fs/nfsd/nfsctl.c
>>> @@ -1694,6 +1694,104 @@ 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)
>>> +{
>>> + u16 nthreads;
>>> + int ret;
>>> +
>>> + if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
>>> + return -EINVAL;
>>> +
>>> + nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
>>> +
>>> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
>>> + return ret == nthreads ? 0 : ret;
>>> +}
>>> +
>>> +/**
>>> + * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
>>> + * @skb: reply buffer
>>> + * @info: netlink metadata and command arguments
>>> + *
>>> + * Return 0 on success or a negative errno.
>>> + */
>>> +int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
>>> +{
>>> +#ifdef CONFIG_NFSD_V4
>>> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
>>> +
>>> + if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
>>> + return -EINVAL;
>>> +
>>> + if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
>>> + nfsd4_end_grace(nn);
>>> +
>> 
>> To be clear here. Issuing this with anything but 0 will end the grace
>> period. A value of 0 is ignored. It might be best to make the value not
> 
> I tried to be aligned with write_v4_end_grace() here but supporting just 1 (or
> any other non-zero value) and skipping 'Y/y'. If we send 0 it should skip the
> release action.
> 
>> matter at all. Do we have to send down a value at all?
> 
> I am not sure if ynl supports a doit operation with a request with no parameters.
> @Chuck, Jakub: any input here?

I think it does, I might have done something like that for one of the
handshake protocol commands.

But I think Jeff's right, end_grace might be better postponed. Pick any of
the others that you think might be easy to implement instead.


> Regards,
> Lorenzo
> 
>> 
>>> + return 0;
>>> +#else
>>> + return -EOPNOTSUPP;
>>> +#endif /* CONFIG_NFSD_V4 */
>>> +}
>>> +
>>> +/**
>>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
>>> + * @cb: netlink metadata and command arguments
>>> + *
>>> + * Return values:
>>> + *   %0: The server_status_get command may proceed
>>> + *   %-ENODEV: There is no NFSD running in this namespace
>>> + */
>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
>>> +{
>>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
>>> +
>>> + return nn->nfsd_serv ? 0 : -ENODEV;
>>> +}
>>> +
>>> +/**
>>> + * nfsd_nl_server_status_get_dumpit - dump server status info
>>> + * @skb: reply buffer
>>> + * @cb: netlink metadata and command arguments
>>> + *
>>> + * Returns the size of the reply or a negative errno.
>>> + */
>>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
>>> +     struct netlink_callback *cb)
>>> +{
>>> + struct net *net = sock_net(skb->sk);
>>> +#ifdef CONFIG_NFSD_V4
>>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> +#endif /* CONFIG_NFSD_V4 */
>>> + 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,
>>> +  NFSD_CMD_SERVER_STATUS_GET);
>>> + if (!hdr)
>>> + return -ENOBUFS;
>>> +
>>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
>>> + return -ENOBUFS;
>>> +#ifdef CONFIG_NFSD_V4
>>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
>>> + return -ENOBUFS;
>>> +#endif /* CONFIG_NFSD_V4 */
>>> +
>>> + genlmsg_end(skb, hdr);
>>> + cb->args[0] = 1;
>>> +
>>> + return skb->len;
>>> +}
>>> +
>>> /**
>>>  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
>>> };
>>> 
>>> +enum {
>>> + NFSD_A_SERVER_ATTR_THREADS = 1,
>>> + NFSD_A_SERVER_ATTR_V4_GRACE,
>>> +
>>> + __NFSD_A_SERVER_ATTR_MAX,
>>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
>>> +};
>>> +
>>> enum {
>>> NFSD_CMD_RPC_STATUS_GET = 1,
>>> + NFSD_CMD_THREADS_SET,
>>> + NFSD_CMD_V4_GRACE_RELEASE,
>>> + NFSD_CMD_SERVER_STATUS_GET,
>>> 
>>> __NFSD_CMD_MAX,
>>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
>> 
>> -- 
>> Jeff Layton <[e-mail]>
>> 

--
Chuck Lever

Lorenzo Bianconi Sept. 22, 2023, 7:25 p.m. UTC | #9

> 
> 
> > On Sep 22, 2023, at 12:20 PM, Lorenzo Bianconi <[e-mail]> wrote:
> > 
> >> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
> >>> Introduce write_threads and write_v4_end_grace netlink commands similar
> >>> to the ones available through the procfs.
> >>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
> >>> report global server metadata.
> >>> 
> >>> Signed-off-by: Lorenzo Bianconi <[e-mail]>
> >>> ---
> >>> This patch can be tested with user-space tool reported below:
> >>> https://github.com/LorenzoBianconi/nfsd-netlink.git
> >>> ---
> >>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
> >>> fs/nfsd/netlink.c                     | 30 ++++++++
> >>> fs/nfsd/netlink.h                     |  5 ++
> >>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
> >>> include/uapi/linux/nfsd_netlink.h     | 11 +++
> >>> 5 files changed, 177 insertions(+)
> >>> 
> >>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> >>> index 403d3e3a04f3..fa1204892703 100644
> >>> --- a/Documentation/netlink/specs/nfsd.yaml
> >>> +++ b/Documentation/netlink/specs/nfsd.yaml
> >>> @@ -62,6 +62,15 @@ attribute-sets:
> >>>         name: compound-ops
> >>>         type: u32
> >>>         multi-attr: true
> >>> +  -
> >>> +    name: server-attr
> >>> +    attributes:
> >>> +      -
> >>> +        name: threads
> >>> +        type: u16
> >> 
> >> 65k threads ought to be enough for anybody!
> > 
> > maybe u8 is fine here :)
> 
> 32-bit is the usual for this kind of interface. I don't think we need to go with 16-bit.

ack, fine

> 
> 
> >>> +      -
> >>> +        name: v4-grace
> >>> +        type: u8
> >>> 
> >>> operations:
> >>>   list:
> >>> @@ -72,3 +81,27 @@ 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: server-attr
> >>> +      flags: [ admin-perm ]

[...]

> > 
> > I am not sure if ynl supports a doit operation with a request with no parameters.
> > @Chuck, Jakub: any input here?
> 
> I think it does, I might have done something like that for one of the
> handshake protocol commands.

please correct me if I am wrong but in Documentation/netlink/specs/handshake.yaml
we have accept and done operations and both of them have some parameters in the
request field, right?

> 
> But I think Jeff's right, end_grace might be better postponed. Pick any of
> the others that you think might be easy to implement instead.

ack, fine. Do you agree to have a global container (server-status-get) for all
the server metadata instead of adding dedicated get APIs?

Regards,
Lorenzo

> 
> 
> > Regards,
> > Lorenzo
> > 
> >> 
> >>> + return 0;
> >>> +#else
> >>> + return -EOPNOTSUPP;
> >>> +#endif /* CONFIG_NFSD_V4 */
> >>> +}
> >>> +
> >>> +/**
> >>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
> >>> + * @cb: netlink metadata and command arguments
> >>> + *
> >>> + * Return values:
> >>> + *   %0: The server_status_get command may proceed
> >>> + *   %-ENODEV: There is no NFSD running in this namespace
> >>> + */
> >>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
> >>> +{
> >>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> >>> +
> >>> + return nn->nfsd_serv ? 0 : -ENODEV;
> >>> +}
> >>> +
> >>> +/**
> >>> + * nfsd_nl_server_status_get_dumpit - dump server status info
> >>> + * @skb: reply buffer
> >>> + * @cb: netlink metadata and command arguments
> >>> + *
> >>> + * Returns the size of the reply or a negative errno.
> >>> + */
> >>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
> >>> +     struct netlink_callback *cb)
> >>> +{
> >>> + struct net *net = sock_net(skb->sk);
> >>> +#ifdef CONFIG_NFSD_V4
> >>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> >>> +#endif /* CONFIG_NFSD_V4 */
> >>> + 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,
> >>> +  NFSD_CMD_SERVER_STATUS_GET);
> >>> + if (!hdr)
> >>> + return -ENOBUFS;
> >>> +
> >>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
> >>> + return -ENOBUFS;
> >>> +#ifdef CONFIG_NFSD_V4
> >>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
> >>> + return -ENOBUFS;
> >>> +#endif /* CONFIG_NFSD_V4 */
> >>> +
> >>> + genlmsg_end(skb, hdr);
> >>> + cb->args[0] = 1;
> >>> +
> >>> + return skb->len;
> >>> +}
> >>> +
> >>> /**
> >>>  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
> >>> };
> >>> 
> >>> +enum {
> >>> + NFSD_A_SERVER_ATTR_THREADS = 1,
> >>> + NFSD_A_SERVER_ATTR_V4_GRACE,
> >>> +
> >>> + __NFSD_A_SERVER_ATTR_MAX,
> >>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
> >>> +};
> >>> +
> >>> enum {
> >>> NFSD_CMD_RPC_STATUS_GET = 1,
> >>> + NFSD_CMD_THREADS_SET,
> >>> + NFSD_CMD_V4_GRACE_RELEASE,
> >>> + NFSD_CMD_SERVER_STATUS_GET,
> >>> 
> >>> __NFSD_CMD_MAX,
> >>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> >> 
> >> -- 
> >> Jeff Layton <[e-mail]>
> >> 
> 
> --
> Chuck Lever
> 
>

Chuck Lever Sept. 22, 2023, 8:49 p.m. UTC | #10

> On Sep 22, 2023, at 3:25 PM, Lorenzo Bianconi <[e-mail]> wrote:
> 
>> 
>> 
>>> On Sep 22, 2023, at 12:20 PM, Lorenzo Bianconi <[e-mail]> wrote:
>>> 
>>>> On Fri, 2023-09-22 at 14:44 +0200, Lorenzo Bianconi wrote:
>>>>> Introduce write_threads and write_v4_end_grace netlink commands similar
>>>>> to the ones available through the procfs.
>>>>> Introduce nfsd_nl_server_status_get_dumpit netlink command in order to
>>>>> report global server metadata.
>>>>> 
>>>>> Signed-off-by: Lorenzo Bianconi <[e-mail]>
>>>>> ---
>>>>> This patch can be tested with user-space tool reported below:
>>>>> https://github.com/LorenzoBianconi/nfsd-netlink.git
>>>>> ---
>>>>> Documentation/netlink/specs/nfsd.yaml | 33 +++++++++
>>>>> fs/nfsd/netlink.c                     | 30 ++++++++
>>>>> fs/nfsd/netlink.h                     |  5 ++
>>>>> fs/nfsd/nfsctl.c                      | 98 +++++++++++++++++++++++++++
>>>>> include/uapi/linux/nfsd_netlink.h     | 11 +++
>>>>> 5 files changed, 177 insertions(+)
>>>>> 
>>>>> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
>>>>> index 403d3e3a04f3..fa1204892703 100644
>>>>> --- a/Documentation/netlink/specs/nfsd.yaml
>>>>> +++ b/Documentation/netlink/specs/nfsd.yaml
>>>>> @@ -62,6 +62,15 @@ attribute-sets:
>>>>>        name: compound-ops
>>>>>        type: u32
>>>>>        multi-attr: true
>>>>> +  -
>>>>> +    name: server-attr
>>>>> +    attributes:
>>>>> +      -
>>>>> +        name: threads
>>>>> +        type: u16
>>>> 
>>>> 65k threads ought to be enough for anybody!
>>> 
>>> maybe u8 is fine here :)
>> 
>> 32-bit is the usual for this kind of interface. I don't think we need to go with 16-bit.
> 
> ack, fine
> 
>> 
>> 
>>>>> +      -
>>>>> +        name: v4-grace
>>>>> +        type: u8
>>>>> 
>>>>> operations:
>>>>>  list:
>>>>> @@ -72,3 +81,27 @@ 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: server-attr
>>>>> +      flags: [ admin-perm ]
> 
> [...]
> 
>>> 
>>> I am not sure if ynl supports a doit operation with a request with no parameters.
>>> @Chuck, Jakub: any input here?
>> 
>> I think it does, I might have done something like that for one of the
>> handshake protocol commands.
> 
> please correct me if I am wrong but in Documentation/netlink/specs/handshake.yaml
> we have accept and done operations and both of them have some parameters in the
> request field, right?

I was probably thinking of a command that did not make it into the
final upstream version of handshake.


>> But I think Jeff's right, end_grace might be better postponed. Pick any of
>> the others that you think might be easy to implement instead.
> 
> ack, fine. Do you agree to have a global container (server-status-get) for all
> the server metadata instead of adding dedicated get APIs?

At this point I would like to have a distinct command for each data
item.


> Regards,
> Lorenzo
> 
>> 
>> 
>>> Regards,
>>> Lorenzo
>>> 
>>>> 
>>>>> + return 0;
>>>>> +#else
>>>>> + return -EOPNOTSUPP;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Return values:
>>>>> + *   %0: The server_status_get command may proceed
>>>>> + *   %-ENODEV: There is no NFSD running in this namespace
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
>>>>> +{
>>>>> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
>>>>> +
>>>>> + return nn->nfsd_serv ? 0 : -ENODEV;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * nfsd_nl_server_status_get_dumpit - dump server status info
>>>>> + * @skb: reply buffer
>>>>> + * @cb: netlink metadata and command arguments
>>>>> + *
>>>>> + * Returns the size of the reply or a negative errno.
>>>>> + */
>>>>> +int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
>>>>> +     struct netlink_callback *cb)
>>>>> +{
>>>>> + struct net *net = sock_net(skb->sk);
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> + 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,
>>>>> +  NFSD_CMD_SERVER_STATUS_GET);
>>>>> + if (!hdr)
>>>>> + return -ENOBUFS;
>>>>> +
>>>>> + if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
>>>>> + return -ENOBUFS;
>>>>> +#ifdef CONFIG_NFSD_V4
>>>>> + if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
>>>>> + return -ENOBUFS;
>>>>> +#endif /* CONFIG_NFSD_V4 */
>>>>> +
>>>>> + genlmsg_end(skb, hdr);
>>>>> + cb->args[0] = 1;
>>>>> +
>>>>> + return skb->len;
>>>>> +}
>>>>> +
>>>>> /**
>>>>> * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
>>>>> };
>>>>> 
>>>>> +enum {
>>>>> + NFSD_A_SERVER_ATTR_THREADS = 1,
>>>>> + NFSD_A_SERVER_ATTR_V4_GRACE,
>>>>> +
>>>>> + __NFSD_A_SERVER_ATTR_MAX,
>>>>> + NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
>>>>> +};
>>>>> +
>>>>> enum {
>>>>> NFSD_CMD_RPC_STATUS_GET = 1,
>>>>> + NFSD_CMD_THREADS_SET,
>>>>> + NFSD_CMD_V4_GRACE_RELEASE,
>>>>> + NFSD_CMD_SERVER_STATUS_GET,
>>>>> 
>>>>> __NFSD_CMD_MAX,
>>>>> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
>>>> 
>>>> -- 
>>>> Jeff Layton <[e-mail]>
>>>> 
>> 
>> --
>> Chuck Lever


--
Chuck Lever

Jakub Kicinski Oct. 4, 2023, 5:04 p.m. UTC | #11

On Fri, 22 Sep 2023 18:20:36 +0200 Lorenzo Bianconi wrote:
> > matter at all. Do we have to send down a value at all?  
> 
> I am not sure if ynl supports a doit operation with a request with no parameters.
> @Chuck, Jakub: any input here?

It should, if it doesn't LMK, I will fix..

Lorenzo Bianconi Oct. 5, 2023, 8:52 a.m. UTC | #12

> On Fri, 22 Sep 2023 18:20:36 +0200 Lorenzo Bianconi wrote:
> > > matter at all. Do we have to send down a value at all?  
> > 
> > I am not sure if ynl supports a doit operation with a request with no parameters.
> > @Chuck, Jakub: any input here?
> 
> It should, if it doesn't LMK, I will fix..

ack, what I want to do is add a 'get' method w/o any parameter in the request and
with just one parameter in the reply (i.e. the number of running threads). E.g:

+++ 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
 
 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: get the number of running threads
+      attribute-set: control-plane
+      do:
+        reply:
+          attributes:
+            - threads

running ynl-regen.sh, I got the following error for the get method:

$ ./tools/net/ynl/ynl-regen.sh
        GEN kernel      fs/nfsd/netlink.h
Traceback (most recent call last):
  File "/home/lorenzo/workspace/nfsd-next/tools/net/ynl/ynl-gen-c.py", line 2609, in <module>
    main()
  File "/home/lorenzo/workspace/nfsd-next/tools/net/ynl/ynl-gen-c.py", line 2445, in main
    print_req_policy_fwd(cw, ri.struct['request'], ri=ri)
                             ~~~~~~~~~^^^^^^^^^^^
KeyError: 'request'

am I missing something?

Regards,
Lorenzo

Jakub Kicinski Oct. 5, 2023, 1:59 p.m. UTC | #13

On Thu, 5 Oct 2023 10:52:30 +0200 Lorenzo Bianconi wrote:
> running ynl-regen.sh, I got the following error for the get method:
> 
> $ ./tools/net/ynl/ynl-regen.sh
>         GEN kernel      fs/nfsd/netlink.h
> Traceback (most recent call last):
>   File "/home/lorenzo/workspace/nfsd-next/tools/net/ynl/ynl-gen-c.py", line 2609, in <module>
>     main()
>   File "/home/lorenzo/workspace/nfsd-next/tools/net/ynl/ynl-gen-c.py", line 2445, in main
>     print_req_policy_fwd(cw, ri.struct['request'], ri=ri)
>                              ~~~~~~~~~^^^^^^^^^^^
> KeyError: 'request'
> 
> am I missing something?

Not at all, the codegen was only handling dumps with no ops.
This change seems to make it a little happier, at least it
doesn't throw any exceptions. Will it work for your case?

diff --git a/tools/net/ynl/ynl-gen-c.py b/tools/net/ynl/ynl-gen-c.py
index 168fe612b029..593be2632f23 100755
--- a/tools/net/ynl/ynl-gen-c.py
+++ b/tools/net/ynl/ynl-gen-c.py
@@ -645,6 +645,33 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         self.inherited = [c_lower(x) for x in sorted(self._inherited)]
 
 
+class StructNone:
+    def __init__(self, family, space_name):
+        self.family = family
+        self.space_name = space_name
+        self.attr_set = family.attr_sets[space_name]
+
+        if family.name == c_lower(space_name):
+            self.render_name = f"{family.name}"
+        else:
+            self.render_name = f"{family.name}_{c_lower(space_name)}"
+
+        self.request = False
+        self.reply = False
+
+        self.attr_list = []
+        self.attrs = dict()
+
+    def __iter__(self):
+        yield from self.attrs
+
+    def __getitem__(self, key):
+        return self.attrs[key]
+
+    def member_list(self):
+        return self.attr_list
+
+
 class EnumEntry(SpecEnumEntry):
     def __init__(self, enum_set, yaml, prev, value_start):
         super().__init__(enum_set, yaml, prev, value_start)
@@ -1041,9 +1068,12 @@ from lib import SpecFamily, SpecAttrSet, SpecAttr, SpecOperation, SpecEnumSet, S
         if op_mode == 'notify':
             op_mode = 'do'
         for op_dir in ['request', 'reply']:
-            if op and op_dir in op[op_mode]:
-                self.struct[op_dir] = Struct(family, self.attr_set,
-                                             type_list=op[op_mode][op_dir]['attributes'])
+            if op:
+                if op_dir in op[op_mode]:
+                    self.struct[op_dir] = Struct(family, self.attr_set,
+                                                 type_list=op[op_mode][op_dir]['attributes'])
+                else:
+                    self.struct[op_dir] = StructNone(family, self.attr_set)
         if op_mode == 'event':
             self.struct['reply'] = Struct(family, self.attr_set, type_list=op['event']['attributes'])
 
@@ -1752,6 +1782,8 @@ _C_KW = {
 
 
 def print_req_type_helpers(ri):
+    if isinstance(ri.struct["request"], StructNone):
+        return
     print_alloc_wrapper(ri, "request")
     print_type_helpers(ri, "request")
 
@@ -1773,6 +1805,8 @@ _C_KW = {
 
 
 def print_req_type(ri):
+    if isinstance(ri.struct["request"], StructNone):
+        return
     print_type(ri, "request")
 
 
@@ -2515,9 +2549,8 @@ _C_KW = {
                 if 'dump' in op:
                     cw.p(f"/* {op.enum_name} - dump */")
                     ri = RenderInfo(cw, parsed, args.mode, op, 'dump')
-                    if 'request' in op['dump']:
-                        print_req_type(ri)
-                        print_req_type_helpers(ri)
+                    print_req_type(ri)
+                    print_req_type_helpers(ri)
                     if not ri.type_consistent:
                         print_rsp_type(ri)
                     print_wrapped_type(ri)

Patch

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index 403d3e3a04f3..fa1204892703 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -62,6 +62,15 @@  attribute-sets:
         name: compound-ops
         type: u32
         multi-attr: true
+  -
+    name: server-attr
+    attributes:
+      -
+        name: threads
+        type: u16
+      -
+        name: v4-grace
+        type: u8
 
 operations:
   list:
@@ -72,3 +81,27 @@  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: server-attr
+      flags: [ admin-perm ]
+      do:
+        request:
+          attributes:
+            - threads
+    -
+      name: v4-grace-release
+      doc: release the grace period for nfsd's v4 lock manager
+      attribute-set: server-attr
+      flags: [ admin-perm ]
+      do:
+        request:
+          attributes:
+            - v4-grace
+    -
+      name: server-status-get
+      doc: dump server status info
+      attribute-set: server-attr
+      dump:
+        pre: nfsd-nl-server-status-get-start
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 0e1d635ec5f9..783a34e69354 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -10,6 +10,16 @@ 
 
 #include <uapi/linux/nfsd_netlink.h>
 
+/* NFSD_CMD_THREADS_SET - do */
+static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_ATTR_THREADS + 1] = {
+	[NFSD_A_SERVER_ATTR_THREADS] = { .type = NLA_U16, },
+};
+
+/* NFSD_CMD_V4_GRACE_RELEASE - do */
+static const struct nla_policy nfsd_v4_grace_release_nl_policy[NFSD_A_SERVER_ATTR_V4_GRACE + 1] = {
+	[NFSD_A_SERVER_ATTR_V4_GRACE] = { .type = NLA_U8, },
+};
+
 /* Ops table for nfsd */
 static const struct genl_split_ops nfsd_nl_ops[] = {
 	{
@@ -19,6 +29,26 @@  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_SERVER_ATTR_THREADS,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd		= NFSD_CMD_V4_GRACE_RELEASE,
+		.doit		= nfsd_nl_v4_grace_release_doit,
+		.policy		= nfsd_v4_grace_release_nl_policy,
+		.maxattr	= NFSD_A_SERVER_ATTR_V4_GRACE,
+		.flags		= GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+	},
+	{
+		.cmd	= NFSD_CMD_SERVER_STATUS_GET,
+		.start	= nfsd_nl_server_status_get_start,
+		.dumpit	= nfsd_nl_server_status_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..2e98061fbb0a 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -12,10 +12,15 @@ 
 #include <uapi/linux/nfsd_netlink.h>
 
 int nfsd_nl_rpc_status_get_start(struct netlink_callback *cb);
+int nfsd_nl_server_status_get_start(struct netlink_callback *cb);
 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_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_server_status_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..c631b59b7a4f 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1694,6 +1694,104 @@  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)
+{
+	u16 nthreads;
+	int ret;
+
+	if (!info->attrs[NFSD_A_SERVER_ATTR_THREADS])
+		return -EINVAL;
+
+	nthreads = nla_get_u16(info->attrs[NFSD_A_SERVER_ATTR_THREADS]);
+
+	ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
+	return ret == nthreads ? 0 : ret;
+}
+
+/**
+ * nfsd_nl_v4_grace_release_doit - release the nfs4 grace period
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_v4_grace_release_doit(struct sk_buff *skb, struct genl_info *info)
+{
+#ifdef CONFIG_NFSD_V4
+	struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
+
+	if (!info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE])
+		return -EINVAL;
+
+	if (nla_get_u8(info->attrs[NFSD_A_SERVER_ATTR_V4_GRACE]))
+		nfsd4_end_grace(nn);
+
+	return 0;
+#else
+	return -EOPNOTSUPP;
+#endif /* CONFIG_NFSD_V4 */
+}
+
+/**
+ * nfsd_nl_server_status_get_start - Prepare server_status_get dumpit
+ * @cb: netlink metadata and command arguments
+ *
+ * Return values:
+ *   %0: The server_status_get command may proceed
+ *   %-ENODEV: There is no NFSD running in this namespace
+ */
+int nfsd_nl_server_status_get_start(struct netlink_callback *cb)
+{
+	struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
+
+	return nn->nfsd_serv ? 0 : -ENODEV;
+}
+
+/**
+ * nfsd_nl_server_status_get_dumpit - dump server status info
+ * @skb: reply buffer
+ * @cb: netlink metadata and command arguments
+ *
+ * Returns the size of the reply or a negative errno.
+ */
+int nfsd_nl_server_status_get_dumpit(struct sk_buff *skb,
+				     struct netlink_callback *cb)
+{
+	struct net *net = sock_net(skb->sk);
+#ifdef CONFIG_NFSD_V4
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+#endif /* CONFIG_NFSD_V4 */
+	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,
+			  NFSD_CMD_SERVER_STATUS_GET);
+	if (!hdr)
+		return -ENOBUFS;
+
+	if (nla_put_u16(skb, NFSD_A_SERVER_ATTR_THREADS, nfsd_nrthreads(net)))
+		return -ENOBUFS;
+#ifdef CONFIG_NFSD_V4
+	if (nla_put_u8(skb, NFSD_A_SERVER_ATTR_V4_GRACE, !nn->grace_ended))
+		return -ENOBUFS;
+#endif /* CONFIG_NFSD_V4 */
+
+	genlmsg_end(skb, hdr);
+	cb->args[0] = 1;
+
+	return skb->len;
+}
+
 /**
  * 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..b82fbc53d336 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 = (__NFSD_A_RPC_STATUS_MAX - 1)
 };
 
+enum {
+	NFSD_A_SERVER_ATTR_THREADS = 1,
+	NFSD_A_SERVER_ATTR_V4_GRACE,
+
+	__NFSD_A_SERVER_ATTR_MAX,
+	NFSD_A_SERVER_ATTR_MAX = (__NFSD_A_SERVER_ATTR_MAX - 1)
+};
+
 enum {
 	NFSD_CMD_RPC_STATUS_GET = 1,
+	NFSD_CMD_THREADS_SET,
+	NFSD_CMD_V4_GRACE_RELEASE,
+	NFSD_CMD_SERVER_STATUS_GET,
 
 	__NFSD_CMD_MAX,
 	NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)