crimson/os/seastore/seastore_types.h: clarify segment_id_t visibility, constants

Signed-off-by: Samuel Just <sjust@redhat.com>

diff --git a/src/crimson/os/seastore/seastore_types.h b/src/crimson/os/seastore/seastore_types.h
index b3517efcb0e1ccb1689386246fdc73169bff1959..40009f73dff51015d97fe4a35ba3bf243a46a91d 100644 (file)
--- a/src/crimson/os/seastore/seastore_types.h
+++ b/src/crimson/os/seastore/seastore_types.h
@@ -39,38 +39,65 @@ struct seastore_meta_t {
 // identifies a specific physical device within seastore
 using device_id_t = uint8_t;
 // order of device_id_t
-constexpr uint16_t device_id_len = 4;
+constexpr uint16_t DEVICE_ID_LEN_BITS = 4;
 // maximum devices supported
-constexpr uint16_t max_devices = 1 << device_id_len;
+constexpr uint16_t max_devices = 1 << DEVICE_ID_LEN_BITS;
 
 // segment ids without a device id encapsulated
 using device_segment_id_t = uint32_t;
 
-struct segment_id_le_t;
-
 // Identifies segment location on disk, see SegmentManager,
 struct segment_id_t {
 private:
   // internal segment id type of segment_id_t, basically
-  // this is a unsigned int with the top "device_id_len"
+  // this is a unsigned int with the top "DEVICE_ID_LEN_BITS"
   // bits representing the id of the device on which the
   // segment resides
   using internal_segment_id_t = uint32_t;
 
   // mask for segment manager id
   static constexpr internal_segment_id_t SM_ID_MASK =
-    0xF << (std::numeric_limits<internal_segment_id_t>::digits - device_id_len);
+    0xF << (std::numeric_limits<internal_segment_id_t>::digits - DEVICE_ID_LEN_BITS);
   // default internal segment id
   static constexpr internal_segment_id_t DEFAULT_INTERNAL_SEG_ID =
     std::numeric_limits<internal_segment_id_t>::max() - 1;
-public:
+
   internal_segment_id_t segment = DEFAULT_INTERNAL_SEG_ID;
 
+  constexpr segment_id_t(uint32_t encoded) : segment(encoded) {}
+public:
+  constexpr static segment_id_t make_max() {
+    return std::numeric_limits<internal_segment_id_t>::max();
+  }
+  constexpr static segment_id_t make_null() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 1;
+  }
+  /* Used to denote relative paddr_t */
+  constexpr static segment_id_t make_record_relative() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 2;
+  }
+  constexpr static segment_id_t make_block_relative() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 3;
+  }
+  // for tests which generate fake paddrs
+  constexpr static segment_id_t make_fake() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 4;
+  }
+
+  /* Used to denote references to notional zero filled segment, mainly
+   * in denoting reserved laddr ranges for unallocated object data.
+   */
+  constexpr static segment_id_t make_zero() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 5;
+  }
+  constexpr static segment_id_t make_delayed() {
+    return std::numeric_limits<internal_segment_id_t>::max() - 6;
+  }
+
+
   segment_id_t() = default;
-  constexpr segment_id_t(device_segment_id_t segment)
-    : segment(segment) {}
   segment_id_t(device_id_t id, device_segment_id_t segment)
