block: improve error checking in blkdev_bio_end_io_async()

Commit Message

Jens Axboe Oct. 19, 2021, 6:59 p.m. UTC

Track the current error status of the dio with DIO_ERROR in the flags,
which can then avoid diving into dio->bio for the fast path of not
having any errors. This reduces the overhead of the function nicely,
which was previously dominated by this seemingly cheap check:

     4.55%     -1.13%  [kernel.vmlinux]  [k] blkdev_bio_end_io_async

Signed-off-by: Jens Axboe <[e-mail]>

---

Comments

Jens Axboe Oct. 19, 2021, 7:05 p.m. UTC | #1

On 10/19/21 12:59 PM, Jens Axboe wrote:
> Track the current error status of the dio with DIO_ERROR in the flags,
> which can then avoid diving into dio->bio for the fast path of not
> having any errors. This reduces the overhead of the function nicely,
> which was previously dominated by this seemingly cheap check:
> 
>      4.55%     -1.13%  [kernel.vmlinux]  [k] blkdev_bio_end_io_async

Disregard, ended up being an older test version. I'll send the right
one.

Pavel Begunkov Oct. 19, 2021, 7:06 p.m. UTC | #2

On 10/19/21 19:59, Jens Axboe wrote:
> Track the current error status of the dio with DIO_ERROR in the flags,
> which can then avoid diving into dio->bio for the fast path of not
> having any errors. This reduces the overhead of the function nicely,
> which was previously dominated by this seemingly cheap check:
> 
>       4.55%     -1.13%  [kernel.vmlinux]  [k] blkdev_bio_end_io_async

Jens, something gone wrong here. blkdev_bio_end_io_async() is a
function from my not yet published branch, the perf here is for it,
but the patch tackles blkdev_bio_end_io().

P.s. once blkdev_bio_end_io_async() doesn't need it, and once in
normal blkdev_bio_end_io() will be a slow-ish path, so probably
we don't care much.


> 
> Signed-off-by: Jens Axboe <[e-mail]>
> 
> ---
> 
> diff --git a/block/fops.c b/block/fops.c
> index d4f4fffb7d32..21d265caecff 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -127,6 +127,7 @@ enum {
>   	DIO_MULTI_BIO		= 1,
>   	DIO_SHOULD_DIRTY	= 2,
>   	DIO_IS_SYNC		= 4,
> +	DIO_ERROR		= 8,
>   };
>   
>   struct blkdev_dio {
> @@ -147,8 +148,10 @@ static void blkdev_bio_end_io(struct bio *bio)
>   	struct blkdev_dio *dio = bio->bi_private;
>   	unsigned int flags = dio->flags;
>   
> -	if (bio->bi_status && !dio->bio.bi_status)
> +	if (!(flags & DIO_ERROR) && !dio->bio.bi_status) {
>   		dio->bio.bi_status = bio->bi_status;
> +		flags |= DIO_ERROR;
> +	}
>   
>   	if (!(flags & DIO_MULTI_BIO) || atomic_dec_and_test(&dio->ref)) {
>   		if (!(flags & DIO_IS_SYNC)) {
> @@ -157,7 +160,7 @@ static void blkdev_bio_end_io(struct bio *bio)
>   
>   			WRITE_ONCE(iocb->private, NULL);
>   
> -			if (likely(!dio->bio.bi_status)) {
> +			if (likely(!(flags & DIO_ERROR))) {
>   				ret = dio->size;
>   				iocb->ki_pos += ret;
>   			} else {
>

Jens Axboe Oct. 19, 2021, 7:11 p.m. UTC | #3

On 10/19/21 1:06 PM, Pavel Begunkov wrote:
> On 10/19/21 19:59, Jens Axboe wrote:
>> Track the current error status of the dio with DIO_ERROR in the flags,
>> which can then avoid diving into dio->bio for the fast path of not
>> having any errors. This reduces the overhead of the function nicely,
>> which was previously dominated by this seemingly cheap check:
>>
>>       4.55%     -1.13%  [kernel.vmlinux]  [k] blkdev_bio_end_io_async
> 
> Jens, something gone wrong here. blkdev_bio_end_io_async() is a
> function from my not yet published branch, the perf here is for it,
> but the patch tackles blkdev_bio_end_io().

Yeah, see followup email...

Patch

diff --git a/block/fops.c b/block/fops.c
index d4f4fffb7d32..21d265caecff 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -127,6 +127,7 @@  enum {
 	DIO_MULTI_BIO		= 1,
 	DIO_SHOULD_DIRTY	= 2,
 	DIO_IS_SYNC		= 4,
+	DIO_ERROR		= 8,
 };
 
 struct blkdev_dio {
@@ -147,8 +148,10 @@  static void blkdev_bio_end_io(struct bio *bio)
 	struct blkdev_dio *dio = bio->bi_private;
 	unsigned int flags = dio->flags;
 
-	if (bio->bi_status && !dio->bio.bi_status)
+	if (!(flags & DIO_ERROR) && !dio->bio.bi_status) {
 		dio->bio.bi_status = bio->bi_status;
+		flags |= DIO_ERROR;
+	}
 
 	if (!(flags & DIO_MULTI_BIO) || atomic_dec_and_test(&dio->ref)) {
 		if (!(flags & DIO_IS_SYNC)) {
@@ -157,7 +160,7 @@  static void blkdev_bio_end_io(struct bio *bio)
 
 			WRITE_ONCE(iocb->private, NULL);
 
-			if (likely(!dio->bio.bi_status)) {
+			if (likely(!(flags & DIO_ERROR))) {
 				ret = dio->size;
 				iocb->ki_pos += ret;
 			} else {