LinuxLists.cc - [PATCH v5 0/3] convert write_threads, write_version and write

[permalink] [raw]

Subject: [PATCH v5 1/3] NFSD: convert write_threads to netlink command

Introduce write_threads netlink command similar to the ones available
through the procfs.

Signed-off-by: Lorenzo Bianconi <[email protected]>
---
Documentation/netlink/specs/nfsd.yaml | 23 +++++++
fs/nfsd/netlink.c | 17 +++++
fs/nfsd/netlink.h | 2 +
fs/nfsd/nfsctl.c | 58 +++++++++++++++++
include/uapi/linux/nfsd_netlink.h | 9 +++
tools/net/ynl/generated/nfsd-user.c | 92 +++++++++++++++++++++++++++
tools/net/ynl/generated/nfsd-user.h | 47 ++++++++++++++
7 files changed, 248 insertions(+)

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index 05acc73e2e33..c92e1425d316 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -62,6 +62,12 @@ attribute-sets:
name: compound-ops
type: u32
multi-attr: true
+ -
+ name: server-worker
+ attributes:
+ -
+ name: threads
+ type: u32

operations:
list:
@@ -87,3 +93,20 @@ operations:
- sport
- dport
- compound-ops
+ -
+ name: threads-set
+ doc: set the number of running threads
+ attribute-set: server-worker
+ flags: [ admin-perm ]
+ do:
+ request:
+ attributes:
+ - threads
+ -
+ name: threads-get
+ doc: get the number of running threads
+ attribute-set: server-worker
+ do:
+ reply:
+ attributes:
+ - threads
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 0e1d635ec5f9..1a59a8e6c7e2 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -10,6 +10,11 @@

#include <uapi/linux/nfsd_netlink.h>

+/* NFSD_CMD_THREADS_SET - do */
+static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_THREADS + 1] = {
+ [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
+};
+
/* Ops table for nfsd */
static const struct genl_split_ops nfsd_nl_ops[] = {
{
@@ -19,6 +24,18 @@ 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_WORKER_THREADS,
+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+ },
+ {
+ .cmd = NFSD_CMD_THREADS_GET,
+ .doit = nfsd_nl_threads_get_doit,
+ .flags = GENL_CMD_CAP_DO,
+ },
};

struct genl_family nfsd_nl_family __ro_after_init = {
diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
index d83dd6bdee92..4137fac477e4 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -16,6 +16,8 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb);

int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb);
+int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);

extern struct genl_family nfsd_nl_family;

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index d6eeee149370..130b3d937a79 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1699,6 +1699,64 @@ 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 (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_WORKER_THREADS))
+ return -EINVAL;
+
+ nthreads = nla_get_u32(info->attrs[NFSD_A_SERVER_WORKER_THREADS]);
+ ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
+
+ return ret == nthreads ? 0 : ret;
+}
+
+/**
+ * nfsd_nl_threads_get_doit - get the number of running threads
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ void *hdr;
+ int err;
+
+ skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ hdr = genlmsg_iput(skb, info);
+ if (!hdr) {
+ err = -EMSGSIZE;
+ goto err_free_msg;
+ }
+
+ if (nla_put_u32(skb, NFSD_A_SERVER_WORKER_THREADS,
+ nfsd_nrthreads(genl_info_net(info)))) {
+ err = -EINVAL;
+ goto err_free_msg;
+ }
+
+ genlmsg_end(skb, hdr);
+
+ return genlmsg_reply(skb, info);
+
+err_free_msg:
+ nlmsg_free(skb);
+ return err;
+}
+
/**
* nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
* @net: a freshly-created network namespace
diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
index 3cd044edee5d..1b6fe1f9ed0e 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -29,8 +29,17 @@ enum {
NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
};

+enum {
+ NFSD_A_SERVER_WORKER_THREADS = 1,
+
+ __NFSD_A_SERVER_WORKER_MAX,
+ NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
+};
+
enum {
NFSD_CMD_RPC_STATUS_GET = 1,
+ NFSD_CMD_THREADS_SET,
+ NFSD_CMD_THREADS_GET,

__NFSD_CMD_MAX,
NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
index 360b6448c6e9..9768328a7751 100644
--- a/tools/net/ynl/generated/nfsd-user.c
+++ b/tools/net/ynl/generated/nfsd-user.c
@@ -15,6 +15,8 @@
/* Enums */
static const char * const nfsd_op_strmap[] = {
[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
+ [NFSD_CMD_THREADS_SET] = "threads-set",
+ [NFSD_CMD_THREADS_GET] = "threads-get",
};

const char *nfsd_op_str(int op)
@@ -47,6 +49,15 @@ struct ynl_policy_nest nfsd_rpc_status_nest = {
.table = nfsd_rpc_status_policy,
};

+struct ynl_policy_attr nfsd_server_worker_policy[NFSD_A_SERVER_WORKER_MAX + 1] = {
+ [NFSD_A_SERVER_WORKER_THREADS] = { .name = "threads", .type = YNL_PT_U32, },
+};
+
+struct ynl_policy_nest nfsd_server_worker_nest = {
+ .max_attr = NFSD_A_SERVER_WORKER_MAX,
+ .table = nfsd_server_worker_policy,
+};
+
/* Common nested types */
/* ============== NFSD_CMD_RPC_STATUS_GET ============== */
/* NFSD_CMD_RPC_STATUS_GET - dump */
@@ -198,6 +209,87 @@ nfsd_rpc_status_get_dump(struct ynl_sock *ys)
return NULL;
}

+/* ============== NFSD_CMD_THREADS_SET ============== */
+/* NFSD_CMD_THREADS_SET - do */
+void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req)
+{
+ free(req);
+}
+
+int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_SET, 1);
+ ys->req_policy = &nfsd_server_worker_nest;
+
+ if (req->_present.threads)
+ mnl_attr_put_u32(nlh, NFSD_A_SERVER_WORKER_THREADS, req->threads);
+
+ err = ynl_exec(ys, nlh, NULL);
+ if (err < 0)
+ return -1;
+
+ return 0;
+}
+
+/* ============== NFSD_CMD_THREADS_GET ============== */
+/* NFSD_CMD_THREADS_GET - do */
+void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp)
+{
+ free(rsp);
+}
+
+int nfsd_threads_get_rsp_parse(const struct nlmsghdr *nlh, void *data)
+{
+ struct ynl_parse_arg *yarg = data;
+ struct nfsd_threads_get_rsp *dst;
+ const struct nlattr *attr;
+
+ dst = yarg->data;
+
+ mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
+ unsigned int type = mnl_attr_get_type(attr);
+
+ if (type == NFSD_A_SERVER_WORKER_THREADS) {
+ if (ynl_attr_validate(yarg, attr))
+ return MNL_CB_ERROR;
+ dst->_present.threads = 1;
+ dst->threads = mnl_attr_get_u32(attr);
+ }
+ }
+
+ return MNL_CB_OK;
+}
+
+struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
+{
+ struct ynl_req_state yrs = { .yarg = { .ys = ys, }, };
+ struct nfsd_threads_get_rsp *rsp;
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_GET, 1);
+ ys->req_policy = &nfsd_server_worker_nest;
+ yrs.yarg.rsp_policy = &nfsd_server_worker_nest;
+
+ rsp = calloc(1, sizeof(*rsp));
+ yrs.yarg.data = rsp;
+ yrs.cb = nfsd_threads_get_rsp_parse;
+ yrs.rsp_cmd = NFSD_CMD_THREADS_GET;
+
+ err = ynl_exec(ys, nlh, &yrs);
+ if (err < 0)
+ goto err_free;
+
+ return rsp;
+
+err_free:
+ nfsd_threads_get_rsp_free(rsp);
+ return NULL;
+}
+
const struct ynl_family ynl_nfsd_family = {
.name = "nfsd",
};
diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
index 989c6e209ced..e162a4f20d91 100644
--- a/tools/net/ynl/generated/nfsd-user.h
+++ b/tools/net/ynl/generated/nfsd-user.h
@@ -64,4 +64,51 @@ nfsd_rpc_status_get_rsp_list_free(struct nfsd_rpc_status_get_rsp_list *rsp);
struct nfsd_rpc_status_get_rsp_list *
nfsd_rpc_status_get_dump(struct ynl_sock *ys);

+/* ============== NFSD_CMD_THREADS_SET ============== */
+/* NFSD_CMD_THREADS_SET - do */
+struct nfsd_threads_set_req {
+ struct {
+ __u32 threads:1;
+ } _present;
+
+ __u32 threads;
+};
+
+static inline struct nfsd_threads_set_req *nfsd_threads_set_req_alloc(void)
+{
+ return calloc(1, sizeof(struct nfsd_threads_set_req));
+}
+void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req);
+
+static inline void
+nfsd_threads_set_req_set_threads(struct nfsd_threads_set_req *req,
+ __u32 threads)
+{
+ req->_present.threads = 1;
+ req->threads = threads;
+}
+
+/*
+ * set the number of running threads
+ */
+int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req);
+
+/* ============== NFSD_CMD_THREADS_GET ============== */
+/* NFSD_CMD_THREADS_GET - do */
+
+struct nfsd_threads_get_rsp {
+ struct {
+ __u32 threads:1;
+ } _present;
+
+ __u32 threads;
+};
+
+void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
+
+/*
+ * get the number of running threads
+ */
+struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
+
#endif /* _LINUX_NFSD_GEN_H */
--
2.43.0

2023-11-29 17:13:33

[permalink] [raw]

Subject: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

Introduce write_ports netlink command similar to the ones available
through the procfs.

Signed-off-by: Lorenzo Bianconi <[email protected]>
---
Documentation/netlink/specs/nfsd.yaml | 28 +++++++
fs/nfsd/netlink.c | 18 +++++
fs/nfsd/netlink.h | 3 +
fs/nfsd/nfsctl.c | 104 ++++++++++++++++++++++++--
include/uapi/linux/nfsd_netlink.h | 10 +++
tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
tools/net/ynl/generated/nfsd-user.h | 54 +++++++++++++
7 files changed, 291 insertions(+), 7 deletions(-)

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index 6c5e42bb20f6..1c342ad3c5fa 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -80,6 +80,15 @@ attribute-sets:
-
name: status
type: u8
+ -
+ name: server-listener
+ attributes:
+ -
+ name: transport-name
+ type: string
+ -
+ name: port
+ type: u32

operations:
list:
@@ -142,3 +151,22 @@ operations:
attributes:
- major
- minor
+ -
+ name: listener-start
+ doc: start server listener
+ attribute-set: server-listener
+ flags: [ admin-perm ]
+ do:
+ request:
+ attributes:
+ - transport-name
+ - port
+ -
+ name: listener-get
+ doc: dump server listeners
+ attribute-set: server-listener
+ dump:
+ reply:
+ attributes:
+ - transport-name
+ - port
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 0608a7bd193b..cd51393ede72 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -22,6 +22,12 @@ static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
[NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
};

+/* NFSD_CMD_LISTENER_START - do */
+static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
+ [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
+ [NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
+};
+
/* Ops table for nfsd */
static const struct genl_split_ops nfsd_nl_ops[] = {
{
@@ -55,6 +61,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
.dumpit = nfsd_nl_version_get_dumpit,
.flags = GENL_CMD_CAP_DUMP,
},
+ {
+ .cmd = NFSD_CMD_LISTENER_START,
+ .doit = nfsd_nl_listener_start_doit,
+ .policy = nfsd_listener_start_nl_policy,
+ .maxattr = NFSD_A_SERVER_LISTENER_PORT,
+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+ },
+ {
+ .cmd = NFSD_CMD_LISTENER_GET,
+ .dumpit = nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -21,6 +21,9 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb);
+int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -721,18 +721,16 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
* A transport listener is added by writing its transport name and
* a port number.
*/
-static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
+static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
+ const char *transport, const int port)
{
- char transport[16];
- struct svc_xprt *xprt;
- int port, err;
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
-
- if (sscanf(buf, "%15s %5u", transport, &port) != 2)
- return -EINVAL;
+ struct svc_xprt *xprt;
+ int err;

if (port < 1 || port > USHRT_MAX)
return -EINVAL;
+
trace_nfsd_ctl_ports_addxprt(net, transport, port);

err = nfsd_create_serv(net);
@@ -765,6 +763,17 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
return err;
}

+static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
+{
+ char transport[16];
+ int port;
+
+ if (sscanf(buf, "%15s %5u", transport, &port) != 2)
+ return -EINVAL;
+
+ return ___write_ports_addxprt(net, cred, transport, port);
+}
+
static ssize_t __write_ports(struct file *file, char *buf, size_t size,
struct net *net)
{
@@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
return ret;
}

+/**
+ * nfsd_nl_listener_start_doit - start the provided nfs server listener
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
+ GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
+ return -EINVAL;
+
+ mutex_lock(&nfsd_mutex);
+ ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
+ nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
+ nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
+ mutex_unlock(&nfsd_mutex);
+
+ return 0;
+}
+
+/**
+ * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
+ * @skb: reply buffer
+ * @cb: netlink metadata and command arguments
+ *
+ * Returns the size of the reply or a negative errno.
+ */
+int nfsd_nl_listener_get_dumpit(struct sk_buff *skb,
+ struct netlink_callback *cb)
+{
+ struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
+ int i = 0, ret = -ENOMEM;
+ struct svc_xprt *xprt;
+ struct svc_serv *serv;
+
+ mutex_lock(&nfsd_mutex);
+
+ serv = nn->nfsd_serv;
+ if (!serv) {
+ mutex_unlock(&nfsd_mutex);
+ return 0;
+ }
+
+ spin_lock_bh(&serv->sv_lock);
+ list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
+ void *hdr;
+
+ if (i < cb->args[0]) /* already consumed */
+ continue;
+
+ hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq, &nfsd_nl_family,
+ 0, NFSD_CMD_LISTENER_GET);
+ if (!hdr)
+ goto out;
+
+ if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
+ xprt->xpt_class->xcl_name))
+ goto out;
+
+ if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
+ svc_xprt_local_port(xprt)))
+ goto out;
+
+ genlmsg_end(skb, hdr);
+ i++;
+ }
+ cb->args[0] = i;
+ ret = skb->len;
+out:
+ spin_unlock_bh(&serv->sv_lock);
+
+ mutex_unlock(&nfsd_mutex);
+
+ return ret;
+}
+
/**
* 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 1b3340f31baa..61f4c5b50ecb 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -45,12 +45,22 @@ enum {
NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
};

+enum {
+ NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
+ NFSD_A_SERVER_LISTENER_PORT,
+
+ __NFSD_A_SERVER_LISTENER_MAX,
+ NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_MAX - 1)
+};
+
enum {
NFSD_CMD_RPC_STATUS_GET = 1,
NFSD_CMD_THREADS_SET,
NFSD_CMD_THREADS_GET,
NFSD_CMD_VERSION_SET,
NFSD_CMD_VERSION_GET,
+ NFSD_CMD_LISTENER_START,
+ NFSD_CMD_LISTENER_GET,

__NFSD_CMD_MAX,
NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
index 4cb71c3cd18d..167e404c9e20 100644
--- a/tools/net/ynl/generated/nfsd-user.c
+++ b/tools/net/ynl/generated/nfsd-user.c
@@ -19,6 +19,8 @@ static const char * const nfsd_op_strmap[] = {
[NFSD_CMD_THREADS_GET] = "threads-get",
[NFSD_CMD_VERSION_SET] = "version-set",
[NFSD_CMD_VERSION_GET] = "version-get",
+ [NFSD_CMD_LISTENER_START] = "listener-start",
+ [NFSD_CMD_LISTENER_GET] = "listener-get",
};

const char *nfsd_op_str(int op)
@@ -71,6 +73,16 @@ struct ynl_policy_nest nfsd_server_version_nest = {
.table = nfsd_server_version_policy,
};

+struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
+ [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
+ [NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
+};
+
+struct ynl_policy_nest nfsd_server_listener_nest = {
+ .max_attr = NFSD_A_SERVER_LISTENER_MAX,
+ .table = nfsd_server_listener_policy,
+};
+
/* Common nested types */
/* ============== NFSD_CMD_RPC_STATUS_GET ============== */
/* NFSD_CMD_RPC_STATUS_GET - dump */
@@ -371,6 +383,75 @@ struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
return NULL;
}

+/* ============== NFSD_CMD_LISTENER_START ============== */
+/* NFSD_CMD_LISTENER_START - do */
+void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
+{
+ free(req->transport_name);
+ free(req);
+}
+
+int nfsd_listener_start(struct ynl_sock *ys,
+ struct nfsd_listener_start_req *req)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
+ ys->req_policy = &nfsd_server_listener_nest;
+
+ if (req->_present.transport_name_len)
+ mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
+ if (req->_present.port)
+ mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
+
+ err = ynl_exec(ys, nlh, NULL);
+ if (err < 0)
+ return -1;
+
+ return 0;
+}
+
+/* ============== NFSD_CMD_LISTENER_GET ============== */
+/* NFSD_CMD_LISTENER_GET - dump */
+void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
+{
+ struct nfsd_listener_get_list *next = rsp;
+
+ while ((void *)next != YNL_LIST_END) {
+ rsp = next;
+ next = rsp->next;
+
+ free(rsp->obj.transport_name);
+ free(rsp);
+ }
+}
+
+struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys)
+{
+ struct ynl_dump_state yds = {};
+ struct nlmsghdr *nlh;
+ int err;
+
+ yds.ys = ys;
+ yds.alloc_sz = sizeof(struct nfsd_listener_get_list);
+ yds.cb = nfsd_listener_get_rsp_parse;
+ yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
+ yds.rsp_policy = &nfsd_server_listener_nest;
+
+ nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
+
+ err = ynl_exec_dump(ys, nlh, &yds);
+ if (err < 0)
+ goto free_list;
+
+ return yds.first;
+
+free_list:
+ nfsd_listener_get_list_free(yds.first);
+ return NULL;
+}
+
const struct ynl_family ynl_nfsd_family = {
.name = "nfsd",
};
diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
index e61c5a9e46fb..da3aaaf3f6c0 100644
--- a/tools/net/ynl/generated/nfsd-user.h
+++ b/tools/net/ynl/generated/nfsd-user.h
@@ -166,4 +166,58 @@ void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);

struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);

+/* ============== NFSD_CMD_LISTENER_START ============== */
+/* NFSD_CMD_LISTENER_START - do */
+struct nfsd_listener_start_req {
+ struct {
+ __u32 transport_name_len;
+ __u32 port:1;
+ } _present;
+
+ char *transport_name;
+ __u32 port;
+};
+
+static inline struct nfsd_listener_start_req *
+nfsd_listener_start_req_alloc(void)
+{
+ return calloc(1, sizeof(struct nfsd_listener_start_req));
+}
+void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
+
+static inline void
+nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
+ const char *transport_name)
+{
+ free(req->transport_name);
+ req->_present.transport_name_len = strlen(transport_name);
+ req->transport_name = malloc(req->_present.transport_name_len + 1);
+ memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
+ req->transport_name[req->_present.transport_name_len] = 0;
+}
+static inline void
+nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
+ __u32 port)
+{
+ req->_present.port = 1;
+ req->port = port;
+}
+
+/*
+ * start server listener
+ */
+int nfsd_listener_start(struct ynl_sock *ys,
+ struct nfsd_listener_start_req *req);
+
+/* ============== NFSD_CMD_LISTENER_GET ============== */
+/* NFSD_CMD_LISTENER_GET - dump */
+struct nfsd_listener_get_list {
+ struct nfsd_listener_get_list *next;
+ struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
+};
+
+void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
+
+struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
+
#endif /* _LINUX_NFSD_GEN_H */
--
2.43.0

2023-11-29 17:13:41

[permalink] [raw]

Subject: [PATCH v5 2/3] NFSD: convert write_version to netlink command

Introduce write_version netlink command similar to the ones available
through the procfs.

Signed-off-by: Lorenzo Bianconi <[email protected]>
---
Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
fs/nfsd/netlink.c | 19 +++++
fs/nfsd/netlink.h | 3 +
fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
include/uapi/linux/nfsd_netlink.h | 11 +++
tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
7 files changed, 306 insertions(+)

diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
index c92e1425d316..6c5e42bb20f6 100644
--- a/Documentation/netlink/specs/nfsd.yaml
+++ b/Documentation/netlink/specs/nfsd.yaml
@@ -68,6 +68,18 @@ attribute-sets:
-
name: threads
type: u32
+ -
+ name: server-version
+ attributes:
+ -
+ name: major
+ type: u32
+ -
+ name: minor
+ type: u32
+ -
+ name: status
+ type: u8

operations:
list:
@@ -110,3 +122,23 @@ operations:
reply:
attributes:
- threads
+ -
+ name: version-set
+ doc: enable/disable server version
+ attribute-set: server-version
+ flags: [ admin-perm ]
+ do:
+ request:
+ attributes:
+ - major
+ - minor
+ - status
+ -
+ name: version-get
+ doc: dump server versions
+ attribute-set: server-version
+ dump:
+ reply:
+ attributes:
+ - major
+ - minor
diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
index 1a59a8e6c7e2..0608a7bd193b 100644
--- a/fs/nfsd/netlink.c
+++ b/fs/nfsd/netlink.c
@@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
[NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
};

+/* NFSD_CMD_VERSION_SET - do */
+static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
+ [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
+ [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
+ [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
+};
+
/* Ops table for nfsd */
static const struct genl_split_ops nfsd_nl_ops[] = {
{
@@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
.doit = nfsd_nl_threads_get_doit,
.flags = GENL_CMD_CAP_DO,
},
+ {
+ .cmd = NFSD_CMD_VERSION_SET,
+ .doit = nfsd_nl_version_set_doit,
+ .policy = nfsd_version_set_nl_policy,
+ .maxattr = NFSD_A_SERVER_VERSION_STATUS,
+ .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
+ },
+ {
+ .cmd = NFSD_CMD_VERSION_GET,
+ .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
--- a/fs/nfsd/netlink.h
+++ b/fs/nfsd/netlink.h
@@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
struct netlink_callback *cb);
int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
+int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
return err;
}

+/**
+ * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
+ * @skb: reply buffer
+ * @info: netlink metadata and command arguments
+ *
+ * Return 0 on success or a negative errno.
+ */
+int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+ struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
+ enum vers_op cmd;
+ u32 major, minor;
+ u8 status;
+ int ret;
+
+ if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
+ GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
+ GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
+ return -EINVAL;
+
+ major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
+ minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
+
+ status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
+ cmd = !!status ? NFSD_SET : NFSD_CLEAR;
+
+ mutex_lock(&nfsd_mutex);
+ switch (major) {
+ case 4:
+ ret = nfsd_minorversion(nn, minor, cmd);
+ break;
+ case 2:
+ case 3:
+ if (!minor) {
+ ret = nfsd_vers(nn, major, cmd);
+ break;
+ }
+ fallthrough;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+ mutex_unlock(&nfsd_mutex);
+
+ return ret;
+}
+
+/**
+ * nfsd_nl_version_get_doit - Handle verion_get dumpit
+ * @skb: reply buffer
+ * @cb: netlink metadata and command arguments
+ *
+ * Returns the size of the reply or a negative errno.
+ */
+int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
+ struct netlink_callback *cb)
+{
+ struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
+ int i, ret = -ENOMEM;
+
+ mutex_lock(&nfsd_mutex);
+
+ for (i = 2; i <= 4; i++) {
+ int j;
+
+ if (i < cb->args[0]) /* already consumed */
+ continue;
+
+ if (!nfsd_vers(nn, i, NFSD_AVAIL))
+ continue;
+
+ for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
+ void *hdr;
+
+ if (!nfsd_vers(nn, i, NFSD_TEST))
+ continue;
+
+ /* NFSv{2,3} does not support minor numbers */
+ if (i < 4 && j)
+ continue;
+
+ if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
+ continue;
+
+ hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
+ cb->nlh->nlmsg_seq, &nfsd_nl_family,
+ 0, NFSD_CMD_VERSION_GET);
+ if (!hdr)
+ goto out;
+
+ if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
+ nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
+ goto out;
+
+ genlmsg_end(skb, hdr);
+ }
+ }
+ cb->args[0] = i;
+ ret = skb->len;
+out:
+ mutex_unlock(&nfsd_mutex);
+
+ return ret;
+}
+
/**
* 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 1b6fe1f9ed0e..1b3340f31baa 100644
--- a/include/uapi/linux/nfsd_netlink.h
+++ b/include/uapi/linux/nfsd_netlink.h
@@ -36,10 +36,21 @@ enum {
NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
};

+enum {
+ NFSD_A_SERVER_VERSION_MAJOR = 1,
+ NFSD_A_SERVER_VERSION_MINOR,
+ NFSD_A_SERVER_VERSION_STATUS,
+
+ __NFSD_A_SERVER_VERSION_MAX,
+ NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
+};
+
enum {
NFSD_CMD_RPC_STATUS_GET = 1,
NFSD_CMD_THREADS_SET,
NFSD_CMD_THREADS_GET,
+ NFSD_CMD_VERSION_SET,
+ NFSD_CMD_VERSION_GET,

__NFSD_CMD_MAX,
NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
index 9768328a7751..4cb71c3cd18d 100644
--- a/tools/net/ynl/generated/nfsd-user.c
+++ b/tools/net/ynl/generated/nfsd-user.c
@@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
[NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
[NFSD_CMD_THREADS_SET] = "threads-set",
[NFSD_CMD_THREADS_GET] = "threads-get",
+ [NFSD_CMD_VERSION_SET] = "version-set",
+ [NFSD_CMD_VERSION_GET] = "version-get",
};

const char *nfsd_op_str(int op)
@@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
.table = nfsd_server_worker_policy,
};

+struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
+ [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
+ [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
+ [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
+};
+
+struct ynl_policy_nest nfsd_server_version_nest = {
+ .max_attr = NFSD_A_SERVER_VERSION_MAX,
+ .table = nfsd_server_version_policy,
+};
+
/* Common nested types */
/* ============== NFSD_CMD_RPC_STATUS_GET ============== */
/* NFSD_CMD_RPC_STATUS_GET - dump */
@@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
return NULL;
}

+/* ============== NFSD_CMD_VERSION_SET ============== */
+/* NFSD_CMD_VERSION_SET - do */
+void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
+{
+ free(req);
+}
+
+int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
+{
+ struct nlmsghdr *nlh;
+ int err;
+
+ nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
+ ys->req_policy = &nfsd_server_version_nest;
+
+ if (req->_present.major)
+ mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
+ if (req->_present.minor)
+ mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
+ if (req->_present.status)
+ mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
+
+ err = ynl_exec(ys, nlh, NULL);
+ if (err < 0)
+ return -1;
+
+ return 0;
+}
+
+/* ============== NFSD_CMD_VERSION_GET ============== */
+/* NFSD_CMD_VERSION_GET - dump */
+void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
+{
+ struct nfsd_version_get_list *next = rsp;
+
+ while ((void *)next != YNL_LIST_END) {
+ rsp = next;
+ next = rsp->next;
+
+ free(rsp);
+ }
+}
+
+struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
+{
+ struct ynl_dump_state yds = {};
+ struct nlmsghdr *nlh;
+ int err;
+
+ yds.ys = ys;
+ yds.alloc_sz = sizeof(struct nfsd_version_get_list);
+ yds.cb = nfsd_version_get_rsp_parse;
+ yds.rsp_cmd = NFSD_CMD_VERSION_GET;
+ yds.rsp_policy = &nfsd_server_version_nest;
+
+ nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
+
+ err = ynl_exec_dump(ys, nlh, &yds);
+ if (err < 0)
+ goto free_list;
+
+ return yds.first;
+
+free_list:
+ nfsd_version_get_list_free(yds.first);
+ return NULL;
+}
+
const struct ynl_family ynl_nfsd_family = {
.name = "nfsd",
};
diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
index e162a4f20d91..e61c5a9e46fb 100644
--- a/tools/net/ynl/generated/nfsd-user.h
+++ b/tools/net/ynl/generated/nfsd-user.h
@@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
*/
struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);

+/* ============== NFSD_CMD_VERSION_SET ============== */
+/* NFSD_CMD_VERSION_SET - do */
+struct nfsd_version_set_req {
+ struct {
+ __u32 major:1;
+ __u32 minor:1;
+ __u32 status:1;
+ } _present;
+
+ __u32 major;
+ __u32 minor;
+ __u8 status;
+};
+
+static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
+{
+ return calloc(1, sizeof(struct nfsd_version_set_req));
+}
+void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
+
+static inline void
+nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
+{
+ req->_present.major = 1;
+ req->major = major;
+}
+static inline void
+nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
+{
+ req->_present.minor = 1;
+ req->minor = minor;
+}
+static inline void
+nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
+{
+ req->_present.status = 1;
+ req->status = status;
+}
+
+/*
+ * enable/disable server version
+ */
+int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
+
+/* ============== NFSD_CMD_VERSION_GET ============== */
+/* NFSD_CMD_VERSION_GET - dump */
+struct nfsd_version_get_list {
+ struct nfsd_version_get_list *next;
+ struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
+};
+
+void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
+
+struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
+
#endif /* _LINUX_NFSD_GEN_H */
--
2.43.0

2023-11-29 18:13:39

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] NFSD: convert write_threads to netlink command

On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> Introduce write_threads netlink command similar to the ones available
> through the procfs.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Documentation/netlink/specs/nfsd.yaml | 23 +++++++
> fs/nfsd/netlink.c | 17 +++++
> fs/nfsd/netlink.h | 2 +
> fs/nfsd/nfsctl.c | 58 +++++++++++++++++
> include/uapi/linux/nfsd_netlink.h | 9 +++
> tools/net/ynl/generated/nfsd-user.c | 92 +++++++++++++++++++++++++++
> tools/net/ynl/generated/nfsd-user.h | 47 ++++++++++++++
> 7 files changed, 248 insertions(+)
>
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 05acc73e2e33..c92e1425d316 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -62,6 +62,12 @@ attribute-sets:
> name: compound-ops
> type: u32
> multi-attr: true
> + -
> + name: server-worker
> + attributes:
> + -
> + name: threads
> + type: u32
>
> operations:
> list:
> @@ -87,3 +93,20 @@ operations:
> - sport
> - dport
> - compound-ops
> + -
> + name: threads-set
> + doc: set the number of running threads
> + attribute-set: server-worker
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - threads
> + -
> + name: threads-get
> + doc: get the number of running threads
> + attribute-set: server-worker
> + do:
> + reply:
> + attributes:
> + - threads
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0e1d635ec5f9..1a59a8e6c7e2 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -10,6 +10,11 @@
>
> #include <uapi/linux/nfsd_netlink.h>
>
> +/* NFSD_CMD_THREADS_SET - do */
> +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_THREADS + 1] = {
> + [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> +};
> +
> /* Ops table for nfsd */
> static const struct genl_split_ops nfsd_nl_ops[] = {
> {
> @@ -19,6 +24,18 @@ 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_WORKER_THREADS,
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
> + {
> + .cmd = NFSD_CMD_THREADS_GET,
> + .doit = nfsd_nl_threads_get_doit,
> + .flags = GENL_CMD_CAP_DO,
> + },
> };
>
> struct genl_family nfsd_nl_family __ro_after_init = {
> diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> index d83dd6bdee92..4137fac477e4 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -16,6 +16,8 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb);
>
> int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb);
> +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
>
> extern struct genl_family nfsd_nl_family;
>
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index d6eeee149370..130b3d937a79 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1699,6 +1699,64 @@ 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 (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_WORKER_THREADS))
> + return -EINVAL;
> +
> + nthreads = nla_get_u32(info->attrs[NFSD_A_SERVER_WORKER_THREADS]);
> + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> +
> + return ret == nthreads ? 0 : ret;
> +}
> +
> +/**
> + * nfsd_nl_threads_get_doit - get the number of running threads
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + void *hdr;
> + int err;
> +
> + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + hdr = genlmsg_iput(skb, info);
> + if (!hdr) {
> + err = -EMSGSIZE;
> + goto err_free_msg;
> + }
> +
> + if (nla_put_u32(skb, NFSD_A_SERVER_WORKER_THREADS,
> + nfsd_nrthreads(genl_info_net(info)))) {
> + err = -EINVAL;
> + goto err_free_msg;
> + }
> +
> + genlmsg_end(skb, hdr);
> +
> + return genlmsg_reply(skb, info);
> +
> +err_free_msg:
> + nlmsg_free(skb);
> + return err;
> +}
> +
> /**
> * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> * @net: a freshly-created network namespace
> diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> index 3cd044edee5d..1b6fe1f9ed0e 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -29,8 +29,17 @@ enum {
> NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
> };
>
> +enum {
> + NFSD_A_SERVER_WORKER_THREADS = 1,
> +
> + __NFSD_A_SERVER_WORKER_MAX,
> + NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> +};
> +
> enum {
> NFSD_CMD_RPC_STATUS_GET = 1,
> + NFSD_CMD_THREADS_SET,
> + NFSD_CMD_THREADS_GET,
>
> __NFSD_CMD_MAX,
> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> index 360b6448c6e9..9768328a7751 100644
> --- a/tools/net/ynl/generated/nfsd-user.c
> +++ b/tools/net/ynl/generated/nfsd-user.c
> @@ -15,6 +15,8 @@
> /* Enums */
> static const char * const nfsd_op_strmap[] = {
> [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> + [NFSD_CMD_THREADS_SET] = "threads-set",
> + [NFSD_CMD_THREADS_GET] = "threads-get",
> };
>
> const char *nfsd_op_str(int op)
> @@ -47,6 +49,15 @@ struct ynl_policy_nest nfsd_rpc_status_nest = {
> .table = nfsd_rpc_status_policy,
> };
>
> +struct ynl_policy_attr nfsd_server_worker_policy[NFSD_A_SERVER_WORKER_MAX + 1] = {
> + [NFSD_A_SERVER_WORKER_THREADS] = { .name = "threads", .type = YNL_PT_U32, },
> +};
> +
> +struct ynl_policy_nest nfsd_server_worker_nest = {
> + .max_attr = NFSD_A_SERVER_WORKER_MAX,
> + .table = nfsd_server_worker_policy,
> +};
> +
> /* Common nested types */
> /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> /* NFSD_CMD_RPC_STATUS_GET - dump */
> @@ -198,6 +209,87 @@ nfsd_rpc_status_get_dump(struct ynl_sock *ys)
> return NULL;
> }
>
> +/* ============== NFSD_CMD_THREADS_SET ============== */
> +/* NFSD_CMD_THREADS_SET - do */
> +void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req)
> +{
> + free(req);
> +}
> +
> +int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req)
> +{
> + struct nlmsghdr *nlh;
> + int err;
> +
> + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_SET, 1);
> + ys->req_policy = &nfsd_server_worker_nest;
> +
> + if (req->_present.threads)
> + mnl_attr_put_u32(nlh, NFSD_A_SERVER_WORKER_THREADS, req->threads);
> +
> + err = ynl_exec(ys, nlh, NULL);
> + if (err < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/* ============== NFSD_CMD_THREADS_GET ============== */
> +/* NFSD_CMD_THREADS_GET - do */
> +void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp)
> +{
> + free(rsp);
> +}
> +
> +int nfsd_threads_get_rsp_parse(const struct nlmsghdr *nlh, void *data)
> +{
> + struct ynl_parse_arg *yarg = data;
> + struct nfsd_threads_get_rsp *dst;
> + const struct nlattr *attr;
> +
> + dst = yarg->data;
> +
> + mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
> + unsigned int type = mnl_attr_get_type(attr);
> +
> + if (type == NFSD_A_SERVER_WORKER_THREADS) {
> + if (ynl_attr_validate(yarg, attr))
> + return MNL_CB_ERROR;
> + dst->_present.threads = 1;
> + dst->threads = mnl_attr_get_u32(attr);
> + }
> + }
> +
> + return MNL_CB_OK;
> +}
> +
> +struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> +{
> + struct ynl_req_state yrs = { .yarg = { .ys = ys, }, };
> + struct nfsd_threads_get_rsp *rsp;
> + struct nlmsghdr *nlh;
> + int err;
> +
> + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_GET, 1);
> + ys->req_policy = &nfsd_server_worker_nest;
> + yrs.yarg.rsp_policy = &nfsd_server_worker_nest;
> +
> + rsp = calloc(1, sizeof(*rsp));
> + yrs.yarg.data = rsp;
> + yrs.cb = nfsd_threads_get_rsp_parse;
> + yrs.rsp_cmd = NFSD_CMD_THREADS_GET;
> +
> + err = ynl_exec(ys, nlh, &yrs);
> + if (err < 0)
> + goto err_free;
> +
> + return rsp;
> +
> +err_free:
> + nfsd_threads_get_rsp_free(rsp);
> + return NULL;
> +}
> +
> const struct ynl_family ynl_nfsd_family = {
> .name = "nfsd",
> };
> diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> index 989c6e209ced..e162a4f20d91 100644
> --- a/tools/net/ynl/generated/nfsd-user.h
> +++ b/tools/net/ynl/generated/nfsd-user.h
> @@ -64,4 +64,51 @@ nfsd_rpc_status_get_rsp_list_free(struct nfsd_rpc_status_get_rsp_list *rsp);
> struct nfsd_rpc_status_get_rsp_list *
> nfsd_rpc_status_get_dump(struct ynl_sock *ys);
>
> +/* ============== NFSD_CMD_THREADS_SET ============== */
> +/* NFSD_CMD_THREADS_SET - do */
> +struct nfsd_threads_set_req {
> + struct {
> + __u32 threads:1;
> + } _present;
> +
> + __u32 threads;
> +};
> +
> +static inline struct nfsd_threads_set_req *nfsd_threads_set_req_alloc(void)
> +{
> + return calloc(1, sizeof(struct nfsd_threads_set_req));
> +}
> +void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req);
> +
> +static inline void
> +nfsd_threads_set_req_set_threads(struct nfsd_threads_set_req *req,
> + __u32 threads)
> +{
> + req->_present.threads = 1;
> + req->threads = threads;
> +}
> +
> +/*
> + * set the number of running threads
> + */
> +int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req);
> +
> +/* ============== NFSD_CMD_THREADS_GET ============== */
> +/* NFSD_CMD_THREADS_GET - do */
> +
> +struct nfsd_threads_get_rsp {
> + struct {
> + __u32 threads:1;
> + } _present;
> +
> + __u32 threads;
> +};
> +
> +void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> +
> +/*
> + * get the number of running threads
> + */
> +struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> +
> #endif /* _LINUX_NFSD_GEN_H */

Ok, Uncle! I'll relent on the quest for a single netlink command to
start the server. This patch looks reasonable to me.

Reviewed-by: Jeff Layton <[email protected]>

