From d7aa5decf369f48616a7b9b83136f2bbbd82ff60 Mon Sep 17 00:00:00 2001 From: Igor Fedotov Date: Mon, 21 Mar 2022 14:58:18 +0300 Subject: [PATCH] os/bluestore: proper locking for Allocators' dump methods Plus renaming parametrized dump to foreach() Fixes: https://tracker.ceph.com/issues/54973 Signed-off-by: Igor Fedotov --- src/os/bluestore/Allocator.cc | 4 ++-- src/os/bluestore/Allocator.h | 3 ++- src/os/bluestore/AvlAllocator.cc | 11 +++++++++-- src/os/bluestore/AvlAllocator.h | 4 +++- src/os/bluestore/BitmapAllocator.cc | 18 ++++------------- src/os/bluestore/BitmapAllocator.h | 8 ++++++-- src/os/bluestore/BlueFS.cc | 2 +- src/os/bluestore/BlueStore.cc | 16 +++++++-------- src/os/bluestore/BtreeAllocator.cc | 3 ++- src/os/bluestore/BtreeAllocator.h | 3 ++- src/os/bluestore/HybridAllocator.cc | 8 +++++--- src/os/bluestore/HybridAllocator.h | 3 ++- src/os/bluestore/StupidAllocator.cc | 2 +- src/os/bluestore/StupidAllocator.h | 2 +- src/os/bluestore/ZonedAllocator.cc | 4 ++-- src/os/bluestore/ZonedAllocator.h | 4 ++-- src/os/bluestore/fastbmap_allocator_impl.cc | 2 +- src/os/bluestore/fastbmap_allocator_impl.h | 22 ++++++++++++++++----- src/test/objectstore/Allocator_test.cc | 2 +- 19 files changed, 71 insertions(+), 50 deletions(-) diff --git a/src/os/bluestore/Allocator.cc b/src/os/bluestore/Allocator.cc index 731ae5de73c5b..fb2356c90f835 100644 --- a/src/os/bluestore/Allocator.cc +++ b/src/os/bluestore/Allocator.cc @@ -87,7 +87,7 @@ public: f->dump_string("length", len_hex); f->close_section(); }; - alloc->dump(iterated_allocation); + alloc->foreach(iterated_allocation); f->close_section(); f->close_section(); } else if (command == "bluestore allocator score " + name) { @@ -213,7 +213,7 @@ double Allocator::get_fragmentation_score() score_sum += get_score(len); sum += len; }; - dump(iterated_allocation); + foreach(iterated_allocation); double ideal = get_score(sum); diff --git a/src/os/bluestore/Allocator.h b/src/os/bluestore/Allocator.h index 5503ed213fb58..e378007c3c003 100644 --- a/src/os/bluestore/Allocator.h +++ b/src/os/bluestore/Allocator.h @@ -53,7 +53,8 @@ public: void release(const PExtentVector& release_set); virtual void dump() = 0; - virtual void dump(std::function notify) = 0; + virtual void foreach( + std::function notify) = 0; virtual void init_add_free(uint64_t offset, uint64_t length) = 0; virtual void init_rm_free(uint64_t offset, uint64_t length) = 0; diff --git a/src/os/bluestore/AvlAllocator.cc b/src/os/bluestore/AvlAllocator.cc index e7a9befef051b..242fcac811128 100644 --- a/src/os/bluestore/AvlAllocator.cc +++ b/src/os/bluestore/AvlAllocator.cc @@ -417,7 +417,6 @@ void AvlAllocator::_dump() const << std::dec << dendl; } - ldout(cct, 0) << __func__ << " range_size_tree: " << dendl; for (auto& rs : range_size_tree) { ldout(cct, 0) << std::hex @@ -427,7 +426,15 @@ void AvlAllocator::_dump() const } } -void AvlAllocator::dump(std::function notify) +void AvlAllocator::foreach( + std::function notify) +{ + std::lock_guard l(lock); + _foreach(notify); +} + +void AvlAllocator::_foreach( + std::function notify) const { for (auto& rs : range_tree) { notify(rs.start, rs.end - rs.start); diff --git a/src/os/bluestore/AvlAllocator.h b/src/os/bluestore/AvlAllocator.h index 3779a6702947e..d79242a521cc5 100644 --- a/src/os/bluestore/AvlAllocator.h +++ b/src/os/bluestore/AvlAllocator.h @@ -86,7 +86,8 @@ public: double get_fragmentation() override; void dump() override; - void dump(std::function notify) override; + void foreach( + std::function notify) override; void init_add_free(uint64_t offset, uint64_t length) override; void init_rm_free(uint64_t offset, uint64_t length) override; void shutdown() override; @@ -241,6 +242,7 @@ protected: return (static_cast(range_tree.size() - 1) / (free_blocks - 1)); } void _dump() const; + void _foreach(std::function) const; uint64_t _lowest_size_available() { auto rs = range_size_tree.begin(); diff --git a/src/os/bluestore/BitmapAllocator.cc b/src/os/bluestore/BitmapAllocator.cc index bbc77f1b45042..2decfcb87812d 100644 --- a/src/os/bluestore/BitmapAllocator.cc +++ b/src/os/bluestore/BitmapAllocator.cc @@ -102,20 +102,10 @@ void BitmapAllocator::dump() auto it = bins_overall.begin(); while (it != bins_overall.end()) { ldout(cct, 0) << __func__ - << " bin " << it->first - << "(< " << byte_u_t((1 << (it->first + 1)) * get_min_alloc_size()) << ")" - << " : " << it->second << " extents" - << dendl; + << " bin " << it->first + << "(< " << byte_u_t((1 << (it->first + 1)) * get_min_alloc_size()) << ")" + << " : " << it->second << " extents" + << dendl; ++it; } } - -void BitmapAllocator::dump(std::function notify) -{ - size_t alloc_size = get_min_alloc_size(); - auto multiply_by_alloc_size = [alloc_size, notify](size_t off, size_t len) { - notify(off * alloc_size, len * alloc_size); - }; - std::lock_guard lck(lock); - l1.dump(multiply_by_alloc_size); -} diff --git a/src/os/bluestore/BitmapAllocator.h b/src/os/bluestore/BitmapAllocator.h index ddaa8ca9eae05..a418718aaffe0 100644 --- a/src/os/bluestore/BitmapAllocator.h +++ b/src/os/bluestore/BitmapAllocator.h @@ -41,10 +41,14 @@ public: } void dump() override; - void dump(std::function notify) override; + void foreach( + std::function notify) override + { + foreach_internal(notify); + } double get_fragmentation() override { - return _get_fragmentation(); + return get_fragmentation_internal(); } void init_add_free(uint64_t offset, uint64_t length) override; diff --git a/src/os/bluestore/BlueFS.cc b/src/os/bluestore/BlueFS.cc index 397b9076e9075..e8458e4ebdc81 100644 --- a/src/os/bluestore/BlueFS.cc +++ b/src/os/bluestore/BlueFS.cc @@ -4325,7 +4325,7 @@ size_t BlueFS::probe_alloc_avail(int dev, uint64_t alloc_size) total += p2align(len, alloc_size); }; if (alloc[dev]) { - alloc[dev]->dump(iterated_allocation); + alloc[dev]->foreach(iterated_allocation); } return total; } diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 2e1e3f99177eb..55d08cfbddf7c 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -18267,7 +18267,7 @@ int BlueStore::copy_allocator(Allocator* src_alloc, Allocator* dest_alloc, uint6 auto count_entries = [&](uint64_t extent_offset, uint64_t extent_length) { (*p_num_entries)++; }; - src_alloc->dump(count_entries); + src_alloc->foreach(count_entries); dout(5) << "count num_entries=" << *p_num_entries << dendl; @@ -18293,7 +18293,7 @@ int BlueStore::copy_allocator(Allocator* src_alloc, Allocator* dest_alloc, uint6 derr << "zero length extent!!! offset=" << extent_offset << ", index=" << idx << dendl; } }; - src_alloc->dump(copy_entries); + src_alloc->foreach(copy_entries); dout(5) << "copy num_entries=" << idx << dendl; if (idx > *p_num_entries) { @@ -18400,7 +18400,7 @@ int BlueStore::store_allocator(Allocator* src_allocator) p_curr = buffer; // recycle the buffer } }; - allocator->dump(iterated_allocation); + allocator->foreach(iterated_allocation); // if got null extent -> fail the operation if (ret != 0) { derr << "Illegal extent, fail store operation" << dendl; @@ -19001,8 +19001,8 @@ int BlueStore::compare_allocators(Allocator* alloc1, Allocator* alloc2, uint64_t } }; - alloc1->dump(iterated_mapper1); - alloc2->dump(iterated_mapper2); + alloc1->foreach(iterated_mapper1); + alloc2->foreach(iterated_mapper2); qsort(arr1.get(), std::min(idx1, extent_count), sizeof(extent_t), cmpfunc); qsort(arr2.get(), std::min(idx2, extent_count), sizeof(extent_t), cmpfunc); @@ -19098,7 +19098,7 @@ int BlueStore::read_allocation_from_drive_for_bluestore_tool() auto count_entries = [&](uint64_t extent_offset, uint64_t extent_length) { stats.insert_count++; }; - allocator->dump(count_entries); + allocator->foreach(count_entries); ret = compare_allocators(allocator.get(), alloc, stats.insert_count, memory_target); if (ret == 0) { dout(5) << "Allocator drive - file integrity check OK" << dendl; @@ -19164,7 +19164,7 @@ void BlueStore::copy_allocator_content_to_fm(Allocator *allocator, FreelistManag txn = db->get_transaction(); } }; - allocator->dump(iterated_insert); + allocator->foreach(iterated_insert); if (idx % max_txn != 0) { db->submit_transaction_sync(txn); } @@ -19238,7 +19238,7 @@ int BlueStore::verify_rocksdb_allocations(Allocator *allocator) auto count_entries = [&](uint64_t extent_offset, uint64_t extent_length) { insert_count++; }; - temp_allocator->dump(count_entries); + temp_allocator->foreach(count_entries); uint64_t memory_target = cct->_conf.get_val("osd_memory_target"); int ret = compare_allocators(allocator, temp_allocator, insert_count, memory_target); diff --git a/src/os/bluestore/BtreeAllocator.cc b/src/os/bluestore/BtreeAllocator.cc index 6184375db81eb..6b919f18c021d 100644 --- a/src/os/bluestore/BtreeAllocator.cc +++ b/src/os/bluestore/BtreeAllocator.cc @@ -429,8 +429,9 @@ void BtreeAllocator::_dump() const } } -void BtreeAllocator::dump(std::function notify) +void BtreeAllocator::foreach(std::function notify) { + std::lock_guard l(lock); for (auto& rs : range_tree) { notify(rs.first, rs.second - rs.first); } diff --git a/src/os/bluestore/BtreeAllocator.h b/src/os/bluestore/BtreeAllocator.h index 0a68015e8075f..4561d9f4c45a1 100644 --- a/src/os/bluestore/BtreeAllocator.h +++ b/src/os/bluestore/BtreeAllocator.h @@ -83,7 +83,8 @@ public: double get_fragmentation() override; void dump() override; - void dump(std::function notify) override; + void foreach( + std::function notify) override; void init_add_free(uint64_t offset, uint64_t length) override; void init_rm_free(uint64_t offset, uint64_t length) override; void shutdown() override; diff --git a/src/os/bluestore/HybridAllocator.cc b/src/os/bluestore/HybridAllocator.cc index a6fa0bd75ed2e..126d902cf3a5d 100644 --- a/src/os/bluestore/HybridAllocator.cc +++ b/src/os/bluestore/HybridAllocator.cc @@ -145,11 +145,13 @@ void HybridAllocator::dump() << dendl; } -void HybridAllocator::dump(std::function notify) +void HybridAllocator::foreach( + std::function notify) { - AvlAllocator::dump(notify); + std::lock_guard l(lock); + AvlAllocator::_foreach(notify); if (bmap_alloc) { - bmap_alloc->dump(notify); + bmap_alloc->foreach(notify); } } diff --git a/src/os/bluestore/HybridAllocator.h b/src/os/bluestore/HybridAllocator.h index e254ba654a30f..a4cf1e2250c6e 100644 --- a/src/os/bluestore/HybridAllocator.h +++ b/src/os/bluestore/HybridAllocator.h @@ -31,7 +31,8 @@ public: double get_fragmentation() override; void dump() override; - void dump(std::function notify) override; + void foreach( + std::function notify) override; void init_rm_free(uint64_t offset, uint64_t length) override; void shutdown() override; diff --git a/src/os/bluestore/StupidAllocator.cc b/src/os/bluestore/StupidAllocator.cc index b4739f28dd11c..550024e67e77d 100644 --- a/src/os/bluestore/StupidAllocator.cc +++ b/src/os/bluestore/StupidAllocator.cc @@ -299,7 +299,7 @@ void StupidAllocator::dump() } } -void StupidAllocator::dump(std::function notify) +void StupidAllocator::foreach(std::function notify) { std::lock_guard l(lock); for (unsigned bin = 0; bin < free.size(); ++bin) { diff --git a/src/os/bluestore/StupidAllocator.h b/src/os/bluestore/StupidAllocator.h index e5aebce8ee8c3..0d50d73f42afd 100644 --- a/src/os/bluestore/StupidAllocator.h +++ b/src/os/bluestore/StupidAllocator.h @@ -61,7 +61,7 @@ public: double get_fragmentation() override; void dump() override; - void dump(std::function notify) override; + void foreach(std::function notify) override; void init_add_free(uint64_t offset, uint64_t length) override; void init_rm_free(uint64_t offset, uint64_t length) override; diff --git a/src/os/bluestore/ZonedAllocator.cc b/src/os/bluestore/ZonedAllocator.cc index 0a535e361ce78..4139b47556974 100644 --- a/src/os/bluestore/ZonedAllocator.cc +++ b/src/os/bluestore/ZonedAllocator.cc @@ -144,8 +144,8 @@ void ZonedAllocator::dump() std::lock_guard l(lock); } -void ZonedAllocator::dump(std::function notify) +void ZonedAllocator::foreach( + std::function notify) { std::lock_guard l(lock); } diff --git a/src/os/bluestore/ZonedAllocator.h b/src/os/bluestore/ZonedAllocator.h index 685af9228fb14..0778bd0da9e6a 100644 --- a/src/os/bluestore/ZonedAllocator.h +++ b/src/os/bluestore/ZonedAllocator.h @@ -94,8 +94,8 @@ public: uint64_t get_free() override; void dump() override; - void dump(std::function notify) override; + void foreach( + std::function notify) override; int64_t pick_zone_to_clean(float min_score, uint64_t min_saved); void set_cleaning_zone(uint32_t zone) { diff --git a/src/os/bluestore/fastbmap_allocator_impl.cc b/src/os/bluestore/fastbmap_allocator_impl.cc index 1b548eab71dde..595b124856f3c 100644 --- a/src/os/bluestore/fastbmap_allocator_impl.cc +++ b/src/os/bluestore/fastbmap_allocator_impl.cc @@ -567,7 +567,7 @@ inline ssize_t AllocatorLevel01Loose::count_0s(slot_t slot_val, size_t start_pos { return count_0s(~slot_val, start_pos); } -void AllocatorLevel01Loose::dump( +void AllocatorLevel01Loose::foreach_internal( std::function notify) { size_t len = 0; diff --git a/src/os/bluestore/fastbmap_allocator_impl.h b/src/os/bluestore/fastbmap_allocator_impl.h index 52a1edee2f8a6..e44fc76d75002 100644 --- a/src/os/bluestore/fastbmap_allocator_impl.h +++ b/src/os/bluestore/fastbmap_allocator_impl.h @@ -503,7 +503,7 @@ public: static inline ssize_t count_0s(slot_t slot_val, size_t start_pos); static inline ssize_t count_1s(slot_t slot_val, size_t start_pos); - void dump(std::function notify); + void foreach_internal(std::function notify); }; @@ -578,6 +578,22 @@ public: _mark_l2_on_l1(l2_pos, l2_pos_end); return allocated; } + + void foreach_internal( + std::function notify) + { + size_t alloc_size = get_min_alloc_size(); + auto multiply_by_alloc_size = [alloc_size, notify](size_t off, size_t len) { + notify(off * alloc_size, len * alloc_size); + }; + std::lock_guard l(lock); + l1.foreach_internal(multiply_by_alloc_size); + } + double get_fragmentation_internal() { + std::lock_guard l(lock); + return l1.get_fragmentation(); + } + protected: ceph::mutex lock = ceph::make_mutex("AllocatorLevel02::lock"); L1 l1; @@ -824,10 +840,6 @@ protected: { last_pos = 0; } - double _get_fragmentation() { - std::lock_guard l(lock); - return l1.get_fragmentation(); - } }; #endif diff --git a/src/test/objectstore/Allocator_test.cc b/src/test/objectstore/Allocator_test.cc index 210bc6d9d7457..f1e3a04f4d259 100644 --- a/src/test/objectstore/Allocator_test.cc +++ b/src/test/objectstore/Allocator_test.cc @@ -388,7 +388,7 @@ TEST_P(AllocTest, test_dump_fragmentation_score) ceph_assert(len > 0); free_sum += len; }; - alloc->dump(iterated_allocation); + alloc->foreach(iterated_allocation); EXPECT_GT(1, alloc->get_fragmentation_score()); EXPECT_EQ(capacity, free_sum + allocated_cnt); } -- 2.39.5