Hu Tao wrote:
From 583be33213e922899b23f036494886397b2549dc Mon Sep 17 00:00:00 2001 From: Jim Fehlig <jfehlig@suse.com> Date: Thu, 21 Jun 2012 09:21:44 -0600 Subject: [PATCH] Fix deadlock on libvirtd shutdown
When shutting down libvirtd, the virNetServer shutdown can deadlock if there are in-flight jobs being handled by virNetServerHandleJob(). virNetServerFree() will acquire the virNetServer lock and call virThreadPoolFree() to terminate the workers, waiting for the workers to finish. But in-flight workers will attempt to acquire the virNetServer lock, resulting in deadlock.
Fix the deadlock by unlocking the virNetServer lock before calling virThreadPoolFree(). This is safe since the virNetServerPtr object is ref-counted and only decrementing the ref count needs to be protected. Additionally, there is no need to re-acquire the lock after virThreadPoolFree() completes as all the workers have terminated. --- src/rpc/virnetserver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index ae19e84..9d71e53 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -766,10 +766,9 @@ void virNetServerFree(virNetServerPtr srv) virNetServerLock(srv); VIR_DEBUG("srv=%p refs=%d", srv, srv->refs); srv->refs--; - if (srv->refs > 0) { - virNetServerUnlock(srv); + virNetServerUnlock(srv);
At this point other threads may have changed srv->refs...
+ if (srv->refs > 0)
...so it's unsafe to test srv->refs here without locking.
For example, assume srv->refs is 2 at the beginning of virNetServerFree,
thread A thread B
lock srv lock srv (blocked)
dec srv->refs
(srv->refs is 1)
unlock srv
lock srv
dec srv->refs
(srv->refs is 0)
unlock src
test srv->refs test srv->refs
In this case both threads have found srv->refs is 0 and are going to free srv...
Hmm, I thought it was dangerous to read srv->refs after unlocking, and then wrote the code that way anyhow :-/.
Following patch fixes problem.
From 25aa97e05aa76054b781a4e5e83781ee16d5afee Mon Sep 17 00:00:00 2001 From: Hu Tao <hutao@cn.fujitsu.com> Date: Fri, 22 Jun 2012 11:15:01 +0800 Subject: [PATCH] fix a bug of ref count in virnetserver.c
The test of ref count is not protected by lock, which is unsafe because the ref count may have been changed by other threads during the test.
This patch fixes this.
ACK, I've pushed this. Thanks! Jim
--- src/rpc/virnetserver.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 9d71e53..247ddd7 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -759,15 +759,16 @@ void virNetServerQuit(virNetServerPtr srv) void virNetServerFree(virNetServerPtr srv) { int i; + int refs;
if (!srv) return;
virNetServerLock(srv); VIR_DEBUG("srv=%p refs=%d", srv, srv->refs); - srv->refs--; + refs = --srv->refs; virNetServerUnlock(srv); - if (srv->refs > 0) + if (refs > 0) return;
for (i = 0 ; i < srv->nservices ; i++)