Re: [dm-crypt] [dm-devel] [RFC PATCH 0/2] dm crypt: Allow unaligned buffer lengths for skcipher devices

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/09/25 4:14, Sudhakar Panneerselvam wrote:
>>
>> On Thu, 24 Sep 2020, Sudhakar Panneerselvam wrote:
>>
>>>> By copying it to a temporary aligned buffer and issuing I/O on this
>>>> buffer.
>>>
>>> I don't like this idea. Because, you need to allocate additional pages
>>> for the entire I/O size(for the misaligned case, if you think through
>>
>> You can break the I/O to smaller pieces. You can use mempool for
>> pre-allocation of the pages.
> 
> Assuming we do this, how is this code simpler(based on your
> comment below) than the fix in dm-crypt? In fact, this approach 
> would make the code change look bad in vhost, at the same time
> having performance penalty. By doing this, we are just moving the 
> responsibility to other unrelated component.

Because vhost is at the top of the block-io food chain. Fixing the unaligned
segments there will ensure that it does not matter what device is under it. It
will work.

I am still baffled that the unaligned segments go through in the first place...
Do we have something missing in the BIO code ?

> 
>>
>>> carefully, you will know why we have to allocate a temporary buffer that
>>> is as big as the original IO) and on top of it, copying the buffer from
>>> original to temporary buffer which is all unnecessary while it can
>>> simply be fixed in dm-crypt without any of these additional overheads.
>>>
>>>>
>>>>> Only other
>>>>> possibility I see is to have windows fix it by always sending 512 byte
>>>>> aligned buffer lengths, but going with my earlier point that every other
>>>>> component in the Linux IO path handles this case well except for
>>>>> dm-crypt, so it make more sense to fix it in dm-crypt.
>>>>>
>>>>> Thanks
>>>>> Sudhakar
>>>>
>>>> Are you sure that the problem is only with dm-crypt? You haven't tried
>> all
>>>> the existing block device drivers, have you?
>>>
>>> My question is, why dm-crypt worries about alignment requirement of
>>> other layers? Is there anything that impacts dm-crypt if the segment
>>> lengths are not aligned?(I believe this case is just not handled so far
>>
>> Because the code is simpler if it assumes aligned buffers.
> 
> Did you get a chance to review my changes? If you want more documentation,
> improve the code, etc, let me know, I can do that if there is scope for that.
> 
>>
>>> in dm-crypt and my patch addresses it). Should dm-crypt not just pass on
>>> all those I/O requests to those respective layers to handle it which
>>> will be more graceful?
>>>
>>> -Sudhakar
>>
>> So, suppose that we change dm-crypt to handle your workload - what are
>> you
>> going to do with other block device drivers that assume aligned bio vector
>> length? How will you find all the other drivers that need to be changed?
>>
>> You are claiming that "every other component in the Linux IO path handles
>> this case well except for dm-crypt", but this claim is wrong. There are
>> other driver that don't handle misaligned length as well.
> 
> I should not have said, "every other component", I take that back, sorry. How
> about doing something like this in crypt_convert_block_skcipher():
> 
> Add a check that looks at the alignment requirement of the low-level driver
> and reject the I/O if it doesn't meet that requirement. This means, we still
> need to handle the case in this function where the low lever driver support
> unaligned buffer lengths, that means, my other changes in this function
> would still be needed.
> 
> Is this acceptable to everyone?
> 
> -Sudhakar
> 
>>
>> Mikulas
>>
>> --
>> dm-devel mailing list
>> dm-devel@redhat.com
>> https://www.redhat.com/mailman/listinfo/dm-devel
>>
> 


-- 
Damien Le Moal
Western Digital Research