[PATCH v3 0/3] migration: dirty-bitmap: Allow control of bitmap persistence

[PATCH v3 0/3] migration: dirty-bitmap: Allow control of bitmap persistence

[Peter Krempa]

See 2/2 for explanation.

Peter Krempa (3):
  migration: dirty-bitmap: Convert alias map inner members to
    BitmapMigrationBitmapAlias
  migration: dirty-bitmap: Allow control of bitmap persistence
  qemu-iotests: 300: Add test case for modifying persistence of bitmap

 migration/block-dirty-bitmap.c | 60 ++++++++++++++++------
 qapi/migration.json            | 19 ++++++-
 tests/qemu-iotests/300         | 91 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out     |  4 +-
 4 files changed, 157 insertions(+), 17 deletions(-)

-- 
2.29.2

[PATCH v3 1/3] migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias

[Peter Krempa]

Currently the alias mapping hash stores just strings of the target
objects internally. In further patches we'll be adding another member
which will need to be stored in the map so pass a copy of the whole
BitmapMigrationBitmapAlias QAPI struct into the map.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 migration/block-dirty-bitmap.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Note that there's a very long line but there doesn't seem to be a
sensible point where to break it.

v3:
 - use a copy of BitmapMigrationBitmapAlias QAPI sctruct to do the
   mapping
 - dropped R-b's because the above is a significant change

v2:
 - NULL-check in freeing function (Eric)
 - style problems (Vladimir)


diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index c61d382be8..0244f9bb1d 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -75,6 +75,8 @@
 #include "qemu/id.h"
 #include "qapi/error.h"
 #include "qapi/qapi-commands-migration.h"
+#include "qapi/qapi-visit-migration.h"
+#include "qapi/clone-visitor.h"
 #include "trace.h"

 #define CHUNK_SIZE     (1 << 10)
@@ -263,8 +265,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
             node_map_to = bmna->node_name;
         }

-        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
-                                            g_free, g_free);
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
+                                            (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias);

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -276,7 +278,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

         for (bmbal = bmna->bitmaps; bmbal; bmbal = bmbal->next) {
             const BitmapMigrationBitmapAlias *bmba = bmbal->value;
-            const char *bmap_map_from, *bmap_map_to;
+            const char *bmap_map_from;

             if (strlen(bmba->alias) > UINT8_MAX) {
                 error_setg(errp,
@@ -293,7 +295,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

             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",
@@ -302,7 +303,6 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
                 }
             } 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",
@@ -312,7 +312,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
             }

             g_hash_table_insert(bitmaps_map,
-                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+                                g_strdup(bmap_map_from),
+                                QAPI_CLONE(BitmapMigrationBitmapAlias, bmba));
         }
     }

