On 08/31/14 06:02, Eric Blake wrote:
> 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@xxxxxxxxxx>
> ---
> 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;
Atomicity is guaranteed by holding the domain lock here. We should
document that the mirrorState field can change even when the _MODIFY job
is held though.
Otherwise I don't see a problem currently with this as long as it is
stated somewhere. Possibly even in a comment here.
> + 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
ACK
Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list