Re: [Qemu-stable] [PATCH 2/3] migration: Cancel migration at exit

[Peter Xu]

On Fri, Sep 15, 2017 at 01:44:03PM +0800, Fam Zheng wrote:
> bdrv_close_all() would abort() due to op blockers added by BMDS, clean
> up migration states when main loop quits to avoid that.
> 
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  include/migration/misc.h | 1 +
>  migration/migration.c    | 7 ++++++-
>  vl.c                     | 3 +++
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index c079b7771b..b9a26b0898 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -54,5 +54,6 @@ bool migration_has_failed(MigrationState *);
>  /* ...and after the device transmission */
>  bool migration_in_postcopy_after_devices(MigrationState *);
>  void migration_global_dump(Monitor *mon);
> +void migrate_cancel(void);
>  
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index 959e8ec88e..2c844945c7 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1274,11 +1274,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>      }
>  }
>  
> -void qmp_migrate_cancel(Error **errp)
> +void migrate_cancel(void)
>  {
>      migrate_fd_cancel(migrate_get_current());
>  }
>  
> +void qmp_migrate_cancel(Error **errp)
> +{
> +    migrate_cancel();
> +}
> +

Nit: I would prefer just call migrate_fd_cancel() below, since I don't
see much point to introduce migrate_cancel() which only calls
migrate_fd_cancel()...

>  void qmp_migrate_set_cache_size(int64_t value, Error **errp)
>  {
>      MigrationState *s = migrate_get_current();
> diff --git a/vl.c b/vl.c
> index fb1f05b937..abbe61f40b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -87,6 +87,7 @@ int main(int argc, char **argv)
>  #include "sysemu/blockdev.h"
>  #include "hw/block/block.h"
>  #include "migration/misc.h"
> +#include "migration/savevm.h"
>  #include "migration/snapshot.h"
>  #include "migration/global_state.h"
>  #include "sysemu/tpm.h"
> @@ -4799,6 +4800,8 @@ int main(int argc, char **argv, char **envp)
>      iothread_stop_all();
>  
>      pause_all_vcpus();
> +    migrate_cancel();

IIUC this is an async cancel, so when reach here the migration thread
can still be alive.  Then...

> +    qemu_savevm_state_cleanup();

... Here calling qemu_savevm_state_cleanup() may be problematic if
migration thread has not yet quitted.

I'm thinking whether we should make migrate_fd_cancel() wait until the
migration thread finishes (state change to CANCELLED).  Then the
migration thread will do the cleanup, and here we can avoid calling
qemu_savevm_state_cleanup() as well.

>      bdrv_close_all();
>      res_free();
>  
> -- 
> 2.13.5
> 

-- 
Peter Xu