| Message ID | 20210325032826.1725667-1-xiubli@redhat.com |
|---|---|
| Headers | show |
| Series | ceph: add IO size metric support | expand |
On Thu, 2021-03-25 at 11: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 | 73 ++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 52 insertions(+), 21 deletions(-) > > diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c > index 28b6b42ad677..f3e68db08760 100644 > --- a/fs/ceph/metric.c > +++ b/fs/ceph/metric.c > @@ -285,19 +285,56 @@ 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) > -{ > - ktime_t total, avg, sq, lsum; > - > - total = ++(*totalp); > - lsum = (*lsump += lat); > +typedef enum { > + CEPH_METRIC_READ, > + CEPH_METRIC_WRITE, > + CEPH_METRIC_METADATA, > +} metric_type; > + > +#define METRIC_UPDATE_MIN_MAX(min, max, new) \ > +{ \ > + if (unlikely(new < min)) \ > + min = new; \ > + if (unlikely(new > max)) \ > + max = new; \ > +} > > - if (unlikely(lat < *min)) > - *min = lat; > - if (unlikely(lat > *max)) > - *max = lat; > +static inline void __update_latency(struct ceph_client_metric *m, > + metric_type type, ktime_t lat) > +{ > + ktime_t total, avg, sq, lsum, *sq_sump; > + > + switch (type) { > + case CEPH_METRIC_READ: > + total = ++m->total_reads; > + m->read_latency_sum += lat; > + lsum = m->read_latency_sum; > + METRIC_UPDATE_MIN_MAX(m->read_latency_min, > + m->read_latency_max, > + lat); > + sq_sump = &m->read_latency_sq_sum; > + break; > + case CEPH_METRIC_WRITE: > + total = ++m->total_writes; > + m->write_latency_sum += lat; > + lsum = m->write_latency_sum; > + METRIC_UPDATE_MIN_MAX(m->write_latency_min, > + m->write_latency_max, > + lat); > + sq_sump = &m->write_latency_sq_sum; > + break; > + case CEPH_METRIC_METADATA: > + total = ++m->total_metadatas; > + m->metadata_latency_sum += lat; > + lsum = m->metadata_latency_sum; > + METRIC_UPDATE_MIN_MAX(m->metadata_latency_min, > + m->metadata_latency_max, > + lat); > + sq_sump = &m->metadata_latency_sq_sum; > + break; > + default: > + return; > + } > I'm not a fan of the above function. __update_latency gets called with each of those values only once. It seems like it'd be better to just open-code the above sections in the respective ceph_update_*_metrics functions, and then have a helper function for the part of __update_latency below this point. With that, you wouldn't need the enum either. > if (unlikely(total == 1)) > return; > @@ -320,9 +357,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); > } > > @@ -336,9 +371,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); > } > > @@ -352,8 +385,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); > } -- Jeff Layton <jlayton@kernel.org>
On 2021/4/28 2:38, Jeff Layton wrote: > On Thu, 2021-03-25 at 11: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 | 73 ++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 52 insertions(+), 21 deletions(-) >> >> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c >> index 28b6b42ad677..f3e68db08760 100644 >> --- a/fs/ceph/metric.c >> +++ b/fs/ceph/metric.c >> @@ -285,19 +285,56 @@ 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) >> -{ >> - ktime_t total, avg, sq, lsum; >> - >> - total = ++(*totalp); >> - lsum = (*lsump += lat); >> +typedef enum { >> + CEPH_METRIC_READ, >> + CEPH_METRIC_WRITE, >> + CEPH_METRIC_METADATA, >> +} metric_type; >> + >> +#define METRIC_UPDATE_MIN_MAX(min, max, new) \ >> +{ \ >> + if (unlikely(new < min)) \ >> + min = new; \ >> + if (unlikely(new > max)) \ >> + max = new; \ >> +} >> >> - if (unlikely(lat < *min)) >> - *min = lat; >> - if (unlikely(lat > *max)) >> - *max = lat; >> +static inline void __update_latency(struct ceph_client_metric *m, >> + metric_type type, ktime_t lat) >> +{ >> + ktime_t total, avg, sq, lsum, *sq_sump; >> + >> + switch (type) { >> + case CEPH_METRIC_READ: >> + total = ++m->total_reads; >> + m->read_latency_sum += lat; >> + lsum = m->read_latency_sum; >> + METRIC_UPDATE_MIN_MAX(m->read_latency_min, >> + m->read_latency_max, >> + lat); >> + sq_sump = &m->read_latency_sq_sum; >> + break; >> + case CEPH_METRIC_WRITE: >> + total = ++m->total_writes; >> + m->write_latency_sum += lat; >> + lsum = m->write_latency_sum; >> + METRIC_UPDATE_MIN_MAX(m->write_latency_min, >> + m->write_latency_max, >> + lat); >> + sq_sump = &m->write_latency_sq_sum; >> + break; >> + case CEPH_METRIC_METADATA: >> + total = ++m->total_metadatas; >> + m->metadata_latency_sum += lat; >> + lsum = m->metadata_latency_sum; >> + METRIC_UPDATE_MIN_MAX(m->metadata_latency_min, >> + m->metadata_latency_max, >> + lat); >> + sq_sump = &m->metadata_latency_sq_sum; >> + break; >> + default: >> + return; >> + } >> > I'm not a fan of the above function. __update_latency gets called with > each of those values only once. > > It seems like it'd be better to just open-code the above sections in the > respective ceph_update_*_metrics functions, and then have a helper > function for the part of __update_latency below this point. With that, > you wouldn't need the enum either. Sounds good to me, will fix it. > >> if (unlikely(total == 1)) >> return; >> @@ -320,9 +357,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); >> } >> >> @@ -336,9 +371,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); >> } >> >> @@ -352,8 +385,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); >> }
From: Xiubo Li <xiubli@redhat.com> V2: - remove the unused parameters in metric.c - a small clean up for the code. For the read/write IO speeds, will leave them to be computed in userspace, where it can get a preciser result with float type. Xiubo Li (2): ceph: update the __update_latency helper ceph: add IO size metrics support fs/ceph/addr.c | 14 +++--- fs/ceph/debugfs.c | 37 ++++++++++++++-- fs/ceph/file.c | 23 +++++----- fs/ceph/metric.c | 109 ++++++++++++++++++++++++++++++++++++---------- fs/ceph/metric.h | 10 ++++- 5 files changed, 146 insertions(+), 47 deletions(-)