Reply [PATCH] dm-verity: unnecessary data blocks that need not read hash blocks

Reply [PATCH] dm-verity: unnecessary data blocks that need not read hash blocks

[xianrong.zhou(周先荣)]

hey Eric:

On Wed, Dec 11, 2019 at 11:32:40AM +0800, zhou xianrong wrote:
> From: "xianrong.zhou" <xianrong.zhou@transsion.com>
> 
> If check_at_most_once enabled, just like verity work the prefetching 
> work should check for data block bitmap firstly before reading hash 
> block as well. Skip bit-set data blocks from both ends of data block 
> range by testing the validated bitmap. This can reduce the amounts of 
> data blocks which need to read hash blocks.
> 
> Launching 91 apps every 15s and repeat 21 rounds on Android Q.
> In prefetching work we can let only 2602/360312 = 0.72% data blocks 
> really need to read hash blocks.
> 
> But the reduced data blocks range would be enlarged again by 
> dm_verity_prefetch_cluster later.
> 
> Signed-off-by: xianrong.zhou <xianrong.zhou@transsion.com>
> Signed-off-by: yuanjiong.gao <yuanjiong.gao@transsion.com>
> Tested-by: ruxian.feng <ruxian.feng@transsion.com>
> ---
>  drivers/md/dm-verity-target.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/md/dm-verity-target.c 
> b/drivers/md/dm-verity-target.c index 4fb33e7562c5..7b8eb754c0b6 
> 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -581,6 +581,22 @@ static void verity_prefetch_io(struct work_struct *work)
>  	struct dm_verity *v = pw->v;
>  	int i;
>  
> +	if (v->validated_blocks) {
> +		while (pw->n_blocks) {
> +			if (unlikely(!test_bit(pw->block, v->validated_blocks)))
> +				break;
> +			pw->block++;
> +			pw->n_blocks--;
> +		}
> +		while (pw->n_blocks) {
> +			if (unlikely(!test_bit(pw->block + pw->n_blocks - 1,
> +				v->validated_blocks)))
> +				break;
> +			pw->n_blocks--;
> +		}
> +		if (!pw->n_blocks)
> +			return;
> +	}

This is a good idea, but shouldn't this logic go in verity_submit_prefetch() prior to the struct dm_verity_prefetch_work being allocated?  Then if no prefeching is needed, allocating and scheduling the work object can be skipped.

Eric, Do you mean it is more suitable in dm_bufio_prefetch which is called on different paths even though prefeching is disabled ?

Also note that you're currently leaking the work object with the early return.	

Right! I leaked this. always so. Thanks!!!

- Eric

Re: [PATCH] dm-verity: unnecessary data blocks that need not read hash blocks

[Eric Biggers]

On Mon, Dec 16, 2019 at 02:02:33AM +0000, xianrong.zhou(周先荣) wrote:
> hey Eric:
> 
> On Wed, Dec 11, 2019 at 11:32:40AM +0800, zhou xianrong wrote:
> > From: "xianrong.zhou" <xianrong.zhou@transsion.com>
> > 
> > If check_at_most_once enabled, just like verity work the prefetching 
> > work should check for data block bitmap firstly before reading hash 
> > block as well. Skip bit-set data blocks from both ends of data block 
> > range by testing the validated bitmap. This can reduce the amounts of 
> > data blocks which need to read hash blocks.
> > 
> > Launching 91 apps every 15s and repeat 21 rounds on Android Q.
> > In prefetching work we can let only 2602/360312 = 0.72% data blocks 
> > really need to read hash blocks.
> > 
> > But the reduced data blocks range would be enlarged again by 
> > dm_verity_prefetch_cluster later.
> > 
> > Signed-off-by: xianrong.zhou <xianrong.zhou@transsion.com>
> > Signed-off-by: yuanjiong.gao <yuanjiong.gao@transsion.com>
> > Tested-by: ruxian.feng <ruxian.feng@transsion.com>
> > ---
> >  drivers/md/dm-verity-target.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/drivers/md/dm-verity-target.c 
> > b/drivers/md/dm-verity-target.c index 4fb33e7562c5..7b8eb754c0b6 
> > 100644
> > --- a/drivers/md/dm-verity-target.c
> > +++ b/drivers/md/dm-verity-target.c
> > @@ -581,6 +581,22 @@ static void verity_prefetch_io(struct work_struct *work)
> >  	struct dm_verity *v = pw->v;
> >  	int i;
> >  
> > +	if (v->validated_blocks) {
> > +		while (pw->n_blocks) {
> > +			if (unlikely(!test_bit(pw->block, v->validated_blocks)))
> > +				break;
> > +			pw->block++;
> > +			pw->n_blocks--;
> > +		}
> > +		while (pw->n_blocks) {
> > +			if (unlikely(!test_bit(pw->block + pw->n_blocks - 1,
> > +				v->validated_blocks)))
> > +				break;
> > +			pw->n_blocks--;
> > +		}
> > +		if (!pw->n_blocks)
> > +			return;
> > +	}
> 
> This is a good idea, but shouldn't this logic go in verity_submit_prefetch()
> prior to the struct dm_verity_prefetch_work being allocated?  Then if no
> prefeching is needed, allocating and scheduling the work object can be
> skipped.
> 
> Eric, Do you mean it is more suitable in dm_bufio_prefetch which is called on
> different paths even though prefeching is disabled ?
> 

