784124 - FreeBSD supports statvfs() since 5.0

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Jan Beich]

Attachment: Bug 784124, part 1 - FreeBSD supports statvfs(2) since 5.0.

Comment 2

[Jan Beich]

Attachment: Bug 784124 - f_bsize in statfs == f_frsize in statvfs on FreeBSD.

Other BSDs could exhibit similar issue.

$ df -h .
Filesystem            Size    Used   Avail Capacity  Mounted on
h/home/foo/.cache      54G    2.2G     51G     4%    /home/foo/.cache

DiskSpaceAvailable: 55523845632 bytes # statfs()
DiskSpaceAvailable: 14214261768192 bytes # statvfs()

DiskSpaceAvailable: 55523824128 bytes # statvfs() after the fix

Comment 3

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #2)
> Created attachment 653475 [details] [diff] [review]
> Bug 784124 - f_bsize in statfs == f_frsize in statvfs on FreeBSD.
> 
> Other BSDs could exhibit similar issue.

Yes, since it uses statvfs() OpenBSD should use f_frsize too like free/mac/solaris. While here the comment should be updated to match the ifdef list.. no idea for netbsd.

Comment 4

[Jan Beich]

Attachment: Bug 784124 - The block size used for the size is f_frsize for POSIX-compliant statvfs.

Confirmed. Here's a nugget I've found in NetBSD linux compat code

	/*
	 * The sizes are expressed in number of blocks. The block
	 * size used for the size is f_frsize for POSIX-compliant
	 * statvfs. Linux statfs uses f_bsize as the block size
	 * (f_frsize used to not be available in Linux struct statfs).
	 * However, glibc 2.3.3 statvfs() wrapper fails to adjust the block
	 * counts for different f_frsize if f_frsize is provided by the kernel.
	 * POSIX conforming apps thus get wrong size if f_frsize
	 * is different to f_bsize. Thus, we just pretend we don't
	 * support f_frsize.
	 */

Not sure if the ifdef still useful. It should work on Linux, too, since
no one complained when I've fixed same bug in Transmission:

  https://trac.transmissionbt.com/ticket/4295

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 653717 [details] [diff] [review]
Bug 784124 - The block size used for the size is f_frsize for POSIX-compliant statvfs.

Isn't the key distinction that bsize should be used with statfs but frsize
with statvfs?
statfs may not have an frsize field.
http://www.manpagez.com/man/2/statfs/osx-10.4.php

Re the situation with Linux/glibc:

The POSIX statvfs documentation is clear enough
http://pubs.opengroup.org/onlinepubs/009604499/basedefs/sys/statvfs.h.html
but the behavior does not seem consistent with that.

There are only two file systems I see in Linux that may report different
frsize and bsize: Ceph and FUSE;

Ceph uses bsize for bavail block sizes in statfs.
http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=fs/ceph/super.c;h=1e67dd7305a4e8596a3c5f609ccea2a866e4e31a;hb=HEAD#l74

FUSE reports for statfs whatever the user-space system reports.

NFS returns frsize == bsize to workaround problems, but the comment suggests
that it expects glibc to adjust blocks/bavail/bfree when converting from
statfs to statvfs.
http://git.kernel.org/?p=linux/kernel/git/stable/linux-stable.git;a=blob;f=fs/nfs/super.c;h=06228192f64efb52e8466afd2334bc04952d50a4;hb=HEAD#l506

glibc still doesn't adjust block counts for statvfs when frsize differs from
bsize:
http://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/internal_statvfs.c;h=30b12f2e0ce131d6eac256b6890a2b7d1ebbf6b6#l225

I suspect the best thing for Linux would be to force use of statfs even when
statvfs is available.  That way we'll still get the right number (except
possibly with FUSE) if/when glibc changes statvfs.

Comment 6

[Jan Beich]

Attachment: Bug 784124 - Properly check for stat(v)fs which is used in xpcom.

It should complement the following checks (with consistent sorting order):

  MOZ_CHECK_HEADERS(sys/statvfs.h sys/statfs.h sys/vfs.h sys/mount.h)
  AC_CHECK_FUNCS(... statvfs memmove rint stat64 lstat64 truncate64 statvfs64 setbuf isatty)

Some guesstimate:

  POSIX systems use sys/statvfs.h
  AIX, SOLARIS and LINUX can also use sys/statfs.h for statfs
  LINUX can also use sys/vfs.h for statfs
  XP_MACOSX and BSDs can also use sys/mount.h for statfs

$ git grep -e STATVFS_H -e STATFS_H origin
origin:xpcom/io/nsLocalFileUnix.h:#ifdef HAVE_SYS_STATVFS_H
origin:xpcom/io/nsLocalFileUnix.h:#ifdef HAVE_SYS_STATFS_H
origin:xpcom/io/nsLocalFileUnix.h:    #define HAVE_SYS_STATFS_H

