Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[Vladimir Sementsov-Ogievskiy]

20.01.2018 03:28, John Snow wrote:


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 
---
   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,
+  >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,
+  >bs,
+  errp);
+    if (!state->bitmap) {

>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.


yes, it should be checked. It think it should be checked in other 
commands too.





+    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 = ,
 };
 blockdev_do_action(, errp);
}

static void block_dirty_bitmap_enable_prepare(BlkActionState *common,
  

Re: [Qemu-block] [Qemu-devel] [PATCH v2 4/6] qmp: transaction support for block-dirty-bitmap-enable/disable

[John Snow]

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 
>> ---
>>   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,
>> +  >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,
>> +  >bs,
>> +  errp);
>> +    if (!state->bitmap) {

>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,