From 397ab11152e292c52addb24cc6751dd6d6cfdc6e Mon Sep 17 00:00:00 2001 From: Mike Kolupaev Date: Wed, 28 Jun 2017 21:26:03 -0700 Subject: [PATCH] Improve Status message for block checksum mismatches Summary: We've got some DBs where iterators return Status with message "Corruption: block checksum mismatch" all the time. That's not very informative. It would be much easier to investigate if the error message contained the file name - then we would know e.g. how old the corrupted file is, which would be very useful for finding the root cause. This PR adds file name, offset and other stuff to some block corruption-related status messages. It doesn't improve all the error messages, just a few that were easy to improve. I'm mostly interested in "block checksum mismatch" and "Bad table magic number" since they're the only corruption errors that I've ever seen in the wild. Closes https://github.com/facebook/rocksdb/pull/2507 Differential Revision: D5345702 Pulled By: al13n321 fbshipit-source-id: fc8023d43f1935ad927cef1b9c55481ab3cb1339 --- db/external_sst_file_ingestion_job.cc | 3 +- db/table_cache.cc | 2 +- db/version_set.cc | 14 ++++----- table/cuckoo_table_builder_test.cc | 8 ++--- table/cuckoo_table_reader_test.cc | 10 +++--- table/format.cc | 31 +++++++++++++++---- table/table_reader_bench.cc | 2 +- tools/sst_dump_tool.cc | 4 +-- util/file_reader_writer.h | 5 +++ util/testutil.cc | 3 +- utilities/blob_db/blob_dump_tool.cc | 2 +- utilities/blob_db/blob_file.cc | 3 +- utilities/column_aware_encoding_util.cc | 2 +- .../persistent_cache/block_cache_tier_file.cc | 2 +- 14 files changed, 59 insertions(+), 32 deletions(-) diff --git a/db/external_sst_file_ingestion_job.cc b/db/external_sst_file_ingestion_job.cc index 2c94d07d3a..102ecbc05e 100644 --- a/db/external_sst_file_ingestion_job.cc +++ b/db/external_sst_file_ingestion_job.cc @@ -286,7 +286,8 @@ Status ExternalSstFileIngestionJob::GetIngestedFileInfo( if (!status.ok()) { return status; } - sst_file_reader.reset(new RandomAccessFileReader(std::move(sst_file))); + sst_file_reader.reset(new RandomAccessFileReader(std::move(sst_file), + external_file)); status = cfd_->ioptions()->table_factory->NewTableReader( TableReaderOptions(*cfd_->ioptions(), env_options_, diff --git a/db/table_cache.cc b/db/table_cache.cc index ad8282cc1b..8aaaf97559 100644 --- a/db/table_cache.cc +++ b/db/table_cache.cc @@ -108,7 +108,7 @@ Status TableCache::GetTableReader( } StopWatch sw(ioptions_.env, ioptions_.statistics, TABLE_OPEN_IO_MICROS); std::unique_ptr file_reader( - new RandomAccessFileReader(std::move(file), ioptions_.env, + new RandomAccessFileReader(std::move(file), fname, ioptions_.env, ioptions_.statistics, record_read_stats, file_read_hist, ioptions_.rate_limiter, for_compaction)); diff --git a/db/version_set.cc b/db/version_set.cc index bd30a35506..849c3cc630 100644 --- a/db/version_set.cc +++ b/db/version_set.cc @@ -598,15 +598,15 @@ Status Version::GetTableProperties(std::shared_ptr* tp, // 2. Table is not present in table cache, we'll read the table properties // directly from the properties block in the file. std::unique_ptr file; + std::string file_name; if (fname != nullptr) { - s = ioptions->env->NewRandomAccessFile( - *fname, &file, vset_->env_options_); + file_name = *fname; } else { - s = ioptions->env->NewRandomAccessFile( - TableFileName(vset_->db_options_->db_paths, file_meta->fd.GetNumber(), - file_meta->fd.GetPathId()), - &file, vset_->env_options_); + file_name = + TableFileName(vset_->db_options_->db_paths, file_meta->fd.GetNumber(), + file_meta->fd.GetPathId()); } + s = ioptions->env->NewRandomAccessFile(file_name, &file, vset_->env_options_); if (!s.ok()) { return s; } @@ -615,7 +615,7 @@ Status Version::GetTableProperties(std::shared_ptr* tp, // By setting the magic number to kInvalidTableMagicNumber, we can by // pass the magic number check in the footer. std::unique_ptr file_reader( - new RandomAccessFileReader(std::move(file))); + new RandomAccessFileReader(std::move(file), file_name)); s = ReadTableProperties( file_reader.get(), file_meta->fd.GetFileSize(), Footer::kInvalidTableMagicNumber /* table's magic number */, *ioptions, &raw_table_properties); diff --git a/table/cuckoo_table_builder_test.cc b/table/cuckoo_table_builder_test.cc index 2750055931..ad0d657022 100644 --- a/table/cuckoo_table_builder_test.cc +++ b/table/cuckoo_table_builder_test.cc @@ -49,14 +49,14 @@ class CuckooBuilderTest : public testing::Test { uint64_t read_file_size; ASSERT_OK(env_->GetFileSize(fname, &read_file_size)); - Options options; - options.allow_mmap_reads = true; - ImmutableCFOptions ioptions(options); + Options options; + options.allow_mmap_reads = true; + ImmutableCFOptions ioptions(options); // Assert Table Properties. TableProperties* props = nullptr; unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); ASSERT_OK(ReadTableProperties(file_reader.get(), read_file_size, kCuckooTableMagicNumber, ioptions, &props)); diff --git a/table/cuckoo_table_reader_test.cc b/table/cuckoo_table_reader_test.cc index 32848131d9..e01417ea98 100644 --- a/table/cuckoo_table_reader_test.cc +++ b/table/cuckoo_table_reader_test.cc @@ -116,7 +116,7 @@ class CuckooReaderTest : public testing::Test { std::unique_ptr read_file; ASSERT_OK(env->NewRandomAccessFile(fname, &read_file, env_options)); unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); const ImmutableCFOptions ioptions(options); CuckooTableReader reader(ioptions, std::move(file_reader), file_size, ucomp, GetSliceHash); @@ -144,7 +144,7 @@ class CuckooReaderTest : public testing::Test { std::unique_ptr read_file; ASSERT_OK(env->NewRandomAccessFile(fname, &read_file, env_options)); unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); const ImmutableCFOptions ioptions(options); CuckooTableReader reader(ioptions, std::move(file_reader), file_size, ucomp, GetSliceHash); @@ -322,7 +322,7 @@ TEST_F(CuckooReaderTest, WhenKeyNotFound) { std::unique_ptr read_file; ASSERT_OK(env->NewRandomAccessFile(fname, &read_file, env_options)); unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); const ImmutableCFOptions ioptions(options); CuckooTableReader reader(ioptions, std::move(file_reader), file_size, ucmp, GetSliceHash); @@ -428,7 +428,7 @@ void WriteFile(const std::vector& keys, std::unique_ptr read_file; ASSERT_OK(env->NewRandomAccessFile(fname, &read_file, env_options)); unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); const ImmutableCFOptions ioptions(options); CuckooTableReader reader(ioptions, std::move(file_reader), file_size, @@ -460,7 +460,7 @@ void ReadKeys(uint64_t num, uint32_t batch_size) { std::unique_ptr read_file; ASSERT_OK(env->NewRandomAccessFile(fname, &read_file, env_options)); unique_ptr file_reader( - new RandomAccessFileReader(std::move(read_file))); + new RandomAccessFileReader(std::move(read_file), fname)); const ImmutableCFOptions ioptions(options); CuckooTableReader reader(ioptions, std::move(file_reader), file_size, diff --git a/table/format.cc b/table/format.cc index c51a117c7e..1ae45f1855 100644 --- a/table/format.cc +++ b/table/format.cc @@ -221,7 +221,9 @@ std::string Footer::ToString() const { Status ReadFooterFromFile(RandomAccessFileReader* file, uint64_t file_size, Footer* footer, uint64_t enforce_table_magic_number) { if (file_size < Footer::kMinEncodedLength) { - return Status::Corruption("file is too short to be an sstable"); + return Status::Corruption( + "file is too short (" + ToString(file_size) + " bytes) to be an " + "sstable: " + file->file_name()); } char footer_space[Footer::kMaxEncodedLength]; @@ -237,7 +239,9 @@ Status ReadFooterFromFile(RandomAccessFileReader* file, uint64_t file_size, // Check that we actually read the whole footer from the file. It may be // that size isn't correct. if (footer_input.size() < Footer::kMinEncodedLength) { - return Status::Corruption("file is too short to be an sstable"); + return Status::Corruption( + "file is too short (" + ToString(file_size) + " bytes) to be an " + "sstable" + file->file_name()); } s = footer->DecodeFrom(&footer_input); @@ -246,7 +250,11 @@ Status ReadFooterFromFile(RandomAccessFileReader* file, uint64_t file_size, } if (enforce_table_magic_number != 0 && enforce_table_magic_number != footer->table_magic_number()) { - return Status::Corruption("Bad table magic number"); + return Status::Corruption( + "Bad table magic number: expected " + + ToString(enforce_table_magic_number) + ", found " + + ToString(footer->table_magic_number()) + + " in " + file->file_name()); } return Status::OK(); } @@ -275,7 +283,11 @@ Status ReadBlock(RandomAccessFileReader* file, const Footer& footer, return s; } if (contents->size() != n + kBlockTrailerSize) { - return Status::Corruption("truncated block read"); + return Status::Corruption( + "truncated block read from " + file->file_name() + " offset " + + ToString(handle.offset()) + ", expected " + + ToString(n + kBlockTrailerSize) + " bytes, got " + + ToString(contents->size())); } // Check the crc of the type and the block contents @@ -293,10 +305,17 @@ Status ReadBlock(RandomAccessFileReader* file, const Footer& footer, actual = XXH32(data, static_cast(n) + 1, 0); break; default: - s = Status::Corruption("unknown checksum type"); + s = Status::Corruption( + "unknown checksum type " + ToString(footer.checksum()) + + " in " + file->file_name() + " offset " + + ToString(handle.offset()) + " size " + ToString(n)); } if (s.ok() && actual != value) { - s = Status::Corruption("block checksum mismatch"); + s = Status::Corruption( + "block checksum mismatch: expected " + ToString(actual) + + ", got " + ToString(value) + " in " + file->file_name() + + " offset " + ToString(handle.offset()) + + " size " + ToString(n)); } if (!s.ok()) { return s; diff --git a/table/table_reader_bench.cc b/table/table_reader_bench.cc index 340c741624..ff2ce43fae 100644 --- a/table/table_reader_bench.cc +++ b/table/table_reader_bench.cc @@ -139,7 +139,7 @@ void TableReaderBenchmark(Options& opts, EnvOptions& env_options, uint64_t file_size; env->GetFileSize(file_name, &file_size); unique_ptr file_reader( - new RandomAccessFileReader(std::move(raf))); + new RandomAccessFileReader(std::move(raf), file_name)); s = opts.table_factory->NewTableReader( TableReaderOptions(ioptions, env_options, ikc), std::move(file_reader), file_size, &table_reader); diff --git a/tools/sst_dump_tool.cc b/tools/sst_dump_tool.cc index dcbdf33e47..000167a777 100644 --- a/tools/sst_dump_tool.cc +++ b/tools/sst_dump_tool.cc @@ -79,7 +79,7 @@ Status SstFileReader::GetTableReader(const std::string& file_path) { s = options_.env->GetFileSize(file_path, &file_size); } - file_.reset(new RandomAccessFileReader(std::move(file))); + file_.reset(new RandomAccessFileReader(std::move(file), file_path)); if (s.ok()) { s = ReadFooterFromFile(file_.get(), file_size, &footer); @@ -93,7 +93,7 @@ Status SstFileReader::GetTableReader(const std::string& file_path) { magic_number == kLegacyPlainTableMagicNumber) { soptions_.use_mmap_reads = true; options_.env->NewRandomAccessFile(file_path, &file, soptions_); - file_.reset(new RandomAccessFileReader(std::move(file))); + file_.reset(new RandomAccessFileReader(std::move(file), file_path)); } options_.comparator = &internal_comparator_; // For old sst format, ReadTableProperties might fail but file can be read diff --git a/util/file_reader_writer.h b/util/file_reader_writer.h index b3c65c95c7..2c524a14a0 100644 --- a/util/file_reader_writer.h +++ b/util/file_reader_writer.h @@ -59,6 +59,7 @@ class SequentialFileReader { class RandomAccessFileReader { private: std::unique_ptr file_; + std::string file_name_; Env* env_; Statistics* stats_; uint32_t hist_type_; @@ -68,6 +69,7 @@ class RandomAccessFileReader { public: explicit RandomAccessFileReader(std::unique_ptr&& raf, + std::string _file_name, Env* env = nullptr, Statistics* stats = nullptr, uint32_t hist_type = 0, @@ -75,6 +77,7 @@ class RandomAccessFileReader { RateLimiter* rate_limiter = nullptr, bool for_compaction = false) : file_(std::move(raf)), + file_name_(std::move(_file_name)), env_(env), stats_(stats), hist_type_(hist_type), @@ -109,6 +112,8 @@ class RandomAccessFileReader { RandomAccessFile* file() { return file_.get(); } + std::string file_name() const { return file_name_; } + bool use_direct_io() const { return file_->use_direct_io(); } }; diff --git a/util/testutil.cc b/util/testutil.cc index 45a301d217..623999342b 100644 --- a/util/testutil.cc +++ b/util/testutil.cc @@ -139,7 +139,8 @@ WritableFileWriter* GetWritableFileWriter(WritableFile* wf) { RandomAccessFileReader* GetRandomAccessFileReader(RandomAccessFile* raf) { unique_ptr file(raf); - return new RandomAccessFileReader(std::move(file)); + return new RandomAccessFileReader(std::move(file), + "[test RandomAccessFileReader]"); } SequentialFileReader* GetSequentialFileReader(SequentialFile* se) { diff --git a/utilities/blob_db/blob_dump_tool.cc b/utilities/blob_db/blob_dump_tool.cc index 7662d2b972..c295712f81 100644 --- a/utilities/blob_db/blob_dump_tool.cc +++ b/utilities/blob_db/blob_dump_tool.cc @@ -50,7 +50,7 @@ Status BlobDumpTool::Run(const std::string& filename, DisplayType show_key, if (file_size == 0) { return Status::Corruption("File is empty."); } - reader_.reset(new RandomAccessFileReader(std::move(file))); + reader_.reset(new RandomAccessFileReader(std::move(file), filename)); uint64_t offset = 0; uint64_t footer_offset = 0; s = DumpBlobLogHeader(&offset); diff --git a/utilities/blob_db/blob_file.cc b/utilities/blob_db/blob_file.cc index 83032ab3ec..28a27e3440 100644 --- a/utilities/blob_db/blob_file.cc +++ b/utilities/blob_db/blob_file.cc @@ -212,7 +212,8 @@ std::shared_ptr BlobFile::GetOrOpenRandomAccessReader( return nullptr; } - ra_file_reader_ = std::make_shared(std::move(rfile)); + ra_file_reader_ = std::make_shared(std::move(rfile), + PathName()); *fresh_open = true; return ra_file_reader_; } diff --git a/utilities/column_aware_encoding_util.cc b/utilities/column_aware_encoding_util.cc index ab8cefdd1d..5fd3e3ffef 100644 --- a/utilities/column_aware_encoding_util.cc +++ b/utilities/column_aware_encoding_util.cc @@ -49,7 +49,7 @@ void ColumnAwareEncodingReader::InitTableReader(const std::string& file_path) { options_.env->NewRandomAccessFile(file_path, &file, soptions_); options_.env->GetFileSize(file_path, &file_size); - file_.reset(new RandomAccessFileReader(std::move(file))); + file_.reset(new RandomAccessFileReader(std::move(file), file_path)); options_.comparator = &internal_comparator_; options_.table_factory = std::make_shared(); diff --git a/utilities/persistent_cache/block_cache_tier_file.cc b/utilities/persistent_cache/block_cache_tier_file.cc index fbf4ed8d9f..2f29ab798f 100644 --- a/utilities/persistent_cache/block_cache_tier_file.cc +++ b/utilities/persistent_cache/block_cache_tier_file.cc @@ -214,7 +214,7 @@ bool RandomAccessCacheFile::OpenImpl(const bool enable_direct_reads) { status.ToString().c_str()); return false; } - freader_.reset(new RandomAccessFileReader(std::move(file), env_)); + freader_.reset(new RandomAccessFileReader(std::move(file), Path(), env_)); return true; }