08.06.2019 1:57, John Snow wrote: > > > On 6/3/19 8:00 AM, Vladimir Sementsov-Ogievskiy 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 <vsement...@virtuozzo.com> >> --- >> qapi/transaction.json | 2 ++ >> blockdev.c | 74 +++++++++++++++++++++++++++++++++++++++---- >> 2 files changed, 70 insertions(+), 6 deletions(-) >> >> diff --git a/qapi/transaction.json b/qapi/transaction.json >> index 95edb78227..da95b804aa 100644 >> --- a/qapi/transaction.json >> +++ b/qapi/transaction.json >> @@ -45,6 +45,7 @@ >> # >> # - @abort: since 1.6 >> # - @block-dirty-bitmap-add: since 2.5 >> +# - @block-dirty-bitmap-remove: since 4.1 >> # - @block-dirty-bitmap-clear: since 2.5 >> # - @block-dirty-bitmap-enable: since 4.0 >> # - @block-dirty-bitmap-disable: since 4.0 >> @@ -61,6 +62,7 @@ >> 'data': { >> 'abort': 'Abort', >> 'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd', >> + 'block-dirty-bitmap-remove': 'BlockDirtyBitmap', >> 'block-dirty-bitmap-clear': 'BlockDirtyBitmap', >> 'block-dirty-bitmap-enable': 'BlockDirtyBitmap', >> 'block-dirty-bitmap-disable': 'BlockDirtyBitmap', >> diff --git a/blockdev.c b/blockdev.c >> index 5b3eef0d3e..0d9aa7f0a1 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2135,6 +2135,46 @@ 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_hide(state->bitmap); >> + } >> +} >> + >> +static void block_dirty_bitmap_remove_abort(BlkActionState *common) >> +{ >> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + >> + bdrv_dirty_bitmap_unhide(state->bitmap); >> +} >> + >> +static void block_dirty_bitmap_remove_commit(BlkActionState *common) >> +{ >> + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, >> + common, common); >> + >> + bdrv_release_dirty_bitmap(state->bs, state->bitmap); >> +} >> + >> static void abort_prepare(BlkActionState *common, Error **errp) >> { >> error_setg(errp, "Transaction aborted using Abort action"); >> @@ -2212,6 +2252,12 @@ static const BlkActionOps actions[] = { >> .commit = block_dirty_bitmap_free_backup, >> .abort = block_dirty_bitmap_restore, >> }, >> + [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_REMOVE] = { >> + .instance_size = sizeof(BlockDirtyBitmapState), >> + .prepare = block_dirty_bitmap_remove_prepare, >> + .commit = block_dirty_bitmap_remove_commit, >> + .abort = block_dirty_bitmap_remove_abort, >> + }, >> /* Where are transactions for MIRROR, COMMIT and STREAM? >> * Although these blockjobs use transaction callbacks like the backup >> job, >> * these jobs do not necessarily adhere to transaction semantics. >> @@ -2870,20 +2916,21 @@ void qmp_block_dirty_bitmap_add(const char *node, >> const char *name, >> bdrv_dirty_bitmap_set_persistence(bitmap, persistent); >> } >> >> -void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> - Error **errp) >> +static BdrvDirtyBitmap *do_block_dirty_bitmap_remove( >> + const char *node, const char *name, bool release, >> + BlockDriverState **bitmap_bs, Error **errp) > > Hm, why does the hide feature need to copy the persistent bit when we're > removing it here anyway? > > If release is false, we still call bdrv_remove_persistent_dirty_bitmap, > yeah? > > And when we go to undo it, we won't have undone the persistent removal, > right?
Hm, good question. I remember there was bad thing on storing persistent bitmap, as this code is not prepared for unnamed persistent bitmaps. Aha, I was wrong in my previous answer, it is valid to go to storing during transaction, and this is exactly reopen to RO. So we must drop persistence. > >> { >> BlockDriverState *bs; >> BdrvDirtyBitmap *bitmap; >> >> bitmap = block_dirty_bitmap_lookup(node, name, &bs, errp); >> if (!bitmap || !bs) { >> - return; >> + return NULL; >> } >> >> if (bdrv_dirty_bitmap_check(bitmap, BDRV_BITMAP_BUSY | BDRV_BITMAP_RO, >> errp)) { >> - return; >> + return NULL; >> } >> >> if (bdrv_dirty_bitmap_get_persistence(bitmap)) { >> @@ -2893,13 +2940,28 @@ void qmp_block_dirty_bitmap_remove(const char *node, >> const char *name, >> aio_context_acquire(aio_context); >> bdrv_remove_persistent_dirty_bitmap(bs, name, &local_err); >> aio_context_release(aio_context); >> + >> if (local_err != NULL) { >> error_propagate(errp, local_err); >> - return; >> + return NULL; >> } >> } >> >> - bdrv_release_dirty_bitmap(bs, bitmap); >> + if (release) { >> + bdrv_release_dirty_bitmap(bs, bitmap); >> + } >> + >> + if (bitmap_bs) { >> + *bitmap_bs = bs; >> + } >> + >> + return bitmap; >> +} >> + >> +void qmp_block_dirty_bitmap_remove(const char *node, const char *name, >> + Error **errp) >> +{ >> + do_block_dirty_bitmap_remove(node, name, true, NULL, errp); >> } >> >> /** >> > > Seems about right otherwise, though! > -- Best regards, Vladimir