qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] migration: dirty-bitmap: Allow control of bitmap persistence
@ 2021-02-10 16:53 Peter Krempa
  2021-02-10 16:53 ` [PATCH v2 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct Peter Krempa
  2021-02-10 16:53 ` [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance Peter Krempa
  0 siblings, 2 replies; 5+ messages in thread
From: Peter Krempa @ 2021-02-10 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, Markus Armbruster, qemu-block

See 2/2 for explanation.

Peter Krempa (2):
  migration: dirty-bitmap: Convert alias map inner members to a struct
  migration: dirty-bitmap: Allow control of bitmap persistance

 migration/block-dirty-bitmap.c | 73 +++++++++++++++++++++++++++++-----
 qapi/migration.json            | 20 +++++++++-
 2 files changed, 81 insertions(+), 12 deletions(-)

-- 
2.29.2



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct
  2021-02-10 16:53 [PATCH v2 0/2] migration: dirty-bitmap: Allow control of bitmap persistence Peter Krempa
@ 2021-02-10 16:53 ` Peter Krempa
  2021-02-10 16:53 ` [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance Peter Krempa
  1 sibling, 0 replies; 5+ messages in thread
From: Peter Krempa @ 2021-02-10 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, Markus Armbruster, qemu-block

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 convert the members to a
struct.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

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

 migration/block-dirty-bitmap.c | 43 +++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index c61d382be8..0169f672df 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -169,6 +169,22 @@ typedef struct DBMState {

 static DBMState dbm_state;

+typedef struct AliasMapInnerBitmap {
+    char *string;
+} AliasMapInnerBitmap;
+
+static void free_alias_map_inner_bitmap(void *amin_ptr)
+{
+    AliasMapInnerBitmap *amin = amin_ptr;
+
+    if (!amin_ptr) {
+        return;
+    }
+
+    g_free(amin->string);
+    g_free(amin);
+}
+
 /* For hash tables that map node/bitmap names to aliases */
 typedef struct AliasMapInnerNode {
     char *string;
@@ -263,8 +279,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,
+                                            free_alias_map_inner_bitmap);

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -277,6 +293,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;
+            AliasMapInnerBitmap *bmap_inner;

             if (strlen(bmba->alias) > UINT8_MAX) {
                 error_setg(errp,
@@ -311,8 +328,11 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
                 }
             }

+            bmap_inner = g_new0(AliasMapInnerBitmap, 1);
+            bmap_inner->string = g_strdup(bmap_map_to);
+
             g_hash_table_insert(bitmaps_map,
-                                g_strdup(bmap_map_from), g_strdup(bmap_map_to));
+                                g_strdup(bmap_map_from), bmap_inner);
         }
     }

@@ -538,11 +558,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) {
+            AliasMapInnerBitmap *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->string;
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -1074,14 +1098,17 @@ 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) {
+            AliasMapInnerBitmap *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);
             }
+
+            bitmap_name = bmap_inner->string;
         }

         if (!s->cancelled) {
-- 
2.29.2



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance
  2021-02-10 16:53 [PATCH v2 0/2] migration: dirty-bitmap: Allow control of bitmap persistence Peter Krempa
  2021-02-10 16:53 ` [PATCH v2 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct Peter Krempa
@ 2021-02-10 16:53 ` Peter Krempa
  2021-02-11 16:04   ` Vladimir Sementsov-Ogievskiy
  2021-02-12 15:36   ` Eric Blake
  1 sibling, 2 replies; 5+ messages in thread
From: Peter Krempa @ 2021-02-10 16:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, Markus Armbruster, qemu-block

Bitmap's source persistance 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 persistance of migrated bitmaps both on the source and
destination sides.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---

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)

 migration/block-dirty-bitmap.c | 38 +++++++++++++++++++++++++++-------
 qapi/migration.json            | 20 +++++++++++++++++-
 2 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 0169f672df..a05bf74073 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -138,6 +138,13 @@ typedef struct LoadBitmapState {
     bool enabled;
 } LoadBitmapState;

