[Qemu-devel] [PATCH v3 0/2] Fix flaw of qemu_put_compression_data

[Qemu-devel] [PATCH v3 0/2] Fix flaw of qemu_put_compression_data

[Liang Li]

The implementation of qemu_put_compression_data only consider the case
QEMUFile is writable, it can't work with a writable QEMUFile and does
not provide any measure to prevent users from using it with a writable
QEMUFile. For safety, it should be improved to avoid some issues.

ram_save_compressed_page can be refined based on the change of
qemu_put_compression_data, very small improvement, but code looks better.

Liang Li (2):
  qemu-file: Fix qemu_put_compression_data flaw
  migration: refine ram_save_compressed_page

 migration/qemu-file.c |   23 +++++++++++++++++++++--
 migration/ram.c       |   34 ++++++++++++++++++----------------
 2 files changed, 39 insertions(+), 18 deletions(-)

[Qemu-devel] [PATCH v3 1/2] qemu-file: Fix qemu_put_compression_data flaw

[Liang Li]

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.

Suggested-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/qemu-file.c |   23 +++++++++++++++++++++--
 migration/ram.c       |    6 +++++-
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 6f4a129..b0ef1f3 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }
 
-/* compress size bytes of data start at p with specific compression
+/* Compress size bytes of data start at p with specific compression
  * level 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.
  */
 
 ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
@@ -617,7 +623,14 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
     ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
 
     if (blen < compressBound(size)) {
-        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;
+        }
     }
     if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
                   (Bytef *)p, size, level) != Z_OK) {
@@ -625,7 +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
         return 0;
     }
     qemu_put_be32(f, blen);
+    if (f->ops->writev_buffer) {
+        add_to_iovec(f, f->buf + f->buf_index, blen);
+    }
     f->buf_index += blen;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
+    }
     return blen + sizeof(int32_t);
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index 704f6a9..4de13c2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -817,7 +817,11 @@ static int do_compress_ram_page(CompressParam *param)
                                   RAM_SAVE_FLAG_COMPRESS_PAGE);
     blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
                                      migrate_compress_level());
-    bytes_sent += blen;
+    if (blen < 0) {
+        error_report("Insufficient buffer for compressed data!");
+    } else {
+        bytes_sent += blen;
+    }
 
     return bytes_sent;
 }
-- 
1.7.1

[Qemu-devel] [PATCH v3 2/2] migration: refine ram_save_compressed_page

[Liang Li]

Use qemu_put_compression_data to do the compression directly
instead of using do_compress_ram_page, avoid some data copy.
very small improvement, at the same time, add code to check
if the compression is successful.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/ram.c |   28 +++++++++++++---------------
 1 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 4de13c2..9be00eb 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -922,24 +922,20 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
                                     uint64_t *bytes_transferred)
 {
     int pages = -1;
-    uint64_t bytes_xmit;
+    uint64_t bytes_xmit = 0;
     uint8_t *p;
-    int ret;
+    int ret, blen;
     RAMBlock *block = pss->block;
     ram_addr_t offset = pss->offset;
 
     p = block->host + offset;
 
-    bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
                                 offset, TARGET_PAGE_SIZE, &bytes_xmit);
     if (bytes_xmit) {
         *bytes_transferred += bytes_xmit;
         pages = 1;
     }
