[PATCH v2 0/3] blktrace: bio-based device tracing improvement

[PATCH v2 0/3] blktrace: bio-based device tracing improvement

[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 v1:
 * kill __bio_endio()
 * use BIO_IN_FLIGHT flag

Any feedbacks are welcome.

Thanks.

Namhyung Kim (3):
  block: move trace_block_bio_remap() before blk_partition_remap
  block: introduce BIO_IN_FLIGHT flag
  block: don't export block_bio_complete tracepoint

 block/blk-core.c          |   12 ++++++++----
 drivers/md/dm.c           |    1 -
 fs/bio.c                  |    9 +++++++++
 include/linux/blk_types.h |    1 +
 4 files changed, 18 insertions(+), 5 deletions(-)

-- 
1.7.6

[PATCH 1/3] block: move trace_block_bio_remap() before blk_partition_remap

[Namhyung Kim]

block_bio_remap tracepoint keep tracks of remapping information of
the @bio. IOW, if underlying disk remaps @bio to other disk, it
could be tracked using the tracepoint in __generic_make_request()
loop. However, blk_partition_remap() also modifies the information
before the tracepoint so that the remapping chain could be diverged
and this could make some potential userspace tools confused.

Moving the tracepoint before blk_partition_remap() can help it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 block/blk-core.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 90e1ffdeb415..a60b46cc9da5 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1494,6 +1494,9 @@ static inline void __generic_make_request(struct bio *bio)
 					bio->bi_size))
 			goto end_io;
 
+		if (old_sector != -1)
+			trace_block_bio_remap(q, bio, old_dev, old_sector);
+
 		/*
 		 * If this device has partitions, remap block n
 		 * of partition p to block n+start(p) of the disk.
@@ -1503,9 +1506,6 @@ static inline void __generic_make_request(struct bio *bio)
 		if (bio_integrity_enabled(bio) && bio_integrity_prep(bio))
 			goto end_io;
 
-		if (old_sector != -1)
-			trace_block_bio_remap(q, bio, old_dev, old_sector);
-
 		old_sector = bio->bi_sector;
 		old_dev = bio->bi_bdev->bd_dev;
 
-- 
1.7.6

[PATCH 2/3] 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                  |    9 +++++++++
 include/linux/blk_types.h |    1 +
 3 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a60b46cc9da5..d905f3c81ca7 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -164,6 +164,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);
 
@@ -1545,6 +1548,8 @@ static inline void __generic_make_request(struct bio *bio)
 
 		trace_block_bio_queue(q, bio);
 
+		set_bit(BIO_IN_FLIGHT, &bio->bi_flags);
+
 		ret = q->make_request_fn(q, bio);
 	} while (ret);
 
diff --git a/fs/bio.c b/fs/bio.c
index 9bfade8a609b..c510e93b6312 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1447,6 +1447,15 @@ 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);
+
+		/* prevent duplicated completion event report */
+		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 32f0076e844b..daa81a7d1522 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -98,6 +98,7 @@ struct bio {
 #define BIO_FS_INTEGRITY 10	/* fs owns integrity data, not block layer */
 #define BIO_QUIET	11	/* Make BIO Quiet */
 #define BIO_MAPPED_INTEGRITY 12/* integrity metadata has been remapped */
+#define BIO_IN_FLIGHT	13	/* report I/O completion event */
 #define bio_flagged(bio, flag)	((bio)->bi_flags & (1 << (flag)))
 
 /*
-- 
1.7.6

[PATCH 3/3] 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 d905f3c81ca7..41c8ea897e09 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -36,7 +36,6 @@
 
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
-EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_complete);
 
 static int __make_request(struct request_queue *q, struct bio *bio);
 
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 52b39f335bb3..e9e671303609 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -639,7 +639,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

[PATCH 3/3] 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.kim@lge.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 1b4fd93af2c0..399c128f516c 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.9.rc1.dirty