[PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")

changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
which maps to EOPNOTSUPP, is not a permanent error.
This useful because when an NFS server starts up there is a small window between
the moment that rpcbind (or portmap) starts responding to lookup requests,
and the moment when nfsd registers with rpcbind.  During that window
rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.

This same reasoning applies to foreground mounts.  They don't wait for
as long, but could still hit the window and fail prematurely.

So revert the above patch and instead add EOPNOTSUPP to the list of
temporary errors known to nfs_is_permanent_error.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mount/stropts.c |    7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index 9de6794c6177..d5dfb5e4a669 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
 	case ETIMEDOUT:
 	case ECONNREFUSED:
 	case EHOSTUNREACH:
+	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
 	case EAGAIN:
 		return 0;	/* temporary */
 	default:
@@ -1019,8 +1020,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
 	if (nfs_try_mount(mi))
 		return EX_SUCCESS;
 
-	/* retry background mounts when the server is not up */
-	if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP) {
+	if (nfs_is_permanent_error(errno)) {
 		mount_error(mi->spec, mi->node, errno);
 		return EX_FAIL;
 	}
@@ -1055,8 +1055,7 @@ static int nfsmount_child(struct nfsmount_info *mi)
 		if (nfs_try_mount(mi))
 			return EX_SUCCESS;
 
-		/* retry background mounts when the server is not up */
-		if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP)
+		if (nfs_is_permanent_error(errno))
 			break;
 
 		if (time(NULL) > timeout)

[PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[NeilBrown]

Commit: 1e41488f428c ("systemd: Order NFS server before client")

added an ordering dependency between network mounts and nfs-server.
This is good for loop-back NFS mounts as it ensures the server
will remain until after the mountpoint is unmounted.

However is is bad for _net mounts (such as those via iSCSI) which
are being NFS exported.

nfs-server needs to be start *after* exported filesystems are mounted,
and *before* NFS filesystems are mounted.  systemd isn't able to make
this distinction natively, so we need to help it.

This patch adds a systemd generator which creates a drop-in for
nfs-server.services so that it is started "After" any "nfs" or "nfs4"
mount, and so that it has a "RequiresMountsFor" dependency on any
exported filesystem.  This creates the required ordering.

Note that if you try to export an "nfs" mount, systemd will detect an
ordering loop and will refused to start the nfs server.  This is
probably the correct thing to do.

This patch also removes the ordering dependency with
remote-fs-pre.target which the above-mentioned commit added.  It is no
longer needed.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 systemd/Makefile.am            |    8 ++
 systemd/nfs-server-generator.c |  144 ++++++++++++++++++++++++++++++++++++++++
 systemd/nfs-server.service     |    3 -
 3 files changed, 152 insertions(+), 3 deletions(-)
 create mode 100644 systemd/nfs-server-generator.c

diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index 03f96e93dccf..49c9b8d3b459 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -39,8 +39,16 @@ endif
 EXTRA_DIST = $(unit_files)
 
 unit_dir = /usr/lib/systemd/system
+generator_dir = /usr/lib/systemd/system-generators
+
+EXTRA_PROGRAMS	= nfs-server-generator
+genexecdir = $(generator_dir)
+nfs_server_generator_LDADD = ../support/export/libexport.a \
+			     ../support/nfs/libnfs.a \
+			     ../support/misc/libmisc.a
 
 if INSTALL_SYSTEMD
+genexec_PROGRAMS = nfs-server-generator
 install-data-hook: $(unit_files)
 	mkdir -p $(DESTDIR)/$(unitdir)
 	cp $(unit_files) $(DESTDIR)/$(unitdir)
diff --git a/systemd/nfs-server-generator.c b/systemd/nfs-server-generator.c
new file mode 100644
index 000000000000..af8bb52bfbd7
--- /dev/null
+++ b/systemd/nfs-server-generator.c
@@ -0,0 +1,144 @@
+/*
+ * nfs-server-generator:
+ *   systemd generator to create ordering dependencies between
+ *   nfs-server and various filesystem mounts
+ *
+ * 1/ nfs-server should start Before any 'nfs' mountpoints are
+ *    mounted, in case they are loop-back mounts.  This ordering is particularly
+ *    important for the shutdown side, so the nfs-server is stopped
+ *    after the filesystems are unmounted.
+ * 2/ nfs-server should start After all exported filesystems are mounted
+ *    so there is no risk of exporting the underlying directory.
+ *    This is particularly important for _net mounts which
+ *    are not caught by "local-fs.target".
+ */
+
+#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 <mntent.h>
+
+#include "misc.h"
+#include "nfslib.h"
+#include "exportfs.h"
+
+/* A simple "set of strings" to remove duplicates
+ * found in /etc/exports
+ */
+struct list {
+	struct list *next;
+	char *name;
+};
+static int is_unique(struct list **lp, char *path)
+{
+	struct list *l = *lp;
+
+	while (l) {
+		if (strcmp(l->name, path) == 0)
+			return 0;
+		l = l->next;
+	}
+	l = malloc(sizeof(*l));
+	l->name = path;
+	l->next = *lp;
+	*lp = l;
+	return 1;
+}
+
+/* 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 == '.')
+			fputc(c, f);
+		else
+			fprintf(f, "\\x%02x", c & 0xff);
+	}
+}
+
+int main(int argc, char *argv[])
+{
+	char		*path;
+	char		dirbase[] = "/nfs-server.service.d";
+	char		filebase[] = "/order-with-mounts.conf";
+	nfs_export	*exp;
+	int		i;
+	struct list	*list = NULL;
+	FILE		*f, *fstab;
+	struct mntent	*mnt;
+
+	if (argc != 4 || argv[1][0] != '/') {
+		fprintf(stderr, "nfs-server-generator: create systemd dependencies for nfs-server\n");
+		fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
+		exit(1);
+	}
+
+	path = malloc(strlen(argv[1]) + sizeof(dirbase) + sizeof(filebase));
+	if (!path)
+		exit(2);
+	if (export_read(_PATH_EXPORTS) +
+	    export_d_read(_PATH_EXPORTS_D) == 0)
+		/* Nothing is exported, so nothing to do */
+		exit(0);
+
+	strcat(strcpy(path, argv[1]), dirbase);
+	mkdir(path, 0755);
+	strcat(path, filebase);
+	f = fopen(path, "w");
+	if (!f)
+		return 1;
+	fprintf(f, "# Automatically generated by nfs-server-generator\n\n[Unit]\n");
+
+	for (i = 0; i < MCL_MAXTYPES; i++) {
+		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
+			if (!is_unique(&list, exp->m_export.e_path))
+				continue;
+			if (strchr(exp->m_export.e_path, ' '))
+				fprintf(f, "RequiresMountsFor=\"%s\"\n",
+					exp->m_export.e_path);
+			else
+				fprintf(f, "RequiresMountsFor=%s\n",
+					exp->m_export.e_path);
+		}
+	}
+
+	fstab = setmntent("/etc/fstab", "r");
+	while ((mnt = getmntent(fstab)) != NULL) {
+		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
+		    strcmp(mnt->mnt_type, "nfs4") != 0)
+			continue;
+		fprintf(f, "Before= ");
+		systemd_escape(f, mnt->mnt_dir);
+		fprintf(f, ".mount\n");
+	}
+
+	fclose(f);
+
+	exit(0);
+}
diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
index 2ccdc6344cd5..196c81849ff4 100644
--- a/systemd/nfs-server.service
+++ b/systemd/nfs-server.service
@@ -16,9 +16,6 @@ Before= rpc-statd-notify.service
 Wants=auth-rpcgss-module.service
 After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
 
-# start/stop server before/after client
-Before=remote-fs-pre.target
-
 Wants=nfs-config.service
 After=nfs-config.service
 

[PATCH 0/2] nfs-utils patches relating to server startup

[NeilBrown]

Two patches for recently discussed functionality.
Both revert earlier patches and provide a more complete
solution.

The first adds a new program.  I've placed this in "systemd/" rather
than "utils/something" because that seemed to make sense.  Is it
inelegant putting code in a directory with unit files?

I don't do a lot of automake coding so if someone were to make sure I
didn't do anything non-standard in the Makefile.am, that would be
appreciated.

Thanks,
NeilBrown

---

NeilBrown (2):
      systemd: improve ordering between nfs-server and various mounts
      mount: RPC_PROGNOTREGISTERED should not be a permanent error


 systemd/Makefile.am            |    8 ++
 systemd/nfs-server-generator.c |  144 ++++++++++++++++++++++++++++++++++++++++
 systemd/nfs-server.service     |    3 -
 utils/mount/stropts.c          |    7 +-
 4 files changed, 155 insertions(+), 7 deletions(-)
 create mode 100644 systemd/nfs-server-generator.c

--
Signature

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[J. Bruce Fields]

On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
> Commit: 1e41488f428c ("systemd: Order NFS server before client")
> 
> added an ordering dependency between network mounts and nfs-server.
> This is good for loop-back NFS mounts as it ensures the server
> will remain until after the mountpoint is unmounted.
> 
> However is is bad for _net mounts (such as those via iSCSI) which
> are being NFS exported.
> 
> nfs-server needs to be start *after* exported filesystems are mounted,
> and *before* NFS filesystems are mounted.  systemd isn't able to make
> this distinction natively, so we need to help it.
> 
> This patch adds a systemd generator which creates a drop-in for
> nfs-server.services so that it is started "After" any "nfs" or "nfs4"

s/After/Before/ ?

The code's right:

