[Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Peter Lieven]

This adds some preparing patches for upcoming multiwrite modifications.
I will leave the dangerous patches for after 2.2 release.

Peter Lieven (4):
  block: add accounting for merged requests
  block: introduce bdrv_runtime_opts
  block: add a knob to disable multiwrite_merge
  hw/virtio-blk: add a constant for max number of merged requests

 block.c                    |   50 ++++++++++++++++++++++++++++++++++++++------
 block/accounting.c         |    8 ++++++-
 block/qapi.c               |    3 +++
 hmp.c                      |   10 ++++++++-
 hw/block/virtio-blk.c      |    4 +++-
 include/block/accounting.h |    3 +++
 include/block/block_int.h  |    1 +
 qapi/block-core.json       |   19 +++++++++++++++--
 qemu-options.hx            |    1 +
 qmp-commands.hx            |    2 ++
 10 files changed, 90 insertions(+), 11 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/4] block: add accounting for merged requests

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c                    |    2 ++
 block/accounting.c         |    8 +++++++-
 block/qapi.c               |    2 ++
 hmp.c                      |    6 +++++-
 include/block/accounting.h |    3 +++
 qapi/block-core.json       |    9 ++++++++-
 6 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 773e87e..ba5281d 100644
--- a/block.c
+++ b/block.c
@@ -4466,6 +4466,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
         }
     }
 
+    block_merge_done(&bs->stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1);
+
     return outidx + 1;
 }
 
diff --git a/block/accounting.c b/block/accounting.c
index edbb1cc..8f6ee81 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -44,7 +44,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
     stats->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
 }
 
-
 void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
                                unsigned int nb_sectors)
 {
@@ -52,3 +51,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
         stats->wr_highest_sector = sector_num + nb_sectors - 1;
     }
 }
+
+void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
+                      int num_requests)
+{
+    assert(type < BLOCK_MAX_IOTYPE);
+    stats->merged[type] += num_requests;
+}
diff --git a/block/qapi.c b/block/qapi.c
index 1301144..a0f26e9 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -338,6 +338,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
     s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
     s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ];
     s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
+    s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
+    s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
     s->stats->wr_highest_offset =
         bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE;
     s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
diff --git a/hmp.c b/hmp.c
index 63d7686..baaa9e5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
                        " wr_total_time_ns=%" PRId64
                        " rd_total_time_ns=%" PRId64
                        " flush_total_time_ns=%" PRId64
+                       " rd_merged=%" PRId64
+                       " wr_merged=%" PRId64
                        "\n",
                        stats->value->stats->rd_bytes,
                        stats->value->stats->wr_bytes,
@@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
                        stats->value->stats->flush_operations,
                        stats->value->stats->wr_total_time_ns,
                        stats->value->stats->rd_total_time_ns,
-                       stats->value->stats->flush_total_time_ns);
+                       stats->value->stats->flush_total_time_ns,
+                       stats->value->stats->rd_merged,
+                       stats->value->stats->wr_merged);
     }
 
     qapi_free_BlockStatsList(stats_list);
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 50b42b3..07394f7 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -39,6 +39,7 @@ typedef struct BlockAcctStats {
     uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
     uint64_t nr_ops[BLOCK_MAX_IOTYPE];
     uint64_t total_time_ns[BLOCK_MAX_IOTYPE];
+    uint64_t merged[BLOCK_MAX_IOTYPE];
     uint64_t wr_highest_sector;
 } BlockAcctStats;
 
@@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
 void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie);
 void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
                                unsigned int nb_sectors);
+void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
+                      int num_requests);
 
 #endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8f7089e..952d50e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -392,13 +392,20 @@
 #                     growable sparse files (like qcow2) that are used on top
 #                     of a physical device.
 #
