On Mon, Oct 30, 2017 at 07:14:38AM -0400, John Ferlan wrote:

From: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>

After virNetDaemonAddServerPostExec call in virtlogd we should have netserver refcount set to 2. One goes to netdaemon servers hashtable and one goes to virtlogd own reference to netserver. Let's add missing increment in virNetDaemonAddServerPostExec itself while holding daemon lock.

We also have to unref new extra ref after virtlockd call to virNetDaemonAddServerPostExec since it mgmt of the @srv would be assumed to be handled by the Hash Table.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_daemon.c | 1 + src/rpc/virnetdaemon.c | 1 + 2 files changed, 2 insertions(+)

ACK to this one as well.

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:

On 02.11.2017 19:32, Martin Kletzander wrote:

...

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

This not really necessary as this has effect only if loop is running.

Sure, good point.

...

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

I was thinking of splitting this function to finish/free too. After finish it is not really usable because there are no more threads in pool anymore so we have to introduce state finished and check it in every thread pool api function to be safe. So this will introduce a bit of complexity only for the sake of some scheme IMO.

Yeah, it's similar to the daemon when you are between close and dispose, but for the daemon we need it. What function do you think we would need to check this for? The finish would end all the threads and since the event loop is not running no new jobs could be posted.

...

3) Kill off clients:    virNetServerClientClose()

4) Clean up drivers:    virStateCleanup()

I think it is better for net daemon/servers not to know about hypervisor drivers directly.

Sure, I was only talking about the libvirtd for the sake of simplicity.

...

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.

This won't work IMO. If we are already in virNetDaemonDispose then @dmn object is invalid but we don't yet call virStateCleanup yet (if it is moved to virNetDaemonClose) and this is the problem 85c3a182 dealt with.

It would solve it if order above is followed. Sure, virNetDaemon should not know about any drivers, but we can simply register a cleanup callback for the daemon which the Dispose function will call after the thread pools are cleaned up (just thought of that now).

...

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:

It makes sense that in cleanup section we can first make all cleanup and then all dispose. It is only not clear to me can we call virStateCleanup before virNetlinkShutdown. Also I would keep comment for @dmn unref until proper refcount for @dmn is implemented.

I don't think virStateCleanup() and virNetlinkShutdown() are dependent. The comment can stay, I was just trying to show that it's also a mess. What I would like, actually, is to have the daemon running with all the references and pointers (that the main() function doesn't need anymore) freed, but both this and what I suggested below is unrelated to what we're trying to fix, I don't know why I started talking about it.

On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:

On 03.11.2017 11:42, Martin Kletzander wrote:

...

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:

...

On 02.11.2017 19:32, Martin Kletzander wrote:

...

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.

Trying to catch up, but this thread has gone beyond where I was previously. I still have to push the first two patches of this series... Yes, libvirtd cleanup path is awful and there's quite a few "issues" that have cropped up because previous patches didn't take care of refcnt'ing properly and just "added" cleanup "somewhere" in the list of things to be cleaned up... Fortunately to a degree I think this is still all edge condition type stuff, but getting it right would be good... For me theory has always been to cleanup in the inverse order that something was created - for sure libvirtd doesn't follow that at all. For libvirtd the shutdown/cleanup logic just seemed too haphazard and fixing never made it to the top of the list of things to think about. Another "caveat" in libvirtd cleanup logic is that driversInitialized is set as part of a thread that could be running or have run if the code enters the cleanup: path as a result of virNetlinkEventServiceStart failure before we enter the event loop (e.g. virNetDaemonRun). Still like Erik - I do like Martin's suggestion and would like to see it as a formal patch - although perhaps with a few tweaks. IIRC, this particular patch "worked" because by removing the dmn->servers refcnt earlier made sure that we didn't have to wait for virObjectUnref(dmn) to occur after virStateCleanup.

...

...

...

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

This not really necessary as this has effect only if loop is running.

Sure, good point.

...

...

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

I was thinking of splitting this function to finish/free too. After finish it is not really usable because there are no more threads in pool anymore so we have to introduce state finished and check it in every thread pool api function to be safe. So this will introduce a bit of complexity only for the sake of some scheme IMO.

Yeah, it's similar to the daemon when you are between close and dispose, but for the daemon we need it.

What function do you think we would need to check this for?  The finish would end all the threads and since the event loop is not running no new jobs could be posted.

virThreadPoolSendJob is already good in this respect as it checks @quit at the beginning. Looks like we need only to add same check to virThreadPoolSetParameters. So not so much complexity.

I can imagine thread pool function being called from another thread pools thread. AFAIK there are not such situation though. So just to be on a safe side.

...

...

...

3) Kill off clients:    virNetServerClientClose()

