[PATCH 0/4] migration: Add block-bitmap-mapping parameter

[PATCH 0/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html
RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html

Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v1
Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v1


Hi,

This new 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 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).

v1 (as opposed to the RFC):
- Added an iotest
- Allow mapping of not only node names, but also of bitmap names to
  aliases
- Make this a migration parameter instead of adding a whole new QMP
  command
- Added patch 1 for good measure


Max Reitz (4):
  migration: Prevent memleak by ...params_test_apply
  migration: Add block-bitmap-mapping parameter
  iotests.py: Add wait_for_runstate()
  iotests: Test node/bitmap aliases during migration

 qapi/migration.json            |  83 +++++-
 migration/migration.h          |   3 +
 migration/block-dirty-bitmap.c | 372 +++++++++++++++++++++----
 migration/migration.c          |  33 ++-
 tests/qemu-iotests/300         | 487 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out     |   5 +
 tests/qemu-iotests/group       |   1 +
 tests/qemu-iotests/iotests.py  |   4 +
 8 files changed, 931 insertions(+), 57 deletions(-)
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out

-- 
2.26.2

[PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Max Reitz]

The created structure is not really a proper QAPI object, so we cannot
and will not free its members.  Strings therein should therefore not be
duplicated, or we will leak them.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 migration/migration.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 481a590f72..47c7da4e55 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1336,12 +1336,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
 
     if (params->has_tls_creds) {
         assert(params->tls_creds->type == QTYPE_QSTRING);
-        dest->tls_creds = g_strdup(params->tls_creds->u.s);
+        dest->tls_creds = params->tls_creds->u.s;
     }
 
     if (params->has_tls_hostname) {
         assert(params->tls_hostname->type == QTYPE_QSTRING);
-        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
+        dest->tls_hostname = params->tls_hostname->u.s;
     }
 
     if (params->has_max_bandwidth) {
-- 
2.26.2

Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Dr. David Alan Gilbert]

* Max Reitz (mreitz@redhat.com) wrote:
> The created structure is not really a proper QAPI object, so we cannot
> and will not free its members.  Strings therein should therefore not be
> duplicated, or we will leak them.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  migration/migration.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 481a590f72..47c7da4e55 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1336,12 +1336,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>  
>      if (params->has_tls_creds) {
>          assert(params->tls_creds->type == QTYPE_QSTRING);
> -        dest->tls_creds = g_strdup(params->tls_creds->u.s);
> +        dest->tls_creds = params->tls_creds->u.s;
>      }
>  
>      if (params->has_tls_hostname) {
>          assert(params->tls_hostname->type == QTYPE_QSTRING);
> -        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> +        dest->tls_hostname = params->tls_hostname->u.s;
>      }

Yeh I think I agree.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  
>      if (params->has_max_bandwidth) {
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Vladimir Sementsov-Ogievskiy]

30.06.2020 11:45, Max Reitz wrote:
> The created structure is not really a proper QAPI object, so we cannot
> and will not free its members.  Strings therein should therefore not be
> duplicated, or we will leak them.
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

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

-- 
Best regards,
Vladimir

Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Eric Blake]

On 6/30/20 3:45 AM, Max Reitz wrote:
> The created structure is not really a proper QAPI object, so we cannot
> and will not free its members.  Strings therein should therefore not be
> duplicated, or we will leak them.

This seems fragile to me; having to code QAPI usage differently 
depending on whether the containing struct was malloc'd or not (and 
therefore whether someone will call qapi_free_MigrateSetParameters or 
not) looks awkward to maintain.  We have 
visit_type_MigrateSetParameters_members, could that be used as a cleaner 
way to free all members of the struct without freeing the struct itself? 
  Should the QAPI generator start generating qapi_free_FOO_members to 
make such cleanup easier?

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   migration/migration.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 481a590f72..47c7da4e55 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1336,12 +1336,12 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>   
>       if (params->has_tls_creds) {
>           assert(params->tls_creds->type == QTYPE_QSTRING);
> -        dest->tls_creds = g_strdup(params->tls_creds->u.s);
> +        dest->tls_creds = params->tls_creds->u.s;
>       }
>   
>       if (params->has_tls_hostname) {
>           assert(params->tls_hostname->type == QTYPE_QSTRING);
> -        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> +        dest->tls_hostname = params->tls_hostname->u.s;
>       }
>   
>       if (params->has_max_bandwidth) {
> 

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

Re: [PATCH 1/4] migration: Prevent memleak by ...params_test_apply

[Max Reitz]

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

On 01.07.20 16:38, Eric Blake wrote:
> On 6/30/20 3:45 AM, Max Reitz wrote:
>> The created structure is not really a proper QAPI object, so we cannot
>> and will not free its members.  Strings therein should therefore not be
>> duplicated, or we will leak them.
> 
> This seems fragile to me; having to code QAPI usage differently
> depending on whether the containing struct was malloc'd or not (and
> therefore whether someone will call qapi_free_MigrateSetParameters or
> not) looks awkward to maintain.

I don’t think that’s the point.  The point is that it’s just a temporary
object to run some check function on.  This is a very... special use case.

> We have
> visit_type_MigrateSetParameters_members, could that be used as a cleaner
> way to free all members of the struct without freeing the struct itself?
>  Should the QAPI generator start generating qapi_free_FOO_members to
> make such cleanup easier?

The whole code is a mess, in my opinion.

The real question is why don’t we just drop migrate_params_test_apply()
and let qmp_migrate_set_parameters() invoke migrate_params_check()
directly on @params.

I think there was some reason why I didn’t do that, but unfortunately I
don’t remember it off the top of my head (if there was a reason).

In any case, I don’t think any of this is the QAPI generator’s fault.

>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   migration/migration.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 481a590f72..47c7da4e55 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1336,12 +1336,12 @@ static void
>> migrate_params_test_apply(MigrateSetParameters *params,
>>         if (params->has_tls_creds) {
>>           assert(params->tls_creds->type == QTYPE_QSTRING);
>> -        dest->tls_creds = g_strdup(params->tls_creds->u.s);
>> +        dest->tls_creds = params->tls_creds->u.s;
>>       }
>>         if (params->has_tls_hostname) {
>>           assert(params->tls_hostname->type == QTYPE_QSTRING);
>> -        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
>> +        dest->tls_hostname = params->tls_hostname->u.s;
>>       }
>>         if (params->has_max_bandwidth) {
>>
> 



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

[PATCH 2/4] migration: Add block-bitmap-mapping parameter

[Max Reitz]

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            |  83 +++++++-
 migration/migration.h          |   3 +
 migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
 migration/migration.c          |  29 +++
 4 files changed, 432 insertions(+), 55 deletions(-)

diff --git a/qapi/migration.json b/qapi/migration.json
index d5000558c6..5aeae9bea8 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
+##
+{ 'struct': 'BitmapMigrationBitmapAlias',
+  'data': {
+      'name': 'str',
+      'alias': 'str'
+  } }
+
+##
+# @BitmapMigrationNodeAlias:
+#
+# Maps a block node name and the bitmaps it has to aliases for dirty
+# bitmap migration.
+#
+# @node-name: A block node name.
+#
+# @alias: An alias block node name for migration (for example the
+#         node name on the opposite site).
+#
+# @bitmaps: Mappings for the bitmaps on this node.
+#
+# Since: 5.1
+##
+{ 'struct': 'BitmapMigrationNodeAlias',
+  'data': {
+      'node-name': 'str',
+      'alias': 'str',
+      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
+  } }
+
 ##
 # @MigrationParameter:
 #
@@ -641,6 +679,18 @@
 #          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.
+#          The mapping must be one-to-one and complete: On the source,
+#          migrating a bitmap from a node when either is not mapped
+#          will result in an error.  On the destination, similarly,
+#          receiving a bitmap (by alias) from a node (by alias) when
+#          either alias is not mapped will result in an error.
+#          By default, all node names and bitmap names are mapped to
+#          themselves. (Since 5.1)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -655,7 +705,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' ] }
 
 ##
 # @MigrateSetParameters:
@@ -781,6 +832,18 @@
 #          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.
+#          The mapping must be one-to-one and complete: On the source,
+#          migrating a bitmap from a node when either is not mapped
+#          will result in an error.  On the destination, similarly,
+#          receiving a bitmap (by alias) from a node (by alias) when
+#          either alias is not mapped will result in an error.
+#          By default, all node names and bitmap names are mapped to
+#          themselves. (Since 5.1)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -811,7 +874,8 @@
             '*max-cpu-throttle': 'int',
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'int',
-            '*multifd-zstd-level': 'int' } }
+            '*multifd-zstd-level': 'int',
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
 # @migrate-set-parameters:
@@ -957,6 +1021,18 @@
 #          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.
+#          The mapping must be one-to-one and complete: On the source,
+#          migrating a bitmap from a node when either is not mapped
+#          will result in an error.  On the destination, similarly,
+#          receiving a bitmap (by alias) from a node (by alias) when
+#          either alias is not mapped will result in an error.
+#          By default, all node names and bitmap names are mapped to
+#          themselves. (Since 5.1)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -985,7 +1061,8 @@
             '*max-cpu-throttle': 'uint8',
             '*multifd-compression': 'MultiFDCompression',
             '*multifd-zlib-level': 'uint8',
-            '*multifd-zstd-level': 'uint8' } }
+            '*multifd-zstd-level': 'uint8',
+            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
 
 ##
 # @query-migrate-parameters:
diff --git a/migration/migration.h b/migration/migration.h
index f617960522..038c318b92 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -336,6 +336,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                      Error **errp);
+
 void migrate_add_address(SocketAddress *address);
 
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 47bc0f650c..621eb7e3f8 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -29,10 +29,10 @@
  *
  * # Header (shared for different chunk types)
  * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
- * [ 1 byte: node name size ] \  flags & DEVICE_NAME
- * [ n bytes: node name     ] /
- * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
- * [ n bytes: bitmap name     ] /
+ * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
+ * [ n bytes: node alias     ] /
+ * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
+ * [ n bytes: bitmap alias     ] /
  *
  * # Start of bitmap migration (flags & START)
  * header
@@ -72,7 +72,9 @@
 #include "migration/register.h"
 #include "qemu/hbitmap.h"
 #include "qemu/cutils.h"
+#include "qemu/id.h"
 #include "qapi/error.h"
+#include "qapi/qapi-commands-migration.h"
 #include "trace.h"
 
 #define CHUNK_SIZE     (1 << 10)
@@ -103,7 +105,8 @@
 typedef struct DirtyBitmapMigBitmapState {
     /* Written during setup phase. */
     BlockDriverState *bs;
-    const char *node_name;
+    char *node_alias;
+    char *bitmap_alias;
     BdrvDirtyBitmap *bitmap;
     uint64_t total_sectors;
     uint64_t sectors_per_chunk;
@@ -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];
     char bitmap_name[256];
     BlockDriverState *bs;
     BdrvDirtyBitmap *bitmap;
@@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
 static GSList *enabled_bitmaps;
 QemuMutex finish_lock;
 
+/* For hash tables that map node/bitmap names to aliases */
+typedef struct AliasMapInnerNode {
+    char *string;
+    GHashTable *subtree;
+} AliasMapInnerNode;
+
+static void free_alias_map_inner_node(void *amin_ptr)
+{
+    AliasMapInnerNode *amin = amin_ptr;
+
+    g_free(amin->string);
+    g_hash_table_unref(amin->subtree);
+    g_free(amin);
+}
+
+/**
+ * Construct an alias map based on the given QMP structure.
+ *
+ * (Note that we cannot store such maps in the MigrationParameters
+ * object, because that struct is defined by the QAPI schema, which
+ * makes it basically impossible to have dicts with arbitrary keys.
+ * Therefore, we instead have to construct these maps when migration
+ * starts.)
+ *
+ * @bbm is the block_bitmap_mapping from the migration parameters.
+ *
+ * If @name_to_alias is true, the returned hash table will map node
+ * and bitmap names to their respective aliases (for outgoing
+ * migration).
+ *
+ * If @name_to_alias is false, the returned hash table will map node
+ * and bitmap aliases to their respective names (for incoming
+ * migration).
+ *
+ * The hash table maps node names/aliases to AliasMapInnerNode
+ * objects, whose .string is the respective node alias/name, and whose
+ * .subtree table maps bitmap names/aliases to the respective bitmap
+ * alias/name.
+ */
+static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                       bool name_to_alias,
+                                       Error **errp)
+{
+    GHashTable *alias_map = NULL;
+
+    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;
+        }
+
+        if (name_to_alias) {
+            if (g_hash_table_contains(alias_map, bmna->node_name)) {
+                error_setg(errp, "The node name %s is mapped twice",
+                           bmna->node_name);
+                goto fail;
+            }
+
+            node_map_from = bmna->node_name;
+            node_map_to = bmna->alias;
+        } else {
+            if (g_hash_table_contains(alias_map, bmna->alias)) {
+                error_setg(errp, "The node alias %s is used twice",
+                           bmna->alias);
+                goto fail;
+            }
+
+            node_map_from = bmna->alias;
+            node_map_to = bmna->node_name;
+        }
+
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
+                                            g_free, g_free);
+
+        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
+            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
+            const char *bmap_map_from, *bmap_map_to;
+
+            if (name_to_alias) {
+                bmap_map_from = bmba->name;
+                bmap_map_to = bmba->alias;
+
+                if (g_hash_table_contains(bitmaps_map, bmba->name)) {
+                    error_setg(errp, "The bitmap %s/%s is mapped twice",
+                               bmna->node_name, bmba->name);
+                    goto fail;
+                }
+            } else {
+                bmap_map_from = bmba->alias;
+                bmap_map_to = bmba->name;
+
+                if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
+                    error_setg(errp, "The bitmap alias %s/%s is used twice",
+                               bmna->alias, bmba->alias);
+                    goto fail;
+                }
+            }
+
+            g_hash_table_insert(bitmaps_map,
+                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+        }
+
+        amin = g_new(AliasMapInnerNode, 1);
+        *amin = (AliasMapInnerNode){
+            .string = g_strdup(node_map_to),
+            .subtree = bitmaps_map,
+        };
+
+        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
+    }
+
+    return alias_map;
+
+fail:
+    g_hash_table_destroy(alias_map);
+    return NULL;
+}
+
+/**
+ * Run construct_alias_map() in both directions to check whether @bbm
+ * is valid.
+ * (This function is to be used by migration/migration.c to validate
+ * the user-specified block-bitmap-mapping migration parameter.)
+ *
+ * Returns true if and only if the mapping is valid.
+ */
+bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
+                                      Error **errp)
+{
+    GHashTable *alias_map;
+
+    alias_map = construct_alias_map(bbm, true, errp);
+    if (!alias_map) {
+        return false;
+    }
+    g_hash_table_destroy(alias_map);
+
+    alias_map = construct_alias_map(bbm, false, errp);
+    if (!alias_map) {
+        return false;
+    }
+    g_hash_table_destroy(alias_map);
+
+    return true;
+}
+
 void init_dirty_bitmap_incoming_migration(void)
 {
     qemu_mutex_init(&finish_lock);
@@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
     qemu_put_bitmap_flags(f, flags);
 
     if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
-        qemu_put_counted_string(f, dbms->node_name);
+        qemu_put_counted_string(f, dbms->node_alias);
     }
 
     if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
-        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
+        qemu_put_counted_string(f, dbms->bitmap_alias);
     }
 }
 
@@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
         QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
         bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
         bdrv_unref(dbms->bs);
+        g_free(dbms->node_alias);
+        g_free(dbms->bitmap_alias);
         g_free(dbms);
     }
 }
 
 /* Called with iothread lock taken. */
-static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
+static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name,
+                               GHashTable *alias_map)
 {
     BdrvDirtyBitmap *bitmap;
     DirtyBitmapMigBitmapState *dbms;
+    GHashTable *bitmap_aliases;
+    const char *node_alias, *bitmap_name, *bitmap_alias;
     Error *local_err = NULL;
 
     bitmap = bdrv_dirty_bitmap_first(bs);
@@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
         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) {
+            error_report("Bitmap '%s' on node '%s' with no alias cannot be "
+                         "migrated", bitmap_name, bs_name);
+            return -1;
+        }
+
+        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;
     }
 
     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
-        if (!bdrv_dirty_bitmap_name(bitmap)) {
+        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
+        if (!bitmap_name) {
             continue;
         }
 
@@ -302,12 +486,24 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
             return -1;
         }
 
+        if (bitmap_aliases) {
+            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+            if (!bitmap_alias) {
+                error_report("Bitmap '%s' with no alias on node '%s' cannot be "
+                             "migrated", bitmap_name, bs_name);
+                return -1;
+            }
+        } else {
+            bitmap_alias = bitmap_name;
+        }
+
         bdrv_ref(bs);
         bdrv_dirty_bitmap_set_busy(bitmap, true);
 
         dbms = g_new0(DirtyBitmapMigBitmapState, 1);
         dbms->bs = bs;
-        dbms->node_name = bs_name;
+        dbms->node_alias = g_strdup(node_alias);
+        dbms->bitmap_alias = g_strdup(bitmap_alias);
         dbms->bitmap = bitmap;
         dbms->total_sectors = bdrv_nb_sectors(bs);
         dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
@@ -333,43 +529,52 @@ static int init_dirty_bitmap_migration(void)
     DirtyBitmapMigBitmapState *dbms;
     GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
     BlockBackend *blk;
+    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
+    GHashTable *alias_map = NULL;
+
+    if (mig_params->has_block_bitmap_mapping) {
+        alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
+                                        &error_abort);
+    }
 
     dirty_bitmap_mig_state.bulk_completed = false;
     dirty_bitmap_mig_state.prev_bs = NULL;
     dirty_bitmap_mig_state.prev_bitmap = NULL;
     dirty_bitmap_mig_state.no_bitmaps = false;
 
-    /*
-     * Use blockdevice name for direct (or filtered) children of named block
-     * backends.
-     */
-    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
-        const char *name = blk_name(blk);
-
-        if (!name || strcmp(name, "") == 0) {
-            continue;
-        }
+    if (!alias_map) {
+        /*
+         * Use blockdevice name for direct (or filtered) children of named block
+         * backends.
+         */
+        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+            const char *name = blk_name(blk);
 
-        bs = blk_bs(blk);
+            if (!name || strcmp(name, "") == 0) {
+                continue;
+            }
 
-        /* Skip filters without bitmaps */
-        while (bs && bs->drv && bs->drv->is_filter &&
-               !bdrv_has_named_bitmaps(bs))
-        {
-            if (bs->backing) {
-                bs = bs->backing->bs;
-            } else if (bs->file) {
-                bs = bs->file->bs;
-            } else {
-                bs = NULL;
+            bs = blk_bs(blk);
+
+            /* Skip filters without bitmaps */
+            while (bs && bs->drv && bs->drv->is_filter &&
+                   !bdrv_has_named_bitmaps(bs))
+            {
+                if (bs->backing) {
+                    bs = bs->backing->bs;
+                } else if (bs->file) {
+                    bs = bs->file->bs;
+                } else {
+                    bs = NULL;
+                }
             }
-        }
 
-        if (bs && bs->drv && !bs->drv->is_filter) {
-            if (add_bitmaps_to_list(bs, name)) {
-                goto fail;
+            if (bs && bs->drv && !bs->drv->is_filter) {
+                if (add_bitmaps_to_list(bs, name, NULL)) {
+                    goto fail;
+                }
+                g_hash_table_add(handled_by_blk, bs);
             }
-            g_hash_table_add(handled_by_blk, bs);
         }
     }
 
@@ -378,7 +583,7 @@ static int init_dirty_bitmap_migration(void)
             continue;
         }
 
-        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
+        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs), alias_map)) {
             goto fail;
         }
     }