-    : segment(add_device_id(segment, id)) {
+    : segment(make_internal(segment, id)) {
     // only lower 4 bits are effective, and we have to reserve 0x0F for
     // special XXX_SEG_IDs
     assert(id < 15);
@@ -78,12 +105,12 @@ public:
 
   [[gnu::always_inline]]
   device_id_t device_id() const {
-    return get_device_id(segment);
+    return internal_to_device(segment);
   }
 
   [[gnu::always_inline]]
   device_segment_id_t device_segment_id() const {
-    return strip_device_id(segment);
+    return internal_to_segment(segment);
   }
 
   bool operator==(const segment_id_t& other) const {
@@ -109,22 +136,26 @@ public:
     denc(v.segment, p);
   }
 private:
-  inline device_id_t get_device_id(internal_segment_id_t id) const {
-    return (device_id_t)((id & SM_ID_MASK) >>
-       (std::numeric_limits<internal_segment_id_t>::digits - device_id_len));
-  }
+  static constexpr unsigned segment_bits = (
+    std::numeric_limits<internal_segment_id_t>::digits - DEVICE_ID_LEN_BITS
+  );
 
-  inline internal_segment_id_t add_device_id(
-    device_segment_id_t id,
-    device_id_t sm_id) const {
-    return id + (sm_id <<
-       (std::numeric_limits<internal_segment_id_t>::digits - device_id_len));
+  static inline device_id_t internal_to_device(internal_segment_id_t id) {
+    return (static_cast<device_id_t>(id) & SM_ID_MASK) >> segment_bits;
   }
 
-  inline device_segment_id_t strip_device_id(internal_segment_id_t id) const {
+  static inline device_segment_id_t internal_to_segment(
+    internal_segment_id_t id) {
     return id & (~SM_ID_MASK);
   }
 
+  static inline internal_segment_id_t make_internal(
+    device_segment_id_t id,
+    device_id_t sm_id) {
+    return static_cast<internal_segment_id_t>(id) |
+      (static_cast<internal_segment_id_t>(sm_id) << segment_bits);
+  }
+
   friend struct segment_id_le_t;
 };
 
@@ -140,25 +171,17 @@ struct __attribute((packed)) segment_id_le_t {
   }
 };
 
-constexpr segment_id_t MAX_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max()};
-constexpr segment_id_t NULL_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 1};
-/* Used to denote relative paddr_t */
-constexpr segment_id_t RECORD_REL_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 2};
-constexpr segment_id_t BLOCK_REL_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 3};
+constexpr segment_id_t MAX_SEG_ID = segment_id_t::make_max();
+constexpr segment_id_t NULL_SEG_ID = segment_id_t::make_null();
+constexpr segment_id_t RECORD_REL_SEG_ID = segment_id_t::make_record_relative();
+constexpr segment_id_t BLOCK_REL_SEG_ID = segment_id_t::make_block_relative();
 // for tests which generate fake paddrs
-constexpr segment_id_t FAKE_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 4};
+constexpr segment_id_t FAKE_SEG_ID = segment_id_t::make_fake();
 /* Used to denote references to notional zero filled segment, mainly
  * in denoting reserved laddr ranges for unallocated object data.
  */
-constexpr segment_id_t ZERO_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 5};
-constexpr segment_id_t DELAYED_TEMP_SEG_ID =
-  {std::numeric_limits<device_segment_id_t>::max() - 6};
+constexpr segment_id_t ZERO_SEG_ID = segment_id_t::make_zero();
+constexpr segment_id_t DELAYED_TEMP_SEG_ID = segment_id_t::make_delayed();
 
 std::ostream &operator<<(std::ostream &out, const segment_id_t&);
 
