Re: [PATCH v8 2/7] migration: Refactor channel discovery mechanism

[Prasad Pandit <ppandit@redhat.com>]

Hello Fabiano,

On Mon, 31 Mar 2025 at 20:31, Fabiano Rosas <farosas@suse.de> wrote:
> > +    } else if (mis->from_src_file) {
> This is redundant.

* This was to ensure (double check) that when the Postcopy connection
comes in, the main channel is established. Also a couple of versions
back migration qtest was failing without this check. Nonetheless,
qtests do work now without this check. I'll remove it if we must.

> > +        channel = CH_POSTCOPY;
> >      } else {
> > -        default_channel = !mis->from_src_file;
> > +        channel = CH_MAIN;
>
> And this is impossible.

    -> https://lore.kernel.org/qemu-devel/20250215123119.814345-1-ppandit@redhat.com/T/#m18b6bf30e877f9eafaa67bba6a209b47782f6eac

* Yes, but a couple of revisions back you suggested adding it saying
CH_MAIN assignment at the top was doing some heavy lifting and it's
more clear this way.

> We should probably expand migration_incoming_setup() to make it clear
> that mis->from_src_file is set at this point. And
> assert(!mis->from_src_file). I can send a patch on top later.

* migration_incoming_setup uses the QEMUFile object only when
mis->from_src_file is not set. I'm wondering if we really need an
assert(!mis->from_src_file) check? Because it'll reach here only when
channel == CH_MAIN and channel is set to CH_MAIN only when
mis->from_src_file is NULL.


> > -    } else {
> > +    } else if (channel == CH_MULTIFD) {
> >          /* Multiple connections */
> > -        assert(migration_needs_multiple_sockets());
> >          if (migrate_multifd()) {
>
> This should be an assert.

Same, 'channel' is set to CH_MULTIFD,  only when migrate_multifd() is
enabled. Do we need another assert(migrate_multifd()) check?

> > +    } else if (channel == CH_POSTCOPY) {
> > +        assert(migrate_postcopy_preempt());
> > +        assert(!mis->postcopy_qemufile_dst);
> > +        f = qemu_file_new_input(ioc);
> > +        postcopy_preempt_new_channel(mis, f);
> > +        return;
> >      }
> >
> > -    if (migration_should_start_incoming(default_channel)) {
> > +    if (migration_has_main_and_multifd_channels()) {
>
> I think there's a bug here. Excluding multifd from the picture, if only
> the main channel needs to be setup, then it's possible to start postcopy
> recovery twice, once when the main channel appears and another time when
> the preempt channel appears.

* When the preempt channel appears 'channel' is set to CH_POSTCOPY, so
it shall 'return' before reaching here, right?

===
        } else if (!mis->from_src_file &&
                        mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
                /* reconnect main channel for postcopy recovery */
                channel = CH_MAIN;
        } else {
===
* When 'main' channel connection arrives for postcopy recovery,
'channel' shall be set to CH_MAIN.

> The previous code worked differently because it did:
>
> if (migrate_postcopy_preempt()) {
>     return main_channel;
>
> which would return false when preempt arrived after main.

* Yes.

> We could use migration_has_all_channels() instead, that would look more
> logically correct, but it would also change the current behavior that
> postcopy recovery can start before the preempt channel is in place. I'm
> not even sure if that's actually part of the design of the feature.

* Not sure if we need this.

Thank you.
---
  - Prasad