Return correct disk free/available size when FS is mounted with size = 0

Return correct disk free/available size when FS is mounted with size = 0

When tmpfs is mounted with size = 0 (i.e. without any limit), then statvfs
will return 0 for f_bfree, f_bavail, and f_blocks. Catch this case and return
max_int64 instead.

BUG=628710
Committed: https://crrev.com/6c917630660164bb28d57f31b970b38591c0b8b1
Cr-Commit-Position: refs/heads/master@{#406473}

[Sriram, 2016-07-15 23:09:15]

The CQ bit was checked by sriramsr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 23:09:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sriram, 2016-07-15 23:10:01]

sriramsr@chromium.org changed reviewers:
+ reveman@chromium.org, thestig@chromium.org

[Lei Zhang, 2016-07-16 00:19:12]

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:83: // f_blocks is 0 when tmpfs is mounted without any
size limit (i.e. size set
You need to check that the file system is tmpfs. The f_blocks == 0 case may not
mean the same thing for other file systems. On Linux, you can use
GetFileSystemType(), but keep in mind the code here is cross-platform.

[commit-bot: I haz the power, 2016-07-16 00:21:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-16 00:21:08]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-07-18 19:05:07]

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:83: // f_blocks is 0 when tmpfs is mounted without any
size limit (i.e. size set
On 2016/07/16 00:19:12, Lei Zhang wrote:
> You need to check that the file system is tmpfs. The f_blocks == 0 case may
not
> mean the same thing for other file systems. On Linux, you can use
> GetFileSystemType(), but keep in mind the code here is cross-platform.

Basically something like the following, though it may not be 100% correct
because GetFileSystemType() can return FILE_SYSTEM_MEMORY for ramfs and
hugetlbfs as well, and I don't know if the 0 -> unlimited characteristic applies
to those file systems as well. Can you check on that? If not, you'll have just
to put in a specific check for tmpfs here instead.

With the const bool for non-Linux builds, hopefully the compiler can just
optimize it as though this CL never happened.

#if defined(OS_LINUX)
  // On Linux, stats.f_blocks is 0 when tmpfs is mounted without any size limit
  // (i.e. size set to 0).
  FileSystemType fs_type;
  const bool zero_size_means_unlimited = stats.f_blocks == 0 &&
                                         GetFileSystemType(path, &fs_type) &&
                                         fs_type == FILE_SYSTEM_MEMORY;
#else
  const bool zero_size_means_unlimited = false;
#endif

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:101: namespace base {
If you can move this up to wrap the anonymous namespace, then you don't have to
use base:: to refer to GetFileSystemType().

[Sriram, 2016-07-19 23:41:32]

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:83: // f_blocks is 0 when tmpfs is mounted without any
size limit (i.e. size set
On 2016/07/16 00:19:12, Lei Zhang wrote:
> You need to check that the file system is tmpfs. The f_blocks == 0 case may
not
> mean the same thing for other file systems. On Linux, you can use
> GetFileSystemType(), but keep in mind the code here is cross-platform.

Acknowledged.

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:83: // f_blocks is 0 when tmpfs is mounted without any
size limit (i.e. size set
On 2016/07/18 19:05:07, Lei Zhang wrote:
> On 2016/07/16 00:19:12, Lei Zhang wrote:
> > You need to check that the file system is tmpfs. The f_blocks == 0 case may
> not
> > mean the same thing for other file systems. On Linux, you can use
> > GetFileSystemType(), but keep in mind the code here is cross-platform.
> 
> Basically something like the following, though it may not be 100% correct
> because GetFileSystemType() can return FILE_SYSTEM_MEMORY for ramfs and
> hugetlbfs as well, and I don't know if the 0 -> unlimited characteristic
applies
> to those file systems as well. Can you check on that? If not, you'll have just
> to put in a specific check for tmpfs here instead.
> 
> With the const bool for non-Linux builds, hopefully the compiler can just
> optimize it as though this CL never happened.
> 
> #if defined(OS_LINUX)
>   // On Linux, stats.f_blocks is 0 when tmpfs is mounted without any size
limit
>   // (i.e. size set to 0).
>   FileSystemType fs_type;
>   const bool zero_size_means_unlimited = stats.f_blocks == 0 &&
>                                          GetFileSystemType(path, &fs_type) &&
>                                          fs_type == FILE_SYSTEM_MEMORY;
> #else
>   const bool zero_size_means_unlimited = false;
> #endif

Done.

https://codereview.chromium.org/2152283003/diff/1/base/sys_info_posix.cc#newc...
base/sys_info_posix.cc:101: namespace base {
On 2016/07/18 19:05:07, Lei Zhang wrote:
> If you can move this up to wrap the anonymous namespace, then you don't have
to
> use base:: to refer to GetFileSystemType().

Done.

[Sriram, 2016-07-19 23:41:58]

The CQ bit was checked by sriramsr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 23:42:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-07-19 23:45:10]

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:76: }  // namespace
No, that's not what I meant.

namespace base {

namespace {

// all the existing anonymous bits now no longer needs base::

}  // namespace

// all the existing non-anoynymous bits

}  // namespace base

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:89: // ramfs, or hugetlbfs (all memory file systems). the
is mounted without any
So have you checked the ramfs / hugetlbfs behavior w.r.t. size == 0?

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:89: // ramfs, or hugetlbfs (all memory file systems). the
is mounted without any
typo: "the is mounted"

[Sriram, 2016-07-19 23:58:37]

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc
File base/sys_info_posix.cc (right):

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:76: }  // namespace
On 2016/07/19 23:45:10, Lei Zhang wrote:
> No, that's not what I meant.
> 
> namespace base {
> 
> namespace {
> 
> // all the existing anonymous bits now no longer needs base::
> 
> }  // namespace
> 
> // all the existing non-anoynymous bits
> 
> }  // namespace base

Done.

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:89: // ramfs, or hugetlbfs (all memory file systems). the
is mounted without any
On 2016/07/19 23:45:10, Lei Zhang wrote:
> So have you checked the ramfs / hugetlbfs behavior w.r.t. size == 0?

Yes, See Linux code for:
hugetlbfs - http://lxr.free-electrons.com/source/fs/hugetlbfs/inode.c#L869
/* If no limits set, just report 0 for max/free/used
 * blocks, like simple_statfs() */

For Ramfs: 
http://lxr.free-electrons.com/source/fs/libfs.c#L33
Which uses simple_statfs().

https://codereview.chromium.org/2152283003/diff/20001/base/sys_info_posix.cc#...
base/sys_info_posix.cc:89: // ramfs, or hugetlbfs (all memory file systems). the
is mounted without any
On 2016/07/19 23:45:10, Lei Zhang wrote:
> typo: "the is mounted"

Done.

[Lei Zhang, 2016-07-20 00:02:25]

lgtm, thanks for doing the homework.

Can you do me one last favor and run "git cl format" if you haven't already, and
upload the clang-formatted CL for the commit queue?

[Sriram, 2016-07-20 00:06:43]

On 2016/07/20 00:02:25, Lei Zhang wrote:
> lgtm, thanks for doing the homework.
> 
> Can you do me one last favor and run "git cl format" if you haven't already,
and
> upload the clang-formatted CL for the commit queue?

Thanks! Done.

[Sriram, 2016-07-20 00:07:10]

The CQ bit was checked by sriramsr@chromium.org

[Sriram, 2016-07-20 00:07:11]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/2152283003/#ps60001
(title: "Format the file before submitting")

[commit-bot: I haz the power, 2016-07-20 00:08:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 02:57:51]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-20 02:58:01]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-20 02:59:55]

