[PATCH 2/3] Fix monotonic_elapsed.

[PATCH 2/3] Fix monotonic_elapsed.

[NeilBrown]

When automount probes multiple hosts to find the one which
responds most quickly, it currently ignores the nanoseconds.
This often makes the cost "0", which makes weights ineffective.

The cause is that monotonic_elapsed() casts tv_nsec to a
double *after* dividing by 1 billion, rather than before.

With this change, weights become effective for choosing
between hosts which respond in under one second.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 lib/rpc_subs.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/rpc_subs.c b/lib/rpc_subs.c
index 73097c9d4238..60ede9f8c085 100644
--- a/lib/rpc_subs.c
+++ b/lib/rpc_subs.c
@@ -1093,9 +1093,9 @@ double monotonic_elapsed(struct timespec start, struct timespec end)
 	double t1, t2;
 
 	t1 =  (double) start.tv_sec +
-		(double) (start.tv_nsec/(1000*1000*1000));
+		((double) start.tv_nsec/(1000*1000*1000));
 	t2 =  (double) end.tv_sec +
-		(double) (end.tv_nsec/(1000*1000*1000));
+		((double) end.tv_nsec/(1000*1000*1000));
 	return t2 - t1;
 }
 


--
To unsubscribe from this list: send the line "unsubscribe autofs" in

[PATCH 3/3] Makefiles.rules: remove 'samples' from SUBDIRS

[NeilBrown]

The rules in Makefile make it quite clear that
'samples' is not expected to be part of SUBDIRS.
e.g.
 - The rule for "clean" handles both $(SUBDIRS) *and* samples.
 - There are separate "install" and "install_samples" targets.

However SUBDIRS does contain 'samples'.  This means that
a simple "make; make install" will over-write your configuration files.

So remove 'samples' from SUBDIRS.  Note that it has been removed in
the past, but then got added back again.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 Makefile.rules |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile.rules b/Makefile.rules
index 0edf9bfe2194..267758e0f94c 100644
--- a/Makefile.rules
+++ b/Makefile.rules
@@ -3,7 +3,7 @@
 #
 
 # Root directory contents
-SUBDIRS = lib daemon modules man samples
+SUBDIRS = lib daemon modules man
 INCDIRS = include
 INCFILES = COPYING COPYRIGHT NEWS README* TODO Makefile Makefile.rules \
 	   Makefile.conf.in .version .autofs-* configure.in aclocal.m4 \


--
To unsubscribe from this list: send the line "unsubscribe autofs" in

[PATCH 0/3] Assorted autofs fixes

[NeilBrown]

Hi Ian,
 following are three fixes for the autofs patches.
 The first is a revised version of a patch were have been
 discussing that relates to use_hostname_for_mounts.
 Rather than comparing weight and path and hostname etc to see
 if two 'struct host' came from the same config-file entry,
 I thought it would be best to record and index number and
 just compare that.  Hopefully that makes it more obvious
 what is happening.

 The second fixes that monotonic_elapsed() problem we discussed at
 LCA.  Customer has confirmed that this fixes the problem.

 The third fixes a small Makefile bug

Thanks,
NeilBrown


---

NeilBrown (3):
      use_hostname_for_mounts shouldn't prevent selection among replicas
      Fix monotonic_elapsed.
      Makefiles.rules: remove 'samples' from SUBDIRS


 Makefile.rules       |    2 +-
 include/replicated.h |    3 ++-
 lib/rpc_subs.c       |    4 ++--
 modules/mount_nfs.c  |    2 +-
 modules/replicated.c |   35 ++++++++++++++++++++---------------
 5 files changed, 26 insertions(+), 20 deletions(-)

--
Signature

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

[PATCH 1/3] use_hostname_for_mounts shouldn't prevent selection among replicas

[NeilBrown]

If several replicas have been specified for a mount point,
and use_hostname_for_mount is set to "yes", the selection
between these replicas is currently disabled and the last in
the list is always chosen.

