[Qemu-devel] [PATCH v4 00/11] backup compression

[Qemu-devel] [PATCH v4 00/11] backup compression

[Denis V. Lunev]

The idea is simple - backup is "written-once" data. It is written block
by block and it is large enough. It would be nice to save storage
space and compress it.

These patches add the ability to compress data during backup. This
functionality is implemented by means of adding options to the qmp/hmp
commands(drive-backup, blockdev-backup). The implementation is quite
simple, because the responsibility for data compression imposed on the
format driver.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>

Changes from v1:
- added unittest for backup compression (10, 11)

Changes from v2:
- implemented a new .bdrv_co_write_compressed interface to replace the
  old .bdrv_write_compressed (2,3,4,5,6)

Changes from v3:
- added the byte-based interfaces:
  blk_pwrite_compressed()/blk_co_pwritev_compressed() (1, 7)
- fix drive/blockdev-backup documentation (8, 9)

Pavel Butsykin (11):
  block: switch blk_write_compressed() to byte-based interface
  block/io: add bdrv_co_write_compressed
  qcow2: add qcow2_co_write_compressed
  vmdk: add vmdk_co_write_compressed
  qcow: add qcow_co_write_compressed
  block: remove BlockDriver.bdrv_write_compressed
  block: optimization blk_pwrite_compressed()
  drive-backup: added support for data compression
  blockdev-backup: added support for data compression
  qemu-iotests: test backup compression in 055
  qemu-iotests: add vmdk for test backup compression in 055

 block/backup.c                 |  13 +++++
 block/block-backend.c          |  23 +++++---
 block/io.c                     |  14 +++--
 block/qcow.c                   |  78 +++++++++++++++------------
 block/qcow2.c                  |  89 ++++++++++++++++++-------------
 block/vmdk.c                   |  56 +++----------------
 blockdev.c                     |  20 ++++++-
 hmp-commands.hx                |   8 +--
 hmp.c                          |   3 +-
 include/block/block.h          |   3 +-
 include/block/block_int.h      |   7 ++-
 include/sysemu/block-backend.h |   7 ++-
 qapi/block-core.json           |   9 +++-
 qemu-img.c                     |   8 +--
 qemu-io-cmds.c                 |   2 +-
 qmp-commands.hx                |   9 +++-
 tests/qemu-iotests/055         | 118 +++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/055.out     |   4 +-
 tests/qemu-iotests/iotests.py  |  10 ++--
 19 files changed, 322 insertions(+), 159 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

This is a preparatory patch, which continues the general trend of the transition
to the byte-based interfaces.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 8 ++++----
 block/io.c                     | 9 +++++----
 include/block/block.h          | 4 ++--
 include/sysemu/block-backend.h | 4 ++--
 qemu-img.c                     | 6 ++++--
 qemu-io-cmds.c                 | 2 +-
 6 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 34500e6..3c1fc50 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                           flags | BDRV_REQ_ZERO_WRITE);
 }
 
-int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
-                         const uint8_t *buf, int nb_sectors)
+int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
+                          int count)
 {
-    int ret = blk_check_request(blk, sector_num, nb_sectors);
+    int ret = blk_check_byte_request(blk, offset, count);
     if (ret < 0) {
         return ret;
     }
 
-    return bdrv_write_compressed(blk_bs(blk), sector_num, buf, nb_sectors);
+    return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count);
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset)
diff --git a/block/io.c b/block/io.c
index 2d832aa..c5bb6ae 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                          const uint8_t *buf, int nb_sectors)
+int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
+                           const void *buf, int count)
 {
     BlockDriver *drv = bs->drv;
     int ret;
@@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
     if (!drv->bdrv_write_compressed) {
         return -ENOTSUP;
     }
-    ret = bdrv_check_request(bs, sector_num, nb_sectors);
+    ret = bdrv_check_byte_request(bs, offset, count);
     if (ret < 0) {
         return ret;
     }
 
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
 
-    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
+    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
+                                      count >> BDRV_SECTOR_BITS);
 }
 
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
diff --git a/include/block/block.h b/include/block/block.h
index 70ea299..b687c0d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -421,8 +421,8 @@ const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
-int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                          const uint8_t *buf, int nb_sectors);
+int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
+                           const void *buf, int count);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c04af8e..57df069 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -203,8 +203,8 @@ void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
                   BlockCompletionFunc *cb, void *opaque);
 int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                       int count, BdrvRequestFlags flags);
-int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
-                         const uint8_t *buf, int nb_sectors);
+int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
+                          int count);
 int blk_truncate(BlockBackend *blk, int64_t offset);
 int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
diff --git a/qemu-img.c b/qemu-img.c
index 4b56ad3..eb744d4 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1580,7 +1580,9 @@ static int convert_write(ImgConvertState *s, int64_t sector_num, int nb_sectors,
                     break;
                 }
 
-                ret = blk_write_compressed(s->target, sector_num, buf, n);
+                ret = blk_pwrite_compressed(s->target,
+                                            sector_num << BDRV_SECTOR_BITS,
+                                            buf, n << BDRV_SECTOR_BITS);
                 if (ret < 0) {
                     return ret;
                 }
@@ -1717,7 +1719,7 @@ static int convert_do_copy(ImgConvertState *s)
 
     if (s->compressed) {
         /* signal EOF to align */
-        ret = blk_write_compressed(s->target, 0, NULL, 0);
+        ret = blk_pwrite_compressed(s->target, 0, NULL, 0);
         if (ret < 0) {
             goto fail;
         }
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 09e879f..2ae3b14 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -504,7 +504,7 @@ static int do_write_compressed(BlockBackend *blk, char *buf, int64_t offset,
         return -ERANGE;
     }
 
-    ret = blk_write_compressed(blk, offset >> 9, (uint8_t *)buf, count >> 9);
+    ret = blk_pwrite_compressed(blk, offset, buf, count);
     if (ret < 0) {
         return ret;
     }
-- 
2.1.4

[Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

This patch just adds the interface to the bdrv_co_pwritev_compressed,
which is currently not used but will be useful for safe implementation of the
bdrv_co_write_compressed callback in format drivers.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
 include/block/block_int.h |  5 +++
 qemu-img.c                |  2 +-
 3 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index c5bb6ae..54cd9a4 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
-int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
-                           const void *buf, int count)
+int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
+    int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
 {
     BlockDriver *drv = bs->drv;
     int ret;
@@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
     if (!drv) {
         return -ENOMEDIUM;
     }
-    if (!drv->bdrv_write_compressed) {
+
+    if (!drv->bdrv_co_write_compressed) {
         return -ENOTSUP;
     }
-    ret = bdrv_check_byte_request(bs, offset, count);
+
+    ret = bdrv_check_byte_request(bs, offset, bytes);
     if (ret < 0) {
         return ret;
     }
 
     assert(QLIST_EMPTY(&bs->dirty_bitmaps));
+    assert(qemu_in_coroutine());
+
+    return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
+                                         bytes >> BDRV_SECTOR_BITS, qiov);
+}
+
+typedef struct BdrvWriteCompressedCo {
+    BlockDriverState *bs;
+    int64_t offset;
+    QEMUIOVector *qiov;
+    int ret;
+} BdrvWriteCompressedCo;
+
+static void bdrv_write_compressed_co_entry(void *opaque)
+{
+    BdrvWriteCompressedCo *co = opaque;
+
+    co->ret = bdrv_co_pwritev_compressed(co->bs, co->offset, co->qiov->size,
+                                         co->qiov);
+}
+
+int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
+                           const void *buf, int count)
+{
+    BdrvWriteCompressedCo data;
+    QEMUIOVector qiov;
+    BlockDriver *drv = bs->drv;
+    struct iovec iov = {
+        .iov_base = (void *)buf,
+        .iov_len = count,
+    };
+    qemu_iovec_init_external(&qiov, &iov, 1);
 
-    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
-                                      count >> BDRV_SECTOR_BITS);
+    data = (BdrvWriteCompressedCo) {
+        .bs     = bs,
+        .offset = offset,
+        .qiov   = &qiov,
+        .ret    = -EINPROGRESS,
+    };
+
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+
+    if (drv->bdrv_write_compressed) {
+        int ret = bdrv_check_byte_request(bs, offset, count);
+        if (ret < 0) {
+            return ret;
+        }
+        assert(QLIST_EMPTY(&bs->dirty_bitmaps));
+        return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
+                                          count >> BDRV_SECTOR_BITS);
+    }
+
+    if (qemu_in_coroutine()) {
+        /* Fast-path if already in coroutine context */
+        bdrv_write_compressed_co_entry(&data);
+    } else {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+
+        Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry);
+        qemu_coroutine_enter(co, &data);
+        while (data.ret == -EINPROGRESS) {
+            aio_poll(aio_context, true);
+        }
+    }
+    return data.ret;
 }
 
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 30a9717..ccba9c9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -207,6 +207,9 @@ struct BlockDriver {
     int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
                                  const uint8_t *buf, int nb_sectors);
 
+    int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
+
     int (*bdrv_snapshot_create)(BlockDriverState *bs,
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
@@ -535,6 +538,8 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
 int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
     int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
     BdrvRequestFlags flags);
+int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
+    int64_t offset, unsigned int bytes, QEMUIOVector *qiov);
 
 int get_tmp_filename(char *filename, int size);
 BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
diff --git a/qemu-img.c b/qemu-img.c
index eb744d4..ab54027 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2024,7 +2024,7 @@ static int img_convert(int argc, char **argv)
         const char *preallocation =
             qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
 
-        if (!drv->bdrv_write_compressed) {
+        if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) {
             error_report("Compression not supported for this file format");
             ret = -1;
             goto out;
-- 
2.1.4

[Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

Added implementation of the qcow2_co_write_compressed function that
will allow us to safely use compressed writes for the qcow2 from running VMs.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 52 insertions(+), 37 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c9306a7..38caa66 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
 
 /* XXX: put compressed sectors first, then all the cluster aligned
    tables to avoid losing bytes in alignment */
-static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                                  const uint8_t *buf, int nb_sectors)
+static coroutine_fn int
+qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                          int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcow2State *s = bs->opaque;
+    QEMUIOVector hd_qiov;
+    struct iovec iov;
     z_stream strm;
     int ret, out_len;
-    uint8_t *out_buf;
+    uint8_t *buf, *out_buf;
     uint64_t cluster_offset;
 
     if (nb_sectors == 0) {
@@ -2551,29 +2554,25 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
         return bdrv_truncate(bs->file->bs, cluster_offset);
     }
 
+    buf = qemu_blockalign(bs, s->cluster_size);
     if (nb_sectors != s->cluster_sectors) {
-        ret = -EINVAL;
-
-        /* Zero-pad last write if image size is not cluster aligned */
-        if (sector_num + nb_sectors == bs->total_sectors &&
-            nb_sectors < s->cluster_sectors) {
-            uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size);
-            memset(pad_buf, 0, s->cluster_size);
-            memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE);
-            ret = qcow2_write_compressed(bs, sector_num,
-                                         pad_buf, s->cluster_sectors);
-            qemu_vfree(pad_buf);
+        if (nb_sectors > s->cluster_sectors ||
+            sector_num + nb_sectors != bs->total_sectors)
+        {
+            qemu_vfree(buf);
+            return -EINVAL;
         }
-        return ret;
+        /* Zero-pad last write if image size is not cluster aligned */
+        memset(buf, 0, s->cluster_size);
     }
+    qemu_iovec_to_buf(qiov, 0, buf, qiov->size);
 
     out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
 
     /* best compression, small window, no zlib header */
     memset(&strm, 0, sizeof(strm));
-    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
-                       Z_DEFLATED, -12,
-                       9, Z_DEFAULT_STRATEGY);
+    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+                       -12, 9, Z_DEFAULT_STRATEGY);
     if (ret != 0) {
         ret = -EINVAL;
         goto fail;
@@ -2595,34 +2594,50 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
     deflateEnd(&strm);
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
+        iov = (struct iovec) {
+            .iov_base   = buf,
+            .iov_len    = out_len,
+        };
+        qemu_iovec_init_external(&hd_qiov, &iov, 1);
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
+        ret = qcow2_co_writev(bs, sector_num, s->cluster_sectors, &hd_qiov);
         if (ret < 0) {
             goto fail;
         }
-    } else {
-        cluster_offset = qcow2_alloc_compressed_cluster_offset(bs,
-            sector_num << 9, out_len);
-        if (!cluster_offset) {
-            ret = -EIO;
-            goto fail;
-        }
-        cluster_offset &= s->cluster_offset_mask;
+        goto success;
+    }
 
