linux-kernel - Re: [PATCH 2/6] block: introduce __bio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87vctdvxye.fsf@gmail.com>
Date:	Thu, 01 Sep 2011 10:43:53 +0900
From:	Namhyung Kim <namhyung@...il.com>
To:	Jens Axboe <axboe@...nel.dk>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] block: introduce __bio_endio()

Jens Axboe <axboe@...nel.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)))
 
 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/