Message-ID: <20200723220752.GF2005@dread.disaster.area>
Date:   Fri, 24 Jul 2020 08:07:52 +1000
From:   Dave Chinner <david@...morbit.com>
To:     Eric Biggers <ebiggers@...nel.org>
Cc:     Satya Tangirala <satyat@...gle.com>, linux-fscrypt@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        linux-f2fs-devel@...ts.sourceforge.net, linux-ext4@...r.kernel.org,
        linux-xfs@...r.kernel.org
Subject: Re: [PATCH v4 3/7] iomap: support direct I/O with fscrypt using
 blk-crypto

On Wed, Jul 22, 2020 at 03:34:04PM -0700, Eric Biggers wrote:
> On Thu, Jul 23, 2020 at 07:16:29AM +1000, Dave Chinner wrote:
> > On Mon, Jul 20, 2020 at 11:37:35PM +0000, Satya Tangirala wrote:
> > > From: Eric Biggers <ebiggers@...gle.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@...gle.com>
> > > Co-developed-by: Satya Tangirala <satyat@...gle.com>
> > > Signed-off-by: Satya Tangirala <satyat@...gle.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.
> 
> In v3 and earlier this instead had the code to set an encryption context:
> 
> 	fscrypt_set_bio_crypt_ctx(bio, inode, pos >> inode->i_blkbits,
> 				  GFP_KERNEL);
> 
> Would you prefer that, even though the call to fscrypt_set_bio_crypt_ctx() would

Actually, I have no idea what that function does. It's not in a
5.8-rc6 kernel, and it's not in this patchset....

> always be a no-op currently (since for now, iomap_dio_zero() will never be
> called with an encrypted file) and thus wouldn't be properly tested?

Same can be said for this WARN_ON_ONCE() code :)

But, in the interests of not leaving landmines, if a fscrypt context
is needed to be attached to the bio for data IO in direct IO, it
should be attached to all bios that are allocated in the dio path
rather than leave a landmine for people in future to trip over.

> BTW, iomap_dio_zero() is actually limited to one page, so it's not quite
> "arbitrary sizes".

Yup, but that's an implentation detail, not a design constraint.
i.e. I typically review/talk about how stuff functions at a
design/architecture level, not how it's been implemented in the
code.

e.g. block size > page size patches in progress make use of the
"arbitrary length" capability of the design:

https://lore.kernel.org/linux-xfs/20181107063127.3902-7-david@fromorbit.com/

> iomap is used for other filesystem operations too, so we need to consider when
> to actually do the limiting.  I don't think we should break up the extents
> returned FS_IOC_FIEMAP, for example.  FIEMAP already has a defined behavior.
> Also, it would be weird for the list of extents that FIEMAP returns to change
> depending on whether the filesystem is mounted with '-o inlinecrypt' or not.

We don't need to care about that in the iomap code. The caller
controls the behaviour of the mapping callbacks themselves via
the iomap_ops structure they pass into high level iomap functions.

> That also avoids any confusion between pages and blocks, which is nice.

FWIW, the latest version of the above patchset (which,
co-incidentally, I was bring up to date yesterday) abstracts away
page and block sizes. It introduces the concept of "chunk size"
which is calculated from the combination of the current page's size
and the current inode's block size.

i.e. in the near future we are going to have both variable page
sizes (on a per-page basis via Willy's current work) and per-inode
blocks sizes smaller, the same and larger than the size of the
current pager. Hence we need to get rid of any assumptions about
page sizes and block sizes in the iomap code, not introduce new
ones.

Hence if there is any limitation of filesystem functionality based
on block size vs page size, it is going to be up to the filesystem
to detect and enforce those restrictions, not the iomap
infrastructure.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists