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: > > 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
