[PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

Hi all!

It's a simpler alternative for
"[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
  <20200902181831.2570048-1-eblake@redhat.com>
  https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
  https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/

Since we have "coroutine: true" feature for qmp commands, I think,
maybe we can merge allocation status to bitmap without bothering with
new block-job?

It's an RFC:

1. Main question: is it OK as a simple blocking command, even in a
coroutine mode. It's a lot simpler, and it can be simply used in a
transaction with other bitmap commands.

2. Transaction support is not here now. Will add in future version, if
general approach is OK.

3. I just do bdrv_co_enter() / bdrv_co_leave() like it is done in the
only coroutine qmp command - block_resize(). I'm not sure how much is it
correct.

4. I don't do any "drain". I think it's not needed, as intended usage
is to merge block-status to _active_ bitmap. So all concurrent
operations will just increase dirtyness of the bitmap and it is OK.

5. Probably we still need to create some BdrvChild to avoid node resize
during the loop of block-status querying.

6. Test is mostly copied from parallels-read-bitmap, I'll refactor it in
next version to avoid copy-paste.

7. Probably patch 01 is better be split into 2-3 patches.

Vladimir Sementsov-Ogievskiy (2):
  qapi: block-dirty-bitmap-merge: support allocation maps
  iotests: add allocation-map-to-bitmap

 qapi/block-core.json                          | 31 ++++++++-
 include/block/block_int.h                     |  4 ++
 block/dirty-bitmap.c                          | 42 ++++++++++++
 block/monitor/bitmap-qmp-cmds.c               | 55 +++++++++++++---
 .../tests/allocation-map-to-bitmap            | 64 +++++++++++++++++++
 .../tests/allocation-map-to-bitmap.out        |  9 +++
 6 files changed, 195 insertions(+), 10 deletions(-)
 create mode 100755 tests/qemu-iotests/tests/allocation-map-to-bitmap
 create mode 100644 tests/qemu-iotests/tests/allocation-map-to-bitmap.out

-- 
2.29.2

[PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps

[Vladimir Sementsov-Ogievskiy]

Add possibility to merge allocation map of specified node into target
bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json            | 31 +++++++++++++++++--
 include/block/block_int.h       |  4 +++
 block/dirty-bitmap.c            | 42 +++++++++++++++++++++++++
 block/monitor/bitmap-qmp-cmds.c | 55 ++++++++++++++++++++++++++++-----
 4 files changed, 122 insertions(+), 10 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6d227924d0..0fafb043bc 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2006,6 +2006,32 @@
   'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
             '*persistent': 'bool', '*disabled': 'bool' } }
 
+##
+# @AllocationMapMode:
+#
+# An enumeration of possible allocation maps that could be merged into target
+# bitmap.
+#
+# @top: The allocation status of the top layer of the attached storage node.
+#
+# Since: 6.1
+##
+{ 'enum': 'AllocationMapMode',
+  'data': ['top'] }
+
+##
+# @BlockDirtyBitmapMergeExternalSource:
+#
+# @node: name of device/node which the bitmap is tracking
+#
+# @name: name of the dirty bitmap
+#
+# Since: 6.1
+##
+{ 'struct': 'BlockDirtyBitmapMergeExternalSource',
+  'data': { 'node': 'str', '*name': 'str',
+            '*allocation-map': 'AllocationMapMode' } }
+
 ##
 # @BlockDirtyBitmapMergeSource:
 #
@@ -2017,7 +2043,7 @@
 ##
 { 'alternate': 'BlockDirtyBitmapMergeSource',
   'data': { 'local': 'str',
-            'external': 'BlockDirtyBitmap' } }
+            'external': 'BlockDirtyBitmapMergeExternalSource' } }
 
 ##
 # @BlockDirtyBitmapMerge:
@@ -2176,7 +2202,8 @@
 #
 ##
 { 'command': 'block-dirty-bitmap-merge',
-  'data': 'BlockDirtyBitmapMerge' }
+  'data': 'BlockDirtyBitmapMerge',
+  'coroutine': true }
 
 ##
 # @BlockDirtyBitmapSha256:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 88e4111939..b5aeaef425 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1361,6 +1361,10 @@ bool bdrv_dirty_bitmap_merge_internal(BdrvDirtyBitmap *dest,
                                       const BdrvDirtyBitmap *src,
                                       HBitmap **backup, bool lock);
 
