Re: [PATCH 2/2] migration/compress: disable compress if failed to setup

[Dr. David Alan Gilbert]

* Wei Yang (address@hidden) wrote:
> In current logic, if compress_threads_save_setup() returns -1 the whole
> migration would fail, while we could handle it gracefully by disable
> compress.

I think it's fine for migration to fail here; the user askd for
compression - if it doesn't work then it's OK to fail and they can
switch it off; since it fails right at the start there's nothing lost.

Dave

> Signed-off-by: Wei Yang <address@hidden>
> ---
>  migration/migration.c |  9 +++++++++
>  migration/migration.h |  1 +
>  migration/ram.c       | 15 ++++++++-------
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 5f7e4d15e9..02b95f4223 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2093,6 +2093,15 @@ bool migrate_use_compression(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS];
>  }
>  
> +void migrate_disable_compression(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    s->enabled_capabilities[MIGRATION_CAPABILITY_COMPRESS] = false;
> +}
> +
>  int migrate_compress_level(void)
>  {
>      MigrationState *s;
> diff --git a/migration/migration.h b/migration/migration.h
> index 4f2fe193dc..51368d3a6e 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -309,6 +309,7 @@ bool migrate_use_return_path(void);
>  uint64_t ram_get_total_transferred_pages(void);
>  
>  bool migrate_use_compression(void);
> +void migrate_disable_compression(void);
>  int migrate_compress_level(void);
>  int migrate_compress_threads(void);
>  int migrate_compress_wait_thread(void);
> diff --git a/migration/ram.c b/migration/ram.c
> index 96c9b16402..39279161a8 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -533,12 +533,12 @@ static void compress_threads_save_cleanup(void)
>      comp_param = NULL;
>  }
>  
> -static int compress_threads_save_setup(void)
> +static void compress_threads_save_setup(void)
>  {
>      int i, thread_count;
>  
>      if (!migrate_use_compression()) {
> -        return 0;
> +        return;
>      }
>      thread_count = migrate_compress_threads();
>      compress_threads = g_new0(QemuThread, thread_count);
> @@ -569,11 +569,14 @@ static int compress_threads_save_setup(void)
>                             do_data_compress, comp_param + i,
>                             QEMU_THREAD_JOINABLE);
>      }
> -    return 0;
> +    return;
>  
>  exit:
>      compress_threads_save_cleanup();
> -    return -1;
> +    migrate_disable_compression();
> +    error_report("%s: failed to setup compress threads, compress disabled",
> +                 __func__);
> +    return;
>  }
>  
>  /* Multiple fd's */
> @@ -3338,9 +3341,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      RAMState **rsp = opaque;
>      RAMBlock *block;
>  
> -    if (compress_threads_save_setup()) {
> -        return -1;
> -    }
> +    compress_threads_save_setup();
>  
>      /* migration has already setup the bitmap, reuse it. */
>      if (!migration_in_colo_state()) {
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK