From 42733637e17511c42b2a2ced9c4a0406434be434 Mon Sep 17 00:00:00 2001 From: Andrew Kryczka Date: Tue, 28 Aug 2018 12:35:17 -0700 Subject: [PATCH] Sync CURRENT file during checkpoint (#4322) Summary: For the CURRENT file forged during checkpoint, we were forgetting to `fsync` or `fdatasync` it after its creation. This PR fixes it. Differential Revision: D9525939 Pulled By: ajkr fbshipit-source-id: a505483644026ee3f501cfc0dcbe74832165b2e3 --- HISTORY.md | 1 + db/repair_test.cc | 4 ++-- util/file_util.cc | 8 +++++-- util/file_util.h | 2 +- utilities/checkpoint/checkpoint_impl.cc | 3 ++- utilities/checkpoint/checkpoint_test.cc | 29 ++++++++++++++++++++++++- 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/HISTORY.md b/HISTORY.md index 7fd81aa3f..728b9d227 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -6,6 +6,7 @@ ### New Features ### Bug Fixes * Avoid creating empty SSTs and subsequently deleting them in certain cases during compaction. +* Sync CURRENT file contents during checkpoint. ## 5.16.0 (8/21/2018) ### Public API Change diff --git a/db/repair_test.cc b/db/repair_test.cc index 93dcfc14d..72abd62a5 100644 --- a/db/repair_test.cc +++ b/db/repair_test.cc @@ -74,7 +74,7 @@ TEST_F(RepairTest, CorruptManifest) { Close(); ASSERT_OK(env_->FileExists(manifest_path)); - CreateFile(env_, manifest_path, "blah"); + CreateFile(env_, manifest_path, "blah", false /* use_fsync */); ASSERT_OK(RepairDB(dbname_, CurrentOptions())); Reopen(CurrentOptions()); @@ -153,7 +153,7 @@ TEST_F(RepairTest, CorruptSst) { Flush(); auto sst_path = GetFirstSstPath(); ASSERT_FALSE(sst_path.empty()); - CreateFile(env_, sst_path, "blah"); + CreateFile(env_, sst_path, "blah", false /* use_fsync */); Close(); ASSERT_OK(RepairDB(dbname_, CurrentOptions())); diff --git a/util/file_util.cc b/util/file_util.cc index 79ba83716..aa2994b1e 100644 --- a/util/file_util.cc +++ b/util/file_util.cc @@ -68,7 +68,7 @@ Status CopyFile(Env* env, const std::string& source, // Utility function to create a file with the provided contents Status CreateFile(Env* env, const std::string& destination, - const std::string& contents) { + const std::string& contents, bool use_fsync) { const EnvOptions soptions; Status s; unique_ptr dest_writer; @@ -80,7 +80,11 @@ Status CreateFile(Env* env, const std::string& destination, } dest_writer.reset( new WritableFileWriter(std::move(destfile), destination, soptions)); - return dest_writer->Append(Slice(contents)); + s = dest_writer->Append(Slice(contents)); + if (!s.ok()) { + return s; + } + return dest_writer->Sync(use_fsync); } Status DeleteSSTFile(const ImmutableDBOptions* db_options, diff --git a/util/file_util.h b/util/file_util.h index 4df597275..5c05c9def 100644 --- a/util/file_util.h +++ b/util/file_util.h @@ -19,7 +19,7 @@ extern Status CopyFile(Env* env, const std::string& source, bool use_fsync); extern Status CreateFile(Env* env, const std::string& destination, - const std::string& contents); + const std::string& contents, bool use_fsync); extern Status DeleteSSTFile(const ImmutableDBOptions* db_options, const std::string& fname, diff --git a/utilities/checkpoint/checkpoint_impl.cc b/utilities/checkpoint/checkpoint_impl.cc index fc8efe883..48f9200fb 100644 --- a/utilities/checkpoint/checkpoint_impl.cc +++ b/utilities/checkpoint/checkpoint_impl.cc @@ -120,7 +120,8 @@ Status CheckpointImpl::CreateCheckpoint(const std::string& checkpoint_dir, } /* copy_file_cb */, [&](const std::string& fname, const std::string& contents, FileType) { ROCKS_LOG_INFO(db_options.info_log, "Creating %s", fname.c_str()); - return CreateFile(db_->GetEnv(), full_private_path + fname, contents); + return CreateFile(db_->GetEnv(), full_private_path + fname, contents, + db_options.use_fsync); } /* create_file_cb */, &sequence_number, log_size_for_flush); // we copied all the files, enable file deletions diff --git a/utilities/checkpoint/checkpoint_test.cc b/utilities/checkpoint/checkpoint_test.cc index b47f240c3..62c78faa8 100644 --- a/utilities/checkpoint/checkpoint_test.cc +++ b/utilities/checkpoint/checkpoint_test.cc @@ -17,12 +17,13 @@ #include #include #include "db/db_impl.h" -#include "port/stack_trace.h" #include "port/port.h" +#include "port/stack_trace.h" #include "rocksdb/db.h" #include "rocksdb/env.h" #include "rocksdb/utilities/checkpoint.h" #include "rocksdb/utilities/transaction_db.h" +#include "util/fault_injection_test_env.h" #include "util/sync_point.h" #include "util/testharness.h" @@ -585,6 +586,32 @@ TEST_F(CheckpointTest, CheckpointWithParallelWrites) { thread.join(); } +TEST_F(CheckpointTest, CheckpointWithUnsyncedDataDropped) { + Options options = CurrentOptions(); + std::unique_ptr env(new FaultInjectionTestEnv(env_)); + options.env = env.get(); + Reopen(options); + ASSERT_OK(Put("key1", "val1")); + Checkpoint* checkpoint; + ASSERT_OK(Checkpoint::Create(db_, &checkpoint)); + ASSERT_OK(checkpoint->CreateCheckpoint(snapshot_name_)); + delete checkpoint; + env->DropUnsyncedFileData(); + + // make sure it's openable even though whatever data that wasn't synced got + // dropped. + options.env = env_; + DB* snapshot_db; + ASSERT_OK(DB::Open(options, snapshot_name_, &snapshot_db)); + ReadOptions read_opts; + std::string get_result; + ASSERT_OK(snapshot_db->Get(read_opts, "key1", &get_result)); + ASSERT_EQ("val1", get_result); + delete snapshot_db; + delete db_; + db_ = nullptr; +} + } // namespace rocksdb int main(int argc, char** argv) {