Re: [ANNOUNCE] autofs 5.1.2 release

[Ian Kent]

On 20/12/17 14:10, Ian Kent wrote:
> On 20/12/17 13:52, Ian Kent wrote:
>> On 20/12/17 11:29, NeilBrown wrote:
>>>
>>> Hi Ian,
>>>  I've been looking at:
>>>
>>>> - add configuration option to use fqdn in mounts.
>>>
>>> (commit 9aeef772604) because using this new option causes a regression.
>>> If you are using the "replicated server" functionality, then
>>>   use_hostname_for_mounts = yes
>>> completely disables it.
>>
>> Yes, that's not quite right.
>>
>> It disables the probe and proximity check for each distinct host
>> name used.
>>
>> Each of the entries in the list of hosts should still be
>> attempted and given that NFS ping is also now used in the NFS
>> mount module what's lost is the preferred ordering of the hosts
>> list.
>>
>>>
>>> This is caused by:
>>>
>>> diff --git a/modules/replicated.c b/modules/replicated.c
>>> index 32860d5fe245..8437f5f3d5b2 100644
>>> --- a/modules/replicated.c
>>> +++ b/modules/replicated.c
>>> @@ -667,6 +667,12 @@ 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 */
>>>
>>> My question is: why what this particular change made.
>>
>> It was a while ago but there were complains about using the IP
>> address for mounts. It was requested to provide a way to prevent
>> that and force the use of the host name in mounts.
>>
>>> Why can't prune_host_list() be allowed to do it's thing
>>> when use_hostname_for_mounts is set.
>>
>> We could if each host name resolved to a single IP address.
>>
>> I'd need to check that use_hostname_for_mounts doesn't get
>> in the road but the host struct should have ->rr set to true
>> if it has multiple addresses so changing it to work the way
>> your recommending shouldn't be hard. I think there's a couple
>> of places that would need to be checked.
>>
>> If the host does resolve to multiple addresses the situation
>> is different. There's no way to stop the actual mount from
>> trying an IP address that's not responding and proximity
>> doesn't make sense either again because every time a lookup
>> is done on the host name (eg. at mount time) the next address
>> in its list will be returned which can and usually is different
>> from what would have been checked.
>>
>>> I understand that it would be pointless choosing between
>>> the different interfaces of a multi-homed host, but there is still value
>>> in choosing between multiple distinct hosts.
>>>
>>> What, if anything, might go wrong if I simply reverse this chunk of the
>>> patch?
>>
>> You'll get IP addresses in the logs in certain cases but that
>> should be all.
>>
>> It would probably be better to ensure that the checks are done
>> if the host name resolves to a single IP address.
> 
> I think that should be "if the host names in the list each resolve
> to a single IP address", otherwise the round robin behavior would
> probably still get in the road.

I think maybe this is sufficient ....

autofs-5.1.4 - use proximity check if all host names are simple

From: Ian Kent <raven@xxxxxxxxxx>

Currently if the configuration option use_hostname_for_mounts is
set then the proximity calcualtion is not done for the list of
hosts.

But if each host name in the host list resolves to a single IP
address then performing the proximity check still makes sense.

Signed-off-by: Ian Kent <raven@xxxxxxxxxx>
---
 modules/replicated.c |   32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/modules/replicated.c b/modules/replicated.c
index 3ac4c70f..e5c2276d 100644
--- a/modules/replicated.c
+++ b/modules/replicated.c
@@ -711,6 +711,24 @@ done:
 	return 0;
 }
 
+static unsigned int is_hosts_list_simple(struct host *list)
+{
+	struct host *this = list;
+	unsigned int ret = 1;
+
+	while (this) {
+		struct host *next = this->next;
+
+		if (this->rr) {
+			ret = 0;
+			break;
+		}
+		this = next;
+	}
+
+	return ret;
+}
+
 int prune_host_list(unsigned logopt, struct host **list,
 		    unsigned int vers, int port)
 {
@@ -726,12 +744,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;
@@ -767,6 +779,14 @@ int prune_host_list(unsigned logopt, struct host **list,
 			return 1;
 	}
 
+	/* If we're using the host name then there's no point probing
+	 * avialability and respose time unless all host names in the
+	 * list each resolve to a single address.
+	 */
+	if (defaults_use_hostname_for_mounts() &&
+	    !is_hosts_list_simple(this))
+		return 1;
+
 	proximity = this->proximity;
 	while (this) {
 		struct host *next = this->next;