@@ -393,11 +598,17 @@ static int init_dirty_bitmap_migration(void)
     }
 
     g_hash_table_destroy(handled_by_blk);
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
 
     return 0;
 
 fail:
     g_hash_table_destroy(handled_by_blk);
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
     dirty_bitmap_mig_cleanup();
 
     return -1;
@@ -662,8 +873,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
     return 0;
 }
 
-static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
+static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
+                                    GHashTable *alias_map)
 {
+    GHashTable *bitmap_alias_map = NULL;
     Error *local_err = NULL;
     bool nothing;
     s->flags = qemu_get_bitmap_flags(f);
@@ -672,25 +885,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *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;
         }
-        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_report("Error: Unknown node alias '%s'", s->node_alias);
+                return -EINVAL;
+            }
+
+            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);
             return -EINVAL;
         }
-    } else if (!s->bs && !nothing) {
+    } 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) {
         error_report("Error: block device name is not set");
         return -EINVAL;
     }
 
+    assert(!!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");
             return -EINVAL;
         }
+
+        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);
+                return -EINVAL;
+            }
+        } 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);
 
         /* bitmap may be NULL here, it wouldn't be an error if it is the
@@ -698,7 +954,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
         if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
             error_report("Error: unknown dirty bitmap "
                          "'%s' for block device '%s'",
-                         s->bitmap_name, s->node_name);
+                         s->bitmap_name, s->bs->node_name);
             return -EINVAL;
         }
     } else if (!s->bitmap && !nothing) {
@@ -711,6 +967,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
 
 static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
 {
+    GHashTable *alias_map = NULL;
+    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
     static DirtyBitmapLoadState s;
     int ret = 0;
 
@@ -720,10 +978,15 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         return -EINVAL;
     }
 
+    if (mig_params->has_block_bitmap_mapping) {
+        alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
+                                        false, &error_abort);
+    }
+
     do {
-        ret = dirty_bitmap_load_header(f, &s);
+        ret = dirty_bitmap_load_header(f, &s, alias_map);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
 
         if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
@@ -739,12 +1002,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
         }
 
         if (ret) {
-            return ret;
+            goto fail;
         }
     } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
 
     trace_dirty_bitmap_load_success();
-    return 0;
+    ret = 0;
+fail:
+    if (alias_map) {
+        g_hash_table_destroy(alias_map);
+    }
+    return ret;
 }
 
 static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
diff --git a/migration/migration.c b/migration/migration.c
index 47c7da4e55..23fc13e527 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -35,6 +35,7 @@
 #include "block/block.h"
 #include "qapi/error.h"
 #include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-migration.h"
 #include "qapi/qapi-visit-sockets.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-events-migration.h"
@@ -825,6 +826,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->has_announce_step = true;
     params->announce_step = s->parameters.announce_step;
 
+    if (s->parameters.has_block_bitmap_mapping) {
+        params->has_block_bitmap_mapping = true;
+        params->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
+                                                  params->block_bitmap_mapping);
+    }
+
     return params;
 }
 
@@ -1292,6 +1299,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
                    "is invalid, it must be in the range of 1 to 10000 ms");
        return false;
     }
+
+    if (params->has_block_bitmap_mapping &&
+        !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
+        error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
+        return false;
+    }
+
     return true;
 }
 
@@ -1386,6 +1400,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_announce_step) {
         dest->announce_step = params->announce_step;
     }
+
+    if (params->has_block_bitmap_mapping) {
+        dest->has_block_bitmap_mapping = true;
+        dest->block_bitmap_mapping = params->block_bitmap_mapping;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -1498,6 +1517,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
     if (params->has_announce_step) {
         s->parameters.announce_step = params->announce_step;
     }
+
+    if (params->has_block_bitmap_mapping) {
+        qapi_free_BitmapMigrationNodeAliasList(
+            s->parameters.block_bitmap_mapping);
+
+        s->parameters.has_block_bitmap_mapping = true;
+        s->parameters.block_bitmap_mapping =
+            QAPI_CLONE(BitmapMigrationNodeAliasList,
+                       params->block_bitmap_mapping);
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
-- 
2.26.2

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

[Dr. David Alan Gilbert]

* Max Reitz (mreitz@redhat.com) 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            |  83 +++++++-
>  migration/migration.h          |   3 +
>  migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
>  migration/migration.c          |  29 +++
>  4 files changed, 432 insertions(+), 55 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..5aeae9bea8 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
> +##
> +{ 'struct': 'BitmapMigrationBitmapAlias',
> +  'data': {
> +      'name': 'str',
> +      'alias': 'str'
> +  } }
> +
> +##
> +# @BitmapMigrationNodeAlias:
> +#
> +# Maps a block node name and the bitmaps it has to aliases for dirty
> +# bitmap migration.
> +#
> +# @node-name: A block node name.
> +#
> +# @alias: An alias block node name for migration (for example the
> +#         node name on the opposite site).
> +#
> +# @bitmaps: Mappings for the bitmaps on this node.
> +#
> +# Since: 5.1
> +##
> +{ 'struct': 'BitmapMigrationNodeAlias',
> +  'data': {
> +      'node-name': 'str',
> +      'alias': 'str',
> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> +  } }
> +
>  ##
>  # @MigrationParameter:
>  #
> @@ -641,6 +679,18 @@
>  #          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.
> +#          The mapping must be one-to-one and complete: On the source,
> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -655,7 +705,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' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -781,6 +832,18 @@
>  #          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.
> +#          The mapping must be one-to-one and complete: On the source,
> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)
> +#
>  # Since: 2.4
>  ##
>  # TODO either fuse back into MigrationParameters, or make
> @@ -811,7 +874,8 @@
>              '*max-cpu-throttle': 'int',
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'int',
> -            '*multifd-zstd-level': 'int' } }
> +            '*multifd-zstd-level': 'int',
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }

That's a hairy type for a migration parameter!
I'm curious what 'info migrate_parameters' does in hmp or what happens
if you try and set it?

Dave

>  
>  ##
>  # @migrate-set-parameters:
> @@ -957,6 +1021,18 @@
>  #          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.
> +#          The mapping must be one-to-one and complete: On the source,
> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -985,7 +1061,8 @@
>              '*max-cpu-throttle': 'uint8',
>              '*multifd-compression': 'MultiFDCompression',
>              '*multifd-zlib-level': 'uint8',
> -            '*multifd-zstd-level': 'uint8' } }
> +            '*multifd-zstd-level': 'uint8',
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>  
>  ##
>  # @query-migrate-parameters:
> diff --git a/migration/migration.h b/migration/migration.h
> index f617960522..038c318b92 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -336,6 +336,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>  
>  void dirty_bitmap_mig_before_vm_start(void);
>  void init_dirty_bitmap_incoming_migration(void);
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                      Error **errp);
> +
>  void migrate_add_address(SocketAddress *address);
>  
>  int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 47bc0f650c..621eb7e3f8 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -29,10 +29,10 @@
>   *
>   * # Header (shared for different chunk types)
>   * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
> - * [ 1 byte: node name size ] \  flags & DEVICE_NAME
> - * [ n bytes: node name     ] /
> - * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
> - * [ n bytes: bitmap name     ] /
> + * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
> + * [ n bytes: node alias     ] /
> + * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
> + * [ n bytes: bitmap alias     ] /
>   *
>   * # Start of bitmap migration (flags & START)
>   * header
> @@ -72,7 +72,9 @@
>  #include "migration/register.h"
>  #include "qemu/hbitmap.h"
>  #include "qemu/cutils.h"
> +#include "qemu/id.h"
>  #include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
>  #include "trace.h"
>  
>  #define CHUNK_SIZE     (1 << 10)
> @@ -103,7 +105,8 @@
>  typedef struct DirtyBitmapMigBitmapState {
>      /* Written during setup phase. */
>      BlockDriverState *bs;
> -    const char *node_name;
> +    char *node_alias;
> +    char *bitmap_alias;
>      BdrvDirtyBitmap *bitmap;
>      uint64_t total_sectors;
>      uint64_t sectors_per_chunk;
> @@ -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];
>      char bitmap_name[256];
>      BlockDriverState *bs;
>      BdrvDirtyBitmap *bitmap;
> @@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
>  static GSList *enabled_bitmaps;
>  QemuMutex finish_lock;
>  
> +/* For hash tables that map node/bitmap names to aliases */
> +typedef struct AliasMapInnerNode {
> +    char *string;
> +    GHashTable *subtree;
> +} AliasMapInnerNode;
> +
> +static void free_alias_map_inner_node(void *amin_ptr)
> +{
> +    AliasMapInnerNode *amin = amin_ptr;
> +
> +    g_free(amin->string);
> +    g_hash_table_unref(amin->subtree);
> +    g_free(amin);
> +}
> +
> +/**
> + * Construct an alias map based on the given QMP structure.
> + *
> + * (Note that we cannot store such maps in the MigrationParameters
> + * object, because that struct is defined by the QAPI schema, which
> + * makes it basically impossible to have dicts with arbitrary keys.
> + * Therefore, we instead have to construct these maps when migration
> + * starts.)
> + *
> + * @bbm is the block_bitmap_mapping from the migration parameters.
> + *
> + * If @name_to_alias is true, the returned hash table will map node
> + * and bitmap names to their respective aliases (for outgoing
> + * migration).
> + *
> + * If @name_to_alias is false, the returned hash table will map node
> + * and bitmap aliases to their respective names (for incoming
> + * migration).
> + *
> + * The hash table maps node names/aliases to AliasMapInnerNode
> + * objects, whose .string is the respective node alias/name, and whose
> + * .subtree table maps bitmap names/aliases to the respective bitmap
> + * alias/name.
> + */
> +static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                       bool name_to_alias,
> +                                       Error **errp)
> +{
> +    GHashTable *alias_map = NULL;
> +
> +    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;
> +        }
> +
> +        if (name_to_alias) {
> +            if (g_hash_table_contains(alias_map, bmna->node_name)) {
> +                error_setg(errp, "The node name %s is mapped twice",
> +                           bmna->node_name);
> +                goto fail;
> +            }
> +
> +            node_map_from = bmna->node_name;
> +            node_map_to = bmna->alias;
> +        } else {
> +            if (g_hash_table_contains(alias_map, bmna->alias)) {
> +                error_setg(errp, "The node alias %s is used twice",
> +                           bmna->alias);
> +                goto fail;
> +            }
> +
> +            node_map_from = bmna->alias;
> +            node_map_to = bmna->node_name;
> +        }
> +
> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> +                                            g_free, g_free);
> +
> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +            const char *bmap_map_from, *bmap_map_to;
> +
> +            if (name_to_alias) {
> +                bmap_map_from = bmba->name;
> +                bmap_map_to = bmba->alias;
> +
> +                if (g_hash_table_contains(bitmaps_map, bmba->name)) {
> +                    error_setg(errp, "The bitmap %s/%s is mapped twice",
> +                               bmna->node_name, bmba->name);
> +                    goto fail;
> +                }
> +            } else {
> +                bmap_map_from = bmba->alias;
> +                bmap_map_to = bmba->name;
> +
> +                if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
> +                    error_setg(errp, "The bitmap alias %s/%s is used twice",
> +                               bmna->alias, bmba->alias);
> +                    goto fail;
> +                }
> +            }
> +
> +            g_hash_table_insert(bitmaps_map,
> +                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
> +        }
> +
> +        amin = g_new(AliasMapInnerNode, 1);
> +        *amin = (AliasMapInnerNode){
> +            .string = g_strdup(node_map_to),
> +            .subtree = bitmaps_map,
> +        };
> +
> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
> +    }
> +
> +    return alias_map;
> +
> +fail:
> +    g_hash_table_destroy(alias_map);
> +    return NULL;
> +}
> +
> +/**
> + * Run construct_alias_map() in both directions to check whether @bbm
> + * is valid.
> + * (This function is to be used by migration/migration.c to validate
> + * the user-specified block-bitmap-mapping migration parameter.)
> + *
> + * Returns true if and only if the mapping is valid.
> + */
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                      Error **errp)
> +{
> +    GHashTable *alias_map;
> +
> +    alias_map = construct_alias_map(bbm, true, errp);
> +    if (!alias_map) {
> +        return false;
> +    }
> +    g_hash_table_destroy(alias_map);
> +
> +    alias_map = construct_alias_map(bbm, false, errp);
> +    if (!alias_map) {
> +        return false;
> +    }
> +    g_hash_table_destroy(alias_map);
> +
> +    return true;
> +}
> +
>  void init_dirty_bitmap_incoming_migration(void)
>  {
>      qemu_mutex_init(&finish_lock);
> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
>      qemu_put_bitmap_flags(f, flags);
>  
>      if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> -        qemu_put_counted_string(f, dbms->node_name);
> +        qemu_put_counted_string(f, dbms->node_alias);
>      }
>  
>      if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> -        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
> +        qemu_put_counted_string(f, dbms->bitmap_alias);
>      }
>  }
>  
> @@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
>          QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
>          bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>          bdrv_unref(dbms->bs);
> +        g_free(dbms->node_alias);
> +        g_free(dbms->bitmap_alias);
>          g_free(dbms);
>      }
>  }
>  
>  /* Called with iothread lock taken. */
> -static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
> +static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name,
> +                               GHashTable *alias_map)
>  {
>      BdrvDirtyBitmap *bitmap;
>      DirtyBitmapMigBitmapState *dbms;
> +    GHashTable *bitmap_aliases;
> +    const char *node_alias, *bitmap_name, *bitmap_alias;
>      Error *local_err = NULL;
>  
>      bitmap = bdrv_dirty_bitmap_first(bs);
> @@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>          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) {
> +            error_report("Bitmap '%s' on node '%s' with no alias cannot be "
> +                         "migrated", bitmap_name, bs_name);
> +            return -1;
> +        }
> +
> +        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;
>      }
>  
>      FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> +        if (!bitmap_name) {
>              continue;
>          }
>  
> @@ -302,12 +486,24 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>              return -1;
>          }
>  
> +        if (bitmap_aliases) {
> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> +            if (!bitmap_alias) {
> +                error_report("Bitmap '%s' with no alias on node '%s' cannot be "
> +                             "migrated", bitmap_name, bs_name);
> +                return -1;
> +            }
> +        } else {
> +            bitmap_alias = bitmap_name;
> +        }
> +
>          bdrv_ref(bs);
>          bdrv_dirty_bitmap_set_busy(bitmap, true);
>  
>          dbms = g_new0(DirtyBitmapMigBitmapState, 1);
>          dbms->bs = bs;
> -        dbms->node_name = bs_name;
> +        dbms->node_alias = g_strdup(node_alias);
> +        dbms->bitmap_alias = g_strdup(bitmap_alias);
>          dbms->bitmap = bitmap;
>          dbms->total_sectors = bdrv_nb_sectors(bs);
>          dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
> @@ -333,43 +529,52 @@ static int init_dirty_bitmap_migration(void)
>      DirtyBitmapMigBitmapState *dbms;
>      GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
>      BlockBackend *blk;
> +    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
> +    GHashTable *alias_map = NULL;
> +
> +    if (mig_params->has_block_bitmap_mapping) {
> +        alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
> +                                        &error_abort);
> +    }
>  
>      dirty_bitmap_mig_state.bulk_completed = false;
>      dirty_bitmap_mig_state.prev_bs = NULL;
>      dirty_bitmap_mig_state.prev_bitmap = NULL;
>      dirty_bitmap_mig_state.no_bitmaps = false;
>  
> -    /*
> -     * Use blockdevice name for direct (or filtered) children of named block
> -     * backends.
> -     */
> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> -        const char *name = blk_name(blk);
> -
> -        if (!name || strcmp(name, "") == 0) {
> -            continue;
> -        }
> +    if (!alias_map) {
> +        /*
> +         * Use blockdevice name for direct (or filtered) children of named block
> +         * backends.
> +         */
> +        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> +            const char *name = blk_name(blk);
>  
> -        bs = blk_bs(blk);
> +            if (!name || strcmp(name, "") == 0) {
> +                continue;
> +            }
>  
> -        /* Skip filters without bitmaps */
> -        while (bs && bs->drv && bs->drv->is_filter &&
> -               !bdrv_has_named_bitmaps(bs))
> -        {
> -            if (bs->backing) {
> -                bs = bs->backing->bs;
> -            } else if (bs->file) {
> -                bs = bs->file->bs;
> -            } else {
> -                bs = NULL;
> +            bs = blk_bs(blk);
> +
> +            /* Skip filters without bitmaps */
> +            while (bs && bs->drv && bs->drv->is_filter &&
> +                   !bdrv_has_named_bitmaps(bs))
> +            {
> +                if (bs->backing) {
> +                    bs = bs->backing->bs;
> +                } else if (bs->file) {
> +                    bs = bs->file->bs;
> +                } else {
> +                    bs = NULL;
> +                }
>              }
> -        }
>  
> -        if (bs && bs->drv && !bs->drv->is_filter) {
> -            if (add_bitmaps_to_list(bs, name)) {
> -                goto fail;
> +            if (bs && bs->drv && !bs->drv->is_filter) {
> +                if (add_bitmaps_to_list(bs, name, NULL)) {
> +                    goto fail;
> +                }
> +                g_hash_table_add(handled_by_blk, bs);
>              }
> -            g_hash_table_add(handled_by_blk, bs);
>          }
>      }
>  
> @@ -378,7 +583,7 @@ static int init_dirty_bitmap_migration(void)
>              continue;
>          }
>  
> -        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
> +        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs), alias_map)) {
>              goto fail;
>          }
>      }
> @@ -393,11 +598,17 @@ static int init_dirty_bitmap_migration(void)
>      }
>  
>      g_hash_table_destroy(handled_by_blk);
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
>  
>      return 0;
>  
>  fail:
>      g_hash_table_destroy(handled_by_blk);
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
>      dirty_bitmap_mig_cleanup();
>  
>      return -1;
> @@ -662,8 +873,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
>      return 0;
>  }
>  
> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
> +                                    GHashTable *alias_map)
>  {
> +    GHashTable *bitmap_alias_map = NULL;
>      Error *local_err = NULL;
>      bool nothing;
>      s->flags = qemu_get_bitmap_flags(f);
> @@ -672,25 +885,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *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;
>          }
> -        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_report("Error: Unknown node alias '%s'", s->node_alias);
> +                return -EINVAL;
> +            }
> +
> +            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);
>              return -EINVAL;
>          }
> -    } else if (!s->bs && !nothing) {
> +    } 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) {
>          error_report("Error: block device name is not set");
>          return -EINVAL;
>      }
>  
> +    assert(!!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");
>              return -EINVAL;
>          }
> +
> +        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);
> +                return -EINVAL;
> +            }
> +        } 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);
>  
>          /* bitmap may be NULL here, it wouldn't be an error if it is the
> @@ -698,7 +954,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>          if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>              error_report("Error: unknown dirty bitmap "
>                           "'%s' for block device '%s'",
> -                         s->bitmap_name, s->node_name);
> +                         s->bitmap_name, s->bs->node_name);
>              return -EINVAL;
>          }
>      } else if (!s->bitmap && !nothing) {
> @@ -711,6 +967,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>  
>  static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>  {
> +    GHashTable *alias_map = NULL;
> +    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
>      static DirtyBitmapLoadState s;
>      int ret = 0;
>  
> @@ -720,10 +978,15 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>          return -EINVAL;
>      }
>  
> +    if (mig_params->has_block_bitmap_mapping) {
> +        alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
> +                                        false, &error_abort);
> +    }
> +
>      do {
> -        ret = dirty_bitmap_load_header(f, &s);
> +        ret = dirty_bitmap_load_header(f, &s, alias_map);
>          if (ret < 0) {
> -            return ret;
> +            goto fail;
>          }
>  
>          if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
> @@ -739,12 +1002,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>          }
>  
>          if (ret) {
> -            return ret;
> +            goto fail;
>          }
>      } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>  
>      trace_dirty_bitmap_load_success();
> -    return 0;
> +    ret = 0;
> +fail:
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
> +    return ret;
>  }
>  
>  static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
> diff --git a/migration/migration.c b/migration/migration.c
> index 47c7da4e55..23fc13e527 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -35,6 +35,7 @@
>  #include "block/block.h"
>  #include "qapi/error.h"
>  #include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-migration.h"
>  #include "qapi/qapi-visit-sockets.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-events-migration.h"
> @@ -825,6 +826,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->has_announce_step = true;
>      params->announce_step = s->parameters.announce_step;
>  
> +    if (s->parameters.has_block_bitmap_mapping) {
> +        params->has_block_bitmap_mapping = true;
> +        params->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
> +                                                  params->block_bitmap_mapping);
> +    }
> +
>      return params;
>  }
>  
> @@ -1292,6 +1299,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>                     "is invalid, it must be in the range of 1 to 10000 ms");
>         return false;
>      }
> +
> +    if (params->has_block_bitmap_mapping &&
> +        !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
> +        error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
> +        return false;
> +    }
> +
>      return true;
>  }
>  
> @@ -1386,6 +1400,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_announce_step) {
>          dest->announce_step = params->announce_step;
>      }
> +
> +    if (params->has_block_bitmap_mapping) {
> +        dest->has_block_bitmap_mapping = true;
> +        dest->block_bitmap_mapping = params->block_bitmap_mapping;
> +    }
>  }
>  
>  static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1498,6 +1517,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_announce_step) {
>          s->parameters.announce_step = params->announce_step;
>      }
> +
> +    if (params->has_block_bitmap_mapping) {
> +        qapi_free_BitmapMigrationNodeAliasList(
> +            s->parameters.block_bitmap_mapping);
> +
> +        s->parameters.has_block_bitmap_mapping = true;
> +        s->parameters.block_bitmap_mapping =
> +            QAPI_CLONE(BitmapMigrationNodeAliasList,
> +                       params->block_bitmap_mapping);
> +    }
>  }
>  
>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> -- 
> 2.26.2
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Max Reitz]

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

On 30.06.20 12:51, Dr. David Alan Gilbert wrote:
> * Max Reitz (mreitz@redhat.com) 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            |  83 +++++++-
>>  migration/migration.h          |   3 +
>>  migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
>>  migration/migration.c          |  29 +++
>>  4 files changed, 432 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..5aeae9bea8 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
>> +##
>> +{ 'struct': 'BitmapMigrationBitmapAlias',
>> +  'data': {
>> +      'name': 'str',
>> +      'alias': 'str'
>> +  } }
>> +
>> +##
>> +# @BitmapMigrationNodeAlias:
>> +#
>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>> +# bitmap migration.
>> +#
>> +# @node-name: A block node name.
>> +#
>> +# @alias: An alias block node name for migration (for example the
>> +#         node name on the opposite site).
>> +#
>> +# @bitmaps: Mappings for the bitmaps on this node.
>> +#
>> +# Since: 5.1
>> +##
>> +{ 'struct': 'BitmapMigrationNodeAlias',
>> +  'data': {
>> +      'node-name': 'str',
>> +      'alias': 'str',
>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>> +  } }
>> +
>>  ##
>>  # @MigrationParameter:
>>  #
>> @@ -641,6 +679,18 @@
>>  #          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.
>> +#          The mapping must be one-to-one and complete: On the source,
>> +#          migrating a bitmap from a node when either is not mapped
>> +#          will result in an error.  On the destination, similarly,
>> +#          receiving a bitmap (by alias) from a node (by alias) when
>> +#          either alias is not mapped will result in an error.
>> +#          By default, all node names and bitmap names are mapped to
>> +#          themselves. (Since 5.1)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>> @@ -655,7 +705,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' ] }
>>  
>>  ##
>>  # @MigrateSetParameters:
>> @@ -781,6 +832,18 @@
>>  #          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.
>> +#          The mapping must be one-to-one and complete: On the source,
>> +#          migrating a bitmap from a node when either is not mapped
>> +#          will result in an error.  On the destination, similarly,
>> +#          receiving a bitmap (by alias) from a node (by alias) when
>> +#          either alias is not mapped will result in an error.
>> +#          By default, all node names and bitmap names are mapped to
>> +#          themselves. (Since 5.1)
>> +#
>>  # Since: 2.4
>>  ##
>>  # TODO either fuse back into MigrationParameters, or make
>> @@ -811,7 +874,8 @@
>>              '*max-cpu-throttle': 'int',
>>              '*multifd-compression': 'MultiFDCompression',
>>              '*multifd-zlib-level': 'int',
>> -            '*multifd-zstd-level': 'int' } }
>> +            '*multifd-zstd-level': 'int',
>> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> 
> That's a hairy type for a migration parameter!
> I'm curious what 'info migrate_parameters' does in hmp or what happens
> if you try and set it?

Oh.  I didn’t know these were accessible via HMP.

As for setting it, that fails an assertion because I thus forgot to
handle it in hmp_migrate_set_parameter().  I think the best thing to do
would be to just error out, stating that you cannot set that parameter
via HMP.

Similarly, info migrate_parameters doesn’t suport it, because I didn’t
implement it in hmp_info_migrate_parameters().  Since
hmp_info_migrate_parameters() seems to allow free-form reporting, I
suppose we could report it as:

block-bitmap-mapping:
  node1 -> alias1
    bitmap1 -> bmap-alias1
    bitmap2 -> bmap-alias2
  node2 -> alias2
    bitmap1 -> bmap-alias1

etc.

Or we just don’t report it there (apart from maybe “(present)”), because
you can’t set it via migrate_set_parameter.


If there’s any problem with exposing this via the migration parameters,
I have no problem with adding a new QMP command as I did in the RFC.  I
was just pointed towards the migration parameters by reviewers, which
made sense to me, because, well, this kind of is a migration parameter.

Max


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

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

[Dr. David Alan Gilbert]

* Max Reitz (mreitz@redhat.com) wrote:
> On 30.06.20 12:51, Dr. David Alan Gilbert wrote:
> > * Max Reitz (mreitz@redhat.com) 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            |  83 +++++++-
> >>  migration/migration.h          |   3 +
> >>  migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
> >>  migration/migration.c          |  29 +++
> >>  4 files changed, 432 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/qapi/migration.json b/qapi/migration.json
> >> index d5000558c6..5aeae9bea8 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
> >> +##
> >> +{ 'struct': 'BitmapMigrationBitmapAlias',
> >> +  'data': {
> >> +      'name': 'str',
> >> +      'alias': 'str'
> >> +  } }
> >> +
> >> +##
> >> +# @BitmapMigrationNodeAlias:
> >> +#
> >> +# Maps a block node name and the bitmaps it has to aliases for dirty
> >> +# bitmap migration.
> >> +#
> >> +# @node-name: A block node name.
> >> +#
> >> +# @alias: An alias block node name for migration (for example the
> >> +#         node name on the opposite site).
> >> +#
> >> +# @bitmaps: Mappings for the bitmaps on this node.
> >> +#
> >> +# Since: 5.1
> >> +##
> >> +{ 'struct': 'BitmapMigrationNodeAlias',
> >> +  'data': {
> >> +      'node-name': 'str',
> >> +      'alias': 'str',
> >> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> >> +  } }
> >> +
> >>  ##
> >>  # @MigrationParameter:
> >>  #
> >> @@ -641,6 +679,18 @@
> >>  #          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.
> >> +#          The mapping must be one-to-one and complete: On the source,
> >> +#          migrating a bitmap from a node when either is not mapped
> >> +#          will result in an error.  On the destination, similarly,
> >> +#          receiving a bitmap (by alias) from a node (by alias) when
> >> +#          either alias is not mapped will result in an error.
> >> +#          By default, all node names and bitmap names are mapped to
> >> +#          themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  { 'enum': 'MigrationParameter',
> >> @@ -655,7 +705,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' ] }
> >>  
> >>  ##
> >>  # @MigrateSetParameters:
> >> @@ -781,6 +832,18 @@
> >>  #          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.
> >> +#          The mapping must be one-to-one and complete: On the source,
> >> +#          migrating a bitmap from a node when either is not mapped
> >> +#          will result in an error.  On the destination, similarly,
> >> +#          receiving a bitmap (by alias) from a node (by alias) when
> >> +#          either alias is not mapped will result in an error.
> >> +#          By default, all node names and bitmap names are mapped to
> >> +#          themselves. (Since 5.1)
> >> +#
> >>  # Since: 2.4
> >>  ##
> >>  # TODO either fuse back into MigrationParameters, or make
> >> @@ -811,7 +874,8 @@
> >>              '*max-cpu-throttle': 'int',
> >>              '*multifd-compression': 'MultiFDCompression',
> >>              '*multifd-zlib-level': 'int',
> >> -            '*multifd-zstd-level': 'int' } }
> >> +            '*multifd-zstd-level': 'int',
> >> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
> > 
> > That's a hairy type for a migration parameter!
> > I'm curious what 'info migrate_parameters' does in hmp or what happens
> > if you try and set it?
> 
> Oh.  I didn’t know these were accessible via HMP.
> 
> As for setting it, that fails an assertion because I thus forgot to
> handle it in hmp_migrate_set_parameter().  I think the best thing to do
> would be to just error out, stating that you cannot set that parameter
> via HMP.

OK, we can clean that up sometime later; it seems an easy enough mapping
to add (I'd appreciate if you did, but won't force it).

> Similarly, info migrate_parameters doesn’t suport it, because I didn’t
> implement it in hmp_info_migrate_parameters().  Since
> hmp_info_migrate_parameters() seems to allow free-form reporting, I
> suppose we could report it as:
> 
> block-bitmap-mapping:
>   node1 -> alias1
>     bitmap1 -> bmap-alias1
>     bitmap2 -> bmap-alias2
>   node2 -> alias2
>     bitmap1 -> bmap-alias1
> 
> etc.
> 
> Or we just don’t report it there (apart from maybe “(present)”), because
> you can’t set it via migrate_set_parameter.
> 
> 
> If there’s any problem with exposing this via the migration parameters,
> I have no problem with adding a new QMP command as I did in the RFC.  I
> was just pointed towards the migration parameters by reviewers, which
> made sense to me, because, well, this kind of is a migration parameter.

Yep, seems like a migration parameter to me; it's just the first time
we've had such a hairy type in there;  make sure the libvirt people are
happy with using it that way please.

Dave

> Max
> 



--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Vladimir Sementsov-Ogievskiy]

30.06.2020 11:45, 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            |  83 +++++++-
>   migration/migration.h          |   3 +
>   migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
>   migration/migration.c          |  29 +++
>   4 files changed, 432 insertions(+), 55 deletions(-)
> 
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d5000558c6..5aeae9bea8 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
> +##
> +{ 'struct': 'BitmapMigrationBitmapAlias',
> +  'data': {
> +      'name': 'str',
> +      'alias': 'str'
> +  } }
> +
> +##
> +# @BitmapMigrationNodeAlias:
> +#
> +# Maps a block node name and the bitmaps it has to aliases for dirty
> +# bitmap migration.
> +#
> +# @node-name: A block node name.
> +#
> +# @alias: An alias block node name for migration (for example the
> +#         node name on the opposite site).
> +#
> +# @bitmaps: Mappings for the bitmaps on this node.
> +#
> +# Since: 5.1
> +##
> +{ 'struct': 'BitmapMigrationNodeAlias',
> +  'data': {
> +      'node-name': 'str',
> +      'alias': 'str',
> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]

So, we still can't migrate bitmaps from one node to different nodes, but we
also don't know a usecase for it, so it seems OK. But with such scheme we
can select and rename bitmaps in-flight, and Peter said about corresponding
use-case.

I'm OK with this, still, just an idea in my mind:

we could instead just have a list of

BitmapMigrationAlias: {
  node-name
  bitmap-name
  node-alias
  bitmap-alias
}

so, mapping is set for each bitmap in separate.

> +  } }
> +
>   ##
>   # @MigrationParameter:
>   #
> @@ -641,6 +679,18 @@
>   #          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.
> +#          The mapping must be one-to-one and complete: On the source,

I've recently faced the case where incomplete mapping make sence: shared
migration of read-only bitmaps in backing files: it seems better to not
migrate them through migration channel, they are migrating through
shared storage automatically (yes, we'll pay the time to load them on
destination, but I'm going to improve it by implementing lazy load of
read-only and disabled bitmaps, so this will not be a problem).

So, now I think that it would be good just ignore all the bitmap not
described by mapping

> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)

This sentense is unclear, want means "by default" - if the mapping is
not specified at all or if some nodes/bitmaps are not covered. Still,
tha latter will conflict with previous sentencies, so "by default" is
about when mapping is not set at all. I suggest to word it directly:
"If mapping is not set (the command never called, or mapping was
removed".

And, actual behavior without mapping is not as simple: it actually tries
to use blk names if possible and node-names if not, and this veries
from version to version. So, I think not worth to document it in detail,
just note, that without mapping the behavior is not well defined and
tries to use block-device name if possible and node-name otherwise. And
of course direct mapping of bitmap names.

> +#
>   # Since: 2.4
>   ##
>   { 'enum': 'MigrationParameter',
> @@ -655,7 +705,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' ] }
>   
>   ##
>   # @MigrateSetParameters:
> @@ -781,6 +832,18 @@
>   #          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.
> +#          The mapping must be one-to-one and complete: On the source,
> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)

Do we really need this duplication? I'd prefere to document it once if possible.

> +#
>   # Since: 2.4
>   ##
>   # TODO either fuse back into MigrationParameters, or make
> @@ -811,7 +874,8 @@
>               '*max-cpu-throttle': 'int',
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'int',
> -            '*multifd-zstd-level': 'int' } }
> +            '*multifd-zstd-level': 'int',
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>   
>   ##
>   # @migrate-set-parameters:
> @@ -957,6 +1021,18 @@
>   #          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.
> +#          The mapping must be one-to-one and complete: On the source,
> +#          migrating a bitmap from a node when either is not mapped
> +#          will result in an error.  On the destination, similarly,
> +#          receiving a bitmap (by alias) from a node (by alias) when
> +#          either alias is not mapped will result in an error.
> +#          By default, all node names and bitmap names are mapped to
> +#          themselves. (Since 5.1)

once again..

> +#
>   # Since: 2.4
>   ##
>   { 'struct': 'MigrationParameters',
> @@ -985,7 +1061,8 @@
>               '*max-cpu-throttle': 'uint8',
>               '*multifd-compression': 'MultiFDCompression',
>               '*multifd-zlib-level': 'uint8',
> -            '*multifd-zstd-level': 'uint8' } }
> +            '*multifd-zstd-level': 'uint8',
> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>   
>   ##
>   # @query-migrate-parameters:
> diff --git a/migration/migration.h b/migration/migration.h
> index f617960522..038c318b92 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -336,6 +336,9 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>   
>   void dirty_bitmap_mig_before_vm_start(void);
>   void init_dirty_bitmap_incoming_migration(void);
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                      Error **errp);
> +
>   void migrate_add_address(SocketAddress *address);
>   
>   int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 47bc0f650c..621eb7e3f8 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -29,10 +29,10 @@
>    *
>    * # Header (shared for different chunk types)
>    * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
> - * [ 1 byte: node name size ] \  flags & DEVICE_NAME
> - * [ n bytes: node name     ] /
> - * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
> - * [ n bytes: bitmap name     ] /
> + * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
> + * [ n bytes: node alias     ] /
> + * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
> + * [ n bytes: bitmap alias     ] /
>    *
>    * # Start of bitmap migration (flags & START)
>    * header
> @@ -72,7 +72,9 @@
>   #include "migration/register.h"
>   #include "qemu/hbitmap.h"
>   #include "qemu/cutils.h"
> +#include "qemu/id.h"
>   #include "qapi/error.h"
> +#include "qapi/qapi-commands-migration.h"
>   #include "trace.h"
>   
>   #define CHUNK_SIZE     (1 << 10)
> @@ -103,7 +105,8 @@
>   typedef struct DirtyBitmapMigBitmapState {
>       /* Written during setup phase. */
>       BlockDriverState *bs;
> -    const char *node_name;
> +    char *node_alias;
> +    char *bitmap_alias;
>       BdrvDirtyBitmap *bitmap;
>       uint64_t total_sectors;
>       uint64_t sectors_per_chunk;
> @@ -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];
>       char bitmap_name[256];
>       BlockDriverState *bs;
>       BdrvDirtyBitmap *bitmap;
> @@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
>   static GSList *enabled_bitmaps;
>   QemuMutex finish_lock;
>   
> +/* For hash tables that map node/bitmap names to aliases */
> +typedef struct AliasMapInnerNode {
> +    char *string;
> +    GHashTable *subtree;
> +} AliasMapInnerNode;

Probably, would be simpler to have (node,bitmap) <-> (node-alias,bitmap-alias) mapping than nested hash-tables, but this should work too, I don't have real arguments.

> +
> +static void free_alias_map_inner_node(void *amin_ptr)
> +{
> +    AliasMapInnerNode *amin = amin_ptr;
> +
> +    g_free(amin->string);
> +    g_hash_table_unref(amin->subtree);
> +    g_free(amin);
> +}
> +
> +/**
> + * Construct an alias map based on the given QMP structure.
> + *
> + * (Note that we cannot store such maps in the MigrationParameters
> + * object, because that struct is defined by the QAPI schema, which
> + * makes it basically impossible to have dicts with arbitrary keys.
> + * Therefore, we instead have to construct these maps when migration
> + * starts.)
> + *
> + * @bbm is the block_bitmap_mapping from the migration parameters.
> + *
> + * If @name_to_alias is true, the returned hash table will map node
> + * and bitmap names to their respective aliases (for outgoing
> + * migration).
> + *
> + * If @name_to_alias is false, the returned hash table will map node
> + * and bitmap aliases to their respective names (for incoming
> + * migration).
> + *
> + * The hash table maps node names/aliases to AliasMapInnerNode
> + * objects, whose .string is the respective node alias/name, and whose
> + * .subtree table maps bitmap names/aliases to the respective bitmap
> + * alias/name.
> + */
> +static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                       bool name_to_alias,
> +                                       Error **errp)
> +{
> +    GHashTable *alias_map = NULL;

dead assignment

> +
> +    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;
> +        }
> +
> +        if (name_to_alias) {
> +            if (g_hash_table_contains(alias_map, bmna->node_name)) {
> +                error_setg(errp, "The node name %s is mapped twice",
> +                           bmna->node_name);
> +                goto fail;
> +            }
> +
> +            node_map_from = bmna->node_name;
> +            node_map_to = bmna->alias;
> +        } else {
> +            if (g_hash_table_contains(alias_map, bmna->alias)) {
> +                error_setg(errp, "The node alias %s is used twice",
> +                           bmna->alias);
> +                goto fail;
> +            }
> +
> +            node_map_from = bmna->alias;
> +            node_map_to = bmna->node_name;
> +        }
> +
> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> +                                            g_free, g_free);
> +
> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
> +            const char *bmap_map_from, *bmap_map_to;
> +
> +            if (name_to_alias) {
> +                bmap_map_from = bmba->name;
> +                bmap_map_to = bmba->alias;
> +
> +                if (g_hash_table_contains(bitmaps_map, bmba->name)) {
> +                    error_setg(errp, "The bitmap %s/%s is mapped twice",
> +                               bmna->node_name, bmba->name);
> +                    goto fail;

bitmaps_map is leaked here and ..

> +                }
> +            } else {
> +                bmap_map_from = bmba->alias;
> +                bmap_map_to = bmba->name;
> +
> +                if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
> +                    error_setg(errp, "The bitmap alias %s/%s is used twice",
> +                               bmna->alias, bmba->alias);
> +                    goto fail;

.. here

Probably, simplest way to fix is to create AliasMapIneerNode and insert it into alias_map immediately after allocating bitmaps_map (prior to this loop).

> +                }
> +            }
> +
> +            g_hash_table_insert(bitmaps_map,
> +                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
> +        }
> +
> +        amin = g_new(AliasMapInnerNode, 1);
> +        *amin = (AliasMapInnerNode){

style: space before '{'

> +            .string = g_strdup(node_map_to),
> +            .subtree = bitmaps_map,
> +        };
> +
> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
> +    }
> +
> +    return alias_map;
> +
> +fail:
> +    g_hash_table_destroy(alias_map);
> +    return NULL;
> +}
> +
> +/**
> + * Run construct_alias_map() in both directions to check whether @bbm
> + * is valid.
> + * (This function is to be used by migration/migration.c to validate
> + * the user-specified block-bitmap-mapping migration parameter.)
> + *
> + * Returns true if and only if the mapping is valid.
> + */
> +bool check_dirty_bitmap_mig_alias_map(const BitmapMigrationNodeAliasList *bbm,
> +                                      Error **errp)
> +{
> +    GHashTable *alias_map;
> +
> +    alias_map = construct_alias_map(bbm, true, errp);
> +    if (!alias_map) {
> +        return false;
> +    }
> +    g_hash_table_destroy(alias_map);
> +
> +    alias_map = construct_alias_map(bbm, false, errp);
> +    if (!alias_map) {
> +        return false;
> +    }
> +    g_hash_table_destroy(alias_map);
> +
> +    return true;
> +}
> +
>   void init_dirty_bitmap_incoming_migration(void)
>   {
>       qemu_mutex_init(&finish_lock);
> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f, DirtyBitmapMigBitmapState *dbms,
>       qemu_put_bitmap_flags(f, flags);
>   
>       if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
> -        qemu_put_counted_string(f, dbms->node_name);
> +        qemu_put_counted_string(f, dbms->node_alias);
>       }
>   
>       if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> -        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
> +        qemu_put_counted_string(f, dbms->bitmap_alias);
>       }
>   }
>   
> @@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
>           QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
>           bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>           bdrv_unref(dbms->bs);
> +        g_free(dbms->node_alias);
> +        g_free(dbms->bitmap_alias);
>           g_free(dbms);
>       }
>   }
>   
>   /* Called with iothread lock taken. */
> -static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
> +static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name,
> +                               GHashTable *alias_map)
>   {
>       BdrvDirtyBitmap *bitmap;
>       DirtyBitmapMigBitmapState *dbms;
> +    GHashTable *bitmap_aliases;

can bitmap_aliases be const ptr too?

> +    const char *node_alias, *bitmap_name, *bitmap_alias;
>       Error *local_err = NULL;
>   
>       bitmap = bdrv_dirty_bitmap_first(bs);
> @@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>           return 0;
>       }
>   
> +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);

