From 486574483aba988c83b20e7d3f1ccd50c4c333d8 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Wed, 5 Sep 2018 18:14:17 +0200 Subject: aio-wait: Increase num_waiters even in home thread Even if AIO_WAIT_WHILE() is called in the home context of the AioContext, we still want to allow the condition to change depending on other threads as long as they kick the AioWait. Specfically block jobs can be running in an I/O thread and should then be able to kick a drain in the main loop context. Signed-off-by: Kevin Wolf Reviewed-by: Fam Zheng --- include/block/aio-wait.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'include/block/aio-wait.h') diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h index c85a62f798..600fad183e 100644 --- a/include/block/aio-wait.h +++ b/include/block/aio-wait.h @@ -76,6 +76,8 @@ typedef struct { bool waited_ = false; \ AioWait *wait_ = (wait); \ AioContext *ctx_ = (ctx); \ + /* Increment wait_->num_waiters before evaluating cond. */ \ + atomic_inc(&wait_->num_waiters); \ if (ctx_ && in_aio_context_home_thread(ctx_)) { \ while ((cond)) { \ aio_poll(ctx_, true); \ @@ -84,8 +86,6 @@ typedef struct { } else { \ assert(qemu_get_current_aio_context() == \ qemu_get_aio_context()); \ - /* Increment wait_->num_waiters before evaluating cond. */ \ - atomic_inc(&wait_->num_waiters); \ while ((cond)) { \ if (ctx_) { \ aio_context_release(ctx_); \ @@ -96,8 +96,8 @@ typedef struct { } \ waited_ = true; \ } \ - atomic_dec(&wait_->num_waiters); \ } \ + atomic_dec(&wait_->num_waiters); \ waited_; }) /** -- cgit v1.2.3 From cfe29d8294e06420e15d4938421ae006c8ac49e7 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 18 Sep 2018 17:09:16 +0200 Subject: block: Use a single global AioWait 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. 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. Signed-off-by: Kevin Wolf Reviewed-by: Eric Blake Reviewed-by: Max Reitz --- include/block/aio-wait.h | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) (limited to 'include/block/aio-wait.h') diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h index 600fad183e..afd0ff7eb8 100644 --- a/include/block/aio-wait.h +++ b/include/block/aio-wait.h @@ -30,14 +30,15 @@ /** * AioWait: * - * An object that facilitates synchronous waiting on a condition. The main - * loop can wait on an operation running in an IOThread as follows: + * An object that facilitates synchronous waiting on a condition. A single + * global AioWait object (global_aio_wait) is used internally. + * + * The main loop can wait on an operation running in an IOThread as follows: * - * AioWait *wait = ...; * AioContext *ctx = ...; * MyWork work = { .done = false }; * schedule_my_work_in_iothread(ctx, &work); - * AIO_WAIT_WHILE(wait, ctx, !work.done); + * AIO_WAIT_WHILE(ctx, !work.done); * * The IOThread must call aio_wait_kick() to notify the main loop when * work.done changes: @@ -46,7 +47,7 @@ * { * ... * work.done = true; - * aio_wait_kick(wait); + * aio_wait_kick(); * } */ typedef struct { @@ -54,9 +55,10 @@ typedef struct { unsigned num_waiters; } AioWait; +extern AioWait global_aio_wait; + /** * AIO_WAIT_WHILE: - * @wait: the aio wait object * @ctx: the aio context, or NULL if multiple aio contexts (for which the * caller does not hold a lock) are involved in the polling condition. * @cond: wait while this conditional expression is true @@ -72,9 +74,9 @@ typedef struct { * wait on conditions between two IOThreads since that could lead to deadlock, * go via the main loop instead. */ -#define AIO_WAIT_WHILE(wait, ctx, cond) ({ \ +#define AIO_WAIT_WHILE(ctx, cond) ({ \ bool waited_ = false; \ - AioWait *wait_ = (wait); \ + AioWait *wait_ = &global_aio_wait; \ AioContext *ctx_ = (ctx); \ /* Increment wait_->num_waiters before evaluating cond. */ \ atomic_inc(&wait_->num_waiters); \ @@ -102,14 +104,12 @@ typedef struct { /** * aio_wait_kick: - * @wait: the aio wait object that should re-evaluate its condition - * * Wake up the main thread if it is waiting on AIO_WAIT_WHILE(). During * synchronous operations performed in an IOThread, the main thread lets the * IOThread's event loop run, waiting for the operation to complete. A * aio_wait_kick() call will wake up the main thread. */ -void aio_wait_kick(AioWait *wait); +void aio_wait_kick(void); /** * aio_wait_bh_oneshot: -- cgit v1.2.3