4) Clean up drivers:    virStateCleanup()

I think it is better for net daemon/servers not to know about hypervisor drivers directly.

Sure, I was only talking about the libvirtd for the sake of simplicity.

...

...

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.

This won't work IMO. If we are already in virNetDaemonDispose then @dmn object is invalid but we don't yet call virStateCleanup yet (if it is moved to virNetDaemonClose) and this is the problem 85c3a182 dealt with.

It would solve it if order above is followed.  Sure, virNetDaemon should not

It is ok if virStateCleanup is called from virNetDaemonClose somehow. I was only against calling virNetDaemonClose from virNetDaemonDispose in this case.

...

know about any drivers, but we can simply register a cleanup callback for the daemon which the Dispose function will call after the thread pools are cleaned up (just thought of that now).

Well to me it is too generic for this case. We can code this order just as order of statements.

1. close netservers (which imply finishing rpc thread pool) 2. close hypervisor drivers 3. unref @dmn

Or we can use refcounts and take point 3 out of consideration. Also there is a patch series [1] that introduces 0 step to help 1 step to finish actually in the situation when loop is down.

0. shutdown hypervirsor drivers.

[1] https://www.redhat.com/archives/libvir-list/2017-October/msg01134.html

Right this still exists out there, but it's hard enough to focus on the issues being discussed here without adding more into the mix. If we can get the shutdown order right as Martin has proposed, then address this afterwards, it'll perhaps make things easier.

...

...

...

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:

It makes sense that in cleanup section we can first make all cleanup and then all dispose. It is only not clear to me can we call virStateCleanup before virNetlinkShutdown. Also I would keep comment for @dmn unref until proper refcount for @dmn is implemented.

