diff mbox series

[v1] libxl: fix migration of PV and PVH domUs with and without qemu

Message ID 20190410103338.5456-1-olaf@aepfle.de (mailing list archive)
State Superseded
Headers show
Series [v1] libxl: fix migration of PV and PVH domUs with and without qemu | expand

Commit Message

Olaf Hering April 10, 2019, 10:33 a.m. UTC
If a domU has a qemu-xen instance attached, it is required to call qemus
"xen-save-devices-state" method. Without it, the receiving side of a PV or
PVH migration may be unable to lock the image:

xen be: qdisk-51712: xen be: qdisk-51712: error: Failed to get "write" lock
error: Failed to get "write" lock
xen be: qdisk-51712: xen be: qdisk-51712: initialise() failed
initialise() failed

To fix this bug, libxl__domain_suspend_device_model() and
libxl__domain_resume_device_model() have to be called not only for HVM,
but also if the active device_model is QEMU_XEN.

Unfortunately, libxl__domain_build_info_setdefault() hardcodes
b_info->device_model_version to QEMU_XEN if it does not know it any
better. This breaks domUs without a device_model. libxl__qmp_stop() would
wait 10 seconds in qmp_open() for a qemu that will never appear.  During
this long timeframe the domU remains in state paused on the sending side.
As a result network connections may be dropped. Once this bug is fixed as
well, by just removing that assumption, there is no code to actually
initialise b_info->device_model_version.

There is a helper function libxl__need_xenpv_qemu(), which is used in
various places to decide if any device_model has to be spawned. This
function can not be used as is, just to fill b_info->device_model_version,
because store_libxl_entry() was already called earlier. Update this
function to receive a domid to work with, instead of reading xenstore.

Rearrange the code and initialize b_info->device_model_version in
libxl__domain_build_info_setdefault() per DOMAIN_TYPE.

Update initiate_domain_create() to set b_info->device_model_version if it
was not set earlier, using the updated libxl__need_xenpv_qemu().

Introduce LIBXL_DEVICE_MODEL_VERSION_NONE_REQUIRED for PV and PVH that
have no need for a device_model.

Update existing users of libxl__need_xenpv_qemu() to use
b_info->device_model_version for their check if a device_model is needed.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
 tools/libxl/libxl_create.c      | 37 ++++++++++++++++++++++++++++++-------
 tools/libxl/libxl_dm.c          | 38 ++++++++++++++++++++++----------------
 tools/libxl/libxl_dom_suspend.c |  8 ++++++--
 tools/libxl/libxl_internal.h    |  3 ++-
 tools/libxl/libxl_types.idl     |  1 +
 5 files changed, 61 insertions(+), 26 deletions(-)

Comments

Olaf Hering April 10, 2019, 10:38 a.m. UTC | #1
Am Wed, 10 Apr 2019 12:33:38 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

> +            /* will be set later */

That should read 'may be set later' because not all callers of libxl__domain_build_info_setdefault() have a need to update this field.

Olaf
Olaf Hering April 10, 2019, 3:08 p.m. UTC | #2
Am Wed, 10 Apr 2019 12:33:38 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

>              goto error_out;

This one should have been removed, now libxl fails right away.
Will do more runtime testing.

Waiting for feedback before sending v2.

Olaf
Anthony PERARD April 11, 2019, 3:56 p.m. UTC | #3
On Wed, Apr 10, 2019 at 12:33:38PM +0200, Olaf Hering wrote:
> If a domU has a qemu-xen instance attached, it is required to call qemus
> "xen-save-devices-state" method. Without it, the receiving side of a PV or
> PVH migration may be unable to lock the image:
> 
> xen be: qdisk-51712: xen be: qdisk-51712: error: Failed to get "write" lock
> error: Failed to get "write" lock
> xen be: qdisk-51712: xen be: qdisk-51712: initialise() failed
> initialise() failed

I haven't managed to reproduce this, but this bug will go away with
QEMU 4.0. The backend in QEMU 4.0 will not lock the disk image anymore.

