From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58826)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1Ysrta-0001am-7P
	for qemu-devel@nongnu.org; Thu, 14 May 2015 08:05:47 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1YsrtX-0001L3-1M
	for qemu-devel@nongnu.org; Thu, 14 May 2015 08:05:46 -0400
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <55548F90.8010709@redhat.com>
Date: Thu, 14 May 2015 14:05:36 +0200
From: Paolo Bonzini <pbonzini@redhat.com>
MIME-Version: 1.0
References: <1431530311-21647-1-git-send-email-yarygin@linux.vnet.ibm.com>	<55536C6B.4040400@redhat.com>
	<87vbfw77xb.fsf@linux.vnet.ibm.com>
In-Reply-To: <87vbfw77xb.fsf@linux.vnet.ibm.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] [PATCH] block: Let bdrv_drain_all() to call
 aio_poll() for each AioContext
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alexander Yarygin <yarygin@linux.vnet.ibm.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>, Christian Borntraeger <borntraeger@de.ibm.com>, Stefan Hajnoczi <stefanha@redhat.com>, qemu-devel@nongnu.org, qemu-block@nongnu.org



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