I don't think virStateCleanup() and virNetlinkShutdown() are dependent.  The comment can stay, I was just trying to show that it's also a mess.  What I would like, actually, is to have the daemon running with all the references and pointers (that the main() function doesn't need anymore) freed, but both this and what I suggested below is unrelated to what we're trying to fix, I don't know why I started talking about it.

So you cut off Martin's patch here, but if one essentially did inverse order of startup they'd get the following: (the // comments are my notes about startup order oddity). cleanup: virNetDaemonClose(dmn); virNetlinkEventServiceStopAll(); if (driversInitialized) { /* Set via daemonRunStateInit in daemonStateInit */ driversInitialized = false; virStateCleanup(); } virObjectUnref(adminProgram); virObjectUnref(srvAdm); virObjectUnref(qemuProgram); virObjectUnref(lxcProgram); virObjectUnref(remoteProgram); // FWIW: Not exact here, but the order of startup should be // changed since srv is added to dmn, so realistically srv // depends on dmn existence. virObjectUnref(srv); virObjectUnref(dmn); virNetlinkShutdown(); // This one is also strange - seems startup should move the claiming // to before run_dir setup after forking into background. if (pid_file_fd != -1) virPidFileReleasePath(pid_file, pid_file_fd); VIR_FREE(run_dir); if (statuswrite != -1) { if (ret != 0) { /* Tell parent of daemon what failed */ char status = ret; while (write(statuswrite, &status, 1) == -1 && errno == EINTR) ; } VIR_FORCE_CLOSE(statuswrite); } VIR_FREE(sock_file); VIR_FREE(sock_file_ro); VIR_FREE(sock_file_adm); VIR_FREE(pid_file); VIR_FREE(remote_config_file); daemonConfigFree(config); Yes, this does differ from Martin's order... John

On 11/07/2017 04:46 AM, Martin Kletzander wrote:

On Mon, Nov 06, 2017 at 03:20:13PM -0500, John Ferlan wrote:

...

On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:

...

On 03.11.2017 11:42, Martin Kletzander wrote:

...

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote:

...

On 02.11.2017 19:32, Martin Kletzander wrote:

...

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.

Trying to catch up, but this thread has gone beyond where I was previously. I still have to push the first two patches of this series...

Yes, libvirtd cleanup path is awful and there's quite a few "issues" that have cropped up because previous patches didn't take care of refcnt'ing properly and just "added" cleanup "somewhere" in the list of things to be cleaned up... Fortunately to a degree I think this is still all edge condition type stuff, but getting it right would be good...

For me theory has always been to cleanup in the inverse order that something was created - for sure libvirtd doesn't follow that at all. For libvirtd the shutdown/cleanup logic just seemed too haphazard and fixing never made it to the top of the list of things to think about.

Another "caveat" in libvirtd cleanup logic is that driversInitialized is set as part of a thread that could be running or have run if the code enters the cleanup: path as a result of virNetlinkEventServiceStart failure before we enter the event loop (e.g. virNetDaemonRun).

Still like Erik - I do like Martin's suggestion and would like to see it as a formal patch - although perhaps with a few tweaks.

IIRC, this particular patch "worked" because by removing the dmn->servers refcnt earlier made sure that we didn't have to wait for virObjectUnref(dmn) to occur after virStateCleanup.

Would it be to daring to ask you guys to send one series that includes all the related patchsets?  We can then discuss there, it would be easier to follow as well.  I don't want anyone to do any extra work, but we might save some review bandwidth by resending those patches in one series.  Or maybe it's just me and I have a hard time keeping up with everything at the same time =)

Originally "this" crash fix was mixed in with the other series to add a shutdown state option, but I asked they be separate since they were IMO addressing different problems. To me this series was attempting to resolve a refcnt problem and a "latent" free of the dmn->servers that was IIRC attempting to be used before the state cleanup and the Unref(dmn) occurred to run the virHashFree in virNetDaemonDispose. The other series seems to be adding a state shutdown step to be run before state cleanup; however, if we alter the cleanup code in libvirtd does that make the state shutdown step unnecessary? I don't know and really didn't want to think about it until we got through thinking about the cleanup processing order. Still if one considers that state initialization is really two steps (init and auto start), then splitting cleanup into shutdown and cleanup seems reasonable, but I don't think affects whether we have a crash or latent usage of dmn->servers. I could be wrong, but it's so hard to tell. [...]

...

So you cut off Martin's patch here, but if one essentially did inverse order of startup they'd get the following: (the // comments are my notes about startup order oddity).

cleanup:    virNetDaemonClose(dmn);

   virNetlinkEventServiceStopAll();

   if (driversInitialized) {        /* Set via daemonRunStateInit in daemonStateInit */        driversInitialized = false;        virStateCleanup();    }

   virObjectUnref(adminProgram);    virObjectUnref(srvAdm);    virObjectUnref(qemuProgram);    virObjectUnref(lxcProgram);    virObjectUnref(remoteProgram);

   // FWIW: Not exact here, but the order of startup should be    // changed since srv is added to dmn, so realistically srv    // depends on dmn existence.    virObjectUnref(srv);

We can unref all of the above whenever we please.  @dmn keeps references for everything it needs, we just have @srv in order to set some things up, we can unref it right after we add it to @dmn.  The dependencies here are not the concern thanks to (hopefully) proper refcnt'ing.

OK - that's a bit more adjustment, but totally possible...

...

   virObjectUnref(dmn);

   virNetlinkShutdown();

   // This one is also strange - seems startup should move the claiming    // to before run_dir setup after forking into background.    if (pid_file_fd != -1)        virPidFileReleasePath(pid_file, pid_file_fd);

During start we should quit immediately, so it should be done before forking into background, otherwise init scripts will not know whether starting the service failed or not.

But don't we want to write the child pid into the pid_file and not the parent? Or more succinctly the grandchild pid... e.g. the call after the fork is: if ((pid_file_fd = virPidFileAcquirePath(pid_file, false, getpid())) < 0) { which essentially writes the getpid value into the pid_file... If we did this before the fork, then we'd be writing the parent pid into the file and that parent exits as does it's child, leaving the grandchild as the daemon (or am I reading all that wrong - it's quite possible ;-)) In any case, I don't mind posting something that alters the cleanup: logic - I wasn't sure if you were going to do that based on a couple of replies back in this thread... John

...

   VIR_FREE(run_dir);

Frees can be together, again, most of them could be freed before virNetDaemonRun since all the string are just used to initialize stuff and I believe it's strdup'd everywhere.

