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

[PATCH 0/6] 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 split such cases
to remove duplicated reports. In this series, __bio_endio() (and its
helper __bio_io_error) was introduced and converted 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.

This patch set is based on v3.1-rc3.
Patch 1 is to correct remapping information and independent to others.
Patch 2 is the core of this series and Patch 3-5 are mechanical converts.
Patch 6 removes manual use of the tracepoint in dm.

Any feedbacks are welcome.

Thanks.


Namhyung Kim (6):
  block: move trace_block_bio_remap() before blk_partition_remap
  block: introduce __bio_endio()
  bounce: convert to __bio_endio() for bounced bio's
  bio-integrity: convert to __bio_endio()
  Btrfs: convert to __bio_endio()
  dm: get rid of block_bio_complete tracepoint

 block/blk-core.c       |   11 +++++------
 drivers/md/dm.c        |    1 -
 fs/bio-integrity.c     |    4 ++--
 fs/bio.c               |   19 +++++++++++++++++++
 fs/btrfs/compression.c |    4 ++--
 fs/btrfs/disk-io.c     |    2 +-
 fs/btrfs/inode.c       |    8 ++++----
 fs/btrfs/volumes.c     |    4 ++--
 include/linux/bio.h    |    2 ++
 mm/bounce.c            |    2 +-
 10 files changed, 38 insertions(+), 19 deletions(-)

-- 
1.7.6

[PATCH 1/6] 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/6] block: introduce __bio_endio()

[Namhyung Kim]

Currently, bio_endio() lacks its completion tracepoint in it,
so that the bio-based devices - except DM which inserted the
tracepoint explicitly - cannot send us such an event when using
blktrace. Adding the tracepoint in the function will fix this.

However, bio_endio() is also used for other ways like some
nested bio-handling path and request-based devices. Simply
adding will result in duplicated event for those cases. Thus
add new __bio_endio() function to do things as before but no
tracepoint. Similarly, __bio_io_error() helper was added too.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 block/blk-core.c    |    4 ++--
 fs/bio.c            |   19 +++++++++++++++++++
 include/linux/bio.h |    2 ++
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index a60b46cc9da5..62b79692c7e8 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -172,7 +172,7 @@ static void req_bio_endio(struct request *rq, struct bio *bio,
 
 	/* don't actually finish bio if it's part of flush sequence */
 	if (bio->bi_size == 0 && !(rq->cmd_flags & REQ_FLUSH_SEQ))
-		bio_endio(bio, error);
+		__bio_endio(bio, error);
 }
 
 void blk_dump_rq_flags(struct request *rq, char *msg)
@@ -1551,7 +1551,7 @@ static inline void __generic_make_request(struct bio *bio)
 	return;
 
 end_io:
-	bio_endio(bio, err);
+	__bio_endio(bio, err);
 }
 
 /*
diff --git a/fs/bio.c b/fs/bio.c
index 9bfade8a609b..9d7403f53efb 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1426,6 +1426,21 @@ void bio_flush_dcache_pages(struct bio *bi)
 EXPORT_SYMBOL(bio_flush_dcache_pages);
 #endif
 
+/*
+ * no-trace version of bio_endio().
+ */
+void __bio_endio(struct bio *bio, int error)
+{
+	if (error)
+		clear_bit(BIO_UPTODATE, &bio->bi_flags);
+	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
+		error = -EIO;
+
+	if (bio->bi_end_io)
+		bio->bi_end_io(bio, error);
+}
+EXPORT_SYMBOL(__bio_endio);
+
 /**
  * bio_endio - end I/O on a bio
  * @bio:	bio
@@ -1442,11 +1457,15 @@ EXPORT_SYMBOL(bio_flush_dcache_pages);
  **/
 void bio_endio(struct bio *bio, int error)
 {
+	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+
 	if (error)
 		clear_bit(BIO_UPTODATE, &bio->bi_flags);
 	else if (!test_bit(BIO_UPTODATE, &bio->bi_flags))
 		error = -EIO;
 
+	trace_block_bio_complete(q, bio, error);
+
 	if (bio->bi_end_io)
 		bio->bi_end_io(bio, error);
 }
diff --git a/include/linux/bio.h b/include/linux/bio.h
index ce33e6868a2f..453a9bec49b8 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -132,6 +132,7 @@ static inline int bio_has_allocated_vec(struct bio *bio)
 #define BIO_SEG_BOUNDARY(q, b1, b2) \
 	BIOVEC_SEG_BOUNDARY((q), __BVEC_END((b1)), __BVEC_START((b2)))
 
