| Message ID | 20191012023932.1863-3-richardw.yang@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration/compress: refine the compress case | expand |
* Wei Yang (richardw.yang@linux.intel.com) 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 <richardw.yang@linux.intel.com> > --- > 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 / dgilbert@redhat.com / Manchester, UK
Wei Yang <richardw.yang@linux.intel.com> 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. > > Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> Fully agree with Dave here. If user asks for compression, and compression fails, we fail migration. If user wants to continue without compression, it just have to disable compression. Thanks, Juan.
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()) {
In current logic, if compress_threads_save_setup() returns -1 the whole migration would fail, while we could handle it gracefully by disable compress. Signed-off-by: Wei Yang <richardw.yang@linux.intel.com> --- migration/migration.c | 9 +++++++++ migration/migration.h | 1 + migration/ram.c | 15 ++++++++------- 3 files changed, 18 insertions(+), 7 deletions(-)