Re: [PATCH v3 04/19] block/copy-before-write: add bitmap open parameter
14.01.2022 20:47, Hanna Reitz wrote:
On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote:
This brings "incremental" mode to copy-before-write filter: user can
specify bitmap so that filter will copy only "dirty" areas.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
qapi/block-core.json | 10 +-
block/copy-before-write.c | 30 +-
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d3dd9cb48..6904daeacf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4167,11 +4167,19 @@
#
# @target: The target for copy-before-write operations.
#
+# @bitmap: If specified, copy-before-write filter will do
+# copy-before-write operations only for dirty regions of the
+# bitmap. Bitmap size must be equal to length of file and
+# target child of the filter. Note also, that bitmap is used
+# only to initialize internal bitmap of the process, so further
+# modifications (or removing) of specified bitmap doesn't
+# influence the filter.
+#
# Since: 6.2
##
{ 'struct': 'BlockdevOptionsCbw',
'base': 'BlockdevOptionsGenericFormat',
- 'data': { 'target': 'BlockdevRef' } }
+ 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
##
# @BlockdevOptions:
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 799223e3fb..4cd90d22df 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -149,6 +149,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
Error **errp)
{
BDRVCopyBeforeWriteState *s = bs->opaque;
+ BdrvDirtyBitmap *bitmap = NULL;
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -163,6 +164,33 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
return -EINVAL;
}
+ if (qdict_haskey(options, "bitmap.node") ||
+ qdict_haskey(options, "bitmap.name"))
+ {
+ const char *bitmap_node, *bitmap_name;
+
+ if (!qdict_haskey(options, "bitmap.node")) {
+ error_setg(errp, "bitmap.node is not specified");
+ return -EINVAL;
+ }
+
+ if (!qdict_haskey(options, "bitmap.name")) {
+ error_setg(errp, "bitmap.name is not specified");
+ return -EINVAL;
+ }
+
+ bitmap_node = qdict_get_str(options, "bitmap.node");
+ bitmap_name = qdict_get_str(options, "bitmap.name");
+ qdict_del(options, "bitmap.node");
+ qdict_del(options, "bitmap.name");
I’m not really a fan of this manual parsing, but I can see nothing technically
wrong with it.
Still, what do you think of using an input visitor, like:
QDict *bitmap_qdict;
qdict_extract_subqdict(options, _qdict, "bitmap.");
if (qdict_size(bitmap_qdict) > 0) {
BlockDirtyBitmap *bmp_param;
Visitor *v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
visit_type_BlockDirtyBitmap(v, NULL, _param, errp);
visit_free(v);
qobject_unref(bitmap_qdict);
bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, ...);
qapi_free_BlockDirtyBitmap(bmp_param);
}
(+ error handling, which is why perhaps the first block should be put into a
separate function cbw_get_bitmap_param() to simplify error handling)
Will try. Hmm. At some point we should start to generate _marshal_ wrappers and
handle _open() realizations like we do we qmp commands..
+
+ bitmap = block_dirty_bitmap_lookup(bitmap_node, bitmap_name, NULL,
+ errp);
+ if (!bitmap) {
+ return -EINVAL;
+ }
+ }
+
bs->total_sectors = bs->file->bs->total_sectors;
bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
@@ -170,7 +198,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
- s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp);
+ s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp);
if (!s->bcs) {
error_prepend(errp, "Cannot create block-copy-state: ");
return -EINVAL;
--
Best regards,
Vladimir
Re: [PATCH v3 04/19] block/copy-before-write: add bitmap open parameter
On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote:
This brings "incremental" mode to copy-before-write filter: user can
specify bitmap so that filter will copy only "dirty" areas.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
qapi/block-core.json | 10 +-
block/copy-before-write.c | 30 +-
2 files changed, 38 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1d3dd9cb48..6904daeacf 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4167,11 +4167,19 @@
#
# @target: The target for copy-before-write operations.
#
+# @bitmap: If specified, copy-before-write filter will do
+# copy-before-write operations only for dirty regions of the
+# bitmap. Bitmap size must be equal to length of file and
+# target child of the filter. Note also, that bitmap is used
+# only to initialize internal bitmap of the process, so further
+# modifications (or removing) of specified bitmap doesn't
+# influence the filter.
+#
# Since: 6.2
##
{ 'struct': 'BlockdevOptionsCbw',
'base': 'BlockdevOptionsGenericFormat',
- 'data': { 'target': 'BlockdevRef' } }
+ 'data': { 'target': 'BlockdevRef', '*bitmap': 'BlockDirtyBitmap' } }
##
# @BlockdevOptions:
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 799223e3fb..4cd90d22df 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -149,6 +149,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
Error **errp)
{
BDRVCopyBeforeWriteState *s = bs->opaque;
+BdrvDirtyBitmap *bitmap = NULL;
bs->file = bdrv_open_child(NULL, options, "file", bs, _of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
@@ -163,6 +164,33 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
return -EINVAL;
}
+if (qdict_haskey(options, "bitmap.node") ||
+qdict_haskey(options, "bitmap.name"))
+{
+const char *bitmap_node, *bitmap_name;
+
+if (!qdict_haskey(options, "bitmap.node")) {
+error_setg(errp, "bitmap.node is not specified");
+return -EINVAL;
+}
+
+if (!qdict_haskey(options, "bitmap.name")) {
+error_setg(errp, "bitmap.name is not specified");
+return -EINVAL;
+}
+
+bitmap_node = qdict_get_str(options, "bitmap.node");
+bitmap_name = qdict_get_str(options, "bitmap.name");
+qdict_del(options, "bitmap.node");
+qdict_del(options, "bitmap.name");
I’m not really a fan of this manual parsing, but I can see nothing
technically wrong with it.
Still, what do you think of using an input visitor, like:
QDict *bitmap_qdict;
qdict_extract_subqdict(options, _qdict, "bitmap.");
if (qdict_size(bitmap_qdict) > 0) {
BlockDirtyBitmap *bmp_param;
Visitor *v = qobject_input_visitor_new_flat_confused(bitmap_qdict,
errp);
visit_type_BlockDirtyBitmap(v, NULL, _param, errp);
visit_free(v);
qobject_unref(bitmap_qdict);
bitmap = block_dirty_bitmap_lookup(bmp_param->node,
bmp_param->name, ...);
qapi_free_BlockDirtyBitmap(bmp_param);
}
(+ error handling, which is why perhaps the first block should be put
into a separate function cbw_get_bitmap_param() to simplify error handling)
+
+bitmap = block_dirty_bitmap_lookup(bitmap_node, bitmap_name, NULL,
+ errp);
+if (!bitmap) {
+return -EINVAL;
+}
+}
+
bs->total_sectors = bs->file->bs->total_sectors;
bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
@@ -170,7 +198,7 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
bs->file->bs->supported_zero_flags);
-s->bcs = block_copy_state_new(bs->file, s->target, NULL, errp);
+s->bcs = block_copy_state_new(bs->file, s->target, bitmap, errp);
if (!s->bcs) {
error_prepend(errp, "Cannot create block-copy-state: ");
return -EINVAL;
