[PATCH 1/2] dm-bufio: improve the performance of __dm_bufio_prefetch

[PATCH 1/2] dm-bufio: improve the performance of __dm_bufio_prefetch

[LongPing Wei]

1. call blk_flush_plug when cache hit as the address of the subsequent
 bio is no longer contiguous with the previous bio;
2. skip cond_resched when ioprio class is rt;

Signed-off-by: LongPing Wei <weilongping@oppo.com>
---
 drivers/md/dm-bufio.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 9c8ed65cd87e..ec8392fbcf5d 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -1981,6 +1981,7 @@ static void __dm_bufio_prefetch(struct dm_bufio_client *c,
 			unsigned short ioprio)
 {
 	struct blk_plug plug;
+	unsigned short ioprio_class = IOPRIO_PRIO_CLASS(ioprio);
 
 	LIST_HEAD(write_list);
 
@@ -1997,6 +1998,7 @@ static void __dm_bufio_prefetch(struct dm_bufio_client *c,
 		if (b) {
 			/* already in cache */
 			cache_put_and_wake(c, b);
+			blk_flush_plug(&plug, false);
 			continue;
 		}
 
@@ -2017,7 +2019,8 @@ static void __dm_bufio_prefetch(struct dm_bufio_client *c,
 				submit_io(b, REQ_OP_READ, ioprio, read_endio);
 			dm_bufio_release(b);
 
-			cond_resched();
+			if (ioprio_class != IOPRIO_CLASS_RT)
+				cond_resched();
 
 			if (!n_blocks)
 				goto flush_plug;
-- 
2.34.1

[PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[LongPing Wei]

At this time, all bios for hash blocks should eventually
be merged into a single large bio.

Signed-off-by: LongPing Wei <weilongping@oppo.com>
---
 drivers/md/dm-verity-target.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 3c427f18a04b..813d5cfc7ffa 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -1683,6 +1683,10 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 
 	verity_verify_sig_opts_cleanup(&verify_args);
 
+	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
+		v->hash_blocks - v->hash_start,
+		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
+
 	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
 
 	return 0;
-- 
2.34.1

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[Mikulas Patocka]

On Wed, 2 Apr 2025, LongPing Wei wrote:

> At this time, all bios for hash blocks should eventually
> be merged into a single large bio.
> 
> Signed-off-by: LongPing Wei <weilongping@oppo.com>
> ---
>  drivers/md/dm-verity-target.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 3c427f18a04b..813d5cfc7ffa 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -1683,6 +1683,10 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>  
>  	verity_verify_sig_opts_cleanup(&verify_args);
>  
> +	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
> +		v->hash_blocks - v->hash_start,
> +		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
> +
>  	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
>  
>  	return 0;
> -- 
> 2.34.1

Hi

I would move it into the "resume" callback, so that if the user 
reconfigures the device stack between "ctr" and "resume", it won't read 
the data too early.

Don't use IOPRIO_CLASS_RT, this is not real-time requirement, 
IOPRIO_CLASS_RT would slow down concurrent I/O.

Another problem with this approach is that when the verity device is big 
and system memory is small, it just causes I/O churn - new bufio blocks 
will be displacing old blocks - and it will degrade performance, not 
improve it.

Please, describe some scenario, where this prefetch actually helps. What 
is the size of the metadata that you are prefetching? What is the total 
memory size? Is there any benchmark that shows the advantage of this 
patch?

Mikulas

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[LongPing Wei]

On 2025/4/2 17:56, Mikulas Patocka wrote:
> 
> 
> On Wed, 2 Apr 2025, LongPing Wei wrote:
> 
>> At this time, all bios for hash blocks should eventually
>> be merged into a single large bio.
>>
>> Signed-off-by: LongPing Wei <weilongping@oppo.com>
>> ---
>>   drivers/md/dm-verity-target.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index 3c427f18a04b..813d5cfc7ffa 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -1683,6 +1683,10 @@ static int verity_ctr(struct dm_target *ti, unsigned int argc, char **argv)
>>   
>>   	verity_verify_sig_opts_cleanup(&verify_args);
>>   
>> +	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
>> +		v->hash_blocks - v->hash_start,
>> +		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
>> +
>>   	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
>>   
>>   	return 0;
>> -- 
>> 2.34.1
> 
> Hi
> 
> I would move it into the "resume" callback, so that if the user
> reconfigures the device stack between "ctr" and "resume", it won't read
> the data too early.
> 
> Don't use IOPRIO_CLASS_RT, this is not real-time requirement,
> IOPRIO_CLASS_RT would slow down concurrent I/O.
> 
If the prefetch io is submitted with non-rt at first, the later dm io
need the same hash block will wait the non-rt bio.
> Another problem with this approach is that when the verity device is big
> and system memory is small, it just causes I/O churn - new bufio blocks
> will be displacing old blocks - and it will degrade performance, not
> improve it.
> 
Do we need a solution to check if the memory is enough to the prefetch?
For Android devices, the verity device should be created on the boot 
procedure.
> Please, describe some scenario, where this prefetch actually helps. What
> is the size of the metadata that you are prefetching? What is the total
> memory size? Is there any benchmark that shows the advantage of this
> patch?
The size of hash blocks for the ROM of our low-end devices is about
71MiB. I want to enhance probability of cache hit when
try_verify_in_tasklet is enabled. How about only doing the prefetch when
try_verify_in_tasklet is enabled?
For example:
	if (v->use_bh_wq)
		dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
			v->hash_blocks - v->hash_start,
			IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));

> 
> Mikulas
> 

LongPing

Re: [PATCH 1/2] dm-bufio: improve the performance of __dm_bufio_prefetch

[Eric Biggers]

On Wed, Apr 02, 2025 at 03:09:34PM +0800, LongPing Wei wrote:
> 1. call blk_flush_plug when cache hit as the address of the subsequent
>  bio is no longer contiguous with the previous bio;
> 2. skip cond_resched when ioprio class is rt;
> 
> Signed-off-by: LongPing Wei <weilongping@oppo.com>
> ---
>  drivers/md/dm-bufio.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Looks fine:

Reviewed-by: Eric Biggers <ebiggers@kernel.org>

But I do think the prefetching should be reworked to be along the lines of what
I suggested here:
https://lore.kernel.org/dm-devel/20250327170524.GF1425@sol.localdomain/
Any chance you're interested in helping with that?

It also *might* be the case that the prefetching is no longer helpful and should
just be removed.  Especially if dm-verity will start prefetching the whole hash
tree anyway, as your other patch does.

- Eric

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[Mikulas Patocka]

On Wed, 2 Apr 2025, LongPing Wei wrote:

> On 2025/4/2 17:56, Mikulas Patocka wrote:
> > 
> > 
> > On Wed, 2 Apr 2025, LongPing Wei wrote:
> > 
> > > At this time, all bios for hash blocks should eventually
> > > be merged into a single large bio.
> > > 
> > > Signed-off-by: LongPing Wei <weilongping@oppo.com>
> > > ---
> > >   drivers/md/dm-verity-target.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> > > index 3c427f18a04b..813d5cfc7ffa 100644
> > > --- a/drivers/md/dm-verity-target.c
> > > +++ b/drivers/md/dm-verity-target.c
> > > @@ -1683,6 +1683,10 @@ static int verity_ctr(struct dm_target *ti,
> > > unsigned int argc, char **argv)
> > >     	verity_verify_sig_opts_cleanup(&verify_args);
> > >   +	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
> > > +		v->hash_blocks - v->hash_start,
> > > +		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
> > > +
> > >   	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
> > >     	return 0;
> > > -- 
> > > 2.34.1
> > 
> > Hi
> > 
> > I would move it into the "resume" callback, so that if the user
> > reconfigures the device stack between "ctr" and "resume", it won't read
> > the data too early.
> > 
> > Don't use IOPRIO_CLASS_RT, this is not real-time requirement,
> > IOPRIO_CLASS_RT would slow down concurrent I/O.
> > 
> If the prefetch io is submitted with non-rt at first, the later dm io
> need the same hash block will wait the non-rt bio.

Submitting large I/O with IOPRIO_CLASS_RT will block every other task that 
does some I/O, so I can't do that. It needs to be changed to 
IOPRIO_CLASS_NONE or IOPRIO_CLASS_IDLE.

> > Another problem with this approach is that when the verity device is big
> > and system memory is small, it just causes I/O churn - new bufio blocks
> > will be displacing old blocks - and it will degrade performance, not
> > improve it.
> > 
> Do we need a solution to check if the memory is enough to the prefetch?

Yes.

> For Android devices, the verity device should be created on the boot
> procedure.

> > Please, describe some scenario, where this prefetch actually helps. What
> > is the size of the metadata that you are prefetching? What is the total
> > memory size? Is there any benchmark that shows the advantage of this
> > patch?
> 
> The size of hash blocks for the ROM of our low-end devices is about
> 71MiB. I want to enhance probability of cache hit when
> try_verify_in_tasklet is enabled. How about only doing the prefetch when
> try_verify_in_tasklet is enabled?
> For example:
> 	if (v->use_bh_wq)
> 		dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
> 			v->hash_blocks - v->hash_start,
> 			IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));

I wouldn't overload the "use_bh_wq" option for that. Perhaps we could add 
a new option (that would be off by default), so that the patch won't cause 
problems to existing users.

How much does this patch improve Android boot time? So that we can decide 
whether the improvement is worth the complexity.

Mikulas

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[LongPing Wei]

On 2025/4/3 2:44, Mikulas Patocka wrote:
> 
> 
> On Wed, 2 Apr 2025, LongPing Wei wrote:
> 
>> On 2025/4/2 17:56, Mikulas Patocka wrote:
>>>
>>>
>>> On Wed, 2 Apr 2025, LongPing Wei wrote:
>>>
>>>> At this time, all bios for hash blocks should eventually
>>>> be merged into a single large bio.
>>>>
>>>> Signed-off-by: LongPing Wei <weilongping@oppo.com>
>>>> ---
>>>>    drivers/md/dm-verity-target.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>>>> index 3c427f18a04b..813d5cfc7ffa 100644
>>>> --- a/drivers/md/dm-verity-target.c
>>>> +++ b/drivers/md/dm-verity-target.c
>>>> @@ -1683,6 +1683,10 @@ static int verity_ctr(struct dm_target *ti,
>>>> unsigned int argc, char **argv)
>>>>      	verity_verify_sig_opts_cleanup(&verify_args);
>>>>    +	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
>>>> +		v->hash_blocks - v->hash_start,
>>>> +		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
>>>> +
>>>>    	dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1);
>>>>      	return 0;
>>>> -- 
>>>> 2.34.1
>>>
>>> Hi
>>>
>>> I would move it into the "resume" callback, so that if the user
>>> reconfigures the device stack between "ctr" and "resume", it won't read
>>> the data too early.
>>>
>>> Don't use IOPRIO_CLASS_RT, this is not real-time requirement,
>>> IOPRIO_CLASS_RT would slow down concurrent I/O.
>>>
>> If the prefetch io is submitted with non-rt at first, the later dm io
>> need the same hash block will wait the non-rt bio.
> 
> Submitting large I/O with IOPRIO_CLASS_RT will block every other task that
> does some I/O, so I can't do that. It needs to be changed to
> IOPRIO_CLASS_NONE or IOPRIO_CLASS_IDLE.
> 
>>> Another problem with this approach is that when the verity device is big
>>> and system memory is small, it just causes I/O churn - new bufio blocks
>>> will be displacing old blocks - and it will degrade performance, not
>>> improve it.
>>>
>> Do we need a solution to check if the memory is enough to the prefetch?
> 
> Yes.
> 
>> For Android devices, the verity device should be created on the boot
>> procedure.
> 
>>> Please, describe some scenario, where this prefetch actually helps. What
>>> is the size of the metadata that you are prefetching? What is the total
>>> memory size? Is there any benchmark that shows the advantage of this
>>> patch?
>>
>> The size of hash blocks for the ROM of our low-end devices is about
>> 71MiB. I want to enhance probability of cache hit when
>> try_verify_in_tasklet is enabled. How about only doing the prefetch when
>> try_verify_in_tasklet is enabled?
>> For example:
>> 	if (v->use_bh_wq)
>> 		dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
>> 			v->hash_blocks - v->hash_start,
>> 			IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));
> 
> I wouldn't overload the "use_bh_wq" option for that. Perhaps we could add
> a new option (that would be off by default), so that the patch won't cause
> problems to existing users.
> 
> How much does this patch improve Android boot time? So that we can decide
> whether the improvement is worth the complexity.
> 
> Mikulas
> 

