Re: [QEMU HCI-7.4 PATCH] migration: set .vm_old_state in migration_stop_vm() #VSTOR-147066

["Denis V. Lunev" <den@virtuozzo.com>]

On 9/28/26 15:23, Andrey Drobyshev wrote:
> Currently, when a postcopy migration starts, the .vm_old_state isn't
> being recorded.  Thus it remains to be set to -1.  That might result into
> a VM crash in case migration fails:
>
>   migration_iteration_finish(s)
>     runstate_set(s->vm_old_state=-1)
>       assert(new_state < RUN_STATE__MAX)
>
> To avoid that, let's just move saving the previous state directly into
> the migration_stop_vm() helper.  Add 2nd function argument to
> migration_stop_vm() to achieve that.
>
> This code is mimicking what's already done in hci-8.0 and master.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  migration/migration.c | 21 ++++++++++-----------
>  migration/migration.h |  2 +-
>  2 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 00924b4a2a7..8f33181e00a 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -157,9 +157,14 @@ static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
>      return (a > b) - (a < b);
>  }
>  
> -int migration_stop_vm(RunState state)
> +int migration_stop_vm(MigrationState *s, RunState state)
>  {
> -    int ret = vm_stop_force_state(state);
> +    int ret;
> +
> +    s->vm_old_state = runstate_get();
> +    global_state_store();
> +
> +    ret = vm_stop_force_state(state);
>  
>      trace_vmstate_downtime_checkpoint("src-vm-stopped");
>  
> @@ -2460,8 +2465,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>      migration_downtime_start(ms);
>  
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
> -    global_state_store();
> -    ret = migration_stop_vm(RUN_STATE_FINISH_MIGRATE);
> +    ret = migration_stop_vm(ms, RUN_STATE_FINISH_MIGRATE);
>      if (ret < 0) {
>          goto fail;
>      }
> @@ -2660,10 +2664,7 @@ static int migration_completion_precopy(MigrationState *s,
>      migration_downtime_start(s);
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
>  
> -    s->vm_old_state = runstate_get();
> -    global_state_store();
> -
> -    ret = migration_stop_vm(RUN_STATE_FINISH_MIGRATE);
> +    ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
>      trace_migration_completion_vm_stop(ret);
>      if (ret < 0) {
>          goto out_unlock;
> @@ -3520,11 +3521,9 @@ static void *bg_migration_thread(void *opaque)
>       * transition in vm_stop_force_state() we need to wakeup it up.
>       */
>      qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER, NULL);
> -    s->vm_old_state = runstate_get();
>  
> -    global_state_store();
>      /* Forcibly stop VM before saving state of vCPUs and devices */
> -    if (migration_stop_vm(RUN_STATE_PAUSED)) {
> +    if (migration_stop_vm(s, RUN_STATE_PAUSED)) {
>          goto fail;
>      }
>      /*
> diff --git a/migration/migration.h b/migration/migration.h
> index f2704a0e3bd..fc33ee465ac 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -549,6 +549,6 @@ int migration_rp_wait(MigrationState *s);
>   */
>  void migration_rp_kick(MigrationState *s);
>  
> -int migration_stop_vm(RunState state);
> +int migration_stop_vm(MigrationState *s, RunState state);
>  
>  #endif
  Why it isn't needed for this ticket. In this incident the destination
crashed first, and the source then aborted on runstate_set(-1). With your
  destination fix, bitmap-only post-copy migration completes, so the
source failure path is never reached. The source bug now fires only when a
  post-copy migration fails for some other reason, such as the
destination dying or a stream error during completion. Your two commits
apply cleanly
  to origin/hci-7.4 without Andrey's patch.
 
  Why his patch alone is risky. Once vm_old_state is RUNNING,
migration_iteration_finish() on 7.4 handles any post-copy failure the
same way:
  bdrv_activate_all(), then vm_start() on the source. That also happens
after the device package has been sent and the destination has run the
  guest.
  - In this incident the destination ran the guest for about 4 seconds,
writing to shared NFS.
  - If the destination is dead, its image locks are gone, so activation
succeeds.
  - The source then resumes with RAM that is older than the disk. That
risks silent guest filesystem corruption.
  - Today's abort loses the VM but leaves the disk crash-consistent,
which is arguably the safer failure.
  
  Upstream says the same in 26f65c01edc (v10.2): once the destination
has started, "dest QEMU's RAM is the only version that is consistent with
  current status of the shared storage", so the source must not start
the VM. That commit only covers the case where the destination still
holds the
  locks.

  Why not take upstream:
  - 9867d4ddd04 is the commit that fixed vm_old_state upstream. It
conflicts on 7.4: it depends on the notifier rework series, about 8
commits that
    touch spice, virtio-net and vfio. It also changes CPR-reboot
behaviour by stopping the VM at migration start. That's too much for a
hotfix, and
    it brings the same restart-after-destination-ran behaviour.
  - 7b842fe354c (POSTCOPY_DEVICE, v10.2) is the real upstream answer. It
lets the source resume only while the destination is still loading device
    state and hasn't started the guest. It is too big a backport to qemu
8.2.