From 3cd9aeb17ac67cd20b6a4e60950ac07d4c4cfa30 Mon Sep 17 00:00:00 2001
From: Igor Golikov <igolikov@ibm.com>
Date: Sun, 10 Aug 2025 17:48:55 +0000
Subject: [PATCH] client: aggregate metrics on the fly

Instead of keeping vector<SimpleIOMetric>, which must be capped in terms
of memory footprint, aggregate metrics per each subvolume on the fly,
keeping memory footprint minimal.

Signed-off-by: Igor Golikov <igolikov@ibm.com>
Fixes: https://tracker.ceph.com/issues/68929
---
 src/client/Client.cc               | 65 ++++++++++++++++--------------
 src/client/Client.h                |  6 +--
 src/include/cephfs/metrics/Types.h |  2 +-
 3 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 208051092df..b735f4e84d3 100644
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -17699,9 +17699,8 @@ void SubvolumeMetricTracker::add_inode(inodeno_t inode, inodeno_t subvol) {
 
   auto [it, inserted] = subvolume_metrics.try_emplace(subvol);
   if (inserted) {
-    it->second.metrics.reserve(initial_reserve);
+    ldout(cct, 10) << __func__ << " inserted " << inode << "-" << subvol << dendl;
   }
-
   inode_subvolume[inode] = subvol;
   ldout(cct, 10) << __func__ << " add " << inode << "-" << subvol << dendl;
 }
@@ -17727,46 +17726,50 @@ void SubvolumeMetricTracker::remove_inode(inodeno_t inode) {
 
 void SubvolumeMetricTracker::add_metric(inodeno_t inode, SimpleIOMetric&& metric) {
   ldout(cct, 10) << __func__ << " " << inode << dendl;
-  std::shared_lock l(metrics_lock);
+  std::unique_lock l(metrics_lock);
+
   auto it = inode_subvolume.find(inode);
-  if (it == inode_subvolume.end()) {
-     return;
-  }
-  ldout(cct, 10) << __func__ << " adding subv_metric for " << inode << dendl;
-  auto& entry = subvolume_metrics[it->second]; // creates entry if not present
-  l.unlock();  // release before modifying
-  std::unique_lock l2(metrics_lock);
-  entry.metrics.emplace_back(metric);
+  if (it == inode_subvolume.end())
+    return;
+
+  auto& entry = subvolume_metrics[it->second];
+  entry.metrics.add(std::move(metric));
 }
 
-std::vector<AggregatedIOMetrics> SubvolumeMetricTracker::aggregate(bool clean) {
+std::vector<AggregatedIOMetrics>
+SubvolumeMetricTracker::aggregate(bool clean) {
   ldout(cct, 20) << __func__ << dendl;
   std::vector<AggregatedIOMetrics> res;
-  res.reserve(subvolume_metrics.size());
-  std::unordered_map<inodeno_t, SubvolumeEntry> local_map;
-  {
-    std::unique_lock l(metrics_lock);
-    if (clean)
-      subvolume_metrics.swap(local_map);
-    else
-      local_map = subvolume_metrics;
-  }
 
-  for (const auto& [subvol_id, entry] : local_map) {
-    if (entry.metrics.empty())
-        continue;
-    auto& agg = res.emplace_back();
-    agg.subvolume_id = subvol_id;
-    for (const auto& metric : entry.metrics) {
-      agg.add(metric);
+  if (clean) {
+    std::unordered_map<inodeno_t, SubvolumeEntry> tmp;
+    // move subvolume map to the local one, to release wlock asap
+    {
+      std::unique_lock l(metrics_lock);
+      subvolume_metrics.swap(tmp);
+    }
+    res.reserve(tmp.size());
+    for (auto &[subv_id, entry]: tmp) {
+      res.emplace_back(std::move(entry.metrics));
+      res.back().subvolume_id = subv_id;
+    }
+  } else {
+    // on rlock is needed, no need to copy the map to the local instance on the metrics map
+    std::shared_lock l(metrics_lock);
+    res.reserve(subvolume_metrics.size());
+    for (const auto &[subv_id, entry]: subvolume_metrics) {
+      res.emplace_back(entry.metrics);
+      res.back().subvolume_id = subv_id;
     }
   }
 
-  std::unique_lock l(metrics_lock);
-  last_subvolume_metrics = res;
+  {
+    std::unique_lock l(metrics_lock);
+    last_subvolume_metrics = res; // since res holds only 1 aggregated metrics per subvolume, copying is not so bad
+  }
 
   ldout(cct, 20) << __func__ << " res size " << res.size() << dendl;
-  return res;
+  return res; // return value optimization
 }
 // --- subvolume metrics tracking --- //
 
diff --git a/src/client/Client.h b/src/client/Client.h
index ada0acb8c28..77790117ef6 100644
--- a/src/client/Client.h
+++ b/src/client/Client.h
@@ -271,11 +271,10 @@ struct dir_result_t {
 class SubvolumeMetricTracker {
 public:
     struct SubvolumeEntry {
-        std::vector<SimpleIOMetric> metrics;
+        AggregatedIOMetrics metrics;
 
         void dump(Formatter *f) const {
-          for(auto const& m : metrics)
-            f->dump_object("", m);
+          f->dump_object("", metrics);
         }
     };
 
@@ -289,7 +288,6 @@ protected:
     std::vector<AggregatedIOMetrics> last_subvolume_metrics;
     std::unordered_map<inodeno_t, SubvolumeEntry> subvolume_metrics;
     std::unordered_map<inodeno_t, inodeno_t> inode_subvolume;
-    size_t initial_reserve = 1000;
     CephContext *cct = nullptr;
     client_t whoami;
     std::shared_mutex metrics_lock;
diff --git a/src/include/cephfs/metrics/Types.h b/src/include/cephfs/metrics/Types.h
index ee658c7f63f..02d4c4d43bc 100644
--- a/src/include/cephfs/metrics/Types.h
+++ b/src/include/cephfs/metrics/Types.h
@@ -648,7 +648,7 @@ struct AggregatedIOMetrics {
     uint64_t write_bytes = 0;
     uint64_t read_latency_us = 0;
     uint64_t write_latency_us = 0;
-    uint64_t time_stamp; // set on MDS
+    uint64_t time_stamp = 0; // set on MDS
 
     void add(const SimpleIOMetric& m) {
       auto lat = m.get_latency_us();
-- 
2.39.5