From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50683) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aLl1t-000213-JJ for qemu-devel@nongnu.org; Wed, 20 Jan 2016 00:10:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aLl1s-0005lW-JM for qemu-devel@nongnu.org; Wed, 20 Jan 2016 00:10:01 -0500 Date: Wed, 20 Jan 2016 13:09:48 +0800 From: Fam Zheng Message-ID: <20160120050948.GC3164@ad.usersys.redhat.com> References: <1451903234-32529-1-git-send-email-famz@redhat.com> <1451903234-32529-6-git-send-email-famz@redhat.com> <568C4B5B.7030205@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <568C4B5B.7030205@redhat.com> Subject: Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Kevin Wolf , Jeff Cody , Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org, qemu-block@nongnu.org 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 > > --- > > 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