[PATCH] blktrace: move trace/ dir to /sys/block/sda/

[PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Li Zefan]

Impact: allow ftrace-plugin blktrace to trace device-mapper devices

blktrace can't trace a single partition, so it makes no sense to
have one trace/ dir in each /sys/block/sda/sdaX. Move it to
/sys/block/sda/.

Thus we fix an issue reported by Ted, that ftrace-plugin blktrace
can't be used to trace device-mapper devices.

Now:

  # echo 1 > /sys/block/dm-0/trace/enable
  echo: write error: No such device or address
  # mount -t ext4 /dev/dm-0 /mnt
  # echo 1 > /sys/block/dm-0/trace/enable
  # echo blk > /debug/tracing/current_tracer

Reported-by: Theodore Tso <tytso@mit.edu>
Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 block/blk-sysfs.c            |    7 ++++++-
 fs/partitions/check.c        |    3 ---
 include/linux/blktrace_api.h |    7 ++++++-
 kernel/trace/blktrace.c      |    7 ++++++-
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 73f36be..8653d71 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -387,16 +387,21 @@ struct kobj_type blk_queue_ktype = {
 int blk_register_queue(struct gendisk *disk)
 {
 	int ret;
+	struct device *dev = disk_to_dev(disk);
 
 	struct request_queue *q = disk->queue;
 
 	if (WARN_ON(!q))
 		return -ENXIO;
 
+	ret = blk_trace_init_sysfs(dev);
+	if (ret)
+		return ret;
+
 	if (!q->request_fn)
 		return 0;
 
-	ret = kobject_add(&q->kobj, kobject_get(&disk_to_dev(disk)->kobj),
+	ret = kobject_add(&q->kobj, kobject_get(&dev->kobj),
 			  "%s", "queue");
 	if (ret < 0)
 		return ret;
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index 99e33ef..445fd2f 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -295,9 +295,6 @@ static struct attribute_group part_attr_group = {
 
 static struct attribute_group *part_attr_groups[] = {
 	&part_attr_group,
-#ifdef CONFIG_BLK_DEV_IO_TRACE
-	&blk_trace_attr_group,
-#endif
 	NULL
 };
 
diff --git a/include/linux/blktrace_api.h b/include/linux/blktrace_api.h
index d960889..518e32a 100644
--- a/include/linux/blktrace_api.h
+++ b/include/linux/blktrace_api.h
@@ -197,7 +197,7 @@ extern int blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
 extern int blk_trace_startstop(struct request_queue *q, int start);
 extern int blk_trace_remove(struct request_queue *q);
 
-extern struct attribute_group blk_trace_attr_group;
+extern int blk_trace_init_sysfs(struct device *dev);
 
 #else /* !CONFIG_BLK_DEV_IO_TRACE */
 #define blk_trace_ioctl(bdev, cmd, arg)		(-ENOTTY)
@@ -209,6 +209,11 @@ extern struct attribute_group blk_trace_attr_group;
 #define blk_trace_remove(q)			(-ENOTTY)
 #define blk_add_trace_msg(q, fmt, ...)		do { } while (0)
 
+static inline int blk_trace_init_sysfs(struct device *dev)
+{
+	return 0;
+}
+
 #endif /* CONFIG_BLK_DEV_IO_TRACE */
 #endif /* __KERNEL__ */
 #endif
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 2b98195..9bae35f 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -1420,7 +1420,7 @@ static struct attribute *blk_trace_attrs[] = {
 	NULL
 };
 
-struct attribute_group blk_trace_attr_group = {
+static struct attribute_group blk_trace_attr_group = {
 	.name  = "trace",
 	.attrs = blk_trace_attrs,
 };
@@ -1620,3 +1620,8 @@ out:
 	return ret ? ret : count;
 }
 
+int blk_trace_init_sysfs(struct device *dev)
+{
+	return sysfs_create_group(&dev->kobj, &blk_trace_attr_group);
+}
+
-- 
1.5.4.rc3

Re: [PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Jens Axboe]

On Mon, Apr 13 2009, Li Zefan wrote:
> Impact: allow ftrace-plugin blktrace to trace device-mapper devices
> 
> blktrace can't trace a single partition, so it makes no sense to
> have one trace/ dir in each /sys/block/sda/sdaX. Move it to
> /sys/block/sda/.
> 
> Thus we fix an issue reported by Ted, that ftrace-plugin blktrace
> can't be used to trace device-mapper devices.

Perhaps I never committed that patch, but it would be trivial to do
partition based blktrace tracing. It's also quite useful. So please
don't go changing things to make that harder to support, it would be
nicer to just add the (small) bits to support per-partition tracing.
It's basically just a start/stop sector range, while some events are
per-device and should just be included always.

-- 
Jens Axboe

Re: [PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Arnaldo Carvalho de Melo]

Em Mon, Apr 13, 2009 at 08:00:54PM +0200, Jens Axboe escreveu:
> On Mon, Apr 13 2009, Li Zefan wrote:
> > Impact: allow ftrace-plugin blktrace to trace device-mapper devices
> > 
> > blktrace can't trace a single partition, so it makes no sense to
> > have one trace/ dir in each /sys/block/sda/sdaX. Move it to
> > /sys/block/sda/.
> > 
> > Thus we fix an issue reported by Ted, that ftrace-plugin blktrace
> > can't be used to trace device-mapper devices.
> 
> Perhaps I never committed that patch, but it would be trivial to do
> partition based blktrace tracing. It's also quite useful. So please
> don't go changing things to make that harder to support, it would be
> nicer to just add the (small) bits to support per-partition tracing.
> It's basically just a start/stop sector range, while some events are
> per-device and should just be included always.

I was a bit skeptical about this one as well, the fact that partition
based tracing was not available annoyed me, but as I was concentrating
just on getting the ftrace plugin merged and not on actually improving
what was there before... Anyway, will continue listening to these
discussions, blktrace was such a early champion for kernel tracing
infrastructure that it _has_ to be studied properly in light of current
tracing woodstock 8-)

- Arnaldo

Re: [PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Ingo Molnar]

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

> On Mon, Apr 13 2009, Li Zefan wrote:
> > Impact: allow ftrace-plugin blktrace to trace device-mapper devices
> > 
> > blktrace can't trace a single partition, so it makes no sense to
> > have one trace/ dir in each /sys/block/sda/sdaX. Move it to
> > /sys/block/sda/.
> > 
> > Thus we fix an issue reported by Ted, that ftrace-plugin 
> > blktrace can't be used to trace device-mapper devices.
> 
> Perhaps I never committed that patch, but it would be trivial to 
> do partition based blktrace tracing. It's also quite useful. So 
> please don't go changing things to make that harder to support, it 
> would be nicer to just add the (small) bits to support 
> per-partition tracing. It's basically just a start/stop sector 
> range, while some events are per-device and should just be 
> included always.

btw., per tracepoint filters could be used for that. That would 
allow multiple partitions to be traced at once.

blktrace user-space could make use of sector range filters straight 
away [ hm, Tom - do we have the <= comparison operator already, or 
is that still WIP? ] - but i think it's better to do this in a more 
integrated way: via the sysfs API, via /sys/block/sda/sda2/trace/.

So when a partition's trace entry is activated, it would 
auto-install a specific filter expression for sda, with the sector 
range of that partition. Or something like that. How does this 
sound?

	Ingo

Re: [PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Li Zefan]

Jens Axboe wrote:
> On Mon, Apr 13 2009, Li Zefan wrote:
>> Impact: allow ftrace-plugin blktrace to trace device-mapper devices
>>
>> blktrace can't trace a single partition, so it makes no sense to
>> have one trace/ dir in each /sys/block/sda/sdaX. Move it to
>> /sys/block/sda/.
>>
>> Thus we fix an issue reported by Ted, that ftrace-plugin blktrace
>> can't be used to trace device-mapper devices.
> 
> Perhaps I never committed that patch, but it would be trivial to do
> partition based blktrace tracing. It's also quite useful. So please
> don't go changing things to make that harder to support, it would be
> nicer to just add the (small) bits to support per-partition tracing.
> It's basically just a start/stop sector range, while some events are
> per-device and should just be included always.
> 

Ok, I found that patch in btrace mailing list. I'll rebase it and
send it out.

How about just add trace/ to /sys/block/sda? Then if we want to trace
the whole sda, we can:
  # echo 1 > /sys/block/sda/enable
If we want to trace a single partition:
  # echo 1 > /sys/block/sda/sda1/enable

Like "btrace /dev/sda" and "btrace /dev/sda1" when using userspace blktrace.

And when this is done, tracing device-mapper is supported, and I think
current md devices can't be traced by ftrace-plugin blktrace too.

--
Zefan

Re: [PATCH] blktrace: move trace/ dir to /sys/block/sda/

[Tom Zanussi]

On Mon, 2009-04-13 at 22:11 +0200, Ingo Molnar wrote:
> * Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Mon, Apr 13 2009, Li Zefan wrote:
> > > Impact: allow ftrace-plugin blktrace to trace device-mapper devices
> > > 
> > > blktrace can't trace a single partition, so it makes no sense to
> > > have one trace/ dir in each /sys/block/sda/sdaX. Move it to
> > > /sys/block/sda/.
> > > 
> > > Thus we fix an issue reported by Ted, that ftrace-plugin 
> > > blktrace can't be used to trace device-mapper devices.
> > 
> > Perhaps I never committed that patch, but it would be trivial to 
> > do partition based blktrace tracing. It's also quite useful. So 
> > please don't go changing things to make that harder to support, it 
> > would be nicer to just add the (small) bits to support 
> > per-partition tracing. It's basically just a start/stop sector 
> > range, while some events are per-device and should just be 
> > included always.
> 
> btw., per tracepoint filters could be used for that. That would 
> allow multiple partitions to be traced at once.
> 
> blktrace user-space could make use of sector range filters straight 
> away [ hm, Tom - do we have the <= comparison operator already, or 
> is that still WIP? ] - but i think it's better to do this in a more 

The new comparison operators aren't there yet, but will be part of the
new parser, which I'll be getting back to this week.

Tom

> integrated way: via the sysfs API, via /sys/block/sda/sda2/trace/.
> 
> So when a partition's trace entry is activated, it would 
> auto-install a specific filter expression for sda, with the sector 
> range of that partition. Or something like that. How does this 
> sound?
> 
> 	Ingo