Re: [Qemu-devel] [PATCH 1/2] Make cow_co_is_allocated and cow_update_bitmap more efficient

[Charlie Shepherd]

On 21/08/2013 10:19, Paolo Bonzini wrote:
> Il 21/08/2013 11:11, Charlie Shepherd ha scritto:
> > It still seems
> > worthwhile to me to be as efficient as possible, I guess that means
> > processing a sector's worth of metadata at a time?
> Yes, that's what my patches do.  My is_allocated and flushing strategy +
> something like your replacement of cow_set_bit (just without the
> unbounded allocation) should be pretty good.
>
> Perhaps you can use a cow_co_is_allocated loop after writing the data.
> If it returns 0, you flush (the first time only) and call your
> cow_update_bitmap.  Then you advance by num_same sectors and go on until
> you did all the nb_sectors.  The disadvantage is that it does two reads
> (one in cow_co_is_allocated, one in cow_update_bitmap).  The advantage
> is that unbounded allocation goes away because cow_co_is_allocated will
> never consider more than a sector of bitmap data.  And you can reuse all
> your cow_update_bitmap code.

Agreed. But can the two functions not share the same read data? As in:

cow_is_allocated(char *buf) {
  return test_bits(buf)
}

cow_co_is_allocated(Bdrv *b) {
  char sector[SECTOR_SIZE]
  bdrv_read(b, sector, sector_num)
  return cow_is_allocated(sector)
}

cow_update_bitmap(Bdrv *b) {
  while (sector) {
    char buf[SECTOR_SIZE]
    bdrv_read(b, buf, sector_num)
    if (cow_is_allocated(buf)) {
        if (!flushed)
            bdrv_flush()
        cow_update_bitmap_sector(buf)
        bdrv_write(buf)
    }
  }
}

> To be as efficient as possible, you could keep a memory copy of the
> bitmap, so that you only have to do writes, not reads.  The memory copy
> would be somewhat expensive of course but perhaps reasonable (256M of
> memory for a 1T image).  The largest cost would be loading the bitmap
> from memory at startup.  But that can be done later.

Yes I considered that option too. I guess it can be considered as 
another patch, with the performance implicated measured against the 
results of this patch.


Charlie