| Message ID | 20240126005032.1985245-1-dlemoal@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | null_blk: Always split BIOs to respect queue limits | expand |
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
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
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
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.
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
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(-)