Message-ID: <YgX/hl4GK85M9mGs@sol.localdomain>
Date:   Thu, 10 Feb 2022 22:17:42 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     linux-fscrypt@...r.kernel.org
Cc:     Theodore Ts'o <tytso@....edu>,
        "Darrick J . Wong" <djwong@...nel.org>,
        Dave Chinner <david@...morbit.com>,
        linux-f2fs-devel@...ts.sourceforge.net, linux-xfs@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, Jaegeuk Kim <jaegeuk@...nel.org>,
        linux-ext4@...r.kernel.org, Christoph Hellwig <hch@....de>
Subject: Re: [PATCH v11 0/5] add support for direct I/O with fscrypt using
 blk-crypto

On Fri, Jan 28, 2022 at 03:39:35PM -0800, Eric Biggers wrote:
> [Note: I'm planning to send a patchset adding STATX_DIRECTIO as was
> discussed on v10, but that will be a separate patchset.]
> 
> Encrypted files traditionally haven't supported DIO, due to the need to
> encrypt/decrypt the data.  However, when the encryption is implemented
> using inline encryption (blk-crypto) instead of the traditional
> filesystem-layer encryption, it is straightforward to support DIO.
> 
> This series adds support for this.  There are multiple use cases for DIO
> on encrypted files, but avoiding double caching on loopback devices
> located in an encrypted directory is the main one currently.
> 
> v1 through v9 of this series were sent out by Satya Tangirala.  I've
> cleaned up a few things since Satya's last version
> (https://lore.kernel.org/all/20210604210908.2105870-1-satyat@google.com/T/#u).
> But more notably, I've made a couple simplifications.
> 
> First, since f2fs has now been converted to use iomap for DIO, I've
> dropped the patch which added fscrypt support to fs/direct-io.c.
> 
> Second, I've returned to the original design where DIO requests must be
> fully aligned to the FS block size in terms of file position, length,
> and memory buffers.  Satya previously was pursuing a slightly different
> design, where the memory buffers (but not the file position and length)
> were allowed to be aligned to just the block device logical block size.
> This was at the request of Dave Chinner on v4 and v6 of the patchset
> (https://lore.kernel.org/linux-fscrypt/20200720233739.824943-1-satyat@google.com/T/#u
> and
> https://lore.kernel.org/linux-fscrypt/20200724184501.1651378-1-satyat@google.com/T/#u).
> 
> I believe that approach is a dead end, for two reasons.  First, it
> necessarily causes it to be possible that crypto data units span bvecs.
> Splits cannot occur at such locations; however the block layer currently
> assumes that bios can be split at any bvec boundary.  Changing that is
> quite difficult, as Satya's v9 patchset demonstrated.  This is not an
> issue if we require FS block aligned buffers instead.  Second, it
> doesn't change the fact that FS block alignment is still required for
> the file position and I/O length; this is unavoidable due to the
> granularity of encryption being the FS block size.  So, it seems that
> relaxing the memory buffer alignment requirement wouldn't make things
> meaningfully easier for applications, which raises the question of why
> we would bother with it in the first place.
> 
> Christoph Hellwig also said that he much prefers that fscrypt DIO be
> supported without sector-only alignment to start:
> https://lore.kernel.org/r/YPu+88KReGlt94o3@infradead.org
> 
> Given the above, as far as I know the only remaining objection to this
> patchset would be that DIO constraints aren't sufficiently discoverable
> by userspace.  Now, to put this in context, this is a longstanding issue
> with all Linux filesystems, except XFS which has XFS_IOC_DIOINFO.  It's
> not specific to this feature, and it doesn't actually seem to be too
> important in practice; many other filesystem features place constraints
> on DIO, and f2fs even *only* allows fully FS block size aligned DIO.
> (And for better or worse, many systems using fscrypt already have
> out-of-tree patches that enable DIO support, and people don't seem to
> have trouble with the FS block size alignment requirement.)
> 
> To address the issue of DIO constraints being insufficiently
> discoverable, I plan to make statx() expose this information.
> 
> This series applies to v5.17-rc1.
> 
> Changed v10 => v11:
>   * Changed fscrypt_dio_unsupported() back to fscrypt_dio_supported().
>   * Removed a mention of f2fs from fscrypt_dio_supported().
>   * Added Reviewed-by and Acked-by tags, including a couple from earlier
>     I had dropped due to the renaming of fscrypt_dio_supported().
>   * In fscrypt_limit_io_blocks(), don't load i_crypt_info until it's
>     known to be valid, to avoid confusion as is done elsewhere.
> 
> Eric Biggers (5):
>   fscrypt: add functions for direct I/O support
>   iomap: support direct I/O with fscrypt using blk-crypto
>   ext4: support direct I/O with fscrypt using blk-crypto
>   f2fs: support direct I/O with fscrypt using blk-crypto
>   fscrypt: update documentation for direct I/O support
> 
>  Documentation/filesystems/fscrypt.rst | 25 ++++++-
>  fs/crypto/crypto.c                    |  8 +++
>  fs/crypto/inline_crypt.c              | 93 +++++++++++++++++++++++++++
>  fs/ext4/file.c                        | 10 +--
>  fs/ext4/inode.c                       |  7 ++
>  fs/f2fs/data.c                        |  7 ++
>  fs/f2fs/f2fs.h                        |  6 +-
>  fs/iomap/direct-io.c                  |  6 ++
>  include/linux/fscrypt.h               | 18 ++++++
>  9 files changed, 173 insertions(+), 7 deletions(-)

I've applied this patchset to fscrypt.git#master for 5.18
(https://git.kernel.org/pub/scm/fs/fscrypt/fscrypt.git/log/).

- Eric

Powered by blists - more mailing lists