Quoting Daniel Veillard (veillard@xxxxxxxxxx):
> 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().
Oh, uh, Huh. This seems like it could be responsible for what I was
reporting a few days ago (*1). I spent most of yesterday trying to
track it down, only to finally realize that the QemuProcessStart would
silently die at various places. So i was getting ready to send an email
postulating that what's happening is that a virsh list starts, then
a virsh start starts, and when the virsh list ends it somehow causes
the virsh start to be killed.
I'll test and see if this patch fixes it.
thanks,
-serge
*1: If I create 4 vms and do
for i in `seq 1 4`; do virsh start o$i > /tmp/o$i 2>&1 &; done; for i in `seq 1 4`; do virsh list > /dev/null 2>&1; done; sleep 1; virsh list
most of the time at least one vm won't start.
> > ---
> > 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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
> http://veillard.com/ | virtualization library http://libvirt.org/
>
> --
> libvir-list mailing list
> libvir-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list