Re: [libvirt] : Re: [PATCH v2] qemu: fix deadlock if createqemuProcessReconnect thread failed
On 09/13/2018 10:11 PM, wang.yechao255(a)zte.com.cn wrote:
> I just code review, found there may be problem.
>
> The follow statement in founction qemuProcessReconnectHelper:
>
> "if (virThreadCreate(&thread, false, qemuProcessReconnect, data) < 0)
"
>
> may be failed (no one can guarantee 'virThreadCreate' always success).
Not sure how, but your response isn't threaded with your
original
posting. Makes it difficult to follow.
I'll fix later.
Posting a few bugs related to qemuProcessReconnectHelper - makes me
wonder "how" you get yourself into this situation. Is it like the
libnuma bug and you're creating scripts that have frequent and continual
libvirtd restarts, which I noted there isn't not really a normal situation.
libnuma bug is reproduced by scripts. But this bug is not, I did not reproduce it.
Only read the code, found may be enter this situation.
In any case, I think in this situation what we want is to call
virDomainObjListRemoveLocked instead of virDomainObjListRemove from
qemuDomainRemoveInactive because all the prerequisites for calling the
Locked function are true (the obj is locked/reffed and the list lock is
held).
Now this is *not* to say qemuDomainRemoveInactive should always call
the
*Locked variant. It means adding a "bool useLocked" to the API and then
passing true *only* from qemuProcessReconnectHelper and then of course
calling the virDomainObjListRemoveLocked variant when the bool is true.
Requires changing all the other callers to pass false.
Another preferred option:
Patch 1. Split up qemuDomainRemoveInactive, such that code from
"cfg =
virQEMUDriverGetConfig(driver);" through/including
"qemuExtDevicesCleanupHost(driver, vm->def);" plus the
"virObjectUnref(cfg);" is in a static helper
"qemuDomainRemoveInactiveCommon".
Patch 2. Create a qemuDomainRemoveInactiveLocked which is a copy of
qemuDomainRemoveInactive except that instead of calling
virDomainObjListRemove it calls virDomainObjListRemoveLocked.
Patch 3. Create a qemuDomainRemoveInactiveJobLocked which copies
qemuDomainRemoveInactiveJob except of course calling
virDomainObjListRemoveLocked.
Patch 4. Modify qemuProcessReconnectHelper to call
qemuDomainRemoveInactiveJobLocked.
John
Yes, your suggested pathes are preferred. Thanks, I'll give v3 patch.
---
Best wishes
Wang Yechao