On 14.12.2017 16:09, John Ferlan wrote:
On 12/14/2017 07:43 AM, Nikolay Shirokovskiy wrote:
On 14.12.2017 15:31, John Ferlan wrote:
On 12/14/2017 06:58 AM, Nikolay Shirokovskiy wrote:
Hi, all.
I looked over thread for this particular patch again and found resolution is we:
1. make a more sane cleanup order in libvirtd's main function (already done by [1]). 2. rewrite linked series [2] by introducing event loop closing callback (ok)
But there is no resolution on this patch itself if I am not mistaken and it is not pushed too.
LINKS
[1] [PATCH 0/5] libvirtd: Adjustments to startup and cleanup processing https://www.redhat.com/archives/libvir-list/2017-November/msg00293.html
The cover letter of this essentially states the cleanup is an alternative based upon the review of "this" series' 3/3 patch (IOW: this patch).
Sorry, somehow I lost this impornant notice on my read. I thought [1] was more about valueable but unrelated to this patch cleanup.
So I agree w/ Erik - the (last) memory strand that I have left on this indicates that the virHashRemoveAll isn't necessary in virNetDaemonClose because we've properly order the cleanup with the new patch 4 from [1].
Unfortunately it is not and looks like patch 4 is not pushed too. Anyway it can't help finishing RPC threads in rigth order to hypervisor drivers cleanup.
Follow the series... Patches 3 & 4 were not accepted, thus patch 5 needed to be adjusted in order to perform the Unref's in the cleanup: code rather than "inline" as patch 3 & 4 had done.
[2] [PATCH 0/4] libvirtd: fix hang on termination in qemu driver https://www.redhat.com/archives/libvir-list/2017-October/msg01134.html
I responded to this series' cover letter with:
https://www.redhat.com/archives/libvir-list/2017-November/msg00455.html
in order to attempt to move the conversations found in "this" series to that series rather than clouding "this" series with information belonging to that series.
So the question *now* is - did you find a condition/case with [1] patches applied, but [2] patches not applied (IOW: current top) where something isn't being Unref'd properly?
In a sense yes - virStateCleanup is still called before RPC threads are joined which causes crashes. It is not fixed by [1].
Nikolay
Now you've lost me. What are the back traces? and now does one reasonably reproduce? Are you trying to advocate here for [2] to be reviewed/accepted?
Sorry for that. Unfortunately back trace was only emailed in the first version of series [3]. The reproducer is there too. I'm not here for [2]. It addresses a different issue. However I want to make some progress on that series too. [3] https://www.redhat.com/archives/libvir-list/2017-September/msg01006.html
John
John
Nikolay
On 30.10.2017 14:14, John Ferlan wrote:
From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
The problem is incorrect order of qemu driver shutdown and shutdown of netserver threads that serve client requests (thru qemu driver particularly).
Net server threads are shutdown upon dispose which is triggered by last daemon object unref at the end of main function. At the same time qemu driver is shutdown earlier in virStateCleanup. As a result netserver threads see invalid driver object in the middle of request processing.
Let's move shutting down netserver threads earlier to virNetDaemonClose.
Note: order of last daemon unref and virStateCleanup is introduced in 85c3a182 for a valid reason.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetdaemon.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c index 8c21414897..33bd8e3b06 100644 --- a/src/rpc/virnetdaemon.c +++ b/src/rpc/virnetdaemon.c @@ -881,6 +881,7 @@ virNetDaemonClose(virNetDaemonPtr dmn) virObjectLock(dmn);
virHashForEach(dmn->servers, daemonServerClose, NULL); + virHashRemoveAll(dmn->servers);
virObjectUnlock(dmn); }