Re: [RFC PATCH v2] ceph: add remote object copy counter to fs client metrics

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2021-10-25 at 16:00 +0100, Luís Henriques wrote:
> This counter will keep track of the number of remote object copies done on
> copy_file_range syscalls.  This counter will be kept using the metrics
> infrastructure and thus accessible through debugfs.  For now, this counter
> won't be sent to the MDS.
> 
> Cc: Patrick Donnelly <pdonnell@redhat.com>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
> Hi!
> 
> So, here's v2 of this RFC.  Now, I guess that Patrick's idea of adding
> this counter was to validate the test results, isn't that right?  If so,
> this has to be done from within the fstest code and not from teuthology
> test.  The reason is that fstests mount and unmount the filesystems under
> test, which effectively wipe the metrics on the client.
> 
> So, the follow-up to this patch would be changes to the corresponding
> fstests so that they would access this debugfs file and check the counter
> is set to the expected value.
> 
> Cheers,
> --
> Luís
> 
>  fs/ceph/debugfs.c | 6 ++++++
>  fs/ceph/file.c    | 1 +
>  fs/ceph/metric.c  | 2 ++
>  fs/ceph/metric.h  | 2 ++
>  4 files changed, 11 insertions(+)
> 
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 38b78b45811f..9f1a09816541 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -235,6 +235,12 @@ static int metric_show(struct seq_file *s, void *p)
>  		   percpu_counter_sum(&m->i_caps_mis),
>  		   percpu_counter_sum(&m->i_caps_hit));
>  
> +	seq_printf(s, "\n");
> +	seq_printf(s, "item          total\n");
> +	seq_printf(s, "-------------------\n");
> +	seq_printf(s, "%-14s%-16lld\n", "copy-from",
> +		   atomic64_read(&m->total_copyfrom));
> +
>  	return 0;
>  }
>  
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index e61018d9764e..b36a7b9c1ab8 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -2253,6 +2253,7 @@ static ssize_t ceph_do_objects_copy(struct ceph_inode_info *src_ci, u64 *src_off
>  				bytes = ret;
>  			goto out;
>  		}
> +		atomic64_inc(&fsc->mdsc->metric.total_copyfrom);
>  		len -= object_size;
>  		bytes += object_size;
>  		*src_off += object_size;
> diff --git a/fs/ceph/metric.c b/fs/ceph/metric.c
> index 04d5df29bbbf..a8a9f96c56a8 100644
> --- a/fs/ceph/metric.c
> +++ b/fs/ceph/metric.c
> @@ -278,6 +278,8 @@ int ceph_metric_init(struct ceph_client_metric *m)
>  	if (ret)
>  		goto err_total_inodes;
>  
> +	atomic64_set(&m->total_copyfrom, 0);
> +
>  	m->session = NULL;
>  	INIT_DELAYED_WORK(&m->delayed_work, metric_delayed_work);
>  
> diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h
> index 0133955a3c6a..a1e2cd46de6b 100644
> --- a/fs/ceph/metric.h
> +++ b/fs/ceph/metric.h
> @@ -169,6 +169,8 @@ struct ceph_client_metric {
>  	struct percpu_counter opened_inodes;
>  	struct percpu_counter total_inodes;
>  
> +	atomic64_t total_copyfrom;
> +
>  	struct ceph_mds_session *session;
>  	struct delayed_work delayed_work;  /* delayed work */
>  };


I know the main interest currently is just the count of ops, but I do
think that we'll want a full set of stats like we track for
reads/writes, and I'd rather not rev the file format any more than we
need to.

Could you extend struct ceph_client_metric with a full set of copy stats
and plumb in the places to record and report them? It should be pretty
similar to how reads/writes are tracked.
-- 
Jeff Layton <jlayton@kernel.org>