| 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 |
Hi Satya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on ext4/dev f2fs/dev-test linus/master v5.13-rc4 next-20210604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/eea1225f680da16dce01bfb2914b9e8b78de298d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
git checkout eea1225f680da16dce01bfb2914b9e8b78de298d
# save the attached .config to linux build tree
make W=1 ARCH=um SUBARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from include/linux/kernel.h:14,
from include/asm-generic/bug.h:20,
from ./arch/um/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/mmdebug.h:5,
from include/linux/mm.h:9,
from block/bio.c:5:
block/bio.c: In function 'bio_iov_iter_get_pages':
>> block/bio.c:1131:10: error: implicit declaration of function 'bio_required_sector_alignment' [-Werror=implicit-function-declaration]
1131 | bio_required_sector_alignment(bio));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/math.h:14:46: note: in definition of macro '__round_mask'
14 | #define __round_mask(x, y) ((__typeof__(x))((y)-1))
| ^
block/bio.c:1130:20: note: in expansion of macro 'round_down'
1130 | aligned_sectors = round_down(bio_sectors(bio),
| ^~~~~~~~~~
cc1: some warnings being treated as errors
vim +/bio_required_sector_alignment +1131 block/bio.c
1083
1084 /**
1085 * bio_iov_iter_get_pages - add user or kernel pages to a bio
1086 * @bio: bio to add pages to
1087 * @iter: iov iterator describing the region to be added
1088 *
1089 * This takes either an iterator pointing to user memory, or one pointing to
1090 * kernel pages (BVEC iterator). If we're adding user pages, we pin them and
1091 * map them into the kernel. On IO completion, the caller should put those
1092 * pages. For bvec based iterators bio_iov_iter_get_pages() uses the provided
1093 * bvecs rather than copying them. Hence anyone issuing kiocb based IO needs
1094 * to ensure the bvecs and pages stay referenced until the submitted I/O is
1095 * completed by a call to ->ki_complete() or returns with an error other than
1096 * -EIOCBQUEUED. The caller needs to check if the bio is flagged BIO_NO_PAGE_REF
1097 * on IO completion. If it isn't, then pages should be released.
1098 *
1099 * The function tries, but does not guarantee, to pin as many pages as
1100 * fit into the bio, or are requested in @iter, whatever is smaller. If
1101 * MM encounters an error pinning the requested pages, it stops. Error
1102 * is returned only if 0 pages could be pinned. It also ensures that the number
1103 * of sectors added to the bio is aligned to bio_required_sector_alignment().
1104 *
1105 * It's intended for direct IO, so doesn't do PSI tracking, the caller is
1106 * responsible for setting BIO_WORKINGSET if necessary.
1107 */
1108 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
1109 {
1110 int ret = 0;
1111 unsigned int aligned_sectors;
1112
1113 if (iov_iter_is_bvec(iter)) {
1114 if (bio_op(bio) == REQ_OP_ZONE_APPEND)
1115 return bio_iov_bvec_set_append(bio, iter);
1116 return bio_iov_bvec_set(bio, iter);
1117 }
1118
1119 do {
1120 if (bio_op(bio) == REQ_OP_ZONE_APPEND)
1121 ret = __bio_iov_append_get_pages(bio, iter);
1122 else
1123 ret = __bio_iov_iter_get_pages(bio, iter);
1124 } while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
1125
1126 /*
1127 * Ensure that number of sectors in bio is aligned to
1128 * bio_required_sector_align()
1129 */
1130 aligned_sectors = round_down(bio_sectors(bio),
> 1131 bio_required_sector_alignment(bio));
1132 iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
1133 bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
1134
1135 /* don't account direct I/O as memory stall */
1136 bio_clear_flag(bio, BIO_WORKINGSET);
1137 return bio->bi_vcnt ? 0 : ret;
1138 }
1139 EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
1140
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Satya,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on block/for-next]
[also build test ERROR on ext4/dev f2fs/dev-test linus/master v5.13-rc4 next-20210604]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: powerpc64-randconfig-r025-20210604 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 5c0d1b2f902aa6a9cf47cc7e42c5b83bb2217cf9)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
# https://github.com/0day-ci/linux/commit/eea1225f680da16dce01bfb2914b9e8b78de298d
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Satya-Tangirala/add-support-for-direct-I-O-with-fscrypt-using-blk-crypto/20210605-051023
git checkout eea1225f680da16dce01bfb2914b9e8b78de298d
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from block/bio.c:5:
In file included from include/linux/mm.h:9:
In file included from include/linux/mmdebug.h:5:
In file included from include/linux/bug.h:5:
In file included from arch/powerpc/include/asm/bug.h:109:
In file included from include/asm-generic/bug.h:20:
In file included from include/linux/kernel.h:12:
In file included from include/linux/bitops.h:32:
In file included from arch/powerpc/include/asm/bitops.h:62:
arch/powerpc/include/asm/barrier.h:49:9: warning: '__lwsync' macro redefined [-Wmacro-redefined]
#define __lwsync() __asm__ __volatile__ (stringify_in_c(LWSYNC) : : :"memory")
^
<built-in>:309:9: note: previous definition is here
#define __lwsync __builtin_ppc_lwsync
^
>> block/bio.c:1131:10: error: implicit declaration of function 'bio_required_sector_alignment' [-Werror,-Wimplicit-function-declaration]
bio_required_sector_alignment(bio));
^
1 warning and 1 error generated.
vim +/bio_required_sector_alignment +1131 block/bio.c
1083
1084 /**
1085 * bio_iov_iter_get_pages - add user or kernel pages to a bio
1086 * @bio: bio to add pages to
1087 * @iter: iov iterator describing the region to be added
1088 *
1089 * This takes either an iterator pointing to user memory, or one pointing to
1090 * kernel pages (BVEC iterator). If we're adding user pages, we pin them and
1091 * map them into the kernel. On IO completion, the caller should put those
1092 * pages. For bvec based iterators bio_iov_iter_get_pages() uses the provided
1093 * bvecs rather than copying them. Hence anyone issuing kiocb based IO needs
1094 * to ensure the bvecs and pages stay referenced until the submitted I/O is
1095 * completed by a call to ->ki_complete() or returns with an error other than
1096 * -EIOCBQUEUED. The caller needs to check if the bio is flagged BIO_NO_PAGE_REF
1097 * on IO completion. If it isn't, then pages should be released.
1098 *
1099 * The function tries, but does not guarantee, to pin as many pages as
1100 * fit into the bio, or are requested in @iter, whatever is smaller. If
1101 * MM encounters an error pinning the requested pages, it stops. Error
1102 * is returned only if 0 pages could be pinned. It also ensures that the number
1103 * of sectors added to the bio is aligned to bio_required_sector_alignment().
1104 *
1105 * It's intended for direct IO, so doesn't do PSI tracking, the caller is
1106 * responsible for setting BIO_WORKINGSET if necessary.
1107 */
1108 int bio_iov_iter_get_pages(struct bio *bio, struct iov_iter *iter)
1109 {
1110 int ret = 0;
1111 unsigned int aligned_sectors;
1112
1113 if (iov_iter_is_bvec(iter)) {
1114 if (bio_op(bio) == REQ_OP_ZONE_APPEND)
1115 return bio_iov_bvec_set_append(bio, iter);
1116 return bio_iov_bvec_set(bio, iter);
1117 }
1118
1119 do {
1120 if (bio_op(bio) == REQ_OP_ZONE_APPEND)
1121 ret = __bio_iov_append_get_pages(bio, iter);
1122 else
1123 ret = __bio_iov_iter_get_pages(bio, iter);
1124 } while (!ret && iov_iter_count(iter) && !bio_full(bio, 0));
1125
1126 /*
1127 * Ensure that number of sectors in bio is aligned to
1128 * bio_required_sector_align()
1129 */
1130 aligned_sectors = round_down(bio_sectors(bio),
> 1131 bio_required_sector_alignment(bio));
1132 iov_iter_revert(iter, (bio_sectors(bio) - aligned_sectors) << SECTOR_SHIFT);
1133 bio_truncate(bio, aligned_sectors << SECTOR_SHIFT);
1134
1135 /* don't account direct I/O as memory stall */
1136 bio_clear_flag(bio, BIO_WORKINGSET);
1137 return bio->bi_vcnt ? 0 : ret;
1138 }
1139 EXPORT_SYMBOL_GPL(bio_iov_iter_get_pages);
1140
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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(-)