os/BlueStore: NCB fix for SimpleBitmap boundary check

The boundary check in SimpleBitmap is off by one causing an assert to trigger
Also fixed a bug when asking for the next clear_extent on a unaligned map when the last bits in the map were set.
Adding unit-tests
Fixes: https://tracker.ceph.com/issues/55145
Signed-off-by: Gabriel BenHanokh <gbenhano@redhat.com>

diff --git a/src/os/bluestore/simple_bitmap.cc b/src/os/bluestore/simple_bitmap.cc
index 38255418865bd437d5863490bf3c88c8ccc6f451..fb12162b9e84d6769a40895ed23725ec160e279a 100644 (file)
--- a/src/os/bluestore/simple_bitmap.cc
+++ b/src/os/bluestore/simple_bitmap.cc
@@ -49,9 +49,9 @@ bool SimpleBitmap::set(uint64_t offset, uint64_t length)
 {
   dout(20) <<" [" << std::hex << offset << ", " << length << "]" << dendl;
 
-  if (offset + length >= m_num_bits) {
+  if (offset + length > m_num_bits) {
     derr << __func__ << "::offset + length = " << offset + length << " exceeds map size = " << m_num_bits << dendl;
-    ceph_assert(offset + length < m_num_bits);
+    ceph_assert(offset + length <= m_num_bits);
     return false;
   }
 
@@ -103,9 +103,9 @@ bool SimpleBitmap::set(uint64_t offset, uint64_t length)
 //----------------------------------------------------------------------------
 bool SimpleBitmap::clr(uint64_t offset, uint64_t length)
 {
-  if (offset + length >= m_num_bits) {
+  if (offset + length > m_num_bits) {
     derr << __func__ << "::offset + length = " << offset + length << " exceeds map size = " << m_num_bits << dendl;
-    ceph_assert(offset + length < m_num_bits);
+    ceph_assert(offset + length <= m_num_bits);
     return false;
   }
 
@@ -186,6 +186,9 @@ extent_t SimpleBitmap::get_next_set_extent(uint64_t offset)
   int           ffs = __builtin_ffsll(word) - 1;
   extent_t      ext;
   ext.offset = words_to_bits(word_idx) + ffs;
+  if (ext.offset >= m_num_bits ) {
+    return null_extent;
+  }
 
   // set all bits from current to LSB
   uint64_t      clr_mask = FULL_MASK << ffs;
@@ -245,6 +248,9 @@ extent_t SimpleBitmap::get_next_clr_extent(uint64_t offset)
   int      ffz = __builtin_ffsll(~word) - 1;
   extent_t ext;
   ext.offset = words_to_bits(word_idx) + ffz;
+  if (ext.offset >= m_num_bits ) {
+    return null_extent;
+  }
 
   // clear all bits from current position to LSB
   word &= (FULL_MASK << ffz);

diff --git a/src/os/bluestore/simple_bitmap.h b/src/os/bluestore/simple_bitmap.h
index 058886323b6591a49761e6ee9f40496ecd452475..5d9d560217b6d6cb25778af9d7f0573cfdf72527 100644 (file)
--- a/src/os/bluestore/simple_bitmap.h
+++ b/src/os/bluestore/simple_bitmap.h
@@ -24,6 +24,9 @@
 struct extent_t {
   uint64_t offset;
   uint64_t length;
+  bool operator==(const extent_t& other) const {
+    return (this->offset == other.offset && this->length == other.length);
+  }
 };
 
 class SimpleBitmap {

diff --git a/src/test/objectstore/test_bluestore_types.cc b/src/test/objectstore/test_bluestore_types.cc
index 8a81c172ef4b5e1f8df802b22216c0bbfc98bc08..c348e5138c8700f09d3eabc5ca93f920fe7e0211 100644 (file)
--- a/src/test/objectstore/test_bluestore_types.cc
+++ b/src/test/objectstore/test_bluestore_types.cc
@@ -2149,6 +2149,39 @@ TEST(SimpleBitmap, boundaries)
   }
 }
 
+//---------------------------------------------------------------------------------
+TEST(SimpleBitmap, boundaries2)
+{
+  const uint64_t bit_count_base = 64 << 10; // 64Kb = 8MB
+  const extent_t null_extent    = {0, 0};
+
+  for (unsigned i = 0; i < 64; i++) {
+    uint64_t     bit_count   = bit_count_base + i;
+    extent_t     full_extent = {0, bit_count};
+    SimpleBitmap sbmap(g_ceph_context, bit_count);
+
+    sbmap.set(0, bit_count);
+    ASSERT_TRUE(sbmap.get_next_set_extent(0) == full_extent);
+    ASSERT_TRUE(sbmap.get_next_clr_extent(0) == null_extent);
+
+    for (uint64_t bit = 0; bit < bit_count; bit++) {
+      sbmap.clr(bit, 1);
+    }
+    ASSERT_TRUE(sbmap.get_next_set_extent(0) == null_extent);
+    ASSERT_TRUE(sbmap.get_next_clr_extent(0) == full_extent);
+
+    for (uint64_t bit = 0; bit < bit_count; bit++) {
+      sbmap.set(bit, 1);
+    }
+    ASSERT_TRUE(sbmap.get_next_set_extent(0) == full_extent);
+    ASSERT_TRUE(sbmap.get_next_clr_extent(0) == null_extent);
+
+    sbmap.clr(0, bit_count);
+    ASSERT_TRUE(sbmap.get_next_set_extent(0) == null_extent);
+    ASSERT_TRUE(sbmap.get_next_clr_extent(0) == full_extent);
+  }
+}
+
 TEST(shared_blob_2hash_tracker_t, basic_test)
 {
   shared_blob_2hash_tracker_t t1(1024 * 1024, 4096);