[Qemu-devel] [PATCH 0/2] bitmaps: merge bitmaps from different nodes

[Qemu-devel] [PATCH 0/2] bitmaps: merge bitmaps from different nodes

[Vladimir Sementsov-Ogievskiy]

Hi all!

We need to copy bitmaps to new top node on external snapshot, to
not break incremental backup chain.

The only thing to do is to allow block-dirty-bitmap-merge to work
with different nodes, here it is.

Vladimir Sementsov-Ogievskiy (2):
  qapi: support external bitmaps in block-dirty-bitmap-merge
  iotests: test external snapshot with bitmap copying

 qapi/block-core.json       | 13 +++++++---
 block/dirty-bitmap.c       |  9 ++++---
 blockdev.c                 | 46 +++++++++++++++++++++++----------
 tests/qemu-iotests/254     | 52 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/254.out | 53 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 154 insertions(+), 20 deletions(-)
 create mode 100755 tests/qemu-iotests/254
 create mode 100644 tests/qemu-iotests/254.out

-- 
2.18.0

[Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

Add new optional parameter making possible to merge bitmaps from
different nodes. It is needed to maintain external snapshots during
incremental backup chain history.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 13 ++++++++++---
 block/dirty-bitmap.c |  9 ++++++---
 blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
 3 files changed, 48 insertions(+), 20 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..933b50771a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2006,16 +2006,23 @@
 ##
 # @BlockDirtyBitmapMerge:
 #
-# @node: name of device/node which the bitmap is tracking
+# @node: name of device/node which the @target and @bitmaps bitmaps are
+#        tracking
 #
 # @target: name of the destination dirty bitmap
 #
-# @bitmaps: name(s) of the source dirty bitmap(s)
+# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
+#           since 4.1.
+#
+# @external-bitmaps: additional list of source dirty bitmaps with specified
+#                    nodes, which allows merging bitmaps between different
+#                    nodes. (Since: 4.1)
 #
 # Since: 4.0
 ##
 { 'struct': 'BlockDirtyBitmapMerge',
-  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
+  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
+            '*external-bitmaps': ['BlockDirtyBitmap'] } }
 
 ##
 # @block-dirty-bitmap-add:
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 59e6ebb861..49646a30e6 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -816,10 +816,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 {
     bool ret;
 
-    /* only bitmaps from one bds are supported */
-    assert(dest->mutex == src->mutex);
-
     qemu_mutex_lock(dest->mutex);
+    if (src->mutex != dest->mutex) {
+        qemu_mutex_lock(src->mutex);
+    }
 
     if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
         goto out;
@@ -845,4 +845,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
 
 out:
     qemu_mutex_unlock(dest->mutex);
+    if (src->mutex != dest->mutex) {
+        qemu_mutex_unlock(src->mutex);
+    }
 }
diff --git a/blockdev.c b/blockdev.c
index 79fbac8450..8d37ce5943 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2112,11 +2112,9 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
     }
 }
 
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
-                                                    const char *target,
-                                                    strList *bitmaps,
-                                                    HBitmap **backup,
-                                                    Error **errp);
+static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
+        const char *node, const char *target, strList *bitmaps,
+        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp);
 
 static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
                                              Error **errp)
@@ -2132,8 +2130,9 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
     action = common->action->u.block_dirty_bitmap_merge.data;
 
     state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
-                                                action->bitmaps, &state->backup,
-                                                errp);
+                                                action->bitmaps,
+                                                action->external_bitmaps,
+                                                &state->backup, errp);
 }
 
 static void abort_prepare(BlkActionState *common, Error **errp)
@@ -2965,15 +2964,14 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
     bdrv_disable_dirty_bitmap(bitmap);
 }
 
-static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
-                                                    const char *target,
-                                                    strList *bitmaps,
-                                                    HBitmap **backup,
-                                                    Error **errp)
+static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
+        const char *node, const char *target, strList *bitmaps,
+        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp)
 {
     BlockDriverState *bs;
     BdrvDirtyBitmap *dst, *src, *anon;
     strList *lst;
+    BlockDirtyBitmapList *ext_lst;
     Error *local_err = NULL;
 
     dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
@@ -3003,6 +3001,22 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
         }
     }
 
