[PATCH 00/12] qemu: migration: Fix crashes when VM shutdowns itself during migration in active state
10:11 a.m.
The daemon crashes due to unexpected cleanup happening due to bad
assumptions about locking and state. See patch 5.
Peter Krempa (12):
qemuBlockJobProcessEventConcludedBackup: Handle potentially NULL
'job->disk'
qemuDomainDiskPrivateDispose: Prevent dangling 'disk' pointer in
blockjob data
qemuDomainDeviceBackendChardevForeach: Fix typo in comment
qemuDomainObjWait: Add documentation
qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks
the VM
qemuProcessStop: Move code not depending on 'vm->def->id' after reset
of the ID
qemu: process: Ensure that 'beingDestroyed' gets cleared only after VM
id is reset
qemu: domain: Introduce qemuDomainObjIsActive helper
qemu: migration: Properly check for live VM after qemuDomainObjWait()
qemu: migration: Inline 'qemuMigrationDstFinishResume()'
qemuMigrationSrcRun: Re-check whether VM is active before accessing
job data
qemu: migration: Preserve error across qemuDomainSetMaxMemLock() on
error paths
src/qemu/qemu_backup.c | 6 +--
src/qemu/qemu_backup.h | 2 +-
src/qemu/qemu_blockjob.c | 9 +++-
src/qemu/qemu_domain.c | 40 +++++++++++++-
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_migration.c | 43 +++++++---------
src/qemu/qemu_process.c | 106 ++++++++++++++++++++++----------------
7 files changed, 131 insertions(+), 77 deletions(-)
--
2.45.2
10:11 a.m.
New subject: [PATCH 01/12] qemuBlockJobProcessEventConcludedBackup: Handle potentially NULL 'job->disk'
Similarly to other blockjob handlers, if there's no disk associated with
the blockjob the handler needs to behave correctly. This is needed as
the disk might have been de-associated on unplug or other operations.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_backup.c | 6 +++---
src/qemu/qemu_backup.h | 2 +-
src/qemu/qemu_blockjob.c | 9 +++++++--
3 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_backup.c b/src/qemu/qemu_backup.c
index 857709b17e..81391c29f7 100644
--- a/src/qemu/qemu_backup.c
+++ b/src/qemu/qemu_backup.c
@@ -966,7 +966,7 @@ qemuBackupGetXMLDesc(virDomainObj *vm,
void
qemuBackupNotifyBlockjobEnd(virDomainObj *vm,
- virDomainDiskDef *disk,
+ const char *diskdst,
qemuBlockjobState state,
const char *errmsg,
unsigned long long cur,
@@ -983,7 +983,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObj *vm,
size_t i;
VIR_DEBUG("vm: '%s', disk:'%s', state:'%d'
errmsg:'%s'",
- vm->def->name, disk->dst, state, NULLSTR(errmsg));
+ vm->def->name, NULLSTR(diskdst), state, NULLSTR(errmsg));
if (!backup)
return;
@@ -1016,7 +1016,7 @@ qemuBackupNotifyBlockjobEnd(virDomainObj *vm,
if (!backupdisk->store)
continue;
- if (STREQ(disk->dst, backupdisk->name)) {
+ if (STREQ_NULLABLE(diskdst, backupdisk->name)) {
switch (state) {
case QEMU_BLOCKJOB_STATE_COMPLETED:
backupdisk->state = VIR_DOMAIN_BACKUP_DISK_STATE_COMPLETE;
diff --git a/src/qemu/qemu_backup.h b/src/qemu/qemu_backup.h
index ec0603026a..768da6cbef 100644
--- a/src/qemu/qemu_backup.h
+++ b/src/qemu/qemu_backup.h
@@ -36,7 +36,7 @@ qemuBackupJobCancelBlockjobs(virDomainObj *vm,
void
qemuBackupNotifyBlockjobEnd(virDomainObj *vm,
- virDomainDiskDef *disk,
+ const char *diskdst,
qemuBlockjobState state,
const char *errmsg,
unsigned long long cur,
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index 4b5b63d287..42856df6d4 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -1372,8 +1372,12 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriver *driver,
unsigned long long progressTotal)
{
g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;
+ const char *diskdst = NULL;
+
+ if (job->disk)
+ diskdst = job->disk->dst;
- qemuBackupNotifyBlockjobEnd(vm, job->disk, newstate, job->errmsg,
+ qemuBackupNotifyBlockjobEnd(vm, diskdst, newstate, job->errmsg,
progressCurrent, progressTotal, asyncJob);
if (job->data.backup.store &&
@@ -1386,7 +1390,8 @@ qemuBlockJobProcessEventConcludedBackup(virQEMUDriver *driver,
if (backend)
qemuBlockStorageSourceAttachRollback(qemuDomainGetMonitor(vm), backend);
- if (job->data.backup.bitmap)
+ if (job->disk &&
+ job->data.backup.bitmap)
qemuMonitorBitmapRemove(qemuDomainGetMonitor(vm),
qemuBlockStorageSourceGetEffectiveNodename(job->disk->src),
job->data.backup.bitmap);
--
2.45.2
10:11 a.m.
New subject: [PATCH 02/12] qemuDomainDiskPrivateDispose: Prevent dangling 'disk' pointer in blockjob data
Clear the 'disk' member of 'blockjob' as we're freeing the disk
object
at this point. While this should not normally happen it was observed
when other bug allowed the VM to be cleared while other threads didn't
yet finish.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 7ba2ea4a5e..a39f361a64 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -798,7 +798,13 @@ qemuDomainDiskPrivateDispose(void *obj)
virObjectUnref(priv->migrSource);
g_free(priv->qomName);
g_free(priv->nodeCopyOnRead);
- virObjectUnref(priv->blockjob);
+ if (priv->blockjob) {
+ /* Prevent dangling 'disk' pointer, as the disk object will be freed
+ * right after this function returns if any of the blockjob instance
+ * outlives this for any reason. */
+ priv->blockjob->disk = NULL;
+ virObjectUnref(priv->blockjob);
+ }
}
static virClass *qemuDomainStorageSourcePrivateClass;
--
2.45.2
10:11 a.m.
New subject: [PATCH 03/12] qemuDomainDeviceBackendChardevForeach: Fix typo in comment
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index a39f361a64..9bbad887e0 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12302,7 +12302,7 @@ qemuDomainDeviceBackendChardevIter(virDomainDef *def
G_GNUC_UNUSED,
/**
- * qemuDomainDeviceBackendChardevForeach:a
+ * qemuDomainDeviceBackendChardevForeach:
* @def: domain definition
* @cb: callback
* @opqaue: data for @cb
--
2.45.2
10:11 a.m.
New subject: [PATCH 04/12] qemuDomainObjWait: Add documentation
Document why this function exists and meaning of return values.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9bbad887e0..8fe1b1924d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12368,6 +12368,18 @@ qemuDomainRemoveLogs(virQEMUDriver *driver,
}
+/**
+ * qemuDomainObjWait:
+ * @vm: domain object
+ *
+ * Wait for a signal on the main domain condition. Take into account internal
+ * qemu state in addition to what virDomainObjWait checks. Code in the qemu
+ * driver must use this function exclusively instead of virDomainObjWait.
+ *
+ * Returns:
+ * - 0 on successful wait AND VM is guaranteed to be running
+ * - -1 on failure to wait or VM was terminated while waiting
+ */
int
qemuDomainObjWait(virDomainObj *vm)
{
--
2.45.2
5:36 a.m.
New subject: [PATCH 04/12] qemuDomainObjWait: Add documentation
On 6/13/24 17:11, Peter Krempa wrote:
Document why this function exists and meaning of return values.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9bbad887e0..8fe1b1924d 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12368,6 +12368,18 @@ qemuDomainRemoveLogs(virQEMUDriver *driver,
}
+/**
+ * qemuDomainObjWait:
+ * @vm: domain object
+ *
+ * Wait for a signal on the main domain condition. Take into account internal
+ * qemu state in addition to what virDomainObjWait checks. Code in the qemu
+ * driver must use this function exclusively instead of virDomainObjWait.
+ *
+ * Returns:
+ * - 0 on successful wait AND VM is guaranteed to be running
+ * - -1 on failure to wait or VM was terminated while waiting
Nitpick, drop those leading '-' symbols. And possibly add a comma/full
stop at EOLs.
+ */
int
qemuDomainObjWait(virDomainObj *vm)
{
Michal
10:11 a.m.
New subject: [PATCH 05/12] qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks the VM
'qemuDomainObjStopWorker()' which is meant to dispose of the event loop
thread for the monitor unlocks the VM object while disposing the thread
to prevent possible deadlocks with events waiting on the monitor thread.
Unfortunately 'qemuDomainObjStopWorker()' is called *before* the VM is
marked as inactive by clearing 'vm->def->id', but at the same time it's
no longer marked as 'beingDestroyed' when we're inside
'qemuProcessStop()'.
If 'vm' would be kept locked this wouldn't be a problem. Same way it's
not a problem for anything that uses non-ASYNC VM jobs, or when the
monitor is accessed in an async job, as the 'destroy' job interlocks
with those.
It is a problem for code inside an async job which uses
'qemuDomainObjWait()' though. The API contract of qemuDomainObjWait()
ensures the caller that the VM on successful return from it, but in this
specific reason it's not the case, as both 'beingDestroyed' is already
false, and 'vm->def->id' is not yet cleared.
To fix the issue move the 'qemuDomainObjStopWorker()' call *after*
clearing 'vm->def->id' and also add a note stating what the function is
doing.
Fixes: 860a999802d3c82538373bb3f314f92a2e258754
Closes: https://gitlab.com/libvirt/libvirt/-/issues/640
Reported-by: grass-lu <https://gitlab.com/grass-lu>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_process.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 7ef7040a85..aef83d2409 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8527,8 +8527,6 @@ void qemuProcessStop(virQEMUDriver *driver,
g_clear_pointer(&priv->monConfig, virObjectUnref);
}
- qemuDomainObjStopWorker(vm);
-
/* Remove the master key */
qemuDomainMasterKeyRemove(priv);
@@ -8562,6 +8560,11 @@ void qemuProcessStop(virQEMUDriver *driver,
/* Wake up anything waiting on domain condition */
virDomainObjBroadcast(vm);
+ /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent
+ * deadlocks with the per-VM event loop thread. This MUST be done after
+ * marking the VM as dead */
+ qemuDomainObjStopWorker(vm);
+
virFileDeleteTree(priv->libDir);
virFileDeleteTree(priv->channelTargetDir);
--
2.45.2
10:11 a.m.
New subject: [PATCH 06/12] qemuProcessStop: Move code not depending on 'vm->def->id' after reset of the ID
There are few function calls done while cleaning up a stopped VM which
do require the old VM id, to e.g. clean up paths containing the 'short'
domain name in the path.
Anything else, which doesn't strictly require it can be moved after
clearing the 'id' in order to decrease likelyhood of potential bugs.
This patch moves all the code which does not require the 'id' (except
for the log entry and closing the monitor socket) after the statement
clearing the id and adds a comment explaining that anything in the
section must not unlock the VM object.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_process.c | 85 ++++++++++++++++++++++-------------------
1 file changed, 46 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index aef83d2409..3187f0a9a2 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8482,10 +8482,11 @@ void qemuProcessStop(virQEMUDriver *driver,
goto endjob;
}
- qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
-
- if (!!g_atomic_int_dec_and_test(&driver->nactive) &&
driver->inhibitCallback)
- driver->inhibitCallback(false, driver->inhibitOpaque);
+ /* BEWARE: At this point 'vm->def->id' is not cleared yet. Any code
that
+ * requires the id (e.g. to call virDomainDefGetShortName()) must be placed
+ * between here (after the VM is killed) and the statement clearing the id.
+ * The code *MUST NOT* unlock vm, otherwise other code might be confused
+ * about the state of the VM. */
if ((timestamp = virTimeStringNow()) != NULL) {
qemuDomainLogAppendMessage(driver, vm, "%s: shutting down,
reason=%s\n",
@@ -8493,6 +8494,47 @@ void qemuProcessStop(virQEMUDriver *driver,
virDomainShutoffReasonTypeToString(reason));
}
+ /* shut it off for sure */
+ ignore_value(qemuProcessKill(vm,
+ VIR_QEMU_PROCESS_KILL_FORCE|
+ VIR_QEMU_PROCESS_KILL_NOCHECK));
+
+ if (priv->agent) {
+ g_clear_pointer(&priv->agent, qemuAgentClose);
+ }
+ priv->agentError = false;
+
+ if (priv->mon) {
+ g_clear_pointer(&priv->mon, qemuMonitorClose);
+ }
+
+ qemuProcessBuildDestroyMemoryPaths(driver, vm, NULL, false);
+
+ /* Do this before we delete the tree and remove pidfile. */
+ qemuProcessKillManagedPRDaemon(vm);
+
+ qemuDomainCleanupRun(driver, vm);
+
+ outgoingMigration = (flags & VIR_QEMU_PROCESS_STOP_MIGRATED) &&
+ (asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT);
+
+ qemuExtDevicesStop(driver, vm, outgoingMigration);
+
+ qemuDBusStop(driver, vm);
+
+ vm->def->id = -1;
+
+ /* Wake up anything waiting on domain condition */
+ virDomainObjBroadcast(vm);
+
+ /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent
+ * deadlocks with the per-VM event loop thread. This MUST be done after
+ * marking the VM as dead */
+ qemuDomainObjStopWorker(vm);
+
+ if (!!g_atomic_int_dec_and_test(&driver->nactive) &&
driver->inhibitCallback)
+ driver->inhibitCallback(false, driver->inhibitOpaque);
+
/* Clear network bandwidth */
virDomainClearNetBandwidth(vm->def);
@@ -8512,15 +8554,6 @@ void qemuProcessStop(virQEMUDriver *driver,
virPortAllocatorRelease(priv->nbdPort);
priv->nbdPort = 0;
- if (priv->agent) {
- g_clear_pointer(&priv->agent, qemuAgentClose);
- }
- priv->agentError = false;
-
- if (priv->mon) {
- g_clear_pointer(&priv->mon, qemuMonitorClose);
- }
-
if (priv->monConfig) {
if (priv->monConfig->type == VIR_DOMAIN_CHR_TYPE_UNIX)
unlink(priv->monConfig->data.nix.path);
@@ -8530,41 +8563,15 @@ void qemuProcessStop(virQEMUDriver *driver,
/* Remove the master key */
qemuDomainMasterKeyRemove(priv);
- /* Do this before we delete the tree and remove pidfile. */
- qemuProcessKillManagedPRDaemon(vm);
-
ignore_value(virDomainChrDefForeach(vm->def,
false,
qemuProcessCleanupChardevDevice,
NULL));
- /* shut it off for sure */
- ignore_value(qemuProcessKill(vm,
- VIR_QEMU_PROCESS_KILL_FORCE|
- VIR_QEMU_PROCESS_KILL_NOCHECK));
-
/* Its namespace is also gone then. */
qemuDomainDestroyNamespace(driver, vm);
- qemuDomainCleanupRun(driver, vm);
-
- outgoingMigration = (flags & VIR_QEMU_PROCESS_STOP_MIGRATED) &&
- (asyncJob == VIR_ASYNC_JOB_MIGRATION_OUT);
- qemuExtDevicesStop(driver, vm, outgoingMigration);
-
- qemuDBusStop(driver, vm);
-
- vm->def->id = -1;
-
- /* Wake up anything waiting on domain condition */
- virDomainObjBroadcast(vm);
-
- /* IMPORTANT: qemuDomainObjStopWorker() unlocks @vm in order to prevent
- * deadlocks with the per-VM event loop thread. This MUST be done after
- * marking the VM as dead */
- qemuDomainObjStopWorker(vm);
-
virFileDeleteTree(priv->libDir);
virFileDeleteTree(priv->channelTargetDir);
--
2.45.2
10:11 a.m.
New subject: [PATCH 07/12] qemu: process: Ensure that 'beingDestroyed' gets cleared only after VM id is reset
Prevent the possibility that a VM could be considered as alive while
inside qemuProcessStop.
A recently fixed bug which unlocked the domain object while inside
qemuProcessStop showed that there's possibility to confuse the state of
the VM to be considered active while 'qemuProcessStop' is processing
shutdown of the VM. Ensure that this doesn't happen by clearing the
'beingDestroyed' flag only after the VM id is cleared.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_process.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 3187f0a9a2..8736738a5b 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -8413,29 +8413,31 @@ qemuProcessBeginStopJob(virDomainObj *vm,
{
qemuDomainObjPrivate *priv = vm->privateData;
unsigned int killFlags = forceKill ? VIR_QEMU_PROCESS_KILL_FORCE : 0;
- int ret = -1;
/* We need to prevent monitor EOF callback from doing our work (and
* sending misleading events) while the vm is unlocked inside
- * BeginJob/ProcessKill API
- */
+ * BeginJob/ProcessKill API or any other code path before 'vm->def->id'
is
+ * cleared inside qemuProcessStop */
priv->beingDestroyed = true;
if (qemuProcessKill(vm, killFlags) < 0)
- goto cleanup;
+ goto error;
/* Wake up anything waiting on domain condition */
VIR_DEBUG("waking up all jobs waiting on the domain condition");
virDomainObjBroadcast(vm);
if (virDomainObjBeginJob(vm, job) < 0)
- goto cleanup;
+ goto error;
- ret = 0;
+ /* priv->beingDestroyed is deliberately left set to 'true' here. Caller
+ * is supposed to call qemuProcessStop, which will reset it after
+ * 'vm->def->id' is set to -1 */
+ return 0;
- cleanup:
+ error:
priv->beingDestroyed = false;
- return ret;
+ return -1;
}
@@ -8522,7 +8524,13 @@ void qemuProcessStop(virQEMUDriver *driver,
qemuDBusStop(driver, vm);
+ /* Only after this point we can reset 'priv->beingDestroyed' so that
+ * there's no point at which the VM could be considered as alive between
+ * entering the destroy job and this point where the active "flag" is
+ * cleared.
+ */
vm->def->id = -1;
+ priv->beingDestroyed = false;
/* Wake up anything waiting on domain condition */
virDomainObjBroadcast(vm);
--
2.45.2
10:11 a.m.
New subject: [PATCH 08/12] qemu: domain: Introduce qemuDomainObjIsActive helper
The helper checks whether VM is active including the internal qemu
state. This helper will become useful in situations when an async job
is in use as VIR_JOB_DESTROY can run along async jobs thus both checks
are necessary.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_domain.c | 18 ++++++++++++++++++
src/qemu/qemu_domain.h | 2 ++
2 files changed, 20 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 8fe1b1924d..64fe1abbc5 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -12397,6 +12397,24 @@ qemuDomainObjWait(virDomainObj *vm)
}
+/**
+ * qemuDomainObjIsActive:
+ * @vm: domain object
+ *
+ * Return whether @vm is active. Take qemu-driver specifics into account.
+ */
+bool
+qemuDomainObjIsActive(virDomainObj *vm)
+{
+ qemuDomainObjPrivate *priv = vm->privateData;
+
+ if (priv->beingDestroyed)
+ return false;
+
+ return virDomainObjIsActive(vm);
+}
+
+
/**
* virDomainRefreshStatsSchema:
* @driver: qemu driver data
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index a3089ea449..d777559119 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1113,6 +1113,8 @@ qemuDomainRemoveLogs(virQEMUDriver *driver,
int
qemuDomainObjWait(virDomainObj *vm);
+bool
+qemuDomainObjIsActive(virDomainObj *vm);
int
qemuDomainRefreshStatsSchema(virDomainObj *dom);
--
2.45.2
10:11 a.m.
New subject: [PATCH 09/12] qemu: migration: Properly check for live VM after qemuDomainObjWait()
Similarly to the one change in commit 4d1a1fdffda19a62d62fa2457d162362
we should be checking that the VM is not being yet destroyed if we've
invoked qemuDomainObjWait().
Use the new helper qemuDomainObjIsActive().
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_migration.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 1faab5dd23..0d8d3fd94f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2058,7 +2058,6 @@ qemuMigrationSrcWaitForCompletion(virDomainObj *vm,
virConnectPtr dconn,
unsigned int flags)
{
- qemuDomainObjPrivate *priv = vm->privateData;
virDomainJobData *jobData = vm->job->current;
int rv;
@@ -2069,7 +2068,7 @@ qemuMigrationSrcWaitForCompletion(virDomainObj *vm,
return rv;
if (qemuDomainObjWait(vm) < 0) {
- if (virDomainObjIsActive(vm) && !priv->beingDestroyed)
+ if (qemuDomainObjIsActive(vm))
jobData->status = VIR_DOMAIN_JOB_STATUS_FAILED;
return -2;
}
@@ -5055,7 +5054,7 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
error:
virErrorPreserveLast(&orig_err);
- if (virDomainObjIsActive(vm)) {
+ if (qemuDomainObjIsActive(vm)) {
int reason;
virDomainState state = virDomainObjGetState(vm, &reason);
@@ -6781,7 +6780,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
* overwrites it. */
virErrorPreserveLast(&orig_err);
- if (virDomainObjIsActive(vm)) {
+ if (qemuDomainObjIsActive(vm)) {
if (doKill) {
qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED,
VIR_ASYNC_JOB_MIGRATION_IN,
@@ -6805,7 +6804,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
jobPriv->migParams, vm->job->apiFlags);
}
- if (!virDomainObjIsActive(vm))
+ if (!qemuDomainObjIsActive(vm))
qemuDomainRemoveInactive(driver, vm, VIR_DOMAIN_UNDEFINE_TPM, false);
virErrorRestore(&orig_err);
@@ -7050,7 +7049,7 @@ qemuMigrationSrcToFile(virQEMUDriver *driver, virDomainObj *vm,
virErrorPreserveLast(&orig_err);
/* Restore max migration bandwidth */
- if (virDomainObjIsActive(vm)) {
+ if (qemuDomainObjIsActive(vm)) {
if (qemuMigrationParamsSetULL(migParams,
QEMU_MIGRATION_PARAM_MAX_BANDWIDTH,
saveMigBandwidth * 1024 * 1024) == 0)
--
2.45.2
10:11 a.m.
New subject: [PATCH 10/12] qemu: migration: Inline 'qemuMigrationDstFinishResume()'
The function is a pointless wrapper on top of
qemuMigrationDstWaitForCompletion.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_migration.c | 19 ++-----------------
1 file changed, 2 insertions(+), 17 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 0d8d3fd94f..3524915e9d 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -2098,7 +2098,7 @@ qemuMigrationDstWaitForCompletion(virDomainObj *vm,
unsigned int flags = 0;
int rv;
- VIR_DEBUG("Waiting for incoming migration to complete");
+ VIR_DEBUG("Waiting for incoming migration to complete (vm='%p')",
vm);
if (postcopy)
flags = QEMU_MIGRATION_COMPLETED_POSTCOPY;
@@ -6689,21 +6689,6 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver,
}
-static int
-qemuMigrationDstFinishResume(virDomainObj *vm)
-{
- VIR_DEBUG("vm=%p", vm);
-
- if (qemuMigrationDstWaitForCompletion(vm,
- VIR_ASYNC_JOB_MIGRATION_IN,
- false) < 0) {
- return -1;
- }
-
- return 0;
-}
-
-
static virDomainPtr
qemuMigrationDstFinishActive(virQEMUDriver *driver,
virConnectPtr dconn,
@@ -6753,7 +6738,7 @@ qemuMigrationDstFinishActive(virQEMUDriver *driver,
}
if (flags & VIR_MIGRATE_POSTCOPY_RESUME) {
- rc = qemuMigrationDstFinishResume(vm);
+ rc = qemuMigrationDstWaitForCompletion(vm, VIR_ASYNC_JOB_MIGRATION_IN, false);
inPostCopy = true;
} else {
rc = qemuMigrationDstFinishFresh(driver, vm, mig, flags, v3proto,
--
2.45.2
10:11 a.m.
New subject: [PATCH 11/12] qemuMigrationSrcRun: Re-check whether VM is active before accessing job data
'qemuProcessStop()' clears the 'current' job data. While the code under
the 'error' label in 'qemuMigrationSrcRun()' does check that the VM is
active before accessing the job, it also invokes multiple helper
functions to clean up the migration including
'qemuMigrationSrcNBDCopyCancel()' which calls 'qemuDomainObjWait()'
invalidating the result of the liveness check as it unlocks the VM.
Duplicate the liveness check and explain why. The rest of the code e.g.
accessing the monitor is safe as 'qemuDomainEnterMonitorAsync()'
performs a liveness check. The cleanup path just ignores the return
values of those functions.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_migration.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 3524915e9d..89ddc586bd 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -5074,7 +5074,13 @@ qemuMigrationSrcRun(virQEMUDriver *driver,
dconn);
qemuMigrationSrcCancelRemoveTempBitmaps(vm, VIR_ASYNC_JOB_MIGRATION_OUT);
+ }
+ /* We need to re-check that the VM is active as functions like
+ * qemuMigrationSrcCancel/qemuMigrationSrcNBDCopyCancel wait on the VM
+ * condition unlocking the VM object which can lead to a cleanup of the
+ * 'current' job via qemuProcessStop */
+ if (qemuDomainObjIsActive(vm)) {
if (vm->job->current->status != VIR_DOMAIN_JOB_STATUS_CANCELED)
vm->job->current->status = VIR_DOMAIN_JOB_STATUS_FAILED;
}
--
2.45.2
10:11 a.m.
New subject: [PATCH 12/12] qemu: migration: Preserve error across qemuDomainSetMaxMemLock() on error paths
When a VM terminates itself while it's being migrated in running state
libvirt would report wrong error:
error: cannot get locked memory limit of process 2502057: No such file or directory
rather than the proper error:
error: operation failed: domain is not running
Remember the error on error paths in qemuMigrationSrcConfirmPhase and
qemuMigrationSrcPerformPhase.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_migration.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 89ddc586bd..26c082fc08 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -4015,8 +4015,6 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver,
qemuMigrationSrcNBDCopyCancel(vm, false,
VIR_ASYNC_JOB_MIGRATION_OUT, NULL);
- virErrorRestore(&orig_err);
-
if (virDomainObjGetState(vm, &reason) == VIR_DOMAIN_PAUSED &&
reason == VIR_DOMAIN_PAUSED_POSTCOPY) {
qemuMigrationSrcPostcopyFailed(vm);
@@ -4029,6 +4027,7 @@ qemuMigrationSrcConfirmPhase(virQEMUDriver *driver,
}
qemuDomainSaveStatus(vm);
+ virErrorRestore(&orig_err);
}
return 0;
@@ -6230,11 +6229,15 @@ qemuMigrationSrcPerformPhase(virQEMUDriver *driver,
cleanup:
if (ret < 0 && !virDomainObjIsFailedPostcopy(vm, vm->job)) {
+ virErrorPtr orig_err;
+ virErrorPreserveLast(&orig_err);
+
qemuMigrationSrcRestoreDomainState(driver, vm);
qemuMigrationParamsReset(vm, VIR_ASYNC_JOB_MIGRATION_OUT,
jobPriv->migParams, vm->job->apiFlags);
qemuDomainSetMaxMemLock(vm, 0, &priv->preMigrationMemlock);
qemuMigrationJobFinish(vm);
+ virErrorRestore(&orig_err);
} else {
if (ret < 0)
ignore_value(qemuMigrationJobSetPhase(vm,
QEMU_MIGRATION_PHASE_POSTCOPY_FAILED));
--
2.45.2
5:36 a.m.
New subject: [PATCH 12/12] qemu: migration: Preserve error across
qemuDomainSetMaxMemLock() on error paths
On 6/13/24 17:11, Peter Krempa wrote:
When a VM terminates itself while it's being migrated in running
state
libvirt would report wrong error:
error: cannot get locked memory limit of process 2502057: No such file or directory
rather than the proper error:
error: operation failed: domain is not running
Remember the error on error paths in qemuMigrationSrcConfirmPhase and
qemuMigrationSrcPerformPhase.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_migration.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
Trying to NOT open pandora's box: sometimes I wish we had stacked error
messages.
Michal
5:36 a.m.
New subject: [PATCH 00/12] qemu: migration: Fix crashes when VM shutdowns
itself during migration in active state
On 6/13/24 17:11, Peter Krempa wrote:
The daemon crashes due to unexpected cleanup happening due to bad
assumptions about locking and state. See patch 5.
Peter Krempa (12):
qemuBlockJobProcessEventConcludedBackup: Handle potentially NULL
'job->disk'
qemuDomainDiskPrivateDispose: Prevent dangling 'disk' pointer in
blockjob data
qemuDomainDeviceBackendChardevForeach: Fix typo in comment
qemuDomainObjWait: Add documentation
qemuProcessStop: Prevent crash when qemuDomainObjStopWorker() unlocks
the VM
qemuProcessStop: Move code not depending on 'vm->def->id' after reset
of the ID
qemu: process: Ensure that 'beingDestroyed' gets cleared only after VM
id is reset
qemu: domain: Introduce qemuDomainObjIsActive helper
qemu: migration: Properly check for live VM after qemuDomainObjWait()
qemu: migration: Inline 'qemuMigrationDstFinishResume()'
qemuMigrationSrcRun: Re-check whether VM is active before accessing
job data
qemu: migration: Preserve error across qemuDomainSetMaxMemLock() on
error paths
src/qemu/qemu_backup.c | 6 +--
src/qemu/qemu_backup.h | 2 +-
src/qemu/qemu_blockjob.c | 9 +++-
src/qemu/qemu_domain.c | 40 +++++++++++++-
src/qemu/qemu_domain.h | 2 +
src/qemu/qemu_migration.c | 43 +++++++---------
src/qemu/qemu_process.c | 106 ++++++++++++++++++++++----------------
7 files changed, 131 insertions(+), 77 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
Michal
155
days inactive
161
days old
15 comments
2 participants
participants (2)
-
Michal Prívozník -
Peter Krempa