diff mbox series

[v6,8/9] ceph: add reset metrics support

Message ID 20200210053407.37237-9-xiubli@redhat.com (mailing list archive)
State New, archived
Headers show
Series ceph: add perf metrics support | expand

Commit Message

Xiubo Li Feb. 10, 2020, 5:34 a.m. UTC 
From: Xiubo Li <xiubli@redhat.com>

Sometimes we need to discard the old perf metrics and start to get
new ones. And this will reset the most metric counters, except the
total numbers for caps and dentries.

URL: https://tracker.ceph.com/issues/43215
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/ceph/debugfs.c | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Comments

Ilya Dryomov Feb. 10, 2020, 3:22 p.m. UTC | #1 
On Mon, Feb 10, 2020 at 6:34 AM <xiubli@redhat.com> wrote:
>
> From: Xiubo Li <xiubli@redhat.com>
>
> Sometimes we need to discard the old perf metrics and start to get
> new ones. And this will reset the most metric counters, except the
> total numbers for caps and dentries.
>
> URL: https://tracker.ceph.com/issues/43215
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/ceph/debugfs.c | 38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
> index 60f3e307fca1..6e595a37af5d 100644
> --- a/fs/ceph/debugfs.c
> +++ b/fs/ceph/debugfs.c
> @@ -179,6 +179,43 @@ static int metric_show(struct seq_file *s, void *p)
>         return 0;
>  }
>
> +static ssize_t metric_store(struct file *file, const char __user *user_buf,
> +                           size_t count, loff_t *ppos)
> +{
> +       struct seq_file *s = file->private_data;
> +       struct ceph_fs_client *fsc = s->private;
> +       struct ceph_mds_client *mdsc = fsc->mdsc;
> +       struct ceph_client_metric *metric = &mdsc->metric;
> +       char buf[8];
> +
> +       if (copy_from_user(buf, user_buf, 8))
> +               return -EFAULT;
> +
> +       if (strncmp(buf, "reset", strlen("reset"))) {
> +               pr_err("Invalid set value '%s', only 'reset' is valid\n", buf);
> +               return -EINVAL;
> +       }

Hi Xiubo,

Why strncmp?  How does this handle inputs like "resetfoobar"?

> +
> +       percpu_counter_set(&metric->d_lease_hit, 0);
> +       percpu_counter_set(&metric->d_lease_mis, 0);
> +
> +       percpu_counter_set(&metric->i_caps_hit, 0);
> +       percpu_counter_set(&metric->i_caps_mis, 0);
> +
> +       percpu_counter_set(&metric->read_latency_sum, 0);
> +       percpu_counter_set(&metric->total_reads, 0);
> +
> +       percpu_counter_set(&metric->write_latency_sum, 0);
> +       percpu_counter_set(&metric->total_writes, 0);
> +
> +       percpu_counter_set(&metric->metadata_latency_sum, 0);
> +       percpu_counter_set(&metric->total_metadatas, 0);
> +
> +       return count;
> +}
> +
> +CEPH_DEFINE_RW_FUNC(metric);

More broadly, how are these metrics going to be used?  I suspect
the MDSes will gradually start relying on the them in the future
and probably make decisions based off of them?  If that is the case,
did you think about clients being able to mess with that by zeroing
these counters on a regular basis?

It looks like all of this is still in flight on the userspace side, but
I don't see anything similar in https://github.com/ceph/ceph/pull/32120.
Is there a different PR or is this kernel-only for some reason?

Thanks,

                Ilya
diff mbox series

Patch

diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c
index 60f3e307fca1..6e595a37af5d 100644
--- a/fs/ceph/debugfs.c
+++ b/fs/ceph/debugfs.c
@@ -179,6 +179,43 @@  static int metric_show(struct seq_file *s, void *p)
 	return 0;
 }
 
+static ssize_t metric_store(struct file *file, const char __user *user_buf,
+			    size_t count, loff_t *ppos)
+{
+	struct seq_file *s = file->private_data;
+	struct ceph_fs_client *fsc = s->private;
+	struct ceph_mds_client *mdsc = fsc->mdsc;
+	struct ceph_client_metric *metric = &mdsc->metric;
+	char buf[8];
+
+	if (copy_from_user(buf, user_buf, 8))
+		return -EFAULT;
+
+	if (strncmp(buf, "reset", strlen("reset"))) {
+		pr_err("Invalid set value '%s', only 'reset' is valid\n", buf);
+		return -EINVAL;
+	}
+
+	percpu_counter_set(&metric->d_lease_hit, 0);
+	percpu_counter_set(&metric->d_lease_mis, 0);
+
+	percpu_counter_set(&metric->i_caps_hit, 0);
+	percpu_counter_set(&metric->i_caps_mis, 0);
+
+	percpu_counter_set(&metric->read_latency_sum, 0);
+	percpu_counter_set(&metric->total_reads, 0);
+
+	percpu_counter_set(&metric->write_latency_sum, 0);
+	percpu_counter_set(&metric->total_writes, 0);
+
+	percpu_counter_set(&metric->metadata_latency_sum, 0);
+	percpu_counter_set(&metric->total_metadatas, 0);
+
+	return count;
+}
+
+CEPH_DEFINE_RW_FUNC(metric);
+
 static int caps_show_cb(struct inode *inode, struct ceph_cap *cap, void *p)
 {
 	struct seq_file *s = p;
@@ -277,7 +314,6 @@  DEFINE_SHOW_ATTRIBUTE(mdsmap);
 DEFINE_SHOW_ATTRIBUTE(mdsc);
 DEFINE_SHOW_ATTRIBUTE(caps);
 DEFINE_SHOW_ATTRIBUTE(mds_sessions);
-DEFINE_SHOW_ATTRIBUTE(metric);
 
 
 /*