+# @rd_merged: Reserved (Since 2.3)
+#
+# @wr_merged: Number of requests that have been merged into another request
+#             while performing a multiwrite_merge. (Since 2.3)
+#
+#
 # Since: 0.14.0
 ##
 { 'type': 'BlockDeviceStats',
   'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
            'wr_operations': 'int', 'flush_operations': 'int',
            'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } }
+           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
+           'rd_merged': 'int', 'wr_merged': 'int' } }
 
 ##
 # @BlockStats:
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/4] block: add accounting for merged requests

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block.c                    |    2 ++
>   block/accounting.c         |    8 +++++++-
>   block/qapi.c               |    2 ++
>   hmp.c                      |    6 +++++-
>   include/block/accounting.h |    3 +++
>   qapi/block-core.json       |    9 ++++++++-
>   6 files changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/block.c b/block.c
> index 773e87e..ba5281d 100644
> --- a/block.c
> +++ b/block.c
> @@ -4466,6 +4466,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>           }
>       }
>   
> +    block_merge_done(&bs->stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1);
> +
>       return outidx + 1;
>   }
>   
> diff --git a/block/accounting.c b/block/accounting.c
> index edbb1cc..8f6ee81 100644
> --- a/block/accounting.c
> +++ b/block/accounting.c
> @@ -44,7 +44,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie)
>       stats->total_time_ns[cookie->type] += get_clock() - cookie->start_time_ns;
>   }
>   
> -
>   void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>                                  unsigned int nb_sectors)
>   {

Was this hunk intended?

> @@ -52,3 +51,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>           stats->wr_highest_sector = sector_num + nb_sectors - 1;
>       }
>   }
> +
> +void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
> +                      int num_requests)
> +{
> +    assert(type < BLOCK_MAX_IOTYPE);
> +    stats->merged[type] += num_requests;
> +}
> diff --git a/block/qapi.c b/block/qapi.c
> index 1301144..a0f26e9 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -338,6 +338,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
>       s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
>       s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ];
>       s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
> +    s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
> +    s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
>       s->stats->wr_highest_offset =
>           bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE;
>       s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
> diff --git a/hmp.c b/hmp.c
> index 63d7686..baaa9e5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>                          " wr_total_time_ns=%" PRId64
>                          " rd_total_time_ns=%" PRId64
>                          " flush_total_time_ns=%" PRId64
> +                       " rd_merged=%" PRId64
> +                       " wr_merged=%" PRId64
>                          "\n",
>                          stats->value->stats->rd_bytes,
>                          stats->value->stats->wr_bytes,
> @@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
>                          stats->value->stats->flush_operations,
>                          stats->value->stats->wr_total_time_ns,
>                          stats->value->stats->rd_total_time_ns,
> -                       stats->value->stats->flush_total_time_ns);
> +                       stats->value->stats->flush_total_time_ns,
> +                       stats->value->stats->rd_merged,
> +                       stats->value->stats->wr_merged);
>       }
>   
>       qapi_free_BlockStatsList(stats_list);
> diff --git a/include/block/accounting.h b/include/block/accounting.h
> index 50b42b3..07394f7 100644
> --- a/include/block/accounting.h
> +++ b/include/block/accounting.h
> @@ -39,6 +39,7 @@ typedef struct BlockAcctStats {
>       uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
>       uint64_t nr_ops[BLOCK_MAX_IOTYPE];
>       uint64_t total_time_ns[BLOCK_MAX_IOTYPE];
> +    uint64_t merged[BLOCK_MAX_IOTYPE];
>       uint64_t wr_highest_sector;
>   } BlockAcctStats;
>   
> @@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
>   void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie);
>   void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
>                                  unsigned int nb_sectors);
> +void block_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
> +                      int num_requests);
>   
>   #endif
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 8f7089e..952d50e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -392,13 +392,20 @@
>   #                     growable sparse files (like qcow2) that are used on top
>   #                     of a physical device.
>   #
> +# @rd_merged: Reserved (Since 2.3)
> +#
> +# @wr_merged: Number of requests that have been merged into another request
> +#             while performing a multiwrite_merge. (Since 2.3)
> +#
> +#

2.2 or 2.3? ;-)

>   # Since: 0.14.0
>   ##
>   { 'type': 'BlockDeviceStats',
>     'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
>              'wr_operations': 'int', 'flush_operations': 'int',
>              'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
> -           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } }
> +           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
> +           'rd_merged': 'int', 'wr_merged': 'int' } }
>   
>   ##
>   # @BlockStats:

With the hunk removing the empty line removed (I know that line should 
not be there, but there's no point in removing it in this patch) and 
with either 2.2 or 2.3 in block-core.json:

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH 2/4] block: introduce bdrv_runtime_opts

[Peter Lieven]

This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
The list will absorb all options that belong to the BDS (and not the
BlockBackend) and will be parsed and handled in bdrv_open_common.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block.c |   39 +++++++++++++++++++++++++++++++++------
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index ba5281d..3b4a59a 100644
--- a/block.c
+++ b/block.c
@@ -27,6 +27,7 @@
 #include "block/block_int.h"
 #include "block/blockjob.h"
 #include "qemu/module.h"
+#include "qapi/qmp/qbool.h"
 #include "qapi/qmp/qjson.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/sysemu.h"
@@ -875,6 +876,19 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
     QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
 }
 
+static QemuOptsList bdrv_runtime_opts = {
+    .name = "bdrv_common",
+    .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
+    .desc = {
+        {
+            .name = "node-name",
+            .type = QEMU_OPT_STRING,
+            .help = "Node name of the block device node",
+        },
+        { /* end of list */ }
+    },
+};
+
 /*
  * Common part for opening disk images and files
  *
@@ -886,6 +900,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     int ret, open_flags;
     const char *filename;
     const char *node_name = NULL;
+    QemuOpts *opts;
     Error *local_err = NULL;
 
     assert(drv != NULL);
@@ -906,19 +921,28 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
 
-    node_name = qdict_get_try_str(options, "node-name");
+    opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail_opts;
+    }
+
+    node_name = qemu_opt_get(opts, "node-name");
     bdrv_assign_node_name(bs, node_name, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail_opts;
     }
-    qdict_del(options, "node-name");
 
     /* bdrv_open() with directly using a protocol as drv. This layer is already
      * opened, so assign it to bs (while file becomes a closed BlockDriverState)
      * and return immediately. */
     if (file != NULL && drv->bdrv_file_open) {
         bdrv_swap(file, bs);
+        qemu_opts_del(opts);
         return 0;
     }
 
@@ -936,7 +960,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
                         ? "Driver '%s' can only be used for read-only devices"
                         : "Driver '%s' is not whitelisted",
                    drv->format_name);
-        return -ENOTSUP;
+        ret = -ENOTSUP;
+        goto fail_opts;
     }
 
     assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
@@ -945,7 +970,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
             bdrv_enable_copy_on_read(bs);
         } else {
             error_setg(errp, "Can't use copy-on-read on read-only device");
-            return -EINVAL;
+            ret = -EINVAL;
+            goto fail_opts;
         }
     }
 
@@ -958,7 +984,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
 
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
-
     bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
 
     /* Open the image, either directly or using a protocol */
@@ -1010,6 +1035,8 @@ free_and_fail:
     g_free(bs->opaque);
     bs->opaque = NULL;
     bs->drv = NULL;
+fail_opts:
+    qemu_opts_del(opts);
     return ret;
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/4] block: introduce bdrv_runtime_opts

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> This patch (orginally by Kevin) adds a bdrv_runtime_opts QemuOptsList.
> The list will absorb all options that belong to the BDS (and not the
> BlockBackend) and will be parsed and handled in bdrv_open_common.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block.c |   39 +++++++++++++++++++++++++++++++++------
>   1 file changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/block.c b/block.c
> index ba5281d..3b4a59a 100644
> --- a/block.c
> +++ b/block.c
> @@ -27,6 +27,7 @@
>   #include "block/block_int.h"
>   #include "block/blockjob.h"
>   #include "qemu/module.h"
> +#include "qapi/qmp/qbool.h"
>   #include "qapi/qmp/qjson.h"
>   #include "sysemu/block-backend.h"
>   #include "sysemu/sysemu.h"
> @@ -875,6 +876,19 @@ static void bdrv_assign_node_name(BlockDriverState *bs,
>       QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs, node_list);
>   }
>   
> +static QemuOptsList bdrv_runtime_opts = {
> +    .name = "bdrv_common",
> +    .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
> +    .desc = {
> +        {
> +            .name = "node-name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Node name of the block device node",
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
>   /*
>    * Common part for opening disk images and files
>    *
> @@ -886,6 +900,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>       int ret, open_flags;
>       const char *filename;
>       const char *node_name = NULL;
> +    QemuOpts *opts;
>       Error *local_err = NULL;
>   
>       assert(drv != NULL);
> @@ -906,19 +921,28 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>   
>       trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
>   
> -    node_name = qdict_get_try_str(options, "node-name");
> +    opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
> +    qemu_opts_absorb_qdict(opts, options, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        ret = -EINVAL;
> +        goto fail_opts;
> +    }
> +
> +    node_name = qemu_opt_get(opts, "node-name");
>       bdrv_assign_node_name(bs, node_name, &local_err);
>       if (local_err) {
>           error_propagate(errp, local_err);
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto fail_opts;
>       }
> -    qdict_del(options, "node-name");
>   
>       /* bdrv_open() with directly using a protocol as drv. This layer is already
>        * opened, so assign it to bs (while file becomes a closed BlockDriverState)
>        * and return immediately. */
>       if (file != NULL && drv->bdrv_file_open) {
>           bdrv_swap(file, bs);
> +        qemu_opts_del(opts);
>           return 0;
>       }
>   
> @@ -936,7 +960,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>                           ? "Driver '%s' can only be used for read-only devices"
>                           : "Driver '%s' is not whitelisted",
>                      drv->format_name);
> -        return -ENOTSUP;
> +        ret = -ENOTSUP;
> +        goto fail_opts;
>       }
>   
>       assert(bs->copy_on_read == 0); /* bdrv_new() and bdrv_close() make it so */
> @@ -945,7 +970,8 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>               bdrv_enable_copy_on_read(bs);
>           } else {
>               error_setg(errp, "Can't use copy-on-read on read-only device");
> -            return -EINVAL;
> +            ret = -EINVAL;
> +            goto fail_opts;
>           }
>       }
>   
> @@ -958,7 +984,6 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>   
>       bs->drv = drv;
>       bs->opaque = g_malloc0(drv->instance_size);
> -
>       bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
>   
>       /* Open the image, either directly or using a protocol */

I know it's not even your patch originally, but I still don't like 
spurious empty line removals, whoever might be responsible for them.

> @@ -1010,6 +1035,8 @@ free_and_fail:
>       g_free(bs->opaque);
>       bs->opaque = NULL;
>       bs->drv = NULL;
> +fail_opts:
> +    qemu_opts_del(opts);
>       return ret;
>   }
>   

With that empty line removal hunk removed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH 3/4] block: add a knob to disable multiwrite_merge

[Peter Lieven]

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 <pl@kamp.de>
---
 block.c                   |    9 +++++++++
 block/qapi.c              |    1 +
 hmp.c                     |    4 ++++
 include/block/block_int.h |    1 +
 qapi/block-core.json      |   10 +++++++++-
 qemu-options.hx           |    1 +
 qmp-commands.hx           |    2 ++
 7 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3b4a59a..ffb2b47 100644
--- a/block.c
+++ b/block.c
@@ -884,6 +884,10 @@ static QemuOptsList bdrv_runtime_opts = {
             .name = "node-name",
             .type = QEMU_OPT_STRING,
             .help = "Node name of the block device node",
+        },{
+            .name = "write-merging",
+            .type = QEMU_OPT_BOOL,
+            .help = "enable write merging (default: true)",
         },
         { /* end of list */ }
     },
@@ -985,6 +989,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     bs->drv = drv;
     bs->opaque = g_malloc0(drv->instance_size);
     bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
+    bs->write_merging = qemu_opt_get_bool(opts, "write-merging", true);
 
     /* Open the image, either directly or using a protocol */
     if (drv->bdrv_file_open) {
@@ -4439,6 +4444,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
 {
     int i, outidx;
 
+    if (!bs->write_merging) {
+        return num_reqs;
+    }
+
     // Sort requests by start sector
     qsort(reqs, num_reqs, sizeof(*reqs), &multiwrite_req_compare);
 
diff --git a/block/qapi.c b/block/qapi.c
index a0f26e9..5f09967 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -59,6 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
 
     info->backing_file_depth = bdrv_get_backing_file_depth(bs);
     info->detect_zeroes = bs->detect_zeroes;
+    info->write_merging = bs->write_merging;
 
     if (bs->io_limits_enabled) {
         ThrottleConfig cfg;
diff --git a/hmp.c b/hmp.c
index baaa9e5..5762444 100644
--- a/hmp.c
+++ b/hmp.c
@@ -348,6 +348,10 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
                            BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]);
         }
 
