qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qmp: transaction support fo


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable
Date: Fri, 19 Jan 2018 19:28:05 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2


On 01/17/2018 10:06 AM, Vladimir Sementsov-Ogievskiy wrote:
> 16.01.2018 15:54, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   qapi/transaction.json |  4 +++
>>   blockdev.c            | 79
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/qapi/transaction.json b/qapi/transaction.json
>> index bd312792da..b643d848f8 100644
>> --- a/qapi/transaction.json
>> +++ b/qapi/transaction.json
>> @@ -46,6 +46,8 @@
>>   # - @abort: since 1.6
>>   # - @block-dirty-bitmap-add: since 2.5
>>   # - @block-dirty-bitmap-clear: since 2.5
>> +# - @block-dirty-bitmap-enable: since 2.12
>> +# - @block-dirty-bitmap-disable: since 2.12
>>   # - @blockdev-backup: since 2.3
>>   # - @blockdev-snapshot: since 2.5
>>   # - @blockdev-snapshot-internal-sync: since 1.7
>> @@ -59,6 +61,8 @@
>>          'abort': 'Abort',
>>          'block-dirty-bitmap-add': 'BlockDirtyBitmapAdd',
>>          'block-dirty-bitmap-clear': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-enable': 'BlockDirtyBitmap',
>> +       'block-dirty-bitmap-disable': 'BlockDirtyBitmap',
>>          'blockdev-backup': 'BlockdevBackup',
>>          'blockdev-snapshot': 'BlockdevSnapshot',
>>          'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal',
>> diff --git a/blockdev.c b/blockdev.c
>> index 997a6f514c..d338363d78 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -1962,6 +1962,7 @@ typedef struct BlockDirtyBitmapState {
>>       AioContext *aio_context;
>>       HBitmap *backup;
>>       bool prepared;
>> +    bool was_enabled;
>>   } BlockDirtyBitmapState;
>>     static void block_dirty_bitmap_add_prepare(BlkActionState *common,
>> @@ -2069,6 +2070,74 @@ static void
>> block_dirty_bitmap_clear_clean(BlkActionState *common)
>>       }
>>   }
>>   +static void block_dirty_bitmap_enable_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_enable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +                                              action->name,
>> +                                              &state->bs,
>> +                                              errp);
>> +    if (!state->bitmap) {
>> +        return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_enable_dirty_bitmap(state->bitmap);
>> +}
>> +
>> +static void block_dirty_bitmap_enable_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (!state->was_enabled) {
>> +        bdrv_disable_dirty_bitmap(state->bitmap);
>> +    }
>> +}
>> +
>> +static void block_dirty_bitmap_disable_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_disable.data;
>> +    state->bitmap = block_dirty_bitmap_lookup(action->node,
>> +                                              action->name,
>> +                                              &state->bs,
>> +                                              errp);
>> +    if (!state->bitmap) {

&state->bs should be NULL if we're not going to use it. If we're going
to use it, we should check the error condition here because it might fail.

>> +        return;
>> +    }
>> +
>> +    state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap);
>> +    bdrv_disable_dirty_bitmap(state->bitmap);
> 
> hm. actually, I just make it like _clear is made. But now I doubt,
> why do we need disable here? we can disable in commit, and do not need
> state->was_enabled..
> 

You need to make sure that there is no way for bdrv_disable_dirty_bitmap
to fail, so you need to add that frozen check in. Then you're alright,
and you can move the actual disable portion to the commit, and get rid
of the undo call.

Or, we can even do this kind of trick to remove the redundant error
checking:

void qmp_block_dirty_bitmap_enable(const char *node, const char *name,
                                   Error **errp)
{
    BlockDirtyBitmap data = {
        .node = node,
        .name = name
    };
    TransactionAction action = {
        .type = TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE,
        .u.block_dirty_bitmap_enable.data = &data,
    };
    blockdev_do_action(&action, errp);
}

static void block_dirty_bitmap_enable_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_enable.data;
    state->bitmap = block_dirty_bitmap_lookup(action->node,
                                              action->name,
                                              NULL,
                                              errp);
    if (!state->bitmap) {
        return;
    }

    if (bdrv_dirty_bitmap_frozen(state->bitmap)) {
        error_setg(errp, "Bitmap '%s' is currently frozen and cannot be
                         " enabled", name);
        return;
    }
}

static void block_dirty_bitmap_enable_commit(BlkActionState *common,
                                              Error **errp)
{
    ...blah...
    bdrv_dirty_bitmap_enable(state->bitmap);
}

You can do the same for disable.

> and for _clear, we not to clear bitmap in commit? and then, we do not need
> bdrv_undo_clear_dirty_bitmap at all?
> 

Yeah, you can apply the same technique here too. You can consolidate the
error checking in .prepare(), then recycle that error checking for the
standalone QMP command.

Then we can be assured we're not missing a safeguard in the transaction
version, and it should be safe to move the action to the commit phase,
because _enable, _disable, and _clear all cannot actually fail.

Then you can get rid of the state->backup state, too, and the extra
argument to bdrv_clear_dirty_bitmap.

Nice!

> or what do I miss? if nothing, I'll respin the series including _clear
> refactoring.
> 

Just pay heed to the error checking in .prepare(), and make sure the
error checking is identical between the QMP and transactional versions.

>> +}
>> +
>> +static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>> +{
>> +    BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState,
>> +                                             common, common);
>> +
>> +    if (state->was_enabled) {
>> +        bdrv_enable_dirty_bitmap(state->bitmap);
>> +    }
>> +}
>> +
>>   static void abort_prepare(BlkActionState *common, Error **errp)
>>   {
>>       error_setg(errp, "Transaction aborted using Abort action");
>> @@ -2130,6 +2199,16 @@ static const BlkActionOps actions[] = {
>>           .commit = block_dirty_bitmap_clear_commit,
>>           .abort = block_dirty_bitmap_clear_abort,
>>           .clean = block_dirty_bitmap_clear_clean,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_ENABLE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_enable_prepare,
>> +        .abort = block_dirty_bitmap_enable_abort,
>> +    },
>> +    [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_DISABLE] = {
>> +        .instance_size = sizeof(BlockDirtyBitmapState),
>> +        .prepare = block_dirty_bitmap_disable_prepare,
>> +        .abort = block_dirty_bitmap_disable_abort,
>>       }
>>   };
>>   
> 
> 

Thanks!



reply via email to

[Prev in Thread] Current Thread [Next in Thread]