qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap pers


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v2 2/2] migration: dirty-bitmap: Allow control of bitmap persistance
Date: Thu, 11 Feb 2021 19:04:31 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.7.1

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]