+int bdrv_merge_allocation_top_to_dirty_bitmap(BdrvDirtyBitmap *dest,
+                                              BlockDriverState *bs,
+                                              Error **errp);
+
 void bdrv_inc_in_flight(BlockDriverState *bs);
 void bdrv_dec_in_flight(BlockDriverState *bs);
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 68d295d6e3..78097e30c5 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -914,6 +914,48 @@ out:
     }
 }
 
+int bdrv_merge_allocation_top_to_dirty_bitmap(BdrvDirtyBitmap *dest,
+                                              BlockDriverState *bs,
+                                              Error **errp)
+{
+    int ret;
+    int64_t offset = 0;
+    int64_t bytes = bdrv_getlength(bs);
+
+    if (bytes < 0) {
+        error_setg(errp, "Failed to get length of node '%s'",
+                   bdrv_get_node_name(bs));
+        return bytes;
+    }
+
+    if (bdrv_dirty_bitmap_size(dest) < bytes) {
+        error_setg(errp, "Bitmap is smaller than node '%s'",
+                   bdrv_get_node_name(bs));
+        return -EINVAL;
+    }
+
+    while (bytes) {
+        int64_t cur_bytes = bytes;
+
+        ret = bdrv_is_allocated(bs, offset, cur_bytes, &cur_bytes);
+        if (ret < 0) {
+            error_setg(errp,
+                       "Failed to get block allocation status of node '%s'",
+                       bdrv_get_node_name(bs));
+            return ret;
+        }
+
+        if (ret) {
+            bdrv_set_dirty_bitmap(dest, offset, cur_bytes);
+        }
+
+        bytes -= cur_bytes;
+        offset += cur_bytes;
+    }
+
+    return 0;
+}
+
 /**
  * bdrv_dirty_bitmap_merge_internal: merge src into dest.
  * Does NOT check bitmap permissions; not suitable for use as public API.
diff --git a/block/monitor/bitmap-qmp-cmds.c b/block/monitor/bitmap-qmp-cmds.c
index 9f11deec64..19845a22c4 100644
--- a/block/monitor/bitmap-qmp-cmds.c
+++ b/block/monitor/bitmap-qmp-cmds.c
@@ -34,6 +34,7 @@
 
 #include "block/block_int.h"
 #include "qapi/qapi-commands-block.h"
+#include "qapi/qapi-types-block-core.h"
 #include "qapi/error.h"
 
 /**
@@ -273,8 +274,11 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
     }
 
     for (lst = bms; lst; lst = lst->next) {
+        src = NULL;
+
         switch (lst->value->type) {
             const char *name, *node;
+            bool has_alloc, has_name;
         case QTYPE_QSTRING:
             name = lst->value->u.local;
             src = bdrv_find_dirty_bitmap(bs, name);
@@ -286,22 +290,57 @@ BdrvDirtyBitmap *block_dirty_bitmap_merge(const char *node, const char *target,
             break;
         case QTYPE_QDICT:
             node = lst->value->u.external.node;
-            name = lst->value->u.external.name;
-            src = block_dirty_bitmap_lookup(node, name, NULL, errp);
-            if (!src) {
+            has_name = lst->value->u.external.has_name;
+            has_alloc = lst->value->u.external.has_allocation_map;
+            if (has_name == has_alloc) {
+                error_setg(errp, "Exactly one of @name and @allocation-map "
+                           "fields must be specified.");
                 dst = NULL;
                 goto out;
             }
+            if (has_name) {
+                name = lst->value->u.external.name;
+                src = block_dirty_bitmap_lookup(node, name, NULL, errp);
+                if (!src) {
+                    dst = NULL;
+                    goto out;
+                }
+            } else {
+                int ret;
+                AioContext *old_ctx;
+                assert(has_alloc);
+                /* The only existing mode currently is 'top' */
+                assert(lst->value->u.external.allocation_map ==
+                       ALLOCATION_MAP_MODE_TOP);
+
+                bs = bdrv_lookup_bs(node, node, NULL);
+                if (!bs) {
+                    error_setg(errp, "Node '%s' not found", node);
+                    dst = NULL;
+                    goto out;
+                }
+
+                old_ctx = bdrv_co_enter(bs);
+                ret = bdrv_merge_allocation_top_to_dirty_bitmap(anon, bs, errp);
+                bdrv_co_leave(bs, old_ctx);
+
+                if (ret < 0) {
+                    dst = NULL;
+                    goto out;
+                }
+            }
             break;
         default:
             abort();
         }
 