> +	fstab = setmntent("/etc/fstab", "r");
> +	while ((mnt = getmntent(fstab)) != NULL) {
> +		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
> +		    strcmp(mnt->mnt_type, "nfs4") != 0)
> +			continue;
> +		fprintf(f, "Before= ");
> +		systemd_escape(f, mnt->mnt_dir);
> +		fprintf(f, ".mount\n");

--b.

[PATCH 0.5/2] Move export_d_read() to support/export/export.c

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5303 bytes --]


This places it in the same place as the similar export_read(),
and allows it to be called from other programs.

Signed-off-by: NeilBrown <neilb@suse.com>
---

Sorry I missed this when I posted the other two patches.  It is
needed for the first one to compile.

NeilBrown


 support/export/export.c    | 65 ++++++++++++++++++++++++++++++++++++++++++++++
 support/include/exportfs.h |  1 +
 utils/exportfs/exportfs.c  | 59 -----------------------------------------
 3 files changed, 66 insertions(+), 59 deletions(-)

diff --git a/support/export/export.c b/support/export/export.c
index e1bebcec12ef..0b8a858c2c74 100644
--- a/support/export/export.c
+++ b/support/export/export.c
@@ -15,6 +15,8 @@
 #include <sys/param.h>
 #include <netinet/in.h>
 #include <stdlib.h>
+#include <dirent.h>
+#include <errno.h>
 #include "xmalloc.h"
 #include "nfslib.h"
 #include "exportfs.h"
@@ -96,6 +98,69 @@ export_read(char *fname)
 }
 
 /**
+ * export_d_read - read entries from /etc/exports.
+ * @fname: name of directory to read from
+ *
+ * Returns number of read entries.
+ * Based on mnt_table_parse_dir() in
+ *  util-linux-ng/shlibs/mount/src/tab_parse.c
+ */
+int
+export_d_read(const char *dname)
+{
+	int n = 0, i;
+	struct dirent **namelist = NULL;
+	int volumes = 0;
+
+
+	n = scandir(dname, &namelist, NULL, versionsort);
+	if (n < 0) {
+		if (errno == ENOENT)
+			/* Silently return */
+			return volumes;
+		xlog(L_NOTICE, "scandir %s: %s", dname, strerror(errno));
+	} else if (n == 0)
+		return volumes;
+
+	for (i = 0; i < n; i++) {
+		struct dirent *d = namelist[i];
+		size_t namesz;
+		char fname[PATH_MAX + 1];
+		int fname_len;
+
+
+		if (d->d_type != DT_UNKNOWN
+		    && d->d_type != DT_REG
+		    && d->d_type != DT_LNK)
+			continue;
+		if (*d->d_name == '.')
+			continue;
+
+#define _EXT_EXPORT_SIZ   (sizeof(_EXT_EXPORT) - 1)
+		namesz = strlen(d->d_name);
+		if (!namesz
+		    || namesz < _EXT_EXPORT_SIZ + 1
+		    || strcmp(d->d_name + (namesz - _EXT_EXPORT_SIZ),
+			      _EXT_EXPORT))
+			continue;
+
+		fname_len = snprintf(fname, PATH_MAX +1, "%s/%s", dname, d->d_name);
+		if (fname_len > PATH_MAX) {
+			xlog(L_WARNING, "Too long file name: %s in %s", d->d_name, dname);
+			continue;
+		}
+
+		volumes += export_read(fname);
+	}
+
+	for (i = 0; i < n; i++)
+		free(namelist[i]);
+	free(namelist);
+
+	return volumes;
+}
+
+/**
  * export_create - create an in-core nfs_export record from an export entry
  * @xep: export entry to lookup
  * @canonical: if set, e_hostname is known to be canonical DNS name
diff --git a/support/include/exportfs.h b/support/include/exportfs.h
index 4cac203fbc29..f033329b5fd2 100644
--- a/support/include/exportfs.h
+++ b/support/include/exportfs.h
@@ -135,6 +135,7 @@ int 				client_member(const char *client,
 						const char *name);
 
 int				export_read(char *fname);
+int				export_d_read(const char *dname);
 void				export_reset(nfs_export *);
 nfs_export *			export_lookup(char *hname, char *path, int caconical);
 nfs_export *			export_find(const struct addrinfo *ai,
diff --git a/utils/exportfs/exportfs.c b/utils/exportfs/exportfs.c
index a00b5ea1853e..4ac2c15ae13e 100644
--- a/utils/exportfs/exportfs.c
+++ b/utils/exportfs/exportfs.c
@@ -26,7 +26,6 @@
 #include <fcntl.h>
 #include <netdb.h>
 #include <errno.h>
-#include <dirent.h>
 #include <limits.h>
 #include <time.h>
 
@@ -47,7 +46,6 @@ static void	error(nfs_export *exp, int err);
 static void	usage(const char *progname, int n);
 static void	validate_export(nfs_export *exp);
 static int	matchhostname(const char *hostname1, const char *hostname2);
-static int	export_d_read(const char *dname);
 static void grab_lockfile(void);
 static void release_lockfile(void);
 
@@ -700,63 +698,6 @@ out:
 	return result;
 }
 
-/* Based on mnt_table_parse_dir() in
-   util-linux-ng/shlibs/mount/src/tab_parse.c */
-static int
-export_d_read(const char *dname)
-{
-	int n = 0, i;
-	struct dirent **namelist = NULL;
-	int volumes = 0;
-
-
-	n = scandir(dname, &namelist, NULL, versionsort);
-	if (n < 0) {
-		if (errno == ENOENT)
-			/* Silently return */
-			return volumes;
-		xlog(L_NOTICE, "scandir %s: %s", dname, strerror(errno));
-	} else if (n == 0)
-		return volumes;
-
-	for (i = 0; i < n; i++) {
-		struct dirent *d = namelist[i];
-		size_t namesz;
-		char fname[PATH_MAX + 1];
-		int fname_len;
-
-
-		if (d->d_type != DT_UNKNOWN
-		    && d->d_type != DT_REG
-		    && d->d_type != DT_LNK)
-			continue;
-		if (*d->d_name == '.')
-			continue;
-
-#define _EXT_EXPORT_SIZ   (sizeof(_EXT_EXPORT) - 1)
-		namesz = strlen(d->d_name);
-		if (!namesz
-		    || namesz < _EXT_EXPORT_SIZ + 1
-		    || strcmp(d->d_name + (namesz - _EXT_EXPORT_SIZ),
-			      _EXT_EXPORT))
-			continue;
-
-		fname_len = snprintf(fname, PATH_MAX +1, "%s/%s", dname, d->d_name);
-		if (fname_len > PATH_MAX) {
-			xlog(L_WARNING, "Too long file name: %s in %s", d->d_name, dname);
-			continue;
-		}
-
-		volumes += export_read(fname);
-	}
-
-	for (i = 0; i < n; i++)
-		free(namelist[i]);
-	free(namelist);
-
-	return volumes;
-}
-
 static char
 dumpopt(char c, char *fmt, ...)
 {
-- 
2.9.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1653 bytes --]

On Sat, Aug 20 2016, J. Bruce Fields wrote:

> On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
>> Commit: 1e41488f428c ("systemd: Order NFS server before client")
>> 
>> added an ordering dependency between network mounts and nfs-server.
>> This is good for loop-back NFS mounts as it ensures the server
>> will remain until after the mountpoint is unmounted.
>> 
>> However is is bad for _net mounts (such as those via iSCSI) which
>> are being NFS exported.
>> 
>> nfs-server needs to be start *after* exported filesystems are mounted,
>> and *before* NFS filesystems are mounted.  systemd isn't able to make
>> this distinction natively, so we need to help it.
>> 
>> This patch adds a systemd generator which creates a drop-in for
>> nfs-server.services so that it is started "After" any "nfs" or "nfs4"
>
> s/After/Before/ ?

Yes.  I suspect this was caused by the fact that my goal was for
nfs-server to stop After nfs mounts.  The concepts start to blur.
It is a bit like doing a git-bisect to find out where some bug was
fixed.  good==bad, bad==good

>
> The code's right:

I remember fixing the code....

Steve: if there are no other revisions, would you still like me to
resend to fix this, or will you just correct it when you eventually
commit it?

Thanks,
NeilBrown


>
>> +	fstab = setmntent("/etc/fstab", "r");
>> +	while ((mnt = getmntent(fstab)) != NULL) {
>> +		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
>> +		    strcmp(mnt->mnt_type, "nfs4") != 0)
>> +			continue;
>> +		fprintf(f, "Before= ");
>> +		systemd_escape(f, mnt->mnt_dir);
>> +		fprintf(f, ".mount\n");
>
> --b.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[J. Bruce Fields]

Just one more thought about this one....

If you mistakenly attempt to mount a server that's using rpc for some
service other than nfs, then this will result in a hang where we
previously got a useful error, right?

Maybe that's a rare enough case not to worry about these days.

--b.

On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
> 
> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
> which maps to EOPNOTSUPP, is not a permanent error.
> This useful because when an NFS server starts up there is a small window between
> the moment that rpcbind (or portmap) starts responding to lookup requests,
> and the moment when nfsd registers with rpcbind.  During that window
> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
> 
> This same reasoning applies to foreground mounts.  They don't wait for
> as long, but could still hit the window and fail prematurely.
> 
> So revert the above patch and instead add EOPNOTSUPP to the list of
> temporary errors known to nfs_is_permanent_error.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  utils/mount/stropts.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 9de6794c6177..d5dfb5e4a669 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>  	case ETIMEDOUT:
>  	case ECONNREFUSED:
>  	case EHOSTUNREACH:
> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>  	case EAGAIN:
>  		return 0;	/* temporary */
>  	default:
> @@ -1019,8 +1020,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>  	if (nfs_try_mount(mi))
>  		return EX_SUCCESS;
>  
> -	/* retry background mounts when the server is not up */
> -	if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP) {
> +	if (nfs_is_permanent_error(errno)) {
>  		mount_error(mi->spec, mi->node, errno);
>  		return EX_FAIL;
>  	}
> @@ -1055,8 +1055,7 @@ static int nfsmount_child(struct nfsmount_info *mi)
>  		if (nfs_try_mount(mi))
>  			return EX_SUCCESS;
>  
> -		/* retry background mounts when the server is not up */
> -		if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP)
> +		if (nfs_is_permanent_error(errno))
>  			break;
>  
>  		if (time(NULL) > timeout)
> 

