* [PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3 @ 2011-12-27 14:28 Namhyung Kim 2011-12-27 14:28 ` [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag Namhyung Kim 2011-12-27 14:28 ` [PATCH 2/2] block: don't export block_bio_complete tracepoint Namhyung Kim 0 siblings, 2 replies; 8+ messages in thread From: Namhyung Kim @ 2011-12-27 14:28 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2011-12-27 14:28 [PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3 Namhyung Kim @ 2011-12-27 14:28 ` Namhyung Kim 2012-01-09 1:56 ` Tejun Heo 2011-12-27 14:28 ` [PATCH 2/2] block: don't export block_bio_complete tracepoint Namhyung Kim 1 sibling, 1 reply; 8+ messages in thread From: Namhyung Kim @ 2011-12-27 14:28 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-fsdevel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2011-12-27 14:28 ` [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag Namhyung Kim @ 2012-01-09 1:56 ` Tejun Heo 2012-01-09 2:57 ` Namhyung Kim 0 siblings, 1 reply; 8+ messages in thread From: Tejun Heo @ 2012-01-09 1:56 UTC (permalink / raw) To: Namhyung Kim; +Cc: Jens Axboe, linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2012-01-09 1:56 ` Tejun Heo @ 2012-01-09 2:57 ` Namhyung Kim 2012-01-09 16:41 ` Tejun Heo 0 siblings, 1 reply; 8+ messages in thread From: Namhyung Kim @ 2012-01-09 2:57 UTC (permalink / raw) To: Tejun Heo; +Cc: Jens Axboe, linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2012-01-09 2:57 ` Namhyung Kim @ 2012-01-09 16:41 ` Tejun Heo 2012-01-10 5:06 ` Namhyung Kim 0 siblings, 1 reply; 8+ messages in thread From: Tejun Heo @ 2012-01-09 16:41 UTC (permalink / raw) To: Namhyung Kim; +Cc: Jens Axboe, linux-fsdevel, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2012-01-09 16:41 ` Tejun Heo @ 2012-01-10 5:06 ` Namhyung Kim 2012-01-10 16:26 ` Tejun Heo 0 siblings, 1 reply; 8+ messages in thread From: Namhyung Kim @ 2012-01-10 5:06 UTC (permalink / raw) To: Tejun Heo; +Cc: Jens Axboe, linux-fsdevel, linux-kernel, namhyung 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag 2012-01-10 5:06 ` Namhyung Kim @ 2012-01-10 16:26 ` Tejun Heo 0 siblings, 0 replies; 8+ messages in thread From: Tejun Heo @ 2012-01-10 16:26 UTC (permalink / raw) To: Namhyung Kim; +Cc: Jens Axboe, linux-fsdevel, linux-kernel, namhyung 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] block: don't export block_bio_complete tracepoint 2011-12-27 14:28 [PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3 Namhyung Kim 2011-12-27 14:28 ` [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag Namhyung Kim @ 2011-12-27 14:28 ` Namhyung Kim 1 sibling, 0 replies; 8+ messages in thread From: Namhyung Kim @ 2011-12-27 14:28 UTC (permalink / raw) To: Jens Axboe; +Cc: linux-fsdevel, linux-kernel, dm-devel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-01-10 16:26 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-12-27 14:28 [PATCH -block/for-next 0/2] blktrace: bio-based device tracing improvement v3 Namhyung Kim 2011-12-27 14:28 ` [PATCH 1/2] block: introduce BIO_IN_FLIGHT flag Namhyung Kim 2012-01-09 1:56 ` Tejun Heo 2012-01-09 2:57 ` Namhyung Kim 2012-01-09 16:41 ` Tejun Heo 2012-01-10 5:06 ` Namhyung Kim 2012-01-10 16:26 ` Tejun Heo 2011-12-27 14:28 ` [PATCH 2/2] block: don't export block_bio_complete tracepoint Namhyung Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).