-        bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            dst = NULL;
-            goto out;
+        if (src) {
+            bdrv_merge_dirty_bitmap(anon, src, NULL, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                dst = NULL;
+                goto out;
+            }
         }
     }
 
-- 
2.29.2

[PATCH 2/2] iotests: add allocation-map-to-bitmap

[Vladimir Sementsov-Ogievskiy]

Add test to check new possibility of block-dirty-bitmap-merge command
to merge allocation map of some node to target dirty bitmap.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 .../tests/allocation-map-to-bitmap            | 64 +++++++++++++++++++
 .../tests/allocation-map-to-bitmap.out        |  9 +++
 2 files changed, 73 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/allocation-map-to-bitmap
 create mode 100644 tests/qemu-iotests/tests/allocation-map-to-bitmap.out

diff --git a/tests/qemu-iotests/tests/allocation-map-to-bitmap b/tests/qemu-iotests/tests/allocation-map-to-bitmap
new file mode 100755
index 0000000000..bd67eed884
--- /dev/null
+++ b/tests/qemu-iotests/tests/allocation-map-to-bitmap
@@ -0,0 +1,64 @@
+#!/usr/bin/env python3
+#
+# Test parallels load bitmap
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# 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 json
+import iotests
+from iotests import qemu_nbd_popen, qemu_img_pipe, log, file_path, qemu_img_create, qemu_io
+
+iotests.script_initialize(supported_fmts=['qcow2'])
+
+nbd_sock = file_path('nbd-sock', base_dir=iotests.sock_dir)
+disk = iotests.file_path('disk')
+bitmap = 'bitmap0'
+nbd_opts = f'driver=nbd,server.type=unix,server.path={nbd_sock}' \
+        f',x-dirty-bitmap=qemu:dirty-bitmap:{bitmap}'
+
+qemu_img_create('-f', 'qcow2', disk, '1M')
+
+qemu_io('-c', 'write 0 512', disk)  # first 64K qcow2 cluster becomes allocated
+qemu_io('-c', 'write 150K 100K', disk)  # 3rd and 4th clusters become allocated
+
+vm = iotests.VM().add_drive(disk)
+vm.launch()
+vm.qmp_log('block-dirty-bitmap-add', node='drive0', name=bitmap,
+           persistent=True)
+vm.qmp_log('block-dirty-bitmap-merge', node='drive0', target=bitmap,
+           bitmaps=[{'node': 'drive0', 'allocation-map': 'top'}])
+vm.shutdown()
+
+with qemu_nbd_popen('--read-only', f'--socket={nbd_sock}',
+                    f'--bitmap={bitmap}', '-f', iotests.imgfmt, disk):
+    out = qemu_img_pipe('map', '--output=json', '--image-opts', nbd_opts)
+    chunks = json.loads(out)
+    cluster = 64 * 1024
+
+    log('dirty clusters (cluster size is 64K):')
+    for c in chunks:
+        assert c['start'] % cluster == 0
+        assert c['length'] % cluster == 0
+        if c['data']:
+            continue
+
+        a = c['start'] // cluster
+        b = (c['start'] + c['length']) // cluster
+        if b - a > 1:
+            log(f'{a}-{b-1}')
+        else:
+            log(a)
diff --git a/tests/qemu-iotests/tests/allocation-map-to-bitmap.out b/tests/qemu-iotests/tests/allocation-map-to-bitmap.out
new file mode 100644
index 0000000000..6cfc42aa4e
--- /dev/null
+++ b/tests/qemu-iotests/tests/allocation-map-to-bitmap.out
@@ -0,0 +1,9 @@
+{"execute": "block-dirty-bitmap-add", "arguments": {"name": "bitmap0", "node": "drive0", "persistent": true}}
+{"return": {}}
+{"execute": "block-dirty-bitmap-merge", "arguments": {"bitmaps": [{"allocation-map": "top", "node": "drive0"}], "node": "drive0", "target": "bitmap0"}}
+{"return": {}}
+Start NBD server
+dirty clusters (cluster size is 64K):
+0
+2-3
+Kill NBD server
-- 
2.29.2

Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

