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

Re: [libvirt] [PATCH] qemu: Fix shutdown regression



On Tue, Sep 20, 2011 at 07:39:15PM +0200, Jiri Denemark wrote:
> The commit that prevents disk corruption on domain shutdown
> (96fc4784177ecb70357518fa863442455e45ad0e) causes regression with QEMU
> 0.14.* and 0.15.* because of a regression bug in QEMU that was fixed
> only recently in QEMU git. With affected QEMU binaries, domains cannot
> be shutdown properly and stay in a paused state. This patch tries to
> avoid this by sending SIGKILL to 0.1[45].* QEMU processes. Though we
> wait a bit more between sending SIGTERM and SIGKILL to reduce the
> possibility of virtual disk corruption.

IMO, SIGKILL should only be sent at the explicit direction of the
user, saying in effect, I'm ok with possible data corruption, I want
the VM killed unconditionally.  I would rather leave VMs paused than
risk corrupting data.  Let's get as much input as we can from the qemu
folks before we go down this path.

Dave


> ---
>  src/qemu/qemu_capabilities.c |    7 +++++++
>  src/qemu/qemu_capabilities.h |    1 +
>  src/qemu/qemu_process.c      |   19 +++++++++++++------
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 36f47a9..823c500 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -136,6 +136,7 @@ VIR_ENUM_IMPL(qemuCaps, QEMU_CAPS_LAST,
>                "pci-ohci",
>                "usb-redir",
>                "usb-hub",
> +              "no-shutdown-bug",
>      );
>  
>  struct qemu_feature_flags {
> @@ -1049,6 +1050,12 @@ qemuCapsComputeCmdFlags(const char *help,
>  
>      if (version >= 13000)
>          qemuCapsSet(flags, QEMU_CAPS_PCI_MULTIFUNCTION);
> +
> +    /* QEMU version 0.14.* and 0.15.* are known not to handle SIGTERM
> +     * properly when started with -no-shutdown
> +     */
> +    if (version >= 14000 && version <= 15999)
> +        qemuCapsSet(flags, QEMU_CAPS_NO_SHUTDOWN_BUG);
>  }
>  
>  /* We parse the output of 'qemu -help' to get the QEMU
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index 96b7a3b..53d5ace 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -110,6 +110,7 @@ enum qemuCapsFlags {
>      QEMU_CAPS_PCI_OHCI          = 71, /* -device pci-ohci */
>      QEMU_CAPS_USB_REDIR         = 72, /* -device usb-redir */
>      QEMU_CAPS_USB_HUB           = 73, /* -device usb-hub */
> +    QEMU_CAPS_NO_SHUTDOWN_BUG   = 74, /* -no-shutdown doesn't exit on SIGTERM */
>  
>      QEMU_CAPS_LAST,                   /* this must always be the last item */
>  };
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 3baaa19..3311699 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -434,6 +434,7 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>                            virDomainObjPtr vm)
>  {
>      qemuDomainObjPrivatePtr priv = vm->privateData;
> +    bool gracefully = true;
>      VIR_DEBUG("vm=%p", vm);
>  
>      virDomainObjLock(vm);
> @@ -443,6 +444,12 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>          goto cleanup;
>      }
>  
> +    if (qemuCapsGet(priv->qemuCaps, QEMU_CAPS_NO_SHUTDOWN_BUG)) {
> +        VIR_DEBUG("Emulator is likely affected by -no-shutdown bug;"
> +                  " we will not avoid sending SIGKILL to it");
> +        gracefully = false;
> +    }
> +
>      priv->gotShutdown = true;
>      if (priv->fakeReboot) {
>          virDomainObjRef(vm);
> @@ -452,12 +459,12 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>                              qemuProcessFakeReboot,
>                              vm) < 0) {
>              VIR_ERROR(_("Failed to create reboot thread, killing domain"));
> -            qemuProcessKill(vm, true);
> +            qemuProcessKill(vm, gracefully);
>              if (virDomainObjUnref(vm) == 0)
>                  vm = NULL;
>          }
>      } else {
> -        qemuProcessKill(vm, true);
> +        qemuProcessKill(vm, gracefully);
>      }
>  
>  cleanup:
> @@ -3227,15 +3234,15 @@ void qemuProcessKill(virDomainObjPtr vm, bool gracefully)
>      }
>  
>      /* This loop sends SIGTERM, then waits a few iterations
> -     * (1.6 seconds) to see if it dies. If still alive then
> +     * (3 seconds) to see if it dies. If still alive then
>       * it does SIGKILL, and waits a few more iterations (1.6
>       * seconds more) to confirm that it has really gone.
>       */
> -    for (i = 0 ; i < 15 ; i++) {
> +    for (i = 0 ; i < 23 ; i++) {
>          int signum;
>          if (i == 0)
>              signum = SIGTERM;
> -        else if (i == 8)
> +        else if (i == 15)
>              signum = SIGKILL;
>          else
>              signum = 0; /* Just check for existence */
> @@ -3249,7 +3256,7 @@ void qemuProcessKill(virDomainObjPtr vm, bool gracefully)
>              break;
>          }
>  
> -        if (i == 0 && gracefully)
> +        if (gracefully)
>              break;
>  
>          usleep(200 * 1000);
> -- 
> 1.7.6.1
> 
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list


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