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

[Charlie Shepherd <charlie@ctshepherd.com>]

On 06/11/2013 13:03, Paolo Bonzini wrote:
> 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 <charlie@ctshepherd.com>
>> ---
>>   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.

Ah true, I thought I could get away without doing that because it looked 
a bit ugly but I'll update it to use a for.

>> -        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.

Ah that's true, I'll do that.

Charlie