There is little point selecting between different addresses
for the one host in this case, but it is still worth
selecting between different hosts, particularly if different
weights have been specified.

This patch restores the "prune_host_list()" functionality
when use_hostname_for_mount is set, and modifies it slightly
so that only on IP address for any host:/path entry in the
config file is willl be successfully probed.  After a
success, further addresses from the same entry are skipped.
This is achieved by tracking an entry number ("ent_num") for
each 'struct host'.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 include/replicated.h |    3 ++-
 modules/mount_nfs.c  |    2 +-
 modules/replicated.c |   35 ++++++++++++++++++++---------------
 3 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/include/replicated.h b/include/replicated.h
index 69ab7800d96b..0f482d210066 100644
--- a/include/replicated.h
+++ b/include/replicated.h
@@ -57,6 +57,7 @@
 
 struct host {
 	char *name;
+	int ent_num;
 	struct sockaddr *addr;
 	size_t addr_len;
 	unsigned int rr;
@@ -70,7 +71,7 @@ struct host {
 };
 
 void seed_random(void);
-struct host *new_host(const char *, struct sockaddr *, size_t,
+struct host *new_host(const char *, int, struct sockaddr *, size_t,
 		      unsigned int, unsigned int, unsigned int);
 void free_host_list(struct host **);
 int parse_location(unsigned, struct host **, const char *, unsigned int);
diff --git a/modules/mount_nfs.c b/modules/mount_nfs.c
index bf712a9374ca..304bef29eec7 100644
--- a/modules/mount_nfs.c
+++ b/modules/mount_nfs.c
@@ -236,7 +236,7 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int
 	    (vers & NFS4_VERS_MASK) != 0 &&
 	    !(vers & UDP6_REQUESTED)) {
 		unsigned int v4_probe_ok = 0;
-		struct host *tmp = new_host(hosts->name,
+		struct host *tmp = new_host(hosts->name, 0,
 					    hosts->addr, hosts->addr_len,
 					    hosts->proximity,
 					    hosts->weight, hosts->options);
diff --git a/modules/replicated.c b/modules/replicated.c
index 3ac4c70f4062..c10efcd91deb 100644
--- a/modules/replicated.c
+++ b/modules/replicated.c
@@ -83,7 +83,7 @@ void seed_random(void)
 	return;
 }
 
-struct host *new_host(const char *name,
+struct host *new_host(const char *name, int ent_num,
 		      struct sockaddr *addr, size_t addr_len,
 		      unsigned int proximity, unsigned int weight,
 		      unsigned int options)
@@ -116,6 +116,7 @@ struct host *new_host(const char *name,
 	memset(new, 0, sizeof(struct host));
 
 	new->name = tmp1;
+	new->ent_num = ent_num;
 	new->addr_len = addr_len;
 	new->addr = tmp2;
 	new->proximity = proximity;
@@ -714,7 +715,7 @@ done:
 int prune_host_list(unsigned logopt, struct host **list,
 		    unsigned int vers, int port)
 {
-	struct host *this, *last, *first;
+	struct host *this, *last, *first, *prev;
 	struct host *new = NULL;
 	unsigned int proximity, selected_version = 0;
 	unsigned int v2_tcp_count, v3_tcp_count, v4_tcp_count;
@@ -726,12 +727,6 @@ int prune_host_list(unsigned logopt, struct host **list,
 	if (!*list)
 		return 0;
 
-	/* If we're using the host name then there's no point probing
-	 * avialability and respose time.
-	 */
-	if (defaults_use_hostname_for_mounts())
-		return 1;
-
 	/* Use closest hosts to choose NFS version */
 
 	first = *list;
@@ -877,11 +872,18 @@ int prune_host_list(unsigned logopt, struct host **list,
 
 	first = last;
 	this = first;
+	prev = NULL;
 	while (this) {
 		struct host *next = this->next;
 		if (!this->name) {
 			remove_host(list, this);
 			add_host(&new, this);
+		} else if (defaults_use_hostname_for_mounts() && prev &&
+			   prev->ent_num == this->ent_num) {
+			/* When we use the hostname to mount, there is no
+			 * point in probing every address it has, just one is
+			 * enough.  Skip the rest.
+			 */
 		} else {
 			status = get_supported_ver_and_cost(logopt, this,
 						selected_version, port);
@@ -889,6 +891,7 @@ int prune_host_list(unsigned logopt, struct host **list,
 				this->version = selected_version;
 				remove_host(list, this);
 				add_host(&new, this);
+				prev = this;
 			}
 		}
 		this = next;
@@ -901,7 +904,7 @@ int prune_host_list(unsigned logopt, struct host **list,
 }
 
 static int add_new_host(struct host **list,
-			const char *host, unsigned int weight,
+			const char *host, int ent_num, unsigned int weight,
 			struct addrinfo *host_addr,
 			unsigned int rr, unsigned int options)
 {
@@ -940,7 +943,7 @@ static int add_new_host(struct host **list,
 	else
 		return 0;
 
-	new = new_host(host, host_addr->ai_addr, addr_len, prx, weight, options);
+	new = new_host(host, ent_num, host_addr->ai_addr, addr_len, prx, weight, options);
 	if (!new)
 		return 0;
 
@@ -953,7 +956,7 @@ static int add_new_host(struct host **list,
 	return 1;
 }
 
-static int add_host_addrs(struct host **list, const char *host,
+static int add_host_addrs(struct host **list, const char *host, int ent_num,
 			  unsigned int weight, unsigned int options)
 {
 	struct addrinfo hints, *ni, *this;
@@ -988,7 +991,7 @@ static int add_host_addrs(struct host **list, const char *host,
 
 	this = ni;
 	while (this) {
-		ret = add_new_host(list, host, weight, this, 0, options);
+		ret = add_new_host(list, host, ent_num, weight, this, 0, options);
 		if (!ret)
 			break;
 		this = this->ai_next;
@@ -1027,7 +1030,7 @@ try_name:
 		rr++;
 	this = ni;
 	while (this) {
-		ret = add_new_host(list, host, weight, this, rr, options);
+		ret = add_new_host(list, host, ent_num, weight, this, rr, options);
 		if (!ret)
 			break;
 		this = this->ai_next;
@@ -1120,6 +1123,7 @@ int parse_location(unsigned logopt, struct host **hosts,
 {
 	char *str, *p, *delim;
 	unsigned int empty = 1;
+	int ent_num = 1;
 
 	if (!list)
 		return 0;
@@ -1177,7 +1181,7 @@ int parse_location(unsigned logopt, struct host **hosts,
 				}
 
 				if (p != delim) {
-					if (!add_host_addrs(hosts, p, weight, options)) {
+					if (!add_host_addrs(hosts, p, ent_num, weight, options)) {
 						if (empty) {
 							p = next;
 							continue;
@@ -1199,7 +1203,7 @@ int parse_location(unsigned logopt, struct host **hosts,
 				*delim = '\0';
 				next = delim + 1;
 
-				if (!add_host_addrs(hosts, p, weight, options)) {
+				if (!add_host_addrs(hosts, p, ent_num, weight, options)) {
 					p = next;
 					continue;
 				}
@@ -1213,6 +1217,7 @@ int parse_location(unsigned logopt, struct host **hosts,
 			return 0;
 		}
 
+		ent_num ++;
 		p = next;
 	}
 


--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 2/3] Fix monotonic_elapsed.

[Ian Kent]

On 29/01/18 15:19, NeilBrown wrote:
> When automount probes multiple hosts to find the one which
> responds most quickly, it currently ignores the nanoseconds.
> This often makes the cost "0", which makes weights ineffective.
> 
> The cause is that monotonic_elapsed() casts tv_nsec to a
> double *after* dividing by 1 billion, rather than before.
> 
> With this change, weights become effective for choosing
> between hosts which respond in under one second.

Yes, when I looked I saw this was part of the series to change
to using the monotonic clock in order to avoid problems with
discontinuous jumps in system time.

I completely missed this when I reviewed the series.

Thanks Neil.

Umm ... I don't see patches 1 and 3, maybe they got lost in the
ether or they'll arrive later .... 
 
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  lib/rpc_subs.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/rpc_subs.c b/lib/rpc_subs.c
> index 73097c9d4238..60ede9f8c085 100644
> --- a/lib/rpc_subs.c
> +++ b/lib/rpc_subs.c
> @@ -1093,9 +1093,9 @@ double monotonic_elapsed(struct timespec start, struct timespec end)
>  	double t1, t2;
>  
>  	t1 =  (double) start.tv_sec +
> -		(double) (start.tv_nsec/(1000*1000*1000));
> +		((double) start.tv_nsec/(1000*1000*1000));
>  	t2 =  (double) end.tv_sec +
> -		(double) (end.tv_nsec/(1000*1000*1000));
> +		((double) end.tv_nsec/(1000*1000*1000));
>  	return t2 - t1;
>  }
>  
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe autofs" in
> 

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 0/3] Assorted autofs fixes

[Ian Kent]

On 29/01/18 15:19, NeilBrown wrote:
> Hi Ian,
>  following are three fixes for the autofs patches.
>  The first is a revised version of a patch were have been
>  discussing that relates to use_hostname_for_mounts.
>  Rather than comparing weight and path and hostname etc to see
>  if two 'struct host' came from the same config-file entry,
>  I thought it would be best to record and index number and
>  just compare that.  Hopefully that makes it more obvious
>  what is happening.

Right, as I say, this was somewhat broken and what you're
recommending is an improvement although I think it doesn't
quite fix the possible problems either.

It's worth applying when it finally arrives.
  
> 
>  The second fixes that monotonic_elapsed() problem we discussed at
>  LCA.  Customer has confirmed that this fixes the problem.

Applied, it's in the queue.

> 
>  The third fixes a small Makefile bug

Yeah, the config files should be backed up but two installs
will cause problems.

Most distribution packaging will want to do that step separately
so it does look like a sensible thing to do.

This is also in my patch queue.

I'm going to need to post the patches for RFC and commit them
fairly soon and possibly another release, there's a deadlock
with the "automount -m" command that needs fixing in 5.1.4.

Thanks Neil.

> 
> Thanks,
> NeilBrown
> 
> 
> ---
> 
> NeilBrown (3):
>       use_hostname_for_mounts shouldn't prevent selection among replicas
>       Fix monotonic_elapsed.
>       Makefiles.rules: remove 'samples' from SUBDIRS
> 
> 
>  Makefile.rules       |    2 +-
>  include/replicated.h |    3 ++-
>  lib/rpc_subs.c       |    4 ++--
>  modules/mount_nfs.c  |    2 +-
>  modules/replicated.c |   35 ++++++++++++++++++++---------------
>  5 files changed, 26 insertions(+), 20 deletions(-)
> 
> --
> Signature
> 

--
To unsubscribe from this list: send the line "unsubscribe autofs" in

Re: [PATCH 1/3] use_hostname_for_mounts shouldn't prevent selection among replicas

[Ian Kent]

On 29/01/18 15:19, NeilBrown wrote:
> If several replicas have been specified for a mount point,
> and use_hostname_for_mount is set to "yes", the selection
> between these replicas is currently disabled and the last in
> the list is always chosen.
> 
> There is little point selecting between different addresses
> for the one host in this case, but it is still worth
> selecting between different hosts, particularly if different
> weights have been specified.
> 
> This patch restores the "prune_host_list()" functionality
> when use_hostname_for_mount is set, and modifies it slightly
> so that only on IP address for any host:/path entry in the
> config file is willl be successfully probed.  After a
> success, further addresses from the same entry are skipped.
> This is achieved by tracking an entry number ("ent_num") for
> each 'struct host'.

Got it now.
Thanks Neil.

> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
>  include/replicated.h |    3 ++-
>  modules/mount_nfs.c  |    2 +-
>  modules/replicated.c |   35 ++++++++++++++++++++---------------
>  3 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/include/replicated.h b/include/replicated.h
> index 69ab7800d96b..0f482d210066 100644
> --- a/include/replicated.h
> +++ b/include/replicated.h
> @@ -57,6 +57,7 @@
>  
>  struct host {
>  	char *name;
> +	int ent_num;
>  	struct sockaddr *addr;
>  	size_t addr_len;
>  	unsigned int rr;
> @@ -70,7 +71,7 @@ struct host {
>  };
>  
>  void seed_random(void);
> -struct host *new_host(const char *, struct sockaddr *, size_t,
> +struct host *new_host(const char *, int, struct sockaddr *, size_t,
>  		      unsigned int, unsigned int, unsigned int);
>  void free_host_list(struct host **);
>  int parse_location(unsigned, struct host **, const char *, unsigned int);
> diff --git a/modules/mount_nfs.c b/modules/mount_nfs.c
> index bf712a9374ca..304bef29eec7 100644
> --- a/modules/mount_nfs.c
> +++ b/modules/mount_nfs.c
> @@ -236,7 +236,7 @@ int mount_mount(struct autofs_point *ap, const char *root, const char *name, int
>  	    (vers & NFS4_VERS_MASK) != 0 &&
>  	    !(vers & UDP6_REQUESTED)) {
>  		unsigned int v4_probe_ok = 0;
> -		struct host *tmp = new_host(hosts->name,
> +		struct host *tmp = new_host(hosts->name, 0,
>  					    hosts->addr, hosts->addr_len,
>  					    hosts->proximity,
>  					    hosts->weight, hosts->options);
> diff --git a/modules/replicated.c b/modules/replicated.c
> index 3ac4c70f4062..c10efcd91deb 100644
> --- a/modules/replicated.c
> +++ b/modules/replicated.c
> @@ -83,7 +83,7 @@ void seed_random(void)
>  	return;
>  }
>  
> -struct host *new_host(const char *name,
> +struct host *new_host(const char *name, int ent_num,
>  		      struct sockaddr *addr, size_t addr_len,
>  		      unsigned int proximity, unsigned int weight,
>  		      unsigned int options)
> @@ -116,6 +116,7 @@ struct host *new_host(const char *name,
>  	memset(new, 0, sizeof(struct host));
>  
>  	new->name = tmp1;
> +	new->ent_num = ent_num;
>  	new->addr_len = addr_len;
>  	new->addr = tmp2;
>  	new->proximity = proximity;
> @@ -714,7 +715,7 @@ done:
>  int prune_host_list(unsigned logopt, struct host **list,
>  		    unsigned int vers, int port)
>  {
> -	struct host *this, *last, *first;
> +	struct host *this, *last, *first, *prev;
>  	struct host *new = NULL;
>  	unsigned int proximity, selected_version = 0;
>  	unsigned int v2_tcp_count, v3_tcp_count, v4_tcp_count;
> @@ -726,12 +727,6 @@ int prune_host_list(unsigned logopt, struct host **list,
>  	if (!*list)
>  		return 0;
>  
> -	/* If we're using the host name then there's no point probing
> -	 * avialability and respose time.
> -	 */
> -	if (defaults_use_hostname_for_mounts())
> -		return 1;
> -
>  	/* Use closest hosts to choose NFS version */
>  
>  	first = *list;
> @@ -877,11 +872,18 @@ int prune_host_list(unsigned logopt, struct host **list,
>  
>  	first = last;
>  	this = first;
> +	prev = NULL;
>  	while (this) {
>  		struct host *next = this->next;
>  		if (!this->name) {
>  			remove_host(list, this);
>  			add_host(&new, this);
> +		} else if (defaults_use_hostname_for_mounts() && prev &&
> +			   prev->ent_num == this->ent_num) {
> +			/* When we use the hostname to mount, there is no
> +			 * point in probing every address it has, just one is
> +			 * enough.  Skip the rest.
> +			 */
>  		} else {
>  			status = get_supported_ver_and_cost(logopt, this,
>  						selected_version, port);
> @@ -889,6 +891,7 @@ int prune_host_list(unsigned logopt, struct host **list,
>  				this->version = selected_version;
>  				remove_host(list, this);
>  				add_host(&new, this);
> +				prev = this;
>  			}
>  		}
>  		this = next;
> @@ -901,7 +904,7 @@ int prune_host_list(unsigned logopt, struct host **list,
>  }
>  
>  static int add_new_host(struct host **list,
> -			const char *host, unsigned int weight,
> +			const char *host, int ent_num, unsigned int weight,
>  			struct addrinfo *host_addr,
>  			unsigned int rr, unsigned int options)
>  {
> @@ -940,7 +943,7 @@ static int add_new_host(struct host **list,
>  	else
>  		return 0;
>  
> -	new = new_host(host, host_addr->ai_addr, addr_len, prx, weight, options);
> +	new = new_host(host, ent_num, host_addr->ai_addr, addr_len, prx, weight, options);
>  	if (!new)
>  		return 0;
>  
> @@ -953,7 +956,7 @@ static int add_new_host(struct host **list,
>  	return 1;
>  }
>  
> -static int add_host_addrs(struct host **list, const char *host,
> +static int add_host_addrs(struct host **list, const char *host, int ent_num,
>  			  unsigned int weight, unsigned int options)
>  {
>  	struct addrinfo hints, *ni, *this;
> @@ -988,7 +991,7 @@ static int add_host_addrs(struct host **list, const char *host,
>  
>  	this = ni;
>  	while (this) {
> -		ret = add_new_host(list, host, weight, this, 0, options);
> +		ret = add_new_host(list, host, ent_num, weight, this, 0, options);
>  		if (!ret)
>  			break;
>  		this = this->ai_next;
> @@ -1027,7 +1030,7 @@ try_name:
>  		rr++;
>  	this = ni;
>  	while (this) {
> -		ret = add_new_host(list, host, weight, this, rr, options);
> +		ret = add_new_host(list, host, ent_num, weight, this, rr, options);
>  		if (!ret)
>  			break;
>  		this = this->ai_next;
> @@ -1120,6 +1123,7 @@ int parse_location(unsigned logopt, struct host **hosts,
>  {
>  	char *str, *p, *delim;
>  	unsigned int empty = 1;
> +	int ent_num = 1;
>  
>  	if (!list)
>  		return 0;
> @@ -1177,7 +1181,7 @@ int parse_location(unsigned logopt, struct host **hosts,
>  				}
>  
>  				if (p != delim) {
> -					if (!add_host_addrs(hosts, p, weight, options)) {
> +					if (!add_host_addrs(hosts, p, ent_num, weight, options)) {
>  						if (empty) {
>  							p = next;
>  							continue;
> @@ -1199,7 +1203,7 @@ int parse_location(unsigned logopt, struct host **hosts,
>  				*delim = '\0';
>  				next = delim + 1;
>  
> -				if (!add_host_addrs(hosts, p, weight, options)) {
> +				if (!add_host_addrs(hosts, p, ent_num, weight, options)) {
>  					p = next;
>  					continue;
>  				}
> @@ -1213,6 +1217,7 @@ int parse_location(unsigned logopt, struct host **hosts,
>  			return 0;
>  		}
>  
> +		ent_num ++;
>  		p = next;
>  	}
>  
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe autofs" in