On 2/18/19 10:18 AM, Kevin Wolf wrote:
> nbd_client_attach_aio_context() schedules connection_co in the new
> AioContext and this way reenters it in any arbitrary place that has
> yielded. We can restrict this a bit to the function call where the
> coroutine actually sits waiting when it's idle.
>
> This doesn't solve any bug yet, but it shows where in the code we need
> to support this random reentrance and where we don't have to care.
>
> Add FIXME comments for the existing bugs that the rest of this series
> will fix.
Wow, that's a lot of comments. Thanks for working on this.
>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> block/nbd-client.h | 1 +
> block/nbd-client.c | 23 +++++++++++++++++++++++
> 2 files changed, 24 insertions(+)
>
Reviewed-by: Eric Blake <ebl...@redhat.com>
> +++ b/block/nbd-client.c
> @@ -76,8 +76,24 @@ static coroutine_fn void nbd_connection_entry(void *opaque)
> Error *local_err = NULL;
>
> while (!s->quit) {
> + /*
> + * The NBD client can only really be considered idle when it has
> + * yielded from qio_channel_readv_all_eof(), waiting for data. This
> is
> + * the point where the additional scheduled coroutine entry happens
> + * after nbd_client_attach_aio_context().
> + *
> + * Therefore we keep an additional in_flight reference all the time
> and
> + * only drop it temporarily here.
> + *
> + * FIXME This is not safe because the QIOChannel could wake up the
> + * coroutine for a second time; it is not prepared for coroutine
> + * resumption from external code.
> + */
> + bdrv_dec_in_flight(s->bs);
> assert(s->reply.handle == 0);
> ret = nbd_receive_reply(s->ioc, &s->reply, &local_err);
> + bdrv_inc_in_flight(s->bs);
> +
> if (local_err) {
> trace_nbd_read_reply_entry_fail(ret,
> error_get_pretty(local_err));
> error_free(local_err);
> @@ -116,6 +132,8 @@ static coroutine_fn void nbd_connection_entry(void
> *opaque)
>
> s->quit = true;
> nbd_recv_coroutines_wake_all(s);
> + bdrv_dec_in_flight(s->bs);
> +
> s->connection_co = NULL;
> aio_wait_kick();
> }
> @@ -970,6 +988,9 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
> {
> 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, but the corresponding
> + * bdrv_dec_in_flight() would have to be in QIOChannel code :-/ */
> aio_co_schedule(new_context, client->connection_co);
> }
>
> @@ -1076,6 +1097,7 @@ static int nbd_client_connect(BlockDriverState *bs,
> * kick the reply mechanism. */
> qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
> client->connection_co = qemu_coroutine_create(nbd_connection_entry,
> client);
> + bdrv_inc_in_flight(bs);
> nbd_client_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
> logout("Established connection with NBD server\n");
> @@ -1108,6 +1130,7 @@ int nbd_client_init(BlockDriverState *bs,
> {
> NBDClientSession *client = nbd_get_client_session(bs);
>
> + client->bs = bs;
> qemu_co_mutex_init(&client->send_mutex);
> qemu_co_queue_init(&client->free_sema);
>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
