Blame SOURCES/kvm-dirty-bitmaps-clean-up-bitmaps-loading-and-migration.patch

26ba25
From ae232c169a5ae37d3989919775332979d4e3b7d5 Mon Sep 17 00:00:00 2001
26ba25
From: John Snow <jsnow@redhat.com>
26ba25
Date: Tue, 20 Nov 2018 18:18:26 +0000
26ba25
Subject: [PATCH 32/35] dirty-bitmaps: clean-up bitmaps loading and migration
26ba25
 logic
26ba25
26ba25
RH-Author: John Snow <jsnow@redhat.com>
26ba25
Message-id: <20181120181828.15132-23-jsnow@redhat.com>
26ba25
Patchwork-id: 83074
26ba25
O-Subject: [RHEL8/rhel qemu-kvm PATCH 22/24] dirty-bitmaps: clean-up bitmaps loading and migration logic
26ba25
Bugzilla: 1518989
26ba25
RH-Acked-by: Laurent Vivier <lvivier@redhat.com>
26ba25
RH-Acked-by: Max Reitz <mreitz@redhat.com>
26ba25
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
26ba25
26ba25
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
26ba25
26ba25
This patch aims to bring the following behavior:
26ba25
26ba25
1. We don't load bitmaps, when started in inactive mode. It's the case
26ba25
of incoming migration. In this case we wait for bitmaps migration
26ba25
through migration channel (if 'dirty-bitmaps' capability is enabled) or
26ba25
for invalidation (to load bitmaps from the image).
26ba25
26ba25
2. We don't remove persistent bitmaps on inactivation. Instead, we only
26ba25
remove bitmaps after storing. This is the only way to restore bitmaps,
26ba25
if we decided to resume source after [failed] migration with
26ba25
'dirty-bitmaps' capability enabled (which means, that bitmaps were not
26ba25
stored).
26ba25
26ba25
3. We load bitmaps on open and any invalidation, it's ok for all cases:
26ba25
  - normal open
26ba25
  - migration target invalidation with dirty-bitmaps capability
26ba25
    (bitmaps are migrating through migration channel, the are not
26ba25
     stored, so they should have IN_USE flag set and will be skipped
26ba25
     when loading. However, it would fail if bitmaps are read-only[1])
26ba25
  - migration target invalidation without dirty-bitmaps capability
26ba25
    (normal load of the bitmaps, if migrated with shared storage)
26ba25
  - source invalidation with dirty-bitmaps capability
26ba25
    (skip because IN_USE)
26ba25
  - source invalidation without dirty-bitmaps capability
26ba25
    (bitmaps were dropped, reload them)
26ba25
26ba25
[1]: to accurately handle this, migration of read-only bitmaps is
26ba25
     explicitly forbidden in this patch.
26ba25
26ba25
New mechanism for not storing bitmaps when migrate with dirty-bitmaps
26ba25
capability is introduced: migration filed in BdrvDirtyBitmap.
26ba25
26ba25
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
26ba25
Signed-off-by: John Snow <jsnow@redhat.com>
26ba25
(cherry picked from commit 9c98f145dfb994e1e9d68a4d606ee5693891280d)
26ba25
Signed-off-by: John Snow <jsnow@redhat.com>
26ba25
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
26ba25
---
26ba25
 block.c                        | 11 ++++---
26ba25
 block/dirty-bitmap.c           | 36 +++++++++--------------
26ba25
 block/qcow2-bitmap.c           | 16 +++++++++++
26ba25
 block/qcow2.c                  | 65 ++++++++++++++++++++++++++++++++++++++++--
26ba25
 include/block/dirty-bitmap.h   |  2 +-
26ba25
 migration/block-dirty-bitmap.c | 10 +++++--
26ba25
 6 files changed, 109 insertions(+), 31 deletions(-)
26ba25
26ba25
diff --git a/block.c b/block.c
26ba25
index fbd569c..18ae591 100644
26ba25
--- a/block.c
26ba25
+++ b/block.c
26ba25
@@ -4316,6 +4316,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
26ba25
     uint64_t perm, shared_perm;
