On Tue, Feb 22, 2022 at 10:57:34AM +0000, Dr. David Alan Gilbert wrote: > * Peter Xu (pet...@redhat.com) wrote: > > We used to use postcopy_try_recover() to replace migration_incoming_setup() > > to > > setup incoming channels. That's fine for the old world, but in the new > > world > > there can be more than one channels that need setup. Better move the > > channel > > setup out of it so that postcopy_try_recover() only handles the last phase > > of > > switching to the recovery phase. > > > > To do that in migration_fd_process_incoming(), move the > > postcopy_try_recover() > > call to be after migration_incoming_setup(), which will setup the channels. > > While in migration_ioc_process_incoming(), postpone the recover() routine > > right > > before we'll jump into migration_incoming_process(). > > > > A side benefit is we don't need to pass in QEMUFile* to > > postcopy_try_recover() > > anymore. Remove it. > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > OK, but note one question below: > > Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
Thanks. > > > --- > > migration/migration.c | 23 +++++++++++------------ > > 1 file changed, 11 insertions(+), 12 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index 67520d3105..b2e6446457 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -665,19 +665,20 @@ void migration_incoming_process(void) > > } > > > > /* Returns true if recovered from a paused migration, otherwise false */ > > -static bool postcopy_try_recover(QEMUFile *f) > > +static bool postcopy_try_recover(void) > > { > > MigrationIncomingState *mis = migration_incoming_get_current(); > > > > if (mis->state == MIGRATION_STATUS_POSTCOPY_PAUSED) { > > /* Resumed from a paused postcopy migration */ > > > > - mis->from_src_file = f; > > + /* This should be set already in migration_incoming_setup() */ > > + assert(mis->from_src_file); > > /* Postcopy has standalone thread to do vm load */ > > - qemu_file_set_blocking(f, true); > > + qemu_file_set_blocking(mis->from_src_file, true); > > Does that set_blocking happen on the 2nd channel somewhere? Nop. I think the rational is that by default all channels are blocking. Then what happened is: migration code only sets the main channel to non-blocking on incoming, that's in migration_incoming_setup(). Hence for postcopy recovery we need to tweak it to blocking here. The 2nd new channel is not operated by migration_incoming_setup(), but by postcopy_preempt_new_channel(), so it keeps the original blocking state, which should be blocking. If we want to make that clear, we can proactively set non-blocking too in postcopy_preempt_new_channel() on the 2nd channel. It's just that it should be optional as long as blocking is the default for any new fd of a socket. Thanks, -- Peter Xu