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

[Cédric Le Goater]

On 3/15/24 14:09, Peter Xu wrote:
> 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.

OK. I just sent the alternative implementation. The code looks a little
ugly  :

    bql_lock();                                                           |
    qemu_savevm_state_header(s->to_dst_file);                             |
    ret = qemu_savevm_state_setup(s->to_dst_file, &local_err);            | in 
SETUP state
    bql_unlock();                                                         |
                                                                          |
    qemu_savevm_wait_unplug(s, MIGRATION_STATUS_SETUP,                    | SETUP 
-> ACTIVE transition
                               MIGRATION_STATUS_ACTIVE);                  |
                                                                          |
    /*                                                                    |
     * Handle SETUP failures after waiting for virtio-net-failover        |
     * devices to unplug. This to preserve migration state transitions.   |
     */                                                                   |
    if (ret) {                                                            |
        migrate_set_error(s, local_err);                                  | 
handling SETUP errors in ACTIVE
        error_free(local_err);                                            |
        migrate_set_state(&s->state, MIGRATION_STATUS_ACTIVE,             |
                          MIGRATION_STATUS_FAILED);                       |
        goto fail_setup;                                                  |
    }                                                                     |
                                                                          |
    s->setup_time = qemu_clock_get_ms(QEMU_CLOCK_HOST) - setup_start;     | 
SETUP duration
                                                                          |
    trace_migration_thread_setup_complete();                              | 
SETUP trace event



Thanks,

C.