Re: [PATCH v5 5/6] migration/postcopy: Release fd before going into 'postcopy-pause'

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> Logically below race could trigger with the old code:
> 
>           test program                        migration thread
>           ------------                        ----------------
>        wait_until('postcopy-pause')
>                                           postcopy_pause()
>                                             set_state('postcopy-pause')
>        do_postcopy_recover()
>          arm s->to_dst_file with new fd
>                                             release s->to_dst_file [1]
> 
> Here [1] could have released the just-installed recoverying channel.  Then the
> migration could hang without really resuming.
> 
> Instead, it should be very safe to release the fd before setting the state 
> into
> 'postcopy-pause', because there's no reason for any other thread to touch it
> during 'postcopy-active'.
> 
> Dave reported a very rare postcopy recovery hang that the migration-test
> program waited for the migration to complete in migrate_postcopy_complete().
> We do suspect it's the same thing that we're gonna fix here.  Hard to tell.
> However since we've noticed this, fix this irrelevant of the hang report.
> 
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0b4fcff01f..50df6251b7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3182,9 +3182,6 @@ static MigThrError postcopy_pause(MigrationState *s)
>      while (true) {
>          QEMUFile *file;
>  
> -        migrate_set_state(&s->state, s->state,
> -                          MIGRATION_STATUS_POSTCOPY_PAUSED);
> -
>          /* Current channel is possibly broken. Release it. */
>          assert(s->to_dst_file);
>          qemu_mutex_lock(&s->qemu_file_lock);
> @@ -3195,6 +3192,9 @@ static MigThrError postcopy_pause(MigrationState *s)
>          qemu_file_shutdown(file);
>          qemu_fclose(file);
>  
> +        migrate_set_state(&s->state, s->state,
> +                          MIGRATION_STATUS_POSTCOPY_PAUSED);
> +
>          error_report("Detected IO failure for postcopy. "
>                       "Migration paused.");
>  
> -- 
> 2.26.2
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK