On Wed, Mar 26, 2025 at 07:50:34PM +0100, Mikulas Patocka wrote: > > > 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 <weilongp...@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?
IOPRIO_CLASS_NONE means an I/O priority isn't set, so it probably should be treated the same as IOPRIO_CLASS_BE which is the "normal" priority. For now I think it makes sense to use 4 or 8 KiB even for IOPRIO_CLASS_RT. That covers the single-block request case which is the one that matters most. We can bump it up later if we can be shown that something higher is better. Softirqs normally get prioritized over all real-time tasks; so while it should be more acceptable to use them for IOPRIO_CLASS_RT, it's not a perfect solution. Another concern I have with longer requests is that when dm-verity runs the verification work in a softirq and finds that a Merkle tree block isn't cached, it punts the request to a kworker and restarts it from the beginning. That not only defeats the point of using softirq context, but it also throws away work that was done. I think we could fix it to not throw away work, but having to fall back at all would still defeat the point of using softirq context. - Eric
