From c737bed5846321c5f25eaa5c09532abb925e9d39 Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Wed, 2 Oct 2024 15:27:38 -0700 Subject: [PATCH] Disable uncache_aggressiveness with allow_mmap_reads Summary: There was a crash test Bus Error crash in `IndexBlockIter::SeekToFirstImpl()` <- .. <- `BlockBasedTable::~BlockBasedTable()` with `--mmap_read=1`, which suggests some kind of incompatibility that I haven't diagnosed. Bus Error is uncommon these days as CPUs support unaligned reads, but are associated with mmap problems. Because mmap reads really only make sense without block cache, it's not a concerning loss to essentially disable the combination. Test Plan: watch crash test --- include/rocksdb/options.h | 2 ++ table/block_based/block_based_table_reader.cc | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/include/rocksdb/options.h b/include/rocksdb/options.h index 9700e25af55..caeedf2fc77 100644 --- a/include/rocksdb/options.h +++ b/include/rocksdb/options.h @@ -388,6 +388,8 @@ struct ColumnFamilyOptions : public AdvancedColumnFamilyOptions { // block cache entries (shared among copies) are obsolete. Such a scenerio // is the best case for uncache_aggressiveness = 0. // + // When using allow_mmap_reads=true, this option is ignored (no un-caching). + // // Once validated in production, the default will likely change to something // around 300. uint32_t uncache_aggressiveness = 0; diff --git a/table/block_based/block_based_table_reader.cc b/table/block_based/block_based_table_reader.cc index 0dfe3e38ab6..d07fd4ef50f 100644 --- a/table/block_based/block_based_table_reader.cc +++ b/table/block_based/block_based_table_reader.cc @@ -137,7 +137,13 @@ extern const std::string kHashIndexPrefixesMetadataBlock; BlockBasedTable::~BlockBasedTable() { auto ua = rep_->uncache_aggressiveness.LoadRelaxed(); - if (ua > 0 && rep_->table_options.block_cache) { + // NOTE: there is an undiagnosed incompatibility with mmap reads, + // where attempting to read the index below can result in bus error. + // In theory the mmap should remain in place until destruction of + // rep_, so even a page fault should be satisfiable. But also, combining + // mmap reads with block cache is weird, so it's not a concerning loss. + if (ua > 0 && rep_->table_options.block_cache && + !rep_->ioptions.allow_mmap_reads) { if (rep_->filter) { rep_->filter->EraseFromCacheBeforeDestruction(ua); }