| Message ID | 20200709194751.2579207-3-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 Thu, Jul 09, 2020 at 07:47:48PM +0000, Satya Tangirala wrote: > From: Eric Biggers <ebiggers@google.com> > > Set bio crypt contexts on bios by calling into fscrypt when required, > and explicitly check for DUN continuity when adding pages to the bio. > (While DUN continuity is usually implied by logical block contiguity, > this is not the case when using certain fscrypt IV generation methods > like IV_INO_LBLK_32). I know it is asking you for more work, but instead of adding more features to the legacy direct I/O code, could you just switch the user of it (I guess this is for f2f2?) to the iomap one?
On Fri, Jul 10, 2020 at 06:34:06AM +0100, Christoph Hellwig wrote: > On Thu, Jul 09, 2020 at 07:47:48PM +0000, Satya Tangirala wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Set bio crypt contexts on bios by calling into fscrypt when required, > > and explicitly check for DUN continuity when adding pages to the bio. > > (While DUN continuity is usually implied by logical block contiguity, > > this is not the case when using certain fscrypt IV generation methods > > like IV_INO_LBLK_32). > > I know it is asking you for more work, but instead of adding more > features to the legacy direct I/O code, could you just switch the user > of it (I guess this is for f2f2?) to the iomap one? Eventually we should do that, as well as convert f2fs's fiemap, bmap, and llseek to use iomap. However there's a nontrivial barrier to entry, at least for someone who isn't an expert in iomap, especially since f2fs currently doesn't use iomap at all and thus doesn't have an iomap_ops implementation. And using ext4 as an example, there will be some subtle cases that need to be handled. Satya says he's looking into it; we'll see what he can come up with and what the f2fs developers say. If it turns out to be difficult and people think this patchset is otherwise ready, we probably shouldn't hold it up on that. This is a very small patch, and Satya and I have to maintain it for years in downstream kernels anyway, so it will be used and tested regardless. It would also be nice to allow userspace (e.g. xfstests) to assume that if the inlinecrypt mount option is supported, then direct I/O is supported too, without having to handle intermediate kernel releases where inlinecrypt was supported but not direct I/O. - Eric
On Mon, Jul 13, 2020 at 11:36:19AM -0700, Eric Biggers wrote: > On Fri, Jul 10, 2020 at 06:34:06AM +0100, Christoph Hellwig wrote: > > On Thu, Jul 09, 2020 at 07:47:48PM +0000, Satya Tangirala wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > Set bio crypt contexts on bios by calling into fscrypt when required, > > > and explicitly check for DUN continuity when adding pages to the bio. > > > (While DUN continuity is usually implied by logical block contiguity, > > > this is not the case when using certain fscrypt IV generation methods > > > like IV_INO_LBLK_32). > > > > I know it is asking you for more work, but instead of adding more > > features to the legacy direct I/O code, could you just switch the user > > of it (I guess this is for f2f2?) to the iomap one? > > Eventually we should do that, as well as convert f2fs's fiemap, bmap, and llseek > to use iomap. However there's a nontrivial barrier to entry, at least for > someone who isn't an expert in iomap, especially since f2fs currently doesn't > use iomap at all and thus doesn't have an iomap_ops implementation. And using > ext4 as an example, there will be some subtle cases that need to be handled. > > Satya says he's looking into it; we'll see what he can come up with and what the > f2fs developers say. > > If it turns out to be difficult and people think this patchset is otherwise > ready, we probably shouldn't hold it up on that. This is a very small patch, > and Satya and I have to maintain it for years in downstream kernels anyway, so > it will be used and tested regardless. It would also be nice to allow userspace > (e.g. xfstests) to assume that if the inlinecrypt mount option is supported, > then direct I/O is supported too, without having to handle intermediate kernel > releases where inlinecrypt was supported but not direct I/O. > As Eric pointed out, it doesn't seem to be completely straightforward to move f2fs to using iomap - I'm still looking into it, but for now I've sent out v2 (and v3 just because I forgot to add a changelog to v2) with the changes to fs/direct-io.c as is from v1, but (again, for the reasons Eric points out) I think it'd be better not to hold this patch up for that. > - Eric
diff --git a/fs/direct-io.c b/fs/direct-io.c index 6d5370eac2a8..f27f7e3780ee 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -24,6 +24,7 @@ #include <linux/module.h> #include <linux/types.h> #include <linux/fs.h> +#include <linux/fscrypt.h> #include <linux/mm.h> #include <linux/slab.h> #include <linux/highmem.h> @@ -411,6 +412,7 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio, sector_t first_sector, int nr_vecs) { struct bio *bio; + struct inode *inode = dio->inode; /* * bio_alloc() is guaranteed to return a bio when allowed to sleep and @@ -418,6 +420,9 @@ dio_bio_alloc(struct dio *dio, struct dio_submit *sdio, */ bio = bio_alloc(GFP_KERNEL, nr_vecs); + fscrypt_set_bio_crypt_ctx(bio, inode, + sdio->cur_page_fs_offset >> inode->i_blkbits, + GFP_KERNEL); bio_set_dev(bio, bdev); bio->bi_iter.bi_sector = first_sector; bio_set_op_attrs(bio, dio->op, dio->op_flags); @@ -782,9 +787,17 @@ static inline int dio_send_cur_page(struct dio *dio, struct dio_submit *sdio, * current logical offset in the file does not equal what would * be the next logical offset in the bio, submit the bio we * have. + * + * When fscrypt inline encryption is used, data unit number + * (DUN) contiguity is also required. Normally that's implied + * by logical contiguity. However, certain IV generation + * methods (e.g. IV_INO_LBLK_32) don't guarantee it. So, we + * must explicitly check fscrypt_mergeable_bio() too. */ if (sdio->final_block_in_bio != sdio->cur_page_block || - cur_offset != bio_next_offset) + cur_offset != bio_next_offset || + !fscrypt_mergeable_bio(sdio->bio, dio->inode, + cur_offset >> dio->inode->i_blkbits)) dio_bio_submit(dio, sdio); }