...

   if (statuswrite != -1) {        if (ret != 0) {            /* Tell parent of daemon what failed */            char status = ret;            while (write(statuswrite, &status, 1) == -1 &&                   errno == EINTR)                ;        }        VIR_FORCE_CLOSE(statuswrite);    }

   VIR_FREE(sock_file);    VIR_FREE(sock_file_ro);    VIR_FREE(sock_file_adm);

   VIR_FREE(pid_file);

   VIR_FREE(remote_config_file);    daemonConfigFree(config);

Yes, this does differ from Martin's order...

Sure, no problem, I just quickly sorted the lines by hand, I just wanted to show that the readability was bad as well.

...

John

On Tue, Nov 07, 2017 at 05:06:33PM +0100, Erik Skultety wrote:

On Tue, Nov 07, 2017 at 10:28:10AM -0500, John Ferlan wrote:

...

On 11/07/2017 04:46 AM, Martin Kletzander wrote:

...

On Mon, Nov 06, 2017 at 03:20:13PM -0500, John Ferlan wrote:

...

On 11/03/2017 05:20 AM, Nikolay Shirokovskiy wrote:

...

On 03.11.2017 11:42, Martin Kletzander wrote:

...

On Fri, Nov 03, 2017 at 11:07:36AM +0300, Nikolay Shirokovskiy wrote: > On 02.11.2017 19:32, Martin Kletzander wrote: >> 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.

Trying to catch up, but this thread has gone beyond where I was previously. I still have to push the first two patches of this series...

Yes, libvirtd cleanup path is awful and there's quite a few "issues" that have cropped up because previous patches didn't take care of refcnt'ing properly and just "added" cleanup "somewhere" in the list of things to be cleaned up... Fortunately to a degree I think this is still all edge condition type stuff, but getting it right would be good...

For me theory has always been to cleanup in the inverse order that something was created - for sure libvirtd doesn't follow that at all. For libvirtd the shutdown/cleanup logic just seemed too haphazard and fixing never made it to the top of the list of things to think about.

Another "caveat" in libvirtd cleanup logic is that driversInitialized is set as part of a thread that could be running or have run if the code enters the cleanup: path as a result of virNetlinkEventServiceStart failure before we enter the event loop (e.g. virNetDaemonRun).

Still like Erik - I do like Martin's suggestion and would like to see it as a formal patch - although perhaps with a few tweaks.

IIRC, this particular patch "worked" because by removing the dmn->servers refcnt earlier made sure that we didn't have to wait for virObjectUnref(dmn) to occur after virStateCleanup.

Would it be to daring to ask you guys to send one series that includes all the related patchsets?  We can then discuss there, it would be easier to follow as well.  I don't want anyone to do any extra work, but we might save some review bandwidth by resending those patches in one series.  Or maybe it's just me and I have a hard time keeping up with everything at the same time =)

Originally "this" crash fix was mixed in with the other series to add a shutdown state option, but I asked they be separate since they were IMO addressing different problems.

To me this series was attempting to resolve a refcnt problem and a "latent" free of the dmn->servers that was IIRC attempting to be used before the state cleanup and the Unref(dmn) occurred to run the virHashFree in virNetDaemonDispose.

Exactly, this was only about refcounting of @dmn, see below for @driver issue.

OK, sorry for that then, I followed up only more recently and honestly I'm getting pretty confused with how many thought threads this has so the context switches get more and more expensive.

...

The other series seems to be adding a state shutdown step to be run before state cleanup; however, if we alter the cleanup code in libvirtd does that make the state shutdown step unnecessary? I don't know and

Actually, no, the stateShutdown was addressing the issue with running <driver>StateCleanup while there still was an active worker thread that could potentially access the @driver object, in which case - SIGSEGV, but as I wrote in my response, I don't like the idea of having another state driver callback, as I feel it introduces a bit of ambiguity, since by just looking at the callback names, you can't have an idea, what the difference between stateCleanup and stateShutdown is. I think this should be handled as part of the final stage of leaving the event loop and simply register an appropriate callback to the handle, that way, you don't need to define a new state callback which would most likely used by a couple of drivers.

If stateShutdown is what makes everything go away, then why not? The names are pretty descriptive, shutdown is called when the daemon is shutting down and cleanup when you need to clean up after yourself. Used by only few drivers? Well, we can implement it everywhere and not make it make optional, but rather mandatory. If that is checked on the registration then we'll know right away when we missed something (aat runtime, but right after someone tries running it).

