From 1f7351357668cdf6868114d65478bbb2e2e2bed1 Mon Sep 17 00:00:00 2001 From: Yanqin Jin Date: Wed, 4 Nov 2020 11:35:15 -0800 Subject: [PATCH] Remove DB::VerifyFileChecksums --- db/corruption_test.cc | 21 ++++++++++++++++-- db/db_basic_test.cc | 20 ++++++++++++++--- db/db_impl/db_impl.cc | 4 ++-- db/db_impl/db_impl.h | 9 ++++++-- db/db_impl/db_impl_open.cc | 28 ++++++++++++++++++++++++ db/db_impl/db_impl_readonly.cc | 3 +++ db/db_test_util.cc | 4 ++++ db/db_test_util.h | 2 ++ include/rocksdb/db.h | 6 ----- include/rocksdb/utilities/stackable_db.h | 5 ----- 10 files changed, 82 insertions(+), 20 deletions(-) diff --git a/db/corruption_test.cc b/db/corruption_test.cc index 19cdb7c35..964ff1170 100644 --- a/db/corruption_test.cc +++ b/db/corruption_test.cc @@ -840,13 +840,15 @@ TEST_F(CorruptionTest, VerifyWholeTableChecksum) { Build(10, 5); - ASSERT_OK(db_->VerifyFileChecksums(ReadOptions())); + auto* dbi = static_cast_with_check(db_); + ASSERT_OK(dbi->VerifyFileChecksums(ReadOptions())); CloseDb(); // Corrupt the first byte of each table file, this must be data block. Corrupt(kTableFile, 0, 1); ASSERT_OK(TryReopen(&options)); + dbi = static_cast_with_check(db_); SyncPoint::GetInstance()->DisableProcessing(); SyncPoint::GetInstance()->ClearAllCallBacks(); @@ -859,8 +861,23 @@ TEST_F(CorruptionTest, VerifyWholeTableChecksum) { ASSERT_NOK(*s); }); SyncPoint::GetInstance()->EnableProcessing(); - ASSERT_TRUE(db_->VerifyFileChecksums(ReadOptions()).IsCorruption()); + ASSERT_TRUE(dbi->VerifyFileChecksums(ReadOptions()).IsCorruption()); ASSERT_EQ(1, count); + + CloseDb(); + ASSERT_OK(DestroyDB(dbname_, options)); + Reopen(&options); + Build(10, 5); + dbi = static_cast_with_check(db_); + ASSERT_OK(dbi->VerifyFileChecksums(ReadOptions())); + CloseDb(); + Corrupt(kTableFile, 0, 1); + + // Set best_efforts_recovery to true + options.best_efforts_recovery = true; +#ifdef OS_LINUX + ASSERT_TRUE(TryReopen(&options).IsCorruption()); +#endif // OS_LINUX } } // namespace ROCKSDB_NAMESPACE diff --git a/db/db_basic_test.cc b/db/db_basic_test.cc index 6f22429c5..47fbf0619 100644 --- a/db/db_basic_test.cc +++ b/db/db_basic_test.cc @@ -2201,6 +2201,8 @@ TEST_F(DBBasicTest, MultiGetIOBufferOverrun) { TEST_F(DBBasicTest, IncrementalRecoveryNoCorrupt) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); DestroyAndReopen(options); CreateAndReopenWithCF({"pikachu", "eevee"}, options); size_t num_cfs = handles_.size(); @@ -2239,6 +2241,8 @@ TEST_F(DBBasicTest, IncrementalRecoveryNoCorrupt) { TEST_F(DBBasicTest, BestEffortsRecoveryWithVersionBuildingFailure) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); DestroyAndReopen(options); ASSERT_OK(Put("foo", "value")); ASSERT_OK(Flush()); @@ -2285,6 +2289,8 @@ TEST_F(DBBasicTest, RecoverWithMissingFiles) { // Disable auto compaction to simplify SST file name tracking. options.disable_auto_compactions = true; options.listeners.emplace_back(listener); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); CreateAndReopenWithCF({"pikachu", "eevee"}, options); std::vector all_cf_names = {kDefaultColumnFamilyName, "pikachu", "eevee"}; @@ -2345,6 +2351,8 @@ TEST_F(DBBasicTest, RecoverWithMissingFiles) { TEST_F(DBBasicTest, BestEffortsRecoveryTryMultipleManifests) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); options.env = env_; DestroyAndReopen(options); ASSERT_OK(Put("foo", "value0")); @@ -2371,6 +2379,8 @@ TEST_F(DBBasicTest, BestEffortsRecoveryTryMultipleManifests) { TEST_F(DBBasicTest, RecoverWithNoCurrentFile) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); options.env = env_; DestroyAndReopen(options); CreateAndReopenWithCF({"pikachu"}, options); @@ -2394,6 +2404,8 @@ TEST_F(DBBasicTest, RecoverWithNoCurrentFile) { TEST_F(DBBasicTest, RecoverWithNoManifest) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); options.env = env_; DestroyAndReopen(options); ASSERT_OK(Put("foo", "value")); @@ -2423,6 +2435,8 @@ TEST_F(DBBasicTest, RecoverWithNoManifest) { TEST_F(DBBasicTest, SkipWALIfMissingTableFiles) { Options options = CurrentOptions(); + options.file_checksum_gen_factory = + ROCKSDB_NAMESPACE::GetFileChecksumGenCrc32cFactory(); DestroyAndReopen(options); TableFileListener* listener = new TableFileListener(); options.listeners.emplace_back(listener); @@ -3311,17 +3325,17 @@ TEST_F(DBBasicTest, VerifyFileChecksums) { DestroyAndReopen(options); ASSERT_OK(Put("a", "value")); ASSERT_OK(Flush()); - ASSERT_TRUE(db_->VerifyFileChecksums(ReadOptions()).IsInvalidArgument()); + ASSERT_TRUE(dbfull()->VerifyFileChecksums(ReadOptions()).IsInvalidArgument()); options.file_checksum_gen_factory = GetFileChecksumGenCrc32cFactory(); Reopen(options); - ASSERT_OK(db_->VerifyFileChecksums(ReadOptions())); + ASSERT_OK(dbfull()->VerifyFileChecksums(ReadOptions())); // Write an L0 with checksum computed. ASSERT_OK(Put("b", "value")); ASSERT_OK(Flush()); - ASSERT_OK(db_->VerifyFileChecksums(ReadOptions())); + ASSERT_OK(dbfull()->VerifyFileChecksums(ReadOptions())); } #endif // !ROCKSDB_LITE diff --git a/db/db_impl/db_impl.cc b/db/db_impl/db_impl.cc index ee7a7afcc..755f64e4e 100644 --- a/db/db_impl/db_impl.cc +++ b/db/db_impl/db_impl.cc @@ -4778,11 +4778,11 @@ Status DBImpl::VerifyChecksumInternal(const ReadOptions& read_options, j++) { const auto& fd_with_krange = vstorage->LevelFilesBrief(i).files[j]; const auto& fd = fd_with_krange.fd; - const FileMetaData* fmeta = fd_with_krange.file_metadata; - assert(fmeta); std::string fname = TableFileName(cfd->ioptions()->cf_paths, fd.GetNumber(), fd.GetPathId()); if (use_file_checksum) { + const FileMetaData* fmeta = fd_with_krange.file_metadata; + assert(fmeta); s = VerifySstFileChecksum(*fmeta, fname, read_options); } else { s = ROCKSDB_NAMESPACE::VerifySstFileChecksum(opts, file_options_, diff --git a/db/db_impl/db_impl.h b/db/db_impl/db_impl.h index 68b053f31..595152760 100644 --- a/db/db_impl/db_impl.h +++ b/db/db_impl/db_impl.h @@ -373,6 +373,12 @@ class DBImpl : public DB { uint64_t start_time, uint64_t end_time, std::unique_ptr* stats_iterator) override; + // If immutable_db_options_.best_efforts_recovery is true, and + // RocksDbFileChecksumsVerificationEnabledOnRecovery is defined and returns + // true, and immutable_db_options_.file_checksum_gen_factory is not nullptr, + // then call VerifyFileChecksums(). + Status MaybeVerifyFileChecksums(); + #ifndef ROCKSDB_LITE using DB::ResetStats; virtual Status ResetStats() override; @@ -431,8 +437,7 @@ class DBImpl : public DB { const ExportImportFilesMetaData& metadata, ColumnFamilyHandle** handle) override; - using DB::VerifyFileChecksums; - Status VerifyFileChecksums(const ReadOptions& read_options) override; + Status VerifyFileChecksums(const ReadOptions& read_options); using DB::VerifyChecksum; virtual Status VerifyChecksum(const ReadOptions& /*read_options*/) override; diff --git a/db/db_impl/db_impl_open.cc b/db/db_impl/db_impl_open.cc index ed37632d1..71f13bacf 100644 --- a/db/db_impl/db_impl_open.cc +++ b/db/db_impl/db_impl_open.cc @@ -23,6 +23,15 @@ #include "test_util/sync_point.h" #include "util/rate_limiter.h" +#if !defined(ROCKSDB_LITE) && defined(OS_LINUX) +// VerifyFileChecksums is a weak symbol. +// If it is defined and returns true, and options.best_efforts_recovery = true, +// and file checksum is enabled, then the checksums of table files will be +// computed and verified with MANIFEST. +extern "C" bool RocksDbFileChecksumsVerificationEnabledOnRecovery() + __attribute__((__weak__)); +#endif // !ROCKSDB_LITE && OS_LINUX + namespace ROCKSDB_NAMESPACE { Options SanitizeOptions(const std::string& dbname, const Options& src) { auto db_options = SanitizeOptions(dbname, DBOptions(src)); @@ -1404,6 +1413,22 @@ Status DBImpl::WriteLevel0TableForRecovery(int job_id, ColumnFamilyData* cfd, return s; } +Status DBImpl::MaybeVerifyFileChecksums() { + Status s; +#if !defined(ROCKSDB_LITE) && defined(OS_LINUX) + // TODO: remove the VerifyFileChecksums() call because it's very expensive. + if (immutable_db_options_.best_efforts_recovery && + RocksDbFileChecksumsVerificationEnabledOnRecovery && + RocksDbFileChecksumsVerificationEnabledOnRecovery() && + immutable_db_options_.file_checksum_gen_factory) { + s = VerifyFileChecksums(ReadOptions()); + ROCKS_LOG_INFO(immutable_db_options_.info_log, + "Verified file checksums: %s\n", s.ToString().c_str()); + } +#endif // !ROCKSDB_LITE && OS_LINUX + return s; +} + Status DB::Open(const Options& options, const std::string& dbname, DB** dbptr) { DBOptions db_options(options); ColumnFamilyOptions cf_options(options); @@ -1779,6 +1804,9 @@ Status DBImpl::Open(const DBOptions& db_options, const std::string& dbname, "Persisting Option File error: %s", persist_options_status.ToString().c_str()); } + if (s.ok()) { + s = impl->MaybeVerifyFileChecksums(); + } if (s.ok()) { impl->StartPeriodicWorkScheduler(); } else { diff --git a/db/db_impl/db_impl_readonly.cc b/db/db_impl/db_impl_readonly.cc index 84d03863d..57825afbb 100644 --- a/db/db_impl/db_impl_readonly.cc +++ b/db/db_impl/db_impl_readonly.cc @@ -233,6 +233,9 @@ Status DBImplReadOnly::OpenForReadOnlyWithoutCheck( } impl->mutex_.Unlock(); sv_context.Clean(); + if (s.ok()) { + s = impl->MaybeVerifyFileChecksums(); + } if (s.ok()) { *dbptr = impl; for (auto* h : *handles) { diff --git a/db/db_test_util.cc b/db/db_test_util.cc index ccfd1c8f9..3c234e92b 100644 --- a/db/db_test_util.cc +++ b/db/db_test_util.cc @@ -15,6 +15,10 @@ #include "rocksdb/utilities/object_registry.h" #include "util/random.h" +extern "C" bool RocksDbFileChecksumsVerificationEnabledOnRecovery() { + return true; +} + namespace ROCKSDB_NAMESPACE { namespace { diff --git a/db/db_test_util.h b/db/db_test_util.h index 7a2fbfc70..2ef7cf406 100644 --- a/db/db_test_util.h +++ b/db/db_test_util.h @@ -49,6 +49,8 @@ #include "util/string_util.h" #include "utilities/merge_operators.h" +extern "C" bool RocksDbFileChecksumsVerificationEnabledOnRecovery(); + namespace ROCKSDB_NAMESPACE { namespace anon { diff --git a/include/rocksdb/db.h b/include/rocksdb/db.h index 1e4a178a3..6eab3f3e9 100644 --- a/include/rocksdb/db.h +++ b/include/rocksdb/db.h @@ -1443,12 +1443,6 @@ class DB { const ExportImportFilesMetaData& metadata, ColumnFamilyHandle** handle) = 0; - // Verify the checksums of files in db. Currently the whole-file checksum of - // table files are checked. - virtual Status VerifyFileChecksums(const ReadOptions& /*read_options*/) { - return Status::NotSupported("File verification not supported"); - } - // Verify the block checksums of files in db. The block checksums of table // files are checked. virtual Status VerifyChecksum(const ReadOptions& read_options) = 0; diff --git a/include/rocksdb/utilities/stackable_db.h b/include/rocksdb/utilities/stackable_db.h index 8f920b5d4..93c9e9a67 100644 --- a/include/rocksdb/utilities/stackable_db.h +++ b/include/rocksdb/utilities/stackable_db.h @@ -141,11 +141,6 @@ class StackableDB : public DB { import_options, metadata, handle); } - using DB::VerifyFileChecksums; - Status VerifyFileChecksums(const ReadOptions& read_opts) override { - return db_->VerifyFileChecksums(read_opts); - } - virtual Status VerifyChecksum() override { return db_->VerifyChecksum(); } virtual Status VerifyChecksum(const ReadOptions& options) override {