11.04.2019 17:15, Kevin Wolf wrote: > Am 11.04.2019 um 15:40 hat Vladimir Sementsov-Ogievskiy geschrieben: >> 25.02.2019 18:19, Kevin Wolf wrote: >>> bdrv_drain() must not leave connection_co scheduled, so bs->in_flight >>> needs to be increased while the coroutine is waiting to be scheduled >>> in the new AioContext after nbd_client_attach_aio_context(). >> >> Hi! >> >> I have some questions, could you explain, please? >> >> "bdrv_drain() must not leave connection_co scheduled" - it's because we want >> to be >> sure that connection_co yielded from nbd_read_eof, yes? >> >> But it is guaranteed by aio_wait_bh_oneshot.. Why do we need additioinally >> inc/dec >> bs->in_flight ? > > Without incrementing bs->in_flight, nothing would guarantee that > aio_poll() is called and the BH is actually executed before bdrv_drain() > returns.
Don't follow.. Don't we want exactly this, we want BH to be executed while node is still drained, as you write in comment? > >>> >>> Signed-off-by: Kevin Wolf <kw...@redhat.com> >>> --- >>> block/nbd-client.c | 20 ++++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/block/nbd-client.c b/block/nbd-client.c >>> index 60f38f0320..bfbaf7ebe9 100644 >>> --- a/block/nbd-client.c >>> +++ b/block/nbd-client.c >>> @@ -977,14 +977,30 @@ void nbd_client_detach_aio_context(BlockDriverState >>> *bs) >>> qio_channel_detach_aio_context(QIO_CHANNEL(client->ioc)); >>> } >>> >>> +static void nbd_client_attach_aio_context_bh(void *opaque) >>> +{ >>> + BlockDriverState *bs = opaque; >>> + NBDClientSession *client = nbd_get_client_session(bs); >>> + >>> + /* The node is still drained, so we know the coroutine has yielded in >>> + * nbd_read_eof(), the only place where bs->in_flight can reach 0, or >>> it is >>> + * entered for the first time. Both places are safe for entering the >>> + * coroutine.*/ >>> + qemu_aio_coroutine_enter(bs->aio_context, client->connection_co); >>> + bdrv_dec_in_flight(bs); >>> +} >>> + >>> void nbd_client_attach_aio_context(BlockDriverState *bs, >>> AioContext *new_context) >>> { >>> NBDClientSession *client = nbd_get_client_session(bs); >>> qio_channel_attach_aio_context(QIO_CHANNEL(client->ioc), >>> new_context); >>> >>> - /* FIXME Really need a bdrv_inc_in_flight() here */ >>> - aio_co_schedule(new_context, client->connection_co); >>> + bdrv_inc_in_flight(bs); >>> + >>> + /* Need to wait here for the BH to run because the BH must run while >>> the >>> + * node is still drained. */ >>> + aio_wait_bh_oneshot(new_context, nbd_client_attach_aio_context_bh, bs); >>> } >>> >>> void nbd_client_close(BlockDriverState *bs) >>> >> >> >> -- >> Best regards, >> Vladimir -- Best regards, Vladimir