Hello Fabiano,
On Mon, 31 Mar 2025 at 20:31, Fabiano Rosas <faro...@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-ppan...@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