+    for (ext_lst = external_bitmaps; ext_lst; ext_lst = ext_lst->next) {
+        src = block_dirty_bitmap_lookup(ext_lst->value->node,
+                                        ext_lst->value->name, NULL, errp);
+        if (!src) {
+            dst = NULL;
+            goto out;
+        }
+
+        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            dst = NULL;
+            goto out;
+        }
+    }
+
     /* Merge into dst; dst is unchanged on failure. */
     bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
 
@@ -3012,9 +3026,13 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
 }
 
 void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
-                                  strList *bitmaps, Error **errp)
+                                  bool has_bitmaps, strList *bitmaps,
+                                  bool has_external_bitmaps,
+                                  BlockDirtyBitmapList *external_bitmaps,
+                                  Error **errp)
 {
-    do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
+    do_block_dirty_bitmap_merge(node, target, bitmaps, external_bitmaps, NULL,
+                                errp);
 }
 
 BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
-- 
2.18.0

[Qemu-devel] [PATCH 2/2] iotests: test external snapshot with bitmap copying

[Vladimir Sementsov-Ogievskiy]

This test shows that external snapshots and incremental backups are
friends.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/254     | 52 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/254.out | 53 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 106 insertions(+)
 create mode 100755 tests/qemu-iotests/254
 create mode 100644 tests/qemu-iotests/254.out

diff --git a/tests/qemu-iotests/254 b/tests/qemu-iotests/254
new file mode 100755
index 0000000000..15688f2c29
--- /dev/null
+++ b/tests/qemu-iotests/254
@@ -0,0 +1,52 @@
+#!/usr/bin/env python
+#
+# Test external snapshot with bitmap copying.
+#
+# Copyright (c) 2019 Virtuozzo International GmbH. All rights reserved.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import iotests
+from iotests import qemu_img_create, file_path, log
+
+disk, top = file_path('disk', 'top')
+size = 1024 * 1024
+
+qemu_img_create('-f', iotests.imgfmt, disk, str(size))
+
+vm = iotests.VM().add_drive(disk, opts='node-name=base')
+vm.launch()
+
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name='bitmap0')
+
+vm.hmp_qemu_io('drive0', 'write 0 512K')
+
+vm.qmp_log('transaction', indent=2, actions=[
+    {'type': 'blockdev-snapshot-sync',
+     'data': {'device': 'drive0', 'snapshot-file': top,
+              'snapshot-node-name': 'snap'}},
+    {'type': 'block-dirty-bitmap-add',
+     'data': {'node': 'snap', 'name': 'bitmap0'}},
+    {'type': 'block-dirty-bitmap-merge',
+     'data': {'node': 'snap', 'target': 'bitmap0', 'bitmaps': [],
+              'external-bitmaps': [{'node': 'base', 'name': 'bitmap0'}]}}
+], filters=[iotests.filter_qmp_testfiles])
+
+result = vm.qmp('query-block')['return'][0]
+log("query-block: device = {}, node-name = {}, dirty-bitmaps:".format(
+    result['device'], result['inserted']['node-name']))
+log(result['dirty-bitmaps'], indent=2)
+
+vm.shutdown()
diff --git a/tests/qemu-iotests/254.out b/tests/qemu-iotests/254.out
new file mode 100644
index 0000000000..14a9cd3d71
--- /dev/null
+++ b/tests/qemu-iotests/254.out
@@ -0,0 +1,53 @@
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0"}}
+{"return": {}}
+{
+  "execute": "transaction",
+  "arguments": {
+    "actions": [
+      {
+        "data": {
+          "device": "drive0",
+          "snapshot-file": "TEST_DIR/PID-top",
+          "snapshot-node-name": "snap"
+        },
+        "type": "blockdev-snapshot-sync"
+      },
+      {
+        "data": {
+          "name": "bitmap0",
+          "node": "snap"
+        },
+        "type": "block-dirty-bitmap-add"
+      },
+      {
+        "data": {
+          "bitmaps": [],
+          "external-bitmaps": [
+            {
+              "name": "bitmap0",
+              "node": "base"
+            }
+          ],
+          "node": "snap",
+          "target": "bitmap0"
+        },
+        "type": "block-dirty-bitmap-merge"
+      }
+    ]
+  }
+}
+{
+  "return": {}
+}
+query-block: device = drive0, node-name = snap, dirty-bitmaps:
+[
+  {
+    "busy": false,
+    "count": 524288,
+    "granularity": 65536,
+    "name": "bitmap0",
+    "persistent": false,
+    "recording": true,
+    "status": "active"
+  }
+]
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 00e474ab0a..5552d0153c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -250,3 +250,4 @@
 248 rw auto quick
 249 rw auto quick
 252 rw auto backing quick