27.04.2021 14:11, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> It's a simpler alternative for
> "[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
>    <20200902181831.2570048-1-eblake@redhat.com>
>    https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
>    https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/
> 
> Since we have "coroutine: true" feature for qmp commands, I think,
> maybe we can merge allocation status to bitmap without bothering with
> new block-job?
> 
> It's an RFC:
> 
> 1. Main question: is it OK as a simple blocking command, even in a
> coroutine mode. It's a lot simpler, and it can be simply used in a
> transaction with other bitmap commands.
> 
> 2. Transaction support is not here now. Will add in future version, if
> general approach is OK.

Ha actually, I think it should work, as block-dirty-bitmap-merge is already transactional, and we don't break it by the commit.

> 
> 3. I just do bdrv_co_enter() / bdrv_co_leave() like it is done in the
> only coroutine qmp command - block_resize(). I'm not sure how much is it
> correct.
> 
> 4. I don't do any "drain". I think it's not needed, as intended usage
> is to merge block-status to_active_  bitmap. So all concurrent
> operations will just increase dirtyness of the bitmap and it is OK.
> 
> 5. Probably we still need to create some BdrvChild to avoid node resize
> during the loop of block-status querying.
> 
> 6. Test is mostly copied from parallels-read-bitmap, I'll refactor it in
> next version to avoid copy-paste.
> 
> 7. Probably patch 01 is better be split into 2-3 patches.

8. Name "bitmaps" for list of sources in block-dirty-bitmap-merge becomes misleading. Probably we should add similar "sources" list as alternative to "bitmaps", and deprecate "bitmaps" field.


-- 
Best regards,
Vladimir

Re: [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps

[Vladimir Sementsov-Ogievskiy]

27.04.2021 14:11, Vladimir Sementsov-Ogievskiy wrote:
> Add possibility to merge allocation map of specified node into target
> bitmap.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com>
> ---
>   qapi/block-core.json            | 31 +++++++++++++++++--
>   include/block/block_int.h       |  4 +++
>   block/dirty-bitmap.c            | 42 +++++++++++++++++++++++++
>   block/monitor/bitmap-qmp-cmds.c | 55 ++++++++++++++++++++++++++++-----
>   4 files changed, 122 insertions(+), 10 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 6d227924d0..0fafb043bc 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2006,6 +2006,32 @@
>     'data': { 'node': 'str', 'name': 'str', '*granularity': 'uint32',
>               '*persistent': 'bool', '*disabled': 'bool' } }
>   
> +##
> +# @AllocationMapMode:
> +#
> +# An enumeration of possible allocation maps that could be merged into target
> +# bitmap.
> +#
> +# @top: The allocation status of the top layer of the attached storage node.
> +#
> +# Since: 6.1
> +##
> +{ 'enum': 'AllocationMapMode',
> +  'data': ['top'] }
> +
> +##
> +# @BlockDirtyBitmapMergeExternalSource:
> +#
> +# @node: name of device/node which the bitmap is tracking
> +#
> +# @name: name of the dirty bitmap
> +#
> +# Since: 6.1
> +##
> +{ 'struct': 'BlockDirtyBitmapMergeExternalSource',
> +  'data': { 'node': 'str', '*name': 'str',
> +            '*allocation-map': 'AllocationMapMode' } }
> +
>   ##
>   # @BlockDirtyBitmapMergeSource:
>   #
> @@ -2017,7 +2043,7 @@
>   ##
>   { 'alternate': 'BlockDirtyBitmapMergeSource',
>     'data': { 'local': 'str',
> -            'external': 'BlockDirtyBitmap' } }
> +            'external': 'BlockDirtyBitmapMergeExternalSource' } }
>   
>   ##
>   # @BlockDirtyBitmapMerge:
> @@ -2176,7 +2202,8 @@
>   #
>   ##
>   { 'command': 'block-dirty-bitmap-merge',
> -  'data': 'BlockDirtyBitmapMerge' }
> +  'data': 'BlockDirtyBitmapMerge',
> +  'coroutine': true }
>   
>   ##


