multiwrite patches for 2.2
cover multiwrite patches for 2.2
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introduce bdrv_runtime_opts
block: add a knob to disable multiwrite_merge hw/virtio-blk: add a constant for max number of merged requests
block.c | 50 ++++++++++++++++++++++++++++++++++++++------ block/accounting.c | 8 ++++++- block/qapi.c | 3 +++ hmp.c | 10 ++++++++- hw/block/virtio-blk.c | 4 +++- include/block/accounting.h | 3 +++ include/block/block_int.h | 1 + qapi/block-core.json | 19 +++++++++++++++-- qemu-options.hx | 1 + qmp-commands.hx | 2 ++ 10 files changed, 90 insertions(+), 11 deletions(-)
Signature · 1 line
1.7.9.5
PATCH 1/4 block: add accounting for merged requests
--- block.c | 2 ++ block/accounting.c | 8 +++++++- block/qapi.c | 2 ++ hmp.c | 6 +++++- include/block/accounting.h | 3 +++ qapi/block-core.json | 9 ++++++++- 6 files changed, 27 insertions(+), 3 deletions(-)
block.c +2 -0
@@ -4466,6 +4466,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, } } +block_merge_done (&bs->stats,BLOCK_ACCT_WRITE , num_reqs - outidx -1 ); +return outidx +1 ; }
block/accounting.c +7 -1
@@ -44,7 +44,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie) stats->total_time_ns[cookie->type] +=get_clock () - cookie->start_time_ns; } -void block_acct_highest_sector (BlockAcctStats *stats,int64_t sector_num,unsigned int nb_sectors) {
@@ -52,3 +51,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num, stats->wr_highest_sector = sector_num + nb_sectors -1 ; } } + +void block_merge_done (BlockAcctStats *stats,enum BlockAcctType type, +int num_requests) +{ +assert (type <BLOCK_MAX_IOTYPE ); + stats->merged[type] += num_requests; +}
block/qapi.c +2 -0
@@ -338,6 +338,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs) s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE ]; s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ ]; s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE ]; + s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ ]; + s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE ]; s->stats->wr_highest_offset = bs->stats.wr_highest_sector *BDRV_SECTOR_SIZE ; s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH ];
hmp.c +5 -1
@@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)" wr_total_time_ns=%" PRId64" rd_total_time_ns=%" PRId64" flush_total_time_ns=%" PRId64 +" rd_merged=%" PRId64 +" wr_merged=%" PRId64"\n" , stats->value->stats->rd_bytes, stats->value->stats->wr_bytes,
@@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict) stats->value->stats->flush_operations, stats->value->stats->wr_total_time_ns, stats->value->stats->rd_total_time_ns, - stats->value->stats->flush_total_time_ns); + stats->value->stats->flush_total_time_ns, + stats->value->stats->rd_merged, + stats->value->stats->wr_merged); }qapi_free_BlockStatsList (stats_list);
include/block/accounting.h +3 -0
@@ -39,6 +39,7 @@ typedef struct BlockAcctStats {uint64_t nr_bytes[BLOCK_MAX_IOTYPE ];uint64_t nr_ops[BLOCK_MAX_IOTYPE ];uint64_t total_time_ns[BLOCK_MAX_IOTYPE ]; +uint64_t merged[BLOCK_MAX_IOTYPE ];uint64_t wr_highest_sector; } BlockAcctStats;
@@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,void block_acct_done (BlockAcctStats *stats,BlockAcctCookie *cookie);void block_acct_highest_sector (BlockAcctStats *stats,int64_t sector_num,unsigned int nb_sectors); +void block_merge_done (BlockAcctStats *stats,enum BlockAcctType type, +int num_requests);#endif
qapi/block-core.json +8 -1
@@ -392,13 +392,20 @@ # growable sparse files (like qcow2) that are used on top # of a physical device. # +# @rd_merged: Reserved (Since 2.3) +# +# @wr_merged: Number of requests that have been merged into another request +# while performing a multiwrite_merge. (Since 2.3) +# +# # Since: 0.14.0 ## { 'type': 'BlockDeviceStats', 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int', 'wr_operations': 'int', 'flush_operations': 'int', 'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int', - 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } } + 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int', + 'rd_merged': 'int', 'wr_merged': 'int' } } ## # @BlockStats:
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 1/4] block: add accounting for merged requests
Quoted 28 lines · Peter Lieven
Signed-off-by Peter Lieven <[email protected]>--- block.c | 2 ++ block/accounting.c | 8 +++++++- block/qapi.c | 2 ++ hmp.c | 6 +++++- include/block/accounting.h | 3 +++ qapi/block-core.json | 9 ++++++++- 6 files changed, 27 insertions(+), 3 deletions(-)block.c +2
-0@@ -4466,6 +4466,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, } } +block_merge_done (&bs->stats,BLOCK_ACCT_WRITE , num_reqs - outidx -1 ); +return outidx +1 ; }block/accounting.c +0
-0
block/accounting.c void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie) · line 49
Quoted 7 lines
block/accounting.c +0
-1@@ -44,7 +44,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie) stats->total_time_ns[cookie->type] +=get_clock () - cookie->start_time_ns; } -void block_acct_highest_sector (BlockAcctStats *stats,int64_t sector_num,unsigned int nb_sectors) {
Was this hunk intended?
Quoted 73 lines
block/accounting.c +7
-0@@ -52,3 +51,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num, stats->wr_highest_sector = sector_num + nb_sectors -1 ; } } + +void block_merge_done (BlockAcctStats *stats,enum BlockAcctType type, +int num_requests) +{ +assert (type <BLOCK_MAX_IOTYPE ); + stats->merged[type] += num_requests; +}block/qapi.c +2
-0@@ -338,6 +338,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs) s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE ]; s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ ]; s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE ]; + s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ ]; + s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE ]; s->stats->wr_highest_offset = bs->stats.wr_highest_sector *BDRV_SECTOR_SIZE ; s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH ];hmp.c +5
-1@@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)" wr_total_time_ns=%" PRId64" rd_total_time_ns=%" PRId64" flush_total_time_ns=%" PRId64 +" rd_merged=%" PRId64 +" wr_merged=%" PRId64"\n" , stats->value->stats->rd_bytes, stats->value->stats->wr_bytes,@@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict) stats->value->stats->flush_operations, stats->value->stats->wr_total_time_ns, stats->value->stats->rd_total_time_ns, - stats->value->stats->flush_total_time_ns); + stats->value->stats->flush_total_time_ns, + stats->value->stats->rd_merged, + stats->value->stats->wr_merged); }qapi_free_BlockStatsList (stats_list);include/block/accounting.h +3
-0@@ -39,6 +39,7 @@ typedef struct BlockAcctStats {uint64_t nr_bytes[BLOCK_MAX_IOTYPE ];uint64_t nr_ops[BLOCK_MAX_IOTYPE ];uint64_t total_time_ns[BLOCK_MAX_IOTYPE ]; +uint64_t merged[BLOCK_MAX_IOTYPE ];uint64_t wr_highest_sector; } BlockAcctStats;@@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,void block_acct_done (BlockAcctStats *stats,BlockAcctCookie *cookie);void block_acct_highest_sector (BlockAcctStats *stats,int64_t sector_num,unsigned int nb_sectors); +void block_merge_done (BlockAcctStats *stats,enum BlockAcctType type, +int num_requests);#endif qapi/block-core.json +0
-0
qapi/block-core.json line 400
Quoted 9 lines
qapi/block-core.json +6
-0@@ -392,13 +392,20 @@ # growable sparse files (like qcow2) that are used on top # of a physical device. # +# @rd_merged: Reserved (Since 2.3) +# +# @wr_merged: Number of requests that have been merged into another request +# while performing a multiwrite_merge. (Since 2.3) +# +#
2.2 or 2.3? ;-)
qapi/block-core.json line 411
Quoted 12 lines
qapi/block-core.json +2
-1# Since: 0.14.0 ## { 'type': 'BlockDeviceStats', 'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int', 'wr_operations': 'int', 'flush_operations': 'int', 'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int', - 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } } + 'rd_total_time_ns': 'int', 'wr_highest_offset': 'int', + 'rd_merged': 'int', 'wr_merged': 'int' } } ## # @BlockStats:
With the hunk removing the empty line removed (I know that line should not be there, but there's no point in removing it in this patch) and with either 2.2 or 2.3 in block-core.json:
PATCH 2/4 block: introduce bdrv_runtime_opts
In reply to Peter LievenThis patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList. The list will absorb all options that belong to the BDS (and not the BlockBackend) and will be parsed and handled in bdrv_open_common.
--- block.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)
block.c +33 -6
@@ -27,6 +27,7 @@#include "block/block_int.h" #include "block/blockjob.h" #include "qemu/module.h" +#include "qapi/qmp/qbool.h" #include "qapi/qmp/qjson.h" #include "sysemu/block-backend.h" #include "sysemu/sysemu.h"
@@ -875,6 +876,19 @@ static void bdrv_assign_node_name(BlockDriverState *bs,QTAILQ_INSERT_TAIL (&graph_bdrv_states, bs, node_list); } +static QemuOptsList bdrv_runtime_opts = { + .name ="bdrv_common" , + .head =QTAILQ_HEAD_INITIALIZER (bdrv_runtime_opts.head), + .desc = { + { + .name ="node-name" , + .type =QEMU_OPT_STRING , + .help ="Node name of the block device node" , + }, + {/* end of list */ } + }, +}; +/* * Common part for opening disk images and files *
@@ -886,6 +900,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,int ret, open_flags;const char *filename;const char *node_name =NULL ; +QemuOpts *opts;Error *local_err =NULL ;assert (drv !=NULL );
@@ -906,19 +921,28 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,trace_bdrv_open_common (bs, filename ?:"" , flags, drv->format_name); - node_name =qdict_get_try_str (options,"node-name" ); + opts =qemu_opts_create (&bdrv_runtime_opts,NULL ,0 , &error_abort); +qemu_opts_absorb_qdict (opts, options, &local_err); +if (local_err) { +error_propagate (errp, local_err); + ret = -EINVAL ; + goto fail_opts; + } + + node_name =qemu_opt_get (opts,"node-name" );bdrv_assign_node_name (bs, node_name, &local_err);if (local_err) {error_propagate (errp, local_err); -return -EINVAL ; + ret = -EINVAL ; + goto fail_opts; } -qdict_del (options,"node-name" );/* bdrv_open() with directly using a protocol as drv. This layer is already * opened, so assign it to bs (while file becomes a closed BlockDriverState) * and return immediately. */ if (file !=NULL && drv->bdrv_file_open) {bdrv_swap (file, bs); +qemu_opts_del (opts);return 0 ; }
@@ -936,7 +960,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, ?"Driver '%s' can only be used for read-only devices" :"Driver '%s' is not whitelisted" , drv->format_name); -return -ENOTSUP ; + ret = -ENOTSUP ; + goto fail_opts; }assert (bs->copy_on_read ==0 );/* bdrv_new() and bdrv_close() make it so */
@@ -945,7 +970,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,bdrv_enable_copy_on_read (bs); }else {error_setg (errp,"Can't use copy-on-read on read-only device" ); -return -EINVAL ; + ret = -EINVAL ; + goto fail_opts; } }
@@ -958,7 +984,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bs->drv = drv; bs->opaque =g_malloc0 (drv->instance_size); - bs->enable_write_cache = !!(flags &BDRV_O_CACHE_WB );/* Open the image, either directly or using a protocol */
@@ -1010,6 +1035,8 @@ free_and_fail:g_free (bs->opaque); bs->opaque =NULL ; bs->drv =NULL ; +fail_opts: +qemu_opts_del (opts);return ret; }
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 2/4] block: introduce bdrv_runtime_opts
Quoted 103 lines · Peter Lieven
This patch (orginally by Kevin) adds a
bdrv_runtime_optsQemuOptsList. The list will absorb all options that belong to the BDS (and not the BlockBackend) and will be parsed and handled inbdrv_open_common.Signed-off-by Kevin Wolf <[email protected]>Signed-off-by Peter Lieven <[email protected]>--- block.c | 39 +++++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-)block.c +31
-5@@ -27,6 +27,7 @@#include "block/block_int.h" #include "block/blockjob.h" #include "qemu/module.h" +#include "qapi/qmp/qbool.h" #include "qapi/qmp/qjson.h" #include "sysemu/block-backend.h" #include "sysemu/sysemu.h" @@ -875,6 +876,19 @@ static void bdrv_assign_node_name(BlockDriverState *bs,QTAILQ_INSERT_TAIL (&graph_bdrv_states, bs, node_list); } +static QemuOptsList bdrv_runtime_opts = { + .name ="bdrv_common" , + .head =QTAILQ_HEAD_INITIALIZER (bdrv_runtime_opts.head), + .desc = { + { + .name ="node-name" , + .type =QEMU_OPT_STRING , + .help ="Node name of the block device node" , + }, + {/* end of list */ } + }, +}; +/* * Common part for opening disk images and files * @@ -886,6 +900,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,int ret, open_flags;const char *filename;const char *node_name =NULL ; +QemuOpts *opts;Error *local_err =NULL ;assert (drv !=NULL );@@ -906,19 +921,28 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,trace_bdrv_open_common (bs, filename ?:"" , flags, drv->format_name); - node_name =qdict_get_try_str (options,"node-name" ); + opts =qemu_opts_create (&bdrv_runtime_opts,NULL ,0 , &error_abort); +qemu_opts_absorb_qdict (opts, options, &local_err); +if (local_err) { +error_propagate (errp, local_err); + ret = -EINVAL ; + goto fail_opts; + } + + node_name =qemu_opt_get (opts,"node-name" );bdrv_assign_node_name (bs, node_name, &local_err);if (local_err) {error_propagate (errp, local_err); -return -EINVAL ; + ret = -EINVAL ; + goto fail_opts; } -qdict_del (options,"node-name" );/* bdrv_open() with directly using a protocol as drv. This layer is already * opened, so assign it to bs (while file becomes a closed BlockDriverState) * and return immediately. */ if (file !=NULL && drv->bdrv_file_open) {bdrv_swap (file, bs); +qemu_opts_del (opts);return 0 ; }@@ -936,7 +960,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, ?"Driver '%s' can only be used for read-only devices" :"Driver '%s' is not whitelisted" , drv->format_name); -return -ENOTSUP ; + ret = -ENOTSUP ; + goto fail_opts; }assert (bs->copy_on_read ==0 );/* bdrv_new() and bdrv_close() make it so */ @@ -945,7 +970,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,bdrv_enable_copy_on_read (bs); }else {error_setg (errp,"Can't use copy-on-read on read-only device" ); -return -EINVAL ; + ret = -EINVAL ; + goto fail_opts; } }
block.c static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, · line 989
Quoted 7 lines
block.c +0
-1@@ -958,7 +984,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bs->drv = drv; bs->opaque =g_malloc0 (drv->instance_size); - bs->enable_write_cache = !!(flags &BDRV_O_CACHE_WB );/* Open the image, either directly or using a protocol */
I know it's not even your patch originally, but I still don't like spurious empty line removals, whoever might be responsible for them.
block.c free_and_fail: · line 1042
Quoted 9 lines
block.c +2
-0@@ -1010,6 +1035,8 @@ free_and_fail:g_free (bs->opaque); bs->opaque =NULL ; bs->drv =NULL ; +fail_opts: +qemu_opts_del (opts);return ret; }
With that empty line removal hunk removed:
PATCH 3/4 block: add a knob to disable multiwrite_merge
In reply to Peter Lieventhe block layer silently merges write requests since commit 40b4f539. This patch adds a knob to disable this feature as there has been some discussion lately if multiwrite is a good idea at all and as it falsifies benchmarks.
--- block.c | 9 +++++++++ block/qapi.c | 1 + hmp.c | 4 ++++ include/block/block_int.h | 1 + qapi/block-core.json | 10 +++++++++- qemu-options.hx | 1 + qmp-commands.hx | 2 ++ 7 files changed, 27 insertions(+), 1 deletion(-)
block.c +9 -0
@@ -884,6 +884,10 @@ static QemuOptsList bdrv_runtime_opts = { .name = "node-name", .type = QEMU_OPT_STRING, .help = "Node name of the block device node", + },{ + .name = "write-merging", + .type = QEMU_OPT_BOOL, + .help = "enable write merging (default: true)", }, {/* end of list */ } },
@@ -985,6 +989,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bs->drv = drv; bs->opaque =g_malloc0 (drv->instance_size); bs->enable_write_cache = !!(flags &BDRV_O_CACHE_WB ); + bs->write_merging =qemu_opt_get_bool (opts,"write-merging" , true);/* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) {
@@ -4439,6 +4444,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, {int i, outidx; +if (!bs->write_merging) { +return num_reqs; + } +// Sort requests by start sector qsort (reqs, num_reqs,sizeof (*reqs), &multiwrite_req_compare);
block/qapi.c +1 -0
@@ -59,6 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) info->backing_file_depth =bdrv_get_backing_file_depth (bs); info->detect_zeroes = bs->detect_zeroes; + info->write_merging = bs->write_merging;if (bs->io_limits_enabled) {ThrottleConfig cfg;
hmp.c +4 -0
@@ -348,6 +348,10 @@ void hmp_info_block(Monitor *mon, const QDict *qdict) BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]); } +if (!info->value->inserted->write_merging) { +monitor_printf (mon," Write Merging: off\n" ); + } +if (info->value->inserted->bps || info->value->inserted->bps_rd || info->value->inserted->bps_wr
include/block/block_int.h +1 -0
@@ -402,6 +402,7 @@ struct BlockDriverState {QDict *options;BlockdevDetectZeroesOptions detect_zeroes; +bool write_merging;/* The error object in use for blocking operations on backing_hd */ Error *backing_blocker;
qapi/block-core.json +9 -1
@@ -214,6 +214,8 @@ # # @detect_zeroes: detect and optimize zero writes (Since 2.1) # +# @write_merging: true if write merging is enabled (Since 2.2) +# # @bps: total throughput limit in bytes per second is specified # # @bps_rd: read throughput limit in bytes per second is specified
@@ -250,6 +252,7 @@ '*backing_file': 'str', 'backing_file_depth': 'int', 'encrypted': 'bool', 'encryption_key_missing': 'bool', 'detect_zeroes': 'BlockdevDetectZeroesOptions', + 'write_merging': 'bool', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', 'image': 'ImageInfo',
@@ -1187,6 +1190,10 @@ # (default: false) # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) # (default: off) +# @write-merging: #optional enable the merging of write requests +# also known as multiwrite_merge (Since 2.2) +# (default: true, but this might change in the future +# depending on format/protocol/features used) # # Since: 1.7 ##
@@ -1200,7 +1207,8 @@ '*rerror': 'BlockdevOnError', '*werror': 'BlockdevOnError', '*read-only': 'bool', - '*detect-zeroes': 'BlockdevDetectZeroesOptions' } } + '*detect-zeroes': 'BlockdevDetectZeroesOptions', + '*write-merging': 'bool' } } ## # @BlockdevOptionsFile
qemu-options.hx +1 -0
@@ -432,6 +432,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, " [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n" " [,readonly=on|off][,copy-on-read=on|off]\n" " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n" + " [,write-merging=on|off]\n" " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n" " [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n" " [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"
qmp-commands.hx +2 -0
@@ -2104,6 +2104,7 @@ Each json-object contain the following: - "iops_size": I/O size when limiting by iops (json-int) - "detect_zeroes": detect and optimize zero writing (json-string) - Possible values: "off", "on", "unmap" + - "write_merging": enable merging of write requests (json-bool) - "image": the detail of the image, it is a json-object containing the following: - "filename": image file name (json-string)
@@ -2181,6 +2182,7 @@ Example: "iops_wr_max": 0, "iops_size": 0, "detect_zeroes": "on", + "write_merging": "true", "image":{ "filename":"disks/test.qcow2", "format":"qcow2",
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 3/4] block: add a knob to disable multiwrite_merge
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
It's still s/^t/T/ ;-)
Quoted 67 lines
commit
40b4f539. This patch adds a knob to disable this feature as there has been some discussion lately if multiwrite is a good idea at all and as it falsifies benchmarks.Signed-off-by Peter Lieven <[email protected]>--- block.c | 9 +++++++++ block/qapi.c | 1 + hmp.c | 4 ++++ include/block/block_int.h | 1 + qapi/block-core.json | 10 +++++++++- qemu-options.hx | 1 + qmp-commands.hx | 2 ++ 7 files changed, 27 insertions(+), 1 deletion(-)block.c +9
-0@@ -884,6 +884,10 @@ static QemuOptsList bdrv_runtime_opts = { .name = "node-name", .type = QEMU_OPT_STRING, .help = "Node name of the block device node", + },{ + .name = "write-merging", + .type = QEMU_OPT_BOOL, + .help = "enable write merging (default: true)", }, {/* end of list */ } },@@ -985,6 +989,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file, bs->drv = drv; bs->opaque =g_malloc0 (drv->instance_size); bs->enable_write_cache = !!(flags &BDRV_O_CACHE_WB ); + bs->write_merging =qemu_opt_get_bool (opts,"write-merging" , true);/* Open the image, either directly or using a protocol */ if (drv->bdrv_file_open) {@@ -4439,6 +4444,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs, {int i, outidx; +if (!bs->write_merging) { +return num_reqs; + } +// Sort requests by start sector qsort (reqs, num_reqs,sizeof (*reqs), &multiwrite_req_compare);block/qapi.c +1
-0@@ -59,6 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs) info->backing_file_depth =bdrv_get_backing_file_depth (bs); info->detect_zeroes = bs->detect_zeroes; + info->write_merging = bs->write_merging;if (bs->io_limits_enabled) {ThrottleConfig cfg;hmp.c +0
-0
hmp.c void hmp_info_block(Monitor *mon, const QDict *qdict) · line 354
Quoted 7 lines
hmp.c +4
-0@@ -348,6 +348,10 @@ void hmp_info_block(Monitor *mon, const QDict *qdict) BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]); } +if (!info->value->inserted->write_merging) { +monitor_printf (mon," Write Merging: off\n" ); + } +
Currently, write_merging == true is the default; but as you yourself describe in block-core.json, this may change. I think always printing ths field's value would be fine, too.
Quoted 20 lines
hmp.c +0
-0if (info->value->inserted->bps || info->value->inserted->bps_rd || info->value->inserted->bps_wrinclude/block/block_int.h +1
-0@@ -402,6 +402,7 @@ struct BlockDriverState {QDict *options;BlockdevDetectZeroesOptions detect_zeroes; +bool write_merging;/* The error object in use for blocking operations on backing_hd */ Error *backing_blocker;qapi/block-core.json +0
-0
qapi/block-core.json line 218
Quoted 5 lines
qapi/block-core.json +2
-0@@ -214,6 +214,8 @@ # # @detect_zeroes: detect and optimize zero writes (Since 2.1) # +# @write_merging: true if write merging is enabled (Since 2.2) +#
If this is 2.2, patch 1 must say 2.2 as well.
Quoted 12 lines
qapi/block-core.json +1
-0# @bps: total throughput limit in bytes per second is specified # # @bps_rd: read throughput limit in bytes per second is specified@@ -250,6 +252,7 @@ '*backing_file': 'str', 'backing_file_depth': 'int', 'encrypted': 'bool', 'encryption_key_missing': 'bool', 'detect_zeroes': 'BlockdevDetectZeroesOptions', + 'write_merging': 'bool', 'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int', 'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int', 'image': 'ImageInfo',
qapi/block-core.json line 1196
Quoted 7 lines
qapi/block-core.json +4
-0@@ -1187,6 +1190,10 @@ # (default: false) # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1) # (default: off) +# @write-merging: #optional enable the merging of write requests +# also known as multiwrite_merge (Since 2.2) +# (default: true, but this might change in the future +# depending on format/protocol/features used)
I'd ignore the parenthesis for alignment (which means remove one space in front of "depending"), but I don't object either.
Quoted 38 lines
qapi/block-core.json +2
-1# # Since: 1.7 ##@@ -1200,7 +1207,8 @@ '*rerror': 'BlockdevOnError', '*werror': 'BlockdevOnError', '*read-only': 'bool', - '*detect-zeroes': 'BlockdevDetectZeroesOptions' } } + '*detect-zeroes': 'BlockdevDetectZeroesOptions', + '*write-merging': 'bool' } } ## # @BlockdevOptionsFileqemu-options.hx +1
-0@@ -432,6 +432,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive, " [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n" " [,readonly=on|off][,copy-on-read=on|off]\n" " [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n" + " [,write-merging=on|off]\n" " [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n" " [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n" " [[,bps_max=bm]|[[,bps_rd_max=rm][,bps_wr_max=wm]]]\n"qmp-commands.hx +1
-0@@ -2104,6 +2104,7 @@ Each json-object contain the following: - "iops_size": I/O size when limiting by iops (json-int) - "detect_zeroes": detect and optimize zero writing (json-string) - Possible values: "off", "on", "unmap" + - "write_merging": enable merging of write requests (json-bool) - "image": the detail of the image, it is a json-object containing the following: - "filename": image file name (json-string)
qmp-commands.hx Example: · line 2188
Quoted 7 lines
qmp-commands.hx +1
-0@@ -2181,6 +2182,7 @@ Example: "iops_wr_max": 0, "iops_size": 0, "detect_zeroes": "on", + "write_merging": "true", "image":{ "filename":"disks/test.qcow2", "format":"qcow2",
PATCH 4/4 hw/virtio-blk: add a constant for max number of merged requests
In reply to Peter LievenAs it was not obvious (at least for me) where the 32 comes from add a constant for it.
--- hw/block/virtio-blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
hw/block/virtio-blk.c +3 -1
@@ -308,6 +308,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,return true; } +#define MAX_MERGED_REQS 32 +static void virtio_blk_handle_write (VirtIOBlockReq *req,MultiReqBuffer *mrb) {BlockRequest *blkreq;
@@ -326,7 +328,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)block_acct_start (blk_get_stats (req->dev->blk), &req->acct, req->qiov.size,BLOCK_ACCT_WRITE ); -if (mrb->num_writes ==32 ) { +if (mrb->num_writes ==MAX_MERGED_REQS ) {virtio_submit_multiwrite (req->dev->blk, mrb); }
Signature · 1 line
1.7.9.5
Re: [Qemu-devel] [PATCH 4/4] hw/virtio-blk: add a constant for max number of merged requests
Quoted 2 lines · Peter Lieven
As it was not obvious (at least for me) where the 32 comes from add a constant for it.
This needs some separator ("...comes from; add a..." or something like that).
Quoted 19 lines
Signed-off-by Peter Lieven <[email protected]>--- hw/block/virtio-blk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)hw/block/virtio-blk.c +2
-0@@ -308,6 +308,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,return true; } +#define MAX_MERGED_REQS 32 +static void virtio_blk_handle_write (VirtIOBlockReq *req,MultiReqBuffer *mrb) {BlockRequest *blkreq;
hw/block/virtio-blk.c static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb) · line 334
Quoted 8 lines
hw/block/virtio-blk.c +1
-1@@ -326,7 +328,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)block_acct_start (blk_get_stats (req->dev->blk), &req->acct, req->qiov.size,BLOCK_ACCT_WRITE ); -if (mrb->num_writes ==32 ) { +if (mrb->num_writes ==MAX_MERGED_REQS ) {virtio_submit_multiwrite (req->dev->blk, mrb); }
I wouldn't have used the past form, but simply MAX_MERGE_REQS or, more complicated, MAX_MERGE_REQ_COUNT. But I'm me, and this mail's "From" field says it's not from me.
Quoted 8 lines · Peter Lieven
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requests
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Max
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 11 lines · Peter Lieven
Quoted 8 lines
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requestsIn addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Peter
Quoted 2 lines
Max
Quoted 11 lines · Peter Lieven
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 9 lines · Peter Lieven
Quoted 8 lines
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requests
Quoted 4 lines
Quoted 1 line
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
Max
Quoted 12 lines · Max Reitz
Quoted 11 lines · Peter Lieven
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 9 lines · Peter Lieven
Quoted 8 lines
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requests
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
Understood.
Thanks
Peter
Quoted 12 lines · Max Reitz
Quoted 11 lines · Peter Lieven
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 9 lines · Peter Lieven
Quoted 8 lines
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requests
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
It seems it is currently not visible in query-block which paramters are set to e.g. file. If I specifiy -drive file.write_merging=off its accepted but not displayed. Any ideas?
Peter
Quoted 13 lines · Peter Lieven
Quoted 12 lines · Max Reitz
Quoted 11 lines · Peter Lieven
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 9 lines · Peter Lieven
Quoted 8 lines
This adds some preparing patches for upcoming multiwrite modifications. I will leave the dangerous patches for after 2.2 release.
Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requests
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
It seems it is currently not visible in query-block which paramters are set to e.g. file. If I specifiy -drive file.write_merging=off its accepted but not displayed. Any ideas?
Hm, I forgot that query-block only gives you the root BDS. Too bad. Then we cannot test that. We probably want to have a QMP command to query the whole BDS graph some time, but it isn't there yet.
Max
Am 21.10.2014 um 11:07 hat Max Reitz geschrieben:
Quoted 26 lines · Peter Lieven
Quoted 25 lines · Max Reitz
Quoted 23 lines · Peter Lieven
Quoted 12 lines
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 10 lines · Peter Lieven
Quoted 9 lines
This adds some preparing patches for upcoming multiwrite modifications.
I will leave the dangerous patches for after 2.2 release.Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requestsQuoted 6 lines
Quoted 3 lines
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
Quoted 7 lines
Quoted 3 lines
It seems it is currently not visible in query-block which paramters are set to e.g. file. If I specifiy -drive file.write_merging=off its accepted but not displayed. Any ideas?
Hm, I forgot that query-block only gives you the root BDS. Too bad. Then we cannot test that. We probably want to have a QMP command to query the whole BDS graph some time, but it isn't there yet.
Does query-named-block-nodes help in this specific case?
Kevin
Quoted 25 lines · Kevin Wolf
Am 21.10.2014 um 11:07 hat Max Reitz geschrieben:
Quoted 24 lines · Peter Lieven
Quoted 23 lines · Max Reitz
Quoted 22 lines · Peter Lieven
Quoted 12 lines
Am 20.10.2014 um 17:56 schrieb Max Reitz <[email protected]>:
Quoted 10 lines · Peter Lieven
Quoted 9 lines
This adds some preparing patches for upcoming multiwrite modifications.
I will leave the dangerous patches for after 2.2 release.Peter Lieven (4):
block: add accounting for merged requests
block: introducebdrv_runtime_opts
block: add a knob to disablemultiwrite_mergehw/virtio-blk: add a constant for max number of merged requestsQuoted 6 lines
Quoted 3 lines
In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
Thanks for your comments. I will respin tomorrow.
What exactly would you like to check in a test?
Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
It seems it is currently not visible in query-block which paramters are set to e.g. file. If I specifiy -drive file.write_merging=off its accepted but not displayed. Any ideas?
Hm, I forgot that query-block only gives you the root BDS. Too bad. Then we cannot test that. We probably want to have a QMP command to query the whole BDS graph some time, but it isn't there yet.
Does query-named-block-nodes help in this specific case?
Oh, yes, it does. :-)
I think I should learn QMP once in a while...
Max