On Wed, Jul 14, 2021 at 07:59:14PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Split out nbd_recv_coroutine_wake(), as it will be used in separate.
s/in separate/separately/
> Also add a possibility to wake only first found sleeping coroutine.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
> ---
> block/nbd.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> +
> +static void nbd_recv_coroutines_wake_all(BDRVNBDState *s, bool only_one)
Without reading docs (including the parameter name), I would have guessed:
wake_all(s, true) - wakes all
wake_all(s, false) - wakes one
but your code does:
wake_all(s, true) - wakes one
wake_all(s, false) - wakes all
Maybe that means this function and/or its parameter is now misnamed.
Having the _all in the name with true to NOT be all is what threw me.
Would the following be any better:
nbd_recv_coroutines_wake(BDRVNBDState *s, bool all)
where
wake(s, true) - wakes all
wake(s, false) - wakes one
and where your helper function needs to be renamed, and callers
updated to match those semantics?
> {
> int i;
>
> for (i = 0; i < MAX_NBD_REQUESTS; i++) {
> - NBDClientRequest *req = &s->requests[i];
> -
> - if (req->coroutine && req->receiving) {
> - req->receiving = false;
> - aio_co_wake(req->coroutine);
> + if (nbd_recv_coroutine_wake(&s->requests[i]) && only_one) {
> + return;
But while I'm not sold on the naming, the change in logic (to be able
to wake any one waiter but not the whole list) looks useful for future
patches.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
