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

[Max Reitz]

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

signature.asc
Description: OpenPGP digital signature