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>

diff --git a/src/os/bluestore/Allocator.cc b/src/os/bluestore/Allocator.cc
index 731ae5de73c5b7768e3408fa98148aa5f6f0880e..fb2356c90f8356a8c661f11fe7bbfdb28de2a4f7 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) {
@@ -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 5503ed213fb580f903fd6d4bafff204fbbda3cac..e378007c3c0036a90619a22be04e805ce54cbe2e 100644 (file)
--- 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<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 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 e7a9befef051b0a112717a130429dbc0f9f233e8..242fcac8111288286630f695e4d49e526a9e2f85 100644 (file)
--- 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<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 3779a6702947ef9488b8cb45251ab5b5f76cf9c1..d79242a521cc53bc92d441f16a8c853cc89bba21 100644 (file)
--- 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<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;
@@ -241,6 +242,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 bbc77f1b450425b40af50c32651b47f1dea409a3..2decfcb87812dcbdcd7a02ed0729e93b01015df5 100644 (file)
--- 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<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 ddaa8ca9eae05177a5d6e84be90268f4b08bda9b..a418718aaffe0333049291edf306af91b6cfdb31 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 397b9076e9075879f3c509ade12c67e87c9b9252..e8458e4ebdc81a70425aefa11612dd170ce32fd0 100644 (file)
--- 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 2e1e3f99177eb4c56d715ed55513080079f3be91..55d08cfbddf7c94802ce6074d8f45bca15b94c22 100644 (file)
--- 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<Option::size_t>("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 6184375db81eb0d84f79b0f6d24cca6564681e09..6b919f18c021dc29d4b6bc4edabbb3982aad08fc 100644 (file)
--- 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<void(uint64_t offset, uint64_t length)> notify)
+void BtreeAllocator::foreach(std::function<void(uint64_t offset, uint64_t length)> 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 0a68015e8075f756af585a9d60aba80c872385d1..4561d9f4c45a19b844463676b39c75a72291eee7 100644 (file)
--- 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<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;

diff --git a/src/os/bluestore/HybridAllocator.cc b/src/os/bluestore/HybridAllocator.cc
index a6fa0bd75ed2e7895e99ab17f3222fc0f9e7abde..126d902cf3a5db278e12357e1eddc6ac1e9d1478 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 e254ba654a30f3858bfad6f8171bb82d97770476..a4cf1e2250c6eff1b572f3b25b81f8d902a35a14 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 b4739f28dd11c4a96d2cd99e034d54fbd1e44f51..550024e67e77dc8d199a473e4cc578d87a2c7af9 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 e5aebce8ee8c32017665760bafbd6c0b6bcc5bdb..0d50d73f42afdfe788715562328d01b59c957207 100644 (file)
--- 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<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 0a535e361ce7891a031c3eea2dba1fdba6330a3b..4139b4755697425935cde7c0ca1975597f64d629 100644 (file)
--- 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<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 685af9228fb14f688aeab6b9cd1e9f6b1d273162..0778bd0da9e6a3b425d9fea982dd610c6cf818ab 100644 (file)
--- 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<void(uint64_t offset,
-                               uint64_t length)> notify) override;
+  void foreach(
+    std::function<void(uint64_t offset, uint64_t length)> 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 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 210bc6d9d74573cad89282172aedb7b2961308af..f1e3a04f4d259011086081b84a9bd12ffe207f67 100644 (file)
--- 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);
   }