block: add a knob to disable multiwrite_merge

PATCH In review Peter Lieven 3 participants 20 messages Oct 20, 2014 Block layer

Share & tools

Peter Lieven

author

2014-10-20 06:15 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---
 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(-)

block.c +4 -0

index 27533f3..1658a72 100644
@@ -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);

block/qapi.c +1 -0

index 9733ebd..02251dd 100644
@@ -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;

blockdev.c +7 -0

index e595910..13e47b8 100644
@@ -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 */ }
     },

hmp.c +4 -0

index 63d7686..8d6ad0b 100644
@@ -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

index 8d86a6c..39bbde2 100644
@@ -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;

qapi/block-core.json +9 -1

index 8f7089e..4931bd9 100644
@@ -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 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
 ##

@@ -1193,7 +1200,8 @@
             '*rerror': 'BlockdevOnError',
             '*werror': 'BlockdevOnError',
             '*read-only': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*write-merging': 'bool' } }
 
 ##
 # @BlockdevOptionsFile

qemu-options.hx +1 -0

index 22cf3b9..d2f756f 100644
@@ -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

index 1abd619..529a563 100644
@@ -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 multiwrite_merge feature (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

Max Reitz

2014-10-20 08:59 UTC #1 lore raw

Quoted 1 line · Peter Lieven

the block layer silently merges write requests since

s/^t/T/

Quoted 57 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                   |    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(-)

block.c +4 -0

index 27533f3..1658a72 100644
@@ -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);

block/qapi.c +1 -0

index 9733ebd..02251dd 100644
@@ -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;

blockdev.c +1 -0

index e595910..13e47b8 100644
@@ -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

blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409

Quoted 4 lines

blockdev.c +1 -0

@@ -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.

Quoted 115 lines

blockdev.c +5 -0

      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 */ }
      },

hmp.c +4 -0

index 63d7686..8d6ad0b 100644
@@ -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

index 8d86a6c..39bbde2 100644
@@ -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;

qapi/block-core.json +9 -1

index 8f7089e..4931bd9 100644
@@ -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 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
  ##

@@ -1193,7 +1200,8 @@
              '*rerror': 'BlockdevOnError',
              '*werror': 'BlockdevOnError',
              '*read-only': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*write-merging': 'bool' } }
 
  ##
  # @BlockdevOptionsFile

qemu-options.hx +1 -0

index 22cf3b9..d2f756f 100644
@@ -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

index 1abd619..529a563 100644
@@ -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 multiwrite_merge feature (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",

Hm, thanks for reminding me of that file. There are some things which I forgot to update (ImageInfoSpecific at least)...

Max

Peter Lieven

author

2014-10-20 09:16 UTC #2 lore raw

Quoted 64 lines · Max Reitz

Quoted 1 line · Peter Lieven

the block layer silently merges write requests since

s/^t/T/

Quoted 52 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                   |    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(-)

block.c +3 -0

index 27533f3..1658a72 100644
@@ -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

block/qapi.c +1 -0

@@ -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;

blockdev.c +1 -0

index e595910..13e47b8 100644
@@ -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

blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409

Quoted 4 lines

blockdev.c +1 -0

@@ -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);

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 121 lines

Quoted 110 lines

blockdev.c +1 -0

      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)",

blockdev.c +4 -0

+        },{
+            .name = "write-merging",
+            .type = QEMU_OPT_BOOL,
+            .help = "enable write merging (default: true)",
          },
          { /* end of list */ }
      },

hmp.c +3 -0

index 63d7686..8d6ad0b 100644
@@ -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

index 8d86a6c..39bbde2 100644
@@ -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;

qapi/block-core.json +9 -1

index 8f7089e..4931bd9 100644
@@ -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 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
  ##

@@ -1193,7 +1200,8 @@
              '*rerror': 'BlockdevOnError',
              '*werror': 'BlockdevOnError',
              '*read-only': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*write-merging': 'bool' } }
    ##
  # @BlockdevOptionsFile

qemu-options.hx +1 -0

