Re: [libvirt] [PATCH v3] qemu: wait for SPICE to migrate

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?