[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bi
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH 3/4] qapi: implement block-dirty-bitmap-remove transaction action |
Date: |
Mon, 10 Jun 2019 09:39:34 +0000 |
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 <address@hidden>
>> ---
>> 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
Re: [Qemu-block] [Qemu-devel] [PATCH 0/4] qapi: block-dirty-bitmap-remove transaction action, John Snow, 2019/06/07