Maybe we could simply apply a patch to our qemu-xen trees which avoid
the lock on disk images for the pv disks? That would avoid a convoluted
patch to libxl.

The lock appeared in QEMU 2.10 (qemu-xen-4.10), and won't exist for pv
guest with QEMU 4.0 (qemu-xen-4.13).
Olaf Hering April 12, 2019, 8:01 a.m. UTC | #4
Am Thu, 11 Apr 2019 16:56:02 +0100
schrieb Anthony PERARD <anthony.perard@citrix.com>:

> I haven't managed to reproduce this, but this bug will go away with
> QEMU 4.0. The backend in QEMU 4.0 will not lock the disk image anymore.

I also have a hard time to reproduce it myself, but QA gave me a hard time.

Are you saying it is not required to fix it anymore in libxl now that QEMU may eventually fix it?

Is it ok for libxl to internally lie about the device_model requirement (none vs. qemu)?

Is it ok to leave this unfixed in released branches?

Olaf
diff mbox series

Patch

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 89fe80fc9c..778417f386 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -87,16 +87,20 @@  int libxl__domain_build_info_setdefault(libxl__gc *gc,
         b_info->device_model_ssidref = SECINITSID_DOMDM;
 
     if (!b_info->device_model_version) {
-        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        switch (b_info->type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
             if (libxl_defbool_val(b_info->device_model_stubdomain)) {
                 b_info->device_model_version =
                     LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
             } else {
                 b_info->device_model_version = libxl__default_device_model(gc);
             }
-        } else {
-            b_info->device_model_version =
-                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
+        case LIBXL_DOMAIN_TYPE_PVH:
+        default:
+            /* will be set later */
+            break;
         }
         if (b_info->device_model_version
                 == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
@@ -978,6 +982,17 @@  static void initiate_domain_create(libxl__egc *egc,
         goto error_out;
     }
 
+    if (d_config->b_info.device_model_version
+        == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN) {
+        ret = libxl__need_xenpv_qemu(gc, d_config, domid);
+        if (ret)
+            d_config->b_info.device_model_version =
+                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+        else
+            d_config->b_info.device_model_version =
+                LIBXL_DEVICE_MODEL_VERSION_NONE_REQUIRED;
+    }
+
     dcs->guest_domid = domid;
     dcs->sdss.dm.guest_domid = 0; /* means we haven't spawned */
 
@@ -1312,6 +1327,7 @@  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
     libxl__domain_create_state *dcs = CONTAINER_OF(multidev, *dcs, multidev);
     STATE_AO_GC(dcs->ao);
     int i;
+    bool need_qemu;
 
     /* convenience aliases */
     const uint32_t domid = dcs->guest_domid;
@@ -1464,10 +1480,17 @@  static void domcreate_launch_dm(libxl__egc *egc, libxl__multidev *multidev,
         libxl__device_console_add(gc, domid, &console, state, &device);
         libxl__device_console_dispose(&console);
 
-        ret = libxl__need_xenpv_qemu(gc, d_config);
-        if (ret < 0)
+        switch (d_config->b_info.device_model_version) {
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                need_qemu = true;
+                break;
+            default:
+                need_qemu = false;
+                break;
+        }
             goto error_out;
-        if (ret) {
+        if (need_qemu) {
             dcs->sdss.dm.guest_domid = domid;
             libxl__spawn_local_dm(egc, &dcs->sdss.dm);
             return;
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 2f19786bdd..5e861a8111 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2268,7 +2268,7 @@  static void spawn_stub_launch_dm(libxl__egc *egc,
     libxl__domain_build_state *const d_state = sdss->dm.build_state;
     libxl__domain_build_state *const stubdom_state = &sdss->dm_state;
     uint32_t dm_domid = sdss->pvqemu.guest_domid;
-    int need_qemu;
+    bool need_qemu;
 
     if (ret) {
         LOGD(ERROR, guest_domid, "error connecting disk devices");
@@ -2337,7 +2337,15 @@  static void spawn_stub_launch_dm(libxl__egc *egc,
         }
     }
 
-    need_qemu = libxl__need_xenpv_qemu(gc, dm_config);
+    switch (dm_config->b_info.device_model_version) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            need_qemu = true;
+            break;
+        default:
+            need_qemu = false;
+            break;
+    }
 
     for (i = 0; i < num_console; i++) {
         libxl__device device;
@@ -3175,18 +3183,11 @@  static void kill_device_model_uid_cb(libxl__egc *egc,
 }
 
 /* Return 0 if no dm needed, 1 if needed and <0 if error. */
-int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config)
+int libxl__need_xenpv_qemu(libxl__gc *gc, libxl_domain_config *d_config, uint32_t domid)
 {
     int idx, i, ret, num;
-    uint32_t domid;
     const struct libxl_device_type *dt;
 
-    ret = libxl__get_domid(gc, &domid);
-    if (ret) {
-        LOG(ERROR, "unable to get domain id");
-        goto out;
-    }
-
     if (d_config->num_vfbs > 0 || d_config->num_p9s > 0) {
         ret = 1;
         goto out;
@@ -3238,21 +3239,26 @@  int libxl__dm_check_start(libxl__gc *gc, libxl_domain_config *d_config,
                           uint32_t domid)
 {
     int rc;
+    bool need_qemu;
 
     if (libxl__dm_active(gc, domid))
         return 0;
 
-    rc = libxl__need_xenpv_qemu(gc, d_config);
-    if (rc < 0)
-        goto out;
-
-    if (!rc)
+    switch (d_config->b_info.device_model_version) {
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+        case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+            need_qemu = true;
+            break;
+        default:
+            need_qemu = false;
+            break;
+    }
+    if (need_qemu == false)
         return 0;
 
     LOGD(ERROR, domid, "device model required but not running");
     rc = ERROR_FAIL;
 
-out:
     return rc;
 }
 
diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index d1af3a6573..c492fe5dd1 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -379,7 +379,9 @@  static void domain_suspend_common_guest_suspended(libxl__egc *egc,
     libxl__ev_xswatch_deregister(gc, &dsps->guest_watch);
     libxl__ev_time_deregister(gc, &dsps->guest_timeout);
 
-    if (dsps->type == LIBXL_DOMAIN_TYPE_HVM) {
+    if (dsps->type == LIBXL_DOMAIN_TYPE_HVM ||
+        libxl__device_model_version_running(gc, dsps->domid) ==
+        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         dsps->callback_device_model_done = domain_suspend_common_done;
         libxl__domain_suspend_device_model(egc, dsps); /* must be last */
         return;
@@ -459,7 +461,9 @@  int libxl__domain_resume(libxl__gc *gc, uint32_t domid, int suspend_cancel)
         goto out;
     }
 
-    if (type == LIBXL_DOMAIN_TYPE_HVM) {
+    if (type == LIBXL_DOMAIN_TYPE_HVM ||
+        libxl__device_model_version_running(gc, domid) ==
+        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
         rc = libxl__domain_resume_device_model(gc, domid);
         if (rc) {
             LOGD(ERROR, domid, "failed to resume device model:%d", rc);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 44e0221284..9eb4211d85 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1817,7 +1817,8 @@  _hidden int libxl__domain_build(libxl__gc *gc,
 _hidden const char *libxl__domain_device_model(libxl__gc *gc,
                                         const libxl_domain_build_info *info);
 _hidden int libxl__need_xenpv_qemu(libxl__gc *gc,
-                                   libxl_domain_config *d_config);
+                                   libxl_domain_config *d_config,
+                                   uint32_t domid);
 _hidden bool libxl__query_qemu_backend(libxl__gc *gc,
                                        uint32_t domid,
                                        uint32_t backend_id,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index cb4702fd7a..7d75bd3850 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -106,6 +106,7 @@  libxl_device_model_version = Enumeration("device_model_version", [
     (0, "UNKNOWN"),
     (1, "QEMU_XEN_TRADITIONAL"), # Historical qemu-xen device model (qemu-dm)
     (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
+    (3, "NONE_REQUIRED"),
     ])
 
 libxl_console_type = Enumeration("console_type", [