[v1] Resolve libvirtd hang on termination with connected long running client

[libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

Posted by John Ferlan 8 years ago

Split up virThreadPoolFree to create a Drain function which will
be called from virNetServerClose in order to ensure the various
worker threads are removed during the close rather than waiting
for the dispose function.

Signed-off-by: John Ferlan <jferlan@redhat.com>
---
 src/libvirt_private.syms |  1 +
 src/rpc/virnetserver.c   |  2 ++
 src/util/virthreadpool.c | 19 +++++++++++++------
 src/util/virthreadpool.h |  2 ++
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index a705fa846..f1e31ffcb 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2864,6 +2864,7 @@ virThreadJobSetWorker;
 
 
 # util/virthreadpool.h
+virThreadPoolDrain;
 virThreadPoolFree;
 virThreadPoolGetCurrentWorkers;
 virThreadPoolGetFreeWorkers;
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c
index 7bab11efb..1ae98c244 100644
--- a/src/rpc/virnetserver.c
+++ b/src/rpc/virnetserver.c
@@ -835,6 +835,8 @@ void virNetServerClose(virNetServerPtr srv)
     for (i = 0; i < srv->nservices; i++)
         virNetServerServiceClose(srv->services[i]);
 
+    virThreadPoolDrain(srv->workers);
+
     for (i = 0; i < srv->nclients; i++)
         virNetServerClientClose(srv->clients[i]);
 
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c
index 10f2bd2c3..f4ac88ddc 100644
--- a/src/util/virthreadpool.c
+++ b/src/util/virthreadpool.c
@@ -269,7 +269,8 @@ virThreadPoolNewFull(size_t minWorkers,
 
 }
 
-void virThreadPoolFree(virThreadPoolPtr pool)
+void
+virThreadPoolDrain(virThreadPoolPtr pool)
 {
     virThreadPoolJobPtr job;
     bool priority = false;
@@ -294,15 +295,21 @@ void virThreadPoolFree(virThreadPoolPtr pool)
         VIR_FREE(job);
     }
 
-    VIR_FREE(pool->workers);
-    virMutexUnlock(&pool->mutex);
-    virMutexDestroy(&pool->mutex);
-    virCondDestroy(&pool->quit_cond);
-    virCondDestroy(&pool->cond);
     if (priority) {
         VIR_FREE(pool->prioWorkers);
         virCondDestroy(&pool->prioCond);
     }
+
+    virMutexUnlock(&pool->mutex);
+}
+
+
+void virThreadPoolFree(virThreadPoolPtr pool)
+{
+    VIR_FREE(pool->workers);
+    virMutexDestroy(&pool->mutex);
+    virCondDestroy(&pool->quit_cond);
+    virCondDestroy(&pool->cond);
     VIR_FREE(pool);
 }
 
diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h
index e1f362f5b..1b897e1fd 100644
--- a/src/util/virthreadpool.h
+++ b/src/util/virthreadpool.h
@@ -50,6 +50,8 @@ size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool);
 size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool);
 size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool);
 
+void virThreadPoolDrain(virThreadPoolPtr pool);
+
 void virThreadPoolFree(virThreadPoolPtr pool);
 
 int virThreadPoolSendJob(virThreadPoolPtr pool,
-- 
2.13.6

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

Re: [libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

Posted by Erik Skultety 8 years ago

On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
> Split up virThreadPoolFree to create a Drain function which will
> be called from virNetServerClose in order to ensure the various
> worker threads are removed during the close rather than waiting
> for the dispose function.
>
> Signed-off-by: John Ferlan <jferlan@redhat.com>
> ---

I think that Dan had a bit different idea about the virThreadPoolDrain (I think
that the name should have been something like virThreadPoolJobsPurge) function.
https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html

Erik

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

Re: [libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

Posted by Daniel P. Berrange 8 years ago

On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
> On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
> > Split up virThreadPoolFree to create a Drain function which will
> > be called from virNetServerClose in order to ensure the various
> > worker threads are removed during the close rather than waiting
> > for the dispose function.
> >
> > Signed-off-by: John Ferlan <jferlan@redhat.com>
> > ---
> 
> I think that Dan had a bit different idea about the virThreadPoolDrain (I think
> that the name should have been something like virThreadPoolJobsPurge) function.
> https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html

Yep, this impl in John's patch is more akin to a StopWorkers()
method, than a Drain() method. To me "Drain" implies the workers
are still available to process further jobs

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

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

Re: [libvirt] [RFC PATCH 04/10] qemu: Introduce virTheadPoolDrain

Posted by John Ferlan 8 years ago

On 01/15/2018 11:57 AM, Daniel P. Berrange wrote:
> On Mon, Jan 15, 2018 at 05:51:28PM +0100, Erik Skultety wrote:
>> On Wed, Jan 10, 2018 at 12:23:29PM -0500, John Ferlan wrote:
>>> Split up virThreadPoolFree to create a Drain function which will
>>> be called from virNetServerClose in order to ensure the various
>>> worker threads are removed during the close rather than waiting
>>> for the dispose function.
>>>
>>> Signed-off-by: John Ferlan <jferlan@redhat.com>
>>> ---
>>
>> I think that Dan had a bit different idea about the virThreadPoolDrain (I think
>> that the name should have been something like virThreadPoolJobsPurge) function.
>> https://www.redhat.com/archives/libvir-list/2017-December/msg00802.html
> 
> Yep, this impl in John's patch is more akin to a StopWorkers()
> method, than a Drain() method. To me "Drain" implies the workers
> are still available to process further jobs
> 
> Regards,
> Daniel
> 

OK - right... there's so many links and thoughts with various
terminology in the former series... I of course read one path about
splitting up PoolFree and just ran with that, but had the more recent
stuff in the front of the brain so the Drain name stuck, but the concept
of Drain certainly wasn't what Dan described.

Let's see what I can come up with for a Drain function. Still trying to
wrap my head around the totality of this code - so many nooks and
crannies. Still not 100% clear how to handle the what happens if some
worker/job is truly stuck and the TERM is attempted.

John

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