Note, that bitmap is wrong here: it may be internal unnamed bitmap which we should ignore.
I have sent a patch for this: "[PATCH] migration/block-dirty-bitmap: fix add_bitmaps_to_list"

> +
>       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) {
> +            error_report("Bitmap '%s' on node '%s' with no alias cannot be "
> +                         "migrated", bitmap_name, bs_name);

As I've said before, it may be reasonable to ignore bitmaps not referenced in the hash-table.
And it seems to be good default behavior. We are really tired from problems with bitmaps which
can't migrate for different reasons, when bitmaps are actually non-critical data, and choosing
from customer problems like:

  1. - Hey, after update migration is broken! It says that some bitmaps can't be migrated, what is that?
  
  2. - Hmm, it seems, that in some cases, incremental backup doesn't work after migration and full backup
       is automatically done instead.. Why?

I now prefer the [2].

> +            return -1;
> +        }
> +
> +        node_alias = amin->string;
> +        bitmap_aliases = amin->subtree;
> +    } else {
> +        node_alias = bs_name;
> +        bitmap_aliases = NULL;
> +    }

Hmm, actually bs_name argument is incompatiblewith alias_map, let's assert that they are not non-null simultaneously.

Ah stop, I see, you use bs_name as node-name later and before.. Hmm, seems all this a bit confused.

Prior the patch, why do we have bs_name: because it may be node-name or blk-name, to be use in migration protocol as (actually) an alias, so bs_name is more like an alias..

So, we have bs, which already have bs->node_name, we have alias_map, where we have relation node_name -> alias, and we have bs_name, which is something like an alias_name.

I think the most clean thing is to allow only one of bs_name and alias_map to be non-null, use bs_name only for old scenario, and for new scenario use bdrv_get_node_name() for error-reporting. And a comment about function semantics won't hurt.

upd: aha, I see that in case of new semantics, bs_name is exactly bdrv_get_node_name(). It's a bit redundant but OK too.. Let's at least add an assertion.

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

OK, this should not relate to mapped case, as aliases are well-formed.

>           return -1;
>       }
>   
>       FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> +        if (!bitmap_name) {
>               continue;
>           }
>   
> @@ -302,12 +486,24 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>               return -1;
>           }
>   
> +        if (bitmap_aliases) {
> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
> +            if (!bitmap_alias) {
> +                error_report("Bitmap '%s' with no alias on node '%s' cannot be "
> +                             "migrated", bitmap_name, bs_name);
> +                return -1;
> +            }
> +        } else {
> +            bitmap_alias = bitmap_name;
> +        }
> +

Hmm, we don't error-out if we didn't find a bitmap, specified in alias-map. But it seems to be an error from the user point-of-view (the required action can't be done).

So, probably, we want loop through the alias-map (and in this case we don't need a map, but can work with QAPI list), find corresponding bitmaps and migrate them, and fail if some specified in the alias-map bitmap is not found.

>           bdrv_ref(bs);
>           bdrv_dirty_bitmap_set_busy(bitmap, true);
>   
>           dbms = g_new0(DirtyBitmapMigBitmapState, 1);
>           dbms->bs = bs;
> -        dbms->node_name = bs_name;
> +        dbms->node_alias = g_strdup(node_alias);
> +        dbms->bitmap_alias = g_strdup(bitmap_alias);
>           dbms->bitmap = bitmap;
>           dbms->total_sectors = bdrv_nb_sectors(bs);
>           dbms->sectors_per_chunk = CHUNK_SIZE * 8 *
> @@ -333,43 +529,52 @@ static int init_dirty_bitmap_migration(void)
>       DirtyBitmapMigBitmapState *dbms;
>       GHashTable *handled_by_blk = g_hash_table_new(NULL, NULL);
>       BlockBackend *blk;
> +    const MigrationParameters *mig_params = &migrate_get_current()->parameters;
> +    GHashTable *alias_map = NULL;
> +
> +    if (mig_params->has_block_bitmap_mapping) {
> +        alias_map = construct_alias_map(mig_params->block_bitmap_mapping, true,
> +                                        &error_abort);
> +    }
>   
>       dirty_bitmap_mig_state.bulk_completed = false;
>       dirty_bitmap_mig_state.prev_bs = NULL;
>       dirty_bitmap_mig_state.prev_bitmap = NULL;
>       dirty_bitmap_mig_state.no_bitmaps = false;
>   
> -    /*
> -     * Use blockdevice name for direct (or filtered) children of named block
> -     * backends.
> -     */
> -    for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> -        const char *name = blk_name(blk);
> -
> -        if (!name || strcmp(name, "") == 0) {
> -            continue;
> -        }
> +    if (!alias_map) {
> +        /*
> +         * Use blockdevice name for direct (or filtered) children of named block
> +         * backends.
> +         */
> +        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> +            const char *name = blk_name(blk);
>   
> -        bs = blk_bs(blk);
> +            if (!name || strcmp(name, "") == 0) {
> +                continue;
> +            }
>   
> -        /* Skip filters without bitmaps */
> -        while (bs && bs->drv && bs->drv->is_filter &&
> -               !bdrv_has_named_bitmaps(bs))
> -        {
> -            if (bs->backing) {
> -                bs = bs->backing->bs;
> -            } else if (bs->file) {
> -                bs = bs->file->bs;
> -            } else {
> -                bs = NULL;
> +            bs = blk_bs(blk);
> +
> +            /* Skip filters without bitmaps */
> +            while (bs && bs->drv && bs->drv->is_filter &&
> +                   !bdrv_has_named_bitmaps(bs))
> +            {
> +                if (bs->backing) {
> +                    bs = bs->backing->bs;
> +                } else if (bs->file) {
> +                    bs = bs->file->bs;
> +                } else {
> +                    bs = NULL;
> +                }
>               }
> -        }
>   
> -        if (bs && bs->drv && !bs->drv->is_filter) {
> -            if (add_bitmaps_to_list(bs, name)) {
> -                goto fail;
> +            if (bs && bs->drv && !bs->drv->is_filter) {
> +                if (add_bitmaps_to_list(bs, name, NULL)) {
> +                    goto fail;
> +                }
> +                g_hash_table_add(handled_by_blk, bs);
>               }
> -            g_hash_table_add(handled_by_blk, bs);
>           }
>       }
>   
> @@ -378,7 +583,7 @@ static int init_dirty_bitmap_migration(void)
>               continue;
>           }
>   
> -        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs))) {
> +        if (add_bitmaps_to_list(bs, bdrv_get_node_name(bs), alias_map)) {
>               goto fail;
>           }
>       }
> @@ -393,11 +598,17 @@ static int init_dirty_bitmap_migration(void)
>       }
>   
>       g_hash_table_destroy(handled_by_blk);
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
>   
>       return 0;
>   
>   fail:
>       g_hash_table_destroy(handled_by_blk);
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
>       dirty_bitmap_mig_cleanup();
>   
>       return -1;
> @@ -662,8 +873,10 @@ static int dirty_bitmap_load_bits(QEMUFile *f, DirtyBitmapLoadState *s)
>       return 0;
>   }
>   
> -static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
> +static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s,
> +                                    GHashTable *alias_map)
>   {
> +    GHashTable *bitmap_alias_map = NULL;
>       Error *local_err = NULL;
>       bool nothing;
>       s->flags = qemu_get_bitmap_flags(f);
> @@ -672,25 +885,68 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *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;
>           }
> -        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_report("Error: Unknown node alias '%s'", s->node_alias);
> +                return -EINVAL;
> +            }
> +
> +            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);
>               return -EINVAL;
>           }
> -    } else if (!s->bs && !nothing) {
> +    } 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) {
>           error_report("Error: block device name is not set");
>           return -EINVAL;
>       }
>   
> +    assert(!!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");
>               return -EINVAL;
>           }
> +
> +        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);
> +                return -EINVAL;
> +            }
> +        } 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);
>   
>           /* bitmap may be NULL here, it wouldn't be an error if it is the
> @@ -698,7 +954,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>           if (!s->bitmap && !(s->flags & DIRTY_BITMAP_MIG_FLAG_START)) {
>               error_report("Error: unknown dirty bitmap "
>                            "'%s' for block device '%s'",
> -                         s->bitmap_name, s->node_name);
> +                         s->bitmap_name, s->bs->node_name);
>               return -EINVAL;
>           }
>       } else if (!s->bitmap && !nothing) {
> @@ -711,6 +967,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DirtyBitmapLoadState *s)
>   
>   static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>   {
> +    GHashTable *alias_map = NULL;
> +    const MigrationParameters *mig_params = &migrate_get_current()->parameters;

is it OK, that we don't use migration_incoming_get_current() instead?

>       static DirtyBitmapLoadState s;
>       int ret = 0;
>   
> @@ -720,10 +978,15 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>           return -EINVAL;
>       }
>   
> +    if (mig_params->has_block_bitmap_mapping) {
> +        alias_map = construct_alias_map(mig_params->block_bitmap_mapping,
> +                                        false, &error_abort);
> +    }
> +
>       do {
> -        ret = dirty_bitmap_load_header(f, &s);
> +        ret = dirty_bitmap_load_header(f, &s, alias_map);
>           if (ret < 0) {
> -            return ret;
> +            goto fail;
>           }
>   
>           if (s.flags & DIRTY_BITMAP_MIG_FLAG_START) {
> @@ -739,12 +1002,17 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
>           }
>   
>           if (ret) {
> -            return ret;
> +            goto fail;
>           }
>       } while (!(s.flags & DIRTY_BITMAP_MIG_FLAG_EOS));
>   
>       trace_dirty_bitmap_load_success();
> -    return 0;
> +    ret = 0;
> +fail:
> +    if (alias_map) {
> +        g_hash_table_destroy(alias_map);
> +    }
> +    return ret;
>   }
>   
>   static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
> diff --git a/migration/migration.c b/migration/migration.c
> index 47c7da4e55..23fc13e527 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -35,6 +35,7 @@
>   #include "block/block.h"
>   #include "qapi/error.h"
>   #include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-migration.h"
>   #include "qapi/qapi-visit-sockets.h"
>   #include "qapi/qapi-commands-migration.h"
>   #include "qapi/qapi-events-migration.h"
> @@ -825,6 +826,12 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>       params->has_announce_step = true;
>       params->announce_step = s->parameters.announce_step;
>   
> +    if (s->parameters.has_block_bitmap_mapping) {
> +        params->has_block_bitmap_mapping = true;
> +        params->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
> +                                                  params->block_bitmap_mapping);
> +    }
> +
>       return params;
>   }
>   
> @@ -1292,6 +1299,13 @@ static bool migrate_params_check(MigrationParameters *params, Error **errp)
>                      "is invalid, it must be in the range of 1 to 10000 ms");
>          return false;
>       }
> +
> +    if (params->has_block_bitmap_mapping &&
> +        !check_dirty_bitmap_mig_alias_map(params->block_bitmap_mapping, errp)) {
> +        error_prepend(errp, "Invalid mapping given for block-bitmap-mapping: ");
> +        return false;
> +    }
> +
>       return true;
>   }
>   
> @@ -1386,6 +1400,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>       if (params->has_announce_step) {
>           dest->announce_step = params->announce_step;
>       }
> +
> +    if (params->has_block_bitmap_mapping) {
> +        dest->has_block_bitmap_mapping = true;
> +        dest->block_bitmap_mapping = params->block_bitmap_mapping;
> +    }
>   }
>   
>   static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1498,6 +1517,16 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>       if (params->has_announce_step) {
>           s->parameters.announce_step = params->announce_step;
>       }
> +
> +    if (params->has_block_bitmap_mapping) {
> +        qapi_free_BitmapMigrationNodeAliasList(
> +            s->parameters.block_bitmap_mapping);
> +
> +        s->parameters.has_block_bitmap_mapping = true;
> +        s->parameters.block_bitmap_mapping =
> +            QAPI_CLONE(BitmapMigrationNodeAliasList,
> +                       params->block_bitmap_mapping);
> +    }
>   }
>   
>   void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> 


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
> 30.06.2020 11:45, 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            |  83 +++++++-
>>   migration/migration.h          |   3 +
>>   migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
>>   migration/migration.c          |  29 +++
>>   4 files changed, 432 insertions(+), 55 deletions(-)
>>
>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index d5000558c6..5aeae9bea8 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
>> +##
>> +{ 'struct': 'BitmapMigrationBitmapAlias',
>> +  'data': {
>> +      'name': 'str',
>> +      'alias': 'str'
>> +  } }
>> +
>> +##
>> +# @BitmapMigrationNodeAlias:
>> +#
>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>> +# bitmap migration.
>> +#
>> +# @node-name: A block node name.
>> +#
>> +# @alias: An alias block node name for migration (for example the
>> +#         node name on the opposite site).
>> +#
>> +# @bitmaps: Mappings for the bitmaps on this node.
>> +#
>> +# Since: 5.1
>> +##
>> +{ 'struct': 'BitmapMigrationNodeAlias',
>> +  'data': {
>> +      'node-name': 'str',
>> +      'alias': 'str',
>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
> 
> So, we still can't migrate bitmaps from one node to different nodes, but we
> also don't know a usecase for it, so it seems OK. But with such scheme we
> can select and rename bitmaps in-flight, and Peter said about corresponding
> use-case.
> 
> I'm OK with this, still, just an idea in my mind:
> 
> we could instead just have a list of
> 
> BitmapMigrationAlias: {
>  node-name
>  bitmap-name
>  node-alias
>  bitmap-alias
> }
> 
> so, mapping is set for each bitmap in separate.

Well, OK, but why?

>> +  } }
>> +
>>   ##
>>   # @MigrationParameter:
>>   #
>> @@ -641,6 +679,18 @@
>>   #          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.
>> +#          The mapping must be one-to-one and complete: On the source,
> 
> I've recently faced the case where incomplete mapping make sence: shared
> migration of read-only bitmaps in backing files: it seems better to not
> migrate them through migration channel, they are migrating through
> shared storage automatically (yes, we'll pay the time to load them on
> destination, but I'm going to improve it by implementing lazy load of
> read-only and disabled bitmaps, so this will not be a problem).
> 
> So, now I think that it would be good just ignore all the bitmap not
> described by mapping

OK.

>> +#          migrating a bitmap from a node when either is not mapped
>> +#          will result in an error.  On the destination, similarly,
>> +#          receiving a bitmap (by alias) from a node (by alias) when
>> +#          either alias is not mapped will result in an error.
>> +#          By default, all node names and bitmap names are mapped to
>> +#          themselves. (Since 5.1)
> 
> This sentense is unclear, want means "by default" - if the mapping is
> not specified at all or if some nodes/bitmaps are not covered.

It is clear.

> Still,
> tha latter will conflict with previous sentencies, so "by default" is
> about when mapping is not set at all.

Precisely.

> I suggest to word it directly:
> "If mapping is not set (the command never called, or mapping was
> removed".

The mapping cannot be removed.

It’s also technically clear because mentioning a default for some
parameter always means that that particular parameter will have that
default.  So in this case “by default” refers to block-bitmap-mapping,
not anything nested in it.  (Defaults for stuff nested in its value
would be described in the respective structs’ definitions.)

I’d be OK with “By default (when this parameter has never been set)…”.

> And, actual behavior without mapping is not as simple: it actually tries
> to use blk names if possible and node-names if not, and this veries
> from version to version. So, I think not worth to document it in detail,
> just note, that without mapping the behavior is not well defined and
> tries to use block-device name if possible and node-name otherwise. And
> of course direct mapping of bitmap names.

OK.

>> +#
>>   # Since: 2.4
>>   ##
>>   { 'enum': 'MigrationParameter',
>> @@ -655,7 +705,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' ] }
>>     ##
>>   # @MigrateSetParameters:
>> @@ -781,6 +832,18 @@
>>   #          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.
>> +#          The mapping must be one-to-one and complete: On the source,
>> +#          migrating a bitmap from a node when either is not mapped
>> +#          will result in an error.  On the destination, similarly,
>> +#          receiving a bitmap (by alias) from a node (by alias) when
>> +#          either alias is not mapped will result in an error.
>> +#          By default, all node names and bitmap names are mapped to
>> +#          themselves. (Since 5.1)
> 
> Do we really need this duplication? I'd prefere to document it once if
> possible.

Well, or maybe this just shouldn’t be a migration parameter.  I don’t know.

I don’t really want to move this documentation or even just parts of it
to BitmapMigrationNodeAlias, because that’s just one element of the
whole list.

>> +#
>>   # Since: 2.4
>>   ##
>>   # TODO either fuse back into MigrationParameters, or make
>> @@ -811,7 +874,8 @@
>>               '*max-cpu-throttle': 'int',
>>               '*multifd-compression': 'MultiFDCompression',
>>               '*multifd-zlib-level': 'int',
>> -            '*multifd-zstd-level': 'int' } }
>> +            '*multifd-zstd-level': 'int',
>> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>>     ##
>>   # @migrate-set-parameters:
>> @@ -957,6 +1021,18 @@
>>   #          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.
>> +#          The mapping must be one-to-one and complete: On the source,
>> +#          migrating a bitmap from a node when either is not mapped
>> +#          will result in an error.  On the destination, similarly,
>> +#          receiving a bitmap (by alias) from a node (by alias) when
>> +#          either alias is not mapped will result in an error.
>> +#          By default, all node names and bitmap names are mapped to
>> +#          themselves. (Since 5.1)
> 
> once again..
> 
>> +#
>>   # Since: 2.4
>>   ##
>>   { 'struct': 'MigrationParameters',
>> @@ -985,7 +1061,8 @@
>>               '*max-cpu-throttle': 'uint8',
>>               '*multifd-compression': 'MultiFDCompression',
>>               '*multifd-zlib-level': 'uint8',
>> -            '*multifd-zstd-level': 'uint8' } }
>> +            '*multifd-zstd-level': 'uint8',
>> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>>     ##
>>   # @query-migrate-parameters:
>> diff --git a/migration/migration.h b/migration/migration.h
>> index f617960522..038c318b92 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -336,6 +336,9 @@ void
>> migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>>     void dirty_bitmap_mig_before_vm_start(void);
>>   void init_dirty_bitmap_incoming_migration(void);
>> +bool check_dirty_bitmap_mig_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> +                                      Error **errp);
>> +
>>   void migrate_add_address(SocketAddress *address);
>>     int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>> diff --git a/migration/block-dirty-bitmap.c
>> b/migration/block-dirty-bitmap.c
>> index 47bc0f650c..621eb7e3f8 100644
>> --- a/migration/block-dirty-bitmap.c
>> +++ b/migration/block-dirty-bitmap.c
>> @@ -29,10 +29,10 @@
>>    *
>>    * # Header (shared for different chunk types)
>>    * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
>> - * [ 1 byte: node name size ] \  flags & DEVICE_NAME
>> - * [ n bytes: node name     ] /
>> - * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
>> - * [ n bytes: bitmap name     ] /
>> + * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
>> + * [ n bytes: node alias     ] /
>> + * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
>> + * [ n bytes: bitmap alias     ] /
>>    *
>>    * # Start of bitmap migration (flags & START)
>>    * header
>> @@ -72,7 +72,9 @@
>>   #include "migration/register.h"
>>   #include "qemu/hbitmap.h"
>>   #include "qemu/cutils.h"
>> +#include "qemu/id.h"
>>   #include "qapi/error.h"
>> +#include "qapi/qapi-commands-migration.h"
>>   #include "trace.h"
>>     #define CHUNK_SIZE     (1 << 10)
>> @@ -103,7 +105,8 @@
>>   typedef struct DirtyBitmapMigBitmapState {
>>       /* Written during setup phase. */
>>       BlockDriverState *bs;
>> -    const char *node_name;
>> +    char *node_alias;
>> +    char *bitmap_alias;
>>       BdrvDirtyBitmap *bitmap;
>>       uint64_t total_sectors;
>>       uint64_t sectors_per_chunk;
>> @@ -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];
>>       char bitmap_name[256];
>>       BlockDriverState *bs;
>>       BdrvDirtyBitmap *bitmap;
>> @@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
>>   static GSList *enabled_bitmaps;
>>   QemuMutex finish_lock;
>>   +/* For hash tables that map node/bitmap names to aliases */
>> +typedef struct AliasMapInnerNode {
>> +    char *string;
>> +    GHashTable *subtree;
>> +} AliasMapInnerNode;
> 
> Probably, would be simpler to have (node,bitmap) <->
> (node-alias,bitmap-alias) mapping than nested hash-tables, but this
> should work too, I don't have real arguments.

