Re: [PATCH 3/3] autofs - fix AT_NO_AUTOMOUNT not being honored

[NeilBrown]

On Thu, Nov 23 2017, Ian Kent wrote:

> On 23/11/17 08:47, NeilBrown wrote:
>> On Wed, Nov 22 2017, Ian Kent wrote:
>> 
>>> On 21/11/17 09:53, NeilBrown wrote:
>>>> On Wed, May 10 2017, Ian Kent wrote:
>>>>
>>>>> The fstatat(2) and statx() calls can pass the flag AT_NO_AUTOMOUNT
>>>>> which is meant to clear the LOOKUP_AUTOMOUNT flag and prevent triggering
>>>>> of an automount by the call. But this flag is unconditionally cleared
>>>>> for all stat family system calls except statx().
>>>>>
>>>>> stat family system calls have always triggered mount requests for the
>>>>> negative dentry case in follow_automount() which is intended but prevents
>>>>> the fstatat(2) and statx() AT_NO_AUTOMOUNT case from being handled.
>>>>>
>>>>> In order to handle the AT_NO_AUTOMOUNT for both system calls the
>>>>> negative dentry case in follow_automount() needs to be changed to
>>>>> return ENOENT when the LOOKUP_AUTOMOUNT flag is clear (and the other
>>>>> required flags are clear).
>>>>>
>>>>> AFAICT this change doesn't have any noticable side effects and may,
>>>>> in some use cases (although I didn't see it in testing) prevent
>>>>> unnecessary callbacks to the automount daemon.
>>>>
>>>> Actually, this patch does have a noticeable side effect.
>>>
>>> That's right.
>>>
>>>>
>>>> Previously, if /home were an indirect mount point so that /home/neilb
>>>> would mount my home directory, "ls -l /home/neilb" would always work.
>>>> Now it doesn't.
>>>
>>> An this is correct too.
>>>
>>> The previous policy was that a ->lookup() would always cause a mount to
>>> occur. It's that policy that prevents the AT_NO_AUTOMOUNT flag from being
>>> able to work consistently.
>> 
>> Just to be clear, the previous policy was:
>>  kernel - a lookup would cause a message to be sent to the automount daemon
>>  daemon - the receipt of a message would cause a mount to occur.
>> 
>> Either part of this policy could be changed to allow AT_NO_AUTOMOUNT to
>> work reliably.  You chose to change the first.  At the time I thought
>> this was a good idea.  I no longer think so.
>> 
>> Specifically, I think the second part of the policy should be revised a little.
>> 
>>>
>>> If you set the indirect mount "browse" option that will cause the mount
>>> point directories for each of the map keys to be pre-created. So a
>>> directory listing will show the mount points and an attempt to access
>>> anything within the mount point directory will cause it to be mounted.
>>>
>>> There is still the problem of not knowing map keys when the wildcard
>>> map entry is used.
>>>
>>> Still, stat family systems calls have always had similar problems that
>>> prevent consistent behavior.
>> 
>> Yes, I understand all this.  stat family sys-call have some odd
>> behaviours at times like "stat(); open(); fstat()" will result in
>> different sets of status info.  This is known.
>> The point is that these odd behaviours have changed in a noticeably way
>> (contrary to the change log comment) and it isn't clear these changes
>> are good.
>> 
>>>
>>>>
>>>> This happens because "ls" calls 'lstat' on the path and when that fails
>>>> with "ENOENT", it doesn't bother trying to open.
>>>
>>> I know, I believe the ENOENT is appropriate because there is no mount
>>> and no directory at the time this happens.
>> 
>> Two distinct statements here.  "no mount" and "no directory".
>> If AT_NO_AUTOMOUNT is given (or implicit) the "no mount" is significant
>> and shouldn't be changed.   It isn't clear that "no directory" is
>> significant.
>> If you think of the list of names stored in the autofs filesystem as a
>> cache of recently used names from the map, then the directory *does*
>> exist (in the map), it is just that the (in-kernel) cache hasn't been
>> populated yet.
>> Should "AT_NO_AUTOMOUNT" prevent the cache from being populated?
>> 
>>>
>>>>
>>>> An alternate approach to the problem that this patch addresses is to
>>>> *not* change follow_automount() but instead change the automount daemon
>>>> and possibly autofs.
>>>
>>> Perhaps, but the daemon probably doesn't have enough information to
>>> make decisions about this so there would need to be something coming
>>> from the kernel too.
>> 
>> I don't think so.
>> The daemon already has a promise that upcalls for a given name are
>> serialized, and it has the ability to test if a name is already in the
>> cache.  This is enough.
>> I applied the following patch:
>> 
>> diff --git a/modules/mount_nfs.c b/modules/mount_nfs.c
>> index c558a7381054..7ddfe9c61038 100644
>> --- a/modules/mount_nfs.c
>> +++ b/modules/mount_nfs.c
>> @@ -269,6 +269,11 @@ dont_probe:
>>  		free_host_list(&hosts);
>>  		return 1;
>>  	}
>> +	if (!status) {
>> +		debug(ap->logopt, MODPREFIX "created dir, that'll do for now");
>> +		free_host_list(&hosts);
>> +		return 0;
>> +	}
>>  
>>  	if (!status)
>>  		existed = 0;
>> 
>> to automount and now it behaves (for NFS mounts) how I would like (though
>> there is room for improvement).
>> 
>>>
>>>>
>>>> When a notification of access for an indirect mount point is received,
>>>> it would firstly just create the directory - not triggering a mount.
>>>> If another notification is then received (after the directory has been
>>>> created), it then triggers the mount.
>>>
>>> Not sure, perhaps.
>>>
>>> But I don't think that will work, I've had many problems with this type
>>> behavior due to bugs and I think the the same sort of problems would be
>>> encountered.
>>>
>>> The indirect mount "browse" option which behaves very much like what your
>>> suggesting is the internal program default, and has been since the initial
>>> v5 release. Historically it is disabled on install to maintain backward
>>> compatibility with the original Linux autofs (which is also the reason for
>>> always triggering an automount on ->lookup()).
>>>
>>> Perhaps now is the time for that to change.
>> 
>> Enabling browse mode does resolve this problem when the map is
>> enumerable (as you say, wildcards can't be supported).
>> But browse mode isn't always wanted.  If you have a very large map, then
>> caching all 10,000 entries in the kernel may be a pointless waste of
>> time and space.
>
> Indeed, that's the main use of nobrowse indirect maps.
>
> In fact another recent change where I moved the last_used field so it
> wouldn't be updated on every walk, to help with mounts never expiring,
> also needs at different approach.
>
> Doing that causes more aggressive expiring of automounts which increases
> umount to mount churn and leads to increased server load at sites with a
> very large number of clients.
>  
>> 
>>>
>>>>
>>>> I suspect this might need changes to autofs to avoid races when two
>>>> threads call lstat() on the same path at the same time.  We would need
>>>> to ensure that automount didn't see this as two requests.... though
>>>> maybe it already has enough locking.
>>>>
>>>> Does that seem reasonable?  Should we revert this patch (as a
>>>> regression) and do something in automount instead?
>>>
>>> Can you check out the "browse" option behavior before we talk further
>>> about this please.
>> 
>> Done that.
>> 
>>>
>>> The problem in user space now is that there is no way to control
>>> whether an indirect mount that uses the "nobrowse" option is triggered
>>> or not (using fstatat() or statx()) regardless of the flags used and it's
>>> essential the AT_NO_AUTOMOUNT flag work as expected to have user space
>>> take more care in not triggering automounts.
>> 
>> I think that user-space has all the control that it needs.
>> There are two cases:
>> 1/ daemon receives "missing indirect" message and the directory
>>    doesn't currently exist (mkdir() by the daemon succeeds).
>>    This could be an AT_NO_AUTOMOUNT lookup, or it could be a full
>>    open() or similar.  In either case the directory gets created if
>>    the map suggests that the name should exist.  The daemon needs to
>>    be careful not to block for long if it goes off-host to check for
>>    validity of the name.
>
> Aren't you assuming the the daemon only receives these lookups
> on the last path component?

No.
follow_managed() calls follow_automount() in a loop until it fails
(including -EISDIR which isn't exactly failure), or until no automount
is needed.
So where-ever the DCACHE_NEED_AUTOMOUNT is in the path, d_automount()
will be called if the dentry is negative, and unless it is the last
component of a NO_AUTOMOUNT lookup, it will be called if the dentry
isn't mounted-on.  So it would now be called twice for indirect
mounts that aren't browseable - once to mkdir and once to mount.  Might
there be a problem there?

>
> Intermediate path components that can trigger an automount must
> be treated as not having AT_NO_AUTOMOUNT in order for the walk to
> continue or fail at that point.
>
>> 
>> 2/ daemon received "missing indirect" message and the directory
>>    *does* exist (mkdir() by daemon fails with -EEXIST).
>>    This cannot be an AT_NO_AUTOMOUNT lookup, it must be a lookup
>>    intended to trigger automounts.  In this case, we trigger the
>>    mount.
>
> An fstatat(<path>, AT_NO_AUTOMOUNT) on a browse indirect map entry
> means the directory will exist but user space has asked not to trigger
> the mount and return the stat info of the autofs dentry.
>
> Please don't get me wrong, I think the suggestion is good, I just
> don't think it's as simple to do as it appears.

Maybe I should write a more complete patch for you to experiment with.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature