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..7794d10919b047581d12856cd0bc967fdfb81866 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;
@@ -38,6 +31,14 @@ const base::FilePath::CharType table_extension[] = FILE_PATH_LITERAL(".ldb");
 static const base::FilePath::CharType kLevelDBTestDirectoryPrefix[]
     = FILE_PATH_LITERAL("leveldb-test-");
 
+static base::File::Error LastFileError() {
+#if defined(OS_WIN)
+  return base::File::OSErrorToFileError(GetLastError());
+#else
+  return base::File::OSErrorToFileError(errno);
+#endif
+}
+
 class ChromiumFileLock : public FileLock {
  public:
   ::base::File file_;
@@ -88,33 +89,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() {}
+
+  Status Read(size_t n, Slice* result, char* scratch) override {
+    int bytes_read = file_->ReadAtCurrentPos(scratch, n);
+    if (bytes_read == -1) {
+      base::File::Error error = LastFileError();
+      uma_logger_->RecordErrorAt(kSequentialFileRead);
+      return MakeIOError(filename_, base::File::ErrorToString(error),
+                         kSequentialFileRead, error);
+    } else {
+      *result = Slice(scratch, bytes_read);
+      return Status::OK();
+    }
+  }
+
+  Status Skip(uint64_t n) override {
+    if (file_->Seek(base::File::FROM_CURRENT, n) == -1) {
+      base::File::Error error = LastFileError();
+      uma_logger_->RecordErrorAt(kSequentialFileSkip);
+      return MakeIOError(filename_, base::File::ErrorToString(error),
+                         kSequentialFileSkip, error);
+    } 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() {}
+
+  Status Read(uint64_t offset,
+                      size_t n,
+                      Slice* result,
+                      char* scratch) const override {
+    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:
+  ChromiumWritableFile(const std::string& fname,
+                       base::File* f,
+                       const UMALogger* uma_logger,
+                       WriteTracker* tracker,
+                       bool make_backup);
+  virtual ~ChromiumWritableFile() {}
+  leveldb::Status Append(const leveldb::Slice& data) override;
+  leveldb::Status Close() override;
+  leveldb::Status Flush() override;
+  leveldb::Status Sync() override;
+
+ 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());
+  }
+  if (!f.Flush()) {
+    base::File::Error error = LastFileError();
+    return MakeIOError(parent_dir_, base::File::ErrorToString(error),
+                       kSyncParent, error);
+  }
+#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()) {
+    base::File::Error error = LastFileError();
+    uma_logger_->RecordOSError(kWritableFileAppend, error);
+    return MakeIOError(filename_, base::File::ErrorToString(error),
+                       kWritableFileAppend, error);
+  }
+
+  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
+  // flush.
+  return Status::OK();
+}
+
+Status ChromiumWritableFile::Sync() {
+  TRACE_EVENT0("leveldb", "WritableFile::Sync");
+
+  if (!file_->Flush()) {
+    base::File::Error error = LastFileError();
+    uma_logger_->RecordErrorAt(kWritableFileSync);
+    return MakeIOError(filename_, base::File::ErrorToString(error),
+                       kWritableFileSync, error);
+  }
+
+  if (make_backup_ && file_type_ == kTable)
+    uma_logger_->RecordBackupResult(ChromiumEnv::MakeBackup(filename_));
+
+  return Status::OK();
+}
+
+class IDBEnv : public ChromiumEnv {
  public:
-  IDBEnvWin() : ChromiumEnvWin() {
+  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
@@ -172,44 +336,22 @@ const char* MethodIDToString(MethodID method) {
 }
 
 Status MakeIOError(Slice filename,
-                   const char* message,
-                   MethodID method,
-                   int saved_errno) {
-  char buf[512];
-  snprintf(buf,
-           sizeof(buf),
-           "%s (ChromeMethodErrno: %d::%s::%d)",
-           message,
-           method,
-           MethodIDToString(method),
-           saved_errno);
-  return Status::IOError(filename, buf);
-}
-
-Status MakeIOError(Slice filename,
-                   const char* message,
+                   const std::string& message,
                    MethodID method,
                    base::File::Error error) {
   DCHECK_LT(error, 0);
   char buf[512];
-  snprintf(buf,
-           sizeof(buf),
-           "%s (ChromeMethodPFE: %d::%s::%d)",
-           message,
-           method,
-           MethodIDToString(method),
-           -error);
+  snprintf(buf, sizeof(buf), "%s (ChromeMethodPFE: %d::%s::%d)",
+           message.c_str(), method, MethodIDToString(method), -error);
   return Status::IOError(filename, buf);
 }
 
-Status MakeIOError(Slice filename, const char* message, MethodID method) {
+Status MakeIOError(Slice filename,
+                   const std::string& message,
+                   MethodID method) {
   char buf[512];
-  snprintf(buf,
-           sizeof(buf),
-           "%s (ChromeMethodOnly: %d::%s)",
-           message,
-           method,
-           MethodIDToString(method));
+  snprintf(buf, sizeof(buf), "%s (ChromeMethodOnly: %d::%s)", message.c_str(),
+           method, MethodIDToString(method));
   return Status::IOError(filename, buf);
 }
 
@@ -466,19 +608,19 @@ 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);
-  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::FilePath dir_path = CreateFilePath(dir_string);
+  if (!DirectoryExists(dir_path)) {
+    RecordErrorAt(kGetChildren);
+    return MakeIOError(dir_string, "Could not open/read directory",
+                       kGetChildren);
+  }
+  base::FileEnumerator enumerator(dir_path,

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

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=...

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

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.

+                                  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 +797,85 @@ 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;
+    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) {
+#if defined(OS_POSIX)
+  GetMaxFDHistogram(type)->Add(base::GetMaxFds());
+#elif defined(OS_WIN)
+  // Windows is only limited by available memory
+#else
+#error "Need to determine limit to open files for this OS"
+#endif
+}
+
+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();
 }
@@ -679,12 +900,6 @@ void ChromiumEnv::RecordOSError(MethodID method,
   GetOSErrorHistogram(method, -base::File::FILE_ERROR_MAX)->Add(-error);
 }
 
-void ChromiumEnv::RecordOSError(MethodID method, int error) const {
-  DCHECK_GT(error, 0);
-  RecordErrorAt(method);
-  GetOSErrorHistogram(method, ERANGE + 1)->Add(error);
-}
-
 void ChromiumEnv::RecordBackupResult(bool result) const {
   std::string uma_name(name_);
   uma_name.append(".TableBackup");
@@ -765,7 +980,7 @@ class Thread : public ::base::PlatformThread::Delegate {
     DCHECK(success);
   }
   virtual ~Thread() {}
-  virtual void ThreadMain() {
+  void ThreadMain() override {
     (*function_)(arg_);
     delete this;
   }