Subject: Re: bug#17833: coreutils 8.22 df
From: Pádraig Brady <[email-masked]>
To: "Gabor Z. Papp" <[email-masked]>
Cc: [email-masked]
Date: Fri, 11 Jul 2014 00:28:34 +0100
List-Id: <debbugs-submit.debbugs.gnu.org>

On 07/08/2014 06:28 PM, Pádraig Brady wrote:
> On 06/25/2014 01:24 PM, Gabor Z. Papp wrote:
>> * Pádraig Brady <[email-masked]>:
>>
>> | > $ cat /etc/mtab 
>> | > /dev/md0 / ext3 rw,errors=remount-ro 0 0
>> | > proc /proc proc rw,relatime 0 0
>> | > sysfs /sys sysfs rw,relatime 0 0
>> | > tmpfs /dev tmpfs rw,relatime,size=4k,mode=755 0 0
>> | > devpts /dev/pts devpts rw,relatime,mode=620 0 0
>> | > /dev/md1 /var/archive ext3 rw,errors=remount-ro 0 1
>> | > /var/archive/home /home none rw,bind 0 0
>> | > /var/archive/spool /var/spool none rw,bind 0 0
>> | > /var/archive/www /var/lib/www none rw,bind 0 0
>>
>> | Is this mtab really a link to /proc/mounts?
>>
>> No, its a standalone file.
>>
>> | That's the normal case on modern Linux distros.
>>
>> Its not a modern linux. :-)
>>
>> | > $ df-8.22 -h
>> | > Filesystem      Size  Used Avail Use% Mounted on
>> | > /dev/md0         21G  8.8G   11G  46% /
>> | > tmpfs           4.0K     0  4.0K   0% /dev
>> | > 
>> | > And where is /dev/md1 ?
>>
>> | Not sure what's going on here.
>> | Since you've compiled various versions,
>> | could you replace src/df.c in your 8.22 build dir with:
>> | http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob_plain;f=src/df.c;hb=HEAD
>> | There have been many df changes since 8.22, so it would
>> | be good to check with the latest.
>>
>> $ /tmp/df-git -h
>> Filesystem      Size  Used Avail Use% Mounted on
>> /dev/md0         21G  9.0G   11G  47% /
>> tmpfs           4.0K     0  4.0K   0% /dev
>>
>> Same problem, /dev/md1 missing.
> 
> The missing entry is due to the bind mounts being classified as "dummy" devices.
> There is code in gnulib to explicitly avoid that for bind mounts,
> however that was negated with this change:
> http://git.sv.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=62bb7a8b
> Now that change is correct in isolation, but when combined
> with the incomplete original change, it triggered the issue.
> The attached patch should fix this up.
> 
>>
>> | > running df from coreutils 8.21 shows different result:
>> | > 
>> | > $ df-8.21 -h
>> | > Filesystem         Size  Used Avail Use% Mounted on
>> | > /dev/md0            21G  8.8G   11G  46% /
>> | > tmpfs              4.0K     0  4.0K   0% /dev
>> | > /var/archive/home  273G  244G   29G  90% /home
>> | > 
>> | > Still weird.
>>
>> | As part of the device de-duplication logic we used in 8.21
>> | we're favoring '/home' since it's shorter than '/var/archive'
>> | and thus we're using the '/var/archive/home' "device".
>>
>> | > Trying with 8.20:
>> | > 
>> | > $ df-8.20 -h
>> | > Filesystem      Size  Used Avail Use% Mounted on
>> | > /dev/md0         21G  8.8G   11G  46% /
>> | > tmpfs           4.0K     0  4.0K   0% /dev
>> | > /dev/md1        273G  244G   29G  90% /var/archive
>>
>> | I agree that this is the best output for your case.
>>
>> Right.
>>
>> | I suppose we could stat the sources in the de-duplication logic
>> | to favor real devices. I.E. stat '/var/archive/home' and '/dev/md1'
>> | in your case to favor the latter.
> 
> That's something we could still do in addition in coreutils.

The attached should handle this by giving precedence
to displaying real device nodes.

thanks,
Pádraig.
From 840e7afb6d7d499d52aaac0f9d34cb90208b2183 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Thu, 10 Jul 2014 23:59:35 +0100
Subject: [PATCH] df: give precedence to real device nodes

This is significant when /etc/mtab is a real file
rather than a symlink to /proc/mounts, in which case
the bind mounted dir will be listed as the source "device"
rather than the real base device.  Therefore we stat
the device node, which should be local and a relatively
cheap operation, and give precedence to real device nodes.

