Re: [PATCH v6 2/9] block: Add encryption context to struct bio

["Martin K. Petersen" <martin.petersen@oracle.com>]

Eric,

> However, the nature of the per-bio information is very different.
> Most of the complexity in blk-integrity is around managing of a
> separate integrity scatterlist for each bio, alongside the regular
> data scatterlist.

> That's not something we need or want for inline encryption.  For each
> bio we just need a key, algorithm, data unit number, and data unit
> size.  Since the data unit number (IV) is automatically incremented
> for each sector and the encryption is length-preserving, there's no
> per-sector data.

Fair enough. I just wanted to make sure that you guys had actually
looked at the integrity stuff and determined it wasn't a good fit.

> There are some ways the two features could be supported simultaneously
> without using more space, like making the pointer point to a linked
> list of tagged structs, or making the struct contain both a
> bio_crypt_ctx and bio_integrity_payload (or whichever combination is
> enabled in kconfig).

We have previously discussed having a facility in which you could chain
several different things (with different prep/endio functions) off a
bio. Similar to how we allow arbitrary stacking of block_devices. That
was actually my main interest in terms of opening the integrity can of
worms in this thread. Trying to find out which pieces of the plumbing,
if any, could potentially be made generic and feature-independent.

The copy offload efforts, which are now again picking up momentum, also
need to hang things off of the bio. That's the original reason the
integrity field became a union, fwiw.

> So if people really aren't willing to accept the extra 8 bytes per bio
> even behind a kconfig option, my vote is we that we put
> bi_crypt_context in the union with bi_integrity, and add a flag
> REQ_INLINECRYPT (like REQ_INTEGRITY) that indicates that the
> bi_crypt_context member of the union is valid.

Agreed.

> We'd also need some error-handling to prevent the two features from
> actually being used together.  It looks like there are several cases
> to consider.  One of them is what happens if bio_crypt_set_ctx() is
> called when blk-integrity verification or generation is enabled for
> the disk.

The integrity profile is only attached if the device driver identifies a
discovered device as capable. At least in the short term no device
should indicate simultaneous support for DIX and your crypto interface.

Not saying that sanity checks shouldn't exist. But I think both of these
features fall into things that are registered at device discovery time
so we shouldn't need to clutter the I/O hot path with mutual exclusivity
checks.

-- 
Martin K. Petersen	Oracle Linux Engineering