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 <igor.fedotov@croit.io>
(cherry picked from commit d7aa5decf369f48616a7b9b83136f2bbbd82ff60)

 Conflicts:
	src/os/bluestore/BlueStore.cc
        src/os/bluestore/BtreeAllocator.cc
	src/os/bluestore/BtreeAllocator.h
	src/os/bluestore/ZonedAllocator.cc
 caused by freshier stuff not present in Pacific

diff --git a/src/os/bluestore/Allocator.cc b/src/os/bluestore/Allocator.cc
index 91a001b5c9749ca07256d16457db2e03dddf1a7f..f92821f0c853beb4c696742f0a59d655b9d40a31 100644 (file)
--- 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) {
@@ -203,7 +203,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 177438025d73cbc8da4130e33aeac69e2b408d48..38ea4f997388b1a963cee076c85b12ad4ecfcdbe 100644 (file)
--- a/src/os/bluestore/Allocator.h
+++ b/src/os/bluestore/Allocator.h
@@ -54,7 +54,8 @@ public:
   void release(const PExtentVector& release_set);
 
   virtual void dump() = 0;
-  virtual void dump(std::function<void(uint64_t offset, uint64_t length)> notify) = 0;
+  virtual void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> notify) = 0;
 
   virtual void zoned_set_zone_states(std::vector<zone_state_t> &&_zone_states) {}
   virtual bool zoned_get_zones_to_clean(std::deque<uint64_t> *zones_to_clean) {

diff --git a/src/os/bluestore/AvlAllocator.cc b/src/os/bluestore/AvlAllocator.cc
index b2eb999906ac4d7876b358d1ce98f3f40ad9b5fc..5f17a3689b18f21c905453d8652d8ce0ff524d2e 100644 (file)
--- a/src/os/bluestore/AvlAllocator.cc
+++ b/src/os/bluestore/AvlAllocator.cc
@@ -419,7 +419,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
@@ -429,7 +428,15 @@ void AvlAllocator::_dump() const
   }
 }
 
-void AvlAllocator::dump(std::function<void(uint64_t offset, uint64_t length)> notify)
+void AvlAllocator::foreach(
+  std::function<void(uint64_t offset, uint64_t length)> notify)
+{
+  std::lock_guard l(lock);
+  _foreach(notify);
+}
+
+void AvlAllocator::_foreach(
+  std::function<void(uint64_t offset, uint64_t length)> 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 47ac9e4e713490e71481774d0747ba6974fffcc4..f47ee5be003e44a31d5f8b44fe9b9978d0a5fac3 100644 (file)
--- a/src/os/bluestore/AvlAllocator.h
+++ b/src/os/bluestore/AvlAllocator.h
@@ -93,7 +93,8 @@ public:
   double get_fragmentation() override;
 
   void dump() override;
-  void dump(std::function<void(uint64_t offset, uint64_t length)> notify) override;
+  void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> 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;
@@ -250,6 +251,7 @@ protected:
     return (static_cast<double>(range_tree.size() - 1) / (free_blocks - 1));
   }
   void _dump() const;
+  void _foreach(std::function<void(uint64_t offset, uint64_t length)>) 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 a744eb17bfea05b5b91eea715305d58cafbe8ec6..9304cb3f239c9a1172420d118f47b48ac0a9e60d 100644 (file)
--- a/src/os/bluestore/BitmapAllocator.cc
+++ b/src/os/bluestore/BitmapAllocator.cc
@@ -95,20 +95,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<void(uint64_t offset, uint64_t length)> 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 5c768a4ac2fb1ede2d42c8175aa343df365b0268..bb6fa73a1a53a4af6d1b8bb80af3c5f5545bb224 100644 (file)
--- a/src/os/bluestore/BitmapAllocator.h
+++ b/src/os/bluestore/BitmapAllocator.h
@@ -41,10 +41,14 @@ public:
   }
 
   void dump() override;
-  void dump(std::function<void(uint64_t offset, uint64_t length)> notify) override;
+  void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> 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 e9c5d7af835ee9f41932c594ab33d749384154cd..e8e1cb688c790def79308161dad3c73e19b256fd 100644 (file)
--- a/src/os/bluestore/BlueFS.cc
+++ b/src/os/bluestore/BlueFS.cc
@@ -3803,7 +3803,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 8eb6f36b2ed0398b19d744890c5637f509cd4325..f14cb68a3f2e74948b85adeb73dc6874ae6584a0 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -16935,3 +16935,4 @@ void RocksDBBlueFSVolumeSelector::dump(ostream& sout) {
 }
 
 // =======================================================
+// =======================================================