So, what I propose makes possible to issue the following command:

block-dirty-bitmap-merge
   bitmaps: [{"allocation-map": "top", "node": "drive0"}]
   node: target-node-name
   target: target-bitmap-name


I've discussed it with Nikolay, and he requested a possibility of querying allocation status of base..top sub-chain (assume several snapshots was done without creating bitmaps, and we want to restore the bitmap for backup).

So, we actually want something like

block-dirty-bitmap-merge
   bitmaps: [{top: "tpp-node-name", bottom: "bottom-node-name"}]
   node: target-node-name
   target: target-bitmap-name


-- 
Best regards,
Vladimir

Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[John Snow]

On 4/27/21 7:11 AM, Vladimir Sementsov-Ogievskiy wrote:
> Hi all!
> 
> It's a simpler alternative for
> "[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
>    <20200902181831.2570048-1-eblake@redhat.com>
>    https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
>    https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/
> 
> Since we have "coroutine: true" feature for qmp commands, I think,
> maybe we can merge allocation status to bitmap without bothering with
> new block-job?
> 
> It's an RFC:
> 
> 1. Main question: is it OK as a simple blocking command, even in a
> coroutine mode. It's a lot simpler, and it can be simply used in a
> transaction with other bitmap commands.
> 

Hm, possibly... I did not follow the discussion of coroutine QMP 
commands closely to know what the qualifying criteria to use them are.

(Any wisdom for me here, Markus?)

> 2. Transaction support is not here now. Will add in future version, if
> general approach is OK.
> 

That should be alright, I think. It means that the operation needs to 
succeed before the transaction returns success, though.

Depending on what else is in the transaction, do we run the risk of a 
deadlock if we need to wait for a coroutine to finish?

> 3. I just do bdrv_co_enter() / bdrv_co_leave() like it is done in the
> only coroutine qmp command - block_resize(). I'm not sure how much is it
> correct.
> 

See above concern!

> 4. I don't do any "drain". I think it's not needed, as intended usage
> is to merge block-status to _active_ bitmap. So all concurrent
> operations will just increase dirtyness of the bitmap and it is OK.
> 

That sounds fine for individual usage, but I can't convince myself it's 
safe for transactions.

> 5. Probably we still need to create some BdrvChild to avoid node resize
> during the loop of block-status querying.
> 

I'm less sure that it's OK to cause temporary graph changes during the 
course of a blocking QMP function... but maybe that's OK?

Peter Krempa is the expert to consult on that one.

> 6. Test is mostly copied from parallels-read-bitmap, I'll refactor it in
> next version to avoid copy-paste.
> 
> 7. Probably patch 01 is better be split into 2-3 patches.
> 
> Vladimir Sementsov-Ogievskiy (2):
>    qapi: block-dirty-bitmap-merge: support allocation maps
>    iotests: add allocation-map-to-bitmap
> 
>   qapi/block-core.json                          | 31 ++++++++-
>   include/block/block_int.h                     |  4 ++
>   block/dirty-bitmap.c                          | 42 ++++++++++++
>   block/monitor/bitmap-qmp-cmds.c               | 55 +++++++++++++---
>   .../tests/allocation-map-to-bitmap            | 64 +++++++++++++++++++
>   .../tests/allocation-map-to-bitmap.out        |  9 +++
>   6 files changed, 195 insertions(+), 10 deletions(-)
>   create mode 100755 tests/qemu-iotests/tests/allocation-map-to-bitmap
>   create mode 100644 tests/qemu-iotests/tests/allocation-map-to-bitmap.out
> 

Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[Vladimir Sementsov-Ogievskiy]

