Re: [PATCH 08/13] migration: Thread-ify precopy vmstate load process

[Peter Xu <peterx@redhat.com>]

On Tue, Jan 13, 2026 at 10:04:02AM -0300, Fabiano Rosas wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Mon, Jan 12, 2026 at 04:04:12PM -0300, Fabiano Rosas wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > On Thu, Jan 08, 2026 at 05:27:37PM -0300, Fabiano Rosas wrote:
> >> >> Peter Xu <peterx@redhat.com> writes:
> >> >> 
> >> >> > Migration module was there for 10+ years.  Initially, it was in most cases
> >> >> > based on coroutines.  As more features were added into the framework, like
> >> >> > postcopy, multifd, etc.. it became a mixture of threads and coroutines.
> >> >> >
> >> >> > I'm guessing coroutines just can't fix all issues that migration want to
> >> >> > resolve.
> >> >> >
> >> >> > After all these years, migration is now heavily based on a threaded model.
> >> >> >
> >> >> > Now there's still a major part of migration framework that is still not
> >> >> > thread-based, which is precopy load.  We do load in a separate thread in
> >> >> > postcopy since the 1st day postcopy was introduced, however that requires a
> >> >> > separate state transition from precopy loading all devices first, which
> >> >> > still happens in the main thread of a coroutine.
> >> >> >
> >> >> > This patch tries to move the migration incoming side to be run inside a
> >> >> > separate thread (mig/dst/main) just like the src (mig/src/main).  The
> >> >> > entrance to be migration_incoming_thread().
> >> >> >
> >> >> > Quite a few things are needed to make it fly..  One note here is we need to
> >> >> > change all these things in one patch to not break anything.  The other way
> >> >> > to do this is add code to make all paths (that this patch touched) be ready
> >> >> > for either coroutine or thread.  That may cause confusions in another way.
> >> >> > So reviewers, please take my sincere apology on the hardness of reviewing
> >> >> > this patch: it covers a few modules at the same time, and with some risky
> >> >> > changes.
> >> >> >
> >> >> > BQL Analysis
> >> >> > ============
> >> >> >
> >> >> > Firstly, when moving it over to the thread, it means the thread cannot take
> >> >> > BQL during the whole process of loading anymore, because otherwise it can
> >> >> > block main thread from using the BQL for all kinds of other concurrent
> >> >> > tasks (for example, processing QMP / HMP commands).
> >> >> >
> >> >> > Here the first question to ask is: what needs BQL during precopy load, and
> >> >> > what doesn't?
> >> >> >
> >> >> 
> >> >> I just noticed that the BQL held at process_incoming_migration_co is
> >> >> also responsible for stopping qmp_migrate_set_capabilities from being
> >> >> dispatched.
> >> >
> >> > I don't know if it is by design, or even if it will be guaranteed to work..
> >> >
> >> 
> >> Regardless, we shouldn't rely on the BQL for this. The BQL should be
> >> left as last resort for things that interact across subsystems. If
> >> someone is issuing a migration command during a migration, the migration
> >> code is exquisitely positioned to handle that itself.
> >
> > Yes I agree.
> >
> >> 
> >> > Consider the migration incoming rocoutine runs into qemu_get_byte(), and
> >> > then proactively yield the migration coroutine (qemu_coroutine_yield())
> >> > when the incoming port is blocked on read..
> >> >
> >> > AFAIU, a proper fix for that (note, this will currently break tests) is:
> >> >
> >> > bool migration_is_running(void)
> >> >  {
> >> > -    MigrationState *s = current_migration;
> >> > +    MigrationStatus state;
> >> >  
> >> > -    if (!s) {
> >> > -        return false;
> >> > +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> >> > +        MigrationIncomingState *mis = migration_incoming_get_current();
> >> > +
> >> > +        if (!mis) {
> >> > +            return false;
> >> > +        }
> >> > +
> >> > +        state = mis->state;
> >> > +    } else {
> >> > +        MigrationState *s = migrate_get_current();
> >> > +
> >> > +        if (!s) {
> >> > +            return false;
> >> > +        }
> >> > +
> >> > +        state = s->state;
> >> >      }
> >> >  
> >> > -    switch (s->state) {
> >> > +    switch (state) {
> >> >      case MIGRATION_STATUS_ACTIVE:
> >> >      case MIGRATION_STATUS_POSTCOPY_DEVICE:
> >> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >> >
> >> 
> >> LGTM
> >> 
> >> >> 
> >> >> Any point during incoming migration when BQL is unlocked we have a
> >> >> window where a capability could be changed. Same for parameters, for
> >> >> that matter.
> >> >> 
> >> >> To make matters worse, the -incoming cmdline will trigger
> >> >> qmp_migrate_incoming->...->migration_transport_compatible early on, but
> >> >> until the channels finally connect and process_incoming_migration_co
> >> >> starts it's possible to just change a capability in an incompatible way
> >> >> and the transport will never be validated again.
> >> >
> >> > Right.  Above should fix it, but I believe it also means after "-incoming
> >> > tcp:xxx" (or anything not "defer") we should forbid changing migration caps
> >> > or params on destination.
> >> >
> >> 
> >> Parameters are never forbidden, right? And we cannot forbid them with
> >> is_running because some parameters are allowed to be changed while
> >> running.
> >
> > Right, my above statement was definitely inaccurate.
> >
> > After merging caps and params we only have params.  We should only allow
> > some params to be changed anytime.  Most of the params (including caps)
> > should not allow changing during migration live on either src/dst.
> >
> >> 
> >> I feel we should have a more fine grained way of saying "this option
> >> cannot be set at this moment", instead of just using the state as a
> >> proxy. States can change, while the fact that from a certain point on,
> >> certain options should not be touched anymore doesn't change.
> >
> > IIUC that's what migration_is_running() about?
> >
> 
> At a high level, yes, but I think that's actually a downside of
> migration_is_running. It's not explicit.
> 
> E.g.: qmp_migrate -> migration_transport_compatible is the first time
> capabilities are checked in the code. Guess what migration_is_running
> returns at that point? (it's false)
> 
> If there's ever a change in BQL behavior, there'll be a bug and we'll
> see another patch extending the scope of migration_is_running, either
> adding existing states to it, or as Prasad proposed, adding a new state
> that is set a little later/earlier.
> 
> What we technically want is to stop accepting new capabilities as soon
> as we're about to use them for the first time. Not as soon as (state ==
> ACTIVE || SETUP || ...) which is a more high level definition.

Isn't migration_incoming_state_setup() almost immediately invoked (after
migration_transport_compatible() check passed), which will start to make
the retval of migration_is_running() reflecting the fact?

IMHO as long as QMP handlers are run with any lock (currently, BQL), then
such layout should keep working?  That will make sure QMP "migrate" and
"migrate_set_parameters" be serialized, that's, AFAICT, what we only need.

> 
> > It's a matter of if such finer granularity is necessary for us. Let's
> > assume the simple scheme is:
> >
> >   (a) Some params are allowed to be modified anytime, examples,
> >       downtime-limit, max-bandwidth, max-postcopy-bandwidth, etc.
> >
> 
> What about during migration_cleanup() or migrate-cancel? Won't there
> ever be something in the path of setting these params that would require
> a "healthy" migration?

If we'll introduce FAILING, then plus CANCELLING I think we should cover
all race against migration_cleanup()?  As long as we'll have both FAILING
and CANCELLING to return true for migration_is_running().

> 
> >   (b) All the rest params and all capabilities are not allowed to be modified
> >       when migration_is_running() returns true (my fixed version above)
> >
> > The whitelisted (a) should really be the smallest set of params, and
> > justified one by one or it should fall into (b).
> >
> > My hope is the simple scheme should work for us already.  After merging
> > caps, it's only about some params that can be set anytime, rest params can
> > only be set when migration is not running.
> >
> 
> Sure, I'd just like to avoid adding another set of "if
> (s->parameters->has_foobar)" if possible.
> 
> >> 
> >> Maybe a little infra like bdrv_op_is_blocked, i.e, a list of blocked
> >> operations. It could be set in qmp_migrate and checked in
> >> qmp_set_parameters/caps.
> >
> > Any example you can think of, that above simple scheme won't work for us?
> >
> 
> It works. I think it's fragile due to:
> 
> - the reliance on BQL to avoid concurrent invocations;
> 
> - the possible addition of new states for unrelated reasons
>   risks regressions and it's just extra work to make sure everything is
>   still blocked at the time it needs to be blocked.

Yes, we'll need to make sure when new states introduced it needs to report
correctly in migration_is_running().  I think it's not an issue because we
need to do it right with/without relying on the function to ban updates on
caps/params.. It always must be done right as migration_is_running() is
also used in other use cases.

> 
> - any bugs need to be fixed by moving states around, which is dubious
>   since the migration flow itself (which the states should represent)
>   has not changed in a while.
> 
>   We're still seeing patches such as dc487044d5 ("migration: Make
>   migration_has_failed() work even for CANCELLING") and the recent patch
>   from Prasad. So there is something changing elsewhere and exposing
>   these bugs and it's not immediately clear what that something is (or
>   else we would have avoided the bug).
> 
> To be clear, I'm not asking we solve these issues at this moment, it's
> fine if we go forward with the simple scheme. I'm just trying to clarify
> what I think the problems are.
> 
> >> 
> >> > As discussed above, that'll at least break our qtests.  But frankly
> >> > speaking I think that's the right thing to do..  I hope libvirt always
> >> > works with "defer" and never update any caps/params after QMP
> >> > migrate_incoming.
> >> >
> >> > So I wonder if I should continue with above patch, and then fix our qtests.
> >> > Your work from the other "merge caps+params" might also work here,
> >> > actually, if we make sure everything will be set alone with the QMP
> >> > migrate_incoming single command.
> >> >
> >> 
> >> For incoming, yes. And this is maybe a point in favor of adding the
> >> 'config'.
> >> 
> >> For outgoing, there's still the point I mentioned above about how to
> >> restrict _some_ options to be allowed at runtime and others not.
> >> 
> >> > Let me know your initial thoughts, then I'll see what I can do..
> >> >
> >> 
> >> We should fix the bug, I think your patch is good for that.
> >> 
> >> Although this kind of overlaps with some things we've been discussing
> >> with Prasad. I'd be super happy if the code magically stopped using
> >> QAPI's MigrationStatus for internal tracking of migration state and
> >> blocking of commands and so on.
> >> 
> >> Whatever comes first =)
> >
> > I didn't follow as closely on the discussion there.  I don't know if
> > changing MigrationStatus is a good idea..  we should have some really good
> > reason to make libvirt and all mgmt need a change.. but maybe I misread the
> > discussion.  I can wait until I read something solid if Prasad is going to
> > propose something.
> >
> 
> We're moving towards decoupling MigrationStatus from these ordinary code
> checks. MigrationStatus should be used for informing management about a
> relevant change in the migration stage of progression.
> 
> As it stands, we're either "exposing implementation details" or
> "inadvertently changing the API". Both are undesirable.
> 
> Of course, we might introduce the "internal state is off-sync with
> externally visible states" issue, but hopefully we'll catch that during
> review. =)

I need to confess I'm still a bit lost on what it is about, and what is the
problem we're solving here.. But I can always wait and read the patches
when they come up.. :)

> 
> >> 
> >> ---
> >> Side note, did we ever discuss something like this?
> >> 
> >> struct MigrationState {
> >>    <state>
> >>    union {
> >>      <outgoing>
> >>      <incoming>
> >>    }
> >> }
> >> 
> >> there's so much stuff in these structs...
> >
> > Yeah..  When merging state, we'll also need to be careful on overlapped
> > fields, e.g. expecting a COMPLETED state (from a completed incoming
> > migration) when starting a new outgoing migration.
> >
> > We can likely leave this for later.
> 
> No worries, I'm just collecting your opinions on it.
> 

-- 
Peter Xu