[Qemu-devel] [PATCH RFC v3 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Qemu-devel] [PATCH RFC v3 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Dou Liyang]

Change log v2 -> v3:
 1. Remove the unnecessary code for the bdrv_next_node().
 2. Remove the change of the locking rules.
    Even if this change can improve the performance, but it may
    effect the consistency.

For the multi-disks guest, we can use the dataplane feature to
hold performance does not drop, if we execute some slow monitor
commands, such as "info blockstats". But, without this feature, 
How to reduce the decline in performance?

These patches aim to refactor the qmp_query_blockstats() and
improve the performance by reducing the running time of it.

There are the two jobs:

1 For the performance:

1.1 the time it takes(ns) in each time:
the disk numbers     | 10    | 500
-------------------------------------
before these patches | 19429 | 667722 
after these patches  | 18536 | 627945

1.2 the I/O performance is degraded(%) during the monitor:

the disk numbers     | 10    | 500
-------------------------------------
before these patches | 1.3   | 14.2
after these patches  | 1.0   | 11.3

used the dd command likes this to test: 
dd if=date_1.dat of=date_2.dat conv=fsync oflag=direct bs=1k count=100k.

2 refactor qmp_query_blockstats():

From:

+--------------+      +---------------------+
 | 1            |      | 4.                  |
 |next_query_bds|      |bdrv_query_bds_stats +---+
 |              |      |                     |   |
 +--------^-----+      +-------------^-------+   |
          |                          |           |
+---------+----------+      +--------+-------+   |
| 0.                 |      | 2.             |   |
|qmp_query_blockstats+------>bdrv_query_stats<----
|                    |      |                |
+--------------------+      +--------+-------+
                                     |
                       +-------------v-------+
                       | 3.                  |
                       |bdrv_query_blk_stats |
                       |                     |
                       +---------------------+

To:

                                    +--------------+
                                    |              |
                           +--------v-----------+  |
                       +--->  3.                |  |
+-------------------+  |   |bdrv_query_bds_stats+--+
| 1.                +--+   |                    |
|                   +      +--------------------+
|qmp_query_blockstats--+
|                   |  |
+-------------------+  |   +--------------------+
                       |   | 2.                 |
                       +--->                    |
                           |bdrv_query_blk_stats|
                           |                    |
                           +--------------------+

Dou Liyang (2):
  block/qapi: reduce the coupling between the bdrv_query_stats and
    bdrv_query_bds_stats
  block/qapi: reduce the execution time of qmp_query_blockstats

 block/qapi.c | 94 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 40 insertions(+), 54 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH RFC v3 1/2] block/qapi: reduce the coupling between the bdrv_query_stats and bdrv_query_bds_stats

[Dou Liyang]

the bdrv_query_stats and bdrv_query_bds_stats functions need to call
each other, that increases the coupling. it also makes the program
complicated and makes some unnecessary judgements.

remove the call from bdrv_query_bds_stats to bdrv_query_stats, just
take some recursion to make it clearly.

avoid judging whether the blk is NULL during querying the bds stats.
it is unnecessary.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 block/qapi.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index a62e862..bc622cd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -357,10 +357,6 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo **p_info,
     qapi_free_BlockInfo(info);
 }
 
-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-                                    const BlockDriverState *bs,
-                                    bool query_backing);
-
 static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
 {
     BlockAcctStats *stats = blk_get_stats(blk);
@@ -428,9 +424,18 @@ static void bdrv_query_blk_stats(BlockDeviceStats *ds, BlockBackend *blk)
     }
 }
 
