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

[Stefan Hajnoczi]

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?

pgpm_eu4Rkqhu.pgp
Description: PGP signature