3:38 a.m.
Crash situation of last 2 patches can easily simulated, just
patch libvirt:
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 30a2830..f6b71d6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4515,6 +4515,8 @@ processMonitorEOFEvent(virQEMUDriverPtr driver,
unsigned int stopFlags = 0;
virObjectEventPtr event = NULL;
+ sleep(3);
+
if (qemuProcessBeginStopJob(driver, vm, QEMU_JOB_DESTROY, true) < 0)
return;
then when it is up and running and there is also a qemu domain running:
kill -9 $QEMU_DOMAIN && kill libvirtd
If first 2 patches are not applied yet then make sure there is no admin
server clients present.
Nikolay Shirokovskiy (4):
daemon: break cyclic deps between admin clients and daemon
daemon: call free callbacks synchronously in event loop impl
daemon: keep daemon until all hypervisors drivers are cleaned up
qemu: first wait all workers finish on state cleanup
daemon/libvirtd.c | 7 ++++-
src/qemu/qemu_driver.c | 2 +-
src/rpc/virnetdaemon.c | 1 +
src/rpc/virnetsocket.c | 70 ++++++++++++++++++++++---------------------------
src/util/vireventpoll.c | 24 +++++++----------
5 files changed, 50 insertions(+), 54 deletions(-)
--
1.8.3.1
3:38 a.m.
New subject: [libvirt] [PATCH 1/4] daemon: break cyclic deps between admin clients and daemon
Libvirtd daemon cleanup is not correct in case there are admin server
clients connected at the moment libvirtd exits. The reason is
that there is cyclic dependency: daemon holds references to its
network servers, networks servers hold references to its clients,
admin server clients hold refs to daemon. These refs are kept until
holder object dispose thus the above objects are never get disposed.
Let's drop references to severs in daemon object at appropriate moment
on cleanup procedure to break the cycle.
The caller should keep references to servers by itself. Otherwise
dropping refences can cause synchronous servers cleanup which
can cause deadlock (server cleanup blocks until workers are
finished and workers can call daemon functions which will try
to get daemon lock). Other option is to make list cleanup
via temporary list copy.
---
daemon/libvirtd.c | 3 +++
src/rpc/virnetdaemon.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 95c1b1c..7fac7b2 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1628,6 +1628,9 @@ int main(int argc, char **argv) {
virObjectUnref(qemuProgram);
virObjectUnref(adminProgram);
virNetDaemonClose(dmn);
+ /* we need to keep servers references up to here
+ so that above function will not cause servers cleanup
+ which can deadlock */
virObjectUnref(dmn);
virObjectUnref(srv);
virObjectUnref(srvAdm);
diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
index dcc89fa..c6c6522 100644
--- a/src/rpc/virnetdaemon.c
+++ b/src/rpc/virnetdaemon.c
@@ -861,6 +861,7 @@ virNetDaemonClose(virNetDaemonPtr dmn)
virObjectLock(dmn);
virHashForEach(dmn->servers, daemonServerClose, NULL);
+ virHashRemoveAll(dmn->servers);
virObjectUnlock(dmn);
}
--
1.8.3.1
3:38 a.m.
New subject: [libvirt] [PATCH 2/4] daemon: call free callbacks synchronously in event loop impl
Default event loop impl delays deleting registered handles and
timeouts so that deleting is safe from event handlers. But
this means deletions after event loop is finished will never
occur and particularly assosiated callback objects will not
be freed. For this reason network clients that exist at
the moment of libvirtd daemon exit are never get disposed
and daemon object itself too if there are admin server clients.
Let's call free callbacks immediately we don't need to delay
this operation, only removing handles from the list.
This breaks virnetsocket.c immediately as socket is locked
in freeing callback and callback is called synchronously
from virNetSocketRemoveIOCallback which holds the lock already.
So fix it to pass intermediate callback object to the loop
instead of socket itself. I've checked other users of
virEventAddHandle, looks like they don't have such problems.
---
src/rpc/virnetsocket.c | 70 ++++++++++++++++++++++---------------------------
src/util/vireventpoll.c | 24 +++++++----------
2 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 405f5ba..732a993 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -69,6 +69,16 @@
VIR_LOG_INIT("rpc.netsocket");
+struct _virNetSocketCallbackObject {
+ virNetSocketPtr sock;
+ virNetSocketIOFunc func;
+ void *opaque;
+ virFreeCallback ff;
+};
+
+typedef struct _virNetSocketCallbackObject virNetSocketCallbackObject;
+typedef virNetSocketCallbackObject *virNetSocketCallbackObjectPtr;
+
struct _virNetSocket {
virObjectLockable parent;
@@ -78,11 +88,6 @@ struct _virNetSocket {
int errfd;
bool client;
- /* Event callback fields */
- virNetSocketIOFunc func;
- void *opaque;
- virFreeCallback ff;
-
virSocketAddr localAddr;
virSocketAddr remoteAddr;
char *localAddrStrSASL;
@@ -1928,38 +1933,19 @@ static void virNetSocketEventHandle(int watch ATTRIBUTE_UNUSED,
int events,
void *opaque)
{
- virNetSocketPtr sock = opaque;
- virNetSocketIOFunc func;
- void *eopaque;
-
- virObjectLock(sock);
- func = sock->func;
- eopaque = sock->opaque;
- virObjectUnlock(sock);
-
- if (func)
- func(sock, events, eopaque);
+ virNetSocketCallbackObjectPtr o = opaque;
+ if (o->func)
+ o->func(o->sock, events, o->opaque);
}
static void virNetSocketEventFree(void *opaque)
{
- virNetSocketPtr sock = opaque;
- virFreeCallback ff;
- void *eopaque;
-
- virObjectLock(sock);
- ff = sock->ff;
- eopaque = sock->opaque;
- sock->func = NULL;
- sock->ff = NULL;
- sock->opaque = NULL;
- virObjectUnlock(sock);
-
- if (ff)
- ff(eopaque);
-
- virObjectUnref(sock);
+ virNetSocketCallbackObjectPtr o = opaque;
+ if (o->ff)
+ o->ff(o->opaque);
+ virObjectUnref(o->sock);
+ VIR_FREE(o);
}
int virNetSocketAddIOCallback(virNetSocketPtr sock,
@@ -1969,32 +1955,40 @@ int virNetSocketAddIOCallback(virNetSocketPtr sock,
virFreeCallback ff)
{
int ret = -1;
+ virNetSocketCallbackObjectPtr cbobj = NULL;
- virObjectRef(sock);
virObjectLock(sock);
if (sock->watch >= 0) {
VIR_DEBUG("Watch already registered on socket %p", sock);
goto cleanup;
}
+ if (VIR_ALLOC(cbobj) < 0)
+ goto cleanup;
+
+ cbobj->sock = virObjectRef(sock);
+ cbobj->func = func;
+ cbobj->opaque = opaque;
+ cbobj->ff = ff;
+
if ((sock->watch = virEventAddHandle(sock->fd,
events,
virNetSocketEventHandle,
- sock,
+ cbobj,
virNetSocketEventFree)) < 0) {
VIR_DEBUG("Failed to register watch on socket %p", sock);
goto cleanup;
}
- sock->func = func;
- sock->opaque = opaque;
- sock->ff = ff;
ret = 0;
cleanup:
virObjectUnlock(sock);
- if (ret != 0)
+ if (ret != 0) {
+ VIR_FREE(cbobj);
virObjectUnref(sock);
+ }
+
return ret;
}
diff --git a/src/util/vireventpoll.c b/src/util/vireventpoll.c
index 81ecab4..e152d23 100644
--- a/src/util/vireventpoll.c
+++ b/src/util/vireventpoll.c
@@ -198,6 +198,11 @@ int virEventPollRemoveHandle(int watch)
if (eventLoop.handles[i].watch == watch) {
EVENT_DEBUG("mark delete %zu %d", i, eventLoop.handles[i].fd);
eventLoop.handles[i].deleted = 1;
+ if (eventLoop.handles[i].ff) {
+ virMutexUnlock(&eventLoop.lock);
+ eventLoop.handles[i].ff(eventLoop.handles[i].opaque);
+ virMutexLock(&eventLoop.lock);
+ }
virEventPollInterruptLocked();
virMutexUnlock(&eventLoop.lock);
return 0;
@@ -315,6 +320,11 @@ int virEventPollRemoveTimeout(int timer)
continue;
if (eventLoop.timeouts[i].timer == timer) {
+ if (eventLoop.timeouts[i].ff) {
+ virMutexUnlock(&eventLoop.lock);
+ eventLoop.timeouts[i].ff(eventLoop.timeouts[i].opaque);
+ virMutexLock(&eventLoop.lock);
+ }
eventLoop.timeouts[i].deleted = 1;
virEventPollInterruptLocked();
virMutexUnlock(&eventLoop.lock);
@@ -536,13 +546,6 @@ static void virEventPollCleanupTimeouts(void)
PROBE(EVENT_POLL_PURGE_TIMEOUT,
"timer=%d",
eventLoop.timeouts[i].timer);
- if (eventLoop.timeouts[i].ff) {
- virFreeCallback ff = eventLoop.timeouts[i].ff;
- void *opaque = eventLoop.timeouts[i].opaque;
- virMutexUnlock(&eventLoop.lock);
- ff(opaque);
- virMutexLock(&eventLoop.lock);
- }
if ((i+1) < eventLoop.timeoutsCount) {
memmove(eventLoop.timeouts+i,
@@ -585,13 +588,6 @@ static void virEventPollCleanupHandles(void)
PROBE(EVENT_POLL_PURGE_HANDLE,
"watch=%d",
eventLoop.handles[i].watch);
- if (eventLoop.handles[i].ff) {
- virFreeCallback ff = eventLoop.handles[i].ff;
- void *opaque = eventLoop.handles[i].opaque;
- virMutexUnlock(&eventLoop.lock);
- ff(opaque);
- virMutexLock(&eventLoop.lock);
- }
if ((i+1) < eventLoop.handlesCount) {
memmove(eventLoop.handles+i,
--
1.8.3.1
3:44 a.m.
New subject: [libvirt] [PATCH 2/4] daemon: call free callbacks synchronously in event loop impl
On Thu, Sep 22, 2016 at 11:38:50AM +0300, Nikolay Shirokovskiy wrote:
Default event loop impl delays deleting registered handles and
timeouts so that deleting is safe from event handlers. But
this means deletions after event loop is finished will never
occur and particularly assosiated callback objects will not
be freed. For this reason network clients that exist at
the moment of libvirtd daemon exit are never get disposed
and daemon object itself too if there are admin server clients.
Let's call free callbacks immediately we don't need to delay
this operation, only removing handles from the list.
This breaks virnetsocket.c immediately as socket is locked
in freeing callback and callback is called synchronously
from virNetSocketRemoveIOCallback which holds the lock already.
So fix it to pass intermediate callback object to the loop
instead of socket itself. I've checked other users of
virEventAddHandle, looks like they don't have such problems.
That is impossible to guarantee. The event loop impl is exposed
via the public API, so arbitrary external applications get to
call it.
So I don't think it is safe to make this change.
Regards,
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
4:11 a.m.
New subject: [libvirt] [PATCH 2/4] daemon: call free callbacks synchronously in event loop impl
On 22.09.2016 11:44, Daniel P. Berrange wrote:
On Thu, Sep 22, 2016 at 11:38:50AM +0300, Nikolay Shirokovskiy
wrote:
> Default event loop impl delays deleting registered handles and
> timeouts so that deleting is safe from event handlers. But
> this means deletions after event loop is finished will never
> occur and particularly assosiated callback objects will not
> be freed. For this reason network clients that exist at
> the moment of libvirtd daemon exit are never get disposed
> and daemon object itself too if there are admin server clients.
>
> Let's call free callbacks immediately we don't need to delay
> this operation, only removing handles from the list.
>
> This breaks virnetsocket.c immediately as socket is locked
> in freeing callback and callback is called synchronously
> from virNetSocketRemoveIOCallback which holds the lock already.
> So fix it to pass intermediate callback object to the loop
> instead of socket itself. I've checked other users of
> virEventAddHandle, looks like they don't have such problems.
That is impossible to guarantee. The event loop impl is exposed
via the public API, so arbitrary external applications get to
call it.
So I don't think it is safe to make this change.
Ok, the other I option I think of is to add call to indicate event loop
is finished so that there is no need to defer cleanups.
Nikolay
3:38 a.m.
New subject: [libvirt] [PATCH 3/4] daemon: keep daemon until all hypervisors drivers are cleaned up
This fix SEGV with next backtrace (shortened a bit):
1 0x00007fd3a791b2e6 in virCondWait (c=<optimized out>, m=<optimized out>) at
util/virthread.c:154
2 0x00007fd3a791bcb0 in virThreadPoolFree (pool=0x7fd38024ee00) at
util/virthreadpool.c:266
3 0x00007fd38edaa00e in qemuStateCleanup () at qemu/qemu_driver.c:1116
4 0x00007fd3a79abfeb in virStateCleanup () at libvirt.c:808
5 0x00007fd3a85f2c9e in main (argc=<optimized out>, argv=<optimized out>) at
libvirtd.c:1660
Thread 1 (Thread 0x7fd38722d700 (LWP 32256)):
0 0x00007fd3a7900910 in virClassIsDerivedFrom (klass=0xdfd36058d4853,
parent=0x7fd3a8f394d0) at util/virobject.c:169
1 0x00007fd3a7900c4e in virObjectIsClass (anyobj=anyobj@entry=0x7fd3a8f2f850,
klass=<optimized out>) at util/virobject.c:365
2 0x00007fd3a7900c74 in virObjectLock (anyobj=0x7fd3a8f2f850) at util/virobject.c:317
3 0x00007fd3a7a24d5d in virNetDaemonRemoveShutdownInhibition (dmn=0x7fd3a8f2f850) at
rpc/virnetdaemon.c:547
4 0x00007fd38ed722cf in qemuProcessStop (driver=driver@entry=0x7fd380103810,
vm=vm@entry=0x7fd38025b6d0, reason=reason@entry=VIR_DOMAIN_SHUTOFF_SHUTDOWN,
asyncJob=asyncJob@entry=QEMU_ASYNC_JOB_NONE,
flags=flags@entry=0) at qemu/qemu_process.c:5786
5 0x00007fd38edd9428 in processMonitorEOFEvent (vm=0x7fd38025b6d0, driver=0x7fd380103810)
at qemu/qemu_driver.c:4588
6 qemuProcessEventHandler (data=<optimized out>, opaque=0x7fd380103810) at
qemu/qemu_driver.c:4632
7 0x00007fd3a791bb55 in virThreadPoolWorker (opaque=opaque@entry=0x7fd3a8f1e4c0) at
util/virthreadpool.c:145
This happens due to race on simultaneous finishing of libvirtd and
qemu process. We need to keep daemon object until all hypervisor
drivers are cleaned up. The other option is to take reference to
daemon in every hypervisor driver but this will take more work
and we really don't need to. Drivers cleanup procedure is synchronous
thus let's just remove last reference to daemon after drivers cleanup.
---
daemon/libvirtd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
index 7fac7b2..97a7587 100644
--- a/daemon/libvirtd.c
+++ b/daemon/libvirtd.c
@@ -1631,7 +1631,6 @@ int main(int argc, char **argv) {
/* we need to keep servers references up to here
so that above function will not cause servers cleanup
which can deadlock */
- virObjectUnref(dmn);
virObjectUnref(srv);
virObjectUnref(srvAdm);
virNetlinkShutdown();
@@ -1661,6 +1660,9 @@ int main(int argc, char **argv) {
driversInitialized = false;
virStateCleanup();
}
+ /* unref daemon only here as hypervisor drivers can
+ call shutdown inhibition functions on cleanup */
+ virObjectUnref(dmn);
return ret;
}
--
1.8.3.1
3:38 a.m.
New subject: [libvirt] [PATCH 4/4] qemu: first wait all workers finish on state cleanup
This fix next crash (bt is shortened a bit):
0 0x00007ffff7282f2b in virClassIsDerivedFrom (klass=0xdeadbeef, parent=0x55555581d650)
at util/virobject.c:169
1 0x00007ffff72835fd in virObjectIsClass (anyobj=0x7fffd024f580, klass=0x55555581d650) at
util/virobject.c:365
2 0x00007ffff7283498 in virObjectLock (anyobj=0x7fffd024f580) at util/virobject.c:317
3 0x00007ffff722f0a3 in virCloseCallbacksUnset (closeCallbacks=0x7fffd024f580,
vm=0x7fffd0194db0,
cb=0x7fffdf1af765 <qemuProcessAutoDestroy>) at util/virclosecallbacks.c:164
4 0x00007fffdf1afa7b in qemuProcessAutoDestroyRemove (driver=0x7fffd00f3a60,
vm=0x7fffd0194db0)
at qemu/qemu_process.c:6365
5 0x00007fffdf1adff1 in qemuProcessStop (driver=0x7fffd00f3a60, vm=0x7fffd0194db0,
reason=VIR_DOMAIN_SHUTOFF_CRASHED, asyncJob=QEMU_ASYNC_JOB_NONE, flags=0) at
qemu/qemu_process.c:5877
6 0x00007fffdf1f711c in processMonitorEOFEvent (driver=0x7fffd00f3a60,
vm=0x7fffd0194db0)
at qemu/qemu_driver.c:4545
7 0x00007fffdf1f7313 in qemuProcessEventHandler (data=0x555555832710,
opaque=0x7fffd00f3a60)
at qemu/qemu_driver.c:4589
8 0x00007ffff72a84c4 in virThreadPoolWorker (opaque=0x555555805da0) at
util/virthreadpool.c:167
Thread 1 (Thread 0x7ffff7fb1880 (LWP 494472)):
1 0x00007ffff72a7898 in virCondWait (c=0x7fffd01c21f8, m=0x7fffd01c21a0) at
util/virthread.c:154
2 0x00007ffff72a8a22 in virThreadPoolFree (pool=0x7fffd01c2160) at
util/virthreadpool.c:290
3 0x00007fffdf1edd44 in qemuStateCleanup () at qemu/qemu_driver.c:1102
4 0x00007ffff736570a in virStateCleanup () at libvirt.c:807
5 0x000055555556f991 in main (argc=1, argv=0x7fffffffe458) at libvirtd.c:1660
This happens on race conditions of simultaneous exiting libvirtd and qemu process.
Let's just finish all workers that keep driver pointer before disposing
driver object or any of its members.
---
src/qemu/qemu_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index e29180d..30a2830 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1068,6 +1068,7 @@ qemuStateCleanup(void)
if (!qemu_driver)
return -1;
+ virThreadPoolFree(qemu_driver->workerPool);
virNWFilterUnRegisterCallbackDriver(&qemuCallbackDriver);
virObjectUnref(qemu_driver->config);
virObjectUnref(qemu_driver->hostdevMgr);
@@ -1099,7 +1100,6 @@ qemuStateCleanup(void)
virLockManagerPluginUnref(qemu_driver->lockManager);
virMutexDestroy(&qemu_driver->lock);
- virThreadPoolFree(qemu_driver->workerPool);
VIR_FREE(qemu_driver);
return 0;
--
1.8.3.1
2983
days inactive
2983
days old
6 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Nikolay Shirokovskiy