@@ -538,11 +539,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
         }

         if (bitmap_aliases) {
-            bitmap_alias = g_hash_table_lookup(bitmap_aliases, bitmap_name);
-            if (!bitmap_alias) {
+            BitmapMigrationBitmapAlias *bmap_inner;
+
+            bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
+            if (!bmap_inner) {
                 /* Skip bitmaps with no alias */
                 continue;
             }
+
+            bitmap_alias = bmap_inner->alias;
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -1074,13 +1079,16 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,

         bitmap_name = s->bitmap_alias;
         if (!s->cancelled && bitmap_alias_map) {
-            bitmap_name = g_hash_table_lookup(bitmap_alias_map,
-                                              s->bitmap_alias);
-            if (!bitmap_name) {
+            BitmapMigrationBitmapAlias *bmap_inner;
+
+            bmap_inner = g_hash_table_lookup(bitmap_alias_map, s->bitmap_alias);
+            if (!bmap_inner) {
                 error_report("Error: Unknown bitmap alias '%s' on node "
                              "'%s' (alias '%s')", s->bitmap_alias,
                              s->bs->node_name, s->node_alias);
                 cancel_incoming_locked(s);
+            } else {
+                bitmap_name = bmap_inner->name;
             }
         }

-- 
2.29.2

[PATCH v3 2/3] migration: dirty-bitmap: Allow control of bitmap persistence

[Peter Krempa]

Bitmap's source persistence is transported over the migration stream and
the destination mirrors it. In some cases the destination might want to
persist bitmaps which are not persistent on the source (e.g. the result
of merge of bitmaps from a number of layers on the source when migrating
into a squashed image) but currently it would need to create another set
of persistent bitmaps and merge them.

This patch adds a 'transform' property to the alias map which allows to
override the persistence of migrated bitmaps both on the source and
destination sides.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++---
 qapi/migration.json            | 19 ++++++++++++++++++-
 2 files changed, 45 insertions(+), 4 deletions(-)

v3:
 - adapted to full passtrhough of BitmapMigrationBitmapAlias into the
   map hash table (Vladimir)
 - schema and commit message language fixes (Eric)

v2:
 - grammar fixes (Eric)
 - added 'transform' object to group other possible transformations (Vladimir)
 - transformation can also be used on source (Vladimir)
 - put bmap_inner directly into DBMLoadState for deduplication  (Vladimir)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 0244f9bb1d..80781de5d8 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -150,6 +150,7 @@ typedef struct DBMLoadState {
     BdrvDirtyBitmap *bitmap;

     bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
+    BitmapMigrationBitmapAlias *bmap_inner;

     /*
      * cancelled
@@ -528,6 +529,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
     }

     FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+        BitmapMigrationBitmapAliasTransform *bitmap_transform = NULL;
         bitmap_name = bdrv_dirty_bitmap_name(bitmap);
         if (!bitmap_name) {
             continue;
@@ -548,6 +550,9 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
             }

             bitmap_alias = bmap_inner->alias;
+            if (bmap_inner->has_transform) {
+                bitmap_transform = bmap_inner->transform;
+            }
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -573,8 +578,15 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
         if (bdrv_dirty_bitmap_enabled(bitmap)) {
             dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_ENABLED;
         }
-        if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
-            dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+        if (bitmap_transform &&
+            bitmap_transform->has_persistent) {
+            if (bitmap_transform->persistent) {
+                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+            }
+        } else {
+            if (bdrv_dirty_bitmap_get_persistence(bitmap)) {
+                dbms->flags |= DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+            }
         }

         QSIMPLEQ_INSERT_TAIL(&s->dbms_list, dbms, entry);
@@ -782,6 +794,7 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
     uint32_t granularity = qemu_get_be32(f);
     uint8_t flags = qemu_get_byte(f);
     LoadBitmapState *b;
+    bool persistent;

     if (s->cancelled) {
         return 0;
@@ -806,7 +819,16 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
         return -EINVAL;
     }

-    if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
+    if (s->bmap_inner &&
+        s->bmap_inner->has_transform &&
+        s->bmap_inner->transform &&
+        s->bmap_inner->transform->has_persistent) {
+        persistent = s->bmap_inner->transform->persistent;
+    } else {
+        persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
+    }
+
+    if (persistent) {
         bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
     }

@@ -1090,6 +1112,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
             } else {
                 bitmap_name = bmap_inner->name;
             }
+
+            s->bmap_inner = bmap_inner;
         }

         if (!s->cancelled) {
diff --git a/qapi/migration.json b/qapi/migration.json
index ce14d78071..8a85a6d041 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -536,6 +536,19 @@
   'data': [ 'none', 'zlib',
             { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

+##
+# @BitmapMigrationBitmapAliasTransform:
+#
+# @persistent: If present, the bitmap will be made persistent
+#              or transient depending on this parameter.
+#
+# Since: 6.0
+##
+{ 'struct': 'BitmapMigrationBitmapAliasTransform',
+  'data': {
+      '*persistent': 'bool'
+  } }
+
 ##
 # @BitmapMigrationBitmapAlias:
 #
@@ -544,12 +557,16 @@
 # @alias: An alias name for migration (for example the bitmap name on
 #         the opposite site).
 #
+# @transform: Allows to modify properties of the migrated bitmap.
+#             (since 6.0)
+#
 # Since: 5.2
 ##
 { 'struct': 'BitmapMigrationBitmapAlias',
   'data': {
       'name': 'str',
-      'alias': 'str'
+      'alias': 'str',
+      '*transform': 'BitmapMigrationBitmapAliasTransform'
   } }

 ##
-- 
2.29.2

[PATCH v3 3/3] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Peter Krempa]

Verify that the modification of the bitmap persistence over migration
which is controlled via BitmapMigrationBitmapAliasTransform works
properly.

Based on TestCrossAliasMigration

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 tests/qemu-iotests/300     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/300.out |  4 +-
 2 files changed, 93 insertions(+), 2 deletions(-)

v3: new in series

diff --git a/tests/qemu-iotests/300 b/tests/qemu-iotests/300
index 43264d883d..9d4ec6a381 100755
--- a/tests/qemu-iotests/300
+++ b/tests/qemu-iotests/300
@@ -600,6 +600,97 @@ class TestCrossAliasMigration(TestDirtyBitmapMigration):
         self.verify_dest_has_all_bitmaps()
         self.verify_dest_error(None)

+class TestAliasTransformMigration(TestDirtyBitmapMigration):
+    """
+    Tests the 'transform' option which modifies bitmap persistence on migration.
+    """
+
+    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 transform_mapping() -> BlockBitmapMapping:
+        return [
+            {
+                'node-name': 'node-a',
+                'alias': 'node-a',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-a',
+                        'transform':
+                            {
+                                'persistent': True
+                            }
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            },
+            {
+                'node-name': 'node-b',
+                'alias': 'node-b',
+                'bitmaps': [
+                    {
+                        'name': 'bmap-a',
+                        'alias': 'bmap-a'
+                    },
+                    {
+                        'name': 'bmap-b',
+                        'alias': 'bmap-b'
+                    }
+                ]
+            }
+        ]
+
+    def verify_dest_bitmap_state(self) -> None:
+        bitmaps = self.vm_b.query_bitmaps()
+
+        for node in bitmaps:
+            bitmaps[node] = sorted(((bmap['name'], bmap['persistent']) for bmap in bitmaps[node]))
+
+        self.assertEqual(bitmaps,
+                         {'node-a': [('bmap-a', True), ('bmap-b', False)],
+                          'node-b': [('bmap-a', False), ('bmap-b', False)]})
+
+    def test_transform_on_src(self) -> None:
+        self.set_mapping(self.vm_a, self.transform_mapping())
+
+        self.migrate()
+        self.verify_dest_bitmap_state()
+        self.verify_dest_error(None)
+
+    def test_transform_on_dst(self) -> None:
+        self.set_mapping(self.vm_b, self.transform_mapping())
+
+        self.migrate()
+        self.verify_dest_bitmap_state()
+        self.verify_dest_error(None)

 if __name__ == '__main__':
     iotests.main(supported_protocols=['file'])
diff --git a/tests/qemu-iotests/300.out b/tests/qemu-iotests/300.out
index cafb8161f7..12e9ab7d57 100644
--- a/tests/qemu-iotests/300.out
+++ b/tests/qemu-iotests/300.out
@@ -1,5 +1,5 @@
-.....................................
+.......................................
 ----------------------------------------------------------------------
-Ran 37 tests
+Ran 39 tests

 OK
-- 
2.29.2

Re: [PATCH v3 0/3] migration: dirty-bitmap: Allow control of bitmap persistence

[no-reply]

Patchew URL: https://patchew.org/QEMU/cover.1613150869.git.pkrempa@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: cover.1613150869.git.pkrempa@redhat.com
Subject: [PATCH v3 0/3] migration: dirty-bitmap: Allow control of bitmap persistence

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210205163720.887197-1-vsementsov@virtuozzo.com -> patchew/20210205163720.887197-1-vsementsov@virtuozzo.com
 * [new tag]         patchew/cover.1613150869.git.pkrempa@redhat.com -> patchew/cover.1613150869.git.pkrempa@redhat.com
Switched to a new branch 'test'
497cda7 qemu-iotests: 300: Add test case for modifying persistence of bitmap
a575a6e migration: dirty-bitmap: Allow control of bitmap persistence
51195fa migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias

=== OUTPUT BEGIN ===
1/3 Checking commit 51195faba445 (migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias)
ERROR: line over 90 characters
#38: FILE: migration/block-dirty-bitmap.c:269:
+                                            (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias);

total: 1 errors, 0 warnings, 85 lines checked

Patch 1/3 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/3 Checking commit a575a6e16810 (migration: dirty-bitmap: Allow control of bitmap persistence)
3/3 Checking commit 497cda72b44c (qemu-iotests: 300: Add test case for modifying persistence of bitmap)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/cover.1613150869.git.pkrempa@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v3 1/3] migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias

[Eric Blake]

On 2/12/21 11:34 AM, Peter Krempa wrote:

Long subject line; if it's okay with you, I'd prefer to use:

migration: dirty-bitmap: Use struct for alias map inner members

> Currently the alias mapping hash stores just strings of the target
> objects internally. In further patches we'll be adding another member
> which will need to be stored in the map so pass a copy of the whole
> BitmapMigrationBitmapAlias QAPI struct into the map.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  migration/block-dirty-bitmap.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> Note that there's a very long line but there doesn't seem to be a
> sensible point where to break it.

In other words, the patchew warning can be ignored if I can't reformat
the line.

> +++ b/migration/block-dirty-bitmap.c
> @@ -75,6 +75,8 @@
>  #include "qemu/id.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-migration.h"
> +#include "qapi/qapi-visit-migration.h"
> +#include "qapi/clone-visitor.h"
>  #include "trace.h"
> 
>  #define CHUNK_SIZE     (1 << 10)
> @@ -263,8 +265,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
>              node_map_to = bmna->node_name;
>          }
> 
> -        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> -                                            g_free, g_free);
> +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> +                                            (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias);

A possible fix: declare a temporary variable of type GDestroyNotify, so
that assigning the variable uses a shorter line, then use that variable
here.

> @@ -312,7 +312,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
>              }
> 
>              g_hash_table_insert(bitmaps_map,
> -                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
> +                                g_strdup(bmap_map_from),

This line could be wrapped with the previous, now.

Reviewed-by: Eric Blake <eblake@redhat.com>

If you like, I can squash the following in before queuing.


diff --git i/migration/block-dirty-bitmap.c w/migration/block-dirty-bitmap.c
index 0244f9bb1d91..e1840a96d8ee 100644
--- i/migration/block-dirty-bitmap.c
+++ w/migration/block-dirty-bitmap.c
@@ -226,6 +226,7 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
         AliasMapInnerNode *amin;
         GHashTable *bitmaps_map;
         const char *node_map_from, *node_map_to;
+        GDestroyNotify gdn;

         if (!id_wellformed(bmna->alias)) {
             error_setg(errp, "The node alias '%s' is not well-formed",
@@ -265,8 +266,8 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
             node_map_to = bmna->node_name;
         }

-        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
g_free,
-                                            (GDestroyNotify)
qapi_free_BitmapMigrationBitmapAlias);
+        gdn = (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias;
+        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
g_free, gdn);

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -311,8 +312,7 @@ static GHashTable *construct_alias_map(const
BitmapMigrationNodeAliasList *bbm,
                 }
             }

-            g_hash_table_insert(bitmaps_map,
-                                g_strdup(bmap_map_from),
+            g_hash_table_insert(bitmaps_map, g_strdup(bmap_map_from),
                                 QAPI_CLONE(BitmapMigrationBitmapAlias,
bmba));
         }
     }

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

Re: [PATCH v3 1/3] migration: dirty-bitmap: Convert alias map inner members to BitmapMigrationBitmapAlias

[Peter Krempa]

On Fri, Feb 12, 2021 at 12:38:10 -0600, Eric Blake wrote:
> On 2/12/21 11:34 AM, Peter Krempa wrote:
> 
> Long subject line; if it's okay with you, I'd prefer to use:
> 
> migration: dirty-bitmap: Use struct for alias map inner members
> 
> > Currently the alias mapping hash stores just strings of the target
> > objects internally. In further patches we'll be adding another member
> > which will need to be stored in the map so pass a copy of the whole
> > BitmapMigrationBitmapAlias QAPI struct into the map.
> > 
> > Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> > ---
> >  migration/block-dirty-bitmap.c | 30 +++++++++++++++++++-----------
> >  1 file changed, 19 insertions(+), 11 deletions(-)
> > 
> > Note that there's a very long line but there doesn't seem to be a
> > sensible point where to break it.
> 
> In other words, the patchew warning can be ignored if I can't reformat
> the line.
> 
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -75,6 +75,8 @@
> >  #include "qemu/id.h"
> >  #include "qapi/error.h"
> >  #include "qapi/qapi-commands-migration.h"
> > +#include "qapi/qapi-visit-migration.h"
> > +#include "qapi/clone-visitor.h"
> >  #include "trace.h"
> > 
> >  #define CHUNK_SIZE     (1 << 10)
> > @@ -263,8 +265,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> >              node_map_to = bmna->node_name;
> >          }
> > 
> > -        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> > -                                            g_free, g_free);
> > +        bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal, g_free,
> > +                                            (GDestroyNotify) qapi_free_BitmapMigrationBitmapAlias);
> 
> A possible fix: declare a temporary variable of type GDestroyNotify, so
> that assigning the variable uses a shorter line, then use that variable
> here.
> 
> > @@ -312,7 +312,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> >              }
> > 
> >              g_hash_table_insert(bitmaps_map,
> > -                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
> > +                                g_strdup(bmap_map_from),
> 
> This line could be wrapped with the previous, now.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> If you like, I can squash the following in before queuing.

Anything that gets the patches in :).

But honestly, nowadays 80 colums hard limit seems very prehistoric. I
understand that shorter lines are better but if you need to hack around
it, it IMO defeats the readability of the code anyways.

Re: [PATCH v3 2/3] migration: dirty-bitmap: Allow control of bitmap persistence

[Eric Blake]

On 2/12/21 11:34 AM, Peter Krempa wrote:
> Bitmap's source persistence is transported over the migration stream and
> the destination mirrors it. In some cases the destination might want to
> persist bitmaps which are not persistent on the source (e.g. the result
> of merge of bitmaps from a number of layers on the source when migrating

Sorry for not proposing this grammar tweak earlier, but
s/merge of/merging/

> into a squashed image) but currently it would need to create another set
> of persistent bitmaps and merge them.
> 
> This patch adds a 'transform' property to the alias map which allows to
> override the persistence of migrated bitmaps both on the source and

Once again, we encounter the non-idiomatic "allows to ${VERB}"; the
easiest solutions are "allows ${SUBJECT} to ${VERB}" or "allows
${VERB}ing". I'll go with the latter.

> destination sides.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++---
>  qapi/migration.json            | 19 ++++++++++++++++++-
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 

> @@ -806,7 +819,16 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
>          return -EINVAL;
>      }
> 
> -    if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
> +    if (s->bmap_inner &&
> +        s->bmap_inner->has_transform &&
> +        s->bmap_inner->transform &&

This leg of the conjunction is always true (if has_transform is set,
transform is necessarily non-NULL).

> +        s->bmap_inner->transform->has_persistent) {
> +        persistent = s->bmap_inner->transform->persistent;
> +    } else {
> +        persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
> +    }
> +
> +    if (persistent) {
>          bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
>      }
> 

> @@ -544,12 +557,16 @@
>  # @alias: An alias name for migration (for example the bitmap name on
>  #         the opposite site).
>  #
> +# @transform: Allows to modify properties of the migrated bitmap.

Allows the modification of

I can make those tweaks while queuing.
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH v3 3/3] qemu-iotests: 300: Add test case for modifying persistence of bitmap

[Eric Blake]

On 2/12/21 11:34 AM, Peter Krempa wrote:
> Verify that the modification of the bitmap persistence over migration
> which is controlled via BitmapMigrationBitmapAliasTransform works
> properly.
> 
> Based on TestCrossAliasMigration
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  tests/qemu-iotests/300     | 91 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/300.out |  4 +-
>  2 files changed, 93 insertions(+), 2 deletions(-)
> 

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

The use of null-co with no mention of the read-zeroes option here is a
(minor) semantic conflict with the proposal by Philippe to flip the
default of that option (minor, because all it impacts would be the
execution speed of the test)
https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg04027.html

Not your fault, so I don't mind touching up your additions in the same
manner Philippe proposed (which is safe regardless of whose patch lands
first).

diff --git i/tests/qemu-iotests/300 w/tests/qemu-iotests/300
index 9d4ec6a38195..63036f6a6e13 100755
--- i/tests/qemu-iotests/300
+++ w/tests/qemu-iotests/300
@@ -615,11 +615,13 @@ class
TestAliasTransformMigration(TestDirtyBitmapMigration):

         # 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')
+                               node_name='node-b', driver='null-co',
+                               read_zeroes=False)
         self.assert_qmp(result, 'return', {})

         result = self.vm_b.qmp('blockdev-add',
-                               node_name='node-a', driver='null-co')
+                               node_name='node-a', driver='null-co',
+                               read_zeroes=False)
         self.assert_qmp(result, 'return', {})

         bmaps_to_add = (('node-a', 'bmap-b'),


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

Re: [PATCH v3 0/3] migration: dirty-bitmap: Allow control of bitmap persistence

[Eric Blake]

On 2/12/21 11:34 AM, Peter Krempa wrote:
> See 2/2 for explanation.
> 
> Peter Krempa (3):
>   migration: dirty-bitmap: Convert alias map inner members to
>     BitmapMigrationBitmapAlias
>   migration: dirty-bitmap: Allow control of bitmap persistence
>   qemu-iotests: 300: Add test case for modifying persistence of bitmap
> 

Queuing this through my dirty-bitmap tree

>  migration/block-dirty-bitmap.c | 60 ++++++++++++++++------
>  qapi/migration.json            | 19 ++++++-
>  tests/qemu-iotests/300         | 91 ++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/300.out     |  4 +-
>  4 files changed, 157 insertions(+), 17 deletions(-)
> 

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