Re: [PATCH for-8.0] nbd/server: Request TCP_NODELAY

[Florian Westphal]

Eric Blake  wrote:
> Nagle's algorithm adds latency in order to reduce network packet
> overhead on small packets.  But when we are already using corking to
> merge smaller packets into transactional requests, the extra delay
> from TCP defaults just gets in the way.
> 
> For reference, qemu as an NBD client already requests TCP_NODELAY (see
> nbd_connect() in nbd/client-connection.c); as does libnbd as a client
> [1], and nbdkit as a server [2].
> 
> [1] 
> https://gitlab.com/nbdkit/libnbd/-/blob/a48a1142/generator/states-connect.c#L39
> [2] https://gitlab.com/nbdkit/nbdkit/-/blob/45b72f5b/server/sockets.c#L430
> 
> CC: Florian Westphal 
> Signed-off-by: Eric Blake 
> ---
>  nbd/server.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index a4750e41880..976223860bf 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2755,6 +2755,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
>  }
>  client->tlsauthz = g_strdup(tlsauthz);
>  client->sioc = sioc;
> +qio_channel_set_delay(QIO_CHANNEL(cioc), false);

../nbd/server.c: In function 'nbd_client_new':
../nbd/server.c:2763:39: error: 'cioc' undeclared (first use in this function); 
did you mean 'sioc'?

Other than that this looks good to me.

Re: [Libguestfs] [PATCH 1/1] nbd/server: push pending frames after sending reply

[Florian Westphal]

Eric Blake  wrote:
> On Fri, Mar 24, 2023 at 02:41:20PM -0500, Eric Blake wrote:
> > On Fri, Mar 24, 2023 at 11:47:20AM +0100, Florian Westphal wrote:
> > > qemu-nbd doesn't set TCP_NODELAY on the tcp socket.
> 
> Replying to myself, WHY aren't we setting TCP_NODELAY on the socket?

If the application protocol is strictly or mostly request/response then
I agree that qemu-nbd should turn nagle off as well.

Re: [PATCH 1/1] nbd/server: push pending frames after sending reply

[Florian Westphal]

Kevin Wolf  wrote:
> Am 24.03.2023 um 11:47 hat Florian Westphal geschrieben:
> > +qio_channel_set_cork(client->ioc, true);
> > +
> >  if (ret < 0) {
> >  /* It wasn't -EIO, so, according to nbd_co_receive_request()
> >   * semantics, we should return the error to the client. */
> > @@ -2692,6 +2694,7 @@ static coroutine_fn void nbd_trip(void *opaque)
> >  goto disconnect;
> >  }
> >  
> > +qio_channel_set_cork(client->ioc, false);
> >  done:
> >  nbd_request_put(req);
> >  nbd_client_put(client);
> 
> In the error paths, we never call set_cork(false) again. I suppose the
> reason that this is okay is because the next thing is actually that we
> close the socket?

Yes, no need to uncork before close.

[PATCH 1/1] nbd/server: push pending frames after sending reply

[Florian Westphal]

qemu-nbd doesn't set TCP_NODELAY on the tcp socket.

Kernel waits for more data and avoids transmission of small packets.
Without TLS this is barely noticeable, but with TLS this really shows.

Booting a VM via qemu-nbd on localhost (with tls) takes more than
2 minutes on my system.  tcpdump shows frequent wait periods, where no
packets get sent for a 40ms period.

Add explicit (un)corking when processing (and responding to) requests.
"TCP_CORK, &zero" after earlier "CORK, &one" will flush pending data.

VM Boot time:
main:no tls:  23s, with tls: 2m45s
patched: no tls:  14s, with tls: 15s

VM Boot time, qemu-nbd via network (same lan):
main:no tls:  18s, with tls: 1m50s
patched: no tls:  17s, with tls: 18s

Future optimization: if we could detect if there is another pending
request we could defer the uncork operation because more data would be
appended.

Signed-off-by: Florian Westphal 
---
 nbd/server.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/nbd/server.c b/nbd/server.c
index a4750e41880a..848836d41405 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2667,6 +2667,8 @@ static coroutine_fn void nbd_trip(void *opaque)
 goto disconnect;
 }
 
+qio_channel_set_cork(client->ioc, true);
+
 if (ret < 0) {
 /* It wasn't -EIO, so, according to nbd_co_receive_request()
  * semantics, we should return the error to the client. */
@@ -2692,6 +2694,7 @@ static coroutine_fn void nbd_trip(void *opaque)
 goto disconnect;
 }
 
+qio_channel_set_cork(client->ioc, false);
 done:
 nbd_request_put(req);
 nbd_client_put(client);
-- 
2.39.2