[PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3

[PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3

[Namhyung Kim]

Hello,

The blktrace is used to report block device activities to user space
using kernel tracepoint but it was focused to block I/O request struct.
Thus bio-based devices (i.e. loop, ram, md, ...) which don't make use
of the request structure could not be supported well - the tool only
can detect a limited number of events such as queuing, cloning and
remapping but it cannot know when the I/O activity is completed.

bio_endio(), the I/O completion callback, can be used to fix this
problem by adding appropriate tracepoint in it. However it was called
from other paths too (normal request-based block devices and some of
nested block I/O handling routines) so that we should recognize such
cases to prevent duplicated reports. In this series, BIO_IN_FLIGHT flag
is introduced and used for that purpose.

Note that (bio-based) dm already supported completion report by adding
the tracepoint into the path manually. With this patches, it will be
converted to use generic mechanism.

Changes from v2:
 * rebased on current block/for-next

Changes from v1:
 * kill __bio_endio()
 * use BIO_IN_FLIGHT flag

Any feedbacks are welcome.

Thanks.


Namhyung Kim (2):
  block: introduce BIO_IN_FLIGHT flag
  block: don't export block_bio_complete tracepoint

 block/blk-core.c          |    6 +++++-
 drivers/md/dm.c           |    1 -
 fs/bio.c                  |    7 +++++++
 include/linux/blk_types.h |    1 +
 4 files changed, 13 insertions(+), 2 deletions(-)

--
1.7.6

[PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Namhyung Kim]

BIO_IN_FLIGHT flag is used for tracing block I/O completion.
This patch fixes tracing bio-based devices - except DM which
inserts completion tracepoint explicitly - that could not be
traced such event using blktrace.

It won't affect tracing normal (request-based) disk devices
and nested bio handling paths.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 block/blk-core.c          |    5 +++++
 fs/bio.c                  |    7 +++++++
 include/linux/blk_types.h |    1 +
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1b4fd93af2c0..f61310323954 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -165,6 +165,9 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
 	if (unlikely(rq->cmd_flags & REQ_QUIET))
 		set_bit(BIO_QUIET, &bio->bi_flags);
 
+	/* completion event was already reported in blk_update_request */
+	clear_bit(BIO_IN_FLIGHT, &bio->bi_flags);
+
 	bio->bi_size -= nbytes;
 	bio->bi_sector += (nbytes >> 9);
 
@@ -1662,6 +1665,8 @@ void generic_make_request(struct bio *bio)
 	do {
 		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
 
+		set_bit(BIO_IN_FLIGHT, &bio->bi_flags);
+
 		q->make_request_fn(q, bio);
 
 		bio = bio_list_pop(current->bio_list);
diff --git a/fs/bio.c b/fs/bio.c
index b1fe82cf88cf..10fb7a1d74ba 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1447,6 +1447,13 @@ void bio_endio(struct bio *bio, int error)
 	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
 		error = -EIO;
 
+	if (test_bit(BIO_IN_FLIGHT, &bio->bi_flags)) {
+		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+
+		trace_block_bio_complete(q, bio, error);
+		clear_bit(BIO_IN_FLIGHT, &bio->bi_flags);
+	}
+
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio, error);
 }
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd4490e..0b9aecd40ac5 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -95,6 +95,7 @@ struct bio {
 #define BIO_FS_INTEGRITY 9	/* fs owns integrity data, not block layer */
 #define BIO_QUIET	10	/* Make BIO Quiet */
 #define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
+#define BIO_IN_FLIGHT	12	/* report I/O completion event */
 #define bio_flagged(bio, flag)	((bio)->bi_flags & (1 << (flag)))
 
 /*
-- 
1.7.6

[PATCH 2/2] block: don't export block_bio_complete tracepoint

[Namhyung Kim]

Now bio_endio() contains the tracepoint in it, so we don't
need to have it twice in DM. Plus, as the only external user
of the tracepoint was the DM, we can unexport the symbol.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: dm-devel@redhat.com
---
 block/blk-core.c |    1 -
 drivers/md/dm.c  |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index f61310323954..f7eab1932543 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -37,7 +37,6 @@
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
-EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_complete);
 
 DEFINE_IDA(blk_queue_ida);
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 4720f68f817e..01185fa0eb74 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -648,7 +648,6 @@ static void dec_pending(struct dm_io *io, int error)
 			queue_io(md, bio);
 		} else {
 			/* done with normal IO or empty flush */
-			trace_block_bio_complete(md->queue, bio, io_error);
 			bio_endio(bio, io_error);
 		}
 	}
