Re: [libvirt] [PATCH v3 03/18] blockjob: split out block info driver handling

The qemu implementation for virDomainGetBlockJobInfo() has a minor bug: it grabs the qemu job with intent to QEMU_JOB_MODIFY, which means it cannot be run in parallel with any other domain-modifying command. Among others, virDomainBlockJobAbort() is such a modifying command, and it defaults to being synchronous, and can wait as long as several seconds to ensure that the job has actually finished. Due to the job rules, this means a user cannot obtain status about the job during that timeframe, even though we know management code is using a polling loop on status to see when a job finishes. This bug has been present ever since blockpull support was first introduced (commit b976165, v0.9.4 in Jul 2011), all because we stupidly tried to cram too much multiplexing through a single helper routine. It's time to disentangle some of that mess, and in the process relax block job query to use QEMU_JOB_QUERY, since it can safely be used in parallel with any long running modify command. Technically, there is one case where getting block job info can modify domain XML - we do snooping to see if a 2-phase job has transitioned into the second phase, for an optimization in the case of old qemu that lacked an event for the transition. But I think that optimization is safe; and if it proves to be a problem, we could use the difference between QEMU_CAPS_BLOCKJOB_{ASYNC,SYNC} to determine whether we even need snooping, and request a modifying job in that case. * src/qemu/qemu_driver.c (qemuDomainBlockJobImpl): Move info handling... (qemuDomainGetBlockJobInfo): ...here, and relax job type. (qemuDomainBlockJobAbort, qemuDomainBlockJobSetSpeed) (qemuDomainBlockRebase, qemuDomainBlockPull): Adjust callers. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- src/qemu/qemu_driver.c | 86 ++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 66 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 177d3e4..bedccc6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c

@@ -15258,8 +15254,58 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, return -1; } - return qemuDomainBlockJobImpl(vm, dom->conn, path, NULL, 0, info, BLOCK_JOB_INFO, - flags); + priv = vm->privateData; + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_ASYNC) && + !virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_BLOCKJOB_SYNC)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("block jobs not supported with this QEMU binary")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + goto cleanup; + + if (!virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + device = qemuDiskPathToAlias(vm, path, &idx); + if (!device) + goto endjob; + disk = vm->def->disks[idx]; + + qemuDomainObjEnterMonitor(driver, vm); + ret = qemuMonitorBlockJob(priv->mon, device, NULL, NULL, 0, + info, BLOCK_JOB_INFO, true); + qemuDomainObjExitMonitor(driver, vm); + if (ret < 0) + goto endjob; + + if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && + disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) + info->type = disk->mirrorJob; + + /* Snoop block copy operations, so future cancel operations can + * avoid checking if pivot is safe. Save the change to XML, but + * we can ignore failure because it is only an optimization. */ + if (ret == 1 && disk->mirror && + info->cur == info->end && !disk->mirrorState) { + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;

+ ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm)); + virObjectUnref(cfg); + } + endjob: + if (!qemuDomainObjEndJob(driver, vm)) + vm = NULL; + + cleanup: + if (vm) + virObjectUnlock(vm); + return ret; } static int