On 12/03/14 10:54, Peter Krempa wrote: > On 12/03/14 06:49, 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> --- >> src/qemu/THREADS.txt | 40 ++- src/qemu/qemu_domain.c | 28 >> +- src/qemu/qemu_domain.h | 12 +- src/qemu/qemu_driver.c | 708 >> ++++++++++++++++------------------------------ >> src/qemu/qemu_migration.c | 108 +++---- src/qemu/qemu_migration.h | >> 10 +- src/qemu/qemu_process.c | 126 ++++----- 7 files changed, 379 >> insertions(+), 653 deletions(-) >> > > ... > >> index 08d6b7c..a0582d3 100644 --- a/src/qemu/qemu_process.c +++ >> b/src/qemu/qemu_process.c > > ... > >> @@ -3756,6 +3736,9 @@ qemuProcessReconnectHelper(virDomainObjPtr >> obj, struct qemuProcessReconnectData *src = opaque; struct >> qemuProcessReconnectData *data; >> >> + virObjectLock(obj); + virObjectRef(obj); + if (!obj->pid) >> return 0; >> > > This hunk causes deadlock of the daemon when starting if you have at > least one inactive VM. You lock the object, find out that the pid > is 0 and return without unlocking. > > The daemon then locks up when trying to reload snapshot list for > the domain as the domain is still left locked: > > Thread 2 (Thread 0x7fd6165f1700 (LWP 847791)): > #0 0x00007fd62c16faf4 in __lll_lock_wait () from /lib64/libpthread.so.0 > #1 0x00007fd62c16b459 in _L_lock_535 () from /lib64/libpthread.so.0 > #2 0x00007fd62c16b280 in pthread_mutex_lock () from /lib64/libpthread.so.0 > #3 0x00007fd62f99b453 in virMutexLock (m=0x7fd61c0e4810) at util/virthread.c:88 > #4 0x00007fd62f97cdc6 in virObjectLock (anyobj=0x7fd61c0e4800) at util/virobject.c:323 > #5 0x00007fd617672af5 in qemuDomainSnapshotLoad (vm=0x7fd61c0e4800, data=0x7fd61c0023f0) at qemu/qemu_driver.c:474 > #6 0x00007fd62f9f20b8 in virDomainObjListHelper (payload=0x7fd61c0e4800, name=0x7fd61c0f0de0, opaque=0x7fd6165f0810) at conf/domain_conf.c:20454 > #7 0x00007fd62f9559be in virHashForEach (table=0x7fd61c008050, iter=0x7fd62f9f2072 <virDomainObjListHelper>, data=0x7fd6165f0810) at util/virhash.c:521 > #8 0x00007fd62f9f213e in virDomainObjListForEach (doms=0x7fd61c03bdd0, callback=0x7fd617672a55 <qemuDomainSnapshotLoad>, opaque=0x7fd61c0023f0) at conf/domain_conf.c:20467 > #9 0x00007fd61767445c in qemuStateInitialize (privileged=true, callback=0x7fd630584ab2 <daemonInhibitCallback>, opaque=0x7fd63258dba0) at qemu/qemu_driver.c:877 > #10 0x00007fd62fa5f05a in virStateInitialize (privileged=true, callback=0x7fd630584ab2 <daemonInhibitCallback>, opaque=0x7fd63258dba0) at libvirt.c:742 > #11 0x00007fd630584b7a in daemonRunStateInit (opaque=0x7fd63258dba0) at libvirtd.c:918 > #12 0x00007fd62f99b8b4 in virThreadHelper (data=0x7fd6325b4480) at util/virthread.c:197 > #13 0x00007fd62c169023 in start_thread () from /lib64/libpthread.so.0 > #14 0x00007fd62bea470d in clone () from /lib64/libc.so.6 > > > > >> @@ -3780,8 +3763,6 @@ qemuProcessReconnectHelper(virDomainObjPtr >> obj, * this early phase. */ >> >> - virObjectLock(obj); - qemuDomainObjRestoreJob(obj, >> &data->oldjob); >> >> if (qemuDomainObjBeginJob(src->driver, obj, QEMU_JOB_MODIFY) < 0) > > Rest of the review will follow once I find a way to solve that issue. > I think the best solution will be to refactor the reconnect function before this change so that it makes more sense. I'll post patches later today hopefully. Peter
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list