09.07.2019 1:05, John Snow wrote: > It is used to do transactional movement of the bitmap (which is > possible in conjunction with merge command). Transactional bitmap > movement is needed in scenarios with external snapshot, when we don't > want to leave copy of the bitmap in the base image. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]> > Signed-off-by: John Snow <[email protected]> > --- > block.c | 2 +- > block/dirty-bitmap.c | 15 +++---- > blockdev.c | 79 +++++++++++++++++++++++++++++++--- > include/block/dirty-bitmap.h | 2 +- > migration/block-dirty-bitmap.c | 2 +- > qapi/transaction.json | 2 + > 6 files changed, 85 insertions(+), 17 deletions(-) > > diff --git a/block.c b/block.c > index c139540f2b..5195d4b910 100644 > --- a/block.c > +++ b/block.c > @@ -5316,7 +5316,7 @@ static void coroutine_fn > bdrv_co_invalidate_cache(BlockDriverState *bs, > for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm; > bm = bdrv_dirty_bitmap_next(bs, bm)) > { > - bdrv_dirty_bitmap_set_migration(bm, false); > + bdrv_dirty_bitmap_skip_store(bm, false); > } > > ret = refresh_total_sectors(bs, bs->total_sectors); > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 95a9c2a5d8..a308e1f84b 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -48,10 +48,9 @@ struct BdrvDirtyBitmap { > bool inconsistent; /* bitmap is persistent, but inconsistent. > It cannot be used at all in any way, > except > a QMP user can remove it. */ > - bool migration; /* Bitmap is selected for migration, it > should > - not be stored on the next inactivation > - (persistent flag doesn't matter until next > - invalidation).*/ > + bool skip_store; /* We are either migrating or deleting this > + * bitmap; it should not be stored on the > next > + * inactivation. */ > QLIST_ENTRY(BdrvDirtyBitmap) list; > }; > > @@ -757,16 +756,16 @@ void bdrv_dirty_bitmap_set_inconsistent(BdrvDirtyBitmap > *bitmap) > } > > /* Called with BQL taken. */ > -void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration) > +void bdrv_dirty_bitmap_skip_store(BdrvDirtyBitmap *bitmap, bool skip) > { > qemu_mutex_lock(bitmap->mutex); > - bitmap->migration = migration; > + bitmap->skip_store = skip; > qemu_mutex_unlock(bitmap->mutex); > } > > bool bdrv_dirty_bitmap_get_persistence(BdrvDirtyBitmap *bitmap) > { > - return bitmap->persistent && !bitmap->migration; > + return bitmap->persistent && !bitmap->skip_store; > } > > bool bdrv_dirty_bitmap_inconsistent(const BdrvDirtyBitmap *bitmap) > @@ -778,7 +777,7 @@ bool bdrv_has_changed_persistent_bitmaps(BlockDriverState > *bs) > { > BdrvDirtyBitmap *bm; > QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { > - if (bm->persistent && !bm->readonly && !bm->migration) { > + if (bm->persistent && !bm->readonly && !bm->skip_store) { > return true; > } > } > diff --git a/blockdev.c b/blockdev.c > index 01248252ca..800b3dcb42 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2134,6 +2134,51 @@ static void > block_dirty_bitmap_merge_prepare(BlkActionState *common, > errp); > } > > +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( > + const char *node, const char *name, bool release, > + BlockDriverState **bitmap_bs, Error **errp); > + > +static void block_dirty_bitmap_remove_prepare(BlkActionState *common, > + Error **errp) > +{ > + BlockDirtyBitmap *action; > + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > + common, common); > + > + if (action_check_completion_mode(common, errp) < 0) { > + return; > + } > + > + action = common->action->u.block_dirty_bitmap_remove.data; > + > + state->bitmap = do_block_dirty_bitmap_remove(action->node, action->name, > + false, &state->bs, errp); > + if (state->bitmap) { > + bdrv_dirty_bitmap_skip_store(state->bitmap, true); > + bdrv_dirty_bitmap_set_busy(state->bitmap, true); > + } > +} > + > +static void block_dirty_bitmap_remove_abort(BlkActionState *common) > +{ > + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, > + common, common); > + > + if (state->bitmap) {
Hmm, interesting, I thought, abort should not be called, if prepare failed, so the following may be done unconditionally? > + bdrv_dirty_bitmap_skip_store(state->bitmap, false); > + bdrv_dirty_bitmap_set_busy(state->bitmap, false); > + } > +} > + [..] OK, I agree, good idea to reuse BUSY and migration field here and avoid new API. Now release-prepare is less "release", but I don't see any real problems with it. Maybe, we need something to be noted in docs. Hmm, as we are not in a hurry now, we may wait for Nikolay to check this on his scenarios. -- Best regards, Vladimir
