Re: [PATCH v4 1/4] migration: Add block-bitmap-mapping parameter

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6192 bytes --]

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 <vsementsov@virtuozzo.com>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]