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@xxxxxxxxxx> > --- > 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; > + }
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list