26ba25
     Error *local_err = NULL;
26ba25
     int ret;
26ba25
+    BdrvDirtyBitmap *bm;
26ba25
 
26ba25
     if (!bs->drv)  {
26ba25
         return;
26ba25
@@ -4365,6 +4366,12 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs,
26ba25
         }
26ba25
     }
26ba25
 
26ba25
+    for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm;
26ba25
+         bm = bdrv_dirty_bitmap_next(bs, bm))
26ba25
+    {
26ba25
+        bdrv_dirty_bitmap_set_migration(bm, false);
26ba25
+    }
26ba25
+
26ba25
     ret = refresh_total_sectors(bs, bs->total_sectors);
26ba25
     if (ret < 0) {
26ba25
         bs->open_flags |= BDRV_O_INACTIVE;
26ba25
@@ -4479,10 +4486,6 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs,
26ba25
         }
26ba25
     }
26ba25
 
26ba25
-    /* At this point persistent bitmaps should be already stored by the format
26ba25
-     * driver */
26ba25
-    bdrv_release_persistent_dirty_bitmaps(bs);
26ba25
-
26ba25
     return 0;
26ba25
 }
26ba25
 
26ba25
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
26ba25
index 9b9ebd7..89fd1d7 100644
26ba25
--- a/block/dirty-bitmap.c
26ba25
+++ b/block/dirty-bitmap.c
26ba25
@@ -55,6 +55,10 @@ struct BdrvDirtyBitmap {
26ba25
                                    and this bitmap must remain unchanged while
26ba25
                                    this flag is set. */
26ba25
     bool persistent;            /* bitmap must be saved to owner disk image */
26ba25
+    bool migration;             /* Bitmap is selected for migration, it should
26ba25
+                                   not be stored on the next inactivation
26ba25
+                                   (persistent flag doesn't matter until next
26ba25
+                                   invalidation).*/
26ba25
     QLIST_ENTRY(BdrvDirtyBitmap) list;
26ba25
 };
26ba25
 
26ba25
@@ -390,26 +394,6 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
26ba25
 }
26ba25
 
26ba25
 /**
26ba25
- * Release all persistent dirty bitmaps attached to a BDS (for use in
26ba25
- * bdrv_inactivate_recurse()).
26ba25
- * There must not be any frozen bitmaps attached.
26ba25
- * This function does not remove persistent bitmaps from the storage.
26ba25
- * Called with BQL taken.
26ba25
- */
26ba25
-void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs)
26ba25
-{
26ba25
-    BdrvDirtyBitmap *bm, *next;
26ba25
-
26ba25
-    bdrv_dirty_bitmaps_lock(bs);
26ba25
-    QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
26ba25
-        if (bdrv_dirty_bitmap_get_persistance(bm)) {
26ba25
-            bdrv_release_dirty_bitmap_locked(bm);
26ba25
-        }
26ba25
-    }
26ba25
-    bdrv_dirty_bitmaps_unlock(bs);
26ba25
-}
26ba25
-
26ba25
-/**
26ba25
  * Remove persistent dirty bitmap from the storage if it exists.
26ba25
  * Absence of bitmap is not an error, because we have the following scenario:
26ba25
  * BdrvDirtyBitmap can have .persistent = true but not yet saved and have no
26ba25
@@ -761,16 +745,24 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent)
26ba25
     qemu_mutex_unlock(bitmap->mutex);
26ba25
 }
26ba25
 
26ba25
+/* Called with BQL taken. */
26ba25
+void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration)
26ba25
+{
26ba25
+    qemu_mutex_lock(bitmap->mutex);
26ba25
+    bitmap->migration = migration;
26ba25
+    qemu_mutex_unlock(bitmap->mutex);
26ba25
+}
26ba25
+
26ba25
 bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
26ba25
 {
26ba25
-    return bitmap->persistent;
26ba25
+    return bitmap->persistent && !bitmap->migration;
26ba25
 }
