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

Commit Message

Peter Krempa Feb. 12, 2021, 5:34 p.m. UTC

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 <[e-mail]>
---
 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)

Comments

Eric Blake Feb. 12, 2021, 6:49 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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 <[e-mail]>

Patch

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'
   } }

 ##