On Mon, Oct 30, 2017 at 07:14:39AM -0400, John Ferlan wrote: > From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > > 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. > I must say I don't believe that's true.  Reading it back, that patch is wrong IMHO.  I haven't gone through all the details of it and I don't remember them from when I rewrote part of it, but the way this should be handled is different. The way you can clearly identify such issues is when you see that one thread determines the validity of data (pointer in this case).  This must never happen.  That means that the pointer is used from more places than how many references that object has.  However each of the pointers for such object should have their own reference. So the problem is probably somewhere else.

> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > --- > 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); > If I get this correctly, you are removing the servers so that their workers get cleaned up, but that should be done in a separate step.  Most probably what daemonServerClose should be doing.  This is a workaround, but not a proper fix.

If that's not true than the previous commit mentioned here should also be fixed differently.

So this is a clear NACK from my POV. >     virObjectUnlock(dmn); > } > --  > 2.13.6 > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On 10/31/2017 02:54 AM, Nikolay Shirokovskiy wrote: > > > On 30.10.2017 19:21, Martin Kletzander wrote: >> On Mon, Oct 30, 2017 at 07:14:39AM -0400, John Ferlan wrote: >>> From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>> >>> 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. >>> >> >> I must say I don't believe that's true.  Reading it back, that patch is >> wrong IMHO.  I haven't gone through all the details of it and I don't >> remember them from when I rewrote part of it, but the way this should >> be handled is different. >> >> The way you can clearly identify such issues is when you see that one >> thread determines the validity of data (pointer in this case).  This >> must never happen.  That means that the pointer is used from more places >> than how many references that object has.  However each of the pointers >> for such object should have their own reference. >> >> So the problem is probably somewhere else. > > If I understand you correctly we can fix issue in 85c3a182 in > a differenct way. Like adding reference to daemon for every > driver that uses it for shutdown inhibition. It will require to > pass unref function to driver init function or a bit of wild casting > to virObjectPtr for unref. Not sure it is worth it in this case. caveat: I'm still in a post KVM Forum travel brain fog... Perhaps a bit more simple than that... Since the 'inhibitCallback' and the 'inhibitOpaque' are essentially the mechanism that would pass/use @dmn, then it'd be up to the inhibitCallback to add/remove the reference with the *given* that the inhibitOpaque is a lockable object anyway... Thus, virNetDaemonAddShutdownInhibition could use virObjectRef and virNetDaemonRemoveShutdownInhibition could use virObjectUnref... The "catch" is that for Shutdown the Unref would need to go after Unlock. I believe that then would "replicate" the virObjectRef done in daemonStateInit and the virObjectUnref done at the end of daemonRunStateInit with respect to "some outside thread" being able to use @dmn and not have it be Unref'd for the last time at any point in time until the "consumer" was done with it. Moving the Unref to after all the StateCleanup calls were done works because it accomplishesd the same/similar task, but just held onto @dmn longer because the original implementation didn't properly reference dmn when it was being used for some other consumer/thread. Of course that led to other problems which this series is addressing and I'm not clear yet as to the impact vis-a-vis your StateShutdown series.

> > Anyway the initical conditions for current patch stay the same - > daemon object will be unrefed after state drivers cleanup and > RPC requests could deal with invalid state driver objects. > >> >>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>> --- >>> 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); >>> >> >> If I get this correctly, you are removing the servers so that their workers get >> cleaned up, but that should be done in a separate step.  Most probably what >> daemonServerClose should be doing.  This is a workaround, but not a proper fix. Are you sure? The daemonServerClose is the callback for virHashForEach which means the table->iterating would be set thus preventing daemonServerClose from performing a virHashRemoveEntry of the server element from the list. So this is to me the right fix. John > > Well, original patch has a different approach for stopping RPC calls (see [1]) close > to what you suggest. I think in this case it is matter of taste because there are > no usecases which help us to prefer one way over another so I'm ok with both of them. > > [1] https://www.redhat.com/archives/libvir-list/2017-September/msg00999.html > [2] https://www.redhat.com/archives/libvir-list/2017-October/msg00643.html (thread dicussing [1]) > >> If that's not true than the previous commit mentioned here should also be fixed >> differently. > > Sorry I don't understand it. Can you elaborate on this point? > > > Nikolay >> >> So this is a clear NACK from my POV. >> >>>     virObjectUnlock(dmn); >>> } >>> --  >>> 2.13.6 >>> >>> -- >>> libvir-list mailing list >>> libvir-list(a)redhat.com >>> https://www.redhat.com/mailman/listinfo/libvir-list >> >> >> -- >> libvir-list mailing list >> libvir-list(a)redhat.com >> https://www.redhat.com/mailman/listinfo/libvir-list >>