Re: [PATCH 0/2] nfs-utils patches relating to server startup

[J. Bruce Fields]

On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
> Two patches for recently discussed functionality.
> Both revert earlier patches and provide a more complete
> solution.
> 
> The first adds a new program.  I've placed this in "systemd/" rather
> than "utils/something" because that seemed to make sense.  Is it
> inelegant putting code in a directory with unit files?

Seems to me like a logical enough spot in this case.--b.

> 
> I don't do a lot of automake coding so if someone were to make sure I
> didn't do anything non-standard in the Makefile.am, that would be
> appreciated.
> 
> Thanks,
> NeilBrown
> 
> ---
> 
> NeilBrown (2):
>       systemd: improve ordering between nfs-server and various mounts
>       mount: RPC_PROGNOTREGISTERED should not be a permanent error
> 
> 
>  systemd/Makefile.am            |    8 ++
>  systemd/nfs-server-generator.c |  144 ++++++++++++++++++++++++++++++++++++++++
>  systemd/nfs-server.service     |    3 -
>  utils/mount/stropts.c          |    7 +-
>  4 files changed, 155 insertions(+), 7 deletions(-)
>  create mode 100644 systemd/nfs-server-generator.c
> 
> --
> Signature

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[J. Bruce Fields]

On Sat, Aug 20, 2016 at 06:43:37AM +1000, NeilBrown wrote:
> On Sat, Aug 20 2016, J. Bruce Fields wrote:
> 
> > On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
> >> Commit: 1e41488f428c ("systemd: Order NFS server before client")
> >> 
> >> added an ordering dependency between network mounts and nfs-server.
> >> This is good for loop-back NFS mounts as it ensures the server
> >> will remain until after the mountpoint is unmounted.
> >> 
> >> However is is bad for _net mounts (such as those via iSCSI) which
> >> are being NFS exported.
> >> 
> >> nfs-server needs to be start *after* exported filesystems are mounted,
> >> and *before* NFS filesystems are mounted.  systemd isn't able to make
> >> this distinction natively, so we need to help it.
> >> 
> >> This patch adds a systemd generator which creates a drop-in for
> >> nfs-server.services so that it is started "After" any "nfs" or "nfs4"
> >
> > s/After/Before/ ?
> 
> Yes.  I suspect this was caused by the fact that my goal was for
> nfs-server to stop After nfs mounts.  The concepts start to blur.
> It is a bit like doing a git-bisect to find out where some bug was
> fixed.  good==bad, bad==good
> 
> >
> > The code's right:
> 
> I remember fixing the code....
> 
> Steve: if there are no other revisions, would you still like me to
> resend to fix this, or will you just correct it when you eventually
> commit it?

(ACK to the patch from me, that one changelog nit aside.)

--b.

> 
> Thanks,
> NeilBrown
> 
> 
> >
> >> +	fstab = setmntent("/etc/fstab", "r");
> >> +	while ((mnt = getmntent(fstab)) != NULL) {
> >> +		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
> >> +		    strcmp(mnt->mnt_type, "nfs4") != 0)
> >> +			continue;
> >> +		fprintf(f, "Before= ");
> >> +		systemd_escape(f, mnt->mnt_dir);
> >> +		fprintf(f, ".mount\n");
> >
> > --b.

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3373 bytes --]

On Sat, Aug 20 2016, J. Bruce Fields wrote:

> Just one more thought about this one....
>
> If you mistakenly attempt to mount a server that's using rpc for some
> service other than nfs, then this will result in a hang where we
> previously got a useful error, right?

That is the one problem case - yes.
I prefer to think that it will result in a timeout, rather than a
hang.... but I'm a half-full kind-a-guy.

>
> Maybe that's a rare enough case not to worry about these days.

Trying to NFS mount a filesystem from a machine that doesn't serve NFS
should always be a rare case.
The most likely credible situation that I can come up with is a typo in
the server name.

If a site uses NIS there could very likely be servers that run rpcbind
but not nfsd, so the typo scenario certainly could happen (maybe file
servers are named after composers, while NIS servers are named after dog
noises) .  I think (hope?) that an unexpected delay it nearly as good as
an error message to say "you did something wrong" and that should be
enough for the typo to be noticed.

Thanks,
NeilBrown

>
> --b.
>
> On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
>> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
>> 
>> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
>> which maps to EOPNOTSUPP, is not a permanent error.
>> This useful because when an NFS server starts up there is a small window between
>> the moment that rpcbind (or portmap) starts responding to lookup requests,
>> and the moment when nfsd registers with rpcbind.  During that window
>> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
>> 
>> This same reasoning applies to foreground mounts.  They don't wait for
>> as long, but could still hit the window and fail prematurely.
>> 
>> So revert the above patch and instead add EOPNOTSUPP to the list of
>> temporary errors known to nfs_is_permanent_error.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  utils/mount/stropts.c |    7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>> 
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 9de6794c6177..d5dfb5e4a669 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>>  	case ETIMEDOUT:
>>  	case ECONNREFUSED:
>>  	case EHOSTUNREACH:
>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>>  	case EAGAIN:
>>  		return 0;	/* temporary */
>>  	default:
>> @@ -1019,8 +1020,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>>  	if (nfs_try_mount(mi))
>>  		return EX_SUCCESS;
>>  
>> -	/* retry background mounts when the server is not up */
>> -	if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP) {
>> +	if (nfs_is_permanent_error(errno)) {
>>  		mount_error(mi->spec, mi->node, errno);
>>  		return EX_FAIL;
>>  	}
>> @@ -1055,8 +1055,7 @@ static int nfsmount_child(struct nfsmount_info *mi)
>>  		if (nfs_try_mount(mi))
>>  			return EX_SUCCESS;
>>  
>> -		/* retry background mounts when the server is not up */
>> -		if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP)
>> +		if (nfs_is_permanent_error(errno))
>>  			break;
>>  
>>  		if (time(NULL) > timeout)
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[J. Bruce Fields]

On Sat, Aug 20, 2016 at 07:37:14AM +1000, NeilBrown wrote:
> On Sat, Aug 20 2016, J. Bruce Fields wrote:
> 
> > Just one more thought about this one....
> >
> > If you mistakenly attempt to mount a server that's using rpc for some
> > service other than nfs, then this will result in a hang where we
> > previously got a useful error, right?
> 
> That is the one problem case - yes.
> I prefer to think that it will result in a timeout, rather than a
> hang.... but I'm a half-full kind-a-guy.
> 
> >
> > Maybe that's a rare enough case not to worry about these days.
> 
> Trying to NFS mount a filesystem from a machine that doesn't serve NFS
> should always be a rare case.
> The most likely credible situation that I can come up with is a typo in
> the server name.
> 
> If a site uses NIS there could very likely be servers that run rpcbind
> but not nfsd, so the typo scenario certainly could happen (maybe file
> servers are named after composers, while NIS servers are named after dog
> noises) .  I think (hope?) that an unexpected delay it nearly as good as
> an error message to say "you did something wrong" and that should be
> enough for the typo to be noticed.

OK.  Might just be worth thinking whether there's anything additional
here worth documenting (in code or just changelog) for future developers
in case this turns out to be a bigger problem than expected.

--b.

> 
> Thanks,
> NeilBrown
> 
> >
> > --b.
> >
> > On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
> >> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
> >> 
> >> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
> >> which maps to EOPNOTSUPP, is not a permanent error.
> >> This useful because when an NFS server starts up there is a small window between
> >> the moment that rpcbind (or portmap) starts responding to lookup requests,
> >> and the moment when nfsd registers with rpcbind.  During that window
> >> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
> >> 
> >> This same reasoning applies to foreground mounts.  They don't wait for
> >> as long, but could still hit the window and fail prematurely.
> >> 
> >> So revert the above patch and instead add EOPNOTSUPP to the list of
> >> temporary errors known to nfs_is_permanent_error.
> >> 
> >> Signed-off-by: NeilBrown <neilb@suse.com>
> >> ---
> >>  utils/mount/stropts.c |    7 +++----
> >>  1 file changed, 3 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> >> index 9de6794c6177..d5dfb5e4a669 100644
> >> --- a/utils/mount/stropts.c
> >> +++ b/utils/mount/stropts.c
> >> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
> >>  	case ETIMEDOUT:
> >>  	case ECONNREFUSED:
> >>  	case EHOSTUNREACH:
> >> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
> >>  	case EAGAIN:
> >>  		return 0;	/* temporary */
> >>  	default:
> >> @@ -1019,8 +1020,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
> >>  	if (nfs_try_mount(mi))
> >>  		return EX_SUCCESS;
> >>  
> >> -	/* retry background mounts when the server is not up */
> >> -	if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP) {
> >> +	if (nfs_is_permanent_error(errno)) {
> >>  		mount_error(mi->spec, mi->node, errno);
> >>  		return EX_FAIL;
> >>  	}
> >> @@ -1055,8 +1055,7 @@ static int nfsmount_child(struct nfsmount_info *mi)
> >>  		if (nfs_try_mount(mi))
> >>  			return EX_SUCCESS;
> >>  
> >> -		/* retry background mounts when the server is not up */
> >> -		if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP)
> >> +		if (nfs_is_permanent_error(errno))
> >>  			break;
> >>  
> >>  		if (time(NULL) > timeout)
> >> 

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[Steve Dickson]