index 22cf3b9..d2f756f 100644
@@ -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

index 1abd619..529a563 100644
@@ -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 multiwrite_merge feature (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",

Hm, thanks for reminding me of that file. There are some things which I forgot to update (ImageInfoSpecific at least)...

Max

Signature · 16 lines

Mit freundlichen Grüßen

Peter Lieven

...........................................................

  KAMP Netzwerkdienste GmbH
  Vestische Str. 89-91 | 46117 Oberhausen
  Tel: +49 (0) 208.89 402-50 | Fax: +49 (0) 208.89 402-40
  [email protected] | http://www.kamp.de

  Geschäftsführer: Heiner Lante | Michael Lante
  Amtsgericht Duisburg | HRB Nr. 12154
  USt-Id-Nr.: DE 120607556

...........................................................

Max Reitz

2014-10-20 09:27 UTC #3 lore raw

Quoted 70 lines · Peter Lieven

Quoted 69 lines · Max Reitz

Quoted 1 line · Peter Lieven

the block layer silently merges write requests since

s/^t/T/

Quoted 56 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                   |    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(-)

block.c +3 -0

index 27533f3..1658a72 100644
@@ -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

block/qapi.c +1 -0

@@ -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;

blockdev.c +1 -0

index e595910..13e47b8 100644
@@ -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

blockdev.c static DriveInfo *blockdev_init(const char · line 410

Quoted 5 lines

blockdev.c +1 -0

@@ -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);

Quoted 3 lines

Quoted 1 line
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.

Quoted 6 lines

Quoted 3 lines
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

Quoted 134 lines

Peter

Quoted 130 lines

Quoted 119 lines

blockdev.c +1 -0

      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)",

blockdev.c +4 -0

+        },{
+            .name = "write-merging",
+            .type = QEMU_OPT_BOOL,
+            .help = "enable write merging (default: true)",
          },
          { /* end of list */ }
      },

hmp.c +3 -0

index 63d7686..8d6ad0b 100644
@@ -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

index 8d86a6c..39bbde2 100644
@@ -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;

qapi/block-core.json +7 -0

index 8f7089e..4931bd9 100644
@@ -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 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

qapi/block-core.json +2 -1

@@ -1193,7 +1200,8 @@
              '*rerror': 'BlockdevOnError',
              '*werror': 'BlockdevOnError',
              '*read-only': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*write-merging': 'bool' } }
    ##
  # @BlockdevOptionsFile

qemu-options.hx +0 -0

index 22cf3b9..d2f756f 100644
@@ -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"

qemu-options.hx +1 -0

+    "       [,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