+#define __bio_io_error(bio) __bio_endio((bio), -EIO)
 #define bio_io_error(bio) bio_endio((bio), -EIO)
 
 /*
@@ -217,6 +218,7 @@ extern struct bio *bio_alloc_bioset(gfp_t, int, struct bio_set *);
 extern void bio_put(struct bio *);
 extern void bio_free(struct bio *, struct bio_set *);
 
+extern void __bio_endio(struct bio *, int);
 extern void bio_endio(struct bio *, int);
 struct request_queue;
 extern int bio_phys_segments(struct request_queue *, struct bio *);
-- 
1.7.6

Re: [PATCH 2/6] block: introduce __bio_endio()

[Jens Axboe]

On 2011-08-28 21:47, Namhyung Kim wrote:
> Currently, bio_endio() lacks its completion tracepoint in it,
> so that the bio-based devices - except DM which inserted the
> tracepoint explicitly - cannot send us such an event when using
> blktrace. Adding the tracepoint in the function will fix this.
> 
> However, bio_endio() is also used for other ways like some
> nested bio-handling path and request-based devices. Simply
> adding will result in duplicated event for those cases. Thus
> add new __bio_endio() function to do things as before but no
> tracepoint. Similarly, __bio_io_error() helper was added too.

Not crazy about this solution, it seems a little fragile. And it's hard
to know what the difference between bio_endio() and __bio_endio() is
without looking at the code.

I think it would be cleaner to mark a bio as going inflight, so that we
can check this flag on completion. If it's never been in flight, don't
trigger a completion event trace for it.

-- 
Jens Axboe

Re: [PATCH 2/6] block: introduce __bio_endio()

[Namhyung Kim]

Jens Axboe <axboe@kernel.dk> writes:

> On 2011-08-28 21:47, Namhyung Kim wrote:
>> Currently, bio_endio() lacks its completion tracepoint in it,
>> so that the bio-based devices - except DM which inserted the
>> tracepoint explicitly - cannot send us such an event when using
>> blktrace. Adding the tracepoint in the function will fix this.
>> 
>> However, bio_endio() is also used for other ways like some
>> nested bio-handling path and request-based devices. Simply
>> adding will result in duplicated event for those cases. Thus
>> add new __bio_endio() function to do things as before but no
>> tracepoint. Similarly, __bio_io_error() helper was added too.
>
> Not crazy about this solution, it seems a little fragile. And it's hard
> to know what the difference between bio_endio() and __bio_endio() is
> without looking at the code.
>
> I think it would be cleaner to mark a bio as going inflight, so that we
> can check this flag on completion. If it's never been in flight, don't
> trigger a completion event trace for it.

OK. Sounds reasonable.

So, how about this:


diff --git a/block/blk-core.c b/block/blk-core.c
index 90e1ffdeb415..b70c086dc7c0 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..0176ca4935c1 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)))
 
 /*

[PATCH 3/6] bounce: convert to __bio_endio() for bounced bio's

[Namhyung Kim]

Use untraced __bio_endio() for nested bio handling path to
suppress duplicated trace event.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: linux-mm@kvack.org
---
 mm/bounce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/bounce.c b/mm/bounce.c
index 1481de68184b..c21289571d4d 100644
--- a/mm/bounce.c
+++ b/mm/bounce.c
@@ -142,7 +142,7 @@ static void bounce_end_io(struct bio *bio, mempool_t *pool, int err)
 		mempool_free(bvec->bv_page, pool);
 	}
 
-	bio_endio(bio_orig, err);
+	__bio_endio(bio_orig, err);
 	bio_put(bio);
 }
 
-- 
1.7.6

[PATCH 3/6] bounce: convert to __bio_endio() for bounced bio's

[Namhyung Kim]

Use untraced __bio_endio() for nested bio handling path to
suppress duplicated trace event.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: linux-mm@kvack.org
---
 mm/bounce.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/bounce.c b/mm/bounce.c
index 1481de68184b..c21289571d4d 100644
--- a/mm/bounce.c
+++ b/mm/bounce.c
@@ -142,7 +142,7 @@ static void bounce_end_io(struct bio *bio, mempool_t *pool, int err)
 		mempool_free(bvec->bv_page, pool);
 	}
 
-	bio_endio(bio_orig, err);
+	__bio_endio(bio_orig, err);
 	bio_put(bio);
 }
 
-- 
1.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 4/6] bio-integrity: convert to __bio_endio()

[Namhyung Kim]

Use untraced __bio_endio() for nested bio handling path to
suppress duplicated trace event.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
---
 fs/bio-integrity.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 9c5e6b2cd11a..85bebe1b77b5 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -541,7 +541,7 @@ static void bio_integrity_verify_fn(struct work_struct *work)
 
 	/* Restore original bio completion handler */
 	bio->bi_end_io = bip->bip_end_io;
