[Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
the 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.
Signed-off-by: Peter Lieven p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
if ((buf = qemu_opt_get(opts, discard)) != NULL) {
if (bdrv_parse_discard_flags(buf, bdrv_flags) != 0) {
@@ -530,6 +532,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
bs-open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
bs-read_only = ro;
bs-detect_zeroes = detect_zeroes;
+bs-write_merging = write_merging;
bdrv_set_on_error(bs, on_read_error, on_write_error);
@@ -2746,6 +2749,10 @@ QemuOptsList qemu_common_drive_opts = {
.name = detect-zeroes,
.type = QEMU_OPT_STRING,
.help = try to optimize zero writes (off, on, unmap),
+},{
+.name = write-merging,
+.type = QEMU_OPT_BOOL,
+.help = enable write merging (default: true),
},
{ /* end of list */ }
},
diff --git a/hmp.c b/hmp.c
index 63d7686..8d6ad0b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -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
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..39bbde2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -407,6 +407,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;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f7089e..4931bd9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -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',
@@ -1180,6 +1183,10 @@
# (default: false)
# @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
# (default: off)
+# @write-merging: #optional enable the merging of write
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or
disable merging for the top layer (the root BDS). Furthermore, since you
don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it
actually defaults to false and only for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite
to its file, the write will not be merged and the user can do nothing
about it. I don't suppose this is intentional...?
I propose evaluating the option in bdrv_open() and setting
bs-write_merging there.
if ((buf = qemu_opt_get(opts, discard)) != NULL) {
if (bdrv_parse_discard_flags(buf, bdrv_flags) != 0) {
@@ -530,6 +532,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
bs-open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
bs-read_only = ro;
bs-detect_zeroes = detect_zeroes;
+bs-write_merging = write_merging;
bdrv_set_on_error(bs, on_read_error, on_write_error);
@@ -2746,6 +2749,10 @@ QemuOptsList qemu_common_drive_opts = {
.name = detect-zeroes,
.type = QEMU_OPT_STRING,
.help = try to optimize zero writes (off, on, unmap),
+},{
+.name = write-merging,
+.type = QEMU_OPT_BOOL,
+.help = enable write merging (default: true),
},
{ /* end of list */ }
},
diff --git a/hmp.c b/hmp.c
index 63d7686..8d6ad0b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -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
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..39bbde2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -407,6 +407,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;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f7089e..4931bd9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -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
#
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other
options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and only
for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite to its
file, the write will not be merged and the user can do nothing about it. I
don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way to
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
I propose evaluating the option in bdrv_open() and setting bs-write_merging
there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP. So we
still might
need to check parameters at 2 positions? It is quite confusing which paramter
has to be parsed where.
Peter
if ((buf = qemu_opt_get(opts, discard)) != NULL) {
if (bdrv_parse_discard_flags(buf, bdrv_flags) != 0) {
@@ -530,6 +532,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
bs-open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
bs-read_only = ro;
bs-detect_zeroes = detect_zeroes;
+bs-write_merging = write_merging;
bdrv_set_on_error(bs, on_read_error, on_write_error);
@@ -2746,6 +2749,10 @@ QemuOptsList qemu_common_drive_opts = {
.name = detect-zeroes,
.type = QEMU_OPT_STRING,
.help = try to optimize zero writes (off, on, unmap),
+},{
+.name = write-merging,
+.type = QEMU_OPT_BOOL,
+.help = enable write merging (default: true),
},
{ /* end of list */ }
},
diff --git a/hmp.c b/hmp.c
index 63d7686..8d6ad0b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -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
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d86a6c..39bbde2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -407,6 +407,7 @@ struct BlockDriverState {
QDict *options;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
/* The error object in use for blocking operations on
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all
other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable
or disable merging for the top layer (the root BDS). Furthermore,
since you don't set bs-write_merging in bdrv_new() (or at least
bdrv_open()), it actually defaults to false and only for the top
layer it defaults to true.
Therefore, if after this patch a format block driver issues a
multiwrite to its file, the write will not be merged and the user can
do nothing about it. I don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only
way to enter multiwrite is from virtio_blk_handle_request in
virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root
BDS as well). But other than that, yes, you're right. So, in practice it
shouldn't matter.
I propose evaluating the option in bdrv_open() and setting
bs-write_merging there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to
QMP. So we still might
need to check parameters at 2 positions? It is quite confusing which
paramter has to be parsed where.
As for me, I don't know why some options are parsed in blockdev_init()
at all. I guess all the options currently parsed in blockdev_init()
should later be moved to the BlockBackend, at least that would be the
idea. In practice, we cannot do that: Things like caching will stay in
the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should be
in blockdev_init() and everything related to the BDS should be in
bdrv_open(). So the question is now whether you want write_merging to be
in the BDS or in the BB. Considering BB is in Kevin's block branch as of
last Friday, you might actually want to work on that branch and move the
field into the BB if you decide that that's the place it should be in.
Max
Peter
if ((buf = qemu_opt_get(opts, discard)) != NULL) {
if (bdrv_parse_discard_flags(buf, bdrv_flags) != 0) {
@@ -530,6 +532,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
bs-open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
bs-read_only = ro;
bs-detect_zeroes = detect_zeroes;
+bs-write_merging = write_merging;
bdrv_set_on_error(bs, on_read_error, on_write_error);
@@ -2746,6 +2749,10 @@ QemuOptsList qemu_common_drive_opts = {
.name = detect-zeroes,
.type = QEMU_OPT_STRING,
.help = try to optimize zero writes (off, on, unmap),
+},{
+.name = write-merging,
+.type = QEMU_OPT_BOOL,
+
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other
options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and only
for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite to its
file, the write will not be merged and the user can do nothing about it. I
don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way to
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root BDS as
well). But other than that, yes, you're right. So, in practice it shouldn't
matter.
I propose evaluating the option in bdrv_open() and setting bs-write_merging
there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP. So we
still might
need to check parameters at 2 positions? It is quite confusing which paramter
has to be parsed where.
As for me, I don't know why some options are parsed in blockdev_init() at all. I guess all the options currently parsed in blockdev_init() should later be moved to the BlockBackend, at least that would be the idea. In practice, we cannot do that: Things
like caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should be in blockdev_init() and everything related to the BDS should be in bdrv_open(). So the question is now whether you want write_merging to be in the BDS or in the BB. Considering BB is
in Kevin's block branch as of last Friday, you might actually want to work on that branch and move the field into the BB if you decide that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my intention
was to be able to turn it off. As there are People who would like to see it
completely disappear I would not spent too much effort in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But this is true
for discard_zeroes (and others) as well. Kevin, Stefan, ultimatively where
should it be parsed?
I have on my todo list the following to give you a figure what might happen.
All this is for 2.3+ except for the accounting maybe:
- add accounting for merged requests (to have a metric for modifications)
- evaluate if sorting the requests really helps
- simplify the merge conditions and do not keep a
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts,
all other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable
or disable merging for the top layer (the root BDS). Furthermore,
since you don't set bs-write_merging in bdrv_new() (or at least
bdrv_open()), it actually defaults to false and only for the top
layer it defaults to true.
Therefore, if after this patch a format block driver issues a
multiwrite to its file, the write will not be merged and the user
can do nothing about it. I don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The
only way to enter multiwrite is from virtio_blk_handle_request in
virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the
root BDS as well). But other than that, yes, you're right. So, in
practice it shouldn't matter.
I propose evaluating the option in bdrv_open() and setting
bs-write_merging there.
I wasn't aware actually. I remember that someone asked me to
implement discard_zeroes in blockdev_init. I think it was something
related to QMP. So we still might
need to check parameters at 2 positions? It is quite confusing which
paramter has to be parsed where.
As for me, I don't know why some options are parsed in
blockdev_init() at all. I guess all the options currently parsed in
blockdev_init() should later be moved to the BlockBackend, at least
that would be the idea. In practice, we cannot do that: Things like
caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should
be in blockdev_init() and everything related to the BDS should be in
bdrv_open(). So the question is now whether you want write_merging to
be in the BDS or in the BB. Considering BB is in Kevin's block branch
as of last Friday, you might actually want to work on that branch and
move the field into the BB if you decide that that's the place it
should be in.
Actually I there a pros and cons for both BDS and BB. As of now my
intention was to be able to turn it off. As there are People who would
like to see it completely disappear I would not spent too much effort
in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But
this is true for discard_zeroes (and others) as well. Kevin, Stefan,
ultimatively where should it be parsed?
Yes, and for cache, too. That's what I meant with it's just broken.
Max
I have on my todo list the following to give you a figure what might
happen. All this is for 2.3+ except for the accounting maybe:
- add accounting for merged
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other
options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and
only for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite to its
file, the write will not be merged and the user can do nothing about it. I
don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way to
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root BDS as
well). But other than that, yes, you're right. So, in practice it shouldn't
matter.
I propose evaluating the option in bdrv_open() and setting bs-write_merging
there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP. So we
still might
need to check parameters at 2 positions? It is quite confusing which paramter
has to be parsed where.
As for me, I don't know why some options are parsed in blockdev_init() at all. I guess all the options currently parsed in blockdev_init() should later be moved to the BlockBackend, at least that would be the idea. In practice, we cannot do that:
Things like caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should be in blockdev_init() and everything related to the BDS should be in bdrv_open(). So the question is now whether you want write_merging to be in the BDS or in the BB. Considering BB
is in Kevin's block branch as of last Friday, you might actually want to work on that branch and move the field into the BB if you decide that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my intention
was to be able to turn it off. As there are People who would like to see it
completely disappear I would not spent too much effort in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But this is true
for discard_zeroes (and others) as well. Kevin, Stefan, ultimatively where
should it be parsed?
Yes, and for cache, too. That's what I meant with it's just broken.
Looking at the old discussion about discard zeroes it was recommended to put it
into bdrv_open_common. If thats still the recommendation I will put it
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other
options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and
only for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite to its
file, the write will not be merged and the user can do nothing about it. I
don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way to
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root BDS as
well). But other than that, yes, you're right. So, in practice it shouldn't
matter.
I propose evaluating the option in bdrv_open() and setting bs-write_merging
there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP. So we
still might
need to check parameters at 2 positions? It is quite confusing which paramter
has to be parsed where.
As for me, I don't know why some options are parsed in blockdev_init() at all. I guess all the options currently parsed in blockdev_init() should later be moved to the BlockBackend, at least that would be the idea. In practice, we cannot do that:
Things like caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should be in blockdev_init() and everything related to the BDS should be in bdrv_open(). So the question is now whether you want write_merging to be in the BDS or in the BB. Considering BB
is in Kevin's block branch as of last Friday, you might actually want to work on that branch and move the field into the BB if you decide that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my intention
was to be able to turn it off. As there are People who would like to see it
completely disappear I would not spent too much effort in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But this is true
for discard_zeroes (and others) as well. Kevin, Stefan, ultimatively where
should it be parsed?
Yes, and for cache, too. That's what I meant with it's just broken.
Can you further help here. I think my problem was that I don't have access to
the commandline options in bdrv_open?!
Thank you,
Peter
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts,
all other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char
*file, QDict *bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read,
false);
+write_merging = qemu_opt_get_bool(opts, write-merging,
true);
Using this option in blockdev_init() means that you can only
enable or disable merging for the top layer (the root BDS).
Furthermore, since you don't set bs-write_merging in bdrv_new()
(or at least bdrv_open()), it actually defaults to false and only
for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a
multiwrite to its file, the write will not be merged and the user
can do nothing about it. I don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The
only way to enter multiwrite is from virtio_blk_handle_request in
virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the
root BDS as well). But other than that, yes, you're right. So, in
practice it shouldn't matter.
I propose evaluating the option in bdrv_open() and setting
bs-write_merging there.
I wasn't aware actually. I remember that someone asked me to
implement discard_zeroes in blockdev_init. I think it was
something related to QMP. So we still might
need to check parameters at 2 positions? It is quite confusing
which paramter has to be parsed where.
As for me, I don't know why some options are parsed in
blockdev_init() at all. I guess all the options currently parsed in
blockdev_init() should later be moved to the BlockBackend, at least
that would be the idea. In practice, we cannot do that: Things like
caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should
be in blockdev_init() and everything related to the BDS should be
in bdrv_open(). So the question is now whether you want
write_merging to be in the BDS or in the BB. Considering BB is in
Kevin's block branch as of last Friday, you might actually want to
work on that branch and move the field into the BB if you decide
that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my
intention was to be able to turn it off. As there are People who
would like to see it completely disappear I would not spent too much
effort in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But
this is true for discard_zeroes (and others) as well. Kevin, Stefan,
ultimatively where should it be parsed?
Yes, and for cache, too. That's what I meant with it's just broken.
Can you further help here. I think my problem was that I don't have
access to the
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all other
options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file, QDict
*bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can only enable or disable merging for the top layer (the root BDS). Furthermore, since you don't set bs-write_merging in bdrv_new() (or at least bdrv_open()), it actually defaults to false and
only for the top layer it defaults to true.
Therefore, if after this patch a format block driver issues a multiwrite to its
file, the write will not be merged and the user can do nothing about it. I
don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way to
enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root BDS as
well). But other than that, yes, you're right. So, in practice it shouldn't
matter.
I propose evaluating the option in bdrv_open() and setting bs-write_merging
there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP. So we
still might
need to check parameters at 2 positions? It is quite confusing which paramter
has to be parsed where.
As for me, I don't know why some options are parsed in blockdev_init() at all. I guess all the options currently parsed in blockdev_init() should later be moved to the BlockBackend, at least that would be the idea. In practice, we cannot do that:
Things like caching will stay in the BlockDriverState.
I think it's just broken. IMHO, everything related to the BB should be in blockdev_init() and everything related to the BDS should be in bdrv_open(). So the question is now whether you want write_merging to be in the BDS or in the BB. Considering BB
is in Kevin's block branch as of last Friday, you might actually want to work on that branch and move the field into the BB if you decide that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my intention
was to be able to turn it off. As there are People who would like to see it
completely disappear I would not spent too much effort in that switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But this is true
for discard_zeroes (and others) as well. Kevin, Stefan, ultimatively where
should it be parsed?
Yes, and for cache, too. That's what I meant with it's just broken.
Can you further help here. I think my problem was that I don't have
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
Am 20.10.2014 um 13:53 hat Peter Lieven geschrieben:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
On 20.10.2014 11:27, Max Reitz wrote:
On 2014-10-20 at 11:14, Peter Lieven wrote:
On 20.10.2014 10:59, Max Reitz wrote:
On 2014-10-20 at 08:14, Peter Lieven wrote:
the block layer silently merges write requests since
s/^t/T/
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 p...@kamp.de
---
block.c |4
block/qapi.c |1 +
blockdev.c|7 +++
hmp.c |4
include/block/block_int.h |1 +
qapi/block-core.json | 10 +-
qemu-options.hx |1 +
qmp-commands.hx |2 ++
8 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 27533f3..1658a72 100644
--- a/block.c
+++ b/block.c
@@ -4531,6 +4531,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);
diff --git a/block/qapi.c b/block/qapi.c
index 9733ebd..02251dd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -58,6 +58,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;
diff --git a/blockdev.c b/blockdev.c
index e595910..13e47b8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -378,6 +378,7 @@ static DriveInfo *blockdev_init(const char *file,
QDict *bs_opts,
const char *id;
bool has_driver_specific_opts;
BlockdevDetectZeroesOptions detect_zeroes;
+bool write_merging;
BlockDriver *drv = NULL;
/* Check common options by copying from bs_opts to opts, all
other options
@@ -405,6 +406,7 @@ static DriveInfo *blockdev_init(const char *file,
QDict *bs_opts,
snapshot = qemu_opt_get_bool(opts, snapshot, 0);
ro = qemu_opt_get_bool(opts, read-only, 0);
copy_on_read = qemu_opt_get_bool(opts, copy-on-read, false);
+write_merging = qemu_opt_get_bool(opts, write-merging, true);
Using this option in blockdev_init() means that you can
only enable or disable merging for the top layer (the root
BDS). Furthermore, since you don't set bs-write_merging
in bdrv_new() (or at least bdrv_open()), it actually
defaults to false and only for the top layer it defaults
to true.
Therefore, if after this patch a format block driver issues a multiwrite
to its file, the write will not be merged and the user can do nothing
about it. I don't suppose this is intentional...?
I am not sure if a block driver actually can do this at all? The only way
to enter multiwrite is from virtio_blk_handle_request in virtio-blk.c.
Well, there's also qemu-io -c multiwrite (which only accesses the root BDS
as well). But other than that, yes, you're right. So, in practice it
shouldn't matter.
I propose evaluating the option in bdrv_open() and setting
bs-write_merging there.
I wasn't aware actually. I remember that someone asked me to implement
discard_zeroes in blockdev_init. I think it was something related to QMP.
So we still might
need to check parameters at 2 positions? It is quite confusing which
paramter has to be parsed where.
As for me, I don't know why some options are parsed in
blockdev_init() at all. I guess all the options currently
parsed in blockdev_init() should later be moved to the
BlockBackend, at least that would be the idea. In practice, we
cannot do that: Things like caching will stay in the
BlockDriverState.
I think it's just broken. IMHO, everything related to the BB
should be in blockdev_init() and everything related to the BDS
should be in bdrv_open(). So the question is now whether you
want write_merging to be in the BDS or in the BB. Considering
BB is in Kevin's block branch as of last Friday, you might
actually want to work on that branch and move the field into
the BB if you decide that that's the place it should be in.
Actually I there a pros and cons for both BDS and BB. As of now my
intention was to be able to turn it off. As there are People who would like
to see it completely disappear I would not spent too much effort in that
switch today.
Looking at BB it is a BDS thing and thus belongs to bdrv_open. But this is
true for discard_zeroes (and others) as well. Kevin, Stefan, ultimatively
where should it be parsed?
Yes, and for cache, too. That's what I meant with it's just
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I don't have
access to the commandline options in bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;, it's there
for me (directly after qdict_del(options, node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open disk
image image.qcow2: Block format 'qcow2' used by device 'virtio0'
doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem is that
there are (at least) two parameter types in qemu: One is just giving a
QDict, and the other are QemuOpts. QDicts are just the raw user input
and the user can only input strings, so everything is just a string. As
far as I know, typing everything correctly is done by converting the
QDict to a QemuOpts object (as you can see in generally every block
driver which supports some options (e.g. qcow2) and also in
blockdev_init(), it's qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast to the
root BDS, which are parsed in blockdev_init()), so you now have the
honorable task of introducing such a QemuOptsList along with
qemu_opts_absorb_qdict() and everything to bdrv_open_common(). *cough*
Max
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I don't have access to
the commandline options in bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;, it's there for me (directly after
qdict_del(options, node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive if=virtio,file=image.qcow2,write-merging=off: could
not open disk image image.qcow2: Block format 'qcow2' used by device 'virtio0'
doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem is that there are (at least) two parameter types in qemu: One is just giving a QDict, and the other are QemuOpts. QDicts are just the raw user input and the user can only input strings,
so everything is just a string. As far as I know, typing everything correctly is done by converting the QDict to a QemuOpts object (as you can see in generally every block driver which supports some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string non-block-driver-specific options for mid-tree BDS (in contrast to the root BDS, which are parsed in blockdev_init()), so you now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole stuff would
start this. Or we postpone this know until all the ongoing conversions are done.
Peter
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I don't
have access to the commandline options in bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;, it's
there for me (directly after qdict_del(options, node-name). The
output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open disk
image image.qcow2: Block format 'qcow2' used by device 'virtio0'
doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem is
that there are (at least) two parameter types in qemu: One is just
giving a QDict, and the other are QemuOpts. QDicts are just the raw
user input and the user can only input strings, so everything is just
a string. As far as I know, typing everything correctly is done by
converting the QDict to a QemuOpts object (as you can see in
generally every block driver which supports some options (e.g. qcow2)
and also in blockdev_init(), it's qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast to
the root BDS, which are parsed in blockdev_init()), so you now have
the honorable task of introducing such a QemuOptsList along with
qemu_opts_absorb_qdict() and everything to bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the ongoing
conversions are done.
I can try and create some barebone which your patches can then be based
on. I probably don't have the knowledge either, but I'm daring enough to
do it anyway. ;-)
Max
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 15:22, Max Reitz wrote:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I don't have access to
the commandline options in bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;, it's there for me (directly after
qdict_del(options, node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive if=virtio,file=image.qcow2,write-merging=off: could
not open disk image image.qcow2: Block format 'qcow2' used by device 'virtio0'
doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem is that there are (at least) two parameter types in qemu: One is just giving a QDict, and the other are QemuOpts. QDicts are just the raw user input and the user can only input
strings, so everything is just a string. As far as I know, typing everything correctly is done by converting the QDict to a QemuOpts object (as you can see in generally every block driver which supports some options (e.g. qcow2) and also in
blockdev_init(), it's qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string non-block-driver-specific options for mid-tree BDS (in contrast to the root BDS, which are parsed in blockdev_init()), so you now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole stuff would
start this. Or we postpone this know until all the ongoing conversions are done.
I can try and create some barebone which your patches can then be based on. I
probably don't have the knowledge either, but I'm daring enough to do it
anyway. ;-)
Thank you.
Peter
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I
don't have access to the commandline options in
bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;,
it's there for me (directly after qdict_del(options,
node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open
disk image image.qcow2: Block format 'qcow2' used by device
'virtio0' doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem
is that there are (at least) two parameter types in qemu: One
is just giving a QDict, and the other are QemuOpts. QDicts are
just the raw user input and the user can only input strings, so
everything is just a string. As far as I know, typing everything
correctly is done by converting the QDict to a QemuOpts object
(as you can see in generally every block driver which supports
some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast
to the root BDS, which are parsed in blockdev_init()), so you
now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to
bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the
ongoing conversions are done.
I can try and create some barebone which your patches can then be
based on. I probably don't have the knowledge either, but I'm daring
enough to do it anyway. ;-)
Actually I have some patches somewhere [1] that introduce a QemuOpts for
bdrv_open_common(). I intended to use that for cache modes, but as I
explained in our KVM Forum presentation, it's not quite as easy as I
thought it would be and so the patch series isn't ready yet.
Anyway, having the QemuOpts there for driver-independent options is
probably the way to go. Feel free to remove the caching from my
patch and keep only the node-name part. Then it can be a preparatory
patch for your series where you simply add a new option to the list.
Kevin
[1]
http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
On 20.10.2014 15:31, Kevin Wolf wrote:
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I
don't have access to the commandline options in
bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;,
it's there for me (directly after qdict_del(options,
node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open
disk image image.qcow2: Block format 'qcow2' used by device
'virtio0' doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem
is that there are (at least) two parameter types in qemu: One
is just giving a QDict, and the other are QemuOpts. QDicts are
just the raw user input and the user can only input strings, so
everything is just a string. As far as I know, typing everything
correctly is done by converting the QDict to a QemuOpts object
(as you can see in generally every block driver which supports
some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast
to the root BDS, which are parsed in blockdev_init()), so you
now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to
bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the
ongoing conversions are done.
I can try and create some barebone which your patches can then be
based on. I probably don't have the knowledge either, but I'm daring
enough to do it anyway. ;-)
Actually I have some patches somewhere [1] that introduce a QemuOpts for
bdrv_open_common(). I intended to use that for cache modes, but as I
explained in our KVM Forum presentation, it's not quite as easy as I
thought it would be and so the patch series isn't ready yet.
Anyway, having the QemuOpts there for driver-independent options is
probably the way to go. Feel free to remove the caching from my
patch and keep only the node-name part. Then it can be a preparatory
patch for your series where you simply add a new option to the list.
Kevin
[1]
http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
Thank you.
Would it be legit to recycle qemu_common_drive_opts from blockdev.c for this?
No, I don't think so. That one should in theory be only for BlockBackend
options. For the short term, it still mixes BB and BDS options, but BDS
options should be moved out step by step. In any case, it is only used
for the top level.
Any option that is parsed with qemu_opts_absorb_qdict() in
bdrv_open_common() must also be handled there. If you don't ensure that
and extract all the options that blockdev_init() knows without actually
handling them, it can happen that invalid options are silently ignored
(e.g. backing.werror should error out, but would be accepted).
And please coordinate with Max, if both of you write a patch, that's
wasted time.
Kevin
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 15:31, Kevin Wolf wrote:
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I
don't have access to the commandline options in
bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;,
it's there for me (directly after qdict_del(options,
node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open
disk image image.qcow2: Block format 'qcow2' used by device
'virtio0' doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem
is that there are (at least) two parameter types in qemu: One
is just giving a QDict, and the other are QemuOpts. QDicts are
just the raw user input and the user can only input strings, so
everything is just a string. As far as I know, typing everything
correctly is done by converting the QDict to a QemuOpts object
(as you can see in generally every block driver which supports
some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast
to the root BDS, which are parsed in blockdev_init()), so you
now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to
bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the
ongoing conversions are done.
I can try and create some barebone which your patches can then be
based on. I probably don't have the knowledge either, but I'm daring
enough to do it anyway. ;-)
Actually I have some patches somewhere [1] that introduce a QemuOpts for
bdrv_open_common(). I intended to use that for cache modes, but as I
explained in our KVM Forum presentation, it's not quite as easy as I
thought it would be and so the patch series isn't ready yet.
Anyway, having the QemuOpts there for driver-independent options is
probably the way to go. Feel free to remove the caching from my
patch and keep only the node-name part. Then it can be a preparatory
patch for your series where you simply add a new option to the list.
Kevin
[1]
http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
Thank you.
Would it be legit to recycle qemu_common_drive_opts from blockdev.c for this?
Peter
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 15:55, Kevin Wolf wrote:
Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
On 20.10.2014 15:31, Kevin Wolf wrote:
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I
don't have access to the commandline options in
bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;,
it's there for me (directly after qdict_del(options,
node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open
disk image image.qcow2: Block format 'qcow2' used by device
'virtio0' doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem
is that there are (at least) two parameter types in qemu: One
is just giving a QDict, and the other are QemuOpts. QDicts are
just the raw user input and the user can only input strings, so
everything is just a string. As far as I know, typing everything
correctly is done by converting the QDict to a QemuOpts object
(as you can see in generally every block driver which supports
some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast
to the root BDS, which are parsed in blockdev_init()), so you
now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to
bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the
ongoing conversions are done.
I can try and create some barebone which your patches can then be
based on. I probably don't have the knowledge either, but I'm daring
enough to do it anyway. ;-)
Actually I have some patches somewhere [1] that introduce a QemuOpts for
bdrv_open_common(). I intended to use that for cache modes, but as I
explained in our KVM Forum presentation, it's not quite as easy as I
thought it would be and so the patch series isn't ready yet.
Anyway, having the QemuOpts there for driver-independent options is
probably the way to go. Feel free to remove the caching from my
patch and keep only the node-name part. Then it can be a preparatory
patch for your series where you simply add a new option to the list.
Kevin
[1]
http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
Thank you.
Would it be legit to recycle qemu_common_drive_opts from blockdev.c for this?
No, I don't think so. That one should in theory be only for BlockBackend
options. For the short term, it still mixes BB and BDS options, but BDS
options should be moved out step by step. In any case, it is only used
for the top level.
Any option that is parsed with qemu_opts_absorb_qdict() in
bdrv_open_common() must also be handled there. If you don't ensure that
and extract all the options that blockdev_init() knows without actually
handling them, it can happen that invalid options are silently ignored
(e.g. backing.werror should error out, but would be accepted).
And please coordinate with Max, if both of you write a patch, that's
wasted time.
Max, if you don't have started I would use Kevins patch as basis?
Peter
Re: [Qemu-devel] [PATCH] block: add a knob to disable multiwrite_merge
On 20.10.2014 at 15:59, Peter Lieven wrote:
On 20.10.2014 15:55, Kevin Wolf wrote:
Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
On 20.10.2014 15:31, Kevin Wolf wrote:
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
On 20.10.2014 at 15:19, Peter Lieven wrote:
On 20.10.2014 15:15, Max Reitz wrote:
On 20.10.2014 at 14:48, Peter Lieven wrote:
On 20.10.2014 14:19, Max Reitz wrote:
On 2014-10-20 at 14:16, Peter Lieven wrote:
On 20.10.2014 13:51, Max Reitz wrote:
On 2014-10-20 at 12:03, Peter Lieven wrote:
[...]
Can you further help here. I think my problem was that I
don't have access to the commandline options in
bdrv_open?!
You do. It's the options QDict. :-)
Maybe I just don't get it.
If I specify
qemu -drive if=virtio,file=image.qcow2,write-merging=off
and check with
qdict_get_try_bool(options, write-merging, true);
in bdrv_open() directly before bdrv_swap I always get true.
Hm, judging from fprintf(stderr, %s\n,
qstring_get_str(qobject_to_json_pretty(QOBJECT(options;,
it's there for me (directly after qdict_del(options,
node-name). The output is:
Qemu wrote:
{
filename: image.qcow2
}
{
write-merging: off
}
qemu-system-x86_64: -drive
if=virtio,file=image.qcow2,write-merging=off: could not open
disk image image.qcow2: Block format 'qcow2' used by device
'virtio0' doesn't support the option 'write-merging'
But as you can see, it's a string and not a bool. So the problem
is that there are (at least) two parameter types in qemu: One
is just giving a QDict, and the other are QemuOpts. QDicts are
just the raw user input and the user can only input strings, so
everything is just a string. As far as I know, typing everything
correctly is done by converting the QDict to a QemuOpts object
(as you can see in generally every block driver which supports
some options (e.g. qcow2) and also in blockdev_init(), it's
qemu_opts_absorb_qdict()).
Sooo, right, I forgot that. Currently, there are no non-string
non-block-driver-specific options for mid-tree BDS (in contrast
to the root BDS, which are parsed in blockdev_init()), so you
now have the honorable task of introducing such a QemuOptsList
along with qemu_opts_absorb_qdict() and everything to
bdrv_open_common(). *cough*
I would appreciate if someone with better knowledge of this whole
stuff would start this. Or we postpone this know until all the
ongoing conversions are done.
I can try and create some barebone which your patches can then be
based on. I probably don't have the knowledge either, but I'm daring
enough to do it anyway. ;-)
Actually I have some patches somewhere [1] that introduce a
QemuOpts for
bdrv_open_common(). I intended to use that for cache modes, but as I
explained in our KVM Forum presentation, it's not quite as easy as I
thought it would be and so the patch series isn't ready yet.
Anyway, having the QemuOpts there for driver-independent options is
probably the way to go. Feel free to remove the caching from my
patch and keep only the node-name part. Then it can be a preparatory
patch for your series where you simply add a new option to the list.
Kevin
[1]
http://repo.or.cz/w/qemu/kevin.git/commitdiff/9c22aee04cf0bdf6a3858340bc6ff27d6805254f
Thank you.
Would it be legit to recycle qemu_common_drive_opts from blockdev.c
for this?
No, I don't think so. That one should in theory be only for BlockBackend
options. For the short term, it still mixes BB and BDS options, but BDS
options should be moved out step by step. In any case, it is only used
for the top level.
Any option that is parsed with qemu_opts_absorb_qdict() in
bdrv_open_common() must also be handled there. If you don't ensure that
and extract all the options that blockdev_init() knows without actually
handling them, it can happen that invalid options are silently ignored
(e.g. backing.werror should error out, but would be accepted).
And please coordinate with Max, if both of you write a patch, that's
wasted time.
Max, if you don't have started I would use Kevins patch as basis?
No, I haven't. Feel free to.
Max
