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

[Marc Hartmayer]

On Mon, Feb 04, 2019 at 02:37 PM +0100, Marc Hartmayer <mhartmay at linux.ibm.com> wrote:
> On Mon, Feb 04, 2019 at 01:41 PM +0100, Peter Krempa <pkrempa at 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 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 {
>>
>> 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