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

[Max Reitz]

On 12.08.20 16:32, Eric Blake wrote:
> 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.

Yes, somehow I missed this, and amended it to 5.2 elsewhere...

>> +# @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.

This describes an injective function, as in, one-to-one.

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

This will need a rewrite anyway.  Because of Vladimir’s patches you
merged in rc2, it looks like it makes more sense now to ignore unknown
aliases on the destination.  So I’ll have to think up something new anyway.

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

Hm, well.  There are assertions guarding against that, but they’re
assertions.

That’s an existing problem, actually.  If you give a bitmap a name
longer than 255 bytes, you run into the same failed assertion.

>> +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.

The limit is applied by qemu_put_counted_string(), but it’s enforced via
assertion, so, yeah.  I’ll fix it.

(That’s going to be the easy part.  The harder part is fixing the
pre-existing issue, because we only see bitmap names once migration
starts. Thanks to the new permissive error policy (as in, no
configuration problem should ever produce errors during bitmap
migration), we then need to drop such bitmaps.)

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

Unfortunately, no.  As Vladimir has pointed out, this will require a
hefty rebase now.

Max

signature.asc
Description: OpenPGP digital signature