16.10.2019 19:28, Andrey Shinkevich wrote:
> To inform the block layer about writing all the data compressed, we
> introduce the 'compress' command line option. Based on that option, the
> written data will be aligned by the cluster size at the generic layer.
>
> Suggested-by: Roman Kagan <rka...@virtuozzo.com>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkev...@virtuozzo.com>
> ---
> block.c | 20 +++++++++++++++++++-
> block/io.c | 14 ++++++++++----
> block/qcow2.c | 4 ++++
> blockdev.c | 9 ++++++++-
> include/block/block.h | 1 +
> include/block/block_int.h | 2 ++
> qapi/block-core.json | 6 +++++-
> qemu-options.hx | 6 ++++--
> 8 files changed, 53 insertions(+), 9 deletions(-)
>
> diff --git a/block.c b/block.c
> index 1946fc6..a674920 100644
> --- a/block.c
> +++ b/block.c
> @@ -1418,6 +1418,11 @@ QemuOptsList bdrv_runtime_opts = {
> .type = QEMU_OPT_BOOL,
> .help = "always accept other writers (default: off)",
> },
> + {
> + .name = BDRV_OPT_COMPRESS,
> + .type = QEMU_OPT_BOOL,
> + .help = "compress all writes to the image (default: off)",
> + },
> { /* end of list */ }
> },
> };
> @@ -1545,6 +1550,14 @@ static int bdrv_open_common(BlockDriverState *bs,
> BlockBackend *file,
> }
> pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
>
> + if (bs->all_write_compressed && !drv->bdrv_co_pwritev_compressed_part) {
> + error_setg(errp, "Compression is not supported for the driver '%s'",
> + drv->format_name);
> + bs->all_write_compressed = false;
> + ret = -ENOTSUP;
> + goto fail_opts;
> + }
> +
> /* Open the image, either directly or using a protocol */
> open_flags = bdrv_open_flags(bs, bs->open_flags);
> node_name = qemu_opt_get(opts, "node-name");
> @@ -2983,6 +2996,11 @@ static BlockDriverState *bdrv_open_inherit(const char
> *filename,
> flags &= ~BDRV_O_RDWR;
> }
>
> + if (!g_strcmp0(qdict_get_try_str(options, BDRV_OPT_COMPRESS), "on") ||
> + qdict_get_try_bool(options, BDRV_OPT_COMPRESS, false)) {
> + bs->all_write_compressed = true;
> + }
> +
> if (flags & BDRV_O_SNAPSHOT) {
> snapshot_options = qdict_new();
> bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> @@ -3208,7 +3226,7 @@ static int bdrv_reset_options_allowed(BlockDriverState
> *bs,
> * in bdrv_reopen_prepare() so they can be left out of @new_opts */
> const char *const common_options[] = {
> "node-name", "discard", "cache.direct", "cache.no-flush",
> - "read-only", "auto-read-only", "detect-zeroes", NULL
> + "read-only", "auto-read-only", "detect-zeroes", "compress", NULL
> };
>
> for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
> diff --git a/block/io.c b/block/io.c
> index f0b86c1..3743a13 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1360,9 +1360,15 @@ static int coroutine_fn
> bdrv_co_do_copy_on_readv(BdrvChild *child,
> /* This does not change the data on the disk, it is not
> * necessary to flush even in cache=writethrough mode.
> */
> - ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> - &local_qiov, 0,
> - BDRV_REQ_WRITE_UNCHANGED);
> + if (bs->all_write_compressed) {
> + ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
> + pnum, &local_qiov,
> + qiov_offset);
last argument must be 0, we are using local_qiov, so, it's qiov_offset is 0.
> + } else {
> + ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> + &local_qiov, 0,
> + BDRV_REQ_WRITE_UNCHANGED);
> + }
> }
>
> if (ret < 0) {
> @@ -1954,7 +1960,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild
> *child,
> } else if (flags & BDRV_REQ_ZERO_WRITE) {
> bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO);
> ret = bdrv_co_do_pwrite_zeroes(bs, offset, bytes, flags);
> - } else if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> + } else if (flags & BDRV_REQ_WRITE_COMPRESSED ||
> bs->all_write_compressed) {
> ret = bdrv_driver_pwritev_compressed(bs, offset, bytes,
> qiov, qiov_offset);
> } else if (bytes <= max_transfer) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7961c05..6b29e16 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1787,6 +1787,10 @@ static void qcow2_refresh_limits(BlockDriverState *bs,
> Error **errp)
> /* Encryption works on a sector granularity */
> bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto);
> }
> + if (bs->all_write_compressed) {
> + bs->bl.request_alignment = MAX(bs->bl.request_alignment,
> + s->cluster_size);
> + }
> bs->bl.pwrite_zeroes_alignment = s->cluster_size;
> bs->bl.pdiscard_alignment = s->cluster_size;
> }
> diff --git a/blockdev.c b/blockdev.c
> index f89e48f..0c0b398 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -471,7 +471,7 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> int bdrv_flags = 0;
> int on_read_error, on_write_error;
> bool account_invalid, account_failed;
> - bool writethrough, read_only;
> + bool writethrough, read_only, compress;
> BlockBackend *blk;
> BlockDriverState *bs;
> ThrottleConfig cfg;
> @@ -570,6 +570,7 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> }
>
> read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> + compress = qemu_opt_get_bool(opts, BDRV_OPT_COMPRESS, false);
>
> /* init */
> if ((!file || !*file) && !qdict_size(bs_opts)) {
Do we need compress for root state here??
> @@ -595,6 +596,8 @@ static BlockBackend *blockdev_init(const char *file,
> QDict *bs_opts,
> qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
> read_only ? "on" : "off");
> qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
> + qdict_set_default_str(bs_opts, BDRV_OPT_COMPRESS,
> + compress ? "on" : "off");
> assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
>
> if (runstate_check(RUN_STATE_INMIGRATE)) {
> @@ -4683,6 +4686,10 @@ QemuOptsList qemu_common_drive_opts = {
> .name = BDRV_OPT_READ_ONLY,
> .type = QEMU_OPT_BOOL,
> .help = "open drive file as read-only",
> + },{
> + .name = BDRV_OPT_COMPRESS,
> + .type = QEMU_OPT_BOOL,
> + .help = "compress all writes to image",
> },
>
> THROTTLE_OPTS,
> diff --git a/include/block/block.h b/include/block/block.h
> index 792bb82..7e0a927 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -139,6 +139,7 @@ typedef struct HDGeometry {
> #define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
> #define BDRV_OPT_DISCARD "discard"
> #define BDRV_OPT_FORCE_SHARE "force-share"
> +#define BDRV_OPT_COMPRESS "compress"
>
>
> #define BDRV_SECTOR_BITS 9
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 05056b3..3fe8923 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -923,6 +923,8 @@ struct BlockDriverState {
>
> /* BdrvChild links to this node may never be frozen */
> bool never_freeze;
> + /* Compress all writes to the image */
> + bool all_write_compressed;
> };
>
> struct BlockBackendRootState {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f66553a..6c1684f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4014,6 +4014,9 @@
> # @force-share: force share all permission on added nodes.
> # Requires read-only=true. (Since 2.10)
> #
No newlines between parameters above, I think don't add one.
> +# @compress: compress all writes to the image (Since 4.2)
> +# (default: false)
> +#
> # Remaining options are determined by the block driver.
> #
> # Since: 2.9
> @@ -4026,7 +4029,8 @@
> '*read-only': 'bool',
> '*auto-read-only': 'bool',
> '*force-share': 'bool',
> - '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> + '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> + '*compress': 'bool' },
> 'discriminator': 'driver',
> 'data': {
> 'blkdebug': 'BlockdevOptionsBlkdebug',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 793d70f..1bfbf1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -850,7 +850,7 @@ DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
> "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
> " [,cache.direct=on|off][,cache.no-flush=on|off]\n"
> " [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> - " [,driver specific parameters...]\n"
> + " [,compress=on|off][,driver specific parameters...]\n"
> " configure a block backend\n", QEMU_ARCH_ALL)
> STEXI
> @item -blockdev @var{option}[,@var{option}[,@var{option}[,...]]]
> @@ -905,6 +905,8 @@ discard requests.
> conversion of plain zero writes by the OS to driver specific optimized
> zero write commands. You may even choose "unmap" if @var{discard} is set
> to "unmap" to allow a zero write to be converted to an @code{unmap}
> operation.
> +@item compress
> +Compress all writes to the image.
> @end table
>
> @item Driver-specific options for @code{file}
> @@ -1026,7 +1028,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
> "
> [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
> " [,snapshot=on|off][,rerror=ignore|stop|report]\n"
> "
> [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> - " [,readonly=on|off][,copy-on-read=on|off]\n"
> + " [,readonly=on|off][,copy-on-read=on|off][,compress=on|off]\n"
> " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
> " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
> " [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
>
With extra new line dropped and qiov_offset fixed to zero:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
But, I can't be really sure, as I see from this patch, we have so many ways for
a simple option, and so many places where it should be added.. I just don't see
anything wrong (except for qiov_offset), so if tests work, it's OK for me.
Hope someone who knows the whole options architecture will look at it.
--
Best regards,
Vladimir
