On 01/27/2014 01:06 AM, Pádraig Brady wrote:
> On 01/26/2014 11:35 PM, Bernhard Voelker wrote:
>> On 01/26/2014 12:28 PM, Pádraig Brady wrote:
>>> On 01/25/2014 11:55 PM, Bernhard Voelker wrote:
>>>> However, I remember some other corner cases with eclipsed file
>>>> systems in the Fedora bug tracker. I think we're quite close
>>>> to solve them all this time (hopefully).
>>>> The idea was to trust the order of mount entries returned by
>>>> the kernel, i.e. in the loop over the mount entries, if the
>>>> mount point is the same one as a previous one, then we should
>>>> process the one mounted later.
>>>>
>>>> E.g. the situation where 2 file systems are mounted on the
>>>> same mount point:
>>>>
>>>> $ findmnt | grep loop
>>>> └─/mnt /dev/loop0 ext4
>>>> rw,relatime,data=ordered
>>>> └─/mnt/dir /dev/loop1 ext4
>>>> rw,relatime,data=ordered
>>>> └─/mnt/dir /dev/loop2 ext4
>>>> rw,relatime,data=ordered
>>>>
>>>> df - the new one with your patch - still shows the wrong device:
>>>>
>>>> $ src/df | grep loop
>>>> /dev/loop0 122835 1551 112110 2% /mnt
>>>> /dev/loop1 122835 1550 112111 2% /mnt/dir
>>>>
>>>> It should say /dev/loop2 here. BTW the numbers are correct.
>>
>> BTW: the fstype is wrong, too (which can only be seen with -T or --output,
>> and if it differs, of course).
>>
>>> Right, that could be handled easy enough.
>>> loop1 is not accessible above and so should be hidden.
>>> But consider a bind mount resulting in something like:
>>>
>>>> └─/mnt /dev/loop0 ext4
>>>> rw,relatime,data=ordered
>>>> └─/mnt/dir /dev/loop1 ext4
>>>> rw,relatime,data=ordered
>>>> └─/some/place/else /dev/loop1 ext4
>>>> rw,relatime,data=ordered
>>>> └─/mnt/dir /dev/loop2 ext4
>>>> rw,relatime,data=ordered
>>>
>>> If we did a linear scan through that, we'd lose the /some/place/else
>>> due to it being a longer mount dir, and then also the original loop1
>>> as we took /dev/loop2 for /mnt/dir.
>>> Seems like when discarding we would need to see if this was the
>>> last entry for a device and then see if there are any other candidate
>>> mount points for that device?
>>
>> Hi Padraig,
>>
>> thanks.
>> Again, mount_list is a little beast - more below.
>>
>> The following patch (on top of yours) would handle both cases
>> without a problem. Feel free to squash it in, if you like.
>>
>> diff --git a/src/df.c b/src/df.c
>> index 23b5156..78768cc 100644
>> --- a/src/df.c
>> +++ b/src/df.c
>> @@ -631,9 +631,20 @@ filter_mount_list (void)
>> else
>> {
>> /* If we've already seen this device... */
>> + struct devlist *d = NULL;
>> for (devlist = devlist_head; devlist; devlist = devlist->next)
>> if (devlist->dev_num == buf.st_dev)
>> - break;
>> + {
>> + d = devlist;
>> + if (!STREQ (devlist->me->me_devname, me->me_devname))
>> + {
>> + /* Fix the devname if the mount dir has been
>> + mounted over by a different devname. */
>> + free (devlist->me->me_devname);
>> + devlist->me->me_devname = xstrdup (me->me_devname);
>> + }
>> + }
>> + devlist = d;
>>
>> if (devlist)
>> {
>>
>> But there is yet another issue with the -a mode for such
>> over-mounted and therefore eclipsed file systems:
>>
>> # Create 2 file system images: 1 ext4, 1 xfs.
>> $ dd if=/dev/zero bs=1M status=none count=128 of=img1
>> $ dd if=/dev/zero bs=1M status=none count=256 of=img2
>> $ mkfs -t ext4 -F img1 >/dev/null 2>&1
>> $ mkfs -t xfs -f img2 >/dev/null 2>&1
>> $ mkdir /mnt{1,2}
>>
>> # Mount both on /mnt1.
>> $ mount -o loop img1 /mnt1
>> $ mount -o loop img2 /mnt1
>>
>> # Mount the former (ext4) also on /mnt2 via its loop device.
>> $ mount /dev/loop0 /mnt2
>>
>> # Result:
>> $ findmnt --output=TARGET,SOURCE,FSTYPE | grep loop
>> ├─/mnt1 /dev/loop0 ext4
>> │ └─/mnt1 /dev/loop1 xfs
>> └─/mnt2 /dev/loop0 ext4
>>
>> Everything is fine now with the filtered df run ...
>>
>> $ src/df --out -h | grep loop
>> /dev/loop1 xfs 256K 3 256K 1% 252M 13M 239M 6% -
>> /mnt1
>> /dev/loop0 ext4 32K 11 32K 1% 120M 1.6M 110M 2% -
>> /mnt2
>>
>> ...but "df -a" prints the wrong statistics for the "over-mounted" /mnt1!
>>
>> $ src/df --out -h -a | grep loop
>> /dev/loop0 ext4 256K 3 256K 1% 252M 13M
>> 239M 6% - /mnt1
>> /dev/loop1 xfs 256K 3 256K 1% 252M 13M
>> 239M 6% - /mnt1
>> /dev/loop0 ext4 32K 11 32K 1% 120M 1.6M
>> 110M 2% - /mnt2
>>
>> Okay, this is nothing new.
>> BTW: strictly speaking, also the output of today's "df -t rootfs -a"
>> is wrong because the numbers are definitely not that of the early-boot
>> rootfs file system.
>>
>> Now, how should df handle this?
>>
>> a)
>> df silently filters out the mount entries of all eclipsed mount dirs,
>> even with -a.
>> --> Hmm, I think this would probably contradict to POSIX.
>>
>> b)
>> df prints an error diagnostic for each eclipsed mount dir, and exits
>> non-Zero.
>> --> Well, there are probably such mounts on every system, e.g. on my box:
>>
>> TARGET SOURCE FSTYPE
>> /proc/sys/fs/binfmt_misc systemd-1 autofs
>> /proc/sys/fs/binfmt_misc binfmt_misc binfmt_misc
>>
>> Therefore, a "df -a" would always fail. ;-(
>> At least on my system, there are
>>
>> c)
>> df prints a warning diagnostic for each eclipsed mount dir, and exits
>> Zero (unless another error occurs).
>>
>> --> Due to the same reason as in b), these warning might be messy
>> and users will probably be irritated.
>>
>> d)
>> df outputs "-" for all numbers of such eclipsed file systems, e.g.
>>
>> $ src/df --out -h -a | grep mnt1
>> /dev/loop0 ext4 - - - - - -
>> - - - /mnt1
>> /dev/loop1 xfs 256K 3 256K 1% 252M 13M
>> 239M 6% - /mnt1
>>
>>
>> Maybe d) is the best solution, as it mirrors what df can know:
>> it knows source, target and the file system type, but it doesn't
>> have access to the block and inode numbers.
>>
>> WDYT?
>
> Thanks for the nice analysis and tests.
> d) seems like the best option here, though we'd have to be careful
> about cases where /proc/mounts was giving a system wide view,
> while df wasn't privy to that due to mount namespaces or
> overmounts etc. I'm not thinking of a specific issue here,
> just the general problem.
>
> wrt c) and annoying warnings, I also notice `df -a` on a default Fedora 20
> install here,
> giving multiple duplicate warnings like:
> df: ‘net:[4026532416]’: No such file or directory
> df: ‘net:[4026532416]’: No such file or directory
> That's due to:
> $ grep net: /proc/mounts
> proc net:[4026532416] proc rw,nosuid,nodev,noexec,relatime 0 0
> proc net:[4026532416] proc rw,nosuid,nodev,noexec,relatime 0 0
> Which is due to support for namespaces.
> Seems like we should not try to lookup non absolute mount points?
Hi Bernhard,
I've attached 4 patches for df to:
df: also deduplicate virtual file systems
df: fix handling of symlinks in mount list
df: ignore non file system entries in /proc/mounts
maint: avoid clang -Wtautological-constant-out-of-range-compare warning
Not included is your "overmount change" or the "d)" adjustment above,
as I was unsure how you wanted to handle exactly.
thanks,
Pádraig.
>From 62112df539326d9f874ad44a94f70b2a9749a975 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Sat, 25 Jan 2014 01:14:29 +0000
Subject: [PATCH 1/4] df: also deduplicate virtual file systems
* src/df.c (filter_mountlist): Remove the constraint that
a '/' needs to be in the device name for a mount entry to
be considered for deduplication. Virtual file systems also
have storage associated with them (like tmpfs for example),
and thus need to be deduplicated since they will be shown
in the default df output and subject to --total processing also.
* test/df/skip-duplicates.sh: Add a test to ensure we deduplicate
all entries, even for virtual file systems. Also avoid possible
length operations on many remote file systems in the initial
check of df operation. Also avoid the assumption that "/root"
is on the same file system as "/".
* NEWS: Mention the change in behavior.
---
NEWS | 2 +
src/df.c | 31 ++++++++++++---------------
tests/df/skip-duplicates.sh | 47 +++++++++++++++++++++++++++++++-----------
3 files changed, 50 insertions(+), 30 deletions(-)
diff --git a/NEWS b/NEWS
index 4efd60d..c204b68 100644
--- a/NEWS
+++ b/NEWS
@@ -44,6 +44,8 @@ GNU coreutils NEWS -*- outline -*-
[These dd bugs were present in "the beginning".]
+ df now correctly elides duplicates for virtual file systems like tmpfs.
+
head --bytes=-N and --lines=-N now handles devices more
consistently, not ignoring data from virtual devices like /dev/zero,
or on BSD systems data from tty devices.
diff --git a/src/df.c b/src/df.c
index e763943..2b5a54e 100644
--- a/src/df.c
+++ b/src/df.c
@@ -630,26 +630,23 @@ filter_mount_list (void)
}
else
{
- /* If the device name is a real path name ... */
- if (strchr (me->me_devname, '/'))
+ /* If we've already seen this device... */
+ for (devlist = devlist_head; devlist; devlist = devlist->next)
+ if (devlist->dev_num == buf.st_dev)
+ break;
+
+ if (devlist)
{
- /* ... try to find its device number in the devlist. */
- for (devlist = devlist_head; devlist; devlist = devlist->next)
- if (devlist->dev_num == buf.st_dev)
- break;
+ discard_me = me;
- 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)))
{
- discard_me = me;
-
- /* Let the shorter mountdir win. */
- if (! strchr (devlist->me->me_devname, '/')
- || (strlen (devlist->me->me_mountdir)
- > strlen (me->me_mountdir)))
- {
- discard_me = devlist->me;
- devlist->me = me;
- }
+ discard_me = devlist->me;
+ devlist->me = me;
}
}
}
diff --git a/tests/df/skip-duplicates.sh b/tests/df/skip-duplicates.sh
index 266520a..d872f27 100755
--- a/tests/df/skip-duplicates.sh
+++ b/tests/df/skip-duplicates.sh
@@ -21,19 +21,28 @@
print_ver_ df
require_gcc_shared_
-df || skip_ "df fails"
+# We use --local here so as to not activate
+# potentially very many remote mounts.
+df --local || skip_ "df fails"
-# Simulate an mtab file with two entries of the same device number.
-# Also add entries with unstatable mount dirs to ensure that's handled.
+export CU_NONROOT_FS=$(df --local --output=target 2>&1 | grep /. | head -n1)
+test -z "$CU_NONROOT_FS" && unique_entries=1 || unique_entries=2
+
+# Simulate an mtab file to test various cases.
cat > k.c <<'EOF' || framework_failure_
#include <stdio.h>
#include <stdlib.h>
+#include <string.h>
#include <mntent.h>
+#define STREQ(a, b) (strcmp (a, b) == 0)
+
struct mntent *getmntent (FILE *fp)
{
+ static char *nonroot_fs;
+ static int done;
+
/* Prove that LD_PRELOAD works. */
- static int done = 0;
if (!done)
{
fclose (fopen ("x", "w"));
@@ -43,18 +52,30 @@ struct mntent *getmntent (FILE *fp)
static struct mntent mntents[] = {
{.mnt_fsname="/short", .mnt_dir="/invalid/mount/dir"},
{.mnt_fsname="fsname", .mnt_dir="/",},
- {.mnt_fsname="/fsname", .mnt_dir="/root"},
+ {.mnt_fsname="/fsname", .mnt_dir="/."},
{.mnt_fsname="/fsname", .mnt_dir="/"},
+ {.mnt_fsname="virtfs", .mnt_dir="/NONROOT"},
+ {.mnt_fsname="virtfs", .mnt_dir="/NONROOT"},
};
- if (!getenv ("CU_TEST_DUPE_INVALID") && done == 1)
+ if (done == 1)
+ {
+ nonroot_fs = getenv ("CU_NONROOT_FS");
+ if (!nonroot_fs || !*nonroot_fs)
+ nonroot_fs = "/"; /* merge into / entries. */
+ }
+
+ if (done == 1 && !getenv ("CU_TEST_DUPE_INVALID"))
done++; /* skip the first entry. */
- while (done++ <= 4)
+ while (done++ <= 6)
{
mntents[done-2].mnt_type = "-";
+ if (STREQ (mntents[done-2].mnt_dir, "/NONROOT"))
+ mntents[done-2].mnt_dir = nonroot_fs;
return &mntents[done-2];
}
+
return NULL;
}
EOF
@@ -69,22 +90,22 @@ test -f x || skip_ "internal test failure: maybe LD_PRELOAD doesn't work?"
# The fake mtab file should only contain entries
# having the same device number; thus the output should
-# consist of a header and one entry.
+# consist of a header and unique entries.
LD_PRELOAD=./k.so df >out || fail=1
-test $(wc -l <out) -eq 2 || { fail=1; cat out; }
+test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; }
# Ensure we fail when unable to stat invalid entries
LD_PRELOAD=./k.so CU_TEST_DUPE_INVALID=1 df >out && fail=1
-test $(wc -l <out) -eq 2 || { fail=1; cat out; }
+test $(wc -l <out) -eq $(expr 1 + $unique_entries) || { fail=1; cat out; }
# df should also prefer "/fsname" over "fsname"
test $(grep -c '/fsname' <out) -eq 1 || { fail=1; cat out; }
-# ... and "/fsname" with '/' as Mounted on over '/root'
-test $(grep -c '/root' <out) -eq 0 || { fail=1; cat out; }
+# ... and "/fsname" with '/' as Mounted on over '/.'
+test $(grep -cF '/.' <out) -eq 0 || { fail=1; cat out; }
# Ensure that filtering duplicates does not affect -a processing.
LD_PRELOAD=./k.so df -a >out || fail=1
-test $(wc -l <out) -eq 4 || { fail=1; cat out; }
+test $(wc -l <out) -eq 6 || { fail=1; cat out; }
# Ensure that filtering duplicates does not affect
# argument processing (now without the fake getmntent()).
--
1.7.7.6
>From 3449cad1576a3d584b248524bd03ed1b3d6bcb0a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Mon, 12 May 2014 13:29:01 +0100
Subject: [PATCH 2/4] df: fix handling of symlinks in mount list
The symlink handling in commit v8.21-172-g33660b4 was incomplete
in the case where there were symlinks in the mount list itself.
For example, in the case where /dev/mapper/fedora-home was in the
mount list and that in turn was a symlink to /dev/dm-2, we have:
before> df --out=source /dev/mapper/fedora-home
devtmpfs
after > df --out=source /dev/mapper/fedora-home
/dev/mapper/fedora-home
* src/df.c (get_disk): Compare canonicalized device names from
the mount list. Note we still display the non canonicalized name,
even if longer, as we assume that is the most representative.
* tests/df/df-symlink.sh: This could theoretically fail on some systems
depending on the content of the mount list, but adjust to fail on any
system where symlinks are present in the mount list for the current dir.
---
src/df.c | 12 ++++++++++--
tests/df/df-symlink.sh | 7 +++++++
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/src/df.c b/src/df.c
index 2b5a54e..24897a3 100644
--- a/src/df.c
+++ b/src/df.c
@@ -1056,13 +1056,19 @@ get_disk (char const *disk)
char const *file = disk;
char *resolved = canonicalize_file_name (disk);
- if (resolved && resolved[0] == '/')
+ if (resolved && IS_ABSOLUTE_FILE_NAME (resolved))
disk = resolved;
size_t best_match_len = SIZE_MAX;
for (me = mount_list; me; me = me->me_next)
{
- if (STREQ (disk, me->me_devname))
+ /* TODO: Should cache canon_dev in the mount_entry struct. */
+ char *devname = me->me_devname;
+ char *canon_dev = canonicalize_file_name (me->me_devname);
+ if (canon_dev && IS_ABSOLUTE_FILE_NAME (canon_dev))
+ devname = canon_dev;
+
+ if (STREQ (disk, devname))
{
size_t len = strlen (me->me_mountdir);
if (len < best_match_len)
@@ -1074,6 +1080,8 @@ get_disk (char const *disk)
best_match_len = len;
}
}
+
+ free (canon_dev);
}
free (resolved);
diff --git a/tests/df/df-symlink.sh b/tests/df/df-symlink.sh
index aaed810..6d96bd2 100755
--- a/tests/df/df-symlink.sh
+++ b/tests/df/df-symlink.sh
@@ -28,4 +28,11 @@ df --out=source,target "$disk" > exp || skip_ "cannot get info for $disk"
df --out=source,target symlink > out || fail=1
compare exp out || fail=1
+# Ensure we output the same values for device nodes and '.'
+# This was not the case in coreutil-8.22 on systems
+# where the device in the mount list was a symlink itself.
+# I.E. '.' => /dev/mapper/fedora-home -> /dev/dm-2
+df --out=source,target '.' > out || fail=1
+compare exp out || fail=1
+
Exit $fail
--
1.7.7.6
>From 3e6da84f0af81b2a8a9539b14889f299680d7f7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Mon, 12 May 2014 14:49:13 +0100
Subject: [PATCH 3/4] df: ignore non file system entries in /proc/mounts
Linux with network namespaces contains entries in /proc/mounts like:
proc net:[4026532464] proc rw,nosuid,nodev,noexec,relatime 0 0
resulting in a failure to stat 'net:[...]', inducing a warning
and an exit with failure status.
* src/df.c (get_dev): Ignore all relative mount points.
* tests/df/skip-duplicates.sh: Add an entry to test relative dirs.
---
src/df.c | 5 +++++
tests/df/skip-duplicates.sh | 3 ++-
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/src/df.c b/src/df.c
index 24897a3..a7fc57f 100644
--- a/src/df.c
+++ b/src/df.c
@@ -853,6 +853,11 @@ get_dev (char const *disk, char const *mount_point, char const* file,
if (!selected_fstype (fstype) || excluded_fstype (fstype))
return;
+ /* Ignore relative MOUNT_POINTs, which are present for example
+ in /proc/mounts on Linux with network namespaces. */
+ if (!force_fsu && mount_point && ! IS_ABSOLUTE_FILE_NAME (mount_point))
+ return;
+
/* If MOUNT_POINT is NULL, then the file system is not mounted, and this
program reports on the file system that the special file is on.
It would be better to report on the unmounted file system,
diff --git a/tests/df/skip-duplicates.sh b/tests/df/skip-duplicates.sh
index d872f27..44f7d4c 100755
--- a/tests/df/skip-duplicates.sh
+++ b/tests/df/skip-duplicates.sh
@@ -56,6 +56,7 @@ struct mntent *getmntent (FILE *fp)
{.mnt_fsname="/fsname", .mnt_dir="/"},
{.mnt_fsname="virtfs", .mnt_dir="/NONROOT"},
{.mnt_fsname="virtfs", .mnt_dir="/NONROOT"},
+ {.mnt_fsname="netns", .mnt_dir="net:[1234567]"},
};
if (done == 1)
@@ -68,7 +69,7 @@ struct mntent *getmntent (FILE *fp)
if (done == 1 && !getenv ("CU_TEST_DUPE_INVALID"))
done++; /* skip the first entry. */
- while (done++ <= 6)
+ while (done++ <= 7)
{
mntents[done-2].mnt_type = "-";
if (STREQ (mntents[done-2].mnt_dir, "/NONROOT"))
--
1.7.7.6
>From 6e4f211d62110a719a42b13da59e5c9b073f29fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email protected]>
Date: Mon, 12 May 2014 15:46:43 +0100
Subject: [PATCH 4/4] maint: avoid clang
-Wtautological-constant-out-of-range-compare warning
* src/df.c (decode_output_arg): Use only enum constants to avoid
clang "warning: comparison of constant -1 with expression of
type 'display_field_t' is always false"
---
src/df.c | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/df.c b/src/df.c
index a7fc57f..01ecca6 100644
--- a/src/df.c
+++ b/src/df.c
@@ -144,7 +144,8 @@ typedef enum
IAVAIL_FIELD, /* inodes available */
IPCENT_FIELD, /* inodes used in percent */
TARGET_FIELD, /* mount point */
- FILE_FIELD /* specified file name */
+ FILE_FIELD, /* specified file name */
+ INVALID_FIELD /* validation marker */
} display_field_t;
/* Flag if a field contains a block, an inode or another value. */
@@ -372,7 +373,7 @@ decode_output_arg (char const *arg)
*comma++ = 0;
/* process S. */
- display_field_t field = -1;
+ display_field_t field = INVALID_FIELD;
for (unsigned int i = 0; i < ARRAY_CARDINALITY (field_data); i++)
{
if (STREQ (field_data[i].arg, s))
@@ -381,7 +382,7 @@ decode_output_arg (char const *arg)
break;
}
}
- if (field == -1)
+ if (field == INVALID_FIELD)
{
error (0, 0, _("option --output: field %s unknown"), quote (s));
usage (EXIT_FAILURE);
--
1.7.7.6
