Re: [Qemu-devel] [PATCH v3 18/19] block: Use a single global AioWait

[Eric Blake]

On 9/20/18 11:19 AM, Kevin Wolf wrote:
> When draining a block node, we recurse to its parent and for subtree
> drains also to its children. A single AIO_WAIT_WHILE() is then used to
> wait for bdrv_drain_poll() to become true, which depends on all of the
> nodes we recursed to. However, if the respective child or parent becomes
> quiescent and calls bdrv_wakeup(), only the AioWait of the child/parent
> is checked, while AIO_WAIT_WHILE() depends on the AioWait of the
> original node.
>
> Fix this by using a single AioWait for all callers of AIO_WAIT_WHILE().
>
> This may mean that the draining thread gets a few more unnecessary
> wakeups because an unrelated operation got completed, but we already
> wake it up when something _could_ have changed rather than only if it
> has certainly changed.

Ah, the classic case of allowing spurious wakeups in order to make 
everything else in the system easier.

> Apart from that, drain is a slow path anyway. In theory it would be
> possible to use wakeups more selectively and still correctly, but the
> gains are likely not worth the additional complexity. In fact, this
> patch is a nice simplification for some places in the code.

I agree with the analysis.

> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>   include/block/aio-wait.h  | 11 +++++------
>   include/block/block.h     |  6 +-----
>   include/block/block_int.h |  3 ---
>   include/block/blockjob.h  | 10 ----------
>   block.c                   |  5 -----
>   block/block-backend.c     | 11 ++++-------
>   block/io.c                |  7 ++-----
>   blockjob.c                | 13 +------------
>   job.c                     |  3 +--
>   util/aio-wait.c           | 11 ++++++-----
>   10 files changed, 20 insertions(+), 60 deletions(-)
>
> diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
> index 600fad183e..46f86f948b 100644
> --- a/include/block/aio-wait.h
> +++ b/include/block/aio-wait.h
> @@ -54,9 +54,10 @@ typedef struct {
>       unsigned num_waiters;
>   } AioWait;
>   
> +extern AioWait global_aio_wait;
> +

Documentation at the top of this file also needs updating:

 30 /**
 31  * AioWait:
 32  *
 33  * An object that facilitates synchronous waiting on a condition. 
The main
 34  * loop can wait on an operation running in an IOThread as follows:
 35  *
 36  *   AioWait *wait = ...;
 37  *   AioContext *ctx = ...;
 38  *   MyWork work = { .done = false };
 39  *   schedule_my_work_in_iothread(ctx, &work);
 40  *   AIO_WAIT_WHILE(wait, ctx, !work.done);
 41  *

As that does not affect code,
Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org