migration: Read state once

[PATCH] migration: Read state once

Posted by Dr. David Alan Gilbert (git) 2 years ago

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

Re: [PATCH] migration: Read state once

Posted by Dr. David Alan Gilbert 2 years ago

* 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

Re: [PATCH] migration: Read state once

Posted by Peter Xu 2 years ago

On Wed, Apr 13, 2022 at 12:33:29PM +0100, Dr. David Alan Gilbert (git) 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>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH] migration: Read state once

Posted by Juan Quintela 2 years ago

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

Reviewed-by: Juan Quintela <quintela@redhat.com>


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