Description was changed from

==========
Return correct disk free/available size when FS is mounted with size = 0

When tmpfs is mounted with size = 0 (i.e. without any limit), then statvfs
will return 0 for f_bfree, f_bavail, and f_blocks. Catch this case and return
max_int64 instead.

BUG=628710
==========

to

==========
Return correct disk free/available size when FS is mounted with size = 0

When tmpfs is mounted with size = 0 (i.e. without any limit), then statvfs
will return 0 for f_bfree, f_bavail, and f_blocks. Catch this case and return
max_int64 instead.

BUG=628710

Committed: https://crrev.com/6c917630660164bb28d57f31b970b38591c0b8b1
Cr-Commit-Position: refs/heads/master@{#406473}
==========

[commit-bot: I haz the power, 2016-07-20 02:59:56]

Patchset 4 (id:??) landed as
https://crrev.com/6c917630660164bb28d57f31b970b38591c0b8b1
Cr-Commit-Position: refs/heads/master@{#406473}

[phoglund_chromium, 2016-07-21 11:31:30]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2168843002/ by phoglund@chromium.org.

The reason for reverting is: Breaks host_forwarder on WebRTC bots:
https://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/buil...:

obj.host/chromium/src/base/base.sys_info_posix.o: In function `base::(anonymous
namespace)::GetDiskSpaceInfo(base::FilePath const&, long*, long*)':
../../chromium/src/base/sys_info_posix.cc:(.text._ZN4base12_GLOBAL__N_116GetDiskSpaceInfoERKNS_8FilePathEPlS4_+0x4c):
undefined reference to `base::GetFileSystemType(base::FilePath const&,
base::FileSystemType*)'

I really don't know why it goes into the OS_LINUX branch since the bot is an
Android bot though. host_forwarder is in the Chromium codebase. I would have
understood it if it was WebRTC code since we have different macros (e.g.
WEBRTC_LINUX) which would explain this breakage, but it appears to break
host_forwarder.

Note that WebRTC will be on GYP a couple more weeks, so this could be a gyp-only
problem. Presumably you'll be able to repro if you generate with gyp and try to
build host_forwarder in a chromium checkout..

[Sriram, 2016-07-22 21:39:50]

How can I reproduce the breakage? I tried doing an android build and it
does not cause the build break (as it did not in CQ too).



On Thu, Jul 21, 2016 at 4:31 AM, <phoglund@chromium.org> wrote:

> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/2168843002/ by phoglund@chromium.org.
>
> The reason for reverting is: Breaks host_forwarder on WebRTC bots:
>
>
https://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/buil...
> :
>
> obj.host/chromium/src/base/base.sys_info_posix.o: In function
> `base::(anonymous
> namespace)::GetDiskSpaceInfo(base::FilePath const&, long*, long*)':
>
>
../../chromium/src/base/sys_info_posix.cc:(.text._ZN4base12_GLOBAL__N_116GetDiskSpaceInfoERKNS_8FilePathEPlS4_+0x4c):
> undefined reference to `base::GetFileSystemType(base::FilePath const&,
> base::FileSystemType*)'
>
> I really don't know why it goes into the OS_LINUX branch since the bot is
> an
> Android bot though. host_forwarder is in the Chromium codebase. I would
> have
> understood it if it was WebRTC code since we have different macros (e.g.
> WEBRTC_LINUX) which would explain this breakage, but it appears to break
> host_forwarder.
>
> Note that WebRTC will be on GYP a couple more weeks, so this could be a
> gyp-only
> problem. Presumably you'll be able to repro if you generate with gyp and
> try to
> build host_forwarder in a chromium checkout..
>
> https://codereview.chromium.org/2152283003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2016-07-22 21:43:01]

On 2016/07/22 21:39:50, Sriram wrote:
> How can I reproduce the breakage? I tried doing an android build and it
> does not cause the build break (as it did not in CQ too).

See the discussion on https://codereview.chromium.org/2168843002/ which you
should have gotten emails for.