Re: [Qemu-devel] [PATCH v3 07/13] nbd-server: simplify reply transmission

[Eric Blake <eblake@redhat.com>]

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

On 10/12/2017 04:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> Send qiov via qio_channel_writev_all instead of calling nbd_write twice
> with a cork.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/server.c | 50 ++++++++++++++++++++++++--------------------------
>  1 file changed, 24 insertions(+), 26 deletions(-)
> 

> @@ -1203,36 +1220,17 @@ static int nbd_co_send_simple_reply(NBDClient *client,
>                                      size_t len,
>                                      Error **errp)
>  {
> -    NBDSimpleReply simple_reply;
> -    int ret;
> -
> -    g_assert(qemu_in_coroutine());
> +    NBDSimpleReply reply;

Why the rename from simple_reply to reply?

> +    struct iovec iov[] = {
> +        {.iov_base = &reply, .iov_len = sizeof(reply)},

I guess it made this line shorter.  But we could reduce some churn by
naming it 'reply' in the first place, back in earlier patches.  At the
same time, I'm not going to bother if there's not a reason to respin the
series (or at least the first half).

> +        {.iov_base = data, .iov_len = len}
> +    };
>  
>      trace_nbd_co_send_simple_reply(handle, error, len);
>  
> -    set_be_simple_reply(&simple_reply, system_errno_to_nbd_errno(error),
> -                        handle);
> -
> -    qemu_co_mutex_lock(&client->send_lock);
> -    client->send_coroutine = qemu_coroutine_self();
> -
> -    if (!len) {
> -        ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL);
> -    } else {
> -        qio_channel_set_cork(client->ioc, true);
> -        ret = nbd_write(client->ioc, &simple_reply, sizeof(simple_reply), NULL);
> -        if (ret == 0) {
> -            ret = nbd_write(client->ioc, data, len, errp);
> -            if (ret < 0) {
> -                ret = -EIO;
> -            }
> -        }
> -        qio_channel_set_cork(client->ioc, false);
> -    }
> +    set_be_simple_reply(&reply, system_errno_to_nbd_errno(error), handle);
>  
> -    client->send_coroutine = NULL;
> -    qemu_co_mutex_unlock(&client->send_lock);
> -    return ret;
> +    return nbd_co_send_iov(client, iov, len ? 2 : 1, errp);

This part is definitely nicer!  Thanks for splitting the v2 patch, it
made review a lot more pleasant.

Reviewed-by: Eric Blake <eblake@redhat.com>

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


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