os/bluestore: fix btree allocator

Fixes: https://tracker.ceph.com/issues/61949
Signed-off-by: Igor Fedotov <igor.fedotov@croit.io>
(cherry picked from commit c4c3e34f819ef13a476093969bb634658b84e026)

diff --git a/src/os/bluestore/BtreeAllocator.cc b/src/os/bluestore/BtreeAllocator.cc
index 2455ec111b17054f6781af01ae3dcbf9a83cff59..2647b87599208fb78d78be9591c117b3ab6c72a2 100644 (file)
--- a/src/os/bluestore/BtreeAllocator.cc
+++ b/src/os/bluestore/BtreeAllocator.cc
@@ -132,14 +132,17 @@ void BtreeAllocator::_process_range_removal(uint64_t start, uint64_t end,
 
   // | left <|////|  right |
   if (left_over && right_over) {
-    // add the spin-off right seg
-    range_seg_t seg_after{end, seg_whole.end};
-    range_tree.emplace_hint(rs, seg_after.start, seg_after.end);
-    range_size_tree.emplace(seg_after);
     // shink the left seg in offset tree
+    // this should be done before calling any emplace/emplace_hint
+    // on range_tree since they invalidate rs iterator.
     rs->second = start;
     // insert the shrinked left seg back into size tree
     range_size_tree.emplace(seg_whole.start, start);
+
+    // add the spin-off right seg
+    range_seg_t seg_after{end, seg_whole.end};
+    range_tree.emplace_hint(rs, seg_after.start, seg_after.end);
+    range_size_tree.emplace(seg_after);
   } else if (left_over) {
     // | left <|///////////|
     // shrink the left seg in the offset tree
@@ -167,8 +170,11 @@ void BtreeAllocator::_remove_from_tree(uint64_t start, uint64_t size)
   ceph_assert(size != 0);
   ceph_assert(size <= num_free);
 
-  auto rs = range_tree.find(start);
-  /* Make sure we completely overlap with someone */
+  // Make sure we completely overlap with someone
+  auto rs = range_tree.lower_bound(start);
+  if ((rs == range_tree.end() || rs->first > start) && rs != range_tree.begin()) {
+    --rs;
+  }
   ceph_assert(rs != range_tree.end());
   ceph_assert(rs->first <= start);
   ceph_assert(rs->second >= end);
@@ -192,6 +198,11 @@ void BtreeAllocator::_try_remove_from_tree(uint64_t start, uint64_t size,
 
   do {
 
+    //FIXME: this is apparently wrong since _process_range_removal might
+    // invalidate existing iterators.
+    // Not a big deal so far since this method is not in use - it's called
+    // when making Hybrid allocator from a regular one. Which isn't an option
+    // for BtreeAllocator for now.
     auto next_rs = rs;
     ++next_rs;
 

diff --git a/src/test/objectstore/Allocator_bench.cc b/src/test/objectstore/Allocator_bench.cc
index 0d04a854e9aa94cef7b3a8d1d14f1b3a17c37675..76598d2a1b2630505e2a50812120172049248e90 100644 (file)
--- a/src/test/objectstore/Allocator_bench.cc
+++ b/src/test/objectstore/Allocator_bench.cc
@@ -365,4 +365,4 @@ TEST_P(AllocTest, mempoolAccounting)
 INSTANTIATE_TEST_SUITE_P(
   Allocator,
   AllocTest,
-  ::testing::Values("stupid", "bitmap", "avl", "btree", "hybrid"));
+  ::testing::Values("stupid", "bitmap", "avl", "hybrid", "btree"));

diff --git a/src/test/objectstore/Allocator_test.cc b/src/test/objectstore/Allocator_test.cc
index 186063aa7f799b8fdb396f7d6c3e31315bc96608..0e76c479002add06a4192377651a9ccc3196b7d1 100644 (file)
--- a/src/test/objectstore/Allocator_test.cc
+++ b/src/test/objectstore/Allocator_test.cc
@@ -631,7 +631,29 @@ TEST_P(AllocTest, test_alloc_50656_first_fit)
   EXPECT_EQ(got, 0x400000);
 }
 
+TEST_P(AllocTest, test_init_rm_free_unbound)
+{
+  int64_t block_size = 1024;
+  int64_t capacity = 4 * 1024 * block_size;
+
+  {
+    init_alloc(capacity, block_size);
+
+    alloc->init_add_free(0, block_size * 2);
+    alloc->init_add_free(block_size * 3, block_size * 3);
+    alloc->init_add_free(block_size * 7, block_size * 2);
+
+    alloc->init_rm_free(block_size * 4, block_size);
+    ASSERT_EQ(alloc->get_free(), block_size * 6);
+
+    auto cb = [&](size_t off, size_t len) {
+      cout << std::hex << "0x" << off << "~" << len << std::dec << std::endl;
+    };
+    alloc->foreach(cb);
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(
   Allocator,
   AllocTest,
-  ::testing::Values("stupid", "bitmap", "avl", "hybrid"));
+  ::testing::Values("stupid", "bitmap", "avl", "hybrid", "btree"));