On Mon, May 20, 2013 at 01:30:37PM +0200, Paolo Bonzini wrote:
> Il 15/05/2013 16:34, Stefan Hajnoczi ha scritto:
> > + wait_for_overlapping_requests(job, start, end);
> > + cow_request_begin(&cow_request, job, start, end);
> > +
> > + for (; start < end; start++) {
> > + if (hbitmap_get(job->bitmap, start)) {
> > + DPRINTF("brdv_co_backup_cow skip C%" PRId64 "\n", start);
> > + continue; /* already copied */
> > + }
> > +
> > + /* immediately set bitmap (avoid coroutine race) */
> > + hbitmap_set(job->bitmap, start, 1);
> > +
>
> HBitmap already has code for finding the next set bit, but you're not
> using it.
>
> If you reverse the direction of the bitmap, you can use an HBitmapIter here:
>
> >
> > + start = 0;
> > + end = DIV_ROUND_UP(bdrv_getlength(bs) / BDRV_SECTOR_SIZE,
> > + BACKUP_BLOCKS_PER_CLUSTER);
> > +
> > + job->bitmap = hbitmap_alloc(end, 0);
> > +
> > + before_write = bdrv_add_before_write_cb(bs, backup_before_write);
> > +
> > + DPRINTF("backup_run start %s %" PRId64 " %" PRId64 "\n",
> > + bdrv_get_device_name(bs), start, end);
> > +
> > + for (; start < end; start++) {
>
> ... instead of iterating through each item manually.
/**
* hbitmap_iter_init:
[...]
* position of the iterator is also okay. However, concurrent resetting of
* bits can lead to unexpected behavior if the iterator has not yet reached
* those bits.
*/
void hbitmap_iter_init(HBitmapIter *hbi, const HBitmap *hb, uint64_t first);
This worries me. We would initialize the bitmap to all 1s. Backing up
a cluster resets the bit. But the documentation says it is not safe to
reset bits while iterating?
Stefan
