Re: [PATCH 08/14] migration: Make qemu_savevm_query_pending() available anytime

[Juraj Marcin <jmarcin@redhat.com>]

On 2026-04-08 12:55, Peter Xu wrote:
> After qemu_savevm_query_pending() be exposed to more code paths, it can be
> used at very early stage when migration started and this may expose some
> race conditions that we don't use to have.  This patch make it prepared
> for such use cases so this API is fine to be used almost anytime.
> 
> What matters here is, querying pending for each module normally depends on
> save_setup() being run first, otherwise modules may not be ready for the
> query request.
> 
> Consider an early cancellation of migration after SETUP status but before
> invocations of save_setup() hooks, source QEMU may fall into CANCELLING
> stage directly from SETUP (not ACTIVE, which is the normal use case), in
> which case save_setup() may not have been invoked and modules are not
> ready.  However qemu_savevm_query_pending() may still be used in QMP
> commands like query-migrate and causing crashes.
> 
> Guard such use case by introducing a boolean reflecting the availability of
> vmstate save handlers on correct completions of save_setup()s.  So far,
> only protect qemu_savevm_query_pending() with it.  Logically other hooks
> face similar concern, but most of them shouldn't be reachable from random
> code path except migration thread so it should be fine.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.h |  8 ++++++++
>  migration/savevm.h    |  2 +-
>  migration/migration.c |  2 +-
>  migration/savevm.c    | 37 +++++++++++++++++++++++++++++++++----
>  4 files changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index b6888daced..e504df6915 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -522,6 +522,14 @@ struct MigrationState {
>       * anything as input.
>       */
>      bool has_block_bitmap_mapping;
> +
> +    /*
> +     * This boolean reflects if the vmstate handlers have been properly
> +     * setup on source side.  It is set after vmstate save_setup() hooks
> +     * are successfully invoked, and cleared after save_cleanup()s.  It
> +     * reflects a general availability of vmstate hooks on the source side.
> +     */
> +    bool save_setup_ready;
>  };
>  
>  void migrate_set_state(MigrationStatus *state, MigrationStatus old_state,
> diff --git a/migration/savevm.h b/migration/savevm.h
> index 96fdf96d4e..04ed09cec2 100644
> --- a/migration/savevm.h
> +++ b/migration/savevm.h
> @@ -42,7 +42,7 @@ int qemu_savevm_state_resume_prepare(MigrationState *s);
>  void qemu_savevm_send_header(QEMUFile *f);
>  void qemu_savevm_state_header(QEMUFile *f);
>  int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> -void qemu_savevm_state_cleanup(void);
> +void qemu_savevm_state_cleanup(MigrationState *s);
>  void qemu_savevm_state_complete_postcopy(QEMUFile *f);
>  int qemu_savevm_state_complete_precopy(MigrationState *s);
>  void qemu_savevm_query_pending(MigPendingData *pending, bool exact);
> diff --git a/migration/migration.c b/migration/migration.c
> index bb17bd0e68..a9ee3360e1 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1283,7 +1283,7 @@ static void migration_cleanup(MigrationState *s)
>      g_free(s->hostname);
>      s->hostname = NULL;
>  
> -    qemu_savevm_state_cleanup();
> +    qemu_savevm_state_cleanup(s);
>      cpr_state_close();
>      cpr_transfer_source_destroy(s);
>  
> diff --git a/migration/savevm.c b/migration/savevm.c
> index b75c311a95..1d3fce45b9 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1387,7 +1387,8 @@ int qemu_savevm_state_non_iterable_early(QEMUFile *f,
>      return 0;
>  }
>  
> -static int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
> +static int qemu_savevm_state_setup(MigrationState *s, QEMUFile *f,
> +                                   Error **errp)
>  {
>      SaveStateEntry *se;
>      int ret;
> @@ -1409,6 +1410,13 @@ static int qemu_savevm_state_setup(QEMUFile *f, Error **errp)
>          }
>      }
>  
> +    /*
> +     * Logically, it should be paired with any hook being used who needs to
> +     * load_acquire() the flag first.  So far, only save_query_pending()
> +     * uses it.
> +     */
> +    qatomic_store_release(&s->save_setup_ready, true);

What other savevm functions would benefit from this? Would it make sense
to include them in this patch/series?

> +
>      return 0;
>  }
>  
> @@ -1429,7 +1437,7 @@ int qemu_savevm_state_do_setup(QEMUFile *f, Error **errp)
>          return ret;
>      }
>  
> -    ret = qemu_savevm_state_setup(f, errp);
> +    ret = qemu_savevm_state_setup(ms, f, errp);
>      if (ret) {
>          return ret;
>      }
> @@ -1764,10 +1772,23 @@ int qemu_savevm_state_complete_precopy(MigrationState *s)
>  
>  void qemu_savevm_query_pending(MigPendingData *pending, bool exact)
>  {
> +    MigrationState *s = migrate_get_current();
>      SaveStateEntry *se;
>  
>      memset(pending, 0, sizeof(*pending));
>  
> +    /*
> +     * This API can be invoked very early before SETUP is properly done, in
> +     * that case don't invoke module queries because they're not ready.
> +     * Just report all zeros.
> +     *
> +     * This is paired with save_setup_ready updates on save_setup() and
> +     * save_cleanup().
> +     */
> +    if (!s || !qatomic_load_acquire(&s->save_setup_ready)) {
> +        return;
> +    }
> +
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_query_pending) {
>              continue;
> @@ -1786,7 +1807,7 @@ void qemu_savevm_query_pending(MigPendingData *pending, bool exact)
>                                      pending->postcopy_bytes);
>  }
>  
> -void qemu_savevm_state_cleanup(void)
> +void qemu_savevm_state_cleanup(MigrationState *s)
>  {
>      SaveStateEntry *se;
>      Error *local_err = NULL;
> @@ -1795,6 +1816,14 @@ void qemu_savevm_state_cleanup(void)
>          error_report_err(local_err);
>      }
>  
> +    s->save_setup_ready = false;
> +    /*
> +     * Make sure we clear the flag before invoking save_cleanup(), so any
> +     * racy QMP query-migrate won't try to invoke any save hooks.  Just use
> +     * an explicit barrier to be simple.
> +     */
> +    smp_mb();
> +
>      trace_savevm_state_cleanup();
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (se->ops && se->ops->save_cleanup) {
> @@ -1841,7 +1870,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>          error_setg_errno(errp, -ret, "Error while writing VM state");
>      }
>  cleanup:
> -    qemu_savevm_state_cleanup();
> +    qemu_savevm_state_cleanup(ms);
>  
>      if (ret != 0) {
>          status = MIGRATION_STATUS_FAILED;
> -- 
> 2.53.0
>