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

[Peter Krempa]

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 at redhat.com>
> ---
>  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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140904/0002589e/attachment-0001.sig>