-        ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
-        if (ret < 0) {
-            goto fail;
-        }
+    qemu_co_mutex_lock(&s->lock);
+    cluster_offset = \
+        qcow2_alloc_compressed_cluster_offset(bs, sector_num << 9, out_len);
+    if (!cluster_offset) {
+        qemu_co_mutex_unlock(&s->lock);
+        ret = -EIO;
+        goto fail;
+    }
+    cluster_offset &= s->cluster_offset_mask;
 
-        BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
-        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf, out_len);
-        if (ret < 0) {
-            goto fail;
-        }
+    ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
+    qemu_co_mutex_unlock(&s->lock);
+    if (ret < 0) {
+        goto fail;
     }
 
+    iov = (struct iovec) {
+        .iov_base   = out_buf,
+        .iov_len    = out_len,
+    };
+    qemu_iovec_init_external(&hd_qiov, &iov, 1);
+
+    BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
+    ret = bdrv_co_pwritev(bs->file->bs, cluster_offset, out_len, &hd_qiov, 0);
+    if (ret < 0) {
+        goto fail;
+    }
+success:
     ret = 0;
 fail:
+    qemu_vfree(buf);
     g_free(out_buf);
     return ret;
 }
@@ -3384,7 +3399,7 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_co_write_zeroes   = qcow2_co_write_zeroes,
     .bdrv_co_discard        = qcow2_co_discard,
     .bdrv_truncate          = qcow2_truncate,
-    .bdrv_write_compressed  = qcow2_write_compressed,
+    .bdrv_co_write_compressed  = qcow2_co_write_compressed,
     .bdrv_make_empty        = qcow2_make_empty,
 
     .bdrv_snapshot_create   = qcow2_snapshot_create,
-- 
2.1.4

[Qemu-devel] [PATCH 04/11] vmdk: add vmdk_co_write_compressed

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

Added implementation of the vmdk_co_write_compressed function that
will allow us to safely use compressed writes for the vmdk from running VMs.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/vmdk.c | 56 ++++++--------------------------------------------------
 1 file changed, 6 insertions(+), 50 deletions(-)

diff --git a/block/vmdk.c b/block/vmdk.c
index 372e5ed..874636e 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1652,56 +1652,12 @@ vmdk_co_pwritev(BlockDriverState *bs, uint64_t offset, uint64_t bytes,
     return ret;
 }
 
-typedef struct VmdkWriteCompressedCo {
-    BlockDriverState *bs;
-    int64_t sector_num;
-    const uint8_t *buf;
-    int nb_sectors;
-    int ret;
-} VmdkWriteCompressedCo;
-
-static void vmdk_co_write_compressed(void *opaque)
-{
-    VmdkWriteCompressedCo *co = opaque;
-    QEMUIOVector local_qiov;
-    uint64_t offset = co->sector_num * BDRV_SECTOR_SIZE;
-    uint64_t bytes = co->nb_sectors * BDRV_SECTOR_SIZE;
-
-    struct iovec iov = (struct iovec) {
-        .iov_base   = (uint8_t*) co->buf,
-        .iov_len    = bytes,
-    };
-    qemu_iovec_init_external(&local_qiov, &iov, 1);
-
-    co->ret = vmdk_pwritev(co->bs, offset, bytes, &local_qiov, false, false);
-}
-
-static int vmdk_write_compressed(BlockDriverState *bs,
-                                 int64_t sector_num,
-                                 const uint8_t *buf,
-                                 int nb_sectors)
+static int coroutine_fn
+vmdk_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                         int nb_sectors, QEMUIOVector *qiov)
 {
-    BDRVVmdkState *s = bs->opaque;
-
-    if (s->num_extents == 1 && s->extents[0].compressed) {
-        Coroutine *co;
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-        VmdkWriteCompressedCo data = {
-            .bs         = bs,
-            .sector_num = sector_num,
-            .buf        = buf,
-            .nb_sectors = nb_sectors,
-            .ret        = -EINPROGRESS,
-        };
-        co = qemu_coroutine_create(vmdk_co_write_compressed);
-        qemu_coroutine_enter(co, &data);
-        while (data.ret == -EINPROGRESS) {
-            aio_poll(aio_context, true);
-        }
-        return data.ret;
-    } else {
-        return -ENOTSUP;
-    }
+    return vmdk_co_pwritev(bs, sector_num << BDRV_SECTOR_BITS,
+                           (uint64_t)nb_sectors << BDRV_SECTOR_BITS, qiov, 0);
 }
 
 static int coroutine_fn vmdk_co_write_zeroes(BlockDriverState *bs,
@@ -2402,7 +2358,7 @@ static BlockDriver bdrv_vmdk = {
     .bdrv_reopen_prepare          = vmdk_reopen_prepare,
     .bdrv_co_preadv               = vmdk_co_preadv,
     .bdrv_co_pwritev              = vmdk_co_pwritev,
-    .bdrv_write_compressed        = vmdk_write_compressed,
+    .bdrv_co_write_compressed     = vmdk_co_write_compressed,
     .bdrv_co_write_zeroes         = vmdk_co_write_zeroes,
     .bdrv_close                   = vmdk_close,
     .bdrv_create                  = vmdk_create,
-- 
2.1.4

[Qemu-devel] [PATCH 05/11] qcow: add qcow_co_write_compressed

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

Added implementation of the qcow_co_write_compressed function that
will allow us to safely use compressed writes for the qcow from running VMs.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c | 78 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 45 insertions(+), 33 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index cb4bf02..7630811 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -910,38 +910,37 @@ static int qcow_make_empty(BlockDriverState *bs)
 
 /* XXX: put compressed sectors first, then all the cluster aligned
    tables to avoid losing bytes in alignment */
-static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
-                                 const uint8_t *buf, int nb_sectors)
+static coroutine_fn int
+qcow_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
+                         int nb_sectors, QEMUIOVector *qiov)
 {
     BDRVQcowState *s = bs->opaque;
+    QEMUIOVector hd_qiov;
+    struct iovec iov;
     z_stream strm;
     int ret, out_len;
-    uint8_t *out_buf;
+    uint8_t *buf, *out_buf;
     uint64_t cluster_offset;
 
+    buf = qemu_blockalign(bs, s->cluster_size);
     if (nb_sectors != s->cluster_sectors) {
-        ret = -EINVAL;
-
-        /* Zero-pad last write if image size is not cluster aligned */
-        if (sector_num + nb_sectors == bs->total_sectors &&
-            nb_sectors < s->cluster_sectors) {
-            uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size);
-            memset(pad_buf, 0, s->cluster_size);
-            memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE);
-            ret = qcow_write_compressed(bs, sector_num,
-                                        pad_buf, s->cluster_sectors);
-            qemu_vfree(pad_buf);
+        if (nb_sectors > s->cluster_sectors ||
+            sector_num + nb_sectors != bs->total_sectors)
+        {
+            qemu_vfree(buf);
+            return -EINVAL;
         }
-        return ret;
+        /* Zero-pad last write if image size is not cluster aligned */
+        memset(buf, 0, s->cluster_size);
     }
+    qemu_iovec_to_buf(qiov, 0, buf, qiov->size);
 
     out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
 
     /* best compression, small window, no zlib header */
     memset(&strm, 0, sizeof(strm));
-    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
-                       Z_DEFLATED, -12,
-                       9, Z_DEFAULT_STRATEGY);
+    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
+                       -12, 9, Z_DEFAULT_STRATEGY);
     if (ret != 0) {
         ret = -EINVAL;
         goto fail;
@@ -963,28 +962,41 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
     deflateEnd(&strm);
 
     if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
+        struct iovec iov = {
+            .iov_base   = buf,
+            .iov_len    = out_len,
+        };
+        qemu_iovec_init_external(&hd_qiov, &iov, 1);
         /* could not compress: write normal cluster */
-        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
-        if (ret < 0) {
-            goto fail;
-        }
-    } else {
-        cluster_offset = get_cluster_offset(bs, sector_num << 9, 2,
-                                            out_len, 0, 0);
-        if (cluster_offset == 0) {
-            ret = -EIO;
-            goto fail;
-        }
-
-        cluster_offset &= s->cluster_offset_mask;
-        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf, out_len);
+        ret = qcow_co_writev(bs, sector_num, s->cluster_sectors, &hd_qiov);
         if (ret < 0) {
             goto fail;
         }
+        goto success;
     }
 
+    qemu_co_mutex_lock(&s->lock);
+    cluster_offset = get_cluster_offset(bs, sector_num << 9, 2, out_len, 0, 0);
+    qemu_co_mutex_unlock(&s->lock);
+    if (cluster_offset == 0) {
+        ret = -EIO;
+        goto fail;
+    }
+    cluster_offset &= s->cluster_offset_mask;
+
+    iov = (struct iovec) {
+        .iov_base   = out_buf,
+        .iov_len    = out_len,
+    };
+    qemu_iovec_init_external(&hd_qiov, &iov, 1);
+    ret = bdrv_co_pwritev(bs->file->bs, cluster_offset, out_len, &hd_qiov, 0);
+    if (ret < 0) {
+        goto fail;
+    }
+success:
     ret = 0;
 fail:
+    qemu_vfree(buf);
     g_free(out_buf);
     return ret;
 }
@@ -1037,7 +1049,7 @@ static BlockDriver bdrv_qcow = {
 
     .bdrv_set_key           = qcow_set_key,
     .bdrv_make_empty        = qcow_make_empty,
-    .bdrv_write_compressed  = qcow_write_compressed,
+    .bdrv_co_write_compressed = qcow_co_write_compressed,
     .bdrv_get_info          = qcow_get_info,
 
     .create_opts            = &qcow_create_opts,
-- 
2.1.4

[Qemu-devel] [PATCH 06/11] block: remove BlockDriver.bdrv_write_compressed

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

There are no block drivers left that implement the old
.bdrv_write_compressed interface, so it can be removed now.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c                | 15 ---------------
 include/block/block_int.h |  3 ---
 qemu-img.c                |  2 +-
 3 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/block/io.c b/block/io.c
index 54cd9a4..4b06de8 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1825,7 +1825,6 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
 {
     BdrvWriteCompressedCo data;
     QEMUIOVector qiov;
-    BlockDriver *drv = bs->drv;
     struct iovec iov = {
         .iov_base = (void *)buf,
         .iov_len = count,
@@ -1839,20 +1838,6 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
         .ret    = -EINPROGRESS,
     };
 
-    if (!drv) {
-        return -ENOMEDIUM;
-    }
-
-    if (drv->bdrv_write_compressed) {
-        int ret = bdrv_check_byte_request(bs, offset, count);
-        if (ret < 0) {
-            return ret;
-        }
-        assert(QLIST_EMPTY(&bs->dirty_bitmaps));
-        return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
-                                          count >> BDRV_SECTOR_BITS);
-    }
-
     if (qemu_in_coroutine()) {
         /* Fast-path if already in coroutine context */
         bdrv_write_compressed_co_entry(&data);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ccba9c9..09b9002 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -204,9 +204,6 @@ struct BlockDriver {
     bool has_variable_length;
     int64_t (*bdrv_get_allocated_file_size)(BlockDriverState *bs);
 
-    int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
-                                 const uint8_t *buf, int nb_sectors);
-
     int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
 
diff --git a/qemu-img.c b/qemu-img.c
index ab54027..8195b93 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2024,7 +2024,7 @@ static int img_convert(int argc, char **argv)
         const char *preallocation =
             qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
 
-        if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) {
+        if (!drv->bdrv_co_write_compressed) {
             error_report("Compression not supported for this file format");
             ret = -1;
             goto out;
-- 
2.1.4

[Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
and use the blk_prw() as a generic one.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 19 +++++++++++------
 block/io.c                     | 48 ------------------------------------------
 include/block/block.h          |  3 +--
 include/sysemu/block-backend.h |  3 +++
 4 files changed, 17 insertions(+), 56 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 3c1fc50..9e1c793 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
         flags |= BDRV_REQ_FUA;
     }
 
-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
+    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
+    } else {
+        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
+    }
 }
 
 typedef struct BlkRwCo {
@@ -1480,12 +1484,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int count)
 {
-    int ret = blk_check_byte_request(blk, offset, count);
-    if (ret < 0) {
-        return ret;
-    }
+    return blk_prw(blk, offset, (void *) buf, count, blk_write_entry,
+                   BDRV_REQ_WRITE_COMPRESSED);
+}
 
-    return bdrv_pwrite_compressed(blk_bs(blk), offset, buf, count);
+int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
+                                           unsigned int bytes,
+                                           QEMUIOVector *qiov)
+{
+    return blk_co_pwritev(blk, offset, bytes, qiov, BDRV_REQ_WRITE_COMPRESSED);
 }
 
 int blk_truncate(BlockBackend *blk, int64_t offset)
