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

[Vladimir Sementsov-Ogievskiy]

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)

> +#          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 = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
>   
>       if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> -        if (!qemu_get_counted_string(f, s->node_name)) {
> -            error_report("Unable to read node name string");
> +        if (!qemu_get_counted_string(f, s->node_alias)) {
> +            error_report("Unable to read node alias string");
>               return -EINVAL;
>           }
> +
>           if (!s->cancelled) {
> -            s->bs = bdrv_lookup_bs(s->node_name, s->node_name, &local_err);
> +            if (alias_map) {
> +                const AliasMapInnerNode *amin;
> +
> +                amin = g_hash_table_lookup(alias_map, s->node_alias);
> +                if (!amin) {
> +                    error_setg(&local_err, "Error: Unknown node alias '%s'",
> +                               s->node_alias);
> +                    s->bs = NULL;
> +                } else {
> +                    bitmap_alias_map = amin->subtree;
> +                    s->bs = bdrv_lookup_bs(NULL, amin->string, &local_err);
> +                }
> +            } else {
> +                s->bs = bdrv_lookup_bs(s->node_alias, s->node_alias,
> +                                       &local_err);
> +            }
>               if (!s->bs) {
>                   error_report_err(local_err);
>                   cancel_incoming_locked(s);
>               }
>           }
> -    } else if (!s->bs && !nothing && !s->cancelled) {
> +    } else if (s->bs) {
> +        if (alias_map) {
> +            const AliasMapInnerNode *amin;
> +
> +            /* Must be present in the map, or s->bs would not be set */
> +            amin = g_hash_table_lookup(alias_map, s->node_alias);
> +            assert(amin != NULL);
> +
> +            bitmap_alias_map = amin->subtree;
> +        }
> +    } else if (!nothing && !s->cancelled) {
>           error_report("Error: block device name is not set");
>           cancel_incoming_locked(s);
>       }
>   
> +    assert(nothing || s->cancelled || !!alias_map == !!bitmap_alias_map);
> +
>       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.

>               return -EINVAL;
>           }
> +
>           if (!s->cancelled) {
> +            if (bitmap_alias_map) {
> +                bitmap_name = g_hash_table_lookup(bitmap_alias_map,
> +                                                  s->bitmap_alias);
> +                if (!bitmap_name) {
> +                    error_report("Error: Unknown bitmap alias '%s' on node "
> +                                 "'%s' (alias '%s')", s->bitmap_alias,
> +                                 s->bs->node_name, s->node_alias);
> +                    cancel_incoming_locked(s);
> +                }
> +            } else {
> +                bitmap_name = s->bitmap_alias;
> +            }
> +
> +            g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>               s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
>   
>               /*

[..]

> --- 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 '~'.

> +                               bmna->node_name, bmna->alias);
> +
> +                for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> +                    const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +
> +                    monitor_printf(mon, "    '%s' -> '%s'\n",
> +                                   bmba->name, bmba->alias);
> +                }
> +            }
> +        }
>       }
>   
>       qapi_free_MigrationParameters(params);
> @@ -1384,6 +1410,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict 
> *qdict)
>           p->has_announce_step = true;
>           visit_type_size(v, param, &p->announce_step, &err);
>           break;
> +    case MIGRATION_PARAMETER_BLOCK_BITMAP_MAPPING:
> +        error_setg(&err, "The block-bitmap-mapping parameter can only be set "
> +                   "through QMP");
> +        break;
>       default:
>           assert(0);
>       }

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

--
Best regards,
Vladimir