[PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[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 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 <olaf@aepfle.de>
---
 tools/libxl/libxl_dom_suspend.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
index 1e904bae8a..0d769eefd8 100644
--- a/tools/libxl/libxl_dom_suspend.c
+++ b/tools/libxl/libxl_dom_suspend.c
@@ -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) {
         rc = libxl__domain_suspend_device_model(gc, dsps);
         if (rc) {
             LOGD(ERROR, dsps->domid,
@@ -460,7 +462,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);

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Juergen Gross]

On 17/05/18 17:51, 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
> 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 <olaf@aepfle.de>

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

In case the maintainers are fine with this patch you can add my:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 240 bytes --]

Am Fri, 18 May 2018 09:25:07 +0200
schrieb Juergen Gross <jgross@suse.com>:

> debianhvm

The patch is for non-HVM, so perhaps these failures are something else.
To me it was not clear from the faillog what exactly failed.


Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Wei Liu]

On Fri, May 18, 2018 at 09:25:07AM +0200, Juergen Gross wrote:
> On 17/05/18 17:51, 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
> > 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 <olaf@aepfle.de>
> 
> 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

I don't think so. The locking issue is known and fixed (at least for
HVM).  It would cause local migration to fail definitively, while the
log says those two tests fail intermittently.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Wei Liu]

Cc Anthony.

On Thu, May 17, 2018 at 05:51:08PM +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
> 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.

> 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 <olaf@aepfle.de>
> ---
>  tools/libxl/libxl_dom_suspend.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
> index 1e904bae8a..0d769eefd8 100644
> --- a/tools/libxl/libxl_dom_suspend.c
> +++ b/tools/libxl/libxl_dom_suspend.c
> @@ -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.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 374 bytes --]

Am Fri, 18 May 2018 18:01:42 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

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


Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Wei Liu]

On Fri, May 18, 2018 at 09:09:10PM +0200, Olaf Hering wrote:
> Am Fri, 18 May 2018 18:01:42 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > 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.

Does that make sense?

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 710 bytes --]

Am Mon, 21 May 2018 09:18:20 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

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

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Wei Liu]

On Tue, May 22, 2018 at 10:52:20AM +0200, Olaf Hering wrote:
> Am Mon, 21 May 2018 09:18:20 +0100
> schrieb Wei Liu <wei.liu2@citrix.com>:
> 
> > 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.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 201 bytes --]

Am Tue, 22 May 2018 12:14:29 +0100
schrieb Wei Liu <wei.liu2@citrix.com>:

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

Is there anything else to be done to get this fixed?

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Anthony PERARD]

On Fri, May 18, 2018 at 06:01:42PM +0100, Wei Liu wrote:
> Cc Anthony.
> 
> On Thu, May 17, 2018 at 05:51:08PM +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
> > 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.
> 
> > 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 <olaf@aepfle.de>
> > ---
> >  tools/libxl/libxl_dom_suspend.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_dom_suspend.c b/tools/libxl/libxl_dom_suspend.c
> > index 1e904bae8a..0d769eefd8 100644
> > --- a/tools/libxl/libxl_dom_suspend.c
> > +++ b/tools/libxl/libxl_dom_suspend.c
> > @@ -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.

I think the patch is fine.

The resume side is not needed for PV because that only resume emulation,
and qemu start with emulation running, but it is armless to call the
command.

Thanks,

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Olaf Hering]

[-- Attachment #1.1: Type: text/plain, Size: 363 bytes --]

Am Thu, 17 May 2018 17:51:08 +0200
schrieb Olaf Hering <olaf@aepfle.de>:

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

Olaf

[-- Attachment #1.2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume"):
> Am Thu, 17 May 2018 17:51:08 +0200
> schrieb Olaf Hering <olaf@aepfle.de>:
> 
> > @@ -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.

OK.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel