From 995ba2c4576f3de0ebfebccf9669d5162fab9fa6 Mon Sep 17 00:00:00 2001 From: Adam Kupczyk Date: Thu, 8 Feb 2024 23:53:25 +0000 Subject: [PATCH] os/bluestore: Minor fixes These were noticed during review. Signed-off-by: Adam Kupczyk (cherry picked from commit e1cc40b133bb6025aff2e99a722ad01d20b67fb5) --- src/os/bluestore/BlueStore.cc | 55 +++++++++++++++++------------ src/os/bluestore/BlueStore.h | 6 ++-- src/os/bluestore/bluestore_common.h | 2 +- src/test/objectstore/store_test.cc | 2 +- 4 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc index 4ba67eed580..30642999169 100644 --- a/src/os/bluestore/BlueStore.cc +++ b/src/os/bluestore/BlueStore.cc @@ -136,7 +136,7 @@ const string BLUESTORE_GLOBAL_STATFS_KEY = "bluestore_statfs"; // were already used so labels won't exist there. static constexpr uint64_t _1G = uint64_t(1024)*1024*1024; const vector bdev_label_positions = { - BDEV_LABEL_POSITION, + BDEV_FIRST_LABEL_POSITION, _1G, 10*_1G, 100*_1G, @@ -6527,16 +6527,26 @@ int BlueStore::_write_bdev_label( VOID_TEMP_FAILURE_RETRY(::close(fd)); return r; } + int failed_r = 0; + bool wrote_at_least_one = false; for (uint64_t position : locations) { if (int64_t(position + BDEV_LABEL_BLOCK_SIZE) <= st.st_size) { r = bl.write_fd(fd, position); - if (r < 0) { + if (r == 0) { + wrote_at_least_one = true; + } else { derr << __func__ << " failed to write to " << path + << " at location 0x" << std::hex << position << std::dec << ": " << cpp_strerror(r) << dendl; - goto out; + failed_r = r; } } } + if (!wrote_at_least_one) { + derr << __func__ << " failed to write to any bdev of locations" << dendl; + r = failed_r; + goto out; + } r = ::fsync(fd); if (r < 0) { derr << __func__ << " failed to fsync " << path @@ -6651,7 +6661,7 @@ int BlueStore::_read_main_bdev_label( if (r == 0 && (fsid.is_zero() || label.osd_uuid == fsid)) { auto i = label.meta.find("multi"); bool is_multi = i != label.meta.end() && i->second == "yes"; - if (position == BDEV_LABEL_POSITION && !is_multi) { + if (position == BDEV_FIRST_LABEL_POSITION && !is_multi) { // we have a single-label case *out_label = label; is_multi = false; @@ -6738,6 +6748,10 @@ void BlueStore::_main_bdev_label_try_reserve() } } }; + // Iterating over free is very inefficient. + // We can do it here only because its only on init, otherwise it would be unacceptable. + // Here we could use some API like: alloc->allocate_at(). + // When we create it, replace code. alloc->foreach(look_for_bdev); for (auto& location : accepted_positions) { alloc->init_rm_free(location, lsize); @@ -6755,15 +6769,14 @@ void BlueStore::_main_bdev_label_remove(Allocator* an_alloc) { ceph_assert(bdev_label_multi == true); uint64_t lsize = std::max(BDEV_LABEL_BLOCK_SIZE, min_alloc_size); - for (size_t location : bdev_label_valid_locations) { - if (location != BDEV_LABEL_POSITION) + if (location != BDEV_FIRST_LABEL_POSITION) an_alloc->init_add_free(location, lsize); } } int BlueStore::_check_or_set_bdev_label( - const string& path, uint64_t size, string desc, bool create) + const string& path, uint64_t size, const string& desc, bool create) { bluestore_bdev_label_t label; if (create) { @@ -6810,7 +6823,7 @@ int BlueStore::_set_main_bdev_label() } } } else { - bdev_label_valid_locations.push_back(BDEV_LABEL_POSITION); + bdev_label_valid_locations.push_back(BDEV_FIRST_LABEL_POSITION); } int r = _write_bdev_label(cct, path + "/block", label, bdev_label_valid_locations); if (r < 0) @@ -6833,11 +6846,9 @@ int BlueStore::_check_main_bdev_label() << " does not match our fsid " << fsid << dendl; return -EIO; } - if (bluestore_bdev_label_require_all) { - if (r != 0) { - derr << __func__ << "not all labels read properly" << dendl; - return -EIO; - } + if (bluestore_bdev_label_require_all && r != 0) { + derr << __func__ << " not all labels read properly" << dendl; + return -EIO; } return 0; } @@ -6994,7 +7005,7 @@ int BlueStore::_open_fm(KeyValueDB::Transaction t, fm->allocate(0, bdev->get_size(), t); } else { // allocate bdev label + bluefs superblock reserved space. - fm->allocate(BDEV_LABEL_POSITION, reserved, t); + fm->allocate(BDEV_FIRST_LABEL_POSITION, reserved, t); // we do not mark other label positions } r = _write_out_fm_meta(0); @@ -8381,7 +8392,7 @@ int BlueStore::mkfs() // full free alloc->init_add_free(0, p2align(bdev->get_size(), min_alloc_size)); // allocate bdev label + bluefs superblock reserved space. - alloc->init_rm_free(BDEV_LABEL_POSITION, reserved); + alloc->init_rm_free(BDEV_FIRST_LABEL_POSITION, reserved); // take possible bdev locations, so it will not be used if (cct->_conf.get_val("bluestore_bdev_label_multi")) { @@ -10376,7 +10387,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair) bdev_label.meta["multi"] = "yes"; bdev_label.meta["epoch"] = "1"; bdev_label_multi = true; - bdev_labels_broken.push_back(BDEV_LABEL_POSITION); + bdev_labels_broken.push_back(BDEV_FIRST_LABEL_POSITION); errors++; } if (bdev->supported_bdev_label() && bdev_label_multi) { @@ -10433,7 +10444,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair) bool is_taken_by_bluefs = false; apply_for_bitset_range(position, length, alloc_size, bluefs_used_blocks, [&](uint64_t pos, mempool_dynamic_bitset& bs) { - is_taken_by_bluefs |= bs.test_set(pos); + is_taken_by_bluefs |= bs.test(pos); } ); if (is_taken_by_bluefs) { @@ -10447,7 +10458,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair) } } } - // Mark bits or locations of all bdev labels. + // Mark locations of those bdev labels that are not taken by bluefs. for (size_t i = 0; i < bdev_label_positions.size(); i++) { uint64_t position = bdev_label_positions[i]; uint64_t length = std::max(BDEV_LABEL_BLOCK_SIZE, alloc_size); @@ -10462,7 +10473,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair) } apply_for_bitset_range( - BDEV_LABEL_POSITION, std::max(min_alloc_size, SUPER_RESERVED), alloc_size, used_blocks, + BDEV_FIRST_LABEL_POSITION, std::max(min_alloc_size, SUPER_RESERVED), alloc_size, used_blocks, [&](uint64_t pos, mempool_dynamic_bitset &bs) { bs.set(pos); } @@ -11030,7 +11041,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair) //unmark extra bdev copies, will collide with the check for (uint64_t location : bdev_label_valid_locations) { uint64_t length = std::max(BDEV_LABEL_BLOCK_SIZE, alloc_size); - if (location != BDEV_LABEL_POSITION) { + if (location != BDEV_FIRST_LABEL_POSITION) { apply_for_bitset_range(location, length, alloc_size, used_blocks, [&](uint64_t pos, mempool_dynamic_bitset& bs) { bs.reset(pos); @@ -13322,9 +13333,9 @@ ObjectMap::ObjectMapIterator BlueStore::get_omap_iterator( // write helpers uint64_t BlueStore::_get_ondisk_reserved() const { - static_assert(BDEV_LABEL_POSITION == 0); + static_assert(BDEV_FIRST_LABEL_POSITION == 0); ceph_assert(min_alloc_size); - uint64_t size = p2roundup(BDEV_LABEL_BLOCK_SIZE + BLUEFS_SUPER_BLOCK_SIZE, min_alloc_size); + uint64_t size = p2roundup(SUPER_RESERVED, min_alloc_size); return size; } diff --git a/src/os/bluestore/BlueStore.h b/src/os/bluestore/BlueStore.h index 6894a75b737..f348584f7c6 100644 --- a/src/os/bluestore/BlueStore.h +++ b/src/os/bluestore/BlueStore.h @@ -2771,12 +2771,12 @@ public: CephContext* cct, const std::string &path, bluestore_bdev_label_t label, - std::vector locations = std::vector({BDEV_LABEL_POSITION})); + std::vector locations = std::vector({BDEV_FIRST_LABEL_POSITION})); static int _read_bdev_label( CephContext* cct, const std::string &path, - bluestore_bdev_label_t *label, uint64_t disk_position = BDEV_LABEL_POSITION); + bluestore_bdev_label_t *label, uint64_t disk_position = BDEV_FIRST_LABEL_POSITION); private: - int _check_or_set_bdev_label(const std::string& path, uint64_t size, std::string desc, + int _check_or_set_bdev_label(const std::string& path, uint64_t size, const std::string& desc, bool create); int _set_main_bdev_label(); int _check_main_bdev_label(); diff --git a/src/os/bluestore/bluestore_common.h b/src/os/bluestore/bluestore_common.h index 6b64fc50547..42a951279ef 100644 --- a/src/os/bluestore/bluestore_common.h +++ b/src/os/bluestore/bluestore_common.h @@ -65,7 +65,7 @@ struct Int64ArrayMergeOperator : public KeyValueDB::MergeOperator { // write a label in the first block. always use this size. note that // bluefs makes a matching assumption about the location of its // superblock (always the second block of the device). -static constexpr uint64_t BDEV_LABEL_POSITION = 0; +static constexpr uint64_t BDEV_FIRST_LABEL_POSITION = 0; static constexpr uint64_t BDEV_LABEL_BLOCK_SIZE = 4096; // reserved for standalone DB volume: diff --git a/src/test/objectstore/store_test.cc b/src/test/objectstore/store_test.cc index 4e61bade6a4..9674fdd3014 100644 --- a/src/test/objectstore/store_test.cc +++ b/src/test/objectstore/store_test.cc @@ -10647,7 +10647,7 @@ TEST_P(MultiLabelTest, UpgradeToMultiLabelCollisionWithBlueFS) { ASSERT_EQ(label.meta["multi"], "yes"); } -TEST_P(MultiLabelTest, UpgradeToMultiLabelCollisionObjects) { +TEST_P(MultiLabelTest, UpgradeToMultiLabelCollisionWithObjects) { static constexpr uint64_t _1G = uint64_t(1024)*1024*1024; static constexpr uint64_t _1M = uint64_t(1)*1024*1024; SetVal(g_conf(), "bluestore_block_db_create", "true"); -- 2.39.5