2023-11-29 18:23:41

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> Introduce write_version netlink command similar to the ones available
> through the procfs.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> fs/nfsd/netlink.c | 19 +++++
> fs/nfsd/netlink.h | 3 +
> fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> include/uapi/linux/nfsd_netlink.h | 11 +++
> tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> 7 files changed, 306 insertions(+)
>
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index c92e1425d316..6c5e42bb20f6 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -68,6 +68,18 @@ attribute-sets:
> -
> name: threads
> type: u32
> + -
> + name: server-version
> + attributes:
> + -
> + name: major
> + type: u32
> + -
> + name: minor
> + type: u32
> + -
> + name: status
> + type: u8
>
> operations:
> list:
> @@ -110,3 +122,23 @@ operations:
> reply:
> attributes:
> - threads
> + -
> + name: version-set
> + doc: enable/disable server version
> + attribute-set: server-version
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - major
> + - minor
> + - status
> + -
> + name: version-get
> + doc: dump server versions
> + attribute-set: server-version
> + dump:
> + reply:
> + attributes:
> + - major
> + - minor
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 1a59a8e6c7e2..0608a7bd193b 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> };
>
> +/* NFSD_CMD_VERSION_SET - do */
> +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> +};
> +
> /* Ops table for nfsd */
> static const struct genl_split_ops nfsd_nl_ops[] = {
> {
> @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> .doit = nfsd_nl_threads_get_doit,
> .flags = GENL_CMD_CAP_DO,
> },
> + {
> + .cmd = NFSD_CMD_VERSION_SET,
> + .doit = nfsd_nl_version_set_doit,
> + .policy = nfsd_version_set_nl_policy,
> + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
> + {
> + .cmd = NFSD_CMD_VERSION_GET,
> + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb);
> int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> return err;
> }
>
> +/**
> + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> + enum vers_op cmd;
> + u32 major, minor;
> + u8 status;
> + int ret;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> + return -EINVAL;
> +
> + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> +
> + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> +
> + mutex_lock(&nfsd_mutex);
> + switch (major) {
> + case 4:
> + ret = nfsd_minorversion(nn, minor, cmd);
> + break;
> + case 2:
> + case 3:
> + if (!minor) {
> + ret = nfsd_vers(nn, major, cmd);
> + break;
> + }
> + fallthrough;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + mutex_unlock(&nfsd_mutex);
> +
> + return ret;
> +}
> +
> +/**
> + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb)
> +{
> + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> + int i, ret = -ENOMEM;
> +
> + mutex_lock(&nfsd_mutex);
> +
> + for (i = 2; i <= 4; i++) {
> + int j;
> +
> + if (i < cb->args[0]) /* already consumed */
> + continue;
> +
> + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> + continue;
> +
> + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> + void *hdr;
> +
> + if (!nfsd_vers(nn, i, NFSD_TEST))
> + continue;
> +
> + /* NFSv{2,3} does not support minor numbers */
> + if (i < 4 && j)
> + continue;
> +
> + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> + continue;
> +
> + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> + 0, NFSD_CMD_VERSION_GET);
> + if (!hdr)
> + goto out;
> +
> + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> + goto out;
> +
> + genlmsg_end(skb, hdr);
> + }
> + }
> + cb->args[0] = i;
> + ret = skb->len;
> +out:
> + mutex_unlock(&nfsd_mutex);
> +
> + return ret;
> +}
> +
> /**
> * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -36,10 +36,21 @@ enum {
> NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> };
>
> +enum {
> + NFSD_A_SERVER_VERSION_MAJOR = 1,
> + NFSD_A_SERVER_VERSION_MINOR,
> + NFSD_A_SERVER_VERSION_STATUS,
> +
> + __NFSD_A_SERVER_VERSION_MAX,
> + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> +};
> +
> enum {
> NFSD_CMD_RPC_STATUS_GET = 1,
> NFSD_CMD_THREADS_SET,
> NFSD_CMD_THREADS_GET,
> + NFSD_CMD_VERSION_SET,
> + NFSD_CMD_VERSION_GET,
>
> __NFSD_CMD_MAX,
> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> index 9768328a7751..4cb71c3cd18d 100644
> --- a/tools/net/ynl/generated/nfsd-user.c
> +++ b/tools/net/ynl/generated/nfsd-user.c
> @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> [NFSD_CMD_THREADS_SET] = "threads-set",
> [NFSD_CMD_THREADS_GET] = "threads-get",
> + [NFSD_CMD_VERSION_SET] = "version-set",
> + [NFSD_CMD_VERSION_GET] = "version-get",
> };
>
> const char *nfsd_op_str(int op)
> @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> .table = nfsd_server_worker_policy,
> };
>
> +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> +};
> +
> +struct ynl_policy_nest nfsd_server_version_nest = {
> + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> + .table = nfsd_server_version_policy,
> +};
> +
> /* Common nested types */
> /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> /* NFSD_CMD_RPC_STATUS_GET - dump */
> @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> return NULL;
> }
>
> +/* ============== NFSD_CMD_VERSION_SET ============== */
> +/* NFSD_CMD_VERSION_SET - do */
> +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> +{
> + free(req);
> +}
> +
> +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> +{
> + struct nlmsghdr *nlh;
> + int err;
> +
> + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> + ys->req_policy = &nfsd_server_version_nest;
> +
> + if (req->_present.major)
> + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> + if (req->_present.minor)
> + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> + if (req->_present.status)
> + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> +
> + err = ynl_exec(ys, nlh, NULL);
> + if (err < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/* ============== NFSD_CMD_VERSION_GET ============== */
> +/* NFSD_CMD_VERSION_GET - dump */
> +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> +{
> + struct nfsd_version_get_list *next = rsp;
> +
> + while ((void *)next != YNL_LIST_END) {
> + rsp = next;
> + next = rsp->next;
> +
> + free(rsp);
> + }
> +}
> +
> +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> +{
> + struct ynl_dump_state yds = {};
> + struct nlmsghdr *nlh;
> + int err;
> +
> + yds.ys = ys;
> + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> + yds.cb = nfsd_version_get_rsp_parse;
> + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> + yds.rsp_policy = &nfsd_server_version_nest;
> +
> + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> +
> + err = ynl_exec_dump(ys, nlh, &yds);
> + if (err < 0)
> + goto free_list;
> +
> + return yds.first;
> +
> +free_list:
> + nfsd_version_get_list_free(yds.first);
> + return NULL;
> +}
> +
> const struct ynl_family ynl_nfsd_family = {
> .name = "nfsd",
> };
> diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> index e162a4f20d91..e61c5a9e46fb 100644
> --- a/tools/net/ynl/generated/nfsd-user.h
> +++ b/tools/net/ynl/generated/nfsd-user.h
> @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> */
> struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
>
> +/* ============== NFSD_CMD_VERSION_SET ============== */
> +/* NFSD_CMD_VERSION_SET - do */
> +struct nfsd_version_set_req {
> + struct {
> + __u32 major:1;
> + __u32 minor:1;
> + __u32 status:1;
> + } _present;
> +
> + __u32 major;
> + __u32 minor;
> + __u8 status;
> +};
> +

This more or less mirrors how the "versions" file works today, but that
interface is quite klunky.? We don't have a use case that requires that
we do this piecemeal like this. I think we'd be better served with a
more declarative interface that reconfigures the supported versions in
one shot:

Instead of having "major,minor,status" and potentially having to call
this command several times from userland, it seems like it would be
nicer to just have userland send down a list "major,minor" that should
be enabled, and then just let the kernel figure out whether to enable or
disable each. An empty list could mean "disable everything".

That's simpler to reason out as an interface from userland too. Trying
to keep track of the enabled and disabled versions and twiddle it is
really tricky in rpc.nfsd today.

> +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> +{
> + return calloc(1, sizeof(struct nfsd_version_set_req));
> +}
> +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> +
> +static inline void
> +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> +{
> + req->_present.major = 1;
> + req->major = major;
> +}
> +static inline void
> +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> +{
> + req->_present.minor = 1;
> + req->minor = minor;
> +}
> +static inline void
> +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> +{
> + req->_present.status = 1;
> + req->status = status;
> +}
> +
> +/*
> + * enable/disable server version
> + */
> +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> +
> +/* ============== NFSD_CMD_VERSION_GET ============== */
> +/* NFSD_CMD_VERSION_GET - dump */
> +struct nfsd_version_get_list {
> + struct nfsd_version_get_list *next;
> + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> +};
> +
> +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> +
> +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> +
> #endif /* _LINUX_NFSD_GEN_H */

--
Jeff Layton <[email protected]>

2023-11-29 18:28:16

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> Introduce write_ports netlink command similar to the ones available
> through the procfs.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> Documentation/netlink/specs/nfsd.yaml | 28 +++++++
> fs/nfsd/netlink.c | 18 +++++
> fs/nfsd/netlink.h | 3 +
> fs/nfsd/nfsctl.c | 104 ++++++++++++++++++++++++--
> include/uapi/linux/nfsd_netlink.h | 10 +++
> tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> tools/net/ynl/generated/nfsd-user.h | 54 +++++++++++++
> 7 files changed, 291 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> index 6c5e42bb20f6..1c342ad3c5fa 100644
> --- a/Documentation/netlink/specs/nfsd.yaml
> +++ b/Documentation/netlink/specs/nfsd.yaml
> @@ -80,6 +80,15 @@ attribute-sets:
> -
> name: status
> type: u8
> + -
> + name: server-listener
> + attributes:
> + -
> + name: transport-name
> + type: string
> + -
> + name: port
> + type: u32
>
> operations:
> list:
> @@ -142,3 +151,22 @@ operations:
> attributes:
> - major
> - minor
> + -
> + name: listener-start
> + doc: start server listener
> + attribute-set: server-listener
> + flags: [ admin-perm ]
> + do:
> + request:
> + attributes:
> + - transport-name
> + - port
> + -
> + name: listener-get
> + doc: dump server listeners
> + attribute-set: server-listener
> + dump:
> + reply:
> + attributes:
> + - transport-name
> + - port
> diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> index 0608a7bd193b..cd51393ede72 100644
> --- a/fs/nfsd/netlink.c
> +++ b/fs/nfsd/netlink.c
> @@ -22,6 +22,12 @@ static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
> [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> };
>
> +/* NFSD_CMD_LISTENER_START - do */
> +static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
> + [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
> + [NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
> +};
> +
> /* Ops table for nfsd */
> static const struct genl_split_ops nfsd_nl_ops[] = {
> {
> @@ -55,6 +61,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> .dumpit = nfsd_nl_version_get_dumpit,
> .flags = GENL_CMD_CAP_DUMP,
> },
> + {
> + .cmd = NFSD_CMD_LISTENER_START,
> + .doit = nfsd_nl_listener_start_doit,
> + .policy = nfsd_listener_start_nl_policy,
> + .maxattr = NFSD_A_SERVER_LISTENER_PORT,
> + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> + },
> + {
> + .cmd = NFSD_CMD_LISTENER_GET,
> + .dumpit = nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
> --- a/fs/nfsd/netlink.h
> +++ b/fs/nfsd/netlink.h
> @@ -21,6 +21,9 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> struct netlink_callback *cb);
> +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
> +int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -721,18 +721,16 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> * A transport listener is added by writing its transport name and
> * a port number.
> */
> -static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> +static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
> + const char *transport, const int port)
> {
> - char transport[16];
> - struct svc_xprt *xprt;
> - int port, err;
> struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> -
> - if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> - return -EINVAL;
> + struct svc_xprt *xprt;
> + int err;
>
> if (port < 1 || port > USHRT_MAX)
> return -EINVAL;
> +
> trace_nfsd_ctl_ports_addxprt(net, transport, port);
>
> err = nfsd_create_serv(net);
> @@ -765,6 +763,17 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> return err;
> }
>
> +static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> +{
> + char transport[16];
> + int port;
> +
> + if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> + return -EINVAL;
> +
> + return ___write_ports_addxprt(net, cred, transport, port);
> +}
> +
> static ssize_t __write_ports(struct file *file, char *buf, size_t size,
> struct net *net)
> {
> @@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> return ret;
> }
>
> +/**
> + * nfsd_nl_listener_start_doit - start the provided nfs server listener
> + * @skb: reply buffer
> + * @info: netlink metadata and command arguments
> + *
> + * Return 0 on success or a negative errno.
> + */
> +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> + int ret;
> +
> + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
> + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
> + return -EINVAL;
> +
> + mutex_lock(&nfsd_mutex);
> + ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
> + nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
> + nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
> + mutex_unlock(&nfsd_mutex);
> +
> + return 0;
> +}
> +
> +/**
> + * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
> + * @skb: reply buffer
> + * @cb: netlink metadata and command arguments
> + *
> + * Returns the size of the reply or a negative errno.
> + */
> +int nfsd_nl_listener_get_dumpit(struct sk_buff *skb,
> + struct netlink_callback *cb)
> +{
> + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> + int i = 0, ret = -ENOMEM;
> + struct svc_xprt *xprt;
> + struct svc_serv *serv;
> +
> + mutex_lock(&nfsd_mutex);
> +
> + serv = nn->nfsd_serv;
> + if (!serv) {
> + mutex_unlock(&nfsd_mutex);
> + return 0;
> + }
> +
> + spin_lock_bh(&serv->sv_lock);
> + list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
> + void *hdr;
> +
> + if (i < cb->args[0]) /* already consumed */
> + continue;
> +
> + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> + 0, NFSD_CMD_LISTENER_GET);
> + if (!hdr)
> + goto out;
> +
> + if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
> + xprt->xpt_class->xcl_name))
> + goto out;
> +
> + if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
> + svc_xprt_local_port(xprt)))
> + goto out;
> +
> + genlmsg_end(skb, hdr);
> + i++;
> + }
> + cb->args[0] = i;
> + ret = skb->len;
> +out:
> + spin_unlock_bh(&serv->sv_lock);
> +
> + mutex_unlock(&nfsd_mutex);
> +
> + return ret;
> +}
> +
> /**
> * 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 1b3340f31baa..61f4c5b50ecb 100644
> --- a/include/uapi/linux/nfsd_netlink.h
> +++ b/include/uapi/linux/nfsd_netlink.h
> @@ -45,12 +45,22 @@ enum {
> NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> };
>
> +enum {
> + NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
> + NFSD_A_SERVER_LISTENER_PORT,
> +
> + __NFSD_A_SERVER_LISTENER_MAX,
> + NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_MAX - 1)
> +};
> +
> enum {
> NFSD_CMD_RPC_STATUS_GET = 1,
> NFSD_CMD_THREADS_SET,
> NFSD_CMD_THREADS_GET,
> NFSD_CMD_VERSION_SET,
> NFSD_CMD_VERSION_GET,
> + NFSD_CMD_LISTENER_START,
> + NFSD_CMD_LISTENER_GET,
>
> __NFSD_CMD_MAX,
> NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> index 4cb71c3cd18d..167e404c9e20 100644
> --- a/tools/net/ynl/generated/nfsd-user.c
> +++ b/tools/net/ynl/generated/nfsd-user.c
> @@ -19,6 +19,8 @@ static const char * const nfsd_op_strmap[] = {
> [NFSD_CMD_THREADS_GET] = "threads-get",
> [NFSD_CMD_VERSION_SET] = "version-set",
> [NFSD_CMD_VERSION_GET] = "version-get",
> + [NFSD_CMD_LISTENER_START] = "listener-start",
> + [NFSD_CMD_LISTENER_GET] = "listener-get",
> };
>
> const char *nfsd_op_str(int op)
> @@ -71,6 +73,16 @@ struct ynl_policy_nest nfsd_server_version_nest = {
> .table = nfsd_server_version_policy,
> };
>
> +struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
> + [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
> + [NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
> +};
> +
> +struct ynl_policy_nest nfsd_server_listener_nest = {
> + .max_attr = NFSD_A_SERVER_LISTENER_MAX,
> + .table = nfsd_server_listener_policy,
> +};
> +
> /* Common nested types */
> /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> /* NFSD_CMD_RPC_STATUS_GET - dump */
> @@ -371,6 +383,75 @@ struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> return NULL;
> }
>
> +/* ============== NFSD_CMD_LISTENER_START ============== */
> +/* NFSD_CMD_LISTENER_START - do */
> +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
> +{
> + free(req->transport_name);
> + free(req);
> +}
> +
> +int nfsd_listener_start(struct ynl_sock *ys,
> + struct nfsd_listener_start_req *req)
> +{
> + struct nlmsghdr *nlh;
> + int err;
> +
> + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
> + ys->req_policy = &nfsd_server_listener_nest;
> +
> + if (req->_present.transport_name_len)
> + mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
> + if (req->_present.port)
> + mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
> +
> + err = ynl_exec(ys, nlh, NULL);
> + if (err < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/* ============== NFSD_CMD_LISTENER_GET ============== */
> +/* NFSD_CMD_LISTENER_GET - dump */
> +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
> +{
> + struct nfsd_listener_get_list *next = rsp;
> +
> + while ((void *)next != YNL_LIST_END) {
> + rsp = next;
> + next = rsp->next;
> +
> + free(rsp->obj.transport_name);
> + free(rsp);
> + }
> +}
> +
> +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys)
> +{
> + struct ynl_dump_state yds = {};
> + struct nlmsghdr *nlh;
> + int err;
> +
> + yds.ys = ys;
> + yds.alloc_sz = sizeof(struct nfsd_listener_get_list);
> + yds.cb = nfsd_listener_get_rsp_parse;
> + yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
> + yds.rsp_policy = &nfsd_server_listener_nest;
> +
> + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
> +
> + err = ynl_exec_dump(ys, nlh, &yds);
> + if (err < 0)
> + goto free_list;
> +
> + return yds.first;
> +
> +free_list:
> + nfsd_listener_get_list_free(yds.first);
> + return NULL;
> +}
> +
> const struct ynl_family ynl_nfsd_family = {
> .name = "nfsd",
> };
> diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> index e61c5a9e46fb..da3aaaf3f6c0 100644
> --- a/tools/net/ynl/generated/nfsd-user.h
> +++ b/tools/net/ynl/generated/nfsd-user.h
> @@ -166,4 +166,58 @@ void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
>
> struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
>
> +/* ============== NFSD_CMD_LISTENER_START ============== */
> +/* NFSD_CMD_LISTENER_START - do */
> +struct nfsd_listener_start_req {
> + struct {
> + __u32 transport_name_len;
> + __u32 port:1;
> + } _present;
> +
> + char *transport_name;
> + __u32 port;
> +};

How do you deconfigure a listener with this interface? i.e. suppose I
want to stop nfsd from listening on a particular port? I think this too
is a place where a declarative interface would be better:

Have userland send down a list of the ports that we should currently be
listening on, and let the kernel do the work to match the request. Again
too, an empty list could mean "close everything".

> +
> +static inline struct nfsd_listener_start_req *
> +nfsd_listener_start_req_alloc(void)
> +{
> + return calloc(1, sizeof(struct nfsd_listener_start_req));
> +}
> +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
> +
> +static inline void
> +nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
> + const char *transport_name)
> +{
> + free(req->transport_name);
> + req->_present.transport_name_len = strlen(transport_name);
> + req->transport_name = malloc(req->_present.transport_name_len + 1);
> + memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
> + req->transport_name[req->_present.transport_name_len] = 0;
> +}
> +static inline void
> +nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
> + __u32 port)
> +{
> + req->_present.port = 1;
> + req->port = port;
> +}
> +
> +/*
> + * start server listener
> + */
> +int nfsd_listener_start(struct ynl_sock *ys,
> + struct nfsd_listener_start_req *req);
> +
> +/* ============== NFSD_CMD_LISTENER_GET ============== */
> +/* NFSD_CMD_LISTENER_GET - dump */
> +struct nfsd_listener_get_list {
> + struct nfsd_listener_get_list *next;
> + struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
> +};
> +
> +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
> +
> +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
> +
> #endif /* _LINUX_NFSD_GEN_H */

--
Jeff Layton <[email protected]>

2023-11-29 18:31:40

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] NFSD: convert write_threads to netlink command

