[RFC][PATCH] blktrace: fix original blktrace

[RFC][PATCH] blktrace: fix original blktrace

[Li Zefan]

I'm wondering what we are going to do with the original blktrace which
is using relay and is used via ioctl.

The problem is currently it's totally broken. You can use ftrace to see
the output of blktrace, but user-space blktrace is unusable.

With this patch, both ioctl and ftrace can be used, but of course you
can't use both of them at the same time.

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/trace/blktrace.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 6fb274f..d8dc48b 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -110,7 +110,7 @@ void __trace_note_message(struct blk_trace *bt, const char *fmt, ...)
 	unsigned long flags;
 	char *buf;
 
-	if (blk_tr) {
+	if (blk_tracer_enabled) {
 		va_start(args, fmt);
 		ftrace_vprintk(fmt, args);
 		va_end(args);
@@ -169,7 +169,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 	pid_t pid;
 	int cpu, pc = 0;
 
-	if (unlikely(bt->trace_state != Blktrace_running ||
+	if (unlikely(bt->trace_state != Blktrace_running &&
 		     !blk_tracer_enabled))
 		return;
 
@@ -185,7 +185,7 @@ static void __blk_add_trace(struct blk_trace *bt, sector_t sector, int bytes,
 		return;
 	cpu = raw_smp_processor_id();
 
-	if (blk_tr) {
+	if (blk_tracer_enabled) {
 		tracing_record_cmdline(current);
 
 		pc = preempt_count();
@@ -235,7 +235,7 @@ record_it:
 		if (pdu_len)
 			memcpy((void *) t + sizeof(*t), pdu_data, pdu_len);
 
-		if (blk_tr) {
+		if (blk_tracer_enabled) {
 			trace_buffer_unlock_commit(blk_tr, event, 0, pc);
 			return;
 		}
@@ -1269,7 +1269,6 @@ static int blk_trace_setup_queue(struct request_queue *q, dev_t dev)
 	bt->dev = dev;
 	bt->act_mask = (u16)-1;
 	bt->end_lba = -1ULL;
-	bt->trace_state = Blktrace_running;
 
 	old_bt = xchg(&q->blk_trace, bt);
 	if (old_bt != NULL) {
-- 
1.5.4.rc3

Re: [RFC][PATCH] blktrace: fix original blktrace

[Jens Axboe]

On Wed, Mar 25 2009, Li Zefan wrote:
> I'm wondering what we are going to do with the original blktrace which
> is using relay and is used via ioctl.
> 
> The problem is currently it's totally broken. You can use ftrace to see
> the output of blktrace, but user-space blktrace is unusable.
> 
> With this patch, both ioctl and ftrace can be used, but of course you
> can't use both of them at the same time.

Even if ftrace was as fast as storing huge amounts of data as blktrace,
it's still of utmost importance that nothing is broken there. There are
people actually USING this tracing to do real work, it's not a
playground.

I appreciate the efforts to unify and improve our tracing, but we must
not be breaking blktrace along the way. Otherwise the whole thing goes
back to block/blktrace.c, period.

-- 
Jens Axboe

Re: [RFC][PATCH] blktrace: fix original blktrace

[Li Zefan]

Jens Axboe wrote:
> On Wed, Mar 25 2009, Li Zefan wrote:
>> I'm wondering what we are going to do with the original blktrace which
>> is using relay and is used via ioctl.
>>
>> The problem is currently it's totally broken. You can use ftrace to see
>> the output of blktrace, but user-space blktrace is unusable.
>>
>> With this patch, both ioctl and ftrace can be used, but of course you
>> can't use both of them at the same time.
> 
> Even if ftrace was as fast as storing huge amounts of data as blktrace,
> it's still of utmost importance that nothing is broken there. There are

agreed

> people actually USING this tracing to do real work, it's not a
> playground.
> 

and we are using blktrace to test the cgroup-based io controller

> I appreciate the efforts to unify and improve our tracing, but we must
> not be breaking blktrace along the way. Otherwise the whole thing goes

this patch fixes it.

it's broken by "blktrace: add ftrace plugin"
(c71a896154119f4ca9e89d6078f5f63ad60ef199)

@@ -131,13 +162,14 @@ static void __blk_add_trace()
- 	if (unlikely(bt->trace_state != Blktrace_running))
+ 	if (unlikely(bt->trace_state != Blktrace_running || !blk_tracer_enabled))
 		return;

> back to block/blktrace.c, period.
> 

Re: [RFC][PATCH] blktrace: fix original blktrace

[Jens Axboe]

On Wed, Mar 25 2009, Li Zefan wrote:
> Jens Axboe wrote:
> > On Wed, Mar 25 2009, Li Zefan wrote:
> >> I'm wondering what we are going to do with the original blktrace which
> >> is using relay and is used via ioctl.
> >>
> >> The problem is currently it's totally broken. You can use ftrace to see
> >> the output of blktrace, but user-space blktrace is unusable.
> >>
> >> With this patch, both ioctl and ftrace can be used, but of course you
> >> can't use both of them at the same time.
> > 
> > Even if ftrace was as fast as storing huge amounts of data as blktrace,
> > it's still of utmost importance that nothing is broken there. There are
> 
> agreed
> 
> > people actually USING this tracing to do real work, it's not a
> > playground.
> > 
> 
> and we are using blktrace to test the cgroup-based io controller
> 
> > I appreciate the efforts to unify and improve our tracing, but we must
> > not be breaking blktrace along the way. Otherwise the whole thing goes
> 
> this patch fixes it.
> 
> it's broken by "blktrace: add ftrace plugin"
> (c71a896154119f4ca9e89d6078f5f63ad60ef199)
> 
> @@ -131,13 +162,14 @@ static void __blk_add_trace()
> - 	if (unlikely(bt->trace_state != Blktrace_running))
> + 	if (unlikely(bt->trace_state != Blktrace_running || !blk_tracer_enabled))
>  		return;

Good, as long as it still works with blktrace, that's all I care about
(for now, at least).

-- 
Jens Axboe

Re: [RFC][PATCH] blktrace: fix original blktrace

[Ingo Molnar]

* Jens Axboe <jens.axboe@oracle.com> wrote:

> On Wed, Mar 25 2009, Li Zefan wrote:
> > Jens Axboe wrote:
> > > On Wed, Mar 25 2009, Li Zefan wrote:
> > >> I'm wondering what we are going to do with the original blktrace which
> > >> is using relay and is used via ioctl.
> > >>
> > >> The problem is currently it's totally broken. You can use ftrace to see
> > >> the output of blktrace, but user-space blktrace is unusable.
> > >>
> > >> With this patch, both ioctl and ftrace can be used, but of course you
> > >> can't use both of them at the same time.
> > > 
> > > Even if ftrace was as fast as storing huge amounts of data as blktrace,
> > > it's still of utmost importance that nothing is broken there. There are
> > 
> > agreed
> > 
> > > people actually USING this tracing to do real work, it's not a
> > > playground.
> > > 
> > 
> > and we are using blktrace to test the cgroup-based io controller
> > 
> > > I appreciate the efforts to unify and improve our tracing, but we must
> > > not be breaking blktrace along the way. Otherwise the whole thing goes
> > 
> > this patch fixes it.
> > 
> > it's broken by "blktrace: add ftrace plugin"
> > (c71a896154119f4ca9e89d6078f5f63ad60ef199)
> > 
> > @@ -131,13 +162,14 @@ static void __blk_add_trace()
> > - 	if (unlikely(bt->trace_state != Blktrace_running))
> > + 	if (unlikely(bt->trace_state != Blktrace_running || !blk_tracer_enabled))
> >  		return;
> 
> Good, as long as it still works with blktrace, that's all I care 
> about (for now, at least).

Sorry about that. I've applied Li's fixes and double checked 
blktrace+blktrace functionality and pushed out a new tracing
tree to linux-next.

Btw., blktrace could be updated in the future to make use the
new per CPU buffering and sys_splice() code available in ftrace
plugin. [ splice is cool! =B-) ]

Plus blktrace could make use of built-in event filtering 
capabilities - for example to only trace events in a specific
sector range on the disk. Or to trace all IO of a given PID only.

But that is a different project that needs changes both on the 
kernel side and on the user-space side (.31-ish for sure) and the 
relayfs+ioctl method must work fine in any case.

	Ingo

Re: [RFC][PATCH] blktrace: fix original blktrace

[Jens Axboe]

On Wed, Mar 25 2009, Ingo Molnar wrote:
> 
> * Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Wed, Mar 25 2009, Li Zefan wrote:
> > > Jens Axboe wrote:
> > > > On Wed, Mar 25 2009, Li Zefan wrote:
> > > >> I'm wondering what we are going to do with the original blktrace which
> > > >> is using relay and is used via ioctl.
> > > >>
> > > >> The problem is currently it's totally broken. You can use ftrace to see
> > > >> the output of blktrace, but user-space blktrace is unusable.
> > > >>
> > > >> With this patch, both ioctl and ftrace can be used, but of course you
> > > >> can't use both of them at the same time.
> > > > 
> > > > Even if ftrace was as fast as storing huge amounts of data as blktrace,
> > > > it's still of utmost importance that nothing is broken there. There are
> > > 
> > > agreed
> > > 
> > > > people actually USING this tracing to do real work, it's not a
> > > > playground.
> > > > 
> > > 
> > > and we are using blktrace to test the cgroup-based io controller
> > > 
> > > > I appreciate the efforts to unify and improve our tracing, but we must
> > > > not be breaking blktrace along the way. Otherwise the whole thing goes
> > > 
> > > this patch fixes it.
> > > 
> > > it's broken by "blktrace: add ftrace plugin"
> > > (c71a896154119f4ca9e89d6078f5f63ad60ef199)
> > > 
> > > @@ -131,13 +162,14 @@ static void __blk_add_trace()
> > > - 	if (unlikely(bt->trace_state != Blktrace_running))
> > > + 	if (unlikely(bt->trace_state != Blktrace_running || !blk_tracer_enabled))
> > >  		return;
> > 
> > Good, as long as it still works with blktrace, that's all I care 
> > about (for now, at least).
> 
> Sorry about that. I've applied Li's fixes and double checked 
> blktrace+blktrace functionality and pushed out a new tracing
> tree to linux-next.
> 
> Btw., blktrace could be updated in the future to make use the
> new per CPU buffering and sys_splice() code available in ftrace
> plugin. [ splice is cool! =B-) ]
> 
> Plus blktrace could make use of built-in event filtering 
> capabilities - for example to only trace events in a specific
> sector range on the disk. Or to trace all IO of a given PID only.
> 
> But that is a different project that needs changes both on the 
> kernel side and on the user-space side (.31-ish for sure) and the 
> relayfs+ioctl method must work fine in any case.

Don't get me wrong, I completely agree with the unification idea, it'll
be awesome to have a single stream of info for several event types. My
worry was just if it was going too fast, I just don't want a broken
blktrace before the dust has even settled. And moving blktrace from
relay to per-cpu buffering and splice definitely sounds like a good
longer term direction.

-- 
Jens Axboe