From e7b1604c5848861db0896b3c7869c0ad1d971084 Mon Sep 17 00:00:00 2001

Message-Id: <e7b1604c5848861db0896b3c7869c0ad1d971084@dist-git>

From: Peter Krempa <pkrempa@redhat.com>

Date: Tue, 23 Jun 2020 12:23:53 +0200

Subject: [PATCH] qemu: checkpoint: Don't merge checkpoints during deletion

MIME-Version: 1.0

Content-Type: text/plain; charset=UTF-8

Content-Transfer-Encoding: 8bit

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>

(cherry picked from commit 4c33c5568c3ee3c5952e5f3b217d78df95683b71)

https://bugzilla.redhat.com/show_bug.cgi?id=1804593

Message-Id: <aa99845392d5d5141672bf8d50655034e6783794.1592906423.git.pkrempa@redhat.com>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

---

 src/qemu/qemu_checkpoint.c | 153 ++++++-------------------------------

 src/qemu/qemu_checkpoint.h |   1 -

 tests/qemublocktest.c      |   7 +-

 3 files changed, 25 insertions(+), 136 deletions(-)

diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c

index 9762a3eb9d..b44ff05c32 100644

--- 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;

@@ -270,8 +170,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)

@@ -280,15 +178,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;

@@ -336,7 +227,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);

@@ -352,14 +242,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 cf1e9e46cb..0d267a188c 100644

--- 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 2f675d7e03..201e5df6b4 100644

--- 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) {

@@ -1318,19 +1316,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 { \

-- 

2.27.0