Re: [PATCH v10 0/6] ceph: add perf metrics support

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2020-03-18 at 10:05 -0400, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> Changed in V10:
> - rebase to the latest testing branch
> - merge all the metric related patches into one
> - [1/6] move metric helpers into a new file metric.c
> - [2/6] move metric helpers into metric.c
> - [3/6] merge the read/write patches into a signal patch and move metric helpers to metric.c
> - [4/6] move metric helpers to metric.c
> - [5/6] min/max latency support
> - [6/6] standard deviation support
> 
> Changed in V9:
> - add an r_ended field to the mds request struct and use that to calculate the metric
> - fix some commit comments
> 
> # cat /sys/kernel/debug/ceph/9a972bfc-68cb-4d52-a610-7cd9a9adbbdd.client52904/metrics
> item          total       avg_lat(us)     min_lat(us)     max_lat(us)     stdev(us)
> -----------------------------------------------------------------------------------
> read          798         32000           4000            196000          560.3
> write         2394        588000          28000           4812000         36673.9
> metadata      7           116000          2000            707000          8282.8
> 
> item          total           miss            hit
> -------------------------------------------------
> d_lease       2               0               0
> caps          2               14              546500
> 
> 
> 

The code all looks reasonable to me.

Ilya mentioned refactoring the set to add the infrastructure up front
first. I too think that would be nice, especially since this will
probably end up being backported to various distros and that would make
that task simpler.

It might also be nice to merge the add in the min/max/stddev support at
the same time you add each latency metric too, rather than adding them
after the fact.

> 
> Xiubo Li (6):
>   ceph: add dentry lease metric support
>   ceph: add caps perf metric for each session
>   ceph: add read/write latency metric support

Can you fold the min/max/stddev changes for read/write into the above
patch? I think that would be cleaner, rather than bolting it on after
the fact.

>   ceph: add metadata perf metric support

Same here. That should just leave us with a 4 patch series, I think.
 
>   ceph: add min/max latency support for read/write/metadata metrics
>   ceph: add standard deviation support for read/write/metadata perf
>     metric
> 
>  fs/ceph/Makefile                |   2 +-
>  fs/ceph/acl.c                   |   2 +-
>  fs/ceph/addr.c                  |  18 ++++
>  fs/ceph/caps.c                  |  19 ++++
>  fs/ceph/debugfs.c               | 116 +++++++++++++++++++++++-
>  fs/ceph/dir.c                   |  17 +++-
>  fs/ceph/file.c                  |  26 ++++++
>  fs/ceph/inode.c                 |   4 +-
>  fs/ceph/mds_client.c            |  21 ++++-
>  fs/ceph/mds_client.h            |   7 +-
>  fs/ceph/metric.c                | 193 ++++++++++++++++++++++++++++++++++++++++
>  fs/ceph/metric.h                |  64 +++++++++++++
>  fs/ceph/super.h                 |   9 +-
>  fs/ceph/xattr.c                 |   4 +-
>  include/linux/ceph/osd_client.h |   1 +
>  net/ceph/osd_client.c           |   2 +
>  16 files changed, 487 insertions(+), 18 deletions(-)
>  create mode 100644 fs/ceph/metric.c
>  create mode 100644 fs/ceph/metric.h
> 



Thanks,
-- 
Jeff Layton <jlayton@kernel.org>