From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:53150 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752328AbdKWDEl (ORCPT ); Wed, 22 Nov 2017 22:04:41 -0500 From: NeilBrown To: Ian Kent , Al Viro Date: Thu, 23 Nov 2017 14:04:31 +1100 Cc: Colin Walters , Ondrej Holy , autofs mailing list , Kernel Mailing List , David Howells , linux-fsdevel Subject: Re: [PATCH 3/3] autofs - fix AT_NO_AUTOMOUNT not being honored In-Reply-To: <74aa625f-29c9-89c8-c58b-f60b183413ed@themaw.net> References: <149438991819.26550.11290804420751932707.stgit@pluto.themaw.net> <149438992850.26550.14370272866390445786.stgit@pluto.themaw.net> <874lpo2y9e.fsf@notabene.neil.brown.name> <8760a1254e.fsf@notabene.neil.brown.name> <74aa625f-29c9-89c8-c58b-f60b183413ed@themaw.net> Message-ID: <87tvxlzoe8.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-fsdevel-owner@vger.kernel.org List-ID: --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Thu, Nov 23 2017, Ian Kent wrote: > On 23/11/17 08:47, NeilBrown wrote: >> On Wed, Nov 22 2017, Ian Kent wrote: >>=20 >>> 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 trigger= ing >>>>> 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 prev= ents >>>>> 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 bei= ng >>> able to work consistently. >>=20 >> Just to be clear, the previous policy was: >> kernel - a lookup would cause a message to be sent to the automount dae= mon >> daemon - the receipt of a message would cause a mount to occur. >>=20 >> 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. >>=20 >> Specifically, I think the second part of the policy should be revised a = little. >>=20 >>> >>> 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. >>=20 >> 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. >>=20 >>> >>>> >>>> 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. >>=20 >> 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? >>=20 >>> >>>> >>>> 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. >>=20 >> 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: >>=20 >> 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; >> + } >>=20=20 >> if (!status) >> existed =3D 0; >>=20 >> to automount and now it behaves (for NFS mounts) how I would like (though >> there is room for improvement). >>=20 >>> >>>> >>>> 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 yo= ur >>> suggesting is the internal program default, and has been since the init= ial >>> 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. >>=20 >> 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. >=20=20 >>=20 >>> >>>> >>>> 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. >>=20 >> Done that. >>=20 >>> >>> 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. >>=20 >> 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. > >>=20 >> 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(, 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 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAloWOr8ACgkQOeye3VZi gbllkBAAo1ycNNWcZt0ptddQ6cvqL3pfkMyqrITEl1KfqAMtLse1wAV7XtfMXelQ B2+VPyUYAom+s74xcRO5R0L5GG7xNX/B+law4bo4PbbYq9QlJuvbRiBMkiih71HD PyBXYKYXhLIthoXFoiOINGz5jTvFQy0szmUAoibUbwghe9BYErxe/05tQbaZ6Vqv a8Vnx6d3xzBWPEub90pfCOJ3qYBA88UDSI+x1d/Wh5k9KYSVKPcvnkvuiLn2ns1i GZf8Q6sXrSjvmpT8uhXWiLgyXesBy2JEqnnXWcuDIcGtTB5m+uKYsOUyXGtXDJI3 iiAB0gOyiKxNHG3c+9zZ0P6KGGkDViPQ3/+5VL4pL2VKLhYaN8WeRFFmBPuvSWyG PQs29nlJiojwCfgVrIWlnD2ds75rUTjCvCbA+OiXhheeQDCnHsTVpcJX9OJJdG7n 3tHw6pv4Su0m/3WCkXxVvhzLUqmBmda4iJTgGNtv7Xj9esZbnAX7H9oV2ZCHboaE W4GD+hhYqjbR+s/DTJZTxvHj7sLX0qJluj9j/dSIY+r2vo5uf8z5fXM7W621RD3m KBgdzjKGaT/fS1TAjFhT9KADRusP2ofn9WGoRcEvbL13pArN39lGs1eqt08VTUY6 v97OoDjF7z4dknDDbx/P3rfh3AgjemO0SVQdhfGLh6fjNuyMuL0= =bOJz -----END PGP SIGNATURE----- --=-=-=--