I’m not sure it would be simpler.  Instead of this, we’d need a
structure to hold two strings (and hash and comparison functions for it,
though they’d be simple).

>> +
>> +static void free_alias_map_inner_node(void *amin_ptr)
>> +{
>> +    AliasMapInnerNode *amin = amin_ptr;
>> +
>> +    g_free(amin->string);
>> +    g_hash_table_unref(amin->subtree);
>> +    g_free(amin);
>> +}
>> +
>> +/**
>> + * Construct an alias map based on the given QMP structure.
>> + *
>> + * (Note that we cannot store such maps in the MigrationParameters
>> + * object, because that struct is defined by the QAPI schema, which
>> + * makes it basically impossible to have dicts with arbitrary keys.
>> + * Therefore, we instead have to construct these maps when migration
>> + * starts.)
>> + *
>> + * @bbm is the block_bitmap_mapping from the migration parameters.
>> + *
>> + * If @name_to_alias is true, the returned hash table will map node
>> + * and bitmap names to their respective aliases (for outgoing
>> + * migration).
>> + *
>> + * If @name_to_alias is false, the returned hash table will map node
>> + * and bitmap aliases to their respective names (for incoming
>> + * migration).
>> + *
>> + * The hash table maps node names/aliases to AliasMapInnerNode
>> + * objects, whose .string is the respective node alias/name, and whose
>> + * .subtree table maps bitmap names/aliases to the respective bitmap
>> + * alias/name.
>> + */
>> +static GHashTable *construct_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> +                                       bool name_to_alias,
>> +                                       Error **errp)
>> +{
>> +    GHashTable *alias_map = NULL;
> 
> dead assignment

Indeed.

>> +
>> +    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;
>> +        }
>> +
>> +        if (name_to_alias) {
>> +            if (g_hash_table_contains(alias_map, bmna->node_name)) {
>> +                error_setg(errp, "The node name %s is mapped twice",
>> +                           bmna->node_name);
>> +                goto fail;
>> +            }
>> +
>> +            node_map_from = bmna->node_name;
>> +            node_map_to = bmna->alias;
>> +        } else {
>> +            if (g_hash_table_contains(alias_map, bmna->alias)) {
>> +                error_setg(errp, "The node alias %s is used twice",
>> +                           bmna->alias);
>> +                goto fail;
>> +            }
>> +
>> +            node_map_from = bmna->alias;
>> +            node_map_to = bmna->node_name;
>> +        }
>> +
>> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
>> +                                            g_free, g_free);
>> +
>> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
>> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
>> +            const char *bmap_map_from, *bmap_map_to;
>> +
>> +            if (name_to_alias) {
>> +                bmap_map_from = bmba->name;
>> +                bmap_map_to = bmba->alias;
>> +
>> +                if (g_hash_table_contains(bitmaps_map, bmba->name)) {
>> +                    error_setg(errp, "The bitmap %s/%s is mapped twice",
>> +                               bmna->node_name, bmba->name);
>> +                    goto fail;
> 
> bitmaps_map is leaked here and ..
> 
>> +                }
>> +            } else {
>> +                bmap_map_from = bmba->alias;
>> +                bmap_map_to = bmba->name;
>> +
>> +                if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
>> +                    error_setg(errp, "The bitmap alias %s/%s is used
>> twice",
>> +                               bmna->alias, bmba->alias);
>> +                    goto fail;
> 
> .. here
> 
> Probably, simplest way to fix is to create AliasMapIneerNode and insert
> it into alias_map immediately after allocating bitmaps_map (prior to
> this loop).

Ah, yes.

>> +                }
>> +            }
>> +
>> +            g_hash_table_insert(bitmaps_map,
>> +                                g_strdup(bmap_map_from),
>> g_strdup(bmap_map_to));
>> +        }
>> +
>> +        amin = g_new(AliasMapInnerNode, 1);
>> +        *amin = (AliasMapInnerNode){
> 
> style: space before '{'

Is that required?

>> +            .string = g_strdup(node_map_to),
>> +            .subtree = bitmaps_map,
>> +        };
>> +
>> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
>> +    }
>> +
>> +    return alias_map;
>> +
>> +fail:
>> +    g_hash_table_destroy(alias_map);
>> +    return NULL;
>> +}
>> +
>> +/**
>> + * Run construct_alias_map() in both directions to check whether @bbm
>> + * is valid.
>> + * (This function is to be used by migration/migration.c to validate
>> + * the user-specified block-bitmap-mapping migration parameter.)
>> + *
>> + * Returns true if and only if the mapping is valid.
>> + */
>> +bool check_dirty_bitmap_mig_alias_map(const
>> BitmapMigrationNodeAliasList *bbm,
>> +                                      Error **errp)
>> +{
>> +    GHashTable *alias_map;
>> +
>> +    alias_map = construct_alias_map(bbm, true, errp);
>> +    if (!alias_map) {
>> +        return false;
>> +    }
>> +    g_hash_table_destroy(alias_map);
>> +
>> +    alias_map = construct_alias_map(bbm, false, errp);
>> +    if (!alias_map) {
>> +        return false;
>> +    }
>> +    g_hash_table_destroy(alias_map);
>> +
>> +    return true;
>> +}
>> +
>>   void init_dirty_bitmap_incoming_migration(void)
>>   {
>>       qemu_mutex_init(&finish_lock);
>> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f,
>> DirtyBitmapMigBitmapState *dbms,
>>       qemu_put_bitmap_flags(f, flags);
>>         if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
>> -        qemu_put_counted_string(f, dbms->node_name);
>> +        qemu_put_counted_string(f, dbms->node_alias);
>>       }
>>         if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>> -        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
>> +        qemu_put_counted_string(f, dbms->bitmap_alias);
>>       }
>>   }
>>   @@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
>>           QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
>>           bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>>           bdrv_unref(dbms->bs);
>> +        g_free(dbms->node_alias);
>> +        g_free(dbms->bitmap_alias);
>>           g_free(dbms);
>>       }
>>   }
>>     /* Called with iothread lock taken. */
>> -static int add_bitmaps_to_list(BlockDriverState *bs, const char
>> *bs_name)
>> +static int add_bitmaps_to_list(BlockDriverState *bs, const char
>> *bs_name,
>> +                               GHashTable *alias_map)
>>   {
>>       BdrvDirtyBitmap *bitmap;
>>       DirtyBitmapMigBitmapState *dbms;
>> +    GHashTable *bitmap_aliases;
> 
> can bitmap_aliases be const ptr too?

Unfortunately no because g_hash_table_lookup() expects the hash table to
not be const, for whatever reason.

>> +    const char *node_alias, *bitmap_name, *bitmap_alias;
>>       Error *local_err = NULL;
>>         bitmap = bdrv_dirty_bitmap_first(bs);
>> @@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState
>> *bs, const char *bs_name)
>>           return 0;
>>       }
>>   +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
> 
> Note, that bitmap is wrong here: it may be internal unnamed bitmap which
> we should ignore.
> I have sent a patch for this: "[PATCH] migration/block-dirty-bitmap: fix
> add_bitmaps_to_list"
> 
>> +
>>       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) {
>> +            error_report("Bitmap '%s' on node '%s' with no alias
>> cannot be "
>> +                         "migrated", bitmap_name, bs_name);
> 
> As I've said before, it may be reasonable to ignore bitmaps not
> referenced in the hash-table.

No problem with that.  We just decided on this behavior when we
discussed the RFC.

> And it seems to be good default behavior. We are really tired from
> problems with bitmaps which
> can't migrate for different reasons, when bitmaps are actually
> non-critical data, and choosing
> from customer problems like:
> 
>  1. - Hey, after update migration is broken! It says that some bitmaps
> can't be migrated, what is that?
>  
>  2. - Hmm, it seems, that in some cases, incremental backup doesn't work
> after migration and full backup
>       is automatically done instead.. Why?
> 
> I now prefer the [2].
> 
>> +            return -1;
>> +        }
>> +
>> +        node_alias = amin->string;
>> +        bitmap_aliases = amin->subtree;
>> +    } else {
>> +        node_alias = bs_name;
>> +        bitmap_aliases = NULL;
>> +    }
> 
> Hmm, actually bs_name argument is incompatiblewith alias_map, let's
> assert that they are not non-null simultaneously.
> 
> Ah stop, I see, you use bs_name as node-name later and before.. Hmm,
> seems all this a bit confused.
> 
> Prior the patch, why do we have bs_name: because it may be node-name or
> blk-name, to be use in migration protocol as (actually) an alias, so
> bs_name is more like an alias..
> 
> So, we have bs, which already have bs->node_name, we have alias_map,
> where we have relation node_name -> alias, and we have bs_name, which is
> something like an alias_name.
> 
> I think the most clean thing is to allow only one of bs_name and
> alias_map to be non-null, use bs_name only for old scenario, and for new
> scenario use bdrv_get_node_name() for error-reporting. And a comment
> about function semantics won't hurt.
> 
> upd: aha, I see that in case of new semantics, bs_name is exactly
> bdrv_get_node_name(). It's a bit redundant but OK too.. Let's at least
> add an assertion.

Now I’m confused.  What assertion?  I don’t think I want to add an
assertion that exactly one of bs_name or alias_map is NULL, because that
would complicate the code.  The caller would have to pass NULL for
bs_name, and then add_bitmaps_to_list() would need to fetch the node
name again.  That seems redundant to me.

