> On Sep 26, 2023, at 7:05 PM, NeilBrown <neilb@xxxxxxx> wrote: > > > Hi, > thanks for doing this. I had a hunt through the patch largely to help > improve my own understanding of netlink. A few things stood out. Not > necessarily problems with the patch, but things that seemed worth > mentioning. > > Firstly, and rather trivially, the word "convert" in the subject lead > me to think that the old approach was being converted to a new > approach. I see that isn't correct - you are just introducing a new > option. > > On Wed, 27 Sep 2023, Lorenzo Bianconi wrote: >> Introduce write_threads, write_maxblksize and write_maxconn netlink >> commands similar to the ones available through the procfs. >> >> Signed-off-by: Lorenzo Bianconi <lorenzo@xxxxxxxxxx> >> --- >> Changes since v2: >> - use u32 to store nthreads in nfsd_nl_threads_set_doit >> - rename server-attr in control-plane in nfsd.yaml specs >> Changes since v1: >> - remove write_v4_end_grace command >> - add write_maxblksize and write_maxconn netlink commands >> >> This patch can be tested with user-space tool reported below: >> https://github.com/LorenzoBianconi/nfsd-netlink.git >> --- >> Documentation/netlink/specs/nfsd.yaml | 63 ++++++++++++ >> fs/nfsd/netlink.c | 51 ++++++++++ >> fs/nfsd/netlink.h | 9 ++ >> fs/nfsd/nfsctl.c | 141 ++++++++++++++++++++++++++ >> include/uapi/linux/nfsd_netlink.h | 15 +++ >> 5 files changed, 279 insertions(+) >> >> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml >> index 403d3e3a04f3..c6af1653bc1d 100644 >> --- a/Documentation/netlink/specs/nfsd.yaml >> +++ b/Documentation/netlink/specs/nfsd.yaml >> @@ -62,6 +62,18 @@ attribute-sets: >> name: compound-ops >> type: u32 >> multi-attr: true >> + - >> + name: control-plane >> + attributes: >> + - >> + name: threads >> + type: u32 >> + - >> + name: max-blksize >> + type: u32 >> + - >> + name: max-conn >> + type: u32 >> >> operations: >> list: >> @@ -72,3 +84,54 @@ operations: >> dump: >> pre: nfsd-nl-rpc-status-get-start >> post: nfsd-nl-rpc-status-get-done >> + - >> + name: threads-set >> + doc: set the number of running threads >> + attribute-set: control-plane >> + flags: [ admin-perm ] >> + do: >> + request: >> + attributes: >> + - threads >> + - >> + name: threads-get >> + doc: dump the number of running threads >> + attribute-set: control-plane >> + dump: >> + reply: >> + attributes: >> + - threads >> + - >> + name: max-blksize-set >> + doc: set the nfs block size >> + attribute-set: control-plane >> + flags: [ admin-perm ] >> + do: >> + request: >> + attributes: >> + - max-blksize >> + - >> + name: max-blksize-get >> + doc: dump the nfs block size >> + attribute-set: control-plane >> + dump: >> + reply: >> + attributes: >> + - max-blksize >> + - >> + name: max-conn-set >> + doc: set the max number of connections >> + attribute-set: control-plane >> + flags: [ admin-perm ] >> + do: >> + request: >> + attributes: >> + - max-conn >> + - >> + name: max-conn-get >> + doc: dump the max number of connections >> + attribute-set: control-plane >> + dump: >> + reply: >> + attributes: >> + - max-conn >> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c >> index 0e1d635ec5f9..8f7d72ae60d6 100644 >> --- a/fs/nfsd/netlink.c >> +++ b/fs/nfsd/netlink.c >> @@ -10,6 +10,21 @@ >> >> #include <uapi/linux/nfsd_netlink.h> >> >> +/* NFSD_CMD_THREADS_SET - do */ >> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_CONTROL_PLANE_THREADS + 1] = { >> + [NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, }, >> +}; > > This array, and the following arrays, is sized exactly large enough to > hold the last element. In that case you don't need to explicitly set > the size: > > +static const struct nla_policy nfsd_threads_set_nl_policy[] = { > + [NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, }, > +}; > > I at first assumed you were following a standard practice, but other > places that declare nla_policy use a variety of different approaches. > I prefer the [] approach, but it is up to you. FYI, here and below, this code was generated by a Python script from the nfsd netlink protocol specification in Documentation/netlink/specs/nfsd.yaml. We don't have control over its style or unused elements. >> + >> +/* NFSD_CMD_MAX_BLKSIZE_SET - do */ >> +static const struct nla_policy nfsd_max_blksize_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE + 1] = { >> + [NFSD_A_CONTROL_PLANE_MAX_BLKSIZE] = { .type = NLA_U32, }, >> +}; >> + >> +/* NFSD_CMD_MAX_CONN_SET - do */ >> +static const struct nla_policy nfsd_max_conn_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_CONN + 1] = { >> + [NFSD_A_CONTROL_PLANE_MAX_CONN] = { .type = NLA_U32, }, >> +}; >> + >> /* Ops table for nfsd */ >> static const struct genl_split_ops nfsd_nl_ops[] = { >> { >> @@ -19,6 +34,42 @@ static const struct genl_split_ops nfsd_nl_ops[] = { >> .done = nfsd_nl_rpc_status_get_done, >> .flags = GENL_CMD_CAP_DUMP, >> }, >> + { >> + .cmd = NFSD_CMD_THREADS_SET, >> + .doit = nfsd_nl_threads_set_doit, >> + .policy = nfsd_threads_set_nl_policy, >> + .maxattr = NFSD_A_CONTROL_PLANE_THREADS, > > maxattr is *always* the largest index for the policy array. > I think it would aid reading to have something like > > #define NLA_POLICY(_pol) .policy = _pol, .maxattr = ARRAY_SIZE(_pol) - 1 > > then you could have > .doit = nfsd_nl_thread_set_doit, > NLA_POLICY(nfsd_thread_set_nl_policy), > > and be certain that all the maxattr values are correct. > >> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, >> + }, >> + { >> + .cmd = NFSD_CMD_THREADS_GET, >> + .dumpit = nfsd_nl_threads_get_dumpit, >> + .flags = GENL_CMD_CAP_DUMP, >> + }, >> + { >> + .cmd = NFSD_CMD_MAX_BLKSIZE_SET, >> + .doit = nfsd_nl_max_blksize_set_doit, >> + .policy = nfsd_max_blksize_set_nl_policy, >> + .maxattr = NFSD_A_CONTROL_PLANE_MAX_BLKSIZE, >> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, >> + }, > > it is a little weird that the dumpit sections are indented differently > to the doit section. But that is probably intentional and I might even > grow to like it... > >> + { >> + .cmd = NFSD_CMD_MAX_BLKSIZE_GET, >> + .dumpit = nfsd_nl_max_blksize_get_dumpit, >> + .flags = GENL_CMD_CAP_DUMP, >> + }, >> + { >> + .cmd = NFSD_CMD_MAX_CONN_SET, >> + .doit = nfsd_nl_max_conn_set_doit, >> + .policy = nfsd_max_conn_set_nl_policy, >> + .maxattr = NFSD_A_CONTROL_PLANE_MAX_CONN, >> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, >> + }, >> + { >> + .cmd = NFSD_CMD_MAX_CONN_GET, >> + .dumpit = nfsd_nl_max_conn_get_dumpit, >> + .flags = GENL_CMD_CAP_DUMP, >> + }, >> }; >> >> struct genl_family nfsd_nl_family __ro_after_init = { >> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h >> index d83dd6bdee92..41b95651c638 100644 >> --- a/fs/nfsd/netlink.h >> +++ b/fs/nfsd/netlink.h >> @@ -16,6 +16,15 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb); >> >> int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb, >> struct netlink_callback *cb); >> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info); >> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb, >> + struct netlink_callback *cb); >> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info); >> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb, >> + struct netlink_callback *cb); >> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info); >> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb, >> + struct netlink_callback *cb); >> >> extern struct genl_family nfsd_nl_family; >> >> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c >> index b71744e355a8..07e7a09e28e3 100644 >> --- a/fs/nfsd/nfsctl.c >> +++ b/fs/nfsd/nfsctl.c >> @@ -1694,6 +1694,147 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb) >> return 0; >> } >> >> +/** >> + * nfsd_nl_threads_set_doit - set the number of running threads >> + * @skb: reply buffer >> + * @info: netlink metadata and command arguments >> + * >> + * Return 0 on success or a negative errno. >> + */ >> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info) >> +{ >> + u32 nthreads; >> + int ret; >> + >> + if (!info->attrs[NFSD_A_CONTROL_PLANE_THREADS]) >> + return -EINVAL; >> + >> + nthreads = nla_get_u32(info->attrs[NFSD_A_CONTROL_PLANE_THREADS]); >> + >> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred()); >> + return ret == nthreads ? 0 : ret; >> +} >> + >> +static int nfsd_nl_get_dump(struct sk_buff *skb, struct netlink_callback *cb, >> + int cmd, int attr, u32 val) >> +{ >> + void *hdr; >> + >> + if (cb->args[0]) /* already consumed */ >> + return 0; >> + >> + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq, >> + &nfsd_nl_family, NLM_F_MULTI, cmd); >> + if (!hdr) >> + return -ENOBUFS; >> + >> + if (nla_put_u32(skb, attr, val)) >> + return -ENOBUFS; >> + >> + genlmsg_end(skb, hdr); >> + cb->args[0] = 1; >> + >> + return skb->len; >> +} >> + >> +/** >> + * nfsd_nl_threads_get_dumpit - dump the number of running threads >> + * @skb: reply buffer >> + * @cb: netlink metadata and command arguments >> + * >> + * Returns the size of the reply or a negative errno. >> + */ >> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb) >> +{ >> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_THREADS_GET, >> + NFSD_A_CONTROL_PLANE_THREADS, >> + nfsd_nrthreads(sock_net(skb->sk))); >> +} >> + >> +/** >> + * nfsd_nl_max_blksize_set_doit - set the nfs block size >> + * @skb: reply buffer >> + * @info: netlink metadata and command arguments >> + * >> + * Return 0 on success or a negative errno. >> + */ >> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info) >> +{ >> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id); >> + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE]; >> + int ret = 0; >> + >> + if (!attr) >> + return -EINVAL; >> + >> + mutex_lock(&nfsd_mutex); >> + if (nn->nfsd_serv) { >> + ret = -EBUSY; >> + goto out; >> + } > > This code is wrong... but then the original in write_maxblksize is wrong > to, so you can't be blamed. > nfsd_max_blksize applies to nfsd in ALL network namespaces. So if we > need to check there are no active services in one namespace, we need to > check the same for *all* namespaces. > > I think we should make nfsd_max_blksize a per-namespace value. > >> + >> + nfsd_max_blksize = nla_get_u32(attr); >> + nfsd_max_blksize = max_t(int, nfsd_max_blksize, 1024); >> + nfsd_max_blksize = min_t(int, nfsd_max_blksize, NFSSVC_MAXBLKSIZE); >> + nfsd_max_blksize &= ~1023; >> +out: >> + mutex_unlock(&nfsd_mutex); >> + >> + return ret; >> +} >> + >> +/** >> + * nfsd_nl_max_blksize_get_dumpit - dump the nfs block size >> + * @skb: reply buffer >> + * @cb: netlink metadata and command arguments >> + * >> + * Returns the size of the reply or a negative errno. >> + */ >> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb, >> + struct netlink_callback *cb) >> +{ >> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_BLKSIZE_GET, >> + NFSD_A_CONTROL_PLANE_MAX_BLKSIZE, >> + nfsd_max_blksize); >> +} >> + >> +/** >> + * nfsd_nl_max_conn_set_doit - set the max number of connections >> + * @skb: reply buffer >> + * @info: netlink metadata and command arguments >> + * >> + * Return 0 on success or a negative errno. >> + */ >> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info) >> +{ >> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id); >> + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_CONN]; >> + >> + if (!attr) >> + return -EINVAL; >> + >> + nn->max_connections = nla_get_u32(attr); >> + >> + return 0; >> +} >> + >> +/** >> + * nfsd_nl_max_conn_get_dumpit - dump the max number of connections >> + * @skb: reply buffer >> + * @cb: netlink metadata and command arguments >> + * >> + * Returns the size of the reply or a negative errno. >> + */ >> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb, >> + struct netlink_callback *cb) >> +{ >> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id); >> + >> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_CONN_GET, >> + NFSD_A_CONTROL_PLANE_MAX_CONN, >> + nn->max_connections); >> +} >> + >> /** >> * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace >> * @net: a freshly-created network namespace >> diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h >> index c8ae72466ee6..145f4811f3d9 100644 >> --- a/include/uapi/linux/nfsd_netlink.h >> +++ b/include/uapi/linux/nfsd_netlink.h >> @@ -29,8 +29,23 @@ enum { >> NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1) >> }; >> >> +enum { >> + NFSD_A_CONTROL_PLANE_THREADS = 1, >> + NFSD_A_CONTROL_PLANE_MAX_BLKSIZE, >> + NFSD_A_CONTROL_PLANE_MAX_CONN, >> + >> + __NFSD_A_CONTROL_PLANE_MAX, >> + NFSD_A_CONTROL_PLANE_MAX = (__NFSD_A_CONTROL_PLANE_MAX - 1) > > This last value is never used. Is it needed? > > > Thanks, > NeilBrown > >> +}; >> + >> enum { >> NFSD_CMD_RPC_STATUS_GET = 1, >> + NFSD_CMD_THREADS_SET, >> + NFSD_CMD_THREADS_GET, >> + NFSD_CMD_MAX_BLKSIZE_SET, >> + NFSD_CMD_MAX_BLKSIZE_GET, >> + NFSD_CMD_MAX_CONN_SET, >> + NFSD_CMD_MAX_CONN_GET, >> >> __NFSD_CMD_MAX, >> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1) >> -- >> 2.41.0 -- Chuck Lever
