From 6c139a45537d021f6fd4287bd3c74cba06e6a264 Mon Sep 17 00:00:00 2001 Message-Id: <6c139a45537d021f6fd4287bd3c74cba06e6a264@dist-git> From: Peter Krempa Date: Tue, 23 Jun 2020 12:23:47 +0200 Subject: [PATCH] qemu: checkpoint: Don't chain bitmaps for checkpoints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Eric Blake (cherry picked from commit 1f92aa454aa3b04ece9f7479d517bf08ba547f0a) https://bugzilla.redhat.com/show_bug.cgi?id=1804593 Message-Id: Reviewed-by: Ján Tomko --- src/qemu/qemu_checkpoint.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-) diff --git a/src/qemu/qemu_checkpoint.c b/src/qemu/qemu_checkpoint.c index 34cf122eb3..9762a3eb9d 100644 --- a/src/qemu/qemu_checkpoint.c +++ b/src/qemu/qemu_checkpoint.c @@ -460,7 +460,6 @@ qemuCheckpointPrepare(virQEMUDriverPtr driver, static int qemuCheckpointAddActions(virDomainObjPtr vm, virJSONValuePtr actions, - virDomainMomentObjPtr old_current, virDomainCheckpointDefPtr def) { size_t i; @@ -468,7 +467,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 */ @@ -479,23 +477,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; } @@ -544,7 +525,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))) -- 2.27.0