[dm-devel] [RFC PATCH 1/1] dm crypt: change maximum sector size to PAGE_SIZE

[Itai Handler]

On 14/11/2021 13:56, Itai Handler wrote:
> On 11/11/2021 15:07, Milan Broz wrote:
>> On 10/11/2021 18:43, Itai Handler wrote:
>>> Maximum sector size of dm-crypt is currently limited to 4096 bytes.
>>>
>>> On systems where PAGE_SIZE is larger than 4096 bytes, using larger
>>> sectors can be beneficial for performance reasons.
>>
>> The limit to 4096 was set because this is the smallest possible
>> page size that all platform supports.
>>
>> If you allow a higher size here, the device cannot be activated on a 
>> platform
>> with the smaller page size. (Encrypted sector size becomes
>> atomic sector size for all upper layers - as you mention below, not
>> all fs support bigger sectors.)
>>
>> For LUKS, this is not acceptable - the format is portable by definition.
>>
>> For specific dm-crypt device, I am not sure. I would better kept
>> the 4096 page size limit here.
>
> I considered only plain dm-crypt since I am unfamiliar with LUKS.
> Does LUKS assume that dm-crypt sector size is limited to 4K?
> If so, maybe I'll be able to also patch LUKS regarding this issue.
>
>>
>> It also depends on crypto API driver here (performance is usually 
>> optimized to 4k).
>> What cipher and encryption mode did you use for test?
>
> The cipher I used for the test is not publicly available but I can say 
> that it's performance
> is not optimized to 4k blocks.
> I believe that this results from the high overhead of setting up DMA 
> transfers. (my
> cipher uses DMA to transfer data between memory and programmable logic).
> There are many additional ciphers that use DMA in the tree, but I 
> cannot run any
> benchmark with them at the moment.
>
> I have performed some additional benchmarks using the ARM 
> Cryptographic Extensions
> CPU ciphers and saw that increasing block size beyond 4K does increase 
> performance,
> albeit the performance improvement isn't as large as I've seen when 
> using my cipher.
>
> Following are "tcrypt mode=600 sec=5 num_mb=512" results for 
> xts-aes-ce decryption
> (ARM CPU Cryptographic Extensions cipher):
>   testing speed of multibuffer xts(aes) (xts-aes-ce) decryption
>   ...
>   trcypt: test 5 (256 bit key, 4096 byte blocks): 801792 operations in 
> 5 seconds (3284140032 bytes)
>   ...
>   trcypt: test 9 (256 bit key, 65536 byte blocks): 63488 operations in 
> 5 seconds (4160749568 bytes)
>
> That translates to:
>   657 MB/s for 4K byte blocks.
>   832 MB/s for 64K blocks.
>
> That means that there is about 27 percents improvement when 
> transitioning to 64K sectors,
> for the cipher alone (only tcrypt benchmark).
>
> This benchmark had been performed on an ARM Cortex A53 CPU.
> (Note that in all of my benchmarks PAGE_SIZE=64K).
>
>> How the number looks for random access? Linear test is usually 
>> misleading.
>> I expect there will be big performance problem if you write small 
>> data chunks,
>> writes and encryption will be amplified to full big sectors here...)
> I understand your concern.
> However my patch does not force anyone to use large sectors - it only 
> opens up this
> possibility for those interested in that option.
> This is similarly to the option to format an ext4 filesystem with 64K 
> sectors.
> By the way: when you do that, you get a warning saying that the 
> filesystem
> will not be usable on most systems.
>
> Sometime users need to store mostly large files on a filesystem, for 
> example for
> backup or for video files.
> I think that in these cases random access time is not so important.
> Some users may also be able to reserve a dedicated partition for 
> storing such
> large files.
>
>>
>> (Technical detail: such pat MUST increase dm-crypt minor version.)
> Thanks for pointing that out. I believe that I can prepare a v2 patch 
> that will
> address that issue.
>>
>> Milan
>>
>>>
>>> This patch changes maximum sector size from 4096 bytes to PAGE_SIZE,
>>> and in addition it changes the type of sector_size in
>>> struct crypt_config from 'unsigned short int' to 'unsigned int', in
>>> order to be able to represent larger values.
>>>
>>> On a prototype system which has PAGE_SIZE of 65536 bytes, I saw about
>>> x2 performance improvement in sequential read throughput benchmark
>>> while using only about half of the CPU usage, after simply increasing
>>> sector size from 4096 to 65536 bytes.
>>> I used ext4 filesystem for that benchmark, which supports 64KiB
>>> sectors.
>>>
>>> Note: A small change should be made in cryptsetup in order to add
>>> support for sectors larger than 4096 bytes.
>>>
>>> Signed-off-by: Itai Handler <itai.handler at gmail.com>
>>> ---
>>>   drivers/md/dm-crypt.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
>>> index 916b7da16de2..78c239443bd5 100644
>>> --- a/drivers/md/dm-crypt.c
>>> +++ b/drivers/md/dm-crypt.c
>>> @@ -168,7 +168,7 @@ struct crypt_config {
>>>          } iv_gen_private;
>>>          u64 iv_offset;
>>>          unsigned int iv_size;
>>> -       unsigned short int sector_size;
>>> +       unsigned int sector_size;
>>>          unsigned char sector_shift;
>>>
>>>          union {
>>> @@ -3115,9 +3115,9 @@ static int crypt_ctr_optional(struct dm_target
>>> *ti, unsigned int argc, char **ar
>>>                          cc->cipher_auth = kstrdup(sval, GFP_KERNEL);
>>>                          if (!cc->cipher_auth)
>>>                                  return -ENOMEM;
>>> -               } else if (sscanf(opt_string, "sector_size:%hu%c",
>>> &cc->sector_size, &dummy) == 1) {
>>> +               } else if (sscanf(opt_string, "sector_size:%u%c",
>>> &cc->sector_size, &dummy) == 1) {
>>>                          if (cc->sector_size < (1 << SECTOR_SHIFT) ||
>>> -                           cc->sector_size > 4096 ||
>>> +                           cc->sector_size > PAGE_SIZE ||
>>>                              (cc->sector_size & (cc->sector_size - 
>>> 1))) {
>>>                                  ti->error = "Invalid feature value for
>>> sector_size";
>>>                                  return -EINVAL;
>>>
>>
> I appreciate your valuable comments.
>
> Itai
>
Milan, can you comment on the above?

Itai