-static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
+static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
                                  bool query_backing)
 {
+    BlockStats *s = NULL;
+
+    s = g_malloc0(sizeof(*s));
+    s->stats = g_malloc0(sizeof(*s->stats));
+
+    if (!bs) {
+        return s;
+    }
+
     if (bdrv_get_node_name(bs)[0]) {
         s->has_node_name = true;
         s->node_name = g_strdup(bdrv_get_node_name(bs));
@@ -440,14 +445,15 @@ static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
 
     if (bs->file) {
         s->has_parent = true;
-        s->parent = bdrv_query_stats(NULL, bs->file->bs, query_backing);
+        s->parent = bdrv_query_bds_stats(bs->file->bs, query_backing);
     }
 
     if (query_backing && bs->backing) {
         s->has_backing = true;
-        s->backing = bdrv_query_stats(NULL, bs->backing->bs, query_backing);
+        s->backing = bdrv_query_bds_stats(bs->backing->bs, query_backing);
     }
 
+    return s;
 }
 
 static BlockStats *bdrv_query_stats(BlockBackend *blk,
@@ -456,17 +462,13 @@ static BlockStats *bdrv_query_stats(BlockBackend *blk,
 {
     BlockStats *s;
 
-    s = g_malloc0(sizeof(*s));
-    s->stats = g_malloc0(sizeof(*s->stats));
+    s = bdrv_query_bds_stats(bs, query_backing);
 
     if (blk) {
         s->has_device = true;
         s->device = g_strdup(blk_name(blk));
         bdrv_query_blk_stats(s->stats, blk);
     }
-    if (bs) {
-        bdrv_query_bds_stats(s, bs, query_backing);
-    }
 
     return s;
 }
-- 
2.5.5

[Qemu-devel] [PATCH RFC v3 2/2] block/qapi: reduce the execution time of qmp_query_blockstats

[Dou Liyang]

In order to reduce the execution time, this patch optimize
the qmp_query_blockstats():
Remove the next_query_bds function.
Remove the bdrv_query_stats function.
Remove some judgement sentence.

The original qmp_query_blockstats calls next_query_bds to get
the next objects in each loops. In the next_query_bds, it checks
the query_nodes and blk. It also call bdrv_query_stats to get
the stats, In the bdrv_query_stats, it checks blk and bs each
times. This waste more times, which may stall the main loop a
bit. And if the disk is too many and donot use the dataplane
feature, this may affect the performance in main loop thread.

This patch removes that two functions, and makes the structure
clearly.

Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
---
 block/qapi.c | 72 +++++++++++++++++++++++-------------------------------------
 1 file changed, 28 insertions(+), 44 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index bc622cd..6e1e8bd 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -456,23 +456,6 @@ static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
     return s;
 }
 
-static BlockStats *bdrv_query_stats(BlockBackend *blk,
-                                    const BlockDriverState *bs,
-                                    bool query_backing)
-{
-    BlockStats *s;
-
-    s = bdrv_query_bds_stats(bs, query_backing);
-
-    if (blk) {
-        s->has_device = true;
-        s->device = g_strdup(blk_name(blk));
-        bdrv_query_blk_stats(s->stats, blk);
-    }
-
-    return s;
-}
-
 BlockInfoList *qmp_query_block(Error **errp)
 {
     BlockInfoList *head = NULL, **p_next = &head;
@@ -496,42 +479,43 @@ BlockInfoList *qmp_query_block(Error **errp)
     return head;
 }
 
-static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
-                           bool query_nodes)
-{
-    if (query_nodes) {
-        *bs = bdrv_next_node(*bs);
-        return !!*bs;
-    }
-
-    *blk = blk_next(*blk);
-    *bs = *blk ? blk_bs(*blk) : NULL;
-
-    return !!*blk;
-}
-
 BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
                                      bool query_nodes,
                                      Error **errp)
 {
     BlockStatsList *head = NULL, **p_next = &head;
-    BlockBackend *blk = NULL;
-    BlockDriverState *bs = NULL;
+    BlockBackend *blk;
+    BlockDriverState *bs;
 
     /* Just to be safe if query_nodes is not always initialized */
-    query_nodes = has_query_nodes && query_nodes;
-
-    while (next_query_bds(&blk, &bs, query_nodes)) {
-        BlockStatsList *info = g_malloc0(sizeof(*info));
-        AioContext *ctx = blk ? blk_get_aio_context(blk)
-                              : bdrv_get_aio_context(bs);
+    if (has_query_nodes && query_nodes) {
+        for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
+            BlockStatsList *info = g_malloc0(sizeof(*info));
+            AioContext *ctx = bdrv_get_aio_context(bs);
 
-        aio_context_acquire(ctx);
-        info->value = bdrv_query_stats(blk, bs, !query_nodes);
-        aio_context_release(ctx);
+            aio_context_acquire(ctx);
+            info->value = bdrv_query_bds_stats(bs, false);
+            aio_context_release(ctx);
 
-        *p_next = info;
-        p_next = &info->next;
+            *p_next = info;
+            p_next = &info->next;
+        }
+    } else {
+        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
+            BlockStatsList *info = g_malloc0(sizeof(*info));
+            AioContext *ctx = blk_get_aio_context(blk);
+
+            aio_context_acquire(ctx);
+            BlockStats *s = bdrv_query_bds_stats(blk_bs(blk), true);
+            s->has_device = true;
+            s->device = g_strdup(blk_name(blk));
+            bdrv_query_blk_stats(s->stats, blk);
+            aio_context_release(ctx);
+
+            info->value = s;
+            *p_next = info;
+            p_next = &info->next;
+        }
     }
 
     return head;
