From: Eric Biggers <ebiggers@kernel.org> To: Satya Tangirala <satyat@google.com> Cc: linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-ext4@vger.kernel.org, linux-xfs@vger.kernel.org Subject: Re: [PATCH v3 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Mon, 20 Jul 2020 12:29:45 -0700 [thread overview] Message-ID: <20200720192945.GG1292162@gmail.com> (raw) In-Reply-To: <20200717014540.71515-4-satyat@google.com> On Fri, Jul 17, 2020 at 01:45:36AM +0000, Satya Tangirala wrote: > From: Eric Biggers <ebiggers@google.com> > > Wire up iomap direct I/O with the fscrypt additions for direct I/O, > and set bio crypt contexts on bios when appropriate. > > Make iomap_dio_bio_actor() call fscrypt_limit_io_pages() to ensure that > DUNs remain contiguous within a bio, since it works directly with logical > ranges and can't call fscrypt_mergeable_bio() on each page. This commit message is still confusing. How about the following: "Wire up iomap direct I/O with the fscrypt additions for direct I/O. This allows ext4 to support direct I/O on encrypted files when inline encryption is enabled. This change consists of two parts: - Set a bio_crypt_ctx on bios for encrypted files, so that the file contents get encrypted (or decrypted). - Ensure that encryption data unit numbers (DUNs) are contiguous within each bio. Use the new function fscrypt_limit_io_pages() for this, since the iomap code works directly with logical ranges and thus doesn't have a chance to call fscrypt_mergeable_bio() on each page. Note that fscrypt_limit_io_pages() is normally a no-op, as normally the DUNs simply increment along with the logical blocks. But it's needed to handle an edge case in one of the fscrypt IV generation methods." > @@ -183,11 +184,14 @@ static void > iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos, > unsigned len) > { > + struct inode *inode = file_inode(dio->iocb->ki_filp); > struct page *page = ZERO_PAGE(0); > int flags = REQ_SYNC | REQ_IDLE; > struct bio *bio; > > bio = bio_alloc(GFP_KERNEL, 1); > + fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > + GFP_KERNEL); > bio_set_dev(bio, iomap->bdev); > bio->bi_iter.bi_sector = iomap_sector(iomap, pos); > bio->bi_private = dio; iomap_dio_zero() is only used on partial filesystem blocks. But, we only allow direct I/O on encrypted files when the I/O is filesystem-block-aligned. So this part appears to be unnecessary. How about replacing it with: /* encrypted direct I/O is guaranteed to be fs-block aligned */ WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); > @@ -253,6 +257,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > ret = nr_pages; > goto out; > } > + nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages); > > if (need_zeroout) { > /* zero out from the start of the block to the write offset */ > @@ -270,6 +275,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > } > > bio = bio_alloc(GFP_KERNEL, nr_pages); > + fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > + GFP_KERNEL); > bio_set_dev(bio, iomap->bdev); > bio->bi_iter.bi_sector = iomap_sector(iomap, pos); > bio->bi_write_hint = dio->iocb->ki_hint; > @@ -307,6 +314,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > copied += n; > > nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES); > + nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages); > iomap_dio_submit_bio(dio, iomap, bio, pos); > pos += n; > } while (nr_pages); I think the part at the end is wrong. We want to limit the *next* bio, not the current one. So 'pos' needs to be updated first. I think it should be: iomap_dio_submit_bio(dio, iomap, bio, pos); pos += n; nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES); nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages);
WARNING: multiple messages have this Message-ID (diff)
From: Eric Biggers <ebiggers@kernel.org> To: Satya Tangirala <satyat@google.com> Cc: linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-ext4@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH v3 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Mon, 20 Jul 2020 12:29:45 -0700 [thread overview] Message-ID: <20200720192945.GG1292162@gmail.com> (raw) In-Reply-To: <20200717014540.71515-4-satyat@google.com> On Fri, Jul 17, 2020 at 01:45:36AM +0000, Satya Tangirala wrote: > From: Eric Biggers <ebiggers@google.com> > > Wire up iomap direct I/O with the fscrypt additions for direct I/O, > and set bio crypt contexts on bios when appropriate. > > Make iomap_dio_bio_actor() call fscrypt_limit_io_pages() to ensure that > DUNs remain contiguous within a bio, since it works directly with logical > ranges and can't call fscrypt_mergeable_bio() on each page. This commit message is still confusing. How about the following: "Wire up iomap direct I/O with the fscrypt additions for direct I/O. This allows ext4 to support direct I/O on encrypted files when inline encryption is enabled. This change consists of two parts: - Set a bio_crypt_ctx on bios for encrypted files, so that the file contents get encrypted (or decrypted). - Ensure that encryption data unit numbers (DUNs) are contiguous within each bio. Use the new function fscrypt_limit_io_pages() for this, since the iomap code works directly with logical ranges and thus doesn't have a chance to call fscrypt_mergeable_bio() on each page. Note that fscrypt_limit_io_pages() is normally a no-op, as normally the DUNs simply increment along with the logical blocks. But it's needed to handle an edge case in one of the fscrypt IV generation methods." > @@ -183,11 +184,14 @@ static void > iomap_dio_zero(struct iomap_dio *dio, struct iomap *iomap, loff_t pos, > unsigned len) > { > + struct inode *inode = file_inode(dio->iocb->ki_filp); > struct page *page = ZERO_PAGE(0); > int flags = REQ_SYNC | REQ_IDLE; > struct bio *bio; > > bio = bio_alloc(GFP_KERNEL, 1); > + fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > + GFP_KERNEL); > bio_set_dev(bio, iomap->bdev); > bio->bi_iter.bi_sector = iomap_sector(iomap, pos); > bio->bi_private = dio; iomap_dio_zero() is only used on partial filesystem blocks. But, we only allow direct I/O on encrypted files when the I/O is filesystem-block-aligned. So this part appears to be unnecessary. How about replacing it with: /* encrypted direct I/O is guaranteed to be fs-block aligned */ WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); > @@ -253,6 +257,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > ret = nr_pages; > goto out; > } > + nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages); > > if (need_zeroout) { > /* zero out from the start of the block to the write offset */ > @@ -270,6 +275,8 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > } > > bio = bio_alloc(GFP_KERNEL, nr_pages); > + fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits, > + GFP_KERNEL); > bio_set_dev(bio, iomap->bdev); > bio->bi_iter.bi_sector = iomap_sector(iomap, pos); > bio->bi_write_hint = dio->iocb->ki_hint; > @@ -307,6 +314,7 @@ iomap_dio_bio_actor(struct inode *inode, loff_t pos, loff_t length, > copied += n; > > nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES); > + nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages); > iomap_dio_submit_bio(dio, iomap, bio, pos); > pos += n; > } while (nr_pages); I think the part at the end is wrong. We want to limit the *next* bio, not the current one. So 'pos' needs to be updated first. I think it should be: iomap_dio_submit_bio(dio, iomap, bio, pos); pos += n; nr_pages = iov_iter_npages(dio->submit.iter, BIO_MAX_PAGES); nr_pages = fscrypt_limit_io_pages(inode, pos, nr_pages); _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2020-07-20 19:29 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-17 1:45 [PATCH v3 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-17 1:45 ` [PATCH v3 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 20:14 ` Eric Biggers 2020-07-20 20:14 ` [f2fs-dev] " Eric Biggers 2020-07-17 1:45 ` [PATCH v3 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-17 1:45 ` [PATCH v3 3/7] iomap: support direct I/O with " Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 19:29 ` Eric Biggers [this message] 2020-07-20 19:29 ` Eric Biggers 2020-07-17 1:45 ` [PATCH v3 4/7] ext4: " Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-17 1:45 ` [PATCH v3 5/7] f2fs: " Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-17 1:45 ` [PATCH v3 6/7] fscrypt: document inline encryption support Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 19:34 ` Eric Biggers 2020-07-20 19:34 ` [f2fs-dev] " Eric Biggers 2020-07-17 1:45 ` [PATCH v3 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala 2020-07-17 1:45 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 19:40 ` Eric Biggers 2020-07-20 19:40 ` [f2fs-dev] " Eric Biggers
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200720192945.GG1292162@gmail.com \ --to=ebiggers@kernel.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-fscrypt@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-xfs@vger.kernel.org \ --cc=satyat@google.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.