From 0eade213f38bcd93ec72974d8bcad683a8c6528c Mon Sep 17 00:00:00 2001 From: Igor Golikov Date: Thu, 31 Jul 2025 05:36:40 +0000 Subject: [PATCH] client: fix code review comments Signed-off-by: Igor Golikov Fixes: https://tracker.ceph.com/issues/68929 --- src/client/Client.cc | 26 +++++++++++--------------- src/client/Client.h | 9 ++++----- src/include/cephfs/metrics/Types.h | 18 +++++++++--------- src/mds/MetricsHandler.cc | 2 -- 4 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/client/Client.cc b/src/client/Client.cc index 6d2511c1a6d..98fd1a525ff 100644 --- 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(cct, whoami)} + subvolume_tracker{std::make_unique(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 SubvolumeTracker::aggregate(bool clean) { +std::vector SubvolumeMetricTracker::aggregate(bool clean) { ldout(cct, 20) << __func__ << dendl; std::vector res; res.reserve(subvolume_metrics.size()); diff --git a/src/client/Client.h b/src/client/Client.h index 4fd75690f82..6d26726b054 100644 --- 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 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 subvolume_tracker = nullptr; + std::unique_ptr subvolume_tracker = nullptr; // file handles, etc. interval_set free_fd_set; // unused fds diff --git a/src/include/cephfs/metrics/Types.h b/src/include/cephfs/metrics/Types.h index e758821a3e7..4a63719b6e2 100644 --- 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(read_count) * 1e9 / read_latency_us : 0.0; + return read_latency_us > 0 ? static_cast(read_count) * 1e6 / read_latency_us : 0.0; } double write_iops() const { - return write_latency_us > 0 ? static_cast(write_count) * 1e9 / write_latency_us : 0.0; + return write_latency_us > 0 ? static_cast(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 2efde2b1299..a98d8e61409 100644 --- a/src/mds/MetricsHandler.cc +++ b/src/mds/MetricsHandler.cc @@ -55,8 +55,6 @@ void MetricsHandler::init() { update_rank0(); } }); - - } void MetricsHandler::shutdown() { -- 2.39.5