Re: [PATCH] NFSD: convert write_threads and write_v4_end_grace to netlink commands

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5920 bytes --]

> 
> 
> > On Sep 22, 2023, at 12:20 PM, Lorenzo Bianconi <lorenzo.bianconi@redhat.com> 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 <lorenzo@kernel.org>
> >>> ---
> >>> 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 <jlayton@kernel.org>
> >> 
> 
> --
> Chuck Lever
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]