[PATCH] null_blk: Always split BIOs to respect queue limits

[PATCH] null_blk: Always split BIOs to respect queue limits

[Damien Le Moal]

The function null_submit_bio() used for null_blk devices configured
with a BIO-based queue never splits BIOs according to the queue limits
set with the various module and configfs parameters that the user can
specify.

Add a call to bio_split_to_limits() to correctly handle large
BIOs that need splitting. Doing so also fixes issues with zoned devices
as a large BIO may cross over a zone boundary, which breaks null_blk
zone emulation.

While at it, remove all the local variable that are not necessary.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/block/null_blk/main.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 36755f263e8e..514c2592046a 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1528,12 +1528,16 @@ static struct nullb_queue *nullb_to_queue(struct nullb *nullb)
 
 static void null_submit_bio(struct bio *bio)
 {
-	sector_t sector = bio->bi_iter.bi_sector;
-	sector_t nr_sectors = bio_sectors(bio);
-	struct nullb *nullb = bio->bi_bdev->bd_disk->private_data;
-	struct nullb_queue *nq = nullb_to_queue(nullb);
+	struct nullb_queue *nq =
+		nullb_to_queue(bio->bi_bdev->bd_disk->private_data);
+
+	/* Respect the queue limits */
+	bio = bio_split_to_limits(bio);
+	if (!bio)
+		return;
 
-	null_handle_cmd(alloc_cmd(nq, bio), sector, nr_sectors, bio_op(bio));
+	null_handle_cmd(alloc_cmd(nq, bio), bio->bi_iter.bi_sector,
+			bio_sectors(bio), bio_op(bio));
 }
 
 #ifdef CONFIG_BLK_DEV_NULL_BLK_FAULT_INJECTION
-- 
2.43.0

Re: [PATCH] null_blk: Always split BIOs to respect queue limits

[Hannes Reinecke]

On 1/26/24 01:50, Damien Le Moal wrote:
> The function null_submit_bio() used for null_blk devices configured
> with a BIO-based queue never splits BIOs according to the queue limits
> set with the various module and configfs parameters that the user can
> specify.
> 
> Add a call to bio_split_to_limits() to correctly handle large
> BIOs that need splitting. Doing so also fixes issues with zoned devices
> as a large BIO may cross over a zone boundary, which breaks null_blk
> zone emulation.
> 
That feels so wrong. Why would we need to apply queue limits to a bio?
(Yes, I know why. We still shouldn't be doing it.)

Maybe indeed time to kill the bio-based path.

But until that happens:

Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Ivo Totev, Andrew McDonald,
Werner Knoblich

Re: [PATCH] null_blk: Always split BIOs to respect queue limits

[Damien Le Moal]

On 1/26/24 16:05, Hannes Reinecke wrote:
> On 1/26/24 01:50, Damien Le Moal wrote:
>> The function null_submit_bio() used for null_blk devices configured
>> with a BIO-based queue never splits BIOs according to the queue limits
>> set with the various module and configfs parameters that the user can
>> specify.
>>
>> Add a call to bio_split_to_limits() to correctly handle large
>> BIOs that need splitting. Doing so also fixes issues with zoned devices
>> as a large BIO may cross over a zone boundary, which breaks null_blk
>> zone emulation.
>>
> That feels so wrong. Why would we need to apply queue limits to a bio?
> (Yes, I know why. We still shouldn't be doing it.)

Splitting is at least needed for zoned devices. Otherwise, everything breaks
with the zone emulation.
> Maybe indeed time to kill the bio-based path.

I have nothing against that :)

> 
> But until that happens:
> 
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] null_blk: Always split BIOs to respect queue limits

[Chaitanya Kulkarni]

On 1/25/24 23:09, Damien Le Moal wrote:
> On 1/26/24 16:05, Hannes Reinecke wrote:
>> On 1/26/24 01:50, Damien Le Moal wrote:
>>> The function null_submit_bio() used for null_blk devices configured
>>> with a BIO-based queue never splits BIOs according to the queue limits
>>> set with the various module and configfs parameters that the user can
>>> specify.
>>>
>>> Add a call to bio_split_to_limits() to correctly handle large
>>> BIOs that need splitting. Doing so also fixes issues with zoned devices
>>> as a large BIO may cross over a zone boundary, which breaks null_blk
>>> zone emulation.
>>>
>> That feels so wrong. Why would we need to apply queue limits to a bio?
>> (Yes, I know why. We still shouldn't be doing it.)
> Splitting is at least needed for zoned devices. Otherwise, everything breaks
> with the zone emulation.
>> Maybe indeed time to kill the bio-based path.
> I have nothing against that :)
>
>> But until that happens:
>>
>> Reviewed-by: Hannes Reinecke <hare@suse.de>
>>
>> Cheers,
>>
>> Hannes

If we are going to kill it we really don't need this patch, irrespective of
that :-

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH] null_blk: Always split BIOs to respect queue limits

[Christoph Hellwig]

On Fri, Jan 26, 2024 at 08:05:21AM +0100, Hannes Reinecke wrote:
> > BIOs that need splitting. Doing so also fixes issues with zoned devices
> > as a large BIO may cross over a zone boundary, which breaks null_blk
> > zone emulation.
> > 
> That feels so wrong. Why would we need to apply queue limits to a bio?
> (Yes, I know why. We still shouldn't be doing it.)

Because a driver that has limits should enforce them.  Your hardware
doesn't suddenly use limits because you're using a bio based driver,
and null_blk shouldn't suddenly ignore the configured limits just
because you're using it in bio mode.

The patch looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But yeah, I'd rather kill the bio mode.  Jens, are you attached
to the bio mode?  Otherwise I'll cook up a patch over the weekend.