| Message ID | 1464686130-12265-9-git-send-email-den@openvz.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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)
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) > >
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)
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 --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.