On Tue, Jun 09, 2015 at 16:56:34 +0200, Peter Krempa wrote: Yeah, although I will also rename "ret" to "status" since the name implicitly suggests semantics of -1... anyone seeing ret = -1 would consider it a failure. But this function does not fail, it just returns the original value stored in blockJobStatus. Jirka

On Wed, Jun 10, 2015 at 07:21:27 -0400, John Ferlan wrote: ... ... qemuBlockJobUpdate returns the original value of diskPriv->blockJobStatus, which is already stored to *ret_status above. Not to mention that one of the following patches will completely remove ret_status from qemuBlockJobSyncEnd. Jirka

Because we are polling we may detect some errors after we asked QEMU for migration status even though they occurred before. If this happens and QEMU reports migration completed successfully, we would happily report the migration succeeded even though we should have cancelled it because of the other error. In practise it is not a big issue now but it will become a much bigger issue once the check for storage migration status is moved inside the loop in qemuMigrationWaitForCompletion. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - really do what commit message describes Already ACKed in version 1 :-) src/qemu/qemu_migration.c | 48 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f7432e8..93e29e7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2442,6 +2442,7 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, qemuDomainJobInfoPtr jobInfo = priv->job.current; const char *job; int pauseReason; + int ret = -1; switch (priv->job.asyncJob) { case QEMU_ASYNC_JOB_MIGRATION_OUT: @@ -2459,12 +2460,12 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED; - while (1) { + while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* 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) == -1) - break; + goto error; /* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && @@ -2472,40 +2473,37 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), job, _("failed due to I/O error")); - break; + goto error; } if (dconn && virConnectIsAlive(dconn) <= 0) { virReportError(VIR_ERR_OPERATION_FAILED, "%s", _("Lost connection to destination host")); - break; + goto error; } - if (jobInfo->type != VIR_DOMAIN_JOB_UNBOUNDED) - break; - - virObjectUnlock(vm); - - nanosleep(&ts, NULL); - - virObjectLock(vm); + if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { + virObjectUnlock(vm); + nanosleep(&ts, NULL); + virObjectLock(vm); + } } - if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) { - qemuDomainJobInfoUpdateDowntime(jobInfo); - VIR_FREE(priv->job.completed); - if (VIR_ALLOC(priv->job.completed) == 0) - *priv->job.completed = *jobInfo; - return 0; - } else if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { - /* The migration was aborted by us rather than QEMU itself so let's - * update the job type and notify the caller to send migrate_cancel. - */ + qemuDomainJobInfoUpdateDowntime(jobInfo); + VIR_FREE(priv->job.completed); + if (VIR_ALLOC(priv->job.completed) == 0) + *priv->job.completed = *jobInfo; + return 0; + + error: + /* Check if the migration was aborted by us rather than QEMU itself. */ + if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED || + jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) { + if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) + ret = -2; jobInfo->type = VIR_DOMAIN_JOB_FAILED; - return -2; - } else { - return -1; } + return ret; } -- 2.4.1