+        if (!info->value->inserted->write_merging) {
+            monitor_printf(mon, "    Write Merging:    off\n");
+        }
+
         if (info->value->inserted->bps
             || info->value->inserted->bps_rd
             || info->value->inserted->bps_wr
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8898c6c..e3d382f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -402,6 +402,7 @@ struct BlockDriverState {
 
     QDict *options;
     BlockdevDetectZeroesOptions detect_zeroes;
+    bool write_merging;
 
     /* The error object in use for blocking operations on backing_hd */
     Error *backing_blocker;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 952d50e..9ac9085 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -214,6 +214,8 @@
 #
 # @detect_zeroes: detect and optimize zero writes (Since 2.1)
 #
+# @write_merging: true if write merging is enabled (Since 2.2)
+#
 # @bps: total throughput limit in bytes per second is specified
 #
 # @bps_rd: read throughput limit in bytes per second is specified
@@ -250,6 +252,7 @@
             '*backing_file': 'str', 'backing_file_depth': 'int',
             'encrypted': 'bool', 'encryption_key_missing': 'bool',
             'detect_zeroes': 'BlockdevDetectZeroesOptions',
+            'write_merging': 'bool',
             'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
             'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
             'image': 'ImageInfo',
@@ -1187,6 +1190,10 @@
 #                 (default: false)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
 #                 (default: off)
+# @write-merging: #optional enable the merging of write requests
+#                 also known as multiwrite_merge (Since 2.2)
+#                 (default: true, but this might change in the future
+#                  depending on format/protocol/features used)
 #
 # Since: 1.7
 ##
@@ -1200,7 +1207,8 @@
             '*rerror': 'BlockdevOnError',
             '*werror': 'BlockdevOnError',
             '*read-only': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*write-merging': 'bool' } }
 
 ##
 # @BlockdevOptionsFile
diff --git a/qemu-options.hx b/qemu-options.hx
index 22cf3b9..d2f756f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -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"
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 1abd619..2c20207 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2104,6 +2104,7 @@ Each json-object contain the following:
          - "iops_size": I/O size when limiting by iops (json-int)
          - "detect_zeroes": detect and optimize zero writing (json-string)
              - Possible values: "off", "on", "unmap"
+         - "write_merging": enable merging of write requests (json-bool)
          - "image": the detail of the image, it is a json-object containing
             the following:
              - "filename": image file name (json-string)
@@ -2181,6 +2182,7 @@ Example:
                "iops_wr_max": 0,
                "iops_size": 0,
                "detect_zeroes": "on",
+               "write_merging": "true",
                "image":{
                   "filename":"disks/test.qcow2",
                   "format":"qcow2",
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 3/4] block: add a knob to disable multiwrite_merge

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> the block layer silently merges write requests since

It's still s/^t/T/ ;-)

> commit 40b4f539. This patch adds a knob to disable
> this feature as there has been some discussion lately
> if multiwrite is a good idea at all and as it falsifies
> benchmarks.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   block.c                   |    9 +++++++++
>   block/qapi.c              |    1 +
>   hmp.c                     |    4 ++++
>   include/block/block_int.h |    1 +
>   qapi/block-core.json      |   10 +++++++++-
>   qemu-options.hx           |    1 +
>   qmp-commands.hx           |    2 ++
>   7 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/block.c b/block.c
> index 3b4a59a..ffb2b47 100644
> --- a/block.c
> +++ b/block.c
> @@ -884,6 +884,10 @@ static QemuOptsList bdrv_runtime_opts = {
>               .name = "node-name",
>               .type = QEMU_OPT_STRING,
>               .help = "Node name of the block device node",
> +        },{
> +            .name = "write-merging",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "enable write merging (default: true)",
>           },
>           { /* end of list */ }
>       },
> @@ -985,6 +989,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
>       bs->drv = drv;
>       bs->opaque = g_malloc0(drv->instance_size);
>       bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
> +    bs->write_merging = qemu_opt_get_bool(opts, "write-merging", true);
>   
>       /* Open the image, either directly or using a protocol */
>       if (drv->bdrv_file_open) {
> @@ -4439,6 +4444,10 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
>   {
>       int i, outidx;
>   
> +    if (!bs->write_merging) {
> +        return num_reqs;
> +    }
> +
>       // Sort requests by start sector
>       qsort(reqs, num_reqs, sizeof(*reqs), &multiwrite_req_compare);
>   
> diff --git a/block/qapi.c b/block/qapi.c
> index a0f26e9..5f09967 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -59,6 +59,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs)
>   
>       info->backing_file_depth = bdrv_get_backing_file_depth(bs);
>       info->detect_zeroes = bs->detect_zeroes;
> +    info->write_merging = bs->write_merging;
>   
>       if (bs->io_limits_enabled) {
>           ThrottleConfig cfg;
> diff --git a/hmp.c b/hmp.c
> index baaa9e5..5762444 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -348,6 +348,10 @@ void hmp_info_block(Monitor *mon, const QDict *qdict)
>                              BlockdevDetectZeroesOptions_lookup[info->value->inserted->detect_zeroes]);
>           }
>   
> +        if (!info->value->inserted->write_merging) {
> +            monitor_printf(mon, "    Write Merging:    off\n");
> +        }
> +

Currently, write_merging == true is the default; but as you yourself 
describe in block-core.json, this may change. I think always printing 
ths field's value would be fine, too.

>           if (info->value->inserted->bps
>               || info->value->inserted->bps_rd
>               || info->value->inserted->bps_wr
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 8898c6c..e3d382f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -402,6 +402,7 @@ struct BlockDriverState {
>   
>       QDict *options;
>       BlockdevDetectZeroesOptions detect_zeroes;
> +    bool write_merging;
>   
>       /* The error object in use for blocking operations on backing_hd */
>       Error *backing_blocker;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 952d50e..9ac9085 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -214,6 +214,8 @@
>   #
>   # @detect_zeroes: detect and optimize zero writes (Since 2.1)
>   #
> +# @write_merging: true if write merging is enabled (Since 2.2)
> +#

If this is 2.2, patch 1 must say 2.2 as well.

>   # @bps: total throughput limit in bytes per second is specified
>   #
>   # @bps_rd: read throughput limit in bytes per second is specified
> @@ -250,6 +252,7 @@
>               '*backing_file': 'str', 'backing_file_depth': 'int',
>               'encrypted': 'bool', 'encryption_key_missing': 'bool',
>               'detect_zeroes': 'BlockdevDetectZeroesOptions',
> +            'write_merging': 'bool',
>               'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>               'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int',
>               'image': 'ImageInfo',
> @@ -1187,6 +1190,10 @@
>   #                 (default: false)
>   # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
>   #                 (default: off)
> +# @write-merging: #optional enable the merging of write requests
> +#                 also known as multiwrite_merge (Since 2.2)
> +#                 (default: true, but this might change in the future
> +#                  depending on format/protocol/features used)

I'd ignore the parenthesis for alignment (which means remove one space 
in front of "depending"), but I don't object either.

>   #
>   # Since: 1.7
>   ##
> @@ -1200,7 +1207,8 @@
>               '*rerror': 'BlockdevOnError',
>               '*werror': 'BlockdevOnError',
>               '*read-only': 'bool',
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' } }
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*write-merging': 'bool' } }
>   
>   ##
>   # @BlockdevOptionsFile
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 22cf3b9..d2f756f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -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"
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 1abd619..2c20207 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2104,6 +2104,7 @@ Each json-object contain the following:
>            - "iops_size": I/O size when limiting by iops (json-int)
>            - "detect_zeroes": detect and optimize zero writing (json-string)
>                - Possible values: "off", "on", "unmap"
> +         - "write_merging": enable merging of write requests (json-bool)
>            - "image": the detail of the image, it is a json-object containing
>               the following:
>                - "filename": image file name (json-string)
> @@ -2181,6 +2182,7 @@ Example:
>                  "iops_wr_max": 0,
>                  "iops_size": 0,
>                  "detect_zeroes": "on",
> +               "write_merging": "true",
>                  "image":{
>                     "filename":"disks/test.qcow2",
>                     "format":"qcow2",

Reviewed-by: Max Reitz <mreitz@redhat.com>

[Qemu-devel] [PATCH 4/4] hw/virtio-blk: add a constant for max number of merged requests

[Peter Lieven]

As it was not obvious (at least for me) where the 32 comes from
add a constant for it.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 hw/block/virtio-blk.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6051027..23bc948 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -308,6 +308,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     return true;
 }
 
+#define MAX_MERGED_REQS 32
+
 static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     BlockRequest *blkreq;
@@ -326,7 +328,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
     block_acct_start(blk_get_stats(req->dev->blk), &req->acct, req->qiov.size,
                      BLOCK_ACCT_WRITE);
 
-    if (mrb->num_writes == 32) {
+    if (mrb->num_writes == MAX_MERGED_REQS) {
         virtio_submit_multiwrite(req->dev->blk, mrb);
     }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 4/4] hw/virtio-blk: add a constant for max number of merged requests

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> As it was not obvious (at least for me) where the 32 comes from
> add a constant for it.

This needs some separator ("...comes from; add a..." or something like 
that).

> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>   hw/block/virtio-blk.c |    4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 6051027..23bc948 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -308,6 +308,8 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>       return true;
>   }
>   
> +#define MAX_MERGED_REQS 32
> +
>   static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>   {
>       BlockRequest *blkreq;
> @@ -326,7 +328,7 @@ static void virtio_blk_handle_write(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>       block_acct_start(blk_get_stats(req->dev->blk), &req->acct, req->qiov.size,
>                        BLOCK_ACCT_WRITE);
>   
> -    if (mrb->num_writes == 32) {
> +    if (mrb->num_writes == MAX_MERGED_REQS) {
>           virtio_submit_multiwrite(req->dev->blk, mrb);
>       }
>   

I wouldn't have used the past form, but simply MAX_MERGE_REQS or, more 
complicated, MAX_MERGE_REQ_COUNT. But I'm me, and this mail's "From" 
field says it's not from me.

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Max Reitz]

On 20.10.2014 at 16:35, Peter Lieven wrote:
> This adds some preparing patches for upcoming multiwrite modifications.
> I will leave the dangerous patches for after 2.2 release.
>
> Peter Lieven (4):
>    block: add accounting for merged requests
>    block: introduce bdrv_runtime_opts
>    block: add a knob to disable multiwrite_merge
>    hw/virtio-blk: add a constant for max number of merged requests

In addition, I'd like a test for this (just the parameter would be 
enough). But I don't object to this series without it.

Max

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Peter Lieven]

Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:

> On 20.10.2014 at 16:35, Peter Lieven wrote:
>> This adds some preparing patches for upcoming multiwrite modifications.
>> I will leave the dangerous patches for after 2.2 release.
>> 
>> Peter Lieven (4):
>>   block: add accounting for merged requests
>>   block: introduce bdrv_runtime_opts
>>   block: add a knob to disable multiwrite_merge
>>   hw/virtio-blk: add a constant for max number of merged requests
> 
> In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.

Thanks for your comments. I will respin tomorrow.

What exactly would you like to check in a test?

Peter

> 
> Max

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Max Reitz]