On 08/19/2016 04:43 PM, NeilBrown wrote:
> On Sat, Aug 20 2016, J. Bruce Fields wrote:
> 
>> On Fri, Aug 19, 2016 at 11:45:56AM +1000, NeilBrown wrote:
>>> Commit: 1e41488f428c ("systemd: Order NFS server before client")
>>>
>>> added an ordering dependency between network mounts and nfs-server.
>>> This is good for loop-back NFS mounts as it ensures the server
>>> will remain until after the mountpoint is unmounted.
>>>
>>> However is is bad for _net mounts (such as those via iSCSI) which
>>> are being NFS exported.
>>>
>>> nfs-server needs to be start *after* exported filesystems are mounted,
>>> and *before* NFS filesystems are mounted.  systemd isn't able to make
>>> this distinction natively, so we need to help it.
>>>
>>> This patch adds a systemd generator which creates a drop-in for
>>> nfs-server.services so that it is started "After" any "nfs" or "nfs4"
>>
>> s/After/Before/ ?
> 
> Yes.  I suspect this was caused by the fact that my goal was for
> nfs-server to stop After nfs mounts.  The concepts start to blur.
> It is a bit like doing a git-bisect to find out where some bug was
> fixed.  good==bad, bad==good
> 
>>
>> The code's right:
> 
> I remember fixing the code....
> 
> Steve: if there are no other revisions, would you still like me to
> resend to fix this, or will you just correct it when you eventually
> commit it?
No need... I'll make the change... 

steved.

> 
> Thanks,
> NeilBrown
> 
> 
>>
>>> +	fstab = setmntent("/etc/fstab", "r");
>>> +	while ((mnt = getmntent(fstab)) != NULL) {
>>> +		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
>>> +		    strcmp(mnt->mnt_type, "nfs4") != 0)
>>> +			continue;
>>> +		fprintf(f, "Before= ");
>>> +		systemd_escape(f, mnt->mnt_dir);
>>> +		fprintf(f, ".mount\n");
>>
>> --b.

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[Steve Dickson]

On 08/18/2016 09:45 PM, NeilBrown wrote:
> Commit: 1e41488f428c ("systemd: Order NFS server before client")
> 
> added an ordering dependency between network mounts and nfs-server.
> This is good for loop-back NFS mounts as it ensures the server
> will remain until after the mountpoint is unmounted.
> 
> However is is bad for _net mounts (such as those via iSCSI) which
> are being NFS exported.
> 
> nfs-server needs to be start *after* exported filesystems are mounted,
> and *before* NFS filesystems are mounted.  systemd isn't able to make
> this distinction natively, so we need to help it.
> 
> This patch adds a systemd generator which creates a drop-in for
> nfs-server.services so that it is started "After" any "nfs" or "nfs4"
> mount, and so that it has a "RequiresMountsFor" dependency on any
> exported filesystem.  This creates the required ordering.
> 
> Note that if you try to export an "nfs" mount, systemd will detect an
> ordering loop and will refused to start the nfs server.  This is
> probably the correct thing to do.
> 
> This patch also removes the ordering dependency with
> remote-fs-pre.target which the above-mentioned commit added.  It is no
> longer needed.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  systemd/Makefile.am            |    8 ++
>  systemd/nfs-server-generator.c |  144 ++++++++++++++++++++++++++++++++++++++++
>  systemd/nfs-server.service     |    3 -
>  3 files changed, 152 insertions(+), 3 deletions(-)
>  create mode 100644 systemd/nfs-server-generator.c
> 
> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> index 03f96e93dccf..49c9b8d3b459 100644
> --- a/systemd/Makefile.am
> +++ b/systemd/Makefile.am
> @@ -39,8 +39,16 @@ endif
>  EXTRA_DIST = $(unit_files)
>  
>  unit_dir = /usr/lib/systemd/system
> +generator_dir = /usr/lib/systemd/system-generators
> +
> +EXTRA_PROGRAMS	= nfs-server-generator
> +genexecdir = $(generator_dir)
> +nfs_server_generator_LDADD = ../support/export/libexport.a \
> +			     ../support/nfs/libnfs.a \
> +			     ../support/misc/libmisc.a
>  
>  if INSTALL_SYSTEMD
> +genexec_PROGRAMS = nfs-server-generator
>  install-data-hook: $(unit_files)
>  	mkdir -p $(DESTDIR)/$(unitdir)
>  	cp $(unit_files) $(DESTDIR)/$(unitdir)
These automake command are not  compiling or installing the generator... 
I'm looking into it but I was just wondering how you tested this code,
you compiled the code by hand?

Also looking at the other system-generators these seems to 
stick to a particular naming convention 'system-<component>-generator
So I'm thinking we change the name to 
   systemd-nfs-server-generator

steved.
> diff --git a/systemd/nfs-server-generator.c b/systemd/nfs-server-generator.c
> new file mode 100644
> index 000000000000..af8bb52bfbd7
> --- /dev/null
> +++ b/systemd/nfs-server-generator.c
> @@ -0,0 +1,144 @@
> +/*
> + * nfs-server-generator:
> + *   systemd generator to create ordering dependencies between
> + *   nfs-server and various filesystem mounts
> + *
> + * 1/ nfs-server should start Before any 'nfs' mountpoints are
> + *    mounted, in case they are loop-back mounts.  This ordering is particularly
> + *    important for the shutdown side, so the nfs-server is stopped
> + *    after the filesystems are unmounted.
> + * 2/ nfs-server should start After all exported filesystems are mounted
> + *    so there is no risk of exporting the underlying directory.
> + *    This is particularly important for _net mounts which
> + *    are not caught by "local-fs.target".
> + */
> +
> +#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 <mntent.h>
> +
> +#include "misc.h"
> +#include "nfslib.h"
> +#include "exportfs.h"
> +
> +/* A simple "set of strings" to remove duplicates
> + * found in /etc/exports
> + */
> +struct list {
> +	struct list *next;
> +	char *name;
> +};
> +static int is_unique(struct list **lp, char *path)
> +{
> +	struct list *l = *lp;
> +
> +	while (l) {
> +		if (strcmp(l->name, path) == 0)
> +			return 0;
> +		l = l->next;
> +	}
> +	l = malloc(sizeof(*l));
> +	l->name = path;
> +	l->next = *lp;
> +	*lp = l;
> +	return 1;
> +}
> +
> +/* 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 == '.')
> +			fputc(c, f);
> +		else
> +			fprintf(f, "\\x%02x", c & 0xff);
> +	}
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +	char		*path;
> +	char		dirbase[] = "/nfs-server.service.d";
> +	char		filebase[] = "/order-with-mounts.conf";
> +	nfs_export	*exp;
> +	int		i;
> +	struct list	*list = NULL;
> +	FILE		*f, *fstab;
> +	struct mntent	*mnt;
> +
> +	if (argc != 4 || argv[1][0] != '/') {
> +		fprintf(stderr, "nfs-server-generator: create systemd dependencies for nfs-server\n");
> +		fprintf(stderr, "Usage: normal-dir early-dir late-dir\n");
> +		exit(1);
> +	}
> +
> +	path = malloc(strlen(argv[1]) + sizeof(dirbase) + sizeof(filebase));
> +	if (!path)
> +		exit(2);
> +	if (export_read(_PATH_EXPORTS) +
> +	    export_d_read(_PATH_EXPORTS_D) == 0)
> +		/* Nothing is exported, so nothing to do */
> +		exit(0);
> +
> +	strcat(strcpy(path, argv[1]), dirbase);
> +	mkdir(path, 0755);
> +	strcat(path, filebase);
> +	f = fopen(path, "w");
> +	if (!f)
> +		return 1;
> +	fprintf(f, "# Automatically generated by nfs-server-generator\n\n[Unit]\n");
> +
> +	for (i = 0; i < MCL_MAXTYPES; i++) {
> +		for (exp = exportlist[i].p_head; exp; exp = exp->m_next) {
> +			if (!is_unique(&list, exp->m_export.e_path))
> +				continue;
> +			if (strchr(exp->m_export.e_path, ' '))
> +				fprintf(f, "RequiresMountsFor=\"%s\"\n",
> +					exp->m_export.e_path);
> +			else
> +				fprintf(f, "RequiresMountsFor=%s\n",
> +					exp->m_export.e_path);
> +		}
> +	}
> +
> +	fstab = setmntent("/etc/fstab", "r");
> +	while ((mnt = getmntent(fstab)) != NULL) {
> +		if (strcmp(mnt->mnt_type, "nfs") != 0 &&
> +		    strcmp(mnt->mnt_type, "nfs4") != 0)
> +			continue;
> +		fprintf(f, "Before= ");
> +		systemd_escape(f, mnt->mnt_dir);
> +		fprintf(f, ".mount\n");
> +	}
> +
> +	fclose(f);
> +
> +	exit(0);
> +}
> diff --git a/systemd/nfs-server.service b/systemd/nfs-server.service
> index 2ccdc6344cd5..196c81849ff4 100644
> --- a/systemd/nfs-server.service
> +++ b/systemd/nfs-server.service
> @@ -16,9 +16,6 @@ Before= rpc-statd-notify.service
>  Wants=auth-rpcgss-module.service
>  After=rpc-gssd.service gssproxy.service rpc-svcgssd.service
>  
> -# start/stop server before/after client
> -Before=remote-fs-pre.target
> -
>  Wants=nfs-config.service
>  After=nfs-config.service
>  
> 
> 