By switching block jobs to use domain conditions, we can drop some pretty complicated code in NBD storage migration. Moreover, we are getting ready for migration events (to replace our 50ms polling on query-migrate). The ultimate goal is to have a single loop waiting (virDomainObjWait) for any migration related event (changed status of a migration, disk mirror events, internal abort requests, ...). This patch makes NBD storage migration ready for this: first we call a QMP command to start or cancel drive mirror on all disks we are interested in and then we wait for a single condition which is signaled on any event related to any of the mirrors. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - slightly modified to use domain conditions po/POTFILES.in | 1 - src/qemu/qemu_blockjob.c | 137 ++------------------- src/qemu/qemu_blockjob.h | 12 +- src/qemu/qemu_domain.c | 17 +-- src/qemu/qemu_domain.h | 1 - src/qemu/qemu_driver.c | 24 ++-- src/qemu/qemu_migration.c | 299 ++++++++++++++++++++++++++-------------------- src/qemu/qemu_process.c | 13 +- 8 files changed, 197 insertions(+), 307 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index bb0f6e1..dd06ab3 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -112,7 +112,6 @@ src/parallels/parallels_utils.h src/parallels/parallels_storage.c src/phyp/phyp_driver.c src/qemu/qemu_agent.c -src/qemu/qemu_blockjob.c src/qemu/qemu_capabilities.c src/qemu/qemu_cgroup.c src/qemu/qemu_command.c diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index 605c2a5..cefb168 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -204,19 +204,17 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, * * During a synchronous block job, a block job event for @disk * will not be processed asynchronously. Instead, it will be - * processed only when qemuBlockJobSyncWait* or - * qemuBlockJobSyncEnd is called. + * processed only when qemuBlockJobUpdate or qemuBlockJobSyncEnd + * is called. */ void qemuBlockJobSyncBegin(virDomainDiskDefPtr disk) { qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - if (diskPriv->blockJobSync) - VIR_WARN("Disk %s already has synchronous block job", - disk->dst); - + VIR_DEBUG("disk=%s", disk->dst); diskPriv->blockJobSync = true; + diskPriv->blockJobStatus = -1; } @@ -225,135 +223,16 @@ qemuBlockJobSyncBegin(virDomainDiskDefPtr disk) * @driver: qemu driver * @vm: domain * @disk: domain disk - * @ret_status: pointer to virConnectDomainEventBlockJobStatus * * End a synchronous block job for @disk. Any pending block job event - * for the disk is processed, and its status is recorded in the - * virConnectDomainEventBlockJobStatus field pointed to by - * @ret_status. + * for the disk is processed. */ void qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virConnectDomainEventBlockJobStatus *ret_status) + virDomainDiskDefPtr disk) { - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - - if (diskPriv->blockJobSync && diskPriv->blockJobStatus != -1) { - if (ret_status) - *ret_status = diskPriv->blockJobStatus; - qemuBlockJobUpdate(driver, vm, disk); - diskPriv->blockJobStatus = -1; - } - diskPriv->blockJobSync = false; -} - - -/** - * qemuBlockJobSyncWaitWithTimeout: - * @driver: qemu driver - * @vm: domain - * @disk: domain disk - * @timeout: timeout in milliseconds - * @ret_status: pointer to virConnectDomainEventBlockJobStatus - * - * Wait up to @timeout milliseconds for a block job event for @disk. - * If an event is received it is processed, and its status is recorded - * in the virConnectDomainEventBlockJobStatus field pointed to by - * @ret_status. - * - * If @timeout is not 0, @vm will be unlocked while waiting for the event. - * - * Returns 0 if an event was received or the timeout expired, - * -1 otherwise. - */ -int -qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - unsigned long long timeout, - virConnectDomainEventBlockJobStatus *ret_status) -{ - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - - if (!diskPriv->blockJobSync) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("No current synchronous block job")); - return -1; - } - - while (diskPriv->blockJobSync && diskPriv->blockJobStatus == -1) { - int r; - - if (!virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("guest unexpectedly quit")); - diskPriv->blockJobSync = false; - return -1; - } - - if (timeout == (unsigned long long)-1) { - r = virCondWait(&diskPriv->blockJobSyncCond, &vm->parent.lock); - } else if (timeout) { - unsigned long long now; - if (virTimeMillisNow(&now) < 0) { - virReportSystemError(errno, "%s", - _("Unable to get current time")); - return -1; - } - r = virCondWaitUntil(&diskPriv->blockJobSyncCond, - &vm->parent.lock, - now + timeout); - if (r < 0 && errno == ETIMEDOUT) - return 0; - } else { - errno = ETIMEDOUT; - return 0; - } - - if (r < 0) { - diskPriv->blockJobSync = false; - virReportSystemError(errno, "%s", - _("Unable to wait on block job sync " - "condition")); - return -1; - } - } - - if (ret_status) - *ret_status = diskPriv->blockJobStatus; + VIR_DEBUG("disk=%s", disk->dst); qemuBlockJobUpdate(driver, vm, disk); - diskPriv->blockJobStatus = -1; - - return 0; -} - - -/** - * qemuBlockJobSyncWait: - * @driver: qemu driver - * @vm: domain - * @disk: domain disk - * @ret_status: pointer to virConnectDomainEventBlockJobStatus - * - * Wait for a block job event for @disk. If an event is received it - * is processed, and its status is recorded in the - * virConnectDomainEventBlockJobStatus field pointed to by - * @ret_status. - * - * @vm will be unlocked while waiting for the event. - * - * Returns 0 if an event was received, - * -1 otherwise. - */ -int -qemuBlockJobSyncWait(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virConnectDomainEventBlockJobStatus *ret_status) -{ - return qemuBlockJobSyncWaitWithTimeout(driver, vm, disk, - (unsigned long long)-1, - ret_status); + QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync = false; } diff --git a/src/qemu/qemu_blockjob.h b/src/qemu/qemu_blockjob.h index 81e893e..775ce95 100644 --- a/src/qemu/qemu_blockjob.h +++ b/src/qemu/qemu_blockjob.h @@ -37,16 +37,6 @@ void qemuBlockJobEventProcess(virQEMUDriverPtr driver, void qemuBlockJobSyncBegin(virDomainDiskDefPtr disk); void qemuBlockJobSyncEnd(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virConnectDomainEventBlockJobStatus *ret_status); -int qemuBlockJobSyncWaitWithTimeout(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - unsigned long long timeout, - virConnectDomainEventBlockJobStatus *ret_status); -int qemuBlockJobSyncWait(virQEMUDriverPtr driver, - virDomainObjPtr vm, - virDomainDiskDefPtr disk, - virConnectDomainEventBlockJobStatus *ret_status); + virDomainDiskDefPtr disk); #endif /* __QEMU_BLOCKJOB_H__ */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 12a1d97..144a968 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -413,7 +413,6 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, static virClassPtr qemuDomainDiskPrivateClass; -static void qemuDomainDiskPrivateDispose(void *obj); static int qemuDomainDiskPrivateOnceInit(void) @@ -421,7 +420,7 @@ qemuDomainDiskPrivateOnceInit(void) qemuDomainDiskPrivateClass = virClassNew(virClassForObject(), "qemuDomainDiskPrivate", sizeof(qemuDomainDiskPrivate), - qemuDomainDiskPrivateDispose); + NULL); if (!qemuDomainDiskPrivateClass) return -1; else @@ -441,23 +440,9 @@ qemuDomainDiskPrivateNew(void) if (!(priv = virObjectNew(qemuDomainDiskPrivateClass))) return NULL; - if (virCondInit(&priv->blockJobSyncCond) < 0) { - virReportSystemError(errno, "%s", _("Failed to initialize condition")); - virObjectUnref(priv); - return NULL; - } - return (virObjectPtr) priv; } -static void -qemuDomainDiskPrivateDispose(void *obj) -{ - qemuDomainDiskPrivatePtr priv = obj; - - virCondDestroy(&priv->blockJobSyncCond); -} - static void * qemuDomainObjPrivateAlloc(void) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index a6df199..7f06cb8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -214,7 +214,6 @@ struct _qemuDomainDiskPrivate { bool blockjob; /* for some synchronous block jobs, we need to notify the owner */ - virCond blockJobSyncCond; int blockJobType; /* type of the block job from the event */ int blockJobStatus; /* status of the finished block job */ bool blockJobSync; /* the block job needs synchronized termination */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d1b00a2..0a57c20 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16680,10 +16680,8 @@ qemuDomainBlockJobAbort(virDomainPtr dom, goto endjob; } - if (modern && !async) { - /* prepare state for event delivery */ + if (modern && !async) qemuBlockJobSyncBegin(disk); - } if (pivot) { if ((ret = qemuDomainBlockPivot(driver, vm, device, disk)) < 0) @@ -16731,21 +16729,21 @@ qemuDomainBlockJobAbort(virDomainPtr dom, VIR_DOMAIN_BLOCK_JOB_TYPE_PULL, VIR_DOMAIN_BLOCK_JOB_CANCELED); } else { - virConnectDomainEventBlockJobStatus status = -1; - if (qemuBlockJobSyncWait(driver, vm, disk, &status) < 0) { - ret = -1; - } else if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("failed to terminate block job on disk '%s'"), - disk->dst); - ret = -1; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuBlockJobUpdate(driver, vm, disk); + while (diskPriv->blockjob) { + if (virDomainObjWait(vm) < 0) { + ret = -1; + goto endjob; + } + qemuBlockJobUpdate(driver, vm, disk); } } } endjob: - if (disk && QEMU_DOMAIN_DISK_PRIVATE(disk)->blockJobSync) - qemuBlockJobSyncEnd(driver, vm, disk, NULL); + if (disk) + qemuBlockJobSyncEnd(driver, vm, disk); qemuDomainObjEndJob(driver, vm); cleanup: diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 93e29e7..61b3e34 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1720,7 +1720,7 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, /** - * qemuMigrationCheckDriveMirror: + * qemuMigrationDriveMirrorReady: * @driver: qemu driver * @vm: domain * @@ -1733,111 +1733,148 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver, * -1 on error. */ static int -qemuMigrationCheckDriveMirror(virQEMUDriverPtr driver, +qemuMigrationDriveMirrorReady(virQEMUDriverPtr driver, virDomainObjPtr vm) { size_t i; - int ret = 1; + size_t notReady = 0; + int status; for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - if (!diskPriv->migrating || !diskPriv->blockJobSync) + if (!diskPriv->migrating) continue; - /* process any pending event */ - if (qemuBlockJobSyncWaitWithTimeout(driver, vm, disk, - 0ull, NULL) < 0) - return -1; - - switch (disk->mirrorState) { - case VIR_DOMAIN_DISK_MIRROR_STATE_NONE: - ret = 0; - break; - case VIR_DOMAIN_DISK_MIRROR_STATE_ABORT: + status = qemuBlockJobUpdate(driver, vm, disk); + if (status == VIR_DOMAIN_BLOCK_JOB_FAILED) { virReportError(VIR_ERR_OPERATION_FAILED, _("migration of disk %s failed"), disk->dst); return -1; } + + if (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) + notReady++; } - return ret; + if (notReady) { + VIR_DEBUG("Waiting for %zu disk mirrors to get ready", notReady); + return 0; + } else { + VIR_DEBUG("All disk mirrors are ready"); + return 1; + } } -/** - * qemuMigrationCancelOneDriveMirror: - * @driver: qemu driver - * @vm: domain +/* + * If @failed is not NULL, the function will report an error and set @failed + * to true in case a block job fails. This way we can properly abort migration + * in case some block job failed once all memory has already been transferred. * - * Cancel all drive-mirrors started by qemuMigrationDriveMirror. - * Any pending block job events for the mirrored disks will be - * processed. - * - * Returns 0 on success, -1 otherwise. + * Returns 1 if all mirrors are gone, + * 0 if some mirrors are still active, + * -1 on error. */ static int -qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, +qemuMigrationDriveMirrorCancelled(virQEMUDriverPtr driver, virDomainObjPtr vm, - virDomainDiskDefPtr disk) + bool *failed) { - qemuDomainObjPrivatePtr priv = vm->privateData; - char *diskAlias = NULL; - int ret = -1; + size_t i; + size_t active = 0; + int status; - /* No need to cancel if mirror already aborted */ - if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) { - ret = 0; - } else { - virConnectDomainEventBlockJobStatus status = -1; + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - if (virAsprintf(&diskAlias, "%s%s", - QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) - goto cleanup; + if (!diskPriv->migrating) + continue; - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) - goto endjob; - ret = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - goto endjob; - - if (ret < 0) { - virDomainBlockJobInfo info; - - /* block-job-cancel can fail if QEMU simultaneously - * aborted the job; probe for it again to detect this */ - if (qemuMonitorBlockJobInfo(priv->mon, diskAlias, - &info, NULL) == 0) { - ret = 0; - } else { + status = qemuBlockJobUpdate(driver, vm, disk); + switch (status) { + case VIR_DOMAIN_BLOCK_JOB_FAILED: + if (failed) { virReportError(VIR_ERR_OPERATION_FAILED, - _("could not cancel migration of disk %s"), + _("migration of disk %s failed"), disk->dst); + *failed = true; } + /* fallthrough */ + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + qemuBlockJobSyncEnd(driver, vm, disk); + diskPriv->migrating = false; + break; - goto endjob; + default: + active++; } + } - /* Mirror may become ready before cancellation takes - * effect; loop if we get that event first */ - do { - ret = qemuBlockJobSyncWait(driver, vm, disk, &status); - if (ret < 0) { - VIR_WARN("Unable to wait for block job on %s to cancel", - diskAlias); - goto endjob; - } - } while (status == VIR_DOMAIN_BLOCK_JOB_READY); + if (active) { + VIR_DEBUG("Waiting for %zu disk mirrors to finish", active); + return 0; + } else { + if (failed && *failed) + VIR_DEBUG("All disk mirrors are gone; some of them failed"); + else + VIR_DEBUG("All disk mirrors are gone"); + return 1; } +} + + +/* + * Returns 0 on success, + * 1 when job is already completed or it failed and failNoJob is false, + * -1 on error or when job failed and failNoJob is true. + */ +static int +qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, + virDomainObjPtr vm, + virDomainDiskDefPtr disk, + bool failNoJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + char *diskAlias = NULL; + int ret = -1; + int status; + int rv; + + status = qemuBlockJobUpdate(driver, vm, disk); + switch (status) { + case VIR_DOMAIN_BLOCK_JOB_FAILED: + case VIR_DOMAIN_BLOCK_JOB_CANCELED: + if (failNoJob) { + virReportError(VIR_ERR_OPERATION_FAILED, + _("migration of disk %s failed"), + disk->dst); + return -1; + } + return 1; + + case VIR_DOMAIN_BLOCK_JOB_COMPLETED: + return 1; + } + + if (virAsprintf(&diskAlias, "%s%s", + QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) + return -1; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cleanup; + + rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true); - endjob: - qemuBlockJobSyncEnd(driver, vm, disk, NULL); + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) + goto cleanup; - if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ABORT) - disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + ret = 0; cleanup: VIR_FREE(diskAlias); @@ -1849,6 +1886,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, * qemuMigrationCancelDriveMirror: * @driver: qemu driver * @vm: domain + * @check: if true report an error when some of the mirrors fails * * Cancel all drive-mirrors started by qemuMigrationDriveMirror. * Any pending block job events for the affected disks will be @@ -1858,28 +1896,48 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, */ static int qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver, - virDomainObjPtr vm) + virDomainObjPtr vm, + bool check) { virErrorPtr err = NULL; - int ret = 0; + int ret = -1; size_t i; + int rv; + bool failed = false; + bool *failedPtr = check ? &failed : NULL; + + VIR_DEBUG("Cancelling drive mirrors for domain %s", vm->def->name); for (i = 0; i < vm->def->ndisks; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - if (!diskPriv->migrating || !diskPriv->blockJobSync) + if (!diskPriv->migrating) continue; - if (qemuMigrationCancelOneDriveMirror(driver, vm, disk) < 0) { - ret = -1; - if (!err) - err = virSaveLastError(); + rv = qemuMigrationCancelOneDriveMirror(driver, vm, disk, check); + if (rv != 0) { + if (rv < 0) { + if (!err) + err = virSaveLastError(); + failed = true; + } + qemuBlockJobSyncEnd(driver, vm, disk); + diskPriv->migrating = false; } + } - diskPriv->migrating = false; + while ((rv = qemuMigrationDriveMirrorCancelled(driver, vm, + failedPtr)) != 1) { + if (failed && !err) + err = virSaveLastError(); + if (rv < 0 || virDomainObjWait(vm) < 0) + goto cleanup; } + ret = failed ? -1 : 0; + + cleanup: if (err) { virSetError(err); virFreeError(err); @@ -1924,6 +1982,9 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, char *nbd_dest = NULL; char *hoststr = NULL; unsigned int mirror_flags = VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; + int rv; + + VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name); /* steal NBD port and thus prevent its propagation back to destination */ port = mig->nbd->port; @@ -1950,60 +2011,46 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, !virDomainDiskGetSource(disk)) continue; - VIR_FREE(diskAlias); - VIR_FREE(nbd_dest); if ((virAsprintf(&diskAlias, "%s%s", QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) || (virAsprintf(&nbd_dest, "nbd:%s:%d:exportname=%s", hoststr, port, diskAlias) < 0)) goto cleanup; + if (qemuDomainObjEnterMonitorAsync(driver, vm, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + goto cleanup; + qemuBlockJobSyncBegin(disk); - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) { - qemuBlockJobSyncEnd(driver, vm, disk, NULL); - goto cleanup; - } - mon_ret = qemuMonitorDriveMirror(priv->mon, diskAlias, nbd_dest, NULL, speed, 0, 0, mirror_flags); + VIR_FREE(diskAlias); + VIR_FREE(nbd_dest); if (qemuDomainObjExitMonitor(driver, vm) < 0 || mon_ret < 0) { - qemuBlockJobSyncEnd(driver, vm, disk, NULL); + qemuBlockJobSyncEnd(driver, vm, disk); goto cleanup; } diskPriv->migrating = true; } - /* Wait for each disk to become ready in turn, but check the status - * for *all* mirrors to determine if any have aborted. */ - for (i = 0; i < vm->def->ndisks; i++) { - virDomainDiskDefPtr disk = vm->def->disks[i]; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - - if (!diskPriv->migrating) - continue; - - while (disk->mirrorState != VIR_DOMAIN_DISK_MIRROR_STATE_READY) { - /* The following check should be race free as long as the variable - * is set only with domain object locked. And here we have the - * domain object locked too. */ - 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 cleanup; - } - - if (qemuBlockJobSyncWaitWithTimeout(driver, vm, disk, - 500ull, NULL) < 0) - goto cleanup; - - if (qemuMigrationCheckDriveMirror(driver, vm) < 0) - goto cleanup; + while ((rv = qemuMigrationDriveMirrorReady(driver, vm)) != 1) { + unsigned long long now; + + if (rv < 0) + goto cleanup; + + 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 cleanup; } + + if (virTimeMillisNow(&now) < 0 || + virDomainObjWaitUntil(vm, now + 500) < 0) + goto cleanup; } /* Okay, all disks are ready. Modify migrate_flags */ @@ -2436,7 +2483,8 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, virDomainObjPtr vm, qemuDomainAsyncJob asyncJob, virConnectPtr dconn, - bool abort_on_error) + bool abort_on_error, + bool storage) { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; @@ -2467,6 +2515,10 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, if (qemuMigrationUpdateJobStatus(driver, vm, job, asyncJob) == -1) goto error; + if (storage && + qemuMigrationDriveMirrorReady(driver, vm) < 0) + break; + /* cancel migration if disk I/O error is emitted while migrating */ if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && @@ -3542,7 +3594,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver, virErrorPtr orig_err = virSaveLastError(); /* cancel any outstanding NBD jobs */ - qemuMigrationCancelDriveMirror(driver, vm); + qemuMigrationCancelDriveMirror(driver, vm, false); virSetError(orig_err); virFreeError(orig_err); @@ -4084,20 +4136,12 @@ qemuMigrationRun(virQEMUDriverPtr driver, rc = qemuMigrationWaitForCompletion(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, - dconn, abort_on_error); + dconn, abort_on_error, !!mig->nbd); if (rc == -2) goto cancel; else if (rc == -1) goto cleanup; - /* Confirm state of drive mirrors */ - if (mig->nbd) { - if (qemuMigrationCheckDriveMirror(driver, vm) != 1) { - ret = -1; - goto cancel; - } - } - /* When migration completed, QEMU will have paused the * CPUs for us, but unless we're using the JSON monitor * we won't have been notified of this, so might still @@ -4121,7 +4165,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, /* cancel any outstanding NBD jobs */ if (mig && mig->nbd) { - if (qemuMigrationCancelDriveMirror(driver, vm) < 0) + if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0) < 0) ret = -1; } @@ -5575,7 +5619,8 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, if (rc < 0) goto cleanup; - rc = qemuMigrationWaitForCompletion(driver, vm, asyncJob, NULL, false); + rc = qemuMigrationWaitForCompletion(driver, vm, asyncJob, + NULL, false, false); if (rc < 0) { if (rc == -2) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 9c5d0f4..a945401 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1001,10 +1001,10 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED, diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); if (diskPriv->blockJobSync) { + /* We have a SYNC API waiting for this event, dispatch it back */ diskPriv->blockJobType = type; diskPriv->blockJobStatus = status; - /* We have an SYNC API waiting for this event, dispatch it back */ - virCondSignal(&diskPriv->blockJobSyncCond); + virDomainObjSignal(vm); } else { /* there is no waiting SYNC API, dispatch the update to a thread */ if (VIR_ALLOC(processEvent) < 0) @@ -5056,13 +5056,8 @@ void qemuProcessStop(virQEMUDriverPtr driver, if (virAtomicIntDecAndTest(&driver->nactive) && driver->inhibitCallback) driver->inhibitCallback(false, driver->inhibitOpaque); - /* Wake up anything waiting on synchronous block jobs */ - for (i = 0; i < vm->def->ndisks; i++) { - qemuDomainDiskPrivatePtr diskPriv = - QEMU_DOMAIN_DISK_PRIVATE(vm->def->disks[i]); - if (diskPriv->blockJobSync && diskPriv->blockJobStatus == -1) - virCondSignal(&diskPriv->blockJobSyncCond); - } + /* Wake up anything waiting on domain condition */ + virDomainObjBroadcast(vm); if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) { /* To not break the normal domain shutdown process, skip the -- 2.4.1

