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 <[email protected]> >> Signed-off-by: Max Reitz <[email protected]> >> --- >> 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;
>> + const BitmapMigrationBitmapAliasList *bmbal;
>> +
>> + monitor_printf(mon, " '%s' -> '%s'\n",
>
> '->' would look strange for incoming. Maybe, change to '--' or '~'.
Hmm, I prefer ->. The object’s name is the node/bitmap name, and that
object gets an alias. So I find this to make sense even on the incoming
side.
Max
signature.asc
Description: OpenPGP digital signature
