Re: [PATCHv4 5/9] block: add a helper function for dio alignment

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/05/26 10:06, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> This will make it easier to add more complex acceptable alignment
> criteria in the future.
> 
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  block/fops.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index b9b83030e0df..bd6c2e13a4e3 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -42,6 +42,16 @@ static unsigned int dio_bio_write_op(struct kiocb *iocb)
>  	return op;
>  }
>  
> +static int blkdev_dio_aligned(struct block_device *bdev, loff_t pos,
> +			      struct iov_iter *iter)
> +{
> +	if ((pos | iov_iter_alignment(iter)) &
> +	    (bdev_logical_block_size(bdev) - 1))
> +		return -EINVAL;
> +
> +	return 0;
> +}

Nit: The name of this helper really suggest a bool return, which would be a more
flexible interface I think:

	if (!blkdev_dio_aligned(bdev, pos, iter))
		return -EINVAL; <-- or whatever error code the caller wants.

And that will allow you to get rid of the ret variable in a least
__blkdev_direct_IO_async.

> +
>  #define DIO_INLINE_BIO_VECS 4
>  
>  static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
> @@ -54,9 +64,9 @@ static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb,
>  	struct bio bio;
>  	ssize_t ret;
>  
> -	if ((pos | iov_iter_alignment(iter)) &
> -	    (bdev_logical_block_size(bdev) - 1))
> -		return -EINVAL;
> +	ret = blkdev_dio_aligned(bdev, pos, iter);
> +	if (ret)
> +		return ret;
>  
>  	if (nr_pages <= DIO_INLINE_BIO_VECS)
>  		vecs = inline_vecs;
> @@ -171,11 +181,11 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter,
>  	bool is_read = (iov_iter_rw(iter) == READ), is_sync;
>  	unsigned int opf = is_read ? REQ_OP_READ : dio_bio_write_op(iocb);
>  	loff_t pos = iocb->ki_pos;
> -	int ret = 0;
> +	int ret;
>  
> -	if ((pos | iov_iter_alignment(iter)) &
> -	    (bdev_logical_block_size(bdev) - 1))
> -		return -EINVAL;
> +	ret = blkdev_dio_aligned(bdev, pos, iter);
> +	if (ret)
> +		return ret;
>  
>  	if (iocb->ki_flags & IOCB_ALLOC_CACHE)
>  		opf |= REQ_ALLOC_CACHE;
> @@ -296,11 +306,11 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>  	struct blkdev_dio *dio;
>  	struct bio *bio;
>  	loff_t pos = iocb->ki_pos;
> -	int ret = 0;
> +	int ret;
>  
> -	if ((pos | iov_iter_alignment(iter)) &
> -	    (bdev_logical_block_size(bdev) - 1))
> -		return -EINVAL;
> +	ret = blkdev_dio_aligned(bdev, pos, iter);
> +	if (ret)
> +		return ret;
>  
>  	if (iocb->ki_flags & IOCB_ALLOC_CACHE)
>  		opf |= REQ_ALLOC_CACHE;


-- 
Damien Le Moal
Western Digital Research