[v11,0/5] add support for direct I/O with fscrypt using blk-crypto

Message

Eric Biggers Jan. 28, 2022, 11:39 p.m. UTC

[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/[e-mail]/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/[e-mail]/T/#u
and
https://lore.kernel.org/linux-fscrypt/[e-mail]/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/[e-mail]

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(-)


base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07

Comments

Eric Biggers Feb. 11, 2022, 6:17 a.m. UTC | #1

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/[e-mail]/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/[e-mail]/T/#u
> and
> https://lore.kernel.org/linux-fscrypt/[e-mail]/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/[e-mail]
> 
> 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