No, I'm talking about verity_submit_prefetch().  verity_submit_prefetch()
allocates and schedules a work object, which executes verity_prefetch_io().
If all data blocks in the I/O request were already validated, there's no need to
allocate and schedule the prefetch work.

- Eric

Re: [dm-devel] [PATCH] dm-verity: unnecessary data blocks that need not read hash blocks

[Eric Biggers]

On Mon, Dec 16, 2019 at 10:50:26AM -0800, Eric Biggers wrote:
> On Mon, Dec 16, 2019 at 02:02:33AM +0000, xianrong.zhou(周先荣) wrote:
> > hey Eric:
> > 
> > On Wed, Dec 11, 2019 at 11:32:40AM +0800, zhou xianrong wrote:
> > > From: "xianrong.zhou" <xianrong.zhou@transsion.com>
> > > 
> > > If check_at_most_once enabled, just like verity work the prefetching 
> > > work should check for data block bitmap firstly before reading hash 
> > > block as well. Skip bit-set data blocks from both ends of data block 
> > > range by testing the validated bitmap. This can reduce the amounts of 
> > > data blocks which need to read hash blocks.
> > > 
> > > Launching 91 apps every 15s and repeat 21 rounds on Android Q.
> > > In prefetching work we can let only 2602/360312 = 0.72% data blocks 
> > > really need to read hash blocks.
> > > 
> > > But the reduced data blocks range would be enlarged again by 
> > > dm_verity_prefetch_cluster later.
> > > 
> > > Signed-off-by: xianrong.zhou <xianrong.zhou@transsion.com>
> > > Signed-off-by: yuanjiong.gao <yuanjiong.gao@transsion.com>
> > > Tested-by: ruxian.feng <ruxian.feng@transsion.com>
> > > ---
> > >  drivers/md/dm-verity-target.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > > 
> > > diff --git a/drivers/md/dm-verity-target.c 
> > > b/drivers/md/dm-verity-target.c index 4fb33e7562c5..7b8eb754c0b6 
> > > 100644
> > > --- a/drivers/md/dm-verity-target.c
> > > +++ b/drivers/md/dm-verity-target.c
> > > @@ -581,6 +581,22 @@ static void verity_prefetch_io(struct work_struct *work)
> > >  	struct dm_verity *v = pw->v;
> > >  	int i;
> > >  
> > > +	if (v->validated_blocks) {
> > > +		while (pw->n_blocks) {
> > > +			if (unlikely(!test_bit(pw->block, v->validated_blocks)))
> > > +				break;
> > > +			pw->block++;
> > > +			pw->n_blocks--;
> > > +		}
> > > +		while (pw->n_blocks) {
> > > +			if (unlikely(!test_bit(pw->block + pw->n_blocks - 1,
> > > +				v->validated_blocks)))
> > > +				break;
> > > +			pw->n_blocks--;
> > > +		}
> > > +		if (!pw->n_blocks)
> > > +			return;
> > > +	}
> > 
> > This is a good idea, but shouldn't this logic go in verity_submit_prefetch()
> > prior to the struct dm_verity_prefetch_work being allocated?  Then if no
> > prefeching is needed, allocating and scheduling the work object can be
> > skipped.
> > 
> > Eric, Do you mean it is more suitable in dm_bufio_prefetch which is called on
> > different paths even though prefeching is disabled ?
> > 
> 
> No, I'm talking about verity_submit_prefetch().  verity_submit_prefetch()
> allocates and schedules a work object, which executes verity_prefetch_io().
> If all data blocks in the I/O request were already validated, there's no need to
> allocate and schedule the prefetch work.
> 
> - Eric
> 