Re: [PATCH 1/2] systemd: improve ordering between nfs-server and various mounts

[Steve Dickson]

On 08/20/2016 11:53 AM, Steve Dickson wrote:
>>  if INSTALL_SYSTEMD
>> > +genexec_PROGRAMS = nfs-server-generator
>> >  install-data-hook: $(unit_files)
>> >  	mkdir -p $(DESTDIR)/$(unitdir)
>> >  	cp $(unit_files) $(DESTDIR)/$(unitdir)
> These automake command are not  compiling or installing the generator... 
> I'm looking into it but I was just wondering how you tested this code,
> you compiled the code by hand?
My bad... it turns out INSTALL_SYSTEMD is not
defined by default... As soon as I enabled that
everything got compiled and installed.

steved.

Re: [PATCH 0/2] nfs-utils patches relating to server startup

[Steve Dickson]

On 08/18/2016 09:45 PM, NeilBrown wrote:
> Two patches for recently discussed functionality.
> Both revert earlier patches and provide a more complete
> solution.
> 
> The first adds a new program.  I've placed this in "systemd/" rather
> than "utils/something" because that seemed to make sense.  Is it
> inelegant putting code in a directory with unit files?
> 
> I don't do a lot of automake coding so if someone were to make sure I
> didn't do anything non-standard in the Makefile.am, that would be
> appreciated.
All three patches committed under the nfs-utils-1-3-5-rc1 tag... 

steved

> 
> Thanks,
> NeilBrown
> 
> ---
> 
> NeilBrown (2):
>       systemd: improve ordering between nfs-server and various mounts
>       mount: RPC_PROGNOTREGISTERED should not be a permanent error
> 
> 
>  systemd/Makefile.am            |    8 ++
>  systemd/nfs-server-generator.c |  144 ++++++++++++++++++++++++++++++++++++++++
>  systemd/nfs-server.service     |    3 -
>  utils/mount/stropts.c          |    7 +-
>  4 files changed, 155 insertions(+), 7 deletions(-)
>  create mode 100644 systemd/nfs-server-generator.c
> 
> --
> Signature
> 

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[Steve Dickson]

On 08/18/2016 09:45 PM, NeilBrown wrote:
> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
> 
> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
> which maps to EOPNOTSUPP, is not a permanent error.
> This useful because when an NFS server starts up there is a small window between
> the moment that rpcbind (or portmap) starts responding to lookup requests,
> and the moment when nfsd registers with rpcbind.  During that window
> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
> 
> This same reasoning applies to foreground mounts.  They don't wait for
> as long, but could still hit the window and fail prematurely.
> 
> So revert the above patch and instead add EOPNOTSUPP to the list of
> temporary errors known to nfs_is_permanent_error.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  utils/mount/stropts.c |    7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index 9de6794c6177..d5dfb5e4a669 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>  	case ETIMEDOUT:
>  	case ECONNREFUSED:
>  	case EHOSTUNREACH:
> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
Neil,

I think this introduced a regression... When the server does not support
a protocol, say UDP, this patch cause the mount to hang forever,
which I don't think we want.

steved.
>  	case EAGAIN:
>  		return 0;	/* temporary */
>  	default:
> @@ -1019,8 +1020,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>  	if (nfs_try_mount(mi))
>  		return EX_SUCCESS;
>  
> -	/* retry background mounts when the server is not up */
> -	if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP) {
> +	if (nfs_is_permanent_error(errno)) {
>  		mount_error(mi->spec, mi->node, errno);
>  		return EX_FAIL;
>  	}
> @@ -1055,8 +1055,7 @@ static int nfsmount_child(struct nfsmount_info *mi)
>  		if (nfs_try_mount(mi))
>  			return EX_SUCCESS;
>  
> -		/* retry background mounts when the server is not up */
> -		if (nfs_is_permanent_error(errno) && errno != EOPNOTSUPP)
> +		if (nfs_is_permanent_error(errno))
>  			break;
>  
>  		if (time(NULL) > timeout)
> 
> 

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]

On Wed, Nov 23 2016, Steve Dickson wrote:

> [Resent due to mailman rejecting the HTML subpart]
(and the resend included HTML too ... how embarrassing :-)

>
> Hey Neil,
>
>
> On 08/18/2016 09:45 PM, NeilBrown wrote:
>> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
>>
>> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
>> which maps to EOPNOTSUPP, is not a permanent error.
>> This useful because when an NFS server starts up there is a small window between
>> the moment that rpcbind (or portmap) starts responding to lookup requests,
>> and the moment when nfsd registers with rpcbind.  During that window
>> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
>>
>> This same reasoning applies to foreground mounts.  They don't wait for
>> as long, but could still hit the window and fail prematurely.
>>
>> So revert the above patch and instead add EOPNOTSUPP to the list of
>> temporary errors known to nfs_is_permanent_error.
>>
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>>  utils/mount/stropts.c |    7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index 9de6794c6177..d5dfb5e4a669 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>>  	case ETIMEDOUT:
>>  	case ECONNREFUSED:
>>  	case EHOSTUNREACH:
>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
> I think this introduced a regression... When the server does not support
> a protocol, say UDP, this patch cause the mount to hang forever,
> which I don't think we want.


I think we do want it to wait a while so that the nfs server has a
chance to start up.  We have no guarantee that the NFS server will be
registered with rpcbind before rpcbind responds to requests.

I disagree with the "hang forever" description.  I just tested after
disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
before a failure was reported.  It might be longer when trying TCP on a
server that only supports UDP.

So I think the current behavior is correct.  You might be able to argue
that certain error codes should trigger a shorter timeout, but it would
need a strong argument.

Or maybe you mean that a "bg" mount would "hang forever" in the
background?  I think that behavior is correct too.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[Steve Dickson]

On 11/22/2016 05:43 PM, NeilBrown wrote:
> On Wed, Nov 23 2016, Steve Dickson wrote:
> 
>> [Resent due to mailman rejecting the HTML subpart]
> (and the resend included HTML too ... how embarrassing :-)
Yeah... :-) I guess an upgrade turned it on.. 

> 
>>
>> Hey Neil,
>>
>>
>> On 08/18/2016 09:45 PM, NeilBrown wrote:
>>> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
>>>
>>> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
>>> which maps to EOPNOTSUPP, is not a permanent error.
>>> This useful because when an NFS server starts up there is a small window between
>>> the moment that rpcbind (or portmap) starts responding to lookup requests,
>>> and the moment when nfsd registers with rpcbind.  During that window
>>> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
>>>
>>> This same reasoning applies to foreground mounts.  They don't wait for
>>> as long, but could still hit the window and fail prematurely.
>>>
>>> So revert the above patch and instead add EOPNOTSUPP to the list of
>>> temporary errors known to nfs_is_permanent_error.
>>>
>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>> ---
>>>  utils/mount/stropts.c |    7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index 9de6794c6177..d5dfb5e4a669 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>>>  	case ETIMEDOUT:
>>>  	case ECONNREFUSED:
>>>  	case EHOSTUNREACH:
>>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>> I think this introduced a regression... When the server does not support
>> a protocol, say UDP, this patch cause the mount to hang forever,
>> which I don't think we want.
> 
> 
> I think we do want it to wait a while so that the nfs server has a
> chance to start up.  We have no guarantee that the NFS server will be
> registered with rpcbind before rpcbind responds to requests.
I do see this race but there it has to be a small window. With
Fedora its under seconds between the time rpcbind started
and the NFS server.

> 
> I disagree with the "hang forever" description.  I just tested after
> disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
> before a failure was reported.  It might be longer when trying TCP on a
> server that only supports UDP.
Yeah I did not wait that long... You are much more of a patient man than I ;-) 
I do think this is a regression. Going an from an instant failure to one
that takes over 2min is not a good thing... IMHO.

> 
> So I think the current behavior is correct.  You might be able to argue
> that certain error codes should trigger a shorter timeout, but it would
> need a strong argument.
Going with the theory the window is very small, how about 
a retry with a timeout then a failure? 

> 
> Or maybe you mean that a "bg" mount would "hang forever" in the
> background?  I think that behavior is correct too.
I agreed... "bg" mounts should hang longer than fg mounts
but they shouldn't for something that will never happen
like the non-support of a protocol.

steved.

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6751 bytes --]

On Thu, Nov 24 2016, Steve Dickson wrote:

