crimson/os/seastore: fix record metrics in cache

Signed-off-by: Yingxin Cheng <[email protected]>

src/crimson/os/seastore/cache.cc

@@ -273,9 +273,9 @@ void Cache::register_metrics()
             {src_label}
           ),
           sm::make_counter(
-            "invalidated_ool_record_overhead_bytes",
-            efforts.ool_record_overhead_bytes,
-            sm::description("bytes of ool-record overhead from invalidated transactions"),
+            "invalidated_ool_record_bytes",
+            efforts.ool_record_bytes,
+            sm::description("bytes of ool-record from invalidated transactions"),
             {src_label}
           ),
         }
@@ -313,15 +313,28 @@ void Cache::register_metrics()
             {src_label}
           ),
           sm::make_counter(
-            "committed_ool_record_overhead_bytes",
-            efforts.ool_record_overhead_bytes,
-            sm::description("bytes of ool-record overhead from committed transactions"),
+            "committed_ool_record_padding_bytes",
+            efforts.ool_record_padding_bytes,
+            sm::description("bytes of ool-record padding from committed transactions"),
             {src_label}
           ),
           sm::make_counter(
-            "committed_inline_record_overhead_bytes",
-            efforts.inline_record_overhead_bytes,
-            sm::description("bytes of inline-record overhead from committed transactions"),
+            "committed_ool_record_metadata_bytes",
+            efforts.ool_record_metadata_bytes,
+            sm::description("bytes of ool-record metadata from committed transactions"),
+            {src_label}
+          ),
+          sm::make_counter(
+            "committed_ool_record_data_bytes",
+            efforts.ool_record_data_bytes,
+            sm::description("bytes of ool-record data from committed transactions"),
+            {src_label}
+          ),
+          sm::make_counter(
+            "committed_inline_record_metadata_bytes",
+            efforts.inline_record_metadata_bytes,
+            sm::description("bytes of inline-record metadata from committed transactions"
+                            "(excludes delta buffer)"),
             {src_label}
           ),
         }
@@ -406,52 +419,6 @@ void Cache::register_metrics()
     );
   }
 
