[PATCH] xfsprogs: ignore autofs mount table entries

[PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

Some of the xfsprogs utilities read the mount table via. getmntent(3).

The mount table may contain (almost always these days since /etc/mtab is
symlinked to /proc/self/mounts) autofs mount entries. During processing
of the mount table entries statfs(2) can be called on mount point paths
which will trigger an automount if those entries are direct or offset
autofs mount triggers (indirect autofs mounts aren't affected).

This can be a problem when there are a lot of autofs direct or offset
mounts because real mounts will be triggered when statfs(2) is called.
This can be particularly bad if the triggered mounts are NFS mounts and
the server is unavailable leading to lengthy boot times or worse.

Simply ignoring autofs mount entries during getmentent(3) traversals
avoids the statfs() call that triggers these mounts. If there are
automounted mounts (real mounts) at the time of reading the mount table
these will still be seen in the list so they will be included if that
actually matters to the reader.

Recent glibc getmntent(3) can ignore autofs mounts but that requires the
autofs user to configure autofs to use the "ignore" pseudo mount option
for autofs mounts. But this isn't yet the autofs default (to prevent
unexpected side effects) so that can't be used.

The autofs direct and offset automount triggers are pseudo file system
mounts and are more or less useless in terms on file system information
so excluding them doesn't sacrifice useful file system information
either.

Consequently excluding autofs mounts shouldn't have any adverse side
effects.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 fsr/xfs_fsr.c   |    3 +++
 libfrog/linux.c |    2 ++
 libfrog/paths.c |    2 ++
 3 files changed, 7 insertions(+)

diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
index 77a10a1d..466ad9e4 100644
--- a/fsr/xfs_fsr.c
+++ b/fsr/xfs_fsr.c
@@ -323,6 +323,9 @@ initallfs(char *mtab)
 	while ((mnt = platform_mntent_next(&cursor)) != NULL) {
 		int rw = 0;
 
+		if (!strcmp(mnt->mnt_type, "autofs"))
+			continue;
+
 		if (strcmp(mnt->mnt_type, MNTTYPE_XFS ) != 0 ||
 		    stat(mnt->mnt_fsname, &sb) == -1 ||
 		    !S_ISBLK(sb.st_mode))
diff --git a/libfrog/linux.c b/libfrog/linux.c
index 40a839d1..a45d99ab 100644
--- a/libfrog/linux.c
+++ b/libfrog/linux.c
@@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
 	 * servers.  So first, a simple check: does the "dev" start with "/" ?
 	 */
 	while ((mnt = getmntent(f)) != NULL) {
+		if (!strcmp(mnt->mnt_type, "autofs"))
+			continue;
 		if (mnt->mnt_fsname[0] != '/')
 			continue;
 		if (stat(mnt->mnt_dir, &mst) < 0)
diff --git a/libfrog/paths.c b/libfrog/paths.c
index 32737223..d6793764 100644
--- a/libfrog/paths.c
+++ b/libfrog/paths.c
@@ -389,6 +389,8 @@ fs_table_initialise_mounts(
 			return errno;
 
 	while ((mnt = getmntent(mtp)) != NULL) {
+		if (!strcmp(mnt->mnt_type, "autofs"))
+			continue;
 		if (!realpath(mnt->mnt_dir, rmnt_dir))
 			continue;
 		if (!realpath(mnt->mnt_fsname, rmnt_fsname))

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Darrick J. Wong]

On Thu, Oct 01, 2020 at 09:06:31AM +0800, Ian Kent wrote:
> Some of the xfsprogs utilities read the mount table via. getmntent(3).
> 
> The mount table may contain (almost always these days since /etc/mtab is
> symlinked to /proc/self/mounts) autofs mount entries. During processing
> of the mount table entries statfs(2) can be called on mount point paths
> which will trigger an automount if those entries are direct or offset
> autofs mount triggers (indirect autofs mounts aren't affected).
> 
> This can be a problem when there are a lot of autofs direct or offset
> mounts because real mounts will be triggered when statfs(2) is called.
> This can be particularly bad if the triggered mounts are NFS mounts and
> the server is unavailable leading to lengthy boot times or worse.
> 
> Simply ignoring autofs mount entries during getmentent(3) traversals
> avoids the statfs() call that triggers these mounts. If there are
> automounted mounts (real mounts) at the time of reading the mount table
> these will still be seen in the list so they will be included if that
> actually matters to the reader.
> 
> Recent glibc getmntent(3) can ignore autofs mounts but that requires the
> autofs user to configure autofs to use the "ignore" pseudo mount option
> for autofs mounts. But this isn't yet the autofs default (to prevent
> unexpected side effects) so that can't be used.
> 
> The autofs direct and offset automount triggers are pseudo file system
> mounts and are more or less useless in terms on file system information
> so excluding them doesn't sacrifice useful file system information
> either.
> 
> Consequently excluding autofs mounts shouldn't have any adverse side
> effects.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fsr/xfs_fsr.c   |    3 +++
>  libfrog/linux.c |    2 ++
>  libfrog/paths.c |    2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 77a10a1d..466ad9e4 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -323,6 +323,9 @@ initallfs(char *mtab)
>  	while ((mnt = platform_mntent_next(&cursor)) != NULL) {
>  		int rw = 0;
>  
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;

Hmm...  the libfrog changes look decent, but it strikes me as a little
odd that we don't just make platform_mntent_next filter that out?

(Or I guess refactor fsr to use the fs table...)

OTOH "Not _another_ herring^Wrefactor!"

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> +
>  		if (strcmp(mnt->mnt_type, MNTTYPE_XFS ) != 0 ||
>  		    stat(mnt->mnt_fsname, &sb) == -1 ||
>  		    !S_ISBLK(sb.st_mode))
> diff --git a/libfrog/linux.c b/libfrog/linux.c
> index 40a839d1..a45d99ab 100644
> --- a/libfrog/linux.c
> +++ b/libfrog/linux.c
> @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
>  	 * servers.  So first, a simple check: does the "dev" start with "/" ?
>  	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (mnt->mnt_fsname[0] != '/')
>  			continue;
>  		if (stat(mnt->mnt_dir, &mst) < 0)
> diff --git a/libfrog/paths.c b/libfrog/paths.c
> index 32737223..d6793764 100644
> --- a/libfrog/paths.c
> +++ b/libfrog/paths.c
> @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
>  			return errno;
>  
>  	while ((mnt = getmntent(mtp)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			^continue;
>  		if (!realpath(mnt->mnt_dir, rmnt_dir))
>  			continue;
>  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> 
> 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Eric Sandeen]

On 9/30/20 8:06 PM, Ian Kent wrote:
> Some of the xfsprogs utilities read the mount table via. getmntent(3).
> 
> The mount table may contain (almost always these days since /etc/mtab is
> symlinked to /proc/self/mounts) autofs mount entries. During processing
> of the mount table entries statfs(2) can be called on mount point paths
> which will trigger an automount if those entries are direct or offset
> autofs mount triggers (indirect autofs mounts aren't affected).
> 
> This can be a problem when there are a lot of autofs direct or offset
> mounts because real mounts will be triggered when statfs(2) is called.
> This can be particularly bad if the triggered mounts are NFS mounts and
> the server is unavailable leading to lengthy boot times or worse.
> 
> Simply ignoring autofs mount entries during getmentent(3) traversals
> avoids the statfs() call that triggers these mounts. If there are
> automounted mounts (real mounts) at the time of reading the mount table
> these will still be seen in the list so they will be included if that
> actually matters to the reader.
> 
> Recent glibc getmntent(3) can ignore autofs mounts but that requires the
> autofs user to configure autofs to use the "ignore" pseudo mount option
> for autofs mounts. But this isn't yet the autofs default (to prevent
> unexpected side effects) so that can't be used.
> 
> The autofs direct and offset automount triggers are pseudo file system
> mounts and are more or less useless in terms on file system information
> so excluding them doesn't sacrifice useful file system information
> either.
> 
> Consequently excluding autofs mounts shouldn't have any adverse side
> effects.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  fsr/xfs_fsr.c   |    3 +++
>  libfrog/linux.c |    2 ++
>  libfrog/paths.c |    2 ++
>  3 files changed, 7 insertions(+)
> 
> diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> index 77a10a1d..466ad9e4 100644
> --- a/fsr/xfs_fsr.c
> +++ b/fsr/xfs_fsr.c
> @@ -323,6 +323,9 @@ initallfs(char *mtab)
>  	while ((mnt = platform_mntent_next(&cursor)) != NULL) {
>  		int rw = 0;
>  
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
> +
>  		if (strcmp(mnt->mnt_type, MNTTYPE_XFS ) != 0 ||
>  		    stat(mnt->mnt_fsname, &sb) == -1 ||
>  		    !S_ISBLK(sb.st_mode))
>			continue;

Forgive me if I'm missing something obvious but isn't this added check redundant?

If mnt_type == "autofs" then mnt_type != MNTTYPE_XFS and we're ignoring it
already in this loop, no?  In this case, the loop is for xfs_fsr so we are really
only ever going to be looking for xfs mounts, as opposed to fs_table_initialise_mounts
which may accept "foreign" (non-xfs) filesystems.

> diff --git a/libfrog/linux.c b/libfrog/linux.c
> index 40a839d1..a45d99ab 100644
> --- a/libfrog/linux.c
> +++ b/libfrog/linux.c
> @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
>  	 * servers.  So first, a simple check: does the "dev" start with "/" ?
>  	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (mnt->mnt_fsname[0] != '/')
>  			continue;

Same sort of question here, but I don't know what these autofs entries look like.
Can their "device" (mnt_fsname) begin with "/" ?

Backing up a bit, which xfsprogs utility saw this behavior with autofs mounts?

I'm mostly ok with just always and forever filtering out anything that matches
"autofs" but if it's unnecessary (like the first case I think?) it may lead
to confusion for future code readers.

Thanks,
-Eric

>  		if (stat(mnt->mnt_dir, &mst) < 0)
> diff --git a/libfrog/paths.c b/libfrog/paths.c
> index 32737223..d6793764 100644
> --- a/libfrog/paths.c
> +++ b/libfrog/paths.c
> @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
>  			return errno;
>  
>  	while ((mnt = getmntent(mtp)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (!realpath(mnt->mnt_dir, rmnt_dir))
>  			continue;
>  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> 
> 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Thu, 2020-10-01 at 16:22 -0500, Eric Sandeen wrote:
> On 9/30/20 8:06 PM, Ian Kent wrote:
> > Some of the xfsprogs utilities read the mount table via.
> > getmntent(3).
> > 
> > The mount table may contain (almost always these days since
> > /etc/mtab is
> > symlinked to /proc/self/mounts) autofs mount entries. During
> > processing
> > of the mount table entries statfs(2) can be called on mount point
> > paths
> > which will trigger an automount if those entries are direct or
> > offset
> > autofs mount triggers (indirect autofs mounts aren't affected).
> > 
> > This can be a problem when there are a lot of autofs direct or
> > offset
> > mounts because real mounts will be triggered when statfs(2) is
> > called.
> > This can be particularly bad if the triggered mounts are NFS mounts
> > and
> > the server is unavailable leading to lengthy boot times or worse.
> > 
> > Simply ignoring autofs mount entries during getmentent(3)
> > traversals
> > avoids the statfs() call that triggers these mounts. If there are
> > automounted mounts (real mounts) at the time of reading the mount
> > table
> > these will still be seen in the list so they will be included if
> > that
> > actually matters to the reader.
> > 
> > Recent glibc getmntent(3) can ignore autofs mounts but that
> > requires the
> > autofs user to configure autofs to use the "ignore" pseudo mount
> > option
> > for autofs mounts. But this isn't yet the autofs default (to
> > prevent
> > unexpected side effects) so that can't be used.
> > 
> > The autofs direct and offset automount triggers are pseudo file
> > system
> > mounts and are more or less useless in terms on file system
> > information
> > so excluding them doesn't sacrifice useful file system information
> > either.
> > 
> > Consequently excluding autofs mounts shouldn't have any adverse
> > side
> > effects.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fsr/xfs_fsr.c   |    3 +++
> >  libfrog/linux.c |    2 ++
> >  libfrog/paths.c |    2 ++
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index 77a10a1d..466ad9e4 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -323,6 +323,9 @@ initallfs(char *mtab)
> >  	while ((mnt = platform_mntent_next(&cursor)) != NULL) {
> >  		int rw = 0;
> >  
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> > +
> >  		if (strcmp(mnt->mnt_type, MNTTYPE_XFS ) != 0 ||
> >  		    stat(mnt->mnt_fsname, &sb) == -1 ||
> >  		    !S_ISBLK(sb.st_mode))
> > 			continue;
> 
> Forgive me if I'm missing something obvious but isn't this added
> check redundant?
> 
> If mnt_type == "autofs" then mnt_type != MNTTYPE_XFS and we're
> ignoring it
> already in this loop, no?  In this case, the loop is for xfs_fsr so
> we are really
> only ever going to be looking for xfs mounts, as opposed to
> fs_table_initialise_mounts
> which may accept "foreign" (non-xfs) filesystems.
> 
> > diff --git a/libfrog/linux.c b/libfrog/linux.c
> > index 40a839d1..a45d99ab 100644
> > --- a/libfrog/linux.c
> > +++ b/libfrog/linux.c
> > @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block,
> > struct stat *s, int flags)
> >  	 * servers.  So first, a simple check: does the "dev" start
> > with "/" ?
> >  	 */
> >  	while ((mnt = getmntent(f)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> >  		if (mnt->mnt_fsname[0] != '/')
> >  			continue;
> 
> Same sort of question here, but I don't know what these autofs
> entries look like.
> Can their "device" (mnt_fsname) begin with "/" ?

It can, I fiddle with the device so it corresponds to the map
name and if that's a path, like /etc/auto.indirect, it will start
with a "/".

> 
> Backing up a bit, which xfsprogs utility saw this behavior with
> autofs mounts?

IIRC the problem I saw ended up being with xfs_spaceman invoked
via udisksd on mount/umount activity. There may be other cases so
I'd rather not assume there won't be problems elsewhere but those
checks for an xfs fs that I didn't notice probably need to change.

> 
> I'm mostly ok with just always and forever filtering out anything
> that matches
> "autofs" but if it's unnecessary (like the first case I think?) it
> may lead
> to confusion for future code readers.

I've got feedback from Darrick too, so let me think about what should
be done.

What I want out of this is that autofs mounts don't get triggered when
I start autofs for testing when xfs is the default (root) file system.
If it isn't the default file system this behaviour mostly doesn't
happen.

My basic test setup has a couple of hundred direct autofs mounts in
two or three maps and they all get mounted when starting autofs.

I'm surprised we haven't had complaints about it TBH but people might
not have noticed it since they expire away if they don't actually
get used.

Ian

> 
> Thanks,
> -Eric
> 
> >  		if (stat(mnt->mnt_dir, &mst) < 0)
> > diff --git a/libfrog/paths.c b/libfrog/paths.c
> > index 32737223..d6793764 100644
> > --- a/libfrog/paths.c
> > +++ b/libfrog/paths.c
> > @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
> >  			return errno;
> >  
> >  	while ((mnt = getmntent(mtp)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> >  		if (!realpath(mnt->mnt_dir, rmnt_dir))
> >  			continue;
> >  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> > 
> > 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Thu, 2020-10-01 at 08:19 -0700, Darrick J. Wong wrote:
> On Thu, Oct 01, 2020 at 09:06:31AM +0800, Ian Kent wrote:
> > Some of the xfsprogs utilities read the mount table via.
> > getmntent(3).
> > 
> > The mount table may contain (almost always these days since
> > /etc/mtab is
> > symlinked to /proc/self/mounts) autofs mount entries. During
> > processing
> > of the mount table entries statfs(2) can be called on mount point
> > paths
> > which will trigger an automount if those entries are direct or
> > offset
> > autofs mount triggers (indirect autofs mounts aren't affected).
> > 
> > This can be a problem when there are a lot of autofs direct or
> > offset
> > mounts because real mounts will be triggered when statfs(2) is
> > called.
> > This can be particularly bad if the triggered mounts are NFS mounts
> > and
> > the server is unavailable leading to lengthy boot times or worse.
> > 
> > Simply ignoring autofs mount entries during getmentent(3)
> > traversals
> > avoids the statfs() call that triggers these mounts. If there are
> > automounted mounts (real mounts) at the time of reading the mount
> > table
> > these will still be seen in the list so they will be included if
> > that
> > actually matters to the reader.
> > 
> > Recent glibc getmntent(3) can ignore autofs mounts but that
> > requires the
> > autofs user to configure autofs to use the "ignore" pseudo mount
> > option
> > for autofs mounts. But this isn't yet the autofs default (to
> > prevent
> > unexpected side effects) so that can't be used.
> > 
> > The autofs direct and offset automount triggers are pseudo file
> > system
> > mounts and are more or less useless in terms on file system
> > information
> > so excluding them doesn't sacrifice useful file system information
> > either.
> > 
> > Consequently excluding autofs mounts shouldn't have any adverse
> > side
> > effects.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fsr/xfs_fsr.c   |    3 +++
> >  libfrog/linux.c |    2 ++
> >  libfrog/paths.c |    2 ++
> >  3 files changed, 7 insertions(+)
> > 
> > diff --git a/fsr/xfs_fsr.c b/fsr/xfs_fsr.c
> > index 77a10a1d..466ad9e4 100644
> > --- a/fsr/xfs_fsr.c
> > +++ b/fsr/xfs_fsr.c
> > @@ -323,6 +323,9 @@ initallfs(char *mtab)
> >  	while ((mnt = platform_mntent_next(&cursor)) != NULL) {
> >  		int rw = 0;
> >  
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> 
> Hmm...  the libfrog changes look decent, but it strikes me as a
> little
> odd that we don't just make platform_mntent_next filter that out?

Perhaps, but is that a better idea?

Putting special case checks in the body of the loop stands out to
a reader but buried away in platform_mntent_next() it could easily
be missed by implicit assumptions about what platform_mntent_next()
should do.

As Eric pointed out there's an explicit check for an xfs fs right
below this in the body of the loop and even that wasn't enough to
get my attention ... but hey, more haste less speed ... ;)

Ian

> 
> (Or I guess refactor fsr to use the fs table...)
> 
> OTOH "Not _another_ herring^Wrefactor!"
> 
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> --D
> 
> > +
> >  		if (strcmp(mnt->mnt_type, MNTTYPE_XFS ) != 0 ||
> >  		    stat(mnt->mnt_fsname, &sb) == -1 ||
> >  		    !S_ISBLK(sb.st_mode))
> > diff --git a/libfrog/linux.c b/libfrog/linux.c
> > index 40a839d1..a45d99ab 100644
> > --- a/libfrog/linux.c
> > +++ b/libfrog/linux.c
> > @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block,
> > struct stat *s, int flags)
> >  	 * servers.  So first, a simple check: does the "dev" start
> > with "/" ?
> >  	 */
> >  	while ((mnt = getmntent(f)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> >  		if (mnt->mnt_fsname[0] != '/')
> >  			continue;
> >  		if (stat(mnt->mnt_dir, &mst) < 0)
> > diff --git a/libfrog/paths.c b/libfrog/paths.c
> > index 32737223..d6793764 100644
> > --- a/libfrog/paths.c
> > +++ b/libfrog/paths.c
> > @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
> >  			return errno;
> >  
> >  	while ((mnt = getmntent(mtp)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			^continue;
> >  		if (!realpath(mnt->mnt_dir, rmnt_dir))
> >  			continue;
> >  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> > 
> > 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Fri, 2020-10-02 at 10:27 +0800, Ian Kent wrote:
> On Thu, 2020-10-01 at 16:22 -0500, Eric Sandeen wrote:

snip ...

> > 
> > Backing up a bit, which xfsprogs utility saw this behavior with
> > autofs mounts?
> 
> IIRC the problem I saw ended up being with xfs_spaceman invoked
> via udisksd on mount/umount activity. There may be other cases so
> I'd rather not assume there won't be problems elsewhere but those
> checks for an xfs fs that I didn't notice probably need to change.

Looking around further, there may be another assumption that's
not right.

It looks like xfs_info is being called via udisksd -> libblockdev
and the xfd_open() triggers the mount not a statfs() call as thought.

I can't see why I saw xfs_spaceman hanging around longer than I
thought it should so I probably don't have the full story.

It's a bit academic though because there are good reasons to ignore
autofs mounts in the libfrog functions, platform_check_mount()
and fs_table_initialise_mounts().

If an autofs user has large direct mount maps there can be thousands
of distinct mount table entries which, mostly, if not always, serve
no useful purpose to utilities. They just add overhead so getting rid
of them at the earliest opportunity is the sensible thing to do.

In fact, before the mtab was symlinked to the proc mount table, I
simply didn't update the mount table for autofs fs mounts so they
never appeared and there were never any problem reports due to doing
this.

I could go on (and on) about this, but I'm starting to digress ...

Ian

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Eric Sandeen]

On 10/1/20 9:27 PM, Ian Kent wrote:
>> I'm mostly ok with just always and forever filtering out anything
>> that matches
>> "autofs" but if it's unnecessary (like the first case I think?) it
>> may lead
>> to confusion for future code readers.
> I've got feedback from Darrick too, so let me think about what should
> be done.
> 
> What I want out of this is that autofs mounts don't get triggered when
> I start autofs for testing when xfs is the default (root) file system.
> If it isn't the default file system this behaviour mostly doesn't
> happen.

Yep I'm totally on board with that plan in general, thanks.

I wouldn't worry about refactoring in service of the goal, I just wanted
to be sure that we were being strategic/surgical about the changes.

Thanks,
-Eric

> My basic test setup has a couple of hundred direct autofs mounts in
> two or three maps and they all get mounted when starting autofs.
> 
> I'm surprised we haven't had complaints about it TBH but people might
> not have noticed it since they expire away if they don't actually
> get used.
> 
> Ian
> 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Fri, 2020-10-02 at 10:15 -0500, Eric Sandeen wrote:
> 
> On 10/1/20 9:27 PM, Ian Kent wrote:
> > > I'm mostly ok with just always and forever filtering out anything
> > > that matches
> > > "autofs" but if it's unnecessary (like the first case I think?)
> > > it
> > > may lead
> > > to confusion for future code readers.
> > I've got feedback from Darrick too, so let me think about what
> > should
> > be done.
> > 
> > What I want out of this is that autofs mounts don't get triggered
> > when
> > I start autofs for testing when xfs is the default (root) file
> > system.
> > If it isn't the default file system this behaviour mostly doesn't
> > happen.
> 
> Yep I'm totally on board with that plan in general, thanks.
> 
> I wouldn't worry about refactoring in service of the goal, I just
> wanted
> to be sure that we were being strategic/surgical about the changes.

Back to this ...

I wasn't thinking so much about strategic/surgical because those autofs
entries only add overhead, particularly if your loading the mounts and
traversing them later for such things as searching.

But looking around further it's platform_test_xfs_path() that does the
statfs() that causes mount triggering. It wants to know the fs type of
the passed in path so it has no choice but to call statfs().

It's called from various places originating from fs_table_initialise()
and one spot in xfs_fsr.c so excluding autofs mounts via the initialise
function is petty effective at getting rid of that extra overhead and
avoiding the platform_test_xfs_path() call.

I don't see a sensible way to eliminate this call from xfs_fsr but
reorganization isn't the sort of thing that udisksd (or others) would
have reason to call anyway so there's no real point in doing it.

So I think just dropping that first hunk, as you recommended, is the
right thing to do.

Ian
> 
> Thanks,
> -Eric
> 
> > My basic test setup has a couple of hundred direct autofs mounts in
> > two or three maps and they all get mounted when starting autofs.
> > 
> > I'm surprised we haven't had complaints about it TBH but people
> > might
> > not have noticed it since they expire away if they don't actually
> > get used.
> > 
> > Ian
> > 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Eric Sandeen]

On 10/1/20 11:40 PM, Ian Kent wrote:
> On Fri, 2020-10-02 at 10:27 +0800, Ian Kent wrote:
>> On Thu, 2020-10-01 at 16:22 -0500, Eric Sandeen wrote:
> 
> snip ...
> 
>>>
>>> Backing up a bit, which xfsprogs utility saw this behavior with
>>> autofs mounts?
>>
>> IIRC the problem I saw ended up being with xfs_spaceman invoked
>> via udisksd on mount/umount activity. There may be other cases so
>> I'd rather not assume there won't be problems elsewhere but those
>> checks for an xfs fs that I didn't notice probably need to change.
> 
> Looking around further, there may be another assumption that's
> not right.
> 
> It looks like xfs_info is being called via udisksd -> libblockdev
> and the xfd_open() triggers the mount not a statfs() call as thought.
> 
> I can't see why I saw xfs_spaceman hanging around longer than I
> thought it should so I probably don't have the full story.

probably because recent xfs_info is a shell script that calls spaceman?

Tho I wonder what udisksd/libblockdev is doing with xfs_info info ...

*shrug*

-Eric

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Thu, 2020-10-08 at 15:02 -0500, Eric Sandeen wrote:
> On 10/1/20 11:40 PM, Ian Kent wrote:
> > On Fri, 2020-10-02 at 10:27 +0800, Ian Kent wrote:
> > > On Thu, 2020-10-01 at 16:22 -0500, Eric Sandeen wrote:
> > 
> > snip ...
> > 
> > > > Backing up a bit, which xfsprogs utility saw this behavior with
> > > > autofs mounts?
> > > 
> > > IIRC the problem I saw ended up being with xfs_spaceman invoked
> > > via udisksd on mount/umount activity. There may be other cases so
> > > I'd rather not assume there won't be problems elsewhere but those
> > > checks for an xfs fs that I didn't notice probably need to
> > > change.
> > 
> > Looking around further, there may be another assumption that's
> > not right.
> > 
> > It looks like xfs_info is being called via udisksd -> libblockdev
> > and the xfd_open() triggers the mount not a statfs() call as
> > thought.
> > 
> > I can't see why I saw xfs_spaceman hanging around longer than I
> > thought it should so I probably don't have the full story.
> 
> probably because recent xfs_info is a shell script that calls
> spaceman?
> 
> Tho I wonder what udisksd/libblockdev is doing with xfs_info info ...

Haha, I wonder that too.

I've only looked very briefly at udiskd so far, enough to know that
its object oriented'ish factorization is enough to make it hard to
follow.

Over the years my impression is that it takes a very special skill
to write object oriented code that is maintainable/readable and
that's skill is sadly lacking in a lot of cases.

But maybe its just my poor old brain that's lacking, ;)

Ian

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

On Thu, 2020-10-08 at 15:03 -0500, Eric Sandeen wrote:
> On 10/7/20 8:52 PM, Ian Kent wrote:
> > Some of the xfsprogs utilities read the mount table via.
> > getmntent(3).
> > 
> > The mount table may contain (almost always these days since
> > /etc/mtab is
> > symlinked to /proc/self/mounts) autofs mount entries. During
> > processing
> > of the mount table entries statfs(2) can be called on mount point
> > paths
> > which will trigger an automount if those entries are direct or
> > offset
> > autofs mount triggers (indirect autofs mounts aren't affected).
> > 
> > This can be a problem when there are a lot of autofs direct or
> > offset
> > mounts because real mounts will be triggered when statfs(2) is
> > called.
> > This can be particularly bad if the triggered mounts are NFS mounts
> > and
> > the server is unavailable leading to lengthy boot times or worse.
> > 
> > Simply ignoring autofs mount entries during getmentent(3)
> > traversals
> > avoids the statfs() call that triggers these mounts. If there are
> > automounted mounts (real mounts) at the time of reading the mount
> > table
> > these will still be seen in the list so they will be included if
> > that
> > actually matters to the reader.
> > 
> > Recent glibc getmntent(3) can ignore autofs mounts but that
> > requires the
> > autofs user to configure autofs to use the "ignore" pseudo mount
> > option
> > for autofs mounts. But this isn't yet the autofs default (to
> > prevent
> > unexpected side effects) so that can't be used.
> > 
> > The autofs direct and offset automount triggers are pseudo file
> > system
> > mounts and are more or less useless in terms on file system
> > information
> > so excluding them doesn't sacrifice useful file system information
> > either.
> > 
> > Consequently excluding autofs mounts shouldn't have any adverse
> > side
> > effects.
> 
> (usually this'd go below the "---")
> 
> > Changes since v1:
> > - drop hunk from fsr/xfs_fsr.c.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  libfrog/linux.c |    2 ++
> >  libfrog/paths.c |    2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/libfrog/linux.c b/libfrog/linux.c
> > index 40a839d1..a45d99ab 100644
> > --- a/libfrog/linux.c
> > +++ b/libfrog/linux.c
> > @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block,
> > struct stat *s, int flags)
> >  	 * servers.  So first, a simple check: does the "dev" start
> > with "/" ?
> >  	 */
> >  	while ((mnt = getmntent(f)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> 
> I may change the order of this test and the next, just so it
> continues to
> align with the comment above.  Shouldn't make any difference, right?

Yep, no difference to resolving the problem.

The only reason for it to be first is cases where there's say, 40 or
50 mounts and a thousand or more autofs direct mounts. Then every
test you do before skipping the autofs entry adds a thousand or more
tests to the traversal.

Ian
> 
> Otherwise:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> >  		if (mnt->mnt_fsname[0] != '/')
> >  			continue;
> >  		if (stat(mnt->mnt_dir, &mst) < 0)
> > diff --git a/libfrog/paths.c b/libfrog/paths.c
> > index 32737223..d6793764 100644
> > --- a/libfrog/paths.c
> > +++ b/libfrog/paths.c
> > @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
> >  			return errno;
> >  
> >  	while ((mnt = getmntent(mtp)) != NULL) {
> > +		if (!strcmp(mnt->mnt_type, "autofs"))
> > +			continue;
> >  		if (!realpath(mnt->mnt_dir, rmnt_dir))
> >  			continue;
> >  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> > 
> > 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Eric Sandeen]

On 10/7/20 8:52 PM, Ian Kent wrote:
> Some of the xfsprogs utilities read the mount table via. getmntent(3).
> 
> The mount table may contain (almost always these days since /etc/mtab is
> symlinked to /proc/self/mounts) autofs mount entries. During processing
> of the mount table entries statfs(2) can be called on mount point paths
> which will trigger an automount if those entries are direct or offset
> autofs mount triggers (indirect autofs mounts aren't affected).
> 
> This can be a problem when there are a lot of autofs direct or offset
> mounts because real mounts will be triggered when statfs(2) is called.
> This can be particularly bad if the triggered mounts are NFS mounts and
> the server is unavailable leading to lengthy boot times or worse.
> 
> Simply ignoring autofs mount entries during getmentent(3) traversals
> avoids the statfs() call that triggers these mounts. If there are
> automounted mounts (real mounts) at the time of reading the mount table
> these will still be seen in the list so they will be included if that
> actually matters to the reader.
> 
> Recent glibc getmntent(3) can ignore autofs mounts but that requires the
> autofs user to configure autofs to use the "ignore" pseudo mount option
> for autofs mounts. But this isn't yet the autofs default (to prevent
> unexpected side effects) so that can't be used.
> 
> The autofs direct and offset automount triggers are pseudo file system
> mounts and are more or less useless in terms on file system information
> so excluding them doesn't sacrifice useful file system information
> either.
> 
> Consequently excluding autofs mounts shouldn't have any adverse side
> effects.

(usually this'd go below the "---")

> Changes since v1:
> - drop hunk from fsr/xfs_fsr.c.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  libfrog/linux.c |    2 ++
>  libfrog/paths.c |    2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/libfrog/linux.c b/libfrog/linux.c
> index 40a839d1..a45d99ab 100644
> --- a/libfrog/linux.c
> +++ b/libfrog/linux.c
> @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
>  	 * servers.  So first, a simple check: does the "dev" start with "/" ?
>  	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;

I may change the order of this test and the next, just so it continues to
align with the comment above.  Shouldn't make any difference, right?

Otherwise:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>  		if (mnt->mnt_fsname[0] != '/')
>  			continue;
>  		if (stat(mnt->mnt_dir, &mst) < 0)
> diff --git a/libfrog/paths.c b/libfrog/paths.c
> index 32737223..d6793764 100644
> --- a/libfrog/paths.c
> +++ b/libfrog/paths.c
> @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
>  			return errno;
>  
>  	while ((mnt = getmntent(mtp)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (!realpath(mnt->mnt_dir, rmnt_dir))
>  			continue;
>  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> 
> 

Re: [PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

Oops!

Didn't add v2 to [PATCH] in the title, sorry about that.

On Thu, 2020-10-08 at 09:52 +0800, Ian Kent wrote:
> Some of the xfsprogs utilities read the mount table via.
> getmntent(3).
> 
> The mount table may contain (almost always these days since /etc/mtab
> is
> symlinked to /proc/self/mounts) autofs mount entries. During
> processing
> of the mount table entries statfs(2) can be called on mount point
> paths
> which will trigger an automount if those entries are direct or offset
> autofs mount triggers (indirect autofs mounts aren't affected).
> 
> This can be a problem when there are a lot of autofs direct or offset
> mounts because real mounts will be triggered when statfs(2) is
> called.
> This can be particularly bad if the triggered mounts are NFS mounts
> and
> the server is unavailable leading to lengthy boot times or worse.
> 
> Simply ignoring autofs mount entries during getmentent(3) traversals
> avoids the statfs() call that triggers these mounts. If there are
> automounted mounts (real mounts) at the time of reading the mount
> table
> these will still be seen in the list so they will be included if that
> actually matters to the reader.
> 
> Recent glibc getmntent(3) can ignore autofs mounts but that requires
> the
> autofs user to configure autofs to use the "ignore" pseudo mount
> option
> for autofs mounts. But this isn't yet the autofs default (to prevent
> unexpected side effects) so that can't be used.
> 
> The autofs direct and offset automount triggers are pseudo file
> system
> mounts and are more or less useless in terms on file system
> information
> so excluding them doesn't sacrifice useful file system information
> either.
> 
> Consequently excluding autofs mounts shouldn't have any adverse side
> effects.
> 
> Changes since v1:
> - drop hunk from fsr/xfs_fsr.c.
> 
> Signed-off-by: Ian Kent <raven@themaw.net>
> ---
>  libfrog/linux.c |    2 ++
>  libfrog/paths.c |    2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/libfrog/linux.c b/libfrog/linux.c
> index 40a839d1..a45d99ab 100644
> --- a/libfrog/linux.c
> +++ b/libfrog/linux.c
> @@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block,
> struct stat *s, int flags)
>  	 * servers.  So first, a simple check: does the "dev" start
> with "/" ?
>  	 */
>  	while ((mnt = getmntent(f)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (mnt->mnt_fsname[0] != '/')
>  			continue;
>  		if (stat(mnt->mnt_dir, &mst) < 0)
> diff --git a/libfrog/paths.c b/libfrog/paths.c
> index 32737223..d6793764 100644
> --- a/libfrog/paths.c
> +++ b/libfrog/paths.c
> @@ -389,6 +389,8 @@ fs_table_initialise_mounts(
>  			return errno;
>  
>  	while ((mnt = getmntent(mtp)) != NULL) {
> +		if (!strcmp(mnt->mnt_type, "autofs"))
> +			continue;
>  		if (!realpath(mnt->mnt_dir, rmnt_dir))
>  			continue;
>  		if (!realpath(mnt->mnt_fsname, rmnt_fsname))
> 
> 

[PATCH] xfsprogs: ignore autofs mount table entries

[Ian Kent]

Some of the xfsprogs utilities read the mount table via. getmntent(3).

The mount table may contain (almost always these days since /etc/mtab is
symlinked to /proc/self/mounts) autofs mount entries. During processing
of the mount table entries statfs(2) can be called on mount point paths
which will trigger an automount if those entries are direct or offset
autofs mount triggers (indirect autofs mounts aren't affected).

This can be a problem when there are a lot of autofs direct or offset
mounts because real mounts will be triggered when statfs(2) is called.
This can be particularly bad if the triggered mounts are NFS mounts and
the server is unavailable leading to lengthy boot times or worse.

Simply ignoring autofs mount entries during getmentent(3) traversals
avoids the statfs() call that triggers these mounts. If there are
automounted mounts (real mounts) at the time of reading the mount table
these will still be seen in the list so they will be included if that
actually matters to the reader.

Recent glibc getmntent(3) can ignore autofs mounts but that requires the
autofs user to configure autofs to use the "ignore" pseudo mount option
for autofs mounts. But this isn't yet the autofs default (to prevent
unexpected side effects) so that can't be used.

The autofs direct and offset automount triggers are pseudo file system
mounts and are more or less useless in terms on file system information
so excluding them doesn't sacrifice useful file system information
either.

Consequently excluding autofs mounts shouldn't have any adverse side
effects.

Changes since v1:
- drop hunk from fsr/xfs_fsr.c.

Signed-off-by: Ian Kent <raven@themaw.net>
---
 libfrog/linux.c |    2 ++
 libfrog/paths.c |    2 ++
 2 files changed, 4 insertions(+)

diff --git a/libfrog/linux.c b/libfrog/linux.c
index 40a839d1..a45d99ab 100644
--- a/libfrog/linux.c
+++ b/libfrog/linux.c
@@ -73,6 +73,8 @@ platform_check_mount(char *name, char *block, struct stat *s, int flags)
 	 * servers.  So first, a simple check: does the "dev" start with "/" ?
 	 */
 	while ((mnt = getmntent(f)) != NULL) {
+		if (!strcmp(mnt->mnt_type, "autofs"))
+			continue;
 		if (mnt->mnt_fsname[0] != '/')
 			continue;
 		if (stat(mnt->mnt_dir, &mst) < 0)
diff --git a/libfrog/paths.c b/libfrog/paths.c
index 32737223..d6793764 100644
--- a/libfrog/paths.c
+++ b/libfrog/paths.c
@@ -389,6 +389,8 @@ fs_table_initialise_mounts(
 			return errno;
 
 	while ((mnt = getmntent(mtp)) != NULL) {
+		if (!strcmp(mnt->mnt_type, "autofs"))
+			continue;
 		if (!realpath(mnt->mnt_dir, rmnt_dir))
 			continue;
 		if (!realpath(mnt->mnt_fsname, rmnt_fsname))