Re: df: use mountinfo from /proc on Linux

[Pádraig Brady]

On 25/08/14 18:23, Fridolin Pokorny wrote:
> On Wednesday 20 August 2014 16:21:27 Pádraig Brady wrote:
>> On 08/20/2014 03:53 PM, Fridolin Pokorny wrote:
>>> Hi,
>>>
>>> I am about to implement /proc/self/mountinfo support for df. This feature
>>> is marked as TODO in sources. I have found a suitable parser provided by
>>> libmount from util-linux [1]. This could be the best option here in my
>>> opinion.
>>>
>>> The change might also reflect gnulib. Items in mount entries (struct
>>> mount_entry from mountlist.h) should be propagated from libmount.
>>>
>>> I want to know if such change will be accepted. Do you have any additional
>>> notes to discuss?
>>>
>>> Have a nice day,
>>> Fridolin Pokorny.
>>>
>>> [1] http://git.kernel.org/cgit/utils/util-linux/util-linux.git
>>
>> Note the kernel processing to generate /proc/self/mountinfo is significant,
>> so ideally gnulib's read_file_system_list() would read that once (falling
>> back to /etc/mtab if not available).
> 
> Agreed.
>  
>> There is more info in /proc/$$/mountinfo wrt bind mounts etc.
>> and I've yet to look into if any of that is useful for df.
> 
> I went through kernel sources and I cannot find any difference in generated
> output. Accessing /proc/self is treated as accessing /proc/$$. I saw that 
> there are used exclusive locks when accessing /proc/self, which will be good 
> to omit by using /proc/$$.
> 
> There can be found info about shared subtrees and parent mount IDs among 
> others in /proc/$$/mountinfo. I don't know if either of this could be useful 
> for df.
> 
>> I'd come up with a concrete set of advantages of using libmount before
>> proceeding. Avoid stat()s may not warrant the change.
> 
> The biggest advantage for me is that mount ID could be propagated from /proc
> instead of using stat()s:
> 
> # mkdir /chroot/home
> # mount /dev/sda1 /chroot/home
> # mount /dev/sda2 /home
> # df
> ...
> /dev/sda1 - - -  -% /chroot/home
> /dev/sda2 - - -  -% /home
> ...
> # mount -t proc proc /chroot/proc/
> # chroot /chroot
> chroot# df
> ...
> /dev/sda2 - - -  -% /home
> ...
> 
> There should be /dev/sda1.
> 
> Filesystems mounted outside of chroot are not displayed in 
> /proc/$$/mountinfo. 
> Now df prints errors complaining ENOENT when filesystems are not accessible 
> inside chroot.
> 
> Another bogus output for me could be simulated by creating directory with same
> name as mountpoint outside of chroot.
> 
> chroot# ls /run
> ls: cannot access /run: No such file or directory
> chroot# df
> ...
> df: '/run': No such file or directory
> ...
> chroot# mkdir /run
> chroot# df -a
> ...
> tmpfs - - - -% /run
> ...
> 
> In all cases above, mountpoints are in mount namespace of the current 
> process, 
> but they are not accessible due to chroot. All of this can be improved by 
> using /proc/$$/mountinfo.

Testing here with Linux kernel 4.0.4,
/proc/mounts has consistent content with /proc/$$/mountinfo.
I.e. this change isn't strictly necessary when /etc/mtab is a link to 
/proc/mounts.
Now it is useful to avoid issues on systems with a static /etc/mtab,
and on older systems assuming the behavior above was seen with /proc/mounts.

The related coreutils patch to use the device IDs could be problematic
though in the presence of over mounts. I.E. using stat() to get the last mounted
device ID for a path is safest. Also there is no point falling back to
using the preopulated device ID if we can't stat, as in that case
we'll not be able to statfs either and thus the extra logic and runtime
would be moot.

I'll apply this as clarification.

diff --git a/src/df.c b/src/df.c
index 9d4c027..2e541b9 100644
--- a/src/df.c
+++ b/src/df.c
@@ -623,10 +623,8 @@ filter_mount_list (bool devices_only)
       struct mount_entry *discard_me = NULL;

       /* Avoid stating remote file systems as that may hang.
-         TODO: On Linux we might avoid this stat() and another in get_dev()
-         by using the device IDs available from /proc/self/mountinfo.
-         read_file_system_list() could populate me_dev from those
-         for efficiency and accuracy.  */
+         On Linux we probably have me_dev populated from /proc/self/mountinfo,
+         however we still stat() in case another device was mounted later.  */
       if ((me->me_remote && show_local_fs)
           || -1 == stat (me->me_mountdir, &buf))
         {


thanks,
Pádraig.