-- 
2.5.5

Re: [Qemu-devel] [PATCH RFC v3 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3394 bytes --]

On Wed, Jan 04, 2017 at 02:58:09PM +0800, Dou Liyang wrote:
> Change log v2 -> v3:
>  1. Remove the unnecessary code for the bdrv_next_node().
>  2. Remove the change of the locking rules.
>     Even if this change can improve the performance, but it may
>     effect the consistency.
> 
> For the multi-disks guest, we can use the dataplane feature to
> hold performance does not drop, if we execute some slow monitor
> commands, such as "info blockstats". But, without this feature, 
> How to reduce the decline in performance?
> 
> These patches aim to refactor the qmp_query_blockstats() and
> improve the performance by reducing the running time of it.
> 
> There are the two jobs:
> 
> 1 For the performance:
> 
> 1.1 the time it takes(ns) in each time:
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 19429 | 667722 
> after these patches  | 18536 | 627945
> 
> 1.2 the I/O performance is degraded(%) during the monitor:
> 
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 1.3   | 14.2
> after these patches  | 1.0   | 11.3
> 
> used the dd command likes this to test: 
> dd if=date_1.dat of=date_2.dat conv=fsync oflag=direct bs=1k count=100k.
> 
> 2 refactor qmp_query_blockstats():
> 
> From:
> 
> +--------------+      +---------------------+
>  | 1            |      | 4.                  |
>  |next_query_bds|      |bdrv_query_bds_stats +---+
>  |              |      |                     |   |
>  +--------^-----+      +-------------^-------+   |
>           |                          |           |
> +---------+----------+      +--------+-------+   |
> | 0.                 |      | 2.             |   |
> |qmp_query_blockstats+------>bdrv_query_stats<----
> |                    |      |                |
> +--------------------+      +--------+-------+
>                                      |
>                        +-------------v-------+
>                        | 3.                  |
>                        |bdrv_query_blk_stats |
>                        |                     |
>                        +---------------------+
> 
> To:
> 
>                                     +--------------+
>                                     |              |
>                            +--------v-----------+  |
>                        +--->  3.                |  |
> +-------------------+  |   |bdrv_query_bds_stats+--+
> | 1.                +--+   |                    |
> |                   +      +--------------------+
> |qmp_query_blockstats--+
> |                   |  |
> +-------------------+  |   +--------------------+
>                        |   | 2.                 |
>                        +--->                    |
>                            |bdrv_query_blk_stats|
>                            |                    |
>                            +--------------------+
> 
> Dou Liyang (2):
>   block/qapi: reduce the coupling between the bdrv_query_stats and
>     bdrv_query_bds_stats
>   block/qapi: reduce the execution time of qmp_query_blockstats
> 
>  block/qapi.c | 94 ++++++++++++++++++++++++++----------------------------------
>  1 file changed, 40 insertions(+), 54 deletions(-)

Leaving review to Markus Armbruster since he maintains this area.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 2/2] block/qapi: reduce the execution time of qmp_query_blockstats

[Markus Armbruster]

Dou Liyang <douly.fnst@cn.fujitsu.com> writes:

> In order to reduce the execution time, this patch optimize
> the qmp_query_blockstats():
> Remove the next_query_bds function.
> Remove the bdrv_query_stats function.
> Remove some judgement sentence.
>
> The original qmp_query_blockstats calls next_query_bds to get
> the next objects in each loops. In the next_query_bds, it checks
> the query_nodes and blk. It also call bdrv_query_stats to get
> the stats, In the bdrv_query_stats, it checks blk and bs each
> times. This waste more times, which may stall the main loop a
> bit. And if the disk is too many and donot use the dataplane
> feature, this may affect the performance in main loop thread.
>
> This patch removes that two functions, and makes the structure
> clearly.
>
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  block/qapi.c | 72 +++++++++++++++++++++++-------------------------------------
>  1 file changed, 28 insertions(+), 44 deletions(-)
>
> diff --git a/block/qapi.c b/block/qapi.c
> index bc622cd..6e1e8bd 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -456,23 +456,6 @@ static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
>      return s;
>  }
>  
> -static BlockStats *bdrv_query_stats(BlockBackend *blk,
> -                                    const BlockDriverState *bs,
> -                                    bool query_backing)
> -{
> -    BlockStats *s;
> -
> -    s = bdrv_query_bds_stats(bs, query_backing);
> -
> -    if (blk) {
> -        s->has_device = true;
> -        s->device = g_strdup(blk_name(blk));
> -        bdrv_query_blk_stats(s->stats, blk);
> -    }
> -
> -    return s;
> -}
> -
>  BlockInfoList *qmp_query_block(Error **errp)
>  {
>      BlockInfoList *head = NULL, **p_next = &head;
> @@ -496,42 +479,43 @@ BlockInfoList *qmp_query_block(Error **errp)
>      return head;
>  }
>  
> -static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
> -                           bool query_nodes)
> -{
> -    if (query_nodes) {
> -        *bs = bdrv_next_node(*bs);
> -        return !!*bs;
> -    }
> -
> -    *blk = blk_next(*blk);
> -    *bs = *blk ? blk_bs(*blk) : NULL;
> -
> -    return !!*blk;
> -}
> -
>  BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
>                                       bool query_nodes,
>                                       Error **errp)
>  {
>      BlockStatsList *head = NULL, **p_next = &head;
> -    BlockBackend *blk = NULL;
> -    BlockDriverState *bs = NULL;
> +    BlockBackend *blk;
> +    BlockDriverState *bs;
>  
>      /* Just to be safe if query_nodes is not always initialized */
> -    query_nodes = has_query_nodes && query_nodes;
> -
> -    while (next_query_bds(&blk, &bs, query_nodes)) {
> -        BlockStatsList *info = g_malloc0(sizeof(*info));
> -        AioContext *ctx = blk ? blk_get_aio_context(blk)
> -                              : bdrv_get_aio_context(bs);
> +    if (has_query_nodes && query_nodes) {
> +        for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
> +            BlockStatsList *info = g_malloc0(sizeof(*info));
> +            AioContext *ctx = bdrv_get_aio_context(bs);
>  
> -        aio_context_acquire(ctx);
> -        info->value = bdrv_query_stats(blk, bs, !query_nodes);
> -        aio_context_release(ctx);
> +            aio_context_acquire(ctx);
> +            info->value = bdrv_query_bds_stats(bs, false);
> +            aio_context_release(ctx);
>  
> -        *p_next = info;
> -        p_next = &info->next;
> +            *p_next = info;
> +            p_next = &info->next;
> +        }
> +    } else {
> +        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
> +            BlockStatsList *info = g_malloc0(sizeof(*info));
> +            AioContext *ctx = blk_get_aio_context(blk);
> +
> +            aio_context_acquire(ctx);
> +            BlockStats *s = bdrv_query_bds_stats(blk_bs(blk), true);

Please don't put declarations after statements.  Suggest:

               BlockStats *s;

               aio_context_acquire(ctx);
               info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);

> +            s->has_device = true;
> +            s->device = g_strdup(blk_name(blk));
> +            bdrv_query_blk_stats(s->stats, blk);
> +            aio_context_release(ctx);
> +
> +            info->value = s;
> +            *p_next = info;
> +            p_next = &info->next;
> +        }
>      }
>  
>      return head;

Re: [Qemu-devel] [PATCH RFC v3 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Markus Armbruster]

Dou Liyang <douly.fnst@cn.fujitsu.com> writes:

> Change log v2 -> v3:
>  1. Remove the unnecessary code for the bdrv_next_node().
>  2. Remove the change of the locking rules.
>     Even if this change can improve the performance, but it may
>     effect the consistency.
>
> For the multi-disks guest, we can use the dataplane feature to
> hold performance does not drop, if we execute some slow monitor
> commands, such as "info blockstats". But, without this feature, 
> How to reduce the decline in performance?
>
> These patches aim to refactor the qmp_query_blockstats() and
> improve the performance by reducing the running time of it.
>
> There are the two jobs:
>
> 1 For the performance:
>
> 1.1 the time it takes(ns) in each time:
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 19429 | 667722 
> after these patches  | 18536 | 627945
>
> 1.2 the I/O performance is degraded(%) during the monitor:
>
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 1.3   | 14.2
> after these patches  | 1.0   | 11.3
>
> used the dd command likes this to test: 
> dd if=date_1.dat of=date_2.dat conv=fsync oflag=direct bs=1k count=100k.
>
> 2 refactor qmp_query_blockstats():
>
> From:
>
> +--------------+      +---------------------+
>  | 1            |      | 4.                  |
>  |next_query_bds|      |bdrv_query_bds_stats +---+
>  |              |      |                     |   |
>  +--------^-----+      +-------------^-------+   |
>           |                          |           |
> +---------+----------+      +--------+-------+   |
> | 0.                 |      | 2.             |   |
> |qmp_query_blockstats+------>bdrv_query_stats<----
> |                    |      |                |
> +--------------------+      +--------+-------+
>                                      |
>                        +-------------v-------+
>                        | 3.                  |
>                        |bdrv_query_blk_stats |
>                        |                     |
>                        +---------------------+
>
> To:
>
>                                     +--------------+
>                                     |              |
>                            +--------v-----------+  |
>                        +--->  3.                |  |
> +-------------------+  |   |bdrv_query_bds_stats+--+
> | 1.                +--+   |                    |
> |                   +      +--------------------+
> |qmp_query_blockstats--+
> |                   |  |
> +-------------------+  |   +--------------------+
>                        |   | 2.                 |
>                        +--->                    |
>                            |bdrv_query_blk_stats|
>                            |                    |
>                            +--------------------+

Thanks for the picture, it helps.

Your patches make the code easier to follow, and shorter, too.  The
performance impact is unexpected.  But the code cleanup is worthwhile
even without it.  Please develop this into a non-RFC patch submission.

Re: [Qemu-devel] [PATCH RFC v3 1/2] block/qapi: reduce the coupling between the bdrv_query_stats and bdrv_query_bds_stats

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1245 bytes --]

On 01/04/2017 12:58 AM, Dou Liyang wrote:
> the bdrv_query_stats and bdrv_query_bds_stats functions need to call
> each other, that increases the coupling. it also makes the program
> complicated and makes some unnecessary judgements.

s/judgements/judgments/ - although I wonder if 'tests' would be a
simpler word.

> 
> remove the call from bdrv_query_bds_stats to bdrv_query_stats, just
> take some recursion to make it clearly.
> 
> avoid judging whether the blk is NULL during querying the bds stats.

Again, 'testing' might read nicer than 'judging'.

> it is unnecessary.
> 
> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
> ---
>  block/qapi.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 

> -static void bdrv_query_bds_stats(BlockStats *s, const BlockDriverState *bs,
> +static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
>                                   bool query_backing)
>  {
> +    BlockStats *s = NULL;
> +
> +    s = g_malloc0(sizeof(*s));

Dead initialization to NULL since the very next statement overwrites it.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC v3 1/2] block/qapi: reduce the coupling between the bdrv_query_stats and bdrv_query_bds_stats

[Dou Liyang]

Hi Eric,

At 01/13/2017 11:00 PM, Eric Blake wrote:
> On 01/04/2017 12:58 AM, Dou Liyang wrote:
>> the bdrv_query_stats and bdrv_query_bds_stats functions need to call
>> each other, that increases the coupling. it also makes the program
>> complicated and makes some unnecessary judgements.
>
> s/judgements/judgments/ - although I wonder if 'tests' would be a
> simpler word.
>

Yes, it is.
I will modify it.

>>
>> remove the call from bdrv_query_bds_stats to bdrv_query_stats, just
>> take some recursion to make it clearly.
>>
>> avoid judging whether the blk is NULL during querying the bds stats.
>
> Again, 'testing' might read nicer than 'judging'.

Yes, it is.


Thanks,
	Liyang

Re: [Qemu-devel] [PATCH RFC v3 2/2] block/qapi: reduce the execution time of qmp_query_blockstats

[Dou Liyang]

Hi, Markus

At 01/13/2017 06:42 PM, Markus Armbruster wrote:
> Dou Liyang <douly.fnst@cn.fujitsu.com> writes:
>
>> In order to reduce the execution time, this patch optimize
>> the qmp_query_blockstats():
>> Remove the next_query_bds function.
>> Remove the bdrv_query_stats function.
>> Remove some judgement sentence.
>>
>> The original qmp_query_blockstats calls next_query_bds to get
>> the next objects in each loops. In the next_query_bds, it checks
>> the query_nodes and blk. It also call bdrv_query_stats to get
>> the stats, In the bdrv_query_stats, it checks blk and bs each
>> times. This waste more times, which may stall the main loop a
>> bit. And if the disk is too many and donot use the dataplane
>> feature, this may affect the performance in main loop thread.
>>
>> This patch removes that two functions, and makes the structure
>> clearly.
>>
>> Signed-off-by: Dou Liyang <douly.fnst@cn.fujitsu.com>
>> ---
>>  block/qapi.c | 72 +++++++++++++++++++++++-------------------------------------
>>  1 file changed, 28 insertions(+), 44 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index bc622cd..6e1e8bd 100644
>> --- a/block/qapi.c
>> +++ b/block/qapi.c
>> @@ -456,23 +456,6 @@ static BlockStats *bdrv_query_bds_stats(const BlockDriverState *bs,
>>      return s;
>>  }
>>
>> -static BlockStats *bdrv_query_stats(BlockBackend *blk,
>> -                                    const BlockDriverState *bs,
>> -                                    bool query_backing)
>> -{
>> -    BlockStats *s;
>> -
>> -    s = bdrv_query_bds_stats(bs, query_backing);
>> -
>> -    if (blk) {
>> -        s->has_device = true;
>> -        s->device = g_strdup(blk_name(blk));
>> -        bdrv_query_blk_stats(s->stats, blk);
>> -    }
>> -
>> -    return s;
>> -}
>> -
>>  BlockInfoList *qmp_query_block(Error **errp)
>>  {
>>      BlockInfoList *head = NULL, **p_next = &head;
>> @@ -496,42 +479,43 @@ BlockInfoList *qmp_query_block(Error **errp)
>>      return head;
>>  }
>>
>> -static bool next_query_bds(BlockBackend **blk, BlockDriverState **bs,
>> -                           bool query_nodes)
>> -{
>> -    if (query_nodes) {
>> -        *bs = bdrv_next_node(*bs);
>> -        return !!*bs;
>> -    }
>> -
>> -    *blk = blk_next(*blk);
>> -    *bs = *blk ? blk_bs(*blk) : NULL;
>> -
>> -    return !!*blk;
>> -}
>> -
>>  BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
>>                                       bool query_nodes,
>>                                       Error **errp)
>>  {
>>      BlockStatsList *head = NULL, **p_next = &head;
>> -    BlockBackend *blk = NULL;
>> -    BlockDriverState *bs = NULL;
>> +    BlockBackend *blk;
>> +    BlockDriverState *bs;
>>
>>      /* Just to be safe if query_nodes is not always initialized */
>> -    query_nodes = has_query_nodes && query_nodes;
>> -
>> -    while (next_query_bds(&blk, &bs, query_nodes)) {
>> -        BlockStatsList *info = g_malloc0(sizeof(*info));
>> -        AioContext *ctx = blk ? blk_get_aio_context(blk)
>> -                              : bdrv_get_aio_context(bs);
>> +    if (has_query_nodes && query_nodes) {
>> +        for (bs = bdrv_next_node(NULL); bs; bs = bdrv_next_node(bs)) {
>> +            BlockStatsList *info = g_malloc0(sizeof(*info));
>> +            AioContext *ctx = bdrv_get_aio_context(bs);
>>
>> -        aio_context_acquire(ctx);
>> -        info->value = bdrv_query_stats(blk, bs, !query_nodes);
>> -        aio_context_release(ctx);
>> +            aio_context_acquire(ctx);
>> +            info->value = bdrv_query_bds_stats(bs, false);
>> +            aio_context_release(ctx);
>>
>> -        *p_next = info;
>> -        p_next = &info->next;
>> +            *p_next = info;
>> +            p_next = &info->next;
>> +        }
>> +    } else {
>> +        for (blk = blk_next(NULL); blk; blk = blk_next(blk)) {
>> +            BlockStatsList *info = g_malloc0(sizeof(*info));
>> +            AioContext *ctx = blk_get_aio_context(blk);
>> +
>> +            aio_context_acquire(ctx);
>> +            BlockStats *s = bdrv_query_bds_stats(blk_bs(blk), true);
>
> Please don't put declarations after statements.  Suggest:
>
>                BlockStats *s;
>
>                aio_context_acquire(ctx);
>                info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);
>

Yes, I see.

Thanks,
	Liyang


>> +            s->has_device = true;
>> +            s->device = g_strdup(blk_name(blk));
>> +            bdrv_query_blk_stats(s->stats, blk);
>> +            aio_context_release(ctx);
>> +
>> +            info->value = s;
>> +            *p_next = info;
>> +            p_next = &info->next;
>> +        }
>>      }
>>
>>      return head;
>
>
>

Re: [Qemu-devel] [PATCH RFC v3 0/2] block/qapi: refactor and optimize the qmp_query_blockstats()

[Dou Liyang]

Hi, Markus

At 01/13/2017 06:46 PM, Markus Armbruster wrote:
> Dou Liyang <douly.fnst@cn.fujitsu.com> writes:
>
>> Change log v2 -> v3:
>>  1. Remove the unnecessary code for the bdrv_next_node().
>>  2. Remove the change of the locking rules.
>>     Even if this change can improve the performance, but it may
>>     effect the consistency.
>>
>> For the multi-disks guest, we can use the dataplane feature to
>> hold performance does not drop, if we execute some slow monitor
>> commands, such as "info blockstats". But, without this feature,
>> How to reduce the decline in performance?
>>
>> These patches aim to refactor the qmp_query_blockstats() and
>> improve the performance by reducing the running time of it.
>>
>> There are the two jobs:
>>
>> 1 For the performance:
>>
>> 1.1 the time it takes(ns) in each time:
>> the disk numbers     | 10    | 500
>> -------------------------------------
>> before these patches | 19429 | 667722
>> after these patches  | 18536 | 627945
>>
>> 1.2 the I/O performance is degraded(%) during the monitor:
>>
>> the disk numbers     | 10    | 500
>> -------------------------------------
>> before these patches | 1.3   | 14.2
>> after these patches  | 1.0   | 11.3
>>
>> used the dd command likes this to test:
>> dd if=date_1.dat of=date_2.dat conv=fsync oflag=direct bs=1k count=100k.
>>
>> 2 refactor qmp_query_blockstats():
>>
>> From:
>>
>> +--------------+      +---------------------+
>>  | 1            |      | 4.                  |
>>  |next_query_bds|      |bdrv_query_bds_stats +---+
>>  |              |      |                     |   |
>>  +--------^-----+      +-------------^-------+   |
>>           |                          |           |
>> +---------+----------+      +--------+-------+   |
>> | 0.                 |      | 2.             |   |
>> |qmp_query_blockstats+------>bdrv_query_stats<----
>> |                    |      |                |
>> +--------------------+      +--------+-------+
>>                                      |
>>                        +-------------v-------+
>>                        | 3.                  |
>>                        |bdrv_query_blk_stats |
>>                        |                     |
>>                        +---------------------+
>>
>> To:
>>
>>                                     +--------------+
>>                                     |              |
>>                            +--------v-----------+  |
>>                        +--->  3.                |  |
>> +-------------------+  |   |bdrv_query_bds_stats+--+
>> | 1.                +--+   |                    |
>> |                   +      +--------------------+
>> |qmp_query_blockstats--+
>> |                   |  |
>> +-------------------+  |   +--------------------+
>>                        |   | 2.                 |
>>                        +--->                    |
>>                            |bdrv_query_blk_stats|
>>                            |                    |
>>                            +--------------------+
>
> Thanks for the picture, it helps.
>
> Your patches make the code easier to follow, and shorter, too.  The
> performance impact is unexpected.  But the code cleanup is worthwhile
> even without it.  Please develop this into a non-RFC patch submission.
>

OK, I understood.

Thanks,
	Liyang.
>
>