Erik

...

really didn't want to think about it until we got through thinking about the cleanup processing order. Still if one considers that state initialization is really two steps (init and auto start), then splitting cleanup into shutdown and cleanup seems reasonable, but I don't think affects whether we have a crash or latent usage of dmn->servers. I could be wrong, but it's so hard to tell.

[...]

Erik

[...]

...

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

This is a very tricky part which might get overly complex in order to be done properly. We currently don't allow the threads finish gracefully, IOW the connections have most likely been closed by the time the worker thread got to respond back to the client or the thread simply hung for some other reason in which case the daemon wouldn't even finish. So to give the threads a chance to finish properly, they need to be decommissioned while the event loop is still running otherwise there's no way for the worker to e.g. get an event back from QEMU's monitor, since there's noone to execute the appropriate callback for that anymore.

Actually, I was wrong here, qemuMonitorClose would wake the worker up with an error, so it wouldn't be stuck indefinitely. However, I'd still prefer the event loop to decommission the threads and do what Nikolay's qemuStateShutdown proposal is doing, since eventloop knows about all of its handles, so when it's finishing, it could simply run an appropriate cleanup callback on all of the handles, thus closing the qemuMonitor as well.

However, even if we did this and let's say we waited for a worker thread to finish its time-consuming job, you still have the problem I mentioned above with distinguishing really long time-consuming jobs from threads that are hung. So I'm not really sure whether any attempt to be terminating them gracefully is really worth the effort, given the circumstances, but at the same time I understand that the outcomes: the daemon might crash (it was terminating anyway) or it terminates properly but leaves a QEMU guest in an inconsistent way.

To your idea of splitting virThreadPoolFree, since we most likely have to stop the threads within the event loop, thus broadcast 'quit' as well as wait for them, I don't see a point in decoupling the function to killing and joining threads and then doing a bunch of VIR_FREEs and vir(Mutex|Cond)Destroys, that's just scraping of some leftovers, I think it makes sense the way it's now, even though the naming is not ideal, I agree with that and we should come up with something else. But this is just a matter of taste and very unimportant, of course I don't have a strong argument about why the approach would be bad, because it's not, I just don't see it worth the effort.

Erik

On Thu, Dec 14, 2017 at 02:58:30PM +0300, 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.

It's been a while and the thread became fairly large to track, correct me if I'm wrong but wouldn't the issue this patch is trying to solve disappear once we go with a reworked version of [2]? Erik

[...]

...

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

Ahh, sigh... I should have been more "proactive" in ensuring that I had gone back through the above test when two of the patches I had posted related to the changes in this code were not ACK'd. Those two patches are: https://www.redhat.com/archives/libvir-list/2017-November/msg00296.html and https://www.redhat.com/archives/libvir-list/2017-November/msg00297.html What those did was perform the Unref from libvirtd as soon as the AddServer or AddProgram added to the respective hash table (and of course incr the refcnt). The result is the Unref for each was done *prior* to the virNetDaemonClose and prior to the virStateCleanup which solves the late run after Close when Dispose runs. What jiggled my memory on this was today there was an OFTC IRC posting regarding a virt-manager crash (see pastebin from cbosdonnat at http://paste.opensuse.org/44522138) as a result of incorrect ordering of daemon unref and running virStateCleanup. A copy/paste of the subsequent conversation is: <cbosdonnat> I'm seeing a crash here when libvirtd is stopping and a client (namely virt-manager) calls networkConnectNetworkEventDeregisterAny().... is it OK to just goto cleanup if the driver is NULL in that function? <danpb> cbosdonnat: you mean the driver is being torn down in virStateCleanup ? and an API is still allowed to be run ? <cbosdonnat> danpb, looks like that indeed <cbosdonnat> where would the API call be rejected otherwise? <danpb> that seems a much more general problem - we should not call virStateCleanup until we've killed off all the API worker threads <danpb> i kind of thought we already did that <danpb> is it perhaps that we're killing off active clients and as part of that we're cleaning up events they have registered ? <cbosdonnat> danpb, here is the backtrace of the crash: http://paste.opensuse.org/44522138 <cbosdonnat> so seems like that indeed <danpb> oh yeah, we're killing off clients <danpb> we need to explicitly be able to kill off clients before shutting down drivers <danpb> surprised we've not seen this before to be honest <danpb> i wonder if some refactoring has introduced this problem recently <cbosdonnat> are those done in separate threads? <danpb> i can't rememeber to be honest <cbosdonnat> I'll chase the order of these then <cbosdonnat> danpb, the state cleanup happens before the daemon unref... looks like we'll need to purge the daemon before that <cbosdonnat> danpb, would that be completely silly to move the client closing code to virNetServerClose()? <danpb> that seems pretty sensible to me actually <danpb> logically they feel related actions <cbosdonnat> we can keep the unref of them where it is though if it makes more sense <cbosdonnat> yep So yes, the "refactoring" that caused this is commit id '2f3054c22'. As a test, I altered the code to perform the all the Unref's before the virStateCleanup and that "resolved" the virt-manager crash; however, that would seem to conflict with commit id '85c3a1820' which noted that performing the last Unref(dmn) prior to virStateCleanup would cause issues for daemonInhibitCallback. So I moved the Unref(dmn) after virStateCleanup... That of course led to the virt-manager issue. So I figured I'd give adding this patch in and that then fixed the virt-manager crash (obviously as we'd be Unref'ing all those servers we added). So after all that if I add that sleep into domstats as shown in the [3] link from above - I don't get a crash, but it also seems to cause the SIGTERM to be ignored at least the first time through. Running a second SIGTERM does the proper kill. 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. John [...]

