Dear Kevin: I notice checkpatch.pl complained about code style problems, you may want to replace `while (aio_poll(bs->aio_context, false));` with ``` while (aio_poll(bs->aio_context, false)) { /* No further work */ } ``` to suppress the complaints.
Best, Su Hang "Kevin Wolf" <kw...@redhat.com>wrote: > Commit 91af091f923 added an additional aio_poll() to BDRV_POLL_WHILE() > in order to make sure that all pending BHs are executed on drain. This > was the wrong place to make the fix, as it is useless overhead for all > other users of the macro and unnecessarily complicates the mechanism. > > This patch effectively reverts said commit (the context has changed a > bit and the code has moved to AIO_WAIT_WHILE()) and instead polls in the > loop condition for drain. > > The effect is probably hard to measure in any real-world use case > because actual I/O will dominate, but if I run only the initialisation > part of 'qemu-img convert' where it calls bdrv_block_status() for the > whole image to find out how much data there is copy, this phase actually > needs only roughly half the time after this patch. > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > --- > include/block/aio-wait.h | 22 ++++++++-------------- > block/io.c | 11 ++++++++++- > 2 files changed, 18 insertions(+), 15 deletions(-) > > diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h > index 8c90a2e66e..783d3678dd 100644 > --- a/include/block/aio-wait.h > +++ b/include/block/aio-wait.h > @@ -73,29 +73,23 @@ typedef struct { > */ > #define AIO_WAIT_WHILE(wait, ctx, cond) ({ \ > bool waited_ = false; \ > - bool busy_ = true; \ > AioWait *wait_ = (wait); \ > AioContext *ctx_ = (ctx); \ > if (in_aio_context_home_thread(ctx_)) { \ > - while ((cond) || busy_) { \ > - busy_ = aio_poll(ctx_, (cond)); \ > - waited_ |= !!(cond) | busy_; \ > + while ((cond)) { \ > + aio_poll(ctx_, true); \ > + waited_ = true; \ > } \ > } else { \ > assert(qemu_get_current_aio_context() == \ > qemu_get_aio_context()); \ > /* Increment wait_->num_waiters before evaluating cond. */ \ > atomic_inc(&wait_->num_waiters); \ > - while (busy_) { \ > - if ((cond)) { \ > - waited_ = busy_ = true; \ > - aio_context_release(ctx_); \ > - aio_poll(qemu_get_aio_context(), true); \ > - aio_context_acquire(ctx_); \ > - } else { \ > - busy_ = aio_poll(ctx_, false); \ > - waited_ |= busy_; \ > - } \ > + while ((cond)) { \ > + aio_context_release(ctx_); \ > + aio_poll(qemu_get_aio_context(), true); \ > + aio_context_acquire(ctx_); \ > + waited_ = true; \ > } \ > atomic_dec(&wait_->num_waiters); \ > } \ > diff --git a/block/io.c b/block/io.c > index ea6f9f023a..6f580f49ff 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -181,13 +181,22 @@ static void bdrv_drain_invoke(BlockDriverState *bs, > bool begin) > BDRV_POLL_WHILE(bs, !data.done); > } > > +/* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */ > +static bool bdrv_drain_poll(BlockDriverState *bs) > +{ > + /* Execute pending BHs first and check everything else only after the BHs > + * have executed. */ > + while (aio_poll(bs->aio_context, false)); > + return atomic_read(&bs->in_flight); > +} > + > static bool bdrv_drain_recurse(BlockDriverState *bs) > { > BdrvChild *child, *tmp; > bool waited; > > /* Wait for drained requests to finish */ > - waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0); > + waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll(bs)); > > QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) { > BlockDriverState *bs = child->bs; > -- > 2.13.6 >