On 05/13/2011 04:11 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.
Actually, we now need a v4, although I'll try to post one shortly.
> @@ -123,6 +135,7 @@ static void qemuDomainObjPrivateFree(void *data)
> virDomainChrSourceDefFree(priv->monConfig);
> VIR_FREE(priv->vcpupids);
> VIR_FREE(priv->lockState);
> + ignore_value(virCondDestroy(&priv->signalCond));
>
We should also add a virCondDestroy for priv->jobCond.
> + 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));
> +
> + priv->jobSignals |= QEMU_JOB_SIGNAL_BLKSTAT;
> + priv->jobSignalsData.statDevName = disk->info.alias;
> + priv->jobSignalsData.blockStat = stats;
> + priv->jobSignalsData.statRetCode = -1;
> +
For safety sake, I'd rather see the jobSignals assignment after the
jobSignalsData assignments. I'm not sure if we need some sort of memory
barrier to ensure that a compiler (and/or hardware) won't rearrange the
assignments to complete out of order, but we really don't want the
migration thread to see the jobSignalsData contents until after they are
stable.
> @@ -796,8 +824,12 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm)
> job = _("job");
> }
>
> - if (qemuMigrationProcessJobSignals(driver, vm, job) < 0)
> - goto cleanup;
> + while (priv->jobSignals) {
> + if (qemuMigrationProcessJobSignals(driver, vm, job) < 0)
> + goto cleanup;
> + }
This while loop continues until priv->jobSignals is 0 _or_ an error
occurs...
> +
> + virCondSignal(&priv->signalCond);
>
> if (qemuMigrationUpdateJobStatus(driver, vm, job) < 0)
> goto cleanup;
> @@ -813,6 +845,8 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm)
> }
>
> cleanup:
> + virCondBroadcast(&priv->signalCond);
> +
...therefore, on error, this can raise the condition variable while
jobSignals is non-zero. We've just introduced a deadlock:
thread 1 starts a migration
thread 2 queries block info, sets the jobSignals bit, and waits for the
cond variable and the bit to be clear
something goes wrong with migration (suppose qemu is killed externally)
thread 1 finally gets to qemuMigrationUpdateJobStatus, but sees that vm
is no longer live so it exits with error
thread 2 is now stuck waiting for the jobSignals to clear, but thread 1
is no longer going to clear it
I'm still working on the right way to fix it, but I think that
qemuMigrationProcessJobSignals needs a bool parameter that says whether
it is in cleanup mode, in which case it always clears at least one
jobSignals bit even on error, and for blkinfo/blkstat, it sets the ret
value to -1 before clearing the bit. That way,
qemuMigrationWaitForCompletion always ends with jobSignals == 0 and
driver lock held, and as long as the driver lock is then not released
until jobActive has been reset, then no new qemudDomainBlockStats will
start, and the existing one in thread 2 will correctly fail for the same
reason that migration in thread 1 failed (that is, the vm went away early).
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list