[PATCH 0/2] migration/compress: refine the compress case

[PATCH 0/2] migration/compress: refine the compress case

[Wei Yang]

Two patches related to compress:

1. simplify the check since compress QEMUFile is not writable
2. handle compress setup failure gracefully

Wei Yang (2):
  migration/compress: compress QEMUFile is not writable
  migration/compress: disable compress if failed to setup

 migration/migration.c |  9 +++++++++
 migration/migration.h |  1 +
 migration/qemu-file.c | 16 +++-------------
 migration/ram.c       | 15 ++++++++-------
 4 files changed, 21 insertions(+), 20 deletions(-)

-- 
2.17.1

[PATCH 1/2] migration/compress: compress QEMUFile is not writable

[Wei Yang]

We open a file with empty_ops for compress QEMUFile, which means this is
not writable.

Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
---
 migration/qemu-file.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 26fb25ddc1..f3d99a96ec 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -744,11 +744,8 @@ static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len,
 /* Compress size bytes of data start at p and store the compressed
  * data to the buffer of f.
  *
- * When f is not writable, return -1 if f has no space to save the
- * compressed data.
- * When f is wirtable and it has no space to save the compressed data,
- * do fflush first, if f still has no space to save the compressed
- * data, return -1.
+ * Since the file is dummy file with empty_ops, return -1 if f has no space to
+ * save the compressed data.
  */
 ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
                                   const uint8_t *p, size_t size)
@@ -756,14 +753,7 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
     ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
 
     if (blen < compressBound(size)) {
-        if (!qemu_file_is_writable(f)) {
-            return -1;
-        }
-        qemu_fflush(f);
-        blen = IO_BUF_SIZE - sizeof(int32_t);
-        if (blen < compressBound(size)) {
-            return -1;
-        }
+        return -1;
     }
 
     blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
-- 
2.17.1

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

[Wei Yang]

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

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

Re: [PATCH 1/2] migration/compress: compress QEMUFile is not writable

[Dr. David Alan Gilbert]

* Wei Yang (richardw.yang@linux.intel.com) wrote:
> We open a file with empty_ops for compress QEMUFile, which means this is
> not writable.

That explanation sounds reasonable; but I'm confused by the history of
this;  the code was added by Liang Li in :

  b3be289 qemu-file: Fix qemu_put_compression_data flaw

  ( https://www.mail-archive.com/qemu-devel@nongnu.org/msg368974.html )

with almost exactly the opposite argument;  can we figure out why?

Dave

> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> ---
>  migration/qemu-file.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 26fb25ddc1..f3d99a96ec 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -744,11 +744,8 @@ static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len,
>  /* Compress size bytes of data start at p and store the compressed
>   * data to the buffer of f.
>   *
> - * When f is not writable, return -1 if f has no space to save the
> - * compressed data.
> - * When f is wirtable and it has no space to save the compressed data,
> - * do fflush first, if f still has no space to save the compressed
> - * data, return -1.
> + * Since the file is dummy file with empty_ops, return -1 if f has no space to
> + * save the compressed data.
>   */
>  ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
>                                    const uint8_t *p, size_t size)
> @@ -756,14 +753,7 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>  
>      if (blen < compressBound(size)) {
> -        if (!qemu_file_is_writable(f)) {
> -            return -1;
> -        }
> -        qemu_fflush(f);
> -        blen = IO_BUF_SIZE - sizeof(int32_t);
> -        if (blen < compressBound(size)) {
> -            return -1;
> -        }
> +        return -1;
>      }
>  
>      blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH 1/2] migration/compress: compress QEMUFile is not writable

[Wei Yang]

On Thu, Nov 07, 2019 at 11:59:10AM +0000, Dr. David Alan Gilbert wrote:
>* Wei Yang (richardw.yang@linux.intel.com) wrote:
>> We open a file with empty_ops for compress QEMUFile, which means this is
>> not writable.
>
>That explanation sounds reasonable; but I'm confused by the history of
>this;  the code was added by Liang Li in :
>
>  b3be289 qemu-file: Fix qemu_put_compression_data flaw
>
>  ( https://www.mail-archive.com/qemu-devel@nongnu.org/msg368974.html )
>
>with almost exactly the opposite argument;  can we figure out why?
>

Hmm... sounds interesting.

Toke a look into the change log, which says

    Current qemu_put_compression_data can only work with no writable
    QEMUFile, and can't work with the writable QEMUFile. But it does
    not provide any measure to prevent users from using it with a
    writable QEMUFile.

    We should fix this flaw to make it works with writable QEMUFile.

While I don't see a chance to use writable QEMUFile. Do I miss something?

>Dave
>

-- 
Wei Yang
Help you, Help me

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

[Dr. David Alan Gilbert]

* 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

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

[Juan Quintela]

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.

Re: [PATCH 1/2] migration/compress: compress QEMUFile is not writable

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Wei Yang (richardw.yang@linux.intel.com) wrote:
>> We open a file with empty_ops for compress QEMUFile, which means this is
>> not writable.
>
> That explanation sounds reasonable; but I'm confused by the history of
> this;  the code was added by Liang Li in :
>
>   b3be289 qemu-file: Fix qemu_put_compression_data flaw
>
>   ( https://www.mail-archive.com/qemu-devel@nongnu.org/msg368974.html )
>
> with almost exactly the opposite argument;  can we figure out why?

Comment of that patch is wrong, code agrees with this patch.
The patch that went in does:

-        return 0;
+        if (!qemu_file_is_writable(f)) {
+            return -1;
+        }
+        qemu_fflush(f);
+        blen = IO_BUF_SIZE - sizeof(int32_t);
+        if (blen < compressBound(size)) {
+            return -1;
+        }

i.e. it move from return 0 to return -1 if we are not able to write, or
if we are not able to get enough space.

>> @@ -756,14 +753,7 @@ ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
>>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>>  
>>      if (blen < compressBound(size)) {
>> -        if (!qemu_file_is_writable(f)) {
>> -            return -1;
>> -        }
>> -        qemu_fflush(f);
>> -        blen = IO_BUF_SIZE - sizeof(int32_t);
>> -        if (blen < compressBound(size)) {
>> -            return -1;
>> -        }
>> +        return -1;
>>      }

This moves from trying some things to just return -1.

And patch is ok.  compression file has a file with empty_os, so
qemu_fflush() will not help at all.

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


Thanks, Juan.