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

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 5341 bytes --]

On Thu, Nov 23 2017, Ian Kent wrote:

> On 23/11/17 10:21, NeilBrown wrote:
>> On Thu, Nov 23 2017, Ian Kent wrote:
>> 
>>>
>>> Hey Neil, I'm looking at this again because RH QE have complained about
>>> a regression test failing with a kernel that has this change.
>>>
>>> Maybe I'm just dumb but I though a "find <base directory> <options>"
>>> would, well, just look at the contents below <base directory> but an
>>> strace shows that it reads and calls fstatat() on "every entry in the
>>> mount table" regardless of the path.
>> 
>> weird ... I can only get find to look at the mount table if given the
>> -fstyp option, and even then it doesn't fstatat anything that isn't in
>> the tree it is searching.
>
> It's probably the -xautofs (exclude autofs fs'es) that was used in
> the test that requires reading the mount table to get info about
> excluding autofs mounts but the fstatat() on all the entries,
> regardless of path, that was a surprise to me.
>
> find did use AT_SYMLINK_NOFOLLOW which historically behaved like
> AT_NO_AUTOMOUNT.
>
>> 
>> 
>>>
>>> And with the move of userspace to use /proc based mount tables (one
>>> example being the symlink of /etc/mtab into /proc) even modest sized
>>> direct mount maps will be a problem with every entry getting mounted.
>> 
>> But the patch in question is only about indirect mount maps, isn't it?
>> How is it relevant to direct mount maps?
>
> The change here will cause fstatat() to trigger direct mounts on access
> if it doesn't use AT_NO_AUTOMOUNT.

Ahhh... light dawns.
This is about this bit of the patch:

 static inline int vfs_fstatat(int dfd, const char __user *filename,
                              struct kstat *stat, int flags)
 {
-       return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
-                        stat, STATX_BASIC_STATS);
+       return vfs_statx(dfd, filename, flags, stat, STATX_BASIC_STATS);
 }

I hadn't paid much attention to that.

I before this patch:
  stat and lstat act as you would expect AT_NO_AUTOMOUNT to act on
      direct mount and browseable indirect mount, but not on unbrowseable
      indirect mounts
  fstatat appeared to accept the AT_NO_AUTOMOUNT flag, but actually
      assumed it was always set, but acted like stat and lstat
  xstatat actually accepted the AT_NO_AUTOMOUNT flag, but it had no
      effect on unbrowseable indirect mounts.

after the patch, the distinction between direct and indirect was gone,
and fstatat now handles AT_NO_AUTOMOUNT the same as xstatat.
So:
  stat and lstat now don't trigger automounts even on indirect, but
    this is a mixed blessing as they don't even trigger the mkdir
  fstatat without AT_NO_AUTOMOUNT now always triggers an automount
    This is a problematic regression that you have noticed and
    likely needs to be reverted.  Maybe we can assume
    AT_NO_AUTOMOUNT when AT_SYMLINK_NOFOLLOW is set, and require
    people to use xstatat if they need to set the flags separately
  xstatat now correctly honours AT_NO_AUTOMOUNT for indirect mounts
    but is otherwise unchanged.

What would you think of changing the above to

 static inline int vfs_fstatat(int dfd, const char __user *filename,
                              struct kstat *stat, int flags)
 {
-       return vfs_statx(dfd, filename, flags | AT_NO_AUTOMOUNT,
-                        stat, STATX_BASIC_STATS);
+       return vfs_statx(dfd, filename,
+                        (flags & AT_SYMLINK_NOFOLLOW) ? (flags |
+                             AT_NO_AUTOMOUNT) : flags,
+                             stat, STATX_BASIC_STATS);
 }

??

Thanks,
NeilBrown

>
> It's not a problem for browse indirect mounts because they are plain
> directories within the autofs mount point not individual autofs mount
> triggers.
>
>> 
>>>
>>> Systems will cope with this fine but larger systems not so much.
>>>
>>> If find does this then the user space changes needed to accommodate
>>> this sort of change are almost certainly far more than I expected.
>>>
>>> I think this is an example of the larger problem I'm faced with and
>>> this change was was meant to be a starting point for resolution.
>>>
>>> The most obvious symptom of the problem is auto-mounts no longer able
>>> to be expired due to being re-mounted immediately after expire. Another
>>> symptom is unwanted (by the user) accesses causing unexpected auto-mount
>>> attempts.
>>>
>>> I believe this monitoring of the mount table is what leads to excessive
>>> CPU consumption I've seen, usually around six processes, under heavy
>>> mount activity. And following this, when the mount table is large and
>>> there is "no mount activity" two of the six processes continue to consume
>>> excessive CPU, until the mount table shrinks.
>>>
>>> So now I'm coming around to the idea of reverting this change ..... and
>>> going back to the drawing board.
>> 
>> I can well imaging that a large mount table could cause problems for
>> applications that are written to expect one, and I can imagine that
>> autofs could cause extra issues for such a program as it might change
>> the mount table more often.  But I haven't yet worked out how this is
>> related to the patch in question....
>> 
>> Thanks,
>> NeilBrown
>> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]