From 6b9b93829c9461457448e2bd17052374794101ff Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Fri, 29 Apr 2022 23:32:43 +0200 Subject: [PATCH] kv/RocksDBStore: Remove feature to make WholeSpaceIterator based on bounded iterator Iterator-bounding feature is introduced to make RocksDB iterators limited, so they would less likely traverse over tombstones. This is used when listing keys in fixed range, for example OMAPS for specific object. It is problematic when extending this logic to WholeSpaceIterator, since prefix must be taken into account. Fixes: https://tracker.ceph.com/issues/55444 Signed-off-by: Adam Kupczyk --- src/kv/KeyValueDB.h | 4 ++-- src/kv/MemDB.h | 2 +- src/kv/RocksDBStore.cc | 8 ++++---- src/kv/RocksDBStore.h | 20 ++------------------ src/test/ObjectMap/KeyValueDBMemory.cc | 2 +- src/test/ObjectMap/KeyValueDBMemory.h | 2 +- 6 files changed, 11 insertions(+), 27 deletions(-) diff --git a/src/kv/KeyValueDB.h b/src/kv/KeyValueDB.h index fee513c5a68..98bf0c07c43 100644 --- a/src/kv/KeyValueDB.h +++ b/src/kv/KeyValueDB.h @@ -331,11 +331,11 @@ public: std::optional upper_bound; }; - virtual WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0, IteratorBounds bounds = IteratorBounds()) = 0; + virtual WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0) = 0; virtual Iterator get_iterator(const std::string &prefix, IteratorOpts opts = 0, IteratorBounds bounds = IteratorBounds()) { return std::make_shared( prefix, - get_wholespace_iterator(opts, std::move(bounds))); + get_wholespace_iterator(opts)); } virtual uint64_t get_estimated_size(std::map &extra) = 0; diff --git a/src/kv/MemDB.h b/src/kv/MemDB.h index 80fed647efb..32d81db225e 100644 --- a/src/kv/MemDB.h +++ b/src/kv/MemDB.h @@ -212,7 +212,7 @@ public: return 0; } - WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0, IteratorBounds bounds = IteratorBounds()) override { + WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0) override { return std::shared_ptr( new MDBWholeSpaceIteratorImpl(&m_map, &m_lock, &iterator_seq_no, m_using_btree)); } diff --git a/src/kv/RocksDBStore.cc b/src/kv/RocksDBStore.cc index 3e8c618837c..d3e3a077f28 100644 --- a/src/kv/RocksDBStore.cc +++ b/src/kv/RocksDBStore.cc @@ -2999,7 +2999,7 @@ KeyValueDB::Iterator RocksDBStore::get_iterator(const std::string& prefix, Itera std::move(bounds)); } } else { - return KeyValueDB::get_iterator(prefix, opts, std::move(bounds)); + return KeyValueDB::get_iterator(prefix, opts); } } @@ -3008,11 +3008,11 @@ rocksdb::Iterator* RocksDBStore::new_shard_iterator(rocksdb::ColumnFamilyHandle* return db->NewIterator(rocksdb::ReadOptions(), cf); } -RocksDBStore::WholeSpaceIterator RocksDBStore::get_wholespace_iterator(IteratorOpts opts, IteratorBounds bounds) +RocksDBStore::WholeSpaceIterator RocksDBStore::get_wholespace_iterator(IteratorOpts opts) { if (cf_handles.size() == 0) { return std::make_shared( - this, default_cf, opts, std::move(bounds)); + this, default_cf, opts); } else { return std::make_shared(this); } @@ -3020,7 +3020,7 @@ RocksDBStore::WholeSpaceIterator RocksDBStore::get_wholespace_iterator(IteratorO RocksDBStore::WholeSpaceIterator RocksDBStore::get_default_cf_iterator() { - return std::make_shared(this, default_cf, 0, IteratorBounds()); + return std::make_shared(this, default_cf, 0); } int RocksDBStore::prepare_for_reshard(const std::string& new_sharding, diff --git a/src/kv/RocksDBStore.h b/src/kv/RocksDBStore.h index f880954dd85..25a2045ffaf 100644 --- a/src/kv/RocksDBStore.h +++ b/src/kv/RocksDBStore.h @@ -353,32 +353,16 @@ public: public KeyValueDB::WholeSpaceIteratorImpl { protected: rocksdb::Iterator *dbiter; - const KeyValueDB::IteratorBounds bounds; - const rocksdb::Slice iterate_lower_bound; - const rocksdb::Slice iterate_upper_bound; public: explicit RocksDBWholeSpaceIteratorImpl(const RocksDBStore* db, rocksdb::ColumnFamilyHandle* cf, - const KeyValueDB::IteratorOpts opts, - KeyValueDB::IteratorBounds bounds_) : - bounds(std::move(bounds_)), - iterate_lower_bound(make_slice(bounds.lower_bound)), - iterate_upper_bound(make_slice(bounds.upper_bound)) + const KeyValueDB::IteratorOpts opts) { rocksdb::ReadOptions options = rocksdb::ReadOptions(); if (opts & ITERATOR_NOCACHE) options.fill_cache=false; - if (db->cct->_conf->osd_rocksdb_iterator_bounds_enabled) { - if (bounds.lower_bound) { - options.iterate_lower_bound = &iterate_lower_bound; - } - if (bounds.upper_bound) { - options.iterate_upper_bound = &iterate_upper_bound; - } - } dbiter = db->db->NewIterator(options, cf); } - //virtual ~RocksDBWholeSpaceIteratorImpl() { } ~RocksDBWholeSpaceIteratorImpl() override; int seek_to_first() override; @@ -533,7 +517,7 @@ err: return nullptr; } - WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0, IteratorBounds bounds = IteratorBounds()) override; + WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0) override; private: WholeSpaceIterator get_default_cf_iterator(); diff --git a/src/test/ObjectMap/KeyValueDBMemory.cc b/src/test/ObjectMap/KeyValueDBMemory.cc index b2b351baef4..234e963397e 100644 --- a/src/test/ObjectMap/KeyValueDBMemory.cc +++ b/src/test/ObjectMap/KeyValueDBMemory.cc @@ -234,7 +234,7 @@ int KeyValueDBMemory::rm_range_keys(const string &prefix, const string &start, c return 0; } -KeyValueDB::WholeSpaceIterator KeyValueDBMemory::get_wholespace_iterator(IteratorOpts opts, IteratorBounds bounds) { +KeyValueDB::WholeSpaceIterator KeyValueDBMemory::get_wholespace_iterator(IteratorOpts opts) { return std::shared_ptr( new WholeSpaceMemIterator(this) ); diff --git a/src/test/ObjectMap/KeyValueDBMemory.h b/src/test/ObjectMap/KeyValueDBMemory.h index 8a154ea296d..de84ede9049 100644 --- a/src/test/ObjectMap/KeyValueDBMemory.h +++ b/src/test/ObjectMap/KeyValueDBMemory.h @@ -184,5 +184,5 @@ private: friend class WholeSpaceMemIterator; public: - WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0, IteratorBounds bounds = IteratorBounds()) override; + WholeSpaceIterator get_wholespace_iterator(IteratorOpts opts = 0) override; }; -- 2.39.5