On Thu, Dec 21, 2017 at 09:39:16AM +0100, Cedric Bosdonnat wrote:

Hi John,

On Wed, 2017-12-20 at 16:52 -0500, John Ferlan wrote:

...

So I moved the Unref(dmn) after virStateCleanup... That of course led to the virt-manager issue. So I figured I'd give adding this patch in and that then fixed the virt-manager crash (obviously as we'd be Unref'ing all those servers we added).

Seems like I forgot to git-send my patch yesterday evening: here it is:

https://www.redhat.com/archives/libvir-list/2017-December/msg00739.html

That fixes the virt-manager crash at least.

Hi, so the patch looks good, at first glance it makes complete sense to close the client connections when closing the server and unreffing the objects when disposing of the server object. However, since this and the other related issues have been bugging us for quite some time I'm not going to say ACK just yet, I'd like to think about it for a bit - but if someone else already has and they feel brave enough :P, then I'm not going to block this to go in. Erik

On Thu, Dec 21, 2017 at 12:48:44PM +0300, Nikolay Shirokovskiy wrote:

On 21.12.2017 11:49, Erik Skultety wrote:

...

On Thu, Dec 21, 2017 at 09:39:16AM +0100, Cedric Bosdonnat wrote:

...

Hi John,

On Wed, 2017-12-20 at 16:52 -0500, John Ferlan wrote:

...

So I moved the Unref(dmn) after virStateCleanup... That of course led to the virt-manager issue. So I figured I'd give adding this patch in and that then fixed the virt-manager crash (obviously as we'd be Unref'ing all those servers we added).

Seems like I forgot to git-send my patch yesterday evening: here it is:

https://www.redhat.com/archives/libvir-list/2017-December/msg00739.html

That fixes the virt-manager crash at least.

Hi, so the patch looks good, at first glance it makes complete sense to close the client connections when closing the server and unreffing the objects when disposing of the server object. However, since this and the other related issues have been bugging us for quite some time I'm not going to say ACK just yet, I'd like to think about it for a bit - but if someone else already has and they feel brave enough :P, then I'm not going to block this to go in.

Erik

Patch looks good to me too. But still original "libvirtd: fix crash on termination" fixes another issue and if applied fixes "virt-manager issue" as well as John figured out.

