From 1fd01969c2a7c4deefc28363c9748786ca5d7169 Mon Sep 17 00:00:00 2001

Message-Id: <1fd01969c2a7c4deefc28363c9748786ca5d7169@dist-git>

From: Peter Krempa <pkrempa@redhat.com>

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

Subject: [PATCH] qemu: backup: Rewrite backup bitmap handling to the new

 bitmap semantics

MIME-Version: 1.0

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

Content-Transfer-Encoding: 8bit

Reuse qemuBlockGetBitmapMergeActions which allows removal of the ad-hoc

implementation of bitmap merging for backup. The new approach is simpler

and also more robust in case some of the bitmaps break as they remove

the dependency on the whole chain of bitmaps working.

The new approach also allows backups if a snapshot is created outside of

libvirt.

Additionally the code is greatly simplified.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>

Reviewed-by: Eric Blake <eblake@redhat.com>

(cherry picked from commit e0d8d989e2bc9024f85b676166da0305c4b4014d)

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

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

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

---

 src/qemu/qemu_backup.c                        | 220 +++---------------

 src/qemu/qemu_backup.h                        |  12 +-

 tests/qemublocktest.c                         |  86 ++-----

 .../backupmerge/empty-out.json                |   4 +-

 4 files changed, 63 insertions(+), 259 deletions(-)

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

index 13e2a7412b..67d646f477 100644

--- a/src/qemu/qemu_backup.c

+++ b/src/qemu/qemu_backup.c

@@ -173,199 +173,58 @@ qemuBackupDiskDataCleanup(virDomainObjPtr vm,

 }

-/**

- * qemuBackupBeginCollectIncrementalCheckpoints:

- * @vm: domain object

- * @incrFrom: name of checkpoint representing starting point of incremental backup

- *

- * Returns a NULL terminated list of pointers to checkpoint definitions in

- * chronological order starting from the 'current' checkpoint until reaching

- * @incrFrom.

- */

-static virDomainMomentDefPtr *

-qemuBackupBeginCollectIncrementalCheckpoints(virDomainObjPtr vm,

-                                             const char *incrFrom)