-- 
1.7.6

Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Tejun Heo]

On Tue, Dec 27, 2011 at 11:28:32PM +0900, Namhyung Kim wrote:
> BIO_IN_FLIGHT flag is used for tracing block I/O completion.
> This patch fixes tracing bio-based devices - except DM which
> inserts completion tracepoint explicitly - that could not be
> traced such event using blktrace.
> 
> It won't affect tracing normal (request-based) disk devices
> and nested bio handling paths.
> 
> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  block/blk-core.c          |    5 +++++
>  fs/bio.c                  |    7 +++++++
>  include/linux/blk_types.h |    1 +
>  3 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1b4fd93af2c0..f61310323954 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -165,6 +165,9 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
>  	if (unlikely(rq->cmd_flags & REQ_QUIET))
>  		set_bit(BIO_QUIET, &bio->bi_flags);
>  
> +	/* completion event was already reported in blk_update_request */
> +	clear_bit(BIO_IN_FLIGHT, &bio->bi_flags);
> +
>  	bio->bi_size -= nbytes;
>  	bio->bi_sector += (nbytes >> 9);
>  
> @@ -1662,6 +1665,8 @@ void generic_make_request(struct bio *bio)
p>  	do {
>  		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>  
> +		set_bit(BIO_IN_FLIGHT, &bio->bi_flags);
> +
>  		q->make_request_fn(q, bio);
>  
>  		bio = bio_list_pop(current->bio_list);
> diff --git a/fs/bio.c b/fs/bio.c
> index b1fe82cf88cf..10fb7a1d74ba 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -1447,6 +1447,13 @@ void bio_endio(struct bio *bio, int error)
>  	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
>  		error = -EIO;
>  
> +	if (test_bit(BIO_IN_FLIGHT, &bio->bi_flags)) {
> +		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> +
> +		trace_block_bio_complete(q, bio, error);
> +		clear_bit(BIO_IN_FLIGHT, &bio->bi_flags);
> +	}

This really should be filtered from the consumer side.  Not on TP
itself.  Doing it like this makes the TP useless for other consumers
and adds unnecessary flag and operations on it even when the TP is not
in use.  Why not just trigger the TP on boi_endio() and let blktrace
probe filter out bounced or cloned completions?

Thanks.

--
tejun

Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Namhyung Kim]

