Re: [PATCH 4/7] migration/block-dirty-bitmap: keep bitmap state for all bitmaps

[Vladimir Sementsov-Ogievskiy]

22.01.2020 16:23, Vladimir Sementsov-Ogievskiy wrote:

Keep bitmap state for disabled bitmaps too. Keep the state until the
end of the process. It's needed for the following commit to implement
bitmap postcopy canceling.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
  migration/block-dirty-bitmap.c | 59 ++++++++++++++++++++++------------
  1 file changed, 38 insertions(+), 21 deletions(-)

diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index eeaab2174e..f96458113c 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -131,6 +131,7 @@ typedef struct DirtyBitmapLoadBitmapState {
      BlockDriverState *bs;
      BdrvDirtyBitmap *bitmap;
      bool migrated;
+    bool enabled;
  } DirtyBitmapLoadBitmapState;

typedef struct DirtyBitmapLoadState {

@@ -140,8 +141,11 @@ typedef struct DirtyBitmapLoadState {
      BlockDriverState *bs;
      BdrvDirtyBitmap *bitmap;

- GSList *enabled_bitmaps;

-    QemuMutex lock; /* protect enabled_bitmaps */
+    bool bitmaps_enabled; /* set in dirty_bitmap_mig_before_vm_start */
+    bool stream_ended; /* set when all migrated data handled */
+
+    GSList *bitmaps;
+    QemuMutex lock; /* protect bitmaps */
  } DirtyBitmapLoadState;
  static DirtyBitmapLoadState dbm_load_state;

@@ -446,6 +450,7 @@ static int dirty_bitmap_load_start(QEMUFile *f)

      Error *local_err = NULL;
      uint32_t granularity = qemu_get_be32(f);
      uint8_t flags = qemu_get_byte(f);
+    DirtyBitmapLoadBitmapState *b;

if (s->bitmap) {

          error_report("Bitmap with the same name ('%s') already exists on "
@@ -472,22 +477,23 @@ static int dirty_bitmap_load_start(QEMUFile *f)

bdrv_disable_dirty_bitmap(s->bitmap);

      if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
-        DirtyBitmapLoadBitmapState *b;
-
          bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
          if (local_err) {
              error_report_err(local_err);
              return -EINVAL;
          }
-
-        b = g_new(DirtyBitmapLoadBitmapState, 1);
-        b->bs = s->bs;
-        b->bitmap = s->bitmap;
-        b->migrated = false;
-        dbm_load_state.enabled_bitmaps =
-            g_slist_prepend(dbm_load_state.enabled_bitmaps, b);
      }

+ b = g_new(DirtyBitmapLoadBitmapState, 1);

+    *b = (DirtyBitmapLoadBitmapState) {
+        .bs = s->bs,
+        .bitmap = s->bitmap,
+        .migrated = false,
+        .enabled = flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED,
+    };
+
+    dbm_load_state.bitmaps = g_slist_prepend(dbm_load_state.bitmaps, b);
+
      return 0;
  }

@@ -497,22 +503,25 @@ void dirty_bitmap_mig_before_vm_start(void) qemu_mutex_lock(&dbm_load_state.lock); - for (item = dbm_load_state.enabled_bitmaps; item;

-         item = g_slist_next(item))
-    {
+    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
          DirtyBitmapLoadBitmapState *b = item->data;

+ if (!b->enabled) {

+            continue;
+        }
+
          if (b->migrated) {
              bdrv_enable_dirty_bitmap_locked(b->bitmap);
          } else {
              bdrv_dirty_bitmap_enable_successor(b->bitmap);
          }
-
-        g_free(b);
      }

- g_slist_free(dbm_load_state.enabled_bitmaps);

-    dbm_load_state.enabled_bitmaps = NULL;
+    dbm_load_state.bitmaps_enabled = true;
+    if (dbm_load_state.stream_ended) {
+        g_slist_free_full(dbm_load_state.bitmaps, g_free);
+        dbm_load_state.bitmaps = NULL;
+    }

qemu_mutex_unlock(&dbm_load_state.lock);

  }
@@ -530,9 +539,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f)
          bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
      }

- for (item = dbm_load_state.enabled_bitmaps; item;

-         item = g_slist_next(item))
-    {
+    for (item = dbm_load_state.bitmaps; item; item = g_slist_next(item)) {
          DirtyBitmapLoadBitmapState *b = item->data;

if (b->bitmap == s->bitmap) {

@@ -671,6 +678,16 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, 
int version_id)
          }
      } while (!(dbm_load_state.flags & DIRTY_BITMAP_MIG_FLAG_EOS));

+ qemu_mutex_lock(&dbm_load_state.lock);

+
+    dbm_load_state.stream_ended = true;

This is wrong. EOS doesn't mean that bitmap migration finished: only one set of 
sequential
bitmap chunks finished. EOS may be sent several times during migration and it 
is needed to
switch to other things migration.

+    if (dbm_load_state.bitmaps_enabled) {
+        g_slist_free_full(dbm_load_state.bitmaps, g_free);
+        dbm_load_state.bitmaps = NULL;
+    }
+
+    qemu_mutex_unlock(&dbm_load_state.lock);
+
      trace_dirty_bitmap_load_success();
      return 0;
  }

--
Best regards,
Vladimir