Linux-ext4 Archive on lore.kernel.org
 help / color / Atom feed
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
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

  parent reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-20 23:37 [PATCH v4 0/7] add support for " Satya Tangirala
2020-07-20 23:37 ` [PATCH v4 1/7] fscrypt: Add functions for direct I/O support Satya Tangirala
2020-07-22 17:04   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 2/7] direct-io: add support for fscrypt using blk-crypto Satya Tangirala
2020-07-22 17:05   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 3/7] iomap: support direct I/O with " Satya Tangirala
2020-07-22 17:06   ` Jaegeuk Kim
2020-07-22 21:16   ` Dave Chinner [this message]
2020-07-22 22:34     ` Eric Biggers
2020-07-22 22:44       ` Matthew Wilcox
2020-07-22 23:12         ` Eric Biggers
2020-07-22 23:26       ` Eric Biggers
2020-07-22 23:32         ` Darrick J. Wong
2020-07-22 23:43           ` Eric Biggers
2020-07-23 22:07       ` Dave Chinner
2020-07-23 23:03         ` Eric Biggers
2020-07-24  1:39           ` Dave Chinner
2020-07-24  3:46             ` Eric Biggers
2020-07-24  5:31               ` Dave Chinner
2020-07-24 17:41                 ` Eric Biggers
2020-07-25 23:47                   ` Dave Chinner
2020-07-25 23:59                     ` Dave Chinner
2020-07-26  2:42                     ` Eric Biggers
2020-07-27 17:16                       ` Eric Biggers
2020-07-20 23:37 ` [PATCH v4 4/7] ext4: " Satya Tangirala
2020-07-22 17:07   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 5/7] f2fs: " Satya Tangirala
2020-07-21 20:11   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 6/7] fscrypt: document inline encryption support Satya Tangirala
2020-07-22 17:01   ` Jaegeuk Kim
2020-07-20 23:37 ` [PATCH v4 7/7] fscrypt: update documentation for direct I/O support Satya Tangirala
2020-07-21  0:47   ` Eric Biggers
2020-07-22 16:57     ` Jaegeuk Kim
2020-07-21  0:56 ` [PATCH v4 0/7] add support for direct I/O with fscrypt using blk-crypto 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: link

Linux-ext4 Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-ext4/0 linux-ext4/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-ext4 linux-ext4/ https://lore.kernel.org/linux-ext4 \
		linux-ext4@vger.kernel.org
	public-inbox-index linux-ext4

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-ext4


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git