diff --git a/block/io.c b/block/io.c
index 4b06de8..29f5962 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1805,54 +1805,6 @@ int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
                                          bytes >> BDRV_SECTOR_BITS, qiov);
 }
 
-typedef struct BdrvWriteCompressedCo {
-    BlockDriverState *bs;
-    int64_t offset;
-    QEMUIOVector *qiov;
-    int ret;
-} BdrvWriteCompressedCo;
-
-static void bdrv_write_compressed_co_entry(void *opaque)
-{
-    BdrvWriteCompressedCo *co = opaque;
-
-    co->ret = bdrv_co_pwritev_compressed(co->bs, co->offset, co->qiov->size,
-                                         co->qiov);
-}
-
-int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
-                           const void *buf, int count)
-{
-    BdrvWriteCompressedCo data;
-    QEMUIOVector qiov;
-    struct iovec iov = {
-        .iov_base = (void *)buf,
-        .iov_len = count,
-    };
-    qemu_iovec_init_external(&qiov, &iov, 1);
-
-    data = (BdrvWriteCompressedCo) {
-        .bs     = bs,
-        .offset = offset,
-        .qiov   = &qiov,
-        .ret    = -EINPROGRESS,
-    };
-
-    if (qemu_in_coroutine()) {
-        /* Fast-path if already in coroutine context */
-        bdrv_write_compressed_co_entry(&data);
-    } else {
-        AioContext *aio_context = bdrv_get_aio_context(bs);
-
-        Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry);
-        qemu_coroutine_enter(co, &data);
-        while (data.ret == -EINPROGRESS) {
-            aio_poll(aio_context, true);
-        }
-    }
-    return data.ret;
-}
-
 int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
                       int64_t pos, int size)
 {
diff --git a/include/block/block.h b/include/block/block.h
index b687c0d..56fcab6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -65,6 +65,7 @@ typedef enum {
     BDRV_REQ_MAY_UNMAP          = 0x4,
     BDRV_REQ_NO_SERIALISING     = 0x8,
     BDRV_REQ_FUA                = 0x10,
+    BDRV_REQ_WRITE_COMPRESSED   = 0x20,
 } BdrvRequestFlags;
 
 typedef struct BlockSizes {
@@ -421,8 +422,6 @@ const char *bdrv_get_node_name(const BlockDriverState *bs);
 const char *bdrv_get_device_name(const BlockDriverState *bs);
 const char *bdrv_get_device_or_node_name(const BlockDriverState *bs);
 int bdrv_get_flags(BlockDriverState *bs);
-int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
-                           const void *buf, int count);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs);
 void bdrv_round_to_clusters(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 57df069..3d7b446 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                       int count, BdrvRequestFlags flags);
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int count);
+int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
+                                           unsigned int bytes,
+                                           QEMUIOVector *qiov);
 int blk_truncate(BlockBackend *blk, int64_t offset);
 int blk_discard(BlockBackend *blk, int64_t sector_num, int nb_sectors);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
-- 
2.1.4

[Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

The idea is simple - backup is "written-once" data. It is written block
by block and it is large enough. It would be nice to save storage
space and compress it.

The patch adds a flag to the qmp/hmp drive-backup command which enables
block compression. Compression should be implemented in the format driver
to enable this feature.

There are some limitations of the format driver to allow compressed writes.
We can write data only once. Though for backup this is perfectly fine.
These limitations are maintained by the driver and the error will be
reported if we are doing something wrong.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c            | 13 +++++++++++++
 blockdev.c                | 12 ++++++++++--
 hmp-commands.hx           |  8 +++++---
 hmp.c                     |  3 ++-
 include/block/block_int.h |  1 +
 qapi/block-core.json      |  5 ++++-
 qmp-commands.hx           |  5 ++++-
 7 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index feeb9f8..408e26d 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -47,6 +47,7 @@ typedef struct BackupBlockJob {
     uint64_t sectors_read;
     unsigned long *done_bitmap;
     int64_t cluster_size;
+    bool compress;
     NotifierWithReturn before_write;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 } BackupBlockJob;
@@ -152,6 +153,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
         if (buffer_is_zero(iov.iov_base, iov.iov_len)) {
             ret = blk_co_pwrite_zeroes(job->target, start * job->cluster_size,
                                        bounce_qiov.size, BDRV_REQ_MAY_UNMAP);
+        } else if (job->compress) {
+            ret = blk_co_pwritev_compressed(job->target,
+                                            start * job->cluster_size,
+                                            bounce_qiov.size, &bounce_qiov);
         } else {
             ret = blk_co_pwritev(job->target, start * job->cluster_size,
                                  bounce_qiov.size, &bounce_qiov, 0);
@@ -471,6 +476,7 @@ static void coroutine_fn backup_run(void *opaque)
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
                   BdrvDirtyBitmap *sync_bitmap,
+                  bool compress,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
@@ -502,6 +508,12 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (compress && target->drv->bdrv_co_write_compressed == NULL) {
+        error_setg(errp, "Compression is not supported for this drive %s",
+                   bdrv_get_device_name(target));
+        return;
+    }
+
     if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
         return;
     }
@@ -549,6 +561,7 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
     job->sync_mode = sync_mode;
     job->sync_bitmap = sync_mode == MIRROR_SYNC_MODE_INCREMENTAL ?
                        sync_bitmap : NULL;
+    job->compress = compress;
 
     /* If there is no backing file on the target, we cannot rely on COW if our
      * backup cluster size is smaller than the target cluster size. Even for
diff --git a/blockdev.c b/blockdev.c
index 717785e..b24f508 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1840,6 +1840,7 @@ static void do_drive_backup(const char *device, const char *target,
                             bool has_mode, enum NewImageMode mode,
                             bool has_speed, int64_t speed,
                             bool has_bitmap, const char *bitmap,
+                            bool has_compress, bool compress,
                             bool has_on_source_error,
                             BlockdevOnError on_source_error,
                             bool has_on_target_error,
@@ -1880,6 +1881,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
                     backup->has_mode, backup->mode,
                     backup->has_speed, backup->speed,
                     backup->has_bitmap, backup->bitmap,
+                    backup->has_compress, backup->compress,
                     backup->has_on_source_error, backup->on_source_error,
                     backup->has_on_target_error, backup->on_target_error,
                     common->block_job_txn, &local_err);
@@ -3175,6 +3177,7 @@ static void do_drive_backup(const char *device, const char *target,
                             bool has_mode, enum NewImageMode mode,
                             bool has_speed, int64_t speed,
                             bool has_bitmap, const char *bitmap,
+                            bool has_compress, bool compress,
                             bool has_on_source_error,
                             BlockdevOnError on_source_error,
                             bool has_on_target_error,
@@ -3204,6 +3207,9 @@ static void do_drive_backup(const char *device, const char *target,
     if (!has_mode) {
         mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     }
+    if (!has_compress) {
+        compress = false;
+    }
 
     blk = blk_by_name(device);
     if (!blk) {
@@ -3290,7 +3296,7 @@ static void do_drive_backup(const char *device, const char *target,
         }
     }
 
-    backup_start(bs, target_bs, speed, sync, bmap,
+    backup_start(bs, target_bs, speed, sync, bmap, compress,
                  on_source_error, on_target_error,
                  block_job_cb, bs, txn, &local_err);
     bdrv_unref(target_bs);
@@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const char *target,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
                       bool has_bitmap, const char *bitmap,
+                      bool has_compress, bool compress,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
@@ -3316,6 +3323,7 @@ void qmp_drive_backup(const char *device, const char *target,
     return do_drive_backup(device, target, has_format, format, sync,
                            has_mode, mode, has_speed, speed,
                            has_bitmap, bitmap,
+                           has_compress, compress,
                            has_on_source_error, on_source_error,
                            has_on_target_error, on_target_error,
                            NULL, errp);
@@ -3379,7 +3387,7 @@ void do_blockdev_backup(const char *device, const char *target,
     target_bs = blk_bs(target_blk);
 
     bdrv_set_aio_context(target_bs, aio_context);
-    backup_start(bs, target_bs, speed, sync, NULL, on_source_error,
+    backup_start(bs, target_bs, speed, sync, NULL, false, on_source_error,
                  on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 98b4b1a..f91c7ba 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1181,8 +1181,8 @@ ETEXI
 
     {
         .name       = "drive_backup",
-        .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
-        .params     = "[-n] [-f] device target [format]",
+        .args_type  = "reuse:-n,full:-f,compress:-c,device:B,target:s,format:s?",
+        .params     = "[-n] [-f] [-c] device target [format]",
         .help       = "initiates a point-in-time\n\t\t\t"
                       "copy for a device. The device's contents are\n\t\t\t"
                       "copied to the new image file, excluding data that\n\t\t\t"
@@ -1190,7 +1190,9 @@ ETEXI
                       "The -n flag requests QEMU to reuse the image found\n\t\t\t"
                       "in new-image-file, instead of recreating it from scratch.\n\t\t\t"
                       "The -f flag requests QEMU to copy the whole disk,\n\t\t\t"
-                      "so that the result does not need a backing file.\n\t\t\t",
+                      "so that the result does not need a backing file.\n\t\t\t"
+                      "The -c flag requests QEMU to compress backup data\n\t\t\t"
+                      "(if the target format supports it).\n\t\t\t",
         .mhandler.cmd = hmp_drive_backup,
     },
 STEXI
diff --git a/hmp.c b/hmp.c
index a4b1d3d..cc12bfd 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1108,6 +1108,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
     const char *format = qdict_get_try_str(qdict, "format");
     bool reuse = qdict_get_try_bool(qdict, "reuse", false);
     bool full = qdict_get_try_bool(qdict, "full", false);
+    bool compress = qdict_get_try_bool(qdict, "compress", false);
     enum NewImageMode mode;
     Error *err = NULL;
 
@@ -1125,7 +1126,7 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 
     qmp_drive_backup(device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, NULL,
+                     true, mode, false, 0, false, NULL, compress, compress,
                      false, 0, false, 0, &err);
     hmp_handle_error(mon, &err);
 }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 09b9002..f7d5023 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -712,6 +712,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
                   int64_t speed, MirrorSyncMode sync_mode,
                   BdrvDirtyBitmap *sync_bitmap,
+                  bool compress,
                   BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockCompletionFunc *cb, void *opaque,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 98a20d2..e54ceb2 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -888,6 +888,9 @@
 #          Must be present if sync is "incremental", must NOT be present
 #          otherwise. (Since 2.4)
 #
+# @compress: #optional the compression data blocks (if the target format
+#            supports it; default: false).
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -905,7 +908,7 @@
 { 'struct': 'DriveBackup',
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
-            '*speed': 'int', '*bitmap': 'str',
+            '*speed': 'int', '*bitmap': 'str', '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 28801a2..ff927ae 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1186,7 +1186,8 @@ EQMP
     {
         .name       = "drive-backup",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
-                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
+                      "bitmap:s?,compress:b?,"
+                      "on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_drive_backup,
     },
 
@@ -1220,6 +1221,8 @@ Arguments:
 - "mode": whether and how QEMU should create a new image
           (NewImageMode, optional, default 'absolute-paths')
 - "speed": the maximum speed, in bytes per second (json-int, optional)
+- "compress": the compression data blocks (if the target format supports it).
+              (json-bool, optional, default false)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
-- 
2.1.4

[Qemu-devel] [PATCH 09/11] blockdev-backup: added support for data compression

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

The idea is simple - backup is "written-once" data. It is written block
by block and it is large enough. It would be nice to save storage
space and compress it.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c           | 10 +++++++++-
 qapi/block-core.json |  4 ++++
 qmp-commands.hx      |  4 +++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b24f508..e72dddf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1924,6 +1924,7 @@ typedef struct BlockdevBackupState {
 static void do_blockdev_backup(const char *device, const char *target,
                                enum MirrorSyncMode sync,
                                bool has_speed, int64_t speed,
+                               bool has_compress, bool compress,
                                bool has_on_source_error,
                                BlockdevOnError on_source_error,
                                bool has_on_target_error,
@@ -1971,6 +1972,7 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp)
     do_blockdev_backup(backup->device, backup->target,
                        backup->sync,
                        backup->has_speed, backup->speed,
+                       backup->has_compress, backup->compress,
                        backup->has_on_source_error, backup->on_source_error,
                        backup->has_on_target_error, backup->on_target_error,
                        common->block_job_txn, &local_err);
@@ -3337,6 +3339,7 @@ BlockDeviceInfoList *qmp_query_named_block_nodes(Error **errp)
 void do_blockdev_backup(const char *device, const char *target,
                          enum MirrorSyncMode sync,
                          bool has_speed, int64_t speed,
+                         bool has_compress, bool compress,
                          bool has_on_source_error,
                          BlockdevOnError on_source_error,
                          bool has_on_target_error,
@@ -3358,6 +3361,9 @@ void do_blockdev_backup(const char *device, const char *target,
     if (!has_on_target_error) {
         on_target_error = BLOCKDEV_ON_ERROR_REPORT;
     }
+    if (!has_compress) {
+        compress = false;
+    }
 
     blk = blk_by_name(device);
     if (!blk) {
@@ -3387,7 +3393,7 @@ void do_blockdev_backup(const char *device, const char *target,
     target_bs = blk_bs(target_blk);
 
     bdrv_set_aio_context(target_bs, aio_context);
-    backup_start(bs, target_bs, speed, sync, NULL, false, on_source_error,
+    backup_start(bs, target_bs, speed, sync, NULL, compress, on_source_error,
                  on_target_error, block_job_cb, bs, txn, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
@@ -3399,6 +3405,7 @@ out:
 void qmp_blockdev_backup(const char *device, const char *target,
                          enum MirrorSyncMode sync,
                          bool has_speed, int64_t speed,
+                         bool has_compress, bool compress,
                          bool has_on_source_error,
                          BlockdevOnError on_source_error,
                          bool has_on_target_error,
@@ -3406,6 +3413,7 @@ void qmp_blockdev_backup(const char *device, const char *target,
                          Error **errp)
 {
     do_blockdev_backup(device, target, sync, has_speed, speed,
+                       has_compress, compress,
                        has_on_source_error, on_source_error,
                        has_on_target_error, on_target_error,
                        NULL, errp);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index e54ceb2..f6a971c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -926,6 +926,9 @@
 # @speed: #optional the maximum speed, in bytes per second. The default is 0,
 #         for unlimited.
 #
+# @compress: #optional the compression data blocks (if the target format
+#            supports it; default: false).
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -944,6 +947,7 @@
   'data': { 'device': 'str', 'target': 'str',
             'sync': 'MirrorSyncMode',
             '*speed': 'int',
+            '*compress': 'bool',
             '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ff927ae..19a0427 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1242,7 +1242,7 @@ EQMP
 
     {
         .name       = "blockdev-backup",
-        .args_type  = "sync:s,device:B,target:B,speed:i?,"
+        .args_type  = "sync:s,device:B,target:B,speed:i?,compress:b?,"
                       "on-source-error:s?,on-target-error:s?",
         .mhandler.cmd_new = qmp_marshal_blockdev_backup,
     },
@@ -1264,6 +1264,8 @@ Arguments:
           sectors allocated in the topmost image, or "none" to only replicate
           new I/O (MirrorSyncMode).
 - "speed": the maximum speed, in bytes per second (json-int, optional)
+- "compress": the compression data blocks (if the target format supports it).
+              (json-bool, optional, default false)
 - "on-source-error": the action to take on an error on the source, default
                      'report'.  'stop' and 'enospc' can only be used
                      if the block device supports io-status.
-- 
2.1.4

[Qemu-devel] [PATCH 10/11] qemu-iotests: test backup compression in 055

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

Added cases to check the backup compression out of qcow2, raw in qcow2
on drive-backup and blockdev-backup.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/055        | 97 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/055.out    |  4 +-
 tests/qemu-iotests/iotests.py | 10 ++---
 3 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index c8e3578..3ac6894 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -451,5 +451,102 @@ class TestSingleTransaction(iotests.QMPTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_no_active_block_jobs()
 
+
+class TestDriveCompression(iotests.QMPTestCase):
+    image_len = 64 * 1024 * 1024 # MB
+    outfmt = 'qcow2'
+
+    def setUp(self):
+        # Write data to the image so we can compare later
+        qemu_img('create', '-f', iotests.imgfmt, test_img, str(TestDriveCompression.image_len))
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x11 0 64k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x00 64k 128k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x22 162k 32k', test_img)
+        qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x33 67043328 64k', test_img)
+
+        qemu_img('create', '-f', TestDriveCompression.outfmt, blockdev_target_img,
+                 str(TestDriveCompression.image_len))
+        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img,
+                                                             format=TestDriveCompression.outfmt)
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(blockdev_target_img)
+        try:
+            os.remove(target_img)
+        except OSError:
+            pass
+
+    def do_test_compress_complete(self, cmd, **args):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed()
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,
+                                               iotests.imgfmt, TestDriveCompression.outfmt),
+                        'target image does not match source after backup')
+
+    def test_complete_compress_drive_bakup(self):
+        self.do_test_compress_complete('drive-backup', target=blockdev_target_img, mode='existing')
+
+    def test_complete_compress_blockdev_backup(self):
+        self.do_test_compress_complete('blockdev-backup', target='drive1')
+
+    def do_test_compress_cancel(self, cmd, **args):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
+        self.assert_qmp(result, 'return', {})
+
+        event = self.cancel_and_wait()
+        self.assert_qmp(event, 'data/type', 'backup')
+
+    def test_compress_cancel_drive_bakup(self):
+        self.do_test_compress_cancel('drive-backup', target=blockdev_target_img, mode='existing')
+
+    def test_compress_cancel_blockdev_backup(self):
+        self.do_test_compress_cancel('blockdev-backup', target='drive1')
+
+    def do_test_compress_pause(self, cmd, **args):
+        self.assert_no_active_block_jobs()
+
+        self.vm.pause_drive('drive0')
+        result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('block-job-pause', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.resume_drive('drive0')
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        offset = self.dictpath(result, 'return[0]/offset')
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/offset', offset)
+
+        result = self.vm.qmp('block-job-resume', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed()
+
+        self.vm.shutdown()
+        self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,
+                                               iotests.imgfmt, TestDriveCompression.outfmt),
+                        'target image does not match source after backup')
+
+    def test_compress_pause_drive_bakup(self):
+        self.do_test_compress_pause('drive-backup', target=blockdev_target_img, mode='existing')
+
+    def test_compress_pause_blockdev_backup(self):
+        self.do_test_compress_pause('blockdev-backup', target='drive1')
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 42314e9..5ce2f9a 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-........................
+..............................
 ----------------------------------------------------------------------
-Ran 24 tests
+Ran 30 tests
 
 OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 1687c33..68830a42 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -89,10 +89,10 @@ def qemu_io(*args):
         sys.stderr.write('qemu-io received signal %i: %s\n' % (-exitcode, ' '.join(args)))
     return subp.communicate()[0]
 
-def compare_images(img1, img2):
+def compare_images(img1, img2, fmt1=imgfmt, fmt2=imgfmt):
     '''Return True if two image files are identical'''
-    return qemu_img('compare', '-f', imgfmt,
-                    '-F', imgfmt, img1, img2) == 0
+    return qemu_img('compare', '-f', fmt1,
+                    '-F', fmt2, img1, img2) == 0
 
 def create_image(name, size):
     '''Create a fully-allocated raw image with sector markers'''
@@ -175,14 +175,14 @@ class VM(object):
         self._args.append(opts)
         return self
 
-    def add_drive(self, path, opts='', interface='virtio'):
+    def add_drive(self, path, opts='', interface='virtio', format=imgfmt):
         '''Add a virtio-blk drive to the VM'''
         options = ['if=%s' % interface,
                    'id=drive%d' % self._num_drives]
 
         if path is not None:
             options.append('file=%s' % path)
-            options.append('format=%s' % imgfmt)
+            options.append('format=%s' % format)
             options.append('cache=%s' % cachemode)
 
         if opts:
-- 
2.1.4

[Qemu-devel] [PATCH 11/11] qemu-iotests: add vmdk for test backup compression in 055

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

The vmdk format has support for compression, it would be fine to add it for
the test backup compression

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Jeff Cody <jcody@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: John Snow <jsnow@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/055 | 57 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 3ac6894..3d26fd3 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -454,7 +454,8 @@ class TestSingleTransaction(iotests.QMPTestCase):
 
 class TestDriveCompression(iotests.QMPTestCase):
     image_len = 64 * 1024 * 1024 # MB
-    outfmt = 'qcow2'
+    fmt_supports_compression = [{'type': 'qcow2', 'args': ()},
+                                {'type': 'vmdk', 'args': ('-o', 'subformat=streamOptimized')}]
 
     def setUp(self):
         # Write data to the image so we can compare later
@@ -464,12 +465,6 @@ class TestDriveCompression(iotests.QMPTestCase):
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x22 162k 32k', test_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P0x33 67043328 64k', test_img)
 
-        qemu_img('create', '-f', TestDriveCompression.outfmt, blockdev_target_img,
-                 str(TestDriveCompression.image_len))
-        self.vm = iotests.VM().add_drive(test_img).add_drive(blockdev_target_img,
-                                                             format=TestDriveCompression.outfmt)
-        self.vm.launch()
-
     def tearDown(self):
         self.vm.shutdown()
         os.remove(test_img)
@@ -479,7 +474,18 @@ class TestDriveCompression(iotests.QMPTestCase):
         except OSError:
             pass
 
-    def do_test_compress_complete(self, cmd, **args):
+    def do_prepare_drives(self, fmt, args):
+        self.vm = iotests.VM().add_drive(test_img)
+
+        qemu_img('create', '-f', fmt, blockdev_target_img,
+                 str(TestDriveCompression.image_len), *args)
+        self.vm.add_drive(blockdev_target_img, format=fmt)
+
+        self.vm.launch()
+
+    def do_test_compress_complete(self, cmd, format, **args):
+        self.do_prepare_drives(format['type'], format['args'])
+
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
@@ -489,16 +495,21 @@ class TestDriveCompression(iotests.QMPTestCase):
 
         self.vm.shutdown()
         self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,
-                                               iotests.imgfmt, TestDriveCompression.outfmt),
+                                               iotests.imgfmt, format['type']),
                         'target image does not match source after backup')
 
     def test_complete_compress_drive_bakup(self):
-        self.do_test_compress_complete('drive-backup', target=blockdev_target_img, mode='existing')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_complete('drive-backup', format,
+                                           target=blockdev_target_img, mode='existing')
 
     def test_complete_compress_blockdev_backup(self):
-        self.do_test_compress_complete('blockdev-backup', target='drive1')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_complete('blockdev-backup', format, target='drive1')
+
+    def do_test_compress_cancel(self, cmd, format, **args):
+        self.do_prepare_drives(format['type'], format['args'])
 
-    def do_test_compress_cancel(self, cmd, **args):
         self.assert_no_active_block_jobs()
 
         result = self.vm.qmp(cmd, device='drive0', sync='full', compress=True, **args)
@@ -507,13 +518,20 @@ class TestDriveCompression(iotests.QMPTestCase):
         event = self.cancel_and_wait()
         self.assert_qmp(event, 'data/type', 'backup')
 
+        self.vm.shutdown()
+
     def test_compress_cancel_drive_bakup(self):
-        self.do_test_compress_cancel('drive-backup', target=blockdev_target_img, mode='existing')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_cancel('drive-backup', format,
+                                         target=blockdev_target_img, mode='existing')
 
     def test_compress_cancel_blockdev_backup(self):
-        self.do_test_compress_cancel('blockdev-backup', target='drive1')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_cancel('blockdev-backup', format, target='drive1')
+
+    def do_test_compress_pause(self, cmd, format, **args):
+        self.do_prepare_drives(format['type'], format['args'])
 
-    def do_test_compress_pause(self, cmd, **args):
         self.assert_no_active_block_jobs()
 
         self.vm.pause_drive('drive0')
@@ -539,14 +557,17 @@ class TestDriveCompression(iotests.QMPTestCase):
 
         self.vm.shutdown()
         self.assertTrue(iotests.compare_images(test_img, blockdev_target_img,
-                                               iotests.imgfmt, TestDriveCompression.outfmt),
+                                               iotests.imgfmt, format['type']),
                         'target image does not match source after backup')
 
     def test_compress_pause_drive_bakup(self):
-        self.do_test_compress_pause('drive-backup', target=blockdev_target_img, mode='existing')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_pause('drive-backup', format,
+                                        target=blockdev_target_img, mode='existing')
 
     def test_compress_pause_blockdev_backup(self):
-        self.do_test_compress_pause('blockdev-backup', target='drive1')
+        for format in TestDriveCompression.fmt_supports_compression:
+            self.do_test_compress_pause('blockdev-backup', format, target='drive1')
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['raw', 'qcow2'])
-- 
2.1.4

Re: [Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Tue, May 31, 2016 at 12:15:20PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This is a preparatory patch, which continues the general trend of the transition
> to the byte-based interfaces.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 8 ++++----
>  block/io.c                     | 9 +++++----
>  include/block/block.h          | 4 ++--
>  include/sysemu/block-backend.h | 4 ++--
>  qemu-img.c                     | 6 ++++--
>  qemu-io-cmds.c                 | 2 +-
>  6 files changed, 18 insertions(+), 15 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1082 bytes --]

On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 57df069..3d7b446 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>                                        int count, BdrvRequestFlags flags);
>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>                            int count);
> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
> +                                           unsigned int bytes,
> +                                           QEMUIOVector *qiov);

Perhaps blk_co_pwritev_compressed() isn't necessary at all since
blk_co_pwritev() already exists and has the flags argument:

int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
                               unsigned int bytes, QEMUIOVector *qiov,
                               BdrvRequestFlags flags);

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 10/11] qemu-iotests: test backup compression in 055

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 203 bytes --]

On Tue, May 31, 2016 at 12:15:29PM +0300, Denis V. Lunev wrote:
> +    def test_complete_compress_drive_bakup(self):

s/bakup/backup/

> +    def test_compress_cancel_drive_bakup(self):

s/bakup/backup/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 11/11] qemu-iotests: add vmdk for test backup compression in 055

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On Tue, May 31, 2016 at 12:15:30PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> The vmdk format has support for compression, it would be fine to add it for
> the test backup compression
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/055 | 57 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 39 insertions(+), 18 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]

On Tue, May 31, 2016 at 12:15:21PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This patch just adds the interface to the bdrv_co_pwritev_compressed,
> which is currently not used but will be useful for safe implementation of the
> bdrv_co_write_compressed callback in format drivers.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
>  include/block/block_int.h |  5 +++
>  qemu-img.c                |  2 +-
>  3 files changed, 78 insertions(+), 7 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 819 bytes --]

On Tue, May 31, 2016 at 12:15:22PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the qcow2_co_write_compressed function that
> will allow us to safely use compressed writes for the qcow2 from running VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 04/11] vmdk: add vmdk_co_write_compressed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

On Tue, May 31, 2016 at 12:15:23PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the vmdk_co_write_compressed function that
> will allow us to safely use compressed writes for the vmdk from running VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/vmdk.c | 56 ++++++--------------------------------------------------
>  1 file changed, 6 insertions(+), 50 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 05/11] qcow: add qcow_co_write_compressed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Tue, May 31, 2016 at 12:15:24PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the qcow_co_write_compressed function that
> will allow us to safely use compressed writes for the qcow from running VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow.c | 78 +++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 45 insertions(+), 33 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 06/11] block: remove BlockDriver.bdrv_write_compressed

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 832 bytes --]

On Tue, May 31, 2016 at 12:15:25PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> There are no block drivers left that implement the old
> .bdrv_write_compressed interface, so it can be removed now.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 15 ---------------
>  include/block/block_int.h |  3 ---
>  qemu-img.c                |  2 +-
>  3 files changed, 1 insertion(+), 19 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1510 bytes --]

On Tue, May 31, 2016 at 12:15:27PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> The idea is simple - backup is "written-once" data. It is written block
> by block and it is large enough. It would be nice to save storage
> space and compress it.
> 
> The patch adds a flag to the qmp/hmp drive-backup command which enables
> block compression. Compression should be implemented in the format driver
> to enable this feature.
> 
> There are some limitations of the format driver to allow compressed writes.
> We can write data only once. Though for backup this is perfectly fine.
> These limitations are maintained by the driver and the error will be
> reported if we are doing something wrong.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/backup.c            | 13 +++++++++++++
>  blockdev.c                | 12 ++++++++++--
>  hmp-commands.hx           |  8 +++++---
>  hmp.c                     |  3 ++-
>  include/block/block_int.h |  1 +
>  qapi/block-core.json      |  5 ++++-
>  qmp-commands.hx           |  5 ++++-
>  7 files changed, 39 insertions(+), 8 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 09/11] blockdev-backup: added support for data compression

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Tue, May 31, 2016 at 12:15:28PM +0300, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> The idea is simple - backup is "written-once" data. It is written block
> by block and it is large enough. It would be nice to save storage
> space and compress it.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c           | 10 +++++++++-
>  qapi/block-core.json |  4 ++++
>  qmp-commands.hx      |  4 +++-
>  3 files changed, 16 insertions(+), 2 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This is a preparatory patch, which continues the general trend of the transition
> to the byte-based interfaces.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c          | 8 ++++----
>  block/io.c                     | 9 +++++----
>  include/block/block.h          | 4 ++--
>  include/sysemu/block-backend.h | 4 ++--
>  qemu-img.c                     | 6 ++++--
>  qemu-io-cmds.c                 | 2 +-
>  6 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 34500e6..3c1fc50 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>                            flags | BDRV_REQ_ZERO_WRITE);
>  }
>  
> -int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
> -                         const uint8_t *buf, int nb_sectors)
> +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
> +                          int count)

Why are you switching the type of buf?  It's not necessarily wrong, but
the commit message should call it out as intentional.


> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                          const uint8_t *buf, int nb_sectors)
> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> +                           const void *buf, int count)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      if (!drv->bdrv_write_compressed) {
>          return -ENOTSUP;
>      }
> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
> +    ret = bdrv_check_byte_request(bs, offset, count);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>  
> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> +                                      count >> BDRV_SECTOR_BITS);

If you are going to shift right, you need to first assert that offset
and count are aligned (and thus that our call to a sector interface
isn't going to operate on the wrong data).  See for example commit 166fe960.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 6512 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This patch just adds the interface to the bdrv_co_pwritev_compressed,
> which is currently not used but will be useful for safe implementation of the
> bdrv_co_write_compressed callback in format drivers.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
>  include/block/block_int.h |  5 +++
>  qemu-img.c                |  2 +-
>  3 files changed, 78 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index c5bb6ae..54cd9a4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> -                           const void *buf, int count)
> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov)

Why the rename s/count/bytes/?  Would it be better to get the name right
in 1/11?

>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
> @@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>      if (!drv) {
>          return -ENOMEDIUM;
>      }
> -    if (!drv->bdrv_write_compressed) {
> +
> +    if (!drv->bdrv_co_write_compressed) {
>          return -ENOTSUP;

This is a (temporary) regression - since none of the drivers have
.bdrv_co_write_compressed yet, you will always fail.  Rather, the
transition period should support both interfaces at once...

>      }
> -    ret = bdrv_check_byte_request(bs, offset, count);
> +
> +    ret = bdrv_check_byte_request(bs, offset, bytes);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +    assert(qemu_in_coroutine());
> +
> +    return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
> +                                         bytes >> BDRV_SECTOR_BITS, qiov);

...and call into either the old or the new interface according to what
is present.


> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> +                           const void *buf, int count)
> +{
> +    BdrvWriteCompressedCo data;
> +    QEMUIOVector qiov;
> +    BlockDriver *drv = bs->drv;
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,
> +        .iov_len = count,
> +    };
> +    qemu_iovec_init_external(&qiov, &iov, 1);
>  
> -    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> -                                      count >> BDRV_SECTOR_BITS);
> +    data = (BdrvWriteCompressedCo) {
> +        .bs     = bs,
> +        .offset = offset,
> +        .qiov   = &qiov,
> +        .ret    = -EINPROGRESS,
> +    };
> +
> +    if (!drv) {
> +        return -ENOMEDIUM;
> +    }
> +
> +    if (drv->bdrv_write_compressed) {
> +        int ret = bdrv_check_byte_request(bs, offset, count);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +        assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +        return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> +                                          count >> BDRV_SECTOR_BITS);
> +    }

Oh, you're catering to the old code up front, without a coroutine, and
only the new code gets coroutine treatment.  Maybe it's okay after all.

> +
> +    if (qemu_in_coroutine()) {
> +        /* Fast-path if already in coroutine context */
> +        bdrv_write_compressed_co_entry(&data);
> +    } else {
> +        AioContext *aio_context = bdrv_get_aio_context(bs);
> +
> +        Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry);
> +        qemu_coroutine_enter(co, &data);
> +        while (data.ret == -EINPROGRESS) {
> +            aio_poll(aio_context, true);
> +        }
> +    }
> +    return data.ret;
>  }
>  
>  int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 30a9717..ccba9c9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -207,6 +207,9 @@ struct BlockDriver {
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>                                   const uint8_t *buf, int nb_sectors);
>  
> +    int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,

Might be better to name this bdrv_co_pwrite_compressed if we want to
make it byte-based...

> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);

...it seems odd that you have to add a new sector-based interface given
that you are trying to convert to byte-based.

> +
>      int (*bdrv_snapshot_create)(BlockDriverState *bs,
>                                  QEMUSnapshotInfo *sn_info);
>      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
> @@ -535,6 +538,8 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
>  int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
>      int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>      BdrvRequestFlags flags);
> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov);
>  
>  int get_tmp_filename(char *filename, int size);
>  BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
> diff --git a/qemu-img.c b/qemu-img.c
> index eb744d4..ab54027 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -2024,7 +2024,7 @@ static int img_convert(int argc, char **argv)
>          const char *preallocation =
>              qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
>  
> -        if (!drv->bdrv_write_compressed) {
> +        if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) {
>              error_report("Compression not supported for this file format");
>              ret = -1;
>              goto out;
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1611 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the qcow2_co_write_compressed function that
> will allow us to safely use compressed writes for the qcow2 from running VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c9306a7..38caa66 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>  
>  /* XXX: put compressed sectors first, then all the cluster aligned
>     tables to avoid losing bytes in alignment */
> -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                                  const uint8_t *buf, int nb_sectors)
> +static coroutine_fn int
> +qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> +                          int nb_sectors, QEMUIOVector *qiov)

Is it worth converting to a byte-based qcow2_co_pwrite_compressed()
while at it?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote:
> On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
>> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
>> index 57df069..3d7b446 100644
>> --- a/include/sysemu/block-backend.h
>> +++ b/include/sysemu/block-backend.h
>> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>>                                        int count, BdrvRequestFlags flags);
>>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>>                            int count);
>> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
>> +                                           unsigned int bytes,
>> +                                           QEMUIOVector *qiov);
> 
> Perhaps blk_co_pwritev_compressed() isn't necessary at all since
> blk_co_pwritev() already exists and has the flags argument:
> 
> int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>                                unsigned int bytes, QEMUIOVector *qiov,
>                                BdrvRequestFlags flags);

Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that
can be set in .supported_write_flags for drivers that know how to do a
compressed write?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> The idea is simple - backup is "written-once" data. It is written block
> by block and it is large enough. It would be nice to save storage
> space and compress it.
> 
> The patch adds a flag to the qmp/hmp drive-backup command which enables
> block compression. Compression should be implemented in the format driver
> to enable this feature.
> 
> There are some limitations of the format driver to allow compressed writes.
> We can write data only once. Though for backup this is perfectly fine.
> These limitations are maintained by the driver and the error will be
> reported if we are doing something wrong.
> 

> @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const char *target,
>                        bool has_mode, enum NewImageMode mode,
>                        bool has_speed, int64_t speed,
>                        bool has_bitmap, const char *bitmap,
> +                      bool has_compress, bool compress,
>                        bool has_on_source_error, BlockdevOnError on_source_error,
>                        bool has_on_target_error, BlockdevOnError on_target_error,
>                        Error **errp)

Might be nice to simplify this signature once my qapi 'box' patches land.


> +++ b/qapi/block-core.json
> @@ -888,6 +888,9 @@
>  #          Must be present if sync is "incremental", must NOT be present
>  #          otherwise. (Since 2.4)
>  #
> +# @compress: #optional the compression data blocks (if the target format
> +#            supports it; default: false).

Missing a '(since 2.7)' notation.

> +++ b/qmp-commands.hx
> @@ -1186,7 +1186,8 @@ EQMP
>      {
>          .name       = "drive-backup",
>          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
> -                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
> +                      "bitmap:s?,compress:b?,"
> +                      "on-source-error:s?,on-target-error:s?",
>          .mhandler.cmd_new = qmp_marshal_drive_backup,
>      },
>  
> @@ -1220,6 +1221,8 @@ Arguments:
>  - "mode": whether and how QEMU should create a new image
>            (NewImageMode, optional, default 'absolute-paths')
>  - "speed": the maximum speed, in bytes per second (json-int, optional)
> +- "compress": the compression data blocks (if the target format supports it).
> +              (json-bool, optional, default false)

Reads awkwardly; maybe "compress": true to compress data, if the target
format supports it. (json-bool, optional, default false)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 09/11] blockdev-backup: added support for data compression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> The idea is simple - backup is "written-once" data. It is written block
> by block and it is large enough. It would be nice to save storage
> space and compress it.
> 

> +++ b/qapi/block-core.json
> @@ -926,6 +926,9 @@
>  # @speed: #optional the maximum speed, in bytes per second. The default is 0,
>  #         for unlimited.
>  #
> +# @compress: #optional the compression data blocks (if the target format
> +#            supports it; default: false).
> +#

Missing a '(since 2.7)' designation.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1576 bytes --]

On Mon, Jun 13, 2016 at 02:16:08PM -0600, Eric Blake wrote:
> On 06/13/2016 07:11 AM, Stefan Hajnoczi wrote:
> > On Tue, May 31, 2016 at 12:15:26PM +0300, Denis V. Lunev wrote:
> >> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> >> index 57df069..3d7b446 100644
> >> --- a/include/sysemu/block-backend.h
> >> +++ b/include/sysemu/block-backend.h
> >> @@ -205,6 +205,9 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
> >>                                        int count, BdrvRequestFlags flags);
> >>  int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
> >>                            int count);
> >> +int coroutine_fn blk_co_pwritev_compressed(BlockBackend *blk, int64_t offset,
> >> +                                           unsigned int bytes,
> >> +                                           QEMUIOVector *qiov);
> > 
> > Perhaps blk_co_pwritev_compressed() isn't necessary at all since
> > blk_co_pwritev() already exists and has the flags argument:
> > 
> > int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> >                                unsigned int bytes, QEMUIOVector *qiov,
> >                                BdrvRequestFlags flags);
> 
> Are you arguing that we should have a new BDRV_REQ_COMPRESSED flag that
> can be set in .supported_write_flags for drivers that know how to do a
> compressed write?

Never mind, it's too much noise and out of scope for this series.

I'm fine with blk_co_pwritev_compressed().

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Pavel Butsykin]

On 13.06.2016 17:23, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This is a preparatory patch, which continues the general trend of the transition
>> to the byte-based interfaces.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/block-backend.c          | 8 ++++----
>>   block/io.c                     | 9 +++++----
>>   include/block/block.h          | 4 ++--
>>   include/sysemu/block-backend.h | 4 ++--
>>   qemu-img.c                     | 6 ++++--
>>   qemu-io-cmds.c                 | 2 +-
>>   6 files changed, 18 insertions(+), 15 deletions(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 34500e6..3c1fc50 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1477,15 +1477,15 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
>>                             flags | BDRV_REQ_ZERO_WRITE);
>>   }
>>
>> -int blk_write_compressed(BlockBackend *blk, int64_t sector_num,
>> -                         const uint8_t *buf, int nb_sectors)
>> +int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
>> +                          int count)
>
> Why are you switching the type of buf?  It's not necessarily wrong, but
> the commit message should call it out as intentional.

Here I just tried to make the interface like blk_pwrite, it has no other
meaning more..

>
>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                          const uint8_t *buf, int nb_sectors)
>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>> +                           const void *buf, int count)
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       if (!drv->bdrv_write_compressed) {
>>           return -ENOTSUP;
>>       }
>> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
>> +    ret = bdrv_check_byte_request(bs, offset, count);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>
>> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
>> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>> +                                      count >> BDRV_SECTOR_BITS);
>
> If you are going to shift right, you need to first assert that offset
> and count are aligned (and thus that our call to a sector interface
> isn't going to operate on the wrong data).  See for example commit 166fe960.
>

ok, thanks

Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Pavel Butsykin]

On 13.06.2016 17:32, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This patch just adds the interface to the bdrv_co_pwritev_compressed,
>> which is currently not used but will be useful for safe implementation of the
>> bdrv_co_write_compressed callback in format drivers.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
>>   include/block/block_int.h |  5 +++
>>   qemu-img.c                |  2 +-
>>   3 files changed, 78 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index c5bb6ae..54cd9a4 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>
>> -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>> -                           const void *buf, int count)
>> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
>> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
>
> Why the rename s/count/bytes/?  Would it be better to get the name right
> in 1/11?

It made like the bdrv_co_pwritev interface. For the
bdrv_pwrite_compressed() func also need rename s/count/bytes/ in 1/11.

>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>>       if (!drv) {
>>           return -ENOMEDIUM;
>>       }
>> -    if (!drv->bdrv_write_compressed) {
>> +
>> +    if (!drv->bdrv_co_write_compressed) {
>>           return -ENOTSUP;
>
> This is a (temporary) regression - since none of the drivers have
> .bdrv_co_write_compressed yet, you will always fail.  Rather, the
> transition period should support both interfaces at once...
>
>>       }
>> -    ret = bdrv_check_byte_request(bs, offset, count);
>> +
>> +    ret = bdrv_check_byte_request(bs, offset, bytes);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>> +    assert(qemu_in_coroutine());
>> +
>> +    return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
>> +                                         bytes >> BDRV_SECTOR_BITS, qiov);
>
> ...and call into either the old or the new interface according to what
> is present.
>
>
>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>> +                           const void *buf, int count)
>> +{
>> +    BdrvWriteCompressedCo data;
>> +    QEMUIOVector qiov;
>> +    BlockDriver *drv = bs->drv;
>> +    struct iovec iov = {
>> +        .iov_base = (void *)buf,
>> +        .iov_len = count,
>> +    };
>> +    qemu_iovec_init_external(&qiov, &iov, 1);
>>
>> -    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>> -                                      count >> BDRV_SECTOR_BITS);
>> +    data = (BdrvWriteCompressedCo) {
>> +        .bs     = bs,
>> +        .offset = offset,
>> +        .qiov   = &qiov,
>> +        .ret    = -EINPROGRESS,
>> +    };
>> +
>> +    if (!drv) {
>> +        return -ENOMEDIUM;
>> +    }
>> +
>> +    if (drv->bdrv_write_compressed) {
>> +        int ret = bdrv_check_byte_request(bs, offset, count);
>> +        if (ret < 0) {
>> +            return ret;
>> +        }
>> +        assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>> +        return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>> +                                          count >> BDRV_SECTOR_BITS);
>> +    }
>
> Oh, you're catering to the old code up front, without a coroutine, and
> only the new code gets coroutine treatment.  Maybe it's okay after all.

Yes, it is made for a gradual transition from
drv->bdrv_write_compressed to drv->bdrv_co_write_compressed.

>> +
>> +    if (qemu_in_coroutine()) {
>> +        /* Fast-path if already in coroutine context */
>> +        bdrv_write_compressed_co_entry(&data);
>> +    } else {
>> +        AioContext *aio_context = bdrv_get_aio_context(bs);
>> +
>> +        Coroutine *co = qemu_coroutine_create(bdrv_write_compressed_co_entry);
>> +        qemu_coroutine_enter(co, &data);
>> +        while (data.ret == -EINPROGRESS) {
>> +            aio_poll(aio_context, true);
>> +        }
>> +    }
>> +    return data.ret;
>>   }
>>
>>   int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf,
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 30a9717..ccba9c9 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -207,6 +207,9 @@ struct BlockDriver {
>>       int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>>                                    const uint8_t *buf, int nb_sectors);
>>
>> +    int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
>
> Might be better to name this bdrv_co_pwrite_compressed if we want to
> make it byte-based...
>
>> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
>
> ...it seems odd that you have to add a new sector-based interface given
> that you are trying to convert to byte-based.
>

When I sent this patch, Kevin had not yet converted the sector-based
interfaces to byte-based for format drivers :)

>> +
>>       int (*bdrv_snapshot_create)(BlockDriverState *bs,
>>                                   QEMUSnapshotInfo *sn_info);
>>       int (*bdrv_snapshot_goto)(BlockDriverState *bs,
>> @@ -535,6 +538,8 @@ int coroutine_fn bdrv_co_preadv(BlockDriverState *bs,
>>   int coroutine_fn bdrv_co_pwritev(BlockDriverState *bs,
>>       int64_t offset, unsigned int bytes, QEMUIOVector *qiov,
>>       BdrvRequestFlags flags);
>> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
>> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov);
>>
>>   int get_tmp_filename(char *filename, int size);
>>   BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
>> diff --git a/qemu-img.c b/qemu-img.c
>> index eb744d4..ab54027 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -2024,7 +2024,7 @@ static int img_convert(int argc, char **argv)
>>           const char *preallocation =
>>               qemu_opt_get(opts, BLOCK_OPT_PREALLOC);
>>
>> -        if (!drv->bdrv_write_compressed) {
>> +        if (!drv->bdrv_write_compressed && !drv->bdrv_co_write_compressed) {
>>               error_report("Compression not supported for this file format");
>>               ret = -1;
>>               goto out;
>>
>

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Pavel Butsykin]

On 13.06.2016 23:14, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> Added implementation of the qcow2_co_write_compressed function that
>> will allow us to safely use compressed writes for the qcow2 from running VMs.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 52 insertions(+), 37 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c9306a7..38caa66 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>
>>   /* XXX: put compressed sectors first, then all the cluster aligned
>>      tables to avoid losing bytes in alignment */
>> -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                                  const uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int
>> +qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> +                          int nb_sectors, QEMUIOVector *qiov)
>
> Is it worth converting to a byte-based qcow2_co_pwrite_compressed()
> while at it?
>

Yes, I'll do it for the next version.

Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Pavel Butsykin]

On 13.06.2016 23:18, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> The idea is simple - backup is "written-once" data. It is written block
>> by block and it is large enough. It would be nice to save storage
>> space and compress it.
>>
>> The patch adds a flag to the qmp/hmp drive-backup command which enables
>> block compression. Compression should be implemented in the format driver
>> to enable this feature.
>>
>> There are some limitations of the format driver to allow compressed writes.
>> We can write data only once. Though for backup this is perfectly fine.
>> These limitations are maintained by the driver and the error will be
>> reported if we are doing something wrong.
>>
>
>> @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const char *target,
>>                         bool has_mode, enum NewImageMode mode,
>>                         bool has_speed, int64_t speed,
>>                         bool has_bitmap, const char *bitmap,
>> +                      bool has_compress, bool compress,
>>                         bool has_on_source_error, BlockdevOnError on_source_error,
>>                         bool has_on_target_error, BlockdevOnError on_target_error,
>>                         Error **errp)
>
> Might be nice to simplify this signature once my qapi 'box' patches land.
>

Yes, it would certainly be better. But your patches are not applied on
the current, maybe you have your own branch?

>
>> +++ b/qapi/block-core.json
>> @@ -888,6 +888,9 @@
>>   #          Must be present if sync is "incremental", must NOT be present
>>   #          otherwise. (Since 2.4)
>>   #
>> +# @compress: #optional the compression data blocks (if the target format
>> +#            supports it; default: false).
>
> Missing a '(since 2.7)' notation.
>
>> +++ b/qmp-commands.hx
>> @@ -1186,7 +1186,8 @@ EQMP
>>       {
>>           .name       = "drive-backup",
>>           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>> -                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
>> +                      "bitmap:s?,compress:b?,"
>> +                      "on-source-error:s?,on-target-error:s?",
>>           .mhandler.cmd_new = qmp_marshal_drive_backup,
>>       },
>>
>> @@ -1220,6 +1221,8 @@ Arguments:
>>   - "mode": whether and how QEMU should create a new image
>>             (NewImageMode, optional, default 'absolute-paths')
>>   - "speed": the maximum speed, in bytes per second (json-int, optional)
>> +- "compress": the compression data blocks (if the target format supports it).
>> +              (json-bool, optional, default false)
>
> Reads awkwardly; maybe "compress": true to compress data, if the target
> format supports it. (json-bool, optional, default false)
>
>

Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On 06/23/2016 03:15 AM, Pavel Butsykin wrote:

>>> @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const
>>> char *target,
>>>                         bool has_mode, enum NewImageMode mode,
>>>                         bool has_speed, int64_t speed,
>>>                         bool has_bitmap, const char *bitmap,
>>> +                      bool has_compress, bool compress,
>>>                         bool has_on_source_error, BlockdevOnError
>>> on_source_error,
>>>                         bool has_on_target_error, BlockdevOnError
>>> on_target_error,
>>>                         Error **errp)
>>
>> Might be nice to simplify this signature once my qapi 'box' patches land.
>>
> 
> Yes, it would certainly be better. But your patches are not applied on
> the current, maybe you have your own branch?

http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi (still
undergoing rebases, and I need to address some of Markus' review
comments, but it's getting closer and still targetted for inclusion in 2.7)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> and use the blk_prw() as a generic one.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---

> +++ b/include/block/block.h
> @@ -65,6 +65,7 @@ typedef enum {
>      BDRV_REQ_MAY_UNMAP          = 0x4,
>      BDRV_REQ_NO_SERIALISING     = 0x8,
>      BDRV_REQ_FUA                = 0x10,
> +    BDRV_REQ_WRITE_COMPRESSED   = 0x20,
>  } BdrvRequestFlags;
>  

Needs to be rebased on top of commit fa166538, and adjust BDRV_REQ_MASK
at that time.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Pavel Butsykin]

