| Message ID | 20220518171131.3525293-4-kbusch@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | direct io alignment relax | expand |
On Wed, May 18, 2022 at 10:11:31AM -0700, Keith Busch wrote: > diff --git a/block/fops.c b/block/fops.c > index b9b83030e0df..d8537c29602f 100644 > --- a/block/fops.c > +++ b/block/fops.c > @@ -54,8 +54,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)) > + 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; The block layer makes a lot of assumptions that bios can be split at any bvec boundary. With this patch, bios whose length isn't a multiple of the logical block size can be generated by splitting, which isn't valid. Also some devices aren't compatible with logical blocks spanning bdevs at all. dm-crypt errors out in this case, for example. What am I missing? - Eric
On Wed, May 18, 2022 at 05:14:49PM -0700, Eric Biggers wrote: > On Wed, May 18, 2022 at 10:11:31AM -0700, Keith Busch wrote: > > diff --git a/block/fops.c b/block/fops.c > > index b9b83030e0df..d8537c29602f 100644 > > --- a/block/fops.c > > +++ b/block/fops.c > > @@ -54,8 +54,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)) > > + 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; > > The block layer makes a lot of assumptions that bios can be split at any bvec > boundary. With this patch, bios whose length isn't a multiple of the logical > block size can be generated by splitting, which isn't valid. How? This patch ensures every segment is block size aligned. We can always split a bio in the middle of a bvec for any lower level hardware constraints, and I'm not finding any splitting criteria that would try to break a bio on a non-block aligned boundary. > Also some devices aren't compatible with logical blocks spanning bdevs at all. > dm-crypt errors out in this case, for example. I'm sorry, but I am not understanding this.
On Wed, May 18, 2022 at 07:00:39PM -0600, Keith Busch wrote: > On Wed, May 18, 2022 at 05:14:49PM -0700, Eric Biggers wrote: > > On Wed, May 18, 2022 at 10:11:31AM -0700, Keith Busch wrote: > > > diff --git a/block/fops.c b/block/fops.c > > > index b9b83030e0df..d8537c29602f 100644 > > > --- a/block/fops.c > > > +++ b/block/fops.c > > > @@ -54,8 +54,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)) > > > + 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; > > > > The block layer makes a lot of assumptions that bios can be split at any bvec > > boundary. With this patch, bios whose length isn't a multiple of the logical > > block size can be generated by splitting, which isn't valid. > > How? This patch ensures every segment is block size aligned. No, it doesn't. It ensures that the *total* length of each bio is logical block size aligned. It doesn't ensure that for the individual bvecs. By decreasing the required memory alignment to below the logical block size, you're allowing logical blocks to span a page boundary. Whenever the two pages involved aren't physically contiguous, the data of the block will be split across two bvecs. > > Also some devices aren't compatible with logical blocks spanning bdevs at all. > > dm-crypt errors out in this case, for example. > > I'm sorry, but I am not understanding this. I meant to write bvecs, not bdevs. - Eric
On Wed, May 18, 2022 at 06:53:11PM -0700, Eric Biggers wrote: > On Wed, May 18, 2022 at 07:00:39PM -0600, Keith Busch wrote: > > On Wed, May 18, 2022 at 05:14:49PM -0700, Eric Biggers wrote: > > > On Wed, May 18, 2022 at 10:11:31AM -0700, Keith Busch wrote: > > > > diff --git a/block/fops.c b/block/fops.c > > > > index b9b83030e0df..d8537c29602f 100644 > > > > --- a/block/fops.c > > > > +++ b/block/fops.c > > > > @@ -54,8 +54,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)) > > > > + 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; > > > > > > The block layer makes a lot of assumptions that bios can be split at any bvec > > > boundary. With this patch, bios whose length isn't a multiple of the logical > > > block size can be generated by splitting, which isn't valid. > > > > How? This patch ensures every segment is block size aligned. > > No, it doesn't. It ensures that the *total* length of each bio is logical block > size aligned. It doesn't ensure that for the individual bvecs. By decreasing > the required memory alignment to below the logical block size, you're allowing > logical blocks to span a page boundary. Whenever the two pages involved aren't > physically contiguous, the data of the block will be split across two bvecs. I'm aware that spanning pages can cause bad splits on the bi_max_vecs condition, but I believe it's well handled here. Unless I'm terribly confused, which is certainly possible, I think you may have missed this part of the patch: @@ -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)); if (unlikely(size <= 0)) return size ? size : -EFAULT;
On Wed, May 18, 2022 at 07:59:36PM -0600, Keith Busch wrote: > On Wed, May 18, 2022 at 06:53:11PM -0700, Eric Biggers wrote: > > On Wed, May 18, 2022 at 07:00:39PM -0600, Keith Busch wrote: > > > On Wed, May 18, 2022 at 05:14:49PM -0700, Eric Biggers wrote: > > > > On Wed, May 18, 2022 at 10:11:31AM -0700, Keith Busch wrote: > > > > > diff --git a/block/fops.c b/block/fops.c > > > > > index b9b83030e0df..d8537c29602f 100644 > > > > > --- a/block/fops.c > > > > > +++ b/block/fops.c > > > > > @@ -54,8 +54,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)) > > > > > + 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; > > > > > > > > The block layer makes a lot of assumptions that bios can be split at any bvec > > > > boundary. With this patch, bios whose length isn't a multiple of the logical > > > > block size can be generated by splitting, which isn't valid. > > > > > > How? This patch ensures every segment is block size aligned. > > > > No, it doesn't. It ensures that the *total* length of each bio is logical block > > size aligned. It doesn't ensure that for the individual bvecs. By decreasing > > the required memory alignment to below the logical block size, you're allowing > > logical blocks to span a page boundary. Whenever the two pages involved aren't > > physically contiguous, the data of the block will be split across two bvecs. > > I'm aware that spanning pages can cause bad splits on the bi_max_vecs > condition, but I believe it's well handled here. Unless I'm terribly confused, > which is certainly possible, I think you may have missed this part of the > patch: > > @@ -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)); > if (unlikely(size <= 0)) > return size ? size : -EFAULT; > That makes the total length of each "batch" of pages be a multiple of the logical block size, but individual logical blocks within that batch can still be divided into multiple bvecs in the loop just below it: for (left = size, i = 0; left > 0; left -= len, i++) { struct page *page = pages[i]; len = min_t(size_t, PAGE_SIZE - offset, left); if (__bio_try_merge_page(bio, page, len, offset, &same_page)) { if (same_page) put_page(page); } else { if (WARN_ON_ONCE(bio_full(bio, len))) { bio_put_pages(pages + i, left, offset); return -EINVAL; } __bio_add_page(bio, page, len, offset); } offset = 0; } - Eric
On Wed, May 18, 2022 at 07:08:11PM -0700, Eric Biggers wrote: > On Wed, May 18, 2022 at 07:59:36PM -0600, Keith Busch wrote: > > I'm aware that spanning pages can cause bad splits on the bi_max_vecs > > condition, but I believe it's well handled here. Unless I'm terribly confused, > > which is certainly possible, I think you may have missed this part of the > > patch: > > > > @@ -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)); > > if (unlikely(size <= 0)) > > return size ? size : -EFAULT; > > > > That makes the total length of each "batch" of pages be a multiple of the > logical block size, but individual logical blocks within that batch can still be > divided into multiple bvecs in the loop just below it: I understand that, but the existing code conservatively assumes all pages are physically discontiguous and wouldn't have requested more pages if it didn't have enough bvecs for each of them: unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; So with the segment alignment guarantee, and ensured available bvec space, the created bio will always be a logical block size multiple. If we need to split it later due to some other constraint, we'll only split on a logical block size, even if its in the middle of a bvec. > for (left = size, i = 0; left > 0; left -= len, i++) { > struct page *page = pages[i]; > > len = min_t(size_t, PAGE_SIZE - offset, left); > > if (__bio_try_merge_page(bio, page, len, offset, &same_page)) { > if (same_page) > put_page(page); > } else { > if (WARN_ON_ONCE(bio_full(bio, len))) { > bio_put_pages(pages + i, left, offset); > return -EINVAL; > } > __bio_add_page(bio, page, len, offset); > } > offset = 0; > } > > - Eric
On Wed, May 18, 2022 at 08:25:26PM -0600, Keith Busch wrote: > On Wed, May 18, 2022 at 07:08:11PM -0700, Eric Biggers wrote: > > On Wed, May 18, 2022 at 07:59:36PM -0600, Keith Busch wrote: > > > I'm aware that spanning pages can cause bad splits on the bi_max_vecs > > > condition, but I believe it's well handled here. Unless I'm terribly confused, > > > which is certainly possible, I think you may have missed this part of the > > > patch: > > > > > > @@ -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)); > > > if (unlikely(size <= 0)) > > > return size ? size : -EFAULT; > > > > > > > That makes the total length of each "batch" of pages be a multiple of the > > logical block size, but individual logical blocks within that batch can still be > > divided into multiple bvecs in the loop just below it: > > I understand that, but the existing code conservatively assumes all pages are > physically discontiguous and wouldn't have requested more pages if it didn't > have enough bvecs for each of them: > > unsigned short nr_pages = bio->bi_max_vecs - bio->bi_vcnt; > > So with the segment alignment guarantee, and ensured available bvec space, the > created bio will always be a logical block size multiple. > > If we need to split it later due to some other constraint, we'll only split on > a logical block size, even if its in the middle of a bvec. > So the bio ends up with a total length that is a multiple of the logical block size, but the lengths of the individual bvecs in the bio are *not* necessarily multiples of the logical block size. That's the problem. Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple of 512. That was implied by it being a multiple of the logical block size. But the DMA alignment can be much lower, like 8 bytes (see nvme_set_queue_limits()). - Eric
On 5/19/22 05:27, Eric Biggers wrote:
> But the DMA alignment can be much lower, like 8 bytes (see nvme_set_queue_limits()).
There are block drivers that support byte alignment of block layer data
buffers. From the iSCSI over TCP driver:
blk_queue_dma_alignment(sdev->request_queue, 0);
Thanks,
Bart.
On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: > > So the bio ends up with a total length that is a multiple of the logical block > size, but the lengths of the individual bvecs in the bio are *not* necessarily > multiples of the logical block size. That's the problem. I'm surely missing something here. I know the bvecs are not necessarily lbs aligned, but why does that matter? Is there some driver that can only take exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment queue limit into account, but I am not sure that we need to. > Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple > of 512. Could you point me to some examples? > That was implied by it being a multiple of the logical block size. But > the DMA alignment can be much lower, like 8 bytes (see nvme_set_queue_limits()). That's the driver this was tested on, though I just changed it to 4 bytes for 5.19.
On 2022/05/19 6:56, Keith Busch wrote: > On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: >> >> So the bio ends up with a total length that is a multiple of the logical block >> size, but the lengths of the individual bvecs in the bio are *not* necessarily >> multiples of the logical block size. That's the problem. > > I'm surely missing something here. I know the bvecs are not necessarily lbs > aligned, but why does that matter? Is there some driver that can only take > exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment > queue limit into account, but I am not sure that we need to. For direct IO, the first bvec will always be aligned to a logical block size. See __blkdev_direct_IO() and __blkdev_direct_IO_simple(): if ((pos | iov_iter_alignment(iter)) & (bdev_logical_block_size(bdev) - 1)) return -EINVAL; And given that, all bvecs should also be LBA aligned since the LBA size is always a divisor of the page size. Since splitting is always done on an LBA boundary, I do not see how we can ever get bvecs that are not LBA aligned. Or I am missing something too... > >> Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple >> of 512. > > Could you point me to some examples? > >> That was implied by it being a multiple of the logical block size. But >> the DMA alignment can be much lower, like 8 bytes (see nvme_set_queue_limits()). > > That's the driver this was tested on, though I just changed it to 4 bytes for > 5.19.
> @@ -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? > + 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? > 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. > +static inline unsigned int bdev_dma_alignment(struct block_device *bdev) > +{ > + return queue_dma_alignment(bdev_get_queue(bdev)); > +} Plase do this in a separate patch.
On Wed, May 18, 2022 at 07:00:39PM -0600, Keith Busch wrote: > How? This patch ensures every segment is block size aligned. We can always > split a bio in the middle of a bvec for any lower level hardware constraints, > and I'm not finding any splitting criteria that would try to break a bio on a > non-block aligned boundary. Do you mean bio_vec with segment here? How do we ensure that?
On Wed, May 18, 2022 at 10:56:04PM -0600, Keith Busch wrote: > I'm surely missing something here. I know the bvecs are not necessarily lbs > aligned, but why does that matter? Is there some driver that can only take > exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment > queue limit into account, but I am not sure that we need to. At least stacking drivers had all kinds of interesting limitations in this area. How much testing did this series get with all kinds of interesting dm targets and md pesonalities?
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.
On Thu, May 19, 2022 at 09:41:14AM +0200, Christoph Hellwig wrote: > On Wed, May 18, 2022 at 10:56:04PM -0600, Keith Busch wrote: > > I'm surely missing something here. I know the bvecs are not necessarily lbs > > aligned, but why does that matter? Is there some driver that can only take > > exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment > > queue limit into account, but I am not sure that we need to. > > At least stacking drivers had all kinds of interesting limitations in > this area. How much testing did this series get with all kinds of > interesting dm targets and md pesonalities? The dma limit doesn't stack (should it?). It gets the default, sector size - 1, so their constraints are effectively unchanged.
On Wed, May 18, 2022 at 10:56:04PM -0600, Keith Busch wrote: > On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: > > Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple > > of 512. > > Could you point me to some examples? Just to answer my own question, blk-crypto and blk-merge appear to have these 512 byte bv_len assumptions.
On Thu, May 19, 2022 at 08:45:55AM +0200, Damien Le Moal wrote: > On 2022/05/19 6:56, Keith Busch wrote: > > On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: > >> > >> So the bio ends up with a total length that is a multiple of the logical block > >> size, but the lengths of the individual bvecs in the bio are *not* necessarily > >> multiples of the logical block size. That's the problem. > > > > I'm surely missing something here. I know the bvecs are not necessarily lbs > > aligned, but why does that matter? Is there some driver that can only take > > exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment > > queue limit into account, but I am not sure that we need to. > > For direct IO, the first bvec will always be aligned to a logical block size. > See __blkdev_direct_IO() and __blkdev_direct_IO_simple(): > > if ((pos | iov_iter_alignment(iter)) & > (bdev_logical_block_size(bdev) - 1)) > return -EINVAL; > > And given that, all bvecs should also be LBA aligned since the LBA size is > always a divisor of the page size. Since splitting is always done on an LBA > boundary, I do not see how we can ever get bvecs that are not LBA aligned. > Or I am missing something too... > You're looking at the current upstream code. This patch changes that to: 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; So, if this patch is accepted, then the file position and total I/O length will need to be logical block aligned (as before), but the memory address and length of each individual iovec will no longer need to be logical block aligned. How the iovecs get turned into bios (and thus bvecs) is a little complicated, but the result is that logical blocks will be able to span bvecs. - Eric
On Thu, May 19, 2022 at 11:01:05AM -0600, Keith Busch wrote: > On Wed, May 18, 2022 at 10:56:04PM -0600, Keith Busch wrote: > > On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: > > > Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple > > > of 512. > > > > Could you point me to some examples? > > Just to answer my own question, blk-crypto and blk-merge appear to have these > 512 byte bv_len assumptions. blk_bio_segment_split() and bio_advance_iter() are two more examples. - Eric
On Thu, May 19, 2022 at 10:27:04AM -0700, Eric Biggers wrote: > On Thu, May 19, 2022 at 11:01:05AM -0600, Keith Busch wrote: > > On Wed, May 18, 2022 at 10:56:04PM -0600, Keith Busch wrote: > > > On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: > > > > Note, there's also lots of code that assumes that bio_vec::bv_len is a multiple > > > > of 512. > > > > > > Could you point me to some examples? > > > > Just to answer my own question, blk-crypto and blk-merge appear to have these > > 512 byte bv_len assumptions. > > blk_bio_segment_split() and bio_advance_iter() are two more examples. I agree about blk_bio_segment_split(), but bio_advance_iter() looks fine. That just assumes the entire length is a multiple of 512, not any particular bvec. Anyway, I accept your point that some existing code has this assumption, and will address these before the next revision.
On Thu, May 19, 2022 at 09:39:12AM +0200, Christoph Hellwig wrote: > On Wed, May 18, 2022 at 07:00:39PM -0600, Keith Busch wrote: > > How? This patch ensures every segment is block size aligned. We can always > > split a bio in the middle of a bvec for any lower level hardware constraints, > > and I'm not finding any splitting criteria that would try to break a bio on a > > non-block aligned boundary. > > Do you mean bio_vec with segment here? How do we ensure that? I may not be understanding what you're asking here, but I'll try to answer. In this path, we are dealing with ITER_IOVEC type. This patch ensures each virtually contiguous segment is a block size multiple via the ALIGN_DOWN part of the patch, and builds the bvec accordingly. An error will occur if the ALIGN_DOWN ever results in a 0 size, so we will know each segment the user provided is appropriately sized. That's not to say each resulting bvec is block sized. Only the sum of all the bvecs is guaranteed that property. Consider something like the below userspace snippet reading a 512b sector crossing a page with an unusual offset. If the two pages are not contiguous: bvec[0].bv_offset = 4092 bvec[0].bv_len = 4 bvec[1].bv_offset = 0 bvec[1].bv_len = 508 If they are contiguous, you'd just get 1 bvec with the same bv_offset, but a 512b len. --- int ret, fd; char *buf; ... ret = posix_memalign((void **)&buf, 4096, 8192); ... ret = pread(fd, buf + 4092, 512, 0); --
On 5/20/22 02:19, Eric Biggers wrote: > On Thu, May 19, 2022 at 08:45:55AM +0200, Damien Le Moal wrote: >> On 2022/05/19 6:56, Keith Busch wrote: >>> On Wed, May 18, 2022 at 08:27:31PM -0700, Eric Biggers wrote: >>>> >>>> So the bio ends up with a total length that is a multiple of the logical block >>>> size, but the lengths of the individual bvecs in the bio are *not* necessarily >>>> multiples of the logical block size. That's the problem. >>> >>> I'm surely missing something here. I know the bvecs are not necessarily lbs >>> aligned, but why does that matter? Is there some driver that can only take >>> exactly 1 bvec, but allows it to be unaligned? If so, we could take the segment >>> queue limit into account, but I am not sure that we need to. >> >> For direct IO, the first bvec will always be aligned to a logical block size. >> See __blkdev_direct_IO() and __blkdev_direct_IO_simple(): >> >> if ((pos | iov_iter_alignment(iter)) & >> (bdev_logical_block_size(bdev) - 1)) >> return -EINVAL; >> >> And given that, all bvecs should also be LBA aligned since the LBA size is >> always a divisor of the page size. Since splitting is always done on an LBA >> boundary, I do not see how we can ever get bvecs that are not LBA aligned. >> Or I am missing something too... >> > > You're looking at the current upstream code. This patch changes that to: > > 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; > > So, if this patch is accepted, then the file position and total I/O length will > need to be logical block aligned (as before), but the memory address and length > of each individual iovec will no longer need to be logical block aligned. How > the iovecs get turned into bios (and thus bvecs) is a little complicated, but > the result is that logical blocks will be able to span bvecs. Indeed. I missed that change in patch 3. Got it. > > - Eric
On Thu, May 19, 2022 at 10:35:23AM -0600, Keith Busch wrote: > > At least stacking drivers had all kinds of interesting limitations in > > this area. How much testing did this series get with all kinds of > > interesting dm targets and md pesonalities? > > The dma limit doesn't stack (should it?). It gets the default, sector size - 1, > so their constraints are effectively unchanged. In general it should stack, as modulo implementation issues stacking drivers sould not care about the alignment. However since it doesn't there might be some of those.
On Thu, May 19, 2022 at 08:08:50AM -0600, Keith Busch wrote: > > > 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. Can you please document this with a comment in the code?
diff --git a/block/bio.c b/block/bio.c index 320514a47527..bde9b475a4d8 100644 --- a/block/bio.c +++ b/block/bio.c @@ -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)); if (unlikely(size <= 0)) return size ? size : -EFAULT; diff --git a/block/fops.c b/block/fops.c index b9b83030e0df..d8537c29602f 100644 --- a/block/fops.c +++ b/block/fops.c @@ -54,8 +54,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)) + 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; if (nr_pages <= DIO_INLINE_BIO_VECS) @@ -80,6 +81,11 @@ static ssize_t __blkdev_direct_IO_simple(struct kiocb *iocb, ret = bio_iov_iter_get_pages(&bio, iter); if (unlikely(ret)) goto out; + if (unlikely(iov_iter_count(iter))) { + /* iov is not aligned for a single bio */ + ret = -EINVAL; + goto out; + } ret = bio.bi_iter.bi_size; if (iov_iter_rw(iter) == WRITE) @@ -173,8 +179,9 @@ static ssize_t __blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter, loff_t pos = iocb->ki_pos; int ret = 0; - if ((pos | iov_iter_alignment(iter)) & - (bdev_logical_block_size(bdev) - 1)) + 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; if (iocb->ki_flags & IOCB_ALLOC_CACHE) @@ -298,8 +305,9 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb, loff_t pos = iocb->ki_pos; int ret = 0; - if ((pos | iov_iter_alignment(iter)) & - (bdev_logical_block_size(bdev) - 1)) + 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; if (iocb->ki_flags & IOCB_ALLOC_CACHE) 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); /* * Avoid references to bdev if not absolutely needed to give @@ -1165,11 +1165,14 @@ ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode, goto fail_dio; } - if (align & blocksize_mask) { - if (bdev) + if ((offset | align) & blocksize_mask) { + if (bdev) { blkbits = blksize_bits(bdev_logical_block_size(bdev)); + if (align & bdev_dma_alignment(bdev)) + goto fail_dio; + } blocksize_mask = (1 << blkbits) - 1; - if (align & blocksize_mask) + if ((offset | count) & blocksize_mask) goto fail_dio; } diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c index 80f9b047aa1b..0256d28baa8e 100644 --- a/fs/iomap/direct-io.c +++ b/fs/iomap/direct-io.c @@ -244,7 +244,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter, size_t copied = 0; size_t orig_count; - if ((pos | length | align) & ((1 << blkbits) - 1)) + if ((pos | length) & ((1 << blkbits) - 1) || + align & bdev_dma_alignment(iomap->bdev)) return -EINVAL; if (iomap->type == IOMAP_UNWRITTEN) { diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 5bdf2ac9142c..834b981ef01b 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1365,6 +1365,11 @@ static inline int queue_dma_alignment(const struct request_queue *q) return q ? q->dma_alignment : 511; } +static inline unsigned int bdev_dma_alignment(struct block_device *bdev) +{ + return queue_dma_alignment(bdev_get_queue(bdev)); +} + static inline int blk_rq_aligned(struct request_queue *q, unsigned long addr, unsigned int len) {