On 06/02/2015 08:34 AM, Jiri Denemark wrote: ... ah - and now I see it cannot be a void function; however, this could cause Coverity checker to complain about other uses. I wasn't able to apply the series to my Coverity branch since it wouldn't git am apply. Still that leaves me wondering why other paths don't check the status. They may not care, but with one or more paths checking (as seen in the rest of this module) it leaves a future reader not knowing whether the paths that aren't checking should check. John

This patch reverts commit 76c61cdca20c106960af033e5d0f5da70177af0f. VIR_DOMAIN_DISK_MIRROR_STATE_ABORT says we asked for a block job to be aborted rather than saying it was aborted. Let's just use VIR_DOMAIN_DISK_MIRROR_STATE_NONE consistently whenever a block job finishes since no caller depends on VIR_DOMAIN_DISK_MIRROR_STATE_ABORT (anymore) to check whether a block job failed or it was cancelled. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - no changes src/qemu/qemu_blockjob.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c index cefb168..51370bf 100644 --- a/src/qemu/qemu_blockjob.c +++ b/src/qemu/qemu_blockjob.c @@ -164,8 +164,7 @@ qemuBlockJobEventProcess(virQEMUDriverPtr driver, case VIR_DOMAIN_BLOCK_JOB_CANCELED: virStorageSourceFree(disk->mirror); disk->mirror = NULL; - disk->mirrorState = status == VIR_DOMAIN_BLOCK_JOB_FAILED ? - VIR_DOMAIN_DISK_MIRROR_STATE_ABORT : VIR_DOMAIN_DISK_MIRROR_STATE_NONE; + disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_NONE; disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; save = true; diskPriv->blockjob = false; -- 2.4.1

