diff mbox series

[1/4] block: fix bio_split_rw_at to take zone_write_granularity into account

Message ID 20241101052206.437530-2-hch@lst.de (mailing list archive)
State New
Headers show
Series [1/4] block: fix bio_split_rw_at to take zone_write_granularity into account | expand

Commit Message

Christoph Hellwig Nov. 1, 2024, 5:21 a.m. UTC
Otherwise it can create unaligned writes on zoned devices.

Fixes: a805a4fa4fa3 ("block: introduce zone_write_granularity limit")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-merge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Damien Le Moal Nov. 1, 2024, 5:34 a.m. UTC | #1
On 11/1/24 14:21, Christoph Hellwig wrote:
> Otherwise it can create unaligned writes on zoned devices.
> 
> Fixes: a805a4fa4fa3 ("block: introduce zone_write_granularity limit")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-merge.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index d813d799cee7..d6895859a2fb 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -358,7 +358,8 @@ int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
>  	 * split size so that each bio is properly block size aligned, even if
>  	 * we do not use the full hardware limits.
>  	 */
> -	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
> +	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
> +			lim->zone_write_granularity : lim->logical_block_size);

Nit: we could also do:

	bytes = ALIGN_DOWN(bytes,
		max(lim->logical_block_size, lim->zone_write_granularity));

Also, I wonder if we should leave read split as is based on the logical block
size only ? Probably does not matter much...

>  
>  	/*
>  	 * Bio splitting may cause subtle trouble such as hang when doing sync
Christoph Hellwig Nov. 1, 2024, 5:47 a.m. UTC | #2
On Fri, Nov 01, 2024 at 02:34:31PM +0900, Damien Le Moal wrote:
> > -	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
> > +	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
> > +			lim->zone_write_granularity : lim->logical_block_size);
> 
> Nit: we could also do:
> 
> 	bytes = ALIGN_DOWN(bytes,
> 		max(lim->logical_block_size, lim->zone_write_granularity));

That's what I had first.  It is a little odd as zone_write_granularity
is defined to be >= logical_block_size, though.

> Also, I wonder if we should leave read split as is based on the logical block
> size only ? Probably does not matter much...

Good point.  Probably doesn't matter much, but randomly forcing it on
reads seems odd.
diff mbox series

Patch

diff --git a/block/blk-merge.c b/block/blk-merge.c
index d813d799cee7..d6895859a2fb 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -358,7 +358,8 @@  int bio_split_rw_at(struct bio *bio, const struct queue_limits *lim,
 	 * split size so that each bio is properly block size aligned, even if
 	 * we do not use the full hardware limits.
 	 */
-	bytes = ALIGN_DOWN(bytes, lim->logical_block_size);
+	bytes = ALIGN_DOWN(bytes, lim->zone_write_granularity ?
+			lim->zone_write_granularity : lim->logical_block_size);
 
 	/*
 	 * Bio splitting may cause subtle trouble such as hang when doing sync