Merge pull request ceph#62838 from cyx1231st/wip-seastore-simplify-cache-access-metrics

crimson/os/seastore: simplify cache access metrics

Reviewed-by: Xuehan Xu <[email protected]>

Author: cyx1231st

src/crimson/os/seastore/backref/btree_backref_manager.cc

@@ -36,12 +36,10 @@ const get_phy_tree_root_node_ret get_phy_tree_root_node<
       return {true,
               c.cache.get_extent_viewable_by_trans(c.trans, backref_root)};
     } else {
-      c.cache.account_absent_access(c.trans.get_src());
       return {false,
               Cache::get_extent_iertr::make_ready_future<CachedExtentRef>()};
     }
   } else {
-    c.cache.account_absent_access(c.trans.get_src());
     return {false,
             Cache::get_extent_iertr::make_ready_future<CachedExtentRef>()};
   }

src/crimson/os/seastore/btree/fixed_kv_btree.h

@@ -1503,7 +1503,6 @@ class FixedKVBtree {
         return on_found(child->template cast<internal_node_t>());
       });
     }
-    c.cache.account_absent_access(c.trans.get_src());
 
     auto child_pos = v.get_child_pos();
     auto next_iter = node_iter + 1;
@@ -1575,7 +1574,6 @@ class FixedKVBtree {
         return on_found(child->template cast<leaf_node_t>());
       });
     }
-    c.cache.account_absent_access(c.trans.get_src());
 
     auto child_pos = v.get_child_pos();
     auto next_iter = node_iter + 1;
@@ -2135,7 +2133,6 @@ class FixedKVBtree {
         return do_merge(child->template cast<NodeType>());
       });
     }