So that they can format private data (e.g., disk private data) stored elsewhere in the domain object. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - no changes src/conf/domain_conf.c | 4 ++-- src/conf/domain_conf.h | 6 ++++-- src/libxl/libxl_domain.c | 10 ++++++---- src/lxc/lxc_domain.c | 12 ++++++++---- src/qemu/qemu_domain.c | 10 ++++++---- 5 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0850b9b..cf0f54b 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -15900,7 +15900,7 @@ virDomainObjParseXML(xmlDocPtr xml, VIR_FREE(nodes); if (xmlopt->privateData.parse && - ((xmlopt->privateData.parse)(ctxt, obj->privateData)) < 0) + xmlopt->privateData.parse(ctxt, obj) < 0) goto error; return obj; @@ -21828,7 +21828,7 @@ virDomainObjFormat(virDomainXMLOptionPtr xmlopt, } if (xmlopt->privateData.format && - ((xmlopt->privateData.format)(&buf, obj->privateData)) < 0) + xmlopt->privateData.format(&buf, obj) < 0) goto error; if (virDomainDefFormatInternal(obj->def, flags, &buf) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 1039f78..05a8884 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2344,8 +2344,10 @@ typedef virDomainXMLOption *virDomainXMLOptionPtr; typedef void *(*virDomainXMLPrivateDataAllocFunc)(void); typedef void (*virDomainXMLPrivateDataFreeFunc)(void *); typedef virObjectPtr (*virDomainXMLPrivateDataNewFunc)(void); -typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr, void *); -typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr, void *); +typedef int (*virDomainXMLPrivateDataFormatFunc)(virBufferPtr, + virDomainObjPtr); +typedef int (*virDomainXMLPrivateDataParseFunc)(xmlXPathContextPtr, + virDomainObjPtr); /* Called once after everything else has been parsed, for adjusting * overall domain defaults. */ diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index c7f0ed9..d38ed61 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -223,9 +223,10 @@ libxlDomainObjPrivateFree(void *data) } static int -libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) +libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, + virDomainObjPtr vm) { - libxlDomainObjPrivatePtr priv = data; + libxlDomainObjPrivatePtr priv = vm->privateData; priv->lockState = virXPathString("string(./lockstate)", ctxt); @@ -233,9 +234,10 @@ libxlDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) } static int -libxlDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) +libxlDomainObjPrivateXMLFormat(virBufferPtr buf, + virDomainObjPtr vm) { - libxlDomainObjPrivatePtr priv = data; + libxlDomainObjPrivatePtr priv = vm->privateData; if (priv->lockState) virBufferAsprintf(buf, "<lockstate>%s</lockstate>\n", priv->lockState); diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index c2180cb..70606f3 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -51,9 +51,11 @@ static void virLXCDomainObjPrivateFree(void *data) } -static int virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) +static int +virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, + virDomainObjPtr vm) { - virLXCDomainObjPrivatePtr priv = data; + virLXCDomainObjPrivatePtr priv = vm->privateData; virBufferAsprintf(buf, "<init pid='%llu'/>\n", (unsigned long long)priv->initpid); @@ -61,9 +63,11 @@ static int virLXCDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) return 0; } -static int virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) +static int +virLXCDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, + virDomainObjPtr vm) { - virLXCDomainObjPrivatePtr priv = data; + virLXCDomainObjPrivatePtr priv = vm->privateData; unsigned long long thepid; if (virXPathULongLong("string(./init[1]/@pid)", ctxt, &thepid) < 0) { diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 144a968..f6c9add 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -511,9 +511,10 @@ qemuDomainObjPrivateFree(void *data) static int -qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) +qemuDomainObjPrivateXMLFormat(virBufferPtr buf, + virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = data; + qemuDomainObjPrivatePtr priv = vm->privateData; const char *monitorpath; qemuDomainJob job; @@ -600,9 +601,10 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, void *data) } static int -qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, void *data) +qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, + virDomainObjPtr vm) { - qemuDomainObjPrivatePtr priv = data; + qemuDomainObjPrivatePtr priv = vm->privateData; char *monitorpath; char *tmp; int n; -- 2.4.1

We don't have an async job when reconnecting to existing domains after libvirtd restart. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - no changes src/qemu/qemu_migration.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 61b3e34..fedadd7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1837,7 +1837,8 @@ static int qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, virDomainObjPtr vm, virDomainDiskDefPtr disk, - bool failNoJob) + bool failNoJob, + qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; char *diskAlias = NULL; @@ -1865,8 +1866,7 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, QEMU_DRIVE_HOST_PREFIX, disk->info.alias) < 0) return -1; - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) goto cleanup; rv = qemuMonitorBlockJobCancel(priv->mon, diskAlias, true); @@ -1897,7 +1897,8 @@ qemuMigrationCancelOneDriveMirror(virQEMUDriverPtr driver, static int qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver, virDomainObjPtr vm, - bool check) + bool check, + qemuDomainAsyncJob asyncJob) { virErrorPtr err = NULL; int ret = -1; @@ -1915,7 +1916,8 @@ qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver, if (!diskPriv->migrating) continue; - rv = qemuMigrationCancelOneDriveMirror(driver, vm, disk, check); + rv = qemuMigrationCancelOneDriveMirror(driver, vm, disk, + check, asyncJob); if (rv != 0) { if (rv < 0) { if (!err) @@ -3594,7 +3596,8 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver, virErrorPtr orig_err = virSaveLastError(); /* cancel any outstanding NBD jobs */ - qemuMigrationCancelDriveMirror(driver, vm, false); + qemuMigrationCancelDriveMirror(driver, vm, false, + QEMU_ASYNC_JOB_MIGRATION_OUT); virSetError(orig_err); virFreeError(orig_err); @@ -4165,7 +4168,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, /* cancel any outstanding NBD jobs */ if (mig && mig->nbd) { - if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0) < 0) + if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0, + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) ret = -1; } -- 2.4.1

"query-block-jobs" QMP command returns all running block jobs at once, while qemuMonitorBlockJobInfo would only report one. This is not very nice in case we need to check several block jobs. This patch refactors the monitor code to always parse all block jobs and store them in a hash. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_driver.c | 45 +++++++++++++++------------- src/qemu/qemu_monitor.c | 37 ++++++++++++++++++----- src/qemu/qemu_monitor.h | 17 ++++++++--- src/qemu/qemu_monitor_json.c | 70 ++++++++++++++++++++++---------------------- src/qemu/qemu_monitor_json.h | 7 ++--- 5 files changed, 104 insertions(+), 72 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0a57c20..0544583 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -16408,7 +16408,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, { int ret = -1, rc; qemuDomainObjPrivatePtr priv = vm->privateData; - virDomainBlockJobInfo info; + qemuMonitorBlockJobInfo info; virStorageSourcePtr oldsrc = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); @@ -16422,7 +16422,7 @@ qemuDomainBlockPivot(virQEMUDriverPtr driver, /* Probe the status, if needed. */ if (!disk->mirrorState) { qemuDomainObjEnterMonitor(driver, vm); - rc = qemuMonitorBlockJobInfo(priv->mon, device, &info, NULL); + rc = qemuMonitorGetBlockJobInfo(priv->mon, disk->info.alias, &info); if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; if (rc < 0) @@ -16755,16 +16755,16 @@ qemuDomainBlockJobAbort(virDomainPtr dom, static int -qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, - virDomainBlockJobInfoPtr info, unsigned int flags) +qemuDomainGetBlockJobInfo(virDomainPtr dom, + const char *path, + virDomainBlockJobInfoPtr info, + unsigned int flags) { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; - char *device = NULL; - int idx; virDomainDiskDefPtr disk; int ret = -1; - unsigned long long bandwidth; + qemuMonitorBlockJobInfo rawInfo; virCheckFlags(VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES, -1); @@ -16787,31 +16787,34 @@ qemuDomainGetBlockJobInfo(virDomainPtr dom, const char *path, if (qemuDomainSupportsBlockJobs(vm, NULL) < 0) goto endjob; - if (!(device = qemuDiskPathToAlias(vm, path, &idx))) + if (!(disk = virDomainDiskByName(vm->def, path, true))) goto endjob; - disk = vm->def->disks[idx]; qemuDomainObjEnterMonitor(driver, vm); - ret = qemuMonitorBlockJobInfo(qemuDomainGetMonitor(vm), device, info, - &bandwidth); + ret = qemuMonitorGetBlockJobInfo(qemuDomainGetMonitor(vm), + disk->info.alias, &rawInfo); if (qemuDomainObjExitMonitor(driver, vm) < 0) ret = -1; if (ret < 0) goto endjob; + info->cur = rawInfo.cur; + info->end = rawInfo.end; + + info->type = rawInfo.type; if (info->type == VIR_DOMAIN_BLOCK_JOB_TYPE_COMMIT && disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_ACTIVE_COMMIT) info->type = disk->mirrorJob; - if (bandwidth) { - if (!(flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES)) - bandwidth = VIR_DIV_UP(bandwidth, 1024 * 1024); - info->bandwidth = bandwidth; - if (info->bandwidth != bandwidth) { - virReportError(VIR_ERR_OVERFLOW, - _("bandwidth %llu cannot be represented in result"), - bandwidth); - goto endjob; - } + + if (rawInfo.bandwidth && + !(flags & VIR_DOMAIN_BLOCK_JOB_INFO_BANDWIDTH_BYTES)) + rawInfo.bandwidth = VIR_DIV_UP(rawInfo.bandwidth, 1024 * 1024); + info->bandwidth = rawInfo.bandwidth; + if (info->bandwidth != rawInfo.bandwidth) { + virReportError(VIR_ERR_OVERFLOW, + _("bandwidth %llu cannot be represented in result"), + rawInfo.bandwidth); + goto endjob; } /* Snoop block copy operations, so future cancel operations can diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f959b74..f612077 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3291,17 +3291,40 @@ qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, } +virHashTablePtr +qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon) +{ + QEMU_CHECK_MONITOR_JSON_NULL(mon); + return qemuMonitorJSONGetAllBlockJobInfo(mon); +} + + +/** + * qemuMonitorGetBlockJobInfo: + * Parse Block Job information, and populate info for the named device. + * Return 1 if info available, 0 if device has no block job, and -1 on error. + */ int -qemuMonitorBlockJobInfo(qemuMonitorPtr mon, - const char *device, - virDomainBlockJobInfoPtr info, - unsigned long long *bandwidth) +qemuMonitorGetBlockJobInfo(qemuMonitorPtr mon, + const char *alias, + qemuMonitorBlockJobInfoPtr info) { - VIR_DEBUG("device=%s, info=%p, bandwidth=%p", device, info, bandwidth); + virHashTablePtr all; + qemuMonitorBlockJobInfoPtr data; + int ret = 0; - QEMU_CHECK_MONITOR_JSON(mon); + VIR_DEBUG("alias=%s, info=%p", alias, info); - return qemuMonitorJSONBlockJobInfo(mon, device, info, bandwidth); + if (!(all = qemuMonitorGetAllBlockJobInfo(mon))) + return -1; + + if ((data = virHashLookup(all, alias))) { + *info = *data; + ret = 1; + } + + virHashFree(all); + return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 76380a0..0d9e413 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -774,10 +774,19 @@ int qemuMonitorBlockJobSetSpeed(qemuMonitorPtr mon, unsigned long long bandwidth, bool modern); -int qemuMonitorBlockJobInfo(qemuMonitorPtr mon, - const char *device, - virDomainBlockJobInfoPtr info, - unsigned long long *bandwidth) +typedef struct _qemuMonitorBlockJobInfo qemuMonitorBlockJobInfo; +typedef qemuMonitorBlockJobInfo *qemuMonitorBlockJobInfoPtr; +struct _qemuMonitorBlockJobInfo { + int type; /* virDomainBlockJobType */ + unsigned long long bandwidth; /* in bytes/s */ + virDomainBlockJobCursor cur; + virDomainBlockJobCursor end; +}; + +virHashTablePtr qemuMonitorGetAllBlockJobInfo(qemuMonitorPtr mon); +int qemuMonitorGetBlockJobInfo(qemuMonitorPtr mon, + const char *device, + qemuMonitorBlockJobInfoPtr info) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); int qemuMonitorOpenGraphics(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e281140..1b92610 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4129,29 +4129,30 @@ int qemuMonitorJSONScreendump(qemuMonitorPtr mon, return ret; } -/* Returns -1 on error, 0 if not the right device, 1 if info was - * populated. However, rather than populate info->bandwidth (which - * might overflow on 32-bit machines), bandwidth is tracked optionally - * on the side. */ + static int -qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry, - const char *device, - virDomainBlockJobInfoPtr info, - unsigned long long *bandwidth) +qemuMonitorJSONParseBlockJobInfo(virHashTablePtr blockJobs, + virJSONValuePtr entry) { - const char *this_dev; + qemuMonitorBlockJobInfoPtr info = NULL; + const char *device; const char *type; - if ((this_dev = virJSONValueObjectGetString(entry, "device")) == NULL) { + if (!(device = virJSONValueObjectGetString(entry, "device"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("entry was missing 'device'")); return -1; } - if (!STREQ(this_dev, device)) - return 0; + if (STRPREFIX(device, QEMU_DRIVE_HOST_PREFIX)) + device += strlen(QEMU_DRIVE_HOST_PREFIX); - type = virJSONValueObjectGetString(entry, "type"); - if (!type) { + if (VIR_ALLOC(info) < 0 || + virHashAddEntry(blockJobs, device, info) < 0) { + VIR_FREE(info); + return -1; + } + + if (!(type = virJSONValueObjectGetString(entry, "type"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("entry was missing 'type'")); return -1; @@ -4165,8 +4166,7 @@ qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry, else info->type = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN; - if (bandwidth && - virJSONValueObjectGetNumberUlong(entry, "speed", bandwidth) < 0) { + if (virJSONValueObjectGetNumberUlong(entry, "speed", &info->bandwidth) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("entry was missing 'speed'")); return -1; @@ -4183,30 +4183,23 @@ qemuMonitorJSONGetBlockJobInfoOne(virJSONValuePtr entry, _("entry was missing 'len'")); return -1; } - return 1; + + return 0; } -/** - * qemuMonitorJSONBlockJobInfo: - * Parse Block Job information, and populate info for the named device. - * Return 1 if info available, 0 if device has no block job, and -1 on error. - */ -int -qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, - const char *device, - virDomainBlockJobInfoPtr info, - unsigned long long *bandwidth) +virHashTablePtr +qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon) { virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr data; int nr_results; size_t i; - int ret = -1; + virHashTablePtr blockJobs = NULL; cmd = qemuMonitorJSONMakeCommand("query-block-jobs", NULL); if (!cmd) - return -1; + return NULL; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; @@ -4228,22 +4221,29 @@ qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, goto cleanup; } - for (i = ret = 0; i < nr_results && ret == 0; i++) { + if (!(blockJobs = virHashCreate(nr_results, virHashValueFree))) + goto cleanup; + + for (i = 0; i < nr_results; i++) { virJSONValuePtr entry = virJSONValueArrayGet(data, i); if (!entry) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing array element")); - ret = -1; - goto cleanup; + goto error; } - ret = qemuMonitorJSONGetBlockJobInfoOne(entry, device, info, - bandwidth); + if (qemuMonitorJSONParseBlockJobInfo(blockJobs, entry) < 0) + goto error; } cleanup: virJSONValueFree(cmd); virJSONValueFree(reply); - return ret; + return blockJobs; + + error: + virHashFree(blockJobs); + blockJobs = NULL; + goto cleanup; } diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 43adc90..b3952fc 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -316,11 +316,8 @@ int qemuMonitorJSONBlockJobSetSpeed(qemuMonitorPtr mon, bool modern) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); -int qemuMonitorJSONBlockJobInfo(qemuMonitorPtr mon, - const char *device, - virDomainBlockJobInfoPtr info, - unsigned long long *bandwidth) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +virHashTablePtr qemuMonitorJSONGetAllBlockJobInfo(qemuMonitorPtr mon) + ATTRIBUTE_NONNULL(1); int qemuMonitorJSONSetLink(qemuMonitorPtr mon, const char *name, -- 2.4.1

When libvirtd is restarted during migration, we properly cancel the ongoing migration (unless it managed to almost finished before the restart). But if we were also migrating storage using NBD, we would completely forget about the running disk mirrors. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - make use of qemuMonitorGetAllBlockJobInfo introduced by the previous patch - undo qemuBlockJobSyncBegin in case of error src/qemu/qemu_domain.c | 45 ++++++++++++++++++++++++- src/qemu/qemu_migration.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_migration.h | 3 ++ src/qemu/qemu_process.c | 8 +---- 4 files changed, 133 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f6c9add..8cb4daa 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -578,7 +578,27 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, qemuDomainAsyncJobPhaseToString( priv->job.asyncJob, priv->job.phase)); } - virBufferAddLit(buf, "/>\n"); + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + virBufferAddLit(buf, "/>\n"); + } else { + size_t i; + virDomainDiskDefPtr disk; + qemuDomainDiskPrivatePtr diskPriv; + + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); + + for (i = 0; i < vm->def->ndisks; i++) { + disk = vm->def->disks[i]; + diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + virBufferAsprintf(buf, "<disk dev='%s' migrating='%s'/>\n", + disk->dst, + diskPriv->migrating ? "yes" : "no"); + } + + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</job>\n"); + } } priv->job.active = job; @@ -736,6 +756,29 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, } } + if ((n = virXPathNodeSet("./job[1]/disk[@migrating='yes']", + ctxt, &nodes)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to parse list of disks marked for migration")); + goto error; + } + if (n > 0) { + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + VIR_WARN("Found disks marked for migration but we were not " + "migrating"); + n = 0; + } + for (i = 0; i < n; i++) { + char *dst = virXMLPropString(nodes[i], "dev"); + virDomainDiskDefPtr disk; + + if (dst && (disk = virDomainDiskByName(vm->def, dst, false))) + QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating = true; + VIR_FREE(dst); + } + } + VIR_FREE(nodes); + priv->fakeReboot = virXPathBoolean("boolean(./fakereboot)", ctxt) == 1; if ((n = virXPathNodeSet("./devices/device", ctxt, &nodes)) < 0) { diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index fedadd7..f6b9aa0 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1985,6 +1985,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, char *hoststr = NULL; unsigned int mirror_flags = VIR_DOMAIN_BLOCK_REBASE_REUSE_EXT; int rv; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); VIR_DEBUG("Starting drive mirrors for domain %s", vm->def->name); @@ -2034,6 +2035,11 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, goto cleanup; } diskPriv->migrating = true; + + if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm) < 0) { + VIR_WARN("Failed to save status on vm %s", vm->def->name); + goto cleanup; + } } while ((rv = qemuMigrationDriveMirrorReady(driver, vm)) != 1) { @@ -2061,6 +2067,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, ret = 0; cleanup: + virObjectUnref(cfg); VIR_FREE(diskAlias); VIR_FREE(nbd_dest); VIR_FREE(hoststr); @@ -5683,6 +5690,84 @@ qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, return ret; } + +int +qemuMigrationCancel(virQEMUDriverPtr driver, + virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + virHashTablePtr blockJobs = NULL; + bool storage = false; + size_t i; + int ret = -1; + + VIR_DEBUG("Canceling unfinished outgoing migration of domain %s", + vm->def->name); + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + if (QEMU_DOMAIN_DISK_PRIVATE(disk)->migrating) { + qemuBlockJobSyncBegin(disk); + storage = true; + } + } + + qemuDomainObjEnterMonitor(driver, vm); + + ignore_value(qemuMonitorMigrateCancel(priv->mon)); + if (storage) + blockJobs = qemuMonitorGetAllBlockJobInfo(priv->mon); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || (storage && !blockJobs)) + goto endsyncjob; + + if (!storage) { + ret = 0; + goto cleanup; + } + + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + + if (!diskPriv->migrating) + continue; + + if (virHashLookup(blockJobs, disk->info.alias)) { + VIR_DEBUG("Drive mirror on disk %s is still running", disk->dst); + } else { + VIR_DEBUG("Drive mirror on disk %s is gone", disk->dst); + qemuBlockJobSyncEnd(driver, vm, disk); + diskPriv->migrating = false; + } + } + + if (qemuMigrationCancelDriveMirror(driver, vm, false, + QEMU_ASYNC_JOB_NONE) < 0) + goto endsyncjob; + + ret = 0; + + cleanup: + virHashFree(blockJobs); + return ret; + + endsyncjob: + if (storage) { + for (i = 0; i < vm->def->ndisks; i++) { + virDomainDiskDefPtr disk = vm->def->disks[i]; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + + if (diskPriv->migrating) { + qemuBlockJobSyncEnd(driver, vm, disk); + diskPriv->migrating = false; + } + } + } + goto cleanup; +} + + int qemuMigrationJobStart(virQEMUDriverPtr driver, virDomainObjPtr vm, diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index 1726455..e47bde5 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -177,4 +177,7 @@ int qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; +int qemuMigrationCancel(virQEMUDriverPtr driver, + virDomainObjPtr vm); + #endif /* __QEMU_MIGRATION_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a945401..24d20e8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3354,8 +3354,6 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, virDomainState state, int reason) { - qemuDomainObjPrivatePtr priv = vm->privateData; - if (job == QEMU_ASYNC_JOB_MIGRATION_IN) { switch (phase) { case QEMU_MIGRATION_PHASE_NONE: @@ -3409,11 +3407,7 @@ qemuProcessRecoverMigration(virQEMUDriverPtr driver, case QEMU_MIGRATION_PHASE_PERFORM3: /* migration is still in progress, let's cancel it and resume the * domain */ - VIR_DEBUG("Canceling unfinished outgoing migration of domain %s", - vm->def->name); - qemuDomainObjEnterMonitor(driver, vm); - ignore_value(qemuMonitorMigrateCancel(priv->mon)); - if (qemuDomainObjExitMonitor(driver, vm) < 0) + if (qemuMigrationCancel(driver, vm) < 0) return -1; /* resume the domain but only if it was paused as a result of * migration */ -- 2.4.1

To avoid polling for asyncAbort flag changes. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - rewritten using domain condition src/qemu/qemu_domain.c | 5 +++-- src/qemu/qemu_domain.h | 2 +- src/qemu/qemu_migration.c | 11 ++++------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 8cb4daa..a1ce988 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -169,7 +169,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv) job->phase = 0; job->mask = QEMU_JOB_DEFAULT_MASK; job->dump_memory_only = false; - job->asyncAbort = false; + job->abortJob = false; VIR_FREE(job->current); } @@ -1652,7 +1652,8 @@ qemuDomainObjAbortAsyncJob(virDomainObjPtr obj) qemuDomainAsyncJobTypeToString(priv->job.asyncJob), obj, obj->def->name); - priv->job.asyncAbort = true; + priv->job.abortJob = true; + virDomainObjBroadcast(obj); } /* diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7f06cb8..7479f70 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -135,7 +135,7 @@ struct qemuDomainJobObj { bool dump_memory_only; /* use dump-guest-memory to do dump */ qemuDomainJobInfoPtr current; /* async job progress data */ qemuDomainJobInfoPtr completed; /* statistics data of a recently completed job */ - bool asyncAbort; /* abort of async job requested */ + bool abortJob; /* abort of the job requested */ }; typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f6b9aa0..74c2657 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2043,12 +2043,10 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, } while ((rv = qemuMigrationDriveMirrorReady(driver, vm)) != 1) { - unsigned long long now; - if (rv < 0) goto cleanup; - if (priv->job.asyncAbort) { + if (priv->job.abortJob) { priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED; virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), qemuDomainAsyncJobTypeToString(priv->job.asyncJob), @@ -2056,8 +2054,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, goto cleanup; } - if (virTimeMillisNow(&now) < 0 || - virDomainObjWaitUntil(vm, now + 500) < 0) + if (virDomainObjWait(vm) < 0) goto cleanup; } @@ -4054,10 +4051,10 @@ qemuMigrationRun(virQEMUDriverPtr driver, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cleanup; - if (priv->job.asyncAbort) { + if (priv->job.abortJob) { /* explicitly do this *after* we entered the monitor, * as this is a critical section so we are guaranteed - * priv->job.asyncAbort will not change */ + * priv->job.abortJob will not change */ ignore_value(qemuDomainObjExitMonitor(driver, vm)); priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED; virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), -- 2.4.1

Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_monitor.c | 12 ++++++++++++ src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 10 ++++++++++ 3 files changed, 28 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index f612077..15f7852 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1570,6 +1570,18 @@ qemuMonitorEmitSerialChange(qemuMonitorPtr mon, int +qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainSpiceMigrated, mon->vm); + + return ret; +} + + +int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { QEMU_CHECK_MONITOR(mon); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0d9e413..86da7ad 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -182,6 +182,10 @@ typedef int (*qemuMonitorDomainSerialChangeCallback)(qemuMonitorPtr mon, bool connected, void *opaque); +typedef int (*qemuMonitorDomainSpiceMigratedCallback)(qemuMonitorPtr mon, + virDomainObjPtr vm, + void *opaque); + typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; struct _qemuMonitorCallbacks { @@ -209,6 +213,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainDeviceDeletedCallback domainDeviceDeleted; qemuMonitorDomainNicRxFilterChangedCallback domainNicRxFilterChanged; qemuMonitorDomainSerialChangeCallback domainSerialChange; + qemuMonitorDomainSpiceMigratedCallback domainSpiceMigrated; }; char *qemuMonitorEscapeArg(const char *in); @@ -307,6 +312,7 @@ int qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon, int qemuMonitorEmitSerialChange(qemuMonitorPtr mon, const char *devAlias, bool connected); +int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon); int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 1b92610..32e4100 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -83,6 +83,7 @@ static void qemuMonitorJSONHandleGuestPanic(qemuMonitorPtr mon, virJSONValuePtr static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -107,6 +108,7 @@ static qemuEventHandler eventHandlers[] = { { "SPICE_CONNECTED", qemuMonitorJSONHandleSPICEConnect, }, { "SPICE_DISCONNECTED", qemuMonitorJSONHandleSPICEDisconnect, }, { "SPICE_INITIALIZED", qemuMonitorJSONHandleSPICEInitialize, }, + { "SPICE_MIGRATE_COMPLETED", qemuMonitorJSONHandleSpiceMigrated, }, { "STOP", qemuMonitorJSONHandleStop, }, { "SUSPEND", qemuMonitorJSONHandlePMSuspend, }, { "SUSPEND_DISK", qemuMonitorJSONHandlePMSuspendDisk, }, @@ -914,6 +916,14 @@ qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, } +static void +qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, + virJSONValuePtr data ATTRIBUTE_UNUSED) +{ + qemuMonitorEmitSpiceMigrated(mon); +} + + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, -- 2.4.1

QEMU_CAPS_SEAMLESS_MIGRATION capability says QEMU supports SPICE_MIGRATE_COMPLETED event. Thus we can just drop all code which polls query-spice and replace it with waiting for the event. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_domain.c | 1 + src/qemu/qemu_domain.h | 1 + src/qemu/qemu_migration.c | 38 ++++++++++------------------------ src/qemu/qemu_monitor.c | 10 --------- src/qemu/qemu_monitor.h | 2 -- src/qemu/qemu_monitor_json.c | 49 -------------------------------------------- src/qemu/qemu_process.c | 28 +++++++++++++++++++++++++ tests/qemumonitorjsontest.c | 40 ------------------------------------ 8 files changed, 41 insertions(+), 128 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a1ce988..fe92a17 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -170,6 +170,7 @@ qemuDomainObjResetAsyncJob(qemuDomainObjPrivatePtr priv) job->mask = QEMU_JOB_DEFAULT_MASK; job->dump_memory_only = false; job->abortJob = false; + job->spiceMigrated = false; VIR_FREE(job->current); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 7479f70..55831a5 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -136,6 +136,7 @@ struct qemuDomainJobObj { qemuDomainJobInfoPtr current; /* async job progress data */ qemuDomainJobInfoPtr completed; /* statistics data of a recently completed job */ bool abortJob; /* abort of the job requested */ + bool spiceMigrated; /* spice migration completed */ }; typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 74c2657..84a66d6 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2375,45 +2375,29 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver, } static int -qemuMigrationWaitForSpice(virQEMUDriverPtr driver, - virDomainObjPtr vm) +qemuMigrationWaitForSpice(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; bool wait_for_spice = false; - bool spice_migrated = false; size_t i = 0; - int rc; - if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) { - for (i = 0; i < vm->def->ngraphics; i++) { - if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { - wait_for_spice = true; - break; - } + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_SEAMLESS_MIGRATION)) + return 0; + + for (i = 0; i < vm->def->ngraphics; i++) { + if (vm->def->graphics[i]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { + wait_for_spice = true; + break; } } if (!wait_for_spice) return 0; - while (!spice_migrated) { - /* Poll every 50ms for progress & to allow cancellation */ - struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; - - if (qemuDomainObjEnterMonitorAsync(driver, vm, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + while (!priv->job.spiceMigrated && !priv->job.abortJob) { + if (virDomainObjWait(vm) < 0) return -1; - - rc = qemuMonitorGetSpiceMigrationStatus(priv->mon, &spice_migrated); - if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; - if (rc < 0) - return -1; - virObjectUnlock(vm); - nanosleep(&ts, NULL); - virObjectLock(vm); } - return 0; } @@ -3587,7 +3571,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver, if (retcode == 0) { /* If guest uses SPICE and supports seamless migration we have to hold * up domain shutdown until SPICE server transfers its data */ - qemuMigrationWaitForSpice(driver, vm); + qemuMigrationWaitForSpice(vm); qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_MIGRATED, VIR_QEMU_PROCESS_STOP_MIGRATED); diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 15f7852..a2a8f0d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2201,16 +2201,6 @@ qemuMonitorGetMigrationStatus(qemuMonitorPtr mon, int -qemuMonitorGetSpiceMigrationStatus(qemuMonitorPtr mon, - bool *spice_migrated) -{ - QEMU_CHECK_MONITOR_JSON(mon); - - return qemuMonitorJSONGetSpiceMigrationStatus(mon, spice_migrated); -} - - -int qemuMonitorMigrateToFd(qemuMonitorPtr mon, unsigned int flags, int fd) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 86da7ad..17b7003 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -498,8 +498,6 @@ struct _qemuMonitorMigrationStatus { int qemuMonitorGetMigrationStatus(qemuMonitorPtr mon, qemuMonitorMigrationStatusPtr status); -int qemuMonitorGetSpiceMigrationStatus(qemuMonitorPtr mon, - bool *spice_migrated); typedef enum { QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 32e4100..a138f1f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -2689,55 +2689,6 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon, } -static int -qemuMonitorJSONSpiceGetMigrationStatusReply(virJSONValuePtr reply, - bool *spice_migrated) -{ - virJSONValuePtr ret; - - if (!(ret = virJSONValueObjectGet(reply, "return"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-spice reply was missing return data")); - return -1; - } - - if (virJSONValueObjectGetBoolean(ret, "migrated", spice_migrated) < 0) { - /* Deliberately don't report error here as we are - * probably dealing with older qemu which doesn't - * report this yet. Pretend spice is migrated. */ - *spice_migrated = true; - } - - return 0; -} - - -int qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitorPtr mon, - bool *spice_migrated) -{ - int ret; - virJSONValuePtr cmd = qemuMonitorJSONMakeCommand("query-spice", - NULL); - virJSONValuePtr reply = NULL; - - if (!cmd) - return -1; - - ret = qemuMonitorJSONCommand(mon, cmd, &reply); - - if (ret == 0) - ret = qemuMonitorJSONCheckError(cmd, reply); - - if (ret == 0) - ret = qemuMonitorJSONSpiceGetMigrationStatusReply(reply, - spice_migrated); - - virJSONValueFree(cmd); - virJSONValueFree(reply); - return ret; -} - - int qemuMonitorJSONMigrate(qemuMonitorPtr mon, unsigned int flags, const char *uri) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 24d20e8..6d1ed28 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1481,6 +1481,33 @@ qemuProcessHandleSerialChanged(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } +static int +qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + + virObjectLock(vm); + + VIR_DEBUG("Spice migration completed for domain %p %s", + vm, vm->def->name); + + priv = vm->privateData; + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT) { + VIR_DEBUG("got SPICE_MIGRATE_COMPLETED event without a migration job"); + goto cleanup; + } + + priv->job.spiceMigrated = true; + virDomainObjSignal(vm); + + cleanup: + virObjectUnlock(vm); + return 0; +} + + static qemuMonitorCallbacks monitorCallbacks = { .eofNotify = qemuProcessHandleMonitorEOF, .errorNotify = qemuProcessHandleMonitorError, @@ -1504,6 +1531,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainDeviceDeleted = qemuProcessHandleDeviceDeleted, .domainNicRxFilterChanged = qemuProcessHandleNicRxFilterChanged, .domainSerialChange = qemuProcessHandleSerialChanged, + .domainSpiceMigrated = qemuProcessHandleSpiceMigrated, }; static int diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 0f82fd8..0623275 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1706,45 +1706,6 @@ testQemuMonitorJSONqemuMonitorJSONGetMigrationStatus(const void *data) } static int -testQemuMonitorJSONqemuMonitorJSONGetSpiceMigrationStatus(const void *data) -{ - virDomainXMLOptionPtr xmlopt = (virDomainXMLOptionPtr)data; - qemuMonitorTestPtr test = qemuMonitorTestNewSimple(true, xmlopt); - int ret = -1; - bool spiceMigrated; - - if (!test) - return -1; - - if (qemuMonitorTestAddItem(test, "query-spice", - "{" - " \"return\": {" - " \"migrated\": true," - " \"enabled\": false," - " \"mouse-mode\": \"client\"" - " }," - " \"id\": \"libvirt-14\"" - "}") < 0) - goto cleanup; - - if (qemuMonitorJSONGetSpiceMigrationStatus(qemuMonitorTestGetMonitor(test), - &spiceMigrated) < 0) - goto cleanup; - - if (!spiceMigrated) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "Invalid spice migration status: %d, expecting 1", - spiceMigrated); - goto cleanup; - } - - ret = 0; - cleanup: - qemuMonitorTestFree(test); - return ret; -} - -static int testHashEqualChardevInfo(const void *value1, const void *value2) { const qemuMonitorChardevInfo *info1 = value1; @@ -2400,7 +2361,6 @@ mymain(void) DO_TEST(qemuMonitorJSONGetBlockStatsInfo); DO_TEST(qemuMonitorJSONGetMigrationCacheSize); DO_TEST(qemuMonitorJSONGetMigrationStatus); - DO_TEST(qemuMonitorJSONGetSpiceMigrationStatus); DO_TEST(qemuMonitorJSONGetChardevInfo); DO_TEST(qemuMonitorJSONSetBlockIoThrottle); DO_TEST(qemuMonitorJSONGetTargetArch); -- 2.4.1

Move common parts of qemuDomainGetJobInfo and qemuDomainGetJobStats into a separate API (qemuDomainGetJobStatsInternal). Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_driver.c | 113 ++++++++++++++++++++++++++----------------------- 1 file changed, 61 insertions(+), 52 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0544583..aa40ddd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13320,42 +13320,72 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } -static int qemuDomainGetJobInfo(virDomainPtr dom, - virDomainJobInfoPtr info) +static int +qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + bool completed, + qemuDomainJobInfoPtr jobInfo) { + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainJobInfoPtr info; + int ret = -1; + + if (!completed && + !virDomainObjIsActive(vm)) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto cleanup; + } + + if (completed) + info = priv->job.completed; + else + info = priv->job.current; + + if (!info) { + jobInfo->type = VIR_DOMAIN_JOB_NONE; + ret = 0; + goto cleanup; + } + *jobInfo = *info; + + if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED || + jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) + ret = qemuDomainJobInfoUpdateTime(jobInfo); + else + ret = 0; + + cleanup: + return ret; +} + + +static int +qemuDomainGetJobInfo(virDomainPtr dom, + virDomainJobInfoPtr info) +{ + virQEMUDriverPtr driver = dom->conn->privateData; + qemuDomainJobInfo jobInfo; virDomainObjPtr vm; int ret = -1; - qemuDomainObjPrivatePtr priv; if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainGetJobInfoEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainObjIsActive(vm)) { - if (priv->job.current) { - /* Refresh elapsed time again just to ensure it - * is fully updated. This is primarily for benefit - * of incoming migration which we don't currently - * monitor actively in the background thread - */ - if (qemuDomainJobInfoUpdateTime(priv->job.current) < 0 || - qemuDomainJobInfoToInfo(priv->job.current, info) < 0) - goto cleanup; - } else { - memset(info, 0, sizeof(*info)); - info->type = VIR_DOMAIN_JOB_NONE; - } - } else { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); + if (qemuDomainGetJobStatsInternal(driver, vm, false, &jobInfo) < 0) + goto cleanup; + + if (jobInfo.type == VIR_DOMAIN_JOB_NONE) { + memset(info, 0, sizeof(*info)); + info->type = VIR_DOMAIN_JOB_NONE; + ret = 0; goto cleanup; } - ret = 0; + ret = qemuDomainJobInfoToInfo(&jobInfo, info); cleanup: virDomainObjEndAPI(&vm); @@ -13370,9 +13400,11 @@ qemuDomainGetJobStats(virDomainPtr dom, int *nparams, unsigned int flags) { + virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; qemuDomainObjPrivatePtr priv; - qemuDomainJobInfoPtr jobInfo; + qemuDomainJobInfo jobInfo; + bool completed = !!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED); int ret = -1; virCheckFlags(VIR_DOMAIN_JOB_STATS_COMPLETED, -1); @@ -13380,24 +13412,14 @@ qemuDomainGetJobStats(virDomainPtr dom, if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; - priv = vm->privateData; - if (virDomainGetJobStatsEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (!(flags & VIR_DOMAIN_JOB_STATS_COMPLETED) && - !virDomainObjIsActive(vm)) { - virReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); + priv = vm->privateData; + if (qemuDomainGetJobStatsInternal(driver, vm, completed, &jobInfo) < 0) goto cleanup; - } - if (flags & VIR_DOMAIN_JOB_STATS_COMPLETED) - jobInfo = priv->job.completed; - else - jobInfo = priv->job.current; - - if (!jobInfo) { + if (jobInfo.type == VIR_DOMAIN_JOB_NONE) { *type = VIR_DOMAIN_JOB_NONE; *params = NULL; *nparams = 0; @@ -13405,24 +13427,11 @@ qemuDomainGetJobStats(virDomainPtr dom, goto cleanup; } - /* Refresh elapsed time again just to ensure it - * is fully updated. This is primarily for benefit - * of incoming migration which we don't currently - * monitor actively in the background thread - */ - if ((jobInfo->type == VIR_DOMAIN_JOB_BOUNDED || - jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) && - qemuDomainJobInfoUpdateTime(jobInfo) < 0) - goto cleanup; + ret = qemuDomainJobInfoToParams(&jobInfo, type, params, nparams); - if (qemuDomainJobInfoToParams(jobInfo, type, params, nparams) < 0) - goto cleanup; - - if (flags & VIR_DOMAIN_JOB_STATS_COMPLETED) + if (completed && ret == 0) VIR_FREE(priv->job.completed); - ret = 0; - cleanup: virDomainObjEndAPI(&vm); return ret; -- 2.4.1

Once we start waiting for migration events instead of polling query-migrate, priv->job.current will not be regularly updated anymore because we will get the current status directly from the events. Thus virDomainGetJob{Info,Stats} will have to query QEMU, but they can't just blindly update priv->job.current structure. This patch introduces qemuMigrationFetchJobStatus which just fills in a caller supplied structure and makes qemuMigrationUpdateJobStatus a tiny wrapper around it. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_migration.c | 133 ++++++++++++++++++++++++++++++---------------- src/qemu/qemu_migration.h | 5 ++ 2 files changed, 93 insertions(+), 45 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 84a66d6..08ea706 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2401,67 +2401,110 @@ qemuMigrationWaitForSpice(virDomainObjPtr vm) return 0; } -static int -qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, - virDomainObjPtr vm, - const char *job, - qemuDomainAsyncJob asyncJob) + +static void +qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo) { - qemuDomainObjPrivatePtr priv = vm->privateData; - qemuMonitorMigrationStatus status; - qemuDomainJobInfoPtr jobInfo; - int ret; - - memset(&status, 0, sizeof(status)); - - ret = qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob); - if (ret < 0) { - /* Guest already exited or waiting for the job timed out; nothing - * further to update. */ - return ret; - } - ret = qemuMonitorGetMigrationStatus(priv->mon, &status); - - if (qemuDomainObjExitMonitor(driver, vm) < 0) - return -1; - - if (ret < 0 || - qemuDomainJobInfoUpdateTime(priv->job.current) < 0) - return -1; - - ret = -1; - jobInfo = priv->job.current; - switch (status.status) { + switch (jobInfo->status.status) { case QEMU_MONITOR_MIGRATION_STATUS_COMPLETED: jobInfo->type = VIR_DOMAIN_JOB_COMPLETED; - /* fall through */ - case QEMU_MONITOR_MIGRATION_STATUS_SETUP: - case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE: - case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING: - ret = 0; break; case QEMU_MONITOR_MIGRATION_STATUS_INACTIVE: jobInfo->type = VIR_DOMAIN_JOB_NONE; - virReportError(VIR_ERR_OPERATION_FAILED, - _("%s: %s"), job, _("is not active")); break; case QEMU_MONITOR_MIGRATION_STATUS_ERROR: jobInfo->type = VIR_DOMAIN_JOB_FAILED; - virReportError(VIR_ERR_OPERATION_FAILED, - _("%s: %s"), job, _("unexpectedly failed")); break; case QEMU_MONITOR_MIGRATION_STATUS_CANCELLED: jobInfo->type = VIR_DOMAIN_JOB_CANCELLED; + break; + + case QEMU_MONITOR_MIGRATION_STATUS_SETUP: + case QEMU_MONITOR_MIGRATION_STATUS_ACTIVE: + case QEMU_MONITOR_MIGRATION_STATUS_CANCELLING: + break; + } +} + + +int +qemuMigrationFetchJobStatus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + qemuDomainJobInfoPtr jobInfo) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + int rv; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0) + return -1; + + memset(&jobInfo->status, 0, sizeof(jobInfo->status)); + rv = qemuMonitorGetMigrationStatus(priv->mon, &jobInfo->status); + + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0) + return -1; + + qemuMigrationUpdateJobType(jobInfo); + return qemuDomainJobInfoUpdateTime(jobInfo); +} + + +static int +qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainJobInfoPtr jobInfo = priv->job.current; + qemuDomainJobInfo newInfo = *jobInfo; + + if (qemuMigrationFetchJobStatus(driver, vm, asyncJob, &newInfo) < 0) + return -1; + + *jobInfo = newInfo; + return 0; +} + + +static int +qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + const char *job, + qemuDomainAsyncJob asyncJob) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainJobInfoPtr jobInfo = priv->job.current; + + if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0) + return -1; + + switch (jobInfo->type) { + case VIR_DOMAIN_JOB_NONE: + virReportError(VIR_ERR_OPERATION_FAILED, + _("%s: %s"), job, _("is not active")); + return -1; + + case VIR_DOMAIN_JOB_FAILED: + virReportError(VIR_ERR_OPERATION_FAILED, + _("%s: %s"), job, _("unexpectedly failed")); + return -1; + + case VIR_DOMAIN_JOB_CANCELLED: virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), job, _("canceled by client")); + return -1; + + case VIR_DOMAIN_JOB_BOUNDED: + case VIR_DOMAIN_JOB_UNBOUNDED: + case VIR_DOMAIN_JOB_COMPLETED: + case VIR_DOMAIN_JOB_LAST: break; } - jobInfo->status = status; - - return ret; + return 0; } @@ -2502,7 +2545,7 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, /* 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) == -1) + if (qemuMigrationCheckJobStatus(driver, vm, job, asyncJob) < 0) goto error; if (storage && @@ -4108,8 +4151,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, * rather failed later on. Check its status before waiting for a * connection from qemu which may never be initiated. */ - if (qemuMigrationUpdateJobStatus(driver, vm, _("migration job"), - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + if (qemuMigrationCheckJobStatus(driver, vm, _("migration job"), + QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cancel; while ((fd = accept(spec->dest.unix_socket.sock, NULL, NULL)) < 0) { diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h index e47bde5..65dfdec 100644 --- a/src/qemu/qemu_migration.h +++ b/src/qemu/qemu_migration.h @@ -180,4 +180,9 @@ int qemuMigrationToFile(virQEMUDriverPtr driver, virDomainObjPtr vm, int qemuMigrationCancel(virQEMUDriverPtr driver, virDomainObjPtr vm); +int qemuMigrationFetchJobStatus(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + qemuDomainJobInfoPtr jobInfo); + #endif /* __QEMU_MIGRATION_H__ */ -- 2.4.1

Instead of passing current job name to several functions which already know what the current job is we can generate the name where we actually need to use it. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_migration.c | 54 ++++++++++++++++++++++++----------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 08ea706..73121c7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2453,6 +2453,24 @@ qemuMigrationFetchJobStatus(virQEMUDriverPtr driver, } +static const char * +qemuMigrationJobName(virDomainObjPtr vm) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + + switch (priv->job.asyncJob) { + case QEMU_ASYNC_JOB_MIGRATION_OUT: + return _("migration job"); + case QEMU_ASYNC_JOB_SAVE: + return _("domain save job"); + case QEMU_ASYNC_JOB_DUMP: + return _("domain core dump job"); + default: + return _("job"); + } +} + + static int qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, virDomainObjPtr vm, @@ -2473,7 +2491,6 @@ qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver, static int qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, virDomainObjPtr vm, - const char *job, qemuDomainAsyncJob asyncJob) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -2484,18 +2501,18 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, switch (jobInfo->type) { case VIR_DOMAIN_JOB_NONE: - virReportError(VIR_ERR_OPERATION_FAILED, - _("%s: %s"), job, _("is not active")); + virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), + qemuMigrationJobName(vm), _("is not active")); return -1; case VIR_DOMAIN_JOB_FAILED: - virReportError(VIR_ERR_OPERATION_FAILED, - _("%s: %s"), job, _("unexpectedly failed")); + virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), + qemuMigrationJobName(vm), _("unexpectedly failed")); return -1; case VIR_DOMAIN_JOB_CANCELLED: - virReportError(VIR_ERR_OPERATION_ABORTED, - _("%s: %s"), job, _("canceled by client")); + virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"), + qemuMigrationJobName(vm), _("canceled by client")); return -1; case VIR_DOMAIN_JOB_BOUNDED: @@ -2521,31 +2538,16 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; - const char *job; int pauseReason; int ret = -1; - switch (priv->job.asyncJob) { - case QEMU_ASYNC_JOB_MIGRATION_OUT: - job = _("migration job"); - break; - case QEMU_ASYNC_JOB_SAVE: - job = _("domain save job"); - break; - case QEMU_ASYNC_JOB_DUMP: - job = _("domain core dump job"); - break; - default: - job = _("job"); - } - jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED; while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; - if (qemuMigrationCheckJobStatus(driver, vm, job, asyncJob) < 0) + if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0) goto error; if (storage && @@ -2556,8 +2558,8 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, if (abort_on_error && virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { - virReportError(VIR_ERR_OPERATION_FAILED, - _("%s: %s"), job, _("failed due to I/O error")); + virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), + qemuMigrationJobName(vm), _("failed due to I/O error")); goto error; } @@ -4151,7 +4153,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, * rather failed later on. Check its status before waiting for a * connection from qemu which may never be initiated. */ - if (qemuMigrationCheckJobStatus(driver, vm, _("migration job"), + if (qemuMigrationCheckJobStatus(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) goto cancel; -- 2.4.1