2012-01-09 10:56 AM, Tejun Heo wrote:
> On Tue, Dec 27, 2011 at 11:28:32PM +0900, Namhyung Kim wrote:
>> BIO_IN_FLIGHT flag is used for tracing block I/O completion.
>> This patch fixes tracing bio-based devices - except DM which
>> inserts completion tracepoint explicitly - that could not be
>> traced such event using blktrace.
>>
>> It won't affect tracing normal (request-based) disk devices
>> and nested bio handling paths.
>>
>> Signed-off-by: Namhyung Kim<namhyung@gmail.com>
>> ---
>>   block/blk-core.c          |    5 +++++
>>   fs/bio.c                  |    7 +++++++
>>   include/linux/blk_types.h |    1 +
>>   3 files changed, 13 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 1b4fd93af2c0..f61310323954 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -165,6 +165,9 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
>>   	if (unlikely(rq->cmd_flags&  REQ_QUIET))
>>   		set_bit(BIO_QUIET,&bio->bi_flags);
>>
>> +	/* completion event was already reported in blk_update_request */
>> +	clear_bit(BIO_IN_FLIGHT,&bio->bi_flags);
>> +
>>   	bio->bi_size -= nbytes;
>>   	bio->bi_sector += (nbytes>>  9);
>>
>> @@ -1662,6 +1665,8 @@ void generic_make_request(struct bio *bio)
> p>   	do {
>>   		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>>
>> +		set_bit(BIO_IN_FLIGHT,&bio->bi_flags);
>> +
>>   		q->make_request_fn(q, bio);
>>
>>   		bio = bio_list_pop(current->bio_list);
>> diff --git a/fs/bio.c b/fs/bio.c
>> index b1fe82cf88cf..10fb7a1d74ba 100644
>> --- a/fs/bio.c
>> +++ b/fs/bio.c
>> @@ -1447,6 +1447,13 @@ void bio_endio(struct bio *bio, int error)
>>   	else if (!test_bit(BIO_UPTODATE,&bio->bi_flags))
>>   		error = -EIO;
>>
>> +	if (test_bit(BIO_IN_FLIGHT,&bio->bi_flags)) {
>> +		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
>> +
>> +		trace_block_bio_complete(q, bio, error);
>> +		clear_bit(BIO_IN_FLIGHT,&bio->bi_flags);
>> +	}
>
> This really should be filtered from the consumer side.  Not on TP
> itself.  Doing it like this makes the TP useless for other consumers
> and adds unnecessary flag and operations on it even when the TP is not
> in use.  Why not just trigger the TP on boi_endio() and let blktrace
> probe filter out bounced or cloned completions?

I understand your concerns. However, the blktrace cannot get 
bio->bi_flags info in its current form AFAIK. Doing it will require 
extending struct blk_io_trace and it'll cause a compatibility issue,
I guess.

Anyway I think the needs of tracing bio based drivers are growing so 
it'd be great the blktrace supports it as well. Also as we've run out of 
act_mask bits, it might be needed a room for extending.

But before proceeding, I'd like to listen comments/advices from people. 
Jens?

Thanks,
Namhyung Kim

Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Tejun Heo]

Hello,

On Mon, Jan 09, 2012 at 11:57:43AM +0900, Namhyung Kim wrote:
> I understand your concerns. However, the blktrace cannot get
> bio->bi_flags info in its current form AFAIK. Doing it will require
> extending struct blk_io_trace and it'll cause a compatibility issue,
> I guess.

Umm?  Why can't blk_add_trace_bio_complete() look at the flags (or
whatever other states) to decide to fire off BLK_TA_COMPLETE or not?
What's the difference?  No userland visible change is necessary at
all.  Just make blktrace.c do the right thing.  Am I missing
something?

Thanks.

-- 
tejun

Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Namhyung Kim]

Hi,

2012-01-10 1:41 AM, Tejun Heo wrote:
> Hello,
>
> On Mon, Jan 09, 2012 at 11:57:43AM +0900, Namhyung Kim wrote:
>> I understand your concerns. However, the blktrace cannot get
>> bio->bi_flags info in its current form AFAIK. Doing it will require
>> extending struct blk_io_trace and it'll cause a compatibility issue,
>> I guess.
>
> Umm?  Why can't blk_add_trace_bio_complete() look at the flags (or
> whatever other states) to decide to fire off BLK_TA_COMPLETE or not?
> What's the difference?  No userland visible change is necessary at
> all.  Just make blktrace.c do the right thing.  Am I missing
> something?
>
> Thanks.
>

Oh I misunderstood what you said. I was thinking about filtering in pure 
userspace, but you meant in-kernel probe side.

Right, we can change the probe to filter BIO_BOUNCED case out. But IMHO 
BIO_CLONED is different, since it usually routed to another device as a 
separate IO request. I'll cook a patch for the former soon.

Thanks for the comment.
Namhyung Kim

Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag

[Tejun Heo]

Hello,

On Tue, Jan 10, 2012 at 02:06:56PM +0900, Namhyung Kim wrote:
> Oh I misunderstood what you said. I was thinking about filtering in
> pure userspace, but you meant in-kernel probe side.

Yeah yeah.

> Right, we can change the probe to filter BIO_BOUNCED case out. But
> IMHO BIO_CLONED is different, since it usually routed to another
> device as a separate IO request. I'll cook a patch for the former
> soon.

It'll probably get dup'd completions from rq and bio completions for
request based drivers, right?  Maybe there are better ways but
blktrace may as well just ignore bio completions if request
completions are happening.

Thanks.

-- 
tejun