From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdLJL-0007fI-Pn for qemu-devel@nongnu.org; Mon, 04 Nov 2013 09:39:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VdLJD-0005HL-RU for qemu-devel@nongnu.org; Mon, 04 Nov 2013 09:39:23 -0500 Received: from nodalink.pck.nerim.net ([62.212.105.220]:40074 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdLJD-0005Gn-2J for qemu-devel@nongnu.org; Mon, 04 Nov 2013 09:39:15 -0500 Date: Mon, 4 Nov 2013 15:38:59 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20131104143858.GC2980@irqsave.net> References: <1383557410-18062-1-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1383557410-18062-1-git-send-email-famz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] block: per caller dirty bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com Le Monday 04 Nov 2013 =E0 17:30:10 (+0800), Fam Zheng a =E9crit : > Previously a BlockDriverState has only one dirty bitmap, so only one > caller (e.g. a block job) can keep track of writing. This changes the > dirty bitmap to a list and creates a BdrvDirtyBitmap for each caller, t= he > lifecycle is managed with these new functions: >=20 > bdrv_create_dirty_bitmap > bdrv_release_dirty_bitmap >=20 > Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap. >=20 > In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap pointer argument > is added to these functions, since each caller has its own dirty bitmap= : >=20 > bdrv_get_dirty > bdrv_dirty_iter_init > bdrv_get_dirty_count >=20 > bdrv_set_dirty and bdrv_reset_dirty prototypes are unchanged but will > internally walk the list of all dirty bitmaps and set them one by one. >=20 > Signed-off-by: Fam Zheng >=20 > --- > v2: Added BdrvDirtyBitmap [Paolo] >=20 > Signed-off-by: Fam Zheng > --- > block-migration.c | 22 +++++++++---- > block.c | 81 ++++++++++++++++++++++++++++-----------= -------- > block/mirror.c | 23 ++++++++------ > block/qapi.c | 8 ----- > include/block/block.h | 11 ++++--- > include/block/block_int.h | 2 +- > 6 files changed, 85 insertions(+), 62 deletions(-) >=20 > diff --git a/block-migration.c b/block-migration.c > index daf9ec1..8f4e826 100644 > --- a/block-migration.c > +++ b/block-migration.c > @@ -58,6 +58,7 @@ typedef struct BlkMigDevState { > /* Protected by block migration lock. */ > unsigned long *aio_bitmap; > int64_t completed_sectors; > + BdrvDirtyBitmap *dirty_bitmap; > } BlkMigDevState; > =20 > typedef struct BlkMigBlock { > @@ -309,12 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkM= igDevState *bmds) > =20 > /* Called with iothread lock taken. */ > =20 > -static void set_dirty_tracking(int enable) > +static void set_dirty_tracking(void) > { > BlkMigDevState *bmds; > =20 > QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { > - bdrv_set_dirty_tracking(bmds->bs, enable ? BLOCK_SIZE : 0); > + bmds->dirty_bitmap =3D bdrv_create_dirty_bitmap(bmds->bs, BLOC= K_SIZE); > + } > +} > + > +static void unset_dirty_tracking(void) > +{ > + BlkMigDevState *bmds; > + > + QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { > + bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap); > } > } > =20 > @@ -432,7 +442,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMi= gDevState *bmds, > } else { > blk_mig_unlock(); > } > - if (bdrv_get_dirty(bmds->bs, sector)) { > + if (bdrv_get_dirty(bmds->bs, bmds->dirty_bitmap, sector)) { > =20 > if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK)= { > nr_sectors =3D total_sectors - sector; > @@ -554,7 +564,7 @@ static int64_t get_remaining_dirty(void) > int64_t dirty =3D 0; > =20 > QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { > - dirty +=3D bdrv_get_dirty_count(bmds->bs); > + dirty +=3D bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap); > } > =20 > return dirty << BDRV_SECTOR_BITS; > @@ -569,7 +579,7 @@ static void blk_mig_cleanup(void) > =20 > bdrv_drain_all(); > =20 > - set_dirty_tracking(0); > + unset_dirty_tracking(); > =20 > blk_mig_lock(); > while ((bmds =3D QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) !=3D = NULL) { > @@ -604,7 +614,7 @@ static int block_save_setup(QEMUFile *f, void *opaq= ue) > init_blk_migration(f); > =20 > /* start track dirty blocks */ > - set_dirty_tracking(1); > + set_dirty_tracking(); > qemu_mutex_unlock_iothread(); > =20 > ret =3D flush_blks(f); > diff --git a/block.c b/block.c > index 58efb5b..ef7a55f 100644 > --- a/block.c > +++ b/block.c > @@ -49,6 +49,11 @@ > #include > #endif > =20 > +typedef struct BdrvDirtyBitmap { > + HBitmap *bitmap; > + QLIST_ENTRY(BdrvDirtyBitmap) list; > +} BdrvDirtyBitmap; > + > #define NOT_DONE 0x7fffffff /* used while emulated sync operation in p= rogress */ > =20 > typedef enum { > @@ -323,6 +328,7 @@ BlockDriverState *bdrv_new(const char *device_name) > BlockDriverState *bs; > =20 > bs =3D g_malloc0(sizeof(BlockDriverState)); > + QLIST_INIT(&bs->dirty_bitmaps); > pstrcpy(bs->device_name, sizeof(bs->device_name), device_name); > if (device_name[0] !=3D '\0') { > QTAILQ_INSERT_TAIL(&bdrv_states, bs, list); > @@ -1615,7 +1621,7 @@ static void bdrv_move_feature_fields(BlockDriverS= tate *bs_dest, > bs_dest->iostatus =3D bs_src->iostatus; > =20 > /* dirty bitmap */ > - bs_dest->dirty_bitmap =3D bs_src->dirty_bitmap; > + bs_dest->dirty_bitmaps =3D bs_src->dirty_bitmaps; > =20 > /* reference count */ > bs_dest->refcnt =3D bs_src->refcnt; > @@ -1648,7 +1654,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDri= verState *bs_old) > =20 > /* bs_new must be anonymous and shouldn't have anything fancy enab= led */ > assert(bs_new->device_name[0] =3D=3D '\0'); > - assert(bs_new->dirty_bitmap =3D=3D NULL); > + assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); > assert(bs_new->job =3D=3D NULL); > assert(bs_new->dev =3D=3D NULL); > assert(bs_new->in_use =3D=3D 0); > @@ -1709,6 +1715,7 @@ static void bdrv_delete(BlockDriverState *bs) > assert(!bs->job); > assert(!bs->in_use); > assert(!bs->refcnt); > + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); > =20 > bdrv_close(bs); > =20 > @@ -2785,9 +2792,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDr= iverState *bs, > ret =3D bdrv_co_flush(bs); > } > =20 > - if (bs->dirty_bitmap) { > bdrv_set_dirty(bs, sector_num, nb_sectors); > - } > =20 > if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { > bs->wr_highest_sector =3D sector_num + nb_sectors - 1; > @@ -3323,7 +3328,7 @@ int bdrv_write_compressed(BlockDriverState *bs, i= nt64_t sector_num, > if (bdrv_check_request(bs, sector_num, nb_sectors)) > return -EIO; > =20 > - assert(!bs->dirty_bitmap); > + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); > =20 > return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors)= ; > } > @@ -4183,9 +4188,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState= *bs, int64_t sector_num, > return -EROFS; > } > =20 > - if (bs->dirty_bitmap) { > - bdrv_reset_dirty(bs, sector_num, nb_sectors); > - } > + bdrv_reset_dirty(bs, sector_num, nb_sectors); > =20 > /* Do nothing if disabled. */ > if (!(bs->open_flags & BDRV_O_UNMAP)) { > @@ -4347,58 +4350,70 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs,= QEMUIOVector *qiov) > return true; > } > =20 > -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity) > +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int gr= anularity) > { > int64_t bitmap_size; > + BdrvDirtyBitmap *bitmap; > =20 > assert((granularity & (granularity - 1)) =3D=3D 0); > =20 > - if (granularity) { > - granularity >>=3D BDRV_SECTOR_BITS; > - assert(!bs->dirty_bitmap); > - bitmap_size =3D (bdrv_getlength(bs) >> BDRV_SECTOR_BITS); > - bs->dirty_bitmap =3D hbitmap_alloc(bitmap_size, ffs(granularit= y) - 1); > - } else { > - if (bs->dirty_bitmap) { > - hbitmap_free(bs->dirty_bitmap); > - bs->dirty_bitmap =3D NULL; > + granularity >>=3D BDRV_SECTOR_BITS; > + assert(granularity); > + bitmap_size =3D (bdrv_getlength(bs) >> BDRV_SECTOR_BITS); > + bitmap =3D g_malloc0(sizeof(BdrvDirtyBitmap)); > + bitmap->bitmap =3D hbitmap_alloc(bitmap_size, ffs(granularity) - 1= ); > + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); > + return bitmap; > +} > + > +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) { > + QLIST_REMOVE(bitmap, list); > + hbitmap_free(bitmap->bitmap); > + g_free(bitmap); > + return; > } > } > } > =20 > -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector) > +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int6= 4_t sector) > { > - if (bs->dirty_bitmap) { > - return hbitmap_get(bs->dirty_bitmap, sector); > + if (bitmap) { > + return hbitmap_get(bitmap->bitmap, sector); Is see some if (bitmap) here but nowhere some code resetting the various dirty_bitmap fields to NULL. Is it missing ? Best regards Beno=EEt > } else { > return 0; > } > } > =20 > -void bdrv_dirty_iter_init(BlockDriverState *bs, HBitmapIter *hbi) > +void bdrv_dirty_iter_init(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, HBitmapIter *hbi) > { > - hbitmap_iter_init(hbi, bs->dirty_bitmap, 0); > + hbitmap_iter_init(hbi, bitmap->bitmap, 0); > } > =20 > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, > int nr_sectors) > { > - hbitmap_set(bs->dirty_bitmap, cur_sector, nr_sectors); > + BdrvDirtyBitmap *bitmap; > + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors); > + } > } > =20 > -void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, > - int nr_sectors) > +void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr= _sectors) > { > - hbitmap_reset(bs->dirty_bitmap, cur_sector, nr_sectors); > + BdrvDirtyBitmap *bitmap; > + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { > + hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors); > + } > } > =20 > -int64_t bdrv_get_dirty_count(BlockDriverState *bs) > +int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bi= tmap) > { > - if (bs->dirty_bitmap) { > - return hbitmap_count(bs->dirty_bitmap); > - } else { > - return 0; > - } > + return hbitmap_count(bitmap->bitmap); > } > =20 > /* Get a reference to bs */ > diff --git a/block/mirror.c b/block/mirror.c > index 7b95acf..6dc27ad 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -39,6 +39,7 @@ typedef struct MirrorBlockJob { > int64_t granularity; > size_t buf_size; > unsigned long *cow_bitmap; > + BdrvDirtyBitmap *dirty_bitmap; > HBitmapIter hbi; > uint8_t *buf; > QSIMPLEQ_HEAD(, MirrorBuffer) buf_free; > @@ -145,9 +146,10 @@ static void coroutine_fn mirror_iteration(MirrorBl= ockJob *s) > =20 > s->sector_num =3D hbitmap_iter_next(&s->hbi); > if (s->sector_num < 0) { > - bdrv_dirty_iter_init(source, &s->hbi); > + bdrv_dirty_iter_init(source, s->dirty_bitmap, &s->hbi); > s->sector_num =3D hbitmap_iter_next(&s->hbi); > - trace_mirror_restart_iter(s, bdrv_get_dirty_count(source)); > + trace_mirror_restart_iter(s, > + bdrv_get_dirty_count(source, s->dirt= y_bitmap)); > assert(s->sector_num >=3D 0); > } > =20 > @@ -183,7 +185,7 @@ static void coroutine_fn mirror_iteration(MirrorBlo= ckJob *s) > do { > int added_sectors, added_chunks; > =20 > - if (!bdrv_get_dirty(source, next_sector) || > + if (!bdrv_get_dirty(source, s->dirty_bitmap, next_sector) || > test_bit(next_chunk, s->in_flight_bitmap)) { > assert(nb_sectors > 0); > break; > @@ -249,7 +251,8 @@ static void coroutine_fn mirror_iteration(MirrorBlo= ckJob *s) > /* Advance the HBitmapIter in parallel, so that we do not exam= ine > * the same sector twice. > */ > - if (next_sector > hbitmap_next_sector && bdrv_get_dirty(source= , next_sector)) { > + if (next_sector > hbitmap_next_sector > + && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) { > hbitmap_next_sector =3D hbitmap_iter_next(&s->hbi); > } > =20 > @@ -355,7 +358,7 @@ static void coroutine_fn mirror_run(void *opaque) > } > } > =20 > - bdrv_dirty_iter_init(bs, &s->hbi); > + bdrv_dirty_iter_init(bs, s->dirty_bitmap, &s->hbi); > last_pause_ns =3D qemu_clock_get_ns(QEMU_CLOCK_REALTIME); > for (;;) { > uint64_t delay_ns; > @@ -367,7 +370,7 @@ static void coroutine_fn mirror_run(void *opaque) > goto immediate_exit; > } > =20 > - cnt =3D bdrv_get_dirty_count(bs); > + cnt =3D bdrv_get_dirty_count(bs, s->dirty_bitmap); > =20 > /* Note that even when no rate limit is applied we need to yie= ld > * periodically with no pending I/O so that qemu_aio_flush() r= eturns. > @@ -409,7 +412,7 @@ static void coroutine_fn mirror_run(void *opaque) > =20 > should_complete =3D s->should_complete || > block_job_is_cancelled(&s->common); > - cnt =3D bdrv_get_dirty_count(bs); > + cnt =3D bdrv_get_dirty_count(bs, s->dirty_bitmap); > } > } > =20 > @@ -424,7 +427,7 @@ static void coroutine_fn mirror_run(void *opaque) > */ > trace_mirror_before_drain(s, cnt); > bdrv_drain_all(); > - cnt =3D bdrv_get_dirty_count(bs); > + cnt =3D bdrv_get_dirty_count(bs, s->dirty_bitmap); > } > =20 > ret =3D 0; > @@ -471,7 +474,7 @@ immediate_exit: > qemu_vfree(s->buf); > g_free(s->cow_bitmap); > g_free(s->in_flight_bitmap); > - bdrv_set_dirty_tracking(bs, 0); > + bdrv_release_dirty_bitmap(bs, s->dirty_bitmap); > bdrv_iostatus_disable(s->target); > if (s->should_complete && ret =3D=3D 0) { > if (bdrv_get_flags(s->target) !=3D bdrv_get_flags(s->common.bs= )) { > @@ -575,7 +578,7 @@ void mirror_start(BlockDriverState *bs, BlockDriver= State *target, > s->granularity =3D granularity; > s->buf_size =3D MAX(buf_size, granularity); > =20 > - bdrv_set_dirty_tracking(bs, granularity); > + s->dirty_bitmap =3D bdrv_create_dirty_bitmap(bs, granularity); > bdrv_set_enable_write_cache(s->target, true); > bdrv_set_on_error(s->target, on_target_error, on_target_error); > bdrv_iostatus_enable(s->target); > diff --git a/block/qapi.c b/block/qapi.c > index 5880b3e..6b0cdcf 100644 > --- a/block/qapi.c > +++ b/block/qapi.c > @@ -204,14 +204,6 @@ void bdrv_query_info(BlockDriverState *bs, > info->io_status =3D bs->iostatus; > } > =20 > - if (bs->dirty_bitmap) { > - info->has_dirty =3D true; > - info->dirty =3D g_malloc0(sizeof(*info->dirty)); > - info->dirty->count =3D bdrv_get_dirty_count(bs) * BDRV_SECTOR_= SIZE; > - info->dirty->granularity =3D > - ((int64_t) BDRV_SECTOR_SIZE << hbitmap_granularity(bs->dirty_= bitmap)); > - } > - > if (bs->drv) { > info->has_inserted =3D true; > info->inserted =3D g_malloc0(sizeof(*info->inserted)); > diff --git a/include/block/block.h b/include/block/block.h > index 3560deb..06f424c 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -388,12 +388,15 @@ void *qemu_blockalign(BlockDriverState *bs, size_= t size); > bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov); > =20 > struct HBitmapIter; > -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity); > -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector); > +typedef struct BdrvDirtyBitmap BdrvDirtyBitmap; > +BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int gr= anularity); > +void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *= bitmap); > +int bdrv_get_dirty(BlockDriverState *bs, BdrvDirtyBitmap *bitmap, int6= 4_t sector); > void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, int nr_s= ectors); > void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector, int nr= _sectors); > -void bdrv_dirty_iter_init(BlockDriverState *bs, struct HBitmapIter *hb= i); > -int64_t bdrv_get_dirty_count(BlockDriverState *bs); > +void bdrv_dirty_iter_init(BlockDriverState *bs, > + BdrvDirtyBitmap *bitmap, struct HBitmapIter = *hbi); > +int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap *bi= tmap); > =20 > void bdrv_enable_copy_on_read(BlockDriverState *bs); > void bdrv_disable_copy_on_read(BlockDriverState *bs); > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 1666066..3f2bee1 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -301,7 +301,7 @@ struct BlockDriverState { > bool iostatus_enabled; > BlockDeviceIoStatus iostatus; > char device_name[32]; > - HBitmap *dirty_bitmap; > + QLIST_HEAD(, BdrvDirtyBitmap) dirty_bitmaps; > int refcnt; > int in_use; /* users other than guest access, eg. block migration = */ > QTAILQ_ENTRY(BlockDriverState) list; > --=20 > 1.8.3.1 >=20 >=20