Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap
Eric, This touches qapi code, forgot to Cc you. Fam On Wed, 10/30 15:08, Fam Zheng wrote: > 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); > asser
Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap
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); > +
Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap
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 *
Re: [Qemu-devel] [PATCH 3/3] block: per caller dirty bitmap
Il 04/11/2013 07:59, Fam Zheng ha scritto: >>> >> 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. If you want to expose BdrvDirtyBitmap, having wrappers is fine. I was thinking of exposing HBitmap instead and keeping BdrvDirtyBitmap internal. Either way is fine, and as the person who writes the code you have the privilege of making the choice. :) Paolo