Re: [PATCH v2] dm-verity: support block number limits for different ioprio classes

[Mikulas Patocka <mpatocka@redhat.com>]

On Wed, 26 Mar 2025, Eric Biggers wrote:

> On Wed, Mar 26, 2025 at 09:32:27PM +0800, LongPing Wei wrote:
> > On 2025/3/26 19:04, Mikulas Patocka wrote:
> > > 
> > > 
> > > On Wed, 26 Mar 2025, LongPing Wei wrote:
> > > 
> > > > Calling verity_verify_io in bh for IO of all sizes is not suitable for
> > > > embedded devices. From our tests, it can improve the performance of 4K
> > > > synchronise random reads.
> > > > For example:
> > > > ./fio --name=rand_read --ioengine=psync --rw=randread --bs=4K \
> > > >   --direct=1 --numjobs=8 --runtime=60 --time_based --group_reporting \
> > > >   --filename=/dev/block/mapper/xx-verity
> > > > 
> > > > But it will degrade the performance of 512K synchronise sequential reads
> > > > on our devices.
> > > > For example:
> > > > ./fio --name=read --ioengine=psync --rw=read --bs=512K --direct=1 \
> > > >   --numjobs=8 --runtime=60 --time_based --group_reporting \
> > > >   --filename=/dev/block/mapper/xx-verity
> > > > 
> > > > A parameter array is introduced by this change. And users can modify the
> > > > default config by /sys/module/dm_verity/parameters/use_bh_bytes.
> > > > 
> > > > The default limits for NONE/RT/BE/IDLE is set to 4096.
> > > > 
> > > > Call verity_verify_io directly when verity_end_io is in softirq.
> > > > 
> > > > Signed-off-by: LongPing Wei <weilongping@oppo.com>
> > > 
> > > Are you sure that 4096 bytes is the correct threshold?
> > > 
> > > I suggest that you run the benchmarks for 4k, 8k, 16k, 32k, 64k, 128k,
> > > 256k, 512k and set the default threshold to the largest value where bh
> > > code performs better than non-bh code.
> > > 
> > > Mikulas
> > > 
> 
> 4096 bytes sounds good to me.  As I mentioned elsewhere in the thread
> (https://lore.kernel.org/dm-devel/20250326164057.GA1243@sol.localdomain/),
> objections keep getting raised to doing more work in softirq context due to
> potential impact on real-time tasks.  So while more I/O benchmarks would be
> interesting, they are also not the whole story and we should probably prefer a
> more conservative threshold.  Also, dm-verity I/O requests have a bimodal
> distribution with peaks at 4 KiB (for synchronous reads) and 128 KiB or
> read_ahead_kb (for readahead) anyway, so most of the impact should come from 4
> KiB anyway.
> 
> 8 KiB could also be reasonable when taking into account multibuffer hashing
> though, since multibuffer hashing makes hashing two 4 KiB blocks almost as fast
> as hashing a single 4 KiB block.  (Though multibuffer hashing is currently
> Android-only, as the crypto maintainer keeps rejecting it upstream.)
> 
> - Eric

OK, so I'll set it to 8KiB.

Do you think that I should increase this limit to "unlimited" for 
IOPRIO_CLASS_RT and disable this feature at all for IOPRIO_CLASS_NONE?

Mikulas