From: Dave Chinner <david@fromorbit.com> 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, Eric Biggers <ebiggers@google.com> Subject: Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Thu, 23 Jul 2020 07:16:29 +1000 [thread overview] Message-ID: <20200722211629.GE2005@dread.disaster.area> (raw) In-Reply-To: <20200720233739.824943-4-satyat@google.com> On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote: > From: Eric Biggers <ebiggers@google.com> > > 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. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > Co-developed-by: Satya Tangirala <satyat@google.com> > Signed-off-by: Satya Tangirala <satyat@google.com> > --- > fs/iomap/direct-io.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index ec7b78e6feca..12064daa3e3d 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -6,6 +6,7 @@ > #include <linux/module.h> > #include <linux/compiler.h> > #include <linux/fs.h> > +#include <linux/fscrypt.h> > #include <linux/iomap.h> > #include <linux/backing-dev.h> > #include <linux/uio.h> > @@ -183,11 +184,16 @@ 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); > + > + /* encrypted direct I/O is guaranteed to be fs-block aligned */ > + WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); Which means you are now placing a new constraint on this code in that we cannot ever, in future, zero entire blocks here. This code can issue arbitrary sized zeroing bios - multiple entire fs blocks blocks if necessary - so I think constraining it to only support partial block zeroing by adding a warning like this is no correct. > @@ -253,6 +259,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); So if "pos" overlaps a 2^32 offset when a certain mode is used, we have to break up the IO? If true, I'm not sure that this belongs here. Limiting the size of the IOs because of filesytem contraints belongs in the filesystem extent mapping code. That's the point where the IO is broken up into maximally sized chunks the filesystem can issue as a contiguous range. If the fscrypt code is breaking that "contiguous IO range" because of the mode being used, the fs mapping code should break the mapping at the boundery where the IO needs to be broken. Hence the dio mapping code here will never build IOs that cross this -filesystem- encryption limitation, and we don't need this fscrypt code in the direct IO path at all. Cheers, Dave. -- Dave Chinner david@fromorbit.com
WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com> To: Satya Tangirala <satyat@google.com> Cc: Eric Biggers <ebiggers@google.com>, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org Subject: Re: [f2fs-dev] [PATCH v4 3/7] iomap: support direct I/O with fscrypt using blk-crypto Date: Thu, 23 Jul 2020 07:16:29 +1000 [thread overview] Message-ID: <20200722211629.GE2005@dread.disaster.area> (raw) In-Reply-To: <20200720233739.824943-4-satyat@google.com> On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote: > From: Eric Biggers <ebiggers@google.com> > > 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. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > Co-developed-by: Satya Tangirala <satyat@google.com> > Signed-off-by: Satya Tangirala <satyat@google.com> > --- > fs/iomap/direct-io.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c > index ec7b78e6feca..12064daa3e3d 100644 > --- a/fs/iomap/direct-io.c > +++ b/fs/iomap/direct-io.c > @@ -6,6 +6,7 @@ > #include <linux/module.h> > #include <linux/compiler.h> > #include <linux/fs.h> > +#include <linux/fscrypt.h> > #include <linux/iomap.h> > #include <linux/backing-dev.h> > #include <linux/uio.h> > @@ -183,11 +184,16 @@ 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); > + > + /* encrypted direct I/O is guaranteed to be fs-block aligned */ > + WARN_ON_ONCE(fscrypt_needs_contents_encryption(inode)); Which means you are now placing a new constraint on this code in that we cannot ever, in future, zero entire blocks here. This code can issue arbitrary sized zeroing bios - multiple entire fs blocks blocks if necessary - so I think constraining it to only support partial block zeroing by adding a warning like this is no correct. > @@ -253,6 +259,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); So if "pos" overlaps a 2^32 offset when a certain mode is used, we have to break up the IO? If true, I'm not sure that this belongs here. Limiting the size of the IOs because of filesytem contraints belongs in the filesystem extent mapping code. That's the point where the IO is broken up into maximally sized chunks the filesystem can issue as a contiguous range. If the fscrypt code is breaking that "contiguous IO range" because of the mode being used, the fs mapping code should break the mapping at the boundery where the IO needs to be broken. Hence the dio mapping code here will never build IOs that cross this -filesystem- encryption limitation, and we don't need this fscrypt code in the direct IO path at all. Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ 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-22 21:16 UTC|newest] Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-20 23:37 [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:04 ` Jaegeuk Kim 2020-07-22 17:04 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:05 ` Jaegeuk Kim 2020-07-22 17:05 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:06 ` Jaegeuk Kim 2020-07-22 17:06 ` [f2fs-dev] " Jaegeuk Kim 2020-07-22 21:16 ` Dave Chinner [this message] 2020-07-22 21:16 ` Dave Chinner 2020-07-22 22:34 ` Eric Biggers 2020-07-22 22:34 ` [f2fs-dev] " Eric Biggers 2020-07-22 22:44 ` Matthew Wilcox 2020-07-22 22:44 ` [f2fs-dev] " Matthew Wilcox 2020-07-22 23:12 ` Eric Biggers 2020-07-22 23:12 ` [f2fs-dev] " Eric Biggers 2020-07-22 23:26 ` Eric Biggers 2020-07-22 23:26 ` [f2fs-dev] " Eric Biggers 2020-07-22 23:32 ` Darrick J. Wong 2020-07-22 23:32 ` [f2fs-dev] " Darrick J. Wong 2020-07-22 23:43 ` Eric Biggers 2020-07-22 23:43 ` [f2fs-dev] " Eric Biggers 2020-07-23 22:07 ` Dave Chinner 2020-07-23 22:07 ` [f2fs-dev] " Dave Chinner 2020-07-23 23:03 ` Eric Biggers 2020-07-23 23:03 ` [f2fs-dev] " Eric Biggers 2020-07-24 1:39 ` Dave Chinner 2020-07-24 1:39 ` [f2fs-dev] " Dave Chinner 2020-07-24 3:46 ` Eric Biggers 2020-07-24 3:46 ` [f2fs-dev] " Eric Biggers 2020-07-24 5:31 ` Dave Chinner 2020-07-24 5:31 ` [f2fs-dev] " Dave Chinner 2020-07-24 17:41 ` Eric Biggers 2020-07-24 17:41 ` [f2fs-dev] " Eric Biggers 2020-07-25 23:47 ` Dave Chinner 2020-07-25 23:47 ` [f2fs-dev] " Dave Chinner 2020-07-25 23:59 ` Dave Chinner 2020-07-25 23:59 ` [f2fs-dev] " Dave Chinner 2020-07-26 2:42 ` Eric Biggers 2020-07-26 2:42 ` [f2fs-dev] " Eric Biggers 2020-07-27 17:16 ` Eric Biggers 2020-07-27 17:16 ` [f2fs-dev] " Eric Biggers 2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:07 ` Jaegeuk Kim 2020-07-22 17:07 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-21 20:11 ` Jaegeuk Kim 2020-07-21 20:11 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-22 17:01 ` Jaegeuk Kim 2020-07-22 17:01 ` [f2fs-dev] " Jaegeuk Kim 2020-07-20 23:37 ` [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala 2020-07-20 23:37 ` [f2fs-dev] " Satya Tangirala via Linux-f2fs-devel 2020-07-21 0:47 ` Eric Biggers 2020-07-21 0:47 ` [f2fs-dev] " Eric Biggers 2020-07-22 16:57 ` Jaegeuk Kim 2020-07-22 16:57 ` [f2fs-dev] " Jaegeuk Kim 2020-07-21 0:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto Eric Biggers 2020-07-21 0:56 ` [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=20200722211629.GE2005@dread.disaster.area \ --to=david@fromorbit.com \ --cc=ebiggers@google.com \ --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.