Re: [PATCH for-5.2 v3 1/3] migration: Add block-bitmap-mapping parameter

[Eric Blake]

On 7/22/20 3:05 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).
>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qapi/migration.json            | 104 ++++++++-
>   migration/migration.h          |   3 +
>   migration/block-dirty-bitmap.c | 373 ++++++++++++++++++++++++++++-----
>   migration/migration.c          |  30 +++
>   monitor/hmp-cmds.c             |  30 +++
>   5 files changed, 485 insertions(+), 55 deletions(-)
>
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..d7e953cd73 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -507,6 +507,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.1

5.2, now, but I can touch that up if it is the only problem raised.

> +# @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, and on the destination also
> +#          complete: On the source, bitmaps on nodes where either the
> +#          bitmap or the node is not mapped will be ignored.  In
> +#          contrast, on the destination, receiving a bitmap (by alias)
> +#          from a node (by alias) when either alias is not mapped will
> +#          result in an error.

Grammar is a bit odd and it feels repetitive.  How about:

The mapping must not provide more than one alias for a bitmap, nor reuse 
the same alias across multiple bitmaps in the same node. On the source, 
an omitted node or bitmap within a node will ignore those bitmaps. In 
contrast, on the destination, receiving a bitmap (by alias) from a node 
(by alias) when either alias is not mapped will result in an error.

> +#          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)

Looks good.


> @@ -781,6 +839,25 @@
>   #          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.

Ah, the joys of duplicated text.

> +++ b/migration/block-dirty-bitmap.c

> @@ -128,7 +131,8 @@ typedef struct DirtyBitmapMigState {
>   
>   typedef struct DirtyBitmapLoadState {
>       uint32_t flags;
> -    char node_name[256];
> +    char node_alias[256];
> +    char bitmap_alias[256];

Do we properly check that alias names will never overflow?

> +static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                       bool name_to_alias,
> +                                       Error **errp)
> +{
> +    GHashTable *alias_map;
> +
> +    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;
> +        }

Maybe id_wellformed should enforce lengths?  Otherwise, I'm not seeing a 
length limit applied during the mapping process.

Modulo that, I think we're ready to go.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org