27.04.2021 21:24, John Snow wrote:
> On 4/27/21 7:11 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>>
>> It's a simpler alternative for
>> "[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
>>    <20200902181831.2570048-1-eblake@redhat.com>
>>    https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
>>    https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/
>>
>> Since we have "coroutine: true" feature for qmp commands, I think,
>> maybe we can merge allocation status to bitmap without bothering with
>> new block-job?
>>
>> It's an RFC:
>>
>> 1. Main question: is it OK as a simple blocking command, even in a
>> coroutine mode. It's a lot simpler, and it can be simply used in a
>> transaction with other bitmap commands.
>>
> 
> Hm, possibly... I did not follow the discussion of coroutine QMP commands closely to know what the qualifying criteria to use them are.
> 
> (Any wisdom for me here, Markus?)
> 
>> 2. Transaction support is not here now. Will add in future version, if
>> general approach is OK.
>>
> 
> That should be alright, I think. It means that the operation needs to succeed before the transaction returns success, though.
> 
> Depending on what else is in the transaction, do we run the risk of a deadlock if we need to wait for a coroutine to finish?
> 
>> 3. I just do bdrv_co_enter() / bdrv_co_leave() like it is done in the
>> only coroutine qmp command - block_resize(). I'm not sure how much is it
>> correct.
>>
> 
> See above concern!
> 
>> 4. I don't do any "drain". I think it's not needed, as intended usage
>> is to merge block-status to _active_ bitmap. So all concurrent
>> operations will just increase dirtyness of the bitmap and it is OK.
>>
> 
> That sounds fine for individual usage, but I can't convince myself it's safe for transactions.

qmp_transaction do drain itself.. Still, it's a bit strange that it does just drain and not drained section around the whole logic.

> 
>> 5. Probably we still need to create some BdrvChild to avoid node resize
>> during the loop of block-status querying.
>>
> 
> I'm less sure that it's OK to cause temporary graph changes during the course of a blocking QMP function... but maybe that's OK?
> 
> Peter Krempa is the expert to consult on that one.
> 
>> 6. Test is mostly copied from parallels-read-bitmap, I'll refactor it in
>> next version to avoid copy-paste.
>>
>> 7. Probably patch 01 is better be split into 2-3 patches.
>>
>> Vladimir Sementsov-Ogievskiy (2):
>>    qapi: block-dirty-bitmap-merge: support allocation maps
>>    iotests: add allocation-map-to-bitmap
>>
>>   qapi/block-core.json                          | 31 ++++++++-
>>   include/block/block_int.h                     |  4 ++
>>   block/dirty-bitmap.c                          | 42 ++++++++++++
>>   block/monitor/bitmap-qmp-cmds.c               | 55 +++++++++++++---
>>   .../tests/allocation-map-to-bitmap            | 64 +++++++++++++++++++
>>   .../tests/allocation-map-to-bitmap.out        |  9 +++
>>   6 files changed, 195 insertions(+), 10 deletions(-)
>>   create mode 100755 tests/qemu-iotests/tests/allocation-map-to-bitmap
>>   create mode 100644 tests/qemu-iotests/tests/allocation-map-to-bitmap.out
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 4/27/21 7:11 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Hi all!
>> It's a simpler alternative for
>> "[PATCH v4 0/5] block: add block-dirty-bitmap-populate job"
>>    <20200902181831.2570048-1-eblake@redhat.com>
>>    https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00978.html
>>    https://patchew.org/QEMU/20200902181831.2570048-1-eblake@redhat.com/
>> Since we have "coroutine: true" feature for qmp commands, I think,
>> maybe we can merge allocation status to bitmap without bothering with
>> new block-job?
>> It's an RFC:
>> 1. Main question: is it OK as a simple blocking command, even in a
>> coroutine mode. It's a lot simpler, and it can be simply used in a
>> transaction with other bitmap commands.
>> 
>
> Hm, possibly... I did not follow the discussion of coroutine QMP
> commands closely to know what the qualifying criteria to use them are.
>
> (Any wisdom for me here, Markus?)

From Kevin's cover letter:

    Some QMP command handlers can block the main loop for a relatively
    long time, for example because they perform some I/O.  This is quite
    nasty.  Allowing such handlers to run in a coroutine where they can
    yield (and therefore release the BQL) while waiting for an event
    such as I/O completion solves the problem.

Running in a coroutine is not a replacement for jobs.  Monitor commands
continue to run one after the other, even with multiple monitors.  All
this does is letting monitor commands yield.

Running in a coroutine is opt-in, because we're scared of command code
misbehaving in coroutine context[*].  To opt-in, add

    'coroutine': true

to the command's QAPI schema.

Misbehaving command code should be rare.  The trouble is finding it.  If
we had a better handle on that, we could make running in a coroutine
opt-out.  Watch out for nested event loops.  Test thoroughly.

Questions?

[...]

[*] Discussed at some length in patch review:

    Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html