@@ -312,7 +335,7 @@ struct paddr_t {
 WRITE_CMP_OPERATORS_2(paddr_t, segment, offset)
 WRITE_EQ_OPERATORS_2(paddr_t, segment, offset)
 constexpr paddr_t P_ADDR_NULL = paddr_t{};
-constexpr paddr_t P_ADDR_MIN = paddr_t{0, 0};
+constexpr paddr_t P_ADDR_MIN = paddr_t{ZERO_SEG_ID, 0};
 constexpr paddr_t P_ADDR_MAX = paddr_t{
   MAX_SEG_ID,
   MAX_SEG_OFF
@@ -390,7 +413,7 @@ WRITE_CMP_OPERATORS_2(journal_seq_t, segment_seq, offset)
 WRITE_EQ_OPERATORS_2(journal_seq_t, segment_seq, offset)
 constexpr journal_seq_t JOURNAL_SEQ_MIN{
   0,
-  paddr_t{0, 0}
+  paddr_t{ZERO_SEG_ID, 0}
 };
 constexpr journal_seq_t JOURNAL_SEQ_MAX{
   MAX_SEG_SEQ,

diff --git a/src/crimson/os/seastore/segment_cleaner.cc b/src/crimson/os/seastore/segment_cleaner.cc
index c68a97baf59ea0f1438efb4d611477893cc8faa6..c50dc6fb4a1b7ffcde35b0d976c94b967ed23f70 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.cc
+++ b/src/crimson/os/seastore/segment_cleaner.cc
@@ -212,7 +212,7 @@ SegmentCleaner::get_segment_ret SegmentCleaner::get_segment(device_id_t id)
   assert(0 == "out of space handling todo");
   return get_segment_ret(
     get_segment_ertr::ready_future_marker{},
-    0);
+    ZERO_SEG_ID);
 }
 
 void SegmentCleaner::update_journal_tail_target(journal_seq_t target)

diff --git a/src/crimson/os/seastore/segment_cleaner.h b/src/crimson/os/seastore/segment_cleaner.h
index 8fe3e60691710fab8dfb6afd2930c44e8c4c5636..014003c97f9fe65b925503100fc9f96e25ab860e 100644 (file)
--- a/src/crimson/os/seastore/segment_cleaner.h
+++ b/src/crimson/os/seastore/segment_cleaner.h
@@ -518,7 +518,6 @@ public:
     segment_id_t segment,
     segment_off_t offset,
     extent_len_t len) final {
-    assert(segment < segment_usage.size());
     return segment_usage[segment].segment_map.allocate(
       segment.device_segment_id(),
       offset,
@@ -538,7 +537,6 @@ public:
   }
 
   int64_t get_usage(segment_id_t segment) const final {
-    assert(segment < segment_usage.size());
     return segment_usage[segment].segment_map.get_usage();
   }
 

diff --git a/src/crimson/os/seastore/segment_manager/ephemeral.cc b/src/crimson/os/seastore/segment_manager/ephemeral.cc
index 244a0806c6c0717efa5311ae4f2d9a71146e9b88..32e3166fe0934bda3e10e10f6af22b95d961e28f 100644 (file)
--- a/src/crimson/os/seastore/segment_manager/ephemeral.cc
+++ b/src/crimson/os/seastore/segment_manager/ephemeral.cc
@@ -193,7 +193,7 @@ SegmentManager::read_ertr::future<> EphemeralSegmentManager::read(
   size_t len,
   ceph::bufferptr &out)
 {
-  if (addr.segment >= get_num_segments()) {
+  if (addr.segment.device_segment_id() >= get_num_segments()) {
     logger().error(
       "EphemeralSegmentManager::read: invalid segment {}",
       addr);
@@ -214,7 +214,7 @@ SegmentManager::read_ertr::future<> EphemeralSegmentManager::read(
   bl.push_back(out);
   logger().debug(
     "segment_read to segment {} at offset {}, physical offset {}, length {}, crc {}",
-    addr.segment,
+    addr.segment.device_segment_id(),
     addr.offset,
     get_offset(addr),
     len,

diff --git a/src/test/crimson/seastore/test_btree_lba_manager.cc b/src/test/crimson/seastore/test_btree_lba_manager.cc
index ff29c81eb1e204452c85550d6c76ec6a64c6da2d..806bbebdf461c6ebed8a40a554b9238ee2b8a969 100644 (file)
--- a/src/test/crimson/seastore/test_btree_lba_manager.cc
+++ b/src/test/crimson/seastore/test_btree_lba_manager.cc
@@ -36,20 +36,22 @@ struct btree_lba_manager_test :
 
   WritePipeline pipeline;
 
+  segment_id_t next;
+
   btree_lba_manager_test()
     : segment_manager(segment_manager::create_test_ephemeral()),
       scanner(new ExtentReader()),
       journal(*segment_manager, *scanner),
       cache(*scanner, segment_manager->get_block_size()),
       lba_manager(new BtreeLBAManager(*segment_manager, cache)),
-      block_size(segment_manager->get_block_size())
+      block_size(segment_manager->get_block_size()),
+      next(segment_manager->get_device_id(), 0)
   {
     scanner->add_segment_manager(segment_manager.get());
     journal.set_segment_provider(this);
     journal.set_write_pipeline(&pipeline);
   }
 
-  segment_id_t next = 0;
   get_segment_ret get_segment(device_id_t id) final {
     auto ret = next;
     next = segment_id_t{

diff --git a/src/test/crimson/seastore/test_seastore_cache.cc b/src/test/crimson/seastore/test_seastore_cache.cc
index 28688d9feed6e83e66641f6c82cdb6f160fddfae..8c8427aea389d3236e37f560bcfe03c0d3bcbdc4 100644 (file)
--- a/src/test/crimson/seastore/test_seastore_cache.cc
+++ b/src/test/crimson/seastore/test_seastore_cache.cc
@@ -23,13 +23,14 @@ struct cache_test_t : public seastar_test_suite_t {
   segment_manager::EphemeralSegmentManagerRef segment_manager;
   ExtentReaderRef reader;
   Cache cache;
-  paddr_t current{0, 0};
+  paddr_t current;
   journal_seq_t seq;
 
   cache_test_t()
     : segment_manager(segment_manager::create_test_ephemeral()),
       reader(new ExtentReader()),
-      cache(*reader, segment_manager->get_block_size()) {
+      cache(*reader, segment_manager->get_block_size()),
+      current(segment_id_t(segment_manager->get_device_id(), 0), 0) {
     reader->add_segment_manager(segment_manager.get());
   }
 

diff --git a/src/test/crimson/seastore/test_seastore_journal.cc b/src/test/crimson/seastore/test_seastore_journal.cc
index 7f42d3889efa3c55e5db14d7045775bad03bd2e0..53679f174a1fa4f9738611777c2a03ad00293d55 100644 (file)
--- a/src/test/crimson/seastore/test_seastore_journal.cc
+++ b/src/test/crimson/seastore/test_seastore_journal.cc
@@ -76,15 +76,17 @@ struct journal_test_t : seastar_test_suite_t, SegmentProvider {
 
   ExtentReaderRef scanner;
 
+  segment_id_t next;
+
   journal_test_t()
     : segment_manager(segment_manager::create_test_ephemeral()),
       block_size(segment_manager->get_block_size()),
-      scanner(new ExtentReader())
+      scanner(new ExtentReader()),
+      next(segment_manager->get_device_id(), 0)
   {
     scanner->add_segment_manager(segment_manager.get());
   }
 
-  segment_id_t next = 0;
   get_segment_ret get_segment(device_id_t id) final {
     auto ret = next;
     next = segment_id_t{
@@ -127,10 +129,14 @@ struct journal_test_t : seastar_test_suite_t, SegmentProvider {
          boost::make_counting_iterator(device_segment_id_t{
            segment_manager->get_num_segments()}),
          [this, &segments](auto segment_id) {
-         return scanner->read_segment_header(segment_id)
+         return scanner->read_segment_header(segment_id_t{0, segment_id})
          .safe_then([&segments, segment_id](auto header) {
            if (!header.out_of_line) {
-             segments.emplace_back(std::make_pair(segment_id, std::move(header)));
+             segments.emplace_back(
+               std::make_pair(
+                 segment_id_t{0, segment_id},
+                 std::move(header)
+               ));
            }
            return seastar::now();
          }).handle_error(

diff --git a/src/test/crimson/seastore/test_transaction_manager.cc b/src/test/crimson/seastore/test_transaction_manager.cc
index 75d1903722502d0d88968252997135b5f9ca12b6..b1c85584cb6ca746a32275fa6a68bfdab5c655af 100644 (file)
--- a/src/test/crimson/seastore/test_transaction_manager.cc
+++ b/src/test/crimson/seastore/test_transaction_manager.cc
@@ -392,7 +392,7 @@ struct transaction_manager_test_t :
          t,
          [&tracker](auto offset, auto len) {
            tracker->allocate(
-             offset.segment.segment,
+             offset.segment,
              offset.offset,
              len);
          });