Re: [Qemu-devel] [PATCH] block: Let bdrv_drain_all() to call aio_poll() for each AioContext

[Paolo Bonzini <pbonzini@redhat.com>]

On 13/05/2015 18:34, Alexander Yarygin wrote:
> Ah, right. We need second loop, something like this:
> 
> @@ -2030,20 +2033,33 @@ void bdrv_drain(BlockDriverState *bs)
>  void bdrv_drain_all(void)
>  {
>      /* Always run first iteration so any pending completion BHs run */
> -    bool busy = true;
> +    bool busy = true, pending = false;
>      BlockDriverState *bs;
> +    GList *aio_ctxs = NULL, *ctx;
> +    AioContext *aio_context;
> 
>      while (busy) {
>          busy = false;
> 
>          QTAILQ_FOREACH(bs, &bdrv_states, device_list) {
> -            AioContext *aio_context = bdrv_get_aio_context(bs);
> +            aio_context = bdrv_get_aio_context(bs);
> 
>              aio_context_acquire(aio_context);
>              busy |= bdrv_drain_one(bs);
>              aio_context_release(aio_context);
> +            if (!aio_ctxs || !g_list_find(aio_ctxs, aio_context))
> +                aio_ctxs = g_list_append(aio_ctxs, aio_context);
> +        }
> +        pending = busy;
> +
> +        for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) {
> +            aio_context = ctx->data;
> +            aio_context_acquire(aio_context);
> +            busy |= aio_poll(aio_context, pending);
> +            aio_context_release(aio_context);
>          }
>      }
> +    g_list_free(aio_ctxs);
>  }
> 
> That looks quite ugly for me and breaks consistence of bdrv_drain_one()
> since it doesn't call aio_poll() anymore...

It's not ugly.  After your patch bdrv_drain_one doesn't call aio_poll,
while bdrv_drain and bdrv_drain_all call bdrv_drain_one + aio_poll.  All
callers of bdrv_drain_one are consistent.

Perhaps you can rename bdrv_drain_one to bdrv_flush_io_queue (inlining
the existing bdrv_flush_io_queue into it)?  That would work very well
for me.

Paolo