Re: statvfs suggestion

Re: statvfs suggestion

[Ulrich Drepper]

Aharon Robbins wrote:
> Isn't the real solution to get statvfs into the kernel?  statfs already
> does most of the work; the problem is geting the mount options.

Well, feel free to try it.  Welcome to the world of kernel politics.

-- 
--------------.                        ,-.            444 Castro Street
Ulrich Drepper \    ,-----------------'   \ Mountain View, CA 94041 USA
Red Hat         `--' drepper at redhat.com `---------------------------

statvfs suggestion

[Jim Meyering]

Hi Uli,

[There's no pressure at all, since the coreutils work around this problem
 simply by using statfs on glibc systems, but I feel I should at least
 report the problem.  Other applications may well be affected. ]

For some time, now the coreutils have been avoiding the use of glibc's
statvfs because it has an undesirable side-effect in some cases.
When df used statvfs, it would sometimes hang unnecessarily due to a
problem with an unrelated hard mount.  That was because statvfs stats
a directory for each entry in /proc/mounts, in order, until it finds
one with matching device number.  Of course, that would make df hang if
even one of the file systems listed in /proc/mounts before the desired
one is hard-mounted, but not available.

What do you think about changing internal_statvfs.c so that it first
reads/saves all entries from /proc/mounts without stat'ing each
corresponding directory?  Then it can stop if any mount point matches
the name of the file.

This is a relatively important optimization, not just for the case
mentioned above that would make df hang unnecessarily, but also to avoid
the potential O(N^2) stat/mount operations that you'd currently incur
when iterating with getmntent, and calling statvfs on each mount point.

Here's some actual code demonstrating the problem, but not the hang:

  $ cat statvfs-glibc-test.c
  #include <stdlib.h>
  #include <sys/statvfs.h>

  int
  main (int argc, char **argv)
  {
    struct statvfs fsd;
    exit (statvfs (argv[1], &fsd));
  }

Note that it will stat the 10 or so directories
before the desired one, while it could have stat'ed only
the selected one in this case, since the file argument, "/my",
to statvfs matches the name of a mount point:

  $ cat /proc/mounts
  rootfs / rootfs rw 0 0
  /dev2/root2 / ext3 rw 0 0
  proc /proc proc rw 0 0
  devpts /dev/pts devpts rw 0 0
  /dev/hda5 /tmp ext3 rw 0 0
  /dev/hda3 /_boot ext3 rw 0 0
  /dev/hda2 /var ext3 rw 0 0
  /dev/hda7 /u ext3 rw 0 0
  tmpfs /t tmpfs rw 0 0
  /dev/hdc2 /misc ext3 rw 0 0
  /dev/hdc3 /nobak ext3 rw 0 0
  /dev/hdc4 /my ext3 rw 0 0
  /dev/hde2 /e-root ext3 rw 0 0
  /dev/hde3 /e ext3 rw 0 0
  /dev/hdg2 /g-root ext3 rw 0 0
  /dev/hdg3 /g ext3 rw 0 0
  automount(pid475) /mnt/auto autofs rw 0 0
  usb /proc/bus/usb usbdevfs rw 0 0

  $ strace -e stat64,open ./a.out /my
  ...
  stat64("/my", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  open("/proc/mounts", O_RDONLY)          = 3
  stat64("/", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  stat64("/", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  stat64("/proc", {st_mode=S_IFDIR|0555, st_size=0, ...}) = 0
  stat64("/dev/pts", {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
  stat64("/tmp", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=8192, ...}) = 0
  stat64("/_boot", {st_mode=S_IFDIR|0755, st_size=1024, ...}) = 0
  stat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  stat64("/u", {st_mode=S_IFDIR|S_ISGID|0775, st_size=4096, ...}) = 0
  stat64("/t", {st_mode=S_IFDIR|S_ISVTX|0777, st_size=340, ...}) = 0
  stat64("/misc", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  stat64("/nobak", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
  stat64("/my", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0

Jim

Re: statvfs suggestion

[Ulrich Drepper]

Jim Meyering wrote:

> What do you think about changing internal_statvfs.c so that it first
> reads/saves all entries from /proc/mounts without stat'ing each
> corresponding directory?  Then it can stop if any mount point matches
> the name of the file.

This really is no solution.  The name passed to statvfs need not be the
actual mount point.  And what about fstatvfs where we don't have a name?

I added some code which cuts down on the number of stats by comparing
another value we know must match: the filesystem name.

While doing this, though, I found that adding additional tests for the
name comparison isn't any more work so I added this as well.  It's no
solution, but it might help in one situation or another.  The result is
in CVS.

-- 
--------------.                        ,-.            444 Castro Street
Ulrich Drepper \    ,-----------------'   \ Mountain View, CA 94041 USA
Red Hat         `--' drepper at redhat.com `---------------------------

Re: statvfs suggestion

[Jim Meyering]

Ulrich Drepper <drepper@redhat.com> wrote:
> Jim Meyering wrote:
>> What do you think about changing internal_statvfs.c so that it first
>> reads/saves all entries from /proc/mounts without stat'ing each
>> corresponding directory?  Then it can stop if any mount point matches
>> the name of the file.
>
> This really is no solution.  The name passed to statvfs need not be the
> actual mount point.  And what about fstatvfs where we don't have a name?
>
> I added some code which cuts down on the number of stats by comparing
> another value we know must match: the filesystem name.
>
> While doing this, though, I found that adding additional tests for the
> name comparison isn't any more work so I added this as well.  It's no
> solution, but it might help in one situation or another.  The result is
> in CVS.

Thanks for doing that.  IMHO any change that converts an O(N^2)
process into an O(N) one is more than a partial solution :-)