-  /**
-   * Record header fullness
-   */
-  auto record_type_label = sm::label("record_type");
-  using namespace std::literals::string_view_literals;
-  const string_view record_type_names[] = {
-    "INLINE"sv,
-    "OOL"sv,
-  };
-  for (auto& [src, src_label] : labels_by_src) {
-    if (src == src_t::READ) {
-      // READ transaction won't commit
-      continue;
-    }
-    auto& record_header_fullness = get_by_src(
-        stats.record_header_fullness_by_src, src);
-    for (auto& record_type_name : record_type_names) {
-      auto& fill_stat = [&record_type_name,
-                         &record_header_fullness]() -> fill_stat_t& {
-        if (record_type_name == "INLINE") {
-          return record_header_fullness.inline_stats;
-        } else {
-          assert(record_type_name == "OOL");
-          return record_header_fullness.ool_stats;
-        }
-      }();
-      metrics.add_group(
-        "cache",
-        {
-          sm::make_counter(
-            "record_header_filled_bytes",
-            fill_stat.filled_bytes,
-            sm::description("filled bytes of record header"),
-            {src_label, record_type_label(record_type_name)}
-          ),
-          sm::make_counter(
-            "record_header_total_bytes",
-            fill_stat.total_bytes,
-            sm::description("total bytes of record header"),
-            {src_label, record_type_label(record_type_name)}
-          ),
-        }
-      );
-    }
-  }
-
   /**
    * Cached extents (including placeholders)
    *
@@ -793,12 +760,9 @@ void Cache::mark_transaction_conflicted(
     efforts.fresh_ool_written.extents += ool_stats.extents.num;
     efforts.fresh_ool_written.bytes += ool_stats.extents.bytes;
     efforts.num_ool_records += ool_stats.num_records;
-    efforts.ool_record_overhead_bytes += ool_stats.header_bytes;
-
-    auto& record_header_fullness = get_by_src(
-        stats.record_header_fullness_by_src, t.get_src());
-    record_header_fullness.ool_stats.filled_bytes += ool_stats.header_raw_bytes;
-    record_header_fullness.ool_stats.total_bytes += ool_stats.header_bytes;
+    efforts.ool_record_bytes +=
+      (ool_stats.header_bytes + ool_stats.data_bytes);
+    // Note: we only account overhead from committed ool records
 
     if (t.get_src() == Transaction::src_t::CLEANER_TRIM ||
         t.get_src() == Transaction::src_t::CLEANER_RECLAIM) {
@@ -1088,23 +1052,13 @@ record_t Cache::prepare_record(Transaction &t)
         ).increment(t.lba_tree_stats);
 
     ++(efforts.num_trans);
-
     efforts.num_ool_records += ool_stats.num_records;
-    efforts.ool_record_overhead_bytes += ool_stats.header_bytes;
-
-    auto& record_header_fullness = get_by_src(
-        stats.record_header_fullness_by_src, t.get_src());
-    record_header_fullness.ool_stats.filled_bytes += ool_stats.header_raw_bytes;
-    record_header_fullness.ool_stats.total_bytes += ool_stats.header_bytes;
-
-    // TODO: move to Journal to get accurate result
-    auto record_size = record_group_size_t(
-        record.size, reader.get_block_size());
-    auto inline_overhead =
-        record_size.get_encoded_length() - record.get_raw_data_size();
-    efforts.inline_record_overhead_bytes += inline_overhead;
-    record_header_fullness.inline_stats.filled_bytes += record_size.get_raw_mdlength();
-    record_header_fullness.inline_stats.total_bytes += record_size.get_mdlength();
+    efforts.ool_record_padding_bytes +=
+      (ool_stats.header_bytes - ool_stats.header_raw_bytes);
+    efforts.ool_record_metadata_bytes += ool_stats.header_raw_bytes;
+    efforts.ool_record_data_bytes += ool_stats.data_bytes;
+    efforts.inline_record_metadata_bytes +=
+      (record.size.get_raw_mdlength() - record.get_delta_size());
   }
 
   return record;

src/crimson/os/seastore/cache.h

@@ -734,7 +734,7 @@ class Cache {
     effort_t fresh_ool_written;
     counter_by_extent_t<uint64_t> num_trans_invalidated;
     uint64_t num_ool_records = 0;
-    uint64_t ool_record_overhead_bytes = 0;
+    uint64_t ool_record_bytes = 0;
   };
 
   struct commit_trans_efforts_t {
@@ -747,8 +747,10 @@ class Cache {
     counter_by_extent_t<effort_t> fresh_ool_by_ext;
     uint64_t num_trans = 0; // the number of inline records
     uint64_t num_ool_records = 0;
-    uint64_t ool_record_overhead_bytes = 0;
-    uint64_t inline_record_overhead_bytes = 0;
+    uint64_t ool_record_padding_bytes = 0;
+    uint64_t ool_record_metadata_bytes = 0;
+    uint64_t ool_record_data_bytes = 0;
+    uint64_t inline_record_metadata_bytes = 0; // metadata exclude the delta bytes
   };
 
   struct success_read_trans_efforts_t {
@@ -769,16 +771,6 @@ class Cache {
   template <typename CounterT>
   using counter_by_src_t = std::array<CounterT, Transaction::SRC_MAX>;
 
-  struct fill_stat_t {
-    uint64_t filled_bytes = 0;
-    uint64_t total_bytes = 0;
-  };
-
-  struct record_header_fullness_t {
-    fill_stat_t inline_stats;
-    fill_stat_t ool_stats;
-  };
-
   static constexpr std::size_t NUM_SRC_COMB =
       Transaction::SRC_MAX * (Transaction::SRC_MAX + 1) / 2;
 
@@ -787,7 +779,6 @@ class Cache {
     counter_by_src_t<commit_trans_efforts_t> committed_efforts_by_src;
     counter_by_src_t<invalid_trans_efforts_t> invalidated_efforts_by_src;
     counter_by_src_t<query_counters_t> cache_query_by_src;
-    counter_by_src_t<record_header_fullness_t> record_header_fullness_by_src;
     success_read_trans_efforts_t success_read_efforts;
     uint64_t dirty_bytes = 0;
 

src/crimson/os/seastore/extent_placement_manager.cc

@@ -94,6 +94,7 @@ SegmentedAllocator::Writer::_write(
   stats.extents.bytes += record_size.dlength;
   stats.header_raw_bytes += record_size.get_raw_mdlength();
   stats.header_bytes += record_size.get_mdlength();
+  stats.data_bytes += record_size.dlength;
   stats.num_records += 1;
 
   return trans_intr::make_interruptible(

src/crimson/os/seastore/seastore_types.cc

@@ -154,6 +154,14 @@ std::ostream &operator<<(std::ostream &out, const segment_header_t &header)
 	     << ")";
 }
 
+extent_len_t record_size_t::get_raw_mdlength() const
+{
+  // FIXME: prevent submitting empty records
+  // assert(!is_empty());
+  return plain_mdlength +
+         ceph::encoded_sizeof_bounded<record_header_t>();
+}
+
 void record_size_t::account_extent(extent_len_t extent_len)
 {
   assert(extent_len);
@@ -183,10 +191,7 @@ void record_group_size_t::account(
   assert(_block_size > 0);
   assert(rsize.dlength % _block_size == 0);
   assert(block_size == 0 || block_size == _block_size);
-  plain_mdlength += (
-      rsize.plain_mdlength +
-      ceph::encoded_sizeof_bounded<record_header_t>()
-  );
+  plain_mdlength += rsize.get_raw_mdlength();
   dlength += rsize.dlength;
   block_size = _block_size;
 }

src/crimson/os/seastore/seastore_types.h

@@ -1196,6 +1196,8 @@ struct record_size_t {
   extent_len_t plain_mdlength = 0; // mdlength without the record header
   extent_len_t dlength = 0;
 
+  extent_len_t get_raw_mdlength() const;
+
   bool is_empty() const {
     return plain_mdlength == 0 &&
            dlength == 0;
@@ -1231,15 +1233,14 @@ struct record_t {
            deltas.size() == 0;
   }
 
-  // the size of extents and delta buffers
-  std::size_t get_raw_data_size() const {
+  std::size_t get_delta_size() const {
     auto delta_size = std::accumulate(
         deltas.begin(), deltas.end(), 0,
         [](uint64_t sum, auto& delta) {
           return sum + delta.bl.length();
         }
     );
-    return size.dlength + delta_size;
+    return delta_size;
   }
 
   void push_back(extent_t&& extent) {

src/crimson/os/seastore/transaction.h

@@ -320,12 +320,14 @@ class Transaction {
     io_stat_t extents;
     uint64_t header_raw_bytes = 0;
     uint64_t header_bytes = 0;
+    uint64_t data_bytes = 0;
     uint64_t num_records = 0;
 
     bool is_clear() const {
       return (extents.is_clear() &&
               header_raw_bytes == 0 &&
               header_bytes == 0 &&
+              data_bytes == 0 &&
               num_records == 0);
     }
   };