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

third_party/leveldatabase/env_chromium.cc

Index: third_party/leveldatabase/env_chromium.cc
diff --git a/third_party/leveldatabase/env_chromium.cc b/third_party/leveldatabase/env_chromium.cc
index b34cecd37023e56271966a8bc2b13290d2719582..f9b46c2f02e398eba0de1c20ad44b8ef411834f1 100644
--- a/third_party/leveldatabase/env_chromium.cc
+++ b/third_party/leveldatabase/env_chromium.cc
@@ -4,25 +4,18 @@
 
 #include "third_party/leveldatabase/env_chromium.h"
 
-#if defined(OS_WIN)
-#include <io.h>
-#endif
-
 #include "base/debug/trace_event.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
+#include "base/memory/shared_memory.h"
 #include "base/metrics/histogram.h"
+#include "base/process/process_metrics.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "third_party/leveldatabase/env_chromium_stdio.h"
+#include "third_party/leveldatabase/chromium_logger.h"
 #include "third_party/re2/re2/re2.h"
 
-#if defined(OS_WIN)
-#include "base/command_line.h"
-#include "base/win/win_util.h"
-#include "third_party/leveldatabase/env_chromium_win.h"
-#endif
-
 using leveldb::FileLock;
 using leveldb::Slice;
 using leveldb::Status;
@@ -88,33 +81,196 @@ class Retrier {
   RetrierProvider* provider_;
 };
 
