Introduce write_threads, write_maxblksize and write_maxconn netlink
commands similar to the ones available through the procfs.
Signed-off-by: Lorenzo Bianconi <[email protected]>
---
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, },
+};
+
+/* 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,
+ .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,
+ },
+ {
+ .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;
+ }
+
+ 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)
+};
+
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
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 <[email protected]>
> ---
> Changes since v2:
> - use u32 to store nthreads in nfsd_nl_threads_set_doit
> - rename server-attr in control-plane in nfsd.yaml specs
> Changes since v1:
> - remove write_v4_end_grace command
> - add write_maxblksize and write_maxconn netlink commands
>
> This patch can be tested with user-space tool reported below:
> https://github.com/LorenzoBianconi/nfsd-netlink.git
> ---
> Documentation/netlink/specs/nfsd.yaml | 63 ++++++++++++
> fs/nfsd/netlink.c | 51 ++++++++++
> fs/nfsd/netlink.h | 9 ++
> fs/nfsd/nfsctl.c | 141 ++++++++++++++++++++++++++
> include/uapi/linux/nfsd_netlink.h | 15 +++
> 5 files changed, 279 insertions(+)
>
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 403d3e3a04f3..c6af1653bc1d 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -62,6 +62,18 @@ attribute-sets:
> name: compound-ops
> type: u32
> multi-attr: true
> + -
> + name: control-plane
> + attributes:
> + -
> + name: threads
> + type: u32
> + -
> + name: max-blksize
> + type: u32
> + -
> + name: max-conn
> + type: u32
>
> operations:
> list:
> @@ -72,3 +84,54 @@ operations:
> dump:
> pre: nfsd-nl-rpc-status-get-start
> post: nfsd-nl-rpc-status-get-done
> + -
> + name: threads-set
> + doc: set the number of running threads
> + attribute-set: control-plane
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - threads
> + -
> + name: threads-get
> + doc: dump the number of running threads
> + attribute-set: control-plane
> + dump:
> + reply:
> + attributes:
> + - threads
> + -
> + name: max-blksize-set
> + doc: set the nfs block size
> + attribute-set: control-plane
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - max-blksize
> + -
> + name: max-blksize-get
> + doc: dump the nfs block size
> + attribute-set: control-plane
> + dump:
> + reply:
> + attributes:
> + - max-blksize
> + -
> + name: max-conn-set
> + doc: set the max number of connections
> + attribute-set: control-plane
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - max-conn
> + -
> + name: max-conn-get
> + doc: dump the max number of connections
> + attribute-set: control-plane
> + dump:
> + reply:
> + attributes:
> + - max-conn
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0e1d635ec5f9..8f7d72ae60d6 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -10,6 +10,21 @@
>
> #include <uapi/linux/nfsd_netlink.h>
>
> +/* NFSD_CMD_THREADS_SET - do */
> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_CONTROL_PLANE_THREADS + 1] = {
> + [NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, },
> +};
This array, and the following arrays, is sized exactly large enough to
hold the last element. In that case you don't need to explicitly set
the size:
+static const struct nla_policy nfsd_threads_set_nl_policy[] = {
+ [NFSD_A_CONTROL_PLANE_THREADS] = { .type = NLA_U32, },
+};
I at first assumed you were following a standard practice, but other
places that declare nla_policy use a variety of different approaches.
I prefer the [] approach, but it is up to you.
> +
> +/* NFSD_CMD_MAX_BLKSIZE_SET - do */
> +static const struct nla_policy nfsd_max_blksize_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE + 1] = {
> + [NFSD_A_CONTROL_PLANE_MAX_BLKSIZE] = { .type = NLA_U32, },
> +};
> +
> +/* NFSD_CMD_MAX_CONN_SET - do */
> +static const struct nla_policy nfsd_max_conn_set_nl_policy[NFSD_A_CONTROL_PLANE_MAX_CONN + 1] = {
> + [NFSD_A_CONTROL_PLANE_MAX_CONN] = { .type = NLA_U32, },
> +};
> +
> /* Ops table for nfsd */
> static const struct genl_split_ops nfsd_nl_ops[] = {
> {
> @@ -19,6 +34,42 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> .done = nfsd_nl_rpc_status_get_done,
> .flags = GENL_CMD_CAP_DUMP,
> },
> + {
> + .cmd = NFSD_CMD_THREADS_SET,
> + .doit = nfsd_nl_threads_set_doit,
> + .policy = nfsd_threads_set_nl_policy,
> + .maxattr = NFSD_A_CONTROL_PLANE_THREADS,
maxattr is *always* the largest index for the policy array.
I think it would aid reading to have something like
#define NLA_POLICY(_pol) .policy = _pol, .maxattr = ARRAY_SIZE(_pol) - 1
then you could have
.doit = nfsd_nl_thread_set_doit,
NLA_POLICY(nfsd_thread_set_nl_policy),
and be certain that all the maxattr values are correct.
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
> + {
> + .cmd = NFSD_CMD_THREADS_GET,
> + .dumpit = nfsd_nl_threads_get_dumpit,
> + .flags = GENL_CMD_CAP_DUMP,
> + },
> + {
> + .cmd = NFSD_CMD_MAX_BLKSIZE_SET,
> + .doit = nfsd_nl_max_blksize_set_doit,
> + .policy = nfsd_max_blksize_set_nl_policy,
> + .maxattr = NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
it is a little weird that the dumpit sections are indented differently
to the doit section. But that is probably intentional and I might even
grow to like it...
> + {
> + .cmd = NFSD_CMD_MAX_BLKSIZE_GET,
> + .dumpit = nfsd_nl_max_blksize_get_dumpit,
> + .flags = GENL_CMD_CAP_DUMP,
> + },
> + {
> + .cmd = NFSD_CMD_MAX_CONN_SET,
> + .doit = nfsd_nl_max_conn_set_doit,
> + .policy = nfsd_max_conn_set_nl_policy,
> + .maxattr = NFSD_A_CONTROL_PLANE_MAX_CONN,
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
> + {
> + .cmd = NFSD_CMD_MAX_CONN_GET,
> + .dumpit = nfsd_nl_max_conn_get_dumpit,
> + .flags = GENL_CMD_CAP_DUMP,
> + },
> };
>
> struct genl_family nfsd_nl_family __ro_after_init = {
> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> index d83dd6bdee92..41b95651c638 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -16,6 +16,15 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb);
>
> int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb);
> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb);
> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb);
> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb);
>
> extern struct genl_family nfsd_nl_family;
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index b71744e355a8..07e7a09e28e3 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1694,6 +1694,147 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb)
> return 0;
> }
>
> +/**
> + * nfsd_nl_threads_set_doit - set the number of running threads
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + u32 nthreads;
> + int ret;
> +
> + if (!info->attrs[NFSD_A_CONTROL_PLANE_THREADS])
> + return -EINVAL;
> +
> + nthreads = nla_get_u32(info->attrs[NFSD_A_CONTROL_PLANE_THREADS]);
> +
> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> + return ret == nthreads ? 0 : ret;
> +}
> +
> +static int nfsd_nl_get_dump(struct sk_buff *skb, struct netlink_callback *cb,
> + int cmd, int attr, u32 val)
> +{
> + void *hdr;
> +
> + if (cb->args[0]) /* already consumed */
> + return 0;
> +
> + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
> + &nfsd_nl_family, NLM_F_MULTI, cmd);
> + if (!hdr)
> + return -ENOBUFS;
> +
> + if (nla_put_u32(skb, attr, val))
> + return -ENOBUFS;
> +
> + genlmsg_end(skb, hdr);
> + cb->args[0] = 1;
> +
> + return skb->len;
> +}
> +
> +/**
> + * nfsd_nl_threads_get_dumpit - dump the number of running threads
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_threads_get_dumpit(struct sk_buff *skb, struct netlink_callback *cb)
> +{
> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_THREADS_GET,
> + NFSD_A_CONTROL_PLANE_THREADS,
> + nfsd_nrthreads(sock_net(skb->sk)));
> +}
> +
> +/**
> + * nfsd_nl_max_blksize_set_doit - set the nfs block size
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_max_blksize_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_BLKSIZE];
> + int ret = 0;
> +
> + if (!attr)
> + return -EINVAL;
> +
> + mutex_lock(&nfsd_mutex);
> + if (nn->nfsd_serv) {
> + ret = -EBUSY;
> + goto out;
> + }
This code is wrong... but then the original in write_maxblksize is wrong
to, so you can't be blamed.
nfsd_max_blksize applies to nfsd in ALL network namespaces. So if we
need to check there are no active services in one namespace, we need to
check the same for *all* namespaces.
I think we should make nfsd_max_blksize a per-namespace value.
> +
> + nfsd_max_blksize = nla_get_u32(attr);
> + nfsd_max_blksize = max_t(int, nfsd_max_blksize, 1024);
> + nfsd_max_blksize = min_t(int, nfsd_max_blksize, NFSSVC_MAXBLKSIZE);
> + nfsd_max_blksize &= ~1023;
> +out:
> + mutex_unlock(&nfsd_mutex);
> +
> + return ret;
> +}
> +
> +/**
> + * nfsd_nl_max_blksize_get_dumpit - dump the nfs block size
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_max_blksize_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb)
> +{
> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_BLKSIZE_GET,
> + NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> + nfsd_max_blksize);
> +}
> +
> +/**
> + * nfsd_nl_max_conn_set_doit - set the max number of connections
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_max_conn_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> + struct nlattr *attr = info->attrs[NFSD_A_CONTROL_PLANE_MAX_CONN];
> +
> + if (!attr)
> + return -EINVAL;
> +
> + nn->max_connections = nla_get_u32(attr);
> +
> + return 0;
> +}
> +
> +/**
> + * nfsd_nl_max_conn_get_dumpit - dump the max number of connections
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_max_conn_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb)
> +{
> + struct nfsd_net *nn = net_generic(sock_net(cb->skb->sk), nfsd_net_id);
> +
> + return nfsd_nl_get_dump(skb, cb, NFSD_CMD_MAX_CONN_GET,
> + NFSD_A_CONTROL_PLANE_MAX_CONN,
> + nn->max_connections);
> +}
> +
> /**
> * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> * @net: a freshly-created network namespace
> diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> index c8ae72466ee6..145f4811f3d9 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -29,8 +29,23 @@ enum {
> NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
> };
>
> +enum {
> + NFSD_A_CONTROL_PLANE_THREADS = 1,
> + NFSD_A_CONTROL_PLANE_MAX_BLKSIZE,
> + NFSD_A_CONTROL_PLANE_MAX_CONN,
> +
> + __NFSD_A_CONTROL_PLANE_MAX,
> + NFSD_A_CONTROL_PLANE_MAX = (__NFSD_A_CONTROL_PLANE_MAX - 1)
This last value is never used. Is it needed?
Thanks,
NeilBrown
> +};
> +
> enum {
> NFSD_CMD_RPC_STATUS_GET = 1,
> + NFSD_CMD_THREADS_SET,
> + NFSD_CMD_THREADS_GET,
> + NFSD_CMD_MAX_BLKSIZE_SET,
> + NFSD_CMD_MAX_BLKSIZE_GET,
> + NFSD_CMD_MAX_CONN_SET,
> + NFSD_CMD_MAX_CONN_GET,
>
> __NFSD_CMD_MAX,
> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> --
> 2.41.0
>
>
> On Sep 26, 2023, at 7:05 PM, NeilBrown <[email protected]> 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 <[email protected]>
>> ---
>> 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
On Wed, Sep 27, 2023 at 09:05:10AM +1000, NeilBrown wrote:
> > 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.
Yes, the original code does look strange and is probably incorrect
with regard to its handling of the mutex. Shall we explore and fix
that issue in the nfsctl code first so that it can be backported to
stable kernels?
> I think we should make nfsd_max_blksize a per-namespace value.
That is a different conversation.
First, the current name of this tunable is incongruent with its
actual function, which is to specify the maximum network buffer size
that is allocated when the NFSD service pool is created. We should
find a more descriptive and specific name for this element in the
netlink protocol.
Second, it does seem like a candidate for becoming namespace-
specific, but TBH I'm not familiar enough with its current user
space consumers to know if that change would be welcome or fraught.
Since more discussion, research, and possibly a fix are needed, we
might drop max_blksize from this round and look for one or two
other tunables to convert for the first round.
> > +
> > + 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
[...]
> >
> > 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.
>
> Yes, the original code does look strange and is probably incorrect
> with regard to its handling of the mutex. Shall we explore and fix
> that issue in the nfsctl code first so that it can be backported to
> stable kernels?
>
>
> > I think we should make nfsd_max_blksize a per-namespace value.
>
> That is a different conversation.
>
> First, the current name of this tunable is incongruent with its
> actual function, which is to specify the maximum network buffer size
> that is allocated when the NFSD service pool is created. We should
> find a more descriptive and specific name for this element in the
> netlink protocol.
>
> Second, it does seem like a candidate for becoming namespace-
> specific, but TBH I'm not familiar enough with its current user
> space consumers to know if that change would be welcome or fraught.
>
> Since more discussion, research, and possibly a fix are needed, we
> might drop max_blksize from this round and look for one or two
> other tunables to convert for the first round.
ack. Are write_unlock_ip() and/or write_unlock_fs() good candidates?
Regards,
Lorenzo
>
>
> > > +
> > > + 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
On Fri, 2023-09-29 at 09:44 -0400, Chuck Lever wrote:
> On Wed, Sep 27, 2023 at 09:05:10AM +1000, NeilBrown wrote:
> > > 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.
>
> Yes, the original code does look strange and is probably incorrect
> with regard to its handling of the mutex. Shall we explore and fix
> that issue in the nfsctl code first so that it can be backported to
> stable kernels?
>
>
> > I think we should make nfsd_max_blksize a per-namespace value.
>
> That is a different conversation.
>
> First, the current name of this tunable is incongruent with its
> actual function, which is to specify the maximum network buffer size
> that is allocated when the NFSD service pool is created. We should
> find a more descriptive and specific name for this element in the
> netlink protocol.
>
> Second, it does seem like a candidate for becoming namespace-
> specific, but TBH I'm not familiar enough with its current user
> space consumers to know if that change would be welcome or fraught.
>
> Since more discussion, research, and possibly a fix are needed, we
> might drop max_blksize from this round and look for one or two
> other tunables to convert for the first round.
>
>
I think we need to step back a bit further even, and consider what we
want this to look like for users. How do we expect users to interact
with these new interfaces in the future?
Most of these settings are things that are "set and forget" and things
that we'd want to set up before we ever start any nfsd threads. I think
as an initial goal here, we ought to aim to replace the guts of
rpc.nfsd(8). Make it (preferentially) use the netlink interfaces for
setting everything instead of writing to files under /proc/fs/nfsd.
That gives us a clear set of interfaces that need to be replaced as a
first step, and gives us a start on integrating this change into nfs-
utils.
> > > +
> > > + 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
--
Jeff Layton <[email protected]>
On Mon, 2023-10-02 at 15:19 +0000, Chuck Lever III wrote:
>
> > On Oct 2, 2023, at 9:25 AM, Jeff Layton <[email protected]> wrote:
> >
> > On Fri, 2023-09-29 at 09:44 -0400, Chuck Lever wrote:
> > > On Wed, Sep 27, 2023 at 09:05:10AM +1000, NeilBrown wrote:
> > > > > 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.
> > >
> > > Yes, the original code does look strange and is probably incorrect
> > > with regard to its handling of the mutex. Shall we explore and fix
> > > that issue in the nfsctl code first so that it can be backported to
> > > stable kernels?
> > >
> > >
> > > > I think we should make nfsd_max_blksize a per-namespace value.
> > >
> > > That is a different conversation.
> > >
> > > First, the current name of this tunable is incongruent with its
> > > actual function, which is to specify the maximum network buffer size
> > > that is allocated when the NFSD service pool is created. We should
> > > find a more descriptive and specific name for this element in the
> > > netlink protocol.
> > >
> > > Second, it does seem like a candidate for becoming namespace-
> > > specific, but TBH I'm not familiar enough with its current user
> > > space consumers to know if that change would be welcome or fraught.
> > >
> > > Since more discussion, research, and possibly a fix are needed, we
> > > might drop max_blksize from this round and look for one or two
> > > other tunables to convert for the first round.
> > >
> > >
> >
> > I think we need to step back a bit further even, and consider what we
> > want this to look like for users. How do we expect users to interact
> > with these new interfaces in the future?
> >
> > Most of these settings are things that are "set and forget" and things
> > that we'd want to set up before we ever start any nfsd threads. I think
> > as an initial goal here, we ought to aim to replace the guts of
> > rpc.nfsd(8). Make it (preferentially) use the netlink interfaces for
> > setting everything instead of writing to files under /proc/fs/nfsd.
> >
> > That gives us a clear set of interfaces that need to be replaced as a
> > first step, and gives us a start on integrating this change into nfs-
> > utils.
>
> Starting with rpc.nfsd as the initial consumer is a fine idea.
> Those are in nfs-utils/utils/nfsd/nfssvc.c.
>
> Looks like threads, ports, and versions are the target APIs?
>
Yeah, those are the most common ones.
Eventually, I think we'd want to add some of the other, more obscure
settings to rpc.nfsd as well (max_block_size, max_connections, etc). We
might want to think about how to subsume the pool_threads handling into
that too. Those can be done in a later phase though, once the core
functionality has been converted.
If we're going to go all-in on netlink, then a long-term goal ought to
be to deprecate /proc/fs/nfsd altogether. Unfortunately, we won't be
able to do that for a _long_ time (years), but I think this is a
reasonable start.
> > > > > +
> > > > > + 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
> >
> > --
> > Jeff Layton <[email protected]>
>
> --
> Chuck Lever
>
>
--
Jeff Layton <[email protected]>
> On Oct 2, 2023, at 9:25 AM, Jeff Layton <[email protected]> wrote:
>
> On Fri, 2023-09-29 at 09:44 -0400, Chuck Lever wrote:
>> On Wed, Sep 27, 2023 at 09:05:10AM +1000, NeilBrown wrote:
>>>> 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.
>>
>> Yes, the original code does look strange and is probably incorrect
>> with regard to its handling of the mutex. Shall we explore and fix
>> that issue in the nfsctl code first so that it can be backported to
>> stable kernels?
>>
>>
>>> I think we should make nfsd_max_blksize a per-namespace value.
>>
>> That is a different conversation.
>>
>> First, the current name of this tunable is incongruent with its
>> actual function, which is to specify the maximum network buffer size
>> that is allocated when the NFSD service pool is created. We should
>> find a more descriptive and specific name for this element in the
>> netlink protocol.
>>
>> Second, it does seem like a candidate for becoming namespace-
>> specific, but TBH I'm not familiar enough with its current user
>> space consumers to know if that change would be welcome or fraught.
>>
>> Since more discussion, research, and possibly a fix are needed, we
>> might drop max_blksize from this round and look for one or two
>> other tunables to convert for the first round.
>>
>>
>
> I think we need to step back a bit further even, and consider what we
> want this to look like for users. How do we expect users to interact
> with these new interfaces in the future?
>
> Most of these settings are things that are "set and forget" and things
> that we'd want to set up before we ever start any nfsd threads. I think
> as an initial goal here, we ought to aim to replace the guts of
> rpc.nfsd(8). Make it (preferentially) use the netlink interfaces for
> setting everything instead of writing to files under /proc/fs/nfsd.
>
> That gives us a clear set of interfaces that need to be replaced as a
> first step, and gives us a start on integrating this change into nfs-
> utils.
Starting with rpc.nfsd as the initial consumer is a fine idea.
Those are in nfs-utils/utils/nfsd/nfssvc.c.
Looks like threads, ports, and versions are the target APIs?
>>>> +
>>>> + 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
>
> --
> Jeff Layton <[email protected]>
--
Chuck Lever
On Wed, 27 Sep 2023 00:13:15 +0200 Lorenzo Bianconi wrote:
> +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;
Consider using GENL_REQ_ATTR_CHECK(), it will auto-add nice error
message to the reply on error.
> + 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)
YNL will dutifully return a list for every dump. If you're only getting
a single reply 'do' will be much better.
> On Wed, 27 Sep 2023 00:13:15 +0200 Lorenzo Bianconi wrote:
> > +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;
>
> Consider using GENL_REQ_ATTR_CHECK(), it will auto-add nice error
> message to the reply on error.
ack, I will fix it.
>
> > + 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)
>
> YNL will dutifully return a list for every dump. If you're only getting
> a single reply 'do' will be much better.
ack, I will fix it.
Regards,
Lorenzo