| Message ID | 20210322122852.322927-3-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: add IO size metric support | expand |
On Mon, 2021-03-22 at 20:28 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > 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@redhat.com> > --- > 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? > 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. > > > > > > - 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); > }
On 2021/3/23 20:34, Jeff Layton wrote: > On Mon, 2021-03-22 at 20:28 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> 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@redhat.com> >> --- >> 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); >> }
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); lsum = (*lsump += lat); - 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); }