leveldb: Env no longer implicitly syncs dir for all newly created files.

A while back ChromiumEnv was enhanced to sync a directory the first time a
newly created file was written to. This was a speculative change added in an
attempt to resolve database corruptions caused by missing files. Since no
improvements were seen as a result of this change it is now being removed.

Additionally leveldb's implicit contract for WritableFile::Sync() is that it
shall sync the directory if that WritableFile is for a manifest - ChromiumEnv
did not do that.

BUG=417769

Review URL: https://codereview.chromium.org/1022983004

Cr-Commit-Position: refs/heads/master@{#321904}

third_party/leveldatabase/env_chromium.cc

@@ -216,7 +216,6 @@ class ChromiumWritableFile : public leveldb::WritableFile {
   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;
@@ -231,7 +230,6 @@ class ChromiumWritableFile : public leveldb::WritableFile {
   std::string filename_;
   scoped_ptr<base::File> file_;
   const UMALogger* uma_logger_;
-  WriteTracker* tracker_;
   Type file_type_;
   std::string parent_dir_;
   bool make_backup_;
@@ -240,12 +238,10 @@ class ChromiumWritableFile : public leveldb::WritableFile {
 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) {
   FilePath path = FilePath::FromUTF8Unsafe(fname);
@@ -253,8 +249,6 @@ ChromiumWritableFile::ChromiumWritableFile(const std::string& fname,
     file_type_ = kManifest;
   else if (path.MatchesExtension(table_extension))
     file_type_ = kTable;
-  if (file_type_ != kManifest)
-    tracker_->DidCreateNewFile(filename_);
   parent_dir_ = FilePath::FromUTF8Unsafe(fname).DirName().AsUTF8Unsafe();
 }
 
@@ -277,13 +271,6 @@ Status ChromiumWritableFile::SyncParent() {
 }
 
 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();
@@ -319,6 +306,12 @@ Status ChromiumWritableFile::Sync() {
   if (make_backup_ && file_type_ == kTable)
     uma_logger_->RecordBackupResult(ChromiumEnv::MakeBackup(filename_));
 
+  // leveldb's implicit contract for Sync() is that if this instance is for a
+  // manifest file then the directory is also sync'ed. See leveldb's
+  // env_posix.cc.
+  if (file_type_ == kManifest)
+    return SyncParent();
+
   return Status::OK();
 }
 
@@ -888,8 +881,7 @@ Status ChromiumEnv::NewWritableFile(const std::string& fname,
     return MakeIOError(fname, "Unable to create writable file",
                        kNewWritableFile, f->error_details());
   } else {
-    *result =
-        new ChromiumWritableFile(fname, f.release(), this, this, make_backup_);
+    *result = new ChromiumWritableFile(fname, f.release(), this, make_backup_);
     return Status::OK();
   }
 }
@@ -905,8 +897,7 @@ Status ChromiumEnv::NewAppendableFile(const std::string& fname,
     return MakeIOError(fname, "Unable to create appendable file",
                        kNewAppendableFile, f->error_details());
   }
-  *result =
-      new ChromiumWritableFile(fname, f.release(), this, this, make_backup_);
+  *result = new ChromiumWritableFile(fname, f.release(), this, make_backup_);
   return Status::OK();
 }
 
@@ -1071,25 +1062,6 @@ void ChromiumEnv::StartThread(void (*function)(void* arg), void* arg) {
   new Thread(function, arg);  // Will self-delete.
 }
 
-static std::string GetDirName(const std::string& filename) {
-  return FilePath::FromUTF8Unsafe(filename).DirName().AsUTF8Unsafe();
-}
-
-void ChromiumEnv::DidCreateNewFile(const std::string& filename) {
-  base::AutoLock auto_lock(directory_sync_lock_);
-  directories_needing_sync_.insert(GetDirName(filename));
-}
-
-bool ChromiumEnv::DoesDirNeedSync(const std::string& filename) {
-  base::AutoLock auto_lock(directory_sync_lock_);
-  return ContainsKey(directories_needing_sync_, GetDirName(filename));
-}
-
-void ChromiumEnv::DidSyncDir(const std::string& filename) {
-  base::AutoLock auto_lock(directory_sync_lock_);
-  directories_needing_sync_.erase(GetDirName(filename));
-}
-
 }  // namespace leveldb_env
 
 namespace leveldb {

third_party/leveldatabase/env_chromium.h

@@ -87,17 +87,9 @@ class RetrierProvider {
       MethodID method) const = 0;
 };
 
-class WriteTracker {
- public:
-  virtual void DidCreateNewFile(const std::string& fname) = 0;
-  virtual bool DoesDirNeedSync(const std::string& fname) = 0;
-  virtual void DidSyncDir(const std::string& fname) = 0;
-};
-
 class ChromiumEnv : public leveldb::Env,
                     public UMALogger,
-                    public RetrierProvider,
-                    public WriteTracker {
+                    public RetrierProvider {
  public:
   ChromiumEnv();
 
@@ -136,16 +128,12 @@ class ChromiumEnv : public leveldb::Env,
                                     leveldb::Logger** result);
 
  protected:
-  virtual void DidSyncDir(const std::string& fname);
-
   std::string name_;
   bool make_backup_;
 
  private:
   static const char* FileErrorString(base::File::Error error);
 
-  virtual void DidCreateNewFile(const std::string& fname);
-  virtual bool DoesDirNeedSync(const std::string& fname);
   virtual void RecordErrorAt(MethodID method) const;
   virtual void RecordOSError(MethodID method,
                              base::File::Error error) const;
@@ -170,9 +158,6 @@ class ChromiumEnv : public leveldb::Env,
     std::set<std::string> locked_files_;
   };
 
-  std::set<std::string> directories_needing_sync_;
-  base::Lock directory_sync_lock_;
-
   const int kMaxRetryTimeMillis;
   // BGThread() is the body of the background thread
   void BGThread();

third_party/leveldatabase/env_chromium_unittest.cc

@@ -58,22 +58,6 @@ TEST(ErrorEncoding, NoEncodedMessage) {
   EXPECT_EQ(base::File::FILE_ERROR_MAX, error);
 }
 
-template <typename T>
-class MyEnv : public T {
- public:
-  MyEnv() : directory_syncs_(0) {}
-  int directory_syncs() { return directory_syncs_; }
-
- protected:
-  virtual void DidSyncDir(const std::string& fname) {
-    ++directory_syncs_;
-    leveldb_env::ChromiumEnv::DidSyncDir(fname);
-  }
-
- private:
-  int directory_syncs_;
-};
-
 template <typename T>
 class ChromiumEnvMultiPlatformTests : public ::testing::Test {
  public:
@@ -83,41 +67,6 @@ typedef ::testing::Types<ChromiumEnv> ChromiumEnvMultiPlatformTestsTypes;
 TYPED_TEST_CASE(ChromiumEnvMultiPlatformTests,
                 ChromiumEnvMultiPlatformTestsTypes);
 
-TYPED_TEST(ChromiumEnvMultiPlatformTests, DirectorySyncing) {
-  MyEnv<TypeParam> env;
-
-  base::ScopedTempDir dir;
-  ASSERT_TRUE(dir.CreateUniqueTempDir());
-  base::FilePath dir_path = dir.path();
-  std::string some_data = "some data";
-  Slice data = some_data;
-
-  std::string manifest_file_name =
-      dir_path.Append(FILE_PATH_LITERAL("MANIFEST-001")).AsUTF8Unsafe();
-  WritableFile* manifest_file_ptr;
-  Status s = env.NewWritableFile(manifest_file_name, &manifest_file_ptr);
-  EXPECT_TRUE(s.ok());
-  scoped_ptr<WritableFile> manifest_file(manifest_file_ptr);
-  manifest_file->Append(data);
-  EXPECT_EQ(0, env.directory_syncs());
-  manifest_file->Append(data);
-  EXPECT_EQ(0, env.directory_syncs());
-
-  std::string sst_file_name =
-      dir_path.Append(FILE_PATH_LITERAL("000003.sst")).AsUTF8Unsafe();
-  WritableFile* sst_file_ptr;
-  s = env.NewWritableFile(sst_file_name, &sst_file_ptr);
-  EXPECT_TRUE(s.ok());
-  scoped_ptr<WritableFile> sst_file(sst_file_ptr);
-  sst_file->Append(data);
-  EXPECT_EQ(0, env.directory_syncs());
-
-  manifest_file->Append(data);
-  EXPECT_EQ(1, env.directory_syncs());
-  manifest_file->Append(data);
-  EXPECT_EQ(1, env.directory_syncs());
-}
-
 int CountFilesWithExtension(const base::FilePath& dir,
                             const base::FilePath::StringType& extension) {
   int matching_files = 0;