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

[Mikulas Patocka <mpatocka@redhat.com>]

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