>> +
>> +    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);
> 
> OK, this should not relate to mapped case, as aliases are well-formed.
> 
>>           return -1;
>>       }
>>         FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
>> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>> +        if (!bitmap_name) {
>>               continue;
>>           }
>>   @@ -302,12 +486,24 @@ static int
>> add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>>               return -1;
>>           }
>>   +        if (bitmap_aliases) {
>> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases,
>> bitmap_name);
>> +            if (!bitmap_alias) {
>> +                error_report("Bitmap '%s' with no alias on node '%s'
>> cannot be "
>> +                             "migrated", bitmap_name, bs_name);
>> +                return -1;
>> +            }
>> +        } else {
>> +            bitmap_alias = bitmap_name;
>> +        }
>> +
> 
> Hmm, we don't error-out if we didn't find a bitmap, specified in
> alias-map. But it seems to be an error from the user point-of-view (the
> required action can't be done).
> 
> So, probably, we want loop through the alias-map (and in this case we
> don't need a map, but can work with QAPI list), find corresponding
> bitmaps and migrate them, and fail if some specified in the alias-map
> bitmap is not found.

Do we?

I don’t consider setting an alias an action request like “Migrate this
bitmap”.  I just consider it establishing a mapping.  If some elements
are not used, so be it.

I don’t know if doing it differently would actually be beneficial for
anyone, but OTOH naively it seems like a more invasive code change.

Max


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

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

[Vladimir Sementsov-Ogievskiy]

02.07.2020 11:09, Max Reitz wrote:
> On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
>> 30.06.2020 11:45, 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            |  83 +++++++-
>>>    migration/migration.h          |   3 +
>>>    migration/block-dirty-bitmap.c | 372 ++++++++++++++++++++++++++++-----
>>>    migration/migration.c          |  29 +++
>>>    4 files changed, 432 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index d5000558c6..5aeae9bea8 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
>>> +##
>>> +{ 'struct': 'BitmapMigrationBitmapAlias',
>>> +  'data': {
>>> +      'name': 'str',
>>> +      'alias': 'str'
>>> +  } }
>>> +
>>> +##
>>> +# @BitmapMigrationNodeAlias:
>>> +#
>>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>>> +# bitmap migration.
>>> +#
>>> +# @node-name: A block node name.
>>> +#
>>> +# @alias: An alias block node name for migration (for example the
>>> +#         node name on the opposite site).
>>> +#
>>> +# @bitmaps: Mappings for the bitmaps on this node.
>>> +#
>>> +# Since: 5.1
>>> +##
>>> +{ 'struct': 'BitmapMigrationNodeAlias',
>>> +  'data': {
>>> +      'node-name': 'str',
>>> +      'alias': 'str',
>>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>>
>> So, we still can't migrate bitmaps from one node to different nodes, but we
>> also don't know a usecase for it, so it seems OK. But with such scheme we
>> can select and rename bitmaps in-flight, and Peter said about corresponding
>> use-case.
>>
>> I'm OK with this, still, just an idea in my mind:
>>
>> we could instead just have a list of
>>
>> BitmapMigrationAlias: {
>>   node-name
>>   bitmap-name
>>   node-alias
>>   bitmap-alias
>> }
>>
>> so, mapping is set for each bitmap in separate.
> 
> Well, OK, but why?

But why not :) Just thinking out loud. May be someone will imaging good reasons for it.

> 
>>> +  } }
>>> +
>>>    ##
>>>    # @MigrationParameter:
>>>    #
>>> @@ -641,6 +679,18 @@
>>>    #          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.
>>> +#          The mapping must be one-to-one and complete: On the source,
>>
>> I've recently faced the case where incomplete mapping make sence: shared
>> migration of read-only bitmaps in backing files: it seems better to not
>> migrate them through migration channel, they are migrating through
>> shared storage automatically (yes, we'll pay the time to load them on
>> destination, but I'm going to improve it by implementing lazy load of
>> read-only and disabled bitmaps, so this will not be a problem).
>>
>> So, now I think that it would be good just ignore all the bitmap not
>> described by mapping
> 
> OK.
> 
>>> +#          migrating a bitmap from a node when either is not mapped
>>> +#          will result in an error.  On the destination, similarly,
>>> +#          receiving a bitmap (by alias) from a node (by alias) when
>>> +#          either alias is not mapped will result in an error.
>>> +#          By default, all node names and bitmap names are mapped to
>>> +#          themselves. (Since 5.1)
>>
>> This sentense is unclear, want means "by default" - if the mapping is
>> not specified at all or if some nodes/bitmaps are not covered.
> 
> It is clear.
> 
>> Still,
>> tha latter will conflict with previous sentencies, so "by default" is
>> about when mapping is not set at all.
> 
> Precisely.
> 
>> I suggest to word it directly:
>> "If mapping is not set (the command never called, or mapping was
>> removed".
> 
> The mapping cannot be removed.
> 
> It’s also technically clear because mentioning a default for some
> parameter always means that that particular parameter will have that
> default.  So in this case “by default” refers to block-bitmap-mapping,
> not anything nested in it.  (Defaults for stuff nested in its value
> would be described in the respective structs’ definitions.)

Reasonable.

> > I’d be OK with “By default (when this parameter has never been set)…”.

Yes, this is absolutely clear.

> 
>> And, actual behavior without mapping is not as simple: it actually tries
>> to use blk names if possible and node-names if not, and this veries
>> from version to version. So, I think not worth to document it in detail,
>> just note, that without mapping the behavior is not well defined and
>> tries to use block-device name if possible and node-name otherwise. And
>> of course direct mapping of bitmap names.
> 
> OK.
> 
>>> +#
>>>    # Since: 2.4
>>>    ##
>>>    { 'enum': 'MigrationParameter',
>>> @@ -655,7 +705,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' ] }
>>>      ##
>>>    # @MigrateSetParameters:
>>> @@ -781,6 +832,18 @@
>>>    #          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.
>>> +#          The mapping must be one-to-one and complete: On the source,
>>> +#          migrating a bitmap from a node when either is not mapped
>>> +#          will result in an error.  On the destination, similarly,
>>> +#          receiving a bitmap (by alias) from a node (by alias) when
>>> +#          either alias is not mapped will result in an error.
>>> +#          By default, all node names and bitmap names are mapped to
>>> +#          themselves. (Since 5.1)
>>
>> Do we really need this duplication? I'd prefere to document it once if
>> possible.
> 
> Well, or maybe this just shouldn’t be a migration parameter.  I don’t know.
> 
> I don’t really want to move this documentation or even just parts of it
> to BitmapMigrationNodeAlias, because that’s just one element of the
> whole list.
> 
>>> +#
>>>    # Since: 2.4
>>>    ##
>>>    # TODO either fuse back into MigrationParameters, or make
>>> @@ -811,7 +874,8 @@
>>>                '*max-cpu-throttle': 'int',
>>>                '*multifd-compression': 'MultiFDCompression',
>>>                '*multifd-zlib-level': 'int',
>>> -            '*multifd-zstd-level': 'int' } }
>>> +            '*multifd-zstd-level': 'int',
>>> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>>>      ##
>>>    # @migrate-set-parameters:
>>> @@ -957,6 +1021,18 @@
>>>    #          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.
>>> +#          The mapping must be one-to-one and complete: On the source,
>>> +#          migrating a bitmap from a node when either is not mapped
>>> +#          will result in an error.  On the destination, similarly,
>>> +#          receiving a bitmap (by alias) from a node (by alias) when
>>> +#          either alias is not mapped will result in an error.
>>> +#          By default, all node names and bitmap names are mapped to
>>> +#          themselves. (Since 5.1)
>>
>> once again..
>>
>>> +#
>>>    # Since: 2.4
>>>    ##
>>>    { 'struct': 'MigrationParameters',
>>> @@ -985,7 +1061,8 @@
>>>                '*max-cpu-throttle': 'uint8',
>>>                '*multifd-compression': 'MultiFDCompression',
>>>                '*multifd-zlib-level': 'uint8',
>>> -            '*multifd-zstd-level': 'uint8' } }
>>> +            '*multifd-zstd-level': 'uint8',
>>> +            '*block-bitmap-mapping': [ 'BitmapMigrationNodeAlias' ] } }
>>>      ##
>>>    # @query-migrate-parameters:
>>> diff --git a/migration/migration.h b/migration/migration.h
>>> index f617960522..038c318b92 100644
>>> --- a/migration/migration.h
>>> +++ b/migration/migration.h
>>> @@ -336,6 +336,9 @@ void
>>> migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
>>>      void dirty_bitmap_mig_before_vm_start(void);
>>>    void init_dirty_bitmap_incoming_migration(void);
>>> +bool check_dirty_bitmap_mig_alias_map(const
>>> BitmapMigrationNodeAliasList *bbm,
>>> +                                      Error **errp);
>>> +
>>>    void migrate_add_address(SocketAddress *address);
>>>      int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
>>> diff --git a/migration/block-dirty-bitmap.c
>>> b/migration/block-dirty-bitmap.c
>>> index 47bc0f650c..621eb7e3f8 100644
>>> --- a/migration/block-dirty-bitmap.c
>>> +++ b/migration/block-dirty-bitmap.c
>>> @@ -29,10 +29,10 @@
>>>     *
>>>     * # Header (shared for different chunk types)
>>>     * 1, 2 or 4 bytes: flags (see qemu_{put,put}_flags)
>>> - * [ 1 byte: node name size ] \  flags & DEVICE_NAME
>>> - * [ n bytes: node name     ] /
>>> - * [ 1 byte: bitmap name size ] \  flags & BITMAP_NAME
>>> - * [ n bytes: bitmap name     ] /
>>> + * [ 1 byte: node alias size ] \  flags & DEVICE_NAME
>>> + * [ n bytes: node alias     ] /
>>> + * [ 1 byte: bitmap alias size ] \  flags & BITMAP_NAME
>>> + * [ n bytes: bitmap alias     ] /
>>>     *
>>>     * # Start of bitmap migration (flags & START)
>>>     * header
>>> @@ -72,7 +72,9 @@
>>>    #include "migration/register.h"
>>>    #include "qemu/hbitmap.h"
>>>    #include "qemu/cutils.h"
>>> +#include "qemu/id.h"
>>>    #include "qapi/error.h"
>>> +#include "qapi/qapi-commands-migration.h"
>>>    #include "trace.h"
>>>      #define CHUNK_SIZE     (1 << 10)
>>> @@ -103,7 +105,8 @@
>>>    typedef struct DirtyBitmapMigBitmapState {
>>>        /* Written during setup phase. */
>>>        BlockDriverState *bs;
>>> -    const char *node_name;
>>> +    char *node_alias;
>>> +    char *bitmap_alias;
>>>        BdrvDirtyBitmap *bitmap;
>>>        uint64_t total_sectors;
>>>        uint64_t sectors_per_chunk;
>>> @@ -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];
>>>        char bitmap_name[256];
>>>        BlockDriverState *bs;
>>>        BdrvDirtyBitmap *bitmap;
>>> @@ -144,6 +148,162 @@ typedef struct DirtyBitmapLoadBitmapState {
>>>    static GSList *enabled_bitmaps;
>>>    QemuMutex finish_lock;
>>>    +/* For hash tables that map node/bitmap names to aliases */
>>> +typedef struct AliasMapInnerNode {
>>> +    char *string;
>>> +    GHashTable *subtree;
>>> +} AliasMapInnerNode;
>>
>> Probably, would be simpler to have (node,bitmap) <->
>> (node-alias,bitmap-alias) mapping than nested hash-tables, but this
>> should work too, I don't have real arguments.
> 
> I’m not sure it would be simpler.  Instead of this, we’d need a
> structure to hold two strings (and hash and comparison functions for it,
> though they’d be simple).

Yes, I thought about that too. So, consider it thinking out loud.

> 
>>> +
>>> +static void free_alias_map_inner_node(void *amin_ptr)
>>> +{
>>> +    AliasMapInnerNode *amin = amin_ptr;
>>> +
>>> +    g_free(amin->string);
>>> +    g_hash_table_unref(amin->subtree);
>>> +    g_free(amin);
>>> +}
>>> +
>>> +/**
>>> + * Construct an alias map based on the given QMP structure.
>>> + *
>>> + * (Note that we cannot store such maps in the MigrationParameters
>>> + * object, because that struct is defined by the QAPI schema, which
>>> + * makes it basically impossible to have dicts with arbitrary keys.
>>> + * Therefore, we instead have to construct these maps when migration
>>> + * starts.)
>>> + *
>>> + * @bbm is the block_bitmap_mapping from the migration parameters.
>>> + *
>>> + * If @name_to_alias is true, the returned hash table will map node
>>> + * and bitmap names to their respective aliases (for outgoing
>>> + * migration).
>>> + *
>>> + * If @name_to_alias is false, the returned hash table will map node
>>> + * and bitmap aliases to their respective names (for incoming
>>> + * migration).
>>> + *
>>> + * The hash table maps node names/aliases to AliasMapInnerNode
>>> + * objects, whose .string is the respective node alias/name, and whose
>>> + * .subtree table maps bitmap names/aliases to the respective bitmap
>>> + * alias/name.
>>> + */
>>> +static GHashTable *construct_alias_map(const
>>> BitmapMigrationNodeAliasList *bbm,
>>> +                                       bool name_to_alias,
>>> +                                       Error **errp)
>>> +{
>>> +    GHashTable *alias_map = NULL;
>>
>> dead assignment
> 
> Indeed.
> 
>>> +
>>> +    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;
>>> +        }
>>> +
>>> +        if (name_to_alias) {
>>> +            if (g_hash_table_contains(alias_map, bmna->node_name)) {
>>> +                error_setg(errp, "The node name %s is mapped twice",
>>> +                           bmna->node_name);
>>> +                goto fail;
>>> +            }
>>> +
>>> +            node_map_from = bmna->node_name;
>>> +            node_map_to = bmna->alias;
>>> +        } else {
>>> +            if (g_hash_table_contains(alias_map, bmna->alias)) {
>>> +                error_setg(errp, "The node alias %s is used twice",
>>> +                           bmna->alias);
>>> +                goto fail;
>>> +            }
>>> +
>>> +            node_map_from = bmna->alias;
>>> +            node_map_to = bmna->node_name;
>>> +        }
>>> +
>>> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
>>> +                                            g_free, g_free);
>>> +
>>> +        for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
>>> +            const BitmapMigrationBitmapAlias *bmba = bmbal->value;
>>> +            const char *bmap_map_from, *bmap_map_to;
>>> +
>>> +            if (name_to_alias) {
>>> +                bmap_map_from = bmba->name;
>>> +                bmap_map_to = bmba->alias;
>>> +
>>> +                if (g_hash_table_contains(bitmaps_map, bmba->name)) {
>>> +                    error_setg(errp, "The bitmap %s/%s is mapped twice",
>>> +                               bmna->node_name, bmba->name);
>>> +                    goto fail;
>>
>> bitmaps_map is leaked here and ..
>>
>>> +                }
>>> +            } else {
>>> +                bmap_map_from = bmba->alias;
>>> +                bmap_map_to = bmba->name;
>>> +
>>> +                if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
>>> +                    error_setg(errp, "The bitmap alias %s/%s is used
>>> twice",
>>> +                               bmna->alias, bmba->alias);
>>> +                    goto fail;
>>
>> .. here
>>
>> Probably, simplest way to fix is to create AliasMapIneerNode and insert
>> it into alias_map immediately after allocating bitmaps_map (prior to
>> this loop).
> 
> Ah, yes.
> 
>>> +                }
>>> +            }
>>> +
>>> +            g_hash_table_insert(bitmaps_map,
>>> +                                g_strdup(bmap_map_from),
>>> g_strdup(bmap_map_to));
>>> +        }
>>> +
>>> +        amin = g_new(AliasMapInnerNode, 1);
>>> +        *amin = (AliasMapInnerNode){
>>
>> style: space before '{'
> 
> Is that required?

If checkpatch doesn't complain, than not..

> 
>>> +            .string = g_strdup(node_map_to),
>>> +            .subtree = bitmaps_map,
>>> +        };
>>> +
>>> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
>>> +    }
>>> +
>>> +    return alias_map;
>>> +
>>> +fail:
>>> +    g_hash_table_destroy(alias_map);
>>> +    return NULL;
>>> +}
>>> +
>>> +/**
>>> + * Run construct_alias_map() in both directions to check whether @bbm
>>> + * is valid.
>>> + * (This function is to be used by migration/migration.c to validate
>>> + * the user-specified block-bitmap-mapping migration parameter.)
>>> + *
>>> + * Returns true if and only if the mapping is valid.
>>> + */
>>> +bool check_dirty_bitmap_mig_alias_map(const
>>> BitmapMigrationNodeAliasList *bbm,
>>> +                                      Error **errp)
>>> +{
>>> +    GHashTable *alias_map;
>>> +
>>> +    alias_map = construct_alias_map(bbm, true, errp);
>>> +    if (!alias_map) {
>>> +        return false;
>>> +    }
>>> +    g_hash_table_destroy(alias_map);
>>> +
>>> +    alias_map = construct_alias_map(bbm, false, errp);
>>> +    if (!alias_map) {
>>> +        return false;
>>> +    }
>>> +    g_hash_table_destroy(alias_map);
>>> +
>>> +    return true;
>>> +}
>>> +
>>>    void init_dirty_bitmap_incoming_migration(void)
>>>    {
>>>        qemu_mutex_init(&finish_lock);
>>> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f,
>>> DirtyBitmapMigBitmapState *dbms,
>>>        qemu_put_bitmap_flags(f, flags);
>>>          if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
>>> -        qemu_put_counted_string(f, dbms->node_name);
>>> +        qemu_put_counted_string(f, dbms->node_alias);
>>>        }
>>>          if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>>> -        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
>>> +        qemu_put_counted_string(f, dbms->bitmap_alias);
>>>        }
>>>    }
>>>    @@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
>>>            QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry);
>>>            bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>>>            bdrv_unref(dbms->bs);
>>> +        g_free(dbms->node_alias);
>>> +        g_free(dbms->bitmap_alias);
>>>            g_free(dbms);
>>>        }
>>>    }
>>>      /* Called with iothread lock taken. */
>>> -static int add_bitmaps_to_list(BlockDriverState *bs, const char
>>> *bs_name)
>>> +static int add_bitmaps_to_list(BlockDriverState *bs, const char
>>> *bs_name,
>>> +                               GHashTable *alias_map)
>>>    {
>>>        BdrvDirtyBitmap *bitmap;
>>>        DirtyBitmapMigBitmapState *dbms;
>>> +    GHashTable *bitmap_aliases;
>>
>> can bitmap_aliases be const ptr too?
> 
> Unfortunately no because g_hash_table_lookup() expects the hash table to
> not be const, for whatever reason.
> 
>>> +    const char *node_alias, *bitmap_name, *bitmap_alias;
>>>        Error *local_err = NULL;
>>>          bitmap = bdrv_dirty_bitmap_first(bs);
>>> @@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState
>>> *bs, const char *bs_name)
>>>            return 0;
>>>        }
>>>    +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>>
>> Note, that bitmap is wrong here: it may be internal unnamed bitmap which
>> we should ignore.
>> I have sent a patch for this: "[PATCH] migration/block-dirty-bitmap: fix
>> add_bitmaps_to_list"
>>
>>> +
>>>        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) {
>>> +            error_report("Bitmap '%s' on node '%s' with no alias
>>> cannot be "
>>> +                         "migrated", bitmap_name, bs_name);
>>
>> As I've said before, it may be reasonable to ignore bitmaps not
>> referenced in the hash-table.
> 
> No problem with that.  We just decided on this behavior when we
> discussed the RFC.

Sorry for that. The reason for my changed opinion is a recent bug from
customers about bitmap migration.

> 
>> And it seems to be good default behavior. We are really tired from
>> problems with bitmaps which
>> can't migrate for different reasons, when bitmaps are actually
>> non-critical data, and choosing
>> from customer problems like:
>>
>>   1. - Hey, after update migration is broken! It says that some bitmaps
>> can't be migrated, what is that?
>>   
>>   2. - Hmm, it seems, that in some cases, incremental backup doesn't work
>> after migration and full backup
>>        is automatically done instead.. Why?
>>
>> I now prefer the [2].
>>
>>> +            return -1;
>>> +        }
>>> +
>>> +        node_alias = amin->string;
>>> +        bitmap_aliases = amin->subtree;
>>> +    } else {
>>> +        node_alias = bs_name;
>>> +        bitmap_aliases = NULL;
>>> +    }
>>
>> Hmm, actually bs_name argument is incompatiblewith alias_map, let's
>> assert that they are not non-null simultaneously.
>>
>> Ah stop, I see, you use bs_name as node-name later and before.. Hmm,
>> seems all this a bit confused.
>>
>> Prior the patch, why do we have bs_name: because it may be node-name or
>> blk-name, to be use in migration protocol as (actually) an alias, so
>> bs_name is more like an alias..
>>
>> So, we have bs, which already have bs->node_name, we have alias_map,
>> where we have relation node_name -> alias, and we have bs_name, which is
>> something like an alias_name.
>>
>> I think the most clean thing is to allow only one of bs_name and
>> alias_map to be non-null, use bs_name only for old scenario, and for new
>> scenario use bdrv_get_node_name() for error-reporting. And a comment
>> about function semantics won't hurt.
>>
>> upd: aha, I see that in case of new semantics, bs_name is exactly
>> bdrv_get_node_name(). It's a bit redundant but OK too.. Let's at least
>> add an assertion.
> 
> Now I’m confused.  What assertion?  I don’t think I want to add an
> assertion that exactly one of bs_name or alias_map is NULL, because that
> would complicate the code.  The caller would have to pass NULL for
> bs_name, and then add_bitmaps_to_list() would need to fetch the node
> name again.  That seems redundant to me.

