[PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

This is a tweak of the patch set Alice Mitchell posted last July [1].
It enables the setting of the nfs4_unique_id kernel module 
parameter from /etc/nfs.conf. 

Things I tweaked:

    * Introduce a new [kernel] section in nfs.conf which only
      contains the nfs4_unique_id setting... For now... 

    * nfs4_unique_id can be set to two different values
    
        - nfs4_unique_id = ${machine-id} will use /etc/machine-id
            as the unique id.
        - nfs4_unique_id = ${hostname} will use the system's hostname
            as the unique id.

    * The new nfs-config systemd service need to be enabled for the
      /etc/modprobe.d/nfs.conf file to be created with 
      the "options nfs nfs4_unique_id=" set. 
  
I see this patch set is not a way to set the nfs4_unique_id 
module parameter... I see it as a beginning of a way to set 
all module parameters from /etc/nfs.conf, which I think
is a good thing... 

[1] https://www.spinics.net/lists/linux-nfs/msg78658.html

Alice Mitchell (3):
  nfs-utils: Enable the retrieval of raw config settings without
    expansion
  nfs-utils: Add support for further ${variable} expansions in nfs.conf
  nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf
    value

 configure.ac                  |   1 +
 nfs.conf                      |   4 +-
 support/include/conffile.h    |   1 +
 support/nfs/conffile.c        | 283 ++++++++++++++++++++++++++++++++--
 systemd/Makefile.am           |   3 +
 systemd/nfs-client.target     |   3 +
 systemd/nfs-conf-export.sh    |  28 ++++
 systemd/nfs-config.service.in |  18 +++
 systemd/nfs.conf.man          |  19 ++-
 tools/nfsconf/nfsconf.man     |  10 +-
 tools/nfsconf/nfsconfcli.c    |  22 ++-
 11 files changed, 372 insertions(+), 20 deletions(-)
 create mode 100755 systemd/nfs-conf-export.sh
 create mode 100644 systemd/nfs-config.service.in

-- 
2.30.2

[PATCH 1/3] nfs-utils: Enable the retrieval of raw config settings without expansion

[Steve Dickson]

From: Alice Mitchell <ajmitchell@redhat.com>

Config entries sometimes contain variable expansions, this adds options
to retrieve the config entry rather than its current expanded value.

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
Signed-off-by: Steve Dickson <steved@redhat.com>
---
 support/include/conffile.h |  1 +
 support/nfs/conffile.c     | 23 +++++++++++++++++++++++
 tools/nfsconf/nfsconf.man  | 10 +++++++++-
 tools/nfsconf/nfsconfcli.c | 22 ++++++++++++++++------
 4 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/support/include/conffile.h b/support/include/conffile.h
index 7d974fe9..c4a3ca62 100644
--- a/support/include/conffile.h
+++ b/support/include/conffile.h
@@ -61,6 +61,7 @@ extern _Bool    conf_get_bool(const char *, const char *, _Bool);
 extern char    *conf_get_str(const char *, const char *);
 extern char    *conf_get_str_with_def(const char *, const char *, char *);
 extern char    *conf_get_section(const char *, const char *, const char *);
+extern char    *conf_get_entry(const char *, const char *, const char *);
 extern int      conf_init_file(const char *);
 extern void     conf_cleanup(void);
 extern int      conf_match_num(const char *, const char *, int);
diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index 1e15e7d5..fd4a17ad 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -891,6 +891,29 @@ conf_get_str_with_def(const char *section, const char *tag, char *def)
 	return result;
 }
 
+/*
+ * Retrieve an entry without interpreting its contents
+ */
+char *
+conf_get_entry(const char *section, const char *arg, const char *tag)
+{
+	struct conf_binding *cb;
+
+	cb = LIST_FIRST (&conf_bindings[conf_hash (section)]);
+	for (; cb; cb = LIST_NEXT (cb, link)) {
+		if (strcasecmp(section, cb->section) != 0)
+			continue;
+		if (arg && (cb->arg == NULL || strcasecmp(arg, cb->arg) != 0))
+			continue;
+		if (!arg && cb->arg)
+			continue;
+		if (strcasecmp(tag, cb->tag) != 0)
+			continue;
+		return cb->value;
+	}
+	return 0;
+}
+
 /*
  * Find a section that may or may not have an argument
  */
diff --git a/tools/nfsconf/nfsconf.man b/tools/nfsconf/nfsconf.man
index 30791988..d44e86fb 100644
--- a/tools/nfsconf/nfsconf.man
+++ b/tools/nfsconf/nfsconf.man
@@ -11,6 +11,12 @@ nfsconf \- Query various NFS configuration settings
 .IR infile.conf ]
 .RI [ outfile ]
 .P
