diff mbox

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

Message ID 1464686130-12265-9-git-send-email-den@openvz.org
State New, archived
Headers show

Commit Message

Denis V. Lunev May 31, 2016, 9:15 a.m. UTC
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(-)

Comments

Stefan Hajnoczi June 13, 2016, 1:19 p.m. UTC | #1
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>
Eric Blake June 13, 2016, 8:18 p.m. UTC | #2
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)
Pavel Butsykin June 23, 2016, 9:15 a.m. UTC | #3
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)
>
>
Eric Blake June 24, 2016, 3:39 p.m. UTC | #4
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)
Pavel Butsykin June 24, 2016, 3:50 p.m. UTC | #5
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!
diff mbox

Patch

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.