[libvirt PATCH 3/3] qemu: Make qemuMigrationSrcCancel optionally synchronous

[Peter Krempa]

On Thu, Sep 01, 2022 at 17:18:55 +0200, Jiri Denemark wrote:
> On Thu, Sep 01, 2022 at 16:51:34 +0200, Peter Krempa wrote:
> > On Thu, Sep 01, 2022 at 14:47:41 +0200, Jiri Denemark wrote:
> > > We have always considered "migrate_cancel" QMP command to return after
> > > successfully cancelling the migration. But this is no longer true (to be
> > > honest I'm not sure it ever was) as it just changes the migration state
> > > to "cancelling". In most cases the migration is canceled pretty quickly
> > > and we don't really notice anything, but sometimes it takes so long we
> > > even get to clearing migration capabilities before the migration is
> > > actually canceled, which fails as capabilities can only be changed when
> > > no migration is running. So to avoid this issue, we can wait for the
> > > migration to be really canceled after sending migrate_cancel. The only
> > > place where we don't need synchronous behavior is when we're cancelling
> > > migration on user's request while it is actively watched by another
> > > thread.
> > > 
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2114866
> > > 
> > > Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> > > ---
> > >  src/qemu/qemu_driver.c    |  2 +-
> > >  src/qemu/qemu_migration.c | 45 +++++++++++++++++++++++++++++++++++----
> > >  src/qemu/qemu_migration.h |  3 ++-
> > >  src/qemu/qemu_process.c   |  2 +-
> > >  4 files changed, 45 insertions(+), 7 deletions(-)
> > 
> > [...]
> > 
> > >  int
> > >  qemuMigrationSrcCancel(virDomainObj *vm,
> > > -                       virDomainAsyncJob asyncJob)
> > > +                       virDomainAsyncJob asyncJob,
> > > +                       bool wait)
> > >  {
> > >      qemuDomainObjPrivate *priv = vm->privateData;
> > >  
> > > @@ -4625,6 +4653,15 @@ qemuMigrationSrcCancel(virDomainObj *vm,
> > >      qemuMonitorMigrateCancel(priv->mon);
> > >      qemuDomainObjExitMonitor(vm);
> > >  
> > > +    if (virDomainObjIsActive(vm) && wait) {
> > 
> > Is the call to virDomainObjIsActive() necessary here? IIUC the domain
> > shutdown code is always executed in a way to make sure that waiting
> > threads are always woken.
> > 
> > 
> > > +        VIR_DEBUG("Waiting for migration to be canceled");
> > > +
> > > +        while (!qemuMigrationSrcIsCanceled(vm)) {
> > > +            if (qemuDomainObjWait(vm) < 0)
> > 
> > So here if the VM would crash before we wait we'd report success and if
> > it crashed during our wait we'll report failure, which seems weird too.
> 
> Oh right, qemuDomainObjWait already checks for virDomainObjIsActive so
> we don't have to do it explicitly here. Just
> 
>         if (wait) {
>             ...
>         }
> 
> is enough.

Reviewed-by: Peter Krempa <pkrempa at redhat.com>