Re: [Qemu-devel] [PATCH 2/4] nbd/client: do negotiation in coroutine

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4550 bytes --]

On 2/11/19 6:55 AM, Vladimir Sementsov-Ogievskiy wrote:
> As a first step to non-blocking negotiation, move it to coroutine.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c | 123 +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 109 insertions(+), 14 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index 10a52ad7d0..2ba2220a4a 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -859,13 +859,14 @@ static int nbd_list_meta_contexts(QIOChannel *ioc,
>   *          2: server is newstyle, but lacks structured replies
>   *          3: server is newstyle and set up for structured replies
>   */
> -static int nbd_start_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                               const char *hostname, QIOChannel **outioc,
> -                               bool structured_reply, bool *zeroes,
> -                               Error **errp)
> +static int coroutine_fn nbd_co_start_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, bool structured_reply, bool *zeroes, Error **errp)
>  {
>      uint64_t magic;
>  
> +    assert(qemu_in_coroutine());
> +

Do we need this assert, since this function is static, and only called by:

>      trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
>  
>      if (zeroes) {
> @@ -990,19 +991,20 @@ static int nbd_negotiate_finish_oldstyle(QIOChannel *ioc, NBDExportInfo *info,
>   * Returns: negative errno: failure talking to server
>   *          0: server is connected
>   */
> -int nbd_receive_negotiate(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> -                          const char *hostname, QIOChannel **outioc,
> -                          NBDExportInfo *info, Error **errp)
> +static int coroutine_fn nbd_co_receive_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
>  {
>      int result;
>      bool zeroes;
>      bool base_allocation = info->base_allocation;
>  
> +    assert(qemu_in_coroutine());
>      assert(info->name);
>      trace_nbd_receive_negotiate_name(info->name);
>  
> -    result = nbd_start_negotiate(ioc, tlscreds, hostname, outioc,
> -                                 info->structured_reply, &zeroes, errp);
> +    result = nbd_co_start_negotiate(ioc, tlscreds, hostname, outioc,
> +                                   info->structured_reply, &zeroes, errp);

other functions in the same file that have also made the same assertion?
 For that matter, does the assert() add any value over the fact that we
already annotated things as coroutine_fn?

I know that at some point, there was a proposal on the list for having
the compiler enforce coroutine_fn annotations (ensuring that a
coroutine_fn only calls other coroutine_fns, and that coroutines can
only be started on an entry point properly marked), but that's a
different task for another day.


> +static int nbd_receive_common(CoroutineEntry *entry,
> +                              NbdReceiveNegotiateCo *data)
> +{
> +    data->ret = -EINPROGRESS;
> +
> +    if (qemu_in_coroutine()) {
> +        entry(data);
> +    } else {
> +        AioContext *ctx = qio_channel_get_attached_aio_context(data->ioc);
> +        bool attach = !ctx;

There's where you used the function added in 1/4.

> +
> +        if (attach) {
> +            ctx = qemu_get_current_aio_context();
> +            qio_channel_attach_aio_context(data->ioc, ctx);
> +        }
> +
> +        qemu_aio_coroutine_enter(ctx, qemu_coroutine_create(entry, data));
> +        AIO_WAIT_WHILE(ctx, data->ret == -EINPROGRESS);
> +
> +        if (attach) {
> +            qio_channel_detach_aio_context(data->ioc);
> +        }
> +    }
> +
> +    return data->ret;
> +}

Looks reasonable.

> +
> +int nbd_receive_negotiate(
> +        QIOChannel *ioc, QCryptoTLSCreds *tlscreds, const char *hostname,
> +        QIOChannel **outioc, NBDExportInfo *info, Error **errp)
> +{

Why the reflowed parameter list, compared to its existing listing (as
shown by the - lines where you added nbd_receive_co_negotiate)? That's
only cosmetic, so I can live with it, but it seems like it makes the
diff slightly larger.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]