Re: [Qemu-devel] [PATCH v2 3/3] COW: Skip setting already set bits

[Paolo Bonzini]

Il 06/11/2013 13:56, Charlie Shepherd ha scritto:
> Rather than unnecessarily setting bits that are already set, re-use 
> cow_find_streak to find how
> many bits are already set for this sector, and only set unset bits. Do this 
> before the flush to
> avoid it if no bits need to be set at all.
> 
> Signed-off-by: Charlie Shepherd <address@hidden>
> ---
>  block/cow.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/block/cow.c b/block/cow.c
> index 41097d8..93207eb 100644
> --- a/block/cow.c
> +++ b/block/cow.c
> @@ -203,7 +203,7 @@ static int cow_update_bitmap(BlockDriverState *bs, 
> int64_t sector_num,
>      bool first = true;
>  
>      while (nb_sectors) {

You still need to make this a "for" and put "offset += BDRV_SECTOR_SIZE"
in the third clause.

> -        int ret;
> +        int ret, set;
>          uint8_t bitmap[BDRV_SECTOR_SIZE];
>  
>          bitnum &= BITS_PER_BITMAP_SECTOR - 1;
> @@ -214,6 +214,15 @@ static int cow_update_bitmap(BlockDriverState *bs, 
> int64_t sector_num,
>              return ret;
>          }
>  
> +        /* Skip over any already set bits */
> +        set = cow_find_streak(bitmap, 1, bitnum, sector_bits);
> +        bitnum += set;
> +        sector_bits -= set;
> +        nb_sectors -= set;
> +        if (set == sector_bits) {
> +            continue;
> +        }

This now has to be "if (set == 0)".  With the change to the "for" above,
that's correct.

>          if (first) {
>              ret = bdrv_flush(bs->file);
>              if (ret < 0) {
> @@ -228,7 +237,6 @@ static int cow_update_bitmap(BlockDriverState *bs, 
> int64_t sector_num,
>          if (ret < 0) {
>              return ret;
>          }
> -
>          bitnum += sector_bits;
>          nb_sectors -= sector_bits;
>          offset += BDRV_SECTOR_SIZE;
> 

I also noticed that patch 1 introduces a small regression, in that it
will always flush data even if the metadata doesn't change.  This patch
fixes it, because the "bdrv_flush" is preceded by the check to skip over
any already set bits.

So I suggest that, together with the above fixes, you squash patches 1
and 3 together.

Paolo