+254 rw auto backing quick
-- 
2.18.0

Re: [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[John Snow]

On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
> Add new optional parameter making possible to merge bitmaps from
> different nodes. It is needed to maintain external snapshots during
> incremental backup chain history.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 13 ++++++++++---
>  block/dirty-bitmap.c |  9 ++++++---
>  blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>  3 files changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..933b50771a 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2006,16 +2006,23 @@
>  ##
>  # @BlockDirtyBitmapMerge:
>  #
> -# @node: name of device/node which the bitmap is tracking
> +# @node: name of device/node which the @target and @bitmaps bitmaps are
> +#        tracking
>  #
>  # @target: name of the destination dirty bitmap
>  #
> -# @bitmaps: name(s) of the source dirty bitmap(s)
> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
> +#           since 4.1.
> +#
> +# @external-bitmaps: additional list of source dirty bitmaps with specified
> +#                    nodes, which allows merging bitmaps between different
> +#                    nodes. (Since: 4.1)
>  #
>  # Since: 4.0
>  ##
>  { 'struct': 'BlockDirtyBitmapMerge',
> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>  

I guess you can specify one, or both, or maybe neither! Seems fine.

>  ##
>  # @block-dirty-bitmap-add:
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 59e6ebb861..49646a30e6 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -816,10 +816,10 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>  {
>      bool ret;
>  
> -    /* only bitmaps from one bds are supported */
> -    assert(dest->mutex == src->mutex);
> -
>      qemu_mutex_lock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_lock(src->mutex);
> +    }
>  
>      if (bdrv_dirty_bitmap_check(dest, BDRV_BITMAP_DEFAULT, errp)) {
>          goto out;
> @@ -845,4 +845,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
>  
>  out:
>      qemu_mutex_unlock(dest->mutex);
> +    if (src->mutex != dest->mutex) {
> +        qemu_mutex_unlock(src->mutex);
> +    }
>  }
> diff --git a/blockdev.c b/blockdev.c
> index 79fbac8450..8d37ce5943 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2112,11 +2112,9 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common)
>      }
>  }
>  
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> -                                                    const char *target,
> -                                                    strList *bitmaps,
> -                                                    HBitmap **backup,
> -                                                    Error **errp);
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
> +        const char *node, const char *target, strList *bitmaps,
> +        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp);
>  

You know, that's actually a much smarter way to reflow these ...

