Re: [libvirt] [PATCH v3 5/6] qemuMigrationDriveMirror: utilize blockJob condition
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(a)redhat.com>
---
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