[libvirt] [PATCH] qemu: allow blkstat/blkinfo calls during migration

[Eric Blake]

On 05/11/2011 07:26 AM, Federico Simoncelli wrote:
> Originally most of libvirt domain-specific calls were blocking
> during a migration.
> A new mechanism to allow specific calls (blkstat/blkinfo) to be
> executed in such condition has been implemented.
> In the long term it'd be desirable to get a more general
> solution to mark further APIs as migration safe, without needing
> special case code.
> 
>  * src/qemu/qemu_migration.c: add some additional job signal
>    flags for doing blkstat/blkinfo during a migration
>  * src/qemu/qemu_domain.c: add a condition variable that can be
>    used to efficiently wait for the migration code to clear the
>    signal flag
>  * src/qemu/qemu_driver.c: execute blkstat/blkinfo using the
>    job signal flags during migration
> ---
>  src/qemu/qemu_domain.c    |    6 +++
>  src/qemu/qemu_domain.h    |    9 ++++
>  src/qemu/qemu_driver.c    |  103 ++++++++++++++++++++++++++++++++-------------
>  src/qemu/qemu_migration.c |   35 +++++++++++++++

This conflicts with patch 10/16 in Dan's migration patches [1].  I'll
hold off pushing it until Dan's patches are in, and hopefully the rebase
is not too difficult.  You could try the rebase now on his preview
repository [2]

[1] https://www.redhat.com/archives/libvir-list/2011-May/msg00771.html
[2] http://gitorious.org/~berrange/libvirt/staging/commits/migrate-locking-3

That said, though, it looks like you addressed my findings from v1.
Unfortunately, I still see a couple of places for improvement.

> +++ b/src/qemu/qemu_domain.c
> @@ -110,6 +110,11 @@ static void *qemuDomainObjPrivateAlloc(void)
>      if (VIR_ALLOC(priv) < 0)
>          return NULL;
>  
> +    if (virCondInit(&priv->signalCond) < 0) {
> +        VIR_FREE(priv);
> +        return NULL;

As long as we're touching this function, let's also fix the bug where
priv->jobCond was never initialized.

> +    if ((priv->jobActive == QEMU_JOB_MIGRATION_OUT)
> +        || (priv->jobActive == QEMU_JOB_SAVE)) {
> +        virDomainObjRef(vm);
> +        while (priv->jobSignals & QEMU_JOB_SIGNAL_BLKSTAT)
> +            ignore_value(virCondWait(&priv->signalCond, &vm->lock));

Hmm, should we mark priv->jobSignals as volatile in the header, to
ensure the compiler won't optimize this into an infinite loop?

> +++ b/src/qemu/qemu_migration.c
> @@ -158,6 +158,38 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm)
>                  VIR_WARN0("Unable to set migration speed");
>          }

The earlier 'else if' chain should likewise be broken into consecutive
'if's.  In Dan's patch, this moved into the new method
qemuMigrationProcessJobSignals

>  
> +        if (priv->jobSignals & QEMU_JOB_SIGNAL_BLKSTAT) {
> +            qemuDomainObjEnterMonitorWithDriver(driver, vm);
> +            rc = qemuMonitorGetBlockStatsInfo(priv->mon,

I'm still wondering if we need to hold the signalLock condition during
the duration where we drop driver lock to call out to the monitor.  I
can't convince myself that we need to, but I also can't convince myself
that your code is safe without it (I guess it goes to show that I
haven't done much programming on condition variables in any prior job -
they're cool tools, but hard to wrap your head around when first
learning them).

> +                                  priv->jobSignalsData.statDevName,
> +                                  &priv->jobSignalsData.blockStat->rd_req,
> +                                  &priv->jobSignalsData.blockStat->rd_bytes,
> +                                  &priv->jobSignalsData.blockStat->wr_req,
> +                                  &priv->jobSignalsData.blockStat->wr_bytes,
> +                                  &priv->jobSignalsData.blockStat->errs);
> +            qemuDomainObjExitMonitorWithDriver(driver, vm);
> +
> +            priv->jobSignalsData.statRetCode = rc;
> +            priv->jobSignals ^= QEMU_JOB_SIGNAL_BLKSTAT;
> +
> +            if (rc < 0)
> +                VIR_WARN0("Unable to get block statistics");
> +        }
> +
> +        if (priv->jobSignals & QEMU_JOB_SIGNAL_BLKINFO) {

Oh dear, I just realized a bug.  By breaking 'else if' into separate
'if', we now need to check if the VM is alive after every time we regain
the driver lock (that is, after every qemuDomainObjExitMonitorWithDriver).

Hmm, maybe the better approach is to keep things as an 'else if' chain,
but to make qemuMigrationProcessJobSignals be a while loop that iterates
until all bits are serviced (so that the live vm check is factored to
only appear once in the loop body).  Back when these checks were part of
the larger qemuMigrationWaitForCompletion function, the overall loop
also included a sleep, which is what I didn't like; but with Dan's patch
in place, a loop with no sleep seems reasonable.

> +            qemuDomainObjEnterMonitorWithDriver(driver, vm);
> +            rc = qemuMonitorGetBlockExtent(priv->mon,
> +                               priv->jobSignalsData.infoDevName,
> +                               &priv->jobSignalsData.blockInfo->allocation);
> +            qemuDomainObjExitMonitorWithDriver(driver, vm);
> +
> +            priv->jobSignalsData.infoRetCode = rc;
> +            priv->jobSignals ^= QEMU_JOB_SIGNAL_BLKINFO;
> +
> +            if (rc < 0)
> +                VIR_WARN0("Unable to get block information");

Hmm, your rebase still hasn't picked up the latest patches; you need to
s/VIR_WARN0/VIR_WARN/ per commit b65f37a.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

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