[libvirt] [PATCH 1/3] daemon: finish threads on close

[John Ferlan]

On 09/27/2017 08:45 AM, Nikolay Shirokovskiy wrote:
> Current daemon shutdown can cause crashes. The problem is that threads
> serving client request are joined on daemon dispose after drivers already
> cleaned up. But this threads typically uses drivers and thus crashes come.
> We need to join threads before virStateCleanup. virNetDaemonClose is
> a good candidate.
> 
> But it turns out that we can hang on join. The problem is that at this
> moment event loop is not functional and for example threads waiting for
> qemu response will never finish. Let's introduce extra shutdown step
> for drivers so that they can signal any API calls in progress to finish.
> ---
>  daemon/libvirtd.c        |  2 ++
>  src/driver-state.h       |  4 ++++
>  src/libvirt.c            | 18 ++++++++++++++++++
>  src/libvirt_internal.h   |  1 +
>  src/libvirt_private.syms |  1 +
>  src/rpc/virnetserver.c   |  5 +++--
>  6 files changed, 29 insertions(+), 2 deletions(-)
> 

So - first off - this patch is doing 2 things:

 1. Introduce a new driver State function - "Shutdown"

 2. Moving virThreadPoolFree from virNetServerDispose to virNetServerClose

The two do not seem to be related so, they'd need to be separated.

It appears the motivation behind the StateShutdown is to "force" the
close the qemu monitor and agent, but it doesn't seem that's really
related to the virNet{Daemon|Server}* timing issue. So let's consider
that separately...  and I'm not considering it now...

Focusing more on #2 - the move of the virThreadPoolFree would seem to
hasten the eventual free. Or more to the point ensure it happens. While
that does resolve the problem, I don't think it's the best or actual fix
to what it appears the real problem is.

Taking it from the Start/Alloc/Ref side - @srv gets a Ref at
virNetServerNew and then again at virNetDaemonAddServer. So each @srv
has two refs, which means in order for virNetServerDispose to be called,
there would need to be two Unref's; however, I can only find one.

During cleanup: @srv is Unref'd after virNetDaemonClose, but I'm not
finding the other one. Do you recall where you may have seen it? I'm
assuming the answer is no, there wasn't one and hence why you moved that
virThreadPoolFree call.

Since at virNetDaemonAddServer we add a Ref to @srv, then during
virNetDaemonClose I'd expect that for each server on dmn->servers
there'd be the virNetServerClose and a removal from the HashTable and
Unref of the @srv object which I'm not seeing. If that was there, then
the virNetServerDispose would call virThreadPoolFree right when the
Unref was done on @srv. The better fix, I believe is a call to
virHashRemoveAll in virNetDaemonClose which does that removal of @srv
from the dmn->servers hash table. NB this would fix a memory leak since
the eventual virHashFree(dmn->servers) doesn't do free the elements of
the hash table when virNetDaemonDispose is called as a result of the
virObjectUnref(dmn) at the bottom of main() in daemon/libvirtd.c.

As an aside (and I think something else that needs to be fixed), there's
virNetDaemonAddServerPostExec which adds a @srv to dmn->services, but
never does the virObjectRef after the virHashAddEntry call. That would
need to be a patch before the patch that adds the virHashRemoveAll.

Make sense?   This is a very interesting/good catch to a problem - let's
just get the right fix!

Tks -

John

> diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> index 589b321..d2bbe1e 100644
> --- a/daemon/libvirtd.c
> +++ b/daemon/libvirtd.c
> @@ -1504,6 +1504,8 @@ int main(int argc, char **argv) {
>      virObjectUnref(lxcProgram);
>      virObjectUnref(qemuProgram);
>      virObjectUnref(adminProgram);
> +    if (driversInitialized)
> +        virStateShutdown();
>      virNetDaemonClose(dmn);
>      virObjectUnref(srv);
>      virObjectUnref(srvAdm);
> diff --git a/src/driver-state.h b/src/driver-state.h
> index 1cb3e4f..ea549a7 100644
> --- a/src/driver-state.h
> +++ b/src/driver-state.h
> @@ -42,6 +42,9 @@ typedef int
>  typedef int
>  (*virDrvStateStop)(void);
>  
> +typedef void
> +(*virDrvStateShutdown)(void);
> +
>  typedef struct _virStateDriver virStateDriver;
>  typedef virStateDriver *virStateDriverPtr;
>  
> @@ -52,6 +55,7 @@ struct _virStateDriver {
>      virDrvStateCleanup stateCleanup;
>      virDrvStateReload stateReload;
>      virDrvStateStop stateStop;
> +    virDrvStateShutdown stateShutdown;
>  };
>  
>  
> diff --git a/src/libvirt.c b/src/libvirt.c
> index 6d66fa4..ff41764 100644
> --- a/src/libvirt.c
> +++ b/src/libvirt.c
> @@ -812,6 +812,24 @@ virStateCleanup(void)
>  
>  
>  /**
> + * virStateShutdown:
> + *
> + * Run each virtualization driver's shutdown method.
> + *
> + */
> +void
> +virStateShutdown(void)
> +{
> +    int r;
> +
> +    for (r = virStateDriverTabCount - 1; r >= 0; r--) {
> +        if (virStateDriverTab[r]->stateShutdown)
> +            virStateDriverTab[r]->stateShutdown();
> +    }
> +}
> +
> +
> +/**
>   * virStateReload:
>   *
>   * Run each virtualization driver's reload method.
> diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
> index 62f490a..9863b07 100644
> --- a/src/libvirt_internal.h
> +++ b/src/libvirt_internal.h
> @@ -36,6 +36,7 @@ int virStateInitialize(bool privileged,
>  int virStateCleanup(void);
>  int virStateReload(void);
>  int virStateStop(void);
> +void virStateShutdown(void);
>  
>  /* Feature detection.  This is a libvirt-private interface for determining
>   * what features are supported by the driver.
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 5b1bc5e..59f8207 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1189,6 +1189,7 @@ virSetSharedStorageDriver;
>  virStateCleanup;
>  virStateInitialize;
>  virStateReload;
> +virStateShutdown;
>  virStateStop;
>  virStreamInData;
>  
> diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
> index 2b76daa..7dc8018 100644
> --- a/src/rpc/virnetserver.c
> +++ b/src/rpc/virnetserver.c
> @@ -764,8 +764,6 @@ void virNetServerDispose(void *obj)
>      for (i = 0; i < srv->nservices; i++)
>          virNetServerServiceToggle(srv->services[i], false);
>  
> -    virThreadPoolFree(srv->workers);
> -
>      for (i = 0; i < srv->nservices; i++)
>          virObjectUnref(srv->services[i]);
>      VIR_FREE(srv->services);
> @@ -796,6 +794,9 @@ void virNetServerClose(virNetServerPtr srv)
>      for (i = 0; i < srv->nservices; i++)
>          virNetServerServiceClose(srv->services[i]);
>  
> +    virThreadPoolFree(srv->workers);
> +    srv->workers = NULL;
> +
>      virObjectUnlock(srv);
>  }
>  
>