[libvirt] [PATCH v9 08/10] backup: Wire up qemu full pull backup commands over QMP
Peter Krempa
pkrempa at redhat.com
Tue Jul 9 16:25:33 UTC 2019
On Mon, Jul 08, 2019 at 11:55:51 -0500, Eric Blake wrote:
> Time to actually issue the QMP transactions that start and
> stop backup commands (for now, just pull mode, not push).
> Starting a job has to kick off several pre-req steps, then
> a transaction, and additionally spawn an NBD server for pull
> mode; ending a job as well as failing partway through
> beginning a job has to unwind the earlier steps. Implementing
> push mode, as well as incremental pull and checkpoint creation,
> is deferred to later patches.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> src/qemu/qemu_domain.c | 18 +-
> src/qemu/qemu_driver.c | 310 ++++++++++++++++++++++++++++++++++-
> src/qemu/qemu_monitor_json.c | 4 +
> src/qemu/qemu_process.c | 9 +
> 4 files changed, 327 insertions(+), 14 deletions(-)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 64c15d8ae6..70fe250880 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2676,17 +2676,25 @@ qemuDomainObjPrivateXMLParseBlockjobs(virQEMUDriverPtr driver,
> {
> xmlNodePtr node;
> int tmp;
> + size_t i;
> VIR_AUTOFREE(char *) active = NULL;
>
> if ((active = virXPathString("string(./blockjobs/@active)", ctxt)) &&
> (tmp = virTristateBoolTypeFromString(active)) > 0)
> priv->reconnectBlockjobs = tmp;
>
> - if ((node = virXPathNode("./domainbackup", ctxt)) &&
> - !(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
> - driver->xmlopt,
> - VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
> - return -1;
> + if ((node = virXPathNode("./domainbackup", ctxt))) {
> + if (!(priv->backup = virDomainBackupDefParseNode(ctxt->doc, node,
> + driver->xmlopt,
> + VIR_DOMAIN_BACKUP_PARSE_INTERNAL)))
> + return -1;
> + /* The backup job is only stored in XML if backupBegin
> + * succeeded at exporting the disk, so no need to store disk
> + * state when we can just force-reset it to a known-good
> + * value. */
> + for (i = 0; i < priv->backup->ndisks; i++)
> + priv->backup->disks[i].state = VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT;
Even for push mode?
> + }
>
> return 0;
> }
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index ec5f37fe94..d9abcfa4c8 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -17555,8 +17555,80 @@ qemuDomainCheckpointDelete(virDomainCheckpointPtr checkpoint,
> return ret;
> }
>
> -static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> - const char *checkpointXml, unsigned int flags)
> +static int
> +qemuDomainBackupPrepare(virQEMUDriverPtr driver, virDomainObjPtr vm,
> + virDomainBackupDefPtr def)
> +{
> + int ret = -1;
> + size_t i;
> +
> + if (qemuBlockNodeNamesDetect(driver, vm, QEMU_ASYNC_JOB_NONE) < 0)
See previous reviews.
> + goto cleanup;
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainBackupDiskDef *disk = &def->disks[i];
> + virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> +
> + if (!disk->store)
> + continue;
> + if (virAsprintf(&disk->store->nodeformat, "tmp-%s", disk->name) < 0)
> + goto cleanup;
> + if (!disk->store->format)
> + disk->store->format = VIR_STORAGE_FILE_QCOW2;
> + if (def->incremental) {
> + if (src->format != VIR_STORAGE_FILE_QCOW2) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
> + _("incremental backup of %s requires qcow2"),
> + disk->name);
> + goto cleanup;
> + }
> + }
> + }
> + ret = 0;
> + cleanup:
> + return ret;
> +}
> +
> +/* Called while monitor lock is held. Best-effort cleanup. */
> +static int
> +qemuDomainBackupDiskCleanup(virQEMUDriverPtr driver, virDomainObjPtr vm,
> + virDomainBackupDiskDef *disk, bool incremental)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + const char *node = vm->def->disks[disk->idx]->src->nodeformat;
> + int ret = 0;
> +
> + if (!disk->store)
> + return 0;
> + if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_EXPORT) {
> + /* No real need to use nbd-server-remove, since we will
> + * shortly be calling nbd-server-stop. */
> + }
> + if (incremental && disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_BITMAP &&
> + qemuMonitorDeleteBitmap(priv->mon, node, disk->store->nodeformat) < 0) {
> + VIR_WARN("Unable to remove temp bitmap for disk %s after backup",
> + disk->name);
> + ret = -1;
> + }
> + if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_READY &&
> + qemuMonitorBlockdevDel(priv->mon, disk->store->nodeformat) < 0) {
> + VIR_WARN("Unable to remove temp disk %s after backup",
> + disk->name);
> + ret = -1;
> + }
> + if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_LABEL)
> + qemuDomainStorageSourceAccessRevoke(driver, vm, disk->store);
You must not call this with @vm unlocked.
> + if (disk->state >= VIR_DOMAIN_BACKUP_DISK_STATE_CREATED &&
> + disk->store->detected && unlink(disk->store->path) < 0) {
> + VIR_WARN("Unable to unlink temp disk %s after backup",
> + disk->store->path);
> + ret = -1;
> + }
> + return ret;
> +}
> +
> +static int
> +qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> + const char *checkpointXml, unsigned int flags)
> {
> virQEMUDriverPtr driver = domain->conn->privateData;
> virDomainObjPtr vm = NULL;
[....]
> @@ -17609,25 +17688,145 @@ static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
[...]
> - if (virDomainBackupAlignDisks(def, vm->def, suffix) < 0)
> + if (virDomainBackupAlignDisks(def, vm->def, suffix) < 0 ||
> + qemuDomainBackupPrepare(driver, vm, def) < 0)
> goto endjob;
>
> /* actually start the checkpoint. 2x2 array of push/pull, full/incr,
> plus additional tweak if checkpoint requested */
> - /* TODO: issue QMP commands:
> - - pull: nbd-server-start with <server> from user (or autogenerate server)
> - - push/pull: blockdev-add per <disk>
> + qemuDomainObjEnterMonitor(driver, vm);
> + /* - push/pull: blockdev-add per <disk> */
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainBackupDiskDef *disk = &def->disks[i];
> + virJSONValuePtr file;
> + virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> + const char *node = src->nodeformat;
> +
> + if (!disk->store)
> + continue;
> + if (qemuDomainStorageFileInit(driver, vm, disk->store, src) < 0)
This takes @vm which is unlocked
> + goto endmon;
> + if (disk->store->detected) {
> + if (virStorageFileCreate(disk->store) < 0) {
> + virReportSystemError(errno,
> + _("failed to create image file '%s'"),
> + NULLSTR(disk->store->path));
> + goto endmon;
> + }
> + disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_CREATED;
> + }
> + if (qemuDomainStorageSourceAccessAllow(driver, vm, disk->store, false,
Also this requires @vm locked
> + true) < 0)
> + goto endmon;
> + disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_LABEL;
> + if (disk->store->detected) {
> + virBuffer buf = VIR_BUFFER_INITIALIZER;
> +
> + /* Force initialization of scratch/target file to new qcow2 */
> + if (!(cmd = virCommandNewArgList(qemuImgPath,
> + "create",
> + "-f",
> + virStorageFileFormatTypeToString(disk->store->format),
> + "-o",
> + NULL)))
> + goto endmon;
AFAIK if we hold the monitor lock too much (and this qualifies as a
long-running operation) without calling actual monitor APIs the event
loop will get stuck.
> + virBufferAsprintf(&buf, "backing_fmt=%s,backing_file=",
> + virStorageFileFormatTypeToString(src->format));
> + virQEMUBuildBufferEscapeComma(&buf, src->path);
> + virCommandAddArgBuffer(cmd, &buf);
> +
> + virQEMUBuildBufferEscapeComma(&buf, disk->store->path);
> + virCommandAddArgBuffer(cmd, &buf);
> + if (virCommandRun(cmd, NULL) < 0)
> + goto endmon;
> + virCommandFree(cmd);
> + cmd = NULL;
> + }
> +
> + if (virJSONValueObjectCreate(&file,
> + "s:driver", "file",
> + "s:filename", disk->store->path,
> + NULL) < 0)
> + goto endmon;
> + if (virJSONValueObjectCreate(&json,
> + "s:driver", virStorageFileFormatTypeToString(disk->store->format),
> + "s:node-name", disk->store->nodeformat,
> + "a:file", &file,
> + "s:backing", node, NULL) < 0) {
Please use the designated blockdev-add JSON props generators. Also
libvirt opted to open the 'file' separately with blockdev add so this
should not do things differently.
> + virJSONValueFree(file);
> + goto endmon;
> + }
> + if (qemuMonitorBlockdevAdd(priv->mon, json) < 0)
> + goto endmon;
> + json = NULL;
> + disk->state = VIR_DOMAIN_BACKUP_DISK_STATE_READY;
> + }
> +
> + /* TODO:
> - incr: bitmap-add of tmp, bitmap-merge per <disk>
> - transaction, containing:
> - push+full: blockdev-backup sync:full
> @@ -17635,8 +17834,76 @@ static int qemuDomainBackupBegin(virDomainPtr domain, const char *diskXml,
> - pull+full: blockdev-backup sync:none
> - pull+incr: blockdev-backup sync:none, bitmap-disable of tmp
> - if checkpoint: bitmap-disable of old, bitmap-add of new
> + */
> + if (!(json = virJSONValueNewArray()))
> + goto endmon;
> + for (i = 0; i < def->ndisks; i++) {
> + virDomainBackupDiskDef *disk = &def->disks[i];
> + virStorageSourcePtr src = vm->def->disks[disk->idx]->src;
> +
> + if (!disk->store)
> + continue;
> + if (qemuMonitorJSONTransactionAdd(json,
> + "blockdev-backup",
> + "s:device", src->nodeformat,
Also since this does not seem to register the blockjob with the disk you
don't get the interlocking with other blockjobs
> + "s:target", disk->store->nodeformat,
> + "s:sync", "none",
> + "s:job-id", disk->name,
This should be prefixed with job type.
> + NULL) < 0)
> + goto endmon;
[...]
> @@ -17719,9 +17990,27 @@ static int qemuDomainBackupEnd(virDomainPtr domain, int id, unsigned int flags)
>
> if (priv->backup->type != VIR_DOMAIN_BACKUP_TYPE_PUSH)
> want_abort = false;
> + def = priv->backup;
> +
> + /* We are going to modify the domain below. */
> + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0)
> + goto cleanup;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + if (def->type == VIR_DOMAIN_BACKUP_TYPE_PULL)
> + ret = qemuMonitorNBDServerStop(priv->mon);
> + for (i = 0; i < def->ndisks; i++) {
> + if (qemuMonitorBlockJobCancel(priv->mon,
If not all disks are part of the job def->disk[i].name might point to a
disk which is not part of the backup and this would then attempt to
cancel a job of that disk.
> + def->disks[i].name) < 0 ||
> + qemuDomainBackupDiskCleanup(driver, vm, &def->disks[i],
> + !!def->incremental) < 0)
> + ret = -1;
> + }
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
> + ret = -1;
> + goto endjob;
> + }
-------------- 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/20190709/68f0dd33/attachment-0001.sig>
More information about the libvir-list
mailing list