On 2021/3/23 20:34, Jeff Layton wrote:
On Mon, 2021-03-22 at 20:28 +0800, xiubli@xxxxxxxxxx wrote:From: Xiubo Li <xiubli@xxxxxxxxxx> Let the __update_latency() helper choose the correcsponding members according to the metric_type. URL: https://tracker.ceph.com/issues/49913 Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx> --- fs/ceph/metric.c | 58 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 42 insertions(+), 16 deletions(-) diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c index 75d309f2fb0c..d5560ff99a9d 100644 --- a/fs/ceph/metric.c +++ b/fs/ceph/metric.c @@ -249,19 +249,51 @@ void ceph_metric_destroy(struct ceph_client_metric *m) ceph_put_mds_session(m->session); }-static inline void __update_latency(ktime_t *totalp, ktime_t *lsump, - ktime_t *min, ktime_t *max, - ktime_t *sq_sump, ktime_t lat) +typedef enum { + CEPH_METRIC_READ, + CEPH_METRIC_WRITE, + CEPH_METRIC_METADATA, +} metric_type; + +static inline void __update_latency(struct ceph_client_metric *m, + metric_type type, ktime_t lat) { + ktime_t *totalp, *minp, *maxp, *lsump, *sq_sump; ktime_t total, avg, sq, lsum;+ switch (type) { + case CEPH_METRIC_READ: + totalp = &m->total_reads; + lsump = &m->read_latency_sum; + minp = &m->read_latency_min; + maxp = &m->read_latency_max; + sq_sump = &m->read_latency_sq_sum; + break; + case CEPH_METRIC_WRITE: + totalp = &m->total_writes; + lsump = &m->write_latency_sum; + minp = &m->write_latency_min; + maxp = &m->write_latency_max; + sq_sump = &m->write_latency_sq_sum; + break; + case CEPH_METRIC_METADATA: + totalp = &m->total_metadatas; + lsump = &m->metadata_latency_sum; + minp = &m->metadata_latency_min; + maxp = &m->metadata_latency_max; + sq_sump = &m->metadata_latency_sq_sum; + break; + default: + return; + } + total = ++(*totalp);Why are you adding one to *totalp above? Is that to avoid it being 0?
No, in the old code we will count the total_reads/total_writes/total_metadatas for each call of the ceph_update_{read/write/metadata}_latency() helpers. And the same here.
lsum = (*lsump += lat);^^^ Instead of doing all of the above with pointers, why not just add to total and lsum directly inside the switch statement? This seems like a lot of pointless indirection.
Okay, sounds good, will change it.
- if (unlikely(lat < *min)) - *min = lat; - if (unlikely(lat > *max)) - *max = lat; + if (unlikely(lat < *minp)) + *minp = lat; + if (unlikely(lat > *maxp)) + *maxp = lat;if (unlikely(total == 1)) return; @@ -284,9 +316,7 @@ void ceph_update_read_metrics(struct ceph_client_metric *m, return;spin_lock(&m->read_metric_lock); - __update_latency(&m->total_reads, &m->read_latency_sum, - &m->read_latency_min, &m->read_latency_max, - &m->read_latency_sq_sum, lat); + __update_latency(m, CEPH_METRIC_READ, lat); spin_unlock(&m->read_metric_lock); }@@ -300,9 +330,7 @@ void ceph_update_write_metrics(struct ceph_client_metric *m, return;spin_lock(&m->write_metric_lock); - __update_latency(&m->total_writes, &m->write_latency_sum, - &m->write_latency_min, &m->write_latency_max, - &m->write_latency_sq_sum, lat); + __update_latency(m, CEPH_METRIC_WRITE, lat); spin_unlock(&m->write_metric_lock); }@@ -316,8 +344,6 @@ void ceph_update_metadata_metrics(struct ceph_client_metric *m, return;spin_lock(&m->metadata_metric_lock); - __update_latency(&m->total_metadatas, &m->metadata_latency_sum, - &m->metadata_latency_min, &m->metadata_latency_max, - &m->metadata_latency_sq_sum, lat); + __update_latency(m, CEPH_METRIC_METADATA, lat); spin_unlock(&m->metadata_metric_lock); }
