Re: [Qemu-block] [PATCH v4 10/20] qmp: Add support of "dirty-bitmap" sync mode for drive-backup

[John Snow]

On 04/02/2015 08:44 AM, Stefan Hajnoczi wrote:
> On Fri, Mar 20, 2015 at 03:16:53PM -0400, John Snow wrote:
> > +    } else if (job->sync_mode == MIRROR_SYNC_MODE_DIRTY_BITMAP) {
> > +        /* Dirty Bitmap sync has a slightly different iteration method */
> > +        HBitmapIter hbi;
> > +        int64_t sector;
> > +        int64_t cluster;
> > +        int64_t last_cluster = -1;
> > +        bool polyrhythmic;
> > +
> > +        bdrv_dirty_iter_init(bs, job->sync_bitmap, &hbi);
> > +        /* Does the granularity happen to match our backup cluster size? */
> > +        polyrhythmic = (bdrv_dirty_bitmap_granularity(job->sync_bitmap) !=
> > +                        BACKUP_CLUSTER_SIZE);
> > +
> > +        /* Find the next dirty /sector/ and copy that /cluster/ */
> > +        while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> > +            cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
> > +
> > +            /* Fake progress updates for any clusters we skipped,
> > +             * excluding this current cluster. */
> > +            if (cluster != last_cluster + 1) {
> > +                job->common.offset += ((cluster - last_cluster - 1) *
> > +                                       BACKUP_CLUSTER_SIZE);
> > +            }
> > +
> > +            if (yield_and_check(job)) {
> > +                goto leave;
> > +            }
> > +
> > +            do {
> > +                ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
> > +                                    BACKUP_SECTORS_PER_CLUSTER, 
> > &error_is_read);
> > +                if ((ret < 0) &&
> > +                    backup_error_action(job, error_is_read, -ret) ==
> > +                    BLOCK_ERROR_ACTION_REPORT) {
> > +                    goto leave;
> > +                }
> > +            } while (ret < 0);
> > +
> > +            /* Advance (or rewind) our iterator if we need to. */
> > +            if (polyrhythmic) {
> > +                bdrv_set_dirty_iter(&hbi,
> > +                                    (cluster + 1) * 
> > BACKUP_SECTORS_PER_CLUSTER);
> > +            }
> > +
> > +            last_cluster = cluster;
> > +        }
>
> What happens when the dirty bitmap granularity is larger than
> BACKUP_SECTORS_PER_CLUSTER?
>
> |-----bitmap granularity-----|
> |---backup cluster---|
>                        ~~~~~~~
> Will these sectors ever be copied?
>
> I think this case causes an infinite loop:
>
>    cluster = hbitmap_iter_next(&hbi) / BACKUP_SECTORS_PER_CLUSTER
>
> The iterator is reset:
>
>    bdrv_set_dirty_iter(&hbi, (cluster + 1) * BACKUP_SECTORS_PER_CLUSTER);
>
> So we get the same cluster ever time and never advance?

I had mistakenly thought that if I advanced to the middle of a 
granularity grouping that the iterator might return the next (virtual) 
index to me, instead of the beginning of the current group.

Anyway, I've fixed this up a bit and added in some granularity variance 
tests for the iotests to test what happens if the granularity is larger, 
equal, or smaller.

v5 is ready to send out, but I need to test it first, so I will probably 
send that out Wednesday night.

Thanks,
--js