26ba25
 
26ba25
 bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs)
26ba25
 {
26ba25
     BdrvDirtyBitmap *bm;
26ba25
     QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
26ba25
-        if (bm->persistent && !bm->readonly) {
26ba25
+        if (bm->persistent && !bm->readonly && !bm->migration) {
26ba25
             return true;
26ba25
         }
26ba25
     }
26ba25
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
26ba25
index 14050eb..a36773c 100644
26ba25
--- a/block/qcow2-bitmap.c
26ba25
+++ b/block/qcow2-bitmap.c
26ba25
@@ -1418,6 +1418,22 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
26ba25
         g_free(tb);
26ba25
     }
26ba25
 
26ba25
+    QSIMPLEQ_FOREACH(bm, bm_list, entry) {
26ba25
+        /* For safety, we remove bitmap after storing.
26ba25
+         * We may be here in two cases:
26ba25
+         * 1. bdrv_close. It's ok to drop bitmap.
26ba25
+         * 2. inactivation. It means migration without 'dirty-bitmaps'
26ba25
+         *    capability, so bitmaps are not marked with
26ba25
+         *    BdrvDirtyBitmap.migration flags. It's not bad to drop them too,
26ba25
+         *    and reload on invalidation.
26ba25
+         */
26ba25
+        if (bm->dirty_bitmap == NULL) {
26ba25
+            continue;
26ba25
+        }
26ba25
+
26ba25
+        bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap);
26ba25
+    }
26ba25
+
26ba25
     bitmap_list_free(bm_list);
26ba25
     return;
26ba25
 
26ba25
diff --git a/block/qcow2.c b/block/qcow2.c
26ba25
index d260cd6..36d1152 100644
26ba25
--- a/block/qcow2.c
26ba25
+++ b/block/qcow2.c
26ba25
@@ -1487,8 +1487,69 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
26ba25
         s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
26ba25
     }
26ba25
 
26ba25
-    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
26ba25
-        update_header = false;
26ba25
+    /* == Handle persistent dirty bitmaps ==
26ba25
+     *
26ba25
+     * We want load dirty bitmaps in three cases:
26ba25
+     *
26ba25
+     * 1. Normal open of the disk in active mode, not related to invalidation
26ba25
+     *    after migration.
26ba25
+     *
26ba25
+     * 2. Invalidation of the target vm after pre-copy phase of migration, if
26ba25
+     *    bitmaps are _not_ migrating through migration channel, i.e.
26ba25
+     *    'dirty-bitmaps' capability is disabled.
26ba25
+     *
26ba25
+     * 3. Invalidation of source vm after failed or canceled migration.
26ba25
+     *    This is a very interesting case. There are two possible types of
26ba25
+     *    bitmaps:
26ba25
+     *
26ba25
+     *    A. Stored on inactivation and removed. They should be loaded from the
26ba25
+     *       image.
26ba25
+     *
26ba25
+     *    B. Not stored: not-persistent bitmaps and bitmaps, migrated through
26ba25
+     *       the migration channel (with dirty-bitmaps capability).
26ba25
+     *
26ba25
+     *    On the other hand, there are two possible sub-cases:
26ba25
+     *
26ba25
+     *    3.1 disk was changed by somebody else while were inactive. In this
26ba25
+     *        case all in-RAM dirty bitmaps (both persistent and not) are
26ba25
+     *        definitely invalid. And we don't have any method to determine
26ba25
+     *        this.
26ba25
+     *
26ba25
+     *        Simple and safe thing is to just drop all the bitmaps of type B on
26ba25
+     *        inactivation. But in this case we lose bitmaps in valid 4.2 case.
26ba25
+     *
26ba25
+     *        On the other hand, resuming source vm, if disk was already changed
26ba25
+     *        is a bad thing anyway: not only bitmaps, the whole vm state is
26ba25
+     *        out of sync with disk.
26ba25
+     *
26ba25
+     *        This means, that user or management tool, who for some reason
26ba25
+     *        decided to resume source vm, after disk was already changed by
26ba25
+     *        target vm, should at least drop all dirty bitmaps by hand.
26ba25
+     *
26ba25
+     *        So, we can ignore this case for now, but TODO: "generation"
26ba25
+     *        extension for qcow2, to determine, that image was changed after
26ba25
+     *        last inactivation. And if it is changed, we will drop (or at least
26ba25
+     *        mark as 'invalid' all the bitmaps of type B, both persistent
26ba25
+     *        and not).
26ba25
+     *
26ba25
+     *    3.2 disk was _not_ changed while were inactive. Bitmaps may be saved
26ba25
+     *        to disk ('dirty-bitmaps' capability disabled), or not saved
26ba25
+     *        ('dirty-bitmaps' capability enabled), but we don't need to care
26ba25
+     *        of: let's load bitmaps as always: stored bitmaps will be loaded,
26ba25
+     *        and not stored has flag IN_USE=1 in the image and will be skipped
26ba25
+     *        on loading.
26ba25
+     *
26ba25
+     * One remaining possible case when we don't want load bitmaps:
26ba25
+     *
26ba25
+     * 4. Open disk in inactive mode in target vm (bitmaps are migrating or
26ba25
+     *    will be loaded on invalidation, no needs try loading them before)
26ba25
+     */
26ba25
+
26ba25
+    if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
26ba25
+        /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
26ba25
+        bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
26ba25
+
26ba25
+        update_header = update_header && !header_updated;
26ba25
     }
