From 7dfa20863090d5eb58c798b6903386dcce6a52f8 Mon Sep 17 00:00:00 2001 From: Gabriel BenHanokh Date: Thu, 31 Mar 2022 14:47:59 +0300 Subject: [PATCH] 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 --- src/os/bluestore/simple_bitmap.cc | 14 ++++++--- src/os/bluestore/simple_bitmap.h | 3 ++ src/test/objectstore/test_bluestore_types.cc | 33 ++++++++++++++++++++ 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/os/bluestore/simple_bitmap.cc b/src/os/bluestore/simple_bitmap.cc index 38255418865b..fb12162b9e84 100644 --- 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 058886323b65..5d9d560217b6 100644 --- 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 8a81c172ef4b..c348e5138c87 100644 --- 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); -- 2.47.3