Re: [PATCH] df: avoid stat() for dummy file systems with -l

[Kamil Dudka]

On Tuesday, August 01, 2017 15:28:11 Bernhard Voelker wrote:
> On 08/01/2017 10:02 AM, Kamil Dudka wrote:
> > On Tuesday, August 01, 2017 01:54:56 Bernhard Voelker wrote:
> >> The attached fixes a hang for unaccessible NFS mounts which
> >> are automounted by systemd.
> > 
> > The summary mentions 'df -l', which is mapped to the show_local_fs flag.
> > However, the condition you are adding does not use the flag at all.  Why?
> > 
> > The condition already contains a special handling for 'df -l':
> >     (me->me_remote && show_local_fs)
> > 
> > Why does not it work without your patch?
> 
> I admit the relation is not so obvious so I tried to explain in the
> commit message.
> 
> The point is that the mount point of the systemd.automount(5)
> is of type "autofs" (where me_dummy=true), and when it would be accessed,
> then systemd would over-mount the same mount point with the remote
> NFS file system.
> 
> Now I think df(1) should not necessarily trigger to automount file systems,
> regardless wether they are local or remote.
> 
> The new condition therefore avoid the stat() on dummy file systems (which
> includes the "autofs" entry of the systemd automounter), and uses the same
> condition as that which filters out dummy entries anyway in get_dev().
> In effect, this prevents to trigger the automounting.
> If the remote file system is mounted, then the existing condition
> prevents to stat() it for the -l case.

If I understand it correctly, your patch takes effect regardless the use
of -l option.  Then please remove it from the summary to make it clear.

The optimized-out stat() call is, however, not the only change done by
your commit.  It has a side-effect that is observable from df's output
(and completely unrelated to autofs or remote file systems).  At first
glance, the side-effect looks useful but it should be properly analyzed
and documented...

Consider the following example:

# mount -t proc none /opt
# mount -t tmpfs none /opt

Now unpached df does not list the /opt entry:

# df | grep opt

... but after your patch, it does:

# df | grep opt
none             2005384        0   2005384   0% /opt

Note that it was important that I have used the same "device" in both
the mount commands.

Kamil

> > How exactly looks the mount entry you want to skip the stat() call for?
> 
> 133 59 0:43 / /mnt/scratch rw,relatime shared:77 - autofs systemd-1
> rw,fd=25,pgrp=1,timeout=0,minproto=5,maxproto=5,direct
> > What is the full content of *me in this case?
> 
> Sorry, I don't have access to my other test system, but I hope the above
> explains enough.
> 
> > Is there a good reason for
> > http://bugzilla.suse.com/show_bug.cgi?id=1043059
> > not being accessible for free?
> 
> Well, it's a bug against SLES12sp2, and some customers don't seem to want
> those being visible to everybody.  I (as openSUSE co-maintainer) can only
> see it because I was asked if the patch could be included upstreams.  I'm
> not sure I could make it available for you.  Josef could probably.
> 
> Thanks & Have a nice day,
> Berny