-class IDBEnvStdio : public ChromiumEnvStdio {
+class ChromiumSequentialFile : public leveldb::SequentialFile {
  public:
-  IDBEnvStdio() : ChromiumEnvStdio() {
-    name_ = "LevelDBEnv.IDB";
-    make_backup_ = true;
+  ChromiumSequentialFile(const std::string& fname,
+                         base::File* f,
+                         const UMALogger* uma_logger)
+      : filename_(fname), file_(f), uma_logger_(uma_logger) {}
+  virtual ~ChromiumSequentialFile() {}
+
+  virtual Status Read(size_t n, Slice* result, char* scratch) {

[jsbell, 2014/11/12 01:04:04]

Drop `virtual`, add `override` here and below?

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

Done.

+    int bytes_read = file_->ReadAtCurrentPos(scratch, n);
+    if (bytes_read == -1) {
+      int saved_errno = errno;

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

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.

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

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

+      uma_logger_->RecordErrorAt(kSequentialFileRead);
+      return MakeIOError(filename_, strerror(saved_errno), kSequentialFileRead,
+                         saved_errno);
+    } else {

[jsbell, 2014/11/12 01:04:04]

This appears to be missing the logic from the old posix version that makes
bytes_read < n an error unless at EOF. Intentionally removed?

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

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?

[jsbell, 2014/11/14 00:12:33]

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.

[cmumford, 2014/11/14 22:43:56]

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

+      *result = Slice(scratch, bytes_read);
+      return Status::OK();
+    }
+  }
+
+  virtual Status Skip(uint64_t n) {
+    if (file_->Seek(base::File::FROM_CURRENT, n) == -1) {
+      int saved_errno = errno;
+      uma_logger_->RecordErrorAt(kSequentialFileSkip);
+      return MakeIOError(filename_, strerror(saved_errno), kSequentialFileSkip,
+                         saved_errno);
+    } else {
+      return Status::OK();
+    }
   }
+
+ private:
+  std::string filename_;
+  scoped_ptr<base::File> file_;
+  const UMALogger* uma_logger_;
 };
 
-#if defined(OS_WIN)
-class IDBEnvWin : public ChromiumEnvWin {
+class ChromiumRandomAccessFile : public leveldb::RandomAccessFile {
+ public:
+  ChromiumRandomAccessFile(const std::string& fname,
+                           ::base::File file,
+                           const UMALogger* uma_logger)
+      : filename_(fname), file_(file.Pass()), uma_logger_(uma_logger) {}
+  virtual ~ChromiumRandomAccessFile() {}
+
+  virtual Status Read(uint64_t offset,
+                      size_t n,
+                      Slice* result,
+                      char* scratch) const {
+    Status s;
+    int r = file_.Read(offset, scratch, n);
+    *result = Slice(scratch, (r < 0) ? 0 : r);
+    if (r < 0) {
+      // An error: return a non-ok status
+      s = MakeIOError(filename_, "Could not perform read",
+                      kRandomAccessFileRead);
+      uma_logger_->RecordErrorAt(kRandomAccessFileRead);
+    }
+    return s;
+  }
+
+ private:
+  std::string filename_;
+  mutable ::base::File file_;
+  const UMALogger* uma_logger_;
+};
+
+class ChromiumWritableFile : public leveldb::WritableFile {
  public:
-  IDBEnvWin() : ChromiumEnvWin() {
+  ChromiumWritableFile(const std::string& fname,
+                       base::File* f,
+                       const UMALogger* uma_logger,
+                       WriteTracker* tracker,
+                       bool make_backup);
+  virtual ~ChromiumWritableFile() {}
+  virtual leveldb::Status Append(const leveldb::Slice& data);
+  virtual leveldb::Status Close();
+  virtual leveldb::Status Flush();
+  virtual leveldb::Status Sync();
+
+ private:
+  enum Type { kManifest, kTable, kOther };
+  leveldb::Status SyncParent();
+
+  std::string filename_;
+  scoped_ptr<base::File> file_;
+  const UMALogger* uma_logger_;
+  WriteTracker* tracker_;
+  Type file_type_;
+  std::string parent_dir_;
+  bool make_backup_;
+};
+
+ChromiumWritableFile::ChromiumWritableFile(const std::string& fname,
+                                           base::File* f,
+                                           const UMALogger* uma_logger,
+                                           WriteTracker* tracker,
+                                           bool make_backup)
+    : filename_(fname),
+      file_(f),
+      uma_logger_(uma_logger),
+      tracker_(tracker),
+      file_type_(kOther),
+      make_backup_(make_backup) {
+  base::FilePath path = base::FilePath::FromUTF8Unsafe(fname);
+  if (FilePathToString(path.BaseName()).find("MANIFEST") == 0)
+    file_type_ = kManifest;
+  else if (ChromiumEnv::HasTableExtension(path))
+    file_type_ = kTable;
+  if (file_type_ != kManifest)
+    tracker_->DidCreateNewFile(filename_);
+  parent_dir_ = FilePathToString(ChromiumEnv::CreateFilePath(fname).DirName());
+}
+
+Status ChromiumWritableFile::SyncParent() {
+  TRACE_EVENT0("leveldb", "SyncParent");
+#if defined(OS_POSIX)
+  base::FilePath path = base::FilePath::FromUTF8Unsafe(parent_dir_);
+  base::File f(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
+  if (!f.IsValid()) {
+    return MakeIOError(parent_dir_, "Unable to open directory", kSyncParent,
+                       f.error_details());
+  } else if (!f.Flush()) {

[jsbell, 2014/11/12 01:04:04]

Since the previous if() clause returns, drop the else.

[cmumford, 2014/11/12 20:59:32]

Done.

+    int saved_errno = errno;
+    return MakeIOError(parent_dir_, strerror(saved_errno), kSyncParent,
+                       saved_errno);
+  }
+#endif
+  return Status::OK();
+}
+
+Status ChromiumWritableFile::Append(const Slice& data) {
+  if (file_type_ == kManifest && tracker_->DoesDirNeedSync(filename_)) {
+    Status s = SyncParent();
+    if (!s.ok())
+      return s;
+    tracker_->DidSyncDir(filename_);
+  }
+
+  int bytes_written = file_->WriteAtCurrentPos(data.data(), data.size());
+  if (bytes_written != data.size()) {
+    int saved_errno = errno;
+    uma_logger_->RecordOSError(kWritableFileAppend, saved_errno);
+    return MakeIOError(filename_, strerror(saved_errno), kWritableFileAppend,
+                       saved_errno);
+  } else {

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

Drop the else, this can be unconditional since the previous clause returns.

[cmumford, 2014/11/12 20:59:32]

Done.

+    return Status::OK();
+  }
+}
+
+Status ChromiumWritableFile::Close() {
+  file_->Close();
+  return Status::OK();
+}
+
+Status ChromiumWritableFile::Flush() {
+  // base::File doesn't do buffered I/O (i.e. POSIX FILE streams) so nothing to

[jsbell, 2014/11/12 01:04:04]

Agreed, this matches previous posix/win32 env behavior... it's weird that
leveldb even requires this.

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

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.

+  // flush.
+  return Status::OK();
+}
+
+Status ChromiumWritableFile::Sync() {
+  TRACE_EVENT0("leveldb", "WritableFile::Sync");
+
+  if (!file_->Flush()) {

[jsbell, 2014/11/12 01:04:04]

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?

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

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.

+    int saved_errno = errno;
+    uma_logger_->RecordErrorAt(kWritableFileSync);
+    return MakeIOError(filename_, strerror(saved_errno), kWritableFileSync,
+                       saved_errno);
+  }
+
+  if (make_backup_ && file_type_ == kTable) {

[jsbell, 2014/11/12 01:04:04]

Can drop the {} here

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

Done.

+    uma_logger_->RecordBackupResult(ChromiumEnv::MakeBackup(filename_));
+  }
+
+  return Status::OK();
+}
+
+class IDBEnv : public ChromiumEnv {
+ public:
+  IDBEnv() : ChromiumEnv() {
     name_ = "LevelDBEnv.IDB";
     make_backup_ = true;
   }
 };
-#endif
 
-#if defined(OS_WIN)
-::base::LazyInstance<IDBEnvWin>::Leaky idb_env =
-    LAZY_INSTANCE_INITIALIZER;
-#else
-::base::LazyInstance<IDBEnvStdio>::Leaky idb_env =
-    LAZY_INSTANCE_INITIALIZER;
-#endif
+::base::LazyInstance<IDBEnv>::Leaky idb_env = LAZY_INSTANCE_INITIALIZER;
 
-::base::LazyInstance<ChromiumEnvStdio>::Leaky default_env =
+::base::LazyInstance<ChromiumEnv>::Leaky default_env =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // unnamed namespace
@@ -466,19 +622,13 @@ void ChromiumEnv::RestoreIfNecessary(const std::string& dir,
 
 Status ChromiumEnv::GetChildren(const std::string& dir_string,
                                 std::vector<std::string>* result) {
-  std::vector<base::FilePath> entries;
-  base::File::Error error =
-      GetDirectoryEntries(CreateFilePath(dir_string), &entries);

[jsbell, 2014/11/12 01:04:04]

This was added to avoid early-exit on error. Does the FileEnumerator have the
same behavior?

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

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.

-  if (error != base::File::FILE_OK) {
-    RecordOSError(kGetChildren, error);
-    return MakeIOError(
-        dir_string, "Could not open/read directory", kGetChildren, error);
-  }
   result->clear();
-  for (std::vector<base::FilePath>::iterator it = entries.begin();
-       it != entries.end();
-       ++it) {
-    result->push_back(FilePathToString(*it));
+  base::FileEnumerator enumerator(CreateFilePath(dir_string),
+                                  false,  // not recursive
+                                  base::FileEnumerator::FILES);
+  for (base::FilePath current = enumerator.Next(); !current.empty();
+       current = enumerator.Next()) {
+    result->push_back(FilePathToString(current.BaseName()));
   }
 
   if (make_backup_)
@@ -655,6 +805,80 @@ Status ChromiumEnv::GetTestDirectory(std::string* path) {
   return Status::OK();
 }
 
+Status ChromiumEnv::NewLogger(const std::string& fname,
+                              leveldb::Logger** result) {
+  base::FilePath path = CreateFilePath(fname);
+  scoped_ptr<base::File> f(new base::File(
+      path, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE));
+  if (!f->IsValid()) {
+    *result = NULL;
+    RecordOSError(kNewLogger, f->error_details());
+    return MakeIOError(fname, "Unable to create log file", kNewLogger,
+                       f->error_details());
+  } else {
+    *result = new leveldb::ChromiumLogger(f.release());
+    return Status::OK();
+  }
+}
+
+Status ChromiumEnv::NewSequentialFile(const std::string& fname,
+                                      leveldb::SequentialFile** result) {
+  base::FilePath path = CreateFilePath(fname);
+  scoped_ptr<base::File> f(
+      new base::File(path, base::File::FLAG_OPEN | base::File::FLAG_READ));
+  if (!f->IsValid()) {
+    *result = NULL;
+    int saved_errno = errno;
+    RecordOSError(kNewSequentialFile, f->error_details());
+    return MakeIOError(fname, "Unable to create sequential file",
+                       kNewSequentialFile, f->error_details());
+  } else {
+    *result = new ChromiumSequentialFile(fname, f.release(), this);
+    return Status::OK();
+  }
+}
+
+void ChromiumEnv::RecordOpenFilesLimit(const std::string& type) {
+  GetMaxFDHistogram(type)->Add(base::SharedMemory::GetHandleLimit());

[jsbell, 2014/11/12 01:04:04]

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:32]

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

+}
+
+Status ChromiumEnv::NewRandomAccessFile(const std::string& fname,
+                                        leveldb::RandomAccessFile** result) {
+  int flags = ::base::File::FLAG_READ | ::base::File::FLAG_OPEN;
+  ::base::File file(ChromiumEnv::CreateFilePath(fname), flags);
+  if (file.IsValid()) {
+    *result = new ChromiumRandomAccessFile(fname, file.Pass(), this);
+    RecordOpenFilesLimit("Success");
+    return Status::OK();
+  }
+  ::base::File::Error error_code = file.error_details();
+  if (error_code == ::base::File::FILE_ERROR_TOO_MANY_OPENED)
+    RecordOpenFilesLimit("TooManyOpened");
+  else
+    RecordOpenFilesLimit("OtherError");
+  *result = NULL;
+  RecordOSError(kNewRandomAccessFile, error_code);
+  return MakeIOError(fname, FileErrorString(error_code), kNewRandomAccessFile,
+                     error_code);
+}
+
+Status ChromiumEnv::NewWritableFile(const std::string& fname,
+                                    leveldb::WritableFile** result) {
+  *result = NULL;
+  base::FilePath path = CreateFilePath(fname);
+  scoped_ptr<base::File> f(new base::File(
+      path, base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE));
+  if (!f->IsValid()) {
+    RecordErrorAt(kNewWritableFile);
+    return MakeIOError(fname, "Unable to create writable file",
+                       kNewWritableFile, f->error_details());
+  } else {
+    *result =
+        new ChromiumWritableFile(fname, f.release(), this, this, make_backup_);
+    return Status::OK();
+  }
+}
+
 uint64_t ChromiumEnv::NowMicros() {
   return ::base::TimeTicks::Now().ToInternalValue();
 }