Hi

I think the patch is OK, I just suggest these changes:

- set the limit for the number of bytes, rather than the number of blocks, 
because different dm-verity devices can have different block size

- run some benchmarks and set the default values according to the 
benchmark results

- increase the target version number

- add a note about this feature to 
Documentation/admin-guide/device-mapper/verity.rst

Mikulas


On Tue, 25 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_blocks.
> 
> The default limits for NONE/RT/BE/IDLE is set to UINT_MAX to keep the
> original behaviour.
> 
> Signed-off-by: LongPing Wei <weilongp...@oppo.com>
> ---
>  drivers/md/dm-verity-target.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index e86c1431b108..1e0d0920d6a9 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -30,6 +30,7 @@
>  #define DM_VERITY_ENV_VAR_NAME               "DM_VERITY_ERR_BLOCK_NR"
>  
>  #define DM_VERITY_DEFAULT_PREFETCH_SIZE      262144
> +#define DM_VERITY_USE_BH_DEFAULT_BLOCKS      UINT_MAX
>  
>  #define DM_VERITY_MAX_CORRUPTED_ERRS 100
>  
> @@ -49,6 +50,15 @@ static unsigned int dm_verity_prefetch_cluster = 
> DM_VERITY_DEFAULT_PREFETCH_SIZE
>  
>  module_param_named(prefetch_cluster, dm_verity_prefetch_cluster, uint, 0644);
>  
> +static unsigned int dm_verity_use_bh_blocks[4] = {
> +     DM_VERITY_USE_BH_DEFAULT_BLOCKS,        // IOPRIO_CLASS_NONE
> +     DM_VERITY_USE_BH_DEFAULT_BLOCKS,        // IOPRIO_CLASS_RT
> +     DM_VERITY_USE_BH_DEFAULT_BLOCKS,        // IOPRIO_CLASS_BE
> +     DM_VERITY_USE_BH_DEFAULT_BLOCKS         // IOPRIO_CLASS_IDLE
> +};
> +
> +module_param_array_named(use_bh_blocks, dm_verity_use_bh_blocks, uint, NULL, 
> 0644);
> +
>  static DEFINE_STATIC_KEY_FALSE(use_bh_wq_enabled);
>  
>  /* Is at least one dm-verity instance using ahash_tfm instead of shash_tfm? 
> */
> @@ -652,6 +662,12 @@ static void verity_bh_work(struct work_struct *w)
>       verity_finish_io(io, errno_to_blk_status(err));
>  }
>  
> +static inline bool verity_use_bh(unsigned int n_blocks, unsigned short 
> ioprio)
> +{
> +     return ioprio <= IOPRIO_CLASS_IDLE &&
> +             n_blocks <= READ_ONCE(dm_verity_use_bh_blocks[ioprio]);
> +}
> +
>  static void verity_end_io(struct bio *bio)
>  {
>       struct dm_verity_io *io = bio->bi_private;
> @@ -664,7 +680,8 @@ static void verity_end_io(struct bio *bio)
>               return;
>       }
>  
> -     if (static_branch_unlikely(&use_bh_wq_enabled) && io->v->use_bh_wq) {
> +     if (static_branch_unlikely(&use_bh_wq_enabled) && io->v->use_bh_wq &&
> +             verity_use_bh(io->n_blocks, IOPRIO_PRIO_CLASS(bio->bi_ioprio))) 
> {
>               INIT_WORK(&io->bh_work, verity_bh_work);
>               queue_work(system_bh_wq, &io->bh_work);
>       } else {
> -- 
> 2.34.1
> 


Reply via email to