On Fri, 11/20 19:08, Vladimir Sementsov-Ogievskiy wrote:
> On 20.11.2015 12:59, 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 <f...@redhat.com>
> >---
> > block.c | 79
> > ++++++++++++++++++++++++++++++++++++++-------------
> > block/backup.c | 14 +++++----
> > block/mirror.c | 14 +++++----
> > include/block/block.h | 9 ++++--
> > 4 files changed, 82 insertions(+), 34 deletions(-)
> >
> >diff --git a/block.c b/block.c
> >index 3a7324b..e225050 100644
> >--- a/block.c
> >+++ b/block.c
> >@@ -63,14 +63,22 @@
> > * or enabled. A frozen bitmap can only abdicate() or reclaim().
> > */
> > struct BdrvDirtyBitmap {
> >+ int gran_shift; /* Bits to right shift from sector number to
> >+ bit index. */
> > HBitmap *bitmap; /* Dirty sector bitmap implementation */
> > BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status
> > */
> > 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;
> >+};
> >+
> > #define NOT_DONE 0x7fffffff /* used while emulated sync operation in
> > progress */
> > struct BdrvStates bdrv_states = QTAILQ_HEAD_INITIALIZER(bdrv_states);
> >@@ -3157,24 +3165,26 @@ BdrvDirtyBitmap
> >*bdrv_create_dirty_bitmap(BlockDriverState *bs,
> > {
> > int64_t bitmap_size;
> > BdrvDirtyBitmap *bitmap;
> >- uint32_t sector_granularity;
> >+ int gran_shift;
> > assert((granularity & (granularity - 1)) == 0);
> >+ /* Caller should check that */
> >+ assert(granularity >= BDRV_SECTOR_SIZE);
> >+ gran_shift = ctz32(granularity) - BDRV_SECTOR_BITS;
> > if (name && bdrv_find_dirty_bitmap(bs, name)) {
> > error_setg(errp, "Bitmap already exists: %s", name);
> > return NULL;
> > }
> >- sector_granularity = granularity >> BDRV_SECTOR_BITS;
> >- assert(sector_granularity);
> >- bitmap_size = bdrv_nb_sectors(bs);
> >+ bitmap_size = DIV_ROUND_UP(bdrv_getlength(bs), granularity);
> > if (bitmap_size < 0) {
> > error_setg_errno(errp, -bitmap_size, "could not get length of
> > device");
> > errno = -bitmap_size;
> > return NULL;
> > }
> > bitmap = g_new0(BdrvDirtyBitmap, 1);
> >- bitmap->bitmap = hbitmap_alloc(bitmap_size, ctz32(sector_granularity));
> >+ bitmap->bitmap = hbitmap_alloc(bitmap_size, 0);
> >+ bitmap->gran_shift = gran_shift;
> > bitmap->size = bitmap_size;
> > bitmap->name = g_strdup(name);
> > bitmap->disabled = false;
> >@@ -3293,9 +3303,10 @@ BdrvDirtyBitmap
> >*bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> > static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> > {
> > BdrvDirtyBitmap *bitmap;
> >- uint64_t size = bdrv_nb_sectors(bs);
> > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> >+ int64_t size = bdrv_nb_sectors(bs) >> bitmap->gran_shift;
> >+ /* TODO: what if size < 0? */
> > assert(!bdrv_dirty_bitmap_frozen(bitmap));
> > hbitmap_truncate(bitmap->bitmap, size);
> > bitmap->size = size;
> >@@ -3307,6 +3318,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);
> > assert(!bdrv_dirty_bitmap_frozen(bm));
> > QLIST_REMOVE(bitmap, list);
> > hbitmap_free(bitmap->bitmap);
> >@@ -3354,7 +3366,7 @@ BlockDirtyInfoList
> >*bdrv_query_dirty_bitmaps(BlockDriverState *bs)
> > int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int64_t
> > sector)
> > {
> > if (bitmap) {
> >- return hbitmap_get(bitmap->bitmap, sector);
> >+ return hbitmap_get(bitmap->bitmap, sector >> bitmap->gran_shift);
> > } else {
> > return 0;
> > }
> >@@ -3382,26 +3394,56 @@ uint32_t
> >bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> > uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap)
> > {
> >- return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> >+ return BDRV_SECTOR_SIZE << bitmap->gran_shift;
> > }
> >-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 >> bitmap->gran_shift);
> >+ 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)
> >+{
> >+ int64_t ret = hbitmap_iter_next(&iter->hbi);
> >+ return ret < 0 ? ret : ret << iter->bitmap->gran_shift;
> > }
> > void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> > int64_t cur_sector, int nr_sectors)
> > {
> >+ int64_t start = cur_sector >> bitmap->gran_shift;
> >+ int64_t end = DIV_ROUND_UP(cur_sector + nr_sectors,
> >+ 1 << bitmap->gran_shift);
> >+
> > assert(bdrv_dirty_bitmap_enabled(bitmap));
> >- hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> >+ hbitmap_set(bitmap->bitmap, start, end - start);
> > }
> > void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> > int64_t cur_sector, int nr_sectors)
> > {
> >+ int64_t start = cur_sector >> bitmap->gran_shift;
> >+ int64_t end = DIV_ROUND_UP(cur_sector + nr_sectors,
> >+ 1 << bitmap->gran_shift);
> >+
> > assert(bdrv_dirty_bitmap_enabled(bitmap));
> >- hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> >+ hbitmap_reset(bitmap->bitmap, start, end - start);
> > }
> > void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> >@@ -3411,8 +3453,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> >HBitmap **out)
> > hbitmap_reset_all(bitmap->bitmap);
> > } else {
> > HBitmap *backup = bitmap->bitmap;
> >- bitmap->bitmap = hbitmap_alloc(bitmap->size,
> >- hbitmap_granularity(backup));
> >+ bitmap->bitmap = hbitmap_alloc(bitmap->size, 0);
> > *out = backup;
> > }
> > }
> >@@ -3433,22 +3474,22 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t
> >cur_sector,
> > if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> > continue;
> > }
> >- hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> >+ bdrv_set_dirty_bitmap(bitmap, cur_sector, nr_sectors);
> > }
> > }
> > /**
> > * 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 >> iter->bitmap->gran_shift);
> > }
> > int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap)
> > {
> >- return hbitmap_count(bitmap->bitmap);
> >+ return hbitmap_count(bitmap->bitmap) << bitmap->gran_shift;
> > }
> > /* Get a reference to bs */
> >diff --git a/block/backup.c b/block/backup.c
> >index d408f98..a3f60ff 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/mirror.c b/block/mirror.c
> >index 52c9abf..6515455 100644
> >--- a/block/mirror.c
> >+++ b/block/mirror.c
> >@@ -51,7 +51,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;
> >@@ -167,10 +167,11 @@ static uint64_t coroutine_fn
> >mirror_iteration(MirrorBlockJob *s)
> > int pnum;
> > int64_t ret;
> >- 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);
> > }
> >@@ -288,7 +289,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;
> >@@ -487,7 +488,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;
> >@@ -600,6 +601,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/block.h b/include/block/block.h
> >index 73edb1a..bc6f2e3 100644
> >--- a/include/block/block.h
> >+++ b/include/block/block.h
> >@@ -470,8 +470,8 @@ void *qemu_try_blockalign(BlockDriverState *bs, size_t
> >size);
> > void *qemu_try_blockalign0(BlockDriverState *bs, size_t size);
> > bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov);
> >-struct HBitmapIter;
> > typedef struct BdrvDirtyBitmap BdrvDirtyBitmap;
> >+typedef struct BdrvDirtyBitmapIter BdrvDirtyBitmapIter;
> > BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> > uint32_t granularity,
> > const char *name,
> >@@ -502,8 +502,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_enable_copy_on_read(BlockDriverState *bs);
>
> IMO, granularity changes (which are not described in commit msg)
> should be moved to separate patch
Which granularity changes do you mean? The interface and the callers have to be
changed in the same patch to keep bisectability.
Fam