On 2014-10-20 at 22:48, Peter Lieven wrote:
> Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
>
>> On 20.10.2014 at 16:35, Peter Lieven wrote:
>>> This adds some preparing patches for upcoming multiwrite modifications.
>>> I will leave the dangerous patches for after 2.2 release.
>>>
>>> Peter Lieven (4):
>>>    block: add accounting for merged requests
>>>    block: introduce bdrv_runtime_opts
>>>    block: add a knob to disable multiwrite_merge
>>>    hw/virtio-blk: add a constant for max number of merged requests
>> In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
> Thanks for your comments. I will respin tomorrow.
>
> What exactly would you like to check in a test?

Just give the parameter and test the query-block against it. I'd test 
the default, switching it on, off and maybe even specify it for a 
non-root BDS to see whether that works.

Max

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Peter Lieven]

On 21.10.2014 09:06, Max Reitz wrote:
> On 2014-10-20 at 22:48, Peter Lieven wrote:
>> Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
>>
>>> On 20.10.2014 at 16:35, Peter Lieven wrote:
>>>> This adds some preparing patches for upcoming multiwrite modifications.
>>>> I will leave the dangerous patches for after 2.2 release.
>>>>
>>>> Peter Lieven (4):
>>>>    block: add accounting for merged requests
>>>>    block: introduce bdrv_runtime_opts
>>>>    block: add a knob to disable multiwrite_merge
>>>>    hw/virtio-blk: add a constant for max number of merged requests
>>> In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
>> Thanks for your comments. I will respin tomorrow.
>>
>> What exactly would you like to check in a test?
>
> Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.