-    c.cache.account_absent_access(c.trans.get_src());
 
     auto child_pos = v.get_child_pos();
     return get_node<NodeType>(

src/crimson/os/seastore/cache.cc

@@ -208,7 +208,7 @@ void Cache::register_metrics()
       sm::make_counter(
         "cache_hit",
         [this] {
-          return stats.access.s.get_cache_hit();
+          return stats.access.get_cache_hit();
         },
         sm::description("total number of cache hits")
       ),
@@ -2417,22 +2417,20 @@ cache_stats_t Cache::get_stats(
     oss << "\ncache total"
         << cache_size_stats_t{extents_index.get_bytes(), extents_index.size()};
 
-    counter_by_src_t<counter_by_extent_t<extent_access_stats_t> >
+    counter_by_src_t<counter_by_extent_t<cache_access_stats_t> >
       _access_by_src_ext = stats.access_by_src_ext;
     counter_by_src_t<cache_access_stats_t> access_by_src;
     for (uint8_t _src=0; _src<TRANSACTION_TYPE_MAX; ++_src) {
       auto src = static_cast<transaction_type_t>(_src);
       cache_access_stats_t& trans_access = get_by_src(access_by_src, src);
-      trans_access.cache_absent = get_by_src(stats.cache_absent_by_src, src);
-      trans_access.cache_absent -= get_by_src(last_cache_absent_by_src, src);
       auto& access_by_ext = get_by_src(_access_by_src_ext, src);
       const auto& last_access_by_ext = get_by_src(last_access_by_src_ext, src);
       for (uint8_t _ext=0; _ext<EXTENT_TYPES_MAX; ++_ext) {
         auto ext = static_cast<extent_types_t>(_ext);
-        extent_access_stats_t& extent_access = get_by_ext(access_by_ext, ext);
+        cache_access_stats_t& extent_access = get_by_ext(access_by_ext, ext);
         const auto& last_extent_access = get_by_ext(last_access_by_ext, ext);
         extent_access.minus(last_extent_access);
-        trans_access.s.add(extent_access);
+        trans_access.add(extent_access);
       }
     }
     oss << "\naccess: total"
@@ -2443,9 +2441,9 @@ cache_stats_t Cache::get_stats(
       if (trans_access.is_empty()) {
         continue;
       }
-      extent_access_stats_t data_access;
-      extent_access_stats_t mdat_access;
-      extent_access_stats_t phys_access;
+      cache_access_stats_t data_access;
+      cache_access_stats_t mdat_access;
+      cache_access_stats_t phys_access;
       const auto& access_by_ext = get_by_src(_access_by_src_ext, src);
       for (uint8_t _ext=0; _ext<EXTENT_TYPES_MAX; ++_ext) {
         auto ext = static_cast<extent_types_t>(_ext);
@@ -2461,11 +2459,11 @@ cache_stats_t Cache::get_stats(
       oss << "\n  " << src << ": "
           << cache_access_stats_printer_t{seconds, trans_access}
           << "\n    data"
-          << extent_access_stats_printer_t{seconds, data_access}
+          << cache_access_stats_printer_t{seconds, data_access}
           << "\n    mdat"
-          << extent_access_stats_printer_t{seconds, mdat_access}
+          << cache_access_stats_printer_t{seconds, mdat_access}
           << "\n    phys"
-          << extent_access_stats_printer_t{seconds, phys_access};
+          << cache_access_stats_printer_t{seconds, phys_access};
     }
 
     INFO("{}", oss.str());

src/crimson/os/seastore/cache.h

@@ -186,11 +186,6 @@ class Cache : public ExtentTransViewRetriever {
     return t.root;
   }
 
-  void account_absent_access(Transaction::src_t src) {
-    ++(get_by_src(stats.cache_absent_by_src, src));
-    ++stats.access.cache_absent;
-  }
-
   /**
    * get_extent_if_cached
    *
@@ -208,7 +203,7 @@ class Cache : public ExtentTransViewRetriever {
     const auto t_src = t.get_src();
     CachedExtentRef ret;
     auto result = t.get_extent(paddr, &ret);
-    extent_access_stats_t& access_stats = get_by_ext(
+    cache_access_stats_t& access_stats = get_by_ext(
       get_by_src(stats.access_by_src_ext, t_src),
       type);
     if (result == Transaction::get_extent_ret::RETIRED) {
@@ -217,27 +212,28 @@ class Cache : public ExtentTransViewRetriever {
         t, type, paddr, len);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     } else if (result == Transaction::get_extent_ret::PRESENT) {
+      if (ret->get_length() != len) {
+        SUBDEBUGT(seastore_cache,
+          "{} {}~0x{:x} is present on t with inconsistent length 0x{:x} -- {}",
+          t, type, paddr, len, ret->get_length(), *ret);
+        return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
+      }
+
+      ceph_assert(ret->get_type() == type);
+
       if (ret->is_stable()) {
         if (ret->is_dirty()) {
           ++access_stats.trans_dirty;
-          ++stats.access.s.trans_dirty;
+          ++stats.access.trans_dirty;
         } else {
           ++access_stats.trans_lru;
-          ++stats.access.s.trans_lru;
+          ++stats.access.trans_lru;
         }
       } else {
         ++access_stats.trans_pending;
-        ++stats.access.s.trans_pending;
-      }
-
-      if (ret->get_length() != len) {
-        SUBDEBUGT(seastore_cache,
-          "{} {}~0x{:x} is present on t with inconsistent length 0x{:x} -- {}",
-          t, type, paddr, len, ret->get_length(), *ret);
-        return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
+        ++stats.access.trans_pending;
       }
 
-      ceph_assert(ret->get_type() == type);
       if (!ret->is_fully_loaded()) {
         SUBDEBUGT(seastore_cache,
           "{} {}~0x{:x} is present on t without fully loaded -- {}",
@@ -261,7 +257,6 @@ class Cache : public ExtentTransViewRetriever {
       SUBDEBUGT(seastore_cache,
         "{} {}~0x{:x} is absent in cache",
         t, type, paddr, len);
-      account_absent_access(t_src);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     }
 
@@ -270,18 +265,9 @@ class Cache : public ExtentTransViewRetriever {
       SUBDEBUGT(seastore_cache,
         "{} {}~0x{:x} ~0x{:x} is absent(placeholder) in cache",
         t, type, paddr, len, ret->get_length());
-      account_absent_access(t_src);
       return get_extent_if_cached_iertr::make_ready_future<CachedExtentRef>();
     }
 
-    if (ret->is_dirty()) {
-      ++access_stats.cache_dirty;
-      ++stats.access.s.cache_dirty;
-    } else {
-      ++access_stats.cache_lru;
-      ++stats.access.s.cache_lru;
-    }
-
     if (ret->get_length() != len) {
       SUBDEBUGT(seastore_cache,
         "{} {}~0x{:x} is present in cache with inconsistent length 0x{:x} -- {}",
@@ -290,6 +276,15 @@ class Cache : public ExtentTransViewRetriever {
     }
 
     ceph_assert(ret->get_type() == type);
+
+    if (ret->is_dirty()) {
+      ++access_stats.cache_dirty;
+      ++stats.access.cache_dirty;
+    } else {
+      ++access_stats.cache_lru;
+      ++stats.access.cache_lru;
+    }
+
     t.add_to_read_set(ret);
     touch_extent(*ret, &t_src, t.get_cache_hint());
     if (!ret->is_fully_loaded()) {
@@ -402,11 +397,11 @@ class Cache : public ExtentTransViewRetriever {
       // FIXME: assert(ext.is_stable_clean());
       assert(ext.is_stable());
       assert(T::TYPE == ext.get_type());
-      extent_access_stats_t& access_stats = get_by_ext(
+      cache_access_stats_t& access_stats = get_by_ext(
         get_by_src(stats.access_by_src_ext, t_src),
         T::TYPE);
       ++access_stats.load_absent;
-      ++stats.access.s.load_absent;
+      ++stats.access.load_absent;
 
       t.add_to_read_set(CachedExtentRef(&ext));
       touch_extent(ext, &t_src, t.get_cache_hint());
@@ -475,7 +470,7 @@ class Cache : public ExtentTransViewRetriever {
 
     const auto t_src = t.get_src();
     auto ext_type = extent->get_type();
-    extent_access_stats_t& access_stats = get_by_ext(
+    cache_access_stats_t& access_stats = get_by_ext(
       get_by_src(stats.access_by_src_ext, t_src),
       ext_type);
 
@@ -487,7 +482,7 @@ class Cache : public ExtentTransViewRetriever {
         assert(p_extent->is_pending_in_trans(t.get_trans_id()));
         assert(!p_extent->is_stable_writting());
         ++access_stats.trans_pending;
-        ++stats.access.s.trans_pending;
+        ++stats.access.trans_pending;
         if (p_extent->is_mutable()) {
           assert(p_extent->is_fully_loaded());
           assert(!p_extent->is_pending_io());
@@ -502,27 +497,27 @@ class Cache : public ExtentTransViewRetriever {
         if (t.maybe_add_to_read_set(p_extent)) {
           if (p_extent->is_dirty()) {
             ++access_stats.cache_dirty;
-            ++stats.access.s.cache_dirty;
+            ++stats.access.cache_dirty;
           } else {
             ++access_stats.cache_lru;
-            ++stats.access.s.cache_lru;
+            ++stats.access.cache_lru;
           }
           touch_extent(*p_extent, &t_src, t.get_cache_hint());
         } else {
           if (p_extent->is_dirty()) {
             ++access_stats.trans_dirty;
-            ++stats.access.s.trans_dirty;
+            ++stats.access.trans_dirty;
           } else {
             ++access_stats.trans_lru;
-            ++stats.access.s.trans_lru;
+            ++stats.access.trans_lru;
           }
         }
       }
     } else {
       assert(!extent->is_stable_writting());
       assert(extent->is_pending_in_trans(t.get_trans_id()));
       ++access_stats.trans_pending;
-      ++stats.access.s.trans_pending;
+      ++stats.access.trans_pending;
       if (extent->is_mutable()) {
         assert(extent->is_fully_loaded());
         assert(!extent->is_pending_io());
@@ -564,11 +559,11 @@ class Cache : public ExtentTransViewRetriever {
         t, extent->get_type(), extent->get_paddr(), extent->get_length(),
         partial_off, partial_len, *extent);
       const auto t_src = t.get_src();
-      extent_access_stats_t& access_stats = get_by_ext(
+      cache_access_stats_t& access_stats = get_by_ext(
         get_by_src(stats.access_by_src_ext, t_src),
         extent->get_type());
       ++access_stats.load_present;
-      ++stats.access.s.load_present;
+      ++stats.access.load_present;
       return trans_intr::make_interruptible(
         do_read_extent_maybe_partial(
           std::move(extent), partial_off, partial_len, &t_src));
@@ -878,11 +873,11 @@ class Cache : public ExtentTransViewRetriever {
     auto f = [&t, this, t_src](CachedExtent &ext) {
       // FIXME: assert(ext.is_stable_clean());
       assert(ext.is_stable());
-      extent_access_stats_t& access_stats = get_by_ext(
+      cache_access_stats_t& access_stats = get_by_ext(
         get_by_src(stats.access_by_src_ext, t_src),
         ext.get_type());
       ++access_stats.load_absent;
-      ++stats.access.s.load_absent;
+      ++stats.access.load_absent;
 
       t.add_to_read_set(CachedExtentRef(&ext));
       touch_extent(ext, &t_src, t.get_cache_hint());
@@ -1763,7 +1758,7 @@ class Cache : public ExtentTransViewRetriever {
 
     cache_access_stats_t access;
     counter_by_src_t<uint64_t> cache_absent_by_src;
-    counter_by_src_t<counter_by_extent_t<extent_access_stats_t> >
+    counter_by_src_t<counter_by_extent_t<cache_access_stats_t> >
       access_by_src_ext;
 
     uint64_t onode_tree_depth = 0;
@@ -1800,7 +1795,7 @@ class Cache : public ExtentTransViewRetriever {
   mutable rewrite_stats_t last_reclaim_rewrites;
   mutable cache_access_stats_t last_access;
   mutable counter_by_src_t<uint64_t> last_cache_absent_by_src;
-  mutable counter_by_src_t<counter_by_extent_t<extent_access_stats_t> >
+  mutable counter_by_src_t<counter_by_extent_t<cache_access_stats_t> >
     last_access_by_src_ext;
 
   void account_conflict(Transaction::src_t src1, Transaction::src_t src2) {

src/crimson/os/seastore/lba_manager/btree/btree_lba_manager.cc

@@ -60,12 +60,10 @@ const get_phy_tree_root_node_ret get_phy_tree_root_node<
       return {true,
               c.cache.get_extent_viewable_by_trans(c.trans, lba_root)};
     } else {
-      c.cache.account_absent_access(c.trans.get_src());
       return {false,
               Cache::get_extent_iertr::make_ready_future<CachedExtentRef>()};
     }
   } else {
-    c.cache.account_absent_access(c.trans.get_src());
     return {false,
             Cache::get_extent_iertr::make_ready_future<CachedExtentRef>()};
   }
@@ -103,7 +101,6 @@ BtreeLBAMapping::get_logical_extent(Transaction &t)
     : get_key();
   auto v = p.template get_child<LogicalChildNode>(ctx.trans, ctx.cache, pos, k);
   if (!v.has_child()) {
-    ctx.cache.account_absent_access(ctx.trans.get_src());
     this->child_pos = v.get_child_pos();
   }
   return v;