[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface |
Date: |
Tue, 5 Jan 2016 18:01:47 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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 <address@hidden>
> ---
> 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 <address@hidden>
- [Qemu-devel] [PATCH 00/13] Dirty bitmap changes for migration/persistence work, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 01/13] backup: Use Bitmap to replace "s->bitmap", Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 02/13] typedefs: Add BdrvDirtyBitmap and HBitmapIter, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 04/13] block: Remove unused typedef of BlockDriverDirtyHandler, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 03/13] block: Move block dirty bitmap code to separate files, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface, Fam Zheng, 2016/01/04
- Re: [Qemu-devel] [PATCH 05/13] block: Hide HBitmap in block dirty bitmap interface,
John Snow <=
- [Qemu-devel] [PATCH 07/13] tests: Add test code for meta bitmap, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 06/13] HBitmap: Introduce "meta" bitmap to track bit changes, Fam Zheng, 2016/01/04
- [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap, Fam Zheng, 2016/01/04