On Tue, Apr 12, 2022 at 09:42:00PM +0200, Paolo Bonzini wrote:
> Elevate s->in_flight early so that other incoming requests will wait
> on the CoQueue in nbd_co_send_request; restart them after getting back
> from nbd_reconnect_attempt. This could be after the reconnect timer or
> nbd_cancel_in_flight have cancelled the attempt, so there is no
> need anymore to cancel the requests there.
>
> nbd_co_send_request now handles both stopping and restarting pending
> requests after a successful connection, and there is no need to
> hold send_mutex in nbd_co_do_establish_connection. The current setup
> is confusing because nbd_co_do_establish_connection is called both with
> send_mutex taken and without it. Before the patch it uses free_sema which
> (at least in theory...) is protected by send_mutex, after the patch it
> does not anymore.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> block/coroutines.h | 4 +--
> block/nbd.c | 66 ++++++++++++++++++++++------------------------
> 2 files changed, 33 insertions(+), 37 deletions(-)
>
> +++ b/block/nbd.c
> @@ -359,25 +354,25 @@ int coroutine_fn
> nbd_co_do_establish_connection(BlockDriverState *bs,
> /* called under s->send_mutex */
> static coroutine_fn void nbd_reconnect_attempt(BDRVNBDState *s)
> {
> - assert(nbd_client_connecting(s));
> - assert(s->in_flight == 0);
> -
> - if (nbd_client_connecting_wait(s) && s->reconnect_delay &&
> - !s->reconnect_delay_timer)
> - {
> - /*
> - * It's first reconnect attempt after switching to
> - * NBD_CLIENT_CONNECTING_WAIT
> - */
> - reconnect_delay_timer_init(s,
> - qemu_clock_get_ns(QEMU_CLOCK_REALTIME) +
> - s->reconnect_delay * NANOSECONDS_PER_SECOND);
> - }
> + bool blocking = nbd_client_connecting_wait(s);
>
> /*
> * Now we are sure that nobody is accessing the channel, and no one will
> * try until we set the state to CONNECTED.
> */
> + assert(nbd_client_connecting(s));
> + assert(s->in_flight == 1);
> +
> + if (blocking && !s->reconnect_delay_timer) {
> + /*
> + * It's first reconnect attempt after switching to
While moving this, we could add the missing article: "It's the first"
> + * NBD_CLIENT_CONNECTING_WAIT
> + */
> + g_assert(s->reconnect_delay);
> + reconnect_delay_timer_init(s,
> + qemu_clock_get_ns(QEMU_CLOCK_REALTIME) +
> + s->reconnect_delay * NANOSECONDS_PER_SECOND);
> + }
>
> /* Finalize previous connection if any */
> if (s->ioc) {
> @@ -388,7 +383,9 @@ static coroutine_fn void
> nbd_reconnect_attempt(BDRVNBDState *s)
> s->ioc = NULL;
> }
>
> - nbd_co_do_establish_connection(s->bs, NULL);
> + qemu_co_mutex_unlock(&s->send_mutex);
> + nbd_co_do_establish_connection(s->bs, blocking, NULL);
> + qemu_co_mutex_lock(&s->send_mutex);
>
> /*
> * The reconnect attempt is done (maybe successfully, maybe not), so
> @@ -474,21 +471,21 @@ static int coroutine_fn
> nbd_co_send_request(BlockDriverState *bs,
> qemu_co_mutex_lock(&s->send_mutex);
>
> while (s->in_flight == MAX_NBD_REQUESTS ||
> - (!nbd_client_connected(s) && s->in_flight > 0))
> - {
> + (!nbd_client_connected(s) && s->in_flight > 0)) {
Mixing in a style change here. Not the end of the world.
The cosmetics are trivial, and the real change of enlarging the scope
of in_flight makes sense to me.
Reviewed-by: Eric Blake <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