Are you planning to send a new version of this patch?

- Eric

Re: [PATCH] dm-verity:unnecessary data blocks that need not read hash blocks

[Eric Biggers]

On Tue, Jan 07, 2020 at 10:48:43AM +0800, zhou_xianrong wrote:

> Subject: [PATCH] dm-verity:unnecessary data blocks that need not read

Please use a proper commit subject.  It should begin with "dm verity: " and use
the imperative tense to describe the change, e.g.

	dm verity: don't prefetch hash blocks for already-verified data

Also this should have been "PATCH v2", not simply PATCH, since you already sent
out a previous version.

You also sent out multiple copies of this email for some reason, so I just chose
one arbitrarily to reply to...

> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 4fb33e7..4127711 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -611,8 +611,27 @@ static void verity_prefetch_io(struct work_struct *work)
>  
>  static void verity_submit_prefetch(struct dm_verity *v, struct dm_verity_io *io)
>  {
> +	sector_t block = io->block;
> +	unsigned int n_blocks = io->n_blocks;
>  	struct dm_verity_prefetch_work *pw;
>  
> +	if (v->validated_blocks) {
> +		while (n_blocks) {
> +			if (unlikely(!test_bit(block, v->validated_blocks)))
> +				break;
> +			block++;
> +			n_blocks--;
> +		}
> +		while (n_blocks) {
> +			if (unlikely(!test_bit(block + n_blocks - 1,
> +				v->validated_blocks)))
> +				break;
> +			n_blocks--;
> +		}
> +		if (!n_blocks)
> +			return;
> +	}

This looks fine now, though it's a bit more verbose than necessary, and I don't
think unlikely() will make any difference here.  Consider simplifying it to:

	if (v->validated_blocks) {
		while (n_blocks && test_bit(block, v->validated_blocks)) {
			block++;
			n_blocks--;
		}
		while (n_blocks && test_bit(block + n_blocks - 1,
					    v->validated_blocks))
			n_blocks--;
		if (!n_blocks)
			return;
	}

Re: [PATCH] dm-verity: unnecessary data blocks that need not read hash blocks

[Eric Biggers]

On Wed, Dec 11, 2019 at 11:32:40AM +0800, zhou xianrong wrote:
> From: "xianrong.zhou" <xianrong.zhou@transsion.com>
> 
> If check_at_most_once enabled, just like verity work the prefetching work
> should check for data block bitmap firstly before reading hash block
> as well. Skip bit-set data blocks from both ends of data block range
> by testing the validated bitmap. This can reduce the amounts of data 
> blocks which need to read hash blocks.
> 
> Launching 91 apps every 15s and repeat 21 rounds on Android Q.
> In prefetching work we can let only 2602/360312 = 0.72% data blocks
> really need to read hash blocks.
> 
> But the reduced data blocks range would be enlarged again by
> dm_verity_prefetch_cluster later.
> 
> Signed-off-by: xianrong.zhou <xianrong.zhou@transsion.com>
> Signed-off-by: yuanjiong.gao <yuanjiong.gao@transsion.com>
> Tested-by: ruxian.feng <ruxian.feng@transsion.com>
> ---
>  drivers/md/dm-verity-target.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 4fb33e7562c5..7b8eb754c0b6 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -581,6 +581,22 @@ static void verity_prefetch_io(struct work_struct *work)
>  	struct dm_verity *v = pw->v;
>  	int i;
>  
> +	if (v->validated_blocks) {
> +		while (pw->n_blocks) {
> +			if (unlikely(!test_bit(pw->block, v->validated_blocks)))
> +				break;
> +			pw->block++;
> +			pw->n_blocks--;
> +		}
> +		while (pw->n_blocks) {
> +			if (unlikely(!test_bit(pw->block + pw->n_blocks - 1,
> +				v->validated_blocks)))
> +				break;
> +			pw->n_blocks--;
> +		}
> +		if (!pw->n_blocks)
> +			return;
> +	}

This is a good idea, but shouldn't this logic go in verity_submit_prefetch()
prior to the struct dm_verity_prefetch_work being allocated?  Then if no
prefeching is needed, allocating and scheduling the work object can be skipped.

Also note that you're currently leaking the work object with the early return.

- Eric