> On 11/22/2016 05:43 PM, NeilBrown wrote:
>> On Wed, Nov 23 2016, Steve Dickson wrote:
>> 
>>> [Resent due to mailman rejecting the HTML subpart]
>> (and the resend included HTML too ... how embarrassing :-)
> Yeah... :-) I guess an upgrade turned it on.. 
>
>> 
>>>
>>> Hey Neil,
>>>
>>>
>>> On 08/18/2016 09:45 PM, NeilBrown wrote:
>>>> Commit: bf66c9facb8e ("mounts.nfs: v2 and v3 background mounts should retry when server is down.")
>>>>
>>>> changed the behaviour of "bg" mounts so that RPC_PROGNOTREGISTERED,
>>>> which maps to EOPNOTSUPP, is not a permanent error.
>>>> This useful because when an NFS server starts up there is a small window between
>>>> the moment that rpcbind (or portmap) starts responding to lookup requests,
>>>> and the moment when nfsd registers with rpcbind.  During that window
>>>> rpcbind will reply with RPC_PROGNOTREGISTERED, but mount should not give up.
>>>>
>>>> This same reasoning applies to foreground mounts.  They don't wait for
>>>> as long, but could still hit the window and fail prematurely.
>>>>
>>>> So revert the above patch and instead add EOPNOTSUPP to the list of
>>>> temporary errors known to nfs_is_permanent_error.
>>>>
>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>> ---
>>>>  utils/mount/stropts.c |    7 +++----
>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>>> index 9de6794c6177..d5dfb5e4a669 100644
>>>> --- a/utils/mount/stropts.c
>>>> +++ b/utils/mount/stropts.c
>>>> @@ -948,6 +948,7 @@ static int nfs_is_permanent_error(int error)
>>>>  	case ETIMEDOUT:
>>>>  	case ECONNREFUSED:
>>>>  	case EHOSTUNREACH:
>>>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>>> I think this introduced a regression... When the server does not support
>>> a protocol, say UDP, this patch cause the mount to hang forever,
>>> which I don't think we want.
>> 
>> 
>> I think we do want it to wait a while so that the nfs server has a
>> chance to start up.  We have no guarantee that the NFS server will be
>> registered with rpcbind before rpcbind responds to requests.
> I do see this race but there it has to be a small window. With
> Fedora its under seconds between the time rpcbind started
> and the NFS server.
>
>> 
>> I disagree with the "hang forever" description.  I just tested after
>> disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
>> before a failure was reported.  It might be longer when trying TCP on a
>> server that only supports UDP.
> Yeah I did not wait that long... You are much more of a patient man than I ;-) 
> I do think this is a regression. Going an from an instant failure to one
> that takes over 2min is not a good thing... IMHO.
>
>> 
>> So I think the current behavior is correct.  You might be able to argue
>> that certain error codes should trigger a shorter timeout, but it would
>> need a strong argument.
> Going with the theory the window is very small, how about 
> a retry with a timeout then a failure? 

I started looking at changing the timeout and it wouldn't be too hard
(if we can agree on a suitable delay), but I feel I must ask why this is
important.
In what situation are you likely to mount with the wrong protocol, that
you aren't able to just Ctrl-C when you realized what a dumb thing you
just did?

If rpcbind isn't running, which is arguably a very similar situation
(no protocols are register) we have always had a long timeout. Why is
"just one protocol not registered" any different?


Anyway, below is the patch I was working on.  I stopped when I wasn't
sure how to handle ECONNREFUSED.

NeilBrown



diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d5dfb5e4a669..084776115b9f 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -935,24 +935,30 @@ static int nfs_try_mount(struct nfsmount_info *mi)
  * failed so far, but fail immediately if there is a local
  * error (like a bad mount option).
  *
- * ESTALE is also a temporary error because some servers
- * return ESTALE when a share is temporarily offline.
+ * If there is a remote error, like ESTALE or RPC_PROGNOTREGISTERED
+ * then it is probably permanent, but there is a small chance
+ * the it is temporary can we caught the server at an awkward
+ * time during start-up.  A shorter timeout is best for such
+ * circumstances, so return a distinct status.
  *
- * Returns 1 if we should fail immediately, or 0 if we
- * should retry.
+ * Returns PERMANENT if we should fail immediately,
+ * TEMPORARY if we should retry normally, or
+ * REMOTE if we should retry with shorter timeout.
  */
-static int nfs_is_permanent_error(int error)
+enum error_type { PERMANENT, TEMPORARY, REMOTE };
+static enum error_type nfs_error_type(int error)
 {
 	switch (error) {
 	case ESTALE:
+	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
+		return REMOTE;
 	case ETIMEDOUT:
 	case ECONNREFUSED:
 	case EHOSTUNREACH:
-	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
 	case EAGAIN:
-		return 0;	/* temporary */
+		return TEMPORARY;
 	default:
-		return 1;	/* permanent */
+		return PERMANENT;
 	}
 }
 
