[libvirt] [PATCH v3 5/6] qemuMigrationDriveMirror: utilize blockJob condition

[Jiri Denemark]

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 at 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