Skip to content

Commit

Permalink
Disable uncache_aggressiveness with allow_mmap_reads
Browse files Browse the repository at this point in the history
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
  • Loading branch information
pdillinger committed Oct 2, 2024
1 parent dd23e84 commit c737bed
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 1 deletion.
2 changes: 2 additions & 0 deletions include/rocksdb/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 7 additions & 1 deletion table/block_based/block_based_table_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit c737bed

Please sign in to comment.