index 1abd619..529a563 100644
@@ -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 multiwrite_merge feature 
 (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",

Hm, thanks for reminding me of that file. There are some things which I forgot to update (ImageInfoSpecific at least)...

Max

Peter Lieven

author

2014-10-20 10:03 UTC #4 lore raw

Quoted 74 lines · Max Reitz

Quoted 65 lines · Peter Lieven
Quoted 64 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 52 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409
Quoted 4 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 2 lines
Quoted 2 lines

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
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 fixed list of requests just merge if the Nth and N-1 requests are mergable (without sorting).
- think about handling the complete merging in virtio-blk with the above simplifications. currently it is a virtio-blk only feature anyway.
- add read merging

Peter

Max Reitz

2014-10-20 11:51 UTC #5 lore raw

Quoted 85 lines · Peter Lieven

Quoted 84 lines · Max Reitz
Quoted 70 lines · Peter Lieven
Quoted 69 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 56 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char · line 410
Quoted 5 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 6 lines

Quoted 3 lines
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

Quoted 12 lines

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 fixed list of
requests just merge if the Nth and N-1 requests are mergable (without sorting).
- think about handling the complete merging in virtio-blk with the
above simplifications. currently it is a virtio-blk only feature anyway.
- add read merging
Peter

Peter Lieven

author

2014-10-20 11:53 UTC #6 lore raw

Quoted 81 lines · Max Reitz

Quoted 80 lines · Peter Lieven
Quoted 79 lines · Max Reitz
Quoted 65 lines · Peter Lieven
Quoted 64 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 52 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409
Quoted 4 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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 in bdrv_open_common and send a fix for discard_zeroes. I can't remember why I finally put it in blockdev_init...

Peter

Kevin Wolf

2014-10-20 12:57 UTC #7 lore raw

Am 20.10.2014 um 13:53 hat Peter Lieven geschrieben:

Quoted 102 lines · Max Reitz

Quoted 101 lines · Peter Lieven
Quoted 100 lines · Max Reitz
Quoted 98 lines · Peter Lieven
Quoted 70 lines · Max Reitz
Quoted 69 lines · Peter Lieven
Quoted 1 line
the block layer silently merges write requests since
s/^t/T/
Quoted 52 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409
Quoted 4 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
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 in bdrv_open_common and send a fix for discard_zeroes. I can't remember why I finally put it in blockdev_init...

Yes, BDS options (and this is one) should be parsed in bdrv_open() or a function called by it. bdrv_open_common() sounds about right here.

Kevin

Peter Lieven

author

2014-10-20 12:16 UTC #8 lore raw

In reply to Max Reitz

Quoted 81 lines · Max Reitz

Quoted 80 lines · Peter Lieven
Quoted 79 lines · Max Reitz
Quoted 65 lines · Peter Lieven
Quoted 64 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 52 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409
Quoted 4 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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

Max Reitz

2014-10-20 12:19 UTC #9 lore raw

Quoted 92 lines · Peter Lieven

Quoted 91 lines · Max Reitz
Quoted 90 lines · Peter Lieven
Quoted 89 lines · Max Reitz
Quoted 73 lines · Peter Lieven
Quoted 72 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 57 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char · line 412
Quoted 7 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 1 line
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?!

You do. It's the "options" QDict. :-)

Max

Peter Lieven

author

2014-10-20 12:48 UTC #10 lore raw

Quoted 87 lines · Max Reitz

Quoted 86 lines · Peter Lieven
Quoted 85 lines · Max Reitz
Quoted 84 lines · Peter Lieven
Quoted 82 lines · Max Reitz
Quoted 65 lines · Peter Lieven
Quoted 64 lines · Max Reitz
Quoted 1 line · Peter Lieven
the block layer silently merges write requests since
s/^t/T/
Quoted 52 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                   |    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(-)
block.c +3 -0
index 27533f3..1658a72 100644
@@ -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
block/qapi.c +1 -0
@@ -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;
blockdev.c +1 -0
index e595910..13e47b8 100644
@@ -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
blockdev.c static DriveInfo *blockdev_init(const char *file, QDict *bs_opts, · line 409
Quoted 4 lines
blockdev.c +1 -0
@@ -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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
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?!
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.

Peter

Max Reitz

2014-10-20 13:15 UTC #11 lore raw

Quoted 14 lines · Peter Lieven

Quoted 8 lines · Max Reitz
Quoted 5 lines · Peter Lieven
Quoted 2 lines · Max Reitz
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

Quoted 7 lines

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:

Quoted 7 lines · Qemu

{
    "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

Peter Lieven

author

2014-10-20 13:20 UTC #12 lore raw

Quoted 34 lines · Max Reitz

Quoted 14 lines · Peter Lieven
Quoted 8 lines · Max Reitz
Quoted 5 lines · Peter Lieven
Quoted 2 lines · Max Reitz
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
Quoted 7 lines
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:
Quoted 7 lines · Qemu
{
    "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'

Quoted 3 lines

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

Max Reitz

2014-10-20 13:22 UTC #13 lore raw

Quoted 35 lines · Peter Lieven

Quoted 34 lines · Max Reitz
Quoted 14 lines · Peter Lieven
Quoted 8 lines · Max Reitz
Quoted 5 lines · Peter Lieven
Quoted 2 lines · Max Reitz
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
Quoted 7 lines
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:
Quoted 7 lines · Qemu
{
    "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'

Quoted 5 lines

Quoted 3 lines
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

Peter Lieven

author

2014-10-20 13:29 UTC #14 lore raw

Quoted 36 lines · Max Reitz

Quoted 35 lines · Peter Lieven
Quoted 34 lines · Max Reitz
Quoted 14 lines · Peter Lieven
Quoted 8 lines · Max Reitz
Quoted 5 lines · Peter Lieven
Quoted 2 lines · Max Reitz
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
Quoted 7 lines
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:
Quoted 7 lines · Qemu
{
    "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'

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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

Kevin Wolf

2014-10-20 13:31 UTC #15 lore raw

In reply to Max Reitz

Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:

Quoted 61 lines · Peter Lieven

Quoted 60 lines · Max Reitz
Quoted 58 lines · Peter Lieven
Quoted 16 lines · Max Reitz
Quoted 10 lines · Peter Lieven
Quoted 7 lines · Max Reitz
Quoted 2 lines · Peter Lieven
[...]
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
Quoted 7 lines
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:
Quoted 10 lines · Qemu
{
   "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*

Quoted 7 lines

Quoted 3 lines
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

Peter Lieven

author

2014-10-20 13:47 UTC #16 lore raw

Quoted 67 lines · Kevin Wolf

Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
Quoted 55 lines · Peter Lieven
Quoted 54 lines · Max Reitz
Quoted 53 lines · Peter Lieven
Quoted 13 lines · Max Reitz
Quoted 8 lines · Peter Lieven
Quoted 6 lines · Max Reitz
Quoted 2 lines · Peter Lieven
[...]
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
Quoted 7 lines
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:
Quoted 10 lines · Qemu
{
    "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*
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

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

Kevin Wolf

2014-10-20 13:56 UTC #17 lore raw

Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:

Quoted 73 lines · Kevin Wolf

Quoted 72 lines
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
Quoted 55 lines · Peter Lieven
Quoted 54 lines · Max Reitz
Quoted 53 lines · Peter Lieven
Quoted 13 lines · Max Reitz
Quoted 8 lines · Peter Lieven
Quoted 6 lines · Max Reitz
Quoted 2 lines · Peter Lieven
[...]
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
Quoted 7 lines
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:
Quoted 10 lines · Qemu
{
   "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*
Quoted 6 lines
Quoted 3 lines
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.

Quoted 7 lines

Quoted 3 lines
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

Peter Lieven

author

2014-10-20 13:59 UTC #18 lore raw

Quoted 85 lines · Kevin Wolf

Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
Quoted 73 lines · Kevin Wolf
Quoted 72 lines
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
Quoted 55 lines · Peter Lieven
Quoted 54 lines · Max Reitz
Quoted 53 lines · Peter Lieven
Quoted 13 lines · Max Reitz
Quoted 8 lines · Peter Lieven
Quoted 6 lines · Max Reitz
Quoted 2 lines · Peter Lieven
[...]
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
Quoted 7 lines
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:
Quoted 10 lines · Qemu
{
    "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*
Quoted 6 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 3 lines
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.

Quoted 8 lines

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

Max Reitz

2014-10-20 14:00 UTC #19 lore raw

Quoted 92 lines · Peter Lieven

Quoted 91 lines · Kevin Wolf
Am 20.10.2014 um 15:47 hat Peter Lieven geschrieben:
Quoted 73 lines · Kevin Wolf
Quoted 72 lines
Am 20.10.2014 um 15:22 hat Max Reitz geschrieben:
Quoted 55 lines · Peter Lieven
Quoted 54 lines · Max Reitz
Quoted 53 lines · Peter Lieven
Quoted 13 lines · Max Reitz
Quoted 8 lines · Peter Lieven
Quoted 6 lines · Max Reitz
Quoted 2 lines · Peter Lieven
[...]
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
Quoted 7 lines
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:
Quoted 10 lines · Qemu
{
    "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*
Quoted 6 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 3 lines
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).

Quoted 4 lines

Quoted 2 lines
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