On Thu, Nov 02, 2017 at 10:49:35AM +0300, Nikolay Shirokovskiy wrote: > > > On 01.11.2017 21:51, John Ferlan wrote: >> >> >> On 10/31/2017 02:54 AM, Nikolay Shirokovskiy wrote: >>> >>> >>> On 30.10.2017 19:21, Martin Kletzander wrote: >>>> On Mon, Oct 30, 2017 at 07:14:39AM -0400, John Ferlan wrote: >>>>> From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>>>> >>>>> 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. >>>>> >>>> >>>> I must say I don't believe that's true.  Reading it back, that patch is >>>> wrong IMHO.  I haven't gone through all the details of it and I don't >>>> remember them from when I rewrote part of it, but the way this should >>>> be handled is different. >>>> >>>> The way you can clearly identify such issues is when you see that one >>>> thread determines the validity of data (pointer in this case).  This >>>> must never happen.  That means that the pointer is used from more places >>>> than how many references that object has.  However each of the pointers >>>> for such object should have their own reference. >>>> >>>> So the problem is probably somewhere else. >>> >>> If I understand you correctly we can fix issue in 85c3a182 in >>> a differenct way. Like adding reference to daemon for every >>> driver that uses it for shutdown inhibition. It will require to >>> pass unref function to driver init function or a bit of wild casting >>> to virObjectPtr for unref. Not sure it is worth it in this case. >> >> caveat: I'm still in a post KVM Forum travel brain fog... >> >> Perhaps a bit more simple than that... Since the 'inhibitCallback' and >> the 'inhibitOpaque' are essentially the mechanism that would pass/use >> @dmn, then it'd be up to the inhibitCallback to add/remove the reference >> with the *given* that the inhibitOpaque is a lockable object anyway... >> >> Thus, virNetDaemonAddShutdownInhibition could use virObjectRef and >> virNetDaemonRemoveShutdownInhibition could use virObjectUnref... The >> "catch" is that for Shutdown the Unref would need to go after Unlock. >> >> I believe that then would "replicate" the virObjectRef done in >> daemonStateInit and the virObjectUnref done at the end of >> daemonRunStateInit with respect to "some outside thread" being able to >> use @dmn and not have it be Unref'd for the last time at any point in >> time until the "consumer" was done with it. >> >> Moving the Unref to after all the StateCleanup calls were done works >> because it accomplishesd the same/similar task, but just held onto @dmn >> longer because the original implementation didn't properly reference dmn >> when it was being used for some other consumer/thread. Of course that >> led to other problems which this series is addressing and I'm not clear >> yet as to the impact vis-a-vis your StateShutdown series. > > Ok, 85c3a182 can be rewritten this way. It is more staightforward to > ref/unref by consumer thread instead of relying on consumer structure > (in this case 85c3a182 relies upon the fact that after virStateCleanup > no hypervisor driver would use @dmn object). > That's what reference counting is for, each consumer should have its reference. > But we still have the issues address by this patch series and state > shutdown series because the order in which @dmn and other objects > will be disposed would be same for 85c3a182 and proposed 85c3a182 > replacement. > Let me summarize below, I hope I'll touch on all the points.  It's hard to follow since I'm currently reading 3 or more threads at the same time :) >>>>> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >>>>> --- >>>>> 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); >>>>> >>>> >>>> If I get this correctly, you are removing the servers so that their workers get >>>> cleaned up, but that should be done in a separate step.  Most probably what >>>> daemonServerClose should be doing.  This is a workaround, but not a proper fix. >> >> Are you sure?  The daemonServerClose is the callback for virHashForEach >> which means the table->iterating would be set thus preventing >> daemonServerClose from performing a virHashRemoveEntry of the server >> element from the list. >> This just makes the window of opportunity (between daemonServerClose() and the actual removal of the virNetServerPtr from the hash) smaller. That's why I don't see it as a fix, rather as a workaround. What I think should be done is the order of what's done with services, servers, drivers, daemon, workers, etc. I read the other threds and I think we're on the same note here, it's just that there is lot of confusion and we're all approaching it differently. So first let's agree on what should be done when virNetDaemonClose() is called and the loop in virNetDaemonRun() exits.  My opinion it is: 1) Stop accepting new calls and services:    virNetServerServiceToggle(false);

