From ed46981bea38ad6ccc6258956ceafce08d7b50e9 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Thu, 4 Jan 2024 18:39:05 -0800 Subject: [PATCH] Fix and defend against FilePrefetchBuffer combined with mmap reads (#12206) Summary: FilePrefetchBuffer makes an unchecked assumption about the behavior of RandomAccessFileReader::Read: that it will write to the provided buffer rather than returning the data in an alternate buffer. FilePrefetchBuffer has been quietly incompatible with mmap reads (e.g. allow_mmap_reads / use_mmap_reads) because in that case an alternate buffer is returned (mmapped memory). This incompatibility currently leads to quiet data corruption, as seen in amplified crash test failure in https://github.com/facebook/rocksdb/issues/12200. In this change, * Check whether RandomAccessFileReader::Read has the expected behavior, and fail if not. (Assertion failure in debug build, return Corruption in release build.) This will detect future regressions synchronously and precisely, rather than relying on debugging downstream data corruption. * Why not recover? My understanding is that FilePrefetchBuffer is not intended for use when RandomAccessFileReader::Read uses an alternate buffer, so quietly recovering could lead to undesirable (inefficient) behavior. * Mention incompatibility with mmap-based readers in the internal API comments for FilePrefetchBuffer * Fix two cases where FilePrefetchBuffer could be used with mmap, both stemming from SstFileDumper, though one fix is in BlockBasedTableReader. There is currently no way to ask a RandomAccessFileReader whether it's using mmap, so we currently have to rely on other options as clues. Keeping separate from https://github.com/facebook/rocksdb/issues/12200 in part because this change is more appropriate for backport than that one. Pull Request resolved: https://github.com/facebook/rocksdb/pull/12206 Test Plan: * Manually verified that the new check aids in debugging. * Unit test added, that fails if either fix is missed. * Ran blackbox_crash_test for hours, with and without https://github.com/facebook/rocksdb/issues/12200 Reviewed By: akankshamahajan15 Differential Revision: D52551701 Pulled By: pdillinger fbshipit-source-id: dea87c5782b7c484a6c6e424585c8832dfc580dc --- file/file_prefetch_buffer.cc | 11 ++++-- file/file_prefetch_buffer.h | 6 +++- table/block_based/block_based_table_reader.cc | 2 +- .../block_based_table_reader_test.cc | 2 +- table/sst_file_dumper.cc | 4 +-- tools/sst_dump_test.cc | 35 +++++++++++++++++++ .../bug_fixes/fix_fileprefetchbuffer_mmap.md | 1 + 7 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 unreleased_history/bug_fixes/fix_fileprefetchbuffer_mmap.md diff --git a/file/file_prefetch_buffer.cc b/file/file_prefetch_buffer.cc index 2bd8c4a858..1d55a0033f 100644 --- a/file/file_prefetch_buffer.cc +++ b/file/file_prefetch_buffer.cc @@ -84,9 +84,9 @@ Status FilePrefetchBuffer::Read(const IOOptions& opts, uint64_t read_len, uint64_t chunk_len, uint64_t start_offset, uint32_t index) { Slice result; + char* to_buf = bufs_[index].buffer_.BufferStart() + chunk_len; Status s = reader->Read(opts, start_offset + chunk_len, read_len, &result, - bufs_[index].buffer_.BufferStart() + chunk_len, - /*aligned_buf=*/nullptr); + to_buf, /*aligned_buf=*/nullptr); #ifndef NDEBUG if (result.size() < read_len) { // Fake an IO error to force db_stress fault injection to ignore @@ -97,6 +97,13 @@ Status FilePrefetchBuffer::Read(const IOOptions& opts, if (!s.ok()) { return s; } + if (result.data() != to_buf) { + // If the read is coming from some other buffer already in memory (such as + // mmap) then it would be inefficient to create another copy in this + // FilePrefetchBuffer. The caller is expected to exclude this case. + assert(false); + return Status::Corruption("File read didn't populate our buffer"); + } if (usage_ == FilePrefetchBufferUsage::kUserScanPrefetch) { RecordTick(stats_, PREFETCH_BYTES, read_len); diff --git a/file/file_prefetch_buffer.h b/file/file_prefetch_buffer.h index 0c6ba7e669..002f176b1b 100644 --- a/file/file_prefetch_buffer.h +++ b/file/file_prefetch_buffer.h @@ -91,7 +91,8 @@ class FilePrefetchBuffer { // max_readahead_size should be greater than equal to readahead_size. // enable : controls whether reading from the buffer is enabled. // If false, TryReadFromCache() always return false, and we only take stats - // for the minimum offset if track_min_offset = true. + // for the minimum offset if track_min_offset = true. See below NOTE about + // mmap reads. // track_min_offset : Track the minimum offset ever read and collect stats on // it. Used for adaptable readahead of the file footer/metadata. // implicit_auto_readahead : Readahead is enabled implicitly by rocksdb after @@ -101,6 +102,9 @@ class FilePrefetchBuffer { // and max_readahead_size are passed in. // A user can construct a FilePrefetchBuffer without any arguments, but use // `Prefetch` to load data into the buffer. + // NOTE: FilePrefetchBuffer is incompatible with prefetching from + // RandomAccessFileReaders using mmap reads, so it is common to use + // `!use_mmap_reads` for the `enable` parameter. FilePrefetchBuffer( size_t readahead_size = 0, size_t max_readahead_size = 0, bool enable = true, bool track_min_offset = false, diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 4de9eba23e..717413aa4d 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -601,7 +601,7 @@ Status BlockBasedTable::Open( const bool prefetch_all = prefetch_index_and_filter_in_cache || level == 0; const bool preload_all = !table_options.cache_index_and_filter_blocks; - if (!ioptions.allow_mmap_reads) { + if (!ioptions.allow_mmap_reads && !env_options.use_mmap_reads) { s = PrefetchTail(ro, file.get(), file_size, force_direct_prefetch, tail_prefetch_stats, prefetch_all, preload_all, &prefetch_buffer, ioptions.stats, tail_size, diff --git a/table/block_based/block_based_table_reader_test.cc b/table/block_based/block_based_table_reader_test.cc index 7255fae7e1..38495ce969 100644 --- a/table/block_based/block_based_table_reader_test.cc +++ b/table/block_based/block_based_table_reader_test.cc @@ -162,7 +162,7 @@ class BlockBasedTableReaderBaseTest : public testing::Test { bool user_defined_timestamps_persisted = true) { const MutableCFOptions moptions(options_); TableReaderOptions table_reader_options = TableReaderOptions( - ioptions, moptions.prefix_extractor, EnvOptions(), comparator, + ioptions, moptions.prefix_extractor, foptions, comparator, 0 /* block_protection_bytes_per_key */, false /* _skip_filters */, false /* _immortal */, false /* _force_direct_prefetch */, -1 /* _level */, nullptr /* _block_cache_tracer */, diff --git a/table/sst_file_dumper.cc b/table/sst_file_dumper.cc index 3972cdfb37..69ca887891 100644 --- a/table/sst_file_dumper.cc +++ b/table/sst_file_dumper.cc @@ -104,8 +104,8 @@ Status SstFileDumper::GetTableReader(const std::string& file_path) { file_.reset(new RandomAccessFileReader(std::move(file), file_path)); FilePrefetchBuffer prefetch_buffer( - 0 /* readahead_size */, 0 /* max_readahead_size */, true /* enable */, - false /* track_min_offset */); + 0 /* readahead_size */, 0 /* max_readahead_size */, + !fopts.use_mmap_reads /* enable */, false /* track_min_offset */); if (s.ok()) { const uint64_t kSstDumpTailPrefetchSize = 512 * 1024; uint64_t prefetch_size = (file_size > kSstDumpTailPrefetchSize) diff --git a/tools/sst_dump_test.cc b/tools/sst_dump_test.cc index 2ebdad1ee0..83489e38e5 100644 --- a/tools/sst_dump_test.cc +++ b/tools/sst_dump_test.cc @@ -16,9 +16,11 @@ #include "rocksdb/filter_policy.h" #include "rocksdb/sst_dump_tool.h" #include "table/block_based/block_based_table_factory.h" +#include "table/sst_file_dumper.h" #include "table/table_builder.h" #include "test_util/testharness.h" #include "test_util/testutil.h" +#include "util/defer.h" namespace ROCKSDB_NAMESPACE { @@ -485,6 +487,39 @@ TEST_F(SSTDumpToolTest, RawOutput) { } } +TEST_F(SSTDumpToolTest, SstFileDumperMmapReads) { + Options opts; + opts.env = env(); + std::string file_path = MakeFilePath("rocksdb_sst_test.sst"); + createSST(opts, file_path, 10); + + EnvOptions env_opts; + uint64_t data_size = 0; + + // Test all combinations of mmap read options + for (int i = 0; i < 4; ++i) { + SaveAndRestore sar_opts(&opts.allow_mmap_reads, (i & 1) != 0); + SaveAndRestore sar_env_opts(&env_opts.use_mmap_reads, (i & 2) != 0); + + SstFileDumper dumper(opts, file_path, Temperature::kUnknown, + 1024 /*readahead_size*/, true /*verify_checksum*/, + false /*output_hex*/, false /*decode_blob_index*/, + env_opts); + ASSERT_OK(dumper.getStatus()); + std::shared_ptr tp; + ASSERT_OK(dumper.ReadTableProperties(&tp)); + ASSERT_NE(tp.get(), nullptr); + if (i == 0) { + // Verify consistency of a populated field with some entropy + data_size = tp->data_size; + ASSERT_GT(data_size, 0); + } else { + ASSERT_EQ(data_size, tp->data_size); + } + } + + cleanup(opts, file_path); +} } // namespace ROCKSDB_NAMESPACE int main(int argc, char** argv) { diff --git a/unreleased_history/bug_fixes/fix_fileprefetchbuffer_mmap.md b/unreleased_history/bug_fixes/fix_fileprefetchbuffer_mmap.md new file mode 100644 index 0000000000..e5cd8be356 --- /dev/null +++ b/unreleased_history/bug_fixes/fix_fileprefetchbuffer_mmap.md @@ -0,0 +1 @@ +Fixed some cases of in-memory data corruption using mmap reads with `BackupEngine`, `sst_dump`, or `ldb`.