From 6798d1f3beb8fa644391079aced24b1ecda39dd4 Mon Sep 17 00:00:00 2001 From: Maysam Yabandeh Date: Tue, 2 May 2017 13:39:09 -0700 Subject: [PATCH] Revert "Delete filter before closing the table" Summary: This reverts commit 89833577a80ad7a2cbf6b99c5957f572b3548152. Closes https://github.com/facebook/rocksdb/pull/2240 Differential Revision: D4986982 Pulled By: maysamyabandeh fbshipit-source-id: 56c4c07b7b5b7c6fe122d5c2f2199d221c8510c0 --- table/block_based_table_factory.cc | 5 ----- table/block_based_table_reader.cc | 26 +++++++++++++++----------- table/block_based_table_reader.h | 5 ++--- table/partitioned_filter_block.cc | 20 ++++++++------------ table/partitioned_filter_block.h | 1 - table/table_test.cc | 17 +++++------------ 6 files changed, 30 insertions(+), 44 deletions(-) diff --git a/table/block_based_table_factory.cc b/table/block_based_table_factory.cc index 199718c08..8db76ea38 100644 --- a/table/block_based_table_factory.cc +++ b/table/block_based_table_factory.cc @@ -101,11 +101,6 @@ Status BlockBasedTableFactory::SanitizeOptions( "Unsupported BlockBasedTable format_version. Please check " "include/rocksdb/table.h for more info"); } - if (table_options_.block_cache && - table_options_.no_block_cache) { - return Status::InvalidArgument("Block cache is initialized" - ", but it is disabled"); - } return Status::OK(); } diff --git a/table/block_based_table_reader.cc b/table/block_based_table_reader.cc index 7bb8900ca..68164b282 100644 --- a/table/block_based_table_reader.cc +++ b/table/block_based_table_reader.cc @@ -2063,17 +2063,21 @@ Status BlockBasedTable::DumpTable(WritableFile* out_file) { } void BlockBasedTable::Close() { - const bool force_erase = true; - auto block_cache = rep_->table_options.block_cache.get(); - if (block_cache) { - // The filter_entry and inde_entry's lifetime ends with table's and is - // forced to be released. - // Note that if the xxx_entry is not set then the cached entry might remian - // in the cache for some more time. The destrcutor hence must take int - // account that the table object migth no longer be available. - rep_->filter_entry.Release(block_cache, force_erase); - rep_->index_entry.Release(block_cache, force_erase); - rep_->range_del_entry.Release(block_cache); + rep_->filter_entry.Release(rep_->table_options.block_cache.get()); + rep_->index_entry.Release(rep_->table_options.block_cache.get()); + rep_->range_del_entry.Release(rep_->table_options.block_cache.get()); + // cleanup index and filter blocks to avoid accessing dangling pointer + if (!rep_->table_options.no_block_cache) { + char cache_key[kMaxCacheKeyPrefixSize + kMaxVarint64Length]; + // Get the filter block key + auto key = GetCacheKey(rep_->cache_key_prefix, rep_->cache_key_prefix_size, + rep_->footer.metaindex_handle(), cache_key); + rep_->table_options.block_cache.get()->Erase(key); + // Get the index block key + key = GetCacheKeyFromOffset(rep_->cache_key_prefix, + rep_->cache_key_prefix_size, + rep_->dummy_index_reader_offset, cache_key); + rep_->table_options.block_cache.get()->Erase(key); } } diff --git a/table/block_based_table_reader.h b/table/block_based_table_reader.h index 6b10ba0a0..d8f8c3fa0 100644 --- a/table/block_based_table_reader.h +++ b/table/block_based_table_reader.h @@ -370,10 +370,9 @@ struct BlockBasedTable::CachableEntry { CachableEntry(TValue* _value, Cache::Handle* _cache_handle) : value(_value), cache_handle(_cache_handle) {} CachableEntry() : CachableEntry(nullptr, nullptr) {} - void Release(Cache* cache, bool force_erase = false) { + void Release(Cache* cache) { if (cache_handle) { - bool erased = cache->Release(cache_handle, force_erase); - assert(!force_erase || erased); + cache->Release(cache_handle); value = nullptr; cache_handle = nullptr; } diff --git a/table/partitioned_filter_block.cc b/table/partitioned_filter_block.cc index b4792046b..e38a35e37 100644 --- a/table/partitioned_filter_block.cc +++ b/table/partitioned_filter_block.cc @@ -82,21 +82,16 @@ PartitionedFilterBlockReader::PartitionedFilterBlockReader( : FilterBlockReader(contents.data.size(), stats, _whole_key_filtering), prefix_extractor_(prefix_extractor), comparator_(comparator), - table_(table), - block_cache_(table_->rep_->table_options.block_cache.get()){ + table_(table) { idx_on_fltr_blk_.reset(new Block(std::move(contents), kDisableGlobalSequenceNumber, 0 /* read_amp_bytes_per_bit */, stats)); } PartitionedFilterBlockReader::~PartitionedFilterBlockReader() { - // The destructor migh be called via cache evict after the table is deleted. - // We should avoid using table_ pointer in destructor then. - if (block_cache_) { - ReadLock rl(&mu_); - for (auto it = handle_list_.begin(); it != handle_list_.end(); ++it) { - block_cache_->Release(*it); - } + ReadLock rl(&mu_); + for (auto it = handle_list_.begin(); it != handle_list_.end(); ++it) { + table_->rep_->table_options.block_cache.get()->Release(*it); } } @@ -127,7 +122,7 @@ bool PartitionedFilterBlockReader::KeyMayMatch( return res; } if (LIKELY(filter_partition.IsSet())) { - filter_partition.Release(block_cache_); + filter_partition.Release(table_->rep_->table_options.block_cache.get()); } else { delete filter_partition.value; } @@ -159,7 +154,7 @@ bool PartitionedFilterBlockReader::PrefixMayMatch( return res; } if (LIKELY(filter_partition.IsSet())) { - filter_partition.Release(block_cache_); + filter_partition.Release(table_->rep_->table_options.block_cache.get()); } else { delete filter_partition.value; } @@ -187,7 +182,8 @@ PartitionedFilterBlockReader::GetFilterPartition(Slice* handle_value, auto s = fltr_blk_handle.DecodeFrom(handle_value); assert(s.ok()); const bool is_a_filter_partition = true; - if (LIKELY(block_cache_ != nullptr)) { + auto block_cache = table_->rep_->table_options.block_cache.get(); + if (LIKELY(block_cache != nullptr)) { bool pin_cached_filters = GetLevel() == 0 && table_->rep_->table_options.pin_l0_filter_and_index_blocks_in_cache; diff --git a/table/partitioned_filter_block.h b/table/partitioned_filter_block.h index 0cc63d7a6..1730604e4 100644 --- a/table/partitioned_filter_block.h +++ b/table/partitioned_filter_block.h @@ -85,7 +85,6 @@ class PartitionedFilterBlockReader : public FilterBlockReader { std::unique_ptr idx_on_fltr_blk_; const Comparator& comparator_; const BlockBasedTable* table_; - Cache* block_cache_; std::unordered_map filter_cache_; autovector handle_list_; port::RWMutex mu_; diff --git a/table/table_test.cc b/table/table_test.cc index d01cf9313..0f51f2722 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -9,6 +9,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. +#include #include #include @@ -1849,10 +1850,7 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) { // -- Table construction Options options; options.create_if_missing = true; - // Keep a ref to statistic to prevent it from being destructed before - // block cache gets cleaned up upon next table_factory.reset - auto statistics = CreateDBStatistics(); - options.statistics = statistics; + options.statistics = CreateDBStatistics(); // Enable the cache for index/filter blocks BlockBasedTableOptions table_options; @@ -1932,17 +1930,15 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) { } // release the iterator so that the block cache can reset correctly. iter.reset(); + c.ResetTableReader(); // -- PART 2: Open with very small block cache // In this test, no block will ever get hit since the block cache is // too small to fit even one entry. table_options.block_cache = NewLRUCache(1, 4); + options.statistics = CreateDBStatistics(); options.table_factory.reset(new BlockBasedTableFactory(table_options)); - // Keep a ref to statistic to prevent it from being destructed before - // block cache gets cleaned up upon next table_factory.reset - statistics = CreateDBStatistics(); - options.statistics = statistics; const ImmutableCFOptions ioptions2(options); c.Reopen(ioptions2); { @@ -1997,10 +1993,7 @@ TEST_F(BlockBasedTableTest, FilterBlockInBlockCache) { // Open table with filter policy table_options.filter_policy.reset(NewBloomFilterPolicy(1)); options.table_factory.reset(new BlockBasedTableFactory(table_options)); - // Keep a ref to statistic to prevent it from being destructed before - // block cache gets cleaned up upon next table_factory.reset - statistics = CreateDBStatistics(); - options.statistics = statistics; + options.statistics = CreateDBStatistics(); ImmutableCFOptions ioptions4(options); ASSERT_OK(c3.Reopen(ioptions4)); reader = dynamic_cast(c3.GetTableReader());