Understood.

Thanks
Peter

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Peter Lieven]

On 21.10.2014 09:06, Max Reitz wrote:
> On 2014-10-20 at 22:48, Peter Lieven wrote:
>> Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
>>
>>> On 20.10.2014 at 16:35, Peter Lieven wrote:
>>>> This adds some preparing patches for upcoming multiwrite modifications.
>>>> I will leave the dangerous patches for after 2.2 release.
>>>>
>>>> Peter Lieven (4):
>>>>    block: add accounting for merged requests
>>>>    block: introduce bdrv_runtime_opts
>>>>    block: add a knob to disable multiwrite_merge
>>>>    hw/virtio-blk: add a constant for max number of merged requests
>>> In addition, I'd like a test for this (just the parameter would be enough). But I don't object to this series without it.
>> Thanks for your comments. I will respin tomorrow.
>>
>> What exactly would you like to check in a test?
>
> Just give the parameter and test the query-block against it. I'd test the default, switching it on, off and maybe even specify it for a non-root BDS to see whether that works.

It seems it is currently not visible in query-block which paramters are set to e.g. file. If I specifiy -drive file.write_merging=off its accepted but not displayed. Any ideas?

Peter

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Max Reitz]

On 2014-10-21 at 10:01, Peter Lieven wrote:
> On 21.10.2014 09:06, Max Reitz wrote:
>> On 2014-10-20 at 22:48, Peter Lieven wrote:
>>> Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
>>>
>>>> On 20.10.2014 at 16:35, Peter Lieven wrote:
>>>>> This adds some preparing patches for upcoming multiwrite 
>>>>> modifications.
>>>>> I will leave the dangerous patches for after 2.2 release.
>>>>>
>>>>> Peter Lieven (4):
>>>>>    block: add accounting for merged requests
>>>>>    block: introduce bdrv_runtime_opts
>>>>>    block: add a knob to disable multiwrite_merge
>>>>>    hw/virtio-blk: add a constant for max number of merged requests
>>>> In addition, I'd like a test for this (just the parameter would be 
>>>> enough). But I don't object to this series without it.
>>> Thanks for your comments. I will respin tomorrow.
>>>
>>> What exactly would you like to check in a test?
>>
>> Just give the parameter and test the query-block against it. I'd test 
>> the default, switching it on, off and maybe even specify it for a 
>> non-root BDS to see whether that works.
>
> It seems it is currently not visible in query-block which paramters 
> are set to e.g. file. If I specifiy -drive file.write_merging=off its 
> accepted but not displayed. Any ideas?

Hm, I forgot that query-block only gives you the root BDS. Too bad. Then 
we cannot test that. We probably want to have a QMP command to query the 
whole BDS graph some time, but it isn't there yet.

Max

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Kevin Wolf]

Am 21.10.2014 um 11:07 hat Max Reitz geschrieben:
> On 2014-10-21 at 10:01, Peter Lieven wrote:
> >On 21.10.2014 09:06, Max Reitz wrote:
> >>On 2014-10-20 at 22:48, Peter Lieven wrote:
> >>>Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
> >>>
> >>>>On 20.10.2014 at 16:35, Peter Lieven wrote:
> >>>>>This adds some preparing patches for upcoming multiwrite
> >>>>>modifications.
> >>>>>I will leave the dangerous patches for after 2.2 release.
> >>>>>
> >>>>>Peter Lieven (4):
> >>>>>   block: add accounting for merged requests
> >>>>>   block: introduce bdrv_runtime_opts
> >>>>>   block: add a knob to disable multiwrite_merge
> >>>>>   hw/virtio-blk: add a constant for max number of merged requests
> >>>>In addition, I'd like a test for this (just the parameter
> >>>>would be enough). But I don't object to this series without
> >>>>it.
> >>>Thanks for your comments. I will respin tomorrow.
> >>>
> >>>What exactly would you like to check in a test?
> >>
> >>Just give the parameter and test the query-block against it. I'd
> >>test the default, switching it on, off and maybe even specify it
> >>for a non-root BDS to see whether that works.
> >
> >It seems it is currently not visible in query-block which
> >paramters are set to e.g. file. If I specifiy -drive
> >file.write_merging=off its accepted but not displayed. Any ideas?
> 
> Hm, I forgot that query-block only gives you the root BDS. Too bad.
> Then we cannot test that. We probably want to have a QMP command to
> query the whole BDS graph some time, but it isn't there yet.