For example given this setup:

  truncate -s10M bind.img
  mkfs -F bind.img
  mount bind.img /var/archive/
  mkdir -p /var/archive/home
  mkdir -p /t/home
  mount --bind /var/archive/home/ /t/home/

on an older system without /proc/mounts we would have displayed:

  Filesystem            Mounted on
  /var/archive/home/    /t/home

but we now display:

  Filesystem            Mounted on
  /dev/loop0            /var/archive

Note on a newer system using /proc/mounts we display:

  Filesystem            Mounted on
  /dev/loop0            /t/home

* src/df.c (filter_mount_list): stat() the device node for
each mount entry and use that to give precedence to real
device nodes.
* NEWS: Mention the bug fix.
Fixes http://bugs.gnu.org/17833
---
 NEWS     |    5 +++++
 src/df.c |   35 ++++++++++++++++++++++++-----------
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/NEWS b/NEWS
index 4e90b79..e942fe8 100644
--- a/NEWS
+++ b/NEWS
@@ -51,6 +51,11 @@ GNU coreutils NEWS                                    -*- outline -*-
   than error messages or values for the wrong file system.
   [These bugs were present in "the beginning".]
 
+  df now give precedence to displaying real device nodes in the presence of
+  bind mounts, on systems where the base device is not listed for all entries
+  in the mount list.
+  [bug introduced in coreutils-8.21]
+
   du now silently ignores directory cycles introduced with bind mounts.
   Previously it would issue a warning and exit with a failure status.
   [bug introduced in coreutils-8.1]
diff --git a/src/df.c b/src/df.c
index 063cabf..46aeef9 100644
--- a/src/df.c
+++ b/src/df.c
@@ -48,6 +48,7 @@
 static struct devlist
 {
   dev_t dev_num;
+  bool real_dev;
   struct mount_entry *me;
   struct devlist *next;
 } *device_list;
@@ -618,35 +619,46 @@ filter_mount_list (bool devices_only)
   /* Sort all 'wanted' entries into the list device_list.  */
   for (me = mount_list; me;)
     {
-      struct stat buf;
+      struct stat mnt_buf;
       struct devlist *devlist;
       struct mount_entry *discard_me = NULL;
+      bool real_dev = false;
 
       /* 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.  */
-      if (-1 == stat (me->me_mountdir, &buf))
+      if (-1 == stat (me->me_mountdir, &mnt_buf))
         {
           /* Stat failed - add ME to be able to complain about it later.  */
-          buf.st_dev = me->me_dev;
+          mnt_buf.st_dev = me->me_dev;
         }
       else
         {
+          /* when /etc/mtab is not linked to /proc/mounts,
+             then in the presence of bind mounts, the source will
+             be the bind mounted directory, rather than the base device.
+             In this case we want to give precedence to the base device.  */
+          struct stat sb;
+          real_dev = (! me->me_dummy
+                      && stat (me->me_devname, &sb) == 0
+                      && (S_ISBLK (sb.st_mode) || S_ISCHR (sb.st_mode)));
+
           /* If we've already seen this device...  */
           for (devlist = device_list; devlist; devlist = devlist->next)
-            if (devlist->dev_num == buf.st_dev)
+            if (devlist->dev_num == mnt_buf.st_dev)
               break;
 
           if (devlist)
             {
               /* ...let the shorter mountdir win.  */
-              if ((strchr (me->me_devname, '/')
-                   && ! strchr (devlist->me->me_devname, '/'))
-                  || (strlen (devlist->me->me_mountdir)
-                      > strlen (me->me_mountdir))
-                  /* or one overmounted on a different device.  */
-                  || ! STREQ (devlist->me->me_devname, me->me_devname))
+              if (real_dev >= devlist->real_dev
+                  && ((strchr (me->me_devname, '/')
+                       && ! strchr (devlist->me->me_devname, '/'))
+                      || (strlen (devlist->me->me_mountdir)
+                          > strlen (me->me_mountdir))
+                      /* or one overmounted on a different device.  */
+                      || ! STREQ (devlist->me->me_devname, me->me_devname)))
                 {
                   /* Discard mount entry for existing device.  */
                   discard_me = devlist->me;
@@ -672,7 +684,8 @@ filter_mount_list (bool devices_only)
           /* Add the device number to the global list devlist.  */
           devlist = xmalloc (sizeof *devlist);
           devlist->me = me;
-          devlist->dev_num = buf.st_dev;
+          devlist->dev_num = mnt_buf.st_dev;
+          devlist->real_dev = real_dev;
           devlist->next = device_list;
           device_list = devlist;
 
-- 
1.7.7.6