On 25.09.2012 19:57, Daniel P. Berrange wrote:
> On Thu, Sep 20, 2012 at 01:29:21PM +0200, Michal Privoznik wrote:
>> Recently, there have been some improvements made to qemu so it
>> supports seamless migration or something very close to it.
>> However, it requires libvirt interaction. Once qemu is migrated,
>> the SPICE server needs to send its internal state to the destination.
>> Once it's done, it fires SPICE_MIGRATE_COMPLETED event and this
>> fact is advertised in 'query-spice' output as well.
>> We must not kill qemu until SPICE server finishes the transfer.
>> ---
>> src/qemu/qemu_capabilities.c | 4 +++
>> src/qemu/qemu_capabilities.h | 1 +
>> src/qemu/qemu_command.c | 8 ++++++
>> src/qemu/qemu_migration.c | 33 ++++++++++++++++++++++++--
>> src/qemu/qemu_monitor.c | 24 +++++++++++++++++++
>> src/qemu/qemu_monitor.h | 2 +
>> src/qemu/qemu_monitor_json.c | 52 ++++++++++++++++++++++++++++++++++++++++++
>> src/qemu/qemu_monitor_json.h | 3 ++
>> 8 files changed, 124 insertions(+), 3 deletions(-)
>>
>> @@ -940,6 +949,8 @@ qemuMigrationUpdateJobStatus(struct qemud_driver *driver,
>>
>> case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED:
>> priv->job.info.type = VIR_DOMAIN_JOB_COMPLETED;
>> + if (spice_migrated)
>> + priv->job.info.type = VIR_DOMAIN_JOB_COMPLETED;
>
> This addition doesn't do anything, since the previous line
> has already set the same value.
Yeah, there should be '-' sign on the previous line...
>
>
> I don't find this very readable - it is better to do it as a
> regular if() IMHO. eg
>
> bool wait_for_spice = false;
>
> if (...)
> wait_for_spice = true;
>
>
>>
>> switch (priv->job.asyncJob) {
>> case QEMU_ASYNC_JOB_MIGRATION_OUT:
>> @@ -988,7 +1006,8 @@ qemuMigrationWaitForCompletion(struct qemud_driver *driver, virDomainObjPtr vm,
>> /* Poll every 50ms for progress & to allow cancellation */
>> struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull };
>>
>> - if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) < 0)
>> + if (qemuMigrationUpdateJobStatus(driver, vm, job,
>> + asyncJob, wait_for_spice) < 0)
>> goto cleanup;
>>
>> if (dconn && virConnectIsAlive(dconn) <= 0) {
>> @@ -1840,6 +1859,7 @@ qemuMigrationRun(struct qemud_driver *driver,
>> int fd = -1;
>> unsigned long migrate_speed = resource ? resource : priv->migMaxBandwidth;
>> virErrorPtr orig_err = NULL;
>> + bool wait_for_spice;
>>
>> VIR_DEBUG("driver=%p, vm=%p, cookiein=%s, cookieinlen=%d, "
>> "cookieout=%p, cookieoutlen=%p, flags=%lx, resource=%lu, "
>> @@ -1848,6 +1868,12 @@ qemuMigrationRun(struct qemud_driver *driver,
>> cookieout, cookieoutlen, flags, resource,
>> spec, spec->destType, spec->fwdType);
>>
>> + /* If guest uses SPICE and supports seamless migration we have to hold up
>> + * migration finish until SPICE server transfers its data */
>> + wait_for_spice = (vm->def->ngraphics == 1) &&
>> + (vm->def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) &&
>> + qemuCapsGet(priv->caps, QEMU_CAPS_SEAMLESS_MIGRATION);
>
> Same comment as before.
>
>
> Since both callers of qemuMigrateUpdateJobStatus() have to repeat
> this logic, why not push this down into that method itself and
> make 'wait_for_spice' be a local variable there instead of a param.
Well, I wanted to optimize here since qemuMigrateUpdateJobStatus is
called every 50 ms so I think accessing memory on each run would hurt
the performance (and this really do a lot of mem traffic since it access
vm->def->[n]graphics; qemuCaps). Or is this a premature optimization
which should be left to compiler and it's caching capabilities?
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list