| Message ID | 20210604210908.2105870-6-satyat@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for direct I/O with fscrypt using blk-crypto | expand |
On Fri, Jun 04, 2021 at 09:09:04PM +0000, Satya Tangirala wrote: > Previously, bio_iov_iter_get_pages() wasn't used with bios that could have > an encryption context. However, direct I/O support using blk-crypto > introduces this possibility, so this function must now respect > bio_required_sector_alignment() (otherwise, xfstests like generic/465 with > ext4 will fail). Can you be more clear that the fscrypt direct I/O support only requires this in order to support I/O segments that aren't fs-block aligned? I do still wonder if we should just not support that... Dave is the only person who has asked for it, and it's a lot of trouble to support. I also noticed that f2fs has always only supported direct I/O that is *fully* fs-block aligned (including the I/O segments) anyway. So presumably that limitation is not really that important after all... Does anyone else have thoughts on this? One more comment on this patch below: > > Signed-off-by: Satya Tangirala <satyat@google.com> > --- > block/bio.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/block/bio.c b/block/bio.c > index 32f75f31bb5c..99c510f706e2 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -1099,7 +1099,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) > * The function tries, but does not guarantee, to pin as many pages as > * fit into the bio, or are requested in @iter, whatever is smaller. If > * MM encounters an error pinning the requested pages, it stops. Error > - * is returned only if 0 pages could be pinned. > + * is returned only if 0 pages could be pinned. It also ensures that the number > + * of sectors added to the bio is aligned to bio_required_sector_alignment(). > * > * It's intended for direct IO, so doesn't do PSI tracking, the caller is > * responsible for setting BIO_WORKINGSET if necessary. > @@ -1107,6 +1108,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) > int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > { > int ret = 0; > + unsigned int aligned_sectors; > > if (iov_iter_is_bvec(iter)) { > if (bio_op(bio) == REQ_OP_ZONE_APPEND) > @@ -1121,6 +1123,15 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) > ret = __bio_iov_iter_get_pages(bio, iter); > } while (!ret && iov_iter_count(iter) && !bio_full(bio, 0)); > > + /* > + * Ensure that number of sectors in bio is aligned to > + * bio_required_sector_align() > + */ > + aligned_sectors = round_down(bio_sectors(bio), > + bio_required_sector_alignment(bio)); > + iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT); > + bio_truncate(bio, aligned_sectors << SECTOR_SHIFT); > + > /* don't account direct I/O as memory stall */ > bio_clear_flag(bio, BIO_WORKINGSET); > return bio->bi_vcnt ? 0 : ret; Doesn't this need to return an error if the bio's size gets rounded down to 0? For example if logical_block_size=512 and data_unit_size=4096, and the iov_iter points to 4096 bytes in 8 512-byte segments but the last one isn't mapped, then 7 pages would be pinned and the last one would fail. This would then truncate the bio's size to 0, but bio->bi_vcnt would be 7, so this would still return 0. It would also be necessary to release the pages before returning an error. - Eric
On Fri, Jul 23, 2021 at 02:33:02PM -0700, Eric Biggers wrote: > I do still wonder if we should just not support that... Dave is the only person > who has asked for it, and it's a lot of trouble to support. > > I also noticed that f2fs has always only supported direct I/O that is *fully* > fs-block aligned (including the I/O segments) anyway. So presumably that > limitation is not really that important after all... > > Does anyone else have thoughts on this? There are some use cases that really like sector aligned direct I/O, what comes to mind is some data bases, and file system repair tools (the latter on the raw block device). So it is nice to support, but not really required. So for now I'd much prefer to initially support inline encryption for direct I/O without that if that simplifies the support. We can revisit the additional complexity later. Also note that for cheap flash media pretending support for 512 byte blocks is actually a bit awwkward, so just presenting the media as having 4096 sectors in these setups would be the better choice anyway.
diff --git a/block/bio.c b/block/bio.c index 32f75f31bb5c..99c510f706e2 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1099,7 +1099,8 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) * The function tries, but does not guarantee, to pin as many pages as * fit into the bio, or are requested in @iter, whatever is smaller. If * MM encounters an error pinning the requested pages, it stops. Error - * is returned only if 0 pages could be pinned. + * is returned only if 0 pages could be pinned. It also ensures that the number + * of sectors added to the bio is aligned to bio_required_sector_alignment(). * * It's intended for direct IO, so doesn't do PSI tracking, the caller is * responsible for setting BIO_WORKINGSET if necessary. @@ -1107,6 +1108,7 @@ static int __bio_iov_append_get_pages(struct bio *bio, struct iov_iter *iter) int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) { int ret = 0; + unsigned int aligned_sectors; if (iov_iter_is_bvec(iter)) { if (bio_op(bio) == REQ_OP_ZONE_APPEND) @@ -1121,6 +1123,15 @@ int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter) ret = __bio_iov_iter_get_pages(bio, iter); } while (!ret && iov_iter_count(iter) && !bio_full(bio, 0)); + /* + * Ensure that number of sectors in bio is aligned to + * bio_required_sector_align() + */ + aligned_sectors = round_down(bio_sectors(bio), + bio_required_sector_alignment(bio)); + iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT); + bio_truncate(bio, aligned_sectors << SECTOR_SHIFT); + /* don't account direct I/O as memory stall */ bio_clear_flag(bio, BIO_WORKINGSET); return bio->bi_vcnt ? 0 : ret;
Previously, bio_iov_iter_get_pages() wasn't used with bios that could have an encryption context. However, direct I/O support using blk-crypto introduces this possibility, so this function must now respect bio_required_sector_alignment() (otherwise, xfstests like generic/465 with ext4 will fail). Signed-off-by: Satya Tangirala <satyat@google.com> --- block/bio.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)