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

[Dou Liyang <douly.fnst@cn.fujitsu.com>]

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