diff mbox series

[v2,for,8.0?] nbd/server: Request TCP_NODELAY

Message ID 20230404004047.142086-1-eblake@redhat.com
State New
Headers show
Series [v2,for,8.0?] nbd/server: Request TCP_NODELAY | expand

Commit Message

Eric Blake April 4, 2023, 12:40 a.m. UTC
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 (see recent commit bd2cd4a4).

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].  Furthermore, the NBD spec recommends
the use of TCP_NODELAY [3].

[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
[3] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md#protocol-phases

CC: Florian Westphal <fw@strlen.de>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20230327192947.1324372-1-eblake@redhat.com>
---

v2 fix typo, enhance commit message

Given that corking made it in through Kevin's tree for 8.0-rc2 but
this one did not, but I didn't get any R-b, is there any objection to
me doing a pull request to get this into 8.0-rc3?

 nbd/server.c | 1 +
 1 file changed, 1 insertion(+)


base-commit: efcd0ec14b0fe9ee0ee70277763b2d538d19238d

Comments

Philippe Mathieu-Daudé April 4, 2023, 8:11 a.m. UTC | #1
On 4/4/23 02:40, 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 (see recent commit bd2cd4a4).
> 
> 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].  Furthermore, the NBD spec recommends
> the use of TCP_NODELAY [3].
> 
> [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
> [3] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md#protocol-phases
> 
> CC: Florian Westphal <fw@strlen.de>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20230327192947.1324372-1-eblake@redhat.com>
> ---
> 
> v2 fix typo, enhance commit message
> 
> Given that corking made it in through Kevin's tree for 8.0-rc2 but
> this one did not, but I didn't get any R-b, is there any objection to
> me doing a pull request to get this into 8.0-rc3?

FWIW, no objection.

>   nbd/server.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Paolo Bonzini April 4, 2023, 1:49 p.m. UTC | #2
On 4/4/23 02:40, 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 (see recent commit bd2cd4a4).
> 
> 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].  Furthermore, the NBD spec recommends
> the use of TCP_NODELAY [3].
> 
> [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
> [3] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/proto.md#protocol-phases
> 
> CC: Florian Westphal <fw@strlen.de>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20230327192947.1324372-1-eblake@redhat.com>
> ---
> 
> v2 fix typo, enhance commit message
> 
> Given that corking made it in through Kevin's tree for 8.0-rc2 but
> this one did not, but I didn't get any R-b, is there any objection to
> me doing a pull request to get this into 8.0-rc3?
> 
>   nbd/server.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index 848836d4140..3d8d0d81df2 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2758,6 +2758,7 @@ void nbd_client_new(QIOChannelSocket *sioc,
>       }
>       client->tlsauthz = g_strdup(tlsauthz);
>       client->sioc = sioc;
> +    qio_channel_set_delay(QIO_CHANNEL(sioc), false);
>       object_ref(OBJECT(client->sioc));
>       client->ioc = QIO_CHANNEL(sioc);
>       object_ref(OBJECT(client->ioc));
> 
> base-commit: efcd0ec14b0fe9ee0ee70277763b2d538d19238d

Acked-by: Paolo Bonzini <pbonzini@redhat.com>
diff mbox series

Patch

diff --git a/nbd/server.c b/nbd/server.c
index 848836d4140..3d8d0d81df2 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2758,6 +2758,7 @@  void nbd_client_new(QIOChannelSocket *sioc,
     }
     client->tlsauthz = g_strdup(tlsauthz);
     client->sioc = sioc;
+    qio_channel_set_delay(QIO_CHANNEL(sioc), false);
     object_ref(OBJECT(client->sioc));
     client->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(client->ioc));