qemu: checkpoint: Don't merge checkpoints during deletion

Now that we've switched to the simple handling, the first thing that can
be massively simplified is checkpoint deletion. We now need to only go
through the backing chain and find the appropriately named bitmaps and
delete them, no complex lookups or merging.

Note that compared to other functions this deletes the bitmap in all
layers compared to others where we expect only exactly 1 bitmap of a
name in the backing chain to prevent potential problems.

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 9b4f3ad3961c536d71ad689321435c1b4b11bc68..c24d97443ce6ddd95cbf9653fb595912f3d2f9a4 100644 (file)
--- a/src/qemu/qemu_checkpoint.c
+++ b/src/qemu/qemu_checkpoint.c
@@ -105,140 +105,41 @@ qemuCheckpointWriteMetadata(virDomainObjPtr vm,
 }
 
 
-/**
- * qemuCheckpointFindActiveDiskInParent:
- * @vm: domain object
- * @from: starting moment object
- * @diskname: name (target) of the disk to find
- *
- * Find the first checkpoint starting from @from continuing through parents
- * of the checkpoint which describes disk @diskname. Return the pointer to the
- * definition of the disk.
- */
-static virDomainCheckpointDiskDef *
-qemuCheckpointFindActiveDiskInParent(virDomainObjPtr vm,
-                                     virDomainMomentObjPtr from,
-                                     const char *diskname)
-{
-    virDomainMomentObjPtr parent = from;
-    virDomainCheckpointDefPtr parentdef = NULL;
-    size_t i;
-
-    while (parent) {
-        parentdef = virDomainCheckpointObjGetDef(parent);
-
-        for (i = 0; i < parentdef->ndisks; i++) {
-            virDomainCheckpointDiskDef *chkdisk = &parentdef->disks[i];
-
-            if (STRNEQ(chkdisk->name, diskname))
-                continue;
-
-            /* currently inspected checkpoint doesn't describe the disk,
-             * continue into parent checkpoint */
-            if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
-                break;
-
-            return chkdisk;
-        }
-
-        parent = virDomainCheckpointFindByName(vm->checkpoints,
-                                               parentdef->parent.parent_name);
-    }
-
-    return NULL;
-}
-
-
 int
 qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src,
                                  virHashTablePtr blockNamedNodeData,
                                  const char *delbitmap,
-                                 const char *parentbitmap,
                                  virJSONValuePtr actions,
                                  const char *diskdst,
                                  GSList **reopenimages)
 {
-    virStorageSourcePtr n = src;
+    virStorageSourcePtr n;
+    bool found = false;
 
     /* find the backing chain entry with bitmap named '@delbitmap' */
-    while (n) {
-        qemuBlockNamedNodeDataBitmapPtr tmp;
-
-        if ((tmp = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData,
-                                                         n, delbitmap))) {
-            break;
-        }
-
-        n = n->backingStore;
-    }
-
-    if (!n) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("bitmap '%s' not found in backing chain of '%s'"),
-                       delbitmap, diskdst);
-        return -1;
-    }
-
-    while (n) {
-        qemuBlockNamedNodeDataBitmapPtr srcbitmap;
-
-        if (!(srcbitmap = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData,
-                                                                n, delbitmap)))
-            break;
-
-        /* For the actual checkpoint deletion we will merge any bitmap into the
-         * bitmap of the parent checkpoint (@parentbitmap) or for any image
-         * where the parent checkpoint bitmap is not present we must rename
-         * the bitmap of the deleted checkpoint into the bitmap of the parent
-         * checkpoint as qemu can't currently take the allocation map and turn
-         * it into a bitmap and thus we wouldn't be able to do a backup. */
-        if (parentbitmap) {
-            qemuBlockNamedNodeDataBitmapPtr dstbitmap;
-            g_autoptr(virJSONValue) arr = NULL;
-
-            dstbitmap = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData,
-                                                              n, parentbitmap);
-
-            if (dstbitmap) {
-                if (srcbitmap->recording && !dstbitmap->recording) {
-                    if (qemuMonitorTransactionBitmapEnable(actions,
-                                                           n->nodeformat,
-                                                           dstbitmap->name) < 0)
-                        return -1;
-                }
-
-            } else {
-                if (qemuMonitorTransactionBitmapAdd(actions,
-                                                    n->nodeformat,
-                                                    parentbitmap,
-                                                    true,
-                                                    !srcbitmap->recording,
-                                                    srcbitmap->granularity) < 0)
-                    return -1;
-            }
+    for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
+        qemuBlockNamedNodeDataBitmapPtr bitmapdata;
 
-            arr = virJSONValueNewArray();
-
-            if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(arr,
-                                                                 n->nodeformat,
-                                                                 srcbitmap->name) < 0)
-                return -1;
+        if (!(bitmapdata = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData,
+                                                                 n, delbitmap)))
+            continue;
 
-            if (qemuMonitorTransactionBitmapMerge(actions,
-                                                  n->nodeformat,
-                                                  parentbitmap, &arr) < 0)
-                return -1;
-        }
+        found = true;
 
         if (qemuMonitorTransactionBitmapRemove(actions,
                                                n->nodeformat,
-                                               srcbitmap->name) < 0)
+                                               bitmapdata->name) < 0)
             return -1;
 
         if (n != src)
             *reopenimages = g_slist_prepend(*reopenimages, n);
+    }
 
-        n = n->backingStore;
+    if (!found) {
+        virReportError(VIR_ERR_INTERNAL_ERROR,
+                       _("bitmap '%s' not found in backing chain of '%s'"),
+                       delbitmap, diskdst);
+        return -1;
     }
 
     return 0;
