[PATCH 0/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[PATCH 0/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

See 2/2 for explanation.

Please let me know if I need to add any tests for this.

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

 migration/block-dirty-bitmap.c | 67 ++++++++++++++++++++++++++++++----
 qapi/migration.json            |  7 +++-
 2 files changed, 66 insertions(+), 8 deletions(-)

-- 
2.29.2

[PATCH 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct

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

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

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index c61d382be8..b0403dd00c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -169,6 +169,18 @@ 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;
+
+    g_free(amin->string);
+    g_free(amin);
+}
+
 /* For hash tables that map node/bitmap names to aliases */
 typedef struct AliasMapInnerNode {
     char *string;
@@ -264,7 +276,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
         }

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

         amin = g_new(AliasMapInnerNode, 1);
         *amin = (AliasMapInnerNode){
@@ -277,6 +289,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 +324,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 +554,16 @@ 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 +1095,18 @@ 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,
+            AliasMapInnerBitmap *bmap_inner;
+
+            bmap_inner = g_hash_table_lookup(bitmap_alias_map,
                                               s->bitmap_alias);
-            if (!bitmap_name) {
+            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

Re: [PATCH 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct

[Eric Blake]

On 2/3/21 6:59 AM, Peter Krempa wrote:
> 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>
> ---
>  migration/block-dirty-bitmap.c | 37 ++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index c61d382be8..b0403dd00c 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -169,6 +169,18 @@ 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;
> +
> +    g_free(amin->string);

Do we want to allow free_alias_map_inner_bitmap(NULL)?

Looks like this patch works without it, but it's less future proof, so I
can add that if you agree.

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 1/2] migration: dirty-bitmap: Convert alias map inner members to a struct

[Vladimir Sementsov-Ogievskiy]

03.02.2021 15:59, Peter Krempa wrote:
> 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>
> ---
>   migration/block-dirty-bitmap.c | 37 ++++++++++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index c61d382be8..b0403dd00c 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -169,6 +169,18 @@ 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;
> +
> +    g_free(amin->string);
> +    g_free(amin);
> +}
> +
>   /* For hash tables that map node/bitmap names to aliases */
>   typedef struct AliasMapInnerNode {
>       char *string;
> @@ -264,7 +276,7 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
>           }
> 
>           bitmaps_map = g_hash_table_new_full(g_str_hash, g_str_equal,
> -                                            g_free, g_free);
> +                                            g_free, free_alias_map_inner_bitmap);

over-80 line

> 
>           amin = g_new(AliasMapInnerNode, 1);
>           *amin = (AliasMapInnerNode){
> @@ -277,6 +289,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 +324,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 +554,16 @@ 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);
> +

I'd drop this line, you are not consistent on it with next hunk anyway)

> +            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 +1095,18 @@ 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,
> +            AliasMapInnerBitmap *bmap_inner;
> +
> +            bmap_inner = g_hash_table_lookup(bitmap_alias_map,
>                                                 s->bitmap_alias);

indentation

> -            if (!bitmap_name) {
> +            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) {
> 

Looks OK for me (I'm OK with Eric's suggestion too):
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

suggest style cleanup:

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0403dd00c..577e32bf75 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -275,8 +275,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, free_alias_map_inner_bitmap);
+        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){
@@ -557,7 +557,6 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
              AliasMapInnerBitmap *bmap_inner;
  
              bmap_inner = g_hash_table_lookup(bitmap_aliases, bitmap_name);
-
              if (!bmap_inner) {
                  /* Skip bitmaps with no alias */
                  continue;
@@ -1097,8 +1096,7 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
          if (!s->cancelled && bitmap_alias_map) {
              AliasMapInnerBitmap *bmap_inner;
  
-            bmap_inner = g_hash_table_lookup(bitmap_alias_map,
-                                              s->bitmap_alias);
+            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,



-- 
Best regards,
Vladimir

[PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[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 adds 'dest-persistent' optional property to
'BitmapMigrationBitmapAlias' which when present overrides the bitmap
presence state from the source.

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

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index b0403dd00c..1876c94c45 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -149,6 +149,9 @@ typedef struct DBMLoadState {

     bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */

+    bool has_dest_persistent;
+    bool dest_persistent;
+
     /*
      * cancelled
      * Incoming migration is cancelled for some reason. That means that we
@@ -171,6 +174,10 @@ static DBMState dbm_state;

 typedef struct AliasMapInnerBitmap {
     char *string;
+
+    /* for destination's properties setting bitmap state */
+    bool has_dest_persistent;
+    bool dest_persistent;
 } AliasMapInnerBitmap;

 static void free_alias_map_inner_bitmap(void *amin_ptr)
@@ -289,6 +296,8 @@ 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;
+            bool bmap_has_dest_persistent = false;
+            bool bmap_dest_persistent = false;
             AliasMapInnerBitmap *bmap_inner;

             if (strlen(bmba->alias) > UINT8_MAX) {
@@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
                 bmap_map_from = bmba->alias;
                 bmap_map_to = bmba->name;

+                bmap_has_dest_persistent = bmba->has_dest_persistent;
+                bmap_dest_persistent = bmba->dest_persistent;
+
                 if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
                     error_setg(errp, "The bitmap alias '%s'/'%s' is used twice",
                                bmna->alias, bmba->alias);
@@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,

             bmap_inner = g_new0(AliasMapInnerBitmap, 1);
             bmap_inner->string = g_strdup(bmap_map_to);
+            bmap_inner->has_dest_persistent = bmap_has_dest_persistent;
+            bmap_inner->dest_persistent = bmap_dest_persistent;

             g_hash_table_insert(bitmaps_map,
                                 g_strdup(bmap_map_from), bmap_inner);
@@ -798,6 +812,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;
@@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
         return -EINVAL;
     }

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

@@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,

     if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
         const char *bitmap_name;
+        bool bitmap_has_dest_persistent = false;
+        bool bitmap_dest_persistent = false;

         if (!qemu_get_counted_string(f, s->bitmap_alias)) {
             error_report("Unable to read bitmap alias string");
@@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
             }

             bitmap_name = bmap_inner->string;
+            bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
+            bitmap_dest_persistent = bmap_inner->dest_persistent;
         }

         if (!s->cancelled) {
             g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
             s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);

+            s->has_dest_persistent = bitmap_has_dest_persistent;
+            s->dest_persistent = bitmap_dest_persistent;
+
             /*
              * bitmap may be NULL here, it wouldn't be an error if it is the
              * first occurrence of the bitmap
diff --git a/qapi/migration.json b/qapi/migration.json
index d1d9632c2a..32e64dbce6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -533,12 +533,17 @@
 # @alias: An alias name for migration (for example the bitmap name on
 #         the opposite site).
 #
+# @dest-persistent: If populated set the bitmap will be turned persistent
+#                   or transient depending on this parameter.
+#                   (since 6.0)
+#
 # Since: 5.2
 ##
 { 'struct': 'BitmapMigrationBitmapAlias',
   'data': {
       'name': 'str',
-      'alias': 'str'
+      'alias': 'str',
+      '*dest-persistent': 'bool'
   } }

 ##
-- 
2.29.2

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Vladimir Sementsov-Ogievskiy]

03.02.2021 16:00, 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
> into a squashed image)

Why not make merge target on source be persistent itself? Then it will be persistent on migration destination.

> but currently it would need to create another set
> of persistent bitmaps and merge them.
> 
> This adds 'dest-persistent' optional property to
> 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> presence state from the source.

It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way?



-- 
Best regards,
Vladimir

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:
> 03.02.2021 16:00, 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
> > into a squashed image)
> 
> Why not make merge target on source be persistent itself? Then it will be persistent on migration destination.

Because they are temporary on the source. I don't want to make it
persistent in case of a failure so that it doesn't get written to the
disk e.g. in case of VM shutdown.

> 
> > but currently it would need to create another set
> > of persistent bitmaps and merge them.
> > 
> > This adds 'dest-persistent' optional property to
> > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> > presence state from the source.
> 
> It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way?

I'm not sure how the internals work entirely. In my case it's way
simpler to do this setup when generating the mapping which I need to do
anyways rather than calling separate commands.

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Peter Krempa]

On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote:
> On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 03.02.2021 16:00, 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
> > > into a squashed image)
> > 
> > Why not make merge target on source be persistent itself? Then it will be persistent on migration destination.
> 
> Because they are temporary on the source. I don't want to make it
> persistent in case of a failure so that it doesn't get written to the
> disk e.g. in case of VM shutdown.

To be a bit more specific, I don't want the bitmaps to stay in the
image, that means that I'd have to also delete them on the source after
a successfull migration before qemu is terminated, which might not even
be possible since source deactivates storage after migration.

So making them persistent on source is impossible.

> 
> > 
> > > but currently it would need to create another set
> > > of persistent bitmaps and merge them.
> > > 
> > > This adds 'dest-persistent' optional property to
> > > 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> > > presence state from the source.
> > 
> > It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way?
> 
> I'm not sure how the internals work entirely. In my case it's way
> simpler to do this setup when generating the mapping which I need to do
> anyways rather than calling separate commands.

Similarly here, after a successful migration I'd have to go and make all
the bitmaps persistent, which is an extra step, and also a vector for
possible failures after migration which also doesn't seem appealing.

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Vladimir Sementsov-Ogievskiy]

03.02.2021 16:39, Peter Krempa wrote:
> On Wed, Feb 03, 2021 at 14:27:49 +0100, Peter Krempa wrote:
>> On Wed, Feb 03, 2021 at 16:23:21 +0300, Vladimir Sementsov-Ogievskiy wrote:
>>> 03.02.2021 16:00, 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
>>>> into a squashed image)
>>>
>>> Why not make merge target on source be persistent itself? Then it will be persistent on migration destination.
>>
>> Because they are temporary on the source. I don't want to make it
>> persistent in case of a failure so that it doesn't get written to the
>> disk e.g. in case of VM shutdown.
> 
> To be a bit more specific, I don't want the bitmaps to stay in the
> image, that means that I'd have to also delete them on the source after
> a successfull migration before qemu is terminated, which might not even
> be possible since source deactivates storage after migration.
> 
> So making them persistent on source is impossible.

Actually on success path, persistent bitmaps are not stored on source.

Normally persistent bitmaps are stored on image inactivation. But bitmaps
involved into migration are an exclusion, they are not stored (otherwise,
stoing will influence downtime of migration). And of-course, we can't
store bitmaps after disks inactivation.

So, on success path, the only way to store bitmaps on source is to do
qmp 'cont' command on source after migration..

I'm not so sure about error path. Of course, if something breaks between
merge target creation and migration start, bitmaps will be stored.

So, I agree that in general it's bad idea to make temporary bitmap 'persistent'.

> 
>>
>>>
>>>> but currently it would need to create another set
>>>> of persistent bitmaps and merge them.
>>>>
>>>> This adds 'dest-persistent' optional property to
>>>> 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
>>>> presence state from the source.
>>>
>>> It's seems simpler to make a separate qmp command block-dirty-bitmap-make-persistent.. Didn't you consider this way?
>>
>> I'm not sure how the internals work entirely. In my case it's way
>> simpler to do this setup when generating the mapping which I need to do
>> anyways rather than calling separate commands.
> 
> Similarly here, after a successful migration I'd have to go and make all
> the bitmaps persistent, which is an extra step, and also a vector for
> possible failures after migration which also doesn't seem appealing.
> 

OK, that's reasonable, thanks for explanation


-- 
Best regards,
Vladimir

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Eric Blake]

On 2/3/21 7:00 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
> into a squashed image) but currently it would need to create another set
> of persistent bitmaps and merge them.
> 
> This adds 'dest-persistent' optional property to
> 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> presence state from the source.

persistance

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

> +++ b/qapi/migration.json
> @@ -533,12 +533,17 @@
>  # @alias: An alias name for migration (for example the bitmap name on
>  #         the opposite site).
>  #
> +# @dest-persistent: If populated set the bitmap will be turned persistent
> +#                   or transient depending on this parameter.

s/populated set/present,/

> +#                   (since 6.0)
> +#
>  # Since: 5.2
>  ##
>  { 'struct': 'BitmapMigrationBitmapAlias',
>    'data': {
>        'name': 'str',
> -      'alias': 'str'
> +      'alias': 'str',
> +      '*dest-persistent': 'bool'
>    } }
> 
>  ##
> 

The grammar fix is trivial, so
Reviewed-by: Eric Blake <eblake@redhat.com>

I see there is discussion over whether this is the best approach, but it
makes sense to me.  Unless there's a good reason why something else
would be better, I'm probably going to queue this through my dirty
bitmaps tree for a pull request sometime next week.

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

Re: [PATCH 2/2] migration: dirty-bitmap: Allow control of bitmap persistence on destination

[Vladimir Sementsov-Ogievskiy]

03.02.2021 16:00, 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
> into a squashed image) but currently it would need to create another set
> of persistent bitmaps and merge them.
> 
> This adds 'dest-persistent' optional property to
> 'BitmapMigrationBitmapAlias' which when present overrides the bitmap
> presence state from the source.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>   migration/block-dirty-bitmap.c | 30 +++++++++++++++++++++++++++++-
>   qapi/migration.json            |  7 ++++++-
>   2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index b0403dd00c..1876c94c45 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -149,6 +149,9 @@ typedef struct DBMLoadState {
> 
>       bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
> 
> +    bool has_dest_persistent;
> +    bool dest_persistent;
> +
>       /*
>        * cancelled
>        * Incoming migration is cancelled for some reason. That means that we
> @@ -171,6 +174,10 @@ static DBMState dbm_state;
> 
>   typedef struct AliasMapInnerBitmap {
>       char *string;
> +
> +    /* for destination's properties setting bitmap state */
> +    bool has_dest_persistent;
> +    bool dest_persistent;
>   } AliasMapInnerBitmap;
> 
>   static void free_alias_map_inner_bitmap(void *amin_ptr)
> @@ -289,6 +296,8 @@ 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;
> +            bool bmap_has_dest_persistent = false;
> +            bool bmap_dest_persistent = false;
>               AliasMapInnerBitmap *bmap_inner;
> 
>               if (strlen(bmba->alias) > UINT8_MAX) {
> @@ -317,6 +326,9 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
>                   bmap_map_from = bmba->alias;
>                   bmap_map_to = bmba->name;
> 
> +                bmap_has_dest_persistent = bmba->has_dest_persistent;
> +                bmap_dest_persistent = bmba->dest_persistent;
> +
>                   if (g_hash_table_contains(bitmaps_map, bmba->alias)) {
>                       error_setg(errp, "The bitmap alias '%s'/'%s' is used twice",
>                                  bmna->alias, bmba->alias);
> @@ -326,6 +338,8 @@ static GHashTable *construct_alias_map(const BitmapMigrationNodeAliasList *bbm,
> 
>               bmap_inner = g_new0(AliasMapInnerBitmap, 1);
>               bmap_inner->string = g_strdup(bmap_map_to);
> +            bmap_inner->has_dest_persistent = bmap_has_dest_persistent;
> +            bmap_inner->dest_persistent = bmap_dest_persistent;
> 
>               g_hash_table_insert(bitmaps_map,
>                                   g_strdup(bmap_map_from), bmap_inner);
> @@ -798,6 +812,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;
> @@ -822,7 +837,13 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
>           return -EINVAL;
>       }
> 
> -    if (flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT) {
> +    if (s->has_dest_persistent) {
> +        persistent = s->dest_persistent;
> +    } else {
> +        persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
> +    }
> +
> +    if (persistent) {
>           bdrv_dirty_bitmap_set_persistence(s->bitmap, true);
>       }
> 
> @@ -1087,6 +1108,8 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
> 
>       if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
>           const char *bitmap_name;
> +        bool bitmap_has_dest_persistent = false;
> +        bool bitmap_dest_persistent = false;
> 
>           if (!qemu_get_counted_string(f, s->bitmap_alias)) {
>               error_report("Unable to read bitmap alias string");
> @@ -1107,12 +1130,17 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
>               }
> 
>               bitmap_name = bmap_inner->string;
> +            bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
> +            bitmap_dest_persistent = bmap_inner->dest_persistent;
>           }
> 
>           if (!s->cancelled) {
>               g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
>               s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
> 
> +            s->has_dest_persistent = bitmap_has_dest_persistent;
> +            s->dest_persistent = bitmap_dest_persistent;
> +
>               /*
>                * bitmap may be NULL here, it wouldn't be an error if it is the
>                * first occurrence of the bitmap
> diff --git a/qapi/migration.json b/qapi/migration.json
> index d1d9632c2a..32e64dbce6 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -533,12 +533,17 @@
>   # @alias: An alias name for migration (for example the bitmap name on
>   #         the opposite site).
>   #
> +# @dest-persistent: If populated set the bitmap will be turned persistent
> +#                   or transient depending on this parameter.
> +#                   (since 6.0)
> +#

Thinking of future: will we want modify other bitmap properties "in-flight"? enabled? maybe granularity? Then we'll add additional dest-* properties. Not bad, but probably better to make it nested, like

transform: { persistent: bool }

So, than we'll can add other properties:

transform: { *persistent: bool, *enable: bool, *granularity: bool }

Also, in code I see you just ignore dest-persistent if it is set on source. I think we should either error-out, or support it. Why not to allow setup property change on source alias-mapping? (and that's why I suggest "transform" which a bit more generic for using both on source and target)


Also, I don't like duplication of AliasMapInnerBitmap in DBMLoadState. Could we instead just add a pointer to bmap_inner, like this:

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 1876c94c45..93ae76734e 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -138,6 +138,14 @@ typedef struct LoadBitmapState {
      bool enabled;
  } LoadBitmapState;
  
+typedef struct AliasMapInnerBitmap {
+    char *string;
+
+    /* for destination's properties setting bitmap state */
+    bool has_dest_persistent;
+    bool dest_persistent;
+} AliasMapInnerBitmap;
+
  /* State of the dirty bitmap migration (DBM) during load process */
  typedef struct DBMLoadState {
      uint32_t flags;
@@ -149,8 +157,7 @@ typedef struct DBMLoadState {
  
      bool before_vm_start_handled; /* set in dirty_bitmap_mig_before_vm_start */
  
-    bool has_dest_persistent;
-    bool dest_persistent;
+    AliasMapInnerBitmap *bmap_inner;
  
      /*
       * cancelled
@@ -172,14 +179,6 @@ typedef struct DBMState {
  
  static DBMState dbm_state;
  
-typedef struct AliasMapInnerBitmap {
-    char *string;
-
-    /* for destination's properties setting bitmap state */
-    bool has_dest_persistent;
-    bool dest_persistent;
-} AliasMapInnerBitmap;
-
  static void free_alias_map_inner_bitmap(void *amin_ptr)
  {
      AliasMapInnerBitmap *amin = amin_ptr;
@@ -837,8 +836,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
          return -EINVAL;
      }
  
-    if (s->has_dest_persistent) {
-        persistent = s->dest_persistent;
+    if (s->bmap_inner && s->bmap_inner->has_dest_persistent) {
+        persistent = s->bmap_inner->dest_persistent;
      } else {
          persistent = flags & DIRTY_BITMAP_MIG_START_FLAG_PERSISTENT;
      }
@@ -1108,8 +1107,6 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
  
      if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
          const char *bitmap_name;
-        bool bitmap_has_dest_persistent = false;
-        bool bitmap_dest_persistent = false;
  
          if (!qemu_get_counted_string(f, s->bitmap_alias)) {
              error_report("Unable to read bitmap alias string");
@@ -1130,17 +1127,13 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
              }
  
              bitmap_name = bmap_inner->string;
-            bitmap_has_dest_persistent = bmap_inner->has_dest_persistent;
-            bitmap_dest_persistent = bmap_inner->dest_persistent;
+            s->bmap_inner = bmap_inner;
          }
  
          if (!s->cancelled) {
              g_strlcpy(s->bitmap_name, bitmap_name, sizeof(s->bitmap_name));
              s->bitmap = bdrv_find_dirty_bitmap(s->bs, s->bitmap_name);
  
-            s->has_dest_persistent = bitmap_has_dest_persistent;
-            s->dest_persistent = bitmap_dest_persistent;
-
              /*
               * bitmap may be NULL here, it wouldn't be an error if it is the
               * first occurrence of the bitmap


-- 
Best regards,
Vladimir