On Tue, Apr 12, 2022 at 09:42:01PM +0200, Paolo Bonzini wrote:
> The condition for waiting on the s->free_sema queue depends on
> both s->in_flight and s->state. The latter is currently using
> atomics, but this is quite dubious and probably wrong.
>
> Because s->state is written in the main thread too, for example by
> the reconnect timer callback, it cannot be protected by a CoMutex.
> Introduce a separate lock that can be used by nbd_co_send_request();
> later on this lock will also be used for s->state. There will not
> be any contention on the lock unless there is a reconnect, so this
> is not performance sensitive.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> block/nbd.c | 46 +++++++++++++++++++++++++++-------------------
> 1 file changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/block/nbd.c b/block/nbd.c
> index 0ff41cb914..c908ea6ae3 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -72,17 +72,22 @@ typedef struct BDRVNBDState {
> QIOChannel *ioc; /* The current I/O channel */
> NBDExportInfo info;
>
> - CoMutex send_mutex;
> + /*
> + * Protects free_sema, in_flight, requests[].coroutine,
> + * reconnect_delay_timer.
> + */
> + QemuMutex requests_lock;
> CoQueue free_sema;
> -
> - CoMutex receive_mutex;
> int in_flight;
> + NBDClientRequest requests[MAX_NBD_REQUESTS];
> + QEMUTimer *reconnect_delay_timer;
> +
> + CoMutex send_mutex;
> + CoMutex receive_mutex;
> NBDClientState state;
>
> - QEMUTimer *reconnect_delay_timer;
> QEMUTimer *open_timer;
>
> - NBDClientRequest requests[MAX_NBD_REQUESTS];
Reordering of the struct makes sense.
> @@ -468,11 +473,10 @@ static int coroutine_fn
> nbd_co_send_request(BlockDriverState *bs,
> BDRVNBDState *s = (BDRVNBDState *)bs->opaque;
> int rc, i = -1;
>
> - qemu_co_mutex_lock(&s->send_mutex);
> -
> + qemu_mutex_lock(&s->requests_lock);
> while (s->in_flight == MAX_NBD_REQUESTS ||
> (!nbd_client_connected(s) && s->in_flight > 0)) {
> - qemu_co_queue_wait(&s->free_sema, &s->send_mutex);
> + qemu_co_queue_wait(&s->free_sema, &s->requests_lock);
> }
>
> s->in_flight++;
> @@ -493,14 +497,14 @@ static int coroutine_fn
> nbd_co_send_request(BlockDriverState *bs,
> }
> }
>
> - g_assert(qemu_in_coroutine());
Why is this assert dropped? Is it because we've marked the function
with coroutine_fn? If so, should we drop it earlier in the series,
when you added the label?
Otherwise, the patch makes sense to me.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
