From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdE8V-00027r-IZ for qemu-devel@nongnu.org; Mon, 04 Nov 2013 01:59:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VdE8P-00034N-DB for qemu-devel@nongnu.org; Mon, 04 Nov 2013 01:59:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:62847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VdE8P-00034C-4g for qemu-devel@nongnu.org; Mon, 04 Nov 2013 01:59:37 -0500 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA46xZPT016804 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 4 Nov 2013 01:59:35 -0500 Message-ID: <527745D2.7020606@redhat.com> Date: Mon, 04 Nov 2013 14:59:30 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1383116892-11047-1-git-send-email-famz@redhat.com> <1383116892-11047-4-git-send-email-famz@redhat.com> <5270BA10.2090904@redhat.com> In-Reply-To: <5270BA10.2090904@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 10/30/2013 03:49 PM, Paolo Bonzini wrote: > Il 30/10/2013 08:08, Fam Zheng ha scritto: >> 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 one HBitmap for each caller, the >> lifecycle is managed with these new functions: >> >> bdrv_create_dirty_bitmap >> bdrv_release_dirty_bitmap >> >> In place of this one: >> >> bdrv_set_dirty_tracking >> >> An HBitmap pointer argument is added to these functions, since each >> caller has its own dirty bitmap: >> >> bdrv_get_dirty >> bdrv_dirty_iter_init >> bdrv_get_dirty_count >> >> While bdrv_set_dirty and bdrv_reset_dirty prototypes unchanged but >> internally walk the list of all dirty bitmaps and set them one by one. >> >> Signed-off-by: Fam Zheng >> --- >> block-migration.c | 22 ++++++++++---- >> block.c | 74 ++++++++++++++++++++++++++--------------------- >> block/mirror.c | 23 ++++++++------- >> block/qapi.c | 8 ----- >> include/block/block.h | 11 ++++--- >> include/block/block_int.h | 2 +- >> 6 files changed, 78 insertions(+), 62 deletions(-) >> >> diff --git a/block-migration.c b/block-migration.c >> index daf9ec1..08df056 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; >> + HBitmap *dirty_bitmap; >> } BlkMigDevState; >> >> typedef struct BlkMigBlock { >> @@ -309,12 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds) >> >> /* Called with iothread lock taken. */ >> >> -static void set_dirty_tracking(int enable) >> +static void set_dirty_tracking(void) >> { >> BlkMigDevState *bmds; >> >> QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { >> - bdrv_set_dirty_tracking(bmds->bs, enable ? BLOCK_SIZE : 0); >> + bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_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); >> } >> } >> >> @@ -432,7 +442,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, >> } else { >> blk_mig_unlock(); >> } >> - if (bdrv_get_dirty(bmds->bs, sector)) { >> + if (bdrv_get_dirty(bmds->bs, bmds->dirty_bitmap, sector)) { >> >> if (total_sectors - sector < BDRV_SECTORS_PER_DIRTY_CHUNK) { >> nr_sectors = total_sectors - sector; >> @@ -554,7 +564,7 @@ static int64_t get_remaining_dirty(void) >> int64_t dirty = 0; >> >> QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) { >> - dirty += bdrv_get_dirty_count(bmds->bs); >> + dirty += bdrv_get_dirty_count(bmds->bs, bmds->dirty_bitmap); >> } >> >> return dirty << BDRV_SECTOR_BITS; >> @@ -569,7 +579,7 @@ static void blk_mig_cleanup(void) >> >> bdrv_drain_all(); >> >> - set_dirty_tracking(0); >> + unset_dirty_tracking(); >> >> blk_mig_lock(); >> while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) { >> @@ -604,7 +614,7 @@ static int block_save_setup(QEMUFile *f, void *opaque) >> init_blk_migration(f); >> >> /* start track dirty blocks */ >> - set_dirty_tracking(1); >> + set_dirty_tracking(); >> qemu_mutex_unlock_iothread(); >> >> ret = flush_blks(f); >> diff --git a/block.c b/block.c >> index fd05a80..9975428 100644 >> --- a/block.c >> +++ b/block.c >> @@ -323,6 +323,7 @@ BlockDriverState *bdrv_new(const char *device_name) >> BlockDriverState *bs; >> >> bs = g_malloc0(sizeof(BlockDriverState)); >> + QLIST_INIT(&bs->dirty_bitmaps); >> pstrcpy(bs->device_name, sizeof(bs->device_name), device_name); >> if (device_name[0] != '\0') { >> QTAILQ_INSERT_TAIL(&bdrv_states, bs, list); >> @@ -1614,7 +1615,7 @@ static void bdrv_move_feature_fields(BlockDriverState *bs_dest, >> bs_dest->iostatus = bs_src->iostatus; >> >> /* dirty bitmap */ >> - bs_dest->dirty_bitmap = bs_src->dirty_bitmap; >> + bs_dest->dirty_bitmaps = bs_src->dirty_bitmaps; >> >> /* reference count */ >> bs_dest->refcnt = bs_src->refcnt; >> @@ -1647,7 +1648,7 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old) >> >> /* bs_new must be anonymous and shouldn't have anything fancy enabled */ >> assert(bs_new->device_name[0] == '\0'); >> - assert(bs_new->dirty_bitmap == NULL); >> + assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); >> assert(bs_new->job == NULL); >> assert(bs_new->dev == NULL); >> assert(bs_new->in_use == 0); >> @@ -1708,6 +1709,7 @@ static void bdrv_delete(BlockDriverState *bs) >> assert(!bs->job); >> assert(!bs->in_use); >> assert(!bs->refcnt); >> + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> >> bdrv_close(bs); >> >> @@ -2784,9 +2786,7 @@ static int coroutine_fn bdrv_co_do_writev(BlockDriverState *bs, >> ret = bdrv_co_flush(bs); >> } >> >> - if (bs->dirty_bitmap) { >> bdrv_set_dirty(bs, sector_num, nb_sectors); >> - } >> >> if (bs->wr_highest_sector < sector_num + nb_sectors - 1) { >> bs->wr_highest_sector = sector_num + nb_sectors - 1; >> @@ -3321,7 +3321,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num, >> if (bdrv_check_request(bs, sector_num, nb_sectors)) >> return -EIO; >> >> - assert(!bs->dirty_bitmap); >> + assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> >> return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors); >> } >> @@ -4181,9 +4181,7 @@ int coroutine_fn bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, >> return -EROFS; >> } >> >> - if (bs->dirty_bitmap) { >> - bdrv_reset_dirty(bs, sector_num, nb_sectors); >> - } >> + bdrv_reset_dirty(bs, sector_num, nb_sectors); >> >> /* Do nothing if disabled. */ >> if (!(bs->open_flags & BDRV_O_UNMAP)) { >> @@ -4345,58 +4343,68 @@ bool bdrv_qiov_is_aligned(BlockDriverState *bs, QEMUIOVector *qiov) >> return true; >> } >> >> -void bdrv_set_dirty_tracking(BlockDriverState *bs, int granularity) >> +HBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity) >> { >> int64_t bitmap_size; >> + HBitmap *bitmap; >> >> assert((granularity & (granularity - 1)) == 0); >> >> - if (granularity) { >> - granularity >>= BDRV_SECTOR_BITS; >> - assert(!bs->dirty_bitmap); >> - bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS); >> - bs->dirty_bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); >> - } else { >> - if (bs->dirty_bitmap) { >> - hbitmap_free(bs->dirty_bitmap); >> - bs->dirty_bitmap = NULL; >> + granularity >>= BDRV_SECTOR_BITS; >> + assert(granularity); >> + bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS); >> + bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1); >> + QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list); >> + return bitmap; >> +} >> + >> +void bdrv_release_dirty_bitmap(BlockDriverState *bs, HBitmap *bitmap) >> +{ >> + HBitmap *bm, *next; >> + QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { >> + if (bm == bitmap) { >> + QLIST_REMOVE(bitmap, list); >> + hbitmap_free(bitmap); >> + return; >> } >> } >> } >> >> -int bdrv_get_dirty(BlockDriverState *bs, int64_t sector) >> +int bdrv_get_dirty(BlockDriverState *bs, HBitmap *bitmap, int64_t sector) >> { >> - if (bs->dirty_bitmap) { >> - return hbitmap_get(bs->dirty_bitmap, sector); >> + if (bitmap) { >> + return hbitmap_get(bitmap, sector); >> } else { >> return 0; >> } >> } >> >> -void bdrv_dirty_iter_init(BlockDriverState *bs, HBitmapIter *hbi) >> +void bdrv_dirty_iter_init(BlockDriverState *bs, >> + HBitmap *bitmap, HBitmapIter *hbi) >> { >> - hbitmap_iter_init(hbi, bs->dirty_bitmap, 0); >> + hbitmap_iter_init(hbi, bitmap, 0); >> } >> >> void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector, >> int nr_sectors) >> { >> - hbitmap_set(bs->dirty_bitmap, cur_sector, nr_sectors); >> + HBitmap *bitmap; >> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + hbitmap_set(bitmap, cur_sector, nr_sectors); >> + } >> } >> >> -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); >> + HBitmap *bitmap; >> + QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { >> + hbitmap_reset(bitmap, cur_sector, nr_sectors); >> + } >> } > I think callers outside block.c should only call > hbitmap_set/hbitmap_reset; resetting is typically done before processing > sectors and setting after an error (both of which happen privately to > each task). > > Thus you probably should add a fourth patch which makes > bdrv_(re)set_dirty static and remove > bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count. I like the idea of adding a wrapper struct (will be BdrvDirtyBitmap) to HBitmap so that patch 1 is not needed, and HBitmap becomes (almost) internal to block.c. But I'm not sure removing bdrv_get_dirty/bdrv_dirty_iter_init/bdrv_get_dirty_count is good, as we are exposing BdrvDirtyBitmap, we should also provide operations on it, instead of let callers to handle HBitmap pointer inside. In other words, I prefer to define BdrvDirtyBitmap structure in block.c and only put a type declaration in header. Fam