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

[Charlie Shepherd]

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

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