2) Wait for all the workers to finish:    First part of virThreadPoolFree()

3) Kill off clients:    virNetServerClientClose() 4) Clean up drivers:    virStateCleanup()

5) Clean up services, servers, daemon, etc.    including the second part of virThreadPoolFree() The last thing is what should be done in virNetDaemonDispose(), but unfortunately we (mis)use it for more than that.  Numbers 1-4 should be, IMO in virNet{Daemon,Server}Close(). Would this solve your problem?  I mean I'm not against more reference counting, it should be done properly, but I think the above would do the trick and also make sense from the naming point of view. Since both the daemon and server will probably not ever be consistent after calling the Close function, it might make sense to just include virNetDaemonClose() into virNetDaemonDispose() and I'm not against that. But the order of the steps should still be as described above IMO.

Let me know what you think.  And don't forget to have a nice day! Martin P.S.: The order of clean ups in libvirtd's main() is absolutely terrible and      unreadable.  Also, even though I guess it goes without saying, if proper      reference counting is in place, the order of virObjectUnref()s and      VIR_FREE()s should not matter.  I think it chould be changed like this:

diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c index 589b32192e3d..3cc14ed20987 100644 --- i/daemon/libvirtd.c +++ w/daemon/libvirtd.c @@ -1500,14 +1500,15 @@ int main(int argc, char **argv) {  cleanup:     virNetlinkEventServiceStopAll(); -    virObjectUnref(remoteProgram); -    virObjectUnref(lxcProgram); -    virObjectUnref(qemuProgram); -    virObjectUnref(adminProgram);     virNetDaemonClose(dmn); -    virObjectUnref(srv); -    virObjectUnref(srvAdm); + +    if (driversInitialized) { +        driversInitialized = false; +        virStateCleanup(); +    } +     virNetlinkShutdown(); +     if (statuswrite != -1) {         if (ret != 0) {             /* Tell parent of daemon what failed */ @@ -1521,6 +1522,14 @@ int main(int argc, char **argv) {     if (pid_file_fd != -1)         virPidFileReleasePath(pid_file, pid_file_fd); +    virObjectUnref(remoteProgram); +    virObjectUnref(lxcProgram); +    virObjectUnref(qemuProgram); +    virObjectUnref(adminProgram); +    virObjectUnref(srv); +    virObjectUnref(srvAdm); +    virObjectUnref(dmn); +     VIR_FREE(sock_file);     VIR_FREE(sock_file_ro);     VIR_FREE(sock_file_adm); @@ -1530,13 +1539,5 @@ int main(int argc, char **argv) {     daemonConfigFree(config); -    if (driversInitialized) { -        driversInitialized = false; -        virStateCleanup(); -    } -    /* Now that the hypervisor shutdown inhibition functions that use -     * 'dmn' as a parameter are done, we can finally unref 'dmn' */ -    virObjectUnref(dmn); -     return ret; } --

On Thu, Nov 02, 2017 at 10:49:35AM +0300, Nikolay Shirokovskiy wrote: > > > On 01.11.2017 21:51, John Ferlan wrote: > > > > > > On 10/31/2017 02:54 AM, Nikolay Shirokovskiy wrote: > > > > > > > > > On 30.10.2017 19:21, Martin Kletzander wrote: > > > > On Mon, Oct 30, 2017 at 07:14:39AM -0400, John Ferlan wrote: > > > > > From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > > > > > > > > > > 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. > > > > > > > > > > > > > I must say I don't believe that's true.  Reading it back, that patch is > > > > wrong IMHO.  I haven't gone through all the details of it and I don't > > > > remember them from when I rewrote part of it, but the way this should > > > > be handled is different. > > > > > > > > The way you can clearly identify such issues is when you see that one > > > > thread determines the validity of data (pointer in this case).  This > > > > must never happen.  That means that the pointer is used from more places > > > > than how many references that object has.  However each of the pointers > > > > for such object should have their own reference. > > > > > > > > So the problem is probably somewhere else. > > > > > > If I understand you correctly we can fix issue in 85c3a182 in > > > a differenct way. Like adding reference to daemon for every > > > driver that uses it for shutdown inhibition. It will require to > > > pass unref function to driver init function or a bit of wild casting > > > to virObjectPtr for unref. Not sure it is worth it in this case. > > > > caveat: I'm still in a post KVM Forum travel brain fog... > > > > Perhaps a bit more simple than that... Since the 'inhibitCallback' and > > the 'inhibitOpaque' are essentially the mechanism that would pass/use > > @dmn, then it'd be up to the inhibitCallback to add/remove the reference > > with the *given* that the inhibitOpaque is a lockable object anyway... > > > > Thus, virNetDaemonAddShutdownInhibition could use virObjectRef and > > virNetDaemonRemoveShutdownInhibition could use virObjectUnref... The > > "catch" is that for Shutdown the Unref would need to go after Unlock. > > > > I believe that then would "replicate" the virObjectRef done in > > daemonStateInit and the virObjectUnref done at the end of > > daemonRunStateInit with respect to "some outside thread" being able to > > use @dmn and not have it be Unref'd for the last time at any point in > > time until the "consumer" was done with it. > > > > Moving the Unref to after all the StateCleanup calls were done works > > because it accomplishesd the same/similar task, but just held onto @dmn > > longer because the original implementation didn't properly reference dmn > > when it was being used for some other consumer/thread. Of course that > > led to other problems which this series is addressing and I'm not clear > > yet as to the impact vis-a-vis your StateShutdown series. > > Ok, 85c3a182 can be rewritten this way. It is more staightforward to > ref/unref by consumer thread instead of relying on consumer structure > (in this case 85c3a182 relies upon the fact that after virStateCleanup > no hypervisor driver would use @dmn object). > That's what reference counting is for, each consumer should have its reference. > But we still have the issues address by this patch series and state > shutdown series because the order in which @dmn and other objects > will be disposed would be same for 85c3a182 and proposed 85c3a182 > replacement. > Let me summarize below, I hope I'll touch on all the points. It's hard to follow since I'm currently reading 3 or more threads at the same time :) > > > > > Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; > > > > > --- > > > > > 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); > > > > > > > > > > > > > If I get this correctly, you are removing the servers so that their workers get > > > > cleaned up, but that should be done in a separate step.  Most probably what > > > > daemonServerClose should be doing.  This is a workaround, but not a proper fix. > > > > Are you sure? The daemonServerClose is the callback for virHashForEach > > which means the table->iterating would be set thus preventing > > daemonServerClose from performing a virHashRemoveEntry of the server > > element from the list. > > This just makes the window of opportunity (between daemonServerClose() and the actual removal of the virNetServerPtr from the hash) smaller. That's why I don't see it as a fix, rather as a workaround. What I think should be done is the order of what's done with services, servers, drivers, daemon, workers, etc. I read the other threds and I think we're on the same note here, it's just that there is lot of confusion and we're all approaching it differently. So first let's agree on what should be done when virNetDaemonClose() is called and the loop in virNetDaemonRun() exits. My opinion it is: 1) Stop accepting new calls and services: virNetServerServiceToggle(false); 2) Wait for all the workers to finish: First part of virThreadPoolFree()

3) Kill off clients: virNetServerClientClose() 4) Clean up drivers: virStateCleanup() 5) Clean up services, servers, daemon, etc. including the second part of virThreadPoolFree() The last thing is what should be done in virNetDaemonDispose(), but unfortunately we (mis)use it for more than that. Numbers 1-4 should be, IMO in virNet{Daemon,Server}Close(). Would this solve your problem? I mean I'm not against more reference counting, it should be done properly, but I think the above would do the trick and also make sense from the naming point of view. Since both the daemon and server will probably not ever be consistent after calling the Close function, it might make sense to just include virNetDaemonClose() into virNetDaemonDispose() and I'm not against that. But the order of the steps should still be as described above IMO. Let me know what you think. And don't forget to have a nice day! Martin P.S.: The order of clean ups in libvirtd's main() is absolutely terrible and unreadable. Also, even though I guess it goes without saying, if proper reference counting is in place, the order of virObjectUnref()s and VIR_FREE()s should not matter. I think it chould be changed like this: diff --git i/daemon/libvirtd.c w/daemon/libvirtd.c index 589b32192e3d..3cc14ed20987 100644 --- i/daemon/libvirtd.c +++ w/daemon/libvirtd.c @@ -1500,14 +1500,15 @@ int main(int argc, char **argv) { cleanup: virNetlinkEventServiceStopAll(); - virObjectUnref(remoteProgram); - virObjectUnref(lxcProgram); - virObjectUnref(qemuProgram); - virObjectUnref(adminProgram); virNetDaemonClose(dmn); - virObjectUnref(srv); - virObjectUnref(srvAdm); + + if (driversInitialized) { + driversInitialized = false; + virStateCleanup(); + } + virNetlinkShutdown(); + if (statuswrite != -1) { if (ret != 0) { /* Tell parent of daemon what failed */ @@ -1521,6 +1522,14 @@ int main(int argc, char **argv) { if (pid_file_fd != -1) virPidFileReleasePath(pid_file, pid_file_fd); + virObjectUnref(remoteProgram); + virObjectUnref(lxcProgram); + virObjectUnref(qemuProgram); + virObjectUnref(adminProgram); + virObjectUnref(srv); + virObjectUnref(srvAdm); + virObjectUnref(dmn); + VIR_FREE(sock_file); VIR_FREE(sock_file_ro); VIR_FREE(sock_file_adm); @@ -1530,13 +1539,5 @@ int main(int argc, char **argv) { daemonConfigFree(config); - if (driversInitialized) { - driversInitialized = false; - virStateCleanup(); - } - /* Now that the hypervisor shutdown inhibition functions that use - * 'dmn' as a parameter are done, we can finally unref 'dmn' */ - virObjectUnref(dmn); - return ret; }

From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; 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(a)redhat.com&gt; --- 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); }

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).

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].

