The nfs.conf and idmapd.conf files both have config options for the
pipefs mountpoint. Currently, changing these from the defaults also
requires manually overriding the systemd unit files that are hard-coded
to mount the filesystem on /var/lib/nfs/rpc_pipefs.
This patch adds two generators to create drop-in config files to
override the dependencies for the systemd units that require the pipefs.
There are two because rpc.idmapd uses a separate configuration file. If
idmapd's configurations are ever folded into the nfs.conf, then the
idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
specified for idmapd in rpc-pipefs-generator.c.
This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
since that is the most logical alternate location for the pipefs
filesystem. Users wanting to use some other location besides
/var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
own systemd mount unit file, but the generators would take care of the
rest.
Finally, this patch removes the dependency on the pipefs from the
rpc-svcgssd.service unit file. rpc.svcgssd uses the sunrpc cache
mechanism to exchange data with the kernel, not the pipefs.
Signed-off-by: Scott Mayhew <[email protected]>
---
systemd/Makefile.am | 5 +-
systemd/idmapd-rpc-pipefs-generator.c | 99 ++++++++++++++++++++++
systemd/rpc-pipefs-generator.c | 153 ++++++++++++++++++++++++++++++++++
systemd/rpc-svcgssd.service | 3 +-
systemd/run-rpc_pipefs.mount | 10 +++
5 files changed, 267 insertions(+), 3 deletions(-)
create mode 100644 systemd/idmapd-rpc-pipefs-generator.c
create mode 100644 systemd/rpc-pipefs-generator.c
create mode 100644 systemd/run-rpc_pipefs.mount
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 0d15b9f..f09be00 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -42,12 +42,15 @@ EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
unit_dir = /usr/lib/systemd/system
generator_dir = /usr/lib/systemd/system-generators
-EXTRA_PROGRAMS = nfs-server-generator
+EXTRA_PROGRAMS = nfs-server-generator rpc-pipefs-generator idmapd-rpc-pipefs-generator
genexecdir = $(generator_dir)
nfs_server_generator_LDADD = ../support/export/libexport.a \
../support/nfs/libnfs.a \
../support/misc/libmisc.a
+rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
+idmapd_rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
+
if INSTALL_SYSTEMD
genexec_PROGRAMS = nfs-server-generator
install-data-hook: $(unit_files)
diff --git a/systemd/idmapd-rpc-pipefs-generator.c b/systemd/idmapd-rpc-pipefs-generator.c
new file mode 100644
index 0000000..6b0e382
--- /dev/null
+++ b/systemd/idmapd-rpc-pipefs-generator.c
@@ -0,0 +1,99 @@
+/*
+ * idmapd-rpc-pipefs-generator:
+ * systemd generator to create ordering dependencies between
+ * the nfs-idmapd service and the rpc_pipefs mount
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
+#include <stdio.h>
+
+#include "nfslib.h"
+#include "conffile.h"
+
+#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
+char *conf_path = _PATH_IDMAPDCONF;
+
+/* We need to convert a path name to a systemd unit
+ * name. This requires some translation ('/' -> '-')
+ * and some escaping.
+ */
+static void systemd_escape(FILE *f, char *path)
+{
+ while (*path == '/')
+ path++;
+ if (!*path) {
+ /* "/" becomes "-", otherwise leading "/" is ignored */
+ fputs("-", f);
+ return;
+ }
+ while (*path) {
+ char c = *path++;
+
+ if (c == '/') {
+ /* multiple non-trailing slashes become '-' */
+ while (*path == '/')
+ path++;
+ if (*path)
+ fputs("-", f);
+ } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
+ fputc(c, f);
+ else
+ fprintf(f, "\\x%02x", c & 0xff);
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ char *path;
+ char dirbase[] = "/nfs-idmapd.service.d";
+ char filebase[] = "/10-pipefs.conf";
+ FILE *f;
+ char *s;
+
+ /* Avoid using any external services */
+ xlog_syslog(0);
+
+ if (argc != 4 || argv[1][0] != '/') {
+ fprintf(stderr, "idmapd-rpc-pipefs-generator: create systemd dependencies for nfs-idmapd\n");
+ fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
+ exit(1);
+ }
+
+ conf_init();
+ s = conf_get_str("General", "pipefs-directory");
+ if (!s)
+ exit(0);
+ if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
+ strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
+ exit(0);
+
+ path = malloc(strlen(argv[1]) + sizeof(dirbase) + sizeof(filebase));
+ if (!path)
+ exit(2);
+ strcat(strcpy(path, argv[1]), dirbase);
+ mkdir(path, 0755);
+ strcat(path, filebase);
+ f = fopen(path, "w");
+ if (!f)
+ exit(1);
+
+ fprintf(f, "# Automatically generated by idmapd-rpc-pipefs-generator\n\n[Unit]\n");
+ fprintf(f, "Requires=\nRequires=");
+ systemd_escape(f, s);
+ fprintf(f, ".mount\n");
+ fprintf(f, "After=\nAfter=");
+ systemd_escape(f, s);
+ fprintf(f, ".mount local-fs.target\n");
+ fclose(f);
+
+ exit(0);
+}
diff --git a/systemd/rpc-pipefs-generator.c b/systemd/rpc-pipefs-generator.c
new file mode 100644
index 0000000..fc65cc9
--- /dev/null
+++ b/systemd/rpc-pipefs-generator.c
@@ -0,0 +1,153 @@
+/*
+ * rpc-pipefs-generator:
+ * systemd generator to create ordering dependencies between
+ * nfs services and the rpc_pipefs mount(s)
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
+#include <stdio.h>
+
+#include "nfslib.h"
+#include "conffile.h"
+
+#define NUM_PIPEFS_USERS 3
+#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
+char *conf_path = NFS_CONFFILE;
+
+/*
+ * conf_name - the name as it appears (or would appear, in the case of idmapd,
+ * in the /etc/nfs.conf
+ * unit_name - the name of the systemd service unit file (minus '.service')
+ * after_local_fs - should the After= directive have local-fs.target or not
+ * generate - should a drop-in config be generated or not
+ */
+struct pipefs_user {
+ char *conf_name;
+ char *unit_name;
+ int after_local_fs;
+ int generate;
+};
+
+/*
+ * blkmapd and idmapd are placeholders for now, hence generate=0. blkmapd
+ * because it does not have nfs.conf support and because the pipefs directory
+ * cannot be overriden. idmapd because it uses a different config file.
+ */
+static struct pipefs_user pipefs_users[NUM_PIPEFS_USERS] = {
+ {
+ .conf_name = "gssd",
+ .unit_name = "rpc-gssd",
+ .after_local_fs = 0,
+ .generate = 1,
+ },
+ {
+ .conf_name = "blkmapd",
+ .unit_name = "nfs-blkmap",
+ .after_local_fs = 0,
+ .generate = 0,
+ },
+ {
+ .conf_name = "idmapd",
+ .unit_name = "nfs-idmapd",
+ .after_local_fs = 1,
+ .generate = 0,
+ }
+};
+
+/* We need to convert a path name to a systemd unit
+ * name. This requires some translation ('/' -> '-')
+ * and some escaping.
+ */
+static void systemd_escape(FILE *f, char *path)
+{
+ while (*path == '/')
+ path++;
+ if (!*path) {
+ /* "/" becomes "-", otherwise leading "/" is ignored */
+ fputs("-", f);
+ return;
+ }
+ while (*path) {
+ char c = *path++;
+
+ if (c == '/') {
+ /* multiple non-trailing slashes become '-' */
+ while (*path == '/')
+ path++;
+ if (*path)
+ fputs("-", f);
+ } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
+ fputc(c, f);
+ else
+ fprintf(f, "\\x%02x", c & 0xff);
+ }
+}
+
+int main(int argc, char *argv[])
+{
+ char *path;
+ char dirbase[] = ".service.d";
+ char filebase[] = "/10-pipefs.conf";
+ FILE *f;
+ char *s;
+ int i;
+ struct pipefs_user p;
+
+ /* Avoid using any external services */
+ xlog_syslog(0);
+
+ if (argc != 4 || argv[1][0] != '/') {
+ fprintf(stderr, "rpc-pipefs-generator: create systemd dependencies for nfs services\n");
+ fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
+ exit(1);
+ }
+
+ conf_init();
+ for (i = 0; i < NUM_PIPEFS_USERS; i++) {
+ p = pipefs_users[i];
+ if (!p.generate)
+ continue;
+
+ s = conf_get_str(p.conf_name, "pipefs-directory");
+ if (!s)
+ continue;
+ if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
+ strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
+ continue;
+
+ path = malloc(strlen(argv[1]) + 1 + sizeof(p.unit_name) +
+ sizeof(dirbase) + sizeof(filebase));
+ if (!path)
+ exit(2);
+ sprintf(path, "%s/%s%s", argv[1], p.unit_name, dirbase);
+ mkdir(path, 0755);
+ strcat(path, filebase);
+ f = fopen(path, "w");
+ if (!f)
+ exit(1);
+
+ fprintf(f, "# Automatically generated by rpc-pipefs-generator\n\n[Unit]\n");
+ fprintf(f, "Requires=\nRequires=");
+ systemd_escape(f, s);
+ fprintf(f, ".mount\n");
+ fprintf(f, "After=\nAfter=");
+ systemd_escape(f, s);
+ fprintf(f, ".mount");
+ if (p.after_local_fs)
+ fprintf(f, " local-fs.target");
+ fprintf(f, "\n");
+
+ fclose(f);
+ }
+
+ exit(0);
+}
diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
index 7187e3c..cb2bcd4 100644
--- a/systemd/rpc-svcgssd.service
+++ b/systemd/rpc-svcgssd.service
@@ -1,8 +1,7 @@
[Unit]
Description=RPC security service for NFS server
DefaultDependencies=no
-Requires=var-lib-nfs-rpc_pipefs.mount
-After=var-lib-nfs-rpc_pipefs.mount local-fs.target
+After=local-fs.target
PartOf=nfs-server.service
PartOf=nfs-utils.service
diff --git a/systemd/run-rpc_pipefs.mount b/systemd/run-rpc_pipefs.mount
new file mode 100644
index 0000000..aab3145
--- /dev/null
+++ b/systemd/run-rpc_pipefs.mount
@@ -0,0 +1,10 @@
+[Unit]
+Description=RPC Pipe File System
+DefaultDependencies=no
+After=systemd-tmpfiles-setup.service
+Conflicts=umount.target
+
+[Mount]
+What=sunrpc
+Where=/run/rpc_pipefs
+Type=rpc_pipefs
--
2.9.3
Hello,
On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> The nfs.conf and idmapd.conf files both have config options for the
> pipefs mountpoint. Currently, changing these from the defaults also
> requires manually overriding the systemd unit files that are hard-coded
> to mount the filesystem on /var/lib/nfs/rpc_pipefs.
The Pipefs-Directory config variable is not documented in either
idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
to know about it as to read the code. I would not call that
a supported interface and can easily be removed. IMHO.
The same thing goes for the Cache-Expiration variable.
>
> This patch adds two generators to create drop-in config files to
> override the dependencies for the systemd units that require the pipefs.
> There are two because rpc.idmapd uses a separate configuration file. If
> idmapd's configurations are ever folded into the nfs.conf, then the
> idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> specified for idmapd in rpc-pipefs-generator.c.
So I'm thinking we just read Pipefs-Directory out of
one spot that would be /etc/nfs.conf. That way we would only
have to support one of these generators.
Also please document the Pipefs-Directory variable in both
the nfs.conf man page and in the example nfs.conf file
in the git tree.
>
> This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> since that is the most logical alternate location for the pipefs
> filesystem. Users wanting to use some other location besides
> /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> own systemd mount unit file, but the generators would take care of the
> rest.
I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
is needed, especially since it is not being install (aka no updates
to the Makefile.am files).
steved.
>
> Finally, this patch removes the dependency on the pipefs from the
> rpc-svcgssd.service unit file. rpc.svcgssd uses the sunrpc cache
> mechanism to exchange data with the kernel, not the pipefs.
>
> Signed-off-by: Scott Mayhew <[email protected]>
> ---
> systemd/Makefile.am | 5 +-
> systemd/idmapd-rpc-pipefs-generator.c | 99 ++++++++++++++++++++++
> systemd/rpc-pipefs-generator.c | 153 ++++++++++++++++++++++++++++++++++
> systemd/rpc-svcgssd.service | 3 +-
> systemd/run-rpc_pipefs.mount | 10 +++
> 5 files changed, 267 insertions(+), 3 deletions(-)
> create mode 100644 systemd/idmapd-rpc-pipefs-generator.c
> create mode 100644 systemd/rpc-pipefs-generator.c
> create mode 100644 systemd/run-rpc_pipefs.mount
>
> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> index 0d15b9f..f09be00 100644
> --- a/systemd/Makefile.am
> +++ b/systemd/Makefile.am
> @@ -42,12 +42,15 @@ EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
> unit_dir = /usr/lib/systemd/system
> generator_dir = /usr/lib/systemd/system-generators
>
> -EXTRA_PROGRAMS = nfs-server-generator
> +EXTRA_PROGRAMS = nfs-server-generator rpc-pipefs-generator idmapd-rpc-pipefs-generator
> genexecdir = $(generator_dir)
> nfs_server_generator_LDADD = ../support/export/libexport.a \
> ../support/nfs/libnfs.a \
> ../support/misc/libmisc.a
>
> +rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
> +idmapd_rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
> +
> if INSTALL_SYSTEMD
> genexec_PROGRAMS = nfs-server-generator
> install-data-hook: $(unit_files)
> diff --git a/systemd/idmapd-rpc-pipefs-generator.c b/systemd/idmapd-rpc-pipefs-generator.c
> new file mode 100644
> index 0000000..6b0e382
> --- /dev/null
> +++ b/systemd/idmapd-rpc-pipefs-generator.c
> @@ -0,0 +1,99 @@
> +/*
> + * idmapd-rpc-pipefs-generator:
> + * systemd generator to create ordering dependencies between
> + * the nfs-idmapd service and the rpc_pipefs mount
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include <stdio.h>
> +
> +#include "nfslib.h"
> +#include "conffile.h"
> +
> +#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
> +char *conf_path = _PATH_IDMAPDCONF;
> +
> +/* We need to convert a path name to a systemd unit
> + * name. This requires some translation ('/' -> '-')
> + * and some escaping.
> + */
> +static void systemd_escape(FILE *f, char *path)
> +{
> + while (*path == '/')
> + path++;
> + if (!*path) {
> + /* "/" becomes "-", otherwise leading "/" is ignored */
> + fputs("-", f);
> + return;
> + }
> + while (*path) {
> + char c = *path++;
> +
> + if (c == '/') {
> + /* multiple non-trailing slashes become '-' */
> + while (*path == '/')
> + path++;
> + if (*path)
> + fputs("-", f);
> + } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
> + fputc(c, f);
> + else
> + fprintf(f, "\\x%02x", c & 0xff);
> + }
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + char *path;
> + char dirbase[] = "/nfs-idmapd.service.d";
> + char filebase[] = "/10-pipefs.conf";
> + FILE *f;
> + char *s;
> +
> + /* Avoid using any external services */
> + xlog_syslog(0);
> +
> + if (argc != 4 || argv[1][0] != '/') {
> + fprintf(stderr, "idmapd-rpc-pipefs-generator: create systemd dependencies for nfs-idmapd\n");
> + fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
> + exit(1);
> + }
> +
> + conf_init();
> + s = conf_get_str("General", "pipefs-directory");
> + if (!s)
> + exit(0);
> + if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
> + strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
> + exit(0);
> +
> + path = malloc(strlen(argv[1]) + sizeof(dirbase) + sizeof(filebase));
> + if (!path)
> + exit(2);
> + strcat(strcpy(path, argv[1]), dirbase);
> + mkdir(path, 0755);
> + strcat(path, filebase);
> + f = fopen(path, "w");
> + if (!f)
> + exit(1);
> +
> + fprintf(f, "# Automatically generated by idmapd-rpc-pipefs-generator\n\n[Unit]\n");
> + fprintf(f, "Requires=\nRequires=");
> + systemd_escape(f, s);
> + fprintf(f, ".mount\n");
> + fprintf(f, "After=\nAfter=");
> + systemd_escape(f, s);
> + fprintf(f, ".mount local-fs.target\n");
> + fclose(f);
> +
> + exit(0);
> +}
> diff --git a/systemd/rpc-pipefs-generator.c b/systemd/rpc-pipefs-generator.c
> new file mode 100644
> index 0000000..fc65cc9
> --- /dev/null
> +++ b/systemd/rpc-pipefs-generator.c
> @@ -0,0 +1,153 @@
> +/*
> + * rpc-pipefs-generator:
> + * systemd generator to create ordering dependencies between
> + * nfs services and the rpc_pipefs mount(s)
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
> +#include <stdio.h>
> +
> +#include "nfslib.h"
> +#include "conffile.h"
> +
> +#define NUM_PIPEFS_USERS 3
> +#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
> +char *conf_path = NFS_CONFFILE;
> +
> +/*
> + * conf_name - the name as it appears (or would appear, in the case of idmapd,
> + * in the /etc/nfs.conf
> + * unit_name - the name of the systemd service unit file (minus '.service')
> + * after_local_fs - should the After= directive have local-fs.target or not
> + * generate - should a drop-in config be generated or not
> + */
> +struct pipefs_user {
> + char *conf_name;
> + char *unit_name;
> + int after_local_fs;
> + int generate;
> +};
> +
> +/*
> + * blkmapd and idmapd are placeholders for now, hence generate=0. blkmapd
> + * because it does not have nfs.conf support and because the pipefs directory
> + * cannot be overriden. idmapd because it uses a different config file.
> + */
> +static struct pipefs_user pipefs_users[NUM_PIPEFS_USERS] = {
> + {
> + .conf_name = "gssd",
> + .unit_name = "rpc-gssd",
> + .after_local_fs = 0,
> + .generate = 1,
> + },
> + {
> + .conf_name = "blkmapd",
> + .unit_name = "nfs-blkmap",
> + .after_local_fs = 0,
> + .generate = 0,
> + },
> + {
> + .conf_name = "idmapd",
> + .unit_name = "nfs-idmapd",
> + .after_local_fs = 1,
> + .generate = 0,
> + }
> +};
> +
> +/* We need to convert a path name to a systemd unit
> + * name. This requires some translation ('/' -> '-')
> + * and some escaping.
> + */
> +static void systemd_escape(FILE *f, char *path)
> +{
> + while (*path == '/')
> + path++;
> + if (!*path) {
> + /* "/" becomes "-", otherwise leading "/" is ignored */
> + fputs("-", f);
> + return;
> + }
> + while (*path) {
> + char c = *path++;
> +
> + if (c == '/') {
> + /* multiple non-trailing slashes become '-' */
> + while (*path == '/')
> + path++;
> + if (*path)
> + fputs("-", f);
> + } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
> + fputc(c, f);
> + else
> + fprintf(f, "\\x%02x", c & 0xff);
> + }
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + char *path;
> + char dirbase[] = ".service.d";
> + char filebase[] = "/10-pipefs.conf";
> + FILE *f;
> + char *s;
> + int i;
> + struct pipefs_user p;
> +
> + /* Avoid using any external services */
> + xlog_syslog(0);
> +
> + if (argc != 4 || argv[1][0] != '/') {
> + fprintf(stderr, "rpc-pipefs-generator: create systemd dependencies for nfs services\n");
> + fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
> + exit(1);
> + }
> +
> + conf_init();
> + for (i = 0; i < NUM_PIPEFS_USERS; i++) {
> + p = pipefs_users[i];
> + if (!p.generate)
> + continue;
> +
> + s = conf_get_str(p.conf_name, "pipefs-directory");
> + if (!s)
> + continue;
> + if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
> + strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
> + continue;
> +
> + path = malloc(strlen(argv[1]) + 1 + sizeof(p.unit_name) +
> + sizeof(dirbase) + sizeof(filebase));
> + if (!path)
> + exit(2);
> + sprintf(path, "%s/%s%s", argv[1], p.unit_name, dirbase);
> + mkdir(path, 0755);
> + strcat(path, filebase);
> + f = fopen(path, "w");
> + if (!f)
> + exit(1);
> +
> + fprintf(f, "# Automatically generated by rpc-pipefs-generator\n\n[Unit]\n");
> + fprintf(f, "Requires=\nRequires=");
> + systemd_escape(f, s);
> + fprintf(f, ".mount\n");
> + fprintf(f, "After=\nAfter=");
> + systemd_escape(f, s);
> + fprintf(f, ".mount");
> + if (p.after_local_fs)
> + fprintf(f, " local-fs.target");
> + fprintf(f, "\n");
> +
> + fclose(f);
> + }
> +
> + exit(0);
> +}
> diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> index 7187e3c..cb2bcd4 100644
> --- a/systemd/rpc-svcgssd.service
> +++ b/systemd/rpc-svcgssd.service
> @@ -1,8 +1,7 @@
> [Unit]
> Description=RPC security service for NFS server
> DefaultDependencies=no
> -Requires=var-lib-nfs-rpc_pipefs.mount
> -After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> +After=local-fs.target
> PartOf=nfs-server.service
> PartOf=nfs-utils.service
>
> diff --git a/systemd/run-rpc_pipefs.mount b/systemd/run-rpc_pipefs.mount
> new file mode 100644
> index 0000000..aab3145
> --- /dev/null
> +++ b/systemd/run-rpc_pipefs.mount
> @@ -0,0 +1,10 @@
> +[Unit]
> +Description=RPC Pipe File System
> +DefaultDependencies=no
> +After=systemd-tmpfiles-setup.service
> +Conflicts=umount.target
> +
> +[Mount]
> +What=sunrpc
> +Where=/run/rpc_pipefs
> +Type=rpc_pipefs
>
On Wed, 29 Mar 2017, Steve Dickson wrote:
> Hello,
>
> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> > The nfs.conf and idmapd.conf files both have config options for the
> > pipefs mountpoint. Currently, changing these from the defaults also
> > requires manually overriding the systemd unit files that are hard-coded
> > to mount the filesystem on /var/lib/nfs/rpc_pipefs.
> The Pipefs-Directory config variable is not documented in either
> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
> to know about it as to read the code. I would not call that
> a supported interface and can easily be removed. IMHO.
> The same thing goes for the Cache-Expiration variable.
So you're saying to document the Pipefs-Directory and Cache-Expiration
variables, not remove them... right? I think they should be documented
in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
>
> >
> > This patch adds two generators to create drop-in config files to
> > override the dependencies for the systemd units that require the pipefs.
> > There are two because rpc.idmapd uses a separate configuration file. If
> > idmapd's configurations are ever folded into the nfs.conf, then the
> > idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> > specified for idmapd in rpc-pipefs-generator.c.
> So I'm thinking we just read Pipefs-Directory out of
> one spot that would be /etc/nfs.conf.
I agree but then rpc.idmapd and nfsidmap should be modified to read
/etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
confusing unless some of the section names and/or variable names from
/etc/idmapd.conf were changed up a bit. That's quite a bit more than
I'm trying to accomplish here.
> That way we would only
> have to support one of these generators.
If your issue is that there are two generators then I can fold them into
single one... then if the idmapd.conf ever get folded into nfs.conf it's
simply a matter of deleting a few lines of code. The only reason I made
two generators is becuase it made more sense to me that way. I'm fine
either way.
>
> Also please document the Pipefs-Directory variable in both
> the nfs.conf man page and in the example nfs.conf file
> in the git tree.
It's actually already documented in both, under the gssd section.
>
> >
> > This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> > since that is the most logical alternate location for the pipefs
> > filesystem. Users wanting to use some other location besides
> > /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> > own systemd mount unit file, but the generators would take care of the
> > rest.
> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
> is needed, especially since it is not being install (aka no updates
> to the Makefile.am files).
I forgot to update the Makefile.am by mistake. I definitely want the
new unit file. The idea is to give users a choice between
/var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
manually messing around with systemd configs.
-Scott
>
> steved.
>
> >
> > Finally, this patch removes the dependency on the pipefs from the
> > rpc-svcgssd.service unit file. rpc.svcgssd uses the sunrpc cache
> > mechanism to exchange data with the kernel, not the pipefs.
> >
> > Signed-off-by: Scott Mayhew <[email protected]>
> > ---
> > systemd/Makefile.am | 5 +-
> > systemd/idmapd-rpc-pipefs-generator.c | 99 ++++++++++++++++++++++
> > systemd/rpc-pipefs-generator.c | 153 ++++++++++++++++++++++++++++++++++
> > systemd/rpc-svcgssd.service | 3 +-
> > systemd/run-rpc_pipefs.mount | 10 +++
> > 5 files changed, 267 insertions(+), 3 deletions(-)
> > create mode 100644 systemd/idmapd-rpc-pipefs-generator.c
> > create mode 100644 systemd/rpc-pipefs-generator.c
> > create mode 100644 systemd/run-rpc_pipefs.mount
> >
> > diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> > index 0d15b9f..f09be00 100644
> > --- a/systemd/Makefile.am
> > +++ b/systemd/Makefile.am
> > @@ -42,12 +42,15 @@ EXTRA_DIST = $(unit_files) $(man5_MANS) $(man7_MANS)
> > unit_dir = /usr/lib/systemd/system
> > generator_dir = /usr/lib/systemd/system-generators
> >
> > -EXTRA_PROGRAMS = nfs-server-generator
> > +EXTRA_PROGRAMS = nfs-server-generator rpc-pipefs-generator idmapd-rpc-pipefs-generator
> > genexecdir = $(generator_dir)
> > nfs_server_generator_LDADD = ../support/export/libexport.a \
> > ../support/nfs/libnfs.a \
> > ../support/misc/libmisc.a
> >
> > +rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
> > +idmapd_rpc_pipefs_generator_LDADD = ../support/nfs/libnfs.a
> > +
> > if INSTALL_SYSTEMD
> > genexec_PROGRAMS = nfs-server-generator
> > install-data-hook: $(unit_files)
> > diff --git a/systemd/idmapd-rpc-pipefs-generator.c b/systemd/idmapd-rpc-pipefs-generator.c
> > new file mode 100644
> > index 0000000..6b0e382
> > --- /dev/null
> > +++ b/systemd/idmapd-rpc-pipefs-generator.c
> > @@ -0,0 +1,99 @@
> > +/*
> > + * idmapd-rpc-pipefs-generator:
> > + * systemd generator to create ordering dependencies between
> > + * the nfs-idmapd service and the rpc_pipefs mount
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <ctype.h>
> > +#include <stdio.h>
> > +
> > +#include "nfslib.h"
> > +#include "conffile.h"
> > +
> > +#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
> > +char *conf_path = _PATH_IDMAPDCONF;
> > +
> > +/* We need to convert a path name to a systemd unit
> > + * name. This requires some translation ('/' -> '-')
> > + * and some escaping.
> > + */
> > +static void systemd_escape(FILE *f, char *path)
> > +{
> > + while (*path == '/')
> > + path++;
> > + if (!*path) {
> > + /* "/" becomes "-", otherwise leading "/" is ignored */
> > + fputs("-", f);
> > + return;
> > + }
> > + while (*path) {
> > + char c = *path++;
> > +
> > + if (c == '/') {
> > + /* multiple non-trailing slashes become '-' */
> > + while (*path == '/')
> > + path++;
> > + if (*path)
> > + fputs("-", f);
> > + } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
> > + fputc(c, f);
> > + else
> > + fprintf(f, "\\x%02x", c & 0xff);
> > + }
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + char *path;
> > + char dirbase[] = "/nfs-idmapd.service.d";
> > + char filebase[] = "/10-pipefs.conf";
> > + FILE *f;
> > + char *s;
> > +
> > + /* Avoid using any external services */
> > + xlog_syslog(0);
> > +
> > + if (argc != 4 || argv[1][0] != '/') {
> > + fprintf(stderr, "idmapd-rpc-pipefs-generator: create systemd dependencies for nfs-idmapd\n");
> > + fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
> > + exit(1);
> > + }
> > +
> > + conf_init();
> > + s = conf_get_str("General", "pipefs-directory");
> > + if (!s)
> > + exit(0);
> > + if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
> > + strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
> > + exit(0);
> > +
> > + path = malloc(strlen(argv[1]) + sizeof(dirbase) + sizeof(filebase));
> > + if (!path)
> > + exit(2);
> > + strcat(strcpy(path, argv[1]), dirbase);
> > + mkdir(path, 0755);
> > + strcat(path, filebase);
> > + f = fopen(path, "w");
> > + if (!f)
> > + exit(1);
> > +
> > + fprintf(f, "# Automatically generated by idmapd-rpc-pipefs-generator\n\n[Unit]\n");
> > + fprintf(f, "Requires=\nRequires=");
> > + systemd_escape(f, s);
> > + fprintf(f, ".mount\n");
> > + fprintf(f, "After=\nAfter=");
> > + systemd_escape(f, s);
> > + fprintf(f, ".mount local-fs.target\n");
> > + fclose(f);
> > +
> > + exit(0);
> > +}
> > diff --git a/systemd/rpc-pipefs-generator.c b/systemd/rpc-pipefs-generator.c
> > new file mode 100644
> > index 0000000..fc65cc9
> > --- /dev/null
> > +++ b/systemd/rpc-pipefs-generator.c
> > @@ -0,0 +1,153 @@
> > +/*
> > + * rpc-pipefs-generator:
> > + * systemd generator to create ordering dependencies between
> > + * nfs services and the rpc_pipefs mount(s)
> > + */
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +#include <unistd.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <ctype.h>
> > +#include <stdio.h>
> > +
> > +#include "nfslib.h"
> > +#include "conffile.h"
> > +
> > +#define NUM_PIPEFS_USERS 3
> > +#define RPC_PIPEFS_DEFAULT "/var/lib/nfs/rpc_pipefs"
> > +char *conf_path = NFS_CONFFILE;
> > +
> > +/*
> > + * conf_name - the name as it appears (or would appear, in the case of idmapd,
> > + * in the /etc/nfs.conf
> > + * unit_name - the name of the systemd service unit file (minus '.service')
> > + * after_local_fs - should the After= directive have local-fs.target or not
> > + * generate - should a drop-in config be generated or not
> > + */
> > +struct pipefs_user {
> > + char *conf_name;
> > + char *unit_name;
> > + int after_local_fs;
> > + int generate;
> > +};
> > +
> > +/*
> > + * blkmapd and idmapd are placeholders for now, hence generate=0. blkmapd
> > + * because it does not have nfs.conf support and because the pipefs directory
> > + * cannot be overriden. idmapd because it uses a different config file.
> > + */
> > +static struct pipefs_user pipefs_users[NUM_PIPEFS_USERS] = {
> > + {
> > + .conf_name = "gssd",
> > + .unit_name = "rpc-gssd",
> > + .after_local_fs = 0,
> > + .generate = 1,
> > + },
> > + {
> > + .conf_name = "blkmapd",
> > + .unit_name = "nfs-blkmap",
> > + .after_local_fs = 0,
> > + .generate = 0,
> > + },
> > + {
> > + .conf_name = "idmapd",
> > + .unit_name = "nfs-idmapd",
> > + .after_local_fs = 1,
> > + .generate = 0,
> > + }
> > +};
> > +
> > +/* We need to convert a path name to a systemd unit
> > + * name. This requires some translation ('/' -> '-')
> > + * and some escaping.
> > + */
> > +static void systemd_escape(FILE *f, char *path)
> > +{
> > + while (*path == '/')
> > + path++;
> > + if (!*path) {
> > + /* "/" becomes "-", otherwise leading "/" is ignored */
> > + fputs("-", f);
> > + return;
> > + }
> > + while (*path) {
> > + char c = *path++;
> > +
> > + if (c == '/') {
> > + /* multiple non-trailing slashes become '-' */
> > + while (*path == '/')
> > + path++;
> > + if (*path)
> > + fputs("-", f);
> > + } else if (isalnum(c) || c == ':' || c == '.' || c == '_')
> > + fputc(c, f);
> > + else
> > + fprintf(f, "\\x%02x", c & 0xff);
> > + }
> > +}
> > +
> > +int main(int argc, char *argv[])
> > +{
> > + char *path;
> > + char dirbase[] = ".service.d";
> > + char filebase[] = "/10-pipefs.conf";
> > + FILE *f;
> > + char *s;
> > + int i;
> > + struct pipefs_user p;
> > +
> > + /* Avoid using any external services */
> > + xlog_syslog(0);
> > +
> > + if (argc != 4 || argv[1][0] != '/') {
> > + fprintf(stderr, "rpc-pipefs-generator: create systemd dependencies for nfs services\n");
> > + fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
> > + exit(1);
> > + }
> > +
> > + conf_init();
> > + for (i = 0; i < NUM_PIPEFS_USERS; i++) {
> > + p = pipefs_users[i];
> > + if (!p.generate)
> > + continue;
> > +
> > + s = conf_get_str(p.conf_name, "pipefs-directory");
> > + if (!s)
> > + continue;
> > + if (strlen(s) == strlen(RPC_PIPEFS_DEFAULT) &&
> > + strcmp(s, RPC_PIPEFS_DEFAULT) == 0)
> > + continue;
> > +
> > + path = malloc(strlen(argv[1]) + 1 + sizeof(p.unit_name) +
> > + sizeof(dirbase) + sizeof(filebase));
> > + if (!path)
> > + exit(2);
> > + sprintf(path, "%s/%s%s", argv[1], p.unit_name, dirbase);
> > + mkdir(path, 0755);
> > + strcat(path, filebase);
> > + f = fopen(path, "w");
> > + if (!f)
> > + exit(1);
> > +
> > + fprintf(f, "# Automatically generated by rpc-pipefs-generator\n\n[Unit]\n");
> > + fprintf(f, "Requires=\nRequires=");
> > + systemd_escape(f, s);
> > + fprintf(f, ".mount\n");
> > + fprintf(f, "After=\nAfter=");
> > + systemd_escape(f, s);
> > + fprintf(f, ".mount");
> > + if (p.after_local_fs)
> > + fprintf(f, " local-fs.target");
> > + fprintf(f, "\n");
> > +
> > + fclose(f);
> > + }
> > +
> > + exit(0);
> > +}
> > diff --git a/systemd/rpc-svcgssd.service b/systemd/rpc-svcgssd.service
> > index 7187e3c..cb2bcd4 100644
> > --- a/systemd/rpc-svcgssd.service
> > +++ b/systemd/rpc-svcgssd.service
> > @@ -1,8 +1,7 @@
> > [Unit]
> > Description=RPC security service for NFS server
> > DefaultDependencies=no
> > -Requires=var-lib-nfs-rpc_pipefs.mount
> > -After=var-lib-nfs-rpc_pipefs.mount local-fs.target
> > +After=local-fs.target
> > PartOf=nfs-server.service
> > PartOf=nfs-utils.service
> >
> > diff --git a/systemd/run-rpc_pipefs.mount b/systemd/run-rpc_pipefs.mount
> > new file mode 100644
> > index 0000000..aab3145
> > --- /dev/null
> > +++ b/systemd/run-rpc_pipefs.mount
> > @@ -0,0 +1,10 @@
> > +[Unit]
> > +Description=RPC Pipe File System
> > +DefaultDependencies=no
> > +After=systemd-tmpfiles-setup.service
> > +Conflicts=umount.target
> > +
> > +[Mount]
> > +What=sunrpc
> > +Where=/run/rpc_pipefs
> > +Type=rpc_pipefs
> >
On 03/29/2017 02:02 PM, Scott Mayhew wrote:
> On Wed, 29 Mar 2017, Steve Dickson wrote:
>
>> Hello,
>>
>> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
>>> The nfs.conf and idmapd.conf files both have config options for the
>>> pipefs mountpoint. Currently, changing these from the defaults also
>>> requires manually overriding the systemd unit files that are hard-coded
>>> to mount the filesystem on /var/lib/nfs/rpc_pipefs.
>> The Pipefs-Directory config variable is not documented in either
>> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
>> to know about it as to read the code. I would not call that
>> a supported interface and can easily be removed. IMHO.
>> The same thing goes for the Cache-Expiration variable.
>
> So you're saying to document the Pipefs-Directory and Cache-Expiration
> variables, not remove them... right? I think they should be documented
> in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
I'm saying since they are not documented interfaces so we can
move them out of idmapd.conf and into nfs.conf
>>
>>>
>>> This patch adds two generators to create drop-in config files to
>>> override the dependencies for the systemd units that require the pipefs.
>>> There are two because rpc.idmapd uses a separate configuration file. If
>>> idmapd's configurations are ever folded into the nfs.conf, then the
>>> idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
>>> specified for idmapd in rpc-pipefs-generator.c.
>> So I'm thinking we just read Pipefs-Directory out of
>> one spot that would be /etc/nfs.conf.
>
> I agree but then rpc.idmapd and nfsidmap should be modified to read
> /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> confusing unless some of the section names and/or variable names from
> /etc/idmapd.conf were changed up a bit. That's quite a bit more than
> I'm trying to accomplish here.
I'm not saying move them all just move Pipefs-Directory into /etc/nfs.conf.
Move them would be a pain and I agree, well beyond the cope of what you
are trying to do here.
Yes, this means rpc.idmap would have to read out of two config files
but in time that could change and looking at the code reading
out of second config file does seems too difficult.
>
>> That way we would only
>> have to support one of these generators.
>
> If your issue is that there are two generators then I can fold them into
> single one... then if the idmapd.conf ever get folded into nfs.conf it's
> simply a matter of deleting a few lines of code. The only reason I made
> two generators is becuase it made more sense to me that way. I'm fine
> either way.
>
>>
>> Also please document the Pipefs-Directory variable in both
>> the nfs.conf man page and in the example nfs.conf file
>> in the git tree.
>
> It's actually already documented in both, under the gssd section.
Perfect... Just cut/past from the gssd section and add a
idmapd section to the examples in nfs.conf showing the
default value.
>
>>
>>>
>>> This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
>>> since that is the most logical alternate location for the pipefs
>>> filesystem. Users wanting to use some other location besides
>>> /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
>>> own systemd mount unit file, but the generators would take care of the
>>> rest.
>> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
>> is needed, especially since it is not being install (aka no updates
>> to the Makefile.am files).
>
> I forgot to update the Makefile.am by mistake. I definitely want the
> new unit file. The idea is to give users a choice between
> /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> manually messing around with systemd configs.
Hmm... Why would users care and would a user do the switch via
a systemd command? Also who is going to create that directory?
The point of all this is I don't think we needed two generator.
I would rather make changes to where (undocumented) config
knobs are read to avoid the second generator.
steved.
On Wed, 29 Mar 2017, Steve Dickson wrote:
>
>
> On 03/29/2017 02:02 PM, Scott Mayhew wrote:
> > On Wed, 29 Mar 2017, Steve Dickson wrote:
> >
> >> Hello,
> >>
> >> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> >>> The nfs.conf and idmapd.conf files both have config options for the
> >>> pipefs mountpoint. Currently, changing these from the defaults also
> >>> requires manually overriding the systemd unit files that are hard-coded
> >>> to mount the filesystem on /var/lib/nfs/rpc_pipefs.
> >> The Pipefs-Directory config variable is not documented in either
> >> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
> >> to know about it as to read the code. I would not call that
> >> a supported interface and can easily be removed. IMHO.
> >> The same thing goes for the Cache-Expiration variable.
> >
> > So you're saying to document the Pipefs-Directory and Cache-Expiration
> > variables, not remove them... right? I think they should be documented
> > in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
> I'm saying since they are not documented interfaces so we can
> move them out of idmapd.conf and into nfs.conf
But what is gained by doing that, really? And "not documented" is not
quite the same thing as "unknown". The RHEL 4 idmapd.conf used to even
include the Pipefs-Directory parameter, and I see references to it in
both Red Hat bugzillas and knowledge articles.
> >>
> >>>
> >>> This patch adds two generators to create drop-in config files to
> >>> override the dependencies for the systemd units that require the pipefs.
> >>> There are two because rpc.idmapd uses a separate configuration file. If
> >>> idmapd's configurations are ever folded into the nfs.conf, then the
> >>> idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> >>> specified for idmapd in rpc-pipefs-generator.c.
> >> So I'm thinking we just read Pipefs-Directory out of
> >> one spot that would be /etc/nfs.conf.
> >
> > I agree but then rpc.idmapd and nfsidmap should be modified to read
> > /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> > confusing unless some of the section names and/or variable names from
> > /etc/idmapd.conf were changed up a bit. That's quite a bit more than
> > I'm trying to accomplish here.
> I'm not saying move them all just move Pipefs-Directory into /etc/nfs.conf.
> Move them would be a pain and I agree, well beyond the cope of what you
> are trying to do here.
>
> Yes, this means rpc.idmap would have to read out of two config files
> but in time that could change and looking at the code reading
> out of second config file does seems too difficult.
>
I'm not saying it would be difficult reading those two settings out of a
different config file, I just don't see the need for it.
If you could specify the parameter once and only once, and have all the
programs that use that parameter use the same value, then I think maybe
it'd be a good idea, particularly for the pipefs filesystem where it
probably doesn't make sense to have multiple filesystems mounted on
different mountpoints... but there's no support for 'global' settings
in nfs.conf (maybe that's a good idea though).
>
> >
> >> That way we would only
> >> have to support one of these generators.
> >
> > If your issue is that there are two generators then I can fold them into
> > single one... then if the idmapd.conf ever get folded into nfs.conf it's
> > simply a matter of deleting a few lines of code. The only reason I made
> > two generators is becuase it made more sense to me that way. I'm fine
> > either way.
> >
> >>
> >> Also please document the Pipefs-Directory variable in both
> >> the nfs.conf man page and in the example nfs.conf file
> >> in the git tree.
> >
> > It's actually already documented in both, under the gssd section.
> Perfect... Just cut/past from the gssd section and add a
> idmapd section to the examples in nfs.conf showing the
> default value.
>
> >
> >>
> >>>
> >>> This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> >>> since that is the most logical alternate location for the pipefs
> >>> filesystem. Users wanting to use some other location besides
> >>> /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> >>> own systemd mount unit file, but the generators would take care of the
> >>> rest.
> >> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
> >> is needed, especially since it is not being install (aka no updates
> >> to the Makefile.am files).
> >
> > I forgot to update the Makefile.am by mistake. I definitely want the
> > new unit file. The idea is to give users a choice between
> > /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> > manually messing around with systemd configs.
> Hmm... Why would users care and would a user do the switch via
> a systemd command?
Most users probably don't care. But the pacemaker cluster resource
agents do a bind mount over top of /var/lib/nfs. In order to do that,
they unmount the pipefs, do the bind mount, and then remount the pipefs.
If unmounting the pipefs fails then the cluster has a tendency to fall
apart. By moving the pipefs out of /var/lib/nfs then that's one less
thing to worry about.
The way the switch is done is edit the nfs.conf and idmapd.conf and then
either reboot, or do a 'systemctl daemon-reload' and then restart idmapd
and gssd.
> Also who is going to create that directory?
systemd creates the directory automatically.
>
> The point of all this is I don't think we needed two generator.
> I would rather make changes to where (undocumented) config
> knobs are read to avoid the second generator.
I've already boiled it down to one generator. Does that make a
difference?
-Scott
>
> steved.
On Wed, Mar 29 2017, Scott Mayhew wrote:
> On Wed, 29 Mar 2017, Steve Dickson wrote:
>
>> Hello,
>>
>> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
>> > The nfs.conf and idmapd.conf files both have config options for the
>> > pipefs mountpoint. Currently, changing these from the defaults also
>> > requires manually overriding the systemd unit files that are hard-coded
>> > to mount the filesystem on /var/lib/nfs/rpc_pipefs.
>> The Pipefs-Directory config variable is not documented in either
>> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
>> to know about it as to read the code. I would not call that
>> a supported interface and can easily be removed. IMHO.
>> The same thing goes for the Cache-Expiration variable.
>
> So you're saying to document the Pipefs-Directory and Cache-Expiration
> variables, not remove them... right? I think they should be documented
> in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
>>
>> >
>> > This patch adds two generators to create drop-in config files to
>> > override the dependencies for the systemd units that require the pipefs.
>> > There are two because rpc.idmapd uses a separate configuration file. If
>> > idmapd's configurations are ever folded into the nfs.conf, then the
>> > idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
>> > specified for idmapd in rpc-pipefs-generator.c.
>> So I'm thinking we just read Pipefs-Directory out of
>> one spot that would be /etc/nfs.conf.
>
> I agree but then rpc.idmapd and nfsidmap should be modified to read
> /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> confusing unless some of the section names and/or variable names from
> /etc/idmapd.conf were changed up a bit. That's quite a bit more than
> I'm trying to accomplish here.
>
>> That way we would only
>> have to support one of these generators.
>
> If your issue is that there are two generators then I can fold them into
> single one... then if the idmapd.conf ever get folded into nfs.conf it's
> simply a matter of deleting a few lines of code. The only reason I made
> two generators is becuase it made more sense to me that way. I'm fine
> either way.
>
>>
>> Also please document the Pipefs-Directory variable in both
>> the nfs.conf man page and in the example nfs.conf file
>> in the git tree.
>
> It's actually already documented in both, under the gssd section.
>
>>
>> >
>> > This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
>> > since that is the most logical alternate location for the pipefs
>> > filesystem. Users wanting to use some other location besides
>> > /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
>> > own systemd mount unit file, but the generators would take care of the
>> > rest.
>> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
>> is needed, especially since it is not being install (aka no updates
>> to the Makefile.am files).
>
> I forgot to update the Makefile.am by mistake. I definitely want the
> new unit file. The idea is to give users a choice between
> /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> manually messing around with systemd configs.
Could the new unit file be generated by the generator???
i.e. the generator creates the foo.mount based on the nfs.conf file
contents, as well as create the dependencies between services and this
file.
??
NeilBrown
On 03/29/2017 06:59 PM, Scott Mayhew wrote:
> On Wed, 29 Mar 2017, Steve Dickson wrote:
>
>>
>>
>> On 03/29/2017 02:02 PM, Scott Mayhew wrote:
>>> On Wed, 29 Mar 2017, Steve Dickson wrote:
>>>
>>>> Hello,
>>>>
>>>> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
>>>>> The nfs.conf and idmapd.conf files both have config options for the
>>>>> pipefs mountpoint. Currently, changing these from the defaults also
>>>>> requires manually overriding the systemd unit files that are hard-coded
>>>>> to mount the filesystem on /var/lib/nfs/rpc_pipefs.
>>>> The Pipefs-Directory config variable is not documented in either
>>>> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
>>>> to know about it as to read the code. I would not call that
>>>> a supported interface and can easily be removed. IMHO.
>>>> The same thing goes for the Cache-Expiration variable.
>>>
>>> So you're saying to document the Pipefs-Directory and Cache-Expiration
>>> variables, not remove them... right? I think they should be documented
>>> in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
>> I'm saying since they are not documented interfaces so we can
>> move them out of idmapd.conf and into nfs.conf
>
> But what is gained by doing that, really? And "not documented" is not
> quite the same thing as "unknown". The RHEL 4 idmapd.conf used to even
> include the Pipefs-Directory parameter, and I see references to it in
> both Red Hat bugzillas and knowledge articles.
What is gained is we are starting to move all the
server configurations to one file. This is upstream
so these changes are not going make a difference to
legacy OS.
Plus I'm guess nobody ever changed Pipefs-Directory config
because nobody knew what it did. It is a pretty low level
knob.
>
>>>>
>>>>>
>>>>> This patch adds two generators to create drop-in config files to
>>>>> override the dependencies for the systemd units that require the pipefs.
>>>>> There are two because rpc.idmapd uses a separate configuration file. If
>>>>> idmapd's configurations are ever folded into the nfs.conf, then the
>>>>> idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
>>>>> specified for idmapd in rpc-pipefs-generator.c.
>>>> So I'm thinking we just read Pipefs-Directory out of
>>>> one spot that would be /etc/nfs.conf.
>>>
>>> I agree but then rpc.idmapd and nfsidmap should be modified to read
>>> /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
>>> confusing unless some of the section names and/or variable names from
>>> /etc/idmapd.conf were changed up a bit. That's quite a bit more than
>>> I'm trying to accomplish here.
>> I'm not saying move them all just move Pipefs-Directory into /etc/nfs.conf.
>> Move them would be a pain and I agree, well beyond the cope of what you
>> are trying to do here.
>>
>> Yes, this means rpc.idmap would have to read out of two config files
>> but in time that could change and looking at the code reading
>> out of second config file does seems too difficult.
>>
> I'm not saying it would be difficult reading those two settings out of a
> different config file, I just don't see the need for it.
Again, to start the migration of all server knobs to move
to one config file.
>
> If you could specify the parameter once and only once, and have all the
> programs that use that parameter use the same value, then I think maybe
> it'd be a good idea, particularly for the pipefs filesystem where it
> probably doesn't make sense to have multiple filesystems mounted on
> different mountpoints... but there's no support for 'global' settings
> in nfs.conf (maybe that's a good idea though).
Well in the nfs.conf man page "introduce a section called global". So
maybe now is the time to do that.
>
>>
>>>
>>>> That way we would only
>>>> have to support one of these generators.
>>>
>>> If your issue is that there are two generators then I can fold them into
>>> single one... then if the idmapd.conf ever get folded into nfs.conf it's
>>> simply a matter of deleting a few lines of code. The only reason I made
>>> two generators is becuase it made more sense to me that way. I'm fine
>>> either way.
>>>
>>>>
>>>> Also please document the Pipefs-Directory variable in both
>>>> the nfs.conf man page and in the example nfs.conf file
>>>> in the git tree.
>>>
>>> It's actually already documented in both, under the gssd section.
>> Perfect... Just cut/past from the gssd section and add a
>> idmapd section to the examples in nfs.conf showing the
>> default value.
>>
>>>
>>>>
>>>>>
>>>>> This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
>>>>> since that is the most logical alternate location for the pipefs
>>>>> filesystem. Users wanting to use some other location besides
>>>>> /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
>>>>> own systemd mount unit file, but the generators would take care of the
>>>>> rest.
>>>> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
>>>> is needed, especially since it is not being install (aka no updates
>>>> to the Makefile.am files).
>>>
>>> I forgot to update the Makefile.am by mistake. I definitely want the
>>> new unit file. The idea is to give users a choice between
>>> /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
>>> manually messing around with systemd configs.
>> Hmm... Why would users care and would a user do the switch via
>> a systemd command?
>
> Most users probably don't care. But the pacemaker cluster resource
> agents do a bind mount over top of /var/lib/nfs. In order to do that,
> they unmount the pipefs, do the bind mount, and then remount the pipefs.
> If unmounting the pipefs fails then the cluster has a tendency to fall
> apart. By moving the pipefs out of /var/lib/nfs then that's one less
> thing to worry about.
For the HA world, moving the pipefs to /run makes thing much easier.
I get it...
>
> The way the switch is done is edit the nfs.conf and idmapd.conf and then
> either reboot, or do a 'systemctl daemon-reload' and then restart idmapd
> and gssd.
To me it seems more straightforward to change the all the daemons
to read from one file so only that file needs to be changed.
>
>> Also who is going to create that directory?
>
> systemd creates the directory automatically.
>>
>> The point of all this is I don't think we needed two generator.
>> I would rather make changes to where (undocumented) config
>> knobs are read to avoid the second generator.
>
> I've already boiled it down to one generator. Does that make a
> difference?
I didn't know that... but still I would like to take any and all
opportunity to migrate things in one config file.
steved.
>
> -Scott
>>
>> steved.
On Thu, 30 Mar 2017, Steve Dickson wrote:
>
>
> On 03/29/2017 06:59 PM, Scott Mayhew wrote:
> > On Wed, 29 Mar 2017, Steve Dickson wrote:
> >
> >>
> >>
> >> On 03/29/2017 02:02 PM, Scott Mayhew wrote:
> >>> On Wed, 29 Mar 2017, Steve Dickson wrote:
> >>>
> >>>> Hello,
> >>>>
> >>>> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> >>>>> The nfs.conf and idmapd.conf files both have config options for the
> >>>>> pipefs mountpoint. Currently, changing these from the defaults also
> >>>>> requires manually overriding the systemd unit files that are hard-coded
> >>>>> to mount the filesystem on /var/lib/nfs/rpc_pipefs.
> >>>> The Pipefs-Directory config variable is not documented in either
> >>>> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
> >>>> to know about it as to read the code. I would not call that
> >>>> a supported interface and can easily be removed. IMHO.
> >>>> The same thing goes for the Cache-Expiration variable.
> >>>
> >>> So you're saying to document the Pipefs-Directory and Cache-Expiration
> >>> variables, not remove them... right? I think they should be documented
> >>> in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
> >> I'm saying since they are not documented interfaces so we can
> >> move them out of idmapd.conf and into nfs.conf
> >
> > But what is gained by doing that, really? And "not documented" is not
> > quite the same thing as "unknown". The RHEL 4 idmapd.conf used to even
> > include the Pipefs-Directory parameter, and I see references to it in
> > both Red Hat bugzillas and knowledge articles.
> What is gained is we are starting to move all the
> server configurations to one file. This is upstream
> so these changes are not going make a difference to
> legacy OS.
>
> Plus I'm guess nobody ever changed Pipefs-Directory config
> because nobody knew what it did. It is a pretty low level
> knob.
>
> >
> >>>>
> >>>>>
> >>>>> This patch adds two generators to create drop-in config files to
> >>>>> override the dependencies for the systemd units that require the pipefs.
> >>>>> There are two because rpc.idmapd uses a separate configuration file. If
> >>>>> idmapd's configurations are ever folded into the nfs.conf, then the
> >>>>> idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> >>>>> specified for idmapd in rpc-pipefs-generator.c.
> >>>> So I'm thinking we just read Pipefs-Directory out of
> >>>> one spot that would be /etc/nfs.conf.
> >>>
> >>> I agree but then rpc.idmapd and nfsidmap should be modified to read
> >>> /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> >>> confusing unless some of the section names and/or variable names from
> >>> /etc/idmapd.conf were changed up a bit. That's quite a bit more than
> >>> I'm trying to accomplish here.
> >> I'm not saying move them all just move Pipefs-Directory into /etc/nfs.conf.
> >> Move them would be a pain and I agree, well beyond the cope of what you
> >> are trying to do here.
> >>
> >> Yes, this means rpc.idmap would have to read out of two config files
> >> but in time that could change and looking at the code reading
> >> out of second config file does seems too difficult.
> >>
> > I'm not saying it would be difficult reading those two settings out of a
> > different config file, I just don't see the need for it.
> Again, to start the migration of all server knobs to move
> to one config file.
>
> >
> > If you could specify the parameter once and only once, and have all the
> > programs that use that parameter use the same value, then I think maybe
> > it'd be a good idea, particularly for the pipefs filesystem where it
> > probably doesn't make sense to have multiple filesystems mounted on
> > different mountpoints... but there's no support for 'global' settings
> > in nfs.conf (maybe that's a good idea though).
> Well in the nfs.conf man page "introduce a section called global". So
> maybe now is the time to do that.
>
> >
> >>
> >>>
> >>>> That way we would only
> >>>> have to support one of these generators.
> >>>
> >>> If your issue is that there are two generators then I can fold them into
> >>> single one... then if the idmapd.conf ever get folded into nfs.conf it's
> >>> simply a matter of deleting a few lines of code. The only reason I made
> >>> two generators is becuase it made more sense to me that way. I'm fine
> >>> either way.
> >>>
> >>>>
> >>>> Also please document the Pipefs-Directory variable in both
> >>>> the nfs.conf man page and in the example nfs.conf file
> >>>> in the git tree.
> >>>
> >>> It's actually already documented in both, under the gssd section.
> >> Perfect... Just cut/past from the gssd section and add a
> >> idmapd section to the examples in nfs.conf showing the
> >> default value.
> >>
> >>>
> >>>>
> >>>>>
> >>>>> This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> >>>>> since that is the most logical alternate location for the pipefs
> >>>>> filesystem. Users wanting to use some other location besides
> >>>>> /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> >>>>> own systemd mount unit file, but the generators would take care of the
> >>>>> rest.
> >>>> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
> >>>> is needed, especially since it is not being install (aka no updates
> >>>> to the Makefile.am files).
> >>>
> >>> I forgot to update the Makefile.am by mistake. I definitely want the
> >>> new unit file. The idea is to give users a choice between
> >>> /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> >>> manually messing around with systemd configs.
> >> Hmm... Why would users care and would a user do the switch via
> >> a systemd command?
> >
> > Most users probably don't care. But the pacemaker cluster resource
> > agents do a bind mount over top of /var/lib/nfs. In order to do that,
> > they unmount the pipefs, do the bind mount, and then remount the pipefs.
> > If unmounting the pipefs fails then the cluster has a tendency to fall
> > apart. By moving the pipefs out of /var/lib/nfs then that's one less
> > thing to worry about.
> For the HA world, moving the pipefs to /run makes thing much easier.
> I get it...
>
> >
> > The way the switch is done is edit the nfs.conf and idmapd.conf and then
> > either reboot, or do a 'systemctl daemon-reload' and then restart idmapd
> > and gssd.
> To me it seems more straightforward to change the all the daemons
> to read from one file so only that file needs to be changed.
>
> >
> >> Also who is going to create that directory?
> >
> > systemd creates the directory automatically.
> >>
> >> The point of all this is I don't think we needed two generator.
> >> I would rather make changes to where (undocumented) config
> >> knobs are read to avoid the second generator.
> >
> > I've already boiled it down to one generator. Does that make a
> > difference?
> I didn't know that... but still I would like to take any and all
> opportunity to migrate things in one config file.
Okay, I'll move Pipefs-Directory and Cache-Expiration to nfs.conf.
-Scott
>
> steved.
>
> >
> > -Scott
> >>
> >> steved.
On Thu, 30 Mar 2017, NeilBrown wrote:
> On Wed, Mar 29 2017, Scott Mayhew wrote:
>
> > On Wed, 29 Mar 2017, Steve Dickson wrote:
> >
> >> Hello,
> >>
> >> On 03/28/2017 09:37 AM, Scott Mayhew wrote:
> >> > The nfs.conf and idmapd.conf files both have config options for the
> >> > pipefs mountpoint. Currently, changing these from the defaults also
> >> > requires manually overriding the systemd unit files that are hard-coded
> >> > to mount the filesystem on /var/lib/nfs/rpc_pipefs.
> >> The Pipefs-Directory config variable is not documented in either
> >> idmapd.conf(5) or rpc.idmapd(8) or nfsidmap(5) so the only way
> >> to know about it as to read the code. I would not call that
> >> a supported interface and can easily be removed. IMHO.
> >> The same thing goes for the Cache-Expiration variable.
> >
> > So you're saying to document the Pipefs-Directory and Cache-Expiration
> > variables, not remove them... right? I think they should be documented
> > in idmapd.conf(5) since the other pages both refer to idmapd.conf(5).
> >>
> >> >
> >> > This patch adds two generators to create drop-in config files to
> >> > override the dependencies for the systemd units that require the pipefs.
> >> > There are two because rpc.idmapd uses a separate configuration file. If
> >> > idmapd's configurations are ever folded into the nfs.conf, then the
> >> > idmapd-rpc-pipefs-generator.c can be deleted and generate=1 can be
> >> > specified for idmapd in rpc-pipefs-generator.c.
> >> So I'm thinking we just read Pipefs-Directory out of
> >> one spot that would be /etc/nfs.conf.
> >
> > I agree but then rpc.idmapd and nfsidmap should be modified to read
> > /etc/nfs.conf instead of /etc/idmapd.conf by default, but that could be
> > confusing unless some of the section names and/or variable names from
> > /etc/idmapd.conf were changed up a bit. That's quite a bit more than
> > I'm trying to accomplish here.
> >
> >> That way we would only
> >> have to support one of these generators.
> >
> > If your issue is that there are two generators then I can fold them into
> > single one... then if the idmapd.conf ever get folded into nfs.conf it's
> > simply a matter of deleting a few lines of code. The only reason I made
> > two generators is becuase it made more sense to me that way. I'm fine
> > either way.
> >
> >>
> >> Also please document the Pipefs-Directory variable in both
> >> the nfs.conf man page and in the example nfs.conf file
> >> in the git tree.
> >
> > It's actually already documented in both, under the gssd section.
> >
> >>
> >> >
> >> > This patch also adds a unit file to mount the pipefs on /run/rpc_pipefs,
> >> > since that is the most logical alternate location for the pipefs
> >> > filesystem. Users wanting to use some other location besides
> >> > /var/lib/nfs/rpc_pipefs or /run/rpc_pipefs would have to create their
> >> > own systemd mount unit file, but the generators would take care of the
> >> > rest.
> >> I'm not sure I understand why this new unit file, run-rpc_pipefs.mount,
> >> is needed, especially since it is not being install (aka no updates
> >> to the Makefile.am files).
> >
> > I forgot to update the Makefile.am by mistake. I definitely want the
> > new unit file. The idea is to give users a choice between
> > /var/lib/nfs/rpc_pipes and /run/rpc_pipefs without requiring them to go
> > manually messing around with systemd configs.
>
> Could the new unit file be generated by the generator???
> i.e. the generator creates the foo.mount based on the nfs.conf file
> contents, as well as create the dependencies between services and this
> file.
Good idea, I'll try it.
-Scott
>
> ??
>
> NeilBrown
On 03/30/2017 09:04 AM, Scott Mayhew wrote:
>>> I've already boiled it down to one generator. Does that make a
>>> difference?
>> I didn't know that... but still I would like to take any and all
>> opportunity to migrate things in one config file.
> Okay, I'll move Pipefs-Directory and Cache-Expiration to nfs.conf.
It looks to me that the Cache-Expiration knob is just broken.
When the knob is set rpc.idmapd tries to open a file
/proc/sys/fs/nfs/idmap_cache_timeout
That will only exist when the client does a mount, meaning
the file will never exist when server starts.
Anybody have clue as to why this knob exists?
The point being, Please leave the knob where it is (reading
from idmapd.conf) because I don't think we want to port
broken knobs into nfs.conf.
steved.