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@xxxxxx>
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@xxxxxxxxxxxxx
