Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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 :
>
> > @@ -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
Re: [Xen-devel] [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 :
> @@ -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
pgpWNXb7_vLB0.pgp
Description: Digitale Signatur von OpenPGP
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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
> > ---
> > 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: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
Am Tue, 22 May 2018 12:14:29 +0100 schrieb Wei Liu : > I think your predicate is correct. Sorry for the noise. Is there anything else to be done to get this fixed? Olaf pgpyQZGDs0CyK.pgp Description: Digitale Signatur von OpenPGP ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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 : > > > 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: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
Am Mon, 21 May 2018 09:18:20 +0100 schrieb Wei Liu : > 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 pgpY4O0cXDrGH.pgp Description: Digitale Signatur von OpenPGP ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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 : > > > 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: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
Am Fri, 18 May 2018 18:01:42 +0100 schrieb Wei Liu : > 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 pgpcqdxN_l_Fm.pgp Description: Digitale Signatur von OpenPGP ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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
> ---
> 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: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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 > > 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: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
Am Fri, 18 May 2018 09:25:07 +0200 schrieb Juergen Gross : > 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 pgpY2ULaICJiC.pgp Description: Digitale Signatur von OpenPGP ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1] libxl: always call qemus xen-save-devices-state in suspend/resume
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 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 Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1] 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
---
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