-{

-    virDomainMomentObjPtr n = virDomainCheckpointGetCurrent(vm->checkpoints);

-    g_autofree virDomainMomentDefPtr *incr = NULL;

-    size_t nincr = 0;

-

-    while (n) {

-        virDomainMomentDefPtr def = n->def;

-

-        if (VIR_APPEND_ELEMENT_COPY(incr, nincr, def) < 0)

-            return NULL;

-

-        if (STREQ(def->name, incrFrom)) {

-            def = NULL;

-            if (VIR_APPEND_ELEMENT_COPY(incr, nincr, def) < 0)

-                return NULL;

-

-            return g_steal_pointer(&incr;;

-        }

-

-        if (!n->def->parent_name)

-            break;

-

-        n = virDomainCheckpointFindByName(vm->checkpoints, n->def->parent_name);

-    }

-

-    virReportError(VIR_ERR_OPERATION_INVALID,

-                   _("could not locate checkpoint '%s' for incremental backup"),

-                   incrFrom);

-    return NULL;

-}

-

-

-static int

-qemuBackupGetBitmapMergeRange(virStorageSourcePtr from,

-                              const char *bitmapname,

-                              virJSONValuePtr *actions,

-                              virStorageSourcePtr *to,

-                              const char *diskdst,

-                              virHashTablePtr blockNamedNodeData)

+int

+qemuBackupDiskPrepareOneBitmapsChain(virStorageSourcePtr backingChain,

+                                     virStorageSourcePtr targetsrc,

+                                     const char *targetbitmap,

+                                     const char *incremental,

+                                     virJSONValuePtr actions,

+                                     virHashTablePtr blockNamedNodeData)

 {

-    g_autoptr(virJSONValue) act = virJSONValueNewArray();

-    virStorageSourcePtr tmpsrc = NULL;

-    virStorageSourcePtr n;

-    bool foundbitmap = false;

+    g_autoptr(virJSONValue) tmpactions = NULL;

-    for (n = from; virStorageSourceIsBacking(n); n = n->backingStore) {

-        qemuBlockNamedNodeDataBitmapPtr bitmap = NULL;

-

-        if (!(bitmap = qemuBlockNamedNodeDataGetBitmapByName(blockNamedNodeData,

-                                                             n,

-                                                             bitmapname)))

-            break;

-

-        foundbitmap = true;

-

-        if (bitmap->inconsistent) {

-            virReportError(VIR_ERR_INVALID_ARG,

-                           _("bitmap '%s' for image '%s%u' is inconsistent"),

-                           bitmap->name, diskdst, n->id);

-            return -1;

-        }

-

-        if (qemuMonitorTransactionBitmapMergeSourceAddBitmap(act,

-                                                             n->nodeformat,

-                                                             bitmapname) < 0)

-            return -1;

-

-        tmpsrc = n;

-    }

-

-    if (!foundbitmap) {

-        virReportError(VIR_ERR_INVALID_ARG,

-                       _("failed to find bitmap '%s' in image '%s%u'"),

-                       bitmapname, diskdst, from->id);

+    if (qemuBlockGetBitmapMergeActions(backingChain, NULL, targetsrc,

+                                       incremental, targetbitmap, NULL,

+                                       &tmpactions,

+                                       blockNamedNodeData) < 0)

         return -1;

-    }

-    *actions = g_steal_pointer(&act;;

-    *to = tmpsrc;

+    if (tmpactions &&

+        virJSONValueArrayConcat(actions, tmpactions) < 0)

+        return -1;

     return 0;

 }

-virJSONValuePtr

-qemuBackupDiskPrepareOneBitmapsChain(virDomainMomentDefPtr *incremental,

-                                     virStorageSourcePtr backingChain,

-                                     virHashTablePtr blockNamedNodeData,

-                                     const char *diskdst)

-{

-    g_autoptr(virJSONValue) ret = NULL;

-    size_t incridx = 0;

-    virStorageSourcePtr n = backingChain;

-

-    ret = virJSONValueNewArray();

-

-    for (incridx = 0; incremental[incridx]; incridx++) {

-        g_autoptr(virJSONValue) tmp = virJSONValueNewArray();

-        virStorageSourcePtr tmpsrc = NULL;

-        virDomainCheckpointDefPtr chkdef = (virDomainCheckpointDefPtr) incremental[incridx];

-        bool checkpoint_has_disk = false;

-        size_t i;

-

-        for (i = 0; i < chkdef->ndisks; i++) {

-            if (STRNEQ_NULLABLE(diskdst, chkdef->disks[i].name))

-                continue;

-

-            if (chkdef->disks[i].type == VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)

-                checkpoint_has_disk = true;

-

-            break;

-        }

-

-        if (!checkpoint_has_disk) {

-            if (!incremental[incridx + 1]) {

-                virReportError(VIR_ERR_INVALID_ARG,

-                               _("disk '%s' not found in checkpoint '%s'"),

-                               diskdst, incremental[incridx]->name);

-                return NULL;

-            }

-

-            continue;

-        }

-

-        if (qemuBackupGetBitmapMergeRange(n, incremental[incridx]->name,

-                                          &tmp, &tmpsrc, diskdst,

-                                          blockNamedNodeData) < 0)

-            return NULL;

-

-        if (virJSONValueArrayConcat(ret, tmp) < 0)

-            return NULL;

-

-        n = tmpsrc;

-    }

-

-    return g_steal_pointer(&ret;;

-}

-

-

 static int

 qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd,

                                 virJSONValuePtr actions,

-                                virDomainMomentDefPtr *incremental,

                                 virHashTablePtr blockNamedNodeData)

 {

-    g_autoptr(virJSONValue) mergebitmaps = NULL;

-    g_autoptr(virJSONValue) mergebitmapsstore = NULL;

-

-    if (!(mergebitmaps = qemuBackupDiskPrepareOneBitmapsChain(incremental,

-                                                              dd->domdisk->src,

-                                                              blockNamedNodeData,

-                                                              dd->domdisk->dst)))

-        return -1;

-

-    if (!(mergebitmapsstore = virJSONValueCopy(mergebitmaps)))

-        return -1;

-

-    if (qemuMonitorTransactionBitmapAdd(actions,

-                                        dd->domdisk->src->nodeformat,

-                                        dd->incrementalBitmap,

-                                        false,

-                                        true, 0) < 0)

-        return -1;

-

-    if (qemuMonitorTransactionBitmapMerge(actions,

-                                          dd->domdisk->src->nodeformat,

-                                          dd->incrementalBitmap,

-                                          &mergebitmaps) < 0)

+    if (!qemuBlockBitmapChainIsValid(dd->domdisk->src,

+                                     dd->backupdisk->incremental,

+                                     blockNamedNodeData)) {

+        virReportError(VIR_ERR_INVALID_ARG,

+                       _("missing or broken bitmap '%s' for disk '%s'"),

+                       dd->backupdisk->incremental, dd->domdisk->dst);

         return -1;

+    }

-    if (qemuMonitorTransactionBitmapAdd(actions,

-                                        dd->store->nodeformat,

-                                        dd->incrementalBitmap,

-                                        false,

-                                        true, 0) < 0)

+    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,

+                                             dd->domdisk->src,

+                                             dd->incrementalBitmap,

+                                             dd->backupdisk->incremental,

+                                             actions,

+                                             blockNamedNodeData) < 0)

         return -1;

-    if (qemuMonitorTransactionBitmapMerge(actions,

-                                          dd->store->nodeformat,

-                                          dd->incrementalBitmap,

-                                          &mergebitmapsstore) < 0)

+    if (qemuBackupDiskPrepareOneBitmapsChain(dd->domdisk->src,

+                                             dd->store,

+                                             dd->incrementalBitmap,

+                                             dd->backupdisk->incremental,

+                                             actions,

+                                             blockNamedNodeData) < 0)

         return -1;

     return 0;

@@ -382,7 +241,6 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,

                              virQEMUDriverConfigPtr cfg)

 {

     qemuDomainObjPrivatePtr priv = vm->privateData;

-    g_autofree virDomainMomentDefPtr *incremental = NULL;

     /* set data structure */

     dd->backupdisk = backupdisk;

@@ -418,16 +276,12 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm,

         return -1;

     if (dd->backupdisk->incremental) {

-        if (!(incremental = qemuBackupBeginCollectIncrementalCheckpoints(vm, dd->backupdisk->incremental)))

-            return -1;

-

         if (dd->backupdisk->exportbitmap)

             dd->incrementalBitmap = g_strdup(dd->backupdisk->exportbitmap);

         else

             dd->incrementalBitmap = g_strdup_printf("backup-%s", dd->domdisk->dst);

-        if (qemuBackupDiskPrepareOneBitmaps(dd, actions, incremental,

-                                            blockNamedNodeData) < 0)

+        if (qemuBackupDiskPrepareOneBitmaps(dd, actions, blockNamedNodeData) < 0)

             return -1;

     }

@@ -881,8 +735,8 @@ qemuBackupBegin(virDomainObjPtr vm,

     if (!(blockNamedNodeData = qemuBlockGetNamedNodeData(vm, QEMU_ASYNC_JOB_BACKUP)))

         goto endjob;

-    if ((ndd = qemuBackupDiskPrepareData(vm, def, blockNamedNodeData,

-                                         actions, cfg, &dd)) <= 0) {

+    if ((ndd = qemuBackupDiskPrepareData(vm, def, blockNamedNodeData, actions,

+                                         cfg, &dd)) <= 0) {

         if (ndd == 0) {

             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",

                            _("no disks selected for backup"));

diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h

index 3321ba0b6f..c821baf0ee 100644

--- a/src/qemu/qemu_backup.h

+++ b/src/qemu/qemu_backup.h

@@ -52,8 +52,10 @@ qemuBackupGetJobInfoStats(virQEMUDriverPtr driver,

                           qemuDomainJobInfoPtr jobInfo);

 /* exported for testing */

-virJSONValuePtr

-qemuBackupDiskPrepareOneBitmapsChain(virDomainMomentDefPtr *incremental,

-                                     virStorageSourcePtr backingChain,

-                                     virHashTablePtr blockNamedNodeData,

-                                     const char *diskdst);

+int

+qemuBackupDiskPrepareOneBitmapsChain(virStorageSourcePtr backingChain,

+                                     virStorageSourcePtr targetsrc,

+                                     const char *targetbitmap,

+                                     const char *incremental,

+                                     virJSONValuePtr actions,

+                                     virHashTablePtr blockNamedNodeData);

diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c

index 7c8f67dd6b..dcdc9eade0 100644

--- a/tests/qemublocktest.c

+++ b/tests/qemublocktest.c

@@ -716,65 +716,6 @@ testQemuBitmapGetFakeChainEntry(virStorageSourcePtr src,

 }

-typedef virDomainMomentDefPtr testMomentList;

-

-static void

-testMomentListFree(testMomentList *list)

-{

-    testMomentList *tmp = list;

-

-    if (!list)

-        return;

-

-    while (*tmp) {

-        virObjectUnref(*tmp);

-        tmp++;

-    }

-

-    g_free(list);

-}

-

-G_DEFINE_AUTOPTR_CLEANUP_FUNC(testMomentList, testMomentListFree);

-

-static virDomainMomentDefPtr

-testQemuBackupGetIncrementalMoment(const char *name)

-{

-    virDomainCheckpointDefPtr checkpoint = NULL;

-

-    if (!(checkpoint = virDomainCheckpointDefNew()))

-        abort();

-

-    checkpoint->disks = g_new0(virDomainCheckpointDiskDef, 1);

-    checkpoint->ndisks = 1;

-

-    checkpoint->disks[0].name = g_strdup("testdisk");

-    checkpoint->disks[0].type = VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP;

-

-    checkpoint->parent.name = g_strdup(name);

-

-    return (virDomainMomentDefPtr) checkpoint;

-}

-

-

-static virDomainMomentDefPtr *

-testQemuBackupGetIncremental(const char *incFrom)

-{

-    const char *checkpoints[] = {"current", "d", "c", "b", "a"};

-    virDomainMomentDefPtr *incr;

-    size_t i;

-

-    incr = g_new0(virDomainMomentDefPtr, G_N_ELEMENTS(checkpoints) + 1);

-

-    for (i = 0; i < G_N_ELEMENTS(checkpoints); i++) {

-        incr[i] = testQemuBackupGetIncrementalMoment(checkpoints[i]);

-

-        if (STREQ(incFrom, checkpoints[i]))

-            break;

-    }

-

-    return incr;

-}

-

 static const char *backupDataPrefix = "qemublocktestdata/backupmerge/";

 struct testQemuBackupIncrementalBitmapCalculateData {

@@ -791,10 +732,10 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)

     const struct testQemuBackupIncrementalBitmapCalculateData *data = opaque;

     g_autoptr(virJSONValue) nodedatajson = NULL;

     g_autoptr(virHashTable) nodedata = NULL;

-    g_autoptr(virJSONValue) mergebitmaps = NULL;

-    g_autofree char *actual = NULL;

+    g_autoptr(virJSONValue) actions = virJSONValueNewArray();

     g_autofree char *expectpath = NULL;

-    g_autoptr(testMomentList) incremental = NULL;

+    g_autoptr(virStorageSource) target = NULL;

+    g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;

     expectpath = g_strdup_printf("%s/%s%s-out.json", abs_srcdir,

                                  backupDataPrefix, data->name);

@@ -808,19 +749,24 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque)

         return -1;

     }

-    incremental = testQemuBackupGetIncremental(data->incremental);

+    if (!(target = virStorageSourceNew()))

+        return -1;

-    if ((mergebitmaps = qemuBackupDiskPrepareOneBitmapsChain(incremental,

-                                                             data->chain,

-                                                             nodedata,

-                                                             "testdisk"))) {

-        if (!(actual = virJSONValueToString(mergebitmaps, true)))

+    target->nodeformat = g_strdup_printf("target_node");

+

+    if (qemuBackupDiskPrepareOneBitmapsChain(data->chain,

+                                             target,

+                                             "target-bitmap-name",

+                                             data->incremental,

+                                             actions,

+                                             nodedata) >= 0) {

+        if (virJSONValueToBuffer(actions, &buf, true) < 0)

             return -1;

     } else {

-        actual = g_strdup("NULL\n");

+        virBufferAddLit(&buf, "NULL\n");

     }

-    return virTestCompareToFile(actual, expectpath);

+    return virTestCompareToFile(virBufferCurrentContent(&buf), expectpath);

 }

diff --git a/tests/qemublocktestdata/backupmerge/empty-out.json b/tests/qemublocktestdata/backupmerge/empty-out.json

index 7951defec1..41b42e677b 100644

--- a/tests/qemublocktestdata/backupmerge/empty-out.json

+++ b/tests/qemublocktestdata/backupmerge/empty-out.json

@@ -1 +1,3 @@

-NULL

+[

+

+]

-- 

2.27.0