On 11.03.2016 14:54, Max Reitz wrote: > 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 <[email protected]> >> --- >> 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.
OK, here it wouldn't because s->dbi is still NULL. Why do we need the
bdrv_dirty_iter_free(), then? It wasn't present in v3.
Max
> 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