Does query-named-block-nodes help in this specific case?

Kevin

Re: [Qemu-devel] [PATCH 0/4] multiwrite patches for 2.2

[Max Reitz]

On 2014-10-21 at 11:38, Kevin Wolf wrote:
> Am 21.10.2014 um 11:07 hat Max Reitz geschrieben:
>> On 2014-10-21 at 10:01, Peter Lieven wrote:
>>> On 21.10.2014 09:06, Max Reitz wrote:
>>>> On 2014-10-20 at 22:48, Peter Lieven wrote:
>>>>> Am 20.10.2014 um 17:56 schrieb Max Reitz <mreitz@redhat.com>:
>>>>>
>>>>>> On 20.10.2014 at 16:35, Peter Lieven wrote:
>>>>>>> This adds some preparing patches for upcoming multiwrite
>>>>>>> modifications.
>>>>>>> I will leave the dangerous patches for after 2.2 release.
>>>>>>>
>>>>>>> Peter Lieven (4):
>>>>>>>    block: add accounting for merged requests
>>>>>>>    block: introduce bdrv_runtime_opts
>>>>>>>    block: add a knob to disable multiwrite_merge
>>>>>>>    hw/virtio-blk: add a constant for max number of merged requests
>>>>>> In addition, I'd like a test for this (just the parameter
>>>>>> would be enough). But I don't object to this series without
>>>>>> it.
>>>>> Thanks for your comments. I will respin tomorrow.
>>>>>
>>>>> What exactly would you like to check in a test?
>>>> Just give the parameter and test the query-block against it. I'd
>>>> test the default, switching it on, off and maybe even specify it
>>>> for a non-root BDS to see whether that works.
>>> It seems it is currently not visible in query-block which
>>> paramters are set to e.g. file. If I specifiy -drive
>>> file.write_merging=off its accepted but not displayed. Any ideas?
>> Hm, I forgot that query-block only gives you the root BDS. Too bad.
>> Then we cannot test that. We probably want to have a QMP command to
>> query the whole BDS graph some time, but it isn't there yet.
> Does query-named-block-nodes help in this specific case?

Oh, yes, it does. :-)

I think I should learn QMP once in a while...

Max

[Qemu-devel] [PATCH 0/4] virtio-blk: add multiread support

[Peter Lieven]

this series adds the long missing multiread support to virtio-blk.

some remarks:
 - i introduced rd_merged and wr_merged block accounting stats to
   blockstats as a generic interface which can be set from any
   driver that will introduce multirequest merging in the future.
 - the knob to disable request merging is not yet there. I would
   add it to the device properties also as a generic interface
   to have the same switch for any driver that might introduce
   request merging in the future. As there has been no knob in
   the past I would post this as a seperate series as it needs
   some mangling in parameter parsing which might lead to further
   discussions.
 - the old multiwrite interface is still there and might be removed.

RFC v2->v1:
 - added Erics grammar corrections to Patch 4
 - Patch 4
  - reworked the IF tree to a SWITCH statement to make it easier
    to understand in virtio_blk_handle_request
  - stop merging if requests switch from read to write or vice
    versa. e..g, otherwise we could introduce big delays as a small
    read request could be followed by a lot of write requests
    and the read requests sits there until the queue is empty.

RFC v1->RFC v2:
 - completed Patch 1 by the example in qmp-commands.hx [Eric]
 - used bool for merge in Patch 4 [Max]
 - fixed a few typos in the commit msg of Patch 4 [Max]
 - do not start merging and directly pass req[0]->qiov in case of num_reqs == 1
 - avoid allocating memory for the multireq [Kevin]
 - do not import block_int.h and add appropiate iface to block-backend [Kevin]
 - removed debug output and added trace event for multireq [Kevin]
 - fixed alloc hint for the merge qiov [Kevin]
 - currently did not split virtio_submit_multireq into rw code since
   the redundant code would now be much bigger part than in the original patch.
 - added a merge_qiov to VirtioBlockRequest. Abusing the qiov was not possible
   because it is initialized externally with guest memory [Kevin]
 - added a pointer to VirtioBlockRequest to create a linked list
   of VirtioBlockBlockRequests. This list is used to complete all
   requests belonging to a multireq [Kevin]

Peter Lieven (4):
  block: add accounting for merged requests
  hw/virtio-blk: add a constant for max number of merged requests
  block-backend: expose bs->bl.max_transfer_length
  virtio-blk: introduce multiread

 block.c                         |    2 +
 block/accounting.c              |    7 ++
 block/block-backend.c           |    5 +
 block/qapi.c                    |    2 +
 hmp.c                           |    6 +-
 hw/block/dataplane/virtio-blk.c |    6 +-
 hw/block/virtio-blk.c           |  239 ++++++++++++++++++++++++---------------
 include/block/accounting.h      |    3 +
 include/hw/virtio/virtio-blk.h  |   20 +++-
 include/sysemu/block-backend.h  |    1 +
 qapi/block-core.json            |    9 +-
 qmp-commands.hx                 |   22 +++-
 trace-events                    |    1 +
 13 files changed, 213 insertions(+), 110 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/4] block: add accounting for merged requests

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c                    |    2 ++
 block/accounting.c         |    7 +++++++
 block/qapi.c               |    2 ++
 hmp.c                      |    6 +++++-
 include/block/accounting.h |    3 +++
 qapi/block-core.json       |    9 ++++++++-
 qmp-commands.hx            |   22 ++++++++++++++++++----
 7 files changed, 45 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a612594..75450e3 100644
