Re: [PATCH] block: fix discard alignment for non-power-of-two granularities

[Christoph Hellwig]

On Mon, Oct 26, 2026 at 12:05:11AM +0000, Hungyu Lin wrote:
> bio_discard_limit() uses round_up() and round_down(), which require a
> power-of-two alignment. However, discard_granularity need not be a power
> of two. For example, dm-thin supports data block sizes that are arbitrary
> multiples of 64 KiB and advertises the block size as its granularity.
> 
> With a 192 KiB granularity, discarding one complete block at offset
> 384 KiB is split into 128 KiB and 64 KiB bios. Neither covers a complete
> thin block, so dm-thin ignores both and the mapping remains allocated.
> 
> Use sector_div() to find the distance to the next granularity boundary,
> and rounddown() to align the maximum bio size. This handles non-power-of-
> two granularities while preserving the alignment for power-of-two ones.
> 
> Fixes: 9b15d109a6b2 ("block: improve discard bio alignment in __blkdev_issue_discard()")
> Reported-by: Ming-Hung Tsai <mtsai@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/CALjSBEvjbiGDVewuapWKhGDtaxZwcn4=22raUiBEOyj22rY7-g@xxxxxxxxxxxxxx
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hungyu Lin <dennylin0707@xxxxxxxxx>
> ---
>  block/blk-lib.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 688bc67cbf73..e0de5bb4d969 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -13,26 +13,25 @@
>  static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
>  {
>  	unsigned int discard_granularity = bdev_discard_granularity(bdev);
> -	sector_t granularity_aligned_sector;
> +	unsigned int remainder;
>  
>  	if (bdev_is_partition(bdev))
>  		sector += bdev->bd_start_sect;
>  
> -	granularity_aligned_sector =
> -		round_up(sector, discard_granularity >> SECTOR_SHIFT);
> +	remainder = sector_div(sector, discard_granularity >> SECTOR_SHIFT);

Please avoid using do_div (or the obsolete sector_div wrapper around) it
becaus it modifies the first parameter in a sneaky and dangerous way.

>  
>  	/*
>  	 * Make sure subsequent bios start aligned to the discard granularity if
>  	 * it needs to be split.
>  	 */
> -	if (granularity_aligned_sector != sector)
> -		return granularity_aligned_sector - sector;
> +	if (remainder)
> +		return (discard_granularity >> SECTOR_SHIFT) - remainder;

Also this is actually suboptimal, we can just remove the remainder from
the full size.  Something like the untested patch below:

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 688bc67cbf73..6c45f15c41a2 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -13,26 +13,22 @@
 static sector_t bio_discard_limit(struct block_device *bdev, sector_t sector)
 {
 	unsigned int discard_granularity = bdev_discard_granularity(bdev);
-	sector_t granularity_aligned_sector;
+	u32 unaligned_sectors;
 
 	if (bdev_is_partition(bdev))
 		sector += bdev->bd_start_sect;
 
-	granularity_aligned_sector =
-		round_up(sector, discard_granularity >> SECTOR_SHIFT);
-
-	/*
-	 * Make sure subsequent bios start aligned to the discard granularity if
-	 * it needs to be split.
-	 */
-	if (granularity_aligned_sector != sector)
-		return granularity_aligned_sector - sector;
-
 	/*
-	 * Align the bio size to the discard granularity to make splitting the bio
-	 * at discard granularity boundaries easier in the driver if needed.
+	 * Align the bio size to the discard granularity to make splitting the
+	 * bio at discard granularity boundaries easier in the driver if needed.
+	 *
+	 * If the current sector is not aligned, shorten the range so that the
+	 * net one is aligned.
 	 */
-	return round_down(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
+	div_u64_rem(sector, discard_granularity >> SECTOR_SHIFT,
+			&unaligned_sectors);
+	return (rounddown(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT) -
+		unaligned_sectors;
 }
 
 struct bio *blk_alloc_discard_bio(struct block_device *bdev,