On Fri, Feb 13, 2015 at 16:24:32 +0100, Michal Privoznik wrote:
> Instead of unlocking and locking the domain object every 50ms
> lets just wait on blockJob condition and run the loop body if and
> BLOCK_JOB even occurred.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/qemu/qemu_migration.c | 15 +++++----------
> src/qemu/qemu_process.c | 4 ++++
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 14a4ec6..998e8f5 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1768,9 +1768,6 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>
> /* wait for completion */
> while (true) {
> - /* Poll every 500ms for progress & to allow cancellation */
> - struct timespec ts = { .tv_sec = 0, .tv_nsec = 500 * 1000 * 1000ull };
> -
> /* Explicitly check if domain is still alive. Maybe qemu
> * died meanwhile so we won't see any event at all. */
> if (!virDomainObjIsActive(vm)) {
> @@ -1800,13 +1797,11 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
> goto error;
> }
>
> - /* XXX Turn this into virCond someday. */
> -
> - virObjectUnlock(vm);
> -
> - nanosleep(&ts, NULL);
> -
> - virObjectLock(vm);
> + if (virCondWait(&priv->blockJob, &vm->parent.lock) < 0) {
Hmm, you'd also need to signal this condition on EOF, otherwise this
could be waiting forever when the domain dies during drive mirror job.
But see below...
> + virReportSystemError(errno, "%s",
> + _("Unable to wait on blockJob condition"));
> + goto error;
> + }
> }
> }
>
The main problem with this patch is hidden in the (missing) context:
if (priv->job.asyncAbort) {
priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
_("canceled by client"));
goto error;
}
The loop would never break when priv->job.asyncAbort is set because it
would still be waiting for priv->blockJob to be signalled. So we need to
somehow turn this into a condition too. And because there's no way we
could wait for more conditions at once, we'll need to think more what to
do. One possibility is to create a general jobCond which would be
signalled in several places: when asyncAbort is set, when block job
changes its state, on monitor EOF, ...
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list