@@ -247,8 +148,7 @@ qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src,
 
 static int
 qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
-                             virDomainCheckpointDefPtr chkdef,
-                             virDomainMomentObjPtr parent)
+                             virDomainCheckpointDefPtr chkdef)
 {
     qemuDomainObjPrivatePtr priv = vm->privateData;
     virQEMUDriverPtr driver = priv->driver;
@@ -268,8 +168,6 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
     for (i = 0; i < chkdef->ndisks; i++) {
         virDomainCheckpointDiskDef *chkdisk = &chkdef->disks[i];
         virDomainDiskDefPtr domdisk = virDomainDiskByTarget(vm->def, chkdisk->name);
-        virDomainCheckpointDiskDef *parentchkdisk = NULL;
-        const char *parentbitmap = NULL;
 
         /* domdisk can be missing e.g. when it was unplugged */
         if (!domdisk)
@@ -278,15 +176,8 @@ qemuCheckpointDiscardBitmaps(virDomainObjPtr vm,
         if (chkdisk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
             continue;
 
-        /* If any ancestor checkpoint has a bitmap for the same
-         * disk, then this bitmap must be merged to the
-         * ancestor. */
-        if ((parentchkdisk = qemuCheckpointFindActiveDiskInParent(vm, parent,
-                                                                  chkdisk->name)))
-            parentbitmap = parentchkdisk->bitmap;
-
         if (qemuCheckpointDiscardDiskBitmaps(domdisk->src, blockNamedNodeData,
-                                             chkdisk->bitmap, parentbitmap,
+                                             chkdisk->bitmap,
                                              actions, domdisk->dst,
                                              &reopenimages) < 0)
             return -1;
@@ -334,7 +225,6 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver,
                       bool update_parent,
                       bool metadata_only)
 {
-    virDomainMomentObjPtr parent = NULL;
     g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver);
     g_autofree char *chkFile = NULL;
     bool chkcurrent = chk == virDomainCheckpointGetCurrent(vm->checkpoints);
@@ -350,14 +240,17 @@ qemuCheckpointDiscard(virQEMUDriverPtr driver,
 
     if (!metadata_only) {
         virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk);
-        parent = virDomainCheckpointFindByName(vm->checkpoints,
-                                               chk->def->parent_name);
-        if (qemuCheckpointDiscardBitmaps(vm, chkdef, parent) < 0)
+        if (qemuCheckpointDiscardBitmaps(vm, chkdef) < 0)
             return -1;
     }
 
     if (chkcurrent) {
+        virDomainMomentObjPtr parent = NULL;
+
         virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
+        parent = virDomainCheckpointFindByName(vm->checkpoints,
+                                               chk->def->parent_name);
+
         if (update_parent && parent) {
             virDomainCheckpointSetCurrent(vm->checkpoints, parent);
             if (qemuCheckpointWriteMetadata(vm, parent,

diff --git a/src/qemu/qemu_checkpoint.h b/src/qemu/qemu_checkpoint.h
index cf1e9e46cb25093f5ab935921ef2b3f0c6e2751a..0d267a188ceaf6e4a41c70e4b12da4ee712d38aa 100644 (file)
--- a/src/qemu/qemu_checkpoint.h
+++ b/src/qemu/qemu_checkpoint.h
@@ -76,7 +76,6 @@ int
 qemuCheckpointDiscardDiskBitmaps(virStorageSourcePtr src,
                                  virHashTablePtr blockNamedNodeData,
                                  const char *delbitmap,
-                                 const char *parentbitmap,
                                  virJSONValuePtr actions,
                                  const char *diskdst,
                                  GSList **reopenimages);

diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index 47e9f0f3633b208947f064aabbced38bfe257f0a..336ff4b3efb1339ee82f54aed8198ee288f25533 100644 (file)
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -830,7 +830,6 @@ struct testQemuCheckpointDeleteMergeData {
     const char *name;
     virStorageSourcePtr chain;
     const char *deletebitmap;
-    const char *parentbitmap;
     const char *nodedatafile;
 };
 
@@ -864,7 +863,6 @@ testQemuCheckpointDeleteMerge(const void *opaque)
     if (qemuCheckpointDiscardDiskBitmaps(data->chain,
                                          nodedata,
                                          data->deletebitmap,
-                                         data->parentbitmap,
                                          actions,
                                          "testdisk",
                                          &reopenimages) >= 0) {
@@ -1319,19 +1317,18 @@ mymain(void)
 
     TEST_BACKUP_BITMAP_CALCULATE("empty", bitmapSourceChain, "a", "empty");
 
-#define TEST_CHECKPOINT_DELETE_MERGE(testname, delbmp, parbmp, named) \
+#define TEST_CHECKPOINT_DELETE_MERGE(testname, delbmp, named) \
     do { \
         checkpointdeletedata.name = testname; \
         checkpointdeletedata.chain = bitmapSourceChain; \
         checkpointdeletedata.deletebitmap = delbmp; \
-        checkpointdeletedata.parentbitmap = parbmp; \
         checkpointdeletedata.nodedatafile = named; \
         if (virTestRun("checkpoint delete " testname, \
                        testQemuCheckpointDeleteMerge, &checkpointdeletedata) < 0) \
         ret = -1; \
     } while (0)
 
-    TEST_CHECKPOINT_DELETE_MERGE("empty", "a", NULL, "empty");
+    TEST_CHECKPOINT_DELETE_MERGE("empty", "a", "empty");
 
 #define TEST_BITMAP_VALIDATE(testname, bitmap, rc) \
     do { \