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

[Milan Broz]

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.

It also depends on crypto API driver here (performance is usually optimized to 4k).
What cipher and encryption mode did you use for test?

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...)

(Technical detail: such pat MUST increase dm-crypt minor version.)

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