[Libguestfs] [PATCH libnbd 2/3] states: Add handle h->wflags field.

[Eric Blake]

On 6/9/19 1:14 PM, Richard W.M. Jones wrote:
> 
> There's an obvious bug in this patch in that it doesn't reset the
> h->wflags field in all cases.  Updated patch below.
> 
> I rechecked the performance measurements and they are the same after
> the updated patch.
> 
> Rich.
> 
>>From 15a687b50acecebcfd3dc6222d93e6df984b83c6 Mon Sep 17 00:00:00 2001
> From: "Richard W.M. Jones" <rjones at redhat.com>
> Date: Sat, 8 Jun 2019 19:12:22 +0100
> Subject: [PATCH] states: Add handle h->wflags field.
> 
> This field contains optimization flags (ie. MSG_MORE) which are passed
> through to the socket layer if it supports them.  The flags are reset
> automatically when we move to another state.
> ---
>  generator/states.c | 10 +++++++---
>  lib/internal.h     |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/generator/states.c b/generator/states.c
> index e879a83..b0dab83 100644
> --- a/generator/states.c
> +++ b/generator/states.c
> @@ -91,8 +91,8 @@ send_from_wbuf (struct nbd_handle *h)
>    ssize_t r;
>  
>    if (h->wlen == 0)
> -    return 0;                   /* move to next state */
> -  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen, 0);
> +    goto next_state;
> +  r = h->sock->ops->send (h, h->sock, h->wbuf, h->wlen, h->wflags);
>    if (r == -1) {
>      if (errno == EAGAIN || errno == EWOULDBLOCK)
>        return 1;                 /* more data */
> @@ -102,9 +102,13 @@ send_from_wbuf (struct nbd_handle *h)
>    h->wbuf += r;
>    h->wlen -= r;
>    if (h->wlen == 0)
> -    return 0;                   /* move to next state */
> +    goto next_state;
>    else
>      return 1;                   /* more data */

Hmm. If we blocked, do we still want to use MSG_MORE on the next
resumption of send()?  Or should we blindly clear h->wflags as soon as
we block as well as when we move to the next state?

In practice, will we ever block when MSG_MORE is set? Ideally, we won't
overflow a TCP segment except on the final payload portion of
NBD_CMD_WRITE. Or are you hoping to set MSG_MORE even on other commands,
if there is a request queue, so that we can bundle multiple commands
into a single TCP segment?

Okay, I think that it's okay to always replay MSG_MORE when resuming a
write() that blocked, even if we'll never hit that situation unless we
rework a caller to pass MSG_MORE even for the last byte of one request
when we know the request queue is non-empty so another request is
immediately ready to also transmit.

> +
> + next_state:
> +  h->wflags = 0;                /* reset this when moving to next state */
> +  return 0;                     /* move to next state */
>  }
>  
>  /*----- End of prologue. -----*/
> diff --git a/lib/internal.h b/lib/internal.h
> index 1ffb5b7..e7be05b 100644
> --- a/lib/internal.h
> +++ b/lib/internal.h
> @@ -110,6 +110,7 @@ struct nbd_handle {
>    /* As above, but for writing using send_from_wbuf. */
>    const void *wbuf;
>    size_t wlen;
> +  int wflags;
>  
>    /* Static buffer used for short amounts of data, such as handshake
>     * and commands.
> 

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190610/7c2d572c/attachment.sig>