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

[Mike Snitzer <snitzer@redhat.com>]

On Thu, Nov 11 2021 at  8:07P -0500,
Milan Broz <gmazyland@gmail.com> 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.
> 
> 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 patch MUST increase dm-crypt minor version.)
> 
> Milan

Thanks for your insight Milan, much appreciated.

> > 
> > 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@gmail.com>

Your patch is way too raw.  It doesn't come with any understanding of
all the nuanced reasons for 4096 that Milan raised above, and so I've
dropped your patch from further consideration.

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel