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

["Cédric Le Goater" <clg@redhat.com>]

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.