[RFC PATCH 33/41] qemu: backup: Rewrite backup bitmap handling to the new bitmap semantics

Reuse qemuBlockGetBitmapMergeActions which allows to remove the ad-hoc iplementatio 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 dependancy 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(a)redhat.com&gt; --- src/qemu/qemu_backup.c | 239 +++++------------- src/qemu/qemu_backup.h | 13 +- tests/qemublocktest.c | 90 ++----- .../backupmerge/empty-out.json | 4 +- 4 files changed, 88 insertions(+), 258 deletions(-) diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c index 5729aac858..242a75431b 100644 --- a/src/qemu/qemu_backup.c +++ b/src/qemu/qemu_backup.c @@ -170,199 +170,68 @@ 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, + GSList **allocationbitmapnodes, + virHashTablePtr blockNamedNodeData) { - g_autoptr(virJSONValue) ret = virJSONValueNewArray(); - virStorageSourcePtr tmpsrc = NULL; - virStorageSourcePtr n; - bool foundbitmap = false; - - 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(ret, - n->nodeformat, - bitmapname) < 0) - return -1; - - tmpsrc = n; - } + g_autoptr(virJSONValue) tmpactions = NULL; + g_autoptr(GSList) tmpallocbitmapnodes = NULL; - 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, &tmpallocbitmapnodes, + blockNamedNodeData) < 0) return -1; - } - *actions = g_steal_pointer(&ret); - *to = tmpsrc; - - 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; + if (tmpactions && + virJSONValueArrayConcat(actions, tmpactions) < 0) + return -1; - n = tmpsrc; + if (allocationbitmapnodes) { + *allocationbitmapnodes = g_slist_concat(*allocationbitmapnodes, tmpallocbitmapnodes); + tmpallocbitmapnodes = NULL; } - return g_steal_pointer(&ret); + return 0; } static int qemuBackupDiskPrepareOneBitmaps(struct qemuBackupDiskData *dd, virJSONValuePtr actions, - virDomainMomentDefPtr *incremental, + GSList **allocationbitmapnodes, 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, + allocationbitmapnodes, + 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, + allocationbitmapnodes, + blockNamedNodeData) < 0) return -1; return 0; @@ -374,12 +243,12 @@ qemuBackupDiskPrepareDataOne(virDomainObjPtr vm, virDomainBackupDiskDefPtr backupdisk, struct qemuBackupDiskData *dd, virJSONValuePtr actions, + GSList **allocationbitmapnodes, bool pull, virHashTablePtr blockNamedNodeData, virQEMUDriverConfigPtr cfg) { qemuDomainObjPrivatePtr priv = vm->privateData; - g_autofree virDomainMomentDefPtr *incremental = NULL; /* set data structure */ dd->backupdisk = backupdisk; @@ -417,15 +286,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, + if (qemuBackupDiskPrepareOneBitmaps(dd, actions, allocationbitmapnodes, blockNamedNodeData) < 0) return -1; } @@ -490,6 +356,7 @@ qemuBackupDiskPrepareData(virDomainObjPtr vm, virDomainBackupDefPtr def, virHashTablePtr blockNamedNodeData, virJSONValuePtr actions, + GSList **allocationbitmapnodes, virQEMUDriverConfigPtr cfg, struct qemuBackupDiskData **rdd) { @@ -509,7 +376,8 @@ qemuBackupDiskPrepareData(virDomainObjPtr vm, ndisks++; - if (qemuBackupDiskPrepareDataOne(vm, backupdisk, dd, actions, pull, + if (qemuBackupDiskPrepareDataOne(vm, backupdisk, dd, actions, + allocationbitmapnodes, pull, blockNamedNodeData, cfg) < 0) goto error; @@ -809,6 +677,7 @@ qemuBackupBegin(virDomainObjPtr vm, bool reuse = (flags & VIR_DOMAIN_BACKUP_BEGIN_REUSE_EXTERNAL); int rc = 0; int ret = -1; + g_autoptr(GSList) allocationbitmapnodes = NULL; virCheckFlags(VIR_DOMAIN_BACKUP_BEGIN_REUSE_EXTERNAL, -1); @@ -880,8 +749,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, + &allocationbitmapnodes, cfg, &dd)) <= 0) { if (ndd == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("no disks selected for backup")); @@ -893,6 +762,10 @@ qemuBackupBegin(virDomainObjPtr vm, if (qemuBackupDiskPrepareStorage(vm, dd, ndd, blockNamedNodeData, reuse) < 0) goto endjob; + if (qemuBlockBitmapTemporaryAdd(vm, blockNamedNodeData, &allocationbitmapnodes, + QEMU_ASYNC_JOB_BACKUP) < 0) + goto endjob; + priv->backup = g_steal_pointer(&def); if (qemuDomainObjEnterMonitorAsync(priv->driver, vm, QEMU_ASYNC_JOB_BACKUP) < 0) @@ -939,6 +812,8 @@ qemuBackupBegin(virDomainObjPtr vm, endjob: qemuBackupDiskDataCleanup(vm, dd, ndd); + qemuBlockBitmapTemporaryRemove(vm, allocationbitmapnodes, QEMU_ASYNC_JOB_BACKUP); + /* if 'chk' is non-NULL here it's a failure and it must be rolled back */ qemuCheckpointRollbackMetadata(vm, chk); diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h index b19c3bf1c9..be280f1f3f 100644 --- a/src/qemu/qemu_backup.h +++ b/src/qemu/qemu_backup.h @@ -53,8 +53,11 @@ 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, + GSList **allocationbitmapnodes, + virHashTablePtr blockNamedNodeData); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index c10eabb6a0..2488aa2200 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,11 @@ 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(GSList) allocationbitmapnodes = 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 +750,27 @@ testQemuBackupIncrementalBitmapCalculate(const void *opaque) return -1; } - incremental = testQemuBackupGetIncremental(data->incremental); + if (!(target = virStorageSourceNew())) + return -1; + + target->nodeformat = g_strdup_printf("target_node"); - if ((mergebitmaps = qemuBackupDiskPrepareOneBitmapsChain(incremental, - data->chain, - nodedata, - "testdisk"))) { - if (!(actual = virJSONValueToString(mergebitmaps, true))) + if (qemuBackupDiskPrepareOneBitmapsChain(data->chain, + target, + "target-bitmap-name", + data->incremental, + actions, + &allocationbitmapnodes, + nodedata) >= 0) { + if (virJSONValueToBuffer(actions, &buf, true) < 0) return -1; } else { - actual = g_strdup("NULL\n"); + virBufferAddLit(&buf, "NULL\n"); } - return virTestCompareToFile(actual, expectpath); + testQemuBitmapListPrint("allocation bitmap:", allocationbitmapnodes, &buf); + + 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.26.2