All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] null_blk: Always split BIOs to respect queue limits
@ 2024-01-26  0:50 Damien Le Moal
  2024-01-26  7:05 ` Hannes Reinecke
  0 siblings, 1 reply; 5+ messages in thread
From: Damien Le Moal @ 2024-01-26  0:50 UTC (permalink / raw)
  To: Jens Axboe, linux-block

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] null_blk: Always split BIOs to respect queue limits
  2024-01-26  0:50 [PATCH] null_blk: Always split BIOs to respect queue limits Damien Le Moal
@ 2024-01-26  7:05 ` Hannes Reinecke
  2024-01-26  7:09   ` Damien Le Moal
  2024-01-26 14:27   ` Christoph Hellwig
  0 siblings, 2 replies; 5+ messages in thread
From: Hannes Reinecke @ 2024-01-26  7:05 UTC (permalink / raw)
  To: Damien Le Moal, Jens Axboe, linux-block

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] null_blk: Always split BIOs to respect queue limits
  2024-01-26  7:05 ` Hannes Reinecke
@ 2024-01-26  7:09   ` Damien Le Moal
  2024-01-26  8:37     ` Chaitanya Kulkarni
  2024-01-26 14:27   ` Christoph Hellwig
  1 sibling, 1 reply; 5+ messages in thread
From: Damien Le Moal @ 2024-01-26  7:09 UTC (permalink / raw)
  To: Hannes Reinecke, Jens Axboe, linux-block

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] null_blk: Always split BIOs to respect queue limits
  2024-01-26  7:09   ` Damien Le Moal
@ 2024-01-26  8:37     ` Chaitanya Kulkarni
  0 siblings, 0 replies; 5+ messages in thread
From: Chaitanya Kulkarni @ 2024-01-26  8:37 UTC (permalink / raw)
  To: Damien Le Moal, Hannes Reinecke; +Cc: linux-block, Jens Axboe

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] null_blk: Always split BIOs to respect queue limits
  2024-01-26  7:05 ` Hannes Reinecke
  2024-01-26  7:09   ` Damien Le Moal
@ 2024-01-26 14:27   ` Christoph Hellwig
  1 sibling, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2024-01-26 14:27 UTC (permalink / raw)
  To: Hannes Reinecke; +Cc: Damien Le Moal, Jens Axboe, linux-block

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-01-26 14:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-26  0:50 [PATCH] null_blk: Always split BIOs to respect queue limits Damien Le Moal
2024-01-26  7:05 ` Hannes Reinecke
2024-01-26  7:09   ` Damien Le Moal
2024-01-26  8:37     ` Chaitanya Kulkarni
2024-01-26 14:27   ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.