[Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Qemu-devel] [PATCH for 2.7 0/2] 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>

Pavel Butsykin (2):
  drive-backup: added support for data compression
  blockdev-backup: added support for data compression

 block/backup.c            | 13 +++++++++++++
 blockdev.c                | 20 ++++++++++++++++++--
 hmp-commands.hx           |  8 +++++---
 hmp.c                     |  3 ++-
 include/block/block_int.h |  1 +
 qapi/block-core.json      |  3 ++-
 qmp-commands.hx           |  7 +++++--
 7 files changed, 46 insertions(+), 9 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/2] 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>
---
 block/backup.c            | 13 +++++++++++++
 blockdev.c                | 12 ++++++++++--
 hmp-commands.hx           |  8 +++++---
 hmp.c                     |  3 ++-
 include/block/block_int.h |  1 +
 qapi/block-core.json      |  2 +-
 qmp-commands.hx           |  4 +++-
 7 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 491fd14..cad0439 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;
     QLIST_HEAD(, CowRequest) inflight_reqs;
 } BackupBlockJob;
 
@@ -157,6 +158,10 @@ static int coroutine_fn backup_do_cow(BlockDriverState *bs,
             ret = bdrv_co_write_zeroes(job->target,
                                        start * sectors_per_cluster,
                                        n, BDRV_REQ_MAY_UNMAP);
+        } else if (job->compress) {
+            ret = bdrv_write_compressed(job->target,
+                                        start * sectors_per_cluster,
+                                        iov.iov_base, n);
         } else {
             ret = bdrv_co_writev(job->target,
                                  start * sectors_per_cluster, n,
@@ -497,6 +502,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,
@@ -534,6 +540,12 @@ void backup_start(BlockDriverState *bs, BlockDriverState *target,
         return;
     }
 
+    if (compress && target->drv->bdrv_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;
     }
@@ -580,6 +592,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 f1f520a..ef72f19 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1856,6 +1856,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,
@@ -1896,6 +1897,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);
@@ -3170,6 +3172,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,
@@ -3200,6 +3203,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) {
@@ -3288,7 +3294,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);
     if (local_err != NULL) {
@@ -3307,6 +3313,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)
@@ -3314,6 +3321,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);
@@ -3378,7 +3386,7 @@ void do_blockdev_backup(const char *device, const char *target,
 
     bdrv_ref(target_bs);
     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) {
         bdrv_unref(target_bs);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4f4f60a..4b1d15b 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 d510236..e34593d 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1094,6 +1094,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;
 
@@ -1111,7 +1112,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 10d8759..142d207 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -701,6 +701,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 1d09079..ebedf0d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -905,7 +905,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 de896a5..ce36518 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,7 @@ 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": compress data blocks (if the target format supports it).
 - "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 2/2] 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>
---
 blockdev.c           | 10 +++++++++-
 qapi/block-core.json |  1 +
 qmp-commands.hx      |  3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ef72f19..353c1c8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1940,6 +1940,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,
@@ -1987,6 +1988,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);
@@ -3335,6 +3337,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,
@@ -3356,6 +3359,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) {
@@ -3386,7 +3392,7 @@ void do_blockdev_backup(const char *device, const char *target,
 
     bdrv_ref(target_bs);
     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) {
         bdrv_unref(target_bs);
@@ -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 ebedf0d..8e47e57 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -941,6 +941,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 ce36518..ff9e491 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1241,7 +1241,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,
     },
@@ -1263,6 +1263,7 @@ 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": compress data blocks (if the target format supports it).
 - "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

Re: [Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Stefan Hajnoczi]

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

On Sat, Apr 23, 2016 at 11:41:51AM +0300, Denis V. Lunev wrote:
> 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>
> 
> Pavel Butsykin (2):
>   drive-backup: added support for data compression
>   blockdev-backup: added support for data compression
> 
>  block/backup.c            | 13 +++++++++++++
>  blockdev.c                | 20 ++++++++++++++++++--
>  hmp-commands.hx           |  8 +++++---
>  hmp.c                     |  3 ++-
>  include/block/block_int.h |  1 +
>  qapi/block-core.json      |  3 ++-
>  qmp-commands.hx           |  7 +++++--
>  7 files changed, 46 insertions(+), 9 deletions(-)

bdrv_write_compressed() hasn't been called from a running VM before, it
was purely a synchronous qemu-img operation.  The
qcow2.c:qcow2_write_compressed() code doesn't seem to be written with
coroutine context in mind but I think the lock in
block/backup.c:backup_do_cow() will protect against race conditions.

Please include a test case (using qcow2?).

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

Re: [Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Denis V. Lunev]

On 04/26/2016 12:46 PM, Stefan Hajnoczi wrote:
> On Sat, Apr 23, 2016 at 11:41:51AM +0300, Denis V. Lunev wrote:
>> 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>
>>
>> Pavel Butsykin (2):
>>    drive-backup: added support for data compression
>>    blockdev-backup: added support for data compression
>>
>>   block/backup.c            | 13 +++++++++++++
>>   blockdev.c                | 20 ++++++++++++++++++--
>>   hmp-commands.hx           |  8 +++++---
>>   hmp.c                     |  3 ++-
>>   include/block/block_int.h |  1 +
>>   qapi/block-core.json      |  3 ++-
>>   qmp-commands.hx           |  7 +++++--
>>   7 files changed, 46 insertions(+), 9 deletions(-)
> bdrv_write_compressed() hasn't been called from a running VM before, it
> was purely a synchronous qemu-img operation.  The
> qcow2.c:qcow2_write_compressed() code doesn't seem to be written with
> coroutine context in mind but I think the lock in
> block/backup.c:backup_do_cow() will protect against race conditions.
>
> Please include a test case (using qcow2?).
I have looked into the code. The only problematic thing there
is that the compression is performed directly in the write,
which could take a bit of time.

Thus responsiveness of the guest system could be reduced.
Though I do not expect here troubles. I think that it is worth
to give this tech a chance. Compression could be moved to
the outer thread in the next step if required.

Den

Re: [Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Fam Zheng]

On Wed, 04/27 09:32, Denis V. Lunev wrote:
> On 04/26/2016 12:46 PM, Stefan Hajnoczi wrote:
> > On Sat, Apr 23, 2016 at 11:41:51AM +0300, Denis V. Lunev wrote:
> > > 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>
> > > 
> > > Pavel Butsykin (2):
> > >    drive-backup: added support for data compression
> > >    blockdev-backup: added support for data compression
> > > 
> > >   block/backup.c            | 13 +++++++++++++
> > >   blockdev.c                | 20 ++++++++++++++++++--
> > >   hmp-commands.hx           |  8 +++++---
> > >   hmp.c                     |  3 ++-
> > >   include/block/block_int.h |  1 +
> > >   qapi/block-core.json      |  3 ++-
> > >   qmp-commands.hx           |  7 +++++--
> > >   7 files changed, 46 insertions(+), 9 deletions(-)
> > bdrv_write_compressed() hasn't been called from a running VM before, it
> > was purely a synchronous qemu-img operation.  The
> > qcow2.c:qcow2_write_compressed() code doesn't seem to be written with
> > coroutine context in mind but I think the lock in
> > block/backup.c:backup_do_cow() will protect against race conditions.
> > 
> > Please include a test case (using qcow2?).
> I have looked into the code. The only problematic thing there
> is that the compression is performed directly in the write,
> which could take a bit of time.
> 
> Thus responsiveness of the guest system could be reduced.
> Though I do not expect here troubles. I think that it is worth
> to give this tech a chance. Compression could be moved to
> the outer thread in the next step if required.

As far as I can tell, in order to be used in a coroutine, we need a
"qemu_co_mutex_lock(&s->lock);" pair in vmdk_write_compressed. Something
similar probably apply to qcow2 as well.

Maybe what we should do is adding a bdrv_co_write_compressed().

Fam

Re: [Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Denis V. Lunev]

On 04/27/2016 10:59 AM, Fam Zheng wrote:
> On Wed, 04/27 09:32, Denis V. Lunev wrote:
>> On 04/26/2016 12:46 PM, Stefan Hajnoczi wrote:
>>> On Sat, Apr 23, 2016 at 11:41:51AM +0300, Denis V. Lunev wrote:
>>>> 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>
>>>>
>>>> Pavel Butsykin (2):
>>>>     drive-backup: added support for data compression
>>>>     blockdev-backup: added support for data compression
>>>>
>>>>    block/backup.c            | 13 +++++++++++++
>>>>    blockdev.c                | 20 ++++++++++++++++++--
>>>>    hmp-commands.hx           |  8 +++++---
>>>>    hmp.c                     |  3 ++-
>>>>    include/block/block_int.h |  1 +
>>>>    qapi/block-core.json      |  3 ++-
>>>>    qmp-commands.hx           |  7 +++++--
>>>>    7 files changed, 46 insertions(+), 9 deletions(-)
>>> bdrv_write_compressed() hasn't been called from a running VM before, it
>>> was purely a synchronous qemu-img operation.  The
>>> qcow2.c:qcow2_write_compressed() code doesn't seem to be written with
>>> coroutine context in mind but I think the lock in
>>> block/backup.c:backup_do_cow() will protect against race conditions.
>>>
>>> Please include a test case (using qcow2?).
>> I have looked into the code. The only problematic thing there
>> is that the compression is performed directly in the write,
>> which could take a bit of time.
>>
>> Thus responsiveness of the guest system could be reduced.
>> Though I do not expect here troubles. I think that it is worth
>> to give this tech a chance. Compression could be moved to
>> the outer thread in the next step if required.
> As far as I can tell, in order to be used in a coroutine, we need a
> "qemu_co_mutex_lock(&s->lock);" pair in vmdk_write_compressed. Something
> similar probably apply to qcow2 as well.
>
> Maybe what we should do is adding a bdrv_co_write_compressed().
>
> Fam
>
ya, you are correct here.

This is definitely missed.

Thank you.

Den

Re: [Qemu-devel] [PATCH for 2.7 0/2] backup compression

[Kevin Wolf]

Am 27.04.2016 um 09:59 hat Fam Zheng geschrieben:
> On Wed, 04/27 09:32, Denis V. Lunev wrote:
> > On 04/26/2016 12:46 PM, Stefan Hajnoczi wrote:
> > > On Sat, Apr 23, 2016 at 11:41:51AM +0300, Denis V. Lunev wrote:
> > > > 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>
> > > > 
> > > > Pavel Butsykin (2):
> > > >    drive-backup: added support for data compression
> > > >    blockdev-backup: added support for data compression
> > > > 
> > > >   block/backup.c            | 13 +++++++++++++
> > > >   blockdev.c                | 20 ++++++++++++++++++--
> > > >   hmp-commands.hx           |  8 +++++---
> > > >   hmp.c                     |  3 ++-
> > > >   include/block/block_int.h |  1 +
> > > >   qapi/block-core.json      |  3 ++-
> > > >   qmp-commands.hx           |  7 +++++--
> > > >   7 files changed, 46 insertions(+), 9 deletions(-)
> > > bdrv_write_compressed() hasn't been called from a running VM before, it
> > > was purely a synchronous qemu-img operation.  The
> > > qcow2.c:qcow2_write_compressed() code doesn't seem to be written with
> > > coroutine context in mind but I think the lock in
> > > block/backup.c:backup_do_cow() will protect against race conditions.
> > > 
> > > Please include a test case (using qcow2?).
> > I have looked into the code. The only problematic thing there
> > is that the compression is performed directly in the write,
> > which could take a bit of time.
> > 
> > Thus responsiveness of the guest system could be reduced.
> > Though I do not expect here troubles. I think that it is worth
> > to give this tech a chance. Compression could be moved to
> > the outer thread in the next step if required.
> 
> As far as I can tell, in order to be used in a coroutine, we need a
> "qemu_co_mutex_lock(&s->lock);" pair in vmdk_write_compressed. Something
> similar probably apply to qcow2 as well.
> 
> Maybe what we should do is adding a bdrv_co_write_compressed().

Yes, I agree.

Actually, I have patched write_compressed in VMDK already as I converted
all of the block drivers from the legacy .bdrv_read/write interface to a
new .bdrv_preadv/pwritev one (will send out the series today, hopefully),
but it would be much nicer to do this properly in block/io.c.

Kevin