| Message ID | 20220526010613.4016118-6-kbusch@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | direct io dma alignment | expand |
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;
On Thu, May 26, 2022 at 10:54:02AM +0900, Damien Le Moal wrote: > 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. I agree, a bool return looks more natural here. Otherwise this looks good to me.
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; +} + #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;