client: fix code review comments

Signed-off-by: Igor Golikov <igolikov@ibm.com>
Fixes: https://tracker.ceph.com/issues/68929

diff --git a/src/client/Client.cc b/src/client/Client.cc
index 6d2511c1a6dddc8811237b61a7e8c99f6148be6f..98fd1a525ffd51312f8aaa986fb2bf6bb7d5741e 100644 (file)
--- a/src/client/Client.cc
+++ b/src/client/Client.cc
@@ -403,7 +403,7 @@ Client::Client(Messenger *m, MonClient *mc, Objecter *objecter_)
     objecter_finisher(m->cct),
     m_command_hook(this),
     fscid(0),
-    subvolume_tracker{std::make_unique<SubvolumeTracker>(cct, whoami)}
+    subvolume_tracker{std::make_unique<SubvolumeMetricTracker>(cct, whoami)}
 {
   /* We only use the locale for normalization/case folding. That is unaffected
    * by the locale but required by the API.
@@ -868,10 +868,6 @@ void Client::update_io_stat_write(utime_t latency) {
   logger->set(l_c_wr_sqsum, n_sqsum);
   logger->set(l_c_wr_ops, nr_write_request);
 }
-
-void Client::update_subvolume_metric(bool write, utime_t start, utime_t end, uint64_t size, Inode *in) {
-}
-
 // ===================
 // metadata cache stuff
 
@@ -3739,14 +3735,14 @@ private:
   Client *client;
   InodeRef inode;
 public:
-  C_Client_FlushComplete(Client *c, Inode *in) : client(c), inode(in) {}
+  C_Client_FlushComplete(Client *c, Inode *in) : client(c), inode(in) { }
   void finish(int r) override {
     ceph_assert(ceph_mutex_is_locked_by_me(client->client_lock));
     if (r != 0) {
       client_t const whoami = client->whoami;  // For the benefit of ldout prefix
       ldout(client->cct, 1) << "I/O error from flush on inode " << inode
-                            << " 0x" << std::hex << inode->ino << std::dec
-                            << ": " << r << "(" << cpp_strerror(r) << ")" << dendl;
+        << " 0x" << std::hex << inode->ino << std::dec
+        << ": " << r << "(" << cpp_strerror(r) << ")" << dendl;
       inode->set_async_err(r);
     }
   }
@@ -17671,9 +17667,9 @@ mds_rank_t Client::_get_random_up_mds() const
 }
 
 // --- subvolume metrics tracking --- //
-SubvolumeTracker::SubvolumeTracker(CephContext *ct, client_t id) : cct(ct), whoami(id) {}
+SubvolumeMetricTracker::SubvolumeMetricTracker(CephContext *ct, client_t id) : cct(ct), whoami(id) {}
 
-void SubvolumeTracker::dump(Formatter *f) {
+void SubvolumeMetricTracker::dump(Formatter *f) {
   auto current_metrics = aggregate(false);
   f->open_array_section("current_metrics");
   for (auto &met : current_metrics) {
@@ -17693,10 +17689,10 @@ void SubvolumeTracker::dump(Formatter *f) {
   f->close_section();
 }
 
-void SubvolumeTracker::add_inode(inodeno_t inode, inodeno_t subvol) {
+void SubvolumeMetricTracker::add_inode(inodeno_t inode, inodeno_t subvol) {
   ldout(cct, 20) << __func__ << " subv_metric " << inode << "-" << subvol << dendl;
   std::unique_lock l(metrics_lock);
-  if (inode_subvolume.contains(inode)) {
+  if (likely(inode_subvolume.contains(inode))) {
     ldout(cct, 10) << __func__ << " " << inode << "-" << subvol << " subv_metric inode exists" << dendl;
     return;
   }
@@ -17710,7 +17706,7 @@ void SubvolumeTracker::add_inode(inodeno_t inode, inodeno_t subvol) {
   ldout(cct, 10) << __func__ << " add " << inode << "-" << subvol << dendl;
 }
 
-void SubvolumeTracker::remove_inode(inodeno_t inode) {
+void SubvolumeMetricTracker::remove_inode(inodeno_t inode) {
   ldout(cct, 20) << __func__ << " subv_metric " << inode << "-" << dendl;
   std::unique_lock l(metrics_lock);
   auto it = inode_subvolume.find(inode);
@@ -17729,7 +17725,7 @@ void SubvolumeTracker::remove_inode(inodeno_t inode) {
   ldout(cct, 20) << __func__ << " subv_metric end " << inode << dendl;
 }
 
-void SubvolumeTracker::add_metric(inodeno_t inode, SimpleIOMetric&& metric) {
+void SubvolumeMetricTracker::add_metric(inodeno_t inode, SimpleIOMetric&& metric) {
   ldout(cct, 10) << __func__ << " " << inode << dendl;
   std::shared_lock l(metrics_lock);
   auto it = inode_subvolume.find(inode);
@@ -17743,7 +17739,7 @@ void SubvolumeTracker::add_metric(inodeno_t inode, SimpleIOMetric&& metric) {
   entry.metrics.emplace_back(metric);
 }
 
-std::vector<AggregatedIOMetrics> SubvolumeTracker::aggregate(bool clean) {
+std::vector<AggregatedIOMetrics> SubvolumeMetricTracker::aggregate(bool clean) {
   ldout(cct, 20) << __func__ << dendl;
   std::vector<AggregatedIOMetrics> res;
   res.reserve(subvolume_metrics.size());

diff --git a/src/client/Client.h b/src/client/Client.h
index 4fd75690f827f736066a04ea404051313713f720..6d26726b0543c8a10af7a68a84f586e5237b60ae 100644 (file)
--- a/src/client/Client.h
+++ b/src/client/Client.h
@@ -265,10 +265,10 @@ struct dir_result_t {
  * Maps subvolume_id(which is in fact inode id) to the vector of SimpleIOMetric instances.
  * Each simple metric records the single IO operation on the client.
  * On clients metric message to the MDS, client will aggregate all simple metrics for each subvolume
- * into the AggregatedIOMetric struct and clean the current metrics list.
+ * into the AggregatedIOMetric struct and clear the current metrics list.
  * TODO: limit the cap for each subvolume? in the case client sends metrics to the MDS not so often?
  */
-class SubvolumeTracker {
+class SubvolumeMetricTracker {
 public:
     struct SubvolumeEntry {
         std::vector<SimpleIOMetric> metrics;
@@ -279,7 +279,7 @@ public:
         }
     };
 
-    SubvolumeTracker(CephContext *ct, client_t id);
+    SubvolumeMetricTracker(CephContext *ct, client_t id);
     void dump(Formatter *f);
     void add_inode(inodeno_t inode, inodeno_t subvol);
     void remove_inode(inodeno_t inode);
@@ -1910,7 +1910,6 @@ private:
   void update_io_stat_metadata(utime_t latency);
   void update_io_stat_read(utime_t latency);
   void update_io_stat_write(utime_t latency);
-  void update_subvolume_metric(bool write, utime_t start, utime_t end, uint64_t size, Inode *in);
 
   bool should_check_perms() const {
     return (is_fuse && !fuse_default_permissions) || (!is_fuse && client_permissions);
@@ -1964,7 +1963,7 @@ private:
   fs_cluster_id_t fscid;
 
   // subvolume metrics tracker
-  std::unique_ptr<SubvolumeTracker> subvolume_tracker = nullptr;
+  std::unique_ptr<SubvolumeMetricTracker> subvolume_tracker = nullptr;
 
   // file handles, etc.
   interval_set<int> free_fd_set;  // unused fds

diff --git a/src/include/cephfs/metrics/Types.h b/src/include/cephfs/metrics/Types.h
index e758821a3e7003e0c2fae936b6e6ff5b2eda8491..4a63719b6e2c421818433667f5e533c3135a9562 100644 (file)
--- a/src/include/cephfs/metrics/Types.h
+++ b/src/include/cephfs/metrics/Types.h
@@ -616,7 +616,7 @@ struct SimpleIOMetric {
 
     SimpleIOMetric() : packed_data(0) {}
 
-    bool get_is_write() const {
+    bool is_write() const {
       return (packed_data & IS_WRITE_MASK) != 0;
     }
 
@@ -630,14 +630,14 @@ struct SimpleIOMetric {
 
     // --- Dump method ---
     void dump(Formatter *f) const {
-      f->dump_string("op", get_is_write() ? "w" : "r");
+      f->dump_string("op", is_write() ? "w" : "r");
       f->dump_unsigned("lat_us", get_latency_us());
       f->dump_unsigned("size", get_payload_size());
     }
 };
 
 /**
- * brief holds resuolt of the SimpleIOMetrics aggregation, for each subvolume, on the client
+ * brief holds result of the SimpleIOMetrics aggregation, for each subvolume, on the client
  * the aggregation occurs just before sending metrics to the MDS
  */
 struct AggregatedIOMetrics {
@@ -652,7 +652,7 @@ struct AggregatedIOMetrics {
 
     void add(const SimpleIOMetric& m) {
       auto lat = m.get_latency_us();
-      if (m.get_is_write()) {
+      if (m.is_write()) {
         ++write_count;
         write_bytes += m.get_payload_size();
         write_latency_us += lat;
@@ -680,23 +680,23 @@ struct AggregatedIOMetrics {
     }
 
     double read_iops() const {
-      return read_latency_us > 0 ? static_cast<double>(read_count) * 1e9 / read_latency_us : 0.0;
+      return read_latency_us > 0 ? static_cast<double>(read_count) * 1e6 / read_latency_us : 0.0;
     }
 
     double write_iops() const {
-      return write_latency_us > 0 ? static_cast<double>(write_count) * 1e9 / write_latency_us : 0.0;
+      return write_latency_us > 0 ? static_cast<double>(write_count) * 1e6 / write_latency_us : 0.0;
     }
 
     void dump(Formatter *f) const {
       f->dump_unsigned("subvolume_id", subvolume_id);
       f->dump_unsigned("read_count", read_count);
       f->dump_unsigned("read_bytes", read_bytes);
-      f->dump_unsigned("avg_read_latency_ns", avg_read_latency_us());
+      f->dump_unsigned("avg_read_latency_us", avg_read_latency_us());
       f->dump_unsigned("read_iops", read_iops());
-      f->dump_float("avg_read_tp_Bpb", avg_read_throughput_Bps());
+      f->dump_float("avg_read_tp_Bps", avg_read_throughput_Bps());
       f->dump_unsigned("write_count", write_count);
       f->dump_unsigned("write_bytes", write_bytes);
-      f->dump_unsigned("avg_write_latency_ns", avg_write_latency_us());
+      f->dump_unsigned("avg_write_latency_us", avg_write_latency_us());
       f->dump_float("write_iops", write_iops());
       f->dump_float("avg_write_tp_Bps", avg_write_throughput_Bps());
       f->dump_unsigned("time_stamp", time_stamp);

diff --git a/src/mds/MetricsHandler.cc b/src/mds/MetricsHandler.cc
index 2efde2b129978f9b44b627ea534afdb5c662b978..a98d8e61409a20b13e793bf348b3ad6254cf0136 100644 (file)
--- a/src/mds/MetricsHandler.cc
+++ b/src/mds/MetricsHandler.cc
@@ -55,8 +55,6 @@ void MetricsHandler::init() {
         update_rank0();
       }
     });
-
-
 }
 
 void MetricsHandler::shutdown() {