+typedef struct AliasMapInnerBitmap {
+    char *string;
+
+    /* 'transform' properties borrowed from QAPI */
+    BitmapMigrationBitmapAliasTransform *transform;
+} AliasMapInnerBitmap;
+
 /* State of the dirty bitmap migration (DBM) during load process */
 typedef struct DBMLoadState {
     uint32_t flags;
@@ -148,6 +155,7 @@ typedef struct DBMLoadState {
     BdrvDirtyBitmap *bitmap;

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

     /*
      * cancelled
@@ -169,10 +177,6 @@ typedef struct DBMState {

 static DBMState dbm_state;

-typedef struct AliasMapInnerBitmap {
-    char *string;
-} AliasMapInnerBitmap;
-
 static void free_alias_map_inner_bitmap(void *amin_ptr)
 {
     AliasMapInnerBitmap *amin = amin_ptr;
@@ -330,6 +334,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

             bmap_inner = g_new0(AliasMapInnerBitmap, 1);
             bmap_inner->string = g_strdup(bmap_map_to);
+            bmap_inner->transform = bmba->transform;

             g_hash_table_insert(bitmaps_map,
                                 g_strdup(bmap_map_from), bmap_inner);
@@ -547,6 +552,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;
@@ -567,6 +573,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
             }

             bitmap_alias = bmap_inner->string;
+            bitmap_transform = bmap_inner->transform;
         } else {
             if (strlen(bitmap_name) > UINT8_MAX) {
                 error_report("Cannot migrate bitmap '%s' on node '%s': "
@@ -592,8 +599,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);
@@ -801,6 +815,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;
@@ -825,7 +840,15 @@ 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->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);
     }

@@ -1109,6 +1132,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
             }

             bitmap_name = bmap_inner->string;
+            s->bmap_inner = bmap_inner;
         }

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

+##
+# @BitmapMigrationBitmapAliasTransform:
+#
+# @persistent: If present, the bitmap will be turned persistent
+#              or transient depending on this parameter.
+#              (since 6.0)
+#
+# Since: 6.0
+##
+{ 'struct': 'BitmapMigrationBitmapAliasTransform',
+  'data': {
+      '*persistent': 'bool'
+  } }
+
 ##
 # @BitmapMigrationBitmapAlias:
 #
@@ -544,12 +558,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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance
  2021-02-10 16:53 ` [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance Peter Krempa
@ 2021-02-11 16:04   ` Vladimir Sementsov-Ogievskiy
  2021-02-12 15:36   ` Eric Blake
  1 sibling, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-02-11 16:04 UTC (permalink / raw)
  To: Peter Krempa, qemu-devel; +Cc: Eric Blake, Markus Armbruster, qemu-block

10.02.2021 19:53, Peter Krempa wrote:
> Bitmap's source persistance 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 persistance of migrated bitmaps both on the source and
> destination sides.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 
> 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)
> 
>   migration/block-dirty-bitmap.c | 38 +++++++++++++++++++++++++++-------
>   qapi/migration.json            | 20 +++++++++++++++++-
>   2 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 0169f672df..a05bf74073 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -138,6 +138,13 @@ typedef struct LoadBitmapState {
>       bool enabled;
>   } LoadBitmapState;
> 
> +typedef struct AliasMapInnerBitmap {
> +    char *string;
> +
> +    /* 'transform' properties borrowed from QAPI */
> +    BitmapMigrationBitmapAliasTransform *transform;
> +} AliasMapInnerBitmap;
> +
>   /* State of the dirty bitmap migration (DBM) during load process */
>   typedef struct DBMLoadState {
>       uint32_t flags;
> @@ -148,6 +155,7 @@ typedef struct DBMLoadState {
>       BdrvDirtyBitmap *bitmap;
> 
>       bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
> +    AliasMapInnerBitmap *bmap_inner;
> 
>       /*
>        * cancelled
> @@ -169,10 +177,6 @@ typedef struct DBMState {
> 
>   static DBMState dbm_state;
> 
> -typedef struct AliasMapInnerBitmap {
> -    char *string;
> -} AliasMapInnerBitmap;
> -
>   static void free_alias_map_inner_bitmap(void *amin_ptr)
>   {
>       AliasMapInnerBitmap *amin = amin_ptr;
> @@ -330,6 +334,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> 
>               bmap_inner = g_new0(AliasMapInnerBitmap, 1);
>               bmap_inner->string = g_strdup(bmap_map_to);
> +            bmap_inner->transform = bmba->transform;

We rely on the fact that bmba->transform will not be freed.. Is it correct, can migration parameters change at some unexpected moment?
Anyway it's strange that we copy string, but just add reference to transform structure.

Looks like we need QAPI_CLONE() here. And we can clone the whole bmba, then we can drop AliasMapInnerBitmap structure at all (hmm and if go this way, this should be done in a previous patch).

Other than that the logic and new QAPI interface looks OK for me.

Also, we need a test-case for new feature in tests/qemu-iotests/300

> 
>               g_hash_table_insert(bitmaps_map,
>                                   g_strdup(bmap_map_from), bmap_inner);
> @@ -547,6 +552,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;
> @@ -567,6 +573,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
>               }
> 
>               bitmap_alias = bmap_inner->string;
> +            bitmap_transform = bmap_inner->transform;
>           } else {
>               if (strlen(bitmap_name) > UINT8_MAX) {
>                   error_report("Cannot migrate bitmap '%s' on node '%s': "
> @@ -592,8 +599,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);
> @@ -801,6 +815,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;
> @@ -825,7 +840,15 @@ 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->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);
>       }
> 
> @@ -1109,6 +1132,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
>               }
> 
>               bitmap_name = bmap_inner->string;
> +            s->bmap_inner = bmap_inner;
>           }
> 
>           if (!s->cancelled) {
> diff --git a/qapi/migration.json b/qapi/migration.json
> index ce14d78071..338135320a 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -536,6 +536,20 @@
>     'data': [ 'none', 'zlib',
>               { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
> +##
> +# @BitmapMigrationBitmapAliasTransform:
> +#
> +# @persistent: If present, the bitmap will be turned persistent
> +#              or transient depending on this parameter.
> +#              (since 6.0)
> +#
> +# Since: 6.0
> +##
> +{ 'struct': 'BitmapMigrationBitmapAliasTransform',
> +  'data': {
> +      '*persistent': 'bool'
> +  } }
> +
>   ##
>   # @BitmapMigrationBitmapAlias:
>   #
> @@ -544,12 +558,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'
>     } }
> 
>   ##
> 


-- 
Best regards,
Vladimir


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance
  2021-02-10 16:53 ` [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance Peter Krempa
  2021-02-11 16:04   ` Vladimir Sementsov-Ogievskiy
@ 2021-02-12 15:36   ` Eric Blake
  1 sibling, 0 replies; 5+ messages in thread
From: Eric Blake @ 2021-02-12 15:36 UTC (permalink / raw)
  To: Peter Krempa, qemu-devel
  Cc: Vladimir Sementsov-Ogievskiy, Markus Armbruster, qemu-block

On 2/10/21 10:53 AM, Peter Krempa wrote:
> Bitmap's source persistance is transported over the migration stream and

persistence

> 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 persistance of migrated bitmaps both on the source and

persistence

> destination sides.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
> 
> 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)

In addition to Vladimir's suggestion to use QAPI_CLONE() and an iotest
addition,

> 
>  migration/block-dirty-bitmap.c | 38 +++++++++++++++++++++++++++-------
>  qapi/migration.json            | 20 +++++++++++++++++-
>  2 files changed, 50 insertions(+), 8 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 0169f672df..a05bf74073 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -138,6 +138,13 @@ typedef struct LoadBitmapState {
>      bool enabled;
>  } LoadBitmapState;
> 
> +typedef struct AliasMapInnerBitmap {
> +    char *string;
> +
> +    /* 'transform' properties borrowed from QAPI */
> +    BitmapMigrationBitmapAliasTransform *transform;
> +} AliasMapInnerBitmap;
> +

You moved this typedef up...

>  /* State of the dirty bitmap migration (DBM) during load process */
>  typedef struct DBMLoadState {
>      uint32_t flags;
> @@ -148,6 +155,7 @@ typedef struct DBMLoadState {
>      BdrvDirtyBitmap *bitmap;
> 
>      bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
> +    AliasMapInnerBitmap *bmap_inner;
> 
>      /*
>       * cancelled
> @@ -169,10 +177,6 @@ typedef struct DBMState {
> 
>  static DBMState dbm_state;
> 
> -typedef struct AliasMapInnerBitmap {
> -    char *string;
> -} AliasMapInnerBitmap;
> -

...from here, although it was just added here in patch 1.  Why not just
declare it up there in the first place in patch 1 to minimize the churn?


> +++ b/qapi/migration.json
> @@ -536,6 +536,20 @@
>    'data': [ 'none', 'zlib',
>              { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
> +##
> +# @BitmapMigrationBitmapAliasTransform:
> +#
> +# @persistent: If present, the bitmap will be turned persistent

s/turned/made/ sounds a little nicer, although what you have wasn't wrong.

> +#              or transient depending on this parameter.
> +#              (since 6.0)

You don't need a '(since 6.0)' tag on the member, since...

> +#
> +# Since: 6.0

...the entire struct was introduced at the same time.

> +##
> +{ 'struct': 'BitmapMigrationBitmapAliasTransform',
> +  'data': {
> +      '*persistent': 'bool'
> +  } }
> +
>  ##
>  # @BitmapMigrationBitmapAlias:
>  #
> @@ -544,12 +558,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)
> +#

whereas this member tag is correct.

>  # Since: 5.2
>  ##
>  { 'struct': 'BitmapMigrationBitmapAlias',
>    'data': {
>        'name': 'str',
> -      'alias': 'str'
> +      'alias': 'str',
> +      '*transform': 'BitmapMigrationBitmapAliasTransform'
>    } }
> 
>  ##
> 

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-02-12 15:39 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-10 16:53 [PATCH v2 0/2] migration: dirty-bitmap: Allow control of bitmap persistence Peter Krempa
2021-02-10 16:53 ` [PATCH v2 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct Peter Krempa
2021-02-10 16:53 ` [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance Peter Krempa
2021-02-11 16:04   ` Vladimir Sementsov-Ogievskiy
2021-02-12 15:36   ` Eric Blake

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).