I mean something like

    assert(!alias_map || !strcmp(alias_map, bdrv_get_node_name(bs));

I am afraid of interfaces with redundant parameters, it seems strange
and unsafe to pass node_name together with bs, which has bs->node_name
which is (and must be, in case of new semantics with alias_map) the same.

Still, I don't mind keeping it as is, I can think of some refactoring (if any) later.

> 
>>> +
>>> +    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);
>>
>> OK, this should not relate to mapped case, as aliases are well-formed.
>>
>>>            return -1;
>>>        }
>>>          FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>>> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
>>> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>>> +        if (!bitmap_name) {
>>>                continue;
>>>            }
>>>    @@ -302,12 +486,24 @@ static int
>>> add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>>>                return -1;
>>>            }
>>>    +        if (bitmap_aliases) {
>>> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases,
>>> bitmap_name);
>>> +            if (!bitmap_alias) {
>>> +                error_report("Bitmap '%s' with no alias on node '%s'
>>> cannot be "
>>> +                             "migrated", bitmap_name, bs_name);
>>> +                return -1;
>>> +            }
>>> +        } else {
>>> +            bitmap_alias = bitmap_name;
>>> +        }
>>> +
>>
>> Hmm, we don't error-out if we didn't find a bitmap, specified in
>> alias-map. But it seems to be an error from the user point-of-view (the
>> required action can't be done).
>>
>> So, probably, we want loop through the alias-map (and in this case we
>> don't need a map, but can work with QAPI list), find corresponding
>> bitmaps and migrate them, and fail if some specified in the alias-map
>> bitmap is not found.
> 
> Do we?
> 
> I don’t consider setting an alias an action request like “Migrate this
> bitmap”.  I just consider it establishing a mapping.  If some elements
> are not used, so be it.

OK. This non-strict behavior is in good relation with ignoring not-mapped bitmaps which I've proposed. We can add any kind of restrictions as an option later.

> 
> I don’t know if doing it differently would actually be beneficial for
> anyone, but OTOH naively it seems like a more invasive code change.
> 

I don't see real benefits, we can go either way, so, not worth rewriting the patch.

===

I feel like a stupid reviewer :)

-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote:
> 02.07.2020 11:09, Max Reitz wrote:
>> On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
>>> 30.06.2020 11:45, 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            |  83 +++++++-
>>>>    migration/migration.h          |   3 +
>>>>    migration/block-dirty-bitmap.c | 372
>>>> ++++++++++++++++++++++++++++-----
>>>>    migration/migration.c          |  29 +++
>>>>    4 files changed, 432 insertions(+), 55 deletions(-)
>>>>
>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>> index d5000558c6..5aeae9bea8 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
>>>> +##
>>>> +{ 'struct': 'BitmapMigrationBitmapAlias',
>>>> +  'data': {
>>>> +      'name': 'str',
>>>> +      'alias': 'str'
>>>> +  } }
>>>> +
>>>> +##
>>>> +# @BitmapMigrationNodeAlias:
>>>> +#
>>>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>>>> +# bitmap migration.
>>>> +#
>>>> +# @node-name: A block node name.
>>>> +#
>>>> +# @alias: An alias block node name for migration (for example the
>>>> +#         node name on the opposite site).
>>>> +#
>>>> +# @bitmaps: Mappings for the bitmaps on this node.
>>>> +#
>>>> +# Since: 5.1
>>>> +##
>>>> +{ 'struct': 'BitmapMigrationNodeAlias',
>>>> +  'data': {
>>>> +      'node-name': 'str',
>>>> +      'alias': 'str',
>>>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>>>
>>> So, we still can't migrate bitmaps from one node to different nodes,
>>> but we
>>> also don't know a usecase for it, so it seems OK. But with such
>>> scheme we
>>> can select and rename bitmaps in-flight, and Peter said about
>>> corresponding
>>> use-case.
>>>
>>> I'm OK with this, still, just an idea in my mind:
>>>
>>> we could instead just have a list of
>>>
>>> BitmapMigrationAlias: {
>>>   node-name
>>>   bitmap-name
>>>   node-alias
>>>   bitmap-alias
>>> }
>>>
>>> so, mapping is set for each bitmap in separate.
>>
>> Well, OK, but why?
> 
> But why not :) Just thinking out loud. May be someone will imaging good
> reasons for it.

The reason for “Why not” is that this code now exists. ;)

>>>> +                }
>>>> +            }
>>>> +
>>>> +            g_hash_table_insert(bitmaps_map,
>>>> +                                g_strdup(bmap_map_from),
>>>> g_strdup(bmap_map_to));
>>>> +        }
>>>> +
>>>> +        amin = g_new(AliasMapInnerNode, 1);
>>>> +        *amin = (AliasMapInnerNode){
>>>
>>> style: space before '{'
>>
>> Is that required?
> 
> If checkpatch doesn't complain, than not..

O:)

>>>> +            .string = g_strdup(node_map_to),
>>>> +            .subtree = bitmaps_map,
>>>> +        };
>>>> +
>>>> +        g_hash_table_insert(alias_map, g_strdup(node_map_from), amin);
>>>> +    }
>>>> +
>>>> +    return alias_map;
>>>> +
>>>> +fail:
>>>> +    g_hash_table_destroy(alias_map);
>>>> +    return NULL;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Run construct_alias_map() in both directions to check whether @bbm
>>>> + * is valid.
>>>> + * (This function is to be used by migration/migration.c to validate
>>>> + * the user-specified block-bitmap-mapping migration parameter.)
>>>> + *
>>>> + * Returns true if and only if the mapping is valid.
>>>> + */
>>>> +bool check_dirty_bitmap_mig_alias_map(const
>>>> BitmapMigrationNodeAliasList *bbm,
>>>> +                                      Error **errp)
>>>> +{
>>>> +    GHashTable *alias_map;
>>>> +
>>>> +    alias_map = construct_alias_map(bbm, true, errp);
>>>> +    if (!alias_map) {
>>>> +        return false;
>>>> +    }
>>>> +    g_hash_table_destroy(alias_map);
>>>> +
>>>> +    alias_map = construct_alias_map(bbm, false, errp);
>>>> +    if (!alias_map) {
>>>> +        return false;
>>>> +    }
>>>> +    g_hash_table_destroy(alias_map);
>>>> +
>>>> +    return true;
>>>> +}
>>>> +
>>>>    void init_dirty_bitmap_incoming_migration(void)
>>>>    {
>>>>        qemu_mutex_init(&finish_lock);
>>>> @@ -191,11 +351,11 @@ static void send_bitmap_header(QEMUFile *f,
>>>> DirtyBitmapMigBitmapState *dbms,
>>>>        qemu_put_bitmap_flags(f, flags);
>>>>          if (flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
>>>> -        qemu_put_counted_string(f, dbms->node_name);
>>>> +        qemu_put_counted_string(f, dbms->node_alias);
>>>>        }
>>>>          if (flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>>>> -        qemu_put_counted_string(f, bdrv_dirty_bitmap_name(bitmap));
>>>> +        qemu_put_counted_string(f, dbms->bitmap_alias);
>>>>        }
>>>>    }
>>>>    @@ -263,15 +423,20 @@ static void dirty_bitmap_mig_cleanup(void)
>>>>            QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list,
>>>> entry);
>>>>            bdrv_dirty_bitmap_set_busy(dbms->bitmap, false);
>>>>            bdrv_unref(dbms->bs);
>>>> +        g_free(dbms->node_alias);
>>>> +        g_free(dbms->bitmap_alias);
>>>>            g_free(dbms);
>>>>        }
>>>>    }
>>>>      /* Called with iothread lock taken. */
>>>> -static int add_bitmaps_to_list(BlockDriverState *bs, const char
>>>> *bs_name)
>>>> +static int add_bitmaps_to_list(BlockDriverState *bs, const char
>>>> *bs_name,
>>>> +                               GHashTable *alias_map)
>>>>    {
>>>>        BdrvDirtyBitmap *bitmap;
>>>>        DirtyBitmapMigBitmapState *dbms;
>>>> +    GHashTable *bitmap_aliases;
>>>
>>> can bitmap_aliases be const ptr too?
>>
>> Unfortunately no because g_hash_table_lookup() expects the hash table to
>> not be const, for whatever reason.
>>
>>>> +    const char *node_alias, *bitmap_name, *bitmap_alias;
>>>>        Error *local_err = NULL;
>>>>          bitmap = bdrv_dirty_bitmap_first(bs);
>>>> @@ -279,21 +444,40 @@ static int add_bitmaps_to_list(BlockDriverState
>>>> *bs, const char *bs_name)
>>>>            return 0;
>>>>        }
>>>>    +    bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>>>
>>> Note, that bitmap is wrong here: it may be internal unnamed bitmap which
>>> we should ignore.
>>> I have sent a patch for this: "[PATCH] migration/block-dirty-bitmap: fix
>>> add_bitmaps_to_list"
>>>
>>>> +
>>>>        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) {
>>>> +            error_report("Bitmap '%s' on node '%s' with no alias
>>>> cannot be "
>>>> +                         "migrated", bitmap_name, bs_name);
>>>
>>> As I've said before, it may be reasonable to ignore bitmaps not
>>> referenced in the hash-table.
>>
>> No problem with that.  We just decided on this behavior when we
>> discussed the RFC.
> 
> Sorry for that. The reason for my changed opinion is a recent bug from
> customers about bitmap migration.

No problem.  (My original proposal was different still, where
non-specified mappings would default to the identity mapping.)

>>> And it seems to be good default behavior. We are really tired from
>>> problems with bitmaps which
>>> can't migrate for different reasons, when bitmaps are actually
>>> non-critical data, and choosing
>>> from customer problems like:
>>>
>>>   1. - Hey, after update migration is broken! It says that some bitmaps
>>> can't be migrated, what is that?
>>>     2. - Hmm, it seems, that in some cases, incremental backup
>>> doesn't work
>>> after migration and full backup
>>>        is automatically done instead.. Why?
>>>
>>> I now prefer the [2].
>>>
>>>> +            return -1;
>>>> +        }
>>>> +
>>>> +        node_alias = amin->string;
>>>> +        bitmap_aliases = amin->subtree;
>>>> +    } else {
>>>> +        node_alias = bs_name;
>>>> +        bitmap_aliases = NULL;
>>>> +    }
>>>
>>> Hmm, actually bs_name argument is incompatiblewith alias_map, let's
>>> assert that they are not non-null simultaneously.
>>>
>>> Ah stop, I see, you use bs_name as node-name later and before.. Hmm,
>>> seems all this a bit confused.
>>>
>>> Prior the patch, why do we have bs_name: because it may be node-name or
>>> blk-name, to be use in migration protocol as (actually) an alias, so
>>> bs_name is more like an alias..
>>>
>>> So, we have bs, which already have bs->node_name, we have alias_map,
>>> where we have relation node_name -> alias, and we have bs_name, which is
>>> something like an alias_name.
>>>
>>> I think the most clean thing is to allow only one of bs_name and
>>> alias_map to be non-null, use bs_name only for old scenario, and for new
>>> scenario use bdrv_get_node_name() for error-reporting. And a comment
>>> about function semantics won't hurt.
>>>
>>> upd: aha, I see that in case of new semantics, bs_name is exactly
>>> bdrv_get_node_name(). It's a bit redundant but OK too.. Let's at least
>>> add an assertion.
>>
>> Now I’m confused.  What assertion?  I don’t think I want to add an
>> assertion that exactly one of bs_name or alias_map is NULL, because that
>> would complicate the code.  The caller would have to pass NULL for
>> bs_name, and then add_bitmaps_to_list() would need to fetch the node
>> name again.  That seems redundant to me.
> 
> I mean something like
> 
>    assert(!alias_map || !strcmp(alias_map, bdrv_get_node_name(bs));

Ah, OK, sure.  (with s/alias_map/bs_name/ in the strcmp(), I presume)

> I am afraid of interfaces with redundant parameters, it seems strange
> and unsafe to pass node_name together with bs, which has bs->node_name
> which is (and must be, in case of new semantics with alias_map) the same.
> 
> Still, I don't mind keeping it as is, I can think of some refactoring
> (if any) later.
> 
>>
>>>> +
>>>> +    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);
>>>
>>> OK, this should not relate to mapped case, as aliases are well-formed.
>>>
>>>>            return -1;
>>>>        }
>>>>          FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
>>>> -        if (!bdrv_dirty_bitmap_name(bitmap)) {
>>>> +        bitmap_name = bdrv_dirty_bitmap_name(bitmap);
>>>> +        if (!bitmap_name) {
>>>>                continue;
>>>>            }
>>>>    @@ -302,12 +486,24 @@ static int
>>>> add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
>>>>                return -1;
>>>>            }
>>>>    +        if (bitmap_aliases) {
>>>> +            bitmap_alias = g_hash_table_lookup(bitmap_aliases,
>>>> bitmap_name);
>>>> +            if (!bitmap_alias) {
>>>> +                error_report("Bitmap '%s' with no alias on node '%s'
>>>> cannot be "
>>>> +                             "migrated", bitmap_name, bs_name);
>>>> +                return -1;
>>>> +            }
>>>> +        } else {
>>>> +            bitmap_alias = bitmap_name;
>>>> +        }
>>>> +
>>>
>>> Hmm, we don't error-out if we didn't find a bitmap, specified in
>>> alias-map. But it seems to be an error from the user point-of-view (the
>>> required action can't be done).
>>>
>>> So, probably, we want loop through the alias-map (and in this case we
>>> don't need a map, but can work with QAPI list), find corresponding
>>> bitmaps and migrate them, and fail if some specified in the alias-map
>>> bitmap is not found.
>>
>> Do we?
>>
>> I don’t consider setting an alias an action request like “Migrate this
>> bitmap”.  I just consider it establishing a mapping.  If some elements
>> are not used, so be it.
> 
> OK. This non-strict behavior is in good relation with ignoring
> not-mapped bitmaps which I've proposed.

I think so, too.

> We can add any kind of restrictions as an option later.

Oh, yes, that, too.  Like you proposed in the RFC, we could add e.g. a
block-bitmap-mapping-strictness migration parameter at a later point if
there’s a use for something like it.

>> I don’t know if doing it differently would actually be beneficial for
>> anyone, but OTOH naively it seems like a more invasive code change.
>>
> 
> I don't see real benefits, we can go either way, so, not worth rewriting
> the patch.
> 
> ===
> 
> I feel like a stupid reviewer :)

Huh?  If anything, a stupid review on a design-changing patch would be a
plain “R-b” without having actually considered the impact.  You do
consider the impact and question it in all places.

I don’t think I need to mention this, but that’s a very good and
important thing to do, because it forces me to reason why we’d want this
or that design.  Without being questioned, I wouldn’t have to reason
about that.  (Which may be a problem in our patch workflow – authors
don’t need to reason unless questioned.[1])

Sorry if I gave the impression of dismissing your comments.  It should
be my burden to reason why I took certain design decisions.

Max


[1] I suppose I should address this for my own patches by meticulously
writing down everything where I had to question myself and then always
include my reasoning in the patch notes somewhere.


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

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

[Vladimir Sementsov-Ogievskiy]

02.07.2020 12:41, Max Reitz wrote:
> On 02.07.20 11:19, Vladimir Sementsov-Ogievskiy wrote:
>> 02.07.2020 11:09, Max Reitz wrote:
>>> On 01.07.20 16:34, Vladimir Sementsov-Ogievskiy wrote:
>>>> 30.06.2020 11:45, 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            |  83 +++++++-
>>>>>     migration/migration.h          |   3 +
>>>>>     migration/block-dirty-bitmap.c | 372
>>>>> ++++++++++++++++++++++++++++-----
>>>>>     migration/migration.c          |  29 +++
>>>>>     4 files changed, 432 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>>>> index d5000558c6..5aeae9bea8 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
>>>>> +##
>>>>> +{ 'struct': 'BitmapMigrationBitmapAlias',
>>>>> +  'data': {
>>>>> +      'name': 'str',
>>>>> +      'alias': 'str'
>>>>> +  } }
>>>>> +
>>>>> +##
>>>>> +# @BitmapMigrationNodeAlias:
>>>>> +#
>>>>> +# Maps a block node name and the bitmaps it has to aliases for dirty
>>>>> +# bitmap migration.
>>>>> +#
>>>>> +# @node-name: A block node name.
>>>>> +#
>>>>> +# @alias: An alias block node name for migration (for example the
>>>>> +#         node name on the opposite site).
>>>>> +#
>>>>> +# @bitmaps: Mappings for the bitmaps on this node.
>>>>> +#
>>>>> +# Since: 5.1
>>>>> +##
>>>>> +{ 'struct': 'BitmapMigrationNodeAlias',
>>>>> +  'data': {
>>>>> +      'node-name': 'str',
>>>>> +      'alias': 'str',
>>>>> +      'bitmaps': [ 'BitmapMigrationBitmapAlias' ]
>>>>
>>>> So, we still can't migrate bitmaps from one node to different nodes,
>>>> but we
>>>> also don't know a usecase for it, so it seems OK. But with such
>>>> scheme we
>>>> can select and rename bitmaps in-flight, and Peter said about
>>>> corresponding
>>>> use-case.
>>>>
>>>> I'm OK with this, still, just an idea in my mind:
>>>>
>>>> we could instead just have a list of
>>>>
>>>> BitmapMigrationAlias: {
>>>>    node-name
>>>>    bitmap-name
>>>>    node-alias
>>>>    bitmap-alias
>>>> }
>>>>
>>>> so, mapping is set for each bitmap in separate.
>>>
>>> Well, OK, but why?
>>
>> But why not :) Just thinking out loud. May be someone will imaging good
>> reasons for it.
> 
> The reason for “Why not” is that this code now exists. ;)

Exactly :) But another arguments may appear, who knows. If not - great.

Actual reason is more flexible interface: you can migrate any bitmap to any node with any name (except for conflicts of course). But do we need it, I don't know. I'd like to hear Peter's opinion, if he don't have preference, then I don't care too.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

