[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 13/16] block: only call aio_poll from iothread
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH 13/16] block: only call aio_poll from iothread |
Date: |
Wed, 9 Mar 2016 16:30:49 +0800 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Tue, 02/16 18:56, Paolo Bonzini wrote:
> aio_poll is not thread safe; for example bdrv_drain can hang if
> the last in-flight I/O operation is completed in the I/O thread after
> the main thread has checked bs->in_flight.
>
> The bug remains latent as long as all of it is called within
> aio_context_acquire/aio_context_release, but this will change soon.
>
> To fix this, if bdrv_drain is called from outside the I/O thread handle
> it internally in the BDS, without involving AioContext and aio_poll.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> block.c | 2 ++
> block/io.c | 21 ++++++++++++++++++---
> include/block/block_int.h | 5 ++++-
> 3 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index fb02d7f..601a73f 100644
> --- a/block.c
> +++ b/block.c
> @@ -267,6 +267,7 @@ BlockDriverState *bdrv_new(void)
> qemu_co_queue_init(&bs->throttled_reqs[1]);
> bs->refcnt = 1;
> bs->aio_context = qemu_get_aio_context();
> + qemu_event_init(&bs->in_flight_event, true);
>
> QTAILQ_INSERT_TAIL(&all_bdrv_states, bs, bs_list);
>
> @@ -2395,6 +2396,7 @@ static void bdrv_delete(BlockDriverState *bs)
> bdrv_make_anon(bs);
>
> QTAILQ_REMOVE(&all_bdrv_states, bs, bs_list);
> + qemu_event_destroy(&bs->in_flight_event);
>
> g_free(bs);
> }
> diff --git a/block/io.c b/block/io.c
> index 04b52c8..ea0546f 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -251,11 +251,24 @@ static void bdrv_drain_recurse(BlockDriverState *bs)
>
> static bool bdrv_drain_io_recurse(BlockDriverState *bs)
> {
> - BdrvChild *child;
> + AioContext *ctx = bdrv_get_aio_context(bs);
> bool waited = false;
> + BdrvChild *child;
>
> while (atomic_read(&bs->in_flight) > 0) {
> - aio_poll(bdrv_get_aio_context(bs), true);
> + if (aio_context_in_iothread(ctx)) {
> + /* This case should not occur at all, except for the
> + * main thread.
> + */
Maybe assert ctx == qemu_get_aio_context()?
> + aio_poll(bdrv_get_aio_context(bs), true);
> + } else {
> + qemu_event_reset(&bs->in_flight_event);
> + if (atomic_read(&bs->in_flight) > 0) {
> + aio_context_release(bdrv_get_aio_context(bs));
> + qemu_event_wait(&bs->in_flight_event);
> + aio_context_acquire(bdrv_get_aio_context(bs));
> + }
> + }
> waited = true;
> }
>
> @@ -465,7 +478,9 @@ void bdrv_inc_in_flight(BlockDriverState *bs)
>
> void bdrv_dec_in_flight(BlockDriverState *bs)
> {
> - atomic_dec(&bs->in_flight);
> + if (atomic_fetch_dec(&bs->in_flight) == 1) {
> + qemu_event_set(&bs->in_flight_event);
> + }
> }
>
> static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 89c38c0..9c96d5d 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -404,9 +404,12 @@ struct BlockDriverState {
> /* Callback before write request is processed */
> NotifierWithReturnList before_write_notifiers;
>
> - /* number of in-flight requests; overall and serialising */
> + /* number of in-flight requests; overall and serialising.
> + * in_flight_event is set when in_flight becomes 0.
> + */
> unsigned int in_flight;
> unsigned int serialising_in_flight;
> + QemuEvent in_flight_event;
>
> /* I/O throttling.
> * throttle_state tells us if this BDS has I/O limits configured.
> --
> 2.5.0
>
>
>
- Re: [Qemu-devel] [PATCH 13/16] block: only call aio_poll from iothread,
Fam Zheng <=