On 08.03.2016 05:44, 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.
It's block/dirty-bitmap.c now. :-)
> Two current users are converted too.
>
> Signed-off-by: Fam Zheng <f...@redhat.com>
> ---
> block/backup.c | 14 ++++++++------
> block/dirty-bitmap.c | 39 +++++++++++++++++++++++++++++++++------
> block/mirror.c | 15 +++++++++------
> include/block/dirty-bitmap.h | 7 +++++--
> include/qemu/typedefs.h | 1 +
> 5 files changed, 56 insertions(+), 20 deletions(-)
>
[...]
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 556e1d1..16f73b2 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
[...]
> @@ -373,12 +401,11 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
> cur_sector,
> }
>
> /**
> - * Advance an HBitmapIter to an arbitrary offset.
> + * Advance an BdrvDirtyBitmapIter to an arbitrary offset.
This should be "a BdrvDirtyBitmapIter".
> */
> -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 9635fa8..87a5a86 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
[...]
> @@ -304,10 +304,11 @@ 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_dirty_iter_free(s->dbi);
> + s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
Works, but wouldn't bdrv_set_dirty_iter(s->dbi, 0); suffice?
> + sector_num = bdrv_dirty_iter_next(s->dbi);
> trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
> assert(sector_num >= 0);
> }
> @@ -330,7 +331,7 @@ static uint64_t coroutine_fn
> mirror_iteration(MirrorBlockJob *s)
> mirror_wait_for_io(s);
> /* Now retry. */
> } else {
> - hbitmap_next = hbitmap_iter_next(&s->hbi);
> + hbitmap_next = bdrv_dirty_iter_next(s->dbi);
It would make sense to change this variable's name from hbitmap_next to
e.g. bdrv_dirty_next or bs_next_dirty.
> assert(hbitmap_next == next_sector);
> nb_chunks++;
> }
> @@ -577,7 +578,8 @@ static void coroutine_fn mirror_run(void *opaque)
> }
> }
>
> - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi);
> + bdrv_dirty_iter_free(s->dbi);
> + s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap, 0);
Again, bdrv_set_dirty_iter(s->dbi, 0); should work just fine.
Since this patch is technically correct, I could still imagine giving an
R-b, but I'm really reluctant to do so because of the
bdrv_dirty_iter_free()/bdrv_dirty_iter_new() pairs.
Max
> for (;;) {
> uint64_t delay_ns = 0;
> int64_t cnt;
> @@ -688,6 +690,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 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 fd039e0..7c39f0f 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;
>
signature.asc
Description: OpenPGP digital signature
