From e19908cba6c1124c396137c96e34f20413b3f4ff Mon Sep 17 00:00:00 2001 From: sdong Date: Wed, 5 May 2021 15:49:29 -0700 Subject: [PATCH] Refactor kill point (#8241) Summary: Refactor kill point to one single class, rather than several extern variables. The intention was to drop unflushed data before killing to simulate some job, and I tried to a pointer to fault ingestion fs to the killing class, but it ended up with harder than I thought. Perhaps we'll need to do this in another way. But I thought the refactoring itself is good so I send it out. Pull Request resolved: https://github.com/facebook/rocksdb/pull/8241 Test Plan: make release and run crash test for a while. Reviewed By: anand1976 Differential Revision: D28078486 fbshipit-source-id: f9182c1455f52e6851c13f88a21bade63bcec45f --- db/version_set.cc | 4 +-- db_stress_tool/db_stress_gflags.cc | 1 - db_stress_tool/db_stress_test_base.cc | 9 ++++--- db_stress_tool/db_stress_tool.cc | 7 +++-- env/io_posix.cc | 16 +++++------ file/filename.cc | 6 ++--- file/read_write_util.cc | 2 +- file/writable_file_writer.cc | 18 ++++++------- port/win/io_win.cc | 4 +-- test_util/sync_point.cc | 1 - test_util/sync_point.h | 38 ++++++++++++++++----------- test_util/sync_point_impl.cc | 13 ++++++--- 12 files changed, 68 insertions(+), 51 deletions(-) diff --git a/db/version_set.cc b/db/version_set.cc index 39c2b9a89..c9042d727 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -4157,8 +4157,8 @@ Status VersionSet::ProcessManifestWrites( e->DebugString(true)); break; } - TEST_KILL_RANDOM("VersionSet::LogAndApply:BeforeAddRecord", - rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("VersionSet::LogAndApply:BeforeAddRecord", + REDUCE_ODDS2); #ifndef NDEBUG if (batch_edits.size() > 1 && batch_edits.size() - 1 == idx) { TEST_SYNC_POINT_CALLBACK( diff --git a/db_stress_tool/db_stress_gflags.cc b/db_stress_tool/db_stress_gflags.cc index 1c3fbf4fe..df2fc38c2 100644 --- a/db_stress_tool/db_stress_gflags.cc +++ b/db_stress_tool/db_stress_gflags.cc @@ -473,7 +473,6 @@ DEFINE_int32(kill_random_test, 0, "probability 1/this"); static const bool FLAGS_kill_random_test_dummy __attribute__((__unused__)) = RegisterFlagValidator(&FLAGS_kill_random_test, &ValidateInt32Positive); -extern int rocksdb_kill_odds; DEFINE_string(kill_exclude_prefixes, "", "If non-empty, kill points with prefix in the list given will be" diff --git a/db_stress_tool/db_stress_test_base.cc b/db_stress_tool/db_stress_test_base.cc index 302ad2e74..9cc602e8b 100644 --- a/db_stress_tool/db_stress_test_base.cc +++ b/db_stress_tool/db_stress_test_base.cc @@ -2083,13 +2083,16 @@ void StressTest::PrintEnv() const { fprintf(stdout, "Memtablerep : %s\n", memtablerep); - fprintf(stdout, "Test kill odd : %d\n", rocksdb_kill_odds); - if (!rocksdb_kill_exclude_prefixes.empty()) { +#ifndef NDEBUG + KillPoint* kp = KillPoint::GetInstance(); + fprintf(stdout, "Test kill odd : %d\n", kp->rocksdb_kill_odds); + if (!kp->rocksdb_kill_exclude_prefixes.empty()) { fprintf(stdout, "Skipping kill points prefixes:\n"); - for (auto& p : rocksdb_kill_exclude_prefixes) { + for (auto& p : kp->rocksdb_kill_exclude_prefixes) { fprintf(stdout, " %s\n", p.c_str()); } } +#endif fprintf(stdout, "Periodic Compaction Secs : %" PRIu64 "\n", FLAGS_periodic_compaction_seconds); fprintf(stdout, "Compaction TTL : %" PRIu64 "\n", diff --git a/db_stress_tool/db_stress_tool.cc b/db_stress_tool/db_stress_tool.cc index e7c36384f..92597e309 100644 --- a/db_stress_tool/db_stress_tool.cc +++ b/db_stress_tool/db_stress_tool.cc @@ -299,8 +299,11 @@ int db_stress_tool(int argc, char** argv) { exit(1); } - rocksdb_kill_odds = FLAGS_kill_random_test; - rocksdb_kill_exclude_prefixes = SplitString(FLAGS_kill_exclude_prefixes); +#ifndef NDEBUG + KillPoint* kp = KillPoint::GetInstance(); + kp->rocksdb_kill_odds = FLAGS_kill_random_test; + kp->rocksdb_kill_exclude_prefixes = SplitString(FLAGS_kill_exclude_prefixes); +#endif unsigned int levels = FLAGS_max_key_len; std::vector weights; diff --git a/env/io_posix.cc b/env/io_posix.cc index d48f04723..de0a74539 100644 --- a/env/io_posix.cc +++ b/env/io_posix.cc @@ -893,7 +893,7 @@ IOStatus PosixMmapReadableFile::InvalidateCache(size_t offset, size_t length) { * knows enough to skip zero suffixes. */ IOStatus PosixMmapFile::UnmapCurrentRegion() { - TEST_KILL_RANDOM("PosixMmapFile::UnmapCurrentRegion:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::UnmapCurrentRegion:0"); if (base_ != nullptr) { int munmap_status = munmap(base_, limit_ - base_); if (munmap_status != 0) { @@ -916,7 +916,7 @@ IOStatus PosixMmapFile::UnmapCurrentRegion() { IOStatus PosixMmapFile::MapNewRegion() { #ifdef ROCKSDB_FALLOCATE_PRESENT assert(base_ == nullptr); - TEST_KILL_RANDOM("PosixMmapFile::UnmapCurrentRegion:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::UnmapCurrentRegion:0"); // we can't fallocate with FALLOC_FL_KEEP_SIZE here if (allow_fallocate_) { IOSTATS_TIMER_GUARD(allocate_nanos); @@ -931,13 +931,13 @@ IOStatus PosixMmapFile::MapNewRegion() { } } - TEST_KILL_RANDOM("PosixMmapFile::Append:1", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::Append:1"); void* ptr = mmap(nullptr, map_size_, PROT_READ | PROT_WRITE, MAP_SHARED, fd_, file_offset_); if (ptr == MAP_FAILED) { return IOStatus::IOError("MMap failed on " + filename_); } - TEST_KILL_RANDOM("PosixMmapFile::Append:2", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::Append:2"); base_ = reinterpret_cast(ptr); limit_ = base_ + map_size_; @@ -958,7 +958,7 @@ IOStatus PosixMmapFile::Msync() { size_t p1 = TruncateToPageBoundary(last_sync_ - base_); size_t p2 = TruncateToPageBoundary(dst_ - base_ - 1); last_sync_ = dst_; - TEST_KILL_RANDOM("PosixMmapFile::Msync:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::Msync:0"); if (msync(base_ + p1, p2 - p1 + page_size_, MS_SYNC) < 0) { return IOError("While msync", filename_, errno); } @@ -1011,7 +1011,7 @@ IOStatus PosixMmapFile::Append(const Slice& data, const IOOptions& /*opts*/, if (!s.ok()) { return s; } - TEST_KILL_RANDOM("PosixMmapFile::Append:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::Append:0"); } size_t n = (left <= avail) ? left : avail; @@ -1109,7 +1109,7 @@ IOStatus PosixMmapFile::Allocate(uint64_t offset, uint64_t len, IODebugContext* /*dbg*/) { assert(offset <= static_cast(std::numeric_limits::max())); assert(len <= static_cast(std::numeric_limits::max())); - TEST_KILL_RANDOM("PosixMmapFile::Allocate:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixMmapFile::Allocate:0"); int alloc_status = 0; if (allow_fallocate_) { alloc_status = @@ -1333,7 +1333,7 @@ IOStatus PosixWritableFile::Allocate(uint64_t offset, uint64_t len, IODebugContext* /*dbg*/) { assert(offset <= static_cast(std::numeric_limits::max())); assert(len <= static_cast(std::numeric_limits::max())); - TEST_KILL_RANDOM("PosixWritableFile::Allocate:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("PosixWritableFile::Allocate:0"); IOSTATS_TIMER_GUARD(allocate_nanos); int alloc_status = 0; if (allow_fallocate_) { diff --git a/file/filename.cc b/file/filename.cc index e821a5a0b..87bf060d1 100644 --- a/file/filename.cc +++ b/file/filename.cc @@ -385,9 +385,9 @@ IOStatus SetCurrentFile(FileSystem* fs, const std::string& dbname, IOStatus s = WriteStringToFile(fs, contents.ToString() + "\n", tmp, true); TEST_SYNC_POINT_CALLBACK("SetCurrentFile:BeforeRename", &s); if (s.ok()) { - TEST_KILL_RANDOM("SetCurrentFile:0", rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("SetCurrentFile:0", REDUCE_ODDS2); s = fs->RenameFile(tmp, CurrentFileName(dbname), IOOptions(), nullptr); - TEST_KILL_RANDOM("SetCurrentFile:1", rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("SetCurrentFile:1", REDUCE_ODDS2); TEST_SYNC_POINT_CALLBACK("SetCurrentFile:AfterRename", &s); } if (s.ok()) { @@ -423,7 +423,7 @@ Status SetIdentityFile(Env* env, const std::string& dbname, IOStatus SyncManifest(const ImmutableDBOptions* db_options, WritableFileWriter* file) { - TEST_KILL_RANDOM("SyncManifest:0", rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("SyncManifest:0", REDUCE_ODDS2); StopWatch sw(db_options->clock, db_options->stats, MANIFEST_FILE_SYNC_MICROS); return file->Sync(db_options->use_fsync); } diff --git a/file/read_write_util.cc b/file/read_write_util.cc index 6b9379c42..9df6c5a39 100644 --- a/file/read_write_util.cc +++ b/file/read_write_util.cc @@ -18,7 +18,7 @@ IOStatus NewWritableFile(FileSystem* fs, const std::string& fname, std::unique_ptr* result, const FileOptions& options) { IOStatus s = fs->NewWritableFile(fname, options, result, nullptr); - TEST_KILL_RANDOM("NewWritableFile:0", rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("NewWritableFile:0", REDUCE_ODDS2); return s; } diff --git a/file/writable_file_writer.cc b/file/writable_file_writer.cc index 495d6b889..d009542c7 100644 --- a/file/writable_file_writer.cc +++ b/file/writable_file_writer.cc @@ -42,8 +42,7 @@ IOStatus WritableFileWriter::Append(const Slice& data) { IOStatus s; pending_sync_ = true; - TEST_KILL_RANDOM("WritableFileWriter::Append:0", - rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("WritableFileWriter::Append:0", REDUCE_ODDS2); // Calculate the checksum of appended data UpdateFileChecksum(data); @@ -104,7 +103,7 @@ IOStatus WritableFileWriter::Append(const Slice& data) { s = WriteBuffered(src, left); } - TEST_KILL_RANDOM("WritableFileWriter::Append:1", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::Append:1"); if (s.ok()) { filesize_ += data.size(); } @@ -192,7 +191,7 @@ IOStatus WritableFileWriter::Close() { } } - TEST_KILL_RANDOM("WritableFileWriter::Close:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::Close:0"); { #ifndef ROCKSDB_LITE FileOperationInfo::StartTimePoint start_ts; @@ -213,7 +212,7 @@ IOStatus WritableFileWriter::Close() { } writable_file_.reset(); - TEST_KILL_RANDOM("WritableFileWriter::Close:1", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::Close:1"); if (s.ok() && checksum_generator_ != nullptr && !checksum_finalized_) { checksum_generator_->Finalize(); @@ -227,8 +226,7 @@ IOStatus WritableFileWriter::Close() { // enabled IOStatus WritableFileWriter::Flush() { IOStatus s; - TEST_KILL_RANDOM("WritableFileWriter::Flush:0", - rocksdb_kill_odds * REDUCE_ODDS2); + TEST_KILL_RANDOM_WITH_WEIGHT("WritableFileWriter::Flush:0", REDUCE_ODDS2); if (buf_.CurrentSize() > 0) { if (use_direct_io()) { @@ -317,14 +315,14 @@ IOStatus WritableFileWriter::Sync(bool use_fsync) { if (!s.ok()) { return s; } - TEST_KILL_RANDOM("WritableFileWriter::Sync:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::Sync:0"); if (!use_direct_io() && pending_sync_) { s = SyncInternal(use_fsync); if (!s.ok()) { return s; } } - TEST_KILL_RANDOM("WritableFileWriter::Sync:1", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::Sync:1"); pending_sync_ = false; return IOStatus::OK(); } @@ -447,7 +445,7 @@ IOStatus WritableFileWriter::WriteBuffered(const char* data, size_t size) { } IOSTATS_ADD(bytes_written, allowed); - TEST_KILL_RANDOM("WritableFileWriter::WriteBuffered:0", rocksdb_kill_odds); + TEST_KILL_RANDOM("WritableFileWriter::WriteBuffered:0"); left -= allowed; src += allowed; diff --git a/port/win/io_win.cc b/port/win/io_win.cc index 2ec7710f4..1e662c06d 100644 --- a/port/win/io_win.cc +++ b/port/win/io_win.cc @@ -539,7 +539,7 @@ IOStatus WinMmapFile::Allocate(uint64_t offset, uint64_t len, const IOOptions& /*options*/, IODebugContext* /*dbg*/) { IOStatus status; - TEST_KILL_RANDOM("WinMmapFile::Allocate", rocksdb_kill_odds); + TEST_KILL_RANDOM("WinMmapFile::Allocate"); // Make sure that we reserve an aligned amount of space // since the reservation block size is driven outside so we want @@ -889,7 +889,7 @@ inline IOStatus WinWritableImpl::SyncImpl(const IOOptions& /*options*/, inline IOStatus WinWritableImpl::AllocateImpl(uint64_t offset, uint64_t len) { IOStatus status; - TEST_KILL_RANDOM("WinWritableFile::Allocate", rocksdb_kill_odds); + TEST_KILL_RANDOM("WinWritableFile::Allocate"); // Make sure that we reserve an aligned amount of space // since the reservation block size is driven outside so we want diff --git a/test_util/sync_point.cc b/test_util/sync_point.cc index c8dda57f5..4c71fc6bc 100644 --- a/test_util/sync_point.cc +++ b/test_util/sync_point.cc @@ -9,7 +9,6 @@ #include "test_util/sync_point_impl.h" -int rocksdb_kill_odds = 0; std::vector rocksdb_kill_exclude_prefixes; #ifndef NDEBUG diff --git a/test_util/sync_point.h b/test_util/sync_point.h index 08d6c037a..775fd5c36 100644 --- a/test_util/sync_point.h +++ b/test_util/sync_point.h @@ -13,34 +13,42 @@ #include "rocksdb/rocksdb_namespace.h" -// This is only set from db_stress.cc and for testing only. -// If non-zero, kill at various points in source code with probability 1/this -extern int rocksdb_kill_odds; -// If kill point has a prefix on this list, will skip killing. -extern std::vector rocksdb_kill_exclude_prefixes; - #ifdef NDEBUG // empty in release build -#define TEST_KILL_RANDOM(kill_point, rocksdb_kill_odds) +#define TEST_KILL_RANDOM_WITH_WEIGHT(kill_point, rocksdb_kill_odds_weight) +#define TEST_KILL_RANDOM(kill_point) #else namespace ROCKSDB_NAMESPACE { -// Kill the process with probability 1/odds for testing. -extern void TestKillRandom(std::string kill_point, int odds, - const std::string& srcfile, int srcline); // To avoid crashing always at some frequently executed codepaths (during // kill random test), use this factor to reduce odds #define REDUCE_ODDS 2 #define REDUCE_ODDS2 4 -#define TEST_KILL_RANDOM(kill_point, rocksdb_kill_odds) \ - { \ - if (rocksdb_kill_odds > 0) { \ - TestKillRandom(kill_point, rocksdb_kill_odds, __FILE__, __LINE__); \ - } \ +// A class used to pass when a kill point is reached. +struct KillPoint { + public: + // This is only set from db_stress.cc and for testing only. + // If non-zero, kill at various points in source code with probability 1/this + int rocksdb_kill_odds = 0; + // If kill point has a prefix on this list, will skip killing. + std::vector rocksdb_kill_exclude_prefixes; + // Kill the process with probability 1/odds for testing. + void TestKillRandom(std::string kill_point, int odds, + const std::string& srcfile, int srcline); + + static KillPoint* GetInstance(); +}; + +#define TEST_KILL_RANDOM_WITH_WEIGHT(kill_point, rocksdb_kill_odds_weight) \ + { \ + KillPoint::GetInstance()->TestKillRandom( \ + kill_point, rocksdb_kill_odds_weight, __FILE__, __LINE__); \ } +#define TEST_KILL_RANDOM(kill_point) TEST_KILL_RANDOM_WITH_WEIGHT(kill_point, 1) } // namespace ROCKSDB_NAMESPACE + #endif #ifdef NDEBUG diff --git a/test_util/sync_point_impl.cc b/test_util/sync_point_impl.cc index e1877e398..28405a0ef 100644 --- a/test_util/sync_point_impl.cc +++ b/test_util/sync_point_impl.cc @@ -7,9 +7,17 @@ #ifndef NDEBUG namespace ROCKSDB_NAMESPACE { +KillPoint* KillPoint::GetInstance() { + static KillPoint kp; + return &kp; +} -void TestKillRandom(std::string kill_point, int odds, - const std::string& srcfile, int srcline) { +void KillPoint::TestKillRandom(std::string kill_point, int odds_weight, + const std::string& srcfile, int srcline) { + if (rocksdb_kill_odds <= 0) { + return; + } + int odds = rocksdb_kill_odds * odds_weight; for (auto& p : rocksdb_kill_exclude_prefixes) { if (kill_point.substr(0, p.length()) == p) { return; @@ -29,7 +37,6 @@ void TestKillRandom(std::string kill_point, int odds, } } - void SyncPoint::Data::LoadDependency(const std::vector& dependencies) { std::lock_guard lock(mutex_); successors_.clear();