qemu: checkpoint: Don't chain bitmaps for checkpoints

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>

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c
index b71b4a7d14dc0f8ed6580ab12fb27029c9d7a9b9..9b4f3ad3961c536d71ad689321435c1b4b11bc68 100644 (file)
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -456,7 +456,6 @@ qemuCheckpointPrepare(virQEMUDriverPtr driver,
 static int
 qemuCheckpointAddActions(virDomainObjPtr vm,
                          virJSONValuePtr actions,
-                         virDomainMomentObjPtr old_current,
                          virDomainCheckpointDefPtr def)
 {
     size_t i;
@@ -464,7 +463,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 */
@@ -475,23 +473,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;
 }
@@ -540,7 +521,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)))