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 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.

> 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)) {