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;
>
