[libvirt] [PATCH v9 12/13] backup: Wire up qemu checkpoint commands over QMP
Peter Krempa
pkrempa at redhat.com
Mon Jul 8 21:00:21 UTC 2019
On Sat, Jul 06, 2019 at 22:56:12 -0500, Eric Blake wrote:
> Time to actually issue the QMP transactions that create and
> delete persistent checkpoints. For create, we only need one
> transaction: inside, we visit all disks affected by the
> checkpoint, and create a new enabled bitmap, as well as
> disabling the bitmap of the parent checkpoint (if any). For
> deletion, we need multiple calls: if the checkpoint to be
> deleted is active, we must enable the parent; then we must
> merge the existing checkpoint into the parent, and finally
> we can delete the checkpoint.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> src/qemu/qemu_domain.c | 105 ++++++++++++++++++++++++++++++-----------
> src/qemu/qemu_driver.c | 92 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 165 insertions(+), 32 deletions(-)
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 03a143a228..2fca7bd0b8 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -8960,45 +8960,93 @@ qemuDomainCheckpointDiscard(virQEMUDriverPtr driver,
> char *chkFile = NULL;
> int ret = -1;
> virDomainMomentObjPtr parentchk = NULL;
> + virDomainCheckpointDefPtr parentdef = NULL;
> virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
> + size_t i, j;
> + virJSONValuePtr arr = NULL;
>
> - if (!metadata_only) {
> - if (!virDomainObjIsActive(vm)) {
> - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> - _("cannot remove checkpoint from inactive domain"));
> - goto cleanup;
> - } else {
> - /* TODO: Implement QMP sequence to merge bitmaps */
> - // qemuDomainObjPrivatePtr priv;
> - // priv = vm->privateData;
> - // qemuDomainObjEnterMonitor(driver, vm);
> - // /* we continue on even in the face of error */
> - // qemuMonitorDeleteCheckpoint(priv->mon, chk->def->name);
> - // ignore_value(qemuDomainObjExitMonitor(driver, vm));
> - }
> + if (!metadata_only && !virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("cannot remove checkpoint from inactive domain"));
> + goto cleanup;
> }
>
> if (virAsprintf(&chkFile, "%s/%s/%s.xml", cfg->checkpointDir,
> vm->def->name, chk->def->name) < 0)
> goto cleanup;
>
> + if (chk->def->parent_name) {
> + parentchk = virDomainCheckpointFindByName(vm->checkpoints,
> + chk->def->parent_name);
> + if (!parentchk) {
> + VIR_WARN("missing parent checkpoint matching name '%s'",
> + chk->def->parent_name);
> + }
> + parentdef = virDomainCheckpointObjGetDef(parentchk);
> + }
> +
> + if (!metadata_only) {
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + bool success = true;
> + virDomainCheckpointDefPtr chkdef = virDomainCheckpointObjGetDef(chk);
> +
> + if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
> + goto cleanup;
As described in previous patch, this is undesired.
> + qemuDomainObjEnterMonitor(driver, vm);
> + for (i = 0; i < chkdef->ndisks; i++) {
> + virDomainCheckpointDiskDef *disk = &chkdef->disks[i];
> + const char *node;
> +
> + if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
> + continue;
> +
> + node = qemuBlockNodeLookup(vm, disk->name);
> + if (parentchk) {
> + arr = virJSONValueNewArray();
> + if (!arr ||
> + virJSONValueArrayAppendString(arr, disk->bitmap) < 0) {
> + success = false;
> + break;
> + }
> +
> + for (j = 0; j < parentdef->ndisks; j++) {
> + virDomainCheckpointDiskDef *disk2;
> +
> + disk2 = &parentdef->disks[j];
> + if (STRNEQ(disk->name, disk2->name))
> + continue;
> + if (chk == virDomainCheckpointGetCurrent(vm->checkpoints) &&
As outlined before in reviews to previous patches and also discussed on
IRC, the "current" checkpoint may not be the only one with active
bitmaps, so this might not work as expected.
> + qemuMonitorEnableBitmap(priv->mon, node,
> + disk2->bitmap) < 0) {
> + success = false;
> + break;
> + }
> + if (qemuMonitorMergeBitmaps(priv->mon, node,
> + disk2->bitmap, &arr) < 0) {
> + success = false;
> + break;
> + }
> + }
> + }
> + if (qemuMonitorDeleteBitmap(priv->mon, node, disk->bitmap) < 0) {
> + success = false;
> + break;
> + }
> + }
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || !success)
> + goto cleanup;
> + }
> +
> if (chk == virDomainCheckpointGetCurrent(vm->checkpoints)) {
> virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> - if (update_parent && chk->def->parent_name) {
> - parentchk = virDomainCheckpointFindByName(vm->checkpoints,
> - chk->def->parent_name);
> - if (!parentchk) {
> - VIR_WARN("missing parent checkpoint matching name '%s'",
> + if (update_parent && parentchk) {
> + virDomainCheckpointSetCurrent(vm->checkpoints, parentchk);
> + if (qemuDomainCheckpointWriteMetadata(vm, parentchk, driver->caps,
> + driver->xmlopt,
> + cfg->checkpointDir) < 0) {
> + VIR_WARN("failed to set parent checkpoint '%s' as current",
> chk->def->parent_name);
> - } else {
> - virDomainCheckpointSetCurrent(vm->checkpoints, parentchk);
> - if (qemuDomainCheckpointWriteMetadata(vm, parentchk, driver->caps,
> - driver->xmlopt,
> - cfg->checkpointDir) < 0) {
> - VIR_WARN("failed to set parent checkpoint '%s' as current",
> - chk->def->parent_name);
> - virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> - }
> + virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
> }
> }
> }
> @@ -9014,6 +9062,7 @@ qemuDomainCheckpointDiscard(virQEMUDriverPtr driver,
> cleanup:
> VIR_FREE(chkFile);
> virObjectUnref(cfg);
> + virJSONValueFree(arr);
> return ret;
> }
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 702a715902..b37307abc7 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -47,6 +47,7 @@
> #include "qemu_hostdev.h"
> #include "qemu_hotplug.h"
> #include "qemu_monitor.h"
> +#include "qemu_monitor_json.h"
I'd prefer if you not include this header directly here but I'm ashamed
of myself for including it in qemu_block.c
> #include "qemu_process.h"
> #include "qemu_migration.h"
> #include "qemu_migration_params.h"
> @@ -16994,6 +16995,53 @@ qemuDomainCheckpointPrepare(virQEMUDriverPtr driver, virCapsPtr caps,
> return ret;
> }
>
> +static int
> +qemuDomainCheckpointAddActions(virDomainObjPtr vm,
> + virJSONValuePtr actions,
> + virDomainMomentObjPtr old_current,
> + virDomainCheckpointDefPtr def)
> +{
> + size_t i, j;
> + int ret = -1;
> + virDomainCheckpointDefPtr olddef;
> +
> + olddef = virDomainCheckpointObjGetDef(old_current);
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainCheckpointDiskDef *disk = &def->disks[i];
> + const char *node;
> +
> + if (disk->type != VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP)
> + continue;
> + node = qemuBlockNodeLookup(vm, disk->name);
> + if (qemuMonitorJSONTransactionAdd(actions,
> + "block-dirty-bitmap-add",
> + "s:node", node,
> + "s:name", disk->bitmap,
> + "b:persistent", true,
> + NULL) < 0)
> + goto cleanup;
> + if (old_current) {
> + for (j = 0; j < olddef->ndisks; j++) {
> + virDomainCheckpointDiskDef *disk2;
> +
> + disk2 = &olddef->disks[j];
> + if (STRNEQ(disk->name, disk2->name))
> + continue;
> + if (disk2->type == VIR_DOMAIN_CHECKPOINT_TYPE_BITMAP &&
> + qemuMonitorJSONTransactionAdd(actions,
> + "block-dirty-bitmap-disable",
As we've discussed on IRC I think that this is an unnecessary
complication of things while we don't really know what the implications
of handling bitmaps accross multiple backing files are.
As long as you are okay to work around the implications and complex
lookup of the correct bitmap I don't mind adding it this way though.
> + "s:node", node,
> + "s:name", disk2->bitmap,
> + NULL) < 0)
> + goto cleanup;
> + }
> + }
> + }
> + ret = 0;
> +
> + cleanup:
> + return ret;
> +}
>
> static virDomainCheckpointPtr
> qemuDomainCheckpointCreateXML(virDomainPtr domain,
[...]
> @@ -17077,10 +17135,10 @@ qemuDomainCheckpointCreateXML(virDomainPtr domain,
> def = NULL;
> }
>
> - current = virDomainCheckpointGetCurrent(vm->checkpoints);
> - if (current) {
> + other = virDomainCheckpointGetCurrent(vm->checkpoints);
> + if (other) {
> if (!redefine &&
> - VIR_STRDUP(chk->def->parent_name, current->def->name) < 0)
> + VIR_STRDUP(chk->def->parent_name, other->def->name) < 0)
> goto endjob;
> if (update_current) {
> virDomainCheckpointSetCurrent(vm->checkpoints, NULL);
Does this hunk belong to the previous patch?
[...]
> @@ -17328,6 +17396,22 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
> if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> goto cleanup;
>
> + priv = vm->privateData;
> + if (!metadata_only) {
> + /* Until qemu-img supports offline bitmap deletion, we are stuck
> + * with requiring a running guest */
> + if (!virDomainObjIsActive(vm)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("cannot delete checkpoint for inactive domain"));
> + goto endjob;
> + }
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BITMAP_MERGE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("qemu binary lacks persistent bitmaps support"));
> + goto endjob;
> + }
> + }
> +
> if (!(chk = qemuCheckObjFromCheckpoint(vm, checkpoint)))
> goto endjob;
The rest looks good, but here this does too much with the "current"
snapshot which in my opinion will not work properly, thus I'd like to
see a fixed version of this.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190708/6b6037ff/attachment-0001.sig>
More information about the libvir-list
mailing list