On Wed, Nov 29, 2023 at 01:13:34PM -0500, Jeff Layton wrote:
> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_threads netlink command similar to the ones available
> > through the procfs.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > Documentation/netlink/specs/nfsd.yaml | 23 +++++++
> > fs/nfsd/netlink.c | 17 +++++
> > fs/nfsd/netlink.h | 2 +
> > fs/nfsd/nfsctl.c | 58 +++++++++++++++++
> > include/uapi/linux/nfsd_netlink.h | 9 +++
> > tools/net/ynl/generated/nfsd-user.c | 92 +++++++++++++++++++++++++++
> > tools/net/ynl/generated/nfsd-user.h | 47 ++++++++++++++
> > 7 files changed, 248 insertions(+)
> >
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index 05acc73e2e33..c92e1425d316 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -62,6 +62,12 @@ attribute-sets:
> > name: compound-ops
> > type: u32
> > multi-attr: true
> > + -
> > + name: server-worker
> > + attributes:
> > + -
> > + name: threads
> > + type: u32
> >
> > operations:
> > list:
> > @@ -87,3 +93,20 @@ operations:
> > - sport
> > - dport
> > - compound-ops
> > + -
> > + name: threads-set
> > + doc: set the number of running threads
> > + attribute-set: server-worker
> > + flags: [ admin-perm ]
> > + do:
> > + request:
> > + attributes:
> > + - threads
> > + -
> > + name: threads-get
> > + doc: get the number of running threads
> > + attribute-set: server-worker
> > + do:
> > + reply:
> > + attributes:
> > + - threads
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 0e1d635ec5f9..1a59a8e6c7e2 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -10,6 +10,11 @@
> >
> > #include <uapi/linux/nfsd_netlink.h>
> >
> > +/* NFSD_CMD_THREADS_SET - do */
> > +static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_THREADS + 1] = {
> > + [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > +};
> > +
> > /* Ops table for nfsd */
> > static const struct genl_split_ops nfsd_nl_ops[] = {
> > {
> > @@ -19,6 +24,18 @@ 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_WORKER_THREADS,
> > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > + },
> > + {
> > + .cmd = NFSD_CMD_THREADS_GET,
> > + .doit = nfsd_nl_threads_get_doit,
> > + .flags = GENL_CMD_CAP_DO,
> > + },
> > };
> >
> > struct genl_family nfsd_nl_family __ro_after_init = {
> > diff --git a/fs/nfsd/netlink.h b/fs/nfsd/netlink.h
> > index d83dd6bdee92..4137fac477e4 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -16,6 +16,8 @@ int nfsd_nl_rpc_status_get_done(struct netlink_callback *cb);
> >
> > int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > struct netlink_callback *cb);
> > +int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> >
> > extern struct genl_family nfsd_nl_family;
> >
> > diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> > index d6eeee149370..130b3d937a79 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1699,6 +1699,64 @@ 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 (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_WORKER_THREADS))
> > + return -EINVAL;
> > +
> > + nthreads = nla_get_u32(info->attrs[NFSD_A_SERVER_WORKER_THREADS]);
> > + ret = nfsd_svc(nthreads, genl_info_net(info), get_current_cred());
> > +
> > + return ret == nthreads ? 0 : ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_threads_get_doit - get the number of running threads
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + void *hdr;
> > + int err;
> > +
> > + skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > + if (!skb)
> > + return -ENOMEM;
> > +
> > + hdr = genlmsg_iput(skb, info);
> > + if (!hdr) {
> > + err = -EMSGSIZE;
> > + goto err_free_msg;
> > + }
> > +
> > + if (nla_put_u32(skb, NFSD_A_SERVER_WORKER_THREADS,
> > + nfsd_nrthreads(genl_info_net(info)))) {
> > + err = -EINVAL;
> > + goto err_free_msg;
> > + }
> > +
> > + genlmsg_end(skb, hdr);
> > +
> > + return genlmsg_reply(skb, info);
> > +
> > +err_free_msg:
> > + nlmsg_free(skb);
> > + return err;
> > +}
> > +
> > /**
> > * nfsd_net_init - Prepare the nfsd_net portion of a new net namespace
> > * @net: a freshly-created network namespace
> > diff --git a/include/uapi/linux/nfsd_netlink.h b/include/uapi/linux/nfsd_netlink.h
> > index 3cd044edee5d..1b6fe1f9ed0e 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -29,8 +29,17 @@ enum {
> > NFSD_A_RPC_STATUS_MAX = (__NFSD_A_RPC_STATUS_MAX - 1)
> > };
> >
> > +enum {
> > + NFSD_A_SERVER_WORKER_THREADS = 1,
> > +
> > + __NFSD_A_SERVER_WORKER_MAX,
> > + NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > +};
> > +
> > enum {
> > NFSD_CMD_RPC_STATUS_GET = 1,
> > + NFSD_CMD_THREADS_SET,
> > + NFSD_CMD_THREADS_GET,
> >
> > __NFSD_CMD_MAX,
> > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 360b6448c6e9..9768328a7751 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -15,6 +15,8 @@
> > /* Enums */
> > static const char * const nfsd_op_strmap[] = {
> > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > + [NFSD_CMD_THREADS_SET] = "threads-set",
> > + [NFSD_CMD_THREADS_GET] = "threads-get",
> > };
> >
> > const char *nfsd_op_str(int op)
> > @@ -47,6 +49,15 @@ struct ynl_policy_nest nfsd_rpc_status_nest = {
> > .table = nfsd_rpc_status_policy,
> > };
> >
> > +struct ynl_policy_attr nfsd_server_worker_policy[NFSD_A_SERVER_WORKER_MAX + 1] = {
> > + [NFSD_A_SERVER_WORKER_THREADS] = { .name = "threads", .type = YNL_PT_U32, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_worker_nest = {
> > + .max_attr = NFSD_A_SERVER_WORKER_MAX,
> > + .table = nfsd_server_worker_policy,
> > +};
> > +
> > /* Common nested types */
> > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -198,6 +209,87 @@ nfsd_rpc_status_get_dump(struct ynl_sock *ys)
> > return NULL;
> > }
> >
> > +/* ============== NFSD_CMD_THREADS_SET ============== */
> > +/* NFSD_CMD_THREADS_SET - do */
> > +void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req)
> > +{
> > + free(req);
> > +}
> > +
> > +int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req)
> > +{
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_SET, 1);
> > + ys->req_policy = &nfsd_server_worker_nest;
> > +
> > + if (req->_present.threads)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_WORKER_THREADS, req->threads);
> > +
> > + err = ynl_exec(ys, nlh, NULL);
> > + if (err < 0)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_THREADS_GET ============== */
> > +/* NFSD_CMD_THREADS_GET - do */
> > +void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp)
> > +{
> > + free(rsp);
> > +}
> > +
> > +int nfsd_threads_get_rsp_parse(const struct nlmsghdr *nlh, void *data)
> > +{
> > + struct ynl_parse_arg *yarg = data;
> > + struct nfsd_threads_get_rsp *dst;
> > + const struct nlattr *attr;
> > +
> > + dst = yarg->data;
> > +
> > + mnl_attr_for_each(attr, nlh, sizeof(struct genlmsghdr)) {
> > + unsigned int type = mnl_attr_get_type(attr);
> > +
> > + if (type == NFSD_A_SERVER_WORKER_THREADS) {
> > + if (ynl_attr_validate(yarg, attr))
> > + return MNL_CB_ERROR;
> > + dst->_present.threads = 1;
> > + dst->threads = mnl_attr_get_u32(attr);
> > + }
> > + }
> > +
> > + return MNL_CB_OK;
> > +}
> > +
> > +struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > +{
> > + struct ynl_req_state yrs = { .yarg = { .ys = ys, }, };
> > + struct nfsd_threads_get_rsp *rsp;
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_THREADS_GET, 1);
> > + ys->req_policy = &nfsd_server_worker_nest;
> > + yrs.yarg.rsp_policy = &nfsd_server_worker_nest;
> > +
> > + rsp = calloc(1, sizeof(*rsp));
> > + yrs.yarg.data = rsp;
> > + yrs.cb = nfsd_threads_get_rsp_parse;
> > + yrs.rsp_cmd = NFSD_CMD_THREADS_GET;
> > +
> > + err = ynl_exec(ys, nlh, &yrs);
> > + if (err < 0)
> > + goto err_free;
> > +
> > + return rsp;
> > +
> > +err_free:
> > + nfsd_threads_get_rsp_free(rsp);
> > + return NULL;
> > +}
> > +
> > const struct ynl_family ynl_nfsd_family = {
> > .name = "nfsd",
> > };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index 989c6e209ced..e162a4f20d91 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -64,4 +64,51 @@ nfsd_rpc_status_get_rsp_list_free(struct nfsd_rpc_status_get_rsp_list *rsp);
> > struct nfsd_rpc_status_get_rsp_list *
> > nfsd_rpc_status_get_dump(struct ynl_sock *ys);
> >
> > +/* ============== NFSD_CMD_THREADS_SET ============== */
> > +/* NFSD_CMD_THREADS_SET - do */
> > +struct nfsd_threads_set_req {
> > + struct {
> > + __u32 threads:1;
> > + } _present;
> > +
> > + __u32 threads;
> > +};
> > +
> > +static inline struct nfsd_threads_set_req *nfsd_threads_set_req_alloc(void)
> > +{
> > + return calloc(1, sizeof(struct nfsd_threads_set_req));
> > +}
> > +void nfsd_threads_set_req_free(struct nfsd_threads_set_req *req);
> > +
> > +static inline void
> > +nfsd_threads_set_req_set_threads(struct nfsd_threads_set_req *req,
> > + __u32 threads)
> > +{
> > + req->_present.threads = 1;
> > + req->threads = threads;
> > +}
> > +
> > +/*
> > + * set the number of running threads
> > + */
> > +int nfsd_threads_set(struct ynl_sock *ys, struct nfsd_threads_set_req *req);
> > +
> > +/* ============== NFSD_CMD_THREADS_GET ============== */
> > +/* NFSD_CMD_THREADS_GET - do */
> > +
> > +struct nfsd_threads_get_rsp {
> > + struct {
> > + __u32 threads:1;
> > + } _present;
> > +
> > + __u32 threads;
> > +};
> > +
> > +void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > +
> > +/*
> > + * get the number of running threads
> > + */
> > +struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > +
> > #endif /* _LINUX_NFSD_GEN_H */
>
> Ok, Uncle! I'll relent on the quest for a single netlink command to
> start the server.

I think we can consider a single config/start-up command, but I
don't feel it has to happen as part of this series, unless I'm
missing something critical. It seems like a declarative mechanism
would need some deeper changes in NFSD.

> This patch looks reasonable to me.
>
> Reviewed-by: Jeff Layton <[email protected]>

Thanks, Jeff!

Before applying these, I'd like an Acked-by from Jakub for the
tools/net/ynl modifications, and at least a fresh sanity check from
Jakub on the changes to the nfsd netlink protocol (though I don't
anything problematic so far).

--
Chuck Lever

2023-11-30 00:28:43

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Wed, 29 Nov 2023 18:12:44 +0100 Lorenzo Bianconi wrote:
> + -
> + name: status
> + type: u8

u8? I guess...

> +/**
> + * nfsd_nl_version_get_doit - Handle verion_get dumpit

doesn't match the function name (do -> dump)

> + /* NFSv{2,3} does not support minor numbers */
> + if (i < 4 && j)
> + continue;
> +
> + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> + continue;
> +
> + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> + 0, NFSD_CMD_VERSION_GET);

Why not iput()?

> + if (!hdr)
> + goto out;
> +
> + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> + goto out;
> +
> + genlmsg_end(skb, hdr);
> + }
> + }

2023-11-30 00:32:04

by Jakub Kicinski

[permalink] [raw]

Subject: Re: [PATCH v5 1/3] NFSD: convert write_threads to netlink command

On Wed, 29 Nov 2023 13:31:17 -0500 Chuck Lever wrote:
> Before applying these, I'd like an Acked-by from Jakub for the
> tools/net/ynl modifications, and at least a fresh sanity check from
> Jakub on the changes to the nfsd netlink protocol (though I don't
> anything problematic so far).

I left a couple of suggestions, looks good overall.

