Re: [PATCH 02/10] block: move discard checks into the ioctl handler

[Dave Chinner <david@fromorbit.com>]

On Thu, Mar 07, 2024 at 08:11:49AM -0700, Christoph Hellwig wrote:
> Most bio operations get basic sanity checking in submit_bio and anything
> more complicated than that is done in the callers.  Discards are a bit
> different from that in that a lot of checking is done in
> __blkdev_issue_discard, and the specific errnos for that are returned
> to userspace.  Move the checks that require specific errnos to the ioctl
> handler instead, and just leave the basic sanity checking in submit_bio
> for the other handlers.  This introduces two changes in behavior:
> 
>  1) the logical block size alignment check of the start and len is lost
>     for non-ioctl callers.
>     This matches what is done for other operations including reads and
>     writes.  We should probably verify this for all bios, but for now
>     make discards match the normal flow.
>  2) for non-ioctl callers all errors are reported on I/O completion now
>     instead of synchronously.  Callers in general mostly ignore or log
>     errors so this will actually simplify the code once cleaned up
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

OK.

> ---
>  block/blk-lib.c | 13 -------------
>  block/ioctl.c   | 13 +++++++++----
>  2 files changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index f873eb9a886f63..50923508a32466 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -59,19 +59,6 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  		sector_t nr_sects, gfp_t gfp_mask, struct bio **biop)
>  {
>  	struct bio *bio = *biop;
> -	sector_t bs_mask;
> -
> -	if (bdev_read_only(bdev))
> -		return -EPERM;
> -	if (!bdev_max_discard_sectors(bdev))
> -		return -EOPNOTSUPP;
> -
> -	bs_mask = (bdev_logical_block_size(bdev) >> 9) - 1;
> -	if ((sector | nr_sects) & bs_mask)
> -		return -EINVAL;
> -
> -	if (!nr_sects)
> -		return -EINVAL;
>  
>  	while (nr_sects) {
>  		sector_t req_sects =
> diff --git a/block/ioctl.c b/block/ioctl.c
> index de0cc0d215c633..1d5de0a890c5e8 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -95,6 +95,8 @@ static int compat_blkpg_ioctl(struct block_device *bdev,
>  static int blk_ioctl_discard(struct block_device *bdev, blk_mode_t mode,
>  		unsigned long arg)
>  {
> +	sector_t bs_mask = (bdev_logical_block_size(bdev) >> SECTOR_SHIFT) - 1;
> +	sector_t sector, nr_sects;

This changes the alignment checks from a hard coded 512 byte sector
to the logical block size of the device. I don't see a problem with
this (it fixes a bug) but it should at least be mentioned in the
commit message.

-Dave.
-- 
Dave Chinner
david@fromorbit.com