Re: [PATCH] migration: Read state once

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Dr. David Alan Gilbert (git) (dgilbert@redhat.com) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> The 'status' field for the migration is updated normally using
> an atomic operation from the migration thread.
> Most readers of it aren't that careful, and in most cases it doesn't
> matter.
> 
> In query_migrate->fill_source_migration_info the 'state'
> is read twice; the first time to decide which state fields to fill in,
> and then secondly to copy the state to the status field; that can end up
> with a status that's inconsistent; e.g. setting up the fields
> for 'setup' and then having an 'active' status.  In that case
> libvirt gets upset by the lack of ram info.
> The symptom is:
>    libvirt.libvirtError: internal error: migration was active, but no RAM info was set
> 
> Read the state exactly once in fill_source_migration_info.
> 
> This is a possible fix for:
> https://bugzilla.redhat.com/show_bug.cgi?id=2074205
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Queued

> ---
>  migration/migration.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 695f0f2900..811c584619 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1073,6 +1073,7 @@ static void populate_disk_info(MigrationInfo *info)
>  static void fill_source_migration_info(MigrationInfo *info)
>  {
>      MigrationState *s = migrate_get_current();
> +    int state = qatomic_read(&s->state);
>      GSList *cur_blocker = migration_blockers;
>  
>      info->blocked_reasons = NULL;
> @@ -1092,7 +1093,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>      }
>      info->has_blocked_reasons = info->blocked_reasons != NULL;
>  
> -    switch (s->state) {
> +    switch (state) {
>      case MIGRATION_STATUS_NONE:
>          /* no migration has happened ever */
>          /* do not overwrite destination migration status */
> @@ -1137,7 +1138,7 @@ static void fill_source_migration_info(MigrationInfo *info)
>          info->has_status = true;
>          break;
>      }
> -    info->status = s->state;
> +    info->status = state;
>  }
>  
>  typedef enum WriteTrackingSupport {
> -- 
> 2.35.1
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK