On Thu, Oct 31, 2019 at 02:22:34PM -0700, Christoph Hellwig wrote:

On Thu, Oct 31, 2019 at 04:50:45PM -0400, Theodore Y. Ts'o wrote:

quoted

One of the reasons I really want this is so I (as an upstream
maintainer of ext4 and fscrypt) can test the new code paths using
xfstests on GCE, without needing special pre-release hardware that has
the ICE support.

Yeah, I could probably get one of those dev boards internally at
Google, but they're a pain in the tuckus to use, and I'd much rather
be able to have my normal test infrastructure using gce-xfstests and
kvm-xfstests be able to test inline-crypto.  So in terms of CI
testing, having the blk-crypto is really going to be helpful.

Implementing the support in qemu or a special device mapper mode
seems like a much better idea for that use case over carrying the
code in the block layer and severely bloating the per-I/O data
structure.

QEMU doesn't support UFS, but even if it did and we added the UFS v2.1 crypto
support, it would preclude testing with anything other than a custom QEMU VM or
a system with real inline encryption hardware.  gce-xfstests wouldn't work.  So
it would be much harder to test inline encrypted I/O, so e.g. in practice it
wouldn't be tested as part of the regular ext4 regression testing.

The advantages of blk-crypto over a device mapper target like "dm-inlinecrypt"
are (a) blk-crypto is much easier for userspace to use, and (b) blk-crypto
allows upper layers to simply use inline encryption rather than have to
implement encryption twice, once manually and once with inline encryption.

It's true that as of this patchset, the only user of this stuff (fscrypt) still
implements both I/O paths anyway.  But that's something that could change later
once blk-crypto is ready for it, with longer IV support, O(1) keyslot lookups,
and a way to configure whether hardware is used or not.  Satya is already
looking into longer IV support, and I have a proposal for making the keyslot
lookups O(1) using a hash table.

I think that "Severely bloating the per-I/O data structure" is an exaggeration,
since that it's only 32 bytes, and it isn't in struct bio directly but rather in
struct bio_crypt_ctx...

In any case, Satya, it might be a good idea to reorganize this patchset so that
it first adds all logic that's needed for "real" inline encryption support
(including the needed parts of blk-crypto.c), then adds the crypto API fallback
as a separate patch.  That would separate the concerns more cleanly and make the
patchset easier to review, and make it easier to make the fallback
de-configurable or even remove it entirely if that turns out to be needed.

- Eric