diff --git a/src/os/bluestore/HybridAllocator.cc b/src/os/bluestore/HybridAllocator.cc
index 3a2da67379b592eda2b8312b741102ff8bf149c7..b78a99ffeba318a38e4822591e0c4dddb9e46fae 100644 (file)
--- 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<void(uint64_t offset, uint64_t length)> notify)
+void HybridAllocator::foreach(
+  std::function<void(uint64_t offset, uint64_t length)> 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 92e8b9e80b18a380c0c24dca7995a87d02f64112..d60fc1a319c717bcaf2ce4288d022bdccf0890f2 100644 (file)
--- 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<void(uint64_t offset, uint64_t length)> notify) override;
+  void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> 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 bdebe10a5cb764a29526d95f24f32daaddb52afe..805a51fb090bed07722aa61b37475e56c78d06c9 100644 (file)
--- a/src/os/bluestore/StupidAllocator.cc
+++ b/src/os/bluestore/StupidAllocator.cc
@@ -299,7 +299,7 @@ void StupidAllocator::dump()
   }
 }
 
-void StupidAllocator::dump(std::function<void(uint64_t offset, uint64_t length)> notify)
+void StupidAllocator::foreach(std::function<void(uint64_t offset, uint64_t length)> 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 bd99a7c835fcddd9576f03d952a5047db36942e8..e1fc101e0879f7bf1e64f273489c01c00b5e2d09 100644 (file)
--- a/src/os/bluestore/StupidAllocator.h
+++ b/src/os/bluestore/StupidAllocator.h
@@ -62,7 +62,7 @@ public:
   double get_fragmentation() override;
 
   void dump() override;
-  void dump(std::function<void(uint64_t offset, uint64_t length)> notify) override;
+  void foreach(std::function<void(uint64_t offset, uint64_t length)> 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 0ea278c37dad866d1d490c46d3304a4562a3de60..71f0e6e8905b2af56857ab5b46c242c4c161b5a6 100644 (file)
--- a/src/os/bluestore/ZonedAllocator.cc
+++ b/src/os/bluestore/ZonedAllocator.cc
@@ -108,8 +108,9 @@ void ZonedAllocator::dump() {
   std::lock_guard l(lock);
 }
 
-void ZonedAllocator::dump(std::function<void(uint64_t offset,
-                                            uint64_t length)> notify) {
+void ZonedAllocator::foreach(
+  std::function<void(uint64_t offset, uint64_t length)> notify)
+{
   std::lock_guard l(lock);
 }
 

diff --git a/src/os/bluestore/ZonedAllocator.h b/src/os/bluestore/ZonedAllocator.h
index 5bdf197a58d5d8ffd15861330efe719cc01528be..47cb46be7646b365c3e9244d6278f5a6633657ec 100644 (file)
--- a/src/os/bluestore/ZonedAllocator.h
+++ b/src/os/bluestore/ZonedAllocator.h
@@ -76,8 +76,8 @@ public:
   uint64_t get_free() override;
 
   void dump() override;
-  void dump(std::function<void(uint64_t offset,
-                               uint64_t length)> notify) override;
+  void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> notify) override;
 
   void zoned_set_zone_states(std::vector<zone_state_t> &&_zone_states) override;
   bool zoned_get_zones_to_clean(std::deque<uint64_t> *zones_to_clean) override;

diff --git a/src/os/bluestore/fastbmap_allocator_impl.cc b/src/os/bluestore/fastbmap_allocator_impl.cc
index 1b548eab71ddee27287b4b3c6872b5ad8292c306..595b124856f3ccdd65cd75004dc72291c2b89667 100644 (file)
--- 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<void(uint64_t offset, uint64_t length)> notify)
 {
   size_t len = 0;

diff --git a/src/os/bluestore/fastbmap_allocator_impl.h b/src/os/bluestore/fastbmap_allocator_impl.h
index 52a1edee2f8a60f040417110fef26f26bdb2f86c..e44fc76d750023a5ad9e677f6890f7c11e36d373 100644 (file)
--- 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<void(uint64_t offset, uint64_t length)> notify);
+  void foreach_internal(std::function<void(uint64_t offset, uint64_t length)> notify);
 };
 
 
@@ -578,6 +578,22 @@ public:
     _mark_l2_on_l1(l2_pos, l2_pos_end);
     return allocated;
   }
+
+  void foreach_internal(
+    std::function<void(uint64_t offset, uint64_t length)> 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 0b417ae18640b969512c05c400ea7e42558bbbd8..3e969abc485b0df545427326e92f45298b0be8f7 100644 (file)
--- a/src/test/objectstore/Allocator_test.cc
+++ b/src/test/objectstore/Allocator_test.cc
@@ -385,7 +385,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);
   }