Hi, Mikulas

Can we add a parameter that is used only once in the process of ctr or
resume?

For normal boot, the improvement is not significant. But for upgrading
boot, it could be valuable to do this as dm-verity works on dm-user and
snapshotd in userspace has a large number of writes in the background.
We have met an issue of system running slowly in this situation.

If a parameter that is used only once in the process of ctr or
resume is acceptable, I want to use it only in upgrading boot mode and
the shrinker of dm-bufio should be skipped until the system is upgraded.

The goal is to reduce the number of read requests to snapuserd.

LongPing

Re: [PATCH 1/2] dm-bufio: improve the performance of __dm_bufio_prefetch

[LongPing Wei]

On 2025/4/3 1:43, Eric Biggers wrote:
> On Wed, Apr 02, 2025 at 03:09:34PM +0800, LongPing Wei wrote:
>> 1. call blk_flush_plug when cache hit as the address of the subsequent
>>   bio is no longer contiguous with the previous bio;
>> 2. skip cond_resched when ioprio class is rt;
>>
>> Signed-off-by: LongPing Wei <weilongping@oppo.com>
>> ---
>>   drivers/md/dm-bufio.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Looks fine:
> 
> Reviewed-by: Eric Biggers <ebiggers@kernel.org>
> 
> But I do think the prefetching should be reworked to be along the lines of what
> I suggested here:
> https://lore.kernel.org/dm-devel/20250327170524.GF1425@sol.localdomain/
> Any chance you're interested in helping with that?
> 