On 24.06.2016 18:39, Eric Blake wrote:
> On 06/23/2016 03:15 AM, Pavel Butsykin wrote:
>
>>>> @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const
>>>> char *target,
>>>>                          bool has_mode, enum NewImageMode mode,
>>>>                          bool has_speed, int64_t speed,
>>>>                          bool has_bitmap, const char *bitmap,
>>>> +                      bool has_compress, bool compress,
>>>>                          bool has_on_source_error, BlockdevOnError
>>>> on_source_error,
>>>>                          bool has_on_target_error, BlockdevOnError
>>>> on_target_error,
>>>>                          Error **errp)
>>>
>>> Might be nice to simplify this signature once my qapi 'box' patches land.
>>>
>>
>> Yes, it would certainly be better. But your patches are not applied on
>> the current, maybe you have your own branch?
>
> http://repo.or.cz/qemu/ericb.git/shortlog/refs/heads/qapi (still
> undergoing rebases, and I need to address some of Markus' review
> comments, but it's getting closer and still targetted for inclusion in 2.7)
>
Thank you very much!

Re: [Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 2149 bytes --]

Am 13.06.2016 um 16:23 hat Eric Blake geschrieben:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> > From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> > 
> > This is a preparatory patch, which continues the general trend of the transition
> > to the byte-based interfaces.
> > 
> > Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> > Signed-off-by: Denis V. Lunev <den@openvz.org>
> > CC: Jeff Cody <jcody@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Eric Blake <eblake@redhat.com>
> > CC: John Snow <jsnow@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Kevin Wolf <kwolf@redhat.com>

> > -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> > -                          const uint8_t *buf, int nb_sectors)
> > +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> > +                           const void *buf, int count)
> >  {
> >      BlockDriver *drv = bs->drv;
> >      int ret;
> > @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
> >      if (!drv->bdrv_write_compressed) {
> >          return -ENOTSUP;
> >      }
> > -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
> > +    ret = bdrv_check_byte_request(bs, offset, count);
> >      if (ret < 0) {
> >          return ret;
> >      }
> >  
> >      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> >  
> > -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
> > +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
> > +                                      count >> BDRV_SECTOR_BITS);
> 
> If you are going to shift right, you need to first assert that offset
> and count are aligned (and thus that our call to a sector interface
> isn't going to operate on the wrong data).  See for example commit 166fe960.

Yes, I would like to have these assertions at least.

But I'm wondering what the point of converting the interface is when we
don't intend to actually support sub-sector requests and the sector
alignment is still required at the end of the series.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Kevin Wolf]

Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> This patch just adds the interface to the bdrv_co_pwritev_compressed,
> which is currently not used but will be useful for safe implementation of the
> bdrv_co_write_compressed callback in format drivers.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
>  include/block/block_int.h |  5 +++
>  qemu-img.c                |  2 +-
>  3 files changed, 78 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index c5bb6ae..54cd9a4 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
> -                           const void *buf, int count)
> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
>  {
>      BlockDriver *drv = bs->drv;
>      int ret;
> @@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>      if (!drv) {
>          return -ENOMEDIUM;
>      }
> -    if (!drv->bdrv_write_compressed) {
> +
> +    if (!drv->bdrv_co_write_compressed) {
>          return -ENOTSUP;
>      }
> -    ret = bdrv_check_byte_request(bs, offset, count);
> +
> +    ret = bdrv_check_byte_request(bs, offset, bytes);
>      if (ret < 0) {
>          return ret;
>      }
>  
>      assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> +    assert(qemu_in_coroutine());
> +
> +    return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
> +                                         bytes >> BDRV_SECTOR_BITS, qiov);
> +}
> +
> +typedef struct BdrvWriteCompressedCo {
> +    BlockDriverState *bs;
> +    int64_t offset;
> +    QEMUIOVector *qiov;
> +    int ret;
> +} BdrvWriteCompressedCo;

I think you could just reuse RwCo here.

And in fact, this made we wonder whether we can reuse more from the
normal I/O path. If compressed writes could be made just another REQ_*
flag and we went through bdrv_co_pwritev(), we would automatically get
the alignment code if we don't want to convert drivers to byte-based.
And also the existing coroutine wrapper code would be reused instead
implementing it once more.

I'm not completely sure how special compressed writes really are, so
reusing the normal I/O path may or may not work. It might be worth
investigating, though.

> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -207,6 +207,9 @@ struct BlockDriver {
>      int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>                                   const uint8_t *buf, int nb_sectors);
>  
> +    int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);

This should be called bdrv_co_writev_compressed because it's a vectored
interface.

Kevin

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Kevin Wolf]

Am 22.06.2016 um 14:27 hat Pavel Butsykin geschrieben:
> On 13.06.2016 23:14, Eric Blake wrote:
> >On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> >>From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>
> >>Added implementation of the qcow2_co_write_compressed function that
> >>will allow us to safely use compressed writes for the qcow2 from running VMs.
> >>
> >>Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Jeff Cody <jcody@redhat.com>
> >>CC: Markus Armbruster <armbru@redhat.com>
> >>CC: Eric Blake <eblake@redhat.com>
> >>CC: John Snow <jsnow@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >>---
> >>  block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
> >>  1 file changed, 52 insertions(+), 37 deletions(-)
> >>
> >>diff --git a/block/qcow2.c b/block/qcow2.c
> >>index c9306a7..38caa66 100644
> >>--- a/block/qcow2.c
> >>+++ b/block/qcow2.c
> >>@@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
> >>
> >>  /* XXX: put compressed sectors first, then all the cluster aligned
> >>     tables to avoid losing bytes in alignment */
> >>-static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
> >>-                                  const uint8_t *buf, int nb_sectors)
> >>+static coroutine_fn int
> >>+qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> >>+                          int nb_sectors, QEMUIOVector *qiov)
> >
> >Is it worth converting to a byte-based qcow2_co_pwrite_compressed()
> >while at it?
> 
> Yes, I'll do it for the next version.

I think it makes sense to do this in two steps. That is, one patch for
making the function vectored and a second one for making it byte-based.
Of course, you can take bytes as a unit even in the first patch, but
then just convert it to sector numbers immediately, so that the actual
conversion comes separately.

Kevin

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Kevin Wolf]

Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Added implementation of the qcow2_co_write_compressed function that
> will allow us to safely use compressed writes for the qcow2 from running VMs.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 52 insertions(+), 37 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c9306a7..38caa66 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>  
>  /* XXX: put compressed sectors first, then all the cluster aligned
>     tables to avoid losing bytes in alignment */
> -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
> -                                  const uint8_t *buf, int nb_sectors)
> +static coroutine_fn int
> +qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
> +                          int nb_sectors, QEMUIOVector *qiov)
>  {
>      BDRVQcow2State *s = bs->opaque;
> +    QEMUIOVector hd_qiov;
> +    struct iovec iov;
>      z_stream strm;
>      int ret, out_len;
> -    uint8_t *out_buf;
> +    uint8_t *buf, *out_buf;
>      uint64_t cluster_offset;
>  
>      if (nb_sectors == 0) {
> @@ -2551,29 +2554,25 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>          return bdrv_truncate(bs->file->bs, cluster_offset);
>      }
>  
> +    buf = qemu_blockalign(bs, s->cluster_size);
>      if (nb_sectors != s->cluster_sectors) {
> -        ret = -EINVAL;
> -
> -        /* Zero-pad last write if image size is not cluster aligned */
> -        if (sector_num + nb_sectors == bs->total_sectors &&
> -            nb_sectors < s->cluster_sectors) {
> -            uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size);
> -            memset(pad_buf, 0, s->cluster_size);
> -            memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE);
> -            ret = qcow2_write_compressed(bs, sector_num,
> -                                         pad_buf, s->cluster_sectors);
> -            qemu_vfree(pad_buf);
> +        if (nb_sectors > s->cluster_sectors ||
> +            sector_num + nb_sectors != bs->total_sectors)
> +        {
> +            qemu_vfree(buf);
> +            return -EINVAL;
>          }
> -        return ret;
> +        /* Zero-pad last write if image size is not cluster aligned */
> +        memset(buf, 0, s->cluster_size);
>      }
> +    qemu_iovec_to_buf(qiov, 0, buf, qiov->size);

This looks less related to the new interface, but more like an unrelated
(but still worthwhile) cleanup to avoid the recursion.

Can we separate this out as a cleanup patch before this one?

Also, the last parameter of qemu_iovec_to_buf() should be
s->cluster_size, it's the buffer size and not the qiov size.
Additionally, we may want to assert(qiov->size == s->cluster_size).

>      out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
>  
>      /* best compression, small window, no zlib header */
>      memset(&strm, 0, sizeof(strm));
> -    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
> -                       Z_DEFLATED, -12,
> -                       9, Z_DEFAULT_STRATEGY);
> +    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
> +                       -12, 9, Z_DEFAULT_STRATEGY);

Unrelated reformatting? Let's drop this, so the semantic changes in the
patch become more visible.

>      if (ret != 0) {
>          ret = -EINVAL;
>          goto fail;
> @@ -2595,34 +2594,50 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>      deflateEnd(&strm);
>  
>      if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
> +        iov = (struct iovec) {
> +            .iov_base   = buf,
> +            .iov_len    = out_len,
> +        };
> +        qemu_iovec_init_external(&hd_qiov, &iov, 1);
>          /* could not compress: write normal cluster */
> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
> +        ret = qcow2_co_writev(bs, sector_num, s->cluster_sectors, &hd_qiov);

Now that it's qcow2_co_pwritev(), you can probably just use the existing
qiov.

>          if (ret < 0) {
>              goto fail;
>          }
> -    } else {
> -        cluster_offset = qcow2_alloc_compressed_cluster_offset(bs,
> -            sector_num << 9, out_len);
> -        if (!cluster_offset) {
> -            ret = -EIO;
> -            goto fail;
> -        }
> -        cluster_offset &= s->cluster_offset_mask;
> +        goto success;
> +    }
>  
> -        ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> +    qemu_co_mutex_lock(&s->lock);
> +    cluster_offset = \

That backslash isn't necessary.

> +        qcow2_alloc_compressed_cluster_offset(bs, sector_num << 9, out_len);
> +    if (!cluster_offset) {
> +        qemu_co_mutex_unlock(&s->lock);
> +        ret = -EIO;
> +        goto fail;
> +    }
> +    cluster_offset &= s->cluster_offset_mask;

Kevin

Re: [Qemu-devel] [PATCH 01/11] block: switch blk_write_compressed() to byte-based interface

[Pavel Butsykin]

