| Message ID | 1484467275-27919-3-git-send-email-douly.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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
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;
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(-)