From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37223) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a0yl2-0006JA-Is for qemu-devel@nongnu.org; Mon, 23 Nov 2015 16:34:46 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a0yl0-00029p-2e for qemu-devel@nongnu.org; Mon, 23 Nov 2015 16:34:44 -0500 References: <1448013593-14282-1-git-send-email-famz@redhat.com> <1448013593-14282-3-git-send-email-famz@redhat.com> From: John Snow Message-ID: <5653866A.2080003@redhat.com> Date: Mon, 23 Nov 2015 16:34:34 -0500 MIME-Version: 1.0 In-Reply-To: <1448013593-14282-3-git-send-email-famz@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH for 2.6 2/3] block: Hide HBitmap in block dirty bitmap interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng , qemu-devel@nongnu.org Cc: Kevin Wolf , vsementsov@virtuozzo.com, qemu-block@nongnu.org, Jeff Cody , mreitz@redhat.com, pbonzini@redhat.com On 11/20/2015 04:59 AM, Fam Zheng wrote: > HBitmap is an implementation detail of block dirty bitmap that should b= e hidden > from users. Introduce a BdrvDirtyBitmapIter to encapsulate the underlyi= ng > HBitmapIter. >=20 > A small difference in the interface is, before, an HBitmapIter is initi= alized > in place, now the new BdrvDirtyBitmapIter must be dynamically allocated= because > the structure definition is in block.c. >=20 > Two current users are converted too. >=20 > Signed-off-by: Fam Zheng > --- > block.c | 79 ++++++++++++++++++++++++++++++++++++++-----= -------- > block/backup.c | 14 +++++---- > block/mirror.c | 14 +++++---- > include/block/block.h | 9 ++++-- > 4 files changed, 82 insertions(+), 34 deletions(-) >=20 > 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 num= ber to > + bit index. */ > HBitmap *bitmap; /* Dirty sector bitmap implementation = */ > BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen sta= tus */ > char *name; /* Optional non-empty unique ID */ > int64_t size; /* Size of the bitmap (Number of secto= rs) */ > bool disabled; /* Bitmap is read-only */ > + int active_iterators; /* How many iterators are active */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > =20 > +struct BdrvDirtyBitmapIter { > + HBitmapIter hbi; > + BdrvDirtyBitmap *bitmap; > +}; > + > #define NOT_DONE 0x7fffffff /* used while emulated sync operation in p= rogress */ > =20 > struct BdrvStates bdrv_states =3D QTAILQ_HEAD_INITIALIZER(bdrv_states)= ; > @@ -3157,24 +3165,26 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(Block= DriverState *bs, > { > int64_t bitmap_size; > BdrvDirtyBitmap *bitmap; > - uint32_t sector_granularity; > + int gran_shift; > =20 > assert((granularity & (granularity - 1)) =3D=3D 0); > + /* Caller should check that */ > + assert(granularity >=3D BDRV_SECTOR_SIZE); > =20 > + gran_shift =3D 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 =3D granularity >> BDRV_SECTOR_BITS; > - assert(sector_granularity); > - bitmap_size =3D bdrv_nb_sectors(bs); > + bitmap_size =3D DIV_ROUND_UP(bdrv_getlength(bs), granularity); > if (bitmap_size < 0) { > error_setg_errno(errp, -bitmap_size, "could not get length of = device"); > errno =3D -bitmap_size; > return NULL; > } > bitmap =3D g_new0(BdrvDirtyBitmap, 1); > - bitmap->bitmap =3D hbitmap_alloc(bitmap_size, ctz32(sector_granula= rity)); > + bitmap->bitmap =3D hbitmap_alloc(bitmap_size, 0); Hmm, what's the idea, here? This patch does a lot more than just hide hbitmap details from callers of block_dirty_bitmap functions. So we're changing the backing hbitmap to always be one where g=3D0 and th= e number of physical bits directly is (now) the same as the number of 'virtual' bits, pre-patch. Then, to compensate, we handle the shift math to convert the bitmap granularity to sector size and vice-versa in the Block Dirty Bitmap layer instead of in the hbitmap layer. What's the benefit? It looks like we just pull all the implementation details up from hbitmap and into BdrvDirtyBitmap, which I am not immediately convinced of as being a benefit. > + bitmap->gran_shift =3D gran_shift; > bitmap->size =3D bitmap_size; > bitmap->name =3D g_strdup(name); > bitmap->disabled =3D false; > @@ -3293,9 +3303,10 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap(Block= DriverState *bs, > static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) > { > BdrvDirtyBitmap *bitmap; > - uint64_t size =3D bdrv_nb_sectors(bs); > =20 > QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + int64_t size =3D 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 =3D 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 =3D=3D 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(Bloc= kDriverState *bs) > int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int6= 4_t sector) > { > if (bitmap) { > - return hbitmap_get(bitmap->bitmap, sector); > + return hbitmap_get(bitmap->bitmap, sector >> bitmap->gran_shif= t); > } else { > return 0; > } > @@ -3382,26 +3394,56 @@ uint32_t bdrv_get_default_bitmap_granularity(Bl= ockDriverState *bs) > =20 > uint32_t bdrv_dirty_bitmap_granularity(BdrvDirtyBitmap *bitmap) > { > - return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap); > + return BDRV_SECTOR_SIZE << bitmap->gran_shift; > } > =20 > -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 =3D g_new(BdrvDirtyBitmapIter, 1); > + hbitmap_iter_init(&iter->hbi, bitmap->bitmap, > + first_sector >> bitmap->gran_shift); > + iter->bitmap =3D 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 =3D hbitmap_iter_next(&iter->hbi); > + return ret < 0 ? ret : ret << iter->bitmap->gran_shift; > } > =20 > void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors) > { > + int64_t start =3D cur_sector >> bitmap->gran_shift; > + int64_t end =3D 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); > } > =20 > void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, > int64_t cur_sector, int nr_sectors) > { > + int64_t start =3D cur_sector >> bitmap->gran_shift; > + int64_t end =3D 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); > } > =20 > void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) > @@ -3411,8 +3453,7 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bit= map, HBitmap **out) > hbitmap_reset_all(bitmap->bitmap); > } else { > HBitmap *backup =3D bitmap->bitmap; > - bitmap->bitmap =3D hbitmap_alloc(bitmap->size, > - hbitmap_granularity(backup)); > + bitmap->bitmap =3D hbitmap_alloc(bitmap->size, 0); > *out =3D 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); > } > } > =20 > /** > * 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); > } > =20 > int64_t bdrv_get_dirty_count(BdrvDirtyBitmap *bitmap) > { > - return hbitmap_count(bitmap->bitmap); > + return hbitmap_count(bitmap->bitmap) << bitmap->gran_shift; > } > =20 > /* 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(Ba= ckupBlockJob *job) > int64_t end; > int64_t last_cluster =3D -1; > BlockDriverState *bs =3D job->common.bs; > - HBitmapIter hbi; > + BdrvDirtyBitmapIter *dbi; > =20 > granularity =3D bdrv_dirty_bitmap_granularity(job->sync_bitmap); > clusters_per_iter =3D MAX((granularity / BACKUP_CLUSTER_SIZE), 1); > - bdrv_dirty_iter_init(job->sync_bitmap, &hbi); > + dbi =3D bdrv_dirty_iter_new(job->sync_bitmap, 0); > =20 > /* Find the next dirty sector(s) */ > - while ((sector =3D hbitmap_iter_next(&hbi)) !=3D -1) { > + while ((sector =3D bdrv_dirty_iter_next(dbi)) !=3D -1) { > cluster =3D sector / BACKUP_SECTORS_PER_CLUSTER; > =20 > /* Fake progress updates for any clusters we skipped */ > @@ -345,7 +345,7 @@ static int coroutine_fn backup_run_incremental(Back= upBlockJob *job) > for (end =3D cluster + clusters_per_iter; cluster < end; clust= er++) { > do { > if (yield_and_check(job)) { > - return ret; > + goto out; > } > ret =3D 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(Back= upBlockJob *job) > if ((ret < 0) && > backup_error_action(job, error_is_read, -ret) =3D=3D > BLOCK_ERROR_ACTION_REPORT) { > - return ret; > + goto out; > } > } while (ret < 0); > } > @@ -361,7 +361,7 @@ static int coroutine_fn backup_run_incremental(Back= upBlockJob *job) > /* If the bitmap granularity is smaller than the backup granul= arity, > * 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_CLU= STER); > + bdrv_set_dirty_iter(dbi, cluster * BACKUP_SECTORS_PER_CLUS= TER); > } > =20 > last_cluster =3D cluster - 1; > @@ -373,6 +373,8 @@ static int coroutine_fn backup_run_incremental(Back= upBlockJob *job) > job->common.offset +=3D ((end - last_cluster - 1) * BACKUP_CLU= STER_SIZE); > } > =20 > +out: > + bdrv_dirty_iter_free(dbi); > return ret; > } > =20 > 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(Mir= rorBlockJob *s) > int pnum; > int64_t ret; > =20 > - s->sector_num =3D hbitmap_iter_next(&s->hbi); > + s->sector_num =3D bdrv_dirty_iter_next(s->dbi); > if (s->sector_num < 0) { > - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi); > - s->sector_num =3D hbitmap_iter_next(&s->hbi); > + bdrv_dirty_iter_free(s->dbi); > + s->dbi =3D bdrv_dirty_iter_new(s->dirty_bitmap, 0); > + s->sector_num =3D bdrv_dirty_iter_next(s->dbi); > trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bit= map)); > assert(s->sector_num >=3D 0); > } > @@ -288,7 +289,7 @@ static uint64_t coroutine_fn mirror_iteration(Mirro= rBlockJob *s) > */ > if (next_sector > hbitmap_next_sector > && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) { > - hbitmap_next_sector =3D hbitmap_iter_next(&s->hbi); > + hbitmap_next_sector =3D bdrv_dirty_iter_next(s->dbi); > } > =20 > next_sector +=3D sectors_per_chunk; > @@ -487,7 +488,7 @@ static void coroutine_fn mirror_run(void *opaque) > } > } > =20 > - bdrv_dirty_iter_init(s->dirty_bitmap, &s->hbi); > + s->dbi =3D bdrv_dirty_iter_new(s->dirty_bitmap, 0); > for (;;) { > uint64_t delay_ns =3D 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, siz= e_t size); > void *qemu_try_blockalign0(BlockDriverState *bs, size_t size); > bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov); > =20 > -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); > =20 > void bdrv_enable_copy_on_read(BlockDriverState *bs); >=20 --=20 =97js