On 28.06.2016 13:53, Kevin Wolf wrote:
> Am 13.06.2016 um 16:23 hat Eric Blake geschrieben:
>> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
>>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>
>>> This is a preparatory patch, which continues the general trend of the transition
>>> to the byte-based interfaces.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> CC: Jeff Cody <jcody@redhat.com>
>>> CC: Markus Armbruster <armbru@redhat.com>
>>> CC: Eric Blake <eblake@redhat.com>
>>> CC: John Snow <jsnow@redhat.com>
>>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>>> CC: Kevin Wolf <kwolf@redhat.com>
>
>>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>> -                          const uint8_t *buf, int nb_sectors)
>>> +int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>>> +                           const void *buf, int count)
>>>   {
>>>       BlockDriver *drv = bs->drv;
>>>       int ret;
>>> @@ -1791,14 +1791,15 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>>       if (!drv->bdrv_write_compressed) {
>>>           return -ENOTSUP;
>>>       }
>>> -    ret = bdrv_check_request(bs, sector_num, nb_sectors);
>>> +    ret = bdrv_check_byte_request(bs, offset, count);
>>>       if (ret < 0) {
>>>           return ret;
>>>       }
>>>
>>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>>>
>>> -    return drv->bdrv_write_compressed(bs, sector_num, buf, nb_sectors);
>>> +    return drv->bdrv_write_compressed(bs, offset >> BDRV_SECTOR_BITS, buf,
>>> +                                      count >> BDRV_SECTOR_BITS);
>>
>> If you are going to shift right, you need to first assert that offset
>> and count are aligned (and thus that our call to a sector interface
>> isn't going to operate on the wrong data).  See for example commit 166fe960.
>
> Yes, I would like to have these assertions at least.
>
> But I'm wondering what the point of converting the interface is when we
> don't intend to actually support sub-sector requests and the sector
> alignment is still required at the end of the series.

Because at the time of sending patches, the format drivers still had
the sector-based interfaces. In the end, the assertions are not
necessary, because the interface format driver now also byte-based.

> Kevin
>

Re: [Qemu-devel] [PATCH 02/11] block/io: add bdrv_co_write_compressed

[Pavel Butsykin]

On 28.06.2016 14:09, Kevin Wolf wrote:
> Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This patch just adds the interface to the bdrv_co_pwritev_compressed,
>> which is currently not used but will be useful for safe implementation of the
>> bdrv_co_write_compressed callback in format drivers.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/io.c                | 78 +++++++++++++++++++++++++++++++++++++++++++----
>>   include/block/block_int.h |  5 +++
>>   qemu-img.c                |  2 +-
>>   3 files changed, 78 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index c5bb6ae..54cd9a4 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -1779,8 +1779,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>
>> -int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>> -                           const void *buf, int count)
>> +int coroutine_fn bdrv_co_pwritev_compressed(BlockDriverState *bs,
>> +    int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1788,18 +1788,84 @@ int bdrv_pwrite_compressed(BlockDriverState *bs, int64_t offset,
>>       if (!drv) {
>>           return -ENOMEDIUM;
>>       }
>> -    if (!drv->bdrv_write_compressed) {
>> +
>> +    if (!drv->bdrv_co_write_compressed) {
>>           return -ENOTSUP;
>>       }
>> -    ret = bdrv_check_byte_request(bs, offset, count);
>> +
>> +    ret = bdrv_check_byte_request(bs, offset, bytes);
>>       if (ret < 0) {
>>           return ret;
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>> +    assert(qemu_in_coroutine());
>> +
>> +    return drv->bdrv_co_write_compressed(bs, offset >> BDRV_SECTOR_BITS,
>> +                                         bytes >> BDRV_SECTOR_BITS, qiov);
>> +}
>> +
>> +typedef struct BdrvWriteCompressedCo {
>> +    BlockDriverState *bs;
>> +    int64_t offset;
>> +    QEMUIOVector *qiov;
>> +    int ret;
>> +} BdrvWriteCompressedCo;
>
> I think you could just reuse RwCo here.
>

I reused BlkRwCo, and it's only a temporary change.

> And in fact, this made we wonder whether we can reuse more from the
> normal I/O path. If compressed writes could be made just another REQ_*
> flag and we went through bdrv_co_pwritev(), we would automatically get
> the alignment code if we don't want to convert drivers to byte-based.
> And also the existing coroutine wrapper code would be reused instead
> implementing it once more.
>
> I'm not completely sure how special compressed writes really are, so
> reusing the normal I/O path may or may not work. It might be worth
> investigating, though.
>
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -207,6 +207,9 @@ struct BlockDriver {
>>       int (*bdrv_write_compressed)(BlockDriverState *bs, int64_t sector_num,
>>                                    const uint8_t *buf, int nb_sectors);
>>
>> +    int coroutine_fn (*bdrv_co_write_compressed)(BlockDriverState *bs,
>> +        int64_t sector_num, int nb_sectors, QEMUIOVector *qiov);
>
> This should be called bdrv_co_writev_compressed because it's a vectored
> interface.
>
agree

> Kevin
>

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Kevin Wolf]

Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> and use the blk_prw() as a generic one.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Jeff Cody <jcody@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Eric Blake <eblake@redhat.com>
> CC: John Snow <jsnow@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>

Oh, so you already do use a flag. Nice. :-)

> diff --git a/block/block-backend.c b/block/block-backend.c
> index 3c1fc50..9e1c793 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>          flags |= BDRV_REQ_FUA;
>      }
>  
> -    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> +    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> +        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
> +    } else {
> +        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> +    }
>  }

If you move the processing of the flag inside bdrv_co_pwritev(), where I
think it belongs anyway, you could use the flag from the start (by going
through bdrv_prwv_co()) instead of temporarily introducing your own
coroutine wrapper. I think that would make the initial conversion
patches quite a bit simpler.

Kevin

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Pavel Butsykin]

On 28.06.2016 14:47, Kevin Wolf wrote:
> Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
>> is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
>> and use the blk_prw() as a generic one.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>
> Oh, so you already do use a flag. Nice. :-)
>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 3c1fc50..9e1c793 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
>>           flags |= BDRV_REQ_FUA;
>>       }
>>
>> -    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
>> +    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
>> +        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
>> +    } else {
>> +        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
>> +    }
>>   }
>
> If you move the processing of the flag inside bdrv_co_pwritev(), where I
> think it belongs anyway, you could use the flag from the start (by going
> through bdrv_prwv_co()) instead of temporarily introducing your own
> coroutine wrapper. I think that would make the initial conversion
> patches quite a bit simpler.

You propose to add bdrv_driver_compressed and call it from 
bdrv_aligned_pwritev ?

> Kevin
>

Re: [Qemu-devel] [PATCH 07/11] block: optimization blk_pwrite_compressed()

[Kevin Wolf]

Am 28.06.2016 um 14:32 hat Pavel Butsykin geschrieben:
> On 28.06.2016 14:47, Kevin Wolf wrote:
> >Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
> >>From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>
> >>For bdrv_pwrite_compressed() it looks like most of the code creating coroutine
> >>is duplicated in blk_prw(). So we can just add a flag(BDRV_REQ_WRITE_COMPRESSED)
> >>and use the blk_prw() as a generic one.
> >>
> >>Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>Signed-off-by: Denis V. Lunev <den@openvz.org>
> >>CC: Jeff Cody <jcody@redhat.com>
> >>CC: Markus Armbruster <armbru@redhat.com>
> >>CC: Eric Blake <eblake@redhat.com>
> >>CC: John Snow <jsnow@redhat.com>
> >>CC: Stefan Hajnoczi <stefanha@redhat.com>
> >>CC: Kevin Wolf <kwolf@redhat.com>
> >
> >Oh, so you already do use a flag. Nice. :-)
> >
> >>diff --git a/block/block-backend.c b/block/block-backend.c
> >>index 3c1fc50..9e1c793 100644
> >>--- a/block/block-backend.c
> >>+++ b/block/block-backend.c
> >>@@ -785,7 +785,11 @@ int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
> >>          flags |= BDRV_REQ_FUA;
> >>      }
> >>
> >>-    return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> >>+        return bdrv_co_pwritev_compressed(blk_bs(blk), offset, bytes, qiov);
> >>+    } else {
> >>+        return bdrv_co_pwritev(blk_bs(blk), offset, bytes, qiov, flags);
> >>+    }
> >>  }
> >
> >If you move the processing of the flag inside bdrv_co_pwritev(), where I
> >think it belongs anyway, you could use the flag from the start (by going
> >through bdrv_prwv_co()) instead of temporarily introducing your own
> >coroutine wrapper. I think that would make the initial conversion
> >patches quite a bit simpler.
> 
> You propose to add bdrv_driver_compressed and call it from
> bdrv_aligned_pwritev ?

I'm not sure if we can easily move it that much down the caller chain.
If we can, great. But here I was just thinking of making the switch at
the start of bdrv_co_pwritev() so that you can reuse the existing
wrappers like bdrv_prwv_co().

The long-term advantage of putting it into bdrv_aligned_pwritev() could
be that we would ge the common alignment handling. But I think qcow2
still errors out if you rewrite an already allocated cluster with
qcow2_pwritev_compressed(), so currently doing sub-cluster (or even
sub-sector) writes doesn't make much sense anyway. So I doubt it's worth
the hassle now.

Kevin

Re: [Qemu-devel] [PATCH 03/11] qcow2: add qcow2_co_write_compressed

[Pavel Butsykin]

On 28.06.2016 14:30, Kevin Wolf wrote:
> Am 31.05.2016 um 11:15 hat Denis V. Lunev geschrieben:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> Added implementation of the qcow2_co_write_compressed function that
>> will allow us to safely use compressed writes for the qcow2 from running VMs.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/qcow2.c | 89 ++++++++++++++++++++++++++++++++++-------------------------
>>   1 file changed, 52 insertions(+), 37 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c9306a7..38caa66 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2535,13 +2535,16 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
>>
>>   /* XXX: put compressed sectors first, then all the cluster aligned
>>      tables to avoid losing bytes in alignment */
>> -static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                                  const uint8_t *buf, int nb_sectors)
>> +static coroutine_fn int
>> +qcow2_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> +                          int nb_sectors, QEMUIOVector *qiov)
>>   {
>>       BDRVQcow2State *s = bs->opaque;
>> +    QEMUIOVector hd_qiov;
>> +    struct iovec iov;
>>       z_stream strm;
>>       int ret, out_len;
>> -    uint8_t *out_buf;
>> +    uint8_t *buf, *out_buf;
>>       uint64_t cluster_offset;
>>
>>       if (nb_sectors == 0) {
>> @@ -2551,29 +2554,25 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>           return bdrv_truncate(bs->file->bs, cluster_offset);
>>       }
>>
>> +    buf = qemu_blockalign(bs, s->cluster_size);
>>       if (nb_sectors != s->cluster_sectors) {
>> -        ret = -EINVAL;
>> -
>> -        /* Zero-pad last write if image size is not cluster aligned */
>> -        if (sector_num + nb_sectors == bs->total_sectors &&
>> -            nb_sectors < s->cluster_sectors) {
>> -            uint8_t *pad_buf = qemu_blockalign(bs, s->cluster_size);
>> -            memset(pad_buf, 0, s->cluster_size);
>> -            memcpy(pad_buf, buf, nb_sectors * BDRV_SECTOR_SIZE);
>> -            ret = qcow2_write_compressed(bs, sector_num,
>> -                                         pad_buf, s->cluster_sectors);
>> -            qemu_vfree(pad_buf);
>> +        if (nb_sectors > s->cluster_sectors ||
>> +            sector_num + nb_sectors != bs->total_sectors)
>> +        {
>> +            qemu_vfree(buf);
>> +            return -EINVAL;
>>           }
>> -        return ret;
>> +        /* Zero-pad last write if image size is not cluster aligned */
>> +        memset(buf, 0, s->cluster_size);
>>       }
>> +    qemu_iovec_to_buf(qiov, 0, buf, qiov->size);
>
> This looks less related to the new interface, but more like an unrelated
> (but still worthwhile) cleanup to avoid the recursion.
>
> Can we separate this out as a cleanup patch before this one?
>
We can :)

> Also, the last parameter of qemu_iovec_to_buf() should be
> s->cluster_size, it's the buffer size and not the qiov size.
> Additionally, we may want to assert(qiov->size == s->cluster_size).

It is not necessary, the qiov size can be less than s->cluster_size. In
this case, the remaining part of the cluster is filled with zeros.

>
>>       out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
>>
>>       /* best compression, small window, no zlib header */
>>       memset(&strm, 0, sizeof(strm));
>> -    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
>> -                       Z_DEFLATED, -12,
>> -                       9, Z_DEFAULT_STRATEGY);
>> +    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
>> +                       -12, 9, Z_DEFAULT_STRATEGY);
>
> Unrelated reformatting? Let's drop this, so the semantic changes in the
> patch become more visible.
>
ok

>>       if (ret != 0) {
>>           ret = -EINVAL;
>>           goto fail;
>> @@ -2595,34 +2594,50 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       deflateEnd(&strm);
>>
>>       if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
>> +        iov = (struct iovec) {
>> +            .iov_base   = buf,
>> +            .iov_len    = out_len,
>> +        };
>> +        qemu_iovec_init_external(&hd_qiov, &iov, 1);
>>           /* could not compress: write normal cluster */
>> -        ret = bdrv_write(bs, sector_num, buf, s->cluster_sectors);
>> +        ret = qcow2_co_writev(bs, sector_num, s->cluster_sectors, &hd_qiov);
>
> Now that it's qcow2_co_pwritev(), you can probably just use the existing
> qiov.
>
>>           if (ret < 0) {
>>               goto fail;
>>           }
>> -    } else {
>> -        cluster_offset = qcow2_alloc_compressed_cluster_offset(bs,
>> -            sector_num << 9, out_len);
>> -        if (!cluster_offset) {
>> -            ret = -EIO;
>> -            goto fail;
>> -        }
>> -        cluster_offset &= s->cluster_offset_mask;
>> +        goto success;
>> +    }
>>
>> -        ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
>> -        if (ret < 0) {
>> -            goto fail;
>> -        }
>> +    qemu_co_mutex_lock(&s->lock);
>> +    cluster_offset = \
>
> That backslash isn't necessary.
>
I know it's just a marker.

>> +        qcow2_alloc_compressed_cluster_offset(bs, sector_num << 9, out_len);
>> +    if (!cluster_offset) {
>> +        qemu_co_mutex_unlock(&s->lock);
>> +        ret = -EIO;
>> +        goto fail;
>> +    }
>> +    cluster_offset &= s->cluster_offset_mask;
>
> Kevin
>