LevelDB: Using base::File for all file I/O

LevelDB: Using base::File for all file I/O

Eliminated platform specific (stdio & Windows) File I/O calls and replaced with
the use of base::File. This makes Chrome's Env (nearly) platform agnostic while
simplifying the implementation.

BUG=431914
Committed: https://crrev.com/bf82318b8929b07d7bb0abce2729c5a83b9dbfcb
Cr-Commit-Position: refs/heads/master@{#305020}

[cmumford, 2014-11-11 23:03:38]

cmumford@chromium.org changed reviewers:
+ dgrogan@chromium.org, jsbell@chromium.org

[cmumford, 2014-11-11 23:03:39]

big change for requesting review by both jsbell & dgrogan.

[jsbell, 2014-11-12 01:04:05]

First pass comments.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/chromium_logger.h (left):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/chromium_logger.h:83: fflush(file_);
Removed because base::File I/O is not buffered?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (left):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:471:
GetDirectoryEntries(CreateFilePath(dir_string), &entries);
This was added to avoid early-exit on error. Does the FileEnumerator have the
same behavior?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:92: virtual Status Read(size_t n,
Slice* result, char* scratch) {
Drop `virtual`, add `override` here and below?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:95: int saved_errno = errno;
Uh... isn't errno a POSIX thing? The Win32 APIs don't set this, and I don't see
any code in file_win.cc that would shim it.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:99: } else {
This appears to be missing the logic from the old posix version that makes
bytes_read < n an error unless at EOF. Intentionally removed?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:207: } else if (!f.Flush()) {
Since the previous if() clause returns, drop the else.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:230: } else {
Drop the else, this can be unconditional since the previous clause returns.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:241: // base::File doesn't do buffered
I/O (i.e. POSIX FILE streams) so nothing to
Agreed, this matches previous posix/win32 env behavior... it's weird that
leveldb even requires this.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:249: if (!file_->Flush()) {
It looks like the old POSIX used fdatasync(), which ends up being
HANDLE_EINTR(fsync(fd)), same as File::Flush() in POSIX... but OS_ANDROID may be
different?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:256: if (make_backup_ && file_type_ ==
kTable) {
Can drop the {} here

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:842:
GetMaxFDHistogram(type)->Add(base::SharedMemory::GetHandleLimit());
Although I think this does the right thing (eventually calling base::GetMaxFds
on POSIX, doing this via base::SharedMemory is yucky.

I'd prefer a #if defined(OS_POSIX)-guarded call to base::GetMaxFds()

[cmumford, 2014-11-12 20:59:33]

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/chromium_logger.h (left):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/chromium_logger.h:83: fflush(file_);
On 2014/11/12 01:04:04, jsbell wrote:
> Removed because base::File I/O is not buffered?

Yes, exactly.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (left):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:471:
GetDirectoryEntries(CreateFilePath(dir_string), &entries);
On 2014/11/12 01:04:04, jsbell wrote:
> This was added to avoid early-exit on error. Does the FileEnumerator have the
> same behavior?

It looks like FileEnumerator mostly ignores errors (not a good design IMHO). I
added in a check to make sure that dir_string is a directory. However, if there
are permissions issues that would make opendir/readdir fail then this would
appear as though there are no children.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:92: virtual Status Read(size_t n,
Slice* result, char* scratch) {
On 2014/11/12 01:04:04, jsbell wrote:
> Drop `virtual`, add `override` here and below?

Done.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:95: int saved_errno = errno;
On 2014/11/12 01:04:05, jsbell wrote:
> Uh... isn't errno a POSIX thing? The Win32 APIs don't set this, and I don't
see
> any code in file_win.cc that would shim it.

There were a bunch of other errno uses - I cleaned those up as well.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:99: } else {
On 2014/11/12 01:04:04, jsbell wrote:
> This appears to be missing the logic from the old posix version that makes
> bytes_read < n an error unless at EOF. Intentionally removed?

I don't think so - or maybe I'm misunderstanding how ReadAtCurrentPos works.
From their header:

// Reads the given number of bytes (or until EOF is reached) starting with the
// given offset. Returns the number of bytes read, or -1 on error. Note that
// this function makes a best effort to read all data on all platforms, so it
// is not intended for stream oriented files but instead for cases when the
// normal expectation is that actually |size| bytes are read unless there is
// an error.

So it's my understanding that File::Read will return either:

1. All the bytes requested.
2. Less than the bytes requested, but *only* when hitting EOF
3. -1 upon error.

So I don't there can be a case where it doesn't hit EOF, but returns <
bytes_requested. Is this the situation you are describing?

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:207: } else if (!f.Flush()) {
On 2014/11/12 01:04:04, jsbell wrote:
> Since the previous if() clause returns, drop the else.

Done.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:230: } else {
On 2014/11/12 01:04:05, jsbell wrote:
> Drop the else, this can be unconditional since the previous clause returns.

Done.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:241: // base::File doesn't do buffered
I/O (i.e. POSIX FILE streams) so nothing to
On 2014/11/12 01:04:04, jsbell wrote:
> Agreed, this matches previous posix/win32 env behavior... it's weird that
> leveldb even requires this.

My read is that LDB's Env is designed so that you *could* do buffered I/O if
desired, and if not doing so then you can no-op this function.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:249: if (!file_->Flush()) {
On 2014/11/12 01:04:04, jsbell wrote:
> It looks like the old POSIX used fdatasync(), which ends up being
> HANDLE_EINTR(fsync(fd)), same as File::Flush() in POSIX... but OS_ANDROID may
be
> different?

The old POSIX used libc's fdatasync - not the one at the top of
env_chromium_stdio.cc - Same with Android. And that call was *not* wrapped in
HANDLE_EINTR. Only on Windows did our locally defined fdatasync function wind up
being called (which called _commit) - part of Microsoft's c runtime. I looked at
it's implementation and it calls FlushFileBuffers which is what base::File's
does on Windows. I believe  this new implementation is identical, but adds some
missing HANDLE_EINTR's.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:256: if (make_backup_ && file_type_ ==
kTable) {
On 2014/11/12 01:04:04, jsbell wrote:
> Can drop the {} here

Done.

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:842:
GetMaxFDHistogram(type)->Add(base::SharedMemory::GetHandleLimit());
On 2014/11/12 01:04:04, jsbell wrote:
> Although I think this does the right thing (eventually calling base::GetMaxFds
> on POSIX, doing this via base::SharedMemory is yucky.
> 
> I'd prefer a #if defined(OS_POSIX)-guarded call to base::GetMaxFds()

Done. GetMaxFds was not exported - I fixed that and am now using it.

[cmumford, 2014-11-12 21:10:23]

cmumford@chromium.org changed reviewers:
+ brettw@chromium.org

[cmumford, 2014-11-12 21:10:24]

+brettw for review of change to base/process/process_metrics.h to export
base::GetMaxFds.

[dgrogan, 2014-11-12 22:45:11]

https://codereview.chromium.org/710373002/diff/60001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/60001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:618: base::FileEnumerator
enumerator(dir_path,
As Josh hinted, moving back to FileEnumerator will regress the corruption rate.
I recommend not doing so, at least not without explicitly weighing the benefit
against the increased data loss users will experience.

First glance suggests that the benefit (-~50 lines of code) is small. The cost
is that +~50 IDB users and +~200 non-IDB users of LevelDB will suffer data loss
each day[1].

So my vote is to stick with the existing, non-FileEnumerator implementations of
GetChildren.

[1]
https://uma.googleplex.com/histograms/?dayCount=1&endDate=11-11-2014&version=...

[dgrogan, 2014-11-12 22:53:31]

Or, as Josh suggested a year ago[1], make FileEnumerator report errors better.

[1] https://codereview.chromium.org/26513005#msg2

[cmumford, 2014-11-12 23:52:41]

https://codereview.chromium.org/710373002/diff/60001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/60001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:618: base::FileEnumerator
enumerator(dir_path,
On 2014/11/12 22:45:11, dgrogan wrote:
> As Josh hinted, moving back to FileEnumerator will regress the corruption
rate.
> I recommend not doing so, at least not without explicitly weighing the benefit
> against the increased data loss users will experience.
> 
> First glance suggests that the benefit (-~50 lines of code) is small. The cost
> is that +~50 IDB users and +~200 non-IDB users of LevelDB will suffer data
loss
> each day[1].
> 
> So my vote is to stick with the existing, non-FileEnumerator implementations
of
> GetChildren.
> 
> [1]
>
https://uma.googleplex.com/histograms/?dayCount=1&endDate=11-11-2014&version=...

So right now LevelDB calls GetChildren in four places. It ignores the status
code for two of them. Of the two that it checks it treats an empty list the same
as an error. But that being said I'm not a fan of ignoring failures. I'm
currently chatting with rvargas to see if he's OK with improving FileEnumerator.
If I can't get buyoff on that then I'll go back to our original implementation.

[dgrogan, 2014-11-13 00:12:46]

> So right now LevelDB calls GetChildren in four places. It ignores the status
> code for two of them. Of the two that it checks it treats an empty list the
same
> as an error.

Rendered unnecessary because of what you say below, but I don't think this is
right. DBImpl::Recover calls GetChildren. If GetChildren returns an IOError as
it does today, that IOError percolates up to IndexedDBBackingStore::Open, which,
for IOErrors, just returns an empty database. If GetChildren returns an empty
list then DBImpl::Recover will probably return a CorruptionError. That
CorruptionError also percolates up to IndexedDBBackingStore::Open, which, for
CorruptionErrors, blows away the database on disk. That difference in behavior
is why we moved away from FileEnumerator in the first place.

> But that being said I'm not a fan of ignoring failures. I'm
> currently chatting with rvargas to see if he's OK with improving
FileEnumerator.
> If I can't get buyoff on that then I'll go back to our original
implementation.

[cmumford, 2014-11-13 00:42:38]

On 2014/11/13 00:12:46, dgrogan wrote:
> > So right now LevelDB calls GetChildren in four places. It ignores the status
> > code for two of them. Of the two that it checks it treats an empty list the
> same
> > as an error.
> 
> Rendered unnecessary because of what you say below, but I don't think this is
> right. DBImpl::Recover calls GetChildren. If GetChildren returns an IOError as
> it does today, that IOError percolates up to IndexedDBBackingStore::Open,
which,
> for IOErrors, just returns an empty database. If GetChildren returns an empty
> list then DBImpl::Recover will probably return a CorruptionError. That
> CorruptionError also percolates up to IndexedDBBackingStore::Open, which, for
> CorruptionErrors, blows away the database on disk. That difference in behavior
> is why we moved away from FileEnumerator in the first place.
> 
> > But that being said I'm not a fan of ignoring failures. I'm
> > currently chatting with rvargas to see if he's OK with improving
> FileEnumerator.
> > If I can't get buyoff on that then I'll go back to our original
> implementation.

It's looking like I am *not* getting buy-in on having FileEnumerator return it's
status so I'll likely return to the current implementation for GetChildren().

[jsbell, 2014-11-14 00:12:34]

lgtm modulo your feedback on the comments, but you should get dgrogan's blessing
too

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:99: } else {
On 2014/11/12 20:59:33, cmumford wrote:
> On 2014/11/12 01:04:04, jsbell wrote:
> > This appears to be missing the logic from the old posix version that makes
> > bytes_read < n an error unless at EOF. Intentionally removed?
> 
> I don't think so - or maybe I'm misunderstanding how ReadAtCurrentPos works.
> From their header:
> 
> // Reads the given number of bytes (or until EOF is reached) starting with the
> // given offset. Returns the number of bytes read, or -1 on error. Note that
> // this function makes a best effort to read all data on all platforms, so it
> // is not intended for stream oriented files but instead for cases when the
> // normal expectation is that actually |size| bytes are read unless there is
> // an error.
> 
> So it's my understanding that File::Read will return either:
> 
> 1. All the bytes requested.
> 2. Less than the bytes requested, but *only* when hitting EOF
> 3. -1 upon error.
> 
> So I don't there can be a case where it doesn't hit EOF, but returns <
> bytes_requested. Is this the situation you are describing?

I was looking at the file_posix.cc impl; if it manages to read anything
bytes_read is set; if it later hits an error then bytes_read is returned rather
than -1. So if it were to hit an error other than EOF we wouldn't know.

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:13: #include
"base/files/file_enumerator.h"
No longer needed?

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:47: static base::File::Error
GetDirectoryEntries(
Add a comment with the rationale for using this instead of FileEnumerator?

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:98: ::base::File file_;
Bit of a tangent but... what's up with ::base vs. just base?

third_party/leveldatabase has 66 out of the 107 uses of this in src/.

Want to remove any new instances from the patch?

[cmumford, 2014-11-14 22:43:57]

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/20001/third_party/leveldatabas...
third_party/leveldatabase/env_chromium.cc:99: } else {
On 2014/11/14 00:12:33, jsbell wrote:
> On 2014/11/12 20:59:33, cmumford wrote:
> > On 2014/11/12 01:04:04, jsbell wrote:
> > > This appears to be missing the logic from the old posix version that makes
> > > bytes_read < n an error unless at EOF. Intentionally removed?
> > 
> > I don't think so - or maybe I'm misunderstanding how ReadAtCurrentPos works.
> > From their header:
> > 
> > // Reads the given number of bytes (or until EOF is reached) starting with
the
> > // given offset. Returns the number of bytes read, or -1 on error. Note that
> > // this function makes a best effort to read all data on all platforms, so
it
> > // is not intended for stream oriented files but instead for cases when the
> > // normal expectation is that actually |size| bytes are read unless there is
> > // an error.
> > 
> > So it's my understanding that File::Read will return either:
> > 
> > 1. All the bytes requested.
> > 2. Less than the bytes requested, but *only* when hitting EOF
> > 3. -1 upon error.
> > 
> > So I don't there can be a case where it doesn't hit EOF, but returns <
> > bytes_requested. Is this the situation you are describing?
> 
> I was looking at the file_posix.cc impl; if it manages to read anything
> bytes_read is set; if it later hits an error then bytes_read is returned
rather
> than -1. So if it were to hit an error other than EOF we wouldn't know.

I believe that calling ReadAtCurrentPosNoBestEffort instead of ReadAtCurrentPos
gives us what we want.

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
File third_party/leveldatabase/env_chromium.cc (right):

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:13: #include
"base/files/file_enumerator.h"
On 2014/11/14 00:12:33, jsbell wrote:
> No longer needed?

Done.

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:47: static base::File::Error
GetDirectoryEntries(
On 2014/11/14 00:12:34, jsbell wrote:
> Add a comment with the rationale for using this instead of FileEnumerator?

Done.

https://codereview.chromium.org/710373002/diff/120001/third_party/leveldataba...
third_party/leveldatabase/env_chromium.cc:98: ::base::File file_;
On 2014/11/14 00:12:33, jsbell wrote:
> Bit of a tangent but... what's up with ::base vs. just base?
> 
> third_party/leveldatabase has 66 out of the 107 uses of this in src/.
> 
> Want to remove any new instances from the patch?

I have another CL that will follow up this one with additional cleanup. I will
clean this up there (or in it's own CL).

[dgrogan, 2014-11-15 01:10:19]

lgtm

It would be awesome if you ran a finch experiment (with a proper control group)
with this change to ensure it doesn't have any unforeseen corruption/performance
effects, which we've had in the past.

[cmumford, 2014-11-17 18:17:47]

On 2014/11/15 01:10:19, dgrogan wrote:
> lgtm
> 
> It would be awesome if you ran a finch experiment (with a proper control
group)
> with this change to ensure it doesn't have any unforeseen
corruption/performance
> effects, which we've had in the past.

Being that this change is merely refactoring and not something to fix a bug we
certainly can delay landing it and do an experiment as you suggest. However, I
know that our dev numbers are very noisy, so this would (at a minimum) need to
be a beta experiment - possibly even stable???

Also, the question the experiment is really asking is "does this code contain a
bug". Is this what finch was designed for, and will the experiment be approved?

If the two of you want to land this in an experiment first then let me know.

[dgrogan, 2014-11-17 19:28:15]

On Mon, Nov 17, 2014 at 10:17 AM, <cmumford@chromium.org> wrote:

> On 2014/11/15 01:10:19, dgrogan wrote:
>
>> lgtm
>>
>
>  It would be awesome if you ran a finch experiment (with a proper control
>>
> group)
>
>> with this change to ensure it doesn't have any unforeseen
>>
> corruption/performance
>
>> effects, which we've had in the past.
>>
>
> Being that this change is merely refactoring and not something to fix a
> bug we
> certainly can delay landing it and do an experiment as you suggest.
> However, I
> know that our dev numbers are very noisy, so this would (at a minimum)
> need to
> be a beta experiment - possibly even stable???
>

Last I heard our dev population was larger than beta. Maybe that is not or
no longer correct though.

Also, the question the experiment is really asking is "does this code
> contain a
> bug". Is this what finch was designed for, and will the experiment be
> approved?
>

I don't know if it would be approved.

The reason I suggested an experiment is to avoid situations similar to
those in the past where the rollout of a change that we thought would not
affect corruption rates roughly corresponded to an increase in corruption.
It was not clear if usage patterns underwent a structural shift (e.g.
facebook started using IDB) or the change was faulty in some subtle way or
some OTHER change that rolled out with our change was responsible (I was
constantly asking the CrOS team if they made any filesystem-level changes
in XX release). Running an experiment, with the current finch facilities
(specifically referring to confidence intervals), would answer all those
questions with clarity.

But, as you allude to, running an experiment is not free. It seems like
your confidence that this change will have no performance or corruption
effects is high enough that perhaps an experiment should not be done. In
which case, fire away. But watch corruption/perf numbers when this rolls
out, just in case!


> If the two of you want to land this in an experiment first then let me
> know.
>
> https://codereview.chromium.org/710373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cmumford, 2014-11-19 16:54:57]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-11-19 16:55:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710373002/140001

[commit-bot: I haz the power, 2014-11-19 16:59:40]

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

[commit-bot: I haz the power, 2014-11-19 16:59:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2014-11-19 17:14:20]

LGTM

[cmumford, 2014-11-20 16:28:55]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-11-20 16:30:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710373002/140001

[commit-bot: I haz the power, 2014-11-20 16:33:09]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-11-20 16:33:50]

Patchset 8 (id:??) landed as
https://crrev.com/bf82318b8929b07d7bb0abce2729c5a83b9dbfcb
Cr-Commit-Position: refs/heads/master@{#305020}