Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter
On 20.08.20 14:58, Vladimir Sementsov-Ogievskiy wrote:
> 18.08.2020 16:32, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> While touching this code, fix a bug where bitmap names longer than 255
>> bytes would fail an assertion in qemu_put_counted_string().
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Max Reitz
>> ---
>> qapi/migration.json | 101 +++-
>> migration/migration.h | 3 +
>> migration/block-dirty-bitmap.c | 409 -
>> migration/migration.c | 30 +++
>> monitor/hmp-cmds.c | 30 +++
>> 5 files changed, 517 insertions(+), 56 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index ea53b23dca..0c4ae102b1 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>
> [..]
>
>> #
>> +# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
>> +# aliases for the purpose of dirty bitmap migration. Such
>> +# aliases may for example be the corresponding names on the
>> +# opposite site.
>> +# The mapping must be one-to-one, but not necessarily
>> +# complete: On the source, unmapped bitmaps and all bitmaps
>> +# on unmapped nodes will be ignored. On the destination,
>> +# all unmapped aliases in the incoming migration stream will
>> +# be reported, but they will not result in failure.
> Actually, on unknown alias we cancel incoming bitmap migration, which
> means that destination vm continues to run, other (non-bitmap) migration
> states continue to migrate but all further chunks of bitmap migration
> will be ignored. (I'm not sure it worth be mentioned)
Ah, yeah.
[...]
>> @@ -303,21 +497,39 @@ static int add_bitmaps_to_list(DBMSaveState *s,
>> BlockDriverState *bs,
>> return 0;
>> }
>> + bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>> +
>> if (!bs_name || strcmp(bs_name, "") == 0) {
>> error_report("Bitmap '%s' in unnamed node can't be migrated",
>> - bdrv_dirty_bitmap_name(bitmap));
>> + bitmap_name);
>> return -1;
>> }
>> - if (bs_name[0] == '#') {
>> + if (alias_map) {
>> + const AliasMapInnerNode *amin =
>> g_hash_table_lookup(alias_map, bs_name);
>> +
>> + if (!amin) {
>> + /* Skip bitmaps on nodes with no alias */
>> + return 0;
>> + }
>> +
>> + node_alias = amin->string;
>> + bitmap_aliases = amin->subtree;
>> + } else {
>> + node_alias = bs_name;
>> + bitmap_aliases = NULL;
>> + }
>> +
>> + if (node_alias[0] == '#') {
>> error_report("Bitmap '%s' in a node with auto-generated "
>> "name '%s' can't be migrated",
>> - bdrv_dirty_bitmap_name(bitmap), bs_name);
>> + bitmap_name, node_alias);
>> return -1;
>> }
>
> This check is related only to pre-alias_map behavior, so it's probably
> better to keep it inside else{} branch above. Still, aliases already
> checked to be wellformed, so this check will be always false anyway for
> aliases and will not hurt.
Hm, it’s a trade off. It does look a bit weird, because how can aliases
be auto-generated? But OTOH it makes it clearer that we’ll never allow
non-wellformed aliases through.
[...]
>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>> - if (!qemu_get_counted_string(f, s->bitmap_name)) {
>> + const char *bitmap_name;
>> +
>> + if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>> error_report("Unable to read bitmap name string");
>
> Probably s/name/alias/ like for node error message.
Why not.
[...]
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -469,6 +469,32 @@ void hmp_info_migrate_parameters(Monitor *mon,
>> const QDict *qdict)
>> monitor_printf(mon, "%s: '%s'\n",
>> MigrationParameter_str(MIGRATION_PARAMETER_TLS_AUTHZ),
>> params->tls_authz);
>> +
>> + if (params->has_block_bitmap_mapping) {
>> + const BitmapMigrationNodeAliasList *bmnal;
>> +
>> + monitor_printf(mon, "%s:\n",
>> + MigrationParameter_str(
>> +
>> MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING));
>> +
>> + for (bmnal = params->block_bitmap_mapping;
>> + bmnal;
>> + bmnal = bmnal->next)
>> + {
>> + const BitmapMigrationNodeAlias *bmna = bmnal->value;
>>
Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter
18.08.2020 16:32, Max Reitz wrote:
This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.
This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).
While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz
---
qapi/migration.json| 101 +++-
migration/migration.h | 3 +
migration/block-dirty-bitmap.c | 409 -
migration/migration.c | 30 +++
monitor/hmp-cmds.c | 30 +++
5 files changed, 517 insertions(+), 56 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index ea53b23dca..0c4ae102b1 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
[..]
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, but not necessarily
+# complete: On the source, unmapped bitmaps and all bitmaps
+# on unmapped nodes will be ignored. On the destination,
+# all unmapped aliases in the incoming migration stream will
+# be reported, but they will not result in failure.
Actually, on unknown alias we cancel incoming bitmap migration, which means
that destination vm continues to run, other (non-bitmap) migration states
continue to migrate but all further chunks of bitmap migration will be ignored.
(I'm not sure it worth be mentioned)
+# Note that the destination does not know about bitmaps it
+# does not receive, so there is no limitation or requirement
+# regarding the number of bitmaps received, or how they are
+# named, or on which nodes they are placed.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
@@ -656,7 +712,8 @@
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
- 'multifd-zlib-level' ,'multifd-zstd-level' ] }
+ 'multifd-zlib-level' ,'multifd-zstd-level',
+ 'block-bitmap-mapping' ] }
##
[..]
@@ -303,21 +497,39 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
return 0;
}
+bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+
if (!bs_name || strcmp(bs_name, "") == 0) {
error_report("Bitmap '%s' in unnamed node can't be migrated",
- bdrv_dirty_bitmap_name(bitmap));
+ bitmap_name);
return -1;
}
-if (bs_name[0] == '#') {
+if (alias_map) {
+const AliasMapInnerNode *amin = g_hash_table_lookup(alias_map,
bs_name);
+
+if (!amin) {
+/* Skip bitmaps on nodes with no alias */
+return 0;
+}
+
+node_alias = amin->string;
+bitmap_aliases = amin->subtree;
+} else {
+node_alias = bs_name;
+bitmap_aliases = NULL;
+}
+
+if (node_alias[0] == '#') {
error_report("Bitmap '%s' in a node with auto-generated "
"name '%s' can't be migrated",
- bdrv_dirty_bitmap_name(bitmap), bs_name);
+ bitmap_name, node_alias);
return -1;
}
This check is related only to pre-alias_map behavior, so it's probably better
to keep it inside else{} branch above. Still, aliases already checked to be
wellformed, so this check will be always false anyway for aliases and will not
hurt.
FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
-if (!bdrv_dirty_bitmap_name(bitmap)) {
+bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+if (!bitmap_name) {
continue;
}
[..]
@@ -770,8 +1014,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f,
DBMLoadState *s)
return 0;
}
-static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s)
+static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
+GHashTable *alias_map)
{
+GHashTable *bitmap_alias_map = NULL;
Error *local_err = NULL;
bool nothing;
s->flags = qemu_get_bitmap_flags(f);
@@ -780,28 +1026,73 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s)
nothing
Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter
On 20.08.20 03:17, Eric Blake wrote:
> On 8/18/20 8:32 AM, Max Reitz wrote:
>> This migration parameter allows mapping block node names and bitmap
>> names to aliases for the purpose of block dirty bitmap migration.
>>
>> This way, management tools can use different node and bitmap names on
>> the source and destination and pass the mapping of how bitmaps are to be
>> transferred to qemu (on the source, the destination, or even both with
>> arbitrary aliases in the migration stream).
>>
>> While touching this code, fix a bug where bitmap names longer than 255
>> bytes would fail an assertion in qemu_put_counted_string().
>>
>> Suggested-by: Vladimir Sementsov-Ogievskiy
>> Signed-off-by: Max Reitz
>> ---
>
>> +##
>> +# @BitmapMigrationNodeAlias:
>> +#
>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>> +# bitmap migration.
>> +#
>> +# @node-name: A block node name.
>> +#
>> +# @alias: An alias block node name for migration (for example the
>> +# node name on the opposite site).
>> +#
>> +# @bitmaps: Mappings for the bitmaps on this node.
>> +#
>> +# Since: 5.2
>> +##
>> +{ 'struct': 'BitmapMigrationNodeAlias',
>> + 'data': {
>> + 'node-name': 'str',
>> + 'alias': 'str',
>> + 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>> + } }
>
> Possible change: should 'alias' be optional (if absent, it defaults to
> 'node-name')? But that can be done on top, if we like it.
I don’t know whether anyone would make use of that, though, so I’d defer
it until someone wants it.
>> +static GHashTable *construct_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> + bool name_to_alias,
>> + Error **errp)
>> +{
>> + GHashTable *alias_map;
>> + size_t max_node_name_len =
>> + sizeof(((BlockDriverState *)NULL)->node_name) - 1;
>
> Looks a bit nicer as = sizeof_field(BlockDriverState, node_name) - 1.
Oh, I didn’t know we had that.
>> +
>> + alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
>> + g_free,
>> free_alias_map_inner_node);
>> +
>> + for (; bbm; bbm = bbm->next) {
>> + const BitmapMigrationNodeAlias *bmna = bbm->value;
>> + const BitmapMigrationBitmapAliasList *bmbal;
>> + AliasMapInnerNode *amin;
>> + GHashTable *bitmaps_map;
>> + const char *node_map_from, *node_map_to;
>> +
>> + if (!id_wellformed(bmna->alias)) {
>> + error_setg(errp, "The node alias '%s' is not well-formed",
>> + bmna->alias);
>> + goto fail;
>> + }
>> +
>> + if (strlen(bmna->alias) > 255) {
>
> Magic number. UINT8_MAX seems better (since the limit really is due to
> our migration format limiting to one byte).
Well, yes, but qemu_put_counted_string() uses that magic number, too.
*shrug*
>> + g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
>> +
>> + for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
>> + const BitmapMigrationBitmapAlias *bmba = bmbal->value;
>> + const char *bmap_map_from, *bmap_map_to;
>> +
>> + if (strlen(bmba->alias) > 255) {
>
> and again
>
>> + error_setg(errp,
>> + "The bitmap alias '%s' is longer than 255
>> bytes",
>> + bmba->alias);
>> + goto fail;
>> + }
>
> Thanks for adding in the length checking since last revision!
>
>
>> @@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s,
>> BlockDriverState *bs,
>> return -1;
>> }
>> + if (bitmap_aliases) {
>> + bitmap_alias = g_hash_table_lookup(bitmap_aliases,
>> bitmap_name);
>> + if (!bitmap_alias) {
>> + /* Skip bitmaps with no alias */
>> + continue;
>> + }
>> + } else {
>> + if (strlen(bitmap_name) > 255) {
>> + error_report("Cannot migrate bitmap '%s' on node '%s': "
>> + "Name is longer than 255 bytes",
>> + bitmap_name, bs_name);
>> + return -1;
>
> Another one.
>
>
> Reviewed-by: Eric Blake
>
> I'm happy to make those touchups, and put this on my bitmaps queue for a
> pull request as soon as Paolo's meson stuff stabilizes.
Sounds good to me, thanks!
Max
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter
On 8/18/20 8:32 AM, Max Reitz wrote:
This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.
This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).
While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz
---
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+# node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+ 'data': {
+ 'node-name': 'str',
+ 'alias': 'str',
+ 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+ } }
Possible change: should 'alias' be optional (if absent, it defaults to
'node-name')? But that can be done on top, if we like it.
+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+ bool name_to_alias,
+ Error **errp)
+{
+GHashTable *alias_map;
+size_t max_node_name_len =
+sizeof(((BlockDriverState *)NULL)->node_name) - 1;
Looks a bit nicer as = sizeof_field(BlockDriverState, node_name) - 1.
+
+alias_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+ g_free, free_alias_map_inner_node);
+
+for (; bbm; bbm = bbm->next) {
+const BitmapMigrationNodeAlias *bmna = bbm->value;
+const BitmapMigrationBitmapAliasList *bmbal;
+AliasMapInnerNode *amin;
+GHashTable *bitmaps_map;
+const char *node_map_from, *node_map_to;
+
+if (!id_wellformed(bmna->alias)) {
+error_setg(errp, "The node alias '%s' is not well-formed",
+ bmna->alias);
+goto fail;
+}
+
+if (strlen(bmna->alias) > 255) {
Magic number. UINT8_MAX seems better (since the limit really is due to
our migration format limiting to one byte).
...
+g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
+
+for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+const char *bmap_map_from, *bmap_map_to;
+
+if (strlen(bmba->alias) > 255) {
and again
+error_setg(errp,
+ "The bitmap alias '%s' is longer than 255 bytes",
+ bmba->alias);
+goto fail;
+}
Thanks for adding in the length checking since last revision!
@@ -326,12 +538,29 @@ static int add_bitmaps_to_list(DBMSaveState *s,
BlockDriverState *bs,
return -1;
}
+if (bitmap_aliases) {
+bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+if (!bitmap_alias) {
+/* Skip bitmaps with no alias */
+continue;
+}
+} else {
+if (strlen(bitmap_name) > 255) {
+error_report("Cannot migrate bitmap '%s' on node '%s': "
+ "Name is longer than 255 bytes",
+ bitmap_name, bs_name);
+return -1;
Another one.
Reviewed-by: Eric Blake
I'm happy to make those touchups, and put this on my bitmaps queue for a
pull request as soon as Paolo's meson stuff stabilizes.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v4 1/4] migration: Add block-bitmap-mapping parameter
This migration parameter allows mapping block node names and bitmap
names to aliases for the purpose of block dirty bitmap migration.
This way, management tools can use different node and bitmap names on
the source and destination and pass the mapping of how bitmaps are to be
transferred to qemu (on the source, the destination, or even both with
arbitrary aliases in the migration stream).
While touching this code, fix a bug where bitmap names longer than 255
bytes would fail an assertion in qemu_put_counted_string().
Suggested-by: Vladimir Sementsov-Ogievskiy
Signed-off-by: Max Reitz
---
qapi/migration.json| 101 +++-
migration/migration.h | 3 +
migration/block-dirty-bitmap.c | 409 -
migration/migration.c | 30 +++
monitor/hmp-cmds.c | 30 +++
5 files changed, 517 insertions(+), 56 deletions(-)
diff --git a/qapi/migration.json b/qapi/migration.json
index ea53b23dca..0c4ae102b1 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -508,6 +508,44 @@
'data': [ 'none', 'zlib',
{ 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
+##
+# @BitmapMigrationBitmapAlias:
+#
+# @name: The name of the bitmap.
+#
+# @alias: An alias name for migration (for example the bitmap name on
+# the opposite site).
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationBitmapAlias',
+ 'data': {
+ 'name': 'str',
+ 'alias': 'str'
+ } }
+
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+# node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.2
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+ 'data': {
+ 'node-name': 'str',
+ 'alias': 'str',
+ 'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+ } }
+
##
# @MigrationParameter:
#
@@ -642,6 +680,24 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, but not necessarily
+# complete: On the source, unmapped bitmaps and all bitmaps
+# on unmapped nodes will be ignored. On the destination,
+# all unmapped aliases in the incoming migration stream will
+# be reported, but they will not result in failure.
+# Note that the destination does not know about bitmaps it
+# does not receive, so there is no limitation or requirement
+# regarding the number of bitmaps received, or how they are
+# named, or on which nodes they are placed.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
{ 'enum': 'MigrationParameter',
@@ -656,7 +712,8 @@
'multifd-channels',
'xbzrle-cache-size', 'max-postcopy-bandwidth',
'max-cpu-throttle', 'multifd-compression',
- 'multifd-zlib-level' ,'multifd-zstd-level' ] }
+ 'multifd-zlib-level' ,'multifd-zstd-level',
+ 'block-bitmap-mapping' ] }
##
# @MigrateSetParameters:
@@ -782,6 +839,24 @@
# will consume more CPU.
# Defaults to 1. (Since 5.0)
#
+# @block-bitmap-mapping: Maps block nodes and bitmaps on them to
+# aliases for the purpose of dirty bitmap migration. Such
+# aliases may for example be the corresponding names on the
+# opposite site.
+# The mapping must be one-to-one, but not necessarily
+# complete: On the source, unmapped bitmaps and all bitmaps
+# on unmapped nodes will be ignored. On the destination,
+# all unmapped aliases in the incoming migration stream will
+# be reported, but they will not result in failure.
+# Note that the destination does not know about bitmaps it
+# does not receive, so there is no limitation or requirement
+# regarding the number of bitmaps received, or how they are
+# named, or on which nodes they are placed.
+# By default (when this parameter has never been set), bitmap
+# names are mapped to themselves. Nodes are mapped to their
+# block device name if there is one, and to their node name
+# otherwise. (Since 5.2)
+#
# Since: 2.4
##
# TODO either fuse back into MigrationParameters, or make
@@ -812,7 +887,8 @@
'*max-cpu-throttle': 'int',
'*multifd-compression': 'MultiFDCompression',
