* [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
@ 2025-01-10 13:46 Jeff Layton
2025-01-10 13:46 ` [PATCH v2 1/3] nfsdctl: convert to xlog() Jeff Layton
` (4 more replies)
0 siblings, 5 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-10 13:46 UTC (permalink / raw)
To: Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs, Jeff Layton
v2 is just a small update to fix the problems that Scott spotted.
This patch series adds support for the new lockd configuration interface
that should fix this RH bug:
https://issues.redhat.com/browse/RHEL-71698
There are some other improvements here too, notably a switch to xlog.
Only lightly tested, but seems to do the right thing.
Port handling with lockd still needs more work. Currently that is
usually configured by rpc.statd. I think we need to convert it to
use netlink to configure the ports as well, when it's able.
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Changes in v2:
- properly regenerate manpages
- fix up bogus merge conflict
- add D_GENERAL xlog messages when nfsdctl starts and exits
- Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
---
Jeff Layton (3):
nfsdctl: convert to xlog()
nfsdctl: fix the --version option
nfsdctl: add necessary bits to configure lockd
configure.ac | 4 +
utils/nfsdctl/lockd_netlink.h | 29 ++++
utils/nfsdctl/nfsdctl.8 | 15 +-
utils/nfsdctl/nfsdctl.adoc | 8 +
utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
5 files changed, 321 insertions(+), 66 deletions(-)
---
base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
change-id: 20250109-lockd-nl-6272fa9e8a5d
Best regards,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH v2 1/3] nfsdctl: convert to xlog()
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
@ 2025-01-10 13:46 ` Jeff Layton
2025-01-10 13:46 ` [PATCH v2 2/3] nfsdctl: fix the --version option Jeff Layton
` (3 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-10 13:46 UTC (permalink / raw)
To: Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs, Jeff Layton
Convert all of the fprintf(stderr, ...) calls to xlog(L_ERROR, ...)
calls. Change the -d option to not take an argument, and add a -s
option that will make nfsdctl log to syslog instead of stderr.
Also remove the extraneous error message in run_commandline, and add
a couple of trivial debug messages when the program starts and ends
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
utils/nfsdctl/nfsdctl.8 | 9 +++-
utils/nfsdctl/nfsdctl.adoc | 3 ++
utils/nfsdctl/nfsdctl.c | 111 +++++++++++++++++++++++++--------------------
3 files changed, 71 insertions(+), 52 deletions(-)
diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
index b08fe8036a70155b8f8713ffccb861b98b15302a..39ae1855ae50e94da113981d5e8cf8ac14440c3a 100644
--- a/utils/nfsdctl/nfsdctl.8
+++ b/utils/nfsdctl/nfsdctl.8
@@ -2,12 +2,12 @@
.\" Title: nfsdctl
.\" Author: Jeff Layton
.\" Generator: Asciidoctor 2.0.20
-.\" Date: 2024-12-30
+.\" Date: 2025-01-09
.\" Manual: \ \&
.\" Source: \ \&
.\" Language: English
.\"
-.TH "NFSDCTL" "8" "2024-12-30" "\ \&" "\ \&"
+.TH "NFSDCTL" "8" "2025-01-09" "\ \&" "\ \&"
.ie \n(.g .ds Aq \(aq
.el .ds Aq '
.ss \n[.ss] 0
@@ -60,6 +60,11 @@ Enable debug logging
Print program help text
.RE
.sp
+\fB\-s, \-\-syslog\fP
+.RS 4
+Log to syslog instead of to stderr (the default)
+.RE
+.sp
\fB\-V, \-\-version\fP
.RS 4
Print program version
diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
index c5921458a8e81e749d264cc7dd8344889ec71ac5..2114693042594297b7c5d8600ca16813a0f2356c 100644
--- a/utils/nfsdctl/nfsdctl.adoc
+++ b/utils/nfsdctl/nfsdctl.adoc
@@ -30,6 +30,9 @@ To get information about different subcommand usage, pass the subcommand the
*-h, --help*::
Print program help text
+*-s, --syslog*::
+ Log to syslog instead of to stderr (the default)
+
*-V, --version*::
Print program version
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
index ef917ff037e4ef9c532f814eb144ade642112036..e6819aec9890ae675a43b8259021ebaa909b08b9 100644
--- a/utils/nfsdctl/nfsdctl.c
+++ b/utils/nfsdctl/nfsdctl.c
@@ -43,8 +43,6 @@
* gcc -I/usr/include/libnl3/ -o <prog-name> <prog-name>.c -lnl-3 -lnl-genl-3
*/
-static int debug_level;
-
struct nfs_version {
uint8_t major;
uint8_t minor;
@@ -388,7 +386,7 @@ static struct nl_sock *netlink_sock_alloc(void)
return NULL;
if (genl_connect(sock)) {
- fprintf(stderr, "Failed to connect to generic netlink\n");
+ xlog(L_ERROR, "Failed to connect to generic netlink");
nl_socket_free(sock);
return NULL;
}
@@ -407,18 +405,18 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock)
id = genl_ctrl_resolve(sock, NFSD_FAMILY_NAME);
if (id < 0) {
- fprintf(stderr, "%s not found\n", NFSD_FAMILY_NAME);
+ xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
return NULL;
}
msg = nlmsg_alloc();
if (!msg) {
- fprintf(stderr, "failed to allocate netlink message\n");
+ xlog(L_ERROR, "failed to allocate netlink message");
return NULL;
}
if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
- fprintf(stderr, "failed to allocate netlink message\n");
+ xlog(L_ERROR, "failed to allocate netlink message");
nlmsg_free(msg);
return NULL;
}
@@ -460,7 +458,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "failed to allocate netlink callbacks");
ret = 1;
goto out;
}
@@ -478,7 +476,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -519,14 +517,14 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "send failed (%d)!\n", ret);
+ xlog(L_ERROR, "send failed (%d)!", ret);
goto out_cb;
}
@@ -539,7 +537,7 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -584,13 +582,13 @@ static int threads_func(struct nl_sock *sock, int argc, char **argv)
/* empty string? */
if (targv[i][0] == '\0') {
- fprintf(stderr, "Invalid threads value %s.\n", targv[i]);
+ xlog(L_ERROR, "Invalid threads value %s.", targv[i]);
return 1;
}
pool_threads[i] = strtol(targv[i], &endptr, 0);
if (!endptr || *endptr != '\0') {
- fprintf(stderr, "Invalid threads value %s.\n", argv[1]);
+ xlog(L_ERROR, "Invalid threads value %s.", argv[1]);
return 1;
}
}
@@ -619,14 +617,14 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "send failed: %d\n", ret);
+ xlog(L_ERROR, "send failed: %d", ret);
goto out_cb;
}
@@ -639,7 +637,7 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -688,7 +686,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
a = nla_nest_start(msg, NLA_F_NESTED | NFSD_A_SERVER_PROTO_VERSION);
if (!a) {
- fprintf(stderr, "Unable to allocate version nest!\n");
+ xlog(L_ERROR, "Unable to allocate version nest!");
ret = 1;
goto out;
}
@@ -705,14 +703,14 @@ static int set_nfsd_versions(struct nl_sock *sock)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "Failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "Failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "Send failed: %d\n", ret);
+ xlog(L_ERROR, "Send failed: %d", ret);
goto out_cb;
}
@@ -725,7 +723,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -750,7 +748,7 @@ static int update_nfsd_version(int major, int minor, bool enabled)
/* the kernel doesn't support this version */
if (!enabled)
return 0;
- fprintf(stderr, "This kernel does not support NFS version %d.%d\n", major, minor);
+ xlog(L_ERROR, "This kernel does not support NFS version %d.%d", major, minor);
return -EINVAL;
}
@@ -792,7 +790,7 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
ret = sscanf(str, "%c%d.%d\n", &sign, &major, &minor);
if (ret < 2) {
- fprintf(stderr, "Invalid version string (%d) %s\n", ret, str);
+ xlog(L_ERROR, "Invalid version string (%d) %s", ret, str);
return -EINVAL;
}
@@ -804,7 +802,7 @@ static int version_func(struct nl_sock *sock, int argc, char ** argv)
enabled = false;
break;
default:
- fprintf(stderr, "Invalid version string %s\n", str);
+ xlog(L_ERROR, "Invalid version string %s", str);
return -EINVAL;
}
@@ -837,14 +835,14 @@ static int fetch_current_listeners(struct nl_sock *sock)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "send failed: %d\n", ret);
+ xlog(L_ERROR, "send failed: %d", ret);
goto out_cb;
}
@@ -857,7 +855,7 @@ static int fetch_current_listeners(struct nl_sock *sock)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -992,7 +990,7 @@ static int update_listeners(const char *str)
*/
ret = getaddrinfo(addr, port, &hints, &res);
if (ret) {
- fprintf(stderr, "getaddrinfo of \"%s\" failed: %s\n",
+ xlog(L_ERROR, "getaddrinfo of \"%s\" failed: %s",
addr, gai_strerror(ret));
return -EINVAL;
}
@@ -1044,7 +1042,7 @@ static int update_listeners(const char *str)
}
return 0;
out_inval:
- fprintf(stderr, "Invalid listener update string: %s", str);
+ xlog(L_ERROR, "Invalid listener update string: %s", str);
return -EINVAL;
}
@@ -1074,7 +1072,7 @@ static int set_listeners(struct nl_sock *sock)
a = nla_nest_start(msg, NLA_F_NESTED | NFSD_A_SERVER_SOCK_ADDR);
if (!a) {
- fprintf(stderr, "Unable to allocate listener nest!\n");
+ xlog(L_ERROR, "Unable to allocate listener nest!");
ret = 1;
goto out;
}
@@ -1089,14 +1087,14 @@ static int set_listeners(struct nl_sock *sock)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "Failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "Failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "Send failed: %d\n", ret);
+ xlog(L_ERROR, "Send failed: %d", ret);
goto out_cb;
}
@@ -1109,7 +1107,7 @@ static int set_listeners(struct nl_sock *sock)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -1186,14 +1184,14 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
cb = nl_cb_alloc(NL_CB_CUSTOM);
if (!cb) {
- fprintf(stderr, "failed to allocate netlink callbacks\n");
+ xlog(L_ERROR, "failed to allocate netlink callbacks");
ret = 1;
goto out;
}
ret = nl_send_auto(sock, msg);
if (ret < 0) {
- fprintf(stderr, "send failed (%d)!\n", ret);
+ xlog(L_ERROR, "send failed (%d)!", ret);
goto out_cb;
}
@@ -1206,7 +1204,7 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
while (ret > 0)
nl_recvmsgs(sock, cb);
if (ret < 0) {
- fprintf(stderr, "Error: %s\n", strerror(-ret));
+ xlog(L_ERROR, "Error: %s", strerror(-ret));
ret = 1;
}
out_cb:
@@ -1243,7 +1241,7 @@ static int pool_mode_func(struct nl_sock *sock, int argc, char **argv)
/* empty string? */
if (*targv[0] == '\0') {
- fprintf(stderr, "Invalid threads value %s.\n", targv[0]);
+ xlog(L_ERROR, "Invalid threads value %s.", targv[0]);
return 1;
}
pool_mode = targv[0];
@@ -1395,7 +1393,7 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
threads[idx++] = strtol(n->field, &endptr, 0);
if (!endptr || *endptr != '\0') {
- fprintf(stderr, "Invalid threads value %s.\n", n->field);
+ xlog(L_ERROR, "Invalid threads value %s.", n->field);
ret = -EINVAL;
goto out;
}
@@ -1451,10 +1449,11 @@ static nfsdctl_func func[] = {
static void usage(void)
{
printf("Usage:\n");
- printf("%s [-hv] [COMMAND] [ARGS]\n", taskname);
+ printf("%s [-hdsV] [COMMAND] [ARGS]\n", taskname);
printf(" options:\n");
printf(" -h | --help usage info\n");
- printf(" -d | --debug=NUM enable debugging\n");
+ printf(" -d | --debug enable debugging\n");
+ printf(" -s | --syslog log messages to syslog\n");
printf(" -V | --version print version info\n");
printf(" commands:\n");
printf(" pool-mode get/set host pool mode setting\n");
@@ -1468,7 +1467,8 @@ static void usage(void)
/* Options given before the command string */
static const struct option pre_options[] = {
{ "help", no_argument, NULL, 'h' },
- { "debug", required_argument, NULL, 'd' },
+ { "debug", no_argument, NULL, 'd' },
+ { "syslog", no_argument, NULL, 's' },
{ "version", no_argument, NULL, 'V' },
{ },
};
@@ -1521,8 +1521,6 @@ static int run_commandline(struct nl_sock *sock)
ret = tokenize_string(line, &argc, argv);
if (!ret)
ret = run_one_command(sock, argc, argv);
- if (ret)
- fprintf(stderr, "Error: %s\n", strerror(ret));
free(line);
}
return 0;
@@ -1531,23 +1529,25 @@ static int run_commandline(struct nl_sock *sock)
int main(int argc, char **argv)
{
int opt, ret;
- struct nl_sock *sock = netlink_sock_alloc();
-
- if (!sock) {
- fprintf(stderr, "Unable to allocate netlink socket!");
- return 1;
- }
+ struct nl_sock *sock;
taskname = argv[0];
+ xlog_syslog(0);
+ xlog_stderr(1);
+
/* Parse the preliminary options */
- while ((opt = getopt_long(argc, argv, "+hd:V", pre_options, NULL)) != -1) {
+ while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
switch (opt) {
case 'h':
usage();
return 0;
case 'd':
- debug_level = atoi(optarg);
+ xlog_config(D_ALL, 1);
+ break;
+ case 's':
+ xlog_syslog(1);
+ xlog_stderr(0);
break;
case 'V':
// FIXME: print_version();
@@ -1555,6 +1555,16 @@ int main(int argc, char **argv)
}
}
+ xlog_open(basename(argv[0]));
+
+ xlog(D_GENERAL, "nfsdctl started");
+
+ sock = netlink_sock_alloc();
+ if (!sock) {
+ xlog(L_ERROR, "Unable to allocate netlink socket!");
+ return 1;
+ }
+
if (optind > argc) {
usage();
return 1;
@@ -1566,5 +1576,6 @@ int main(int argc, char **argv)
ret = run_one_command(sock, argc - optind, &argv[optind]);
nl_socket_free(sock);
+ xlog(D_GENERAL, "nfsdctl exiting");
return ret;
}
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 2/3] nfsdctl: fix the --version option
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
2025-01-10 13:46 ` [PATCH v2 1/3] nfsdctl: convert to xlog() Jeff Layton
@ 2025-01-10 13:46 ` Jeff Layton
2025-01-10 13:46 ` [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd Jeff Layton
` (2 subsequent siblings)
4 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-10 13:46 UTC (permalink / raw)
To: Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs, Jeff Layton
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
utils/nfsdctl/nfsdctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
index e6819aec9890ae675a43b8259021ebaa909b08b9..5c319a74273fd2bbe84003c1261043c4b2f1ff29 100644
--- a/utils/nfsdctl/nfsdctl.c
+++ b/utils/nfsdctl/nfsdctl.c
@@ -1550,7 +1550,7 @@ int main(int argc, char **argv)
xlog_stderr(0);
break;
case 'V':
- // FIXME: print_version();
+ fprintf(stdout, "nfsdctl: " VERSION "\n");
return 0;
}
}
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
2025-01-10 13:46 ` [PATCH v2 1/3] nfsdctl: convert to xlog() Jeff Layton
2025-01-10 13:46 ` [PATCH v2 2/3] nfsdctl: fix the --version option Jeff Layton
@ 2025-01-10 13:46 ` Jeff Layton
2025-01-10 15:05 ` Tom Talpey
2025-01-14 21:09 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Scott Mayhew
2025-03-19 19:47 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Steve Dickson
4 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-10 13:46 UTC (permalink / raw)
To: Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs, Jeff Layton
The legacy rpc.nfsd program would configure the nlm_grace_period to
match the NFSv4 grace period when starting the server. This adds similar
functionality to the autostart subcommand using the new lockd netlink
configuration interfaces. In addition, if lockd's udp or tcp listener
ports are specified, also configure them before starting the server.
A "nlm" subcommand is also added that will display the current lockd
config settings in the current net ns. In the future, we could add the
ability to set these values using the "nlm" subcommand, but for now I
didn't see a real use-case for that, so I left it out.
Link: https://issues.redhat.com/browse/RHEL-71698
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
configure.ac | 4 +
utils/nfsdctl/lockd_netlink.h | 29 ++++++
utils/nfsdctl/nfsdctl.8 | 6 ++
utils/nfsdctl/nfsdctl.adoc | 5 +
utils/nfsdctl/nfsdctl.c | 218 +++++++++++++++++++++++++++++++++++++++---
5 files changed, 249 insertions(+), 13 deletions(-)
diff --git a/configure.ac b/configure.ac
index 561e894dc46f48997df4ba6dc3e7691876589fdb..1d865fbc1c7f79e3ac6152bc59995e99fe10a38e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -268,6 +268,10 @@ AC_ARG_ENABLE(nfsdctl,
[[int foo = NFSD_CMD_POOL_MODE_GET;]])],
[AC_DEFINE([USE_SYSTEM_NFSD_NETLINK_H], 1,
["Use system's linux/nfsd_netlink.h"])])
+ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <linux/lockd_netlink.h>]],
+ [[int foo = LOCKD_CMD_SERVER_GET;]])],
+ [AC_DEFINE([USE_SYSTEM_LOCKD_NETLINK_H], 1,
+ ["Use system's linux/lockd_netlink.h"])])
fi
AC_ARG_ENABLE(nfsv4server,
diff --git a/utils/nfsdctl/lockd_netlink.h b/utils/nfsdctl/lockd_netlink.h
new file mode 100644
index 0000000000000000000000000000000000000000..21c65aec3bc6d1839961937081e6d16540332179
--- /dev/null
+++ b/utils/nfsdctl/lockd_netlink.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
+/* Do not edit directly, auto-generated from: */
+/* Documentation/netlink/specs/lockd.yaml */
+/* YNL-GEN uapi header */
+
+#ifndef _UAPI_LINUX_LOCKD_NETLINK_H
+#define _UAPI_LINUX_LOCKD_NETLINK_H
+
+#define LOCKD_FAMILY_NAME "lockd"
+#define LOCKD_FAMILY_VERSION 1
+
+enum {
+ LOCKD_A_SERVER_GRACETIME = 1,
+ LOCKD_A_SERVER_TCP_PORT,
+ LOCKD_A_SERVER_UDP_PORT,
+
+ __LOCKD_A_SERVER_MAX,
+ LOCKD_A_SERVER_MAX = (__LOCKD_A_SERVER_MAX - 1)
+};
+
+enum {
+ LOCKD_CMD_SERVER_SET = 1,
+ LOCKD_CMD_SERVER_GET,
+
+ __LOCKD_CMD_MAX,
+ LOCKD_CMD_MAX = (__LOCKD_CMD_MAX - 1)
+};
+
+#endif /* _UAPI_LINUX_LOCKD_NETLINK_H */
diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
index 39ae1855ae50e94da113981d5e8cf8ac14440c3a..d69922448eb17fb155f05dc4ddc9aefffbf966e4 100644
--- a/utils/nfsdctl/nfsdctl.8
+++ b/utils/nfsdctl/nfsdctl.8
@@ -127,6 +127,12 @@ colon separated form, and must be enclosed in square brackets.
.if n .RE
.RE
.sp
+\fBnlm\fP
+.RS 4
+Get information about NLM (lockd) settings in the current net namespace. This
+subcommand takes no arguments.
+.RE
+.sp
\fBstatus\fP
.RS 4
Get information about RPCs currently executing in the server. This subcommand
diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
index 2114693042594297b7c5d8600ca16813a0f2356c..0207eff6118d2dcc5a794d2013c039d9beb11ddc 100644
--- a/utils/nfsdctl/nfsdctl.adoc
+++ b/utils/nfsdctl/nfsdctl.adoc
@@ -67,6 +67,11 @@ Each subcommand can also accept its own set of options and arguments. The
addresses must be in dotted-quad form. IPv6 addresses should be in standard
colon separated form, and must be enclosed in square brackets.
+*nlm*::
+
+ Get information about NLM (lockd) settings in the current net namespace. This
+ subcommand takes no arguments.
+
*status*::
Get information about RPCs currently executing in the server. This subcommand
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
index 5c319a74273fd2bbe84003c1261043c4b2f1ff29..003daba5f30a403eb4168f6103e5a496d96968a4 100644
--- a/utils/nfsdctl/nfsdctl.c
+++ b/utils/nfsdctl/nfsdctl.c
@@ -35,6 +35,12 @@
#include "nfsd_netlink.h"
#endif
+#ifdef USE_SYSTEM_LOCKD_NETLINK_H
+#include <linux/lockd_netlink.h>
+#else
+#include "lockd_netlink.h"
+#endif
+
#include "nfsdctl.h"
#include "conffile.h"
#include "xlog.h"
@@ -348,6 +354,28 @@ static void parse_pool_mode_get(struct genlmsghdr *gnlh)
}
}
+static void parse_lockd_get(struct genlmsghdr *gnlh)
+{
+ struct nlattr *attr;
+ int rem;
+
+ nla_for_each_attr(attr, genlmsg_attrdata(gnlh, 0),
+ genlmsg_attrlen(gnlh, 0), rem) {
+ switch (nla_type(attr)) {
+ case LOCKD_A_SERVER_GRACETIME:
+ printf("gracetime: %u\n", nla_get_u32(attr));
+ break;
+ case LOCKD_A_SERVER_TCP_PORT:
+ printf("tcp_port: %hu\n", nla_get_u16(attr));
+ break;
+ case LOCKD_A_SERVER_UDP_PORT:
+ printf("udp_port: %hu\n", nla_get_u16(attr));
+ break;
+ default:
+ break;
+ }
+ }
+}
static int recv_handler(struct nl_msg *msg, void *arg)
{
struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
@@ -368,6 +396,9 @@ static int recv_handler(struct nl_msg *msg, void *arg)
case NFSD_CMD_POOL_MODE_GET:
parse_pool_mode_get(gnlh);
break;
+ case LOCKD_CMD_SERVER_GET:
+ parse_lockd_get(gnlh);
+ break;
default:
break;
}
@@ -398,12 +429,12 @@ static struct nl_sock *netlink_sock_alloc(void)
return sock;
}
-static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock)
+static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family)
{
struct nl_msg *msg;
int id;
- id = genl_ctrl_resolve(sock, NFSD_FAMILY_NAME);
+ id = genl_ctrl_resolve(sock, family);
if (id < 0) {
xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
return NULL;
@@ -447,7 +478,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
}
}
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -495,7 +526,7 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
struct nl_cb *cb;
int ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -607,7 +638,7 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
struct nl_cb *cb;
int ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -672,7 +703,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
struct nl_cb *cb;
int i, ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -825,7 +856,7 @@ static int fetch_current_listeners(struct nl_sock *sock)
struct nl_cb *cb;
int ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -1054,7 +1085,7 @@ static int set_listeners(struct nl_sock *sock)
struct nl_cb *cb;
int i, ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -1170,7 +1201,7 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
struct nl_cb *cb;
int ret;
- msg = netlink_msg_alloc(sock);
+ msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
if (!msg)
return 1;
@@ -1249,6 +1280,131 @@ static int pool_mode_func(struct nl_sock *sock, int argc, char **argv)
return pool_mode_doit(sock, cmd, pool_mode);
}
+static int lockd_config_doit(struct nl_sock *sock, int cmd, int grace, int tcpport, int udpport)
+{
+ struct genlmsghdr *ghdr;
+ struct nlmsghdr *nlh;
+ struct nl_msg *msg;
+ struct nl_cb *cb;
+ int ret;
+
+ if (cmd == LOCKD_CMD_SERVER_SET) {
+ /* Do nothing if there is nothing to set */
+ if (!grace && !tcpport && !udpport)
+ return 0;
+ }
+
+ msg = netlink_msg_alloc(sock, LOCKD_FAMILY_NAME);
+ if (!msg)
+ return 1;
+
+ nlh = nlmsg_hdr(msg);
+ if (cmd == LOCKD_CMD_SERVER_SET) {
+ if (grace)
+ nla_put_u32(msg, LOCKD_A_SERVER_GRACETIME, grace);
+ if (tcpport)
+ nla_put_u16(msg, LOCKD_A_SERVER_TCP_PORT, tcpport);
+ if (udpport)
+ nla_put_u16(msg, LOCKD_A_SERVER_UDP_PORT, udpport);
+ }
+
+ ghdr = nlmsg_data(nlh);
+ ghdr->cmd = cmd;
+
+ cb = nl_cb_alloc(NL_CB_CUSTOM);
+ if (!cb) {
+ xlog(L_ERROR, "failed to allocate netlink callbacks\n");
+ ret = 1;
+ goto out;
+ }
+
+ ret = nl_send_auto(sock, msg);
+ if (ret < 0) {
+ xlog(L_ERROR, "send failed (%d)!\n", ret);
+ goto out_cb;
+ }
+
+ ret = 1;
+ nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &ret);
+ nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, finish_handler, &ret);
+ nl_cb_set(cb, NL_CB_ACK, NL_CB_CUSTOM, ack_handler, &ret);
+ nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, recv_handler, NULL);
+
+ while (ret > 0)
+ nl_recvmsgs(sock, cb);
+ if (ret < 0) {
+ xlog(L_ERROR, "Error: %s\n", strerror(-ret));
+ ret = 1;
+ }
+out_cb:
+ nl_cb_put(cb);
+out:
+ nlmsg_free(msg);
+ return ret;
+}
+
+static int get_service(const char *svc)
+{
+ struct addrinfo *res, hints = { .ai_flags = AI_PASSIVE };
+ int ret, port;
+
+ if (!svc)
+ return 0;
+
+ ret = getaddrinfo(NULL, svc, &hints, &res);
+ if (ret) {
+ xlog(L_ERROR, "getaddrinfo of \"%s\" failed: %s\n",
+ svc, gai_strerror(ret));
+ return -1;
+ }
+
+ switch (res->ai_family) {
+ case AF_INET:
+ {
+ struct sockaddr_in *sin = (struct sockaddr_in *)res->ai_addr;
+
+ port = ntohs(sin->sin_port);
+ }
+ break;
+ case AF_INET6:
+ {
+ struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
+
+ port = ntohs(sin6->sin6_port);
+ }
+ break;
+ default:
+ xlog(L_ERROR, "Bad address family: %d\n", res->ai_family);
+ port = -1;
+ }
+ freeaddrinfo(res);
+ return port;
+}
+
+static int lockd_configure(struct nl_sock *sock, int grace)
+{
+ char *tcp_svc, *udp_svc;
+ int tcpport = 0, udpport = 0;
+ int ret;
+
+ tcp_svc = conf_get_str("lockd", "port");
+ if (tcp_svc) {
+ tcpport = get_service(tcp_svc);
+ if (tcpport < 0)
+ return 1;
+ }
+
+ udp_svc = conf_get_str("lockd", "udp-port");
+ if (udp_svc) {
+ udpport = get_service(udp_svc);
+ if (udpport < 0)
+ return 1;
+ }
+
+ return lockd_config_doit(sock, LOCKD_CMD_SERVER_SET, grace, tcpport, udpport);
+}
+
+
#define MAX_LISTENER_LEN (64 * 2 + 16)
static void
@@ -1355,6 +1511,13 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
read_nfsd_conf();
+ grace = conf_get_num("nfsd", "grace-time", 0);
+ ret = lockd_configure(sock, grace);
+ if (ret) {
+ xlog(L_ERROR, "lockd configuration failure");
+ return ret;
+ }
+
pool_mode = conf_get_str("nfsd", "pool-mode");
if (pool_mode) {
ret = pool_mode_doit(sock, NFSD_CMD_POOL_MODE_SET, pool_mode);
@@ -1370,15 +1533,12 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
if (ret)
return ret;
+ xlog(D_GENERAL, "configuring listeners");
configure_listeners();
ret = set_listeners(sock);
if (ret)
return ret;
- grace = conf_get_num("nfsd", "grace-time", 0);
- lease = conf_get_num("nfsd", "lease-time", 0);
- scope = conf_get_str("nfsd", "scope");
-
thread_str = conf_get_list("nfsd", "threads");
pools = thread_str ? thread_str->cnt : 1;
@@ -1402,6 +1562,9 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
threads[0] = DEFAULT_AUTOSTART_THREADS;
}
+ lease = conf_get_num("nfsd", "lease-time", 0);
+ scope = conf_get_str("nfsd", "scope");
+
ret = threads_doit(sock, NFSD_CMD_THREADS_SET, grace, lease, pools,
threads, scope);
out:
@@ -1409,6 +1572,30 @@ out:
return ret;
}
+static void nlm_usage(void)
+{
+ printf("Usage: %s nlm\n", taskname);
+ printf(" Get the current settings for the NLM (lockd) server.\n");
+}
+
+static int nlm_func(struct nl_sock *sock, int argc, char ** argv)
+{
+ int *threads, grace, lease, idx, ret, opt, pools;
+ struct conf_list *thread_str;
+ struct conf_list_node *n;
+ char *scope, *pool_mode;
+
+ optind = 1;
+ while ((opt = getopt_long(argc, argv, "h", help_only_options, NULL)) != -1) {
+ switch (opt) {
+ case 'h':
+ nlm_usage();
+ return 0;
+ }
+ }
+ return lockd_config_doit(sock, LOCKD_CMD_SERVER_GET, 0, 0, 0);
+}
+
enum nfsdctl_commands {
NFSDCTL_STATUS,
NFSDCTL_THREADS,
@@ -1416,6 +1603,7 @@ enum nfsdctl_commands {
NFSDCTL_LISTENER,
NFSDCTL_AUTOSTART,
NFSDCTL_POOL_MODE,
+ NFSDCTL_NLM,
};
static int parse_command(char *str)
@@ -1432,6 +1620,8 @@ static int parse_command(char *str)
return NFSDCTL_AUTOSTART;
if (!strcmp(str, "pool-mode"))
return NFSDCTL_POOL_MODE;
+ if (!strcmp(str, "nlm"))
+ return NFSDCTL_NLM;
return -1;
}
@@ -1444,6 +1634,7 @@ static nfsdctl_func func[] = {
[NFSDCTL_LISTENER] = listener_func,
[NFSDCTL_AUTOSTART] = autostart_func,
[NFSDCTL_POOL_MODE] = pool_mode_func,
+ [NFSDCTL_NLM] = nlm_func,
};
static void usage(void)
@@ -1460,6 +1651,7 @@ static void usage(void)
printf(" listener get/set listener info\n");
printf(" version get/set supported NFS versions\n");
printf(" threads get/set nfsd thread settings\n");
+ printf(" nlm get current nlm settings\n");
printf(" status get current RPC processing info\n");
printf(" autostart start server with settings from /etc/nfs.conf\n");
}
--
2.47.1
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-10 13:46 ` [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd Jeff Layton
@ 2025-01-10 15:05 ` Tom Talpey
2025-01-10 15:21 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Tom Talpey @ 2025-01-10 15:05 UTC (permalink / raw)
To: Jeff Layton, Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs
On 1/10/2025 8:46 AM, Jeff Layton wrote:
> The legacy rpc.nfsd program would configure the nlm_grace_period to
> match the NFSv4 grace period when starting the server. This adds similar
> functionality to the autostart subcommand using the new lockd netlink
> configuration interfaces. In addition, if lockd's udp or tcp listener
> ports are specified, also configure them before starting the server.
>
> A "nlm" subcommand is also added that will display the current lockd
> config settings in the current net ns. In the future, we could add the
> ability to set these values using the "nlm" subcommand, but for now I
> didn't see a real use-case for that, so I left it out.
It seems unnatural that the nlm_grace_period is tied to the netns.
It seems to me it's more dependent on the network and its likely
failure modes, the backend storage/filesystem, and perhaps the
scale of clients performing possibly-conflicting locks. Oh, and
also perhaps the minor version, since 4.1+ have the RECLAIM_COMPLETE
termination event.
Food for thought, perhaps.
Tom.
>
> Link: https://issues.redhat.com/browse/RHEL-71698
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> configure.ac | 4 +
> utils/nfsdctl/lockd_netlink.h | 29 ++++++
> utils/nfsdctl/nfsdctl.8 | 6 ++
> utils/nfsdctl/nfsdctl.adoc | 5 +
> utils/nfsdctl/nfsdctl.c | 218 +++++++++++++++++++++++++++++++++++++++---
> 5 files changed, 249 insertions(+), 13 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 561e894dc46f48997df4ba6dc3e7691876589fdb..1d865fbc1c7f79e3ac6152bc59995e99fe10a38e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -268,6 +268,10 @@ AC_ARG_ENABLE(nfsdctl,
> [[int foo = NFSD_CMD_POOL_MODE_GET;]])],
> [AC_DEFINE([USE_SYSTEM_NFSD_NETLINK_H], 1,
> ["Use system's linux/nfsd_netlink.h"])])
> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <linux/lockd_netlink.h>]],
> + [[int foo = LOCKD_CMD_SERVER_GET;]])],
> + [AC_DEFINE([USE_SYSTEM_LOCKD_NETLINK_H], 1,
> + ["Use system's linux/lockd_netlink.h"])])
> fi
>
> AC_ARG_ENABLE(nfsv4server,
> diff --git a/utils/nfsdctl/lockd_netlink.h b/utils/nfsdctl/lockd_netlink.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..21c65aec3bc6d1839961937081e6d16540332179
> --- /dev/null
> +++ b/utils/nfsdctl/lockd_netlink.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
> +/* Do not edit directly, auto-generated from: */
> +/* Documentation/netlink/specs/lockd.yaml */
> +/* YNL-GEN uapi header */
> +
> +#ifndef _UAPI_LINUX_LOCKD_NETLINK_H
> +#define _UAPI_LINUX_LOCKD_NETLINK_H
> +
> +#define LOCKD_FAMILY_NAME "lockd"
> +#define LOCKD_FAMILY_VERSION 1
> +
> +enum {
> + LOCKD_A_SERVER_GRACETIME = 1,
> + LOCKD_A_SERVER_TCP_PORT,
> + LOCKD_A_SERVER_UDP_PORT,
> +
> + __LOCKD_A_SERVER_MAX,
> + LOCKD_A_SERVER_MAX = (__LOCKD_A_SERVER_MAX - 1)
> +};
> +
> +enum {
> + LOCKD_CMD_SERVER_SET = 1,
> + LOCKD_CMD_SERVER_GET,
> +
> + __LOCKD_CMD_MAX,
> + LOCKD_CMD_MAX = (__LOCKD_CMD_MAX - 1)
> +};
> +
> +#endif /* _UAPI_LINUX_LOCKD_NETLINK_H */
> diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
> index 39ae1855ae50e94da113981d5e8cf8ac14440c3a..d69922448eb17fb155f05dc4ddc9aefffbf966e4 100644
> --- a/utils/nfsdctl/nfsdctl.8
> +++ b/utils/nfsdctl/nfsdctl.8
> @@ -127,6 +127,12 @@ colon separated form, and must be enclosed in square brackets.
> .if n .RE
> .RE
> .sp
> +\fBnlm\fP
> +.RS 4
> +Get information about NLM (lockd) settings in the current net namespace. This
> +subcommand takes no arguments.
> +.RE
> +.sp
> \fBstatus\fP
> .RS 4
> Get information about RPCs currently executing in the server. This subcommand
> diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
> index 2114693042594297b7c5d8600ca16813a0f2356c..0207eff6118d2dcc5a794d2013c039d9beb11ddc 100644
> --- a/utils/nfsdctl/nfsdctl.adoc
> +++ b/utils/nfsdctl/nfsdctl.adoc
> @@ -67,6 +67,11 @@ Each subcommand can also accept its own set of options and arguments. The
> addresses must be in dotted-quad form. IPv6 addresses should be in standard
> colon separated form, and must be enclosed in square brackets.
>
> +*nlm*::
> +
> + Get information about NLM (lockd) settings in the current net namespace. This
> + subcommand takes no arguments.
> +
> *status*::
>
> Get information about RPCs currently executing in the server. This subcommand
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 5c319a74273fd2bbe84003c1261043c4b2f1ff29..003daba5f30a403eb4168f6103e5a496d96968a4 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -35,6 +35,12 @@
> #include "nfsd_netlink.h"
> #endif
>
> +#ifdef USE_SYSTEM_LOCKD_NETLINK_H
> +#include <linux/lockd_netlink.h>
> +#else
> +#include "lockd_netlink.h"
> +#endif
> +
> #include "nfsdctl.h"
> #include "conffile.h"
> #include "xlog.h"
> @@ -348,6 +354,28 @@ static void parse_pool_mode_get(struct genlmsghdr *gnlh)
> }
> }
>
> +static void parse_lockd_get(struct genlmsghdr *gnlh)
> +{
> + struct nlattr *attr;
> + int rem;
> +
> + nla_for_each_attr(attr, genlmsg_attrdata(gnlh, 0),
> + genlmsg_attrlen(gnlh, 0), rem) {
> + switch (nla_type(attr)) {
> + case LOCKD_A_SERVER_GRACETIME:
> + printf("gracetime: %u\n", nla_get_u32(attr));
> + break;
> + case LOCKD_A_SERVER_TCP_PORT:
> + printf("tcp_port: %hu\n", nla_get_u16(attr));
> + break;
> + case LOCKD_A_SERVER_UDP_PORT:
> + printf("udp_port: %hu\n", nla_get_u16(attr));
> + break;
> + default:
> + break;
> + }
> + }
> +}
> static int recv_handler(struct nl_msg *msg, void *arg)
> {
> struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
> @@ -368,6 +396,9 @@ static int recv_handler(struct nl_msg *msg, void *arg)
> case NFSD_CMD_POOL_MODE_GET:
> parse_pool_mode_get(gnlh);
> break;
> + case LOCKD_CMD_SERVER_GET:
> + parse_lockd_get(gnlh);
> + break;
> default:
> break;
> }
> @@ -398,12 +429,12 @@ static struct nl_sock *netlink_sock_alloc(void)
> return sock;
> }
>
> -static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock)
> +static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family)
> {
> struct nl_msg *msg;
> int id;
>
> - id = genl_ctrl_resolve(sock, NFSD_FAMILY_NAME);
> + id = genl_ctrl_resolve(sock, family);
> if (id < 0) {
> xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> return NULL;
> @@ -447,7 +478,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
> }
> }
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -495,7 +526,7 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
> struct nl_cb *cb;
> int ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -607,7 +638,7 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
> struct nl_cb *cb;
> int ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -672,7 +703,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
> struct nl_cb *cb;
> int i, ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -825,7 +856,7 @@ static int fetch_current_listeners(struct nl_sock *sock)
> struct nl_cb *cb;
> int ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -1054,7 +1085,7 @@ static int set_listeners(struct nl_sock *sock)
> struct nl_cb *cb;
> int i, ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -1170,7 +1201,7 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
> struct nl_cb *cb;
> int ret;
>
> - msg = netlink_msg_alloc(sock);
> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> if (!msg)
> return 1;
>
> @@ -1249,6 +1280,131 @@ static int pool_mode_func(struct nl_sock *sock, int argc, char **argv)
> return pool_mode_doit(sock, cmd, pool_mode);
> }
>
> +static int lockd_config_doit(struct nl_sock *sock, int cmd, int grace, int tcpport, int udpport)
> +{
> + struct genlmsghdr *ghdr;
> + struct nlmsghdr *nlh;
> + struct nl_msg *msg;
> + struct nl_cb *cb;
> + int ret;
> +
> + if (cmd == LOCKD_CMD_SERVER_SET) {
> + /* Do nothing if there is nothing to set */
> + if (!grace && !tcpport && !udpport)
> + return 0;
> + }
> +
> + msg = netlink_msg_alloc(sock, LOCKD_FAMILY_NAME);
> + if (!msg)
> + return 1;
> +
> + nlh = nlmsg_hdr(msg);
> + if (cmd == LOCKD_CMD_SERVER_SET) {
> + if (grace)
> + nla_put_u32(msg, LOCKD_A_SERVER_GRACETIME, grace);
> + if (tcpport)
> + nla_put_u16(msg, LOCKD_A_SERVER_TCP_PORT, tcpport);
> + if (udpport)
> + nla_put_u16(msg, LOCKD_A_SERVER_UDP_PORT, udpport);
> + }
> +
> + ghdr = nlmsg_data(nlh);
> + ghdr->cmd = cmd;
> +
> + cb = nl_cb_alloc(NL_CB_CUSTOM);
> + if (!cb) {
> + xlog(L_ERROR, "failed to allocate netlink callbacks\n");
> + ret = 1;
> + goto out;
> + }
> +
> + ret = nl_send_auto(sock, msg);
> + if (ret < 0) {
> + xlog(L_ERROR, "send failed (%d)!\n", ret);
> + goto out_cb;
> + }
> +
> + ret = 1;
> + nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &ret);
> + nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, finish_handler, &ret);
> + nl_cb_set(cb, NL_CB_ACK, NL_CB_CUSTOM, ack_handler, &ret);
> + nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, recv_handler, NULL);
> +
> + while (ret > 0)
> + nl_recvmsgs(sock, cb);
> + if (ret < 0) {
> + xlog(L_ERROR, "Error: %s\n", strerror(-ret));
> + ret = 1;
> + }
> +out_cb:
> + nl_cb_put(cb);
> +out:
> + nlmsg_free(msg);
> + return ret;
> +}
> +
> +static int get_service(const char *svc)
> +{
> + struct addrinfo *res, hints = { .ai_flags = AI_PASSIVE };
> + int ret, port;
> +
> + if (!svc)
> + return 0;
> +
> + ret = getaddrinfo(NULL, svc, &hints, &res);
> + if (ret) {
> + xlog(L_ERROR, "getaddrinfo of \"%s\" failed: %s\n",
> + svc, gai_strerror(ret));
> + return -1;
> + }
> +
> + switch (res->ai_family) {
> + case AF_INET:
> + {
> + struct sockaddr_in *sin = (struct sockaddr_in *)res->ai_addr;
> +
> + port = ntohs(sin->sin_port);
> + }
> + break;
> + case AF_INET6:
> + {
> + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
> +
> + port = ntohs(sin6->sin6_port);
> + }
> + break;
> + default:
> + xlog(L_ERROR, "Bad address family: %d\n", res->ai_family);
> + port = -1;
> + }
> + freeaddrinfo(res);
> + return port;
> +}
> +
> +static int lockd_configure(struct nl_sock *sock, int grace)
> +{
> + char *tcp_svc, *udp_svc;
> + int tcpport = 0, udpport = 0;
> + int ret;
> +
> + tcp_svc = conf_get_str("lockd", "port");
> + if (tcp_svc) {
> + tcpport = get_service(tcp_svc);
> + if (tcpport < 0)
> + return 1;
> + }
> +
> + udp_svc = conf_get_str("lockd", "udp-port");
> + if (udp_svc) {
> + udpport = get_service(udp_svc);
> + if (udpport < 0)
> + return 1;
> + }
> +
> + return lockd_config_doit(sock, LOCKD_CMD_SERVER_SET, grace, tcpport, udpport);
> +}
> +
> +
> #define MAX_LISTENER_LEN (64 * 2 + 16)
>
> static void
> @@ -1355,6 +1511,13 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>
> read_nfsd_conf();
>
> + grace = conf_get_num("nfsd", "grace-time", 0);
> + ret = lockd_configure(sock, grace);
> + if (ret) {
> + xlog(L_ERROR, "lockd configuration failure");
> + return ret;
> + }
> +
> pool_mode = conf_get_str("nfsd", "pool-mode");
> if (pool_mode) {
> ret = pool_mode_doit(sock, NFSD_CMD_POOL_MODE_SET, pool_mode);
> @@ -1370,15 +1533,12 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> if (ret)
> return ret;
>
> + xlog(D_GENERAL, "configuring listeners");
> configure_listeners();
> ret = set_listeners(sock);
> if (ret)
> return ret;
>
> - grace = conf_get_num("nfsd", "grace-time", 0);
> - lease = conf_get_num("nfsd", "lease-time", 0);
> - scope = conf_get_str("nfsd", "scope");
> -
> thread_str = conf_get_list("nfsd", "threads");
> pools = thread_str ? thread_str->cnt : 1;
>
> @@ -1402,6 +1562,9 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> threads[0] = DEFAULT_AUTOSTART_THREADS;
> }
>
> + lease = conf_get_num("nfsd", "lease-time", 0);
> + scope = conf_get_str("nfsd", "scope");
> +
> ret = threads_doit(sock, NFSD_CMD_THREADS_SET, grace, lease, pools,
> threads, scope);
> out:
> @@ -1409,6 +1572,30 @@ out:
> return ret;
> }
>
> +static void nlm_usage(void)
> +{
> + printf("Usage: %s nlm\n", taskname);
> + printf(" Get the current settings for the NLM (lockd) server.\n");
> +}
> +
> +static int nlm_func(struct nl_sock *sock, int argc, char ** argv)
> +{
> + int *threads, grace, lease, idx, ret, opt, pools;
> + struct conf_list *thread_str;
> + struct conf_list_node *n;
> + char *scope, *pool_mode;
> +
> + optind = 1;
> + while ((opt = getopt_long(argc, argv, "h", help_only_options, NULL)) != -1) {
> + switch (opt) {
> + case 'h':
> + nlm_usage();
> + return 0;
> + }
> + }
> + return lockd_config_doit(sock, LOCKD_CMD_SERVER_GET, 0, 0, 0);
> +}
> +
> enum nfsdctl_commands {
> NFSDCTL_STATUS,
> NFSDCTL_THREADS,
> @@ -1416,6 +1603,7 @@ enum nfsdctl_commands {
> NFSDCTL_LISTENER,
> NFSDCTL_AUTOSTART,
> NFSDCTL_POOL_MODE,
> + NFSDCTL_NLM,
> };
>
> static int parse_command(char *str)
> @@ -1432,6 +1620,8 @@ static int parse_command(char *str)
> return NFSDCTL_AUTOSTART;
> if (!strcmp(str, "pool-mode"))
> return NFSDCTL_POOL_MODE;
> + if (!strcmp(str, "nlm"))
> + return NFSDCTL_NLM;
> return -1;
> }
>
> @@ -1444,6 +1634,7 @@ static nfsdctl_func func[] = {
> [NFSDCTL_LISTENER] = listener_func,
> [NFSDCTL_AUTOSTART] = autostart_func,
> [NFSDCTL_POOL_MODE] = pool_mode_func,
> + [NFSDCTL_NLM] = nlm_func,
> };
>
> static void usage(void)
> @@ -1460,6 +1651,7 @@ static void usage(void)
> printf(" listener get/set listener info\n");
> printf(" version get/set supported NFS versions\n");
> printf(" threads get/set nfsd thread settings\n");
> + printf(" nlm get current nlm settings\n");
> printf(" status get current RPC processing info\n");
> printf(" autostart start server with settings from /etc/nfs.conf\n");
> }
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-10 15:05 ` Tom Talpey
@ 2025-01-10 15:21 ` Jeff Layton
2025-01-10 15:40 ` Tom Talpey
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-10 15:21 UTC (permalink / raw)
To: Tom Talpey, Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs
On Fri, 2025-01-10 at 10:05 -0500, Tom Talpey wrote:
> On 1/10/2025 8:46 AM, Jeff Layton wrote:
> > The legacy rpc.nfsd program would configure the nlm_grace_period to
> > match the NFSv4 grace period when starting the server. This adds similar
> > functionality to the autostart subcommand using the new lockd netlink
> > configuration interfaces. In addition, if lockd's udp or tcp listener
> > ports are specified, also configure them before starting the server.
> >
> > A "nlm" subcommand is also added that will display the current lockd
> > config settings in the current net ns. In the future, we could add the
> > ability to set these values using the "nlm" subcommand, but for now I
> > didn't see a real use-case for that, so I left it out.
>
> It seems unnatural that the nlm_grace_period is tied to the netns.
>
> It seems to me it's more dependent on the network and its likely
> failure modes, the backend storage/filesystem, and perhaps the
> scale of clients performing possibly-conflicting locks. Oh, and
> also perhaps the minor version, since 4.1+ have the RECLAIM_COMPLETE
> termination event.
>
> Food for thought, perhaps.
>
> Tom.
>
Fair point. More food:
My guess is that nlm_grace_period handling is likely horribly broken in
multi-container scenarios anyway. If you have multiple nfs server
containers with different grace period settings, then they will all be
competing to set the global nlm_grace_period.
Most clients end up reclaiming quickly enough that we don't hit major
problems here, but it would be good to make this more "airtight".
> >
> > Link: https://issues.redhat.com/browse/RHEL-71698
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > configure.ac | 4 +
> > utils/nfsdctl/lockd_netlink.h | 29 ++++++
> > utils/nfsdctl/nfsdctl.8 | 6 ++
> > utils/nfsdctl/nfsdctl.adoc | 5 +
> > utils/nfsdctl/nfsdctl.c | 218 +++++++++++++++++++++++++++++++++++++++---
> > 5 files changed, 249 insertions(+), 13 deletions(-)
> >
> > diff --git a/configure.ac b/configure.ac
> > index 561e894dc46f48997df4ba6dc3e7691876589fdb..1d865fbc1c7f79e3ac6152bc59995e99fe10a38e 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -268,6 +268,10 @@ AC_ARG_ENABLE(nfsdctl,
> > [[int foo = NFSD_CMD_POOL_MODE_GET;]])],
> > [AC_DEFINE([USE_SYSTEM_NFSD_NETLINK_H], 1,
> > ["Use system's linux/nfsd_netlink.h"])])
> > + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <linux/lockd_netlink.h>]],
> > + [[int foo = LOCKD_CMD_SERVER_GET;]])],
> > + [AC_DEFINE([USE_SYSTEM_LOCKD_NETLINK_H], 1,
> > + ["Use system's linux/lockd_netlink.h"])])
> > fi
> >
> > AC_ARG_ENABLE(nfsv4server,
> > diff --git a/utils/nfsdctl/lockd_netlink.h b/utils/nfsdctl/lockd_netlink.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..21c65aec3bc6d1839961937081e6d16540332179
> > --- /dev/null
> > +++ b/utils/nfsdctl/lockd_netlink.h
> > @@ -0,0 +1,29 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
> > +/* Do not edit directly, auto-generated from: */
> > +/* Documentation/netlink/specs/lockd.yaml */
> > +/* YNL-GEN uapi header */
> > +
> > +#ifndef _UAPI_LINUX_LOCKD_NETLINK_H
> > +#define _UAPI_LINUX_LOCKD_NETLINK_H
> > +
> > +#define LOCKD_FAMILY_NAME "lockd"
> > +#define LOCKD_FAMILY_VERSION 1
> > +
> > +enum {
> > + LOCKD_A_SERVER_GRACETIME = 1,
> > + LOCKD_A_SERVER_TCP_PORT,
> > + LOCKD_A_SERVER_UDP_PORT,
> > +
> > + __LOCKD_A_SERVER_MAX,
> > + LOCKD_A_SERVER_MAX = (__LOCKD_A_SERVER_MAX - 1)
> > +};
> > +
> > +enum {
> > + LOCKD_CMD_SERVER_SET = 1,
> > + LOCKD_CMD_SERVER_GET,
> > +
> > + __LOCKD_CMD_MAX,
> > + LOCKD_CMD_MAX = (__LOCKD_CMD_MAX - 1)
> > +};
> > +
> > +#endif /* _UAPI_LINUX_LOCKD_NETLINK_H */
> > diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
> > index 39ae1855ae50e94da113981d5e8cf8ac14440c3a..d69922448eb17fb155f05dc4ddc9aefffbf966e4 100644
> > --- a/utils/nfsdctl/nfsdctl.8
> > +++ b/utils/nfsdctl/nfsdctl.8
> > @@ -127,6 +127,12 @@ colon separated form, and must be enclosed in square brackets.
> > .if n .RE
> > .RE
> > .sp
> > +\fBnlm\fP
> > +.RS 4
> > +Get information about NLM (lockd) settings in the current net namespace. This
> > +subcommand takes no arguments.
> > +.RE
> > +.sp
> > \fBstatus\fP
> > .RS 4
> > Get information about RPCs currently executing in the server. This subcommand
> > diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
> > index 2114693042594297b7c5d8600ca16813a0f2356c..0207eff6118d2dcc5a794d2013c039d9beb11ddc 100644
> > --- a/utils/nfsdctl/nfsdctl.adoc
> > +++ b/utils/nfsdctl/nfsdctl.adoc
> > @@ -67,6 +67,11 @@ Each subcommand can also accept its own set of options and arguments. The
> > addresses must be in dotted-quad form. IPv6 addresses should be in standard
> > colon separated form, and must be enclosed in square brackets.
> >
> > +*nlm*::
> > +
> > + Get information about NLM (lockd) settings in the current net namespace. This
> > + subcommand takes no arguments.
> > +
> > *status*::
> >
> > Get information about RPCs currently executing in the server. This subcommand
> > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > index 5c319a74273fd2bbe84003c1261043c4b2f1ff29..003daba5f30a403eb4168f6103e5a496d96968a4 100644
> > --- a/utils/nfsdctl/nfsdctl.c
> > +++ b/utils/nfsdctl/nfsdctl.c
> > @@ -35,6 +35,12 @@
> > #include "nfsd_netlink.h"
> > #endif
> >
> > +#ifdef USE_SYSTEM_LOCKD_NETLINK_H
> > +#include <linux/lockd_netlink.h>
> > +#else
> > +#include "lockd_netlink.h"
> > +#endif
> > +
> > #include "nfsdctl.h"
> > #include "conffile.h"
> > #include "xlog.h"
> > @@ -348,6 +354,28 @@ static void parse_pool_mode_get(struct genlmsghdr *gnlh)
> > }
> > }
> >
> > +static void parse_lockd_get(struct genlmsghdr *gnlh)
> > +{
> > + struct nlattr *attr;
> > + int rem;
> > +
> > + nla_for_each_attr(attr, genlmsg_attrdata(gnlh, 0),
> > + genlmsg_attrlen(gnlh, 0), rem) {
> > + switch (nla_type(attr)) {
> > + case LOCKD_A_SERVER_GRACETIME:
> > + printf("gracetime: %u\n", nla_get_u32(attr));
> > + break;
> > + case LOCKD_A_SERVER_TCP_PORT:
> > + printf("tcp_port: %hu\n", nla_get_u16(attr));
> > + break;
> > + case LOCKD_A_SERVER_UDP_PORT:
> > + printf("udp_port: %hu\n", nla_get_u16(attr));
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +}
> > static int recv_handler(struct nl_msg *msg, void *arg)
> > {
> > struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
> > @@ -368,6 +396,9 @@ static int recv_handler(struct nl_msg *msg, void *arg)
> > case NFSD_CMD_POOL_MODE_GET:
> > parse_pool_mode_get(gnlh);
> > break;
> > + case LOCKD_CMD_SERVER_GET:
> > + parse_lockd_get(gnlh);
> > + break;
> > default:
> > break;
> > }
> > @@ -398,12 +429,12 @@ static struct nl_sock *netlink_sock_alloc(void)
> > return sock;
> > }
> >
> > -static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock)
> > +static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family)
> > {
> > struct nl_msg *msg;
> > int id;
> >
> > - id = genl_ctrl_resolve(sock, NFSD_FAMILY_NAME);
> > + id = genl_ctrl_resolve(sock, family);
> > if (id < 0) {
> > xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> > return NULL;
> > @@ -447,7 +478,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
> > }
> > }
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -495,7 +526,7 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
> > struct nl_cb *cb;
> > int ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -607,7 +638,7 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
> > struct nl_cb *cb;
> > int ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -672,7 +703,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
> > struct nl_cb *cb;
> > int i, ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -825,7 +856,7 @@ static int fetch_current_listeners(struct nl_sock *sock)
> > struct nl_cb *cb;
> > int ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -1054,7 +1085,7 @@ static int set_listeners(struct nl_sock *sock)
> > struct nl_cb *cb;
> > int i, ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -1170,7 +1201,7 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
> > struct nl_cb *cb;
> > int ret;
> >
> > - msg = netlink_msg_alloc(sock);
> > + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
> > if (!msg)
> > return 1;
> >
> > @@ -1249,6 +1280,131 @@ static int pool_mode_func(struct nl_sock *sock, int argc, char **argv)
> > return pool_mode_doit(sock, cmd, pool_mode);
> > }
> >
> > +static int lockd_config_doit(struct nl_sock *sock, int cmd, int grace, int tcpport, int udpport)
> > +{
> > + struct genlmsghdr *ghdr;
> > + struct nlmsghdr *nlh;
> > + struct nl_msg *msg;
> > + struct nl_cb *cb;
> > + int ret;
> > +
> > + if (cmd == LOCKD_CMD_SERVER_SET) {
> > + /* Do nothing if there is nothing to set */
> > + if (!grace && !tcpport && !udpport)
> > + return 0;
> > + }
> > +
> > + msg = netlink_msg_alloc(sock, LOCKD_FAMILY_NAME);
> > + if (!msg)
> > + return 1;
> > +
> > + nlh = nlmsg_hdr(msg);
> > + if (cmd == LOCKD_CMD_SERVER_SET) {
> > + if (grace)
> > + nla_put_u32(msg, LOCKD_A_SERVER_GRACETIME, grace);
> > + if (tcpport)
> > + nla_put_u16(msg, LOCKD_A_SERVER_TCP_PORT, tcpport);
> > + if (udpport)
> > + nla_put_u16(msg, LOCKD_A_SERVER_UDP_PORT, udpport);
> > + }
> > +
> > + ghdr = nlmsg_data(nlh);
> > + ghdr->cmd = cmd;
> > +
> > + cb = nl_cb_alloc(NL_CB_CUSTOM);
> > + if (!cb) {
> > + xlog(L_ERROR, "failed to allocate netlink callbacks\n");
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + ret = nl_send_auto(sock, msg);
> > + if (ret < 0) {
> > + xlog(L_ERROR, "send failed (%d)!\n", ret);
> > + goto out_cb;
> > + }
> > +
> > + ret = 1;
> > + nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &ret);
> > + nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, finish_handler, &ret);
> > + nl_cb_set(cb, NL_CB_ACK, NL_CB_CUSTOM, ack_handler, &ret);
> > + nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, recv_handler, NULL);
> > +
> > + while (ret > 0)
> > + nl_recvmsgs(sock, cb);
> > + if (ret < 0) {
> > + xlog(L_ERROR, "Error: %s\n", strerror(-ret));
> > + ret = 1;
> > + }
> > +out_cb:
> > + nl_cb_put(cb);
> > +out:
> > + nlmsg_free(msg);
> > + return ret;
> > +}
> > +
> > +static int get_service(const char *svc)
> > +{
> > + struct addrinfo *res, hints = { .ai_flags = AI_PASSIVE };
> > + int ret, port;
> > +
> > + if (!svc)
> > + return 0;
> > +
> > + ret = getaddrinfo(NULL, svc, &hints, &res);
> > + if (ret) {
> > + xlog(L_ERROR, "getaddrinfo of \"%s\" failed: %s\n",
> > + svc, gai_strerror(ret));
> > + return -1;
> > + }
> > +
> > + switch (res->ai_family) {
> > + case AF_INET:
> > + {
> > + struct sockaddr_in *sin = (struct sockaddr_in *)res->ai_addr;
> > +
> > + port = ntohs(sin->sin_port);
> > + }
> > + break;
> > + case AF_INET6:
> > + {
> > + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
> > +
> > + port = ntohs(sin6->sin6_port);
> > + }
> > + break;
> > + default:
> > + xlog(L_ERROR, "Bad address family: %d\n", res->ai_family);
> > + port = -1;
> > + }
> > + freeaddrinfo(res);
> > + return port;
> > +}
> > +
> > +static int lockd_configure(struct nl_sock *sock, int grace)
> > +{
> > + char *tcp_svc, *udp_svc;
> > + int tcpport = 0, udpport = 0;
> > + int ret;
> > +
> > + tcp_svc = conf_get_str("lockd", "port");
> > + if (tcp_svc) {
> > + tcpport = get_service(tcp_svc);
> > + if (tcpport < 0)
> > + return 1;
> > + }
> > +
> > + udp_svc = conf_get_str("lockd", "udp-port");
> > + if (udp_svc) {
> > + udpport = get_service(udp_svc);
> > + if (udpport < 0)
> > + return 1;
> > + }
> > +
> > + return lockd_config_doit(sock, LOCKD_CMD_SERVER_SET, grace, tcpport, udpport);
> > +}
> > +
> > +
> > #define MAX_LISTENER_LEN (64 * 2 + 16)
> >
> > static void
> > @@ -1355,6 +1511,13 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> >
> > read_nfsd_conf();
> >
> > + grace = conf_get_num("nfsd", "grace-time", 0);
> > + ret = lockd_configure(sock, grace);
> > + if (ret) {
> > + xlog(L_ERROR, "lockd configuration failure");
> > + return ret;
> > + }
> > +
> > pool_mode = conf_get_str("nfsd", "pool-mode");
> > if (pool_mode) {
> > ret = pool_mode_doit(sock, NFSD_CMD_POOL_MODE_SET, pool_mode);
> > @@ -1370,15 +1533,12 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > if (ret)
> > return ret;
> >
> > + xlog(D_GENERAL, "configuring listeners");
> > configure_listeners();
> > ret = set_listeners(sock);
> > if (ret)
> > return ret;
> >
> > - grace = conf_get_num("nfsd", "grace-time", 0);
> > - lease = conf_get_num("nfsd", "lease-time", 0);
> > - scope = conf_get_str("nfsd", "scope");
> > -
> > thread_str = conf_get_list("nfsd", "threads");
> > pools = thread_str ? thread_str->cnt : 1;
> >
> > @@ -1402,6 +1562,9 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > threads[0] = DEFAULT_AUTOSTART_THREADS;
> > }
> >
> > + lease = conf_get_num("nfsd", "lease-time", 0);
> > + scope = conf_get_str("nfsd", "scope");
> > +
> > ret = threads_doit(sock, NFSD_CMD_THREADS_SET, grace, lease, pools,
> > threads, scope);
> > out:
> > @@ -1409,6 +1572,30 @@ out:
> > return ret;
> > }
> >
> > +static void nlm_usage(void)
> > +{
> > + printf("Usage: %s nlm\n", taskname);
> > + printf(" Get the current settings for the NLM (lockd) server.\n");
> > +}
> > +
> > +static int nlm_func(struct nl_sock *sock, int argc, char ** argv)
> > +{
> > + int *threads, grace, lease, idx, ret, opt, pools;
> > + struct conf_list *thread_str;
> > + struct conf_list_node *n;
> > + char *scope, *pool_mode;
> > +
> > + optind = 1;
> > + while ((opt = getopt_long(argc, argv, "h", help_only_options, NULL)) != -1) {
> > + switch (opt) {
> > + case 'h':
> > + nlm_usage();
> > + return 0;
> > + }
> > + }
> > + return lockd_config_doit(sock, LOCKD_CMD_SERVER_GET, 0, 0, 0);
> > +}
> > +
> > enum nfsdctl_commands {
> > NFSDCTL_STATUS,
> > NFSDCTL_THREADS,
> > @@ -1416,6 +1603,7 @@ enum nfsdctl_commands {
> > NFSDCTL_LISTENER,
> > NFSDCTL_AUTOSTART,
> > NFSDCTL_POOL_MODE,
> > + NFSDCTL_NLM,
> > };
> >
> > static int parse_command(char *str)
> > @@ -1432,6 +1620,8 @@ static int parse_command(char *str)
> > return NFSDCTL_AUTOSTART;
> > if (!strcmp(str, "pool-mode"))
> > return NFSDCTL_POOL_MODE;
> > + if (!strcmp(str, "nlm"))
> > + return NFSDCTL_NLM;
> > return -1;
> > }
> >
> > @@ -1444,6 +1634,7 @@ static nfsdctl_func func[] = {
> > [NFSDCTL_LISTENER] = listener_func,
> > [NFSDCTL_AUTOSTART] = autostart_func,
> > [NFSDCTL_POOL_MODE] = pool_mode_func,
> > + [NFSDCTL_NLM] = nlm_func,
> > };
> >
> > static void usage(void)
> > @@ -1460,6 +1651,7 @@ static void usage(void)
> > printf(" listener get/set listener info\n");
> > printf(" version get/set supported NFS versions\n");
> > printf(" threads get/set nfsd thread settings\n");
> > + printf(" nlm get current nlm settings\n");
> > printf(" status get current RPC processing info\n");
> > printf(" autostart start server with settings from /etc/nfs.conf\n");
> > }
> >
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-10 15:21 ` Jeff Layton
@ 2025-01-10 15:40 ` Tom Talpey
2025-01-13 13:39 ` Benjamin Coddington
0 siblings, 1 reply; 27+ messages in thread
From: Tom Talpey @ 2025-01-10 15:40 UTC (permalink / raw)
To: Jeff Layton, Steve Dickson; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs
On 1/10/2025 10:21 AM, Jeff Layton wrote:
> On Fri, 2025-01-10 at 10:05 -0500, Tom Talpey wrote:
>> On 1/10/2025 8:46 AM, Jeff Layton wrote:
>>> The legacy rpc.nfsd program would configure the nlm_grace_period to
>>> match the NFSv4 grace period when starting the server. This adds similar
>>> functionality to the autostart subcommand using the new lockd netlink
>>> configuration interfaces. In addition, if lockd's udp or tcp listener
>>> ports are specified, also configure them before starting the server.
>>>
>>> A "nlm" subcommand is also added that will display the current lockd
>>> config settings in the current net ns. In the future, we could add the
>>> ability to set these values using the "nlm" subcommand, but for now I
>>> didn't see a real use-case for that, so I left it out.
>>
>> It seems unnatural that the nlm_grace_period is tied to the netns.
>>
>> It seems to me it's more dependent on the network and its likely
>> failure modes, the backend storage/filesystem, and perhaps the
>> scale of clients performing possibly-conflicting locks. Oh, and
>> also perhaps the minor version, since 4.1+ have the RECLAIM_COMPLETE
>> termination event.
>>
>> Food for thought, perhaps.
>>
>> Tom.
>>
>
> Fair point. More food:
>
> My guess is that nlm_grace_period handling is likely horribly broken in
> multi-container scenarios anyway. If you have multiple nfs server
> containers with different grace period settings, then they will all be
> competing to set the global nlm_grace_period.
>
> Most clients end up reclaiming quickly enough that we don't hit major
> problems here, but it would be good to make this more "airtight".
The grace period can has been kicked down the road since nineteen
ninety ump so, sure, what the heck :)
I wonder how many sysadmins are actually changing it, and why. It's
not truly a correctness thing, more of an availability setting, so
the choice of value is pretty soft. As you say, if the clients all
reclaim promptly, nobody's the wiser. Do we have any data on how
often the setting actually gets used?
Tom.
>
>>>
>>> Link: https://issues.redhat.com/browse/RHEL-71698
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> configure.ac | 4 +
>>> utils/nfsdctl/lockd_netlink.h | 29 ++++++
>>> utils/nfsdctl/nfsdctl.8 | 6 ++
>>> utils/nfsdctl/nfsdctl.adoc | 5 +
>>> utils/nfsdctl/nfsdctl.c | 218 +++++++++++++++++++++++++++++++++++++++---
>>> 5 files changed, 249 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index 561e894dc46f48997df4ba6dc3e7691876589fdb..1d865fbc1c7f79e3ac6152bc59995e99fe10a38e 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -268,6 +268,10 @@ AC_ARG_ENABLE(nfsdctl,
>>> [[int foo = NFSD_CMD_POOL_MODE_GET;]])],
>>> [AC_DEFINE([USE_SYSTEM_NFSD_NETLINK_H], 1,
>>> ["Use system's linux/nfsd_netlink.h"])])
>>> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <linux/lockd_netlink.h>]],
>>> + [[int foo = LOCKD_CMD_SERVER_GET;]])],
>>> + [AC_DEFINE([USE_SYSTEM_LOCKD_NETLINK_H], 1,
>>> + ["Use system's linux/lockd_netlink.h"])])
>>> fi
>>>
>>> AC_ARG_ENABLE(nfsv4server,
>>> diff --git a/utils/nfsdctl/lockd_netlink.h b/utils/nfsdctl/lockd_netlink.h
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..21c65aec3bc6d1839961937081e6d16540332179
>>> --- /dev/null
>>> +++ b/utils/nfsdctl/lockd_netlink.h
>>> @@ -0,0 +1,29 @@
>>> +/* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) */
>>> +/* Do not edit directly, auto-generated from: */
>>> +/* Documentation/netlink/specs/lockd.yaml */
>>> +/* YNL-GEN uapi header */
>>> +
>>> +#ifndef _UAPI_LINUX_LOCKD_NETLINK_H
>>> +#define _UAPI_LINUX_LOCKD_NETLINK_H
>>> +
>>> +#define LOCKD_FAMILY_NAME "lockd"
>>> +#define LOCKD_FAMILY_VERSION 1
>>> +
>>> +enum {
>>> + LOCKD_A_SERVER_GRACETIME = 1,
>>> + LOCKD_A_SERVER_TCP_PORT,
>>> + LOCKD_A_SERVER_UDP_PORT,
>>> +
>>> + __LOCKD_A_SERVER_MAX,
>>> + LOCKD_A_SERVER_MAX = (__LOCKD_A_SERVER_MAX - 1)
>>> +};
>>> +
>>> +enum {
>>> + LOCKD_CMD_SERVER_SET = 1,
>>> + LOCKD_CMD_SERVER_GET,
>>> +
>>> + __LOCKD_CMD_MAX,
>>> + LOCKD_CMD_MAX = (__LOCKD_CMD_MAX - 1)
>>> +};
>>> +
>>> +#endif /* _UAPI_LINUX_LOCKD_NETLINK_H */
>>> diff --git a/utils/nfsdctl/nfsdctl.8 b/utils/nfsdctl/nfsdctl.8
>>> index 39ae1855ae50e94da113981d5e8cf8ac14440c3a..d69922448eb17fb155f05dc4ddc9aefffbf966e4 100644
>>> --- a/utils/nfsdctl/nfsdctl.8
>>> +++ b/utils/nfsdctl/nfsdctl.8
>>> @@ -127,6 +127,12 @@ colon separated form, and must be enclosed in square brackets.
>>> .if n .RE
>>> .RE
>>> .sp
>>> +\fBnlm\fP
>>> +.RS 4
>>> +Get information about NLM (lockd) settings in the current net namespace. This
>>> +subcommand takes no arguments.
>>> +.RE
>>> +.sp
>>> \fBstatus\fP
>>> .RS 4
>>> Get information about RPCs currently executing in the server. This subcommand
>>> diff --git a/utils/nfsdctl/nfsdctl.adoc b/utils/nfsdctl/nfsdctl.adoc
>>> index 2114693042594297b7c5d8600ca16813a0f2356c..0207eff6118d2dcc5a794d2013c039d9beb11ddc 100644
>>> --- a/utils/nfsdctl/nfsdctl.adoc
>>> +++ b/utils/nfsdctl/nfsdctl.adoc
>>> @@ -67,6 +67,11 @@ Each subcommand can also accept its own set of options and arguments. The
>>> addresses must be in dotted-quad form. IPv6 addresses should be in standard
>>> colon separated form, and must be enclosed in square brackets.
>>>
>>> +*nlm*::
>>> +
>>> + Get information about NLM (lockd) settings in the current net namespace. This
>>> + subcommand takes no arguments.
>>> +
>>> *status*::
>>>
>>> Get information about RPCs currently executing in the server. This subcommand
>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>> index 5c319a74273fd2bbe84003c1261043c4b2f1ff29..003daba5f30a403eb4168f6103e5a496d96968a4 100644
>>> --- a/utils/nfsdctl/nfsdctl.c
>>> +++ b/utils/nfsdctl/nfsdctl.c
>>> @@ -35,6 +35,12 @@
>>> #include "nfsd_netlink.h"
>>> #endif
>>>
>>> +#ifdef USE_SYSTEM_LOCKD_NETLINK_H
>>> +#include <linux/lockd_netlink.h>
>>> +#else
>>> +#include "lockd_netlink.h"
>>> +#endif
>>> +
>>> #include "nfsdctl.h"
>>> #include "conffile.h"
>>> #include "xlog.h"
>>> @@ -348,6 +354,28 @@ static void parse_pool_mode_get(struct genlmsghdr *gnlh)
>>> }
>>> }
>>>
>>> +static void parse_lockd_get(struct genlmsghdr *gnlh)
>>> +{
>>> + struct nlattr *attr;
>>> + int rem;
>>> +
>>> + nla_for_each_attr(attr, genlmsg_attrdata(gnlh, 0),
>>> + genlmsg_attrlen(gnlh, 0), rem) {
>>> + switch (nla_type(attr)) {
>>> + case LOCKD_A_SERVER_GRACETIME:
>>> + printf("gracetime: %u\n", nla_get_u32(attr));
>>> + break;
>>> + case LOCKD_A_SERVER_TCP_PORT:
>>> + printf("tcp_port: %hu\n", nla_get_u16(attr));
>>> + break;
>>> + case LOCKD_A_SERVER_UDP_PORT:
>>> + printf("udp_port: %hu\n", nla_get_u16(attr));
>>> + break;
>>> + default:
>>> + break;
>>> + }
>>> + }
>>> +}
>>> static int recv_handler(struct nl_msg *msg, void *arg)
>>> {
>>> struct genlmsghdr *gnlh = nlmsg_data(nlmsg_hdr(msg));
>>> @@ -368,6 +396,9 @@ static int recv_handler(struct nl_msg *msg, void *arg)
>>> case NFSD_CMD_POOL_MODE_GET:
>>> parse_pool_mode_get(gnlh);
>>> break;
>>> + case LOCKD_CMD_SERVER_GET:
>>> + parse_lockd_get(gnlh);
>>> + break;
>>> default:
>>> break;
>>> }
>>> @@ -398,12 +429,12 @@ static struct nl_sock *netlink_sock_alloc(void)
>>> return sock;
>>> }
>>>
>>> -static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock)
>>> +static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family)
>>> {
>>> struct nl_msg *msg;
>>> int id;
>>>
>>> - id = genl_ctrl_resolve(sock, NFSD_FAMILY_NAME);
>>> + id = genl_ctrl_resolve(sock, family);
>>> if (id < 0) {
>>> xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
>>> return NULL;
>>> @@ -447,7 +478,7 @@ static int status_func(struct nl_sock *sock, int argc, char ** argv)
>>> }
>>> }
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -495,7 +526,7 @@ static int threads_doit(struct nl_sock *sock, int cmd, int grace, int lease,
>>> struct nl_cb *cb;
>>> int ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -607,7 +638,7 @@ static int fetch_nfsd_versions(struct nl_sock *sock)
>>> struct nl_cb *cb;
>>> int ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -672,7 +703,7 @@ static int set_nfsd_versions(struct nl_sock *sock)
>>> struct nl_cb *cb;
>>> int i, ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -825,7 +856,7 @@ static int fetch_current_listeners(struct nl_sock *sock)
>>> struct nl_cb *cb;
>>> int ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -1054,7 +1085,7 @@ static int set_listeners(struct nl_sock *sock)
>>> struct nl_cb *cb;
>>> int i, ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -1170,7 +1201,7 @@ static int pool_mode_doit(struct nl_sock *sock, int cmd, const char *pool_mode)
>>> struct nl_cb *cb;
>>> int ret;
>>>
>>> - msg = netlink_msg_alloc(sock);
>>> + msg = netlink_msg_alloc(sock, NFSD_FAMILY_NAME);
>>> if (!msg)
>>> return 1;
>>>
>>> @@ -1249,6 +1280,131 @@ static int pool_mode_func(struct nl_sock *sock, int argc, char **argv)
>>> return pool_mode_doit(sock, cmd, pool_mode);
>>> }
>>>
>>> +static int lockd_config_doit(struct nl_sock *sock, int cmd, int grace, int tcpport, int udpport)
>>> +{
>>> + struct genlmsghdr *ghdr;
>>> + struct nlmsghdr *nlh;
>>> + struct nl_msg *msg;
>>> + struct nl_cb *cb;
>>> + int ret;
>>> +
>>> + if (cmd == LOCKD_CMD_SERVER_SET) {
>>> + /* Do nothing if there is nothing to set */
>>> + if (!grace && !tcpport && !udpport)
>>> + return 0;
>>> + }
>>> +
>>> + msg = netlink_msg_alloc(sock, LOCKD_FAMILY_NAME);
>>> + if (!msg)
>>> + return 1;
>>> +
>>> + nlh = nlmsg_hdr(msg);
>>> + if (cmd == LOCKD_CMD_SERVER_SET) {
>>> + if (grace)
>>> + nla_put_u32(msg, LOCKD_A_SERVER_GRACETIME, grace);
>>> + if (tcpport)
>>> + nla_put_u16(msg, LOCKD_A_SERVER_TCP_PORT, tcpport);
>>> + if (udpport)
>>> + nla_put_u16(msg, LOCKD_A_SERVER_UDP_PORT, udpport);
>>> + }
>>> +
>>> + ghdr = nlmsg_data(nlh);
>>> + ghdr->cmd = cmd;
>>> +
>>> + cb = nl_cb_alloc(NL_CB_CUSTOM);
>>> + if (!cb) {
>>> + xlog(L_ERROR, "failed to allocate netlink callbacks\n");
>>> + ret = 1;
>>> + goto out;
>>> + }
>>> +
>>> + ret = nl_send_auto(sock, msg);
>>> + if (ret < 0) {
>>> + xlog(L_ERROR, "send failed (%d)!\n", ret);
>>> + goto out_cb;
>>> + }
>>> +
>>> + ret = 1;
>>> + nl_cb_err(cb, NL_CB_CUSTOM, error_handler, &ret);
>>> + nl_cb_set(cb, NL_CB_FINISH, NL_CB_CUSTOM, finish_handler, &ret);
>>> + nl_cb_set(cb, NL_CB_ACK, NL_CB_CUSTOM, ack_handler, &ret);
>>> + nl_cb_set(cb, NL_CB_VALID, NL_CB_CUSTOM, recv_handler, NULL);
>>> +
>>> + while (ret > 0)
>>> + nl_recvmsgs(sock, cb);
>>> + if (ret < 0) {
>>> + xlog(L_ERROR, "Error: %s\n", strerror(-ret));
>>> + ret = 1;
>>> + }
>>> +out_cb:
>>> + nl_cb_put(cb);
>>> +out:
>>> + nlmsg_free(msg);
>>> + return ret;
>>> +}
>>> +
>>> +static int get_service(const char *svc)
>>> +{
>>> + struct addrinfo *res, hints = { .ai_flags = AI_PASSIVE };
>>> + int ret, port;
>>> +
>>> + if (!svc)
>>> + return 0;
>>> +
>>> + ret = getaddrinfo(NULL, svc, &hints, &res);
>>> + if (ret) {
>>> + xlog(L_ERROR, "getaddrinfo of \"%s\" failed: %s\n",
>>> + svc, gai_strerror(ret));
>>> + return -1;
>>> + }
>>> +
>>> + switch (res->ai_family) {
>>> + case AF_INET:
>>> + {
>>> + struct sockaddr_in *sin = (struct sockaddr_in *)res->ai_addr;
>>> +
>>> + port = ntohs(sin->sin_port);
>>> + }
>>> + break;
>>> + case AF_INET6:
>>> + {
>>> + struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)res->ai_addr;
>>> +
>>> + port = ntohs(sin6->sin6_port);
>>> + }
>>> + break;
>>> + default:
>>> + xlog(L_ERROR, "Bad address family: %d\n", res->ai_family);
>>> + port = -1;
>>> + }
>>> + freeaddrinfo(res);
>>> + return port;
>>> +}
>>> +
>>> +static int lockd_configure(struct nl_sock *sock, int grace)
>>> +{
>>> + char *tcp_svc, *udp_svc;
>>> + int tcpport = 0, udpport = 0;
>>> + int ret;
>>> +
>>> + tcp_svc = conf_get_str("lockd", "port");
>>> + if (tcp_svc) {
>>> + tcpport = get_service(tcp_svc);
>>> + if (tcpport < 0)
>>> + return 1;
>>> + }
>>> +
>>> + udp_svc = conf_get_str("lockd", "udp-port");
>>> + if (udp_svc) {
>>> + udpport = get_service(udp_svc);
>>> + if (udpport < 0)
>>> + return 1;
>>> + }
>>> +
>>> + return lockd_config_doit(sock, LOCKD_CMD_SERVER_SET, grace, tcpport, udpport);
>>> +}
>>> +
>>> +
>>> #define MAX_LISTENER_LEN (64 * 2 + 16)
>>>
>>> static void
>>> @@ -1355,6 +1511,13 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>>
>>> read_nfsd_conf();
>>>
>>> + grace = conf_get_num("nfsd", "grace-time", 0);
>>> + ret = lockd_configure(sock, grace);
>>> + if (ret) {
>>> + xlog(L_ERROR, "lockd configuration failure");
>>> + return ret;
>>> + }
>>> +
>>> pool_mode = conf_get_str("nfsd", "pool-mode");
>>> if (pool_mode) {
>>> ret = pool_mode_doit(sock, NFSD_CMD_POOL_MODE_SET, pool_mode);
>>> @@ -1370,15 +1533,12 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>> if (ret)
>>> return ret;
>>>
>>> + xlog(D_GENERAL, "configuring listeners");
>>> configure_listeners();
>>> ret = set_listeners(sock);
>>> if (ret)
>>> return ret;
>>>
>>> - grace = conf_get_num("nfsd", "grace-time", 0);
>>> - lease = conf_get_num("nfsd", "lease-time", 0);
>>> - scope = conf_get_str("nfsd", "scope");
>>> -
>>> thread_str = conf_get_list("nfsd", "threads");
>>> pools = thread_str ? thread_str->cnt : 1;
>>>
>>> @@ -1402,6 +1562,9 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>> threads[0] = DEFAULT_AUTOSTART_THREADS;
>>> }
>>>
>>> + lease = conf_get_num("nfsd", "lease-time", 0);
>>> + scope = conf_get_str("nfsd", "scope");
>>> +
>>> ret = threads_doit(sock, NFSD_CMD_THREADS_SET, grace, lease, pools,
>>> threads, scope);
>>> out:
>>> @@ -1409,6 +1572,30 @@ out:
>>> return ret;
>>> }
>>>
>>> +static void nlm_usage(void)
>>> +{
>>> + printf("Usage: %s nlm\n", taskname);
>>> + printf(" Get the current settings for the NLM (lockd) server.\n");
>>> +}
>>> +
>>> +static int nlm_func(struct nl_sock *sock, int argc, char ** argv)
>>> +{
>>> + int *threads, grace, lease, idx, ret, opt, pools;
>>> + struct conf_list *thread_str;
>>> + struct conf_list_node *n;
>>> + char *scope, *pool_mode;
>>> +
>>> + optind = 1;
>>> + while ((opt = getopt_long(argc, argv, "h", help_only_options, NULL)) != -1) {
>>> + switch (opt) {
>>> + case 'h':
>>> + nlm_usage();
>>> + return 0;
>>> + }
>>> + }
>>> + return lockd_config_doit(sock, LOCKD_CMD_SERVER_GET, 0, 0, 0);
>>> +}
>>> +
>>> enum nfsdctl_commands {
>>> NFSDCTL_STATUS,
>>> NFSDCTL_THREADS,
>>> @@ -1416,6 +1603,7 @@ enum nfsdctl_commands {
>>> NFSDCTL_LISTENER,
>>> NFSDCTL_AUTOSTART,
>>> NFSDCTL_POOL_MODE,
>>> + NFSDCTL_NLM,
>>> };
>>>
>>> static int parse_command(char *str)
>>> @@ -1432,6 +1620,8 @@ static int parse_command(char *str)
>>> return NFSDCTL_AUTOSTART;
>>> if (!strcmp(str, "pool-mode"))
>>> return NFSDCTL_POOL_MODE;
>>> + if (!strcmp(str, "nlm"))
>>> + return NFSDCTL_NLM;
>>> return -1;
>>> }
>>>
>>> @@ -1444,6 +1634,7 @@ static nfsdctl_func func[] = {
>>> [NFSDCTL_LISTENER] = listener_func,
>>> [NFSDCTL_AUTOSTART] = autostart_func,
>>> [NFSDCTL_POOL_MODE] = pool_mode_func,
>>> + [NFSDCTL_NLM] = nlm_func,
>>> };
>>>
>>> static void usage(void)
>>> @@ -1460,6 +1651,7 @@ static void usage(void)
>>> printf(" listener get/set listener info\n");
>>> printf(" version get/set supported NFS versions\n");
>>> printf(" threads get/set nfsd thread settings\n");
>>> + printf(" nlm get current nlm settings\n");
>>> printf(" status get current RPC processing info\n");
>>> printf(" autostart start server with settings from /etc/nfs.conf\n");
>>> }
>>>
>>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-10 15:40 ` Tom Talpey
@ 2025-01-13 13:39 ` Benjamin Coddington
2025-01-14 15:53 ` Tom Talpey
0 siblings, 1 reply; 27+ messages in thread
From: Benjamin Coddington @ 2025-01-13 13:39 UTC (permalink / raw)
To: Tom Talpey
Cc: Jeff Layton, Steve Dickson, Scott Mayhew, Yongcheng Yang,
linux-nfs
On 10 Jan 2025, at 10:40, Tom Talpey wrote:
> I wonder how many sysadmins are actually changing it, and why. It's
> not truly a correctness thing, more of an availability setting, so
> the choice of value is pretty soft. As you say, if the clients all
> reclaim promptly, nobody's the wiser. Do we have any data on how
> often the setting actually gets used?
Here's an anecdote - when I was a sysadmin (~12 years ago), we were very
aware of the grace period and tried to optimize it because too long meant
many of our clients applications would hang for a longer outage, and too
short meant that all the locks weren't recovered.
We also depended on lock recovery working well because we would often
update and migrate servers.
This was before knfsd v4 could end it early, which is.. quite nice.
Ben
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd
2025-01-13 13:39 ` Benjamin Coddington
@ 2025-01-14 15:53 ` Tom Talpey
0 siblings, 0 replies; 27+ messages in thread
From: Tom Talpey @ 2025-01-14 15:53 UTC (permalink / raw)
To: Benjamin Coddington
Cc: Jeff Layton, Steve Dickson, Scott Mayhew, Yongcheng Yang,
linux-nfs
On 1/13/2025 8:39 AM, Benjamin Coddington wrote:
> On 10 Jan 2025, at 10:40, Tom Talpey wrote:
>
>> I wonder how many sysadmins are actually changing it, and why. It's
>> not truly a correctness thing, more of an availability setting, so
>> the choice of value is pretty soft. As you say, if the clients all
>> reclaim promptly, nobody's the wiser. Do we have any data on how
>> often the setting actually gets used?
>
> Here's an anecdote - when I was a sysadmin (~12 years ago), we were very
> aware of the grace period and tried to optimize it because too long meant
> many of our clients applications would hang for a longer outage, and too
> short meant that all the locks weren't recovered.
>
> We also depended on lock recovery working well because we would often
> update and migrate servers.
Ok good to know - v3 and v4.0 I assume.
> This was before knfsd v4 could end it early, which is.. quite nice.
Yep, v4.1 RECLAIM_COMPLETE, right? You're welcome. :)
Tom.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
` (2 preceding siblings ...)
2025-01-10 13:46 ` [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd Jeff Layton
@ 2025-01-14 21:09 ` Scott Mayhew
2025-01-14 21:18 ` Jeff Layton
2025-03-19 19:47 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Steve Dickson
4 siblings, 1 reply; 27+ messages in thread
From: Scott Mayhew @ 2025-01-14 21:09 UTC (permalink / raw)
To: Jeff Layton; +Cc: Steve Dickson, Yongcheng Yang, linux-nfs
On Fri, 10 Jan 2025, Jeff Layton wrote:
> v2 is just a small update to fix the problems that Scott spotted.
>
> This patch series adds support for the new lockd configuration interface
> that should fix this RH bug:
>
> https://issues.redhat.com/browse/RHEL-71698
>
> There are some other improvements here too, notably a switch to xlog.
> Only lightly tested, but seems to do the right thing.
>
> Port handling with lockd still needs more work. Currently that is
> usually configured by rpc.statd. I think we need to convert it to
> use netlink to configure the ports as well, when it's able.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
I think the read_nfsd_conf call should be moved out of autostart_func
and into main (right before the command-line options are parsed). Right
now if you enable debugging in nfs.conf, you get the "configuring
listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
message. It's not a big deal though and could be done if additional
debug logging is added in the future.
Reviewed-by: Scott Mayhew <smayhew@redhat.com>
> ---
> Changes in v2:
> - properly regenerate manpages
> - fix up bogus merge conflict
> - add D_GENERAL xlog messages when nfsdctl starts and exits
> - Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
>
> ---
> Jeff Layton (3):
> nfsdctl: convert to xlog()
> nfsdctl: fix the --version option
> nfsdctl: add necessary bits to configure lockd
>
> configure.ac | 4 +
> utils/nfsdctl/lockd_netlink.h | 29 ++++
> utils/nfsdctl/nfsdctl.8 | 15 +-
> utils/nfsdctl/nfsdctl.adoc | 8 +
> utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
> 5 files changed, 321 insertions(+), 66 deletions(-)
> ---
> base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
> change-id: 20250109-lockd-nl-6272fa9e8a5d
>
> Best regards,
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-14 21:09 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Scott Mayhew
@ 2025-01-14 21:18 ` Jeff Layton
2025-01-15 14:44 ` Scott Mayhew
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-14 21:18 UTC (permalink / raw)
To: Scott Mayhew; +Cc: Steve Dickson, Yongcheng Yang, linux-nfs
On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
> On Fri, 10 Jan 2025, Jeff Layton wrote:
>
> > v2 is just a small update to fix the problems that Scott spotted.
> >
> > This patch series adds support for the new lockd configuration interface
> > that should fix this RH bug:
> >
> > https://issues.redhat.com/browse/RHEL-71698
> >
> > There are some other improvements here too, notably a switch to xlog.
> > Only lightly tested, but seems to do the right thing.
> >
> > Port handling with lockd still needs more work. Currently that is
> > usually configured by rpc.statd. I think we need to convert it to
> > use netlink to configure the ports as well, when it's able.
> >
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
>
> I think the read_nfsd_conf call should be moved out of autostart_func
> and into main (right before the command-line options are parsed). Right
> now if you enable debugging in nfs.conf, you get the "configuring
> listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
> message. It's not a big deal though and could be done if additional
> debug logging is added in the future.
>
That sounds good. We can do that in a separate patch.
> Reviewed-by: Scott Mayhew <smayhew@redhat.com>
Thanks!
> > ---
> > Changes in v2:
> > - properly regenerate manpages
> > - fix up bogus merge conflict
> > - add D_GENERAL xlog messages when nfsdctl starts and exits
> > - Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
> >
> > ---
> > Jeff Layton (3):
> > nfsdctl: convert to xlog()
> > nfsdctl: fix the --version option
> > nfsdctl: add necessary bits to configure lockd
> >
> > configure.ac | 4 +
> > utils/nfsdctl/lockd_netlink.h | 29 ++++
> > utils/nfsdctl/nfsdctl.8 | 15 +-
> > utils/nfsdctl/nfsdctl.adoc | 8 +
> > utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
> > 5 files changed, 321 insertions(+), 66 deletions(-)
> > ---
> > base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
> > change-id: 20250109-lockd-nl-6272fa9e8a5d
> >
> > Best regards,
> > --
> > Jeff Layton <jlayton@kernel.org>
> >
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-14 21:18 ` Jeff Layton
@ 2025-01-15 14:44 ` Scott Mayhew
2025-01-15 14:56 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Scott Mayhew @ 2025-01-15 14:44 UTC (permalink / raw)
To: Jeff Layton; +Cc: Steve Dickson, Yongcheng Yang, linux-nfs
On Tue, 14 Jan 2025, Jeff Layton wrote:
> On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
> > On Fri, 10 Jan 2025, Jeff Layton wrote:
> >
> > > v2 is just a small update to fix the problems that Scott spotted.
> > >
> > > This patch series adds support for the new lockd configuration interface
> > > that should fix this RH bug:
> > >
> > > https://issues.redhat.com/browse/RHEL-71698
> > >
> > > There are some other improvements here too, notably a switch to xlog.
> > > Only lightly tested, but seems to do the right thing.
> > >
> > > Port handling with lockd still needs more work. Currently that is
> > > usually configured by rpc.statd. I think we need to convert it to
> > > use netlink to configure the ports as well, when it's able.
> > >
> > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >
> > I think the read_nfsd_conf call should be moved out of autostart_func
> > and into main (right before the command-line options are parsed). Right
> > now if you enable debugging in nfs.conf, you get the "configuring
> > listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
> > message. It's not a big deal though and could be done if additional
> > debug logging is added in the future.
> >
>
> That sounds good. We can do that in a separate patch.
>
> > Reviewed-by: Scott Mayhew <smayhew@redhat.com>
>
> Thanks!
Hey, Jeff. I was testing this against a kernel without the lockd
netlink patch, and I get this:
Jan 15 09:39:16 systemd[1]: Starting nfs-server.service - NFS server and services...
Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl started
Jan 15 09:39:17 sh[1603]: nfsdctl: nfsd not found
Jan 15 09:39:17 sh[1603]: nfsdctl: lockd configuration failure
Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl exiting
Jan 15 09:39:17 sh[1601]: rpc.nfsd: knfsd is currently down
Jan 15 09:39:17 sh[1601]: rpc.nfsd: Writing version string to kernel: -2 +3 +4 +4.1 +4.2
Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET TCP socket.
Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET6 TCP socket.
Do we really want it falling back to rpc.nfsd if it can't configure
lockd? Maybe it should emit a warning instead?
At the very least, NFSD_FAMILY_NAME should no longer be hard-coded in
that "not found" error message in netlink_msg_alloc().
-Scott
>
> > > ---
> > > Changes in v2:
> > > - properly regenerate manpages
> > > - fix up bogus merge conflict
> > > - add D_GENERAL xlog messages when nfsdctl starts and exits
> > > - Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
> > >
> > > ---
> > > Jeff Layton (3):
> > > nfsdctl: convert to xlog()
> > > nfsdctl: fix the --version option
> > > nfsdctl: add necessary bits to configure lockd
> > >
> > > configure.ac | 4 +
> > > utils/nfsdctl/lockd_netlink.h | 29 ++++
> > > utils/nfsdctl/nfsdctl.8 | 15 +-
> > > utils/nfsdctl/nfsdctl.adoc | 8 +
> > > utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
> > > 5 files changed, 321 insertions(+), 66 deletions(-)
> > > ---
> > > base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
> > > change-id: 20250109-lockd-nl-6272fa9e8a5d
> > >
> > > Best regards,
> > > --
> > > Jeff Layton <jlayton@kernel.org>
> > >
> >
>
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-15 14:44 ` Scott Mayhew
@ 2025-01-15 14:56 ` Jeff Layton
2025-01-15 15:12 ` Steve Dickson
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-15 14:56 UTC (permalink / raw)
To: Scott Mayhew; +Cc: Steve Dickson, Yongcheng Yang, linux-nfs
On Wed, 2025-01-15 at 09:44 -0500, Scott Mayhew wrote:
> On Tue, 14 Jan 2025, Jeff Layton wrote:
>
> > On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
> > > On Fri, 10 Jan 2025, Jeff Layton wrote:
> > >
> > > > v2 is just a small update to fix the problems that Scott spotted.
> > > >
> > > > This patch series adds support for the new lockd configuration interface
> > > > that should fix this RH bug:
> > > >
> > > > https://issues.redhat.com/browse/RHEL-71698
> > > >
> > > > There are some other improvements here too, notably a switch to xlog.
> > > > Only lightly tested, but seems to do the right thing.
> > > >
> > > > Port handling with lockd still needs more work. Currently that is
> > > > usually configured by rpc.statd. I think we need to convert it to
> > > > use netlink to configure the ports as well, when it's able.
> > > >
> > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > >
> > > I think the read_nfsd_conf call should be moved out of autostart_func
> > > and into main (right before the command-line options are parsed). Right
> > > now if you enable debugging in nfs.conf, you get the "configuring
> > > listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
> > > message. It's not a big deal though and could be done if additional
> > > debug logging is added in the future.
> > >
> >
> > That sounds good. We can do that in a separate patch.
> >
> > > Reviewed-by: Scott Mayhew <smayhew@redhat.com>
> >
> > Thanks!
>
> Hey, Jeff. I was testing this against a kernel without the lockd
> netlink patch, and I get this:
>
> Jan 15 09:39:16 systemd[1]: Starting nfs-server.service - NFS server and services...
> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl started
> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsd not found
> Jan 15 09:39:17 sh[1603]: nfsdctl: lockd configuration failure
> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl exiting
> Jan 15 09:39:17 sh[1601]: rpc.nfsd: knfsd is currently down
> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Writing version string to kernel: -2 +3 +4 +4.1 +4.2
> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET TCP socket.
> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET6 TCP socket.
>
> Do we really want it falling back to rpc.nfsd if it can't configure
> lockd? Maybe it should emit a warning instead?
>
I thought about that, and I think it's better to error out here.
Falling back to rpc.nfsd is harmless, and only people who are trying to
set the grace period or lockd ports will ever hit this. lockd
configuration is a no-op if none of those settings are set.
> At the very least, NFSD_FAMILY_NAME should no longer be hard-coded in
> that "not found" error message in netlink_msg_alloc().
>
Yeah, that would be good to fix.
>
> >
> > > > ---
> > > > Changes in v2:
> > > > - properly regenerate manpages
> > > > - fix up bogus merge conflict
> > > > - add D_GENERAL xlog messages when nfsdctl starts and exits
> > > > - Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
> > > >
> > > > ---
> > > > Jeff Layton (3):
> > > > nfsdctl: convert to xlog()
> > > > nfsdctl: fix the --version option
> > > > nfsdctl: add necessary bits to configure lockd
> > > >
> > > > configure.ac | 4 +
> > > > utils/nfsdctl/lockd_netlink.h | 29 ++++
> > > > utils/nfsdctl/nfsdctl.8 | 15 +-
> > > > utils/nfsdctl/nfsdctl.adoc | 8 +
> > > > utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
> > > > 5 files changed, 321 insertions(+), 66 deletions(-)
> > > > ---
> > > > base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
> > > > change-id: 20250109-lockd-nl-6272fa9e8a5d
> > > >
> > > > Best regards,
> > > > --
> > > > Jeff Layton <jlayton@kernel.org>
> > > >
> > >
> >
> > --
> > Jeff Layton <jlayton@kernel.org>
> >
>
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-15 14:56 ` Jeff Layton
@ 2025-01-15 15:12 ` Steve Dickson
2025-01-15 15:28 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2025-01-15 15:12 UTC (permalink / raw)
To: Jeff Layton, Scott Mayhew; +Cc: Yongcheng Yang, linux-nfs
On 1/15/25 9:56 AM, Jeff Layton wrote:
> On Wed, 2025-01-15 at 09:44 -0500, Scott Mayhew wrote:
>> On Tue, 14 Jan 2025, Jeff Layton wrote:
>>
>>> On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
>>>> On Fri, 10 Jan 2025, Jeff Layton wrote:
>>>>
>>>>> v2 is just a small update to fix the problems that Scott spotted.
>>>>>
>>>>> This patch series adds support for the new lockd configuration interface
>>>>> that should fix this RH bug:
>>>>>
>>>>> https://issues.redhat.com/browse/RHEL-71698
>>>>>
>>>>> There are some other improvements here too, notably a switch to xlog.
>>>>> Only lightly tested, but seems to do the right thing.
>>>>>
>>>>> Port handling with lockd still needs more work. Currently that is
>>>>> usually configured by rpc.statd. I think we need to convert it to
>>>>> use netlink to configure the ports as well, when it's able.
>>>>>
>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>
>>>> I think the read_nfsd_conf call should be moved out of autostart_func
>>>> and into main (right before the command-line options are parsed). Right
>>>> now if you enable debugging in nfs.conf, you get the "configuring
>>>> listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
>>>> message. It's not a big deal though and could be done if additional
>>>> debug logging is added in the future.
>>>>
>>>
>>> That sounds good. We can do that in a separate patch.
>>>
>>>> Reviewed-by: Scott Mayhew <smayhew@redhat.com>
>>>
>>> Thanks!
>>
>> Hey, Jeff. I was testing this against a kernel without the lockd
>> netlink patch, and I get this:
>>
>> Jan 15 09:39:16 systemd[1]: Starting nfs-server.service - NFS server and services...
>> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl started
>> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsd not found
>> Jan 15 09:39:17 sh[1603]: nfsdctl: lockd configuration failure
>> Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl exiting
>> Jan 15 09:39:17 sh[1601]: rpc.nfsd: knfsd is currently down
>> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Writing version string to kernel: -2 +3 +4 +4.1 +4.2
>> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET TCP socket.
>> Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET6 TCP socket.
>>
>> Do we really want it falling back to rpc.nfsd if it can't configure
>> lockd? Maybe it should emit a warning instead?
>>
>
> I thought about that, and I think it's better to error out here.
>
> Falling back to rpc.nfsd is harmless, and only people who are trying to
> set the grace period or lockd ports will ever hit this. lockd
> configuration is a no-op if none of those settings are set.
>
>> At the very least, NFSD_FAMILY_NAME should no longer be hard-coded in
>> that "not found" error message in netlink_msg_alloc().
>>
>
> Yeah, that would be good to fix.
>
On a rawhide kernel (6.13.0-0.rc6) the server does
come up with 'nfsdctl autostart' but with the
new argument 'nlm' I'm getting
$ nfsdctl nlm
nfsdctl: nfsd not found
steved.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-15 15:12 ` Steve Dickson
@ 2025-01-15 15:28 ` Jeff Layton
2025-01-15 16:40 ` Scott Mayhew
2025-01-15 17:00 ` [nfs-utils PATCH] nfsdctl: debug logging fixups Scott Mayhew
0 siblings, 2 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-15 15:28 UTC (permalink / raw)
To: Steve Dickson, Scott Mayhew; +Cc: Yongcheng Yang, linux-nfs
On Wed, 2025-01-15 at 10:12 -0500, Steve Dickson wrote:
>
> On 1/15/25 9:56 AM, Jeff Layton wrote:
> > On Wed, 2025-01-15 at 09:44 -0500, Scott Mayhew wrote:
> > > On Tue, 14 Jan 2025, Jeff Layton wrote:
> > >
> > > > On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
> > > > > On Fri, 10 Jan 2025, Jeff Layton wrote:
> > > > >
> > > > > > v2 is just a small update to fix the problems that Scott spotted.
> > > > > >
> > > > > > This patch series adds support for the new lockd configuration interface
> > > > > > that should fix this RH bug:
> > > > > >
> > > > > > https://issues.redhat.com/browse/RHEL-71698
> > > > > >
> > > > > > There are some other improvements here too, notably a switch to xlog.
> > > > > > Only lightly tested, but seems to do the right thing.
> > > > > >
> > > > > > Port handling with lockd still needs more work. Currently that is
> > > > > > usually configured by rpc.statd. I think we need to convert it to
> > > > > > use netlink to configure the ports as well, when it's able.
> > > > > >
> > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > >
> > > > > I think the read_nfsd_conf call should be moved out of autostart_func
> > > > > and into main (right before the command-line options are parsed). Right
> > > > > now if you enable debugging in nfs.conf, you get the "configuring
> > > > > listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
> > > > > message. It's not a big deal though and could be done if additional
> > > > > debug logging is added in the future.
> > > > >
> > > >
> > > > That sounds good. We can do that in a separate patch.
> > > >
> > > > > Reviewed-by: Scott Mayhew <smayhew@redhat.com>
> > > >
> > > > Thanks!
> > >
> > > Hey, Jeff. I was testing this against a kernel without the lockd
> > > netlink patch, and I get this:
> > >
> > > Jan 15 09:39:16 systemd[1]: Starting nfs-server.service - NFS server and services...
> > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl started
> > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsd not found
> > > Jan 15 09:39:17 sh[1603]: nfsdctl: lockd configuration failure
> > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl exiting
> > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: knfsd is currently down
> > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Writing version string to kernel: -2 +3 +4 +4.1 +4.2
> > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET TCP socket.
> > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET6 TCP socket.
> > >
> > > Do we really want it falling back to rpc.nfsd if it can't configure
> > > lockd? Maybe it should emit a warning instead?
> > >
> >
> > I thought about that, and I think it's better to error out here.
> >
> > Falling back to rpc.nfsd is harmless, and only people who are trying to
> > set the grace period or lockd ports will ever hit this. lockd
> > configuration is a no-op if none of those settings are set.
> >
> > > At the very least, NFSD_FAMILY_NAME should no longer be hard-coded in
> > > that "not found" error message in netlink_msg_alloc().
> > >
> >
> > Yeah, that would be good to fix.
> >
>
> On a rawhide kernel (6.13.0-0.rc6) the server does
> come up with 'nfsdctl autostart' but with the
> new argument 'nlm' I'm getting
>
> $ nfsdctl nlm
> nfsdctl: nfsd not found
>
Yeah, that's what Scott pointed out too. We should make that error
message a bit more friendly. It may be a bit before I can get to it. Do
you guys want to propose a patch to fix that?
Thanks,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-15 15:28 ` Jeff Layton
@ 2025-01-15 16:40 ` Scott Mayhew
2025-01-15 17:00 ` [nfs-utils PATCH] nfsdctl: debug logging fixups Scott Mayhew
1 sibling, 0 replies; 27+ messages in thread
From: Scott Mayhew @ 2025-01-15 16:40 UTC (permalink / raw)
To: Jeff Layton; +Cc: Steve Dickson, Yongcheng Yang, linux-nfs
On Wed, 15 Jan 2025, Jeff Layton wrote:
> On Wed, 2025-01-15 at 10:12 -0500, Steve Dickson wrote:
> >
> > On 1/15/25 9:56 AM, Jeff Layton wrote:
> > > On Wed, 2025-01-15 at 09:44 -0500, Scott Mayhew wrote:
> > > > On Tue, 14 Jan 2025, Jeff Layton wrote:
> > > >
> > > > > On Tue, 2025-01-14 at 16:09 -0500, Scott Mayhew wrote:
> > > > > > On Fri, 10 Jan 2025, Jeff Layton wrote:
> > > > > >
> > > > > > > v2 is just a small update to fix the problems that Scott spotted.
> > > > > > >
> > > > > > > This patch series adds support for the new lockd configuration interface
> > > > > > > that should fix this RH bug:
> > > > > > >
> > > > > > > https://issues.redhat.com/browse/RHEL-71698
> > > > > > >
> > > > > > > There are some other improvements here too, notably a switch to xlog.
> > > > > > > Only lightly tested, but seems to do the right thing.
> > > > > > >
> > > > > > > Port handling with lockd still needs more work. Currently that is
> > > > > > > usually configured by rpc.statd. I think we need to convert it to
> > > > > > > use netlink to configure the ports as well, when it's able.
> > > > > > >
> > > > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > > > > >
> > > > > > I think the read_nfsd_conf call should be moved out of autostart_func
> > > > > > and into main (right before the command-line options are parsed). Right
> > > > > > now if you enable debugging in nfs.conf, you get the "configuring
> > > > > > listeners" and "nfsdctl exiting" messages, but not the "nfsdctl started"
> > > > > > message. It's not a big deal though and could be done if additional
> > > > > > debug logging is added in the future.
> > > > > >
> > > > >
> > > > > That sounds good. We can do that in a separate patch.
> > > > >
> > > > > > Reviewed-by: Scott Mayhew <smayhew@redhat.com>
> > > > >
> > > > > Thanks!
> > > >
> > > > Hey, Jeff. I was testing this against a kernel without the lockd
> > > > netlink patch, and I get this:
> > > >
> > > > Jan 15 09:39:16 systemd[1]: Starting nfs-server.service - NFS server and services...
> > > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl started
> > > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsd not found
> > > > Jan 15 09:39:17 sh[1603]: nfsdctl: lockd configuration failure
> > > > Jan 15 09:39:17 sh[1603]: nfsdctl: nfsdctl exiting
> > > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: knfsd is currently down
> > > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Writing version string to kernel: -2 +3 +4 +4.1 +4.2
> > > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET TCP socket.
> > > > Jan 15 09:39:17 sh[1601]: rpc.nfsd: Created AF_INET6 TCP socket.
> > > >
> > > > Do we really want it falling back to rpc.nfsd if it can't configure
> > > > lockd? Maybe it should emit a warning instead?
> > > >
> > >
> > > I thought about that, and I think it's better to error out here.
> > >
> > > Falling back to rpc.nfsd is harmless, and only people who are trying to
> > > set the grace period or lockd ports will ever hit this. lockd
> > > configuration is a no-op if none of those settings are set.
> > >
> > > > At the very least, NFSD_FAMILY_NAME should no longer be hard-coded in
> > > > that "not found" error message in netlink_msg_alloc().
> > > >
> > >
> > > Yeah, that would be good to fix.
> > >
> >
> > On a rawhide kernel (6.13.0-0.rc6) the server does
> > come up with 'nfsdctl autostart' but with the
> > new argument 'nlm' I'm getting
> >
> > $ nfsdctl nlm
> > nfsdctl: nfsd not found
> >
>
> Yeah, that's what Scott pointed out too. We should make that error
> message a bit more friendly. It may be a bit before I can get to it. Do
> you guys want to propose a patch to fix that?
Sure, I can do that.
>
> Thanks,
> --
> Jeff Layton <jlayton@kernel.org>
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 15:28 ` Jeff Layton
2025-01-15 16:40 ` Scott Mayhew
@ 2025-01-15 17:00 ` Scott Mayhew
2025-01-15 17:02 ` Jeff Layton
2025-01-15 17:32 ` Steve Dickson
1 sibling, 2 replies; 27+ messages in thread
From: Scott Mayhew @ 2025-01-15 17:00 UTC (permalink / raw)
To: steved; +Cc: jlayton, yoyang, linux-nfs
Move read_nfsd_conf() out of autostart_func() and into main(). Remove
hard-coded NFSD_FAMILY_NAME in the first error message in
netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
more descriptive/unique.
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
SteveD - this would go on top of Jeff's "nfsdctl: add support for new
lockd configuration interface" patches.
utils/nfsdctl/nfsdctl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
index 003daba5..f81c78ae 100644
--- a/utils/nfsdctl/nfsdctl.c
+++ b/utils/nfsdctl/nfsdctl.c
@@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
id = genl_ctrl_resolve(sock, family);
if (id < 0) {
- xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
+ xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
return NULL;
}
@@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
}
if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
- xlog(L_ERROR, "failed to allocate netlink message");
+ xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
nlmsg_free(msg);
return NULL;
}
@@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
}
}
- read_nfsd_conf();
-
grace = conf_get_num("nfsd", "grace-time", 0);
ret = lockd_configure(sock, grace);
if (ret) {
@@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
xlog_syslog(0);
xlog_stderr(1);
+ read_nfsd_conf();
+
/* Parse the preliminary options */
while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
switch (opt) {
--
2.48.0
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 17:00 ` [nfs-utils PATCH] nfsdctl: debug logging fixups Scott Mayhew
@ 2025-01-15 17:02 ` Jeff Layton
2025-01-15 17:32 ` Steve Dickson
1 sibling, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-15 17:02 UTC (permalink / raw)
To: Scott Mayhew, steved; +Cc: yoyang, linux-nfs
On Wed, 2025-01-15 at 12:00 -0500, Scott Mayhew wrote:
> Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> hard-coded NFSD_FAMILY_NAME in the first error message in
> netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> more descriptive/unique.
>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> lockd configuration interface" patches.
>
> utils/nfsdctl/nfsdctl.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 003daba5..f81c78ae 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>
> id = genl_ctrl_resolve(sock, family);
> if (id < 0) {
> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> return NULL;
> }
>
> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> }
>
> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> - xlog(L_ERROR, "failed to allocate netlink message");
> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> nlmsg_free(msg);
> return NULL;
> }
> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> }
> }
>
> - read_nfsd_conf();
> -
> grace = conf_get_num("nfsd", "grace-time", 0);
> ret = lockd_configure(sock, grace);
> if (ret) {
> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> xlog_syslog(0);
> xlog_stderr(1);
>
> + read_nfsd_conf();
> +
> /* Parse the preliminary options */
> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> switch (opt) {
LGTM!
Reviewed-by: Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 17:00 ` [nfs-utils PATCH] nfsdctl: debug logging fixups Scott Mayhew
2025-01-15 17:02 ` Jeff Layton
@ 2025-01-15 17:32 ` Steve Dickson
2025-01-15 17:35 ` Jeff Layton
1 sibling, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2025-01-15 17:32 UTC (permalink / raw)
To: Scott Mayhew; +Cc: jlayton, yoyang, linux-nfs
On 1/15/25 12:00 PM, Scott Mayhew wrote:
> Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> hard-coded NFSD_FAMILY_NAME in the first error message in
> netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> more descriptive/unique.
>
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
> SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> lockd configuration interface" patches.
Got it...
>
> utils/nfsdctl/nfsdctl.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> index 003daba5..f81c78ae 100644
> --- a/utils/nfsdctl/nfsdctl.c
> +++ b/utils/nfsdctl/nfsdctl.c
> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>
> id = genl_ctrl_resolve(sock, family);
> if (id < 0) {
> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> return NULL;
> }
>
> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> }
>
> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> - xlog(L_ERROR, "failed to allocate netlink message");
> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> nlmsg_free(msg);
> return NULL;
> }
> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> }
> }
>
> - read_nfsd_conf();
> -
> grace = conf_get_num("nfsd", "grace-time", 0);
> ret = lockd_configure(sock, grace);
> if (ret) {
> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> xlog_syslog(0);
> xlog_stderr(1);
>
> + read_nfsd_conf();
> +
> /* Parse the preliminary options */
> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> switch (opt) {
Ok... at this point we a prettier error message
$ nfsdctl nlm
nfsdctl: failed to resolve lockd generic netlink family
But the point of this argument is:
Get information about NLM (lockd) settings in the current net
namespace. This subcommand takes no arguments.
How is that giving information from the running lockd?
What am I missing??
steved.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 17:32 ` Steve Dickson
@ 2025-01-15 17:35 ` Jeff Layton
2025-01-15 17:47 ` Steve Dickson
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-15 17:35 UTC (permalink / raw)
To: Steve Dickson, Scott Mayhew; +Cc: yoyang, linux-nfs
On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
>
> On 1/15/25 12:00 PM, Scott Mayhew wrote:
> > Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> > hard-coded NFSD_FAMILY_NAME in the first error message in
> > netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> > more descriptive/unique.
> >
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> > SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> > lockd configuration interface" patches.
> Got it...
>
> >
> > utils/nfsdctl/nfsdctl.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > index 003daba5..f81c78ae 100644
> > --- a/utils/nfsdctl/nfsdctl.c
> > +++ b/utils/nfsdctl/nfsdctl.c
> > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> >
> > id = genl_ctrl_resolve(sock, family);
> > if (id < 0) {
> > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> > return NULL;
> > }
> >
> > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > }
> >
> > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> > - xlog(L_ERROR, "failed to allocate netlink message");
> > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> > nlmsg_free(msg);
> > return NULL;
> > }
> > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > }
> > }
> >
> > - read_nfsd_conf();
> > -
> > grace = conf_get_num("nfsd", "grace-time", 0);
> > ret = lockd_configure(sock, grace);
> > if (ret) {
> > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> > xlog_syslog(0);
> > xlog_stderr(1);
> >
> > + read_nfsd_conf();
> > +
> > /* Parse the preliminary options */
> > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> > switch (opt) {
> Ok... at this point we a prettier error message
> $ nfsdctl nlm
> nfsdctl: failed to resolve lockd generic netlink family
>
> But the point of this argument is:
>
> Get information about NLM (lockd) settings in the current net
> namespace. This subcommand takes no arguments.
>
> How is that giving information from the running lockd?
>
> What am I missing??
>
You're missing a kernel that has the required netlink interface. To
test this properly, you'll need to patch your kernel, until that patch
makes it upstream.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 17:35 ` Jeff Layton
@ 2025-01-15 17:47 ` Steve Dickson
2025-01-15 18:33 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2025-01-15 17:47 UTC (permalink / raw)
To: Jeff Layton, Scott Mayhew; +Cc: yoyang, linux-nfs
On 1/15/25 12:35 PM, Jeff Layton wrote:
> On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
>>
>> On 1/15/25 12:00 PM, Scott Mayhew wrote:
>>> Move read_nfsd_conf() out of autostart_func() and into main(). Remove
>>> hard-coded NFSD_FAMILY_NAME in the first error message in
>>> netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
>>> more descriptive/unique.
>>>
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>> SteveD - this would go on top of Jeff's "nfsdctl: add support for new
>>> lockd configuration interface" patches.
>> Got it...
>>
>>>
>>> utils/nfsdctl/nfsdctl.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>> index 003daba5..f81c78ae 100644
>>> --- a/utils/nfsdctl/nfsdctl.c
>>> +++ b/utils/nfsdctl/nfsdctl.c
>>> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>>
>>> id = genl_ctrl_resolve(sock, family);
>>> if (id < 0) {
>>> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
>>> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
>>> return NULL;
>>> }
>>>
>>> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>> }
>>>
>>> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
>>> - xlog(L_ERROR, "failed to allocate netlink message");
>>> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
>>> nlmsg_free(msg);
>>> return NULL;
>>> }
>>> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>> }
>>> }
>>>
>>> - read_nfsd_conf();
>>> -
>>> grace = conf_get_num("nfsd", "grace-time", 0);
>>> ret = lockd_configure(sock, grace);
>>> if (ret) {
>>> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
>>> xlog_syslog(0);
>>> xlog_stderr(1);
>>>
>>> + read_nfsd_conf();
>>> +
>>> /* Parse the preliminary options */
>>> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
>>> switch (opt) {
>> Ok... at this point we a prettier error message
>> $ nfsdctl nlm
>> nfsdctl: failed to resolve lockd generic netlink family
>>
>> But the point of this argument is:
>>
>> Get information about NLM (lockd) settings in the current net
>> namespace. This subcommand takes no arguments.
>>
>> How is that giving information from the running lockd?
>>
>> What am I missing??
>>
>
> You're missing a kernel that has the required netlink interface. To
> test this properly, you'll need to patch your kernel, until that patch
> makes it upstream.
Okay... I figured it was something like that. But doesn't make sense to
wait until the patch is in upstream so the argument can be properly
tested? Why add an argument that will always fail?
steved.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 17:47 ` Steve Dickson
@ 2025-01-15 18:33 ` Jeff Layton
2025-01-15 20:53 ` Steve Dickson
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-15 18:33 UTC (permalink / raw)
To: Steve Dickson, Scott Mayhew; +Cc: yoyang, linux-nfs
On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote:
>
> On 1/15/25 12:35 PM, Jeff Layton wrote:
> > On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
> > >
> > > On 1/15/25 12:00 PM, Scott Mayhew wrote:
> > > > Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> > > > hard-coded NFSD_FAMILY_NAME in the first error message in
> > > > netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> > > > more descriptive/unique.
> > > >
> > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > > ---
> > > > SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> > > > lockd configuration interface" patches.
> > > Got it...
> > >
> > > >
> > > > utils/nfsdctl/nfsdctl.c | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > > > index 003daba5..f81c78ae 100644
> > > > --- a/utils/nfsdctl/nfsdctl.c
> > > > +++ b/utils/nfsdctl/nfsdctl.c
> > > > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > >
> > > > id = genl_ctrl_resolve(sock, family);
> > > > if (id < 0) {
> > > > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> > > > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> > > > return NULL;
> > > > }
> > > >
> > > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > > }
> > > >
> > > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> > > > - xlog(L_ERROR, "failed to allocate netlink message");
> > > > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> > > > nlmsg_free(msg);
> > > > return NULL;
> > > > }
> > > > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > > > }
> > > > }
> > > >
> > > > - read_nfsd_conf();
> > > > -
> > > > grace = conf_get_num("nfsd", "grace-time", 0);
> > > > ret = lockd_configure(sock, grace);
> > > > if (ret) {
> > > > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> > > > xlog_syslog(0);
> > > > xlog_stderr(1);
> > > >
> > > > + read_nfsd_conf();
> > > > +
> > > > /* Parse the preliminary options */
> > > > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> > > > switch (opt) {
> > > Ok... at this point we a prettier error message
> > > $ nfsdctl nlm
> > > nfsdctl: failed to resolve lockd generic netlink family
> > >
> > > But the point of this argument is:
> > >
> > > Get information about NLM (lockd) settings in the current net
> > > namespace. This subcommand takes no arguments.
> > >
> > > How is that giving information from the running lockd?
> > >
> > > What am I missing??
> > >
> >
> > You're missing a kernel that has the required netlink interface. To
> > test this properly, you'll need to patch your kernel, until that patch
> > makes it upstream.
> Okay... I figured it was something like that. But doesn't make sense to
> wait until the patch is in upstream so the argument can be properly
> tested? Why add an argument that will always fail?
>
Why can't it be properly tested? It's just a matter of running a more
recent kernel that has the right interfaces. That should be in linux-
next soon (if not already).
I think the question is whether we want to wait until the kernel
interfaces trickle out into downstream distro kernels before we ship
any userland support in an upstream project (nfs-utils).
If you want to wait until it hits Fedora Rawhide kernels, then you're
looking at about 10-12 weeks from now. If you want to wait until it
makes it into a stable Fedora release kernel then we're looking at
about 6 months from now.
I'll note that that it took 6 months to get the original nfsdctl
patches merged because of the lag on kernel patches making it into
distros, and I think that was way too long.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 18:33 ` Jeff Layton
@ 2025-01-15 20:53 ` Steve Dickson
2025-01-16 11:50 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2025-01-15 20:53 UTC (permalink / raw)
To: Jeff Layton, Scott Mayhew; +Cc: yoyang, linux-nfs
On 1/15/25 1:33 PM, Jeff Layton wrote:
> On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote:
>>
>> On 1/15/25 12:35 PM, Jeff Layton wrote:
>>> On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
>>>>
>>>> On 1/15/25 12:00 PM, Scott Mayhew wrote:
>>>>> Move read_nfsd_conf() out of autostart_func() and into main(). Remove
>>>>> hard-coded NFSD_FAMILY_NAME in the first error message in
>>>>> netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
>>>>> more descriptive/unique.
>>>>>
>>>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>>>> ---
>>>>> SteveD - this would go on top of Jeff's "nfsdctl: add support for new
>>>>> lockd configuration interface" patches.
>>>> Got it...
>>>>
>>>>>
>>>>> utils/nfsdctl/nfsdctl.c | 8 ++++----
>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>>>> index 003daba5..f81c78ae 100644
>>>>> --- a/utils/nfsdctl/nfsdctl.c
>>>>> +++ b/utils/nfsdctl/nfsdctl.c
>>>>> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>>>>
>>>>> id = genl_ctrl_resolve(sock, family);
>>>>> if (id < 0) {
>>>>> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
>>>>> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
>>>>> return NULL;
>>>>> }
>>>>>
>>>>> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>>>> }
>>>>>
>>>>> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
>>>>> - xlog(L_ERROR, "failed to allocate netlink message");
>>>>> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
>>>>> nlmsg_free(msg);
>>>>> return NULL;
>>>>> }
>>>>> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>>>> }
>>>>> }
>>>>>
>>>>> - read_nfsd_conf();
>>>>> -
>>>>> grace = conf_get_num("nfsd", "grace-time", 0);
>>>>> ret = lockd_configure(sock, grace);
>>>>> if (ret) {
>>>>> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
>>>>> xlog_syslog(0);
>>>>> xlog_stderr(1);
>>>>>
>>>>> + read_nfsd_conf();
>>>>> +
>>>>> /* Parse the preliminary options */
>>>>> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
>>>>> switch (opt) {
>>>> Ok... at this point we a prettier error message
>>>> $ nfsdctl nlm
>>>> nfsdctl: failed to resolve lockd generic netlink family
>>>>
>>>> But the point of this argument is:
>>>>
>>>> Get information about NLM (lockd) settings in the current net
>>>> namespace. This subcommand takes no arguments.
>>>>
>>>> How is that giving information from the running lockd?
>>>>
>>>> What am I missing??
>>>>
>>>
>>> You're missing a kernel that has the required netlink interface. To
>>> test this properly, you'll need to patch your kernel, until that patch
>>> makes it upstream.
>> Okay... I figured it was something like that. But doesn't make sense to
>> wait until the patch is in upstream so the argument can be properly
>> tested? Why add an argument that will always fail?
>>
>
> Why can't it be properly tested? It's just a matter of running a more
> recent kernel that has the right interfaces. That should be in linux-
> next soon (if not already).
I'm doing my testing on a 6.13.0-0.rc6 which will soon be
a 6.14 kernel... its my understanding the needed kernel
patch will be in the 6.15 kernel... Please correct me
if that is not true.
>
> I think the question is whether we want to wait until the kernel
> interfaces trickle out into downstream distro kernels before we ship
> any userland support in an upstream project (nfs-utils).
Yes! As soon as the kernel support hits the upstream kernel,
we will be good to go. I just don't want to put a feature
in that will fail %100 of the time.
>
> If you want to wait until it hits Fedora Rawhide kernels, then you're
> looking at about 10-12 weeks from now. If you want to wait until it
> makes it into a stable Fedora release kernel then we're looking at
> about 6 months from now.
nfsdctl is in all current Fedora stable releases, which
is the reason I'm pushing back. I do not want to put something
in that will make it fail. That just does not make sense to me.
>
> I'll note that that it took 6 months to get the original nfsdctl
> patches merged because of the lag on kernel patches making it into
> distros, and I think that was way too long.
It took that long because there were issues with the command.
In which I was glad to help debug some of the issues...
New technology takes time to develop... I just think this
is one of those cases.
steved.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-15 20:53 ` Steve Dickson
@ 2025-01-16 11:50 ` Jeff Layton
2025-01-16 21:00 ` Steve Dickson
0 siblings, 1 reply; 27+ messages in thread
From: Jeff Layton @ 2025-01-16 11:50 UTC (permalink / raw)
To: Steve Dickson, Scott Mayhew; +Cc: yoyang, linux-nfs
On Wed, 2025-01-15 at 15:53 -0500, Steve Dickson wrote:
>
> On 1/15/25 1:33 PM, Jeff Layton wrote:
> > On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote:
> > >
> > > On 1/15/25 12:35 PM, Jeff Layton wrote:
> > > > On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
> > > > >
> > > > > On 1/15/25 12:00 PM, Scott Mayhew wrote:
> > > > > > Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> > > > > > hard-coded NFSD_FAMILY_NAME in the first error message in
> > > > > > netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> > > > > > more descriptive/unique.
> > > > > >
> > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > > > > ---
> > > > > > SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> > > > > > lockd configuration interface" patches.
> > > > > Got it...
> > > > >
> > > > > >
> > > > > > utils/nfsdctl/nfsdctl.c | 8 ++++----
> > > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > > > > > index 003daba5..f81c78ae 100644
> > > > > > --- a/utils/nfsdctl/nfsdctl.c
> > > > > > +++ b/utils/nfsdctl/nfsdctl.c
> > > > > > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > > > >
> > > > > > id = genl_ctrl_resolve(sock, family);
> > > > > > if (id < 0) {
> > > > > > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> > > > > > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> > > > > > return NULL;
> > > > > > }
> > > > > >
> > > > > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > > > > }
> > > > > >
> > > > > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> > > > > > - xlog(L_ERROR, "failed to allocate netlink message");
> > > > > > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> > > > > > nlmsg_free(msg);
> > > > > > return NULL;
> > > > > > }
> > > > > > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > - read_nfsd_conf();
> > > > > > -
> > > > > > grace = conf_get_num("nfsd", "grace-time", 0);
> > > > > > ret = lockd_configure(sock, grace);
> > > > > > if (ret) {
> > > > > > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> > > > > > xlog_syslog(0);
> > > > > > xlog_stderr(1);
> > > > > >
> > > > > > + read_nfsd_conf();
> > > > > > +
> > > > > > /* Parse the preliminary options */
> > > > > > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> > > > > > switch (opt) {
> > > > > Ok... at this point we a prettier error message
> > > > > $ nfsdctl nlm
> > > > > nfsdctl: failed to resolve lockd generic netlink family
> > > > >
> > > > > But the point of this argument is:
> > > > >
> > > > > Get information about NLM (lockd) settings in the current net
> > > > > namespace. This subcommand takes no arguments.
> > > > >
> > > > > How is that giving information from the running lockd?
> > > > >
> > > > > What am I missing??
> > > > >
> > > >
> > > > You're missing a kernel that has the required netlink interface. To
> > > > test this properly, you'll need to patch your kernel, until that patch
> > > > makes it upstream.
> > > Okay... I figured it was something like that. But doesn't make sense to
> > > wait until the patch is in upstream so the argument can be properly
> > > tested? Why add an argument that will always fail?
> > >
> >
> > Why can't it be properly tested? It's just a matter of running a more
> > recent kernel that has the right interfaces. That should be in linux-
> > next soon (if not already).
> I'm doing my testing on a 6.13.0-0.rc6 which will soon be
> a 6.14 kernel... its my understanding the needed kernel
> patch will be in the 6.15 kernel... Please correct me
> if that is not true.
>
> >
> > I think the question is whether we want to wait until the kernel
> > interfaces trickle out into downstream distro kernels before we ship
> > any userland support in an upstream project (nfs-utils).
> Yes! As soon as the kernel support hits the upstream kernel,
> we will be good to go. I just don't want to put a feature
> in that will fail %100 of the time.
>
> >
> > If you want to wait until it hits Fedora Rawhide kernels, then you're
> > looking at about 10-12 weeks from now. If you want to wait until it
> > makes it into a stable Fedora release kernel then we're looking at
> > about 6 months from now.
> nfsdctl is in all current Fedora stable releases, which
> is the reason I'm pushing back. I do not want to put something
> in that will make it fail. That just does not make sense to me.
>
> >
> > I'll note that that it took 6 months to get the original nfsdctl
> > patches merged because of the lag on kernel patches making it into
> > distros, and I think that was way too long.
> It took that long because there were issues with the command.
> In which I was glad to help debug some of the issues...
>
> New technology takes time to develop... I just think this
> is one of those cases.
>
Ok, your call. To be clear though, that patch is part of my solution
for this bug.
https://issues.redhat.com/browse/RHEL-71698
If you're going to delay it for several months, then can I trouble you
to come up with a fix for it that you find acceptable?
Thanks,
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-16 11:50 ` Jeff Layton
@ 2025-01-16 21:00 ` Steve Dickson
2025-01-16 21:12 ` Jeff Layton
0 siblings, 1 reply; 27+ messages in thread
From: Steve Dickson @ 2025-01-16 21:00 UTC (permalink / raw)
To: Jeff Layton, Scott Mayhew; +Cc: yoyang, linux-nfs
On 1/16/25 6:50 AM, Jeff Layton wrote:
> On Wed, 2025-01-15 at 15:53 -0500, Steve Dickson wrote:
>>
>> On 1/15/25 1:33 PM, Jeff Layton wrote:
>>> On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote:
>>>>
>>>> On 1/15/25 12:35 PM, Jeff Layton wrote:
>>>>> On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
>>>>>>
>>>>>> On 1/15/25 12:00 PM, Scott Mayhew wrote:
>>>>>>> Move read_nfsd_conf() out of autostart_func() and into main(). Remove
>>>>>>> hard-coded NFSD_FAMILY_NAME in the first error message in
>>>>>>> netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
>>>>>>> more descriptive/unique.
>>>>>>>
>>>>>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>>>>>> ---
>>>>>>> SteveD - this would go on top of Jeff's "nfsdctl: add support for new
>>>>>>> lockd configuration interface" patches.
>>>>>> Got it...
>>>>>>
>>>>>>>
>>>>>>> utils/nfsdctl/nfsdctl.c | 8 ++++----
>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
>>>>>>> index 003daba5..f81c78ae 100644
>>>>>>> --- a/utils/nfsdctl/nfsdctl.c
>>>>>>> +++ b/utils/nfsdctl/nfsdctl.c
>>>>>>> @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>>>>>>
>>>>>>> id = genl_ctrl_resolve(sock, family);
>>>>>>> if (id < 0) {
>>>>>>> - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
>>>>>>> + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
>>>>>>> return NULL;
>>>>>>> }
>>>>>>>
>>>>>>> @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
>>>>>>> }
>>>>>>>
>>>>>>> if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
>>>>>>> - xlog(L_ERROR, "failed to allocate netlink message");
>>>>>>> + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
>>>>>>> nlmsg_free(msg);
>>>>>>> return NULL;
>>>>>>> }
>>>>>>> @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
>>>>>>> }
>>>>>>> }
>>>>>>>
>>>>>>> - read_nfsd_conf();
>>>>>>> -
>>>>>>> grace = conf_get_num("nfsd", "grace-time", 0);
>>>>>>> ret = lockd_configure(sock, grace);
>>>>>>> if (ret) {
>>>>>>> @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
>>>>>>> xlog_syslog(0);
>>>>>>> xlog_stderr(1);
>>>>>>>
>>>>>>> + read_nfsd_conf();
>>>>>>> +
>>>>>>> /* Parse the preliminary options */
>>>>>>> while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
>>>>>>> switch (opt) {
>>>>>> Ok... at this point we a prettier error message
>>>>>> $ nfsdctl nlm
>>>>>> nfsdctl: failed to resolve lockd generic netlink family
>>>>>>
>>>>>> But the point of this argument is:
>>>>>>
>>>>>> Get information about NLM (lockd) settings in the current net
>>>>>> namespace. This subcommand takes no arguments.
>>>>>>
>>>>>> How is that giving information from the running lockd?
>>>>>>
>>>>>> What am I missing??
>>>>>>
>>>>>
>>>>> You're missing a kernel that has the required netlink interface. To
>>>>> test this properly, you'll need to patch your kernel, until that patch
>>>>> makes it upstream.
>>>> Okay... I figured it was something like that. But doesn't make sense to
>>>> wait until the patch is in upstream so the argument can be properly
>>>> tested? Why add an argument that will always fail?
>>>>
>>>
>>> Why can't it be properly tested? It's just a matter of running a more
>>> recent kernel that has the right interfaces. That should be in linux-
>>> next soon (if not already).
>> I'm doing my testing on a 6.13.0-0.rc6 which will soon be
>> a 6.14 kernel... its my understanding the needed kernel
>> patch will be in the 6.15 kernel... Please correct me
>> if that is not true.
>>
>>>
>>> I think the question is whether we want to wait until the kernel
>>> interfaces trickle out into downstream distro kernels before we ship
>>> any userland support in an upstream project (nfs-utils).
>> Yes! As soon as the kernel support hits the upstream kernel,
>> we will be good to go. I just don't want to put a feature
>> in that will fail %100 of the time.
>>
>>>
>>> If you want to wait until it hits Fedora Rawhide kernels, then you're
>>> looking at about 10-12 weeks from now. If you want to wait until it
>>> makes it into a stable Fedora release kernel then we're looking at
>>> about 6 months from now.
>> nfsdctl is in all current Fedora stable releases, which
>> is the reason I'm pushing back. I do not want to put something
>> in that will make it fail. That just does not make sense to me.
>>
>>>
>>> I'll note that that it took 6 months to get the original nfsdctl
>>> patches merged because of the lag on kernel patches making it into
>>> distros, and I think that was way too long.
>> It took that long because there were issues with the command.
>> In which I was glad to help debug some of the issues...
>>
>> New technology takes time to develop... I just think this
>> is one of those cases.
>>
>
> Ok, your call. To be clear though, that patch is part of my solution
> for this bug.
>
> https://issues.redhat.com/browse/RHEL-71698
>
> If you're going to delay it for several months, then can I trouble you
> to come up with a fix for it that you find acceptable?
How is this a fix when the subcommand will not work
without the kernel patch?
I'm sure the subcommand works with the kernel patch
but without it... what's the point?
steved.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [nfs-utils PATCH] nfsdctl: debug logging fixups
2025-01-16 21:00 ` Steve Dickson
@ 2025-01-16 21:12 ` Jeff Layton
0 siblings, 0 replies; 27+ messages in thread
From: Jeff Layton @ 2025-01-16 21:12 UTC (permalink / raw)
To: Steve Dickson, Scott Mayhew; +Cc: yoyang, linux-nfs
On Thu, 2025-01-16 at 16:00 -0500, Steve Dickson wrote:
>
> On 1/16/25 6:50 AM, Jeff Layton wrote:
> > On Wed, 2025-01-15 at 15:53 -0500, Steve Dickson wrote:
> > >
> > > On 1/15/25 1:33 PM, Jeff Layton wrote:
> > > > On Wed, 2025-01-15 at 12:47 -0500, Steve Dickson wrote:
> > > > >
> > > > > On 1/15/25 12:35 PM, Jeff Layton wrote:
> > > > > > On Wed, 2025-01-15 at 12:32 -0500, Steve Dickson wrote:
> > > > > > >
> > > > > > > On 1/15/25 12:00 PM, Scott Mayhew wrote:
> > > > > > > > Move read_nfsd_conf() out of autostart_func() and into main(). Remove
> > > > > > > > hard-coded NFSD_FAMILY_NAME in the first error message in
> > > > > > > > netlink_msg_alloc() and make the error messages in netlink_msg_alloc()
> > > > > > > > more descriptive/unique.
> > > > > > > >
> > > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > > > > > > > ---
> > > > > > > > SteveD - this would go on top of Jeff's "nfsdctl: add support for new
> > > > > > > > lockd configuration interface" patches.
> > > > > > > Got it...
> > > > > > >
> > > > > > > >
> > > > > > > > utils/nfsdctl/nfsdctl.c | 8 ++++----
> > > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/utils/nfsdctl/nfsdctl.c b/utils/nfsdctl/nfsdctl.c
> > > > > > > > index 003daba5..f81c78ae 100644
> > > > > > > > --- a/utils/nfsdctl/nfsdctl.c
> > > > > > > > +++ b/utils/nfsdctl/nfsdctl.c
> > > > > > > > @@ -436,7 +436,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > > > > > >
> > > > > > > > id = genl_ctrl_resolve(sock, family);
> > > > > > > > if (id < 0) {
> > > > > > > > - xlog(L_ERROR, "%s not found", NFSD_FAMILY_NAME);
> > > > > > > > + xlog(L_ERROR, "failed to resolve %s generic netlink family", family);
> > > > > > > > return NULL;
> > > > > > > > }
> > > > > > > >
> > > > > > > > @@ -447,7 +447,7 @@ static struct nl_msg *netlink_msg_alloc(struct nl_sock *sock, const char *family
> > > > > > > > }
> > > > > > > >
> > > > > > > > if (!genlmsg_put(msg, 0, 0, id, 0, 0, 0, 0)) {
> > > > > > > > - xlog(L_ERROR, "failed to allocate netlink message");
> > > > > > > > + xlog(L_ERROR, "failed to add generic netlink headers to netlink message");
> > > > > > > > nlmsg_free(msg);
> > > > > > > > return NULL;
> > > > > > > > }
> > > > > > > > @@ -1509,8 +1509,6 @@ static int autostart_func(struct nl_sock *sock, int argc, char ** argv)
> > > > > > > > }
> > > > > > > > }
> > > > > > > >
> > > > > > > > - read_nfsd_conf();
> > > > > > > > -
> > > > > > > > grace = conf_get_num("nfsd", "grace-time", 0);
> > > > > > > > ret = lockd_configure(sock, grace);
> > > > > > > > if (ret) {
> > > > > > > > @@ -1728,6 +1726,8 @@ int main(int argc, char **argv)
> > > > > > > > xlog_syslog(0);
> > > > > > > > xlog_stderr(1);
> > > > > > > >
> > > > > > > > + read_nfsd_conf();
> > > > > > > > +
> > > > > > > > /* Parse the preliminary options */
> > > > > > > > while ((opt = getopt_long(argc, argv, "+hdsV", pre_options, NULL)) != -1) {
> > > > > > > > switch (opt) {
> > > > > > > Ok... at this point we a prettier error message
> > > > > > > $ nfsdctl nlm
> > > > > > > nfsdctl: failed to resolve lockd generic netlink family
> > > > > > >
> > > > > > > But the point of this argument is:
> > > > > > >
> > > > > > > Get information about NLM (lockd) settings in the current net
> > > > > > > namespace. This subcommand takes no arguments.
> > > > > > >
> > > > > > > How is that giving information from the running lockd?
> > > > > > >
> > > > > > > What am I missing??
> > > > > > >
> > > > > >
> > > > > > You're missing a kernel that has the required netlink interface. To
> > > > > > test this properly, you'll need to patch your kernel, until that patch
> > > > > > makes it upstream.
> > > > > Okay... I figured it was something like that. But doesn't make sense to
> > > > > wait until the patch is in upstream so the argument can be properly
> > > > > tested? Why add an argument that will always fail?
> > > > >
> > > >
> > > > Why can't it be properly tested? It's just a matter of running a more
> > > > recent kernel that has the right interfaces. That should be in linux-
> > > > next soon (if not already).
> > > I'm doing my testing on a 6.13.0-0.rc6 which will soon be
> > > a 6.14 kernel... its my understanding the needed kernel
> > > patch will be in the 6.15 kernel... Please correct me
> > > if that is not true.
> > >
> > > >
> > > > I think the question is whether we want to wait until the kernel
> > > > interfaces trickle out into downstream distro kernels before we ship
> > > > any userland support in an upstream project (nfs-utils).
> > > Yes! As soon as the kernel support hits the upstream kernel,
> > > we will be good to go. I just don't want to put a feature
> > > in that will fail %100 of the time.
> > >
> > > >
> > > > If you want to wait until it hits Fedora Rawhide kernels, then you're
> > > > looking at about 10-12 weeks from now. If you want to wait until it
> > > > makes it into a stable Fedora release kernel then we're looking at
> > > > about 6 months from now.
> > > nfsdctl is in all current Fedora stable releases, which
> > > is the reason I'm pushing back. I do not want to put something
> > > in that will make it fail. That just does not make sense to me.
> > >
> > > >
> > > > I'll note that that it took 6 months to get the original nfsdctl
> > > > patches merged because of the lag on kernel patches making it into
> > > > distros, and I think that was way too long.
> > > It took that long because there were issues with the command.
> > > In which I was glad to help debug some of the issues...
> > >
> > > New technology takes time to develop... I just think this
> > > is one of those cases.
> > >
> >
> > Ok, your call. To be clear though, that patch is part of my solution
> > for this bug.
> >
> > https://issues.redhat.com/browse/RHEL-71698
> >
> > If you're going to delay it for several months, then can I trouble you
> > to come up with a fix for it that you find acceptable?
> How is this a fix when the subcommand will not work
> without the kernel patch?
>
The nfs-server.service file defines this:
ExecStart=/bin/sh -c '/usr/sbin/nfsdctl autostart || /usr/sbin/rpc.nfsd'
When the lockd netlink interface is needed, but isn't available, then
startup will fall back to just calling rpc.nfsd. Currently, the
grace_period setting is just ignored, so that fallback just doesn't
happen. Very few people will need this; only those that set lockd's
ports, or that set the grace_period.
> I'm sure the subcommand works with the kernel patch
> but without it... what's the point?
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
` (3 preceding siblings ...)
2025-01-14 21:09 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Scott Mayhew
@ 2025-03-19 19:47 ` Steve Dickson
4 siblings, 0 replies; 27+ messages in thread
From: Steve Dickson @ 2025-03-19 19:47 UTC (permalink / raw)
To: Jeff Layton; +Cc: Scott Mayhew, Yongcheng Yang, linux-nfs
On 1/10/25 8:46 AM, Jeff Layton wrote:
> v2 is just a small update to fix the problems that Scott spotted.
>
> This patch series adds support for the new lockd configuration interface
> that should fix this RH bug:
>
> https://issues.redhat.com/browse/RHEL-71698
>
> There are some other improvements here too, notably a switch to xlog.
> Only lightly tested, but seems to do the right thing.
>
> Port handling with lockd still needs more work. Currently that is
> usually configured by rpc.statd. I think we need to convert it to
> use netlink to configure the ports as well, when it's able.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
I have reluctantly committed these because the
nlm arg still fails (genl_ctrl_resolve() is not finding the family)
but all the other args seem to work and other patches
are dependent on these that do fixes bugs.
The Bakeathon is coming up so hopefully we can test these
patch there.
Committed (tag: nfs-utils-2-8-3-rc7)
steved.
> ---
> Changes in v2:
> - properly regenerate manpages
> - fix up bogus merge conflict
> - add D_GENERAL xlog messages when nfsdctl starts and exits
> - Link to v1: https://lore.kernel.org/r/20250109-lockd-nl-v1-0-108548ab0b6b@kernel.org
>
> ---
> Jeff Layton (3):
> nfsdctl: convert to xlog()
> nfsdctl: fix the --version option
> nfsdctl: add necessary bits to configure lockd
>
> configure.ac | 4 +
> utils/nfsdctl/lockd_netlink.h | 29 ++++
> utils/nfsdctl/nfsdctl.8 | 15 +-
> utils/nfsdctl/nfsdctl.adoc | 8 +
> utils/nfsdctl/nfsdctl.c | 331 ++++++++++++++++++++++++++++++++++--------
> 5 files changed, 321 insertions(+), 66 deletions(-)
> ---
> base-commit: 65f4cc3a6ce1472ee4092c4bbf4b19beb0a8217b
> change-id: 20250109-lockd-nl-6272fa9e8a5d
>
> Best regards,
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2025-03-19 19:47 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-10 13:46 [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Jeff Layton
2025-01-10 13:46 ` [PATCH v2 1/3] nfsdctl: convert to xlog() Jeff Layton
2025-01-10 13:46 ` [PATCH v2 2/3] nfsdctl: fix the --version option Jeff Layton
2025-01-10 13:46 ` [PATCH v2 3/3] nfsdctl: add necessary bits to configure lockd Jeff Layton
2025-01-10 15:05 ` Tom Talpey
2025-01-10 15:21 ` Jeff Layton
2025-01-10 15:40 ` Tom Talpey
2025-01-13 13:39 ` Benjamin Coddington
2025-01-14 15:53 ` Tom Talpey
2025-01-14 21:09 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Scott Mayhew
2025-01-14 21:18 ` Jeff Layton
2025-01-15 14:44 ` Scott Mayhew
2025-01-15 14:56 ` Jeff Layton
2025-01-15 15:12 ` Steve Dickson
2025-01-15 15:28 ` Jeff Layton
2025-01-15 16:40 ` Scott Mayhew
2025-01-15 17:00 ` [nfs-utils PATCH] nfsdctl: debug logging fixups Scott Mayhew
2025-01-15 17:02 ` Jeff Layton
2025-01-15 17:32 ` Steve Dickson
2025-01-15 17:35 ` Jeff Layton
2025-01-15 17:47 ` Steve Dickson
2025-01-15 18:33 ` Jeff Layton
2025-01-15 20:53 ` Steve Dickson
2025-01-16 11:50 ` Jeff Layton
2025-01-16 21:00 ` Steve Dickson
2025-01-16 21:12 ` Jeff Layton
2025-03-19 19:47 ` [PATCH v2 0/3] nfsdctl: add support for new lockd configuration interface Steve Dickson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox