Re: [PATCH] fix deadlock when qemu cannot start

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]