>  static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>                                               Error **errp)
> @@ -2132,8 +2130,9 @@ static void block_dirty_bitmap_merge_prepare(BlkActionState *common,
>      action = common->action->u.block_dirty_bitmap_merge.data;
>  
>      state->bitmap = do_block_dirty_bitmap_merge(action->node, action->target,
> -                                                action->bitmaps, &state->backup,
> -                                                errp);
> +                                                action->bitmaps,
> +                                                action->external_bitmaps,
> +                                                &state->backup, errp);
>  }
>  
>  static void abort_prepare(BlkActionState *common, Error **errp)
> @@ -2965,15 +2964,14 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name,
>      bdrv_disable_dirty_bitmap(bitmap);
>  }
>  
> -static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
> -                                                    const char *target,
> -                                                    strList *bitmaps,
> -                                                    HBitmap **backup,
> -                                                    Error **errp)
> +static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(
> +        const char *node, const char *target, strList *bitmaps,
> +        BlockDirtyBitmapList *external_bitmaps, HBitmap **backup, Error **errp)
>  {
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *dst, *src, *anon;
>      strList *lst;
> +    BlockDirtyBitmapList *ext_lst;
>      Error *local_err = NULL;
>  
>      dst = block_dirty_bitmap_lookup(node, target, &bs, errp);
> @@ -3003,6 +3001,22 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
>          }
>      }
>  
> +    for (ext_lst = external_bitmaps; ext_lst; ext_lst = ext_lst->next) {
> +        src = block_dirty_bitmap_lookup(ext_lst->value->node,
> +                                        ext_lst->value->name, NULL, errp);
> +        if (!src) {
> +            dst = NULL;
> +            goto out;
> +        }
> +
> +        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            dst = NULL;
> +            goto out;
> +        }
> +    }
> +
>      /* Merge into dst; dst is unchanged on failure. */
>      bdrv_merge_dirty_bitmap(dst, anon, backup, errp);
>  
> @@ -3012,9 +3026,13 @@ static BdrvDirtyBitmap *do_block_dirty_bitmap_merge(const char *node,
>  }
>  
>  void qmp_block_dirty_bitmap_merge(const char *node, const char *target,
> -                                  strList *bitmaps, Error **errp)
> +                                  bool has_bitmaps, strList *bitmaps,
> +                                  bool has_external_bitmaps,
> +                                  BlockDirtyBitmapList *external_bitmaps,
> +                                  Error **errp)
>  {
> -    do_block_dirty_bitmap_merge(node, target, bitmaps, NULL, errp);
> +    do_block_dirty_bitmap_merge(node, target, bitmaps, external_bitmaps, NULL,
> +                                errp);
>  }
>  
>  BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node,
> 

