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

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

[Hungyu Lin]

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@redhat.com>
Link: https://lore.kernel.org/r/CALjSBEvjbiGDVewuapWKhGDtaxZwcn4=22raUiBEOyj22rY7-g@mail.gmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Hungyu Lin <dennylin0707@gmail.com>
---
 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);
 
 	/*
 	 * 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;
 
 	/*
 	 * Align the bio size to the discard granularity to make splitting the bio
 	 * at discard granularity boundaries easier in the driver if needed.
 	 */
-	return round_down(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
+	return rounddown(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
 }
 
 struct bio *blk_alloc_discard_bio(struct block_device *bdev,
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] block: divide-by-zero in bio_discard_limit() when discard_granularity is 0
--

commit c2a5682afd9058576d36c11b303c1f6073d6bfa9
Author: Hungyu Lin <dennylin0707@gmail.com>

block: fix discard alignment for non-power-of-two granularities

This commit replaces round_up() and round_down() in bio_discard_limit()
with sector_div() and rounddown() to properly align discard bios when
discard_granularity is not a power of two.

Link: https://lore.kernel.org/r/CALjSBEvjbiGDVewuapWKhGDtaxZwcn4=22raUiBEOyj22rY7-g@mail.gmail.com

> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 688bc67cbf733..e0de5bb4d969e 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);
>  
>  	/*
>  	 * 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;
>  
>  	/*
>  	 * Align the bio size to the discard granularity to make splitting the bio
>  	 * at discard granularity boundaries easier in the driver if needed.
>  	 */
> -	return round_down(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
> +	return rounddown(BIO_MAX_SIZE, discard_granularity) >> SECTOR_SHIFT;
>  }

[Severity: Critical]
Can this trigger a divide-by-zero when discard_granularity is 0?

When a block device does not support discard or has discard disabled via
sysfs, blk_validate_limits() in block/blk-settings.c explicitly sets
discard_granularity to 0:

	if (lim->max_discard_sectors)
		lim->discard_granularity =
			max(lim->discard_granularity, lim->physical_block_size);
	else
		lim->discard_granularity = 0;

Since commit 094259204578 ("block: remove the discard_granularity check
in __blkdev_issue_discard") and commit 30f1e7241422 ("block: move discard
checks into the ioctl handler"), neither blkdev_issue_discard(),
__blkdev_issue_discard(), nor blk_alloc_discard_bio() checks
bdev_max_discard_sectors(bdev) or bdev_discard_granularity(bdev) before
calling bio_discard_limit(bdev, *sector):

block/blk-lib.c:blk_alloc_discard_bio() {
	sector_t bio_sects = min(*nr_sects, bio_discard_limit(bdev, *sector));
	...
}

Instead, they rely on submit_bio_noacct() -> blk_check_bio() in
block/blk-core.c to reject unsupported REQ_OP_DISCARD bios with
BLK_STS_NOTSUPP.

Several callers can reach blk_alloc_discard_bio() on a device with
discard_granularity == 0, such as:

- Callers that do not pre-check bdev_max_discard_sectors(), for example
  hpfs_ioctl() -> hpfs_trim_fs() -> do_trim() -> sb_issue_discard(),
  nvmet_bdev_execute_discard() -> __blkdev_issue_discard(),
  fat_free_clusters() / fat_trim_fs() when mounted with -o discard,
  nilfs_discard_segments(), jfs_issue_discard(), __ext4_remount() with
  -o remount,discard, f2fs_secure_erase(), Xen dispatch_discard_io(),
  r5l_write_super_and_discard_space(), and fd_execute_unmap().

- Any discard caller (such as blk_ioctl_discard(), blkdev_cmd_discard(),
  or active filesystem/swap discard) racing with runtime discard
  disablement when 0 is written to discard_max_bytes via
  queue_max_discard_sectors_store().

Previously, round_up(sector, 0) and round_down(BIO_MAX_SIZE, 0) used
bitwise masks and did not trap. With sector_div() and rounddown(), a zero
discard_granularity divides by zero.

Should discard_granularity be guarded here (similar to
max(lim->discard_granularity >> 9, 1U) in __bio_split_discard()), or
should bio_discard_limit() / blk_alloc_discard_bio() handle
discard_granularity == 0 before dividing?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026000511.73371-1-dennylin0707@gmail.com?part=1