Checking status of all part of migration and aborting it when something failed is a complex thing which makes the waiting loop hard to read. This patch moves all the checks into a separate function similarly to what was done for drive mirror loops. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_migration.c | 106 +++++++++++++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 40 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 73121c7..b79bbd1 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2525,6 +2525,63 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, } +/** + * Returns 1 if migration completed successfully, + * 0 if the domain is still being migrated, + * -1 migration failed, + * -2 something else failed, we need to cancel migration. + */ +static int +qemuMigrationCompleted(virQEMUDriverPtr driver, + virDomainObjPtr vm, + qemuDomainAsyncJob asyncJob, + virConnectPtr dconn, + bool abort_on_error, + bool storage) +{ + qemuDomainObjPrivatePtr priv = vm->privateData; + qemuDomainJobInfoPtr jobInfo = priv->job.current; + int pauseReason; + + if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0) + goto error; + + if (storage && qemuMigrationDriveMirrorReady(driver, vm) < 0) + goto error; + + if (abort_on_error && + virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && + pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { + virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), + qemuMigrationJobName(vm), _("failed due to I/O error")); + goto error; + } + + if (dconn && virConnectIsAlive(dconn) <= 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Lost connection to destination host")); + goto error; + } + + if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) + return 1; + else + return 0; + + error: + if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { + /* The migration was aborted by us rather than QEMU itself. */ + jobInfo->type = VIR_DOMAIN_JOB_FAILED; + return -2; + } else if (jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) { + jobInfo->type = VIR_DOMAIN_JOB_FAILED; + return -1; + } else { + return -1; + } +} + + /* Returns 0 on success, -2 when migration needs to be cancelled, or -1 when * QEMU reports failed migration. */ @@ -2538,59 +2595,28 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; - int pauseReason; - int ret = -1; + int rv; jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED; - - while (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { + while ((rv = qemuMigrationCompleted(driver, vm, asyncJob, dconn, + abort_on_error, storage)) != 1) { /* Poll every 50ms for progress & to allow cancellation */ struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; - if (qemuMigrationCheckJobStatus(driver, vm, asyncJob) < 0) - goto error; + if (rv < 0) + return rv; - if (storage && - qemuMigrationDriveMirrorReady(driver, vm) < 0) - break; - - /* cancel migration if disk I/O error is emitted while migrating */ - if (abort_on_error && - virDomainObjGetState(vm, &pauseReason) == VIR_DOMAIN_PAUSED && - pauseReason == VIR_DOMAIN_PAUSED_IOERROR) { - virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"), - qemuMigrationJobName(vm), _("failed due to I/O error")); - goto error; - } - - if (dconn && virConnectIsAlive(dconn) <= 0) { - virReportError(VIR_ERR_OPERATION_FAILED, "%s", - _("Lost connection to destination host")); - goto error; - } - - if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { - virObjectUnlock(vm); - nanosleep(&ts, NULL); - virObjectLock(vm); - } + virObjectUnlock(vm); + nanosleep(&ts, NULL); + virObjectLock(vm); } qemuDomainJobInfoUpdateDowntime(jobInfo); VIR_FREE(priv->job.completed); if (VIR_ALLOC(priv->job.completed) == 0) *priv->job.completed = *jobInfo; - return 0; - error: - /* Check if the migration was aborted by us rather than QEMU itself. */ - if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED || - jobInfo->type == VIR_DOMAIN_JOB_COMPLETED) { - if (jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) - ret = -2; - jobInfo->type = VIR_DOMAIN_JOB_FAILED; - } - return ret; + return 0; } -- 2.4.1

