Re: [Qemu-devel] [PATCH v2 05/13] block: Hide HBitmap in block dirty bitmap interface

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

On 20.01.2016 09:11, Fam Zheng wrote:
> HBitmap is an implementation detail of block dirty bitmap that should be hidden
> from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlying
> HBitmapIter.
>
> A small difference in the interface is, before, an HBitmapIter is initialized
> in place, now the new BdrvDirtyBitmapIter must be dynamically allocated because
> the structure definition is in block.c.
>
> Two current users are converted too.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>   block/backup.c               | 14 ++++++++------
>   block/dirty-bitmap.c         | 37 ++++++++++++++++++++++++++++++++-----
>   block/mirror.c               | 14 ++++++++------
>   include/block/dirty-bitmap.h |  7 +++++--
>   include/qemu/typedefs.h      |  1 +
>   5 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/block/backup.c b/block/backup.c
> index 56ddec0..2388039 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -326,14 +326,14 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>       int64_t end;
>       int64_t last_cluster = -1;
>       BlockDriverState *bs = job->common.bs;
> -    HBitmapIter hbi;
> +    BdrvDirtyBitmapIter *dbi;
>   
>       granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
>       clusters_per_iter = MAX((granularity / BACKUP_CLUSTER_SIZE), 1);
> -    bdrv_dirty_iter_init(job->sync_bitmap, &hbi);
> +    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
>   
>       /* Find the next dirty sector(s) */
> -    while ((sector = hbitmap_iter_next(&hbi)) != -1) {
> +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
>           cluster = sector / BACKUP_SECTORS_PER_CLUSTER;
>   
>           /* Fake progress updates for any clusters we skipped */
> @@ -345,7 +345,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>           for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
>               do {
>                   if (yield_and_check(job)) {
> -                    return ret;
> +                    goto out;
>                   }
>                   ret = backup_do_cow(bs, cluster * BACKUP_SECTORS_PER_CLUSTER,
>                                       BACKUP_SECTORS_PER_CLUSTER, &error_is_read,
> @@ -353,7 +353,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>                   if ((ret < 0) &&
>                       backup_error_action(job, error_is_read, -ret) ==
>                       BLOCK_ERROR_ACTION_REPORT) {
> -                    return ret;
> +                    goto out;
>                   }
>               } while (ret < 0);
>           }
> @@ -361,7 +361,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>           /* If the bitmap granularity is smaller than the backup granularity,
>            * we need to advance the iterator pointer to the next cluster. */
>           if (granularity < BACKUP_CLUSTER_SIZE) {
> -            bdrv_set_dirty_iter(&hbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
> +            bdrv_set_dirty_iter(dbi, cluster * BACKUP_SECTORS_PER_CLUSTER);
>           }
>   
>           last_cluster = cluster - 1;
> @@ -373,6 +373,8 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>           job->common.offset += ((end - last_cluster - 1) * BACKUP_CLUSTER_SIZE);
>       }
>   
> +out:
> +    bdrv_dirty_iter_free(dbi);
>       return ret;
>   }
>   
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 7924c38..bd7758b 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -41,9 +41,15 @@ struct BdrvDirtyBitmap {
>       char *name;                 /* Optional non-empty unique ID */
>       int64_t size;               /* Size of the bitmap (Number of sectors) */
>       bool disabled;              /* Bitmap is read-only */
> +    int active_iterators;       /* How many iterators are active */
>       QLIST_ENTRY(BdrvDirtyBitmap) list;
>   };
>   
> +struct BdrvDirtyBitmapIter {
> +    HBitmapIter hbi;
> +    BdrvDirtyBitmap *bitmap;
> +};
> +
>   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
>   {
>       BdrvDirtyBitmap *bm;
> @@ -211,6 +217,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>   
>       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>           assert(!bdrv_dirty_bitmap_frozen(bitmap));
> +        assert(!bitmap->active_iterators);
>           hbitmap_truncate(bitmap->bitmap, size);
>           bitmap->size = size;
>       }
> @@ -221,6 +228,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
>       BdrvDirtyBitmap *bm, *next;
>       QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>           if (bm == bitmap) {
> +            assert(!bitmap->active_iterators);
>               assert(!bdrv_dirty_bitmap_frozen(bm));
>               QLIST_REMOVE(bitmap, list);
>               hbitmap_free(bitmap->bitmap);
> @@ -299,9 +307,29 @@ uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
>       return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
>   }
>   
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, HBitmapIter *hbi)
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> +                                         uint64_t first_sector)
>   {
> -    hbitmap_iter_init(hbi, bitmap->bitmap, 0);
> +    BdrvDirtyBitmapIter *iter = g_new(BdrvDirtyBitmapIter, 1);
> +    hbitmap_iter_init(&iter->hbi, bitmap->bitmap, first_sector);
> +    iter->bitmap = bitmap;
> +    bitmap->active_iterators++;
> +    return iter;
> +}
> +
> +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter)
> +{
> +    if (!iter) {
> +        return;
> +    }
> +    assert(iter->bitmap->active_iterators > 0);
> +    iter->bitmap->active_iterators--;
> +    g_free(iter);
> +}
> +
> +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter)
> +{
> +    return hbitmap_iter_next(&iter->hbi);
>   }
>   
>   void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> @@ -354,10 +382,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>   /**
>    * Advance an HBitmapIter to an arbitrary offset.

fix comment.

>    */
> -void bdrv_set_dirty_iter(HBitmapIter *hbi, int64_t offset)
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *iter, int64_t sector_num)
>   {
> -    assert(hbi->hb);
> -    hbitmap_iter_init(hbi, hbi->hb, offset);
> +    hbitmap_iter_init(&iter->hbi, iter->bitmap->bitmap, sector_num);

why? The old way works for meta iters. The new code - doesn't. I suppose

assert(iter->hbi.hb);
hbitmap_iter_init(&iter->hbi, iter->hbi.hb, sector_num);

>   }
>   
>   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index f201f2b..70ca844 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -52,7 +52,7 @@ typedef struct MirrorBlockJob {
>       int64_t bdev_length;
>       unsigned long *cow_bitmap;
>       BdrvDirtyBitmap *dirty_bitmap;
> -    HBitmapIter hbi;
> +    BdrvDirtyBitmapIter *dbi;
>       uint8_t *buf;
>       QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
>       int buf_free_count;
> @@ -170,10 +170,11 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>   
>       max_iov = MIN(source->bl.max_iov, s->target->bl.max_iov);
>   
> -    s->sector_num = hbitmap_iter_next(&s->hbi);
> +    s->sector_num = bdrv_dirty_iter_next(s->dbi);
>       if (s->sector_num < 0) {
> -        bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> -        s->sector_num = hbitmap_iter_next(&s->hbi);
> +        bdrv_dirty_iter_free(s->dbi);
> +        s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
> +        s->sector_num = bdrv_dirty_iter_next(s->dbi);
>           trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
>           assert(s->sector_num >= 0);
>       }
> @@ -291,7 +292,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>            */
>           if (next_sector > hbitmap_next_sector
>               && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
> -            hbitmap_next_sector = hbitmap_iter_next(&s->hbi);
> +            hbitmap_next_sector = bdrv_dirty_iter_next(s->dbi);
>           }
>   
>           next_sector += sectors_per_chunk;
> @@ -502,7 +503,7 @@ static void coroutine_fn mirror_run(void *opaque)
>           }
>       }
>   
> -    bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
>       for (;;) {
>           uint64_t delay_ns = 0;
>           int64_t cnt;
> @@ -615,6 +616,7 @@ immediate_exit:
>       qemu_vfree(s->buf);
>       g_free(s->cow_bitmap);
>       g_free(s->in_flight_bitmap);
> +    bdrv_dirty_iter_free(s->dbi);
>       bdrv_release_dirty_bitmap(bs, s->dirty_bitmap);
>       if (s->target->blk) {
>           blk_iostatus_disable(s->target->blk);
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 02a9337..120bac6 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -35,8 +35,11 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                              int64_t cur_sector, int nr_sectors);
>   void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>                                int64_t cur_sector, int nr_sectors);
> -void bdrv_dirty_iter_init(BdrvDirtyBitmap *bitmap, struct HBitmapIter *hbi);
> -void bdrv_set_dirty_iter(struct HBitmapIter *hbi, int64_t offset);
> +BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
> +                                         uint64_t first_sector);
> +void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);
> +int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter);
> +void bdrv_set_dirty_iter(BdrvDirtyBitmapIter *hbi, int64_t sector_num);
>   int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap);
>   void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
>   
> diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> index 0cf9d74..a1bdb5a 100644
> --- a/include/qemu/typedefs.h
> +++ b/include/qemu/typedefs.h
> @@ -11,6 +11,7 @@ typedef struct AioContext AioContext;
>   typedef struct AllwinnerAHCIState AllwinnerAHCIState;
>   typedef struct AudioState AudioState;
>   typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> +typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
>   typedef struct BlockBackend BlockBackend;
>   typedef struct BlockBackendRootState BlockBackendRootState;
>   typedef struct BlockDriverState BlockDriverState;


-- 
Best regards,
Vladimir
* now, @virtuozzo.com instead of @parallels.com. Sorry for this inconvenience.