-    if (block == last_sent_block) {
-        offset |= RAM_SAVE_FLAG_CONTINUE;
-    }
     if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
         if (ret != RAM_SAVE_CONTROL_DELAYED) {
             if (bytes_xmit > 0) {
@@ -959,17 +955,19 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
             flush_compressed_data(f);
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
-                set_compress_params(&comp_param[0], block, offset);
-                /* Use the qemu thread to compress the data to make sure the
-                 * first page is sent out before other pages
-                 */
-                bytes_xmit = do_compress_ram_page(&comp_param[0]);
-                acct_info.norm_pages++;
-                qemu_put_qemu_file(f, comp_param[0].file);
-                *bytes_transferred += bytes_xmit;
-                pages = 1;
+                /* Make sure the first page is sent out before other pages */
+                bytes_xmit = save_page_header(f, block, offset |
+                                              RAM_SAVE_FLAG_COMPRESS_PAGE);
+                blen = qemu_put_compression_data(f, p, TARGET_PAGE_SIZE,
+                                                 migrate_compress_level());
+                if (blen > 0) {
+                    *bytes_transferred += bytes_xmit + blen;
+                    acct_info.norm_pages++;
+                    pages = 1;
+                }
             }
         } else {
+            offset |= RAM_SAVE_FLAG_CONTINUE;
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
                 pages = compress_page_with_multi_thread(f, block, offset,
-- 
1.7.1

Re: [Qemu-devel] [PATCH v3 0/2] Fix flaw of qemu_put_compression_data

[Li, Liang Z]

> -----Original Message-----
> From: Li, Liang Z
> Sent: Thursday, February 25, 2016 11:50 AM
> To: quintela@redhat.com; amit.shah@redhat.com
> Cc: qemu-devel@nongnu.org; dgilbert@redhat.com;
> zhang.zhanghailiang@huawei.com; Li, Liang Z
> Subject: [PATCH v3 0/2] Fix flaw of qemu_put_compression_data
> 
> The implementation of qemu_put_compression_data only consider the case
> QEMUFile is writable, it can't work with a writable QEMUFile and does not
> provide any measure to prevent users from using it with a writable QEMUFile.
> For safety, it should be improved to avoid some issues.
> 
> ram_save_compressed_page can be refined based on the change of
> qemu_put_compression_data, very small improvement, but code looks
> better.
> 
> Liang Li (2):
>   qemu-file: Fix qemu_put_compression_data flaw
>   migration: refine ram_save_compressed_page
> 
>  migration/qemu-file.c |   23 +++++++++++++++++++++--
>  migration/ram.c       |   34 ++++++++++++++++++----------------
>  2 files changed, 39 insertions(+), 18 deletions(-)

Hi Juan & Amit,

How about this patch set?

Liang

Re: [Qemu-devel] [PATCH v3 0/2] Fix flaw of qemu_put_compression_data

[Li, Liang Z]

> > Subject: [PATCH v3 0/2] Fix flaw of qemu_put_compression_data
> >
> > The implementation of qemu_put_compression_data only consider the
> case
> > QEMUFile is writable, it can't work with a writable QEMUFile and does
> > not provide any measure to prevent users from using it with a writable
> QEMUFile.
> > For safety, it should be improved to avoid some issues.
> >
> > ram_save_compressed_page can be refined based on the change of
> > qemu_put_compression_data, very small improvement, but code looks
> > better.
> >
> > Liang Li (2):
> >   qemu-file: Fix qemu_put_compression_data flaw
> >   migration: refine ram_save_compressed_page
> >
> >  migration/qemu-file.c |   23 +++++++++++++++++++++--
> >  migration/ram.c       |   34 ++++++++++++++++++----------------
> >  2 files changed, 39 insertions(+), 18 deletions(-)
> 
> Hi Juan & Amit,
> 
> How about this patch set?
> 
> Liang

The thread link:
http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg05766.html
http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg05768.html
http://lists.gnu.org/archive/html/qemu-devel/2016-02/msg05767.html

Liang

Re: [Qemu-devel] [PATCH v3 0/2] Fix flaw of qemu_put_compression_data

[Amit Shah]

On (Wed) 09 Mar 2016 [08:41:41], Li, Liang Z wrote:
> 
> > -----Original Message-----
> > From: Li, Liang Z
> > Sent: Thursday, February 25, 2016 11:50 AM
> > To: quintela@redhat.com; amit.shah@redhat.com
> > Cc: qemu-devel@nongnu.org; dgilbert@redhat.com;
> > zhang.zhanghailiang@huawei.com; Li, Liang Z
> > Subject: [PATCH v3 0/2] Fix flaw of qemu_put_compression_data
> > 
> > The implementation of qemu_put_compression_data only consider the case
> > QEMUFile is writable, it can't work with a writable QEMUFile and does not
> > provide any measure to prevent users from using it with a writable QEMUFile.
> > For safety, it should be improved to avoid some issues.
> > 
> > ram_save_compressed_page can be refined based on the change of
> > qemu_put_compression_data, very small improvement, but code looks
> > better.
> > 
> > Liang Li (2):
> >   qemu-file: Fix qemu_put_compression_data flaw
> >   migration: refine ram_save_compressed_page
> > 
> >  migration/qemu-file.c |   23 +++++++++++++++++++++--
> >  migration/ram.c       |   34 ++++++++++++++++++----------------
> >  2 files changed, 39 insertions(+), 18 deletions(-)
> 
> Hi Juan & Amit,
> 
> How about this patch set?

I know Juan had some reservations.  Don't know if all his concerns
were addressed.  I'll let him decide on this one.

		Amit