Thanks to Juan's work QEMU finally emits an event whenever migration state changes. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: The MIGRATION event is not supported by QEMU yet, this patch is based on the current patches available on qemu-devel mailing list. However, there were no objections to the design of the event, which makes it unlikely to change. Anyway this will have to wait until the patches are applied to QEMU. Version 2: - new patch src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_monitor.c | 14 ++++++++++++++ src/qemu/qemu_monitor.h | 8 ++++++++ src/qemu/qemu_monitor_json.c | 23 +++++++++++++++++++++++ 5 files changed, 48 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 960afa4..51f8a60 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -284,6 +284,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "aes-key-wrap", "dea-key-wrap", "pci-serial", + "migration-event", ); @@ -1480,6 +1481,7 @@ struct virQEMUCapsStringFlags virQEMUCapsEvents[] = { { "BALLOON_CHANGE", QEMU_CAPS_BALLOON_EVENT }, { "SPICE_MIGRATE_COMPLETED", QEMU_CAPS_SEAMLESS_MIGRATION }, { "DEVICE_DELETED", QEMU_CAPS_DEVICE_DEL_EVENT }, + { "MIGRATION", QEMU_CAPS_MIGRATION_EVENT }, }; struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 9c956f3..3f4638f 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -228,6 +228,7 @@ typedef enum { QEMU_CAPS_AES_KEY_WRAP = 186, /* -machine aes_key_wrap */ QEMU_CAPS_DEA_KEY_WRAP = 187, /* -machine dea_key_wrap */ QEMU_CAPS_DEVICE_PCI_SERIAL = 188, /* -device pci-serial */ + QEMU_CAPS_MIGRATION_EVENT = 189, /* MIGRATION event */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a2a8f0d..00b8380 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1582,6 +1582,20 @@ qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon) int +qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon, + int status) +{ + int ret = -1; + VIR_DEBUG("mon=%p, status=%s", + mon, NULLSTR(qemuMonitorMigrationStatusTypeToString(status))); + + QEMU_MONITOR_CALLBACK(mon, ret, domainMigrationStatus, mon->vm, status); + + return ret; +} + + +int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { QEMU_CHECK_MONITOR(mon); diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 17b7003..da74492 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -186,6 +186,11 @@ typedef int (*qemuMonitorDomainSpiceMigratedCallback)(qemuMonitorPtr mon, virDomainObjPtr vm, void *opaque); +typedef int (*qemuMonitorDomainMigrationStatusCallback)(qemuMonitorPtr mon, + virDomainObjPtr vm, + int status, + void *opaque); + typedef struct _qemuMonitorCallbacks qemuMonitorCallbacks; typedef qemuMonitorCallbacks *qemuMonitorCallbacksPtr; struct _qemuMonitorCallbacks { @@ -214,6 +219,7 @@ struct _qemuMonitorCallbacks { qemuMonitorDomainNicRxFilterChangedCallback domainNicRxFilterChanged; qemuMonitorDomainSerialChangeCallback domainSerialChange; qemuMonitorDomainSpiceMigratedCallback domainSpiceMigrated; + qemuMonitorDomainMigrationStatusCallback domainMigrationStatus; }; char *qemuMonitorEscapeArg(const char *in); @@ -313,6 +319,8 @@ int qemuMonitorEmitSerialChange(qemuMonitorPtr mon, const char *devAlias, bool connected); int qemuMonitorEmitSpiceMigrated(qemuMonitorPtr mon); +int qemuMonitorEmitMigrationStatus(qemuMonitorPtr mon, + int status); int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a138f1f..487039d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -84,6 +84,7 @@ static void qemuMonitorJSONHandleDeviceDeleted(qemuMonitorPtr mon, virJSONValueP static void qemuMonitorJSONHandleNicRxFilterChanged(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleSerialChange(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandleMigrationStatus(qemuMonitorPtr mon, virJSONValuePtr data); typedef struct { const char *type; @@ -99,6 +100,7 @@ static qemuEventHandler eventHandlers[] = { { "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, { "GUEST_PANICKED", qemuMonitorJSONHandleGuestPanic, }, + { "MIGRATION", qemuMonitorJSONHandleMigrationStatus, }, { "NIC_RX_FILTER_CHANGED", qemuMonitorJSONHandleNicRxFilterChanged, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, @@ -924,6 +926,27 @@ qemuMonitorJSONHandleSpiceMigrated(qemuMonitorPtr mon, } +static void +qemuMonitorJSONHandleMigrationStatus(qemuMonitorPtr mon, + virJSONValuePtr data) +{ + const char *str; + int status; + + if (!(str = virJSONValueObjectGetString(data, "status"))) { + VIR_WARN("missing status in migration event"); + return; + } + + if ((status = qemuMonitorMigrationStatusTypeFromString(str)) == -1) { + VIR_WARN("unknown status '%s' in migration event", str); + return; + } + + qemuMonitorEmitMigrationStatus(mon, status); +} + + int qemuMonitorJSONHumanCommandWithFd(qemuMonitorPtr mon, const char *cmd_str, -- 2.4.1

