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

[John Snow <jsnow@redhat.com>]

Should we skip adding the typedef for HBitmapIter if we're just going to
use this instead?

On 01/04/2016 05:27 AM, 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>
> ---
>  block/backup.c               | 14 ++++++++------
>  block/dirty-bitmap.c         | 36 +++++++++++++++++++++++++++++++-----
>  block/mirror.c               | 14 ++++++++------
>  include/block/dirty-bitmap.h |  7 +++++--
>  include/qemu/typedefs.h      |  1 +
>  5 files changed, 53 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..53cf88d 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;
> @@ -221,6 +227,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);

Should we add any assertions into the truncate function, too?

>              assert(!bdrv_dirty_bitmap_frozen(bm));
>              QLIST_REMOVE(bitmap, list);
>              hbitmap_free(bitmap->bitmap);
> @@ -299,9 +306,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 +381,9 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
>  /**
>   * Advance an HBitmapIter to an arbitrary offset.
>   */
> -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);
>  }
>  
>  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 6175cf3..16bb15a 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -34,8 +34,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 e83934e..0e9efcd 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;
> 

Regardless of answers to above questions:

Reviewed-by: John Snow <jsnow@redhat.com>