From 4b50bd14f1aba682cb53bf2d036f85b94208c9e7 Mon Sep 17 00:00:00 2001 From: "dgrogan@chromium.org" Date: Fri, 7 Jun 2013 01:36:31 +0000 Subject: [PATCH] Only fsync leveldb's directory when the manifest is being updated. That is, when the manifest is being updated and new have files have been created since the last directory fsync. BUG=242269 R=jsbell@chromium.org Review URL: https://codereview.chromium.org/15843010 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204688 0039d316-1c4b-4281-b951-d872f2087c98 --- third_party/leveldatabase/env_chromium.cc | 118 +++++++++++++-------- third_party/leveldatabase/env_chromium.h | 29 ++++- third_party/leveldatabase/env_chromium_unittest.cc | 48 +++++++++ 3 files changed, 148 insertions(+), 47 deletions(-) diff --git a/third_party/leveldatabase/env_chromium.cc b/third_party/leveldatabase/env_chromium.cc index 4d296fa6d9c8..d88f46027b7e 100644 --- a/third_party/leveldatabase/env_chromium.cc +++ b/third_party/leveldatabase/env_chromium.cc @@ -128,28 +128,6 @@ base::FilePath CreateFilePath(const std::string& file_path) { #endif } -std::string FilePathToString(const base::FilePath& file_path) { -#if defined(OS_WIN) - return UTF16ToUTF8(file_path.value()); -#else - return file_path.value(); -#endif -} - -bool sync_parent(const std::string& fname) { -#if !defined(OS_WIN) - TRACE_EVENT0("leveldb", "sync_parent"); - base::FilePath parent_dir = CreateFilePath(fname).DirName(); - int parent_fd = - HANDLE_EINTR(open(FilePathToString(parent_dir).c_str(), O_RDONLY)); - if (parent_fd < 0) - return false; - HANDLE_EINTR(fsync(parent_fd)); - HANDLE_EINTR(close(parent_fd)); -#endif - return true; -} - const char* MethodIDToString(MethodID method) { switch (method) { case kSequentialFileRead: @@ -190,6 +168,8 @@ const char* MethodIDToString(MethodID method) { return "GetTestDirectory"; case kNewLogger: return "NewLogger"; + case kSyncParent: + return "SyncParent"; case kNumEntries: NOTREACHED(); return "kNumEntries"; @@ -427,10 +407,27 @@ bool ParseMethodAndError(const char* string, int* method, int* error) { return false; } +std::string FilePathToString(const base::FilePath& file_path) { +#if defined(OS_WIN) + return UTF16ToUTF8(file_path.value()); +#else + return file_path.value(); +#endif +} + ChromiumWritableFile::ChromiumWritableFile(const std::string& fname, FILE* f, - const UMALogger* uma_logger) - : filename_(fname), file_(f), uma_logger_(uma_logger) {} + const UMALogger* uma_logger, + WriteTracker* tracker) + : filename_(fname), file_(f), uma_logger_(uma_logger), tracker_(tracker) { + base::FilePath path = base::FilePath::FromUTF8Unsafe(fname); + is_manifest_ = + FilePathToString(path.BaseName()).find("MANIFEST") != + std::string::npos; + if (!is_manifest_) + tracker_->DidCreateNewFile(filename_); + parent_dir_ = FilePathToString(CreateFilePath(fname).DirName()); +} ChromiumWritableFile::~ChromiumWritableFile() { if (file_ != NULL) { @@ -439,15 +436,37 @@ ChromiumWritableFile::~ChromiumWritableFile() { } } +Status ChromiumWritableFile::SyncParent() { + Status s; +#if !defined(OS_WIN) + TRACE_EVENT0("leveldb", "SyncParent"); + + int parent_fd = + HANDLE_EINTR(open(parent_dir_.c_str(), O_RDONLY)); + if (parent_fd < 0) + return MakeIOError(parent_dir_, strerror(errno), kSyncParent); + if (HANDLE_EINTR(fsync(parent_fd)) != 0) { + s = MakeIOError(parent_dir_, strerror(errno), kSyncParent); + }; + HANDLE_EINTR(close(parent_fd)); +#endif + return s; +} + Status ChromiumWritableFile::Append(const Slice& data) { + if (is_manifest_ && tracker_->DoesDirNeedSync(filename_)) { + Status s = SyncParent(); + if (!s.ok()) + return s; + tracker_->DidSyncDir(filename_); + } + size_t r = fwrite_wrapper(data.data(), 1, data.size(), file_); - Status result; if (r != data.size()) { uma_logger_->RecordOSError(kWritableFileAppend, errno); - result = - MakeIOError(filename_, strerror(errno), kWritableFileAppend, errno); + return MakeIOError(filename_, strerror(errno), kWritableFileAppend, errno); } - return result; + return Status::OK(); } Status ChromiumWritableFile::Close() { @@ -497,7 +516,11 @@ ChromiumEnv::ChromiumEnv() kMaxRetryTimeMillis(1000) { } -ChromiumEnv::~ChromiumEnv() { NOTREACHED(); } +ChromiumEnv::~ChromiumEnv() { + // In chromium, ChromiumEnv is leaked. It'd be nice to add NOTREACHED here to + // ensure that behavior isn't accidentally changed, but there's an instance in + // a unit test that is deleted. +} Status ChromiumEnv::NewSequentialFile(const std::string& fname, SequentialFile** result) { @@ -555,12 +578,7 @@ Status ChromiumEnv::NewWritableFile(const std::string& fname, RecordErrorAt(kNewWritableFile); return MakeIOError(fname, strerror(errno), kNewWritableFile, errno); } else { - if (!sync_parent(fname)) { - fclose(f); - RecordErrorAt(kNewWritableFile); - return MakeIOError(fname, strerror(errno), kNewWritableFile, errno); - } - *result = new ChromiumWritableFile(fname, f, this); + *result = new ChromiumWritableFile(fname, f, this, this); return Status::OK(); } } @@ -639,12 +657,6 @@ Status ChromiumEnv::RenameFile(const std::string& src, const std::string& dst) { do { if (::file_util::ReplaceFileAndGetError( src_file_path, destination, &error)) { - sync_parent(dst); - if (src_file_path.DirName() != destination.DirName()) { - // As leveldb is implemented now this branch will never be taken, but - // this is future-proof. - sync_parent(src); - } return result; } } while (retrier.ShouldKeepTrying(error)); @@ -738,10 +750,6 @@ Status ChromiumEnv::NewLogger(const std::string& fname, Logger** result) { RecordOSError(kNewLogger, saved_errno); return MakeIOError(fname, strerror(saved_errno), kNewLogger, saved_errno); } else { - if (!sync_parent(fname)) { - fclose(f); - return MakeIOError(fname, strerror(errno), kNewLogger, errno); - } *result = new ChromiumLogger(f); return Status::OK(); } @@ -907,6 +915,26 @@ void ChromiumEnv::StartThread(void (*function)(void* arg), void* arg) { new Thread(function, arg); // Will self-delete. } +static std::string GetDirName(const std::string& filename) { + base::FilePath file = base::FilePath::FromUTF8Unsafe(filename); + return FilePathToString(file.DirName()); +} + +void ChromiumEnv::DidCreateNewFile(const std::string& filename) { + base::AutoLock auto_lock(map_lock_); + needs_sync_map_[GetDirName(filename)] = true; +} + +bool ChromiumEnv::DoesDirNeedSync(const std::string& filename) { + base::AutoLock auto_lock(map_lock_); + return needs_sync_map_.find(GetDirName(filename)) != needs_sync_map_.end(); +} + +void ChromiumEnv::DidSyncDir(const std::string& filename) { + base::AutoLock auto_lock(map_lock_); + needs_sync_map_.erase(GetDirName(filename)); +} + } // namespace leveldb_env namespace leveldb { diff --git a/third_party/leveldatabase/env_chromium.h b/third_party/leveldatabase/env_chromium.h index 5b7b2122eb0f..38a37590b598 100644 --- a/third_party/leveldatabase/env_chromium.h +++ b/third_party/leveldatabase/env_chromium.h @@ -5,6 +5,8 @@ #ifndef THIRD_PARTY_LEVELDATABASE_ENV_CHROMIUM_H_ #define THIRD_PARTY_LEVELDATABASE_ENV_CHROMIUM_H_ +#include + #include "base/metrics/histogram.h" #include "base/platform_file.h" #include "base/synchronization/condition_variable.h" @@ -34,6 +36,7 @@ enum MethodID { kUnlockFile, kGetTestDirectory, kNewLogger, + kSyncParent, kNumEntries }; @@ -50,6 +53,7 @@ leveldb::Status MakeIOError(leveldb::Slice filename, MethodID method); bool ParseMethodAndError(const char* string, int* method, int* error); +std::string FilePathToString(const base::FilePath& file_path); class UMALogger { public: @@ -67,11 +71,19 @@ 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 ChromiumWritableFile : public leveldb::WritableFile { public: ChromiumWritableFile(const std::string& fname, FILE* f, - const UMALogger* uma_logger); + const UMALogger* uma_logger, + WriteTracker* tracker); virtual ~ChromiumWritableFile(); virtual leveldb::Status Append(const leveldb::Slice& data); virtual leveldb::Status Close(); @@ -79,14 +91,20 @@ class ChromiumWritableFile : public leveldb::WritableFile { virtual leveldb::Status Sync(); private: + leveldb::Status SyncParent(); + std::string filename_; FILE* file_; const UMALogger* uma_logger_; + WriteTracker* tracker_; + bool is_manifest_; + std::string parent_dir_; }; class ChromiumEnv : public leveldb::Env, public UMALogger, - public RetrierProvider { + public RetrierProvider, + public WriteTracker { public: ChromiumEnv(); virtual ~ChromiumEnv(); @@ -119,9 +137,16 @@ class ChromiumEnv : public leveldb::Env, virtual void SleepForMicroseconds(int micros); protected: + virtual void DidCreateNewFile(const std::string& fname); + virtual bool DoesDirNeedSync(const std::string& fname); + virtual void DidSyncDir(const std::string& fname); + std::string name_; private: + std::map needs_sync_map_; + base::Lock map_lock_; + const int kMaxRetryTimeMillis; // BGThread() is the body of the background thread void BGThread(); diff --git a/third_party/leveldatabase/env_chromium_unittest.cc b/third_party/leveldatabase/env_chromium_unittest.cc index 8db8aee7131c..eae2d6a1bb21 100644 --- a/third_party/leveldatabase/env_chromium_unittest.cc +++ b/third_party/leveldatabase/env_chromium_unittest.cc @@ -2,6 +2,8 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/files/file_path.h" +#include "base/files/scoped_temp_dir.h" #include "base/test/test_suite.h" #include "env_chromium.h" #include "testing/gtest/include/gtest/gtest.h" @@ -52,4 +54,50 @@ TEST(ErrorEncoding, NoEncodedMessage) { EXPECT_EQ(4, error); } +class MyEnv : public ChromiumEnv { + public: + MyEnv() : directory_syncs_(0) {} + int directory_syncs() { return directory_syncs_; } + + protected: + virtual void DidSyncDir(const std::string& fname) { + ++directory_syncs_; + ChromiumEnv::DidSyncDir(fname); + } + + private: + int directory_syncs_; +}; + +TEST(ChromiumEnv, DirectorySyncing) { + MyEnv env; + base::ScopedTempDir dir; + dir.CreateUniqueTempDir(); + base::FilePath dir_path = dir.path(); + std::string some_data = "some data"; + Slice data = some_data; + + std::string manifest_file_name = + FilePathToString(dir_path.Append("MANIFEST-001")); + WritableFile* manifest_file; + Status s = env.NewWritableFile(manifest_file_name, &manifest_file); + EXPECT_TRUE(s.ok()); + 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 = FilePathToString(dir_path.Append("000003.sst")); + WritableFile* sst_file; + s = env.NewWritableFile(sst_file_name, &sst_file); + EXPECT_TRUE(s.ok()); + 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 main(int argc, char** argv) { return base::TestSuite(argc, argv).Run(); } -- 2.11.4.GIT