On Monday 25 August 2014 18:40:10 Pádraig Brady wrote: > On 08/25/2014 06:23 PM, 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. > > Thanks for investigating. > > With both the performance and correctness advantages > doing this is worthwhile.
Proposing the patch. A gnulib patch was already proposed [1]. Have a nice day! Fridolin Pokorny [1] http://lists.gnu.org/archive/html/bug-gnulib/2014-08/msg00039.html
>From 21d983eac2cb2b955fae7cda3c723a18736ba3d8 Mon Sep 17 00:00:00 2001 From: Fridolin Pokorny <[email protected]> Date: Wed, 27 Aug 2014 15:00:14 +0200 Subject: [PATCH] df: avoid stat()s when using /proc/<PID>/mountinfo * src/df.c (filter_mount_list, get_dev): Use stat() only if device ID is not known. It could be propagated from gnulib when /proc/<PID>/mountinfo is used. There was added new argument for get_dev() to enable device ID propagation. --- src/df.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/df.c b/src/df.c index e907b94..156c7c1 100644 --- a/src/df.c +++ b/src/df.c @@ -622,11 +622,11 @@ filter_mount_list (bool devices_only) struct devlist *devlist; struct mount_entry *discard_me = NULL; - /* 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. */ +#ifdef _linux_ + if (me->me_dev == (dev_t) -1 && stat (me->me_mountdir, &buf) == -1) +#else if (-1 == stat (me->me_mountdir, &buf)) +#endif { /* Stat failed - add ME to be able to complain about it later. */ buf.st_dev = me->me_dev; @@ -870,13 +870,14 @@ add_to_grand_total (struct field_values_t *bv, struct field_values_t *iv) If MOUNT_POINT is non-NULL, then DISK may be NULL -- certain systems may not be able to produce statistics in this case. ME_DUMMY and ME_REMOTE are the mount entry flags. + ME_DEV is mount entry device id or -1. Caller must set PROCESS_ALL to true when iterating over all entries, as when df is invoked with no non-option argument. See below for details. */ static void get_dev (char const *disk, char const *mount_point, char const* file, char const *stat_file, char const *fstype, - bool me_dummy, bool me_remote, + bool me_dummy, bool me_remote, dev_t me_dev, const struct fs_usage *force_fsu, bool process_all) { @@ -930,9 +931,15 @@ get_dev (char const *disk, char const *mount_point, char const* file, /* Ensure we don't output incorrect stats for over-mounted directories. Discard stats when the device name doesn't match. */ struct stat sb; +#ifdef _linux_ + if (me_dev != (dev_t) -1 || stat (stat_file, &sb) == 0) + { +#else if (stat (stat_file, &sb) == 0) { - char const * devname = devname_for_dev (sb.st_dev); + me_dev = sb.st_dev; +#endif + char const * devname = devname_for_dev (me_dev); if (devname && ! STREQ (devname, disk)) { fstype = "-"; @@ -1212,7 +1219,7 @@ get_disk (char const *disk) { get_dev (best_match->me_devname, best_match->me_mountdir, file, NULL, best_match->me_type, best_match->me_dummy, - best_match->me_remote, NULL, false); + best_match->me_remote, best_match->me_dev, NULL, false); return true; } else if (eclipsed_device) @@ -1307,7 +1314,7 @@ get_point (const char *point, const struct stat *statp) if (best_match) get_dev (best_match->me_devname, best_match->me_mountdir, point, point, best_match->me_type, best_match->me_dummy, best_match->me_remote, - NULL, false); + best_match->me_dev, NULL, false); else { /* We couldn't find the mount entry corresponding to POINT. Go ahead and @@ -1318,7 +1325,8 @@ get_point (const char *point, const struct stat *statp) char *mp = find_mount_point (point, statp); if (mp) { - get_dev (NULL, mp, point, NULL, NULL, false, false, NULL, false); + get_dev (NULL, mp, point, NULL, NULL, false, false, + (dev_t) -1, NULL, false); free (mp); } } @@ -1349,7 +1357,7 @@ get_all_entries (void) for (me = mount_list; me; me = me->me_next) get_dev (me->me_devname, me->me_mountdir, NULL, NULL, me->me_type, - me->me_dummy, me->me_remote, NULL, true); + me->me_dummy, me->me_remote, me->me_dev, NULL, true); } /* Add FSTYPE to the list of file system types to display. */ @@ -1699,8 +1707,8 @@ main (int argc, char **argv) { if (print_grand_total) get_dev ("total", - (field_data[SOURCE_FIELD].used ? "-" : "total"), - NULL, NULL, NULL, false, false, &grand_fsu, false); + (field_data[SOURCE_FIELD].used ? "-" : "total"), NULL, + NULL, NULL, false, false, (dev_t) -1, &grand_fsu, false); print_table (); } -- 1.9.3
