Re: [PATCH v2 2/3] dm-inlinecrypt: add target for inline block device encryption

[Linlin Zhang <linlin.zhang@oss.qualcomm.com>]

On 4/29/2026 12:36 AM, Benjamin Marzinski wrote:
> On Tue, Apr 28, 2026 at 05:20:07PM +0800, Linlin Zhang wrote:
>>
>>
>> On 4/28/2026 7:21 AM, Benjamin Marzinski wrote:
>>> On Mon, Apr 27, 2026 at 01:23:27AM -0400, Benjamin Marzinski wrote:
>>>> On Fri, Apr 10, 2026 at 06:40:30AM -0700, Linlin Zhang wrote:
>>>>> From: Eric Biggers <ebiggers@google.com>
>>>>> +	/*
>>>>> +	 * Since we've added an encryption context to the bio and
>>>>> +	 * blk-crypto-fallback may be needed to process it, it's necessary to
>>>>> +	 * use the fallback-aware bio submission code rather than
>>>>> +	 * unconditionally returning DM_MAPIO_REMAPPED.
>>>>> +	 *
>>>>> +	 * To get the correct accounting for a dm target in the case where
>>>>> +	 * __blk_crypto_submit_bio() doesn't take ownership of the bio (returns
>>>>> +	 * true), call __blk_crypto_submit_bio() directly and return
>>>>> +	 * DM_MAPIO_REMAPPED in that case, rather than relying on
>>>>> +	 * blk_crypto_submit_bio() which calls submit_bio() in that case.
>>>>> +	 */
>>>>> +	if (__blk_crypto_submit_bio(bio))
>>>>
>>>> This will still double account for fallback writes (which call
>>>> submit_bio() on the encrypted bios, and return DM_MAPIO_SUBMITTED here). 
>>>
>>> Just to clarify, I'm talking about the vmstats accounting. The IO
>>> originally gets accounted by submit_bio() when the bio is submitted to
>>> the dm device. For actual inline encryption and fallback reads, dm will
>>> submit the bio to the underlying device using submit_bio_noacct() to
>>> avoid double-counting the IO.
>>>
>>> For fallback writes, __blk_crypto_submit_bio() will submit the encrypted
>>> bios to the underlying device with submit_bio(). This adds the IO
>>> sectors again, even though it's the same IO, only encrypted now.
>>
>>
>> Right, thanks for calling this out.
>>
>> For fallback writes, the IO is still double-counted. Given that this only
>> affects IO accounting in the blk-crypto fallback write slow-path and not
>> correctness, I think this is an acceptable tradeoff, and we can leave a
>> TODO to revisit the accounting once a better solution exists.
>>
>> Add the bellow to the annotate.
>>
>>   /*
>>    * TODO: blk-crypto fallback write slow-path currently double-accounts
>>    * IO in vmstat, as encrypted bios are submitted via submit_bio().
>>    * This does not affect data correctness. Consider fixing this if
>>    * a cleaner accounting model for derived bios is introduced.
>>    */
>>
>> Do you agree?
> 
> You could add an extra argument, for instance "bool need_acct", to
> __blk_crypto_submit_bio(), and plumb it through to
> __blk_crypto_fallback_encrypt_bio(), where it could be used to choose
> between calling submit_bio() and and submit_bio_noacct().
> 
> We could even add a flag to cloned bios for stacked devices, that could
> be checked in submit_bio(), so we didn't need to have
> submit_bio_noacct(). But this is a pretty niche case with other
> solutions, so I'm not sure if it warrants adding more checks to
> submit_bio().
> 
> I do agree that people probably aren't using dm-inlinecrypt for devices
> where they don't actually have inline encryption capabilities, so it's
> not a major issue. What to you think, Mikulas?

Thanks for the suggestions.

Adding a bool need_acct parameter to __blk_crypto_submit_bio() would require
updating all existing callers, which feels rather intrusive given that the
accounting issue only affects the blk‑crypto fallback write slow‑path. I’m a
bit concerned that this would broaden the scope of the change more than
necessary for the problem at hand.

An alternative might be to track this at the bio level instead — for example,
by introducing a bio flag or metadata that indicates whether accounting should
be charged for a derived/cloned bio. That would avoid having to thread additional
parameters through multiple blk‑crypto entry points. However, this likely needs
a broader discussion to make sure it fits well with existing stacking and
accounting semantics.

Also, as discussed, the current behavior does not affect data correctness; it
only results in double vmstat accounting for fallback writes. Given that
dm‑inlinecrypt is expected to be used primarily on devices with actual inline
encryption support, the fallback write path should be relatively uncommon in
practice.

With that in mind, would it be acceptable to merge the current change as‑is,
with an explicit TODO documenting the double‑accounting in the fallback write path,
and revisit the accounting model in a follow‑up patch once we have agreement on a
cleaner solution? What do you think, Ben and Mikulas?

Happy to iterate further if there’s a preferred direction here. 

> 
> -Ben
>  
>>>
>>> -Ben
>>>
>>>>
>>>> -Ben
>>>>
>>>>> +		return DM_MAPIO_REMAPPED;
>>>>> +	return DM_MAPIO_SUBMITTED;
>>>>> +}
>>>>
>>>
>