I don't think I like the name "external-bitmaps" but I guess I don't
really have a better suggestion. Technically you can use this for
node-local source bitmaps too, it's just that it's a more verbose way to
specify them. (They're like "fully-qualified" bitmap identifiers instead
of the local/relative references we've been using.)

Well, I can't hold you up if I have no better ideas.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

On 5/16/19 7:32 PM, John Snow wrote:
> 
> 
> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Add new optional parameter making possible to merge bitmaps from
>> different nodes. It is needed to maintain external snapshots during
>> incremental backup chain history.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>  qapi/block-core.json | 13 ++++++++++---
>>  block/dirty-bitmap.c |  9 ++++++---
>>  blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>>  3 files changed, 48 insertions(+), 20 deletions(-)

>> -# @bitmaps: name(s) of the source dirty bitmap(s)
>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
>> +#           since 4.1.
>> +#
>> +# @external-bitmaps: additional list of source dirty bitmaps with specified
>> +#                    nodes, which allows merging bitmaps between different
>> +#                    nodes. (Since: 4.1)
>>  #
>>  # Since: 4.0
>>  ##
>>  { 'struct': 'BlockDirtyBitmapMerge',
>> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
>> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
>> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>>  
> 
> I guess you can specify one, or both, or maybe neither! Seems fine.


> 
> I don't think I like the name "external-bitmaps" but I guess I don't
> really have a better suggestion.

I do - we could use an alternate type instead:

{ 'alternate': 'BitmapSource',
  'data': { 'local': 'str',
             'external': 'BlockDirtyBitmap' } }

then use 'bitmaps': ['BitmapSource']

so that the caller can pass:

"bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ]

and we only have to deal with one array at all times, and not have the
name 'external-bitmaps' to worry about.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

17.05.2019 16:50, Eric Blake wrote:
> On 5/16/19 7:32 PM, John Snow wrote:
>>
>>
>> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Add new optional parameter making possible to merge bitmaps from
>>> different nodes. It is needed to maintain external snapshots during
>>> incremental backup chain history.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   qapi/block-core.json | 13 ++++++++++---
>>>   block/dirty-bitmap.c |  9 ++++++---
>>>   blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>>>   3 files changed, 48 insertions(+), 20 deletions(-)
> 
>>> -# @bitmaps: name(s) of the source dirty bitmap(s)
>>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
>>> +#           since 4.1.
>>> +#
>>> +# @external-bitmaps: additional list of source dirty bitmaps with specified
>>> +#                    nodes, which allows merging bitmaps between different
>>> +#                    nodes. (Since: 4.1)
>>>   #
>>>   # Since: 4.0
>>>   ##
>>>   { 'struct': 'BlockDirtyBitmapMerge',
>>> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
>>> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
>>> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>>>   
>>
>> I guess you can specify one, or both, or maybe neither! Seems fine.
> 
> 
>>
>> I don't think I like the name "external-bitmaps" but I guess I don't
>> really have a better suggestion.
> 
> I do - we could use an alternate type instead:
> 
> { 'alternate': 'BitmapSource',
>    'data': { 'local': 'str',
>               'external': 'BlockDirtyBitmap' } }
> 
> then use 'bitmaps': ['BitmapSource']
> 
> so that the caller can pass:
> 
> "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ]
> 
> and we only have to deal with one array at all times, and not have the
> name 'external-bitmaps' to worry about.
> 

Oh, I wanted to do something like this, but looked at union type,  which also needs some
discriminator field, and decided that it's impossible to make it backward-compatible.

Will resend.



-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] qapi: support external bitmaps in block-dirty-bitmap-merge

[John Snow]

On 5/17/19 10:09 AM, Vladimir Sementsov-Ogievskiy wrote:
> 17.05.2019 16:50, Eric Blake wrote:
>> On 5/16/19 7:32 PM, John Snow wrote:
>>>
>>>
>>> On 5/16/19 8:27 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Add new optional parameter making possible to merge bitmaps from
>>>> different nodes. It is needed to maintain external snapshots during
>>>> incremental backup chain history.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   qapi/block-core.json | 13 ++++++++++---
>>>>   block/dirty-bitmap.c |  9 ++++++---
>>>>   blockdev.c           | 46 ++++++++++++++++++++++++++++++--------------
>>>>   3 files changed, 48 insertions(+), 20 deletions(-)
>>
>>>> -# @bitmaps: name(s) of the source dirty bitmap(s)
>>>> +# @bitmaps: name(s) of the source dirty bitmap(s). The field is optional
>>>> +#           since 4.1.
>>>> +#
>>>> +# @external-bitmaps: additional list of source dirty bitmaps with specified
>>>> +#                    nodes, which allows merging bitmaps between different
>>>> +#                    nodes. (Since: 4.1)
>>>>   #
>>>>   # Since: 4.0
>>>>   ##
>>>>   { 'struct': 'BlockDirtyBitmapMerge',
>>>> -  'data': { 'node': 'str', 'target': 'str', 'bitmaps': ['str'] } }
>>>> +  'data': { 'node': 'str', 'target': 'str', '*bitmaps': ['str'],
>>>> +            '*external-bitmaps': ['BlockDirtyBitmap'] } }
>>>>   
>>>
>>> I guess you can specify one, or both, or maybe neither! Seems fine.
>>
>>
>>>
>>> I don't think I like the name "external-bitmaps" but I guess I don't
>>> really have a better suggestion.
>>
>> I do - we could use an alternate type instead:
>>
>> { 'alternate': 'BitmapSource',
>>    'data': { 'local': 'str',
>>               'external': 'BlockDirtyBitmap' } }
>>
>> then use 'bitmaps': ['BitmapSource']
>>
>> so that the caller can pass:
>>
>> "bitmaps": [ "bitmap1", { "node": "other", "name", "bitmap2" } ]
>>
>> and we only have to deal with one array at all times, and not have the
>> name 'external-bitmaps' to worry about.
>>
> 
> Oh, I wanted to do something like this, but looked at union type,  which also needs some
> discriminator field, and decided that it's impossible to make it backward-compatible.
> 
> Will resend.
> 

Excellent! Thanks Eric!

--js