Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface

[John Snow]

On 06/03/2016 12:32 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/dirty-bitmap.c.
> 
> Two current users are converted too.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/backup.c               | 14 ++++++++------
>  block/dirty-bitmap.c         | 39 +++++++++++++++++++++++++++++++++------
>  block/mirror.c               | 24 +++++++++++++-----------
>  include/block/dirty-bitmap.h |  7 +++++--
>  include/qemu/typedefs.h      |  1 +
>  5 files changed, 60 insertions(+), 25 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index feeb9f8..ac7ca45 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -317,14 +317,14 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>      int64_t end;
>      int64_t last_cluster = -1;
>      int64_t sectors_per_cluster = cluster_size_sectors(job);
> -    HBitmapIter hbi;
> +    BdrvDirtyBitmapIter *dbi;
>  
>      granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
>      clusters_per_iter = MAX((granularity / job->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 / sectors_per_cluster;
>  
>          /* Fake progress updates for any clusters we skipped */
> @@ -336,7 +336,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(job, cluster * sectors_per_cluster,
>                                      sectors_per_cluster, &error_is_read,
> @@ -344,7 +344,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);
>          }
> @@ -352,7 +352,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 < job->cluster_size) {
> -            bdrv_set_dirty_iter(&hbi, cluster * sectors_per_cluster);
> +            bdrv_set_dirty_iter(dbi, cluster * sectors_per_cluster);
>          }
>  
>          last_cluster = cluster - 1;
> @@ -364,6 +364,8 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>          job->common.offset += ((end - last_cluster - 1) * job->cluster_size);
>      }
>  
> +out:
> +    bdrv_dirty_iter_free(dbi);
>      return ret;
>  }
>  
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4902ca5..ec073ee 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -42,9 +42,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;
> @@ -212,6 +218,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;
>      }
> @@ -224,6 +231,7 @@ static void 
> bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
>      BdrvDirtyBitmap *bm, *next;
>      QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
>          if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
> +            assert(!bitmap->active_iterators);

No good -- this function is also used to clear out all named bitmaps
belonging to a BDS, so 'bitmap' may be NULL, so this assertion is bad.

Fixing up in my local branch.

--js

>              assert(!bdrv_dirty_bitmap_frozen(bm));
>              QLIST_REMOVE(bm, list);
>              hbitmap_free(bm->bitmap);
> @@ -320,9 +328,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,
> @@ -373,12 +401,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> cur_sector,
>  }
>  
>  /**
> - * Advance an HBitmapIter to an arbitrary offset.
> + * Advance a BdrvDirtyBitmapIter 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->hbi.hb, sector_num);
>  }
>  
>  int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> diff --git a/block/mirror.c b/block/mirror.c
> index 80fd3c7..1d90aa5 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;
> @@ -317,10 +317,10 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>      int64_t end = s->bdev_length / BDRV_SECTOR_SIZE;
>      int sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
>  
> -    sector_num = hbitmap_iter_next(&s->hbi);
> +    sector_num = bdrv_dirty_iter_next(s->dbi);
>      if (sector_num < 0) {
> -        bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> -        sector_num = hbitmap_iter_next(&s->hbi);
> +        bdrv_set_dirty_iter(s->dbi, 0);
> +        sector_num = bdrv_dirty_iter_next(s->dbi);
>          trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
>          assert(sector_num >= 0);
>      }
> @@ -334,7 +334,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>      /* Find the number of consective dirty chunks following the first dirty
>       * one, and wait for in flight requests in them. */
>      while (nb_chunks * sectors_per_chunk < (s->buf_size >> 
> BDRV_SECTOR_BITS)) {
> -        int64_t hbitmap_next;
> +        int64_t next_dirty;
>          int64_t next_sector = sector_num + nb_chunks * sectors_per_chunk;
>          int64_t next_chunk = next_sector / sectors_per_chunk;
>          if (next_sector >= end ||
> @@ -345,13 +345,13 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>              break;
>          }
>  
> -        hbitmap_next = hbitmap_iter_next(&s->hbi);
> -        if (hbitmap_next > next_sector || hbitmap_next < 0) {
> +        next_dirty = bdrv_dirty_iter_next(s->dbi);
> +        if (next_dirty > next_sector || next_dirty < 0) {
>              /* The bitmap iterator's cache is stale, refresh it */
> -            bdrv_set_dirty_iter(&s->hbi, next_sector);
> -            hbitmap_next = hbitmap_iter_next(&s->hbi);
> +            bdrv_set_dirty_iter(s->dbi, next_sector);
> +            next_dirty = bdrv_dirty_iter_next(s->dbi);
>          }
> -        assert(hbitmap_next == next_sector);
> +        assert(next_dirty == next_sector);
>          nb_chunks++;
>      }
>  
> @@ -601,7 +601,8 @@ static void coroutine_fn mirror_run(void *opaque)
>          }
>      }
>  
> -    bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> +    assert(!s->dbi);
> +    s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
>      for (;;) {
>          uint64_t delay_ns = 0;
>          int64_t cnt;
> @@ -712,6 +713,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);
>  
>      data = g_malloc(sizeof(*data));
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 80afe60..2ea601b 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -36,8 +36,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 b113fcf..1b8c30a 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;
>