Finally I'm back on track with this (sorry for it taking so long), although you're right about this, it's not the correct fix, it's just a byproduct of your patch, however, the whole thing about closing connections and releasing memory is a bit of a mess. For example, right now, we only dispose of service objs (but we don't close them, we do that in virNetServerClose), but we both close and dispose of the client objs. Another thing, we toggle the service to stop accepting any new connections, but that's irrelevant at that point because we've closed them already by that time as a product of calling virNetDaemonClose->virNetServerClose->virNetServerServiceClose - so that should be removed...I drifted a bit here, anyway, we need to make a clear distinction when we're releasing memory and when we're shutting down some kind of service - sometimes both can be done at the same time (see below), however it's very clear we can't do it here. Your issue is that Close currently only closes the sockets (this reflects my point of "shutting down a service") but does nothing with the threadpool connected to the server, thus leaving the worker threads to continue running and executing APIs the results of which they shouldn't even be able to return back to the client, since we're shutting down. Now the thing with threadpool is that both memory release and teardown are merged into one "object disposal" operation and therefore done as part of virNetServerDispose. Since I understand a removal from the hash table as a memory release operation, we should not be calling virHashRemoveAll from virNetDaemonClose. Now, I see 2 options: 1) split virThreadPoolFree into 2 functions, one which only broadcasts the "die" message and joins the threads (or waits for them in this case...) and the other releasing the resources - can't say I'm a fan of this one 2) call virThreadPoolFree from virNetServerClose instead... None of those approaches is ideal, but I can't seem to think off anything better at the moment. I'm open to discuss any other suggestions. Erik

On 21.12.2017 17:59, John Ferlan wrote:

[...]

...

...

Patch looks good to me too. But still original "libvirtd: fix crash on termination" fixes another issue and if applied fixes "virt-manager issue" as well as John figured out.

Not sure there's enough coffee in the house this morning to make me "awake enough" for all this stuff, especially just before a holiday break. But perhaps better now than after the new year... sooo....

...

Finally I'm back on track with this (sorry for it taking so long), although you're right about this, it's not the correct fix, it's just a byproduct of your patch, however, the whole thing about closing connections and releasing memory is a bit of a mess. For example, right now, we only dispose of service objs (but we don't close them, we do that in virNetServerClose), but we both close and dispose of the client objs. Another thing, we toggle the service to stop accepting any new connections, but that's irrelevant at that point because we've closed them already by that time as a product of calling virNetDaemonClose->virNetServerClose->virNetServerServiceClose - so that

virNetServerServiceClose could call virNetServerServiceToggle(,false), even though it probably doesn't matter at this point.

Makes me wonder why virNetServerUpdateServicesLocked wasn't called in virNetServerDispose instead of open coding the nservices loop (sigh).

...

should be removed...I drifted a bit here, anyway, we need to make a clear distinction when we're releasing memory and when we're shutting down some kind of service - sometimes both can be done at the same time (see below), however it's very clear we can't do it here. Your issue is that Close currently only closes the sockets (this reflects my point of "shutting down a service") but does nothing with the threadpool connected to the server, thus leaving the worker threads to continue running and executing APIs the results of which they shouldn't even be able to return back to the client, since we're shutting down. Now the thing with threadpool is that both memory release and teardown are merged into one "object disposal" operation and therefore done as part of virNetServerDispose. Since I understand a removal from the hash table as a memory release operation, we should not be calling virHashRemoveAll from virNetDaemonClose. Now, I see 2 options:

1) split virThreadPoolFree into 2 functions, one which only broadcasts the "die" message and joins the threads (or waits for them in this case...) and the other releasing the resources - can't say I'm a fan of this one

Kind of a "virThreadPoolStop" type operation...

...

2) call virThreadPoolFree from virNetServerClose instead...

None of those approaches is ideal, but I can't seem to think off anything better at the moment.

I like 2 better, but it doesn't fix the problem of a long running thread (such as GetAllDomainStats), it just moves the cheese. Although I have a feeling the virStateShutdown series Nikolay is promoting may solve that issue.

No, no. The problem will be fixed as I mentined in a different reply. It just can not be solved by solely virHashRemoveAll with current unref ordering in libvirtd.c.

It's really a conundrum w/r/t how much time to spend on this especially if the short/long term goal is a shims for libvirtd (e.g. libvirt_qemu) which will move the cheese even more.

I'm going to move away from this for now, maybe a fresh look will help. Right now I'm not sure I can focus on this with the other threads I'm involved in.

John

...

I'm open to discuss any other suggestions. Erik

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Dec 22, 2017 at 10:52:45AM +0300, Nikolay Shirokovskiy wrote:

On 21.12.2017 15:14, Erik Skultety wrote:

...

On Thu, Dec 21, 2017 at 12:48:44PM +0300, Nikolay Shirokovskiy wrote:

...

On 21.12.2017 11:49, Erik Skultety wrote:

...

