Re: [PATCH v4 04/18] block/copy-before-write: add bitmap open parameter
24.02.2022 15:07, Hanna Reitz wrote:
On 16.02.22 20:46, 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 | 51 ++-
2 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9a5a3641d0..3bab597506 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4171,11 +4171,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.
Sorry, missed this last time: There should be a “since: 7.0” here.
+#
# 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..91a2288b66 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -34,6 +34,8 @@
#include "block/copy-before-write.h"
+#include "qapi/qapi-visit-block-core.h"
+
typedef struct BDRVCopyBeforeWriteState {
BlockCopyState *bcs;
BdrvChild *target;
@@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs,
BdrvChild *c,
}
}
+static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
+ Error **errp)
+{
+ QDict *bitmap_qdict = NULL;
+ BlockDirtyBitmap *bmp_param = NULL;
+ Visitor *v = NULL;
+ bool ret = false;
+
+ *bitmap = NULL;
+
+ qdict_extract_subqdict(options, _qdict, "bitmap.");
+ if (!qdict_size(bitmap_qdict)) {
+ ret = true;
+ goto out;
+ }
+
+ v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
+ if (!v) {
+ goto out;
+ }
+
+ visit_type_BlockDirtyBitmap(v, NULL, _param, errp);
+ if (!bmp_param) {
+ goto out;
+ }
+
+ *bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
+ errp);
+ if (!*bitmap) {
+ goto out;
+ }
+
+ ret = true;
+
+out:
+ qapi_free_BlockDirtyBitmap(bmp_param);
+ visit_free(v);
+ qobject_unref(bitmap_qdict);
+
+ return ret;
+}
+
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 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
return -EINVAL;
}
+ if (!cbw_parse_bitmap_option(options, , errp)) {
+ return -EINVAL;
Hm... Just to get a second opinion on this: We don’t need to close s->target
here, because the failure paths of bdrv_open_inherit() and
bdrv_new_open_driver_opts() both call bdrv_unref(), which will call bdrv_close(),
which will close all children including s->target, right?
I think I just followed existing error path in cbw_open() on
block_copy_state_new() failure. But I think you are right and bdrv_close()
should take care of all bs children.
+ }
+
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 +219,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 v4 04/18] block/copy-before-write: add bitmap open parameter
On 16.02.22 20:46, 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 | 51 ++-
2 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9a5a3641d0..3bab597506 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4171,11 +4171,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.
Sorry, missed this last time: There should be a “since: 7.0” here.
+#
# 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..91a2288b66 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -34,6 +34,8 @@
#include "block/copy-before-write.h"
+#include "qapi/qapi-visit-block-core.h"
+
typedef struct BDRVCopyBeforeWriteState {
BlockCopyState *bcs;
BdrvChild *target;
@@ -145,10 +147,53 @@ static void cbw_child_perm(BlockDriverState *bs,
BdrvChild *c,
}
}
+static bool cbw_parse_bitmap_option(QDict *options, BdrvDirtyBitmap **bitmap,
+Error **errp)
+{
+QDict *bitmap_qdict = NULL;
+BlockDirtyBitmap *bmp_param = NULL;
+Visitor *v = NULL;
+bool ret = false;
+
+*bitmap = NULL;
+
+qdict_extract_subqdict(options, _qdict, "bitmap.");
+if (!qdict_size(bitmap_qdict)) {
+ret = true;
+goto out;
+}
+
+v = qobject_input_visitor_new_flat_confused(bitmap_qdict, errp);
+if (!v) {
+goto out;
+}
+
+visit_type_BlockDirtyBitmap(v, NULL, _param, errp);
+if (!bmp_param) {
+goto out;
+}
+
+*bitmap = block_dirty_bitmap_lookup(bmp_param->node, bmp_param->name, NULL,
+errp);
+if (!*bitmap) {
+goto out;
+}
+
+ret = true;
+
+out:
+qapi_free_BlockDirtyBitmap(bmp_param);
+visit_free(v);
+qobject_unref(bitmap_qdict);
+
+return ret;
+}
+
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 +208,10 @@ static int cbw_open(BlockDriverState *bs, QDict *options,
int flags,
return -EINVAL;
}
+if (!cbw_parse_bitmap_option(options, , errp)) {
+return -EINVAL;
Hm... Just to get a second opinion on this: We don’t need to close
s->target here, because the failure paths of bdrv_open_inherit() and
bdrv_new_open_driver_opts() both call bdrv_unref(), which will call
bdrv_close(), which will close all children including s->target, right?
+}
+
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 +219,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;