> > [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?

John > > Nikolay > > On 30.10.2017 14:14, John Ferlan wrote: >> From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >> >> 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(a)redhat.com&gt; >> --- >> 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); >> } >>

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?

John >> >> John >> >>> >>> Nikolay >>> >>> On 30.10.2017 14:14, John Ferlan wrote: >>>> From: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; >>>> >>>> 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(a)redhat.com&gt; >>>> --- >>>> 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); >>>> } >>>>

[...] >> >> So short story made really long, I think the best course of action will >> be to add this patch and reorder the Unref()'s (adminProgram thru srv, >> but not dmn). It seems to resolve these corner cases, but I'm also open >> to other suggestions. Still need to think about it some more too before >> posting any patches. >> >> > Hi. > > I'm not grasp the whole picture yet but I've managed to find out what > triggered the crash. It is not 2f3054c22 where you reordered unrefs but > 1fd1b766105 which moves events unregistering from netserver client closing to > netservec client disposing. Before 1fd1b766105 we don't have crash > because clients simply do not get disposed. Oh yeah, that one.... But considering Erik's most recent response in this overall thread vis-a-vis the separation of "close" vs. "dispose" and the timing of each w/r/t Unref and Free, I think having the call to remoteClientFreePrivateCallbacks in remoteClientCloseFunc is perhaps better than in remoteClientFreeFunc. > > As to fixing the crash with this patch I thinks its is coincidence. I want > do dispose netservers early to join rpc threads and it turns out that > disposing also closing clients too and this fixes the problem. > > Nikolay > With Cedric's patch in place, the virt-manager client issue is fixed. So that's goodness. If I then add the sleep (or usleep) into qemuConnectGetAllDomainStats as noted in what started this all, then I can either get libvirtd to crash dereferencing a NULL driver pointer or (my favorite) hang with two threads stuck waiting: (gdb) t a a bt Thread 5 (Thread 0x7fffe535b700 (LWP 15568)): #0 0x00007ffff3dc909d in __lll_lock_wait () from /lib64/libpthread.so.0 #1 0x00007ffff3dc1e23 in pthread_mutex_lock () from /lib64/libpthread.so.0 #2 0x00007ffff7299a15 in virMutexLock (m=<optimized out>) at util/virthread.c:89 #3 0x00007fffc760621e in qemuDriverLock (driver=0x7fffbc190510) at qemu/qemu_conf.c:100 #4 virQEMUDriverGetConfig (driver=driver@entry=0x7fffbc190510) at qemu/qemu_conf.c:1002 #5 0x00007fffc75dfa89 in qemuDomainObjBeginJobInternal ( driver=driver@entry=0x7fffbc190510, obj=obj@entry=0x7fffbc3bcd60, job=job@entry=QEMU_JOB_QUERY, asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE) at qemu/qemu_domain.c:4690 #6 0x00007fffc75e3b2b in qemuDomainObjBeginJob ( driver=driver@entry=0x7fffbc190510, obj=obj@entry=0x7fffbc3bcd60, job=job@entry=QEMU_JOB_QUERY) at qemu/qemu_domain.c:4842 #7 0x00007fffc764f744 in qemuConnectGetAllDomainStats (conn=0x7fffb80009a0, doms=<optimized out>, ndoms=<optimized out>, stats=<optimized out>, retStats=0x7fffe535aaf0, flags=<optimized out>) at qemu/qemu_driver.c:20219 #8 0x00007ffff736430a in virDomainListGetStats (doms=0x7fffa8000950, stats=0, retStats=retStats@entry=0x7fffe535aaf0, flags=0) at libvirt-domain.c:11595 #9 0x000055555557948d in remoteDispatchConnectGetAllDomainStats ( server=<optimized out>, msg=<optimized out>, ret=0x7fffa80008e0, args=0x7fffa80008c0, rerr=0x7fffe535abf0, client=<optimized out>) at remote.c:6538 #10 remoteDispatchConnectGetAllDomainStatsHelper (server=<optimized out>, client=<optimized out>, msg=<optimized out>, rerr=0x7fffe535abf0, args=0x7fffa80008c0, ret=0x7fffa80008e0) at remote_dispatch.h:615 #11 0x00007ffff73bf59c in virNetServerProgramDispatchCall (msg=0x55555586cdd0, client=0x55555586bea0, server=0x55555582ed90, prog=0x555555869190) at rpc/virnetserverprogram.c:437 #12 virNetServerProgramDispatch (prog=0x555555869190, server=server@entry=0x55555582ed90, client=0x55555586bea0, msg=0x55555586cdd0) at rpc/virnetserverprogram.c:307 #13 0x00005555555a9318 in virNetServerProcessMsg (msg=<optimized out>, prog=<optimized out>, client=<optimized out>, srv=0x55555582ed90) at rpc/virnetserver.c:148 #14 virNetServerHandleJob (jobOpaque=<optimized out>, opaque=0x55555582ed90) at rpc/virnetserver.c:169 #15 0x00007ffff729a521 in virThreadPoolWorker ( opaque=opaque@entry=0x55555583aa40) at util/virthreadpool.c:167 #16 0x00007ffff7299898 in virThreadHelper (data=<optimized out>) at util/virthread.c:206 #17 0x00007ffff3dbf36d in start_thread () from /lib64/libpthread.so.0 #18 0x00007ffff3af3e1f in clone () from /lib64/libc.so.6 Thread 1 (Thread 0x7ffff7ef9d80 (LWP 15561)): #0 0x00007ffff3dc590b in pthread_cond_wait@(a)GLIBC_2.3.2 () ---Type <return> to continue, or q <return> to quit--- from /lib64/libpthread.so.0 #1 0x00007ffff7299af6 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 #2 0x00007ffff729a760 in virThreadPoolFree (pool=<optimized out>) at util/virthreadpool.c:290 #3 0x00005555555a8ec2 in virNetServerDispose (obj=0x55555582ed90) at rpc/virnetserver.c:767 #4 0x00007ffff727923b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:356 #5 0x00007ffff724f069 in virHashFree (table=<optimized out>) at util/virhash.c:318 #6 0x00007ffff73b8295 in virNetDaemonDispose (obj=0x55555582eb10) at rpc/virnetdaemon.c:105 #7 0x00007ffff727923b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:356 #8 0x000055555556f2eb in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1524 (gdb) Of course this could be a red herring because sleep/usleep and the condition handling nature of these jobs could be interfering with one another. Still adding the "virHashRemoveAll(dmn->servers);" into virNetDaemonClose doesn't help the situation as I can still either crash randomly or hang, so I'm less convinced this would really fix anything. It does change the "nature" of the hung thread stack trace though, as the second thread is now:

Thread 1 (Thread 0x7ffff7ef9d80 (LWP 20159)): #0 0x00007ffff3dc590b in pthread_cond_wait@(a)GLIBC_2.3.2 () ---Type <return> to continue, or q <return> to quit--- from /lib64/libpthread.so.0 #1 0x00007ffff7299b06 in virCondWait (c=<optimized out>, m=<optimized out>) at util/virthread.c:154 #2 0x00007ffff729a770 in virThreadPoolFree (pool=<optimized out>) at util/virthreadpool.c:290 #3 0x00005555555a8ec2 in virNetServerDispose (obj=0x55555582ed90) at rpc/virnetserver.c:767 #4 0x00007ffff727924b in virObjectUnref (anyobj=<optimized out>) at util/virobject.c:356 #5 0x000055555556f2e3 in main (argc=<optimized out>, argv=<optimized out>) at libvirtd.c:1523 (gdb) So we still haven't found the "root cause", but I think Erik is on to something in the other part of this thread. I'll go there. John