os/bluestore: Minor fixes

These were noticed during review.

Signed-off-by: Adam Kupczyk <akupczyk@ibm.com>

diff --git a/src/os/bluestore/BlueStore.cc b/src/os/bluestore/BlueStore.cc
index d213a081ddd931be7b577b91705597d3e0d057ed..24d2a5814a29f0067a1301185749f90c093f65c5 100644 (file)
--- a/src/os/bluestore/BlueStore.cc
+++ b/src/os/bluestore/BlueStore.cc
@@ -137,7 +137,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<uint64_t> bdev_label_positions = {
-  BDEV_LABEL_POSITION,
+  BDEV_FIRST_LABEL_POSITION,
   _1G,
   10*_1G,
   100*_1G,
@@ -6544,16 +6544,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
@@ -6668,7 +6678,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;
@@ -6755,6 +6765,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);
@@ -6772,15 +6786,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) {
@@ -6827,7 +6840,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)
@@ -6850,11 +6863,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;
 }
@@ -7011,7 +7022,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);
@@ -8423,7 +8434,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<bool>("bluestore_bdev_label_multi")) {
@@ -10580,7 +10591,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) {
@@ -10637,7 +10648,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) {
@@ -10651,7 +10662,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<uint64_t>(BDEV_LABEL_BLOCK_SIZE, alloc_size);
@@ -10666,7 +10677,7 @@ int BlueStore::_fsck_on_open(BlueStore::FSCKDepth depth, bool repair)
   }
 
   apply_for_bitset_range(
-    BDEV_LABEL_POSITION, std::max<uint64_t>(min_alloc_size, SUPER_RESERVED), alloc_size, used_blocks,
+    BDEV_FIRST_LABEL_POSITION, std::max<uint64_t>(min_alloc_size, SUPER_RESERVED), alloc_size, used_blocks,
     [&](uint64_t pos, mempool_dynamic_bitset &bs) {
       bs.set(pos);
     }
@@ -11261,7 +11272,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<uint64_t>(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);
@@ -13582,9 +13593,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 386154998c6ad7a6d6b3ebf0486826b9522c6455..591bd23af98fd2f36b99485acedfc6927c70c1fb 100644 (file)
--- a/src/os/bluestore/BlueStore.h
+++ b/src/os/bluestore/BlueStore.h
@@ -2758,12 +2758,12 @@ public:
     CephContext* cct,
     const std::string &path,
     bluestore_bdev_label_t label,
-    std::vector<uint64_t> locations = std::vector<uint64_t>({BDEV_LABEL_POSITION}));
+    std::vector<uint64_t> locations = std::vector<uint64_t>({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 6b64fc50547a247876e121a18370f0eed9deaff5..42a951279ef5e3ae59f92a8e30ac994625b83722 100644 (file)
--- 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 77cbc226fa3f69c2550edee0795df4daf7d4d91b..b5a56ac06aad8f41d4c72a944e1f4484806e2a8d 100644 (file)
--- a/src/test/objectstore/store_test.cc
+++ b/src/test/objectstore/store_test.cc
@@ -11169,7 +11169,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");