[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bit
From: |
John Snow |
Subject: |
Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface |
Date: |
Tue, 12 Jul 2016 18:49:19 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 |
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;
>
- Re: [Qemu-devel] [PATCH v5 01/10] block: Hide HBitmap in block dirty bitmap interface,
John Snow <=