diff mbox

[ANNOUNCE] autofs 5.1.2 release

Message ID 876092krew.fsf@notabene.neil.brown.name
State New, archived
Headers show

Commit Message

NeilBrown Dec. 20, 2017, 3:29 a.m. UTC
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.

This is caused by:

Why can't prune_host_list() be allowed to do it's thing
when use_hostname_for_mounts is set.
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?

Thanks,
NeilBrown

Comments

Ian Kent Dec. 20, 2017, 5:52 a.m. UTC | #1
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.

Ian
Ian Kent Dec. 20, 2017, 6:10 a.m. UTC | #2
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.

Ian
Ian Kent Dec. 20, 2017, 7:30 a.m. UTC | #3
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.

Mmm .... that's also not right.

An NFS ping is only done on failed local bind mount to check
the NFS server is running on the local machine.

So that availability check needs to be done at mount time if
the proximity check is not done ....

Ian
diff mbox

Patch

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.