[PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[NeilBrown]

Hi,
 here is a revision of a recent patch series.
 Thanks to Bruce's uncompromising review I have significantly reduced
 the changes to mountpoint handling to just that which is needed.

 If we assume the NFSv3 behavior is "correct" (because it has been
 that way for over a decade), the NFSv4 behavior differs
 particularly in that a mount request arrives as a lookup from an
 pseduo-root parent.  This is seen by rpc.mountd as an upcall through
 nfsd.export.
 The nfsd_export() handled was ignoring the mountpoint option, and
 that is incorrect.  So a patch in this series handles the option correctly.

 The changes to blocking and ESTALE return, and the monitoring of
 /proc/mounts have all been removed.

 It might be beneficial to change the kernel so that the reported
 mtime of V4ROOT directories is that last time any change was made to
 any V4ROOT export.  That might allow the client to see changes in
 exports more quickly.

 It might also be generally useful for mountd to monitor /proc/mounts
 (using e.g. select()) and flush the export cache shortly after any
 changes.  Again, this would allow the client to see changes more
 promptly.

 However I don't plan either of these immediately, which means they
 almost certainly won't happen unless the issue is raised again for
 some reason.

Thanks,
NeilBrown

---

NeilBrown (7):
      nfs.man: clarify effect of 'retry' option.
      mountd: remove the --exports-file option
      mount: don't treat temporary name resolution failure as permanent.
      mount: use a public address for IPv6 callback.
      mount: fix memory leak in v4root_add_parents
      mountd: allow alternate ttl to be specified for dump_to_cache.
      mountd: fail nfsd.export lookup for path to unmounted exportpoint


 utils/mount/network.c   |    5 ++++
 utils/mount/nfs.man     |   14 ++++++++++--
 utils/mount/stropts.c   |   54 +++++++++++++++++++++++++++--------------------
 utils/mountd/auth.c     |    5 +---
 utils/mountd/cache.c    |   36 ++++++++++++++++++++++++-------
 utils/mountd/mountd.c   |   11 +++-------
 utils/mountd/mountd.h   |    2 +-
 utils/mountd/mountd.man |    8 -------
 utils/mountd/v4root.c   |    6 +++--
 9 files changed, 84 insertions(+), 57 deletions(-)

--
Signature

[PATCH 1/7] nfs.man: clarify effect of 'retry' option.

[NeilBrown]

The total timeout for a "mount" attempt to a non-responsive server
will always be a multiple of the time a single mount attempt in the
kernel takes, which for TCP defaults to about 4 minutes.

The documentation for the "retry" option seems to suggest that this can be used
to set a maximum but it really sets a time after which to stop retrying.
The total can be as much as "retry" plus the time for a single attempt.

So clarify the documentation a bit, and also note that retrans
defaults are different for UDP and TCP:
   #define NFS_DEF_UDP_RETRANS	(3)
   #define NFS_DEF_TCP_RETRANS	(2)

Reported-by: Howard Guo<hguo@suse.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mount/nfs.man |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/utils/mount/nfs.man b/utils/mount/nfs.man
index e541cdc95cb1..a0f790a5961b 100644
--- a/utils/mount/nfs.man
+++ b/utils/mount/nfs.man
@@ -158,8 +158,8 @@ up to a maximum timeout length of 60 seconds.
 The number of times the NFS client retries a request before
 it attempts further recovery action. If the
 .B retrans
-option is not specified, the NFS client tries each request
-three times.
+option is not specified, the NFS client tries each UDP request
+three times and each TCP request twice.
 .IP
 The NFS client generates a "server not responding" message
 after
@@ -391,6 +391,16 @@ is 2 minutes, and the default value for background mounts is 10000 minutes
 If a value of zero is specified, the
 .BR mount (8)
 command exits immediately after the first failure.
+.IP
+Note that this only affects how many retries are made and doesn't
+affect the delay caused by each retry.  For UDP each retry takes the
+time determined by the
+.BR timeo
+and
+.BR retrans
+options, which by default will be about 7 seconds.  For TCP the
+default is 3 minutes, but system TCP connection timeouts will
+sometimes limit the timeout of each retransmission to around 2 minutes.
 .TP 1.5i
 .BI sec= flavors
 A colon-separated list of one or more security flavors to use for accessing

[PATCH 7/7] mountd: fail nfsd.export lookup for path to unmounted exportpoint

[NeilBrown]

If an export point should be mounted ("mountpoint" option set) but
isn't, then an attempt to mount using the MOUNT protocol for NFSv3
will fail and an attempt to access the filesystem using a pre-existing
filehandle will block because nfsd_fh wont tell the kernel about it.

However a lookup from the parent, as happens with an NFSv4 mount
request, will pass the name to nfsd_export(), and it doesn't check the
mointpoint option, and so exports the underlying (typically "/")
filesystem.

So change nfsd_export() to refused to export that exportpoint, but
instead to explictly say that it isn't exported.
This will cause an 'ls' in the parent pseudo-root directory to not show
the name and will cause a "mount" attempt which walks down through the
pseudo root to fail in the same way that it does with NFSv3.

An access from a pre-existing NFSv4 mount will still hang until the
filesystem is mounted, just like it does with NFSv3.

In order to be a bit more responsive to the filesystem getting mounted,
just a short timeout (1 minutes) on exports of missing "mountpoint"
exportpoints.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mountd/cache.c |   18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index 9cc270690d90..ca6c84f4d93d 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -1334,7 +1334,23 @@ static void nfsd_export(int f)
 	found = lookup_export(dom, path, ai);
 
 	if (found) {
-		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export, 0) < 0) {
+		char *mp = found->m_export.e_mountpoint;
+
+		if (mp && !*mp)
+			mp = found->m_export.e_path;
+		if (mp && !is_mountpoint(mp))
+			/* Exportpoint is not mounted, so tell kernel it is
+			 * not available.
+			 * This will cause it not to appear in the V4 Pseudo-root
+			 * and so a "mount" of this path will fail, just like with
+			 * V3.
+			 * And filehandle for this mountpoint from an earlier
+			 * mount will block in nfsd.fh lookup.
+			 */
+			dump_to_cache(f, buf, sizeof(buf), dom, path,
+				      NULL, 60);
+		else if (dump_to_cache(f, buf, sizeof(buf), dom, path,
+					 &found->m_export, 0) < 0) {
 			xlog(L_WARNING,
 			     "Cannot export %s, possibly unsupported filesystem"
 			     " or fsid= required", path);

Re: [PATCH 7/7] mountd: fail nfsd.export lookup for path to unmounted exportpoint

[J. Bruce Fields]

On Fri, Jul 29, 2016 at 03:03:36PM +1000, NeilBrown wrote:
> If an export point should be mounted ("mountpoint" option set) but
> isn't, then an attempt to mount using the MOUNT protocol for NFSv3
> will fail and an attempt to access the filesystem using a pre-existing
> filehandle will block because nfsd_fh wont tell the kernel about it.
> 
> However a lookup from the parent, as happens with an NFSv4 mount
> request, will pass the name to nfsd_export(), and it doesn't check the
> mointpoint option, and so exports the underlying (typically "/")
> filesystem.
> 
> So change nfsd_export() to refused to export that exportpoint, but
> instead to explictly say that it isn't exported.
> This will cause an 'ls' in the parent pseudo-root directory to not show
> the name and will cause a "mount" attempt which walks down through the
> pseudo root to fail in the same way that it does with NFSv3.
> 
> An access from a pre-existing NFSv4 mount will still hang until the
> filesystem is mounted, just like it does with NFSv3.
> 
> In order to be a bit more responsive to the filesystem getting mounted,
> just a short timeout (1 minutes) on exports of missing "mountpoint"
> exportpoints.

OK, I'm still haven't learned to love "mountpoint", but, I agree, it
looks like this fixes the complaint about the v4 case with minimal risk
of disruption to existing users.

ACK.--b.

> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  utils/mountd/cache.c |   18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index 9cc270690d90..ca6c84f4d93d 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -1334,7 +1334,23 @@ static void nfsd_export(int f)
>  	found = lookup_export(dom, path, ai);
>  
>  	if (found) {
> -		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export, 0) < 0) {
> +		char *mp = found->m_export.e_mountpoint;
> +
> +		if (mp && !*mp)
> +			mp = found->m_export.e_path;
> +		if (mp && !is_mountpoint(mp))
> +			/* Exportpoint is not mounted, so tell kernel it is
> +			 * not available.
> +			 * This will cause it not to appear in the V4 Pseudo-root
> +			 * and so a "mount" of this path will fail, just like with
> +			 * V3.
> +			 * And filehandle for this mountpoint from an earlier
> +			 * mount will block in nfsd.fh lookup.
> +			 */
> +			dump_to_cache(f, buf, sizeof(buf), dom, path,
> +				      NULL, 60);
> +		else if (dump_to_cache(f, buf, sizeof(buf), dom, path,
> +					 &found->m_export, 0) < 0) {
>  			xlog(L_WARNING,
>  			     "Cannot export %s, possibly unsupported filesystem"
>  			     " or fsid= required", path);
> 

[PATCH 4/7] mount: use a public address for IPv6 callback.

[NeilBrown]

If IPv6 address privacy is active, the "clientaddr" given to the server
will likely be a temporary address which will eventually expire, thus
breaking callback.
So ask for a public address to ensure continued service.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mount/network.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/utils/mount/network.c b/utils/mount/network.c
index 0d12613e86a4..7dceb2db3b93 100644
--- a/utils/mount/network.c
+++ b/utils/mount/network.c
@@ -38,6 +38,7 @@
 #include <sys/socket.h>
 #include <sys/wait.h>
 #include <sys/stat.h>
+#include <linux/in6.h>
 #include <netinet/in.h>
 #include <rpc/rpc.h>
 #include <rpc/pmap_prot.h>
@@ -1113,6 +1114,7 @@ static int nfs_ca_sockname(const struct sockaddr *sap, const socklen_t salen,
 		.sin6_addr		= IN6ADDR_ANY_INIT,
 	};
 	int sock, result = 0;
+	int val;
 
 	sock = socket(sap->sa_family, SOCK_DGRAM, IPPROTO_UDP);
 	if (sock < 0)
@@ -1124,6 +1126,9 @@ static int nfs_ca_sockname(const struct sockaddr *sap, const socklen_t salen,
 			goto out;
 		break;
 	case AF_INET6:
+		/* Make sure the call-back address is public/permanent */
+		val = IPV6_PREFER_SRC_PUBLIC;
+		setsockopt(sock, SOL_IPV6, IPV6_ADDR_PREFERENCES, &val, sizeof(val));
 		if (bind(sock, SAFE_SOCKADDR(&sin6), sizeof(sin6)) < 0)
 			goto out;
 		break;

[PATCH 5/7] mount: fix memory leak in v4root_add_parents

[NeilBrown]

If pseudofs_update failed, we weren't freeing 'path'.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mountd/v4root.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/utils/mountd/v4root.c b/utils/mountd/v4root.c
index d52172592823..f978f4ceef01 100644
--- a/utils/mountd/v4root.c
+++ b/utils/mountd/v4root.c
@@ -152,6 +152,7 @@ static int v4root_add_parents(nfs_export *exp)
 	char *hostname = exp->m_export.e_hostname;
 	char *path;
 	char *ptr;
+	int ret = 0;
 
 	path = strdup(exp->m_export.e_path);
 	if (!path) {
@@ -160,19 +161,18 @@ static int v4root_add_parents(nfs_export *exp)
 		return -ENOMEM;
 	}
 	for (ptr = path; ptr; ptr = strchr(ptr, '/')) {
-		int ret;
 		char saved;
 
 		saved = *ptr;
 		*ptr = '\0';
 		ret = pseudofs_update(hostname, *path ? path : "/", exp);
 		if (ret)
-			return ret;
+			break;
 		*ptr = saved;
 		ptr++;
 	}
 	free(path);
-	return 0;
+	return ret;
 }
 
 /*

[PATCH 3/7] mount: don't treat temporary name resolution failure as permanent.

[NeilBrown]

If getaddrinfo() returns EAI_AGAIN, we shouldn't just give up, but
should continue normal retries as the nameserver may be unavailable
for the same reason as the NFS server.

So move the getaddrinfo() call from nfs_validate_options() into
nfs_try_mounts() which is always called soon after, except in the
'remount' case when we don't want it anyway.

If EAI_AGAIN is returned, set errno to EAGAIN and allow this to be a
temporary failure.  Otherwise report error and set errno to EALREADY
so no further message is given.

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

diff --git a/utils/mount/stropts.c b/utils/mount/stropts.c
index d60b484ab960..9de6794c6177 100644
--- a/utils/mount/stropts.c
+++ b/utils/mount/stropts.c
@@ -84,6 +84,7 @@ struct nfsmount_info {
 				*type;		/* "nfs" or "nfs4" */
 	char			*hostname;	/* server's hostname */
 	struct addrinfo		*address;	/* server's addresses */
+	sa_family_t		family;		/* Address family */
 
 	struct mount_options	*options;	/* parsed mount options */
 	char			**extra_opts;	/* string for /etc/mtab */
@@ -371,39 +372,19 @@ static int nfs_set_version(struct nfsmount_info *mi)
  */
 static int nfs_validate_options(struct nfsmount_info *mi)
 {
-	struct addrinfo hint = {
-		.ai_protocol	= (int)IPPROTO_UDP,
-	};
-	sa_family_t family;
-	int error;
-
 	if (!nfs_parse_devname(mi->spec, &mi->hostname, NULL))
 		return 0;
 
-	if (!nfs_nfs_proto_family(mi->options, &family))
+	if (!nfs_nfs_proto_family(mi->options, &mi->family))
 		return 0;
 
 	/*
 	 * A remount is not going to be able to change the server's address,
 	 * nor should we try to resolve another address for the server as we
 	 * may end up with a different address.
+	 * A non-remount will set 'addr' from ->hostname
 	 */
-	if (mi->flags & MS_REMOUNT) {
-		po_remove_all(mi->options, "addr");
-	} else {
-		hint.ai_family = (int)family;
-		error = getaddrinfo(mi->hostname, NULL, &hint, &mi->address);
-		if (error != 0) {
-			nfs_error(_("%s: Failed to resolve server %s: %s"),
-				progname, mi->hostname, gai_strerror(error));
-			mi->address = NULL;
-			return 0;
-		}
-
-		if (!nfs_append_addr_option(mi->address->ai_addr,
-						mi->address->ai_addrlen, mi->options))
-			return 0;
-	}
+	po_remove_all(mi->options, "addr");
 
 	if (!nfs_set_version(mi))
 		return 0;
@@ -903,6 +884,32 @@ static int nfs_try_mount(struct nfsmount_info *mi)
 {
 	int result = 0;
 
+	if (mi->address == NULL) {
+		struct addrinfo hint = {
+			.ai_protocol	= (int)IPPROTO_UDP,
+		};
+		int error;
+		struct addrinfo *address;
+
+		hint.ai_family = (int)mi->family;
+		error = getaddrinfo(mi->hostname, NULL, &hint, &address);
+		if (error != 0) {
+			if (error == EAI_AGAIN)
+				errno = EAGAIN;
+			else {
+				nfs_error(_("%s: Failed to resolve server %s: %s"),
+					  progname, mi->hostname, gai_strerror(error));
+				errno = EALREADY;
+			}
+			return 0;
+		}
+
+		if (!nfs_append_addr_option(address->ai_addr,
+					    address->ai_addrlen, mi->options))
+			return 0;
+		mi->address = address;
+	}
+
 	switch (mi->version.major) {
 		case 2:
 		case 3:
@@ -941,6 +948,7 @@ static int nfs_is_permanent_error(int error)
 	case ETIMEDOUT:
 	case ECONNREFUSED:
 	case EHOSTUNREACH:
+	case EAGAIN:
 		return 0;	/* temporary */
 	default:
 		return 1;	/* permanent */

[PATCH 2/7] mountd: remove the --exports-file option

[NeilBrown]

It is completely ineffective.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mountd/auth.c     |    5 +----
 utils/mountd/mountd.c   |   11 +++--------
 utils/mountd/mountd.h   |    2 +-
 utils/mountd/mountd.man |    8 --------
 4 files changed, 5 insertions(+), 21 deletions(-)

diff --git a/utils/mountd/auth.c b/utils/mountd/auth.c
index 894a7a53957f..0881d9a6edba 100644
--- a/utils/mountd/auth.c
+++ b/utils/mountd/auth.c
@@ -36,7 +36,6 @@ enum auth_error
 };
 
 static void		auth_fixpath(char *path);
-static char	*export_file = NULL;
 static nfs_export my_exp;
 static nfs_client my_client;
 
@@ -44,10 +43,8 @@ extern int new_cache;
 extern int use_ipaddr;
 
 void
-auth_init(char *exports)
+auth_init(void)
 {
-
-	export_file = exports;
 	auth_reload();
 	xtab_mount_write();
 }
diff --git a/utils/mountd/mountd.c b/utils/mountd/mountd.c
index 063da269f895..7a51b093f66a 100644
--- a/utils/mountd/mountd.c
+++ b/utils/mountd/mountd.c
@@ -57,7 +57,6 @@ static struct option longopts[] =
 	{ "descriptors", 1, 0, 'o' },
 	{ "debug", 1, 0, 'd' },
 	{ "help", 0, 0, 'h' },
-	{ "exports-file", 1, 0, 'f' },
 	{ "nfs-version", 1, 0, 'V' },
 	{ "no-nfs-version", 1, 0, 'N' },
 	{ "version", 0, 0, 'v' },
@@ -689,7 +688,6 @@ get_exportlist(void)
 int
 main(int argc, char **argv)
 {
-	char	*export_file = _PATH_EXPORTS;
 	char    *state_dir = NFS_STATEDIR;
 	char	*progname;
 	unsigned int listeners = 0;
@@ -709,7 +707,7 @@ main(int argc, char **argv)
 
 	/* Parse the command line options and arguments. */
 	opterr = 0;
-	while ((c = getopt_long(argc, argv, "o:nFd:f:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
+	while ((c = getopt_long(argc, argv, "o:nFd:p:P:hH:N:V:vurs:t:g", longopts, NULL)) != EOF)
 		switch (c) {
 		case 'g':
 			manage_gids = 1;
@@ -728,9 +726,6 @@ main(int argc, char **argv)
 		case 'd':
 			xlog_sconfig(optarg, 1);
 			break;
-		case 'f':
-			export_file = optarg;
-			break;
 		case 'H': /* PRC: specify a high-availability callout program */
 			ha_callout_prog = optarg;
 			break;
@@ -862,7 +857,7 @@ main(int argc, char **argv)
 	sa.sa_handler = sig_hup;
 	sigaction(SIGHUP, &sa, NULL);
 
-	auth_init(export_file);
+	auth_init();
 
 	if (!foreground) {
 		/* We first fork off a child. */
@@ -908,7 +903,7 @@ usage(const char *prog, int n)
 {
 	fprintf(stderr,
 "Usage: %s [-F|--foreground] [-h|--help] [-v|--version] [-d kind|--debug kind]\n"
-"	[-o num|--descriptors num] [-f exports-file|--exports-file=file]\n"
+"	[-o num|--descriptors num]\n"
 "	[-p|--port port] [-V version|--nfs-version version]\n"
 "	[-N version|--no-nfs-version version] [-n|--no-tcp]\n"
 "	[-H prog |--ha-callout prog] [-r |--reverse-lookup]\n"
diff --git a/utils/mountd/mountd.h b/utils/mountd/mountd.h
index 6d358a75d9f3..f058f01d3584 100644
--- a/utils/mountd/mountd.h
+++ b/utils/mountd/mountd.h
@@ -39,7 +39,7 @@ bool_t		mount_pathconf_2_svc(struct svc_req *, dirpath *, ppathcnf *);
 bool_t		mount_mnt_3_svc(struct svc_req *, dirpath *, mountres3 *);
 
 void		mount_dispatch(struct svc_req *, SVCXPRT *);
-void		auth_init(char *export_file);
+void		auth_init(void);
 unsigned int	auth_reload(void);
 nfs_export *	auth_authenticate(const char *what,
 					const struct sockaddr *caller,
diff --git a/utils/mountd/mountd.man b/utils/mountd/mountd.man
index 66e3bba7e865..e0d1a0acba3a 100644
--- a/utils/mountd/mountd.man
+++ b/utils/mountd/mountd.man
@@ -86,14 +86,6 @@ Turn on debugging. Valid kinds are: all, auth, call, general and parse.
 .B \-F " or " \-\-foreground
 Run in foreground (do not daemonize)
 .TP
-.B \-f export-file " or " \-\-exports-file export-file
-This option specifies the exports file, listing the clients that this
-server is prepared to serve and parameters to apply to each
-such mount (see
-.BR exports (5)).
-By default, export information is read from
-.IR /etc/exports .
-.TP
 .B \-h " or " \-\-help
 Display usage message.
 .TP

[PATCH 6/7] mountd: allow alternate ttl to be specified for dump_to_cache.

[NeilBrown]

The default will not always be best.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 utils/mountd/cache.c |   20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
index ec86a22613cf..9cc270690d90 100644
--- a/utils/mountd/cache.c
+++ b/utils/mountd/cache.c
@@ -881,12 +881,16 @@ static void write_secinfo(char **bp, int *blen, struct exportent *ep, int flag_m
 
 }
 
-static int dump_to_cache(int f, char *buf, int buflen, char *domain, char *path, struct exportent *exp)
+static int dump_to_cache(int f, char *buf, int buflen, char *domain,
+			 char *path, struct exportent *exp, int ttl)
 {
 	char *bp = buf;
 	int blen = buflen;
 	time_t now = time(0);
 
+	if (ttl <= 1)
+		ttl = DEFAULT_TTL;
+
 	qword_add(&bp, &blen, domain);
 	qword_add(&bp, &blen, path);
 	if (exp) {
@@ -913,7 +917,7 @@ static int dump_to_cache(int f, char *buf, int buflen, char *domain, char *path,
 			qword_addhex(&bp, &blen, u, 16);
 		}
 	} else
-		qword_adduint(&bp, &blen, now + DEFAULT_TTL);
+		qword_adduint(&bp, &blen, now + ttl);
 	qword_addeol(&bp, &blen);
 	if (blen <= 0) return -1;
 	if (write(f, buf, bp - buf) != bp - buf) return -1;
@@ -1273,7 +1277,7 @@ static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path
 	struct exportent *eep;
 
 	eep = lookup_junction(dom, path, ai);
-	dump_to_cache(f, buf, buflen, dom, path, eep);
+	dump_to_cache(f, buf, buflen, dom, path, eep, 0);
 	if (eep == NULL)
 		return;
 	exportent_release(eep);
@@ -1283,7 +1287,7 @@ static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path
 static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path,
 		struct addrinfo *UNUSED(ai))
 {
-	dump_to_cache(f, buf, buflen, dom, path, NULL);
+	dump_to_cache(f, buf, buflen, dom, path, NULL, 0);
 }
 #endif	/* !HAVE_NFS_PLUGIN_H */
 
@@ -1330,11 +1334,11 @@ static void nfsd_export(int f)
 	found = lookup_export(dom, path, ai);
 
 	if (found) {
-		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export) < 0) {
+		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export, 0) < 0) {
 			xlog(L_WARNING,
 			     "Cannot export %s, possibly unsupported filesystem"
 			     " or fsid= required", path);
-			dump_to_cache(f, buf, sizeof(buf), dom, path, NULL);
+			dump_to_cache(f, buf, sizeof(buf), dom, path, NULL, 0);
 		}
 	} else
 		lookup_nonexport(f, buf, sizeof(buf), dom, path, ai);
@@ -1423,7 +1427,7 @@ static int cache_export_ent(char *buf, int buflen, char *domain, struct exporten
 	f = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY);
 	if (f < 0) return -1;
 
-	err = dump_to_cache(f, buf, buflen, domain, exp->e_path, exp);
+	err = dump_to_cache(f, buf, buflen, domain, exp->e_path, exp, 0);
 	if (err) {
 		xlog(L_WARNING,
 		     "Cannot export %s, possibly unsupported filesystem or"
@@ -1464,7 +1468,7 @@ static int cache_export_ent(char *buf, int buflen, char *domain, struct exporten
 				continue;
 			dev = stb.st_dev;
 			path[l] = 0;
-			dump_to_cache(f, buf, buflen, domain, path, exp);
+			dump_to_cache(f, buf, buflen, domain, path, exp, 0);
 			path[l] = c;
 		}
 		break;

Re: [PATCH 6/7] mountd: allow alternate ttl to be specified for dump_to_cache.

[Steve Dickson]

On 07/29/2016 01:03 AM, NeilBrown wrote:
> The default will not always be best.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  utils/mountd/cache.c |   20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/utils/mountd/cache.c b/utils/mountd/cache.c
> index ec86a22613cf..9cc270690d90 100644
> --- a/utils/mountd/cache.c
> +++ b/utils/mountd/cache.c
> @@ -881,12 +881,16 @@ static void write_secinfo(char **bp, int *blen, struct exportent *ep, int flag_m
>  
>  }
>  
> -static int dump_to_cache(int f, char *buf, int buflen, char *domain, char *path, struct exportent *exp)
> +static int dump_to_cache(int f, char *buf, int buflen, char *domain,
> +			 char *path, struct exportent *exp, int ttl)
>  {
>  	char *bp = buf;
>  	int blen = buflen;
>  	time_t now = time(0);
>  
> +	if (ttl <= 1)
> +		ttl = DEFAULT_TTL;
> +
>  	qword_add(&bp, &blen, domain);
>  	qword_add(&bp, &blen, path);
>  	if (exp) {
> @@ -913,7 +917,7 @@ static int dump_to_cache(int f, char *buf, int buflen, char *domain, char *path,
>  			qword_addhex(&bp, &blen, u, 16);
>  		}
>  	} else
> -		qword_adduint(&bp, &blen, now + DEFAULT_TTL);
> +		qword_adduint(&bp, &blen, now + ttl);
>  	qword_addeol(&bp, &blen);
>  	if (blen <= 0) return -1;
>  	if (write(f, buf, bp - buf) != bp - buf) return -1;
> @@ -1273,7 +1277,7 @@ static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path
>  	struct exportent *eep;
>  
>  	eep = lookup_junction(dom, path, ai);
> -	dump_to_cache(f, buf, buflen, dom, path, eep);
> +	dump_to_cache(f, buf, buflen, dom, path, eep, 0);
>  	if (eep == NULL)
>  		return;
>  	exportent_release(eep);
> @@ -1283,7 +1287,7 @@ static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path
>  static void lookup_nonexport(int f, char *buf, int buflen, char *dom, char *path,
>  		struct addrinfo *UNUSED(ai))
>  {
> -	dump_to_cache(f, buf, buflen, dom, path, NULL);
> +	dump_to_cache(f, buf, buflen, dom, path, NULL, 0);
>  }
>  #endif	/* !HAVE_NFS_PLUGIN_H */
>  
> @@ -1330,11 +1334,11 @@ static void nfsd_export(int f)
>  	found = lookup_export(dom, path, ai);
>  
>  	if (found) {
> -		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export) < 0) {
> +		if (dump_to_cache(f, buf, sizeof(buf), dom, path, &found->m_export, 0) < 0) {
>  			xlog(L_WARNING,
>  			     "Cannot export %s, possibly unsupported filesystem"
>  			     " or fsid= required", path);
> -			dump_to_cache(f, buf, sizeof(buf), dom, path, NULL);
> +			dump_to_cache(f, buf, sizeof(buf), dom, path, NULL, 0);
>  		}
>  	} else
>  		lookup_nonexport(f, buf, sizeof(buf), dom, path, ai);
> @@ -1423,7 +1427,7 @@ static int cache_export_ent(char *buf, int buflen, char *domain, struct exporten
>  	f = open("/proc/net/rpc/nfsd.export/channel", O_WRONLY);
>  	if (f < 0) return -1;
>  
> -	err = dump_to_cache(f, buf, buflen, domain, exp->e_path, exp);
> +	err = dump_to_cache(f, buf, buflen, domain, exp->e_path, exp, 0);
>  	if (err) {
>  		xlog(L_WARNING,
>  		     "Cannot export %s, possibly unsupported filesystem or"
> @@ -1464,7 +1468,7 @@ static int cache_export_ent(char *buf, int buflen, char *domain, struct exporten
>  				continue;
>  			dev = stb.st_dev;
>  			path[l] = 0;
> -			dump_to_cache(f, buf, buflen, domain, path, exp);
> +			dump_to_cache(f, buf, buflen, domain, path, exp, 0);
>  			path[l] = c;
>  		}
>  		break;
> 
> 
Hey Neil,

This one is a bit confusing... Unless I'm missing something this
patch enables the passing of the TTL in to dump_to_cache() but
a zero is always passed in which causes the default TTL to
be used... So I guess my question is... what's the point?
Is there an upcoming patch that will  actually passing a 
non-default TTL

steved.

Re: [PATCH 6/7] mountd: allow alternate ttl to be specified for dump_to_cache.

[Steve Dickson]

On 08/03/2016 01:31 PM, Steve Dickson wrote:
> Hey Neil,
> 
> This one is a bit confusing... Unless I'm missing something this
> patch enables the passing of the TTL in to dump_to_cache() but
> a zero is always passed in which causes the default TTL to
> be used... So I guess my question is... what's the point?
> Is there an upcoming patch that will  actually passing a 
> non-default TTL

Never mind... I see how its used in patch 7..

Sorry for the noise.

steved.

Re: [PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[J. Bruce Fields]

ACK to the series.

On Fri, Jul 29, 2016 at 03:03:36PM +1000, NeilBrown wrote:
> Hi,
>  here is a revision of a recent patch series.
>  Thanks to Bruce's uncompromising review I have significantly reduced
>  the changes to mountpoint handling to just that which is needed.
> 
>  If we assume the NFSv3 behavior is "correct" (because it has been
>  that way for over a decade), the NFSv4 behavior differs
>  particularly in that a mount request arrives as a lookup from an
>  pseduo-root parent.  This is seen by rpc.mountd as an upcall through
>  nfsd.export.
>  The nfsd_export() handled was ignoring the mountpoint option, and
>  that is incorrect.  So a patch in this series handles the option correctly.
> 
>  The changes to blocking and ESTALE return, and the monitoring of
>  /proc/mounts have all been removed.
> 
>  It might be beneficial to change the kernel so that the reported
>  mtime of V4ROOT directories is that last time any change was made to
>  any V4ROOT export.  That might allow the client to see changes in
>  exports more quickly.
> 
>  It might also be generally useful for mountd to monitor /proc/mounts
>  (using e.g. select()) and flush the export cache shortly after any
>  changes.  Again, this would allow the client to see changes more
>  promptly.
> 
>  However I don't plan either of these immediately, which means they
>  almost certainly won't happen unless the issue is raised again for
>  some reason.

Makes sense, thanks.--b.

> 
> Thanks,
> NeilBrown
> 
> ---
> 
> NeilBrown (7):
>       nfs.man: clarify effect of 'retry' option.
>       mountd: remove the --exports-file option
>       mount: don't treat temporary name resolution failure as permanent.
>       mount: use a public address for IPv6 callback.
>       mount: fix memory leak in v4root_add_parents
>       mountd: allow alternate ttl to be specified for dump_to_cache.
>       mountd: fail nfsd.export lookup for path to unmounted exportpoint
> 
> 
>  utils/mount/network.c   |    5 ++++
>  utils/mount/nfs.man     |   14 ++++++++++--
>  utils/mount/stropts.c   |   54 +++++++++++++++++++++++++++--------------------
>  utils/mountd/auth.c     |    5 +---
>  utils/mountd/cache.c    |   36 ++++++++++++++++++++++++-------
>  utils/mountd/mountd.c   |   11 +++-------
>  utils/mountd/mountd.h   |    2 +-
>  utils/mountd/mountd.man |    8 -------
>  utils/mountd/v4root.c   |    6 +++--
>  9 files changed, 84 insertions(+), 57 deletions(-)
> 
> --
> Signature

Re: [PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[Steve Dickson]

On 07/29/2016 01:03 AM, NeilBrown wrote:
> Hi,
>  here is a revision of a recent patch series.
>  Thanks to Bruce's uncompromising review I have significantly reduced
>  the changes to mountpoint handling to just that which is needed.
> 
>  If we assume the NFSv3 behavior is "correct" (because it has been
>  that way for over a decade), the NFSv4 behavior differs
>  particularly in that a mount request arrives as a lookup from an
>  pseduo-root parent.  This is seen by rpc.mountd as an upcall through
>  nfsd.export.
>  The nfsd_export() handled was ignoring the mountpoint option, and
>  that is incorrect.  So a patch in this series handles the option correctly.
> 
>  The changes to blocking and ESTALE return, and the monitoring of
>  /proc/mounts have all been removed.
> 
>  It might be beneficial to change the kernel so that the reported
>  mtime of V4ROOT directories is that last time any change was made to
>  any V4ROOT export.  That might allow the client to see changes in
>  exports more quickly.
> 
>  It might also be generally useful for mountd to monitor /proc/mounts
>  (using e.g. select()) and flush the export cache shortly after any
>  changes.  Again, this would allow the client to see changes more
>  promptly.
> 
>  However I don't plan either of these immediately, which means they
>  almost certainly won't happen unless the issue is raised again for
>  some reason.
> 
> Thanks,
> NeilBrown
> 
> ---
> 
> NeilBrown (7):
>       nfs.man: clarify effect of 'retry' option.
>       mountd: remove the --exports-file option
>       mount: don't treat temporary name resolution failure as permanent.
>       mount: use a public address for IPv6 callback.
>       mount: fix memory leak in v4root_add_parents
>       mountd: allow alternate ttl to be specified for dump_to_cache.
>       mountd: fail nfsd.export lookup for path to unmounted exportpoint
The patch series committed... 

steved.

> 
> 
>  utils/mount/network.c   |    5 ++++
>  utils/mount/nfs.man     |   14 ++++++++++--
>  utils/mount/stropts.c   |   54 +++++++++++++++++++++++++++--------------------
>  utils/mountd/auth.c     |    5 +---
>  utils/mountd/cache.c    |   36 ++++++++++++++++++++++++-------
>  utils/mountd/mountd.c   |   11 +++-------
>  utils/mountd/mountd.h   |    2 +-
>  utils/mountd/mountd.man |    8 -------
>  utils/mountd/v4root.c   |    6 +++--
>  9 files changed, 84 insertions(+), 57 deletions(-)
> 
> --
> Signature
> 

Re: [PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[NeilBrown]

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

On Thu, Aug 04 2016, Steve Dickson <SteveD@redhat.com> wrote:
>> 
>> NeilBrown (7):
>>       nfs.man: clarify effect of 'retry' option.
>>       mountd: remove the --exports-file option
>>       mount: don't treat temporary name resolution failure as permanent.
>>       mount: use a public address for IPv6 callback.
>>       mount: fix memory leak in v4root_add_parents
>>       mountd: allow alternate ttl to be specified for dump_to_cache.
>>       mountd: fail nfsd.export lookup for path to unmounted exportpoint
> The patch series committed... 

Thanks!  Have you any idea when 1.3.4 might be released.  I want
several bits for openSUSE and I'm wondering whether to take them
piece-meal, or to wait for a new release.

Thanks,
NeilBrown

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

Re: [PATCH 0/7] Assorted mount/mountd nfs-utils patches - V2

[Steve Dickson]

On 08/04/2016 08:06 AM, NeilBrown wrote:
> On Thu, Aug 04 2016, Steve Dickson <SteveD@redhat.com> wrote:
>>>
>>> NeilBrown (7):
>>>       nfs.man: clarify effect of 'retry' option.
>>>       mountd: remove the --exports-file option
>>>       mount: don't treat temporary name resolution failure as permanent.
>>>       mount: use a public address for IPv6 callback.
>>>       mount: fix memory leak in v4root_add_parents
>>>       mountd: allow alternate ttl to be specified for dump_to_cache.
>>>       mountd: fail nfsd.export lookup for path to unmounted exportpoint
>> The patch series committed... 
> 
> Thanks!  Have you any idea when 1.3.4 might be released.  I want
> several bits for openSUSE and I'm wondering whether to take them
> piece-meal, or to wait for a new release.
I plan doing that this weekend... 

steved.