Re: [Qemu-devel] [PATCH RFC v2 3/4] block/qapi: acquire a reference instead of a lock during querying blockstats

[Stefan Hajnoczi]

On Mon, Dec 19, 2016 at 04:51:25PM +0800, Dou Liyang wrote:
> This patch works to improve the performance of the query requests.
> 
> From the commit 13344f3a, it adds a lock to make query-blockstats
> safe by the aio_context_acquire(). the qmp_query_blockstats func
> requires/releases the AioContext lock, which takes some time and
> blocks the I/O processing. It affects the performance, especially
> in the multi-disks guests.
> 
> As the low-level details of block statistics inside QEMU, we can
> acquire a reference instead of the lock.
> 
> Signed-off-by: Dou Liyang <address@hidden>
> ---
>  block/qapi.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

This patch changes the locking rules for blk_get_stats() (this covers a
lot of fields), bdrv_get_node_name(), and blk_name().

You must document the new locking rules for these fields in
block-backend.h, block_int.h, etc.

> diff --git a/block/qapi.c b/block/qapi.c
> index bc622cd..2262918 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -457,17 +457,21 @@ static BlockStats *bdrv_query_bds_stats(const 
> BlockDriverState *bs,
>  }
>  
>  static BlockStats *bdrv_query_stats(BlockBackend *blk,
> -                                    const BlockDriverState *bs,
> +                                    BlockDriverState *bs,
>                                      bool query_backing)
>  {
>      BlockStats *s;
>  
> +    bdrv_ref(bs);
>      s = bdrv_query_bds_stats(bs, query_backing);
> +    bdrv_unref(bs);
>  
>      if (blk) {
> +        blk_ref(blk);
>          s->has_device = true;
>          s->device = g_strdup(blk_name(blk));
>          bdrv_query_blk_stats(s->stats, blk);
> +        blk_unref(blk);

This does not look correct.  The caller passed in bs and blk so they
must already have a reference.  If not, then what protects bs and blk
from deletion before/after this function is called?

>      }
>  
>      return s;
> @@ -523,13 +527,8 @@ BlockStatsList *qmp_query_blockstats(bool 
> has_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);
>  
> -        aio_context_acquire(ctx);
>          info->value = bdrv_query_stats(blk, bs, !query_nodes);
> -        aio_context_release(ctx);
> -
>          *p_next = info;
>          p_next = &info->next;
>      }
> -- 
> 2.5.5
> 
> 
> 
>

signature.asc
Description: PGP signature