--- a/block.c
+++ b/block.c
@@ -4508,6 +4508,8 @@ static int multiwrite_merge(BlockDriverState *bs, BlockRequest *reqs,
         }
     }
 
+    block_acct_merge_done(&bs->stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1);
+
     return outidx + 1;
 }
 
diff --git a/block/accounting.c b/block/accounting.c
index edbb1cc..d1afcd9 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -52,3 +52,10 @@ void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
         stats->wr_highest_sector = sector_num + nb_sectors - 1;
     }
 }
+
+void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
+                      int num_requests)
+{
+    assert(type < BLOCK_MAX_IOTYPE);
+    stats->merged[type] += num_requests;
+}
diff --git a/block/qapi.c b/block/qapi.c
index a87a34a..ec28240 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -316,6 +316,8 @@ static BlockStats *bdrv_query_stats(const BlockDriverState *bs)
     s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
     s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ];
     s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
+    s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
+    s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
     s->stats->wr_highest_offset =
         bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE;
     s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
diff --git a/hmp.c b/hmp.c
index 63d7686..baaa9e5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -419,6 +419,8 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
                        " wr_total_time_ns=%" PRId64
                        " rd_total_time_ns=%" PRId64
                        " flush_total_time_ns=%" PRId64
+                       " rd_merged=%" PRId64
+                       " wr_merged=%" PRId64
                        "\n",
                        stats->value->stats->rd_bytes,
                        stats->value->stats->wr_bytes,
@@ -427,7 +429,9 @@ void hmp_info_blockstats(Monitor *mon, const QDict *qdict)
                        stats->value->stats->flush_operations,
                        stats->value->stats->wr_total_time_ns,
                        stats->value->stats->rd_total_time_ns,
-                       stats->value->stats->flush_total_time_ns);
+                       stats->value->stats->flush_total_time_ns,
+                       stats->value->stats->rd_merged,
+                       stats->value->stats->wr_merged);
     }
 
     qapi_free_BlockStatsList(stats_list);
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 50b42b3..4c406cf 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -39,6 +39,7 @@ typedef struct BlockAcctStats {
     uint64_t nr_bytes[BLOCK_MAX_IOTYPE];
     uint64_t nr_ops[BLOCK_MAX_IOTYPE];
     uint64_t total_time_ns[BLOCK_MAX_IOTYPE];
+    uint64_t merged[BLOCK_MAX_IOTYPE];
     uint64_t wr_highest_sector;
 } BlockAcctStats;
 
@@ -53,5 +54,7 @@ void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
 void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie);
 void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
                                unsigned int nb_sectors);
+void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
+                           int num_requests);
 
 #endif
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a14e6ab..ead6d5b 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -389,13 +389,20 @@
 #                     growable sparse files (like qcow2) that are used on top
 #                     of a physical device.
 #
+# @rd_merged: Number of read requests that have been merged into another
+#             request (Since 2.3).
+#
+# @wr_merged: Number of write requests that have been merged into another
+#             request (Since 2.3).
+#
 # Since: 0.14.0
 ##
 { 'type': 'BlockDeviceStats',
   'data': {'rd_bytes': 'int', 'wr_bytes': 'int', 'rd_operations': 'int',
            'wr_operations': 'int', 'flush_operations': 'int',
            'flush_total_time_ns': 'int', 'wr_total_time_ns': 'int',
-           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int' } }
+           'rd_total_time_ns': 'int', 'wr_highest_offset': 'int',
+           'rd_merged': 'int', 'wr_merged': 'int' } }
 
 ##
 # @BlockStats:
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 718dd92..42f1e59 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2261,6 +2261,10 @@ Each json-object contain the following:
     - "flush_total_time_ns": total time spend on cache flushes in nano-seconds (json-int)
     - "wr_highest_offset": Highest offset of a sector written since the
                            BlockDriverState has been opened (json-int)
+    - "rd_merged": number of read requests that have been merged into
+                   another request (json-int)
+    - "wr_merged": number of write requests that have been merged into
+                   another request (json-int)
 - "parent": Contains recursively the statistics of the underlying
             protocol (e.g. the host file for a qcow2 image). If there is
             no underlying protocol, this field is omitted
@@ -2284,6 +2288,8 @@ Example:
                   "rd_total_times_ns":3465673657
                   "flush_total_times_ns":49653
                   "flush_operations":61,
+                  "rd_merged":0,
+                  "wr_merged":0
                }
             },
             "stats":{
@@ -2295,7 +2301,9 @@ Example:
                "flush_operations":51,
                "wr_total_times_ns":313253456
                "rd_total_times_ns":3465673657
-               "flush_total_times_ns":49653
+               "flush_total_times_ns":49653,
+               "rd_merged":0,
+               "wr_merged":0
             }
          },
          {
@@ -2309,7 +2317,9 @@ Example:
                "flush_operations":0,
                "wr_total_times_ns":0
                "rd_total_times_ns":0
-               "flush_total_times_ns":0
+               "flush_total_times_ns":0,
+               "rd_merged":0,
+               "wr_merged":0
             }
          },
          {
@@ -2323,7 +2333,9 @@ Example:
                "flush_operations":0,
                "wr_total_times_ns":0
                "rd_total_times_ns":0
-               "flush_total_times_ns":0
+               "flush_total_times_ns":0,
+               "rd_merged":0,
+               "wr_merged":0
             }
          },
          {
@@ -2337,7 +2349,9 @@ Example:
                "flush_operations":0,
                "wr_total_times_ns":0
                "rd_total_times_ns":0
-               "flush_total_times_ns":0
+               "flush_total_times_ns":0,
+               "rd_merged":0,
+               "wr_merged":0
             }
          }
       ]
-- 
1.7.9.5