Closed Bug 784124 Opened 7 years ago Closed 7 years ago

FreeBSD supports statvfs() since 5.0

Categories

(Core :: XPCOM, defect)

x86_64
FreeBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jbeich, Assigned: jbeich)

References

(Blocks 1 open bug, )

Details

Attachments

(6 files, 11 obsolete files)

813 bytes, patch
benjamin
: review+
Details | Diff | Splinter Review
1.06 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.01 KB, patch
Details | Diff | Splinter Review
3.47 KB, patch
karlt
: review+
Details | Diff | Splinter Review
4.79 KB, patch
Details | Diff | Splinter Review
931 bytes, patch
Details | Diff | Splinter Review
No description provided.
Blocks: 784058
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
Attachment #653475 - Flags: review?(benjamin)
Attachment #653475 - Flags: feedback?(martin)
Attachment #653475 - Flags: feedback?(landry)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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
Attachment #653475 - Attachment is obsolete: true
Attachment #653475 - Flags: review?(benjamin)
Attachment #653475 - Flags: feedback?(martin)
Attachment #653475 - Flags: feedback?(landry)
Attachment #653717 - Flags: review?(benjamin)
Attachment #653473 - Flags: review?(benjamin) → review+
Attachment #653717 - Flags: review?(benjamin) → review?(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.
Attachment #653717 - Flags: review?(karlt) → review-
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 */
Attachment #654572 - Flags: review?(benjamin)
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
Attachment #653717 - Attachment is obsolete: true
Attachment #654575 - Flags: review?(karlt)
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
Attachment #654577 - Flags: review?(karlt)
(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.
Attachment #654575 - Flags: review?(karlt) → review+
Attachment #654577 - Flags: review?(karlt) → review+
(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 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.
(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
v2. Remove HAVE_STATVFS from xpcom-config.h. Other functions and headers are already in xpcom-private.h.
Attachment #654572 - Attachment is obsolete: true
Attachment #654572 - Flags: review?(benjamin)
Attachment #655300 - Flags: review?(benjamin)
v1 used f_frsize with statfs, v2 should use f_bsize.
Attachment #654577 - Attachment is obsolete: true
Attachment #655301 - Flags: review?(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)
Attachment #655301 - Flags: review?(karlt) → review+
Attachment #655301 - Attachment is obsolete: true
Attachment #654577 - Attachment is obsolete: false
(In reply to Karl Tomlinson (:karlt) from comment #17)
> # if (STATFS == statvfs64) || (STATFS == statvfs)

OK. I just wasn't sure about == for ifdefs.
Attachment #654575 - Attachment is obsolete: true
Attachment #655356 - Flags: review?(karlt)
Attachment #653473 - Attachment description: Bug 784124 - FreeBSD supports statvfs(2) since 5.0. → Bug 784124, part 1 - FreeBSD supports statvfs(2) since 5.0.
Attachment #655300 - Attachment description: Bug 784124 - Properly check for stat(v)fs, only used by XPCOM implementation. → Bug 784124, part 2 - Properly check for stat(v)fs, only used by XPCOM implementation.
Attachment #654574 - Attachment description: Bug 784124 - Let it reach NS_ERROR_NOT_IMPLEMENTED if stat(v)fs is not available. → Bug 784124, part 3 - Let it reach NS_ERROR_NOT_IMPLEMENTED if stat(v)fs is not available.
Attachment #655356 - Attachment description: Bug 784124 - Always use .f_frsise with statvfs(). → Bug 784124, part 4 - Always use .f_frsise with statvfs().
Attachment #654577 - Attachment description: Bug 784124 - Don't use statvfs() on Linux and Tru64, it's buggy. → Bug 784124, part 5 - Don't use statvfs() on Linux and Tru64, it's buggy.
Attachment #655356 - Flags: review?(karlt) → review+
Attachment #654574 - Flags: review?(benjamin) → review+
Attachment #655300 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Keywords: checkin-needed
b2g builds fail with these patches applied.
https://tbpl.mozilla.org/?tree=Try&rev=b6b3b824d59a
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.
Attachment #655356 - Attachment is obsolete: true
Attachment #655695 - Flags: review?(karlt)
(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.
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.
Attachment #655300 - Attachment is obsolete: true
Attachment #655716 - Flags: review?(benjamin)
no changes, only resolve conflict with attachment 655695 [details] [diff] [review]
Attachment #654577 - Attachment is obsolete: true
v4. Replace f_bsize in linux quota code and update comment.
Attachment #655695 - Attachment is obsolete: true
Attachment #655695 - Flags: review?(karlt)
Attachment #655727 - Flags: review?(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.
Attachment #655727 - Flags: review?(karlt) → review+
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.
Attachment #655716 - Attachment is obsolete: true
Attachment #655716 - Flags: review?(benjamin)
Attachment #655877 - Flags: review?(benjamin)
(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
(In reply to Ryan VanderMeulen from comment #31)
> https://tbpl.mozilla.org/?tree=Try&rev=87999a5100fd

Looks like you got it this time :)
Comment on attachment 656000 [details] [diff] [review]
Bug 784124, part 6 - Remove OS X sanity check, statvfs now always uses f_frsize.

r=me
Attachment #656000 - Flags: review?(bzbarsky) → review+
improve commit message a bit to not be a copy of part 4
Attachment #656000 - Attachment is obsolete: true
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.
Attachment #655877 - Flags: review?(benjamin)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.