| Message ID | 20210209152759.209074-2-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | NBD socket backlog | expand |
On Tue, Feb 09, 2021 at 09:27:58AM -0600, Eric Blake wrote: > Our default of a backlog of 1 connection is rather puny; it gets in > the way when we are explicitly allowing multiple clients (such as > qemu-nbd -e N [--shared], or nbd-server-start with its default > "max-connections":0 for unlimited), but is even a problem when we > stick to qemu-nbd's default of only 1 active client but use -t > [--persistent] where a second client can start using the server once > the first finishes. While the effects are less noticeable on TCP > sockets (since the client can poll() to learn when the server is ready > again), it is definitely observable on Unix sockets, where on Unix, a > client will fail with EAGAIN and no recourse but to sleep an arbitrary > amount of time before retrying if the server backlog is already full. > > Since QMP nbd-server-start is always persistent, it now always > requests a backlog of SOMAXCONN; meanwhile, qemu-nbd will request > SOMAXCONN if persistent, otherwise its backlog should be based on the > expected number of clients. > > See https://bugzilla.redhat.com/1925045 for a demonstration of where > our low backlog prevents libnbd from connecting as many parallel > clients as it wants. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > CC: qemu-stable@nongnu.org > --- > blockdev-nbd.c | 7 ++++++- > qemu-nbd.c | 10 +++++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index d8443d235b73..b264620b98d8 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -134,7 +134,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, > qio_net_listener_set_name(nbd_server->listener, > "nbd-listener"); > > - if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) { > + /* > + * Because this server is persistent, a backlog of SOMAXCONN is > + * better than trying to size it to max_connections. > + */ > + if (qio_net_listener_open_sync(nbd_server->listener, addr, SOMAXCONN, > + errp) < 0) { > goto error; > } > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 608c63e82a25..1a340ea4858d 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -964,8 +964,16 @@ int main(int argc, char **argv) > > server = qio_net_listener_new(); > if (socket_activation == 0) { > + int backlog; > + > + if (persistent) { > + backlog = SOMAXCONN; > + } else { > + backlog = MIN(shared, SOMAXCONN); > + } > saddr = nbd_build_socket_address(sockpath, bindto, port); > - if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) { > + if (qio_net_listener_open_sync(server, saddr, backlog, > + &local_err) < 0) { > object_unref(OBJECT(server)); > error_report_err(local_err); > exit(EXIT_FAILURE); Works fine here, so: Tested-by: Richard W.M. Jones <rjones@redhat.com> Rich.
On 2/9/21 10:08 AM, Richard W.M. Jones wrote: > On Tue, Feb 09, 2021 at 09:27:58AM -0600, Eric Blake wrote: >> Our default of a backlog of 1 connection is rather puny; it gets in >> the way when we are explicitly allowing multiple clients (such as >> qemu-nbd -e N [--shared], or nbd-server-start with its default >> "max-connections":0 for unlimited), but is even a problem when we >> stick to qemu-nbd's default of only 1 active client but use -t >> [--persistent] where a second client can start using the server once >> the first finishes. While the effects are less noticeable on TCP >> sockets (since the client can poll() to learn when the server is ready >> again), it is definitely observable on Unix sockets, where on Unix, a s/where on Unix/where on Linux/ >> client will fail with EAGAIN and no recourse but to sleep an arbitrary >> amount of time before retrying if the server backlog is already full. >> >> Since QMP nbd-server-start is always persistent, it now always >> requests a backlog of SOMAXCONN; meanwhile, qemu-nbd will request >> SOMAXCONN if persistent, otherwise its backlog should be based on the >> expected number of clients. >> >> See https://bugzilla.redhat.com/1925045 for a demonstration of where >> our low backlog prevents libnbd from connecting as many parallel >> clients as it wants. >> >> Reported-by: Richard W.M. Jones <rjones@redhat.com> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> CC: qemu-stable@nongnu.org >> --- >> blockdev-nbd.c | 7 ++++++- >> qemu-nbd.c | 10 +++++++++- >> 2 files changed, 15 insertions(+), 2 deletions(-) >> > > Works fine here, so: > > Tested-by: Richard W.M. Jones <rjones@redhat.com> > Thanks for testing. > Rich. >
On Tue, Feb 09, 2021 at 09:27:58AM -0600, Eric Blake wrote: > Our default of a backlog of 1 connection is rather puny; it gets in > the way when we are explicitly allowing multiple clients (such as > qemu-nbd -e N [--shared], or nbd-server-start with its default > "max-connections":0 for unlimited), but is even a problem when we > stick to qemu-nbd's default of only 1 active client but use -t > [--persistent] where a second client can start using the server once > the first finishes. While the effects are less noticeable on TCP > sockets (since the client can poll() to learn when the server is ready > again), it is definitely observable on Unix sockets, where on Unix, a > client will fail with EAGAIN and no recourse but to sleep an arbitrary > amount of time before retrying if the server backlog is already full. > > Since QMP nbd-server-start is always persistent, it now always > requests a backlog of SOMAXCONN; meanwhile, qemu-nbd will request > SOMAXCONN if persistent, otherwise its backlog should be based on the > expected number of clients. > > See https://bugzilla.redhat.com/1925045 for a demonstration of where > our low backlog prevents libnbd from connecting as many parallel > clients as it wants. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > CC: qemu-stable@nongnu.org > --- > blockdev-nbd.c | 7 ++++++- > qemu-nbd.c | 10 +++++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel
On Tue, Feb 9, 2021 at 5:28 PM Eric Blake <eblake@redhat.com> wrote: > > Our default of a backlog of 1 connection is rather puny; it gets in > the way when we are explicitly allowing multiple clients (such as > qemu-nbd -e N [--shared], or nbd-server-start with its default > "max-connections":0 for unlimited), but is even a problem when we > stick to qemu-nbd's default of only 1 active client but use -t > [--persistent] where a second client can start using the server once > the first finishes. While the effects are less noticeable on TCP > sockets (since the client can poll() to learn when the server is ready > again), it is definitely observable on Unix sockets, where on Unix, a > client will fail with EAGAIN and no recourse but to sleep an arbitrary > amount of time before retrying if the server backlog is already full. > > Since QMP nbd-server-start is always persistent, it now always > requests a backlog of SOMAXCONN; This makes sense since we don't limit the number of connections. > meanwhile, qemu-nbd will request > SOMAXCONN if persistent, otherwise its backlog should be based on the > expected number of clients. If --persistent is used without --shared, we allow only one concurrent connection, so not clear why we need maximum backlog. I think that separating --persistent and --shared would be easier to understand and use. The backlog will always be based on shared value. > See https://bugzilla.redhat.com/1925045 for a demonstration of where > our low backlog prevents libnbd from connecting as many parallel > clients as it wants. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Eric Blake <eblake@redhat.com> > CC: qemu-stable@nongnu.org > --- > blockdev-nbd.c | 7 ++++++- > qemu-nbd.c | 10 +++++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index d8443d235b73..b264620b98d8 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -134,7 +134,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, > qio_net_listener_set_name(nbd_server->listener, > "nbd-listener"); > > - if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) { > + /* > + * Because this server is persistent, a backlog of SOMAXCONN is > + * better than trying to size it to max_connections. The comment is not clear. Previously we used hard code value (1) but we do support more than one connection. Maybe it is better to explain that we don't know how many connections are needed? > + */ > + if (qio_net_listener_open_sync(nbd_server->listener, addr, SOMAXCONN, > + errp) < 0) { > goto error; > } > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 608c63e82a25..1a340ea4858d 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -964,8 +964,16 @@ int main(int argc, char **argv) > > server = qio_net_listener_new(); > if (socket_activation == 0) { > + int backlog; > + > + if (persistent) { > + backlog = SOMAXCONN; This increases the backlog, but since default shared is still 1, we will not accept more than 1 connection, so not clear why SOMAXCONN is better. > + } else { > + backlog = MIN(shared, SOMAXCONN); > + } > saddr = nbd_build_socket_address(sockpath, bindto, port); > - if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) { > + if (qio_net_listener_open_sync(server, saddr, backlog, > + &local_err) < 0) { > object_unref(OBJECT(server)); > error_report_err(local_err); > exit(EXIT_FAILURE); > -- > 2.30.0 >
On 2/10/21 10:58 AM, Nir Soffer wrote: > On Tue, Feb 9, 2021 at 5:28 PM Eric Blake <eblake@redhat.com> wrote: >> >> Our default of a backlog of 1 connection is rather puny; it gets in >> the way when we are explicitly allowing multiple clients (such as >> qemu-nbd -e N [--shared], or nbd-server-start with its default >> "max-connections":0 for unlimited), but is even a problem when we >> stick to qemu-nbd's default of only 1 active client but use -t >> [--persistent] where a second client can start using the server once >> the first finishes. While the effects are less noticeable on TCP >> sockets (since the client can poll() to learn when the server is ready >> again), it is definitely observable on Unix sockets, where on Unix, a >> client will fail with EAGAIN and no recourse but to sleep an arbitrary >> amount of time before retrying if the server backlog is already full. >> >> Since QMP nbd-server-start is always persistent, it now always >> requests a backlog of SOMAXCONN; > > This makes sense since we don't limit the number of connections. > >> meanwhile, qemu-nbd will request >> SOMAXCONN if persistent, otherwise its backlog should be based on the >> expected number of clients. > > If --persistent is used without --shared, we allow only one concurrent > connection, so not clear why we need maximum backlog. We only allow one active connection, but other clients can queue up to also take advantage of the server once the first client disconnects. A larger backlog allows those additional clients to reach the point where they can poll() for activity, rather than getting EAGAIN failures. > > I think that separating --persistent and --shared would be easier to > understand and use. The backlog will always be based on shared value. > >> +++ b/qemu-nbd.c >> @@ -964,8 +964,16 @@ int main(int argc, char **argv) >> >> server = qio_net_listener_new(); >> if (socket_activation == 0) { >> + int backlog; >> + >> + if (persistent) { >> + backlog = SOMAXCONN; > > This increases the backlog, but since default shared is still 1, we will > not accept more than 1 connection, so not clear why SOMAXCONN > is better. While we aren't servicing the next client yet, we are at least allowing them to make it further in their connection by supporting a backlog.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index d8443d235b73..b264620b98d8 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -134,7 +134,12 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds, qio_net_listener_set_name(nbd_server->listener, "nbd-listener"); - if (qio_net_listener_open_sync(nbd_server->listener, addr, 1, errp) < 0) { + /* + * Because this server is persistent, a backlog of SOMAXCONN is + * better than trying to size it to max_connections. + */ + if (qio_net_listener_open_sync(nbd_server->listener, addr, SOMAXCONN, + errp) < 0) { goto error; } diff --git a/qemu-nbd.c b/qemu-nbd.c index 608c63e82a25..1a340ea4858d 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -964,8 +964,16 @@ int main(int argc, char **argv) server = qio_net_listener_new(); if (socket_activation == 0) { + int backlog; + + if (persistent) { + backlog = SOMAXCONN; + } else { + backlog = MIN(shared, SOMAXCONN); + } saddr = nbd_build_socket_address(sockpath, bindto, port); - if (qio_net_listener_open_sync(server, saddr, 1, &local_err) < 0) { + if (qio_net_listener_open_sync(server, saddr, backlog, + &local_err) < 0) { object_unref(OBJECT(server)); error_report_err(local_err); exit(EXIT_FAILURE);
Our default of a backlog of 1 connection is rather puny; it gets in the way when we are explicitly allowing multiple clients (such as qemu-nbd -e N [--shared], or nbd-server-start with its default "max-connections":0 for unlimited), but is even a problem when we stick to qemu-nbd's default of only 1 active client but use -t [--persistent] where a second client can start using the server once the first finishes. While the effects are less noticeable on TCP sockets (since the client can poll() to learn when the server is ready again), it is definitely observable on Unix sockets, where on Unix, a client will fail with EAGAIN and no recourse but to sleep an arbitrary amount of time before retrying if the server backlog is already full. Since QMP nbd-server-start is always persistent, it now always requests a backlog of SOMAXCONN; meanwhile, qemu-nbd will request SOMAXCONN if persistent, otherwise its backlog should be based on the expected number of clients. See https://bugzilla.redhat.com/1925045 for a demonstration of where our low backlog prevents libnbd from connecting as many parallel clients as it wants. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> CC: qemu-stable@nongnu.org --- blockdev-nbd.c | 7 ++++++- qemu-nbd.c | 10 +++++++++- 2 files changed, 15 insertions(+), 2 deletions(-)