* [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps
2021-04-27 11:11 [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
@ 2021-04-27 11:11 ` Vladimir Sementsov-Ogievskiy
2021-04-27 11:59 ` Vladimir Sementsov-Ogievskiy
2021-04-27 11:11 ` [PATCH 2/2] iotests: add allocation-map-to-bitmap Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-27 11:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, vsementsov, eblake,
pkrempa, nshirokovskiy, den
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps
2021-04-27 11:11 ` [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps Vladimir Sementsov-Ogievskiy
@ 2021-04-27 11:59 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-27 11:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake, pkrempa,
nshirokovskiy, den
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] iotests: add allocation-map-to-bitmap
2021-04-27 11:11 [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
2021-04-27 11:11 ` [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps Vladimir Sementsov-Ogievskiy
@ 2021-04-27 11:11 ` Vladimir Sementsov-Ogievskiy
2021-04-27 11:46 ` [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
2021-04-27 18:24 ` John Snow
3 siblings, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-27 11:11 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, vsementsov, eblake,
pkrempa, nshirokovskiy, den
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge
2021-04-27 11:11 [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
2021-04-27 11:11 ` [PATCH 1/2] qapi: block-dirty-bitmap-merge: support allocation maps Vladimir Sementsov-Ogievskiy
2021-04-27 11:11 ` [PATCH 2/2] iotests: add allocation-map-to-bitmap Vladimir Sementsov-Ogievskiy
@ 2021-04-27 11:46 ` Vladimir Sementsov-Ogievskiy
2021-04-27 18:24 ` John Snow
3 siblings, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-27 11:46 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, mreitz, kwolf, jsnow, eblake, pkrempa,
nshirokovskiy, den
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge
2021-04-27 11:11 [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-04-27 11:46 ` [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge Vladimir Sementsov-Ogievskiy
@ 2021-04-27 18:24 ` John Snow
2021-04-28 0:49 ` Vladimir Sementsov-Ogievskiy
2021-04-28 5:23 ` Markus Armbruster
3 siblings, 2 replies; 8+ messages in thread
From: John Snow @ 2021-04-27 18:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, pkrempa, armbru, qemu-devel, mreitz, nshirokovskiy, den
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge
2021-04-27 18:24 ` John Snow
@ 2021-04-28 0:49 ` Vladimir Sementsov-Ogievskiy
2021-04-28 5:23 ` Markus Armbruster
1 sibling, 0 replies; 8+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-04-28 0:49 UTC (permalink / raw)
To: John Snow, qemu-block
Cc: qemu-devel, armbru, mreitz, kwolf, eblake, pkrempa, nshirokovskiy, den
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH RFC C0/2] support allocation-map for block-dirty-bitmap-merge
2021-04-27 18:24 ` John Snow
2021-04-28 0:49 ` Vladimir Sementsov-Ogievskiy
@ 2021-04-28 5:23 ` Markus Armbruster
1 sibling, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2021-04-28 5:23 UTC (permalink / raw)
To: John Snow
Cc: kwolf, Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel,
mreitz, pkrempa, nshirokovskiy, den
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
^ permalink raw reply [flat|nested] 8+ messages in thread