+.B nfsconf \-\-entry
+.RB [ \-\-arg  
+.IR subsection]
+.IR section
+.IR tag
+.P
 .B nfsconf \-\-get
 .RB [ \-v | \-\-verbose ]
 .RB [ \-f | \-\-file
@@ -58,6 +64,8 @@ from a range of nfs-utils configuration files.
 The following modes are available:
 .IP "\fB\-d, \-\-dump\fP"
 Output an alphabetically sorted dump of the current configuration in conf file format. Accepts an optional filename in which to write the output.
+.IP "\fB\-e, \-\-entry\fP"
+retrieve the config entry rather than its current expanded value
 .IP "\fB\-i, \-\-isset\fP"
 Test if a specific tag has a value set.
 .IP "\fB\-g, \-\-get\fP"
@@ -75,7 +83,7 @@ Increase verbosity and print debugging information.
 .B \-f, \-\-file \fIinfile\fR
 Select a different config file to operate upon, default is
 .I /etc/nfs.conf
-.SS Options only valid in \fB\-\-get\fR and \fB\-\-isset\fR modes.
+.SS Options only valid in \fB\-\-entry\fR and \fB\-\-get\fR and \fB\-\-isset\fR modes.
 .TP
 .B \-a, \-\-arg \fIsubsection\fR
 Select a specific sub-section
diff --git a/tools/nfsconf/nfsconfcli.c b/tools/nfsconf/nfsconfcli.c
index 361d386e..b2ef96d1 100644
--- a/tools/nfsconf/nfsconfcli.c
+++ b/tools/nfsconf/nfsconfcli.c
@@ -11,6 +11,7 @@
 typedef enum {
 	MODE_NONE,
 	MODE_GET,
+	MODE_ENTRY,
 	MODE_ISSET,
 	MODE_DUMP,
 	MODE_SET,
@@ -30,6 +31,8 @@ static void usage(const char *name)
 	fprintf(stderr, "      Outputs the configuration to the named file\n");
 	fprintf(stderr, "  --get [--arg subsection] {section} {tag}\n");
 	fprintf(stderr, "      Output one specific config value\n");
+	fprintf(stderr, "  --entry [--arg subsection] {section} {tag}\n");
+	fprintf(stderr, "      Output the uninterpreted config entry\n");
 	fprintf(stderr, "  --isset [--arg subsection] {section} {tag}\n");
 	fprintf(stderr, "      Return code indicates if config value is present\n");
 	fprintf(stderr, "  --set [--arg subsection] {section} {tag} {value}\n");
@@ -55,6 +58,7 @@ int main(int argc, char **argv)
 		int index = 0;
 		struct option long_options[] = {
 			{"get",		no_argument, 0, 'g' },
+			{"entry",	no_argument, 0, 'e' },
 			{"set",		no_argument, 0, 's' },
 			{"unset",	no_argument, 0, 'u' },
 			{"arg",	  required_argument, 0, 'a' },
@@ -66,7 +70,7 @@ int main(int argc, char **argv)
 			{NULL,			  0, 0, 0 }
 		};
 
-		c = getopt_long(argc, argv, "gsua:id::f:vm:", long_options, &index);
+		c = getopt_long(argc, argv, "gesua:id::f:vm:", long_options, &index);
 		if (c == -1) break;
 
 		switch (c) {
@@ -86,6 +90,9 @@ int main(int argc, char **argv)
 			case 'g':
 				mode = MODE_GET;
 				break;
+			case 'e':
+				mode = MODE_ENTRY;
+				break;
 			case 's':
 				mode = MODE_SET;
 				break;
@@ -167,8 +174,8 @@ int main(int argc, char **argv)
 		if (dumpfile)
 			fclose(out);
 	} else
-	/* --iset and --get share a lot of code */
-	if (mode == MODE_GET || mode == MODE_ISSET) {
+	/* --isset and --get share a lot of code */
+	if (mode == MODE_GET || mode == MODE_ISSET || mode == MODE_ENTRY) {
 		char * section = NULL;
 		char * tag = NULL;
 		const char * val;
@@ -186,14 +193,17 @@ int main(int argc, char **argv)
 		tag = argv[optind++];
 
 		/* retrieve the specified tags value */
-		val = conf_get_section(section, arg, tag);
+		if (mode == MODE_ENTRY)
+			val = conf_get_entry(section, arg, tag);
+		else
+			val = conf_get_section(section, arg, tag);
 		if (val != NULL) {
 			/* ret=0, success, mode --get wants to output the value as well */
-			if (mode == MODE_GET)
+			if (mode != MODE_ISSET)
 				printf("%s\n", val);
 		} else {
 			/* ret=1, no value found, tell the user if they asked */
-			if (mode == MODE_GET && verbose)
+			if (mode != MODE_ISSET && verbose)
 				fprintf(stderr, "Tag '%s' not found\n", tag);
 			ret = 1;
 		}
-- 
2.30.2

Re: [PATCH 1/3] nfs-utils: Enable the retrieval of raw config settings without expansion

[Steve Dickson]

On 4/14/21 2:10 PM, Steve Dickson wrote:
> From: Alice Mitchell <ajmitchell@redhat.com>
> 
> Config entries sometimes contain variable expansions, this adds options
> to retrieve the config entry rather than its current expanded value.
> 
> Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
> Signed-off-by: Steve Dickson <steved@redhat.com>
> ---
>  support/include/conffile.h |  1 +
>  support/nfs/conffile.c     | 23 +++++++++++++++++++++++
>  tools/nfsconf/nfsconf.man  | 10 +++++++++-
>  tools/nfsconf/nfsconfcli.c | 22 ++++++++++++++++------
>  4 files changed, 49 insertions(+), 7 deletions(-)
Committed (tag: nfs-utils-2-5-4-rc3)

steved.
> 
> diff --git a/support/include/conffile.h b/support/include/conffile.h
> index 7d974fe9..c4a3ca62 100644
> --- a/support/include/conffile.h
> +++ b/support/include/conffile.h
> @@ -61,6 +61,7 @@ extern _Bool    conf_get_bool(const char *, const char *, _Bool);
>  extern char    *conf_get_str(const char *, const char *);
>  extern char    *conf_get_str_with_def(const char *, const char *, char *);
>  extern char    *conf_get_section(const char *, const char *, const char *);
> +extern char    *conf_get_entry(const char *, const char *, const char *);
>  extern int      conf_init_file(const char *);
>  extern void     conf_cleanup(void);
>  extern int      conf_match_num(const char *, const char *, int);
> diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
> index 1e15e7d5..fd4a17ad 100644
> --- a/support/nfs/conffile.c
> +++ b/support/nfs/conffile.c
> @@ -891,6 +891,29 @@ conf_get_str_with_def(const char *section, const char *tag, char *def)
>  	return result;
>  }
>  
> +/*
> + * Retrieve an entry without interpreting its contents
> + */
> +char *
> +conf_get_entry(const char *section, const char *arg, const char *tag)
> +{
> +	struct conf_binding *cb;
> +
> +	cb = LIST_FIRST (&conf_bindings[conf_hash (section)]);
> +	for (; cb; cb = LIST_NEXT (cb, link)) {
> +		if (strcasecmp(section, cb->section) != 0)
> +			continue;
> +		if (arg && (cb->arg == NULL || strcasecmp(arg, cb->arg) != 0))
> +			continue;
> +		if (!arg && cb->arg)
> +			continue;
> +		if (strcasecmp(tag, cb->tag) != 0)
> +			continue;
> +		return cb->value;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Find a section that may or may not have an argument
>   */
> diff --git a/tools/nfsconf/nfsconf.man b/tools/nfsconf/nfsconf.man
> index 30791988..d44e86fb 100644
> --- a/tools/nfsconf/nfsconf.man
> +++ b/tools/nfsconf/nfsconf.man
> @@ -11,6 +11,12 @@ nfsconf \- Query various NFS configuration settings
>  .IR infile.conf ]
>  .RI [ outfile ]
>  .P
> +.B nfsconf \-\-entry
> +.RB [ \-\-arg  
> +.IR subsection]
> +.IR section
> +.IR tag
> +.P
>  .B nfsconf \-\-get
>  .RB [ \-v | \-\-verbose ]
>  .RB [ \-f | \-\-file
> @@ -58,6 +64,8 @@ from a range of nfs-utils configuration files.
>  The following modes are available:
>  .IP "\fB\-d, \-\-dump\fP"
>  Output an alphabetically sorted dump of the current configuration in conf file format. Accepts an optional filename in which to write the output.
> +.IP "\fB\-e, \-\-entry\fP"
> +retrieve the config entry rather than its current expanded value
>  .IP "\fB\-i, \-\-isset\fP"
>  Test if a specific tag has a value set.
>  .IP "\fB\-g, \-\-get\fP"
> @@ -75,7 +83,7 @@ Increase verbosity and print debugging information.
>  .B \-f, \-\-file \fIinfile\fR
>  Select a different config file to operate upon, default is
>  .I /etc/nfs.conf
> -.SS Options only valid in \fB\-\-get\fR and \fB\-\-isset\fR modes.
> +.SS Options only valid in \fB\-\-entry\fR and \fB\-\-get\fR and \fB\-\-isset\fR modes.
>  .TP
>  .B \-a, \-\-arg \fIsubsection\fR
>  Select a specific sub-section
> diff --git a/tools/nfsconf/nfsconfcli.c b/tools/nfsconf/nfsconfcli.c
> index 361d386e..b2ef96d1 100644
> --- a/tools/nfsconf/nfsconfcli.c
> +++ b/tools/nfsconf/nfsconfcli.c
> @@ -11,6 +11,7 @@
>  typedef enum {
>  	MODE_NONE,
>  	MODE_GET,
> +	MODE_ENTRY,
>  	MODE_ISSET,
>  	MODE_DUMP,
>  	MODE_SET,
> @@ -30,6 +31,8 @@ static void usage(const char *name)
>  	fprintf(stderr, "      Outputs the configuration to the named file\n");
>  	fprintf(stderr, "  --get [--arg subsection] {section} {tag}\n");
>  	fprintf(stderr, "      Output one specific config value\n");
> +	fprintf(stderr, "  --entry [--arg subsection] {section} {tag}\n");
> +	fprintf(stderr, "      Output the uninterpreted config entry\n");
>  	fprintf(stderr, "  --isset [--arg subsection] {section} {tag}\n");
>  	fprintf(stderr, "      Return code indicates if config value is present\n");
>  	fprintf(stderr, "  --set [--arg subsection] {section} {tag} {value}\n");
> @@ -55,6 +58,7 @@ int main(int argc, char **argv)
>  		int index = 0;
>  		struct option long_options[] = {
>  			{"get",		no_argument, 0, 'g' },
> +			{"entry",	no_argument, 0, 'e' },
>  			{"set",		no_argument, 0, 's' },
>  			{"unset",	no_argument, 0, 'u' },
>  			{"arg",	  required_argument, 0, 'a' },
> @@ -66,7 +70,7 @@ int main(int argc, char **argv)
>  			{NULL,			  0, 0, 0 }
>  		};
>  
> -		c = getopt_long(argc, argv, "gsua:id::f:vm:", long_options, &index);
> +		c = getopt_long(argc, argv, "gesua:id::f:vm:", long_options, &index);
>  		if (c == -1) break;
>  
>  		switch (c) {
> @@ -86,6 +90,9 @@ int main(int argc, char **argv)
>  			case 'g':
>  				mode = MODE_GET;
>  				break;
> +			case 'e':
> +				mode = MODE_ENTRY;
> +				break;
>  			case 's':
>  				mode = MODE_SET;
>  				break;
> @@ -167,8 +174,8 @@ int main(int argc, char **argv)
>  		if (dumpfile)
>  			fclose(out);
>  	} else
> -	/* --iset and --get share a lot of code */
> -	if (mode == MODE_GET || mode == MODE_ISSET) {
> +	/* --isset and --get share a lot of code */
> +	if (mode == MODE_GET || mode == MODE_ISSET || mode == MODE_ENTRY) {
>  		char * section = NULL;
>  		char * tag = NULL;
>  		const char * val;
> @@ -186,14 +193,17 @@ int main(int argc, char **argv)
>  		tag = argv[optind++];
>  
>  		/* retrieve the specified tags value */
> -		val = conf_get_section(section, arg, tag);
> +		if (mode == MODE_ENTRY)
> +			val = conf_get_entry(section, arg, tag);
> +		else
> +			val = conf_get_section(section, arg, tag);
>  		if (val != NULL) {
>  			/* ret=0, success, mode --get wants to output the value as well */
> -			if (mode == MODE_GET)
> +			if (mode != MODE_ISSET)
>  				printf("%s\n", val);
>  		} else {
>  			/* ret=1, no value found, tell the user if they asked */
> -			if (mode == MODE_GET && verbose)
> +			if (mode != MODE_ISSET && verbose)
>  				fprintf(stderr, "Tag '%s' not found\n", tag);
>  			ret = 1;
>  		}
> 

[PATCH 2/3] nfs-utils: Add support for further ${variable} expansions in nfs.conf

[Steve Dickson]

From: Alice Mitchell <ajmitchell@redhat.com>

This adds support for substituting in the systems machine_id or
the hostname as the unique id, and caches the results

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
Signed-off-by: Steve Dickson <steved@redhat.com>
---
 support/nfs/conffile.c | 260 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 249 insertions(+), 11 deletions(-)

diff --git a/support/nfs/conffile.c b/support/nfs/conffile.c
index fd4a17ad..d03de012 100644
--- a/support/nfs/conffile.c
+++ b/support/nfs/conffile.c
@@ -40,6 +40,7 @@
 #include <sys/stat.h>
 #include <netinet/in.h>
 #include <arpa/inet.h>
+#include <linux/if_alg.h>
 #include <ctype.h>
 #include <fcntl.h>
 #include <stdio.h>
@@ -114,12 +115,66 @@ struct conf_binding {
   char *tag;
   char *value;
   int is_default;
+  char *cache;
 };
 
 LIST_HEAD (conf_bindings, conf_binding) conf_bindings[256];
 
+typedef char * (*expand_fn_t)(void);
+struct expansion_types {
+	const char *name;
+	expand_fn_t func;
+};
+
+typedef struct {
+	uint8_t bytes[16];
+} id128_t;
+
+/*
+ * Application ID for use with generating a machine-id string
+ */
+static id128_t nfs_appid = {.bytes = {0xff,0x3b,0xf0,0x0f,0x34,0xa6,0x43,0xc5, \
+                                       0x93,0xdd,0x16,0xdc,0x7c,0xeb,0x88,0xc8}};
+
 const char *modified_by = NULL;
 
+static __inline__ char
+hexchar(int x) {
+	static const char table[16] = "0123456789abcdef";
+	return table[x & 15];
+}
+
+static __inline__ int
+unhexchar(char h)
+{
+	if (h >= '0' && h <= '9')
+		return h - '0';
+	if (h >= 'a' && h <= 'f')
+		return h - 'a' + 10;
+	if (h >= 'A' && h <= 'F')
+		return h - 'A' + 10;
+	return -1;
+}
+
+static char *
+tohexstr(const unsigned char *data, int len)
+{
+	int i;
+	char *result = NULL;
+
+	result = calloc(1, (len*2)+1);
+	if (!result) {
+		xlog(L_ERROR, "malloc error formatting string");
+		return NULL;
+	}
+
+	for (i = 0; i < len; i++) {
+		result[i*2] = hexchar(data[i] >> 4);
+		result[i*2+1] = hexchar(data[i] & 0x0F);
+	}
+	return result;
+}
+
 static __inline__ uint8_t
 conf_hash(const char *s)
 {
@@ -132,6 +187,193 @@ conf_hash(const char *s)
 	return hash;
 }
 
+static int
+id128_from_string(const char s[], id128_t *ret)
+{
+	id128_t t;
+	unsigned int n, i;
+	for (n=0, i=0; n<16; ) {
+		int a, b;
+		a = unhexchar(s[i++]);
+		if (a < 0)
+			return 1;
+		b = unhexchar(s[i++]);
+		if (b < 0)
+			return 1;
+
+		t.bytes[n++] = (a << 4) | b;
+	}
+	if (s[i] != 0)
+		return 1;
+	if (ret)
+		*ret = t;
+	return 0;
+}
+
+/*
+ * cryptographic hash (sha256) data into a hex encoded string
+ */
+static char *
+strhash(unsigned char *key, size_t keylen, unsigned char *data, size_t dlen)
+{
+	union {
+		struct sockaddr sa;
+		struct sockaddr_alg alg;
+	} sa;
+	int sock = -1;
+	int hfd = -1;
+	uint8_t digest[129];
+	int n;
+	char *result = NULL;
+
+	memset(&sa, 0, sizeof(sa));
+	sa.alg.salg_family = AF_ALG;
+	strcpy((char *)sa.alg.salg_type, "hash");
+	strcpy((char *)sa.alg.salg_name, "hmac(sha256)");
+
+	sock = socket(AF_ALG, SOCK_SEQPACKET|SOCK_CLOEXEC, 0);
+	if (sock < 0) {
+		xlog(L_ERROR, "error creating socket");
+		goto cleanup;
+	}
+
+	if (bind(sock, (struct sockaddr *)&sa.sa, sizeof(sa)) < 0) {
+		xlog(L_ERROR, "error opening khash interface");
+		goto cleanup;
+	}
+
+	if (key && keylen > 0) {
+		if (setsockopt(sock, SOL_ALG, ALG_SET_KEY, key, keylen) < 0) {
+			xlog(L_ERROR, "Error setting key: %s", strerror(errno));
+			goto cleanup;
+		}
+	}
+
+	hfd = accept4(sock, NULL, 0, SOCK_CLOEXEC);
+	if (hfd < 0) {
+		xlog(L_ERROR, "Error initiating khash: %s", strerror(errno));
+		goto cleanup;
+	}
+
+	n = send(hfd, data, dlen, 0);
+	if (n < 0) {
+		xlog(L_ERROR, "Error updating khash: %s", strerror(errno));
+		goto cleanup;
+	}
+
+	n = recv(hfd, digest, sizeof(digest), 0);
+	if (n < 0) {
+		xlog(L_ERROR, "Error fetching khash: %s", strerror(errno));
+		goto cleanup;
+	}
+
+	result = tohexstr(digest, n);
+cleanup:
+	if (sock != -1)
+		close(sock);
+	if (hfd != -1)
+		close(hfd);
+	if (hfd != -1)
+		close(hfd);
+
+	return result;
+}
+
+/*
+ * Read one line of content from a file
+ */
+static char *
+read_oneline(const char *filename)
+{
+	char *content = conf_readfile(filename);
+	char *end;
+
+	if (content == NULL)
+		return NULL;
+
+	/* trim to only the first line */
+	end = strchr(content, '\n');
+	if (end != NULL)
+		*end = '\0';
+	end = strchr(content, '\r');
+	if (end != NULL)
+		*end = '\0';
+
+	return content;
+}
+
+static char *
+expand_machine_id(void)
+{
+	char *key = read_oneline("/etc/machine-id");
+	id128_t mid;
+	char * result = NULL;
+	size_t idlen = 0;
+
+	if (key == NULL)
+		return NULL;
+
+	idlen = strlen(key);
+	if (!id128_from_string(key, &mid)) {
+		result = strhash(mid.bytes, sizeof(mid), nfs_appid.bytes, sizeof(nfs_appid));
+		if (result && strlen(result) > idlen)
+			result[idlen]=0;
+	}
+	free(key);
+	return result;
+}
+
+static char *
+expand_hostname(void)
+{
+	int maxlen = HOST_NAME_MAX + 1;
+	char * hostname = calloc(1, maxlen);
+
+	if (!hostname)
+		return NULL;
+	if ((gethostname(hostname, maxlen)) == -1) {
+		free(hostname);
+		return NULL;
+	}
+	return hostname;
+}
+
+static struct expansion_types  var_expansions[] = {
+	{ "machine-id", expand_machine_id },
+	{ "hostname", expand_hostname },
+};
+
+/* Deal with more complex variable substitutions */
+static char *
+expand_variable(const char *name)
+{
+	size_t len;
+
+	if (name == NULL || name[0] != '$')
+		return NULL;
+
+	len = strlen(name);
+	if (name[1] == '{' && name[len-1] == '}') {
+		char *varname = strndupa(&name[2], len-3);
+
+		for (size_t i=0; i<sizeof(var_expansions); i++) {
+			if (!strcasecmp(varname, var_expansions[i].name)) {
+				return var_expansions[i].func();
+			}
+		}
+		xlog_warn("get_conf: Unknown variable ${%s}", varname);
+	} else {
+		/* expand $name from [environment] section,
+		* or from environment
+		*/
+		char *env = getenv(&name[1]);
+		if (env == NULL || *env == 0)
+			env = conf_get_section("environment", NULL, &name[1]);
+		return env;
+	}
+	return NULL;
+}
+
 /*
  * free all the component parts of a conf_binding struct
  */
@@ -147,6 +389,8 @@ static void free_confbind(struct conf_binding *cb)
 		free(cb->tag);
 	if (cb->value)
 		free(cb->value);
+	if (cb->cache)
+		free(cb->cache);
 	free(cb);
 }
 
@@ -921,7 +1165,7 @@ char *
 conf_get_section(const char *section, const char *arg, const char *tag)
 {
 	struct conf_binding *cb;
-retry:
+
 	cb = LIST_FIRST (&conf_bindings[conf_hash (section)]);
 	for (; cb; cb = LIST_NEXT (cb, link)) {
 		if (strcasecmp(section, cb->section) != 0)
@@ -933,19 +1177,13 @@ retry:
 		if (strcasecmp(tag, cb->tag) != 0)
 			continue;
 		if (cb->value[0] == '$') {
-			/* expand $name from [environment] section,
-			 * or from environment
-			 */
-			char *env = getenv(cb->value+1);
-			if (env && *env)
-				return env;
-			section = "environment";
-			tag = cb->value + 1;
-			goto retry;
+			if (!cb->cache)
+				cb->cache = expand_variable(cb->value);
+			return cb->cache;
 		}
 		return cb->value;
 	}
-	return 0;
+	return NULL;
 }
 
 /*
-- 
2.30.2

[PATCH 3/3] nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf value

[Steve Dickson]

From: Alice Mitchell <ajmitchell@redhat.com>

systemd service to grab the config value and feed it to the kernel module

Signed-off-by: Alice Mitchell <ajmitchell@redhat.com>
Signed-off-by: Steve Dickson <steved@redhat.com>
---
 configure.ac                  |  1 +
 nfs.conf                      |  4 +++-
 systemd/Makefile.am           |  3 +++
 systemd/nfs-client.target     |  3 +++
 systemd/nfs-conf-export.sh    | 28 ++++++++++++++++++++++++++++
 systemd/nfs-config.service.in | 18 ++++++++++++++++++
 systemd/nfs.conf.man          | 19 ++++++++++++++++++-
 7 files changed, 74 insertions(+), 2 deletions(-)
 create mode 100755 systemd/nfs-conf-export.sh
 create mode 100644 systemd/nfs-config.service.in

diff --git a/configure.ac b/configure.ac
index f2e1bd30..2db7214e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -684,6 +684,7 @@ AC_CONFIG_COMMANDS_PRE([eval eval _sysconfdir=$sysconfdir])
 AC_CONFIG_FILES([
 	Makefile
 	systemd/rpc-gssd.service
+	systemd/nfs-config.service
 	linux-nfs/Makefile
 	support/Makefile
 	support/export/Makefile
diff --git a/nfs.conf b/nfs.conf
index 31994f61..faa58071 100644
--- a/nfs.conf
+++ b/nfs.conf
@@ -1,9 +1,11 @@
 #
 # This is a general configuration for the
-# NFS daemons and tools
+# NFS daemons and tools and kernel module parameters
 #
 [general]
 # pipefs-directory=/var/lib/nfs/rpc_pipefs
+[kernel]
+# nfs4_unique_id = [${machine-id} || ${hostname}]
 #
 [exports]
 # rootdir=/export
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 650ad25c..c48fc80d 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -9,6 +9,7 @@ unit_files =  \
     nfs-mountd.service \
     nfs-server.service \
     nfs-utils.service \
+    nfs-config.service \
     rpc-statd-notify.service \
     rpc-statd.service \
     \
@@ -75,4 +76,6 @@ genexec_PROGRAMS = nfs-server-generator rpc-pipefs-generator
 install-data-hook: $(unit_files)
 	mkdir -p $(DESTDIR)/$(unitdir)
 	cp $(unit_files) $(DESTDIR)/$(unitdir)
+	mkdir -p $(DESTDIR)/$(libexecdir)/nfs-utils
+	install  nfs-conf-export.sh $(DESTDIR)/$(libexecdir)/nfs-utils/
 endif
diff --git a/systemd/nfs-client.target b/systemd/nfs-client.target
index 8a8300a1..3ca45752 100644
--- a/systemd/nfs-client.target
+++ b/systemd/nfs-client.target
@@ -11,6 +11,9 @@ Wants=rpc-statd-notify.service
 Wants=auth-rpcgss-module.service
 After=rpc-gssd.service rpc-svcgssd.service gssproxy.service
 
+# Run the config settings that are in nfs.conf
+After=nfs-config.service
+
 [Install]
 WantedBy=multi-user.target
 WantedBy=remote-fs.target
diff --git a/systemd/nfs-conf-export.sh b/systemd/nfs-conf-export.sh
new file mode 100755
index 00000000..905ece1b
--- /dev/null
+++ b/systemd/nfs-conf-export.sh
@@ -0,0 +1,28 @@
+#!/bin/bash
+#
+# This script pulls values out of /etc/nfs.conf and configures
+# the appropriate kernel modules which cannot read it directly
+
+NFSMOD=/sys/module/nfs/parameters/nfs4_unique_id
+NFSPROBE=/etc/modprobe.d/nfs.conf
+
+# Now read the values from nfs.conf
+MACHINEID=`nfsconf --get kernel nfs4_unique_id`
+if [ $? -ne 0 ] || [ "$MACHINEID" == "" ]
+then
+# No config value found, nothing to do
+	exit 0
+fi
+
+# Kernel module is already loaded, update the live one
+if [ -e $NFSMOD ]; then
+echo -n "$MACHINEID" >> $NFSMOD
+fi
+
+# Rewrite the modprobe file for next reboot
+echo "# This file is overwritten by systemd nfs-config.service" > $NFSPROBE
+echo "# with values taken from /etc/nfs.conf" >> $NFSPROBE
+echo "# Do not hand modify" >> $NFSPROBE
+echo "options nfs nfs4_unique_id=\"$MACHINEID\"" >> $NFSPROBE
+
+echo "Set to: $MACHINEID"
diff --git a/systemd/nfs-config.service.in b/systemd/nfs-config.service.in
new file mode 100644
index 00000000..08a09a5c
--- /dev/null
+++ b/systemd/nfs-config.service.in
@@ -0,0 +1,18 @@
+[Unit]
+Description=Preprocess NFS configuration
+PartOf=nfs-client.target
+Before=nfs-client.target
+After=local-fs.target
+DefaultDependencies=no
+
+[Service]
+Type=oneshot
+# This service needs to run any time any nfs service
+# is started, so changes to local config files get
+# incorporated.  Having "RemainAfterExit=no" (the default)
+# ensures this happens.
+RemainAfterExit=no
+ExecStart=/usr/libexec/nfs-utils/nfs-conf-export.sh
+
+[Install]
+WantedBy=nfs-client.target
diff --git a/systemd/nfs.conf.man b/systemd/nfs.conf.man
index 4436a38a..8f073fe5 100644
--- a/systemd/nfs.conf.man
+++ b/systemd/nfs.conf.man
@@ -101,7 +101,7 @@ When a list is given, the members should be comma-separated.
 .TP
 .B general
 Recognized values:
-.BR pipefs-directory .
+.BR pipefs-directory.
 
 See
 .BR blkmapd (8),
@@ -148,6 +148,23 @@ is equivalent to providing the
 .B \-\-log\-auth
 option.
 
+.TP
+.B kernel
+.br
+Recognized value:
+.BR nfs4_unique_id .
+
+Setting 
+.B "nfs4_unique_id= ${machine-id}"
+will set the nfs4_unique_id kernel module parameter
+to the systems machine_id (/etc/machine-id)
+.BR
+
+Setting
+.BR "nfs4_unique_id= ${hostname}"
+will set the nfs4_unique_id kernel module parameter
+to the systems hostname (/etc/hostname)
+
 .TP
 .B nfsdcltrack
 Recognized values:
-- 
2.30.2

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Chuck Lever III]

Hi Steve-

> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
> 
> This is a tweak of the patch set Alice Mitchell posted last July [1].

That approach was dropped last July because it is not container-aware.
It should be simple for someone to write a udev script that uses the
existing sysfs API that can update nfs4_client_id in a namespace. I
would prefer the sysfs/udev approach for setting nfs4_client_id,
since it is container-aware and makes this setting completely
automatic (zero touch).


> It enables the setting of the nfs4_unique_id kernel module 
> parameter from /etc/nfs.conf.

> Things I tweaked:
> 
>    * Introduce a new [kernel] section in nfs.conf which only
>      contains the nfs4_unique_id setting... For now... 
> 
>    * nfs4_unique_id can be set to two different values
> 
>        - nfs4_unique_id = ${machine-id} will use /etc/machine-id
>            as the unique id.
>        - nfs4_unique_id = ${hostname} will use the system's hostname
>            as the unique id.
> 
>    * The new nfs-config systemd service need to be enabled for the
>      /etc/modprobe.d/nfs.conf file to be created with 
>      the "options nfs nfs4_unique_id=" set. 
> 
> I see this patch set is not a way to set the nfs4_unique_id 
> module parameter... I see it as a beginning of a way to set 
> all module parameters from /etc/nfs.conf, which I think
> is a good thing... 
> 
> [1] https://www.spinics.net/lists/linux-nfs/msg78658.html
> 
> Alice Mitchell (3):
>  nfs-utils: Enable the retrieval of raw config settings without
>    expansion
>  nfs-utils: Add support for further ${variable} expansions in nfs.conf
>  nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf
>    value
> 
> configure.ac                  |   1 +
> nfs.conf                      |   4 +-
> support/include/conffile.h    |   1 +
> support/nfs/conffile.c        | 283 ++++++++++++++++++++++++++++++++--
> systemd/Makefile.am           |   3 +
> systemd/nfs-client.target     |   3 +
> systemd/nfs-conf-export.sh    |  28 ++++
> systemd/nfs-config.service.in |  18 +++
> systemd/nfs.conf.man          |  19 ++-
> tools/nfsconf/nfsconf.man     |  10 +-
> tools/nfsconf/nfsconfcli.c    |  22 ++-
> 11 files changed, 372 insertions(+), 20 deletions(-)
> create mode 100755 systemd/nfs-conf-export.sh
> create mode 100644 systemd/nfs-config.service.in
> 
> -- 
> 2.30.2
> 

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

Hey Chuck! 

On 4/14/21 7:26 PM, Chuck Lever III wrote:
> Hi Steve-
> 
>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>
>> This is a tweak of the patch set Alice Mitchell posted last July [1].
> 
> That approach was dropped last July because it is not container-aware.
> It should be simple for someone to write a udev script that uses the
> existing sysfs API that can update nfs4_client_id in a namespace. I
> would prefer the sysfs/udev approach for setting nfs4_client_id,
> since it is container-aware and makes this setting completely
> automatic (zero touch).
As I said in in my cover letter, I see this more as introduction of
a mechanism more than a way to set the unique id. The mechanism being
a way to set kernel module params from nfs.conf. The setting of
the id is just a side effect... 

Why spread out the NFS configuration?  Why not
just keep it in one place... aka nfs.conf?

Plus we could document all the kernel params in nfs.conf
and the nfs.conf man page. The only documentation I know 
of is in the kernel tree.

As far as not being container-aware... that might true
but it does not mean its not useful to set the id from
nfs.conf... Actual I have customers asking for this type
of functionality

steved.
> 
> 
>> It enables the setting of the nfs4_unique_id kernel module 
>> parameter from /etc/nfs.conf.
> 
>> Things I tweaked:
>>
>>    * Introduce a new [kernel] section in nfs.conf which only
>>      contains the nfs4_unique_id setting... For now... 
>>
>>    * nfs4_unique_id can be set to two different values
>>
>>        - nfs4_unique_id = ${machine-id} will use /etc/machine-id
>>            as the unique id.
>>        - nfs4_unique_id = ${hostname} will use the system's hostname
>>            as the unique id.
>>
>>    * The new nfs-config systemd service need to be enabled for the
>>      /etc/modprobe.d/nfs.conf file to be created with 
>>      the "options nfs nfs4_unique_id=" set. 
>>
>> I see this patch set is not a way to set the nfs4_unique_id 
>> module parameter... I see it as a beginning of a way to set 
>> all module parameters from /etc/nfs.conf, which I think
>> is a good thing... 
>>
>> [1] https://www.spinics.net/lists/linux-nfs/msg78658.html
>>
>> Alice Mitchell (3):
>>  nfs-utils: Enable the retrieval of raw config settings without
>>    expansion
>>  nfs-utils: Add support for further ${variable} expansions in nfs.conf
>>  nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf
>>    value
>>
>> configure.ac                  |   1 +
>> nfs.conf                      |   4 +-
>> support/include/conffile.h    |   1 +
>> support/nfs/conffile.c        | 283 ++++++++++++++++++++++++++++++++--
>> systemd/Makefile.am           |   3 +
>> systemd/nfs-client.target     |   3 +
>> systemd/nfs-conf-export.sh    |  28 ++++
>> systemd/nfs-config.service.in |  18 +++
>> systemd/nfs.conf.man          |  19 ++-
>> tools/nfsconf/nfsconf.man     |  10 +-
>> tools/nfsconf/nfsconfcli.c    |  22 ++-
>> 11 files changed, 372 insertions(+), 20 deletions(-)
>> create mode 100755 systemd/nfs-conf-export.sh
>> create mode 100644 systemd/nfs-config.service.in
>>
>> -- 
>> 2.30.2
>>

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Chuck Lever III]

> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> Hey Chuck! 
> 
> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>> Hi Steve-
>> 
>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>> 
>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>> 
>> That approach was dropped last July because it is not container-aware.
>> It should be simple for someone to write a udev script that uses the
>> existing sysfs API that can update nfs4_client_id in a namespace. I
>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>> since it is container-aware and makes this setting completely
>> automatic (zero touch).
> As I said in in my cover letter, I see this more as introduction of
> a mechanism more than a way to set the unique id.

Yep, I got that.

I'm not addressing the question of whether adding a
mechanism to set a module parameter in nfs.conf is good
or not. I'm saying nfs4_client_id is not an appropriate
parameter to add to nfs.conf. Can you pick another
module parameter as an example for your mechanism?


> The mechanism being
> a way to set kernel module params from nfs.conf. The setting of
> the id is just a side effect... 
> 
> Why spread out the NFS configuration?  Why not
> just keep it in one place... aka nfs.conf?

We need to understand whether a module parameter is not
going to work in nfs.conf because that setting needs to
be namespace-aware. In this case, this setting does indeed
need to be namespace-aware. nfs.conf is not aware of
network namespaces.


> Plus we could document all the kernel params in nfs.conf
> and the nfs.conf man page. The only documentation I know 
> of is in the kernel tree.

OK, but that's not relevant to whether nfs.conf is the
right place to set nfs4_client_id.


> As far as not being container-aware... that might true
> but it does not mean its not useful to set the id from
> nfs.conf...

Yes, it does mean that in that case. It's completely
broken to use the same nfs4_client_id in every network
namespace.


> Actual I have customers asking for this type
> of functionality

Ask yourself why they might want it. It's probably because
we don't set it correctly currently. If we have a way to
automatically get it right every time, there's really no
need for this setting to be exposed.

I do agree that it's long past time we should be setting
nfs4_client_id properly. I would rather see a udev script
developed (you, me, or Alice could do it in an afternoon)
first. If that doesn't meet the actual customer need, then
we can revisit.


> steved.
>> 
>> 
>>> It enables the setting of the nfs4_unique_id kernel module 
>>> parameter from /etc/nfs.conf.
>> 
>>> Things I tweaked:
>>> 
>>>   * Introduce a new [kernel] section in nfs.conf which only
>>>     contains the nfs4_unique_id setting... For now... 
>>> 
>>>   * nfs4_unique_id can be set to two different values
>>> 
>>>       - nfs4_unique_id = ${machine-id} will use /etc/machine-id
>>>           as the unique id.
>>>       - nfs4_unique_id = ${hostname} will use the system's hostname
>>>           as the unique id.
>>> 
>>>   * The new nfs-config systemd service need to be enabled for the
>>>     /etc/modprobe.d/nfs.conf file to be created with 
>>>     the "options nfs nfs4_unique_id=" set. 
>>> 
>>> I see this patch set is not a way to set the nfs4_unique_id 
>>> module parameter... I see it as a beginning of a way to set 
>>> all module parameters from /etc/nfs.conf, which I think
>>> is a good thing... 
>>> 
>>> [1] https://www.spinics.net/lists/linux-nfs/msg78658.html
>>> 
>>> Alice Mitchell (3):
>>> nfs-utils: Enable the retrieval of raw config settings without
>>>   expansion
>>> nfs-utils: Add support for further ${variable} expansions in nfs.conf
>>> nfs-utils: Update nfs4_unique_id module parameter from the nfs.conf
>>>   value
>>> 
>>> configure.ac                  |   1 +
>>> nfs.conf                      |   4 +-
>>> support/include/conffile.h    |   1 +
>>> support/nfs/conffile.c        | 283 ++++++++++++++++++++++++++++++++--
>>> systemd/Makefile.am           |   3 +
>>> systemd/nfs-client.target     |   3 +
>>> systemd/nfs-conf-export.sh    |  28 ++++
>>> systemd/nfs-config.service.in |  18 +++
>>> systemd/nfs.conf.man          |  19 ++-
>>> tools/nfsconf/nfsconf.man     |  10 +-
>>> tools/nfsconf/nfsconfcli.c    |  22 ++-
>>> 11 files changed, 372 insertions(+), 20 deletions(-)
>>> create mode 100755 systemd/nfs-conf-export.sh
>>> create mode 100644 systemd/nfs-config.service.in
>>> 
>>> -- 
>>> 2.30.2
>>> 
> 

--
Chuck Lever

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Thu, 2021-04-15 at 16:37 +0000, Chuck Lever III wrote:
> 
> 
> > On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com>
> > wrote:
> > 
> > Hey Chuck! 
> > 
> > On 4/14/21 7:26 PM, Chuck Lever III wrote:
> > > Hi Steve-
> > > 
> > > > On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com>
> > > > wrote:
> > > > 
> > > > This is a tweak of the patch set Alice Mitchell posted last
> > > > July [1].
> > > 
> > > That approach was dropped last July because it is not container-
> > > aware.
> > > It should be simple for someone to write a udev script that uses
> > > the
> > > existing sysfs API that can update nfs4_client_id in a namespace.
> > > I
> > > would prefer the sysfs/udev approach for setting nfs4_client_id,
> > > since it is container-aware and makes this setting completely
> > > automatic (zero touch).
> > As I said in in my cover letter, I see this more as introduction of
> > a mechanism more than a way to set the unique id.
> 
> Yep, I got that.
> 
> I'm not addressing the question of whether adding a
> mechanism to set a module parameter in nfs.conf is good
> or not. I'm saying nfs4_client_id is not an appropriate
> parameter to add to nfs.conf. Can you pick another
> module parameter as an example for your mechanism?
> 
> 
> > The mechanism being
> > a way to set kernel module params from nfs.conf. The setting of
> > the id is just a side effect... 
> > 
> > Why spread out the NFS configuration?  Why not
> > just keep it in one place... aka nfs.conf?
> 
> We need to understand whether a module parameter is not
> going to work in nfs.conf because that setting needs to
> be namespace-aware. In this case, this setting does indeed
> need to be namespace-aware. nfs.conf is not aware of
> network namespaces.
> 
> 
> > Plus we could document all the kernel params in nfs.conf
> > and the nfs.conf man page. The only documentation I know 
> > of is in the kernel tree.
> 
> OK, but that's not relevant to whether nfs.conf is the
> right place to set nfs4_client_id.
> 
> 
> > As far as not being container-aware... that might true
> > but it does not mean its not useful to set the id from
> > nfs.conf...
> 
> Yes, it does mean that in that case. It's completely
> broken to use the same nfs4_client_id in every network
> namespace.
> 
> 
> > Actual I have customers asking for this type
> > of functionality
> 
> Ask yourself why they might want it. It's probably because
> we don't set it correctly currently. If we have a way to
> automatically get it right every time, there's really no
> need for this setting to be exposed.
> 
> I do agree that it's long past time we should be setting
> nfs4_client_id properly. I would rather see a udev script
> developed (you, me, or Alice could do it in an afternoon)
> first. If that doesn't meet the actual customer need, then
> we can revisit.
> 

Right. The only sensible solution in a containerised world is a udev
script that sets /sys/fs/nfs/net/nfs_client/identifier when triggered.

Note that we really want something that generates a random uuid, and
then persists it so that it can be retrieved on reboot or restart of
the container. Something similar to systemd-machine-id-setup, but that
can be called from udev.

> 
> > steved.
> > > 
> > > 
> > > > It enables the setting of the nfs4_unique_id kernel module 
> > > > parameter from /etc/nfs.conf.
> > > 
> > > > Things I tweaked:
> > > > 
> > > >   * Introduce a new [kernel] section in nfs.conf which only
> > > >     contains the nfs4_unique_id setting... For now... 
> > > > 
> > > >   * nfs4_unique_id can be set to two different values
> > > > 
> > > >       - nfs4_unique_id = ${machine-id} will use /etc/machine-id
> > > >           as the unique id.
> > > >       - nfs4_unique_id = ${hostname} will use the system's
> > > > hostname
> > > >           as the unique id.
> > > > 
> > > >   * The new nfs-config systemd service need to be enabled for
> > > > the
> > > >     /etc/modprobe.d/nfs.conf file to be created with 
> > > >     the "options nfs nfs4_unique_id=" set. 
> > > > 
> > > > I see this patch set is not a way to set the nfs4_unique_id 
> > > > module parameter... I see it as a beginning of a way to set 
> > > > all module parameters from /etc/nfs.conf, which I think
> > > > is a good thing... 
> > > > 
> > > > [1] https://www.spinics.net/lists/linux-nfs/msg78658.html
> > > > 
> > > > Alice Mitchell (3):
> > > > nfs-utils: Enable the retrieval of raw config settings without
> > > >   expansion
> > > > nfs-utils: Add support for further ${variable} expansions in
> > > > nfs.conf
> > > > nfs-utils: Update nfs4_unique_id module parameter from the
> > > > nfs.conf
> > > >   value
> > > > 
> > > > configure.ac                  |   1 +
> > > > nfs.conf                      |   4 +-
> > > > support/include/conffile.h    |   1 +
> > > > support/nfs/conffile.c        | 283
> > > > ++++++++++++++++++++++++++++++++--
> > > > systemd/Makefile.am           |   3 +
> > > > systemd/nfs-client.target     |   3 +
> > > > systemd/nfs-conf-export.sh    |  28 ++++
> > > > systemd/nfs-config.service.in |  18 +++
> > > > systemd/nfs.conf.man          |  19 ++-
> > > > tools/nfsconf/nfsconf.man     |  10 +-
> > > > tools/nfsconf/nfsconfcli.c    |  22 ++-
> > > > 11 files changed, 372 insertions(+), 20 deletions(-)
> > > > create mode 100755 systemd/nfs-conf-export.sh
> > > > create mode 100644 systemd/nfs-config.service.in
> > > > 
> > > > -- 
> > > > 2.30.2
> > > > 
> > 
> 
> --
> Chuck Lever
> 
> 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Thu, 2021-04-15 at 23:30 +0000, Trond Myklebust wrote:
> On Thu, 2021-04-15 at 16:37 +0000, Chuck Lever III wrote:
> > 
> > 
> > > On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com>
> > > wrote:
> > > 
> > > Hey Chuck! 
> > > 
> > > On 4/14/21 7:26 PM, Chuck Lever III wrote:
> > > > Hi Steve-
> > > > 
> > > > > On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com>
> > > > > wrote:
> > > > > 
> > > > > This is a tweak of the patch set Alice Mitchell posted last
> > > > > July [1].
> > > > 
> > > > That approach was dropped last July because it is not container-
> > > > aware.
> > > > It should be simple for someone to write a udev script that uses
> > > > the
> > > > existing sysfs API that can update nfs4_client_id in a namespace.
> > > > I
> > > > would prefer the sysfs/udev approach for setting nfs4_client_id,
> > > > since it is container-aware and makes this setting completely
> > > > automatic (zero touch).
> > > As I said in in my cover letter, I see this more as introduction of
> > > a mechanism more than a way to set the unique id.
> > 
> > Yep, I got that.
> > 
> > I'm not addressing the question of whether adding a
> > mechanism to set a module parameter in nfs.conf is good
> > or not. I'm saying nfs4_client_id is not an appropriate
> > parameter to add to nfs.conf. Can you pick another
> > module parameter as an example for your mechanism?
> > 
> > 
> > > The mechanism being
> > > a way to set kernel module params from nfs.conf. The setting of
> > > the id is just a side effect... 
> > > 
> > > Why spread out the NFS configuration?  Why not
> > > just keep it in one place... aka nfs.conf?
> > 
> > We need to understand whether a module parameter is not
> > going to work in nfs.conf because that setting needs to
> > be namespace-aware. In this case, this setting does indeed
> > need to be namespace-aware. nfs.conf is not aware of
> > network namespaces.
> > 
> > 
> > > Plus we could document all the kernel params in nfs.conf
> > > and the nfs.conf man page. The only documentation I know 
> > > of is in the kernel tree.
> > 
> > OK, but that's not relevant to whether nfs.conf is the
> > right place to set nfs4_client_id.
> > 
> > 
> > > As far as not being container-aware... that might true
> > > but it does not mean its not useful to set the id from
> > > nfs.conf...
> > 
> > Yes, it does mean that in that case. It's completely
> > broken to use the same nfs4_client_id in every network
> > namespace.
> > 
> > 
> > > Actual I have customers asking for this type
> > > of functionality
> > 
> > Ask yourself why they might want it. It's probably because
> > we don't set it correctly currently. If we have a way to
> > automatically get it right every time, there's really no
> > need for this setting to be exposed.
> > 
> > I do agree that it's long past time we should be setting
> > nfs4_client_id properly. I would rather see a udev script
> > developed (you, me, or Alice could do it in an afternoon)
> > first. If that doesn't meet the actual customer need, then
> > we can revisit.
> > 
> 
> Right. The only sensible solution in a containerised world is a udev
> script that sets /sys/fs/nfs/net/nfs_client/identifier when triggered.
> 
> Note that we really want something that generates a random uuid, and
> then persists it so that it can be retrieved on reboot or restart of
> the container. Something similar to systemd-machine-id-setup, but that
> can be called from udev.
> 

Here is a skeleton example:

[root@leira rules.d]# cat /etc/udev/rules.d/50-nfs4.rules 
ACTION=="add" KERNEL=="nfs_client" ATTR{identifier}=="(null)" PROGRAM="/usr/sbin/nfs4_uuid" ATTR{identifier}="%c"

[root@leira rules.d]# cat /usr/sbin/nfs4_uuid 
#!/bin/bash
#
if [ ! -f /etc/nfs4_uuid ]
then
	uuid="$(uuidgen -r)"
	echo -n ${uuid} > /etc/nfs4_uuid
else
	uuid="$(cat /etc/nfs4_uuid)"
fi
echo ${uuid}


Obviously, the /usr/sbin/nfs4_uuid would need to be fleshed out a
little more to ensure that the file /etc/nfs4_uuid actually contains a
uuid in the right format, but you get the gist...

With the above additions, I end up with a repeatable

[root@leira rules.d]# modprobe nfs4
[root@leira rules.d]# cat /sys/fs/nfs/net/nfs_client/identifier 
7f9f211b-0253-4ef8-a970-b1b0f600af02
[root@leira rules.d]# cat /etc/nfs4_uuid 
7f9f211b-0253-4ef8-a970-b1b0f600af02

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

Hey!

On 4/15/21 8:40 PM, Trond Myklebust wrote:
> Here is a skeleton example:
> 
> [root@leira rules.d]# cat /etc/udev/rules.d/50-nfs4.rules 
> ACTION=="add" KERNEL=="nfs_client" ATTR{identifier}=="(null)" PROGRAM="/usr/sbin/nfs4_uuid" ATTR{identifier}="%c"
> 
> [root@leira rules.d]# cat /usr/sbin/nfs4_uuid 
> #!/bin/bash
> #
> if [ ! -f /etc/nfs4_uuid ]
> then
> 	uuid="$(uuidgen -r)"
> 	echo -n ${uuid} > /etc/nfs4_uuid
> else
> 	uuid="$(cat /etc/nfs4_uuid)"
> fi
> echo ${uuid}
> 
> 
> Obviously, the /usr/sbin/nfs4_uuid would need to be fleshed out a
> little more to ensure that the file /etc/nfs4_uuid actually contains a
> uuid in the right format, but you get the gist...
> 
> With the above additions, I end up with a repeatable
> 
> [root@leira rules.d]# modprobe nfs4
> [root@leira rules.d]# cat /sys/fs/nfs/net/nfs_client/identifier 
> 7f9f211b-0253-4ef8-a970-b1b0f600af02
> [root@leira rules.d]# cat /etc/nfs4_uuid 
> 7f9f211b-0253-4ef8-a970-b1b0f600af02

I see that this example does populate nfs_client/identifier and
I'm sure we could beef up the mechanism but the may question
is this.... 

How does populating nfs_client/identifier via udev
actually setting the nfs4_unique_id parameter which is used to set
the unique id? I look and i've must have missed it...

If the answer is we need to change the client to look a
the nfs_client/identifier... then we should get rid of the
nfs4_unique_id param all together... 

steved.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Sat, 2021-04-17 at 12:33 -0400, Steve Dickson wrote:
> Hey!
> 
> On 4/15/21 8:40 PM, Trond Myklebust wrote:
> > Here is a skeleton example:
> > 
> > [root@leira rules.d]# cat /etc/udev/rules.d/50-nfs4.rules 
> > ACTION=="add" KERNEL=="nfs_client" ATTR{identifier}=="(null)"
> > PROGRAM="/usr/sbin/nfs4_uuid" ATTR{identifier}="%c"
> > 
> > [root@leira rules.d]# cat /usr/sbin/nfs4_uuid 
> > #!/bin/bash
> > #
> > if [ ! -f /etc/nfs4_uuid ]
> > then
> >         uuid="$(uuidgen -r)"
> >         echo -n ${uuid} > /etc/nfs4_uuid
> > else
> >         uuid="$(cat /etc/nfs4_uuid)"
> > fi
> > echo ${uuid}
> > 
> > 
> > Obviously, the /usr/sbin/nfs4_uuid would need to be fleshed out a
> > little more to ensure that the file /etc/nfs4_uuid actually
> > contains a
> > uuid in the right format, but you get the gist...
> > 
> > With the above additions, I end up with a repeatable
> > 
> > [root@leira rules.d]# modprobe nfs4
> > [root@leira rules.d]# cat /sys/fs/nfs/net/nfs_client/identifier 
> > 7f9f211b-0253-4ef8-a970-b1b0f600af02
> > [root@leira rules.d]# cat /etc/nfs4_uuid 
> > 7f9f211b-0253-4ef8-a970-b1b0f600af02
> 
> I see that this example does populate nfs_client/identifier and
> I'm sure we could beef up the mechanism but the may question
> is this.... 
> 
> How does populating nfs_client/identifier via udev
> actually setting the nfs4_unique_id parameter which is used to set
> the unique id? I look and i've must have missed it...
> 
> If the answer is we need to change the client to look a
> the nfs_client/identifier... then we should get rid of the
> nfs4_unique_id param all together... 
> 

Commit 39d43d164127 ("NFSv4: Use the net namespace uniquifier if it is
set") should default to using the nfs_client identifier if it is set.
Otherwise it falls back to using the nfs4_unique_id. So kernels >= 5.10
are ready to use this udev-based mechanism.

The reason why I added udev support is, as I said, because we need
something that works correctly inside containers. Unfortunately, module
parameters are system-wide, so the older mechanism works just fine
right up to the moment where you fire up 'docker', 'kubernetes' or
'podman' (which is an increasingly important use case for NFS).

We do need something a little more sophisticated than the naive script
that I provided. As I said, that was intended as a skeleton example
just to demonstrate the basic mechanism and how to configure udev. It
would be very good to have something similar to what I showed there be
installed by default when you install nfs-utils.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/15/21 12:37 PM, Chuck Lever III wrote:
> 
> 
>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>
>> Hey Chuck! 
>>
>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>> Hi Steve-
>>>
>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>
>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>
>>> That approach was dropped last July because it is not container-aware.
>>> It should be simple for someone to write a udev script that uses the
>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>> since it is container-aware and makes this setting completely
>>> automatic (zero touch).
>> As I said in in my cover letter, I see this more as introduction of
>> a mechanism more than a way to set the unique id.
> 
> Yep, I got that.
> 
> I'm not addressing the question of whether adding a
> mechanism to set a module parameter in nfs.conf is good
> or not. I'm saying nfs4_client_id is not an appropriate
> parameter to add to nfs.conf. Can you pick another
> module parameter as an example for your mechanism?
The request was to set that parameter... 

> 
> 
>> The mechanism being
>> a way to set kernel module params from nfs.conf. The setting of
>> the id is just a side effect... 
>>
>> Why spread out the NFS configuration?  Why not
>> just keep it in one place... aka nfs.conf?
> 
> We need to understand whether a module parameter is not
> going to work in nfs.conf because that setting needs to
> be namespace-aware. In this case, this setting does indeed
> need to be namespace-aware. nfs.conf is not aware of
> network namespaces.
You probably can say that for every /etc/*.conf file...  
not being namespace aware... how can they be...

In the kernel document you say "Specifying a uniquifier string is 
not support for NFS clients running in containers."

Why isn't that enough? 

> 
> 
>> Plus we could document all the kernel params in nfs.conf
>> and the nfs.conf man page. The only documentation I know 
>> of is in the kernel tree.
> 
> OK, but that's not relevant to whether nfs.conf is the
> right place to set nfs4_client_id.
Point.

> 
> 
>> As far as not being container-aware... that might true
>> but it does not mean its not useful to set the id from
>> nfs.conf...
> 
> Yes, it does mean that in that case. It's completely
> broken to use the same nfs4_client_id in every network
> namespace.
How does this break? If all the clients have unique ids
what breaks?

Or are you saying setting the unique id like this:

options nfs nfs4_unique_id="64fd26280451566d648ab0e0b7384421"

in modprobe.d/nfs.conf is not namespace safe?

> 
>> Actual I have customers asking for this type
>> of functionality
> 
> Ask yourself why they might want it. It's probably because
> we don't set it correctly currently. If we have a way to
> automatically get it right every time, there's really no
> need for this setting to be exposed.
If we shouldn't expose it... Lets get rid of it... 
You added the param in the fall 2012... If it hasn't
been used correctly or can't be used correctly for
all theses years... why does it exist?

> 
> I do agree that it's long past time we should be setting
> nfs4_client_id properly. I would rather see a udev script
> developed (you, me, or Alice could do it in an afternoon)
> first. If that doesn't meet the actual customer need, then
> we can revisit.
I'll address this in Trond's reply... 

thanks!

steved.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Chuck Lever III]

> On Apr 17, 2021, at 12:18 PM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> 
> 
> On 4/15/21 12:37 PM, Chuck Lever III wrote:
>> 
>> 
>>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>> 
>>> Hey Chuck! 
>>> 
>>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>>> Hi Steve-
>>>> 
>>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>> 
>>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>> 
>>>> That approach was dropped last July because it is not container-aware.
>>>> It should be simple for someone to write a udev script that uses the
>>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>>> since it is container-aware and makes this setting completely
>>>> automatic (zero touch).
>>> As I said in in my cover letter, I see this more as introduction of
>>> a mechanism more than a way to set the unique id.
>> 
>> Yep, I got that.
>> 
>> I'm not addressing the question of whether adding a
>> mechanism to set a module parameter in nfs.conf is good
>> or not. I'm saying nfs4_client_id is not an appropriate
>> parameter to add to nfs.conf. Can you pick another
>> module parameter as an example for your mechanism?
> The request was to set that parameter...

The cover letter and the quoted e-mail above state that
you are just demonstrating a mechanism to set module
parameters via nfs.conf. I guess that statement was not
entirely accurate, then. Thanks for clarifying.

If there's a bug report somewhere that requests a
feature, it's normal practice to include a URL pointing
to that report in the patch description.


>>> The mechanism being
>>> a way to set kernel module params from nfs.conf. The setting of
>>> the id is just a side effect... 
>>> 
>>> Why spread out the NFS configuration?  Why not
>>> just keep it in one place... aka nfs.conf?
>> 
>> We need to understand whether a module parameter is not
>> going to work in nfs.conf because that setting needs to
>> be namespace-aware. In this case, this setting does indeed
>> need to be namespace-aware. nfs.conf is not aware of
>> network namespaces.
> You probably can say that for every /etc/*.conf file...  
> not being namespace aware... how can they be...
> 
> In the kernel document you say "Specifying a uniquifier string is 
> not support for NFS clients running in containers."
> 
> Why isn't that enough?

Because that statement is noting a limitation which is a
bug that has to be addressed to support NFSv4 properly in
containers.


>>> As far as not being container-aware... that might true
>>> but it does not mean its not useful to set the id from
>>> nfs.conf...
>> 
>> Yes, it does mean that in that case. It's completely
>> broken to use the same nfs4_client_id in every network
>> namespace.
> How does this break? If all the clients have unique ids
> what breaks?
> 
> Or are you saying setting the unique id like this:
> 
> options nfs nfs4_unique_id="64fd26280451566d648ab0e0b7384421"
> 
> in modprobe.d/nfs.conf is not namespace safe?

Setting the client_id via module parameter is not
namespace-aware. That's the bug that the sysfs/udev
contraption is designed to fix.


>>> Actual I have customers asking for this type
>>> of functionality
>> 
>> Ask yourself why they might want it. It's probably because
>> we don't set it correctly currently. If we have a way to
>> automatically get it right every time, there's really no
>> need for this setting to be exposed.
> If we shouldn't expose it... Lets get rid of it...
> You added the param in the fall 2012... If it hasn't
> been used correctly or can't be used correctly for
> all theses years... why does it exist?

Because back then we didn't care about container awareness
enough to make it an initial part of the feature. We were
trying to address the problem of how to ensure that the
nfs4_client_id is unique. But clearly it was only half a
solution.

The module parameter still exists because the general rule
about these things is that module parameters are part of the
kernel API, and can't just be removed. If there's a process
for removing it, then I would agree to that now that we have
a sysfs API to manage the nfs4_client_id setting.


>> I do agree that it's long past time we should be setting
>> nfs4_client_id properly. I would rather see a udev script
>> developed (you, me, or Alice could do it in an afternoon)
>> first. If that doesn't meet the actual customer need, then
>> we can revisit.
> I'll address this in Trond's reply... 
> 
> thanks!
> 
> steved.

--
Chuck Lever

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/17/21 12:36 PM, Chuck Lever III wrote:
> 
> 
>> On Apr 17, 2021, at 12:18 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>
>>
>>
>> On 4/15/21 12:37 PM, Chuck Lever III wrote:
>>>
>>>
>>>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>
>>>> Hey Chuck! 
>>>>
>>>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>>>> Hi Steve-
>>>>>
>>>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>
>>>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>>>
>>>>> That approach was dropped last July because it is not container-aware.
>>>>> It should be simple for someone to write a udev script that uses the
>>>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>>>> since it is container-aware and makes this setting completely
>>>>> automatic (zero touch).
>>>> As I said in in my cover letter, I see this more as introduction of
>>>> a mechanism more than a way to set the unique id.
>>>
>>> Yep, I got that.
>>>
>>> I'm not addressing the question of whether adding a
>>> mechanism to set a module parameter in nfs.conf is good
>>> or not. I'm saying nfs4_client_id is not an appropriate
>>> parameter to add to nfs.conf. Can you pick another
>>> module parameter as an example for your mechanism?
>> The request was to set that parameter...
> 
> The cover letter and the quoted e-mail above state that
> you are just demonstrating a mechanism to set module
> parameters via nfs.conf. I guess that statement was not
> entirely accurate, then. Thanks for clarifying.
I was trying to keep the conversation off of what
was being set to how it was being set... 

I had no idea the entire "options nfs" API is compromised
when it comes to containers... Not sure why... but I will
believe you... But is the API  compromised in a 
non-container env? Other than the cloud, isn't non-containers
envs the majority of our install based? 

> 
> If there's a bug report somewhere that requests a
> feature, it's normal practice to include a URL pointing
> to that report in the patch description.
I just assumed, since it had a customer case, the bz was 
private... it turns out not to be the case
https://bugzilla.redhat.com/show_bug.cgi?id=1801326

> 
> 
>>>> The mechanism being
>>>> a way to set kernel module params from nfs.conf. The setting of
>>>> the id is just a side effect... 
>>>>
>>>> Why spread out the NFS configuration?  Why not
>>>> just keep it in one place... aka nfs.conf?
>>>
>>> We need to understand whether a module parameter is not
>>> going to work in nfs.conf because that setting needs to
>>> be namespace-aware. In this case, this setting does indeed
>>> need to be namespace-aware. nfs.conf is not aware of
>>> network namespaces.
>> You probably can say that for every /etc/*.conf file...  
>> not being namespace aware... how can they be...
>>
>> In the kernel document you say "Specifying a uniquifier string is 
>> not support for NFS clients running in containers."
>>
>> Why isn't that enough?
> 
> Because that statement is noting a limitation which is a
> bug that has to be addressed to support NFSv4 properly in
> containers.
It seems to me we could use similar documentation when 
setting the id from nfs.conf.
 
> 
> 
>>>> As far as not being container-aware... that might true
>>>> but it does not mean its not useful to set the id from
>>>> nfs.conf...
>>>
>>> Yes, it does mean that in that case. It's completely
>>> broken to use the same nfs4_client_id in every network
>>> namespace.
>> How does this break? If all the clients have unique ids
>> what breaks?
>>
>> Or are you saying setting the unique id like this:
>>
>> options nfs nfs4_unique_id="64fd26280451566d648ab0e0b7384421"
>>
>> in modprobe.d/nfs.conf is not namespace safe?
> 
> Setting the client_id via module parameter is not
> namespace-aware. That's the bug that the sysfs/udev
> contraption is designed to fix.
Fair enough... So we know the sysfs/udev is
actually setting the id? 

> 
> 
>>>> Actual I have customers asking for this type
>>>> of functionality
>>>
>>> Ask yourself why they might want it. It's probably because
>>> we don't set it correctly currently. If we have a way to
>>> automatically get it right every time, there's really no
>>> need for this setting to be exposed.
>> If we shouldn't expose it... Lets get rid of it...
>> You added the param in the fall 2012... If it hasn't
>> been used correctly or can't be used correctly for
>> all theses years... why does it exist?
> 
> Because back then we didn't care about container awareness
> enough to make it an initial part of the feature. We were
> trying to address the problem of how to ensure that the
> nfs4_client_id is unique. But clearly it was only half a
> solution.
Right... I was just trying to build a mechanism to
set the value, and not worrying (yet) about what the
value is set to... 

So at this point, all of the nfs kernel module parameter
can be set through the sysfs/udev interface? 

If that is the cause... have the variables in 
nfs.conf create sysfs/udev file would work better?

steved.

> 
> The module parameter still exists because the general rule
> about these things is that module parameters are part of the
> kernel API, and can't just be removed. If there's a process
> for removing it, then I would agree to that now that we have
> a sysfs API to manage the nfs4_client_id setting.
> 
> 
>>> I do agree that it's long past time we should be setting
>>> nfs4_client_id properly. I would rather see a udev script
>>> developed (you, me, or Alice could do it in an afternoon)
>>> first. If that doesn't meet the actual customer need, then
>>> we can revisit.
>> I'll address this in Trond's reply... 
>>
>> thanks!
>>
>> steved.
> 
> --
> Chuck Lever
> 
> 
> 

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Chuck Lever III]

> On Apr 17, 2021, at 1:50 PM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> 
> 
> On 4/17/21 12:36 PM, Chuck Lever III wrote:
>> 
>> 
>>> On Apr 17, 2021, at 12:18 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>> 
>>> 
>>> 
>>> On 4/15/21 12:37 PM, Chuck Lever III wrote:
>>>> 
>>>> 
>>>>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>> 
>>>>> Hey Chuck! 
>>>>> 
>>>>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>>>>> Hi Steve-
>>>>>> 
>>>>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>> 
>>>>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>>>> 
>>>>>> That approach was dropped last July because it is not container-aware.
>>>>>> It should be simple for someone to write a udev script that uses the
>>>>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>>>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>>>>> since it is container-aware and makes this setting completely
>>>>>> automatic (zero touch).
>>>>> As I said in in my cover letter, I see this more as introduction of
>>>>> a mechanism more than a way to set the unique id.
>>>> 
>>>> Yep, I got that.
>>>> 
>>>> I'm not addressing the question of whether adding a
>>>> mechanism to set a module parameter in nfs.conf is good
>>>> or not. I'm saying nfs4_client_id is not an appropriate
>>>> parameter to add to nfs.conf. Can you pick another
>>>> module parameter as an example for your mechanism?
>>> The request was to set that parameter...
>> 
>> The cover letter and the quoted e-mail above state that
>> you are just demonstrating a mechanism to set module
>> parameters via nfs.conf. I guess that statement was not
>> entirely accurate, then. Thanks for clarifying.
> I was trying to keep the conversation off of what
> was being set to how it was being set... 
> 
> I had no idea the entire "options nfs" API is compromised
> when it comes to containers... Not sure why... but I will
> believe you...

Module parameters take effect for all namespaces. It's
not just nfs.ko that has this issue.


>> If there's a bug report somewhere that requests a
>> feature, it's normal practice to include a URL pointing
>> to that report in the patch description.
> I just assumed, since it had a customer case, the bz was 
> private... it turns out not to be the case
> https://bugzilla.redhat.com/show_bug.cgi?id=1801326

>>>>> Actual I have customers asking for this type
>>>>> of functionality

Hrm. The reporter of 1801326 is dwyso, a Red Hat employee,
not a customer.

Also, I see nothing that requires it to be set in nfs.conf.
So what actual functionality is being requested, and why
can't it be addressed with a udev script, which has been
the design already in the works for many months?


>>>> Ask yourself why they might want it. It's probably because
>>>> we don't set it correctly currently. If we have a way to
>>>> automatically get it right every time, there's really no
>>>> need for this setting to be exposed.
>>> If we shouldn't expose it... Lets get rid of it...
>>> You added the param in the fall 2012... If it hasn't
>>> been used correctly or can't be used correctly for
>>> all theses years... why does it exist?
>> 
>> Because back then we didn't care about container awareness
>> enough to make it an initial part of the feature. We were
>> trying to address the problem of how to ensure that the
>> nfs4_client_id is unique. But clearly it was only half a
>> solution.
> Right... I was just trying to build a mechanism to
> set the value, and not worrying (yet) about what the
> value is set to... 
> 
> So at this point, all of the nfs kernel module parameter
> can be set through the sysfs/udev interface?

The only module parameter that has been explicitly replaced
is the one that sets nfs4_client_id, as far as I am aware.
There might be some other settings available in /sys:

# ls /sys/module/nfsv4/parameters
delegation_watermark  layoutstats_timer
#

[cel@manet linux]$ git grep MODULE_PARM -- fs/nfs/
fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent, "Path to the client cache upcall program");
fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent_timeout, "Timeout (in seconds) after which "
fs/nfs/dir.c:MODULE_PARM_DESC(nfs_access_max_cachesize, "NFS access maximum total cache length");
fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
fs/nfs/flexfilelayout/flexfilelayout.c:MODULE_PARM_DESC(io_maxretrans, "The  number of times the NFSv4.1 client "
fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
fs/nfs/namespace.c:MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout,
fs/nfs/super.c:MODULE_PARM_DESC(callback_nr_threads, "Number of threads that will be "
fs/nfs/super.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
fs/nfs/super.c:MODULE_PARM_DESC(max_session_slots, "Maximum number of outstanding NFSv4.1 "
fs/nfs/super.c:MODULE_PARM_DESC(max_session_cb_slots, "Maximum number of parallel NFSv4.1 "
fs/nfs/super.c:MODULE_PARM_DESC(send_implementation_id,
fs/nfs/super.c:MODULE_PARM_DESC(nfs4_unique_id, "nfs_client_id4 uniquifier string");
fs/nfs/super.c:MODULE_PARM_DESC(recover_lost_locks,
[cel@manet linux]$ git grep MODULE_PARM -- fs/nfsd/
fs/nfsd/nfs4idmap.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
fs/nfsd/nfs4proc.c:MODULE_PARM_DESC(inter_copy_offload_enable,
fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_legacy_disable,
[cel@manet linux]$

Each of the above has to be considered on a case-by-case
basis, IMO.


> If that is the cause... have the variables in 
> nfs.conf create sysfs/udev file would work better?

Seems to me we have the same argument for a separate file
to store the nfs4_unique_id that we have for breaking
/etc/exports into a directory of individual files: no
parsing is necessary. Scripts and applications can simply
read the string right out of the file, or update it just
by writing the string into that file.

Conversely, there's no clear need for administrators to be
aware of the setting, once it is being set properly.


--
Chuck Lever

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/18/21 12:51 PM, Chuck Lever III wrote:
> 
> 
>> On Apr 17, 2021, at 1:50 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>
>>
>>
>> On 4/17/21 12:36 PM, Chuck Lever III wrote:
>>>
>>>
>>>> On Apr 17, 2021, at 12:18 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>
>>>>
>>>>
>>>> On 4/15/21 12:37 PM, Chuck Lever III wrote:
>>>>>
>>>>>
>>>>>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>>>
>>>>>> Hey Chuck! 
>>>>>>
>>>>>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>>>>>> Hi Steve-
>>>>>>>
>>>>>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>>>
>>>>>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>>>>>
>>>>>>> That approach was dropped last July because it is not container-aware.
>>>>>>> It should be simple for someone to write a udev script that uses the
>>>>>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>>>>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>>>>>> since it is container-aware and makes this setting completely
>>>>>>> automatic (zero touch).
>>>>>> As I said in in my cover letter, I see this more as introduction of
>>>>>> a mechanism more than a way to set the unique id.
>>>>>
>>>>> Yep, I got that.
>>>>>
>>>>> I'm not addressing the question of whether adding a
>>>>> mechanism to set a module parameter in nfs.conf is good
>>>>> or not. I'm saying nfs4_client_id is not an appropriate
>>>>> parameter to add to nfs.conf. Can you pick another
>>>>> module parameter as an example for your mechanism?
>>>> The request was to set that parameter...
>>>
>>> The cover letter and the quoted e-mail above state that
>>> you are just demonstrating a mechanism to set module
>>> parameters via nfs.conf. I guess that statement was not
>>> entirely accurate, then. Thanks for clarifying.
>> I was trying to keep the conversation off of what
>> was being set to how it was being set... 
>>
>> I had no idea the entire "options nfs" API is compromised
>> when it comes to containers... Not sure why... but I will
>> believe you...
> 
> Module parameters take effect for all namespaces. It's
> not just nfs.ko that has this issue.
Right... 
> 
> 
>>> If there's a bug report somewhere that requests a
>>> feature, it's normal practice to include a URL pointing
>>> to that report in the patch description.
>> I just assumed, since it had a customer case, the bz was 
>> private... it turns out not to be the case
>> https://bugzilla.redhat.com/show_bug.cgi?id=1801326
> 
>>>>>> Actual I have customers asking for this type
>>>>>> of functionality
> 
> Hrm. The reporter of 1801326 is dwyso, a Red Hat employee,
> not a customer.
> 
> Also, I see nothing that requires it to be set in nfs.conf.
> So what actual functionality is being requested, and why
> can't it be addressed with a udev script, which has been
> the design already in the works for many months?
The bz was open by a request of a customer... There is a
lot of info in bzs that are not public... 

Having a one, centralized place to configure NFS
I thought was a good idea... 

> 
> 
>>>>> Ask yourself why they might want it. It's probably because
>>>>> we don't set it correctly currently. If we have a way to
>>>>> automatically get it right every time, there's really no
>>>>> need for this setting to be exposed.
>>>> If we shouldn't expose it... Lets get rid of it...
>>>> You added the param in the fall 2012... If it hasn't
>>>> been used correctly or can't be used correctly for
>>>> all theses years... why does it exist?
>>>
>>> Because back then we didn't care about container awareness
>>> enough to make it an initial part of the feature. We were
>>> trying to address the problem of how to ensure that the
>>> nfs4_client_id is unique. But clearly it was only half a
>>> solution.
>> Right... I was just trying to build a mechanism to
>> set the value, and not worrying (yet) about what the
>> value is set to... 
>>
>> So at this point, all of the nfs kernel module parameter
>> can be set through the sysfs/udev interface?
> 
> The only module parameter that has been explicitly replaced
> is the one that sets nfs4_client_id, as far as I am aware.
> There might be some other settings available in /sys:
> 
> # ls /sys/module/nfsv4/parameters
> delegation_watermark  layoutstats_timer
> #
> 
> [cel@manet linux]$ git grep MODULE_PARM -- fs/nfs/
> fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent, "Path to the client cache upcall program");
> fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent_timeout, "Timeout (in seconds) after which "
> fs/nfs/dir.c:MODULE_PARM_DESC(nfs_access_max_cachesize, "NFS access maximum total cache length");
> fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
> fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
> fs/nfs/flexfilelayout/flexfilelayout.c:MODULE_PARM_DESC(io_maxretrans, "The  number of times the NFSv4.1 client "
> fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
> fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
> fs/nfs/namespace.c:MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout,
> fs/nfs/super.c:MODULE_PARM_DESC(callback_nr_threads, "Number of threads that will be "
> fs/nfs/super.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
> fs/nfs/super.c:MODULE_PARM_DESC(max_session_slots, "Maximum number of outstanding NFSv4.1 "
> fs/nfs/super.c:MODULE_PARM_DESC(max_session_cb_slots, "Maximum number of parallel NFSv4.1 "
> fs/nfs/super.c:MODULE_PARM_DESC(send_implementation_id,
> fs/nfs/super.c:MODULE_PARM_DESC(nfs4_unique_id, "nfs_client_id4 uniquifier string");
> fs/nfs/super.c:MODULE_PARM_DESC(recover_lost_locks,
> [cel@manet linux]$ git grep MODULE_PARM -- fs/nfsd/
> fs/nfsd/nfs4idmap.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
> fs/nfsd/nfs4proc.c:MODULE_PARM_DESC(inter_copy_offload_enable,
> fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
> fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_legacy_disable,
> [cel@manet linux]$
> 
> Each of the above has to be considered on a case-by-case
> basis, IMO.
> 
> 
>> If that is the cause... have the variables in 
>> nfs.conf create sysfs/udev file would work better?
> 
> Seems to me we have the same argument for a separate file
> to store the nfs4_unique_id that we have for breaking
> /etc/exports into a directory of individual files: no
> parsing is necessary. Scripts and applications can simply
> read the string right out of the file, or update it just
> by writing the string into that file.
I'm sure I'm following this analogy with the export...
but being able to set things up from one configuration 
file and command is key. 

nfsconf does an excellent job parsing nfs.conf and I would
like to build on that... 

I understand we have to work well in containers which 
is one of the reason I was trying to come up with a
nfsv4 only nfs-utils... That went over like a lead balloon ;-) 

What I don't understand is why we can't come up with a 
solution that uniquely set a param that is set by 
nfsconf using nfs.conf.

steved.  
> 
> Conversely, there's no clear need for administrators to be
> aware of the setting, once it is being set properly.
> 
> 
> --
> Chuck Lever
> 
> 
> 

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Chuck Lever III]

> On Apr 20, 2021, at 9:11 AM, Steve Dickson <SteveD@RedHat.com> wrote:
> 
> 
> 
> On 4/18/21 12:51 PM, Chuck Lever III wrote:
>> 
>> 
>>> On Apr 17, 2021, at 1:50 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>> 
>>> 
>>> 
>>> On 4/17/21 12:36 PM, Chuck Lever III wrote:
>>>> 
>>>> 
>>>>> On Apr 17, 2021, at 12:18 PM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 4/15/21 12:37 PM, Chuck Lever III wrote:
>>>>>> 
>>>>>> 
>>>>>>> On Apr 15, 2021, at 11:33 AM, Steve Dickson <SteveD@RedHat.com> wrote:
>>>>>>> 
>>>>>>> Hey Chuck! 
>>>>>>> 
>>>>>>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>>>>>>> Hi Steve-
>>>>>>>> 
>>>>>>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>>>>>> 
>>>>>>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>>>>>> 
>>>>>>>> That approach was dropped last July because it is not container-aware.
>>>>>>>> It should be simple for someone to write a udev script that uses the
>>>>>>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>>>>>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>>>>>>> since it is container-aware and makes this setting completely
>>>>>>>> automatic (zero touch).
>>>>>>> As I said in in my cover letter, I see this more as introduction of
>>>>>>> a mechanism more than a way to set the unique id.
>>>>>> 
>>>>>> Yep, I got that.
>>>>>> 
>>>>>> I'm not addressing the question of whether adding a
>>>>>> mechanism to set a module parameter in nfs.conf is good
>>>>>> or not. I'm saying nfs4_client_id is not an appropriate
>>>>>> parameter to add to nfs.conf. Can you pick another
>>>>>> module parameter as an example for your mechanism?
>>>>> The request was to set that parameter...
>>>> 
>>>> The cover letter and the quoted e-mail above state that
>>>> you are just demonstrating a mechanism to set module
>>>> parameters via nfs.conf. I guess that statement was not
>>>> entirely accurate, then. Thanks for clarifying.
>>> I was trying to keep the conversation off of what
>>> was being set to how it was being set... 
>>> 
>>> I had no idea the entire "options nfs" API is compromised
>>> when it comes to containers... Not sure why... but I will
>>> believe you...
>> 
>> Module parameters take effect for all namespaces. It's
>> not just nfs.ko that has this issue.
> Right... 
>> 
>> 
>>>> If there's a bug report somewhere that requests a
>>>> feature, it's normal practice to include a URL pointing
>>>> to that report in the patch description.
>>> I just assumed, since it had a customer case, the bz was 
>>> private... it turns out not to be the case
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1801326
>> 
>>>>>>> Actual I have customers asking for this type
>>>>>>> of functionality
>> 
>> Hrm. The reporter of 1801326 is dwyso, a Red Hat employee,
>> not a customer.
>> 
>> Also, I see nothing that requires it to be set in nfs.conf.
>> So what actual functionality is being requested, and why
>> can't it be addressed with a udev script, which has been
>> the design already in the works for many months?
> The bz was open by a request of a customer... There is a
> lot of info in bzs that are not public... 
> 
> Having a one, centralized place to configure NFS
> I thought was a good idea... 
> 
>> 
>> 
>>>>>> Ask yourself why they might want it. It's probably because
>>>>>> we don't set it correctly currently. If we have a way to
>>>>>> automatically get it right every time, there's really no
>>>>>> need for this setting to be exposed.
>>>>> If we shouldn't expose it... Lets get rid of it...
>>>>> You added the param in the fall 2012... If it hasn't
>>>>> been used correctly or can't be used correctly for
>>>>> all theses years... why does it exist?
>>>> 
>>>> Because back then we didn't care about container awareness
>>>> enough to make it an initial part of the feature. We were
>>>> trying to address the problem of how to ensure that the
>>>> nfs4_client_id is unique. But clearly it was only half a
>>>> solution.
>>> Right... I was just trying to build a mechanism to
>>> set the value, and not worrying (yet) about what the
>>> value is set to... 
>>> 
>>> So at this point, all of the nfs kernel module parameter
>>> can be set through the sysfs/udev interface?
>> 
>> The only module parameter that has been explicitly replaced
>> is the one that sets nfs4_client_id, as far as I am aware.
>> There might be some other settings available in /sys:
>> 
>> # ls /sys/module/nfsv4/parameters
>> delegation_watermark  layoutstats_timer
>> #
>> 
>> [cel@manet linux]$ git grep MODULE_PARM -- fs/nfs/
>> fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent, "Path to the client cache upcall program");
>> fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent_timeout, "Timeout (in seconds) after which "
>> fs/nfs/dir.c:MODULE_PARM_DESC(nfs_access_max_cachesize, "NFS access maximum total cache length");
>> fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
>> fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
>> fs/nfs/flexfilelayout/flexfilelayout.c:MODULE_PARM_DESC(io_maxretrans, "The  number of times the NFSv4.1 client "
>> fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_retrans, "The  number of times the NFSv4.1 client "
>> fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(dataserver_timeo, "The time (in tenths of a second) the "
>> fs/nfs/namespace.c:MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout,
>> fs/nfs/super.c:MODULE_PARM_DESC(callback_nr_threads, "Number of threads that will be "
>> fs/nfs/super.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
>> fs/nfs/super.c:MODULE_PARM_DESC(max_session_slots, "Maximum number of outstanding NFSv4.1 "
>> fs/nfs/super.c:MODULE_PARM_DESC(max_session_cb_slots, "Maximum number of parallel NFSv4.1 "
>> fs/nfs/super.c:MODULE_PARM_DESC(send_implementation_id,
>> fs/nfs/super.c:MODULE_PARM_DESC(nfs4_unique_id, "nfs_client_id4 uniquifier string");
>> fs/nfs/super.c:MODULE_PARM_DESC(recover_lost_locks,
>> [cel@manet linux]$ git grep MODULE_PARM -- fs/nfsd/
>> fs/nfsd/nfs4idmap.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
>> fs/nfsd/nfs4proc.c:MODULE_PARM_DESC(inter_copy_offload_enable,
>> fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_prog, "Path to the nfsdcltrack upcall program");
>> fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_legacy_disable,
>> [cel@manet linux]$
>> 
>> Each of the above has to be considered on a case-by-case
>> basis, IMO.
>> 
>> 
>>> If that is the cause... have the variables in 
>>> nfs.conf create sysfs/udev file would work better?
>> 
>> Seems to me we have the same argument for a separate file
>> to store the nfs4_unique_id that we have for breaking
>> /etc/exports into a directory of individual files: no
>> parsing is necessary. Scripts and applications can simply
>> read the string right out of the file, or update it just
>> by writing the string into that file.
> I'm sure I'm following this analogy with the export...
> but being able to set things up from one configuration 
> file and command is key.

I find that to be a red herring. We're not ever going to be
at a place where everything is configured in one file. Are
you going to replace /etc/exports.d and automounter.conf
and krb5.conf and all the other things with just /etc/nfs.conf?
Probably not. So let's stop using this strange logic to
insist that /etc/nfs.conf is the only place for the clientid
uniqifier.

Please, let's just focus on what's right for this one setting.


> nfsconf does an excellent job parsing nfs.conf and I would
> like to build on that... 

Just because it is technically possible to save the uniqifier
in /etc/nfs.conf doesn't mean that's what's best for our users.
We're in better shape if we can guarantee that setting is
correct and administrators can't break it.


> I understand we have to work well in containers which 
> is one of the reason I was trying to come up with a
> nfsv4 only nfs-utils... That went over like a lead balloon ;-)

I didn't have a problem with an nfsv4-only configuration.
What I didn't like about that is that you were making the
administrative interface more complex, and it didn't need to
be.


> What I don't understand is why we can't come up with a 
> solution that uniquely set a param that is set by 
> nfsconf using nfs.conf.

Once we have an automated mechanism to set the uniqifier,
it does not need to be set by humans. Let's keep it out of
nfs.conf.

I'm in favor of making this as automatic as possible. No
setting is better than an exposed setting that is never
touched.

I prefer no change to nfs.conf, and put the uniqifier in a
separate file.


--
Chuck Lever

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Tue, 2021-04-20 at 14:09 +0000, Chuck Lever III wrote:
> 
> 
> > On Apr 20, 2021, at 9:11 AM, Steve Dickson <SteveD@RedHat.com>
> > wrote:
> > 
> > 
> > 
> > On 4/18/21 12:51 PM, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Apr 17, 2021, at 1:50 PM, Steve Dickson <SteveD@RedHat.com>
> > > > wrote:
> > > > 
> > > > 
> > > > 
> > > > On 4/17/21 12:36 PM, Chuck Lever III wrote:
> > > > > 
> > > > > 
> > > > > > On Apr 17, 2021, at 12:18 PM, Steve Dickson < 
> > > > > > SteveD@RedHat.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 4/15/21 12:37 PM, Chuck Lever III wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > > On Apr 15, 2021, at 11:33 AM, Steve Dickson < 
> > > > > > > > SteveD@RedHat.com> wrote:
> > > > > > > > 
> > > > > > > > Hey Chuck! 
> > > > > > > > 
> > > > > > > > On 4/14/21 7:26 PM, Chuck Lever III wrote:
> > > > > > > > > Hi Steve-
> > > > > > > > > 
> > > > > > > > > > On Apr 14, 2021, at 2:10 PM, Steve Dickson < 
> > > > > > > > > > SteveD@redhat.com> wrote:
> > > > > > > > > > 
> > > > > > > > > > This is a tweak of the patch set Alice Mitchell
> > > > > > > > > > posted last July [1].
> > > > > > > > > 
> > > > > > > > > That approach was dropped last July because it is not
> > > > > > > > > container-aware.
> > > > > > > > > It should be simple for someone to write a udev
> > > > > > > > > script that uses the
> > > > > > > > > existing sysfs API that can update nfs4_client_id in
> > > > > > > > > a namespace. I
> > > > > > > > > would prefer the sysfs/udev approach for setting
> > > > > > > > > nfs4_client_id,
> > > > > > > > > since it is container-aware and makes this setting
> > > > > > > > > completely
> > > > > > > > > automatic (zero touch).
> > > > > > > > As I said in in my cover letter, I see this more as
> > > > > > > > introduction of
> > > > > > > > a mechanism more than a way to set the unique id.
> > > > > > > 
> > > > > > > Yep, I got that.
> > > > > > > 
> > > > > > > I'm not addressing the question of whether adding a
> > > > > > > mechanism to set a module parameter in nfs.conf is good
> > > > > > > or not. I'm saying nfs4_client_id is not an appropriate
> > > > > > > parameter to add to nfs.conf. Can you pick another
> > > > > > > module parameter as an example for your mechanism?
> > > > > > The request was to set that parameter...
> > > > > 
> > > > > The cover letter and the quoted e-mail above state that
> > > > > you are just demonstrating a mechanism to set module
> > > > > parameters via nfs.conf. I guess that statement was not
> > > > > entirely accurate, then. Thanks for clarifying.
> > > > I was trying to keep the conversation off of what
> > > > was being set to how it was being set... 
> > > > 
> > > > I had no idea the entire "options nfs" API is compromised
> > > > when it comes to containers... Not sure why... but I will
> > > > believe you...
> > > 
> > > Module parameters take effect for all namespaces. It's
> > > not just nfs.ko that has this issue.
> > Right... 
> > > 
> > > 
> > > > > If there's a bug report somewhere that requests a
> > > > > feature, it's normal practice to include a URL pointing
> > > > > to that report in the patch description.
> > > > I just assumed, since it had a customer case, the bz was 
> > > > private... it turns out not to be the case
> > > > https://bugzilla.redhat.com/show_bug.cgi?id=1801326
> > > 
> > > > > > > > Actual I have customers asking for this type
> > > > > > > > of functionality
> > > 
> > > Hrm. The reporter of 1801326 is dwyso, a Red Hat employee,
> > > not a customer.
> > > 
> > > Also, I see nothing that requires it to be set in nfs.conf.
> > > So what actual functionality is being requested, and why
> > > can't it be addressed with a udev script, which has been
> > > the design already in the works for many months?
> > The bz was open by a request of a customer... There is a
> > lot of info in bzs that are not public... 
> > 
> > Having a one, centralized place to configure NFS
> > I thought was a good idea... 
> > 
> > > 
> > > 
> > > > > > > Ask yourself why they might want it. It's probably
> > > > > > > because
> > > > > > > we don't set it correctly currently. If we have a way to
> > > > > > > automatically get it right every time, there's really no
> > > > > > > need for this setting to be exposed.
> > > > > > If we shouldn't expose it... Lets get rid of it...
> > > > > > You added the param in the fall 2012... If it hasn't
> > > > > > been used correctly or can't be used correctly for
> > > > > > all theses years... why does it exist?
> > > > > 
> > > > > Because back then we didn't care about container awareness
> > > > > enough to make it an initial part of the feature. We were
> > > > > trying to address the problem of how to ensure that the
> > > > > nfs4_client_id is unique. But clearly it was only half a
> > > > > solution.
> > > > Right... I was just trying to build a mechanism to
> > > > set the value, and not worrying (yet) about what the
> > > > value is set to... 
> > > > 
> > > > So at this point, all of the nfs kernel module parameter
> > > > can be set through the sysfs/udev interface?
> > > 
> > > The only module parameter that has been explicitly replaced
> > > is the one that sets nfs4_client_id, as far as I am aware.
> > > There might be some other settings available in /sys:
> > > 
> > > # ls /sys/module/nfsv4/parameters
> > > delegation_watermark  layoutstats_timer
> > > #
> > > 
> > > [cel@manet linux]$ git grep MODULE_PARM -- fs/nfs/
> > > fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent, "Path to the
> > > client cache upcall program");
> > > fs/nfs/cache_lib.c:MODULE_PARM_DESC(cache_getent_timeout,
> > > "Timeout (in seconds) after which "
> > > fs/nfs/dir.c:MODULE_PARM_DESC(nfs_access_max_cachesize, "NFS
> > > access maximum total cache length");
> > > fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_ret
> > > rans, "The  number of times the NFSv4.1 client "
> > > fs/nfs/filelayout/filelayoutdev.c:MODULE_PARM_DESC(dataserver_tim
> > > eo, "The time (in tenths of a second) the "
> > > fs/nfs/flexfilelayout/flexfilelayout.c:MODULE_PARM_DESC(io_maxret
> > > rans, "The  number of times the NFSv4.1 client "
> > > fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(datase
> > > rver_retrans, "The  number of times the NFSv4.1 client "
> > > fs/nfs/flexfilelayout/flexfilelayoutdev.c:MODULE_PARM_DESC(datase
> > > rver_timeo, "The time (in tenths of a second) the "
> > > fs/nfs/namespace.c:MODULE_PARM_DESC(nfs_mountpoint_expiry_timeout
> > > ,
> > > fs/nfs/super.c:MODULE_PARM_DESC(callback_nr_threads, "Number of
> > > threads that will be "
> > > fs/nfs/super.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
> > > fs/nfs/super.c:MODULE_PARM_DESC(max_session_slots, "Maximum
> > > number of outstanding NFSv4.1 "
> > > fs/nfs/super.c:MODULE_PARM_DESC(max_session_cb_slots, "Maximum
> > > number of parallel NFSv4.1 "
> > > fs/nfs/super.c:MODULE_PARM_DESC(send_implementation_id,
> > > fs/nfs/super.c:MODULE_PARM_DESC(nfs4_unique_id, "nfs_client_id4
> > > uniquifier string");
> > > fs/nfs/super.c:MODULE_PARM_DESC(recover_lost_locks,
> > > [cel@manet linux]$ git grep MODULE_PARM -- fs/nfsd/
> > > fs/nfsd/nfs4idmap.c:MODULE_PARM_DESC(nfs4_disable_idmapping,
> > > fs/nfsd/nfs4proc.c:MODULE_PARM_DESC(inter_copy_offload_enable,
> > > fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_prog, "Path to the
> > > nfsdcltrack upcall program");
> > > fs/nfsd/nfs4recover.c:MODULE_PARM_DESC(cltrack_legacy_disable,
> > > [cel@manet linux]$
> > > 
> > > Each of the above has to be considered on a case-by-case
> > > basis, IMO.
> > > 
> > > 
> > > > If that is the cause... have the variables in 
> > > > nfs.conf create sysfs/udev file would work better?
> > > 
> > > Seems to me we have the same argument for a separate file
> > > to store the nfs4_unique_id that we have for breaking
> > > /etc/exports into a directory of individual files: no
> > > parsing is necessary. Scripts and applications can simply
> > > read the string right out of the file, or update it just
> > > by writing the string into that file.
> > I'm sure I'm following this analogy with the export...
> > but being able to set things up from one configuration 
> > file and command is key.
> 
> I find that to be a red herring. We're not ever going to be
> at a place where everything is configured in one file. Are
> you going to replace /etc/exports.d and automounter.conf
> and krb5.conf and all the other things with just /etc/nfs.conf?
> Probably not. So let's stop using this strange logic to
> insist that /etc/nfs.conf is the only place for the clientid
> uniqifier.
> 
> Please, let's just focus on what's right for this one setting.
> 
> 
> > nfsconf does an excellent job parsing nfs.conf and I would
> > like to build on that... 
> 
> Just because it is technically possible to save the uniqifier
> in /etc/nfs.conf doesn't mean that's what's best for our users.
> We're in better shape if we can guarantee that setting is
> correct and administrators can't break it.
> 
> 
> > I understand we have to work well in containers which 
> > is one of the reason I was trying to come up with a
> > nfsv4 only nfs-utils... That went over like a lead balloon ;-)
> 
> I didn't have a problem with an nfsv4-only configuration.
> What I didn't like about that is that you were making the
> administrative interface more complex, and it didn't need to
> be.
> 
> 
> > What I don't understand is why we can't come up with a 
> > solution that uniquely set a param that is set by 
> > nfsconf using nfs.conf.
> 
> Once we have an automated mechanism to set the uniqifier,
> it does not need to be set by humans. Let's keep it out of
> nfs.conf.
> 
> I'm in favor of making this as automatic as possible. No
> setting is better than an exposed setting that is never
> touched.
> 
> I prefer no change to nfs.conf, and put the uniqifier in a
> separate file.
> 

I think the important thing is, as Chuck said, that the setting of the
uniquifier has to be automated. There are too many instances out there
of people who get confused because they are using a default hostname,
such as 'localhost.localdomain' and are setting no uniquifier.

So the point is that it needs to be persisted by an automated script if
unset.

While that script could use nfsconf to get/set the persisted
uniquifier, the worry is that such an automated change might be made
while the user is performing some other edit of nfs.conf. What happens
then?

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[J. Bruce Fields]

On Tue, Apr 20, 2021 at 02:31:58PM +0000, Trond Myklebust wrote:
> I think the important thing is, as Chuck said, that the setting of the
> uniquifier has to be automated. There are too many instances out there
> of people who get confused because they are using a default hostname,
> such as 'localhost.localdomain' and are setting no uniquifier.
> 
> So the point is that it needs to be persisted by an automated script if
> unset.
> 
> While that script could use nfsconf to get/set the persisted
> uniquifier, the worry is that such an automated change might be made
> while the user is performing some other edit of nfs.conf. What happens
> then?

The one thing I'm a little uneasy about is ignoring /etc/machine-id.
Seems like distros *should* be creating it for us.  And it would be
convenient to have one source of machine identity rather than separate
ones for different subsystems.

Maybe we could use that if it exists, and fall back on generating our
own only if it doesn't?

(Well, where "use it" actually means take a hash of it, as explained in
machine-id(5).)

--b.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Tue, 2021-04-20 at 13:18 -0400, J. Bruce Fields wrote:
> On Tue, Apr 20, 2021 at 02:31:58PM +0000, Trond Myklebust wrote:
> > I think the important thing is, as Chuck said, that the setting of
> > the
> > uniquifier has to be automated. There are too many instances out
> > there
> > of people who get confused because they are using a default
> > hostname,
> > such as 'localhost.localdomain' and are setting no uniquifier.
> > 
> > So the point is that it needs to be persisted by an automated
> > script if
> > unset.
> > 
> > While that script could use nfsconf to get/set the persisted
> > uniquifier, the worry is that such an automated change might be
> > made
> > while the user is performing some other edit of nfs.conf. What
> > happens
> > then?
> 
> The one thing I'm a little uneasy about is ignoring /etc/machine-id.
> Seems like distros *should* be creating it for us.  And it would be
> convenient to have one source of machine identity rather than
> separate
> ones for different subsystems.
> 
> Maybe we could use that if it exists, and fall back on generating our
> own only if it doesn't?
> 
> (Well, where "use it" actually means take a hash of it, as explained
> in
> machine-id(5).)
> 

Maybe, but that ties the nfs-utils package irrevocably to systemd.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[bfields]

On Tue, Apr 20, 2021 at 05:28:08PM +0000, Trond Myklebust wrote:
> On Tue, 2021-04-20 at 13:18 -0400, J. Bruce Fields wrote:
> > On Tue, Apr 20, 2021 at 02:31:58PM +0000, Trond Myklebust wrote:
> > > I think the important thing is, as Chuck said, that the setting of
> > > the
> > > uniquifier has to be automated. There are too many instances out
> > > there
> > > of people who get confused because they are using a default
> > > hostname,
> > > such as 'localhost.localdomain' and are setting no uniquifier.
> > > 
> > > So the point is that it needs to be persisted by an automated
> > > script if
> > > unset.
> > > 
> > > While that script could use nfsconf to get/set the persisted
> > > uniquifier, the worry is that such an automated change might be
> > > made
> > > while the user is performing some other edit of nfs.conf. What
> > > happens
> > > then?
> > 
> > The one thing I'm a little uneasy about is ignoring /etc/machine-id.
> > Seems like distros *should* be creating it for us.  And it would be
> > convenient to have one source of machine identity rather than
> > separate
> > ones for different subsystems.
> > 
> > Maybe we could use that if it exists, and fall back on generating our
> > own only if it doesn't?
> > 
> > (Well, where "use it" actually means take a hash of it, as explained
> > in
> > machine-id(5).)
> > 
> 
> Maybe, but that ties the nfs-utils package irrevocably to systemd.

Well, like I say, we could have a fallback.  Or even provide alternative
scripts in nfs-utils and let the distro decide which to install
depending on whether they use systemd.

But, whatever, those two alternatives (machine-id or vs. nfs generating
its own uuid) are basically the same on some level.

I agree with the basic idea that this should be automated rather than
living in a configuration file that humans might have to deal with.

--b.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Trond Myklebust]

On Tue, 2021-04-20 at 13:40 -0400, bfields@fieldses.org wrote:
> On Tue, Apr 20, 2021 at 05:28:08PM +0000, Trond Myklebust wrote:
> > On Tue, 2021-04-20 at 13:18 -0400, J. Bruce Fields wrote:
> > > On Tue, Apr 20, 2021 at 02:31:58PM +0000, Trond Myklebust wrote:
> > > > I think the important thing is, as Chuck said, that the setting
> > > > of
> > > > the
> > > > uniquifier has to be automated. There are too many instances
> > > > out
> > > > there
> > > > of people who get confused because they are using a default
> > > > hostname,
> > > > such as 'localhost.localdomain' and are setting no uniquifier.
> > > > 
> > > > So the point is that it needs to be persisted by an automated
> > > > script if
> > > > unset.
> > > > 
> > > > While that script could use nfsconf to get/set the persisted
> > > > uniquifier, the worry is that such an automated change might be
> > > > made
> > > > while the user is performing some other edit of nfs.conf. What
> > > > happens
> > > > then?
> > > 
> > > The one thing I'm a little uneasy about is ignoring /etc/machine-
> > > id.
> > > Seems like distros *should* be creating it for us.  And it would
> > > be
> > > convenient to have one source of machine identity rather than
> > > separate
> > > ones for different subsystems.
> > > 
> > > Maybe we could use that if it exists, and fall back on generating
> > > our
> > > own only if it doesn't?
> > > 
> > > (Well, where "use it" actually means take a hash of it, as
> > > explained
> > > in
> > > machine-id(5).)
> > > 
> > 
> > Maybe, but that ties the nfs-utils package irrevocably to systemd.
> 
> Well, like I say, we could have a fallback.  Or even provide
> alternative
> scripts in nfs-utils and let the distro decide which to install
> depending on whether they use systemd.
> 
> But, whatever, those two alternatives (machine-id or vs. nfs
> generating
> its own uuid) are basically the same on some level.

Not quite. They cause the behaviour to differ depending on whether or
not systemd is installed. So if you imagine a system that gets updated
from "traditional init" to systemd, then that could cause the NFS
identity of the machine to change.

It would be better to be able to specify the identity in a form that is
independent of the platform.

So if the machine-id exists, then maybe we could indeed generate the
identity using the uuid in that file (although the question remains as
to why you'd want that?). However the generated value should then be
persisted separately so that it can be platform independent.

> 
> I agree with the basic idea that this should be automated rather than
> living in a configuration file that humans might have to deal with.
> 
> --b.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[bfields]

On Tue, Apr 20, 2021 at 05:53:34PM +0000, Trond Myklebust wrote:
> So if the machine-id exists, then maybe we could indeed generate the
> identity using the uuid in that file (although the question remains as
> to why you'd want that?).

I was assuming: When you clone a machine image, either you want the
clone to have the same identity (maybe it's a backup, or you're doing
some sort of migration) or you want it to act like a new machine (say
you've got a base image that you're using to make a bunch of hosts).  In
the latter case you've got to track down everything on the filesystem
that needs to differ between hosts and fix it up.  The fewer of those,
the better.

> However the generated value should then be persisted separately so
> that it can be platform independent.

That'd be OK.

I don't think switching a host between systemd and not systemd-based is
a common case.

If somebody has a really weird case they can always write their own
script.

--b.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/20/21 2:16 PM, bfields@fieldses.org wrote:
> On Tue, Apr 20, 2021 at 05:53:34PM +0000, Trond Myklebust wrote:
>> So if the machine-id exists, then maybe we could indeed generate the
>> identity using the uuid in that file (although the question remains as
>> to why you'd want that?).
> 
> I was assuming: When you clone a machine image, either you want the
> clone to have the same identity (maybe it's a backup, or you're doing
> some sort of migration) or you want it to act like a new machine (say
> you've got a base image that you're using to make a bunch of hosts).  In
> the latter case you've got to track down everything on the filesystem
> that needs to differ between hosts and fix it up.  The fewer of those,
> the better.
For the record... I cloned a VM and the /etc/machine-id were the same.
The later would have to happen. 

> 
>> However the generated value should then be persisted separately so
>> that it can be platform independent.
> 
> That'd be OK.
> 
> I don't think switching a host between systemd and not systemd-based is
> a common case.
> 
> If somebody has a really weird case they can always write their own
> script.
+1 

steved.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/20/21 10:31 AM, Trond Myklebust wrote:
>>> What I don't understand is why we can't come up with a 
>>> solution that uniquely set a param that is set by 
>>> nfsconf using nfs.conf.
>> Once we have an automated mechanism to set the uniqifier,
>> it does not need to be set by humans. Let's keep it out of
>> nfs.conf.
>>
>> I'm in favor of making this as automatic as possible. No
>> setting is better than an exposed setting that is never
>> touched.
>>
>> I prefer no change to nfs.conf, and put the uniqifier in a
>> separate file.
>>
> I think the important thing is, as Chuck said, that the setting of the
> uniquifier has to be automated. There are too many instances out there
> of people who get confused because they are using a default hostname,
> such as 'localhost.localdomain' and are setting no uniquifier.
The current patches use either /etc/machine-id or hostname 
to generate the uniquifier. Alice's patches also included 
/proc/sys/kernel/random/uuid as as way generate. 

People could have those choices... and we (aka nfs-utils) would be 
doing the generations.

> 
> So the point is that it needs to be persisted by an automated script if
> unset.
Yes this is one thing that is missing... Making sure it is not already set.

> 
> While that script could use nfsconf to get/set the persisted
> uniquifier, the worry is that such an automated change might be made
> while the user is performing some other edit of nfs.conf. What happens
> then?
Cat will start dating Dogs??? IDK! :-) 

So it sound like we need a way to generate an uniquifier
which the patches do (we could add your sysfs one) 
but you don't what that way tied to /etc/nfs.conf.

So that means we will generate the uniquifier one
way and only one way that has to work on all distro 
that happens automatically... If id is not already
set... 

There should be a way for distro to decide who the 
uniquifier is generated?

steved.

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

On 4/20/21 10:09 AM, Chuck Lever III wrote:
>>>> If that is the cause... have the variables in 
>>>> nfs.conf create sysfs/udev file would work better?
>>>
>>> Seems to me we have the same argument for a separate file
>>> to store the nfs4_unique_id that we have for breaking
>>> /etc/exports into a directory of individual files: no
>>> parsing is necessary. Scripts and applications can simply
>>> read the string right out of the file, or update it just
>>> by writing the string into that file.
>> I'm sure I'm following this analogy with the export...
>> but being able to set things up from one configuration 
>> file and command is key.
> 
> I find that to be a red herring. We're not ever going to be
> at a place where everything is configured in one file. Are
> you going to replace /etc/exports.d and automounter.conf
> and krb5.conf and all the other things with just /etc/nfs.conf?
Obviously not... and you for got /etc/idmapd.conf ;-) 
but I have thought about rolling nfsmount.conf into nfs.conf.
 
> Probably not. So let's stop using this strange logic to
> insist that /etc/nfs.conf is the only place for the clientid
> uniqifier.
Maybe this is splitting hairs... but the actual uniqifier does
exist in nfs.conf... Patches introduce a way to generate one
for nfs.conf. 

> 
> Please, let's just focus on what's right for this one setting.
> 
> 
>> nfsconf does an excellent job parsing nfs.conf and I would
>> like to build on that... 
> 
> Just because it is technically possible to save the uniqifier
> in /etc/nfs.conf doesn't mean that's what's best for our users.
> We're in better shape if we can guarantee that setting is
> correct and administrators can't break it.
Again... the id is not saved in nfs.conf... Just a couple
methods to generate one.

> 
> 
>> I understand we have to work well in containers which 
>> is one of the reason I was trying to come up with a
>> nfsv4 only nfs-utils... That went over like a lead balloon ;-)
> 
> I didn't have a problem with an nfsv4-only configuration.
> What I didn't like about that is that you were making the
> administrative interface more complex, and it didn't need to
> be.
I'm sure what you mean... but that is for another day 8-)

> 
> 
>> What I don't understand is why we can't come up with a 
>> solution that uniquely set a param that is set by 
>> nfsconf using nfs.conf.
> 
> Once we have an automated mechanism to set the uniqifier,
> it does not need to be set by humans. Let's keep it out of
> nfs.conf.
> 
> I'm in favor of making this as automatic as possible. No
> setting is better than an exposed setting that is never
> touched.
> 
> I prefer no change to nfs.conf, and put the uniqifier in a
> separate file.
Again the id will end up in a different file... Trond
wants it in the sysfs filesystem verses the /etc/modprob.d/nfs.conf
file... Which is fine... 

To me this is sound more like a distro issue of how the
uniqifier is created and what should be used to create it
when nfs-utils is installed. 

steved.

Re: Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[NeilBrown]

On Fri, 16 Apr 2021, Steve Dickson wrote:
> Hey Chuck! 
> 
> On 4/14/21 7:26 PM, Chuck Lever III wrote:
> > Hi Steve-
> > 
> >> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
> >>
> >> This is a tweak of the patch set Alice Mitchell posted last July [1].
> > 
> > That approach was dropped last July because it is not container-aware.
> > It should be simple for someone to write a udev script that uses the
> > existing sysfs API that can update nfs4_client_id in a namespace. I
> > would prefer the sysfs/udev approach for setting nfs4_client_id,
> > since it is container-aware and makes this setting completely
> > automatic (zero touch).
> As I said in in my cover letter, I see this more as introduction of
> a mechanism more than a way to set the unique id. The mechanism being
> a way to set kernel module params from nfs.conf. The setting of
> the id is just a side effect... 

I wonder if this is the best approach for setting module parameters.

rpc.nfsd already sets grace-time and lease-time - which aren't
exactly module parameters, but are similar - using values from nfs.conf.
Similarly statd sets /proc/fs/nfs/nlm_tcport based on nfs.conf.

I don't think these things should appear in nfs.conf as "kernel
parameters", but as service parameters for the particular service.
How they are communicate to the kernel is an internal implementation
detail.  Maybe it will involve setting module parameters (at least on
older kernels).

For the "identity" setting, I think it would be best if this were
checked and updated by mount.nfs (similar to the way mount.nfs will
check if statd is running, and will start it if necessary).  So should
it go in nfsmount.conf instead of nfs.conf?? I'm not sure.

It isn't clear to me where the identity should come from.
In some circumstances it might make sense to take it from nfs.conf.
In that case we would want to support reading /etc/netnfs/NAME/nfs.conf
where NAME was determined in much the same way that "ip netns identify"
determines a name.  (Compare inum of /proc/self/ns/net with the inum of
each name in /run/netns/).
If we did that, we could then support "$netns" in the conf file, and
allow

 [nfs]
  identity = ${hostname}-${netns}

in /etc/nfs.conf, and it would Do The Right Thing for many cases.

We have a partner who wants to make use of 'nconnect' but is
particularly inconvenienced by the fact that once there is any mount
from a given server it is no longer possible to change the nconnect
setting.  I have suggested they explore setting up a separate
net-namespace for "their" mounts which can be independent from "other"
mounts on the same machine.  If we could make that work with a degree of
transparency - maybe even a "-o netfs=foobar" mount option - that would
be a big help.

Thanks,
NeilBrown

Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[Steve Dickson]

Sorry for the delay... I took some PTO... 

On 5/12/21 8:29 PM, NeilBrown wrote:
> On Fri, 16 Apr 2021, Steve Dickson wrote:
>> Hey Chuck! 
>>
>> On 4/14/21 7:26 PM, Chuck Lever III wrote:
>>> Hi Steve-
>>>
>>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
>>>>
>>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
>>>
>>> That approach was dropped last July because it is not container-aware.
>>> It should be simple for someone to write a udev script that uses the
>>> existing sysfs API that can update nfs4_client_id in a namespace. I
>>> would prefer the sysfs/udev approach for setting nfs4_client_id,
>>> since it is container-aware and makes this setting completely
>>> automatic (zero touch).
>> As I said in in my cover letter, I see this more as introduction of
>> a mechanism more than a way to set the unique id. The mechanism being
>> a way to set kernel module params from nfs.conf. The setting of
>> the id is just a side effect... 
> 
> I wonder if this is the best approach for setting module parameters.
> 
> rpc.nfsd already sets grace-time and lease-time - which aren't
> exactly module parameters, but are similar - using values from nfs.conf.
> Similarly statd sets /proc/fs/nfs/nlm_tcport based on nfs.conf.
> 
> I don't think these things should appear in nfs.conf as "kernel
> parameters", but as service parameters for the particular service.
> How they are communicate to the kernel is an internal implementation
> detail.  Maybe it will involve setting module parameters (at least on
> older kernels).
I think I understand you idea of look at thing as "service parameters"
instead of "kernel parameters", but looking at the actual parameters
that might be a bit difficult. 

Some do map to a service like nfs4_disable_idmapping could be set 
from /etc/idmapd.conf, but things like send_implementation_id or 
delegation_watermark do not really map to a particular service
or am I missing something?

> 
> For the "identity" setting, I think it would be best if this were
> checked and updated by mount.nfs (similar to the way mount.nfs will
> check if statd is running, and will start it if necessary).  So should
> it go in nfsmount.conf instead of nfs.conf?? I'm not sure.
Interesting idea...I would think nfsmount.conf would be the
right place.

> 
> It isn't clear to me where the identity should come from.
> In some circumstances it might make sense to take it from nfs.conf.
> In that case we would want to support reading /etc/netnfs/NAME/nfs.conf
> where NAME was determined in much the same way that "ip netns identify"
> determines a name.  (Compare inum of /proc/self/ns/net with the inum of
> each name in /run/netns/).
I think supporting configs per namespaces is a good idea. I don't
think it would be too difficult to do since we already support
the nfs.d directory. 


> If we did that, we could then support "$netns" in the conf file, and
> allow
> 
>  [nfs]
>   identity = ${hostname}-${netns}
> 
> in /etc/nfs.conf, and it would Do The Right Thing for many cases.
I'm a bit namespace challenged... but as I see it using 
"ip netns identify" (w/out the [PID]) would return all of
the current network network namespaces. Then we would run through 
the /etc/nfs.conf.d/ directory looking for a matching directory
for any of the returned namespaces. If found that config
would be used. Something along those lines? 

With multiple namespaces, how would we know which one to use? 
 
> 
> We have a partner who wants to make use of 'nconnect' but is
> particularly inconvenienced by the fact that once there is any mount
> from a given server it is no longer possible to change the nconnect
> setting.  I have suggested they explore setting up a separate
> net-namespace for "their" mounts which can be independent from "other"
> mounts on the same machine.  If we could make that work with a degree of
> transparency - maybe even a "-o netfs=foobar" mount option - that would
> be a big help.
I think I would like to continue exploring the namespace patch verse 
adding another mount option.

steved.

Re: Re: [PATCH 0/3] Enable the setting of a kernel module parameter from nfs.conf

[NeilBrown]

On Tue, 18 May 2021, Steve Dickson wrote:
> Sorry for the delay... I took some PTO... 
> 
> On 5/12/21 8:29 PM, NeilBrown wrote:
> > On Fri, 16 Apr 2021, Steve Dickson wrote:
> >> Hey Chuck! 
> >>
> >> On 4/14/21 7:26 PM, Chuck Lever III wrote:
> >>> Hi Steve-
> >>>
> >>>> On Apr 14, 2021, at 2:10 PM, Steve Dickson <SteveD@redhat.com> wrote:
> >>>>
> >>>> This is a tweak of the patch set Alice Mitchell posted last July [1].
> >>>
> >>> That approach was dropped last July because it is not container-aware.
> >>> It should be simple for someone to write a udev script that uses the
> >>> existing sysfs API that can update nfs4_client_id in a namespace. I
> >>> would prefer the sysfs/udev approach for setting nfs4_client_id,
> >>> since it is container-aware and makes this setting completely
> >>> automatic (zero touch).
> >> As I said in in my cover letter, I see this more as introduction of
> >> a mechanism more than a way to set the unique id. The mechanism being
> >> a way to set kernel module params from nfs.conf. The setting of
> >> the id is just a side effect... 
> > 
> > I wonder if this is the best approach for setting module parameters.
> > 
> > rpc.nfsd already sets grace-time and lease-time - which aren't
> > exactly module parameters, but are similar - using values from nfs.conf.
> > Similarly statd sets /proc/fs/nfs/nlm_tcport based on nfs.conf.
> > 
> > I don't think these things should appear in nfs.conf as "kernel
> > parameters", but as service parameters for the particular service.
> > How they are communicate to the kernel is an internal implementation
> > detail.  Maybe it will involve setting module parameters (at least on
> > older kernels).
> I think I understand you idea of look at thing as "service parameters"
> instead of "kernel parameters", but looking at the actual parameters
> that might be a bit difficult. 
> 
> Some do map to a service like nfs4_disable_idmapping could be set 
> from /etc/idmapd.conf, but things like send_implementation_id or 
> delegation_watermark do not really map to a particular service
> or am I missing something?

There are two "nfs4_disable_idmapping" parameters.  One for server, one
for client.
The server one should, I think, be set by rpc.nfsd based on a setting in
the [nfsd] section of nfs.conf.

The client one should (I think) be set by mount.nfs using whatever
config language we decide is appropriate.

> 
> > 
> > For the "identity" setting, I think it would be best if this were
> > checked and updated by mount.nfs (similar to the way mount.nfs will
> > check if statd is running, and will start it if necessary).  So should
> > it go in nfsmount.conf instead of nfs.conf?? I'm not sure.
> Interesting idea...I would think nfsmount.conf would be the
> right place.

Maybe...  nfsmount.conf is currently only for mount options.  These can
all be per-server or per-mountpoint, or global.
It might make sense to have other things in the global section ...
though it is named "NFSMount_Global_Options" which seems to explicitly
suggest that these are mount options.

I think I lean towards an [nfs] or possibly [mount] section of nfs.conf.

> 
> > 
> > It isn't clear to me where the identity should come from.
> > In some circumstances it might make sense to take it from nfs.conf.
> > In that case we would want to support reading /etc/netnfs/NAME/nfs.conf
> > where NAME was determined in much the same way that "ip netns identify"
> > determines a name.  (Compare inum of /proc/self/ns/net with the inum of
> > each name in /run/netns/).
> I think supporting configs per namespaces is a good idea. I don't
> think it would be too difficult to do since we already support
> the nfs.d directory. 

Yes, reading multiple files should be easy enough once we know what we
want to do.

> 
> 
> > If we did that, we could then support "$netns" in the conf file, and
> > allow
> > 
> >  [nfs]
> >   identity = ${hostname}-${netns}
> > 
> > in /etc/nfs.conf, and it would Do The Right Thing for many cases.
> I'm a bit namespace challenged... but as I see it using 
> "ip netns identify" (w/out the [PID]) would return all of
> the current network network namespaces. Then we would run through 
> the /etc/nfs.conf.d/ directory looking for a matching directory
> for any of the returned namespaces. If found that config
> would be used. Something along those lines? 
> 
> With multiple namespaces, how would we know which one to use? 

(I'm only just coming up to speed on network namespaces too....)

A given process can only be in one network namespace.  If it is in the
initial namespace (same as the 'init' process) then "ip netns identify"
reports nothing.  If in some other namespace, then that namespace is
reported.

So if 'mount.nfs' is run in some other net-namespace, it should let
settings in /etc/netfs/NAME/nfs.conf over-ride settings in /etc/nfs.conf

I'm becoming less enamoured with the idea of using network namespaces to
ensure separate transports are used.  Creating a new namespace means
that either you need a new IP address for that namespace, or you need to
set up NAT so processes in the namespace can access the network.  Both
of these seem like a bit too much overhead just to get an independent
TCP connection (or set of connections) to the server.
I almost want an "NFS namespace" which shares the network but has
separate transports.  I have something like that in our SLE12 kernels
(-o sharetransport=NN) but I'd like a better solution.

Being able to insisting on a separate transport is really useful for
problem analysis, and has other administrative uses.

NeilBrown