We don't need to call query-migrate every 50ms when we get the current migration state via MIGRATION event. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_migration.c | 14 ++++++++++++-- src/qemu/qemu_process.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index b79bbd1..f27f1ca 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2496,7 +2496,11 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; - if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0) + bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); + + if (events) + qemuMigrationUpdateJobType(jobInfo); + else if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0) return -1; switch (jobInfo->type) { @@ -2515,9 +2519,15 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver, qemuMigrationJobName(vm), _("canceled by client")); return -1; + case VIR_DOMAIN_JOB_COMPLETED: + /* Fetch statistics of a completed migration */ + if (events && + qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0) + return -1; + break; + case VIR_DOMAIN_JOB_BOUNDED: case VIR_DOMAIN_JOB_UNBOUNDED: - case VIR_DOMAIN_JOB_COMPLETED: case VIR_DOMAIN_JOB_LAST: break; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6d1ed28..0427e5d 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1508,6 +1508,36 @@ qemuProcessHandleSpiceMigrated(qemuMonitorPtr mon ATTRIBUTE_UNUSED, } +static int +qemuProcessHandleMigrationStatus(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm, + int status, + void *opaque ATTRIBUTE_UNUSED) +{ + qemuDomainObjPrivatePtr priv; + + virObjectLock(vm); + + VIR_DEBUG("Migration of domain %p %s changed state to %s", + vm, vm->def->name, + qemuMonitorMigrationStatusTypeToString(status)); + + priv = vm->privateData; + if (priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_OUT && + priv->job.asyncJob != QEMU_ASYNC_JOB_MIGRATION_IN) { + VIR_DEBUG("got MIGRATION event without a migration job"); + goto cleanup; + } + + priv->job.current->status.status = status; + virDomainObjSignal(vm); + + cleanup: + virObjectUnlock(vm); + return 0; +} + + static qemuMonitorCallbacks monitorCallbacks = { .eofNotify = qemuProcessHandleMonitorEOF, .errorNotify = qemuProcessHandleMonitorError, @@ -1532,6 +1562,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainNicRxFilterChanged = qemuProcessHandleNicRxFilterChanged, .domainSerialChange = qemuProcessHandleSerialChanged, .domainSpiceMigrated = qemuProcessHandleSpiceMigrated, + .domainMigrationStatus = qemuProcessHandleMigrationStatus, }; static int -- 2.4.1