26ba25
     if (local_err != NULL) {
26ba25
         error_propagate(errp, local_err);
26ba25
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
26ba25
index 1463943..8f38a3d 100644
26ba25
--- a/include/block/dirty-bitmap.h
26ba25
+++ b/include/block/dirty-bitmap.h
26ba25
@@ -26,7 +26,6 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
26ba25
                                         const char *name);
26ba25
 void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap);
26ba25
 void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs);
26ba25
-void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs);
26ba25
 void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs,
26ba25
                                          const char *name,
26ba25
                                          Error **errp);
26ba25
@@ -72,6 +71,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
26ba25
 void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked);
26ba25
 void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src,
26ba25
                              HBitmap **backup, Error **errp);
26ba25
+void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration);
26ba25
 
26ba25
 /* Functions that require manual locking.  */
26ba25
 void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap);
26ba25
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
26ba25
index 47251af..ffe7aca 100644
26ba25
--- a/migration/block-dirty-bitmap.c
26ba25
+++ b/migration/block-dirty-bitmap.c
26ba25
@@ -307,6 +307,12 @@ static int init_dirty_bitmap_migration(void)
26ba25
                 goto fail;
26ba25
             }
26ba25
 
26ba25
+            if (bdrv_dirty_bitmap_readonly(bitmap)) {
26ba25
+                error_report("Can't migrate read-only dirty bitmap: '%s",
26ba25
+                             bdrv_dirty_bitmap_name(bitmap));
26ba25
+                goto fail;
26ba25
+            }
26ba25
+
26ba25
             bdrv_ref(bs);
26ba25
             bdrv_dirty_bitmap_set_qmp_locked(bitmap, true);
26ba25
 
26ba25
@@ -329,9 +335,9 @@ static int init_dirty_bitmap_migration(void)
26ba25
         }
26ba25
     }
26ba25
 
26ba25
-    /* unset persistance here, to not roll back it */
26ba25
+    /* unset migration flags here, to not roll back it */
26ba25
     QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) {
26ba25
-        bdrv_dirty_bitmap_set_persistance(dbms->bitmap, false);
26ba25
+        bdrv_dirty_bitmap_set_migration(dbms->bitmap, true);
26ba25
     }
26ba25
 
26ba25
     if (QSIMPLEQ_EMPTY(&dirty_bitmap_mig_state.dbms_list)) {
26ba25
-- 
26ba25
1.8.3.1
26ba25