Re: [libvirt] [PATCH 12/13] backup: Wire up qemu checkpoint commands over QMP

On Tue, Jun 18, 2019 at 22:47:53 -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.

At this point I'm lacking the interlocking with snapshots. It may be posted as a separate patch, but none of this code should go in unless that one is ACK'd.

Signed-off-by: Eric Blake <eblake@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 e128dc169b..1364227e42 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -8809,45 +8809,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;

I was complaining about this in the previous version.

+ 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) && + qemuMonitorEnableBitmap(priv->mon, node, + disk2->bitmap) < 0) { + success = false; + break; + } + if (qemuMonitorMergeBitmaps(priv->mon, node, + disk2->bitmap, &arr) < 0) {

We definitely need interlocking as this will not work across the backing chain as it seems to reference the 'node'

+ 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); } } }

[...]

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5fa319b656..0cdb2dd1e2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c

[...]

@@ -17016,6 +17017,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", + "s:node", node, + "s:name", disk2->bitmap, + NULL) < 0) + goto cleanup;

optimization: break;

+ } + } + } + ret = 0; + + cleanup:

pointless label

+ return ret; +}

static virDomainCheckpointPtr qemuDomainCheckpointCreateXML(virDomainPtr domain,

[...]

@@ -17094,10 +17152,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)

Looks like it's refactoring code from previous patch.

goto endjob; if (update_current) { virDomainCheckpointSetCurrent(vm->checkpoints, NULL);

[...]