2023-11-30 09:45:29

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_version netlink command similar to the ones available
> > through the procfs.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > fs/nfsd/netlink.c | 19 +++++
> > fs/nfsd/netlink.h | 3 +
> > fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > include/uapi/linux/nfsd_netlink.h | 11 +++
> > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > 7 files changed, 306 insertions(+)
> >
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index c92e1425d316..6c5e42bb20f6 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -68,6 +68,18 @@ attribute-sets:
> > -
> > name: threads
> > type: u32
> > + -
> > + name: server-version
> > + attributes:
> > + -
> > + name: major
> > + type: u32
> > + -
> > + name: minor
> > + type: u32
> > + -
> > + name: status
> > + type: u8
> >
> > operations:
> > list:
> > @@ -110,3 +122,23 @@ operations:
> > reply:
> > attributes:
> > - threads
> > + -
> > + name: version-set
> > + doc: enable/disable server version
> > + attribute-set: server-version
> > + flags: [ admin-perm ]
> > + do:
> > + request:
> > + attributes:
> > + - major
> > + - minor
> > + - status
> > + -
> > + name: version-get
> > + doc: dump server versions
> > + attribute-set: server-version
> > + dump:
> > + reply:
> > + attributes:
> > + - major
> > + - minor
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 1a59a8e6c7e2..0608a7bd193b 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > };
> >
> > +/* NFSD_CMD_VERSION_SET - do */
> > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > +};
> > +
> > /* Ops table for nfsd */
> > static const struct genl_split_ops nfsd_nl_ops[] = {
> > {
> > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > .doit = nfsd_nl_threads_get_doit,
> > .flags = GENL_CMD_CAP_DO,
> > },
> > + {
> > + .cmd = NFSD_CMD_VERSION_SET,
> > + .doit = nfsd_nl_version_set_doit,
> > + .policy = nfsd_version_set_nl_policy,
> > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > + },
> > + {
> > + .cmd = NFSD_CMD_VERSION_GET,
> > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > struct netlink_callback *cb);
> > int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > return err;
> > }
> >
> > +/**
> > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > + enum vers_op cmd;
> > + u32 major, minor;
> > + u8 status;
> > + int ret;
> > +
> > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > + return -EINVAL;
> > +
> > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > +
> > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > +
> > + mutex_lock(&nfsd_mutex);
> > + switch (major) {
> > + case 4:
> > + ret = nfsd_minorversion(nn, minor, cmd);
> > + break;
> > + case 2:
> > + case 3:
> > + if (!minor) {
> > + ret = nfsd_vers(nn, major, cmd);
> > + break;
> > + }
> > + fallthrough;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > + int i, ret = -ENOMEM;
> > +
> > + mutex_lock(&nfsd_mutex);
> > +
> > + for (i = 2; i <= 4; i++) {
> > + int j;
> > +
> > + if (i < cb->args[0]) /* already consumed */
> > + continue;
> > +
> > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > + continue;
> > +
> > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > + void *hdr;
> > +
> > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > + continue;
> > +
> > + /* NFSv{2,3} does not support minor numbers */
> > + if (i < 4 && j)
> > + continue;
> > +
> > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > + continue;
> > +
> > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > + 0, NFSD_CMD_VERSION_GET);
> > + if (!hdr)
> > + goto out;
> > +
> > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > + goto out;
> > +
> > + genlmsg_end(skb, hdr);
> > + }
> > + }
> > + cb->args[0] = i;
> > + ret = skb->len;
> > +out:
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -36,10 +36,21 @@ enum {
> > NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > };
> >
> > +enum {
> > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > + NFSD_A_SERVER_VERSION_MINOR,
> > + NFSD_A_SERVER_VERSION_STATUS,
> > +
> > + __NFSD_A_SERVER_VERSION_MAX,
> > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > +};
> > +
> > enum {
> > NFSD_CMD_RPC_STATUS_GET = 1,
> > NFSD_CMD_THREADS_SET,
> > NFSD_CMD_THREADS_GET,
> > + NFSD_CMD_VERSION_SET,
> > + NFSD_CMD_VERSION_GET,
> >
> > __NFSD_CMD_MAX,
> > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 9768328a7751..4cb71c3cd18d 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > [NFSD_CMD_THREADS_SET] = "threads-set",
> > [NFSD_CMD_THREADS_GET] = "threads-get",
> > + [NFSD_CMD_VERSION_SET] = "version-set",
> > + [NFSD_CMD_VERSION_GET] = "version-get",
> > };
> >
> > const char *nfsd_op_str(int op)
> > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > .table = nfsd_server_worker_policy,
> > };
> >
> > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_version_nest = {
> > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > + .table = nfsd_server_version_policy,
> > +};
> > +
> > /* Common nested types */
> > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > return NULL;
> > }
> >
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > +{
> > + free(req);
> > +}
> > +
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > +{
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > + ys->req_policy = &nfsd_server_version_nest;
> > +
> > + if (req->_present.major)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > + if (req->_present.minor)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > + if (req->_present.status)
> > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > +
> > + err = ynl_exec(ys, nlh, NULL);
> > + if (err < 0)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > +{
> > + struct nfsd_version_get_list *next = rsp;
> > +
> > + while ((void *)next != YNL_LIST_END) {
> > + rsp = next;
> > + next = rsp->next;
> > +
> > + free(rsp);
> > + }
> > +}
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > +{
> > + struct ynl_dump_state yds = {};
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + yds.ys = ys;
> > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > + yds.cb = nfsd_version_get_rsp_parse;
> > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > + yds.rsp_policy = &nfsd_server_version_nest;
> > +
> > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > +
> > + err = ynl_exec_dump(ys, nlh, &yds);
> > + if (err < 0)
> > + goto free_list;
> > +
> > + return yds.first;
> > +
> > +free_list:
> > + nfsd_version_get_list_free(yds.first);
> > + return NULL;
> > +}
> > +
> > const struct ynl_family ynl_nfsd_family = {
> > .name = "nfsd",
> > };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index e162a4f20d91..e61c5a9e46fb 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > */
> > struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> >
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +struct nfsd_version_set_req {
> > + struct {
> > + __u32 major:1;
> > + __u32 minor:1;
> > + __u32 status:1;
> > + } _present;
> > +
> > + __u32 major;
> > + __u32 minor;
> > + __u8 status;
> > +};
> > +
>
> This more or less mirrors how the "versions" file works today, but that
> interface is quite klunky.? We don't have a use case that requires that
> we do this piecemeal like this. I think we'd be better served with a
> more declarative interface that reconfigures the supported versions in
> one shot:
>
> Instead of having "major,minor,status" and potentially having to call
> this command several times from userland, it seems like it would be
> nicer to just have userland send down a list "major,minor" that should
> be enabled, and then just let the kernel figure out whether to enable or
> disable each. An empty list could mean "disable everything".
>
> That's simpler to reason out as an interface from userland too. Trying
> to keep track of the enabled and disabled versions and twiddle it is
> really tricky in rpc.nfsd today.

Ack. So far I have just converted the current implementation to netlink
and I have not changed any logic. Anyway I am fine to change the current
logic. Should we have 2 rpc.nfsd version in this case?

Regards,
Lorenzo

>
> > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > +{
> > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > +}
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > +
> > +static inline void
> > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > +{
> > + req->_present.major = 1;
> > + req->major = major;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > +{
> > + req->_present.minor = 1;
> > + req->minor = minor;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > +{
> > + req->_present.status = 1;
> > + req->status = status;
> > +}
> > +
> > +/*
> > + * enable/disable server version
> > + */
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +struct nfsd_version_get_list {
> > + struct nfsd_version_get_list *next;
> > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > +
> > #endif /* _LINUX_NFSD_GEN_H */
>
> --
> Jeff Layton <[email protected]>

Attachments:

(No filename) (14.95 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-30 09:57:19

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_ports netlink command similar to the ones available
> > through the procfs.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > Documentation/netlink/specs/nfsd.yaml | 28 +++++++
> > fs/nfsd/netlink.c | 18 +++++
> > fs/nfsd/netlink.h | 3 +
> > fs/nfsd/nfsctl.c | 104 ++++++++++++++++++++++++--
> > include/uapi/linux/nfsd_netlink.h | 10 +++
> > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > tools/net/ynl/generated/nfsd-user.h | 54 +++++++++++++
> > 7 files changed, 291 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index 6c5e42bb20f6..1c342ad3c5fa 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -80,6 +80,15 @@ attribute-sets:
> > -
> > name: status
> > type: u8
> > + -
> > + name: server-listener
> > + attributes:
> > + -
> > + name: transport-name
> > + type: string
> > + -
> > + name: port
> > + type: u32
> >
> > operations:
> > list:
> > @@ -142,3 +151,22 @@ operations:
> > attributes:
> > - major
> > - minor
> > + -
> > + name: listener-start
> > + doc: start server listener
> > + attribute-set: server-listener
> > + flags: [ admin-perm ]
> > + do:
> > + request:
> > + attributes:
> > + - transport-name
> > + - port
> > + -
> > + name: listener-get
> > + doc: dump server listeners
> > + attribute-set: server-listener
> > + dump:
> > + reply:
> > + attributes:
> > + - transport-name
> > + - port
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 0608a7bd193b..cd51393ede72 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -22,6 +22,12 @@ static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_
> > [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > };
> >
> > +/* NFSD_CMD_LISTENER_START - do */
> > +static const struct nla_policy nfsd_listener_start_nl_policy[NFSD_A_SERVER_LISTENER_PORT + 1] = {
> > + [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .type = NLA_NUL_STRING, },
> > + [NFSD_A_SERVER_LISTENER_PORT] = { .type = NLA_U32, },
> > +};
> > +
> > /* Ops table for nfsd */
> > static const struct genl_split_ops nfsd_nl_ops[] = {
> > {
> > @@ -55,6 +61,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > .dumpit = nfsd_nl_version_get_dumpit,
> > .flags = GENL_CMD_CAP_DUMP,
> > },
> > + {
> > + .cmd = NFSD_CMD_LISTENER_START,
> > + .doit = nfsd_nl_listener_start_doit,
> > + .policy = nfsd_listener_start_nl_policy,
> > + .maxattr = NFSD_A_SERVER_LISTENER_PORT,
> > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > + },
> > + {
> > + .cmd = NFSD_CMD_LISTENER_GET,
> > + .dumpit = nfsd_nl_listener_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 7d203cec08e4..9a51cb83f343 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -21,6 +21,9 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > struct netlink_callback *cb);
> > +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_listener_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 f04430f79687..53129b5b7d3c 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -721,18 +721,16 @@ static ssize_t __write_ports_addfd(char *buf, struct net *net, const struct cred
> > * A transport listener is added by writing its transport name and
> > * a port number.
> > */
> > -static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> > +static ssize_t ___write_ports_addxprt(struct net *net, const struct cred *cred,
> > + const char *transport, const int port)
> > {
> > - char transport[16];
> > - struct svc_xprt *xprt;
> > - int port, err;
> > struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > -
> > - if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> > - return -EINVAL;
> > + struct svc_xprt *xprt;
> > + int err;
> >
> > if (port < 1 || port > USHRT_MAX)
> > return -EINVAL;
> > +
> > trace_nfsd_ctl_ports_addxprt(net, transport, port);
> >
> > err = nfsd_create_serv(net);
> > @@ -765,6 +763,17 @@ static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cr
> > return err;
> > }
> >
> > +static ssize_t __write_ports_addxprt(char *buf, struct net *net, const struct cred *cred)
> > +{
> > + char transport[16];
> > + int port;
> > +
> > + if (sscanf(buf, "%15s %5u", transport, &port) != 2)
> > + return -EINVAL;
> > +
> > + return ___write_ports_addxprt(net, cred, transport, port);
> > +}
> > +
> > static ssize_t __write_ports(struct file *file, char *buf, size_t size,
> > struct net *net)
> > {
> > @@ -1862,6 +1871,87 @@ int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > return ret;
> > }
> >
> > +/**
> > + * nfsd_nl_listener_start_doit - start the provided nfs server listener
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_listener_start_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + int ret;
> > +
> > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME) ||
> > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_LISTENER_PORT))
> > + return -EINVAL;
> > +
> > + mutex_lock(&nfsd_mutex);
> > + ret = ___write_ports_addxprt(genl_info_net(info), get_current_cred(),
> > + nla_data(info->attrs[NFSD_A_SERVER_LISTENER_TRANSPORT_NAME]),
> > + nla_get_u32(info->attrs[NFSD_A_SERVER_LISTENER_PORT]));
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_dumpit - Handle listener_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_listener_get_dumpit(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > + int i = 0, ret = -ENOMEM;
> > + struct svc_xprt *xprt;
> > + struct svc_serv *serv;
> > +
> > + mutex_lock(&nfsd_mutex);
> > +
> > + serv = nn->nfsd_serv;
> > + if (!serv) {
> > + mutex_unlock(&nfsd_mutex);
> > + return 0;
> > + }
> > +
> > + spin_lock_bh(&serv->sv_lock);
> > + list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
> > + void *hdr;
> > +
> > + if (i < cb->args[0]) /* already consumed */
> > + continue;
> > +
> > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > + 0, NFSD_CMD_LISTENER_GET);
> > + if (!hdr)
> > + goto out;
> > +
> > + if (nla_put_string(skb, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME,
> > + xprt->xpt_class->xcl_name))
> > + goto out;
> > +
> > + if (nla_put_u32(skb, NFSD_A_SERVER_LISTENER_PORT,
> > + svc_xprt_local_port(xprt)))
> > + goto out;
> > +
> > + genlmsg_end(skb, hdr);
> > + i++;
> > + }
> > + cb->args[0] = i;
> > + ret = skb->len;
> > +out:
> > + spin_unlock_bh(&serv->sv_lock);
> > +
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * 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 1b3340f31baa..61f4c5b50ecb 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -45,12 +45,22 @@ enum {
> > NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > };
> >
> > +enum {
> > + NFSD_A_SERVER_LISTENER_TRANSPORT_NAME = 1,
> > + NFSD_A_SERVER_LISTENER_PORT,
> > +
> > + __NFSD_A_SERVER_LISTENER_MAX,
> > + NFSD_A_SERVER_LISTENER_MAX = (__NFSD_A_SERVER_LISTENER_MAX - 1)
> > +};
> > +
> > enum {
> > NFSD_CMD_RPC_STATUS_GET = 1,
> > NFSD_CMD_THREADS_SET,
> > NFSD_CMD_THREADS_GET,
> > NFSD_CMD_VERSION_SET,
> > NFSD_CMD_VERSION_GET,
> > + NFSD_CMD_LISTENER_START,
> > + NFSD_CMD_LISTENER_GET,
> >
> > __NFSD_CMD_MAX,
> > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 4cb71c3cd18d..167e404c9e20 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -19,6 +19,8 @@ static const char * const nfsd_op_strmap[] = {
> > [NFSD_CMD_THREADS_GET] = "threads-get",
> > [NFSD_CMD_VERSION_SET] = "version-set",
> > [NFSD_CMD_VERSION_GET] = "version-get",
> > + [NFSD_CMD_LISTENER_START] = "listener-start",
> > + [NFSD_CMD_LISTENER_GET] = "listener-get",
> > };
> >
> > const char *nfsd_op_str(int op)
> > @@ -71,6 +73,16 @@ struct ynl_policy_nest nfsd_server_version_nest = {
> > .table = nfsd_server_version_policy,
> > };
> >
> > +struct ynl_policy_attr nfsd_server_listener_policy[NFSD_A_SERVER_LISTENER_MAX + 1] = {
> > + [NFSD_A_SERVER_LISTENER_TRANSPORT_NAME] = { .name = "transport-name", .type = YNL_PT_NUL_STR, },
> > + [NFSD_A_SERVER_LISTENER_PORT] = { .name = "port", .type = YNL_PT_U32, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_listener_nest = {
> > + .max_attr = NFSD_A_SERVER_LISTENER_MAX,
> > + .table = nfsd_server_listener_policy,
> > +};
> > +
> > /* Common nested types */
> > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -371,6 +383,75 @@ struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > return NULL;
> > }
> >
> > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > +/* NFSD_CMD_LISTENER_START - do */
> > +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req)
> > +{
> > + free(req->transport_name);
> > + free(req);
> > +}
> > +
> > +int nfsd_listener_start(struct ynl_sock *ys,
> > + struct nfsd_listener_start_req *req)
> > +{
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_LISTENER_START, 1);
> > + ys->req_policy = &nfsd_server_listener_nest;
> > +
> > + if (req->_present.transport_name_len)
> > + mnl_attr_put_strz(nlh, NFSD_A_SERVER_LISTENER_TRANSPORT_NAME, req->transport_name);
> > + if (req->_present.port)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_LISTENER_PORT, req->port);
> > +
> > + err = ynl_exec(ys, nlh, NULL);
> > + if (err < 0)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_LISTENER_GET ============== */
> > +/* NFSD_CMD_LISTENER_GET - dump */
> > +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp)
> > +{
> > + struct nfsd_listener_get_list *next = rsp;
> > +
> > + while ((void *)next != YNL_LIST_END) {
> > + rsp = next;
> > + next = rsp->next;
> > +
> > + free(rsp->obj.transport_name);
> > + free(rsp);
> > + }
> > +}
> > +
> > +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys)
> > +{
> > + struct ynl_dump_state yds = {};
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + yds.ys = ys;
> > + yds.alloc_sz = sizeof(struct nfsd_listener_get_list);
> > + yds.cb = nfsd_listener_get_rsp_parse;
> > + yds.rsp_cmd = NFSD_CMD_LISTENER_GET;
> > + yds.rsp_policy = &nfsd_server_listener_nest;
> > +
> > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_LISTENER_GET, 1);
> > +
> > + err = ynl_exec_dump(ys, nlh, &yds);
> > + if (err < 0)
> > + goto free_list;
> > +
> > + return yds.first;
> > +
> > +free_list:
> > + nfsd_listener_get_list_free(yds.first);
> > + return NULL;
> > +}
> > +
> > const struct ynl_family ynl_nfsd_family = {
> > .name = "nfsd",
> > };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index e61c5a9e46fb..da3aaaf3f6c0 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -166,4 +166,58 @@ void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> >
> > struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> >
> > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > +/* NFSD_CMD_LISTENER_START - do */
> > +struct nfsd_listener_start_req {
> > + struct {
> > + __u32 transport_name_len;
> > + __u32 port:1;
> > + } _present;
> > +
> > + char *transport_name;
> > + __u32 port;
> > +};
>
> How do you deconfigure a listener with this interface? i.e. suppose I
> want to stop nfsd from listening on a particular port? I think this too
> is a place where a declarative interface would be better:

Is it possible with current APIs? as for 2/3 so far I have just added netlink
counter for current implementation but I am fine to change the logic here to
better APIs.

Regards,
Lorenzo

>
> Have userland send down a list of the ports that we should currently be
> listening on, and let the kernel do the work to match the request. Again
> too, an empty list could mean "close everything".
>
> > +
> > +static inline struct nfsd_listener_start_req *
> > +nfsd_listener_start_req_alloc(void)
> > +{
> > + return calloc(1, sizeof(struct nfsd_listener_start_req));
> > +}
> > +void nfsd_listener_start_req_free(struct nfsd_listener_start_req *req);
> > +
> > +static inline void
> > +nfsd_listener_start_req_set_transport_name(struct nfsd_listener_start_req *req,
> > + const char *transport_name)
> > +{
> > + free(req->transport_name);
> > + req->_present.transport_name_len = strlen(transport_name);
> > + req->transport_name = malloc(req->_present.transport_name_len + 1);
> > + memcpy(req->transport_name, transport_name, req->_present.transport_name_len);
> > + req->transport_name[req->_present.transport_name_len] = 0;
> > +}
> > +static inline void
> > +nfsd_listener_start_req_set_port(struct nfsd_listener_start_req *req,
> > + __u32 port)
> > +{
> > + req->_present.port = 1;
> > + req->port = port;
> > +}
> > +
> > +/*
> > + * start server listener
> > + */
> > +int nfsd_listener_start(struct ynl_sock *ys,
> > + struct nfsd_listener_start_req *req);
> > +
> > +/* ============== NFSD_CMD_LISTENER_GET ============== */
> > +/* NFSD_CMD_LISTENER_GET - dump */
> > +struct nfsd_listener_get_list {
> > + struct nfsd_listener_get_list *next;
> > + struct nfsd_listener_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_listener_get_list_free(struct nfsd_listener_get_list *rsp);
> > +
> > +struct nfsd_listener_get_list *nfsd_listener_get_dump(struct ynl_sock *ys);
> > +
> > #endif /* _LINUX_NFSD_GEN_H */
>
> --
> Jeff Layton <[email protected]>
>

Attachments:

(No filename) (15.47 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-30 10:32:28

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

> On Wed, 29 Nov 2023 18:12:44 +0100 Lorenzo Bianconi wrote:
> > + -
> > + name: status
> > + type: u8
>
> u8? I guess...

here we need just 0 or 1 I would say. Do you suggest create something like an
enum?

> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
>
> doesn't match the function name (do -> dump)

ack, I will fix it.

>
> > + /* NFSv{2,3} does not support minor numbers */
> > + if (i < 4 && j)
> > + continue;
> > +
> > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > + continue;
> > +
> > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > + 0, NFSD_CMD_VERSION_GET);
>
> Why not iput()?

ack, I will fix it.

Regards,
Lorenzo

>
> > + if (!hdr)
> > + goto out;
> > +
> > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > + goto out;
> > +
> > + genlmsg_end(skb, hdr);
> > + }
> > + }
>

Attachments:

(No filename) (1.04 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-30 16:12:55

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > Introduce write_version netlink command similar to the ones available
> > through the procfs.
> >
> > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > ---
> > Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > fs/nfsd/netlink.c | 19 +++++
> > fs/nfsd/netlink.h | 3 +
> > fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > include/uapi/linux/nfsd_netlink.h | 11 +++
> > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > 7 files changed, 306 insertions(+)
> >
> > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > index c92e1425d316..6c5e42bb20f6 100644
> > --- a/Documentation/netlink/specs/nfsd.yaml
> > +++ b/Documentation/netlink/specs/nfsd.yaml
> > @@ -68,6 +68,18 @@ attribute-sets:
> > -
> > name: threads
> > type: u32
> > + -
> > + name: server-version
> > + attributes:
> > + -
> > + name: major
> > + type: u32
> > + -
> > + name: minor
> > + type: u32
> > + -
> > + name: status
> > + type: u8
> >
> > operations:
> > list:
> > @@ -110,3 +122,23 @@ operations:
> > reply:
> > attributes:
> > - threads
> > + -
> > + name: version-set
> > + doc: enable/disable server version
> > + attribute-set: server-version
> > + flags: [ admin-perm ]
> > + do:
> > + request:
> > + attributes:
> > + - major
> > + - minor
> > + - status
> > + -
> > + name: version-get
> > + doc: dump server versions
> > + attribute-set: server-version
> > + dump:
> > + reply:
> > + attributes:
> > + - major
> > + - minor
> > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > index 1a59a8e6c7e2..0608a7bd193b 100644
> > --- a/fs/nfsd/netlink.c
> > +++ b/fs/nfsd/netlink.c
> > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > };
> >
> > +/* NFSD_CMD_VERSION_SET - do */
> > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > +};
> > +
> > /* Ops table for nfsd */
> > static const struct genl_split_ops nfsd_nl_ops[] = {
> > {
> > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > .doit = nfsd_nl_threads_get_doit,
> > .flags = GENL_CMD_CAP_DO,
> > },
> > + {
> > + .cmd = NFSD_CMD_VERSION_SET,
> > + .doit = nfsd_nl_version_set_doit,
> > + .policy = nfsd_version_set_nl_policy,
> > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > + },
> > + {
> > + .cmd = NFSD_CMD_VERSION_GET,
> > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > --- a/fs/nfsd/netlink.h
> > +++ b/fs/nfsd/netlink.h
> > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > struct netlink_callback *cb);
> > int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > --- a/fs/nfsd/nfsctl.c
> > +++ b/fs/nfsd/nfsctl.c
> > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > return err;
> > }
> >
> > +/**
> > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > + * @skb: reply buffer
> > + * @info: netlink metadata and command arguments
> > + *
> > + * Return 0 on success or a negative errno.
> > + */
> > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > +{
> > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > + enum vers_op cmd;
> > + u32 major, minor;
> > + u8 status;
> > + int ret;
> > +
> > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > + return -EINVAL;
> > +
> > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > +
> > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > +
> > + mutex_lock(&nfsd_mutex);
> > + switch (major) {
> > + case 4:
> > + ret = nfsd_minorversion(nn, minor, cmd);
> > + break;
> > + case 2:
> > + case 3:
> > + if (!minor) {
> > + ret = nfsd_vers(nn, major, cmd);
> > + break;
> > + }
> > + fallthrough;
> > + default:
> > + ret = -EINVAL;
> > + break;
> > + }
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > + * @skb: reply buffer
> > + * @cb: netlink metadata and command arguments
> > + *
> > + * Returns the size of the reply or a negative errno.
> > + */
> > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > + struct netlink_callback *cb)
> > +{
> > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > + int i, ret = -ENOMEM;
> > +
> > + mutex_lock(&nfsd_mutex);
> > +
> > + for (i = 2; i <= 4; i++) {
> > + int j;
> > +
> > + if (i < cb->args[0]) /* already consumed */
> > + continue;
> > +
> > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > + continue;
> > +
> > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > + void *hdr;
> > +
> > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > + continue;
> > +
> > + /* NFSv{2,3} does not support minor numbers */
> > + if (i < 4 && j)
> > + continue;
> > +
> > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > + continue;
> > +
> > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > + 0, NFSD_CMD_VERSION_GET);
> > + if (!hdr)
> > + goto out;
> > +
> > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > + goto out;
> > +
> > + genlmsg_end(skb, hdr);
> > + }
> > + }
> > + cb->args[0] = i;
> > + ret = skb->len;
> > +out:
> > + mutex_unlock(&nfsd_mutex);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > --- a/include/uapi/linux/nfsd_netlink.h
> > +++ b/include/uapi/linux/nfsd_netlink.h
> > @@ -36,10 +36,21 @@ enum {
> > NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > };
> >
> > +enum {
> > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > + NFSD_A_SERVER_VERSION_MINOR,
> > + NFSD_A_SERVER_VERSION_STATUS,
> > +
> > + __NFSD_A_SERVER_VERSION_MAX,
> > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > +};
> > +
> > enum {
> > NFSD_CMD_RPC_STATUS_GET = 1,
> > NFSD_CMD_THREADS_SET,
> > NFSD_CMD_THREADS_GET,
> > + NFSD_CMD_VERSION_SET,
> > + NFSD_CMD_VERSION_GET,
> >
> > __NFSD_CMD_MAX,
> > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > index 9768328a7751..4cb71c3cd18d 100644
> > --- a/tools/net/ynl/generated/nfsd-user.c
> > +++ b/tools/net/ynl/generated/nfsd-user.c
> > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > [NFSD_CMD_THREADS_SET] = "threads-set",
> > [NFSD_CMD_THREADS_GET] = "threads-get",
> > + [NFSD_CMD_VERSION_SET] = "version-set",
> > + [NFSD_CMD_VERSION_GET] = "version-get",
> > };
> >
> > const char *nfsd_op_str(int op)
> > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > .table = nfsd_server_worker_policy,
> > };
> >
> > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > +};
> > +
> > +struct ynl_policy_nest nfsd_server_version_nest = {
> > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > + .table = nfsd_server_version_policy,
> > +};
> > +
> > /* Common nested types */
> > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > return NULL;
> > }
> >
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > +{
> > + free(req);
> > +}
> > +
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > +{
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > + ys->req_policy = &nfsd_server_version_nest;
> > +
> > + if (req->_present.major)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > + if (req->_present.minor)
> > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > + if (req->_present.status)
> > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > +
> > + err = ynl_exec(ys, nlh, NULL);
> > + if (err < 0)
> > + return -1;
> > +
> > + return 0;
> > +}
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > +{
> > + struct nfsd_version_get_list *next = rsp;
> > +
> > + while ((void *)next != YNL_LIST_END) {
> > + rsp = next;
> > + next = rsp->next;
> > +
> > + free(rsp);
> > + }
> > +}
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > +{
> > + struct ynl_dump_state yds = {};
> > + struct nlmsghdr *nlh;
> > + int err;
> > +
> > + yds.ys = ys;
> > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > + yds.cb = nfsd_version_get_rsp_parse;
> > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > + yds.rsp_policy = &nfsd_server_version_nest;
> > +
> > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > +
> > + err = ynl_exec_dump(ys, nlh, &yds);
> > + if (err < 0)
> > + goto free_list;
> > +
> > + return yds.first;
> > +
> > +free_list:
> > + nfsd_version_get_list_free(yds.first);
> > + return NULL;
> > +}
> > +
> > const struct ynl_family ynl_nfsd_family = {
> > .name = "nfsd",
> > };
> > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > index e162a4f20d91..e61c5a9e46fb 100644
> > --- a/tools/net/ynl/generated/nfsd-user.h
> > +++ b/tools/net/ynl/generated/nfsd-user.h
> > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > */
> > struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> >
> > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > +/* NFSD_CMD_VERSION_SET - do */
> > +struct nfsd_version_set_req {
> > + struct {
> > + __u32 major:1;
> > + __u32 minor:1;
> > + __u32 status:1;
> > + } _present;
> > +
> > + __u32 major;
> > + __u32 minor;
> > + __u8 status;
> > +};
> > +
>
> This more or less mirrors how the "versions" file works today, but that
> interface is quite klunky.? We don't have a use case that requires that
> we do this piecemeal like this. I think we'd be better served with a
> more declarative interface that reconfigures the supported versions in
> one shot:
>
> Instead of having "major,minor,status" and potentially having to call
> this command several times from userland, it seems like it would be
> nicer to just have userland send down a list "major,minor" that should
> be enabled, and then just let the kernel figure out whether to enable or
> disable each. An empty list could mean "disable everything".
>
> That's simpler to reason out as an interface from userland too. Trying
> to keep track of the enabled and disabled versions and twiddle it is
> really tricky in rpc.nfsd today.

