Re: [ANNOUNCE] autofs 5.1.2 release

[Ian Kent]

On 21/12/17 09:09, NeilBrown wrote:
> --------8<---------------
> Subject: use_hostname_for_mounts shouldn't prevent selection among replica
> 
> 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 interfaces on 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 once
> an IP address with a given proximity has been successfully probed,
> other IP address for the same host(weight):/path and proximity are ignored.
> 
> Signed-off-by: NeilBrown <neilb@xxxxxxxx>
> 
> diff --git a/modules/replicated.c b/modules/replicated.c
> index 3ac4c70f4062..16cf873513ff 100644
> --- a/modules/replicated.c
> +++ b/modules/replicated.c
> @@ -714,7 +714,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 +726,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 +871,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->proximity == this->proximity &&
> +			   strcmp(prev->name, this->name) == 0 &&
> +			   strcmp(prev->path, this->path) == 0 &&
> +			   prev->weight == this->weight) {
> +			/* No need to probe same host(weight):/path again */

Mmm ... so maybe I'm the one that's missing the point.

You are trying to eliminate multiple occurrences of list entries that
correspond to a specific host name entry from probing.

It might be sensible to add a "this->rr" following the
defaults_use_hostname_for_mounts() check to avoid the additional
checks when the host doesn't have additional addresses, particularly
the string comparison.

There's nothing stopping people from adding this same host name with a
different weight, even though that doesn't seem like a sensible thing
to do.

I'm not sure if this exposes mounting to problems that aren't already
present with the current implementation.

I'll think a little more about that case but at first glance the DNS
round robin problem of addresses referring to different devices is
still present, a possible false negative.

But that problem exits in the current implementation too as a round
robin lookup can just as easily return an address of a host that isn't
responding at mount time.....

>  		} else {
>  			status = get_supported_ver_and_cost(logopt, this,
>  						selected_version, port);
> @@ -889,6 +890,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;
>