@@ -967,6 +973,7 @@ static int nfsmount_fg(struct nfsmount_info *mi)
 {
 	unsigned int secs = 1;
 	time_t timeout;
+	int last_errno = 0;
 
 	timeout = nfs_parse_retry_option(mi->options,
 					 NFS_DEF_FG_TIMEOUT_MINUTES);
@@ -987,13 +994,22 @@ static int nfsmount_fg(struct nfsmount_info *mi)
 			 */
 			return EX_SUCCESS;
 
-		if (nfs_is_permanent_error(errno))
+		switch(nfs_error_type(errno)) {
+		case PERMANENT:
+			timeout = 0;
 			break;
-
-		if (time(NULL) > timeout) {
+		case REMOTE:
+			if (errno == last_errno)
+				timeout = 0;
+			break;
+		case TEMPORARY:
 			errno = ETIMEDOUT;
 			break;
 		}
+		last_errno = errno;
+
+		if (time(NULL) > timeout)
+			break;
 
 		if (errno != ETIMEDOUT) {
 			if (sleep(secs))
@@ -1020,7 +1036,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
 	if (nfs_try_mount(mi))
 		return EX_SUCCESS;
 
-	if (nfs_is_permanent_error(errno)) {
+	if (nfs_error_type(errno) == PERMANENT) {
 		mount_error(mi->spec, mi->node, errno);
 		return EX_FAIL;
 	}
@@ -1055,8 +1071,14 @@ static int nfsmount_child(struct nfsmount_info *mi)
 		if (nfs_try_mount(mi))
 			return EX_SUCCESS;
 
-		if (nfs_is_permanent_error(errno))
+		switch (nfs_error_type(errno)) {
+		case REMOTE: /* Doesn't hurt to keep trying remote errors
+			      * when in the background
+			      */
+		case PERMANENT:
+			timeout = 0;
 			break;
+		}
 
 		if (time(NULL) > timeout)
 			break;

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[Steve Dickson]

My apologies for the delayed response... I just saw this... 

On 11/23/2016 06:26 PM, NeilBrown wrote:
> On Thu, Nov 24 2016, Steve Dickson wrote:
>>>
>>> So I think the current behavior is correct.  You might be able to argue
>>> that certain error codes should trigger a shorter timeout, but it would
>>> need a strong argument.
>> Going with the theory the window is very small, how about 
>> a retry with a timeout then a failure? 
> 
> I started looking at changing the timeout and it wouldn't be too hard
> (if we can agree on a suitable delay), but I feel I must ask why this is
> important.
Over the last few Connectathon and bakeathons I've been
floating the idea of dismantling the UDP support and
nobody to objected... The main reason is to cut the testing 
matrix in half.

I keep getting these goofy UDP bugs from our QE guys that
nobody is going to fix... and I have not seen a UDP bug
from a customer in years.. I really don't think 
it being used so why continue to support it?

> In what situation are you likely to mount with the wrong protocol, that
> you aren't able to just Ctrl-C when you realized what a dumb thing you
> just did?
I turned off the UDP support in rpc.nfsd and mounts started to hang.

> 
> If rpcbind isn't running, which is arguably a very similar situation
> (no protocols are register) we have always had a long timeout. Why is
> "just one protocol not registered" any different?
ECONNREFUSED can also me the server is not up so we 
need to wait. 

> 
> 
> Anyway, below is the patch I was working on.  I stopped when I wasn't
> sure how to handle ECONNREFUSED.
I've quick took and it does look a little messy or we
revert the EOPNOTSUPP commit... 

But at least you know my motivation.

steved. 
> 
> NeilBrown
> 
> 
> 
> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
> index d5dfb5e4a669..084776115b9>>> I disagree with the "hang forever" description.  I just tested after
>>> disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
>>> before a failure was reported.  It might be longer when trying TCP on a
>>> server that only supports UDP.
>> Yeah I did not wait that long... You are much more of a patient man than I ;-) 
>> I do think this is a regression. Going an from an instant failure to one
>> that takes over 2min is not a good thing... IMHO.
>>f 100644
> --- a/utils/mount/stropts.c
> +++ b/utils/mount/stropts.c
> @@ -935,24 +935,30 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>   * failed so far, but fail immediately if there is a local
>   * error (like a bad mount option).
>   *
> - * ESTALE is also a temporary error because some servers
> - * return ESTALE when a share is temporarily offline.
> + * If there is a remote error, like ESTALE or RPC_PROGNOTREGISTERED
> + * then it is probably permanent, but there is a small chance
> + * the it is temporary can we caught the server at an awkward
> + * time during start-up.  A shorter timeout is best for such
> + * circumstances, so return a distinct status.
>   *
> - * Returns 1 if we should fail immediately, or 0 if we
> - * should retry.
> + * Returns PERMANENT if we should fail immediately,
> + * TEMPORARY if we should retry normally, or
> + * REMOTE if we should retry with shorter timeout.
>   */
> -static int nfs_is_permanent_error(int error)
> +enum error_type { PERMANENT, TEMPORARY, REMOTE };
> +static enum error_type nfs_error_type(int error)
>  {
>  	switch (error) {
>  	case ESTALE:
> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
> +		return REMOTE;
>  	case ETIMEDOUT:
>  	case ECONNREFUSED:
>  	case EHOSTUNREACH:
> -	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>  	case EAGAIN:
> -		return 0;	/* temporary */
> +		return TEMPORARY;
>  	default:
> -		return 1;	/* permanent */
> +		return PERMANENT;
>  	}
>  }
>  
> @@ -967,6 +973,7 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>  {
>  	unsigned int secs = 1;
>  	time_t timeout;
> +	int last_errno = 0;
>  
>  	timeout = nfs_parse_retry_option(mi->options,
>  					 NFS_DEF_FG_TIMEOUT_MINUTES);
> @@ -987,13 +994,22 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>  			 */
>  			return EX_SUCCESS;
>  
> -		if (nfs_is_permanent_error(errno))
> +		switch(nfs_error_type(errno)) {
> +		case PERMANENT:
> +			timeout = 0;
>  			break;
> -
> -		if (time(NULL) > timeout) {
> +		case REMOTE:
> +			if (errno == last_errno)
> +				timeout = 0;
> +			break;
> +		case TEMPORARY:
>  			errno = ETIMEDOUT;
>  			break;
>  		}
> +		last_errno = errno;
> +
> +		if (time(NULL) > timeout)
> +			break;
>  
>  		if (errno != ETIMEDOUT) {
>  			if (sleep(secs))
> @@ -1020,7 +1036,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>  	if (nfs_try_mount(mi))
>  		return EX_SUCCESS;
>  
> -	if (nfs_is_permanent_error(errno)) {
> +	if (nfs_error_type(errno) == PERMANENT) {
>  		mount_error(mi->spec, mi->node, errno);
>  		return EX_FAIL;
>  	}
> @@ -1055,8 +1071,14 @@ static int nfsmount_child(struct nfsmount_info *mi)
>  		if (nfs_try_mount(mi))
>  			return EX_SUCCESS;
>  
> -		if (nfs_is_permanent_error(errno))
> +		switch (nfs_error_type(errno)) {
> +		case REMOTE: /* Doesn't hurt to keep trying remote errors
> +			      * when in the background
> +			      */
> +		case PERMANENT:
> +			timeout = 0;
>  			break;
> +		}
>  
>  		if (time(NULL) > timeout)
>  			break;
> 

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 7077 bytes --]

On Tue, Nov 29 2016, Steve Dickson wrote:

> My apologies for the delayed response... I just saw this... 


Yeah, life happens.

>
> On 11/23/2016 06:26 PM, NeilBrown wrote:
>> On Thu, Nov 24 2016, Steve Dickson wrote:
>>>>
>>>> So I think the current behavior is correct.  You might be able to argue
>>>> that certain error codes should trigger a shorter timeout, but it would
>>>> need a strong argument.
>>> Going with the theory the window is very small, how about 
>>> a retry with a timeout then a failure? 
>> 
>> I started looking at changing the timeout and it wouldn't be too hard
>> (if we can agree on a suitable delay), but I feel I must ask why this is
>> important.
> Over the last few Connectathon and bakeathons I've been
> floating the idea of dismantling the UDP support and
> nobody to objected... The main reason is to cut the testing 
> matrix in half.

Dismantling?  As in: removing the code?  I wouldn't like that.
Certainly switch the default and even pop up a warning "UDP bad - use
TCP ok?".

>
> I keep getting these goofy UDP bugs from our QE guys that
> nobody is going to fix... and I have not seen a UDP bug
> from a customer in years.. I really don't think 
> it being used so why continue to support it?

Completely happy for distros to choose not to support it.  But there are
more users of Linux than we distro people hear from.

>
>> In what situation are you likely to mount with the wrong protocol, that
>> you aren't able to just Ctrl-C when you realized what a dumb thing you
>> just did?
> I turned off the UDP support in rpc.nfsd and mounts started to hang.

Mounts that would otherwise fail - correct?
Exactly the same behavior as if you disabled NFS service on the server.

I have difficulty seeing this as being a serious problem.  It gets
notices, someone does "s/udp/tcp/" on /etc/fstab.  Problem gone.


>
>> 
>> If rpcbind isn't running, which is arguably a very similar situation
>> (no protocols are register) we have always had a long timeout. Why is
>> "just one protocol not registered" any different?
> ECONNREFUSED can also me the server is not up so we 
> need to wait.

If there servers in not up you get EHOSTUNREACHABLE (or however it is
spelled) or a timeout.
We need to wait on RPC_PROGNOTREGISTERED for exactly the same reason
that we need to wait for ECONNREFUSED: the server might still be coming
up and hasn't quite got everything registered properly yet.

>
>> 
>> 
>> Anyway, below is the patch I was working on.  I stopped when I wasn't
>> sure how to handle ECONNREFUSED.
> I've quick took and it does look a little messy or we
> revert the EOPNOTSUPP commit...

The EOPNOTSUPP commit fixed a real bug.  The bug would result in a mount
attempt failing when it should succeed.  Reverting that is not an
option.
Your symptom is not so much a bug and an inconvenience.  You issue a
command that will fail, and it takes a bit longer to fail than you would
like.  You still get the correct end result.

I don't think the patch is messy ... or at least, the resulting code
isn't.
I'll have another go and submit a proper patch.

Thanks,
NeilBrown

>
> But at least you know my motivation.
>
> steved. 
>> 
>> NeilBrown
>> 
>> 
>> 
>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>> index d5dfb5e4a669..084776115b9>>> I disagree with the "hang forever" description.  I just tested after
>>>> disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
>>>> before a failure was reported.  It might be longer when trying TCP on a
>>>> server that only supports UDP.
>>> Yeah I did not wait that long... You are much more of a patient man than I ;-) 
>>> I do think this is a regression. Going an from an instant failure to one
>>> that takes over 2min is not a good thing... IMHO.
>>>f 100644
>> --- a/utils/mount/stropts.c
>> +++ b/utils/mount/stropts.c
>> @@ -935,24 +935,30 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>>   * failed so far, but fail immediately if there is a local
>>   * error (like a bad mount option).
>>   *
>> - * ESTALE is also a temporary error because some servers
>> - * return ESTALE when a share is temporarily offline.
>> + * If there is a remote error, like ESTALE or RPC_PROGNOTREGISTERED
>> + * then it is probably permanent, but there is a small chance
>> + * the it is temporary can we caught the server at an awkward
>> + * time during start-up.  A shorter timeout is best for such
>> + * circumstances, so return a distinct status.
>>   *
>> - * Returns 1 if we should fail immediately, or 0 if we
>> - * should retry.
>> + * Returns PERMANENT if we should fail immediately,
>> + * TEMPORARY if we should retry normally, or
>> + * REMOTE if we should retry with shorter timeout.
>>   */
>> -static int nfs_is_permanent_error(int error)
>> +enum error_type { PERMANENT, TEMPORARY, REMOTE };
>> +static enum error_type nfs_error_type(int error)
>>  {
>>  	switch (error) {
>>  	case ESTALE:
>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>> +		return REMOTE;
>>  	case ETIMEDOUT:
>>  	case ECONNREFUSED:
>>  	case EHOSTUNREACH:
>> -	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>>  	case EAGAIN:
>> -		return 0;	/* temporary */
>> +		return TEMPORARY;
>>  	default:
>> -		return 1;	/* permanent */
>> +		return PERMANENT;
>>  	}
>>  }
>>  
>> @@ -967,6 +973,7 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>>  {
>>  	unsigned int secs = 1;
>>  	time_t timeout;
>> +	int last_errno = 0;
>>  
>>  	timeout = nfs_parse_retry_option(mi->options,
>>  					 NFS_DEF_FG_TIMEOUT_MINUTES);
>> @@ -987,13 +994,22 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>>  			 */
>>  			return EX_SUCCESS;
>>  
>> -		if (nfs_is_permanent_error(errno))
>> +		switch(nfs_error_type(errno)) {
>> +		case PERMANENT:
>> +			timeout = 0;
>>  			break;
>> -
>> -		if (time(NULL) > timeout) {
>> +		case REMOTE:
>> +			if (errno == last_errno)
>> +				timeout = 0;
>> +			break;
>> +		case TEMPORARY:
>>  			errno = ETIMEDOUT;
>>  			break;
>>  		}
>> +		last_errno = errno;
>> +
>> +		if (time(NULL) > timeout)
>> +			break;
>>  
>>  		if (errno != ETIMEDOUT) {
>>  			if (sleep(secs))
>> @@ -1020,7 +1036,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>>  	if (nfs_try_mount(mi))
>>  		return EX_SUCCESS;
>>  
>> -	if (nfs_is_permanent_error(errno)) {
>> +	if (nfs_error_type(errno) == PERMANENT) {
>>  		mount_error(mi->spec, mi->node, errno);
>>  		return EX_FAIL;
>>  	}
>> @@ -1055,8 +1071,14 @@ static int nfsmount_child(struct nfsmount_info *mi)
>>  		if (nfs_try_mount(mi))
>>  			return EX_SUCCESS;
>>  
>> -		if (nfs_is_permanent_error(errno))
>> +		switch (nfs_error_type(errno)) {
>> +		case REMOTE: /* Doesn't hurt to keep trying remote errors
>> +			      * when in the background
>> +			      */
>> +		case PERMANENT:
>> +			timeout = 0;
>>  			break;
>> +		}
>>  
>>  		if (time(NULL) > timeout)
>>  			break;
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[Steve Dickson]

On 11/29/2016 04:36 PM, NeilBrown wrote:
> On Tue, Nov 29 2016, Steve Dickson wrote:
> 
>> My apologies for the delayed response... I just saw this... 
> 
> 
> Yeah, life happens.
> 
>>
>> On 11/23/2016 06:26 PM, NeilBrown wrote:
>>> On Thu, Nov 24 2016, Steve Dickson wrote:
>>>>>
>>>>> So I think the current behavior is correct.  You might be able to argue
>>>>> that certain error codes should trigger a shorter timeout, but it would
>>>>> need a strong argument.
>>>> Going with the theory the window is very small, how about 
>>>> a retry with a timeout then a failure? 
>>>
>>> I started looking at changing the timeout and it wouldn't be too hard
>>> (if we can agree on a suitable delay), but I feel I must ask why this is
>>> important.
>> Over the last few Connectathon and bakeathons I've been
>> floating the idea of dismantling the UDP support and
>> nobody to objected... The main reason is to cut the testing 
>> matrix in half.
> 
> Dismantling?  As in: removing the code?  I wouldn't like that.
No. Basically not to use the code. 

> Certainly switch the default and even pop up a warning "UDP bad - use
> TCP ok?".
Right... the first step on the client would be stop 
negotiating UDP mounts but still allow mount -o udp,v3 
to work, but not hang for 2.5 minutes before failing.

Then, I guess, not to have all the other daemons 
not to advertise UDP unless configured to do so,
but off by default. 

> 
>>
>> I keep getting these goofy UDP bugs from our QE guys that
>> nobody is going to fix... and I have not seen a UDP bug
>> from a customer in years.. I really don't think 
>> it being used so why continue to support it?
> 
> Completely happy for distros to choose not to support it.  But there are
> more users of Linux than we distro people hear from.
Fair enough... So leave the code in, but off by default
so it's not something that has to be tested. 

> 
>>
>>> In what situation are you likely to mount with the wrong protocol, that
>>> you aren't able to just Ctrl-C when you realized what a dumb thing you
>>> just did?
>> I turned off the UDP support in rpc.nfsd and mounts started to hang.
> 
> Mounts that would otherwise fail - correct?
> Exactly the same behavior as if you disabled NFS service on the server.
Yes, fail immediately which happen before commit df0b9998

> 
> I have difficulty seeing this as being a serious problem.  It gets
> notices, someone does "s/udp/tcp/" on /etc/fstab.  Problem gone.
I have not looked, but will it hang the reboot for a few minutes?

>>> If rpcbind isn't running, which is arguably a very similar situation
>>> (no protocols are register) we have always had a long timeout. Why is
>>> "just one protocol not registered" any different?
>> ECONNREFUSED can also me the server is not up so we 
>> need to wait.
> 
> If there servers in not up you get EHOSTUNREACHABLE (or however it is
> spelled) or a timeout.
> We need to wait on RPC_PROGNOTREGISTERED for exactly the same reason
> that we need to wait for ECONNREFUSED: the server might still be coming
> up and hasn't quite got everything registered properly yet.
Well when the server is down I'm seeing 
"mount.nfs: mount(2): Connection refused" on the v4 mount then mounts
then "RPC: Program not registered" on both the TCP and UDP v3 mounts.
And that cycles through for 2.5 mins until the failure.

So you are correct, there is no difference between when a server
is down and when a protocol is not supported. But this is where
we differ. A server could come up but a protocol is not all of
a sudden be supported. I think we should handle that case better. 
 
> 
>>
>>>
>>>
>>> Anyway, below is the patch I was working on.  I stopped when I wasn't
>>> sure how to handle ECONNREFUSED.
>> I've quick took and it does look a little messy or we
>> revert the EOPNOTSUPP commit...
> 
> The EOPNOTSUPP commit fixed a real bug.  The bug would result in a mount
> attempt failing when it should succeed.  Reverting that is not an
> option.
So the bug was that fact rpcbind was not up but the NFS server was?
Or both were on there way up?

> Your symptom is not so much a bug and an inconvenience.  You issue a
> command that will fail, and it takes a bit longer to fail than you would
> like.  You still get the correct end result.
No, I see it as a regression as how we used to handle a 
protocol not being supported.

> 
> I don't think the patch is messy ... or at least, the resulting code
> isn't.
> I'll have another go and submit a proper patch.
Thank you... I'm confident things will work out well.

steved.

> 
> Thanks,
> NeilBrown
> 
>>
>> But at least you know my motivation.
>>
>> steved. 
>>>
>>> NeilBrown
>>>
>>>
>>>
>>> diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
>>> index d5dfb5e4a669..084776115b9>>> I disagree with the "hang forever" description.  I just tested after
>>>>> disabling UDP on an nfs server, and the delay was 2 minutes, 5 seconds
>>>>> before a failure was reported.  It might be longer when trying TCP on a
>>>>> server that only supports UDP.
>>>> Yeah I did not wait that long... You are much more of a patient man than I ;-) 
>>>> I do think this is a regression. Going an from an instant failure to one
>>>> that takes over 2min is not a good thing... IMHO.
>>>> f 100644
>>> --- a/utils/mount/stropts.c
>>> +++ b/utils/mount/stropts.c
>>> @@ -935,24 +935,30 @@ static int nfs_try_mount(struct nfsmount_info *mi)
>>>   * failed so far, but fail immediately if there is a local
>>>   * error (like a bad mount option).
>>>   *
>>> - * ESTALE is also a temporary error because some servers
>>> - * return ESTALE when a share is temporarily offline.
>>> + * If there is a remote error, like ESTALE or RPC_PROGNOTREGISTERED
>>> + * then it is probably permanent, but there is a small chance
>>> + * the it is temporary can we caught the server at an awkward
>>> + * time during start-up.  A shorter timeout is best for such
>>> + * circumstances, so return a distinct status.
>>>   *
>>> - * Returns 1 if we should fail immediately, or 0 if we
>>> - * should retry.
>>> + * Returns PERMANENT if we should fail immediately,
>>> + * TEMPORARY if we should retry normally, or
>>> + * REMOTE if we should retry with shorter timeout.
>>>   */
>>> -static int nfs_is_permanent_error(int error)
>>> +enum error_type { PERMANENT, TEMPORARY, REMOTE };
>>> +static enum error_type nfs_error_type(int error)
>>>  {
>>>  	switch (error) {
>>>  	case ESTALE:
>>> +	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>>> +		return REMOTE;
>>>  	case ETIMEDOUT:
>>>  	case ECONNREFUSED:
>>>  	case EHOSTUNREACH:
>>> -	case EOPNOTSUPP:	/* aka RPC_PROGNOTREGISTERED */
>>>  	case EAGAIN:
>>> -		return 0;	/* temporary */
>>> +		return TEMPORARY;
>>>  	default:
>>> -		return 1;	/* permanent */
>>> +		return PERMANENT;
>>>  	}
>>>  }
>>>  
>>> @@ -967,6 +973,7 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>>>  {
>>>  	unsigned int secs = 1;
>>>  	time_t timeout;
>>> +	int last_errno = 0;
>>>  
>>>  	timeout = nfs_parse_retry_option(mi->options,
>>>  					 NFS_DEF_FG_TIMEOUT_MINUTES);
>>> @@ -987,13 +994,22 @@ static int nfsmount_fg(struct nfsmount_info *mi)
>>>  			 */
>>>  			return EX_SUCCESS;
>>>  
>>> -		if (nfs_is_permanent_error(errno))
>>> +		switch(nfs_error_type(errno)) {
>>> +		case PERMANENT:
>>> +			timeout = 0;
>>>  			break;
>>> -
>>> -		if (time(NULL) > timeout) {
>>> +		case REMOTE:
>>> +			if (errno == last_errno)
>>> +				timeout = 0;
>>> +			break;
>>> +		case TEMPORARY:
>>>  			errno = ETIMEDOUT;
>>>  			break;
>>>  		}
>>> +		last_errno = errno;
>>> +
>>> +		if (time(NULL) > timeout)
>>> +			break;
>>>  
>>>  		if (errno != ETIMEDOUT) {
>>>  			if (sleep(secs))
>>> @@ -1020,7 +1036,7 @@ static int nfsmount_parent(struct nfsmount_info *mi)
>>>  	if (nfs_try_mount(mi))
>>>  		return EX_SUCCESS;
>>>  
>>> -	if (nfs_is_permanent_error(errno)) {
>>> +	if (nfs_error_type(errno) == PERMANENT) {
>>>  		mount_error(mi->spec, mi->node, errno);
>>>  		return EX_FAIL;
>>>  	}
>>> @@ -1055,8 +1071,14 @@ static int nfsmount_child(struct nfsmount_info *mi)
>>>  		if (nfs_try_mount(mi))
>>>  			return EX_SUCCESS;
>>>  
>>> -		if (nfs_is_permanent_error(errno))
>>> +		switch (nfs_error_type(errno)) {
>>> +		case REMOTE: /* Doesn't hurt to keep trying remote errors
>>> +			      * when in the background
>>> +			      */
>>> +		case PERMANENT:
>>> +			timeout = 0;
>>>  			break;
>>> +		}
>>>  
>>>  		if (time(NULL) > timeout)
>>>  			break;
>>>

Re: [PATCH 2/2] mount: RPC_PROGNOTREGISTERED should not be a permanent error

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1293 bytes --]

On Wed, Nov 30 2016, Steve Dickson wrote:

> On 11/29/2016 04:36 PM, NeilBrown wrote:
>> 
>> Completely happy for distros to choose not to support it.  But there are
>> more users of Linux than we distro people hear from.
> Fair enough... So leave the code in, but off by default
> so it's not something that has to be tested.

Thanks for clarifying.  Sounds good to me.

>> 
>> The EOPNOTSUPP commit fixed a real bug.  The bug would result in a mount
>> attempt failing when it should succeed.  Reverting that is not an
>> option.
> So the bug was that fact rpcbind was not up but the NFS server was?
> Or both were on there way up?

You could say that the bug was that rpcbind starts before the NFS server
has registered... but as the NFS server *cannot* register before rpcbind
starts, that would be a little harsh.

RPC_PROGNOTREGISTERED is a very close analogy to ECONNREFUSED.
They both say "The server process isn't listening (yet)".  One at the
TCP/IP level, one at the SUNRPC level.

If we started all daemons before enabling the network interface, and if
we could register all RPC services before rpcbind responds to external
requests, then these windows could be closed.  Unfortunately our
forefathers didn't have that foresight.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]