Re: [PATCH v4 10/25] migration: Add Error** argument to qemu_savevm_state_setup()

[Peter Xu]

On Fri, Mar 15, 2024 at 01:20:49PM +0100, Cédric Le Goater wrote:
> On 3/15/24 12:01, Peter Xu wrote:
> > On Fri, Mar 15, 2024 at 11:17:45AM +0100, Cédric Le Goater wrote:
> > > > migrate_set_state is also unintuitive because it ignores invalid state
> > > > transitions and we've been using that property to deal with special
> > > > states such as POSTCOPY_PAUSED and FAILED:
> > > > 
> > > > - After the migration goes into POSTCOPY_PAUSED, the resumed migration's
> > > >     migrate_init() will try to set the state NONE->SETUP, which is not
> > > >     valid.
> > > > 
> > > > - After save_setup fails, the migration goes into FAILED, but 
> > > > wait_unplug
> > > >     will try to transition SETUP->ACTIVE, which is also not valid.
> > > > 
> > > 
> > > I am not sure I understand what the plan is. Both solutions are 
> > > problematic
> > > regarding the state transitions.
> > > 
> > > Should we consider that waiting for failover devices to unplug is an 
> > > internal
> > > step of the SETUP phase not transitioning to ACTIVE ?
> > 
> > If to unblock this series, IIUC the simplest solution is to do what Fabiano
> > suggested, that we move qemu_savevm_wait_unplug() to be before the check of
> > setup() ret.
> 
> The simplest is IMHO moving qemu_savevm_wait_unplug() before
> qemu_savevm_state_setup() and leave patch 10 is unchanged. See
> below the extra patch. It looks much cleaner than what we have
> today.

Yes it looks cleaner indeed, it's just that then we'll have one more
possible state conversions like SETUP->UNPLUG->SETUP.  I'd say it's fine,
but let's also copy Laruent and Laine if it's going to be posted formally.

Thanks,

> 
> > In that case, the state change in qemu_savevm_wait_unplug()
> > should be benign and we should see a super small window it became ACTIVE
> > but then it should be FAILED (and IIUC the patch itself will need to use
> > ACTIVE as "old_state", not SETUP anymore).
> 
> OK. I will give it a try to compare.
> 
> > For the long term, maybe we can remove the WAIT_UNPLUG state?
> 
> I hope so, it's an internal SETUP state for me.
> 
> > The only Libvirt support seems to be here:
> > 
> > commit 8a226ddb3602586a2ba2359afc4448c02f566a0e
> > Author: Laine Stump <laine@redhat.com>
> > Date:   Wed Jan 15 16:38:57 2020 -0500
> > 
> >      qemu: add wait-unplug to qemu migration status enum
> > 
> > Considering that qemu_savevm_wait_unplug() can be a noop if the device is
> > already unplugged, I think it means no upper layer app should rely on this
> > state to present.
> 
> Thanks,
> 
> C.
> 
> 
> > 
> @@ -3383,11 +3383,10 @@ bool migration_rate_limit(void)
>   * unplugged
>   */
> -static void qemu_savevm_wait_unplug(MigrationState *s, int old_state,
> -                                    int new_state)
> +static void qemu_savevm_wait_unplug(MigrationState *s, int state)
>  {
>      if (qemu_savevm_state_guest_unplug_pending()) {
> -        migrate_set_state(&s->state, old_state, 
> MIGRATION_STATUS_WAIT_UNPLUG);
> +        migrate_set_state(&s->state, state, MIGRATION_STATUS_WAIT_UNPLUG);
>          while (s->state == MIGRATION_STATUS_WAIT_UNPLUG &&
>                 qemu_savevm_state_guest_unplug_pending()) {
> @@ -3410,9 +3409,7 @@ static void qemu_savevm_wait_unplug(Migr
>              }
>          }
> -        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, 
> new_state);
> -    } else {
> -        migrate_set_state(&s->state, old_state, new_state);
> +        migrate_set_state(&s->state, MIGRATION_STATUS_WAIT_UNPLUG, state);
>      }
>  }
> @@ -3469,17 +3466,19 @@ static void *migration_thread(void *opaq
>          qemu_savevm_send_colo_enable(s->to_dst_file);
>      }
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP);
> +
>      bql_lock();
>      qemu_savevm_state_setup(s->to_dst_file);
>      bql_unlock();
> -    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> -                               MIGRATION_STATUS_ACTIVE);
> -
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
>      trace_migration_thread_setup_complete();
> +    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> +                      MIGRATION_STATUS_ACTIVE);
> +
>      while (migration_is_active()) {
>          if (urgent || !migration_rate_exceeded(s->to_dst_file)) {
>              MigIterateState iter_state = migration_iteration_run(s);
> @@ -3580,18 +3579,20 @@ static void *bg_migration_thread(void *o
>      ram_write_tracking_prepare();
>  #endif
> +    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP);
> +
>      bql_lock();
>      qemu_savevm_state_header(s->to_dst_file);
>      qemu_savevm_state_setup(s->to_dst_file);
>      bql_unlock();
> -    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,
> -                               MIGRATION_STATUS_ACTIVE);
> -
>      s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;
>      trace_migration_thread_setup_complete();
> +    migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> +                      MIGRATION_STATUS_ACTIVE);
> +
>      bql_lock();
>      if (migration_stop_vm(s, RUN_STATE_PAUSED)) {
> 

-- 
Peter Xu