[RFC] Trace-events for mtd

[RFC] Trace-events for mtd

[Daniel Walter]

Hi,

I currently working on ways to find out
if any processes are producing heavy I/O
loads on a mtd device.
I've implemented traceevents for mtd_read and
mtd_write which allow us to at least get a
rough overview on the read/write load.
Although it kinda works, I could not find a
way to find out which processes actually produce
write load, since all writes are writebacks from
page_cache and therefor we mostly get a kworker thread
as the main origin of any write load.

I wanted to reach out to the broader community to find
out if I missed somethings and to get overall feedback
if more traceevents would be useful to help us
debugging mtd i/o a little bit better.

any comments are highly appreciated.

Cheers,

daniel

[RFC] [PATCH] mtd: add tracepoints for mtd_read and mtd_write

[Daniel Walter]

Add tracepoints to mtd subsystem for read and write
operations. This should allow us to identify applications
which are generating too much reads/writes on a flash-device.

Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
---
 drivers/mtd/mtdcore.c      |  6 +++++-
 include/trace/events/mtd.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)
 create mode 100644 include/trace/events/mtd.h

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 8bbbb75..8f85bfb 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -43,6 +43,9 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/mtd.h>
+
 #include "mtdcore.h"
 
 static struct backing_dev_info mtd_bdi = {
@@ -882,7 +885,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
 		return -EINVAL;
 	if (!len)
 		return 0;
-
+	trace_mtd_read(current, len);
 	/*
 	 * In the absence of an error, drivers return a non-negative integer
 	 * representing the maximum number of bitflips that were corrected on
@@ -907,6 +910,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
 		return -EROFS;
 	if (!len)
 		return 0;
+	trace_mtd_write(current, len);
 	return mtd->_write(mtd, to, len, retlen, buf);
 }
 EXPORT_SYMBOL_GPL(mtd_write);
diff --git a/include/trace/events/mtd.h b/include/trace/events/mtd.h
new file mode 100644
index 0000000..7191b72
--- /dev/null
+++ b/include/trace/events/mtd.h
@@ -0,0 +1,45 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM mtd
+
+#if !defined(_TRACE_MTD_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_MTD_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(mtd_read,
+	TP_PROTO(struct task_struct *task, size_t rlen),
+	TP_ARGS(task, rlen),
+	TP_STRUCT__entry(
+		__field(pid_t, pid)
+		__array(char, comm, TASK_COMM_LEN)
+		__field(size_t, rlen)
+	),
+	TP_fast_assign(
+		__entry->pid = task->pid;
+		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->rlen = rlen;
+	),
+	TP_printk("pid=%d comm=%s rlen=%zu",
+		__entry->pid, __entry->comm, __entry->rlen)
+);
+
+TRACE_EVENT(mtd_write,
+	TP_PROTO(struct task_struct *task, size_t wlen),
+	TP_ARGS(task, wlen),
+	TP_STRUCT__entry(
+		__field(pid_t, pid)
+		__array(char, comm, TASK_COMM_LEN)
+		__field(size_t, wlen)
+	),
+	TP_fast_assign(
+		__entry->pid = task->pid;
+		memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
+		__entry->wlen = wlen;
+	),
+	TP_printk("pid=%d comm=%s wlen=%zu",
+		__entry->pid, __entry->comm, __entry->wlen)
+);
+#endif /* _TRACE_MTD_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
2.6.2

Re: [RFC] [PATCH] mtd: add tracepoints for mtd_read and mtd_write

[Richard Weinberger]

On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
> Add tracepoints to mtd subsystem for read and write
> operations. This should allow us to identify applications
> which are generating too much reads/writes on a flash-device.
>
> Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
> ---
>  drivers/mtd/mtdcore.c      |  6 +++++-
>  include/trace/events/mtd.h | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletion(-)
>  create mode 100644 include/trace/events/mtd.h
>
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 8bbbb75..8f85bfb 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -43,6 +43,9 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/mtd.h>
> +
>  #include "mtdcore.h"
>
>  static struct backing_dev_info mtd_bdi = {
> @@ -882,7 +885,7 @@ int mtd_read(struct mtd_info *mtd, loff_t from, size_t len, size_t *retlen,
>                 return -EINVAL;
>         if (!len)
>                 return 0;
> -
> +       trace_mtd_read(current, len);
>         /*
>          * In the absence of an error, drivers return a non-negative integer
>          * representing the maximum number of bitflips that were corrected on
> @@ -907,6 +910,7 @@ int mtd_write(struct mtd_info *mtd, loff_t to, size_t len, size_t *retlen,
>                 return -EROFS;
>         if (!len)
>                 return 0;
> +       trace_mtd_write(current, len);
>         return mtd->_write(mtd, to, len, retlen, buf);
>  }
>  EXPORT_SYMBOL_GPL(mtd_write);
> diff --git a/include/trace/events/mtd.h b/include/trace/events/mtd.h
> new file mode 100644
> index 0000000..7191b72
> --- /dev/null
> +++ b/include/trace/events/mtd.h
> @@ -0,0 +1,45 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM mtd
> +
> +#if !defined(_TRACE_MTD_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_MTD_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(mtd_read,
> +       TP_PROTO(struct task_struct *task, size_t rlen),
> +       TP_ARGS(task, rlen),
> +       TP_STRUCT__entry(
> +               __field(pid_t, pid)
> +               __array(char, comm, TASK_COMM_LEN)
> +               __field(size_t, rlen)
> +       ),
> +       TP_fast_assign(
> +               __entry->pid = task->pid;
> +               memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
> +               __entry->rlen = rlen;
> +       ),
> +       TP_printk("pid=%d comm=%s rlen=%zu",
> +               __entry->pid, __entry->comm, __entry->rlen)
> +);
> +
> +TRACE_EVENT(mtd_write,
> +       TP_PROTO(struct task_struct *task, size_t wlen),
> +       TP_ARGS(task, wlen),
> +       TP_STRUCT__entry(
> +               __field(pid_t, pid)
> +               __array(char, comm, TASK_COMM_LEN)
> +               __field(size_t, wlen)
> +       ),
> +       TP_fast_assign(
> +               __entry->pid = task->pid;
> +               memcpy(__entry->comm, task->comm, TASK_COMM_LEN);
> +               __entry->wlen = wlen;
> +       ),
> +       TP_printk("pid=%d comm=%s wlen=%zu",
> +               __entry->pid, __entry->comm, __entry->wlen)
> +);
> +#endif /* _TRACE_MTD_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>

Brian, any comments on this?

-- 
Thanks,
//richard

Re: [RFC] [PATCH] mtd: add tracepoints for mtd_read and mtd_write

[Brian Norris]

On Fri, Jan 08, 2016 at 10:33:03AM +0100, Richard Weinberger wrote:
> On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
> > Add tracepoints to mtd subsystem for read and write
> > operations. This should allow us to identify applications
> > which are generating too much reads/writes on a flash-device.
> >
> > Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
> 
> Brian, any comments on this?

Only a few. I'm not very familiar with writing trace events. I've only
barely used them! And I haven't run this patch yet.

But I thought Daniel reported having some difficulty here -- that it
isn't actually that useful to get real "user" information, since most
writing/erasing happens from background UBI tasks in practice. So is
this still the best mechanism for tracking this information? (Even if
there's better ways to get info directly out of UBI, I guess maybe it'd
still be good to have this infrastructure.)

Also, maybe I'm missing something, but it doesn't look like this
attempts to record *which* MTD is being accessed, right? I'd think
logging mtd->name could help?

Brian

Re: [RFC] Trace-events for mtd

[Brian Norris]

On Tue, Nov 17, 2015 at 08:14:48PM +0100, Daniel Walter wrote:
> I wanted to reach out to the broader community to find
> out if I missed somethings and to get overall feedback
> if more traceevents would be useful to help us
> debugging mtd i/o a little bit better.

Regarding this point: is there a good reason not to track mtd_erase()?
How about mtd_{read,write}_oob()? (NB: The latter can be used to get
both in-band and out-of-band data.)

Brian

Re: [RFC] [PATCH] mtd: add tracepoints for mtd_read and mtd_write

[Daniel Walter]

On 01/08/2016 06:57 PM, Brian Norris wrote:
> On Fri, Jan 08, 2016 at 10:33:03AM +0100, Richard Weinberger wrote:
>> On Tue, Nov 17, 2015 at 8:18 PM, Daniel Walter <dwalter@sigma-star.at> wrote:
>>> Add tracepoints to mtd subsystem for read and write
>>> operations. This should allow us to identify applications
>>> which are generating too much reads/writes on a flash-device.
>>>
>>> Signed-off-by: Daniel Walter <dwalter@sigma-star.at>
>>
>> Brian, any comments on this?
> 
> Only a few. I'm not very familiar with writing trace events. I've only
> barely used them! And I haven't run this patch yet.
> 
> But I thought Daniel reported having some difficulty here -- that it
> isn't actually that useful to get real "user" information, since most
> writing/erasing happens from background UBI tasks in practice. So is
> this still the best mechanism for tracking this information? (Even if
> there's better ways to get info directly out of UBI, I guess maybe it'd
> still be good to have this infrastructure.)
> 
> Also, maybe I'm missing something, but it doesn't look like this
> attempts to record *which* MTD is being accessed, right? I'd think
> logging mtd->name could help?
> 
> Brian
> 
Hi,

sorry for the late reply, filters ate your mail :/
I'll add mtd->name, since this sounds like a great idea.
regarding your other mail (mtd_erase/read/write_oob), I'll add
tracepoints for them as well (they have been on my list anyway).

Thanks for the input, expect a v2 later this week.

cheers,

daniel