[libvirt] [PATCH v5 3/3] libvirtd: fix crash on termination

[Erik Skultety]

On Thu, Nov 02, 2017 at 05:32:35PM +0100, Martin Kletzander wrote:
> 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 at 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.
> > > > > >
> > > > >
> > > > > 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 at 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);
> > > > > >
> > > > >
> > > > > 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()

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

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

ACK to ^this.

Erik