On Thu, Dec 21, 2017 at 09:39:16AM +0100, Cedric Bosdonnat wrote:

...

Hi John,

On Wed, 2017-12-20 at 16:52 -0500, John Ferlan wrote:

...

So I moved the Unref(dmn) after virStateCleanup... That of course led to the virt-manager issue. So I figured I'd give adding this patch in and that then fixed the virt-manager crash (obviously as we'd be Unref'ing all those servers we added).

Seems like I forgot to git-send my patch yesterday evening: here it is:

https://www.redhat.com/archives/libvir-list/2017-December/msg00739.html

That fixes the virt-manager crash at least.

Hi, so the patch looks good, at first glance it makes complete sense to close the client connections when closing the server and unreffing the objects when disposing of the server object. However, since this and the other related issues have been bugging us for quite some time I'm not going to say ACK just yet, I'd like to think about it for a bit - but if someone else already has and they feel brave enough :P, then I'm not going to block this to go in.

Erik

Patch looks good to me too. But still original "libvirtd: fix crash on termination" fixes another issue and if applied fixes "virt-manager issue" as well as John figured out.

Finally I'm back on track with this (sorry for it taking so long), although you're right about this, it's not the correct fix, it's just a byproduct of your patch, however, the whole thing about closing connections and releasing memory is a bit of a mess. For example, right now, we only dispose of service objs (but we don't close them, we do that in virNetServerClose), but we both close and dispose of the client objs. Another thing, we toggle the service to stop accepting any new connections, but that's irrelevant at that point because we've closed them already by that time as a product of calling virNetDaemonClose->virNetServerClose->virNetServerServiceClose - so that should be removed...I drifted a bit here, anyway, we need to make a clear distinction when we're releasing memory and when we're shutting down some kind of service - sometimes both can be done at the same time (see below), however it's very clear we can't do it here. Your issue is that Close currently only closes the sockets (this reflects my point of "shutting down a service") but does nothing with the threadpool connected to the server, thus leaving the worker threads to continue running and executing APIs the results of which they shouldn't even be able to return back to the client, since we're shutting down.

To me it is not the first order problem. On daemon shutdown clients will anyway most probably receive some kind of error.

But we should finish worker threads before we cleanup drivers.

Yes, that is absolutely critical. With current situation where we leave clients connected & worker threads running, while we cleanup drivers, we're pretty much guaranteed a tonne of bizarre crashes. For killing clients we should simply make virNetServerClose() tear down all client connections, at the same time as it tears down listener sockets. There is no point separating those actions, as they're conceptually related.

...

1) split virThreadPoolFree into 2 functions, one which only broadcasts the "die" message and joins the threads (or waits for them in this case...) and the other releasing the resources - can't say I'm a fan of this one 2) call virThreadPoolFree from virNetServerClose instead...

None of those approaches is ideal, but I can't seem to think off anything better at the moment.

Whether the workers threads are running or not is actually not the key issue here. What causes the crashes is whether the worker threads are processing jobs or not. IOW, all we need todo is first purge all queued jobs, and then wait for the threads to finish any jobs they already had. The threads themselves can stay running. This is a conceptually useful operation beyond just the shutdown scenario. IOW, we should create a virThreadPoolDrain() method to purge all jobs and wait for idle. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

[...]

...

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:

virHashRemoveAll is not enough now. Due to unref reordeing last ref to @srv is unrefed after virStateCleanup. So we need to virObjectUnref(srv|srvAdm) before virStateCleanup. Or we can call virThreadPoolFree from virNetServerClose ( as in the first version of the patch and as Erik suggests) instead of virHashRemoveAll.

Patches w/ 1. Long pause before GetAllStats (without using [u]sleep) 2. Adjustment to call virNetServerServiceToggle in virNetServerServiceClose (instead of virNetServerDispose) 3. Call virHashRemoveAll in virNetDaemonClose 4. Call virThreadPoolFree in virNetServerClose 5. Perform Unref (adminProgram, srvAdm, qemuProgram, lxcProgram, remoteProgream, and srv) before virNetDaemonClose Still has the virCondWait's - so as Daniel points out there's quite a bit more work to be done. Like most Red Hat engineers - I will not be very active over the next week or so (until the New Year) as it's a holiday break/vacation for us. So unless you have the burning desire to put together some patches and do the work yourself, more thoughts/work will need to wait. John