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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
