libxl: always call qemus xen-save-devices-state in suspend/resume

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

The proper way to decide if a PV or PVH domU has a qemu-xen running is to use the libxl__need_xenpv_qemu API. But since there is no copy of a libxl_domain_config available in these places, it should be enough to check if xenstore contains the relevant info.

Signed-off-by Olaf Hering <[email protected]>

debianhvm

Quoted 9 lines · Olaf Hering

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

Quoted 6 lines

The proper way to decide if a PV or PVH domU has a qemu-xen running is to use the libxl__need_xenpv_qemu API. But since there is no copy of a libxl_domain_config available in these places, it should be enough to check if xenstore contains the relevant info.

Signed-off-by Olaf Hering <[email protected]>

I'm rather sure this patch is repairing the following:

Tests which are failing intermittently (not blocking):
test-amd64-amd64-xl-qemut-debianhvm-amd64 16 guest-localmigrate/x10
fail pass in 122715
test-amd64-amd64-xl-qemuu-debianhvm-amd64 16 guest-localmigrate/x10
fail pass in 122715

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

The proper way to decide if a PV or PVH domU has a qemu-xen running is to use the libxl__need_xenpv_qemu API. But since there is no copy of a libxl_domain_config available in these places, it should be enough to check if xenstore contains the relevant info.

Signed-off-by Olaf Hering <[email protected]>

---
 tools/libxl/libxl_dom_suspend.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/libxl/libxl_dom_suspend.c +0 -0

index 1e904bae8a..0d769eefd8 100644

You don't need to test if the guest is HVM anymore. You only need to know if QEMU upstream is running.

Am Fri, 18 May 2018 18:01:42 +0100
schrieb Wei Liu <[email protected]>:

Quoted 2 lines

You don't need to test if the guest is HVM anymore. You only need to know if QEMU upstream is running.

libxl__domain_suspend_device_model handles qemu-xen and qemu-xen-trad. That function can not be called unconditionally I think. Perhaps I should adjust the commit message.

What I mean is: now you seem to want to call libxl__domain_suspend_device_model when QEMU upstream is running, regardless of whether a guest is HVM or PV, so the check for HVM should be gone.

Am Mon, 21 May 2018 09:18:20 +0100
schrieb Wei Liu <[email protected]>:

Quoted 4 lines

What I mean is: now you seem to want to call libxl__domain_suspend_device_model when QEMU upstream is running, regardless of whether a guest is HVM or PV, so the check for HVM should be gone.

libxl__domain_suspend_device_model calls libxl__device_model_version_running, which may return LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL in case "dm-version" does not exist.
Is it valid to assume that a non-existant "dm-version" means qemu-trad? So far I have not tested a PV domU with qemu-trad, not sure if that can work anyway.

Also, libxl__device_model_version_running may return -1. Is that valid?

I think your predicate is correct. Sorry for the noise.

Cc Anthony.

Quoted 9 lines · Olaf Hering

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

The commit message can use a bit of improvement. Strictly speaking, this is only a problem when qdisk is in use.

But I think it should be harmless to issue that command anyway.

Quoted 15 lines

The proper way to decide if a PV or PVH domU has a qemu-xen running is to use the libxl__need_xenpv_qemu API. But since there is no copy of a libxl_domain_config available in these places, it should be enough to check if xenstore contains the relevant info.

Signed-off-by Olaf Hering <[email protected]>

---
 tools/libxl/libxl_dom_suspend.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/libxl/libxl_dom_suspend.c +0 -0

index 1e904bae8a..0d769eefd8 100644

tools/libxl/libxl_dom_suspend.c static void domain_suspend_common_guest_suspended(libxl__egc *egc, · line 382

Quoted 7 lines

tools/libxl/libxl_dom_suspend.c +3 -1

@@ -377,7 +377,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) {

You don't need to test if the guest is HVM anymore. You only need to know if QEMU upstream is running.

Anthony, please check if I've said anything wrong.

Am Thu, 17 May 2018 17:51:08 +0200
schrieb Olaf Hering <[email protected]>:

tools/libxl/libxl_dom_suspend.c static void domain_suspend_common_guest_suspended(libxl__egc *egc, · line 378

Quoted 3 lines

tools/libxl/libxl_dom_suspend.c +2 -0

@@ -377,7 +377,9 @@ static void domain_suspend_common_guest_suspended(libxl__egc *egc,
+        libxl__device_model_version_running(gc, dsps->domid) ==
+        LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {

Please withdraw that patch. This condition is always true for PV.