Re: [libvirt] [PATCH] fix deadlock when qemu cannot start

On Fri, Mar 16, 2012 at 02:37:42PM +0800, Wen Congyang wrote: > When qemu cannot start, we may call qemuProcessStop() twice. > We have check whether the vm is running at the beginning of > qemuProcessStop() to avoid libvirt deadlock. We call > qemuProcessStop() with driver and vm locked. It seems that > we can avoid libvirt deadlock. But unfortunately we may > unlock driver and vm in the function qemuProcessKill() while > vm->def->id is not -1. So qemuProcessStop() will be run twice, > and monitor will be freed unexpectedly. So we should set > vm->def->id to -1 at the beginning of qemuProcessStop().

> --- > src/qemu/qemu_process.c | 20 ++++++++++++++------ > src/qemu/qemu_process.h | 2 ++ > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 0af3751..44814df 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -3633,10 +3633,11 @@ qemuProcessKill(struct qemud_driver *driver, > VIR_DEBUG("vm=%s pid=%d flags=%x", > vm->def->name, vm->pid, flags); > > - if (!virDomainObjIsActive(vm)) { > - VIR_DEBUG("VM '%s' not active", vm->def->name); > - return 0; > - } > + if (!(flags & VIR_QEMU_PROCESS_KILL_NOCHECK)) > + if (!virDomainObjIsActive(vm)) { > + VIR_DEBUG("VM '%s' not active", vm->def->name); > + return 0; > + } > > /* This loop sends SIGTERM (or SIGKILL if flags has > * VIR_QEMU_PROCESS_KILL_FORCE and VIR_QEMU_PROCESS_KILL_NOWAIT), > @@ -3739,6 +3740,13 @@ void qemuProcessStop(struct qemud_driver *driver, > return; > } > > + /* > + * We may unlock driver and vm in qemuProcessKill(), so the other thread > + * can lock driver and vm, and then call qemuProcessStop(). So we should > + * set vm->def->id to -1 here to avoid qemuProcessStop() called twice. > + */ > + vm->def->id = -1; > + > if ((logfile = qemuDomainCreateLog(driver, vm, true)) < 0) { > /* To not break the normal domain shutdown process, skip the > * timestamp log writing if failed on opening log file. */ > @@ -3801,7 +3809,8 @@ void qemuProcessStop(struct qemud_driver *driver, > } > > /* shut it off for sure */ > - ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE)); > + ignore_value(qemuProcessKill(driver, vm, VIR_QEMU_PROCESS_KILL_FORCE| > + VIR_QEMU_PROCESS_KILL_NOCHECK)); > > /* Stop autodestroy in case guest is restarted */ > qemuProcessAutoDestroyRemove(driver, vm); > @@ -3892,7 +3901,6 @@ retry: > > vm->taint = 0; > vm->pid = -1; > - vm->def->id = -1; > virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason); > VIR_FREE(priv->vcpupids); > priv->nvcpupids = 0; > diff --git a/src/qemu/qemu_process.h b/src/qemu/qemu_process.h > index 761db6f..891f7a5 100644 > --- a/src/qemu/qemu_process.h > +++ b/src/qemu/qemu_process.h > @@ -72,6 +72,8 @@ int qemuProcessAttach(virConnectPtr conn, > typedef enum { > VIR_QEMU_PROCESS_KILL_FORCE = 1 << 0, > VIR_QEMU_PROCESS_KILL_NOWAIT = 1 << 1, > + VIR_QEMU_PROCESS_KILL_NOCHECK = 1 << 2, /* donot check whether the vm is > + running */ > } virQemuProcessKillMode; > > int qemuProcessKill(struct qemud_driver *driver, Hi Wen, sorry for the delay in reviewing the patch. I think I understand the issue, and removing the pid and using an extra flag to qemuProcessKill to avoid the race sounds reasonable. I rebased the patch a bit, made some cosmetic changes especially on the comments and pushed it so it is included in rc2 for more testing, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list