Re: [PATCHv2 3/3] block: relax direct io memory alignment

[Keith Busch <kbusch@kernel.org>]

On Thu, May 19, 2022 at 09:38:11AM +0200, Christoph Hellwig wrote:
> > @@ -1207,6 +1207,7 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> >  {
> >  	unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt;
> >  	unsigned short entries_left = bio->bi_max_vecs - bio->bi_vcnt;
> > +	struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> >  	struct bio_vec *bv = bio->bi_io_vec + bio->bi_vcnt;
> >  	struct page **pages = (struct page **)bv;
> >  	bool same_page = false;
> > @@ -1223,6 +1224,8 @@ static int __bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
> >  	pages += entries_left * (PAGE_PTRS_PER_BVEC - 1);
> >  
> >  	size = iov_iter_get_pages(iter, pages, LONG_MAX, nr_pages, &offset);
> > +	if (size > 0)
> > +		size = ALIGN_DOWN(size, queue_logical_block_size(q));
> 
> So if we do get a size that is not logical block size alignment here,
> we reduce it to the block size aligned one below.  Why do we do that?

There are two possibilities:

In the first case, the number of pages in this iteration exceeds bi_max_vecs.
Rounding down completes the bio with a block aligned size, and the remainder
will be picked up for the next bio, or possibly even the current bio if the
pages are sufficiently physically contiguous.

The other case is a bad iov. If we're doing __blkdev_direct_IO(), it will error
out immediately if the rounded size is 0, or the next iteration when the next
size is rounded to 0. If we're doing the __blkdev_direct_IO_simple(), it will
error out when it sees the iov hasn't advanced to the end.

And ... I just noticed I missed the size check __blkdev_direct_IO_async().
 
> > +	if ((pos | iov_iter_count(iter)) & (bdev_logical_block_size(bdev) - 1))
> > +		return -EINVAL;
> > +	if (iov_iter_alignment(iter) & bdev_dma_alignment(bdev))
> >  		return -EINVAL;
> 
> Can we have a little inline helper for these checks instead of
> duplicating them three times?

Absolutely.

> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index 840752006f60..64cc176be60c 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -1131,7 +1131,7 @@ ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> >  	struct dio_submit sdio = { 0, };
> >  	struct buffer_head map_bh = { 0, };
> >  	struct blk_plug plug;
> > -	unsigned long align = offset | iov_iter_alignment(iter);
> > +	unsigned long align = iov_iter_alignment(iter);
> 
> I'd much prefer to not just relax this for random file systems,
> and especially not the legacy direct I/O code.  I think we can eventually
> do iomap, but only after an audit and test of each file system, which
> might require a new IOMAP_DIO_* flag at least initially.

I did some testing with xfs, but I can certainly run more a lot more tests. I
do think filesystem support for this capability is important, so I hope we
eventually get there.