Re: [PATCH v5 3/5] block/nbd: refactor nbd_recv_coroutines_wake_all()

[Eric Blake <eblake@redhat.com>]

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 <vsementsov@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