-	bio_endio(bio, error);
+	__bio_endio(bio, error);
 }
 
 /**
@@ -568,7 +568,7 @@ void bio_integrity_endio(struct bio *bio, int error)
 	 */
 	if (error) {
 		bio->bi_end_io = bip->bip_end_io;
-		bio_endio(bio, error);
+		__bio_endio(bio, error);
 
 		return;
 	}
-- 
1.7.6

[PATCH 5/6] Btrfs: convert to __bio_endio()

[Namhyung Kim]

Use untraced __bio_endio() for nested bio handling path to
suppress duplicated trace event.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
Cc: linux-btrfs@vger.kernel.org
---
 fs/btrfs/compression.c |    4 ++--
 fs/btrfs/disk-io.c     |    2 +-
 fs/btrfs/inode.c       |    8 ++++----
 fs/btrfs/volumes.c     |    4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 8ec5d86f1734..864d3ad6dae7 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -197,7 +197,7 @@ csum_failed:
 
 	/* do io completion on the original bio */
 	if (cb->errors) {
-		bio_io_error(cb->orig_bio);
+		__bio_io_error(cb->orig_bio);
 	} else {
 		int bio_index = 0;
 		struct bio_vec *bvec = cb->orig_bio->bi_io_vec;
@@ -211,7 +211,7 @@ csum_failed:
 			bvec++;
 			bio_index++;
 		}
-		bio_endio(cb->orig_bio, 0);
+		__bio_endio(cb->orig_bio, 0);
 	}
 
 	/* finally free the cb struct */
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 07b3ac662e19..64ed856bdde6 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1490,7 +1490,7 @@ static void end_workqueue_fn(struct btrfs_work *work)
 	bio->bi_private = end_io_wq->private;
 	bio->bi_end_io = end_io_wq->end_io;
 	kfree(end_io_wq);
-	bio_endio(bio, error);
+	__bio_endio(bio, error);
 }
 
 static int cleaner_kthread(void *arg)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0ccc7438ad34..c98716474a57 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5893,10 +5893,10 @@ static void btrfs_end_dio_bio(struct bio *bio, int err)
 		goto out;
 
 	if (dip->errors)
-		bio_io_error(dip->orig_bio);
+		__bio_io_error(dip->orig_bio);
 	else {
 		set_bit(BIO_UPTODATE, &dip->orig_bio->bi_flags);
-		bio_endio(dip->orig_bio, 0);
+		__bio_endio(dip->orig_bio, 0);
 	}
 out:
 	bio_put(bio);
@@ -6059,7 +6059,7 @@ out_err:
 	 */
 	smp_mb__before_atomic_dec();
 	if (atomic_dec_and_test(&dip->pending_bios))
-		bio_io_error(dip->orig_bio);
+		__bio_io_error(dip->orig_bio);
 
 	/* bio_end_io() will handle error, so we needn't return it */
 	return 0;
@@ -6133,7 +6133,7 @@ free_ordered:
 		btrfs_put_ordered_extent(ordered);
 		btrfs_put_ordered_extent(ordered);
 	}
-	bio_endio(bio, ret);
+	__bio_endio(bio, ret);
 }
 
 static ssize_t check_direct_IO(struct btrfs_root *root, int rw, struct kiocb *iocb,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index f2a4cc79da61..0f745a97805d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3224,7 +3224,7 @@ static void end_bio_multi_stripe(struct bio *bio, int err)
 		}
 		kfree(multi);
 
-		bio_endio(bio, err);
+		__bio_endio(bio, err);
 	} else if (!is_orig_bio) {
 		bio_put(bio);
 	}
@@ -3350,7 +3350,7 @@ int btrfs_map_bio(struct btrfs_root *root, int rw, struct bio *bio,
 		} else {
 			bio->bi_bdev = root->fs_info->fs_devices->latest_bdev;
 			bio->bi_sector = logical >> 9;
-			bio_endio(bio, -EIO);
+			__bio_io_error(bio);
 		}
 		dev_nr++;
 	}
-- 
1.7.6

[PATCH 6/6] dm: get rid of block_bio_complete tracepoint

[Namhyung Kim]

Now bio_endio() contains the tracepoint in it, so we don't
need to have it twice. Plus, since 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 62b79692c7e8..57e8c2b8d9bd 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