Re: [PATCH v2 1/3] dm-inlinecrypt: Add inline encryption support

Mikulas Patocka
2024-10-15T10:59:31Z

Hi

The patch seems OK. Should it go in via the device mapper tree or the
block layer tree?

On Mon, 16 Sep 2024, Md Sadre Alam wrote:

> +#define DM_CRYPT_DEFAULT_MAX_READ_SIZE 131072
> +#define DM_CRYPT_DEFAULT_MAX_WRITE_SIZE 131072
> +
> +static unsigned int get_max_request_size(struct inlinecrypt_config *cc, bool wrt)
> +{
> + unsigned int val, sector_align;
> +
> + val = !wrt ? DM_CRYPT_DEFAULT_MAX_READ_SIZE : DM_CRYPT_DEFAULT_MAX_WRITE_SIZE;
> + if (wrt) {
> + if (unlikely(val > BIO_MAX_VECS << PAGE_SHIFT))
> + val = BIO_MAX_VECS << PAGE_SHIFT;
> + }
> + sector_align = max(bdev_logical_block_size(cc->dev->bdev), (unsigned int)cc->sector_size);
> + val = round_down(val, sector_align);
> + if (unlikely(!val))
> + val = sector_align;
> + return val >> SECTOR_SHIFT;
> +}

This piece of code was copied from the dm-crypt target. For dm-crypt, I
was actually benchmarking the performance for various
DM_CRYPT_DEFAULT_MAX_READ_SIZE and DM_CRYPT_DEFAULT_MAX_WRITE_SIZE values
and I selected the values that resulted in the best performance.

You should benchmark it too to find the optimal I/O size. Perhaps you find
out that there is no need to split big requests and this piece of code can
be dropped.

> + /* Omit the key for now. */
> + DMEMIT("%s - %llu %s %llu", ctx->cipher_string, ctx->iv_offset,
> + ctx->dev->name, (unsigned long long)ctx->start);

What if someone reloads the table? I think you should display the key.
dmsetup does not display the key if the "--showkeys" parameter is not
specified.

Mikulas