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 <f...@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
