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
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>
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
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
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
© 2016 - 2026 Red Hat, Inc.