From 6c139a45537d021f6fd4287bd3c74cba06e6a264 Mon Sep 17 00:00:00 2001
Message-Id: <6c139a45537d021f6fd4287bd3c74cba06e6a264@dist-git>
From: Peter Krempa <pkrempa@redhat.com>
Date: Tue, 23 Jun 2020 12:23:47 +0200
Subject: [PATCH] qemu: checkpoint: Don't chain bitmaps for checkpoints
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Chaining bitmaps for checkpoints (disabling the active one and creating
a new) severely overcomplicated all operations in regards to bitmaps.

Specifically it requires us re-matching the on-disk state to the
internal metadata and in case of merging during block jobs it makes it
almost impossible to cover all corner cases.

Since the checkpoints and incremental backups were not yet enabled,
let's change the design to keep one bitmap per checkpoint. In case of
layered snapshots this will be filled in by using dirty-bitmap-populate.

Finally the main reason for this unnecessary complexity was the fear
that qemu's performance could degrade. In the end I think that
addressing the performance issue will be better done in qemu (e.g by
keeping an internal bitmap updated with changes and merging it
periodically back to the real bitmaps. QEMU writes out changes to disk
at shutdown so consistency is not a problem).

Removing the relationships between bitmaps frees us from complex
handling and also makes all the surrounding code more robust as one
broken bitmap doesn't necessarily invalidate whole chains of backups.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
(cherry picked from commit 1f92aa454aa3b04ece9f7479d517bf08ba547f0a)
https://bugzilla.redhat.com/show_bug.cgi?id=1804593
Message-Id: <a863b2015f72690ebd90fd518e8163e194909214.1592906423.git.pkrempa@redhat.com>
Reviewed-by: Ján Tomko <jtomko@redhat.com>
---
 src/qemu/qemu_checkpoint.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index 34cf122eb3..9762a3eb9d 100644
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -460,7 +460,6 @@ qemuCheckpointPrepare(virQEMUDriverPtr driver,
 static int
 qemuCheckpointAddActions(virDomainObjPtr vm,
                          virJSONValuePtr actions,
-                         virDomainMomentObjPtr old_current,
                          virDomainCheckpointDefPtr def)
 {
     size_t i;
@@ -468,7 +467,6 @@ qemuCheckpointAddActions(virDomainObjPtr vm,
     for (i = 0; i < def->ndisks; i++) {
         virDomainCheckpointDiskDef *chkdisk = &def->disks[i];
         virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name);
-        virDomainCheckpointDiskDef *parentchkdisk = NULL;
 
         /* checkpoint definition validator mandates that the corresponding
          * domdisk should exist */
@@ -479,23 +477,6 @@ qemuCheckpointAddActions(virDomainObjPtr vm,
         if (qemuMonitorTransactionBitmapAdd(actions, domdisk->src->nodeformat,
                                             chkdisk->bitmap, true, false, 0) < 0)
             return -1;
-
-        /* We only want one active bitmap for a disk along the
-         * checkpoint chain, then later differential backups will
-         * merge the bitmaps (only one active) between the bounding
-         * checkpoint and the leaf checkpoint.  If the same disks are
-         * involved in each checkpoint, this search terminates in one
-         * iteration; but it is also possible to have to search
-         * further than the immediate parent to find another
-         * checkpoint with a bitmap on the same disk.  */
-        if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, old_current,
-                                                                  chkdisk->name))) {
-
-            if (qemuMonitorTransactionBitmapDisable(actions,
-                                                    domdisk->src->nodeformat,
-                                                    parentchkdisk->bitmap) < 0)
-                return -1;
-        }
     }
     return 0;
 }
@@ -544,7 +525,7 @@ qemuCheckpointCreateCommon(virQEMUDriverPtr driver,
 
     tmpactions = virJSONValueNewArray();
 
-    if (qemuCheckpointAddActions(vm, tmpactions, parent, *def) < 0)
+    if (qemuCheckpointAddActions(vm, tmpactions, *def) < 0)
         return -1;
 
     if (!(*chk = virDomainCheckpointAssignDef(vm->checkpoints, *def)))
-- 
2.27.0