On Tue, Dec 16, 2014 at 02:00:09PM +0100, Michal Privoznik wrote:
On 16.12.2014 11:15, Martin Kletzander wrote:There is one problem that causes various errors in the daemon. When domain is waiting for a job, it is unlocked while waiting on the condition. However, if that domain is for example transient and being removed in another API (e.g. cancelling incoming migration), it get's unref'd. If the first call, that was waiting, fails to get the job, it unref's the domain object, and because it was the last reference, it causes clearing of the whole domain object. However, when finishing the call, the domain must be unlocked, but there is no way for the API to know whether it was cleaned or not (unless there is some ugly temporary variable, but let's scratch that). The root cause is that our APIs don't ref the objects they are using and all use the implicit reference that the object has when it is in the domain list. That reference can be removed when the API is waiting for a job. And because each domain doesn't do its ref'ing, it results in the ugly checking of the return value of virObjectUnref() that we have everywhere. This patch changes qemuDomObjFromDomain() to ref the domain (using virDomainObjListFindByUUIDRef()) and adds qemuDomObjEndAPI() which should be the only function in which the return value of virObjectUnref() is checked. This makes all reference counting deterministic and makes the code a bit clearer. Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx> --- v2: - Comments from Peter and Daniel on v1 implemented, rather not listing them here as the list was pretty comprehensive and it would make the reviewer focus on that. src/qemu/THREADS.txt | 40 ++- src/qemu/qemu_domain.c | 49 ++-- src/qemu/qemu_domain.h | 12 +- src/qemu/qemu_driver.c | 708 ++++++++++++++++------------------------------ src/qemu/qemu_migration.c | 111 +++----- src/qemu/qemu_migration.h | 10 +- src/qemu/qemu_process.c | 77 ++--- 7 files changed, 371 insertions(+), 636 deletions(-)diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f652237..30ea9df 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c@@ -4409,8 +4333,7 @@ static void qemuProcessEventHandler(void *data, void *opaque) break; } - if (virObjectUnref(vm)) - virObjectUnlock(vm); + qemuDomObjEndAPI(&vm);Interesting, why the heck have we Unref() here? I mean, in the calltrace I don't see where the @vm is Ref()-ed. And this should not use the reference obtained via qemuMonitorOpen() as we are connecting only once and receiving events multiple times. But this is pre-existing code.
You won't see that as that is done in another thread. Each one of processing methods (e.g. qemuProcessHandleDeviceDeleted, qemuProcessHandleGuestPanic) reference the VM themselves before unlocking it to make sure it is not getting disposed of before the event is processed. And that processing is left to the worker pool that does qemuProcessEventHandler().
VIR_FREE(processEvent); }diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a19e71a..4d9ce09 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c@@ -5491,6 +5470,7 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver, virConnectPtr conn) { VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn); + virObjectRef(vm); return virCloseCallbacksSet(driver->closeCallbacks, vm, conn, qemuProcessAutoDestroy);This function is called conditionally in qemuProcessStart() depending on VIR_QEMU_PROCESS_START_AUTODESTROY flag passed...} @@ -5498,9 +5478,12 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver, int qemuProcessAutoDestroyRemove(virQEMUDriverPtr driver, virDomainObjPtr vm) { + int ret; VIR_DEBUG("vm=%s", vm->def->name); - return virCloseCallbacksUnset(driver->closeCallbacks, vm, - qemuProcessAutoDestroy); + ret = virCloseCallbacksUnset(driver->closeCallbacks, vm, + qemuProcessAutoDestroy); + virObjectUnref(vm); + return ret;.. on the other hand, this function is called unconditionally in qemuProcessStop(). So after 'virsh start' and 'virsh destroy' the reference counts don't match. I've been able to debug this and I came up with this diff:
This diff seems accurate even for use outside of qemu. I'm squashing it in, but I'll wait with the pushing so others have a chance to react. Thanks for the review, Martin
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 4d9ce09..cbd0389 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -5470,7 +5470,6 @@ int qemuProcessAutoDestroyAdd(virQEMUDriverPtr driver,
virConnectPtr conn)
{
VIR_DEBUG("vm=%s, conn=%p", vm->def->name, conn);
- virObjectRef(vm);
return virCloseCallbacksSet(driver->closeCallbacks, vm, conn,
qemuProcessAutoDestroy);
}
@@ -5482,7 +5481,6 @@ int qemuProcessAutoDestroyRemove(virQEMUDriverPtr
driver,
VIR_DEBUG("vm=%s", vm->def->name);
ret = virCloseCallbacksUnset(driver->closeCallbacks, vm,
qemuProcessAutoDestroy);
- virObjectUnref(vm);
return ret;
}
diff --git a/src/util/virclosecallbacks.c b/src/util/virclosecallbacks.c
index 4f26172..4128057 100644
--- a/src/util/virclosecallbacks.c
+++ b/src/util/virclosecallbacks.c
@@ -138,6 +138,7 @@ virCloseCallbacksSet(virCloseCallbacksPtr
closeCallbacks,
VIR_FREE(closeDef);
goto cleanup;
}
+ virObjectRef(vm);
}
ret = 0;
@@ -172,7 +173,11 @@ virCloseCallbacksUnset(virCloseCallbacksPtr
closeCallbacks,
goto cleanup;
}
- ret = virHashRemoveEntry(closeCallbacks->list, uuidstr);
+ if (virHashRemoveEntry(closeCallbacks->list, uuidstr) < 0)
+ goto cleanup;
+
+ virObjectUnref(vm);
+ ret = 0;
cleanup:
virObjectUnlock(closeCallbacks);
return ret;
ACK with this (or equivalent) diff squashed in.
Michal
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list