From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41771) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdX4b-0002FH-MC for qemu-devel@nongnu.org; Mon, 04 Nov 2013 22:13:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VdX4V-0002ww-L2 for qemu-devel@nongnu.org; Mon, 04 Nov 2013 22:12:57 -0500 Received: from mx1.redhat.com ([209.132.183.28]:19169) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdX4V-0002ws-C4 for qemu-devel@nongnu.org; Mon, 04 Nov 2013 22:12:51 -0500 Message-ID: <52786228.9050603@redhat.com> Date: Tue, 05 Nov 2013 11:12:40 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1383557410-18062-1-git-send-email-famz@redhat.com> <20131104143858.GC2980@irqsave.net> In-Reply-To: <20131104143858.GC2980@irqsave.net> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: =?ISO-8859-1?Q?Beno=EEt_Canet?= Cc: kwolf@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 11/04/2013 10:38 PM, Beno=EEt Canet wrote: > 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, = the >> lifecycle is managed with these new functions: >> >> bdrv_create_dirty_bitmap >> bdrv_release_dirty_bitmap >> >> Where BdrvDirtyBitmap is a linked list wrapper structure of HBitmap. >> >> In place of bdrv_set_dirty_tracking, a BdrvDirtyBitmap pointer argumen= t >> is added to these functions, since each caller has its own dirty bitma= p: >> >> bdrv_get_dirty >> bdrv_dirty_iter_init >> bdrv_get_dirty_count >> >> 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. >> >> Signed-off-by: Fam Zheng >> >> --- >> v2: Added BdrvDirtyBitmap [Paolo] >> >> 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(-) >> >> 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, Blk= MigDevState *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, BLO= CK_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, BlkM= igDevState *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_CHUN= K) { >> 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 *opa= que) >> 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= progress */ >> =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(BlockDriver= State *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, BlockDr= iverState *bs_old) >> =20 >> /* bs_new must be anonymous and shouldn't have anything fancy en= abled */ >> 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(BlockD= riverState *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, = int64_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_sector= s); >> } >> @@ -4183,9 +4188,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverStat= e *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 g= ranularity) >> { >> 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(granulari= ty) - 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, int= 64_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 variou= s > dirty_bitmap fields to NULL. > Is it missing ? I just kept the logic here, but callers should always pass in a non NULL=20 for now. And the dirty_bitmap fields are released by caller with=20 bdrv_release_dirty_bitmap(). Is this the answer to your question? Thanks, Fam