Jeff and Lorenzo, this sounds to me like a useful and narrow
improvement to this interface, one that should be implemented as
part of this patch series.

Ditto for Jeff's review comment on 3/3.

> > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > +{
> > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > +}
> > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > +
> > +static inline void
> > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > +{
> > + req->_present.major = 1;
> > + req->major = major;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > +{
> > + req->_present.minor = 1;
> > + req->minor = minor;
> > +}
> > +static inline void
> > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > +{
> > + req->_present.status = 1;
> > + req->status = status;
> > +}
> > +
> > +/*
> > + * enable/disable server version
> > + */
> > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > +
> > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > +/* NFSD_CMD_VERSION_GET - dump */
> > +struct nfsd_version_get_list {
> > + struct nfsd_version_get_list *next;
> > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > +};
> > +
> > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > +
> > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > +
> > #endif /* _LINUX_NFSD_GEN_H */
>
> --
> Jeff Layton <[email protected]>
>

--
Chuck Lever

2023-11-30 16:18:01

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Thu, 2023-11-30 at 10:45 +0100, Lorenzo Bianconi wrote:
> > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > Introduce write_version netlink command similar to the ones available
> > > through the procfs.
> > >
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > ---
> > > ?Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > > ?fs/nfsd/netlink.c | 19 +++++
> > > ?fs/nfsd/netlink.h | 3 +
> > > ?fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > > ?include/uapi/linux/nfsd_netlink.h | 11 +++
> > > ?tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > > ?tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > > ?7 files changed, 306 insertions(+)
> > >
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index c92e1425d316..6c5e42bb20f6 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -68,6 +68,18 @@ attribute-sets:
> > > ???????-
> > > ?????????name: threads
> > > ?????????type: u32
> > > + -
> > > + name: server-version
> > > + attributes:
> > > + -
> > > + name: major
> > > + type: u32
> > > + -
> > > + name: minor
> > > + type: u32
> > > + -
> > > + name: status
> > > + type: u8
> > > ?
> > >
> > >
> > >
> > > ?operations:
> > > ???list:
> > > @@ -110,3 +122,23 @@ operations:
> > > ?????????reply:
> > > ???????????attributes:
> > > ?????????????- threads
> > > + -
> > > + name: version-set
> > > + doc: enable/disable server version
> > > + attribute-set: server-version
> > > + flags: [ admin-perm ]
> > > + do:
> > > + request:
> > > + attributes:
> > > + - major
> > > + - minor
> > > + - status
> > > + -
> > > + name: version-get
> > > + doc: dump server versions
> > > + attribute-set: server-version
> > > + dump:
> > > + reply:
> > > + attributes:
> > > + - major
> > > + - minor
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > ? [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > ?};
> > > ?
> > >
> > >
> > >
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > +};
> > > +
> > > ?/* Ops table for nfsd */
> > > ?static const struct genl_split_ops nfsd_nl_ops[] = {
> > > ? {
> > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > ? .doit = nfsd_nl_threads_get_doit,
> > > ? .flags = GENL_CMD_CAP_DO,
> > > ? },
> > > + {
> > > + .cmd = NFSD_CMD_VERSION_SET,
> > > + .doit = nfsd_nl_version_set_doit,
> > > + .policy = nfsd_version_set_nl_policy,
> > > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > + },
> > > + {
> > > + .cmd = NFSD_CMD_VERSION_GET,
> > > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > ? struct netlink_callback *cb);
> > > ?int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > ?int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > ? return err;
> > > ?}
> > > ?
> > >
> > >
> > >
> > > +/**
> > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > + enum vers_op cmd;
> > > + u32 major, minor;
> > > + u8 status;
> > > + int ret;
> > > +
> > > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > + return -EINVAL;
> > > +
> > > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > +
> > > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > +
> > > + mutex_lock(&nfsd_mutex);
> > > + switch (major) {
> > > + case 4:
> > > + ret = nfsd_minorversion(nn, minor, cmd);
> > > + break;
> > > + case 2:
> > > + case 3:
> > > + if (!minor) {
> > > + ret = nfsd_vers(nn, major, cmd);
> > > + break;
> > > + }
> > > + fallthrough;
> > > + default:
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > + mutex_unlock(&nfsd_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > + struct netlink_callback *cb)
> > > +{
> > > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > + int i, ret = -ENOMEM;
> > > +
> > > + mutex_lock(&nfsd_mutex);
> > > +
> > > + for (i = 2; i <= 4; i++) {
> > > + int j;
> > > +
> > > + if (i < cb->args[0]) /* already consumed */
> > > + continue;
> > > +
> > > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > + continue;
> > > +
> > > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > + void *hdr;
> > > +
> > > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > > + continue;
> > > +
> > > + /* NFSv{2,3} does not support minor numbers */
> > > + if (i < 4 && j)
> > > + continue;
> > > +
> > > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > + continue;
> > > +
> > > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > + 0, NFSD_CMD_VERSION_GET);
> > > + if (!hdr)
> > > + goto out;
> > > +
> > > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > + goto out;
> > > +
> > > + genlmsg_end(skb, hdr);
> > > + }
> > > + }
> > > + cb->args[0] = i;
> > > + ret = skb->len;
> > > +out:
> > > + mutex_unlock(&nfsd_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > ?/**
> > > ??* 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > > --- a/include/uapi/linux/nfsd_netlink.h
> > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > @@ -36,10 +36,21 @@ enum {
> > > ? NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > ?};
> > > ?
> > >
> > >
> > >
> > > +enum {
> > > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > + NFSD_A_SERVER_VERSION_MINOR,
> > > + NFSD_A_SERVER_VERSION_STATUS,
> > > +
> > > + __NFSD_A_SERVER_VERSION_MAX,
> > > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > +};
> > > +
> > > ?enum {
> > > ? NFSD_CMD_RPC_STATUS_GET = 1,
> > > ? NFSD_CMD_THREADS_SET,
> > > ? NFSD_CMD_THREADS_GET,
> > > + NFSD_CMD_VERSION_SET,
> > > + NFSD_CMD_VERSION_GET,
> > > ?
> > >
> > >
> > >
> > > ? __NFSD_CMD_MAX,
> > > ? NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > index 9768328a7751..4cb71c3cd18d 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > ? [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > ? [NFSD_CMD_THREADS_SET] = "threads-set",
> > > ? [NFSD_CMD_THREADS_GET] = "threads-get",
> > > + [NFSD_CMD_VERSION_SET] = "version-set",
> > > + [NFSD_CMD_VERSION_GET] = "version-get",
> > > ?};
> > > ?
> > >
> > >
> > >
> > > ?const char *nfsd_op_str(int op)
> > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > ? .table = nfsd_server_worker_policy,
> > > ?};
> > > ?
> > >
> > >
> > >
> > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > +};
> > > +
> > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > + .table = nfsd_server_version_policy,
> > > +};
> > > +
> > > ?/* Common nested types */
> > > ?/* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > ?/* NFSD_CMD_RPC_STATUS_GET - dump */
> > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > ? return NULL;
> > > ?}
> > > ?
> > >
> > >
> > >
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > +{
> > > + free(req);
> > > +}
> > > +
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > +{
> > > + struct nlmsghdr *nlh;
> > > + int err;
> > > +
> > > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > + ys->req_policy = &nfsd_server_version_nest;
> > > +
> > > + if (req->_present.major)
> > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > + if (req->_present.minor)
> > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > + if (req->_present.status)
> > > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > +
> > > + err = ynl_exec(ys, nlh, NULL);
> > > + if (err < 0)
> > > + return -1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > +{
> > > + struct nfsd_version_get_list *next = rsp;
> > > +
> > > + while ((void *)next != YNL_LIST_END) {
> > > + rsp = next;
> > > + next = rsp->next;
> > > +
> > > + free(rsp);
> > > + }
> > > +}
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > +{
> > > + struct ynl_dump_state yds = {};
> > > + struct nlmsghdr *nlh;
> > > + int err;
> > > +
> > > + yds.ys = ys;
> > > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > + yds.cb = nfsd_version_get_rsp_parse;
> > > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > + yds.rsp_policy = &nfsd_server_version_nest;
> > > +
> > > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > +
> > > + err = ynl_exec_dump(ys, nlh, &yds);
> > > + if (err < 0)
> > > + goto free_list;
> > > +
> > > + return yds.first;
> > > +
> > > +free_list:
> > > + nfsd_version_get_list_free(yds.first);
> > > + return NULL;
> > > +}
> > > +
> > > ?const struct ynl_family ynl_nfsd_family = {
> > > ? .name = "nfsd",
> > > ?};
> > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > index e162a4f20d91..e61c5a9e46fb 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > ??*/
> > > ?struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > > ?
> > >
> > >
> > >
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +struct nfsd_version_set_req {
> > > + struct {
> > > + __u32 major:1;
> > > + __u32 minor:1;
> > > + __u32 status:1;
> > > + } _present;
> > > +
> > > + __u32 major;
> > > + __u32 minor;
> > > + __u8 status;
> > > +};
> > > +
> >
> > This more or less mirrors how the "versions" file works today, but that
> > interface is quite klunky.? We don't have a use case that requires that
> > we do this piecemeal like this. I think we'd be better served with a
> > more declarative interface that reconfigures the supported versions in
> > one shot:
> >
> > Instead of having "major,minor,status" and potentially having to call
> > this command several times from userland, it seems like it would be
> > nicer to just have userland send down a list "major,minor" that should
> > be enabled, and then just let the kernel figure out whether to enable or
> > disable each. An empty list could mean "disable everything".
> >
> > That's simpler to reason out as an interface from userland too. Trying
> > to keep track of the enabled and disabled versions and twiddle it is
> > really tricky in rpc.nfsd today.
>
> Ack. So far I have just converted the current implementation to netlink
> and I have not changed any logic. Anyway I am fine to change the current
> logic. Should we have 2 rpc.nfsd version in this case?
>

No. The goal is to make this a seamless change for systems
administrators who are already familiar with the userland tools.

I think what we want to aim for is to teach the existing rpc.nfsd to
speak netlink if it's available. If it's not, then it can fall back to
using the nfsdfs interfaces instead. Eventually (years from now) we can
remove the old interface support, once most everything in the field has
netlink support.

>
> > ?
> >
> > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > +{
> > > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > > +}
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > +
> > > +static inline void
> > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > +{
> > > + req->_present.major = 1;
> > > + req->major = major;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > +{
> > > + req->_present.minor = 1;
> > > + req->minor = minor;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > +{
> > > + req->_present.status = 1;
> > > + req->status = status;
> > > +}
> > > +
> > > +/*
> > > + * enable/disable server version
> > > + */
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +struct nfsd_version_get_list {
> > > + struct nfsd_version_get_list *next;
> > > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > +};
> > > +
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > +
> > > ?#endif /* _LINUX_NFSD_GEN_H */
> >
> > --
> > Jeff Layton <[email protected]>

--
Jeff Layton <[email protected]>

2023-11-30 16:22:53

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > >
> > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > +/* NFSD_CMD_LISTENER_START - do */
> > > +struct nfsd_listener_start_req {
> > > + struct {
> > > + __u32 transport_name_len;
> > > + __u32 port:1;
> > > + } _present;
> > > +
> > > + char *transport_name;
> > > + __u32 port;
> > > +};
> >
> > How do you deconfigure a listener with this interface? i.e. suppose I
> > want to stop nfsd from listening on a particular port? I think this too
> > is a place where a declarative interface would be better:
>
> Is it possible with current APIs? as for 2/3 so far I have just added netlink
> counter for current implementation but I am fine to change the logic here to
> better APIs.
>
> >

No, I don't think you can do this with the current API at all. I
consider it a major deficiency. I don't think we want to repeat that
mistake in the new interface.

> > Have userland send down a list of the ports that we should currently be
> > listening on, and let the kernel do the work to match the request. Again
> > too, an empty list could mean "close everything".
> >
> >

--
Jeff Layton <[email protected]>

2023-11-30 16:26:00

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

> On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > Introduce write_version netlink command similar to the ones available
> > > through the procfs.
> > >
> > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > ---
> > > Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > > fs/nfsd/netlink.c | 19 +++++
> > > fs/nfsd/netlink.h | 3 +
> > > fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > > include/uapi/linux/nfsd_netlink.h | 11 +++
> > > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > > tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > > 7 files changed, 306 insertions(+)
> > >
> > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > index c92e1425d316..6c5e42bb20f6 100644
> > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > @@ -68,6 +68,18 @@ attribute-sets:
> > > -
> > > name: threads
> > > type: u32
> > > + -
> > > + name: server-version
> > > + attributes:
> > > + -
> > > + name: major
> > > + type: u32
> > > + -
> > > + name: minor
> > > + type: u32
> > > + -
> > > + name: status
> > > + type: u8
> > >
> > > operations:
> > > list:
> > > @@ -110,3 +122,23 @@ operations:
> > > reply:
> > > attributes:
> > > - threads
> > > + -
> > > + name: version-set
> > > + doc: enable/disable server version
> > > + attribute-set: server-version
> > > + flags: [ admin-perm ]
> > > + do:
> > > + request:
> > > + attributes:
> > > + - major
> > > + - minor
> > > + - status
> > > + -
> > > + name: version-get
> > > + doc: dump server versions
> > > + attribute-set: server-version
> > > + dump:
> > > + reply:
> > > + attributes:
> > > + - major
> > > + - minor
> > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > --- a/fs/nfsd/netlink.c
> > > +++ b/fs/nfsd/netlink.c
> > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > };
> > >
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > +};
> > > +
> > > /* Ops table for nfsd */
> > > static const struct genl_split_ops nfsd_nl_ops[] = {
> > > {
> > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > .doit = nfsd_nl_threads_get_doit,
> > > .flags = GENL_CMD_CAP_DO,
> > > },
> > > + {
> > > + .cmd = NFSD_CMD_VERSION_SET,
> > > + .doit = nfsd_nl_version_set_doit,
> > > + .policy = nfsd_version_set_nl_policy,
> > > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > + },
> > > + {
> > > + .cmd = NFSD_CMD_VERSION_GET,
> > > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > > --- a/fs/nfsd/netlink.h
> > > +++ b/fs/nfsd/netlink.h
> > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > struct netlink_callback *cb);
> > > int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > > --- a/fs/nfsd/nfsctl.c
> > > +++ b/fs/nfsd/nfsctl.c
> > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > return err;
> > > }
> > >
> > > +/**
> > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > + * @skb: reply buffer
> > > + * @info: netlink metadata and command arguments
> > > + *
> > > + * Return 0 on success or a negative errno.
> > > + */
> > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > +{
> > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > + enum vers_op cmd;
> > > + u32 major, minor;
> > > + u8 status;
> > > + int ret;
> > > +
> > > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > + return -EINVAL;
> > > +
> > > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > +
> > > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > +
> > > + mutex_lock(&nfsd_mutex);
> > > + switch (major) {
> > > + case 4:
> > > + ret = nfsd_minorversion(nn, minor, cmd);
> > > + break;
> > > + case 2:
> > > + case 3:
> > > + if (!minor) {
> > > + ret = nfsd_vers(nn, major, cmd);
> > > + break;
> > > + }
> > > + fallthrough;
> > > + default:
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > + mutex_unlock(&nfsd_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > +/**
> > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > + * @skb: reply buffer
> > > + * @cb: netlink metadata and command arguments
> > > + *
> > > + * Returns the size of the reply or a negative errno.
> > > + */
> > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > + struct netlink_callback *cb)
> > > +{
> > > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > + int i, ret = -ENOMEM;
> > > +
> > > + mutex_lock(&nfsd_mutex);
> > > +
> > > + for (i = 2; i <= 4; i++) {
> > > + int j;
> > > +
> > > + if (i < cb->args[0]) /* already consumed */
> > > + continue;
> > > +
> > > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > + continue;
> > > +
> > > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > + void *hdr;
> > > +
> > > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > > + continue;
> > > +
> > > + /* NFSv{2,3} does not support minor numbers */
> > > + if (i < 4 && j)
> > > + continue;
> > > +
> > > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > + continue;
> > > +
> > > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > + 0, NFSD_CMD_VERSION_GET);
> > > + if (!hdr)
> > > + goto out;
> > > +
> > > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > + goto out;
> > > +
> > > + genlmsg_end(skb, hdr);
> > > + }
> > > + }
> > > + cb->args[0] = i;
> > > + ret = skb->len;
> > > +out:
> > > + mutex_unlock(&nfsd_mutex);
> > > +
> > > + return ret;
> > > +}
> > > +
> > > /**
> > > * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > > --- a/include/uapi/linux/nfsd_netlink.h
> > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > @@ -36,10 +36,21 @@ enum {
> > > NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > };
> > >
> > > +enum {
> > > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > + NFSD_A_SERVER_VERSION_MINOR,
> > > + NFSD_A_SERVER_VERSION_STATUS,
> > > +
> > > + __NFSD_A_SERVER_VERSION_MAX,
> > > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > +};
> > > +
> > > enum {
> > > NFSD_CMD_RPC_STATUS_GET = 1,
> > > NFSD_CMD_THREADS_SET,
> > > NFSD_CMD_THREADS_GET,
> > > + NFSD_CMD_VERSION_SET,
> > > + NFSD_CMD_VERSION_GET,
> > >
> > > __NFSD_CMD_MAX,
> > > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > index 9768328a7751..4cb71c3cd18d 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > [NFSD_CMD_THREADS_SET] = "threads-set",
> > > [NFSD_CMD_THREADS_GET] = "threads-get",
> > > + [NFSD_CMD_VERSION_SET] = "version-set",
> > > + [NFSD_CMD_VERSION_GET] = "version-get",
> > > };
> > >
> > > const char *nfsd_op_str(int op)
> > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > .table = nfsd_server_worker_policy,
> > > };
> > >
> > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > +};
> > > +
> > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > + .table = nfsd_server_version_policy,
> > > +};
> > > +
> > > /* Common nested types */
> > > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > return NULL;
> > > }
> > >
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > +{
> > > + free(req);
> > > +}
> > > +
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > +{
> > > + struct nlmsghdr *nlh;
> > > + int err;
> > > +
> > > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > + ys->req_policy = &nfsd_server_version_nest;
> > > +
> > > + if (req->_present.major)
> > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > + if (req->_present.minor)
> > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > + if (req->_present.status)
> > > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > +
> > > + err = ynl_exec(ys, nlh, NULL);
> > > + if (err < 0)
> > > + return -1;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > +{
> > > + struct nfsd_version_get_list *next = rsp;
> > > +
> > > + while ((void *)next != YNL_LIST_END) {
> > > + rsp = next;
> > > + next = rsp->next;
> > > +
> > > + free(rsp);
> > > + }
> > > +}
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > +{
> > > + struct ynl_dump_state yds = {};
> > > + struct nlmsghdr *nlh;
> > > + int err;
> > > +
> > > + yds.ys = ys;
> > > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > + yds.cb = nfsd_version_get_rsp_parse;
> > > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > + yds.rsp_policy = &nfsd_server_version_nest;
> > > +
> > > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > +
> > > + err = ynl_exec_dump(ys, nlh, &yds);
> > > + if (err < 0)
> > > + goto free_list;
> > > +
> > > + return yds.first;
> > > +
> > > +free_list:
> > > + nfsd_version_get_list_free(yds.first);
> > > + return NULL;
> > > +}
> > > +
> > > const struct ynl_family ynl_nfsd_family = {
> > > .name = "nfsd",
> > > };
> > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > index e162a4f20d91..e61c5a9e46fb 100644
> > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > */
> > > struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > >
> > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > +/* NFSD_CMD_VERSION_SET - do */
> > > +struct nfsd_version_set_req {
> > > + struct {
> > > + __u32 major:1;
> > > + __u32 minor:1;
> > > + __u32 status:1;
> > > + } _present;
> > > +
> > > + __u32 major;
> > > + __u32 minor;
> > > + __u8 status;
> > > +};
> > > +
> >
> > This more or less mirrors how the "versions" file works today, but that
> > interface is quite klunky.? We don't have a use case that requires that
> > we do this piecemeal like this. I think we'd be better served with a
> > more declarative interface that reconfigures the supported versions in
> > one shot:
> >
> > Instead of having "major,minor,status" and potentially having to call
> > this command several times from userland, it seems like it would be
> > nicer to just have userland send down a list "major,minor" that should
> > be enabled, and then just let the kernel figure out whether to enable or
> > disable each. An empty list could mean "disable everything".
> >
> > That's simpler to reason out as an interface from userland too. Trying
> > to keep track of the enabled and disabled versions and twiddle it is
> > really tricky in rpc.nfsd today.
>
> Jeff and Lorenzo, this sounds to me like a useful and narrow
> improvement to this interface, one that should be implemented as
> part of this patch series.

ack, I am fine with it, I will work on patch 2/3 and 3/3.
@Chuck: am I suppose to respin patch 1/3 too?

Regards,
Lorenzo

>
> Ditto for Jeff's review comment on 3/3.
>
>
> > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > +{
> > > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > > +}
> > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > +
> > > +static inline void
> > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > +{
> > > + req->_present.major = 1;
> > > + req->major = major;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > +{
> > > + req->_present.minor = 1;
> > > + req->minor = minor;
> > > +}
> > > +static inline void
> > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > +{
> > > + req->_present.status = 1;
> > > + req->status = status;
> > > +}
> > > +
> > > +/*
> > > + * enable/disable server version
> > > + */
> > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > +
> > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > +/* NFSD_CMD_VERSION_GET - dump */
> > > +struct nfsd_version_get_list {
> > > + struct nfsd_version_get_list *next;
> > > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > +};
> > > +
> > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > +
> > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > +
> > > #endif /* _LINUX_NFSD_GEN_H */
> >
> > --
> > Jeff Layton <[email protected]>
> >
>
> --
> Chuck Lever

Attachments:

(No filename) (16.03 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-30 16:29:59

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Thu, Nov 30, 2023 at 05:25:52PM +0100, Lorenzo Bianconi wrote:
> > On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > > Introduce write_version netlink command similar to the ones available
> > > > through the procfs.
> > > >
> > > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > > ---
> > > > Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > > > fs/nfsd/netlink.c | 19 +++++
> > > > fs/nfsd/netlink.h | 3 +
> > > > fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > > > include/uapi/linux/nfsd_netlink.h | 11 +++
> > > > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > > > tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > > > 7 files changed, 306 insertions(+)
> > > >
> > > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > > index c92e1425d316..6c5e42bb20f6 100644
> > > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > > @@ -68,6 +68,18 @@ attribute-sets:
> > > > -
> > > > name: threads
> > > > type: u32
> > > > + -
> > > > + name: server-version
> > > > + attributes:
> > > > + -
> > > > + name: major
> > > > + type: u32
> > > > + -
> > > > + name: minor
> > > > + type: u32
> > > > + -
> > > > + name: status
> > > > + type: u8
> > > >
> > > > operations:
> > > > list:
> > > > @@ -110,3 +122,23 @@ operations:
> > > > reply:
> > > > attributes:
> > > > - threads
> > > > + -
> > > > + name: version-set
> > > > + doc: enable/disable server version
> > > > + attribute-set: server-version
> > > > + flags: [ admin-perm ]
> > > > + do:
> > > > + request:
> > > > + attributes:
> > > > + - major
> > > > + - minor
> > > > + - status
> > > > + -
> > > > + name: version-get
> > > > + doc: dump server versions
> > > > + attribute-set: server-version
> > > > + dump:
> > > > + reply:
> > > > + attributes:
> > > > + - major
> > > > + - minor
> > > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > > --- a/fs/nfsd/netlink.c
> > > > +++ b/fs/nfsd/netlink.c
> > > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > > [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > > };
> > > >
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > > +};
> > > > +
> > > > /* Ops table for nfsd */
> > > > static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > {
> > > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > .doit = nfsd_nl_threads_get_doit,
> > > > .flags = GENL_CMD_CAP_DO,
> > > > },
> > > > + {
> > > > + .cmd = NFSD_CMD_VERSION_SET,
> > > > + .doit = nfsd_nl_version_set_doit,
> > > > + .policy = nfsd_version_set_nl_policy,
> > > > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > + },
> > > > + {
> > > > + .cmd = NFSD_CMD_VERSION_GET,
> > > > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > > > --- a/fs/nfsd/netlink.h
> > > > +++ b/fs/nfsd/netlink.h
> > > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > > struct netlink_callback *cb);
> > > > int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > > > --- a/fs/nfsd/nfsctl.c
> > > > +++ b/fs/nfsd/nfsctl.c
> > > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > > return err;
> > > > }
> > > >
> > > > +/**
> > > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > > + * @skb: reply buffer
> > > > + * @info: netlink metadata and command arguments
> > > > + *
> > > > + * Return 0 on success or a negative errno.
> > > > + */
> > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > > +{
> > > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > > + enum vers_op cmd;
> > > > + u32 major, minor;
> > > > + u8 status;
> > > > + int ret;
> > > > +
> > > > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > > + return -EINVAL;
> > > > +
> > > > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > > +
> > > > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > > +
> > > > + mutex_lock(&nfsd_mutex);
> > > > + switch (major) {
> > > > + case 4:
> > > > + ret = nfsd_minorversion(nn, minor, cmd);
> > > > + break;
> > > > + case 2:
> > > > + case 3:
> > > > + if (!minor) {
> > > > + ret = nfsd_vers(nn, major, cmd);
> > > > + break;
> > > > + }
> > > > + fallthrough;
> > > > + default:
> > > > + ret = -EINVAL;
> > > > + break;
> > > > + }
> > > > + mutex_unlock(&nfsd_mutex);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > > + * @skb: reply buffer
> > > > + * @cb: netlink metadata and command arguments
> > > > + *
> > > > + * Returns the size of the reply or a negative errno.
> > > > + */
> > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > + struct netlink_callback *cb)
> > > > +{
> > > > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > > + int i, ret = -ENOMEM;
> > > > +
> > > > + mutex_lock(&nfsd_mutex);
> > > > +
> > > > + for (i = 2; i <= 4; i++) {
> > > > + int j;
> > > > +
> > > > + if (i < cb->args[0]) /* already consumed */
> > > > + continue;
> > > > +
> > > > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > > + continue;
> > > > +
> > > > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > > + void *hdr;
> > > > +
> > > > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > > > + continue;
> > > > +
> > > > + /* NFSv{2,3} does not support minor numbers */
> > > > + if (i < 4 && j)
> > > > + continue;
> > > > +
> > > > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > > + continue;
> > > > +
> > > > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > > + 0, NFSD_CMD_VERSION_GET);
> > > > + if (!hdr)
> > > > + goto out;
> > > > +
> > > > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > > + goto out;
> > > > +
> > > > + genlmsg_end(skb, hdr);
> > > > + }
> > > > + }
> > > > + cb->args[0] = i;
> > > > + ret = skb->len;
> > > > +out:
> > > > + mutex_unlock(&nfsd_mutex);
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > /**
> > > > * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > > > --- a/include/uapi/linux/nfsd_netlink.h
> > > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > > @@ -36,10 +36,21 @@ enum {
> > > > NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > > };
> > > >
> > > > +enum {
> > > > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > > + NFSD_A_SERVER_VERSION_MINOR,
> > > > + NFSD_A_SERVER_VERSION_STATUS,
> > > > +
> > > > + __NFSD_A_SERVER_VERSION_MAX,
> > > > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > > +};
> > > > +
> > > > enum {
> > > > NFSD_CMD_RPC_STATUS_GET = 1,
> > > > NFSD_CMD_THREADS_SET,
> > > > NFSD_CMD_THREADS_GET,
> > > > + NFSD_CMD_VERSION_SET,
> > > > + NFSD_CMD_VERSION_GET,
> > > >
> > > > __NFSD_CMD_MAX,
> > > > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > > index 9768328a7751..4cb71c3cd18d 100644
> > > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > > [NFSD_CMD_THREADS_SET] = "threads-set",
> > > > [NFSD_CMD_THREADS_GET] = "threads-get",
> > > > + [NFSD_CMD_VERSION_SET] = "version-set",
> > > > + [NFSD_CMD_VERSION_GET] = "version-get",
> > > > };
> > > >
> > > > const char *nfsd_op_str(int op)
> > > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > > .table = nfsd_server_worker_policy,
> > > > };
> > > >
> > > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > > +};
> > > > +
> > > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > > + .table = nfsd_server_version_policy,
> > > > +};
> > > > +
> > > > /* Common nested types */
> > > > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > > return NULL;
> > > > }
> > > >
> > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > > +{
> > > > + free(req);
> > > > +}
> > > > +
> > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > > +{
> > > > + struct nlmsghdr *nlh;
> > > > + int err;
> > > > +
> > > > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > > + ys->req_policy = &nfsd_server_version_nest;
> > > > +
> > > > + if (req->_present.major)
> > > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > > + if (req->_present.minor)
> > > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > > + if (req->_present.status)
> > > > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > > +
> > > > + err = ynl_exec(ys, nlh, NULL);
> > > > + if (err < 0)
> > > > + return -1;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > > +{
> > > > + struct nfsd_version_get_list *next = rsp;
> > > > +
> > > > + while ((void *)next != YNL_LIST_END) {
> > > > + rsp = next;
> > > > + next = rsp->next;
> > > > +
> > > > + free(rsp);
> > > > + }
> > > > +}
> > > > +
> > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > > +{
> > > > + struct ynl_dump_state yds = {};
> > > > + struct nlmsghdr *nlh;
> > > > + int err;
> > > > +
> > > > + yds.ys = ys;
> > > > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > > + yds.cb = nfsd_version_get_rsp_parse;
> > > > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > > + yds.rsp_policy = &nfsd_server_version_nest;
> > > > +
> > > > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > > +
> > > > + err = ynl_exec_dump(ys, nlh, &yds);
> > > > + if (err < 0)
> > > > + goto free_list;
> > > > +
> > > > + return yds.first;
> > > > +
> > > > +free_list:
> > > > + nfsd_version_get_list_free(yds.first);
> > > > + return NULL;
> > > > +}
> > > > +
> > > > const struct ynl_family ynl_nfsd_family = {
> > > > .name = "nfsd",
> > > > };
> > > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > > index e162a4f20d91..e61c5a9e46fb 100644
> > > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > > */
> > > > struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > > >
> > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > +struct nfsd_version_set_req {
> > > > + struct {
> > > > + __u32 major:1;
> > > > + __u32 minor:1;
> > > > + __u32 status:1;
> > > > + } _present;
> > > > +
> > > > + __u32 major;
> > > > + __u32 minor;
> > > > + __u8 status;
> > > > +};
> > > > +
> > >
> > > This more or less mirrors how the "versions" file works today, but that
> > > interface is quite klunky.? We don't have a use case that requires that
> > > we do this piecemeal like this. I think we'd be better served with a
> > > more declarative interface that reconfigures the supported versions in
> > > one shot:
> > >
> > > Instead of having "major,minor,status" and potentially having to call
> > > this command several times from userland, it seems like it would be
> > > nicer to just have userland send down a list "major,minor" that should
> > > be enabled, and then just let the kernel figure out whether to enable or
> > > disable each. An empty list could mean "disable everything".
> > >
> > > That's simpler to reason out as an interface from userland too. Trying
> > > to keep track of the enabled and disabled versions and twiddle it is
> > > really tricky in rpc.nfsd today.
> >
> > Jeff and Lorenzo, this sounds to me like a useful and narrow
> > improvement to this interface, one that should be implemented as
> > part of this patch series.
>
> ack, I am fine with it, I will work on patch 2/3 and 3/3.
> @Chuck: am I suppose to respin patch 1/3 too?

I assumed there might be minor changes to 1/3 after Jakub's second
review, so I have not applied it yet. You can resend all three to
me.

> Regards,
> Lorenzo
>
> >
> > Ditto for Jeff's review comment on 3/3.
> >
> >
> > > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > > +{
> > > > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > > > +}
> > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > > +
> > > > +static inline void
> > > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > > +{
> > > > + req->_present.major = 1;
> > > > + req->major = major;
> > > > +}
> > > > +static inline void
> > > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > > +{
> > > > + req->_present.minor = 1;
> > > > + req->minor = minor;
> > > > +}
> > > > +static inline void
> > > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > > +{
> > > > + req->_present.status = 1;
> > > > + req->status = status;
> > > > +}
> > > > +
> > > > +/*
> > > > + * enable/disable server version
> > > > + */
> > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > > +
> > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > +struct nfsd_version_get_list {
> > > > + struct nfsd_version_get_list *next;
> > > > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > > +};
> > > > +
> > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > > +
> > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > > +
> > > > #endif /* _LINUX_NFSD_GEN_H */
> > >
> > > --
> > > Jeff Layton <[email protected]>
> > >
> >
> > --
> > Chuck Lever

--
Chuck Lever

2023-11-30 16:32:35

[permalink] [raw]

Subject: Re: [PATCH v5 2/3] NFSD: convert write_version to netlink command

On Nov 30, Chuck Lever wrote:
> On Thu, Nov 30, 2023 at 05:25:52PM +0100, Lorenzo Bianconi wrote:
> > > On Wed, Nov 29, 2023 at 01:23:37PM -0500, Jeff Layton wrote:
> > > > On Wed, 2023-11-29 at 18:12 +0100, Lorenzo Bianconi wrote:
> > > > > Introduce write_version netlink command similar to the ones available
> > > > > through the procfs.
> > > > >
> > > > > Signed-off-by: Lorenzo Bianconi <[email protected]>
> > > > > ---
> > > > > Documentation/netlink/specs/nfsd.yaml | 32 ++++++++
> > > > > fs/nfsd/netlink.c | 19 +++++
> > > > > fs/nfsd/netlink.h | 3 +
> > > > > fs/nfsd/nfsctl.c | 105 ++++++++++++++++++++++++++
> > > > > include/uapi/linux/nfsd_netlink.h | 11 +++
> > > > > tools/net/ynl/generated/nfsd-user.c | 81 ++++++++++++++++++++
> > > > > tools/net/ynl/generated/nfsd-user.h | 55 ++++++++++++++
> > > > > 7 files changed, 306 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/netlink/specs/nfsd.yaml b/Documentation/netlink/specs/nfsd.yaml
> > > > > index c92e1425d316..6c5e42bb20f6 100644
> > > > > --- a/Documentation/netlink/specs/nfsd.yaml
> > > > > +++ b/Documentation/netlink/specs/nfsd.yaml
> > > > > @@ -68,6 +68,18 @@ attribute-sets:
> > > > > -
> > > > > name: threads
> > > > > type: u32
> > > > > + -
> > > > > + name: server-version
> > > > > + attributes:
> > > > > + -
> > > > > + name: major
> > > > > + type: u32
> > > > > + -
> > > > > + name: minor
> > > > > + type: u32
> > > > > + -
> > > > > + name: status
> > > > > + type: u8
> > > > >
> > > > > operations:
> > > > > list:
> > > > > @@ -110,3 +122,23 @@ operations:
> > > > > reply:
> > > > > attributes:
> > > > > - threads
> > > > > + -
> > > > > + name: version-set
> > > > > + doc: enable/disable server version
> > > > > + attribute-set: server-version
> > > > > + flags: [ admin-perm ]
> > > > > + do:
> > > > > + request:
> > > > > + attributes:
> > > > > + - major
> > > > > + - minor
> > > > > + - status
> > > > > + -
> > > > > + name: version-get
> > > > > + doc: dump server versions
> > > > > + attribute-set: server-version
> > > > > + dump:
> > > > > + reply:
> > > > > + attributes:
> > > > > + - major
> > > > > + - minor
> > > > > diff --git a/fs/nfsd/netlink.c b/fs/nfsd/netlink.c
> > > > > index 1a59a8e6c7e2..0608a7bd193b 100644
> > > > > --- a/fs/nfsd/netlink.c
> > > > > +++ b/fs/nfsd/netlink.c
> > > > > @@ -15,6 +15,13 @@ static const struct nla_policy nfsd_threads_set_nl_policy[NFSD_A_SERVER_WORKER_T
> > > > > [NFSD_A_SERVER_WORKER_THREADS] = { .type = NLA_U32, },
> > > > > };
> > > > >
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +static const struct nla_policy nfsd_version_set_nl_policy[NFSD_A_SERVER_VERSION_STATUS + 1] = {
> > > > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .type = NLA_U32, },
> > > > > + [NFSD_A_SERVER_VERSION_MINOR] = { .type = NLA_U32, },
> > > > > + [NFSD_A_SERVER_VERSION_STATUS] = { .type = NLA_U8, },
> > > > > +};
> > > > > +
> > > > > /* Ops table for nfsd */
> > > > > static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > > {
> > > > > @@ -36,6 +43,18 @@ static const struct genl_split_ops nfsd_nl_ops[] = {
> > > > > .doit = nfsd_nl_threads_get_doit,
> > > > > .flags = GENL_CMD_CAP_DO,
> > > > > },
> > > > > + {
> > > > > + .cmd = NFSD_CMD_VERSION_SET,
> > > > > + .doit = nfsd_nl_version_set_doit,
> > > > > + .policy = nfsd_version_set_nl_policy,
> > > > > + .maxattr = NFSD_A_SERVER_VERSION_STATUS,
> > > > > + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO,
> > > > > + },
> > > > > + {
> > > > > + .cmd = NFSD_CMD_VERSION_GET,
> > > > > + .dumpit = nfsd_nl_version_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 4137fac477e4..7d203cec08e4 100644
> > > > > --- a/fs/nfsd/netlink.h
> > > > > +++ b/fs/nfsd/netlink.h
> > > > > @@ -18,6 +18,9 @@ int nfsd_nl_rpc_status_get_dumpit(struct sk_buff *skb,
> > > > > struct netlink_callback *cb);
> > > > > int nfsd_nl_threads_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > > int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info);
> > > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info);
> > > > > +int nfsd_nl_version_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 130b3d937a79..f04430f79687 100644
> > > > > --- a/fs/nfsd/nfsctl.c
> > > > > +++ b/fs/nfsd/nfsctl.c
> > > > > @@ -1757,6 +1757,111 @@ int nfsd_nl_threads_get_doit(struct sk_buff *skb, struct genl_info *info)
> > > > > return err;
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * nfsd_nl_version_set_doit - enable/disable the provided nfs server version
> > > > > + * @skb: reply buffer
> > > > > + * @info: netlink metadata and command arguments
> > > > > + *
> > > > > + * Return 0 on success or a negative errno.
> > > > > + */
> > > > > +int nfsd_nl_version_set_doit(struct sk_buff *skb, struct genl_info *info)
> > > > > +{
> > > > > + struct nfsd_net *nn = net_generic(genl_info_net(info), nfsd_net_id);
> > > > > + enum vers_op cmd;
> > > > > + u32 major, minor;
> > > > > + u8 status;
> > > > > + int ret;
> > > > > +
> > > > > + if (GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MAJOR) ||
> > > > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_MINOR) ||
> > > > > + GENL_REQ_ATTR_CHECK(info, NFSD_A_SERVER_VERSION_STATUS))
> > > > > + return -EINVAL;
> > > > > +
> > > > > + major = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MAJOR]);
> > > > > + minor = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_MINOR]);
> > > > > +
> > > > > + status = nla_get_u32(info->attrs[NFSD_A_SERVER_VERSION_STATUS]);
> > > > > + cmd = !!status ? NFSD_SET : NFSD_CLEAR;
> > > > > +
> > > > > + mutex_lock(&nfsd_mutex);
> > > > > + switch (major) {
> > > > > + case 4:
> > > > > + ret = nfsd_minorversion(nn, minor, cmd);
> > > > > + break;
> > > > > + case 2:
> > > > > + case 3:
> > > > > + if (!minor) {
> > > > > + ret = nfsd_vers(nn, major, cmd);
> > > > > + break;
> > > > > + }
> > > > > + fallthrough;
> > > > > + default:
> > > > > + ret = -EINVAL;
> > > > > + break;
> > > > > + }
> > > > > + mutex_unlock(&nfsd_mutex);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * nfsd_nl_version_get_doit - Handle verion_get dumpit
> > > > > + * @skb: reply buffer
> > > > > + * @cb: netlink metadata and command arguments
> > > > > + *
> > > > > + * Returns the size of the reply or a negative errno.
> > > > > + */
> > > > > +int nfsd_nl_version_get_dumpit(struct sk_buff *skb,
> > > > > + struct netlink_callback *cb)
> > > > > +{
> > > > > + struct nfsd_net *nn = net_generic(sock_net(skb->sk), nfsd_net_id);
> > > > > + int i, ret = -ENOMEM;
> > > > > +
> > > > > + mutex_lock(&nfsd_mutex);
> > > > > +
> > > > > + for (i = 2; i <= 4; i++) {
> > > > > + int j;
> > > > > +
> > > > > + if (i < cb->args[0]) /* already consumed */
> > > > > + continue;
> > > > > +
> > > > > + if (!nfsd_vers(nn, i, NFSD_AVAIL))
> > > > > + continue;
> > > > > +
> > > > > + for (j = 0; j <= NFSD_SUPPORTED_MINOR_VERSION; j++) {
> > > > > + void *hdr;
> > > > > +
> > > > > + if (!nfsd_vers(nn, i, NFSD_TEST))
> > > > > + continue;
> > > > > +
> > > > > + /* NFSv{2,3} does not support minor numbers */
> > > > > + if (i < 4 && j)
> > > > > + continue;
> > > > > +
> > > > > + if (i == 4 && !nfsd_minorversion(nn, j, NFSD_TEST))
> > > > > + continue;
> > > > > +
> > > > > + hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid,
> > > > > + cb->nlh->nlmsg_seq, &nfsd_nl_family,
> > > > > + 0, NFSD_CMD_VERSION_GET);
> > > > > + if (!hdr)
> > > > > + goto out;
> > > > > +
> > > > > + if (nla_put_u32(skb, NFSD_A_SERVER_VERSION_MAJOR, i) ||
> > > > > + nla_put_u32(skb, NFSD_A_SERVER_VERSION_MINOR, j))
> > > > > + goto out;
> > > > > +
> > > > > + genlmsg_end(skb, hdr);
> > > > > + }
> > > > > + }
> > > > > + cb->args[0] = i;
> > > > > + ret = skb->len;
> > > > > +out:
> > > > > + mutex_unlock(&nfsd_mutex);
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * 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 1b6fe1f9ed0e..1b3340f31baa 100644
> > > > > --- a/include/uapi/linux/nfsd_netlink.h
> > > > > +++ b/include/uapi/linux/nfsd_netlink.h
> > > > > @@ -36,10 +36,21 @@ enum {
> > > > > NFSD_A_SERVER_WORKER_MAX = (__NFSD_A_SERVER_WORKER_MAX - 1)
> > > > > };
> > > > >
> > > > > +enum {
> > > > > + NFSD_A_SERVER_VERSION_MAJOR = 1,
> > > > > + NFSD_A_SERVER_VERSION_MINOR,
> > > > > + NFSD_A_SERVER_VERSION_STATUS,
> > > > > +
> > > > > + __NFSD_A_SERVER_VERSION_MAX,
> > > > > + NFSD_A_SERVER_VERSION_MAX = (__NFSD_A_SERVER_VERSION_MAX - 1)
> > > > > +};
> > > > > +
> > > > > enum {
> > > > > NFSD_CMD_RPC_STATUS_GET = 1,
> > > > > NFSD_CMD_THREADS_SET,
> > > > > NFSD_CMD_THREADS_GET,
> > > > > + NFSD_CMD_VERSION_SET,
> > > > > + NFSD_CMD_VERSION_GET,
> > > > >
> > > > > __NFSD_CMD_MAX,
> > > > > NFSD_CMD_MAX = (__NFSD_CMD_MAX - 1)
> > > > > diff --git a/tools/net/ynl/generated/nfsd-user.c b/tools/net/ynl/generated/nfsd-user.c
> > > > > index 9768328a7751..4cb71c3cd18d 100644
> > > > > --- a/tools/net/ynl/generated/nfsd-user.c
> > > > > +++ b/tools/net/ynl/generated/nfsd-user.c
> > > > > @@ -17,6 +17,8 @@ static const char * const nfsd_op_strmap[] = {
> > > > > [NFSD_CMD_RPC_STATUS_GET] = "rpc-status-get",
> > > > > [NFSD_CMD_THREADS_SET] = "threads-set",
> > > > > [NFSD_CMD_THREADS_GET] = "threads-get",
> > > > > + [NFSD_CMD_VERSION_SET] = "version-set",
> > > > > + [NFSD_CMD_VERSION_GET] = "version-get",
> > > > > };
> > > > >
> > > > > const char *nfsd_op_str(int op)
> > > > > @@ -58,6 +60,17 @@ struct ynl_policy_nest nfsd_server_worker_nest = {
> > > > > .table = nfsd_server_worker_policy,
> > > > > };
> > > > >
> > > > > +struct ynl_policy_attr nfsd_server_version_policy[NFSD_A_SERVER_VERSION_MAX + 1] = {
> > > > > + [NFSD_A_SERVER_VERSION_MAJOR] = { .name = "major", .type = YNL_PT_U32, },
> > > > > + [NFSD_A_SERVER_VERSION_MINOR] = { .name = "minor", .type = YNL_PT_U32, },
> > > > > + [NFSD_A_SERVER_VERSION_STATUS] = { .name = "status", .type = YNL_PT_U8, },
> > > > > +};
> > > > > +
> > > > > +struct ynl_policy_nest nfsd_server_version_nest = {
> > > > > + .max_attr = NFSD_A_SERVER_VERSION_MAX,
> > > > > + .table = nfsd_server_version_policy,
> > > > > +};
> > > > > +
> > > > > /* Common nested types */
> > > > > /* ============== NFSD_CMD_RPC_STATUS_GET ============== */
> > > > > /* NFSD_CMD_RPC_STATUS_GET - dump */
> > > > > @@ -290,6 +303,74 @@ struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys)
> > > > > return NULL;
> > > > > }
> > > > >
> > > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req)
> > > > > +{
> > > > > + free(req);
> > > > > +}
> > > > > +
> > > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req)
> > > > > +{
> > > > > + struct nlmsghdr *nlh;
> > > > > + int err;
> > > > > +
> > > > > + nlh = ynl_gemsg_start_req(ys, ys->family_id, NFSD_CMD_VERSION_SET, 1);
> > > > > + ys->req_policy = &nfsd_server_version_nest;
> > > > > +
> > > > > + if (req->_present.major)
> > > > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MAJOR, req->major);
> > > > > + if (req->_present.minor)
> > > > > + mnl_attr_put_u32(nlh, NFSD_A_SERVER_VERSION_MINOR, req->minor);
> > > > > + if (req->_present.status)
> > > > > + mnl_attr_put_u8(nlh, NFSD_A_SERVER_VERSION_STATUS, req->status);
> > > > > +
> > > > > + err = ynl_exec(ys, nlh, NULL);
> > > > > + if (err < 0)
> > > > > + return -1;
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp)
> > > > > +{
> > > > > + struct nfsd_version_get_list *next = rsp;
> > > > > +
> > > > > + while ((void *)next != YNL_LIST_END) {
> > > > > + rsp = next;
> > > > > + next = rsp->next;
> > > > > +
> > > > > + free(rsp);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys)
> > > > > +{
> > > > > + struct ynl_dump_state yds = {};
> > > > > + struct nlmsghdr *nlh;
> > > > > + int err;
> > > > > +
> > > > > + yds.ys = ys;
> > > > > + yds.alloc_sz = sizeof(struct nfsd_version_get_list);
> > > > > + yds.cb = nfsd_version_get_rsp_parse;
> > > > > + yds.rsp_cmd = NFSD_CMD_VERSION_GET;
> > > > > + yds.rsp_policy = &nfsd_server_version_nest;
> > > > > +
> > > > > + nlh = ynl_gemsg_start_dump(ys, ys->family_id, NFSD_CMD_VERSION_GET, 1);
> > > > > +
> > > > > + err = ynl_exec_dump(ys, nlh, &yds);
> > > > > + if (err < 0)
> > > > > + goto free_list;
> > > > > +
> > > > > + return yds.first;
> > > > > +
> > > > > +free_list:
> > > > > + nfsd_version_get_list_free(yds.first);
> > > > > + return NULL;
> > > > > +}
> > > > > +
> > > > > const struct ynl_family ynl_nfsd_family = {
> > > > > .name = "nfsd",
> > > > > };
> > > > > diff --git a/tools/net/ynl/generated/nfsd-user.h b/tools/net/ynl/generated/nfsd-user.h
> > > > > index e162a4f20d91..e61c5a9e46fb 100644
> > > > > --- a/tools/net/ynl/generated/nfsd-user.h
> > > > > +++ b/tools/net/ynl/generated/nfsd-user.h
> > > > > @@ -111,4 +111,59 @@ void nfsd_threads_get_rsp_free(struct nfsd_threads_get_rsp *rsp);
> > > > > */
> > > > > struct nfsd_threads_get_rsp *nfsd_threads_get(struct ynl_sock *ys);
> > > > >
> > > > > +/* ============== NFSD_CMD_VERSION_SET ============== */
> > > > > +/* NFSD_CMD_VERSION_SET - do */
> > > > > +struct nfsd_version_set_req {
> > > > > + struct {
> > > > > + __u32 major:1;
> > > > > + __u32 minor:1;
> > > > > + __u32 status:1;
> > > > > + } _present;
> > > > > +
> > > > > + __u32 major;
> > > > > + __u32 minor;
> > > > > + __u8 status;
> > > > > +};
> > > > > +
> > > >
> > > > This more or less mirrors how the "versions" file works today, but that
> > > > interface is quite klunky.? We don't have a use case that requires that
> > > > we do this piecemeal like this. I think we'd be better served with a
> > > > more declarative interface that reconfigures the supported versions in
> > > > one shot:
> > > >
> > > > Instead of having "major,minor,status" and potentially having to call
> > > > this command several times from userland, it seems like it would be
> > > > nicer to just have userland send down a list "major,minor" that should
> > > > be enabled, and then just let the kernel figure out whether to enable or
> > > > disable each. An empty list could mean "disable everything".
> > > >
> > > > That's simpler to reason out as an interface from userland too. Trying
> > > > to keep track of the enabled and disabled versions and twiddle it is
> > > > really tricky in rpc.nfsd today.
> > >
> > > Jeff and Lorenzo, this sounds to me like a useful and narrow
> > > improvement to this interface, one that should be implemented as
> > > part of this patch series.
> >
> > ack, I am fine with it, I will work on patch 2/3 and 3/3.
> > @Chuck: am I suppose to respin patch 1/3 too?
>
> I assumed there might be minor changes to 1/3 after Jakub's second
> review, so I have not applied it yet. You can resend all three to
> me.

iirc jakub's review was just for patch 2/3, anyway up to you.

Regards,
Lorenzo

>
>
> > Regards,
> > Lorenzo
> >
> > >
> > > Ditto for Jeff's review comment on 3/3.
> > >
> > >
> > > > > +static inline struct nfsd_version_set_req *nfsd_version_set_req_alloc(void)
> > > > > +{
> > > > > + return calloc(1, sizeof(struct nfsd_version_set_req));
> > > > > +}
> > > > > +void nfsd_version_set_req_free(struct nfsd_version_set_req *req);
> > > > > +
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_major(struct nfsd_version_set_req *req, __u32 major)
> > > > > +{
> > > > > + req->_present.major = 1;
> > > > > + req->major = major;
> > > > > +}
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_minor(struct nfsd_version_set_req *req, __u32 minor)
> > > > > +{
> > > > > + req->_present.minor = 1;
> > > > > + req->minor = minor;
> > > > > +}
> > > > > +static inline void
> > > > > +nfsd_version_set_req_set_status(struct nfsd_version_set_req *req, __u8 status)
> > > > > +{
> > > > > + req->_present.status = 1;
> > > > > + req->status = status;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * enable/disable server version
> > > > > + */
> > > > > +int nfsd_version_set(struct ynl_sock *ys, struct nfsd_version_set_req *req);
> > > > > +
> > > > > +/* ============== NFSD_CMD_VERSION_GET ============== */
> > > > > +/* NFSD_CMD_VERSION_GET - dump */
> > > > > +struct nfsd_version_get_list {
> > > > > + struct nfsd_version_get_list *next;
> > > > > + struct nfsd_version_get_rsp obj __attribute__ ((aligned (8)));
> > > > > +};
> > > > > +
> > > > > +void nfsd_version_get_list_free(struct nfsd_version_get_list *rsp);
> > > > > +
> > > > > +struct nfsd_version_get_list *nfsd_version_get_dump(struct ynl_sock *ys);
> > > > > +
> > > > > #endif /* _LINUX_NFSD_GEN_H */
> > > >
> > > > --
> > > > Jeff Layton <[email protected]>
> > > >
> > >
> > > --
> > > Chuck Lever
>
>
>
> --
> Chuck Lever

Attachments:

(No filename) (18.20 kB)
signature.asc (235.00 B)
Download all attachments

2023-11-30 16:55:34

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > >
> > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > +struct nfsd_listener_start_req {
> > > > + struct {
> > > > + __u32 transport_name_len;
> > > > + __u32 port:1;
> > > > + } _present;
> > > > +
> > > > + char *transport_name;
> > > > + __u32 port;
> > > > +};
> > >
> > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > want to stop nfsd from listening on a particular port? I think this too
> > > is a place where a declarative interface would be better:
> >
> > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > counter for current implementation but I am fine to change the logic here to
> > better APIs.
> >
> > >
>
> No, I don't think you can do this with the current API at all. I
> consider it a major deficiency. I don't think we want to repeat that
> mistake in the new interface.
>
> > > Have userland send down a list of the ports that we should currently be
> > > listening on, and let the kernel do the work to match the request. Again
> > > too, an empty list could mean "close everything".
> > >
> > >
>

Another thought: should this interface also report and allow you to
specify the address to listen on?

When the write_ports interface was first created, it lacked a field for
the address to listen on.?Later we added a way to just hand off a socket
to the kernel to pass that info.

I think it's possible today to send down a socket that only listens on a
particular address, and you have no real way to tell that with the
current "ports" file.

Should we instead plumb a complete struct sockaddr_storage (or some
other suitable address structure) into this interface?
--
Jeff Layton <[email protected]>

2023-11-30 17:40:15

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

On Thu, Nov 30, 2023 at 11:55:29AM -0500, Jeff Layton wrote:
> On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> > On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > >
> > > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > > +struct nfsd_listener_start_req {
> > > > > + struct {
> > > > > + __u32 transport_name_len;
> > > > > + __u32 port:1;
> > > > > + } _present;
> > > > > +
> > > > > + char *transport_name;
> > > > > + __u32 port;
> > > > > +};
> > > >
> > > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > > want to stop nfsd from listening on a particular port? I think this too
> > > > is a place where a declarative interface would be better:
> > >
> > > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > > counter for current implementation but I am fine to change the logic here to
> > > better APIs.
> > >
> > > >
> >
> > No, I don't think you can do this with the current API at all. I
> > consider it a major deficiency. I don't think we want to repeat that
> > mistake in the new interface.
> >
> > > > Have userland send down a list of the ports that we should currently be
> > > > listening on, and let the kernel do the work to match the request. Again
> > > > too, an empty list could mean "close everything".
>
> Another thought: should this interface also report and allow you to
> specify the address to listen on?
>
> When the write_ports interface was first created, it lacked a field for
> the address to listen on.?Later we added a way to just hand off a socket
> to the kernel to pass that info.
>
> I think it's possible today to send down a socket that only listens on a
> particular address, and you have no real way to tell that with the
> current "ports" file.

All agreed, but listening on a particular address isn't something we
need today. (Or is it?)

Does the socket-passing thing work for non socket-based transports
like RDMA? I would think that mechanism is legacy.

> Should we instead plumb a complete struct sockaddr_storage (or some
> other suitable address structure) into this interface?

How difficult would it be to add this later, when we actually have a
specific use case?

--
Chuck Lever

2023-11-30 18:01:18

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] NFSD: convert write_ports to netlink command

On Thu, 2023-11-30 at 12:39 -0500, Chuck Lever wrote:
> On Thu, Nov 30, 2023 at 11:55:29AM -0500, Jeff Layton wrote:
> > On Thu, 2023-11-30 at 11:22 -0500, Jeff Layton wrote:
> > > On Thu, 2023-11-30 at 10:57 +0100, Lorenzo Bianconi wrote:
> > > > > >
> > > > > > +/* ============== NFSD_CMD_LISTENER_START ============== */
> > > > > > +/* NFSD_CMD_LISTENER_START - do */
> > > > > > +struct nfsd_listener_start_req {
> > > > > > + struct {
> > > > > > + __u32 transport_name_len;
> > > > > > + __u32 port:1;
> > > > > > + } _present;
> > > > > > +
> > > > > > + char *transport_name;
> > > > > > + __u32 port;
> > > > > > +};
> > > > >
> > > > > How do you deconfigure a listener with this interface? i.e. suppose I
> > > > > want to stop nfsd from listening on a particular port? I think this too
> > > > > is a place where a declarative interface would be better:
> > > >
> > > > Is it possible with current APIs? as for 2/3 so far I have just added netlink
> > > > counter for current implementation but I am fine to change the logic here to
> > > > better APIs.
> > > >
> > > > >
> > >
> > > No, I don't think you can do this with the current API at all. I
> > > consider it a major deficiency. I don't think we want to repeat that
> > > mistake in the new interface.
> > >
> > > > > Have userland send down a list of the ports that we should currently be
> > > > > listening on, and let the kernel do the work to match the request. Again
> > > > > too, an empty list could mean "close everything".
> >
> > Another thought: should this interface also report and allow you to
> > specify the address to listen on?
> >
> > When the write_ports interface was first created, it lacked a field for
> > the address to listen on.?Later we added a way to just hand off a socket
> > to the kernel to pass that info.
> >
> > I think it's possible today to send down a socket that only listens on a
> > particular address, and you have no real way to tell that with the
> > current "ports" file.
>
> All agreed, but listening on a particular address isn't something we
> need today. (Or is it?)
>

It is for TCP/UDP -- see the -H option to rpc.nfsd.

> Does the socket-passing thing work for non socket-based transports
> like RDMA? I would think that mechanism is legacy.
>

To the contrary actually. rpc.nfsd almost always does this today unless
you're configuring rdma. But, that was just a convenient way to do this
with a fd based interface.

I think with netlink, we just want to send down a list of
(transport_name, sockaddr) pairs and let the kernel open and close the
appropriate sockets. For RDMA, we can just fill out the sa_port field
and ignore the rest.

>
> > Should we instead plumb a complete struct sockaddr_storage (or some
> > other suitable address structure) into this interface?
>
> How difficult would it be to add this later, when we actually have a
> specific use case?

I think we have one now. rpc.nfsd passes down sockets to the kernel for
TCP and UDP listeners, so we need a way to send down a complete sockaddr
for the listeners.
--
Jeff Layton <[email protected]>

2023-11-30 18:47:24