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

[Peter Krempa]

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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190204/778b5cb2/attachment-0001.sig>