Here's how defines work after the patch (on FreeBSD):

$ ./configure

$ egrep -i -e 'v?fs' -e mount mozilla-config.h

$ egrep -A1 -i -e 'v?fs' -e mount xpcom/xpcom-private.h
/* Define if statvfs() is available */

--
/* Define if statvfs64() is available */
/* #undef HAVE_STATVFS64 */

--
/* Define if statfs() is available */

--
/* Define if statfs64() is available */
/* #undef HAVE_STATFS64 */

--
/* Define if <sys/statvfs.h> is present */

--
/* Define if <sys/statfs.h> is present */
/* #undef HAVE_SYS_STATFS_H */

--
/* Define if <sys/vfs.h> is present */
/* #undef HAVE_SYS_VFS_H */

--
/* Define if <sys/mount.h> is present */

Comment 7

[Jan Beich]

Haha, formatting for egrep examples got screwed. 2nd try:

    $ egrep -i -e 'v?fs' -e mount mozilla-config.h
    #define HAVE_SYS_CDEFS_H 1

    $ egrep -A1 -i -e 'v?fs' -e mount xpcom/xpcom-private.h
    /* Define if statvfs() is available */
    #define HAVE_STATVFS 1

    --
    /* Define if statvfs64() is available */
    /* #undef HAVE_STATVFS64 */

    --
    /* Define if statfs() is available */
    #define HAVE_STATFS 1

    --
    /* Define if statfs64() is available */
    /* #undef HAVE_STATFS64 */

    --
    /* Define if <sys/statvfs.h> is present */
    #define HAVE_SYS_STATVFS_H 1

    --
    /* Define if <sys/statfs.h> is present */
    /* #undef HAVE_SYS_STATFS_H */

    --
    /* Define if <sys/vfs.h> is present */
    /* #undef HAVE_SYS_VFS_H */

    --
    /* Define if <sys/mount.h> is present */
    #define HAVE_SYS_MOUNT_H 1

Comment 8

[Jan Beich]

Attachment: Bug 784124, part 3 - Let it reach NS_ERROR_NOT_IMPLEMENTED if stat(v)fs is not available.

Comment 9

[Jan Beich]

Attachment: Bug 784124 - Always use .f_frsise with statvfs().

Comment 10

[Jan Beich]

Attachment: Bug 784124, part 5 - Don't use statvfs() on Linux and Tru64, it's buggy.

I'll use advice from :karlt and a nugget of wisdom from gnulib (m4/fsusage.m4):

  #if (defined __GLIBC__ || defined __UCLIBC__) && defined __linux__
  Do not use statvfs on systems with GNU libc on Linux, because that function
  stats all preceding entries in /proc/mounts, and that makes df hang if even
  one of the corresponding file systems is hard-mounted, but not available.
  statvfs in GNU libc on Hurd, BeOS, Haiku operates differently: it only makes
  a system call.
  #endif

  #ifdef __osf__
  "Do not use Tru64's statvfs implementation"
  #endif

Comment 11

[Jan Beich]

(In reply to Jan Beich from comment #1)
> Created attachment 653473 [details] [diff] [review]
> Bug 784124 - FreeBSD supports statvfs(2) since 5.0.

Oops, it's not a syscall but a function in libc - a mistake in commit message. Not gonna resubmit.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Jan Beich from comment #10)
> ... a nugget of wisdom from gnulib
> (m4/fsusage.m4):
> 
>   #if (defined __GLIBC__ || defined __UCLIBC__) && defined __linux__
>   Do not use statvfs on systems with GNU libc on Linux, because that function
>   stats all preceding entries in /proc/mounts, and that makes df hang if even
>   one of the corresponding file systems is hard-mounted, but not available.

I don't see that code in glibc, so that comment may be quite outdated.

>   #ifdef __osf__
>   "Do not use Tru64's statvfs implementation"
>   #endif

From
http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=7a39483e4437689768808245226e17f1be0029e3

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 654572 [details] [diff] [review]
Bug 784124 - Properly check for stat(v)fs which is used in xpcom.

Ah, I was wondering why we were not using statvfs64.
Only HAVE_STATVFS is in xpcom-config.h.in.

Comment 14

[Jan Beich]

(In reply to Karl Tomlinson (:karlt) from comment #12)
> I don't see that code in glibc, so that comment may be quite outdated.

http://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commit;h=c25bdba

Comment 15

[Jan Beich]

Attachment: Bug 784124, part 2 - Properly check for stat(v)fs, only used by XPCOM implementation.

v2. Remove HAVE_STATVFS from xpcom-config.h. Other functions and headers are already in xpcom-private.h.

Comment 16

[Jan Beich]

Attachment: Bug 784124 - Don't use statvfs() on Linux and Tru64, it's buggy.

v1 used f_frsize with statfs, v2 should use f_bsize.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 655301 [details] [diff] [review]
Bug 784124 - Don't use statvfs() on Linux and Tru64, it's buggy.

This is fine, but it may be clearer to still change

#if defined(HAVE_STATVFS64) || defined(HAVE_STATVFS)

to 

# if (STATFS == statvfs64) || (STATFS == statvfs)

Comment 18

[Jan Beich]

Attachment: Bug 784124, part 4 - Always use .f_frsise with statvfs().

(In reply to Karl Tomlinson (:karlt) from comment #17)
> # if (STATFS == statvfs64) || (STATFS == statvfs)

OK. I just wasn't sure about == for ifdefs.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

b2g builds fail with these patches applied.
https://tbpl.mozilla.org/?tree=Try&rev=b6b3b824d59a

Comment 20

[Jan Beich]

Attachment: Bug 784124, part 4 - Always use .f_frsize with statvfs().

v3. Mask f_bsize like we already do with statfs.

stat(v)fs (the macros) cannot be used for comparison. Comparing two undefined macros always yields true.

  $ cat a.h
  #define STATFS statfs

  #if (STATFS == statvfs)
  #error statvfs != statfs
  #endif

  $ clang a.h
  a.h:4:2: error: statvfs != statfs
  #error statvfs != statfs
   ^
  1 error generated.

Comment 21

[Jan Beich]

(In reply to Ryan VanderMeulen from comment #19)
> b2g builds fail with these patches applied.
> https://tbpl.mozilla.org/?tree=Try&rev=b6b3b824d59a

It builds fine on Linux because it has .f_frsize in statfs unlike BSDs.

Comment 22

[Jan Beich]

Attachment: Bug 784124, part 2 - Properly check for stat(v)fs, only used by XPCOM implementation. r=bsmedberg

v3. It seems gonk uses nsLocalFile.h outside of XPCOM.

  #ifdef _IMPL_NS_COM
  #include "xpcom-private.h"
  #endif

Without xpcom-private.h only STAT macro still pollutes nsLocalFileUnix.h.

Comment 23

[Jan Beich]

Attachment: Bug 784124, part 5 - Don't use statvfs() on Linux and Tru64, it's buggy. r=karlt

no changes, only resolve conflict with attachment 655695 [details] [diff] [review]

Comment 24

[Jan Beich]

Attachment: Bug 784124, part 4 - Always use .f_frsize with statvfs().

v4. Replace f_bsize in linux quota code and update comment.

Comment 25

[Karl Tomlinson (:karlt)]

Comment on attachment 655727 [details] [diff] [review]
Bug 784124, part 4 - Always use .f_frsize with statvfs().

Sorry about my poor advice.  I gather CPP comparisons are always integer.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=1ed3fb2fbaa6

Comment 27

[Jan Beich]

Attachment: Bug 784124, part 2 - Properly check for stat(v)fs, only used by XPCOM implementation. r=bsmedberg

v4 (attempt #2). -D_IMPL_NS_COM appears in flags under dom/system/gonk/. Trying .cpp file: xpcom-private.h just before nsFoo.h includes, similar to nsComponentManager.h.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=5e5b36f81a2c

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #28)
> https://tbpl.mozilla.org/?tree=Try&rev=5e5b36f81a2c

On the bright side, you fixed b2g. On the down side, you broke OSX :-P

Comment 30

[Jan Beich]

Attachment: Bug 784124, part 6 - Remove OS X sanity check, statvfs now always uses f_frsize.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=87999a5100fd

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #31)
> https://tbpl.mozilla.org/?tree=Try&rev=87999a5100fd

Looks like you got it this time :)

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656000 [details] [diff] [review]
Bug 784124, part 6 - Remove OS X sanity check, statvfs now always uses f_frsize.

r=me

Comment 34

[Jan Beich]

Attachment: Bug 784124, part 6 - Remove OS X sanity check, F_BSIZE should reduce ambiguity. r=bz

improve commit message a bit to not be a copy of part 4

Comment 35

[Jan Beich]

Comment on attachment 655877 [details] [diff] [review]
Bug 784124, part 2 - Properly check for stat(v)fs, only used by XPCOM implementation. r=bsmedberg

Skipping review for a tiny change, it already carries r+, anyway.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #31)
> https://tbpl.mozilla.org/?tree=Try&rev=87999a5100fd

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbeea411b904
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ffa5cb41f54
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffa907d23a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47f403c7636
https://hg.mozilla.org/integration/mozilla-inbound/rev/b77576a0b262
https://hg.mozilla.org/integration/mozilla-inbound/rev/395978f39deb

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cbeea411b904
https://hg.mozilla.org/mozilla-central/rev/1ffa5cb41f54
https://hg.mozilla.org/mozilla-central/rev/fffa907d23a6
https://hg.mozilla.org/mozilla-central/rev/d47f403c7636
https://hg.mozilla.org/mozilla-central/rev/b77576a0b262
https://hg.mozilla.org/mozilla-central/rev/395978f39deb