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

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

[Dou Liyang]

Change log v3 -> v4:
 1. Develop these into the non-RFC patches.
 2. Fix some comments.
 3. do declarations first.

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.

The Qemu uses the qmp_query_blockstats() function to get the
blockstats. As the function has been changed several times, it
becomes more complex and longer.

For the multi-disks guest, if we want to execute I/O operations
and this function at the same time. we can use the dataplane
feature to hold I/O performance does not drop. But, Normally
without this feature, How to reduce the decline in performance?

These patches refactor the qmp_query_blockstats() to make the
code easier to follow, and shorter as follows:

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|
                           |                    |
                           +--------------------+


They also optimize the fuction by reducing its running time.

1. The function running time

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

2. For the performance
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.

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

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 | 95 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 41 insertions(+), 54 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v4 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 tests.

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

Avoid testing 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 v4 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 | 73 ++++++++++++++++++++++++------------------------------------
 1 file changed, 29 insertions(+), 44 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index bc622cd..45e9d47 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,44 @@ 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);
+            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;
-- 
2.5.5

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

[Dou Liyang]

Hi, Markus

Here is the non-RFC patch.
Any suggestions are welcome. :)

Thanks,
	Liyang.

At 01/15/2017 04:01 PM, Dou Liyang wrote:
> Change log v3 -> v4:
>  1. Develop these into the non-RFC patches.
>  2. Fix some comments.
>  3. do declarations first.
>
> 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.
>
> The Qemu uses the qmp_query_blockstats() function to get the
> blockstats. As the function has been changed several times, it
> becomes more complex and longer.
>
> For the multi-disks guest, if we want to execute I/O operations
> and this function at the same time. we can use the dataplane
> feature to hold I/O performance does not drop. But, Normally
> without this feature, How to reduce the decline in performance?
>
> These patches refactor the qmp_query_blockstats() to make the
> code easier to follow, and shorter as follows:
>
> 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|
>                            |                    |
>                            +--------------------+
>
>
> They also optimize the fuction by reducing its running time.
>
> 1. The function running time
>
> the time it takes(ns) in each requests:
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 19429 | 667722
> after these patches  | 18536 | 627945
>
> 2. For the performance
> 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.
>
> 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
>
> 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 | 95 ++++++++++++++++++++++++++----------------------------------
>  1 file changed, 41 insertions(+), 54 deletions(-)
>

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

[Markus Armbruster]

Cc'ing qemu-block.

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

> Change log v3 -> v4:
>  1. Develop these into the non-RFC patches.
>  2. Fix some comments.
>  3. do declarations first.
>
> 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.
>
> The Qemu uses the qmp_query_blockstats() function to get the
> blockstats. As the function has been changed several times, it
> becomes more complex and longer.
>
> For the multi-disks guest, if we want to execute I/O operations
> and this function at the same time. we can use the dataplane
> feature to hold I/O performance does not drop. But, Normally
> without this feature, How to reduce the decline in performance?
>
> These patches refactor the qmp_query_blockstats() to make the
> code easier to follow, and shorter as follows:
>
> 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|
>                            |                    |
>                            +--------------------+
>
>
> They also optimize the fuction by reducing its running time.
>
> 1. The function running time
>
> the time it takes(ns) in each requests:
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 19429 | 667722 
> after these patches  | 18536 | 627945
>
> 2. For the performance
> 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.
>
> 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

Series
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Max Reitz]

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

On 15.01.2017 09:01, Dou Liyang wrote:
> 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 | 73 ++++++++++++++++++++++++------------------------------------
>  1 file changed, 29 insertions(+), 44 deletions(-)
> 
> diff --git a/block/qapi.c b/block/qapi.c
> index bc622cd..45e9d47 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,44 @@ 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);
> +            BlockStats *s;
> +
> +            aio_context_acquire(ctx);
> +            info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);

Superfluous assignment to info->value here...

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

...since it's written here anyway. I'll remove the first assignment when
applying the patch to my tree.

Max

> +            *p_next = info;
> +            p_next = &info->next;
> +        }
>      }
>  
>      return head;
> 



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

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

[Max Reitz]

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

On 15.01.2017 09:01, Dou Liyang wrote:
> Change log v3 -> v4:
>  1. Develop these into the non-RFC patches.
>  2. Fix some comments.
>  3. do declarations first.
> 
> 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.
> 
> The Qemu uses the qmp_query_blockstats() function to get the
> blockstats. As the function has been changed several times, it
> becomes more complex and longer.
> 
> For the multi-disks guest, if we want to execute I/O operations
> and this function at the same time. we can use the dataplane
> feature to hold I/O performance does not drop. But, Normally
> without this feature, How to reduce the decline in performance?
> 
> These patches refactor the qmp_query_blockstats() to make the
> code easier to follow, and shorter as follows:
> 
> 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|
>                            |                    |
>                            +--------------------+
> 
> 
> They also optimize the fuction by reducing its running time.
> 
> 1. The function running time
> 
> the time it takes(ns) in each requests:
> the disk numbers     | 10    | 500
> -------------------------------------
> before these patches | 19429 | 667722 
> after these patches  | 18536 | 627945
> 
> 2. For the performance
> 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.
> 
> 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
> 
> 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 | 95 ++++++++++++++++++++++++++----------------------------------
>  1 file changed, 41 insertions(+), 54 deletions(-)

Thanks, dropped one of the duplicate info->value assignments in patch 2
and applied the series to my block tree:

https://github.com/XanClic/qemu/commits/block

(Assuming Markus' R-b meant that he does not intends to take it through
his tree.)

Max


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

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

[Dou Liyang]

Hi, Max

At 02/01/2017 11:02 AM, Max Reitz wrote:
> On 15.01.2017 09:01, Dou Liyang wrote:
>> 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 | 73 ++++++++++++++++++++++++------------------------------------
>>  1 file changed, 29 insertions(+), 44 deletions(-)
>>
>> diff --git a/block/qapi.c b/block/qapi.c
>> index bc622cd..45e9d47 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,44 @@ 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);
>> +            BlockStats *s;
>> +
>> +            aio_context_acquire(ctx);
>> +            info->value = s = bdrv_query_bds_stats(blk_bs(blk), true);
>
> Superfluous assignment to info->value here...

Yes, It is.

>
>> +            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;
>
> ...since it's written here anyway. I'll remove the first assignment when
> applying the patch to my tree.

Thanks very much!

>
> Max
>
>> +            *p_next = info;
>> +            p_next = &info->next;
>> +        }
>>      }
>>
>>      return head;
>>
>
>

Thanks,
	Liyang

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> Thanks, dropped one of the duplicate info->value assignments in patch 2
> and applied the series to my block tree:
>
> https://github.com/XanClic/qemu/commits/block
>
> (Assuming Markus' R-b meant that he does not intends to take it through
> his tree.)

"Does not intend" is a bit too strong, but please go ahead and save me
work :)