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

[Fam Zheng <famz@redhat.com>]

On Tue, 01/05 18:01, John Snow wrote:
> Should we skip adding the typedef for HBitmapIter if we're just going to
> use this instead?

Yes, we can clean this up.

> 
> 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?

Good idea.

Thanks.

Fam