When cancelling migration we can see the following conversation on QMP monitor: {"execute":"migrate_cancel","id":"libvirt-33"} {"timestamp": {"seconds": 1432899178, "microseconds": 844907}, "event": "MIGRATION", "data": {"status": "cancelling"}} {"return": {}, "id": "libvirt-33"} {"timestamp": {"seconds": 1432899178, "microseconds": 845625}, "event": "MIGRATION", "data": {"status": "failed"}} {"timestamp": {"seconds": 1432899178, "microseconds": 846432}, "event": "MIGRATION", "data": {"status": "cancelled"}} That is, migration status first changes to "failed" just to change to the correct "cancelled" state in a few moments later. However, this is enough to let libvirt report migration failed for unknown reason instead of having been cancelled by a user. This should really be fixed in QEMU but I'm not sure how easy it is. However, it's pretty easy for us to work around it. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_process.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 0427e5d..8575d90 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1515,6 +1515,7 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { qemuDomainObjPrivatePtr priv; + qemuDomainJobInfoPtr jobInfo; virObjectLock(vm); @@ -1529,7 +1530,15 @@ qemuProcessHandleMigrationStatus(qemuMonitorPtr mon ATTRIBUTE_UNUSED, goto cleanup; } - priv->job.current->status.status = status; + jobInfo = priv->job.current; + if (status == QEMU_MONITOR_MIGRATION_STATUS_ERROR && + jobInfo->status.status == QEMU_MONITOR_MIGRATION_STATUS_CANCELLING) { + VIR_DEBUG("State changed from \"cancelling\" to \"failed\"; setting " + "current state to \"cancelled\""); + status = QEMU_MONITOR_MIGRATION_STATUS_CANCELLED; + } + + jobInfo->status.status = status; virDomainObjSignal(vm); cleanup: -- 2.4.1

When QEMU supports migration events qemuDomainJobInfo structure is no longer updated with migration statistics. We have to enter a job and explicitly ask QEMU every time virDomainGetJob{Info,Stats} is called. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_driver.c | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aa40ddd..a53840b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13321,15 +13321,27 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, static int -qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, +qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver, virDomainObjPtr vm, bool completed, qemuDomainJobInfoPtr jobInfo) { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr info; + bool fetch = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); int ret = -1; + if (completed) + fetch = false; + + /* Do not ask QEMU if migration is not even running yet */ + if (!priv->job.current || !priv->job.current->status.status) + fetch = false; + + if (fetch && + qemuDomainObjBeginJob(driver, vm, QEMU_JOB_QUERY) < 0) + return -1; + if (!completed && !virDomainObjIsActive(vm)) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", @@ -13350,12 +13362,19 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver ATTRIBUTE_UNUSED, *jobInfo = *info; if (jobInfo->type == VIR_DOMAIN_JOB_BOUNDED || - jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) - ret = qemuDomainJobInfoUpdateTime(jobInfo); - else + jobInfo->type == VIR_DOMAIN_JOB_UNBOUNDED) { + if (fetch) + ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE, + jobInfo); + else + ret = qemuDomainJobInfoUpdateTime(jobInfo); + } else { ret = 0; + } cleanup: + if (fetch) + qemuDomainObjEndJob(driver, vm); return ret; } -- 2.4.1

Since we already support the MIGRATION event, we just need to make sure the domain condition is signalled whenever a p2p connection drops or the domain is paused due to IO error and we can avoid waking up every 50 ms to check whether something happened. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_migration.c | 45 +++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_process.c | 3 +++ 3 files changed, 45 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 55831a5..09bc0e1 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -199,6 +199,9 @@ struct _qemuDomainObjPrivate { /* Bitmaps below hold data from the auto NUMA feature */ virBitmapPtr autoNodeset; virBitmapPtr autoCpuset; + + bool signalIOError; /* true if the domain condition should be signalled on + I/O error */ }; # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f27f1ca..f4edac8 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -2605,20 +2605,28 @@ qemuMigrationWaitForCompletion(virQEMUDriverPtr driver, { qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainJobInfoPtr jobInfo = priv->job.current; + bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); int rv; jobInfo->type = VIR_DOMAIN_JOB_UNBOUNDED; while ((rv = qemuMigrationCompleted(driver, vm, asyncJob, dconn, abort_on_error, storage)) != 1) { - /* Poll every 50ms for progress & to allow cancellation */ - struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; - if (rv < 0) return rv; - virObjectUnlock(vm); - nanosleep(&ts, NULL); - virObjectLock(vm); + if (events) { + if (virDomainObjWait(vm) < 0) { + jobInfo->type = VIR_DOMAIN_JOB_FAILED; + return -2; + } + } else { + /* Poll every 50ms for progress & to allow cancellation */ + struct timespec ts = { .tv_sec = 0, .tv_nsec = 50 * 1000 * 1000ull }; + + virObjectUnlock(vm); + nanosleep(&ts, NULL); + virObjectLock(vm); + } } qemuDomainJobInfoUpdateDowntime(jobInfo); @@ -4035,6 +4043,7 @@ qemuMigrationRun(virQEMUDriverPtr driver, virErrorPtr orig_err = NULL; unsigned int cookieFlags = 0; bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR); + bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT); int rc; VIR_DEBUG("driver=%p, vm=%p, cookiein=%s, cookieinlen=%d, " @@ -4064,6 +4073,9 @@ qemuMigrationRun(virQEMUDriverPtr driver, return -1; } + if (events) + priv->signalIOError = abort_on_error; + mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen, cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS); if (!mig) @@ -4269,6 +4281,9 @@ qemuMigrationRun(virQEMUDriverPtr driver, qemuMigrationCookieFree(mig); + if (events) + priv->signalIOError = false; + if (orig_err) { virSetError(orig_err); virFreeError(orig_err); @@ -4893,6 +4908,18 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, } +static void +qemuMigrationConnectionClosed(virConnectPtr conn, + int reason, + void *opaque) +{ + virDomainObjPtr vm = opaque; + + VIR_DEBUG("conn=%p, reason=%d, vm=%s", conn, reason, vm->def->name); + virDomainObjSignal(vm); +} + + static int virConnectCredType[] = { VIR_CRED_AUTHNAME, VIR_CRED_PASSPHRASE, @@ -4966,6 +4993,11 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, cfg->keepAliveCount) < 0) goto cleanup; + if (virConnectRegisterCloseCallback(dconn, qemuMigrationConnectionClosed, + vm, NULL) < 0) { + goto cleanup; + } + qemuDomainObjEnterRemote(vm); p2p = VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn, VIR_DRV_FEATURE_MIGRATION_P2P); @@ -5030,6 +5062,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, cleanup: orig_err = virSaveLastError(); qemuDomainObjEnterRemote(vm); + virConnectUnregisterCloseCallback(dconn, qemuMigrationConnectionClosed); virObjectUnref(dconn); qemuDomainObjExitRemote(vm); if (orig_err) { diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 8575d90..99bc8b7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -952,6 +952,9 @@ qemuProcessHandleIOError(qemuMonitorPtr mon ATTRIBUTE_UNUSED, qemuDomainObjPrivatePtr priv = vm->privateData; VIR_DEBUG("Transitioned guest %s to paused state due to IO error", vm->def->name); + if (priv->signalIOError) + virDomainObjSignal(vm); + virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, VIR_DOMAIN_PAUSED_IOERROR); lifecycleEvent = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED, -- 2.4.1

When a connection to the destination host during a p2p migration drops, we know we will have to cancel the migration; it doesn't make sense to waste resources by trying to finish the migration. We already do so after sending "migrate" command to QEMU and we should do it while waiting for drive mirrors to become ready too. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- Notes: Version 2: - new patch src/qemu/qemu_migration.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index f4edac8..fda775d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1898,7 +1898,8 @@ static int qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver, virDomainObjPtr vm, bool check, - qemuDomainAsyncJob asyncJob) + qemuDomainAsyncJob asyncJob, + virConnectPtr dconn) { virErrorPtr err = NULL; int ret = -1; @@ -1931,8 +1932,16 @@ qemuMigrationCancelDriveMirror(virQEMUDriverPtr driver, while ((rv = qemuMigrationDriveMirrorCancelled(driver, vm, failedPtr)) != 1) { + if (check && !failed && + dconn && virConnectIsAlive(dconn) <= 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Lost connection to destination host")); + failed = true; + } + if (failed && !err) err = virSaveLastError(); + if (rv < 0 || virDomainObjWait(vm) < 0) goto cleanup; } @@ -1974,7 +1983,8 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, qemuMigrationCookiePtr mig, const char *host, unsigned long speed, - unsigned int *migrate_flags) + unsigned int *migrate_flags, + virConnectPtr dconn) { qemuDomainObjPrivatePtr priv = vm->privateData; int ret = -1; @@ -2054,6 +2064,12 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver, goto cleanup; } + if (dconn && virConnectIsAlive(dconn) <= 0) { + virReportError(VIR_ERR_OPERATION_FAILED, "%s", + _("Lost connection to destination host")); + goto cleanup; + } + if (virDomainObjWait(vm) < 0) goto cleanup; } @@ -3674,7 +3690,7 @@ qemuMigrationConfirmPhase(virQEMUDriverPtr driver, /* cancel any outstanding NBD jobs */ qemuMigrationCancelDriveMirror(driver, vm, false, - QEMU_ASYNC_JOB_MIGRATION_OUT); + QEMU_ASYNC_JOB_MIGRATION_OUT, NULL); virSetError(orig_err); virFreeError(orig_err); @@ -4091,7 +4107,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, if (qemuMigrationDriveMirror(driver, vm, mig, spec->dest.host.name, migrate_speed, - &migrate_flags) < 0) { + &migrate_flags, + dconn) < 0) { goto cleanup; } } else { @@ -4250,7 +4267,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, /* cancel any outstanding NBD jobs */ if (mig && mig->nbd) { if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + QEMU_ASYNC_JOB_MIGRATION_OUT, + dconn) < 0) ret = -1; } @@ -5838,7 +5856,7 @@ qemuMigrationCancel(virQEMUDriverPtr driver, } if (qemuMigrationCancelDriveMirror(driver, vm, false, - QEMU_ASYNC_JOB_NONE) < 0) + QEMU_ASYNC_JOB_NONE, NULL) < 0) goto endsyncjob; ret = 0; -- 2.4.1