02.07.2020 12:41, Max Reitz wrote:
>>> I don’t know if doing it differently would actually be beneficial for
>>> anyone, but OTOH naively it seems like a more invasive code change.
>>>
>> I don't see real benefits, we can go either way, so, not worth rewriting
>> the patch.
>>
>> ===
>>
>> I feel like a stupid reviewer:)
> Huh?  If anything, a stupid review on a design-changing patch would be a
> plain “R-b” without having actually considered the impact.  You do
> consider the impact and question it in all places.
> 
> I don’t think I need to mention this, but that’s a very good and
> important thing to do, because it forces me to reason why we’d want this
> or that design.  Without being questioned, I wouldn’t have to reason
> about that.  (Which may be a problem in our patch workflow – authors
> don’t need to reason unless questioned.[1])
> 
> Sorry if I gave the impression of dismissing your comments.  It should
> be my burden to reason why I took certain design decisions.

No problem. I should better describe reasons for my suggestions as well, or
if I have no one, mark it as "thinking-out-loud" instead of recommended change.

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

02.07.2020 12:41, Max Reitz wrote:
>>>> As I've said before, it may be reasonable to ignore bitmaps not
>>>> referenced in the hash-table.
>>> No problem with that.  We just decided on this behavior when we
>>> discussed the RFC.
>> Sorry for that. The reason for my changed opinion is a recent bug from
>> customers about bitmap migration.
> No problem.  (My original proposal was different still, where
> non-specified mappings would default to the identity mapping.)
> 

And, remembering, what my series "[PATCH v2 00/22] Fix error handling during bitmap postcopy" is for, I have one more reason for non-strict behavior around bitmap migration in general:

Bitmaps are migrating postcopy. We can't rollback in general, so we should ignore any error during bitmaps migration, as it's non critical data, and we must not break already running target due to some bitmap issue. Failed bitmap migration should never be a problem, the worst consequence is necessity to create a full backup instead of incremental.


-- 
Best regards,
Vladimir

[PATCH 3/4] iotests.py: Add wait_for_runstate()

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/iotests.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index ef739dd1e3..32f262db5a 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -799,6 +799,10 @@ class VM(qtest.QEMUQtestMachine):
                    'Found node %s under %s (but expected %s)' % \
                    (node['name'], path, expected_node)
 
+    def wait_for_runstate(self, runstate: str) -> None:
+        while self.qmp('query-status')['return']['status'] != runstate:
+            pass
+
 index_re = re.compile(r'([^\[]+)\[([^\]]+)\]')
 
 class QMPTestCase(unittest.TestCase):
-- 
2.26.2

[PATCH 4/4] iotests: Test node/bitmap aliases during migration

[Max Reitz]

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/300     | 487 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 493 insertions(+)
 create mode 100755 tests/qemu-iotests/300
 create mode 100644 tests/qemu-iotests/300.out

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
new file mode 100755
index 0000000000..621a60e179
--- /dev/null
+++ b/tests/qemu-iotests/300
@@ -0,0 +1,487 @@
+#!/usr/bin/env python3
+#
+# Tests for dirty bitmaps migration with node aliases
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import random
+from typing import Dict, List, Union
+import iotests
+
+assert iotests.sock_dir is not None
+mig_sock = os.path.join(iotests.sock_dir, 'mig_sock')
+
+class TestDirtyBitmapMigration(iotests.QMPTestCase):
+    src_node_name: str = ''
+    dst_node_name: str = ''
+    src_bmap_name: str = ''
+    dst_bmap_name: str = ''
+
+    def setUp(self) -> None:
+        self.vm_a = iotests.VM(path_suffix='-a')
+        self.vm_a.add_blockdev(f'node-name={self.src_node_name},' \
+                               'driver=null-co')
+        self.vm_a.launch()
+
+        self.vm_b = iotests.VM(path_suffix='-b')
+        self.vm_b.add_blockdev(f'node-name={self.dst_node_name},' \
+                               'driver=null-co')
+        self.vm_b.add_incoming(f'unix:{mig_sock}')
+        self.vm_b.launch()
+
+        result = self.vm_a.qmp('block-dirty-bitmap-add',
+                               node=self.src_node_name,
+                               name=self.src_bmap_name)
+        self.assert_qmp(result, 'return', {})
+
+        # Dirty some random megabytes
+        for _ in range(9):
+            mb_ofs = random.randrange(1024)
+            self.vm_a.hmp_qemu_io(self.src_node_name, 'write %dM 1M' % mb_ofs)
+
+        result = self.vm_a.qmp('x-debug-block-dirty-bitmap-sha256',
+                               node=self.src_node_name,
+                               name=self.src_bmap_name)
+        self.bitmap_hash_reference = result['return']['sha256']
+
+        caps = [{'capability': name, 'state': True} \
+                for name in ('dirty-bitmaps', 'events')]
+
+        for vm in (self.vm_a, self.vm_b):
+            result = vm.qmp('migrate-set-capabilities', capabilities=caps)
+            self.assert_qmp(result, 'return', {})
+
+    def tearDown(self) -> None:
+        self.vm_a.shutdown()
+        self.vm_b.shutdown()
+        try:
+            os.remove(mig_sock)
+        except OSError:
+            pass
+
+    def check_bitmap(self, bitmap_name_valid: bool) -> None:
+        result = self.vm_b.qmp('x-debug-block-dirty-bitmap-sha256',
+                               node=self.dst_node_name,
+                               name=self.dst_bmap_name)
+        if bitmap_name_valid:
+            self.assert_qmp(result, 'return/sha256',
+                            self.bitmap_hash_reference)
+        else:
+            self.assert_qmp(result, 'error/desc',
+                            f"Dirty bitmap '{self.dst_bmap_name}' not found")
+
+    def migrate(self, migration_success: bool = True,
+                bitmap_name_valid: bool = True) -> None:
+        result = self.vm_a.qmp('migrate', uri=f'unix:{mig_sock}')
+        self.assert_qmp(result, 'return', {})
+
+        status = None
+        while status not in ('completed', 'failed'):
+            status = self.vm_a.event_wait('MIGRATION')['data']['status']
+
+        self.assertEqual(status == 'completed', migration_success)
+        if status == 'failed':
+            # Wait until the migration has been cleaned up
+            # (Otherwise, bdrv_close_all() will abort because the
+            # dirty bitmap migration code still holds a reference to
+            # the BDS)
+            # (Unfortunately, there does not appear to be a nicer way
+            # of waiting until a failed migration has been cleaned up)
+            timeout_msg = 'Timeout waiting for migration to be cleaned up'
+            with iotests.Timeout(30, timeout_msg):
+                while os.path.exists(mig_sock):
+                    pass
+
+                # Dropping src_node_name will only work once the
+                # bitmap migration code has released it
+                while 'error' in self.vm_a.qmp('blockdev-del',
+                                               node_name=self.src_node_name):
+                    pass
+
+            return
+
+        self.vm_a.wait_for_runstate('postmigrate')
+        self.vm_b.wait_for_runstate('running')
+
+        self.check_bitmap(bitmap_name_valid)
+
+    @staticmethod
+    def mapping(node_name: str, node_alias: str,
+                bitmap_name: str, bitmap_alias: str) \
+        -> List[Dict[str, Union[str, List[Dict[str, str]]]]]:
+        return [{
+            'node-name': node_name,
+            'alias': node_alias,
+            'bitmaps': [{
+                'name': bitmap_name,
+                'alias': bitmap_alias
+            }]
+        }]
+
+
+class TestAliasMigration(TestDirtyBitmapMigration):
+    src_node_name = 'node0'
+    dst_node_name = 'node0'
+    src_bmap_name = 'bmap0'
+    dst_bmap_name = 'bmap0'
+
+    def test_migration_without_alias(self) -> None:
+        self.migrate(self.src_node_name == self.dst_node_name,
+                     self.src_bmap_name == self.dst_bmap_name)
+
+    def test_alias_on_src_migration(self) -> None:
+        mapping = self.mapping(self.src_node_name, self.dst_node_name,
+                               self.src_bmap_name, self.dst_bmap_name)
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate()
+
+    def test_alias_on_dst_migration(self) -> None:
+        mapping = self.mapping(self.dst_node_name, self.src_node_name,
+                               self.dst_bmap_name, self.src_bmap_name)
+
+        result = self.vm_b.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate()
+
+    def test_alias_on_both_migration(self) -> None:
+        src_map = self.mapping(self.src_node_name, 'node-alias',
+                               self.src_bmap_name, 'bmap-alias')
+
+        dst_map = self.mapping(self.dst_node_name, 'node-alias',
+                               self.dst_bmap_name, 'bmap-alias')
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=src_map)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm_b.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=dst_map)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate()
+
+
+class TestNodeAliasMigration(TestAliasMigration):
+    src_node_name = 'node-src'
+    dst_node_name = 'node-dst'
+
+
+class TestBitmapAliasMigration(TestAliasMigration):
+    src_bmap_name = 'bmap-src'
+    dst_bmap_name = 'bmap-dst'
+
+
+class TestFullAliasMigration(TestAliasMigration):
+    src_node_name = 'node-src'
+    dst_node_name = 'node-dst'
+    src_bmap_name = 'bmap-src'
+    dst_bmap_name = 'bmap-dst'
+
+
+class TestBlockBitmapMappingErrors(TestDirtyBitmapMigration):
+    src_node_name = 'node0'
+    dst_node_name = 'node0'
+    src_bmap_name = 'bmap0'
+    dst_bmap_name = 'bmap0'
+
+    """
+    Note that mapping nodes or bitmaps that do not exist is not an error.
+    """
+
+    def test_non_injective_node_mapping(self) -> None:
+        mapping = [
+            {
+                'node-name': 'node0',
+                'alias': 'common-alias',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            },
+            {
+                'node-name': 'node1',
+                'alias': 'common-alias',
+                'bitmaps': [{
+                    'name': 'bmap1',
+                    'alias': 'bmap-alias1'
+                }]
+            }
+        ]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'error/desc',
+                        'Invalid mapping given for block-bitmap-mapping: ' \
+                        'The node alias common-alias is used twice')
+
+    def test_non_injective_bitmap_mapping(self) -> None:
+        mapping = [{
+            'node-name': 'node0',
+            'alias': 'node-alias0',
+            'bitmaps': [
+                {
+                    'name': 'bmap0',
+                    'alias': 'common-alias'
+                },
+                {
+                    'name': 'bmap1',
+                    'alias': 'common-alias'
+                }
+            ]
+        }]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'error/desc',
+                        'Invalid mapping given for block-bitmap-mapping: ' \
+                        'The bitmap alias node-alias0/common-alias is used ' \
+                        'twice')
+
+    def test_ambiguous_node_mapping(self) -> None:
+        mapping = [
+            {
+                'node-name': 'node0',
+                'alias': 'node-alias0',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            },
+            {
+                'node-name': 'node0',
+                'alias': 'node-alias1',
+                'bitmaps': [{
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                }]
+            }
+        ]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'error/desc',
+                        'Invalid mapping given for block-bitmap-mapping: ' \
+                        'The node name node0 is mapped twice')
+
+    def test_ambiguous_bitmap_mapping(self) -> None:
+        mapping = [{
+            'node-name': 'node0',
+            'alias': 'node-alias0',
+            'bitmaps': [
+                {
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias0'
+                },
+                {
+                    'name': 'bmap0',
+                    'alias': 'bmap-alias1'
+                }
+            ]
+        }]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'error/desc',
+                        'Invalid mapping given for block-bitmap-mapping: ' \
+                        'The bitmap node0/bmap0 is mapped twice')
+
+    def test_migratee_node_is_not_mapped_on_src(self) -> None:
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=[])
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate(False, False)
+
+        self.vm_a.shutdown()
+        self.assertIn(f"Bitmap '{self.src_bmap_name}' on node " \
+                      f"'{self.src_node_name}' with no alias cannot be " \
+                      "migrated",
+                      self.vm_a.get_log())
+
+    def test_migratee_node_is_not_mapped_on_dst(self) -> None:
+        result = self.vm_b.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=[])
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate(False, False)
+
+        self.vm_b.shutdown()
+        self.assertIn(f"Unknown node alias '{self.src_node_name}'",
+                      self.vm_b.get_log())
+
+    def test_migratee_bitmap_is_not_mapped_on_src(self) -> None:
+        mapping = [{
+            'node-name': self.src_node_name,
+            'alias': self.dst_node_name,
+            'bitmaps': []
+        }]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate(False, False)
+
+        self.vm_a.shutdown()
+        self.assertIn(f"Bitmap '{self.src_bmap_name}' with no alias on node " \
+                      f"'{self.src_node_name}' cannot be migrated",
+                      self.vm_a.get_log())
+
+    def test_migratee_bitmap_is_not_mapped_on_dst(self) -> None:
+        mapping = [{
+            'node-name': self.dst_node_name,
+            'alias': self.src_node_name,
+            'bitmaps': []
+        }]
+
+        result = self.vm_b.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'return', {})
+
+        self.migrate(False, False)
+
+        self.vm_b.shutdown()
+        self.assertIn(f"Unknown bitmap alias '{self.src_bmap_name}' on node " \
+                      f"'{self.dst_node_name}' (alias '{self.src_node_name}')",
+                      self.vm_b.get_log())
+
+    def test_non_wellformed_node_alias(self) -> None:
+        mapping = [{
+            'node-name': self.src_bmap_name,
+            'alias': '123-foo',
+            'bitmaps': []
+        }]
+
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=mapping)
+        self.assert_qmp(result, 'error/desc',
+                        'Invalid mapping given for block-bitmap-mapping: ' \
+                        'The node alias 123-foo is not well-formed')
+
+
+class TestCrossAliasMigration(TestDirtyBitmapMigration):
+    """
+    Swap aliases, both to see that qemu does not get confused, and
+    that we can migrate multiple things at once.
+
+    So we migrate this:
+      node-a.bmap-a -> node-b.bmap-b
+      node-a.bmap-b -> node-b.bmap-a
+      node-b.bmap-a -> node-a.bmap-b
+      node-b.bmap-b -> node-a.bmap-a
+    """
+
+    src_node_name = 'node-a'
+    dst_node_name = 'node-b'
+    src_bmap_name = 'bmap-a'
+    dst_bmap_name = 'bmap-b'
+
+    def setUp(self) -> None:
+        TestDirtyBitmapMigration.setUp(self)
+
+        # Now create another block device and let both have two bitmaps each
+        result = self.vm_a.qmp('blockdev-add',
+                               node_name='node-b', driver='null-co')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm_b.qmp('blockdev-add',
+                               node_name='node-a', driver='null-co')
+        self.assert_qmp(result, 'return', {})
+
+        bmaps_to_add = (('node-a', 'bmap-b'),
+                        ('node-b', 'bmap-a'),
+                        ('node-b', 'bmap-b'))
+
+        for (node, bmap) in bmaps_to_add:
+            result = self.vm_a.qmp('block-dirty-bitmap-add',
+                                   node=node, name=bmap)
+            self.assert_qmp(result, 'return', {})
+
+    @staticmethod
+    def cross_mapping() -> List[Dict[str, Union[str, List[Dict[str, str]]]]]:
+        return [
+            {
+                'node-name': 'node-a',
+                'alias': 'node-b',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-b'
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-a'
+                    }
+                ]
+            },
+            {
+                'node-name': 'node-b',
+                'alias': 'node-a',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-a'
+                    },
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            }
+        ]
+
+    def verify_dest_has_all_bitmaps(self) -> None:
+        bitmaps = self.vm_a.query_bitmaps()
+
+        # Extract and sort bitmap names
+        for node in bitmaps:
+            bitmaps[node] = sorted((bmap['name'] for bmap in bitmaps[node]))
+
+        self.assertEqual(bitmaps,
+                         {'node-a': ['bmap-a', 'bmap-b'],
+                          'node-b': ['bmap-a', 'bmap-b']})
+
+    def test_alias_on_src(self) -> None:
+        result = self.vm_a.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=self.cross_mapping())
+        self.assert_qmp(result, 'return', {})
+
+        # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+        # that is enough
+        self.migrate()
+
+        self.verify_dest_has_all_bitmaps()
+
+    def test_alias_on_dst(self) -> None:
+        result = self.vm_b.qmp('migrate-set-parameters',
+                               block_bitmap_mapping=self.cross_mapping())
+        self.assert_qmp(result, 'return', {})
+
+        # Checks that node-a.bmap-a was migrated to node-b.bmap-b, and
+        # that is enough
+        self.migrate()
+
+        self.verify_dest_has_all_bitmaps()
+
+
+if __name__ == '__main__':
+    iotests.main(supported_protocols=['file'])
diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
new file mode 100644
index 0000000000..6d9bee1a4b
--- /dev/null
+++ b/tests/qemu-iotests/300.out
@@ -0,0 +1,5 @@
+...........................
+----------------------------------------------------------------------
+Ran 27 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index d886fa0cb3..be93b513b8 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -302,3 +302,4 @@
 291 rw quick
 292 rw auto quick
 297 meta
+300 migration
-- 
2.26.2

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

[Vladimir Sementsov-Ogievskiy]

Hmm, seems, you didn't use scripts/get_maintainer.pl, as neither Eric nor John are in Cc. Add them.

30.06.2020 11:45, Max Reitz wrote:
> RFC v1: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00912.html
> RFC v2: https://lists.nongnu.org/archive/html/qemu-block/2020-05/msg00915.html
> 
> Branch: https://github.com/XanClic/qemu.git migration-bitmap-mapping-v1
> Branch: https://git.xanclic.moe/XanClic/qemu.git migration-bitmap-mapping-v1
> 
> 
> Hi,
> 
> This new 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 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).
> 
> v1 (as opposed to the RFC):
> - Added an iotest
> - Allow mapping of not only node names, but also of bitmap names to
>    aliases
> - Make this a migration parameter instead of adding a whole new QMP
>    command
> - Added patch 1 for good measure
> 
> 
> Max Reitz (4):
>    migration: Prevent memleak by ...params_test_apply
>    migration: Add block-bitmap-mapping parameter
>    iotests.py: Add wait_for_runstate()
>    iotests: Test node/bitmap aliases during migration
> 
>   qapi/migration.json            |  83 +++++-
>   migration/migration.h          |   3 +
>   migration/block-dirty-bitmap.c | 372 +++++++++++++++++++++----
>   migration/migration.c          |  33 ++-
>   tests/qemu-iotests/300         | 487 +++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/300.out     |   5 +
>   tests/qemu-iotests/group       |   1 +
>   tests/qemu-iotests/iotests.py  |   4 +
>   8 files changed, 931 insertions(+), 57 deletions(-)
>   create mode 100755 tests/qemu-iotests/300
>   create mode 100644 tests/qemu-iotests/300.out
> 


-- 
Best regards,
Vladimir