How about calling verity_prefetch_io directly in verity_map only when
use_bh_wq return true for this dm io? The size of dm io may be a greate
number as dm-verity doesn't set a max io length. Mikulas once mentioned
that there was a problem that caused him to give up doing so. Has that
issue been fixed?

> It also *might* be the case that the prefetching is no longer helpful and should
> just be removed.  Especially if dm-verity will start prefetching the whole hash
> tree anyway, as your other patch does.

It seems that we cannot just removed the prefetching in verity_map as
the cahche in dm-bufio may be shrinked.

> 
> - Eric

LongPing

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[Christoph Hellwig]

On Wed, Apr 02, 2025 at 03:09:36PM +0800, LongPing Wei wrote:
>  
> +	dm_bufio_prefetch_with_ioprio(v->bufio, v->hash_start,
> +		v->hash_blocks - v->hash_start,
> +		IOPRIO_PRIO_VALUE(IOPRIO_CLASS_RT, 0));

Err, no.  Random code should not prefetch with a RT priority.

Re: [PATCH 2/2] dm-verity: prefetch all hash blocks in verity_ctr

[Christoph Hellwig]

On Wed, Apr 02, 2025 at 08:44:39PM +0200, Mikulas Patocka wrote:
> Submitting large I/O with IOPRIO_CLASS_RT will block every other task that 
> does some I/O, so I can't do that. It needs to be changed to 
> IOPRIO_CLASS_NONE or IOPRIO_CLASS_IDLE.

Also random kernel code should not simply upgrade the I/O priority.
Your magic snowflake is not going to be as magic for everyone else.

Re: [PATCH 1/2] dm-bufio: improve the performance of __dm_bufio_prefetch

[Christoph Hellwig]

This commit message is extremtly sparse.  Not helped by the fact that
the series also misses a cover letter.

On Wed, Apr 02, 2025 at 03:09:34PM +0800, LongPing Wei wrote:
> 1. call blk_flush_plug when cache hit as the address of the subsequent
>  bio is no longer contiguous with the previous bio;

As you found out blk_flush_plug is not exported, and that for a good
reason.  This also completely fails to explain why you want to do
this and what the measured benefit is.

> 2. skip cond_resched when ioprio class is rt;

Same here.  Also these look compeltely unrelated and I have no idea
why they are in the same patch.