1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemu: Refresh state before starting the VCPUs

Marc Hartmayer posted 1 patch 5 years, 2 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190204123624.20572-1-mhartmay@linux.ibm.com
src/qemu/qemu_process.c | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
[libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Marc Hartmayer 5 years, 2 months ago 
For normal starts (no incoming migration) the refresh of the QEMU
state must be done before the VCPUs getting started since otherwise
there might be a race condition between a possible shutdown of the
guest OS and the QEMU monitor queries.

This fixes "qemu: migration: Refresh device information after
transferring state" (93db7eea1b864).

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
---
 src/qemu/qemu_process.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dace5aaca102..2a3763f40d49 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
     }
     relabel = true;
 
-    if (incoming &&
-        incoming->deferredURI &&
-        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
-        goto stop;
+    if (incoming) {
+        if (incoming->deferredURI &&
+            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
+            goto stop;
+    } else {
+        /* Refresh state of devices from QEMU. During migration this
+         * needs to happen after the state information is fully
+         * transferred. */
+        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
+            goto stop;
+    }
 
     if (qemuProcessFinishStartup(driver, vm, asyncJob,
                                  !(flags & VIR_QEMU_PROCESS_START_PAUSED),
@@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
         /* Keep watching qemu log for errors during incoming migration, otherwise
          * unset reporting errors from qemu log. */
         qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
-
-        /* Refresh state of devices from qemu. During migration this needs to
-         * happen after the state information is fully transferred. */
-        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
-            goto stop;
     }
 
     ret = 0;
-- 
2.17.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Peter Krempa 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
> For normal starts (no incoming migration) the refresh of the QEMU
> state must be done before the VCPUs getting started since otherwise
> there might be a race condition between a possible shutdown of the
> guest OS and the QEMU monitor queries.
> 
> This fixes "qemu: migration: Refresh device information after
> transferring state" (93db7eea1b864).
> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/qemu/qemu_process.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dace5aaca102..2a3763f40d49 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>      }
>      relabel = true;
>  
> -    if (incoming &&
> -        incoming->deferredURI &&
> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> -        goto stop;
> +    if (incoming) {
> +        if (incoming->deferredURI &&
> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> +            goto stop;
> +    } else {

This logic does not seem right ...

> +        /* Refresh state of devices from QEMU. During migration this
> +         * needs to happen after the state information is fully
> +         * transferred. */

as this comment clearly states that this should happen after migration.

Here it would happen only when migration is not done.


> +        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> +            goto stop;
> +    }
>  
>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>          /* Keep watching qemu log for errors during incoming migration, otherwise
>           * unset reporting errors from qemu log. */
>          qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
> -
> -        /* Refresh state of devices from qemu. During migration this needs to
> -         * happen after the state information is fully transferred. */
> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> -            goto stop;
>      }
>  
>      ret = 0;
> -- 
> 2.17.0
> 
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Marc Hartmayer 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa <pkrempa@redhat.com> wrote:
> On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
>> For normal starts (no incoming migration) the refresh of the QEMU
>> state must be done before the VCPUs getting started since otherwise
>> there might be a race condition between a possible shutdown of the
>> guest OS and the QEMU monitor queries.
>>
>> This fixes "qemu: migration: Refresh device information after
>> transferring state" (93db7eea1b864).
>>
>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>> ---
>>  src/qemu/qemu_process.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index dace5aaca102..2a3763f40d49 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>>      }
>>      relabel = true;
>>
>> -    if (incoming &&
>> -        incoming->deferredURI &&
>> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>> -        goto stop;
>> +    if (incoming) {
>> +        if (incoming->deferredURI &&
>> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>> +            goto stop;
>> +    } else {
>
> This logic does not seem right ...

I’m not familiar with the usage of this function for migration… so
there's a good chance I'm missing something.

>
>> +        /* Refresh state of devices from QEMU. During migration this
>> +         * needs to happen after the state information is fully
>> +         * transferred. */
>
> as this comment clearly states that this should happen after
> migration.

Is qemuProcessFinishStartup not called explicitly in qemuMigrationFinish
for migration?

>
> Here it would happen only when migration is not done.

Without this patch qemuProcessRefreshState is called after
qemuProcessFinishStartup if (!incoming). Now it’s called directly before
qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
What happens in qemuProcessFinishStartup?

Before your change in 93db7eea1b86408e the function call
'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
any condition).

Thanks.

>
>
>> +        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> +            goto stop;
>> +    }
>>
>>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>>          /* Keep watching qemu log for errors during incoming migration, otherwise
>>           * unset reporting errors from qemu log. */
>>          qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
>> -
>> -        /* Refresh state of devices from qemu. During migration this needs to
>> -         * happen after the state information is fully transferred. */
>> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>> -            goto stop;
>>      }
>>
>>      ret = 0;
>> --
>> 2.17.0
>>
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Marc Hartmayer 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 02:37 PM +0100, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa <pkrempa@redhat.com> wrote:
>> On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
>>> For normal starts (no incoming migration) the refresh of the QEMU
>>> state must be done before the VCPUs getting started since otherwise
>>> there might be a race condition between a possible shutdown of the
>>> guest OS and the QEMU monitor queries.
>>>
>>> This fixes "qemu: migration: Refresh device information after
>>> transferring state" (93db7eea1b864).
>>>
>>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
>>> ---
>>>  src/qemu/qemu_process.c | 20 +++++++++++---------
>>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>>> index dace5aaca102..2a3763f40d49 100644
>>> --- a/src/qemu/qemu_process.c
>>> +++ b/src/qemu/qemu_process.c
>>> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>>>      }
>>>      relabel = true;
>>>
>>> -    if (incoming &&
>>> -        incoming->deferredURI &&
>>> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>>> -        goto stop;
>>> +    if (incoming) {
>>> +        if (incoming->deferredURI &&
>>> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
>>> +            goto stop;
>>> +    } else {
>>
>> This logic does not seem right ...
>
> I’m not familiar with the usage of this function for migration… so
> there's a good chance I'm missing something.
>
>>
>>> +        /* Refresh state of devices from QEMU. During migration this
>>> +         * needs to happen after the state information is fully
>>> +         * transferred. */
>>
>> as this comment clearly states that this should happen after
>> migration.
>
> Is qemuProcessFinishStartup
     ^^^^^^^^^^^^^^^^^^^^^^^^
     qemuProcessRefreshState

> not called explicitly in qemuMigrationFinish
> for migration?
>
>>
>> Here it would happen only when migration is not done.
>
> Without this patch qemuProcessRefreshState is called after
> qemuProcessFinishStartup if (!incoming). Now it’s called directly before
> qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
> What happens in qemuProcessFinishStartup?
>
> Before your change in 93db7eea1b86408e the function call
> 'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
> any condition).
>
> Thanks.
>
>>
>>
>>> +        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>>> +            goto stop;
>>> +    }
>>>
>>>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>>>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
>>> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>>>          /* Keep watching qemu log for errors during incoming migration, otherwise
>>>           * unset reporting errors from qemu log. */
>>>          qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
>>> -
>>> -        /* Refresh state of devices from qemu. During migration this needs to
>>> -         * happen after the state information is fully transferred. */
>>> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
>>> -            goto stop;
>>>      }
>>>
>>>      ret = 0;
>>> --
>>> 2.17.0
>>>
> --
> Kind regards / Beste Grüße
>    Marc Hartmayer
>
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Matthias Hartmann
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
--
Kind regards / Beste Grüße
   Marc Hartmayer

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Peter Krempa 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 14:41:05 +0100, Marc Hartmayer wrote:
> On Mon, Feb 04, 2019 at 02:37 PM +0100, Marc Hartmayer <mhartmay@linux.ibm.com> wrote:
> > On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa <pkrempa@redhat.com> wrote:
> >> On Mon, Feb 04, 2019 at 13:36:24 +0100, Marc Hartmayer wrote:
> >>> For normal starts (no incoming migration) the refresh of the QEMU
> >>> state must be done before the VCPUs getting started since otherwise
> >>> there might be a race condition between a possible shutdown of the
> >>> guest OS and the QEMU monitor queries.
> >>>
> >>> This fixes "qemu: migration: Refresh device information after
> >>> transferring state" (93db7eea1b864).
> >>>
> >>> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> >>> ---
> >>>  src/qemu/qemu_process.c | 20 +++++++++++---------
> >>>  1 file changed, 11 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> >>> index dace5aaca102..2a3763f40d49 100644
> >>> --- a/src/qemu/qemu_process.c
> >>> +++ b/src/qemu/qemu_process.c
> >>> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
> >>>      }
> >>>      relabel = true;
> >>>
> >>> -    if (incoming &&
> >>> -        incoming->deferredURI &&
> >>> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> >>> -        goto stop;
> >>> +    if (incoming) {
> >>> +        if (incoming->deferredURI &&
> >>> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> >>> +            goto stop;
> >>> +    } else {
> >>
> >> This logic does not seem right ...
> >
> > I’m not familiar with the usage of this function for migration… so
> > there's a good chance I'm missing something.
> >
> >>
> >>> +        /* Refresh state of devices from QEMU. During migration this
> >>> +         * needs to happen after the state information is fully
> >>> +         * transferred. */
> >>
> >> as this comment clearly states that this should happen after
> >> migration.
> >
> > Is qemuProcessFinishStartup
>      ^^^^^^^^^^^^^^^^^^^^^^^^
>      qemuProcessRefreshState
> 
> > not called explicitly in qemuMigrationFinish
> > for migration?
> >

Yes it is, in this case the comment is wrong/misleading so it should be
fixed.
> >>
> >> Here it would happen only when migration is not done.
> >
> > Without this patch qemuProcessRefreshState is called after
> > qemuProcessFinishStartup if (!incoming). Now it’s called directly before
> > qemuProcessFinishStartup if (!incoming). Why should this be wrong now?
> > What happens in qemuProcessFinishStartup?
> >
> > Before your change in 93db7eea1b86408e the function call
> > 'qemuProcessRefreshState' was done in qemuProcessFinishStartup (without
> > any condition).

As I've said in the other thread I think this fix is okay if we fix the
comment. I'll do this in a moment so you don't need to send another
patch.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Daniel P. Berrangé 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 01:36:24PM +0100, Marc Hartmayer wrote:
> For normal starts (no incoming migration) the refresh of the QEMU
> state must be done before the VCPUs getting started since otherwise
> there might be a race condition between a possible shutdown of the
> guest OS and the QEMU monitor queries.
> 
> This fixes "qemu: migration: Refresh device information after
> transferring state" (93db7eea1b864).

Hmm, this is the second bug that this patch has been responsible for
now

It has also resulted in a  deadlock in libguestfs during startup
when there is a chardev in blocking connect mode in QEMU. If the
guest writes data to the chardev before it is accepted by the
server libvirt can deadlock in startup as QEMU is blocked and
won't respond to QMP.  The core problem is that the startup code
must not rely on being able to use QMP /after/ the vCPUs have
been started untill the virDomainCreate API has completed

  https://bugzilla.redhat.com/show_bug.cgi?id=1661940

IMHO we need to just revert that original broken commit entirely
and find a different way to fix the problem it was attacking.

> 
> Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  src/qemu/qemu_process.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index dace5aaca102..2a3763f40d49 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
>      }
>      relabel = true;
>  
> -    if (incoming &&
> -        incoming->deferredURI &&
> -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> -        goto stop;
> +    if (incoming) {
> +        if (incoming->deferredURI &&
> +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> +            goto stop;
> +    } else {
> +        /* Refresh state of devices from QEMU. During migration this
> +         * needs to happen after the state information is fully
> +         * transferred. */
> +        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> +            goto stop;
> +    }
>  
>      if (qemuProcessFinishStartup(driver, vm, asyncJob,
>                                   !(flags & VIR_QEMU_PROCESS_START_PAUSED),
> @@ -6945,11 +6952,6 @@ qemuProcessStart(virConnectPtr conn,
>          /* Keep watching qemu log for errors during incoming migration, otherwise
>           * unset reporting errors from qemu log. */
>          qemuMonitorSetDomainLog(priv->mon, NULL, NULL, NULL);
> -
> -        /* Refresh state of devices from qemu. During migration this needs to
> -         * happen after the state information is fully transferred. */
> -        if (qemuProcessRefreshState(driver, vm, asyncJob) < 0)
> -            goto stop;
>      }
>  
>      ret = 0;
> -- 
> 2.17.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Refresh state before starting the VCPUs
Posted by Peter Krempa 5 years, 2 months ago 
On Mon, Feb 04, 2019 at 13:46:15 +0000, Daniel Berrange wrote:
> On Mon, Feb 04, 2019 at 01:36:24PM +0100, Marc Hartmayer wrote:
> > For normal starts (no incoming migration) the refresh of the QEMU
> > state must be done before the VCPUs getting started since otherwise
> > there might be a race condition between a possible shutdown of the
> > guest OS and the QEMU monitor queries.
> > 
> > This fixes "qemu: migration: Refresh device information after
> > transferring state" (93db7eea1b864).
> 
> Hmm, this is the second bug that this patch has been responsible for
> now
> 
> It has also resulted in a  deadlock in libguestfs during startup
> when there is a chardev in blocking connect mode in QEMU. If the
> guest writes data to the chardev before it is accepted by the
> server libvirt can deadlock in startup as QEMU is blocked and
> won't respond to QMP.  The core problem is that the startup code

QEMU allowing a guest to overwhelm it by spamming too much data which
results in a event loop lock up should be fixed as well regardless of
this.

> must not rely on being able to use QMP /after/ the vCPUs have
> been started untill the virDomainCreate API has completed
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1661940
> 
> IMHO we need to just revert that original broken commit entirely
> and find a different way to fix the problem it was attacking.

I went through the code, and it seems that this is in fact the correct
fix.

> 
> > 
> > Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> > ---
> >  src/qemu/qemu_process.c | 20 +++++++++++---------
> >  1 file changed, 11 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> > index dace5aaca102..2a3763f40d49 100644
> > --- a/src/qemu/qemu_process.c
> > +++ b/src/qemu/qemu_process.c
> > @@ -6929,10 +6929,17 @@ qemuProcessStart(virConnectPtr conn,
> >      }
> >      relabel = true;
> >  
> > -    if (incoming &&
> > -        incoming->deferredURI &&
> > -        qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> > -        goto stop;
> > +    if (incoming) {
> > +        if (incoming->deferredURI &&
> > +            qemuMigrationDstRun(driver, vm, incoming->deferredURI, asyncJob) < 0)
> > +            goto stop;
> > +    } else {
> > +        /* Refresh state of devices from QEMU. During migration this
> > +         * needs to happen after the state information is fully
> > +         * transferred. */

We just should mention that during migration the same code is called
from 'qemuMigrationDstFinish' right before starting the vCPUs.

In that function it's also executed before vCPUs are started so it's
okay to move it here.

I'll do the adjustment to the comment and push the patch.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.