[PATCH 0/2] Move blk_mq_submit_bio() error handling

[PATCH 0/2] Move blk_mq_submit_bio() error handling

[Bart Van Assche]

Hi Jens,

Some of the blk_mq_submit_bio() error handling occurs in that function itself
while the remaining error handling code occurs in functions called by
blk_mq_submit_bio(). This patch series makes blk_mq_submit_bio() easier to
understand by moving all error handling code into blk_mq_submit_bio(). No
functionality has been changed.

Please consider this patch series for the next merge window.

Thanks,

Bart.

Bart Van Assche (2):
  block: Optimize blk_mq_submit_bio() for the cache hit scenario
  blk-mq: Move more error handling into blk_mq_submit_bio()

 block/blk-mq.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

[PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Bart Van Assche]

Help the CPU branch predictor in case of a cache hit by handling the cache
hit scenario first.

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-mq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 7ee21346a41e..8d2aab4d9ba9 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -3102,12 +3102,12 @@ void blk_mq_submit_bio(struct bio *bio)
 		goto queue_exit;
 
 new_request:
-	if (!rq) {
+	if (rq) {
+		blk_mq_use_cached_rq(rq, plug, bio);
+	} else {
 		rq = blk_mq_get_new_requests(q, plug, bio, nr_segs);
 		if (unlikely(!rq))
 			goto queue_exit;
-	} else {
-		blk_mq_use_cached_rq(rq, plug, bio);
 	}
 
 	trace_block_getrq(bio);

[PATCH 2/2] blk-mq: Move more error handling into blk_mq_submit_bio()

[Bart Van Assche]

The error handling code in blk_mq_get_new_requests() cannot be understood
without knowing that this function is only called by blk_mq_submit_bio().
Hence move the code for handling blk_mq_get_new_requests() failures into
blk_mq_submit_bio().

Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 block/blk-mq.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8d2aab4d9ba9..80eb91296142 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2971,8 +2971,6 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	if (rq)
 		return rq;
 	rq_qos_cleanup(q, bio);
-	if (bio->bi_opf & REQ_NOWAIT)
-		bio_wouldblock_error(bio);
 	return NULL;
 }
 
@@ -3106,8 +3104,11 @@ void blk_mq_submit_bio(struct bio *bio)
 		blk_mq_use_cached_rq(rq, plug, bio);
 	} else {
 		rq = blk_mq_get_new_requests(q, plug, bio, nr_segs);
-		if (unlikely(!rq))
+		if (unlikely(!rq)) {
+			if (bio->bi_opf & REQ_NOWAIT)
+				bio_wouldblock_error(bio);
 			goto queue_exit;
+		}
 	}
 
 	trace_block_getrq(bio);

Re: [PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 12:19:00PM -0800, Bart Van Assche wrote:
> Help the CPU branch predictor in case of a cache hit by handling the cache
> hit scenario first.

Numbers, please.

Re: [PATCH 2/2] blk-mq: Move more error handling into blk_mq_submit_bio()

[Christoph Hellwig]

On Mon, Dec 16, 2024 at 12:19:01PM -0800, Bart Van Assche wrote:
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 8d2aab4d9ba9..80eb91296142 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2971,8 +2971,6 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>  	if (rq)
>  		return rq;
>  	rq_qos_cleanup(q, bio);
> -	if (bio->bi_opf & REQ_NOWAIT)
> -		bio_wouldblock_error(bio);
>  	return NULL;

Please turn this into:

	if (rq)
		rq_qos_cleanup(q, bio);
	return rq;

Otherwise this looks like a nice cleanup.

Re: [PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Bart Van Assche]

On 12/16/24 8:16 PM, Christoph Hellwig wrote:
> On Mon, Dec 16, 2024 at 12:19:00PM -0800, Bart Van Assche wrote:
>> Help the CPU branch predictor in case of a cache hit by handling the cache
>> hit scenario first.
> 
> Numbers, please.

For a single CPU core and with the brd driver and fio and the io_uring
I/O engine, I see the following performance in a VM (three test runs for
each test case):

Without this patch:      1619K, 1641K, 1638K IOPS or 1633 K +/- 10 K.
With this patch applied: 1650K, 1633K, 1635K IOPS or 1639 K +/-  8 K.

So there is a small performance improvement but the improvement is
smaller than the measurement error. Is this sufficient data to proceed
with this patch?

Thanks,

Bart.

Re: [PATCH 2/2] blk-mq: Move more error handling into blk_mq_submit_bio()

[Bart Van Assche]

On 12/16/24 8:18 PM, Christoph Hellwig wrote:
> On Mon, Dec 16, 2024 at 12:19:01PM -0800, Bart Van Assche wrote:
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 8d2aab4d9ba9..80eb91296142 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -2971,8 +2971,6 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
>>   	if (rq)
>>   		return rq;
>>   	rq_qos_cleanup(q, bio);
>> -	if (bio->bi_opf & REQ_NOWAIT)
>> -		bio_wouldblock_error(bio);
>>   	return NULL;
> 
> Please turn this into:
> 
> 	if (rq)
> 		rq_qos_cleanup(q, bio);
> 	return rq;
> 
> Otherwise this looks like a nice cleanup.

It seems to me that there is a typo in the above code? Unless if you 
tell me otherwise, I will assume that this is what you meant:

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 8d2aab4d9ba9..f4300e608ed8 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2968,12 +2968,9 @@ static struct request 
*blk_mq_get_new_requests(struct request_queue *q,
  	}

  	rq = __blk_mq_alloc_requests(&data);
-	if (rq)
-		return rq;
-	rq_qos_cleanup(q, bio);
-	if (bio->bi_opf & REQ_NOWAIT)
-		bio_wouldblock_error(bio);
-	return NULL;
+	if (!rq)
+		rq_qos_cleanup(q, bio);
+	return rq;
  }

  /*
@@ -3106,8 +3103,11 @@ void blk_mq_submit_bio(struct bio *bio)
  		blk_mq_use_cached_rq(rq, plug, bio);
  	} else {
  		rq = blk_mq_get_new_requests(q, plug, bio, nr_segs);
-		if (unlikely(!rq))
+		if (unlikely(!rq)) {
+			if (bio->bi_opf & REQ_NOWAIT)
+				bio_wouldblock_error(bio);
  			goto queue_exit;
+		}
  	}

  	trace_block_getrq(bio);



Thanks,

Bart.

Re: [PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Jens Axboe]

On 12/17/24 12:22 PM, Bart Van Assche wrote:
> On 12/16/24 8:16 PM, Christoph Hellwig wrote:
>> On Mon, Dec 16, 2024 at 12:19:00PM -0800, Bart Van Assche wrote:
>>> Help the CPU branch predictor in case of a cache hit by handling the cache
>>> hit scenario first.
>>
>> Numbers, please.
> 
> For a single CPU core and with the brd driver and fio and the io_uring
> I/O engine, I see the following performance in a VM (three test runs for
> each test case):
> 
> Without this patch:      1619K, 1641K, 1638K IOPS or 1633 K +/- 10 K.
> With this patch applied: 1650K, 1633K, 1635K IOPS or 1639 K +/-  8 K.
> 
> So there is a small performance improvement but the improvement is
> smaller than the measurement error. Is this sufficient data to proceed
> with this patch?

I think it's fine, it's going to be very hard to reliably measure. I
tend to prefer the expected case to be the one checked for, eg:

rq = get_cached();
if (rq)
	...

anyway, as then the expected outcome is what you're reading next
anyway. Either that or

rq = get_cached();
if (unlikely(!rq))
	goto some_failure_path;

If you really wanted to measure it, you'd probably skip the IOPS
part and just look at branch misprediction in perf.

-- 
Jens Axboe

Re: [PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Christoph Hellwig]

On Tue, Dec 17, 2024 at 11:22:47AM -0800, Bart Van Assche wrote:
> For a single CPU core and with the brd driver and fio and the io_uring
> I/O engine, I see the following performance in a VM (three test runs for
> each test case):
>
> Without this patch:      1619K, 1641K, 1638K IOPS or 1633 K +/- 10 K.
> With this patch applied: 1650K, 1633K, 1635K IOPS or 1639 K +/-  8 K.
>
> So there is a small performance improvement but the improvement is
> smaller than the measurement error. Is this sufficient data to proceed
> with this patch?

I think it's sufficient data that should not claim that it is an
optimizations.  The resulting code still looks nicer to me, but
arguing with optimizations feels like BS.

Re: [PATCH 2/2] blk-mq: Move more error handling into blk_mq_submit_bio()

[Christoph Hellwig]

On Tue, Dec 17, 2024 at 01:06:12PM -0800, Bart Van Assche wrote:
> It seems to me that there is a typo in the above code? Unless if you tell 
> me otherwise, I will assume that this is what you meant:

Yes.

Re: [PATCH 1/2] block: Optimize blk_mq_submit_bio() for the cache hit scenario

[Bart Van Assche]

On 12/17/24 10:59 PM, Christoph Hellwig wrote:
> On Tue, Dec 17, 2024 at 11:22:47AM -0800, Bart Van Assche wrote:
>> For a single CPU core and with the brd driver and fio and the io_uring
>> I/O engine, I see the following performance in a VM (three test runs for
>> each test case):
>>
>> Without this patch:      1619K, 1641K, 1638K IOPS or 1633 K +/- 10 K.
>> With this patch applied: 1650K, 1633K, 1635K IOPS or 1639 K +/-  8 K.
>>
>> So there is a small performance improvement but the improvement is
>> smaller than the measurement error. Is this sufficient data to proceed
>> with this patch?
> 
> I think it's sufficient data that should not claim that it is an
> optimizations.  The resulting code still looks nicer to me, but
> arguing with optimizations feels like BS.

I will change the patch title.

Thanks,

Bart.