[PATCH libnbd 0/2] Support concurrent connections using socket activation

[PATCH v2v 0/2] Remove...

libnbd | Failed pipeline for...

Nir Soffer

Sunday, 7 November 2021 Sun, 7 Nov '21

7:27 a.m.

Add missing test for socket activation and fix the server socket to use larger backlog, allowing concurrent connections. Nir Soffer (2): tests: Test nbd_connect_systemd_socket_activation socket-activation: Allow multiple connections generator/states-connect-socket-activation.c | 2 +- tests/Makefile.am | 5 ++ tests/connect-systemd-socket-activation.c | 83 ++++++++++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 tests/connect-systemd-socket-activation.c -- 2.31.1

Show replies by date

Nir Soffer

Sunday, 7 November Sun, 7 Nov

7:27 a.m.

New subject: [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation

Test connecting using systemd socket activation, and using the handle URI to connect additional handles. The tests reveals a deadlock when closing the first handle owning the nbdkit server. Closing the first handle last avoids this issue. Signed-off-by: Nir Soffer <nsoffer(a)redhat.com> --- tests/Makefile.am | 5 ++ tests/connect-systemd-socket-activation.c | 83 +++++++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 tests/connect-systemd-socket-activation.c diff --git a/tests/Makefile.am b/tests/Makefile.am index 7f00f6f..1451fb8 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -192,6 +192,7 @@ check_PROGRAMS += \ opt-list \ opt-info \ opt-list-meta \ + connect-systemd-socket-activation \ connect-unix \ connect-tcp \ connect-tcp6 \ @@ -235,6 +236,7 @@ TESTS += \ opt-list \ opt-info \ opt-list-meta \ + connect-systemd-socket-activation \ connect-unix \ connect-tcp \ connect-tcp6 \ @@ -424,6 +426,9 @@ opt_info_LDADD = $(top_builddir)/lib/libnbd.la opt_list_meta_SOURCES = opt-list-meta.c opt_list_meta_LDADD = $(top_builddir)/lib/libnbd.la +connect_systemd_socket_activation_SOURCES = connect-systemd-socket-activation.c +connect_systemd_socket_activation_LDADD = $(top_builddir)/lib/libnbd.la + connect_unix_SOURCES = connect-unix.c connect_unix_LDADD = $(top_builddir)/lib/libnbd.la diff --git a/tests/connect-systemd-socket-activation.c b/tests/connect-systemd-socket-activation.c new file mode 100644 index 0000000..d64319a --- /dev/null +++ b/tests/connect-systemd-socket-activation.c @@ -0,0 +1,83 @@ +/* NBD client library in userspace + * Copyright (C) 2021 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +/* Test connecting using systemd socket activation. */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> + +#include <libnbd.h> + +#define CONNECTIONS 4 + +int +main (int argc, char *argv[]) +{ + char *args[] = {"nbdkit", "-f", "memory", "size=1m", NULL}; + struct nbd_handle *nbd[CONNECTIONS] = {0}; + char *uri = NULL; + int result = EXIT_FAILURE; + + printf("Connecting handle 0 with systemd socket actication\n"); + + nbd[0] = nbd_create (); + if (nbd[0] == NULL) + goto out; + + if (nbd_connect_systemd_socket_activation (nbd[0], args) == -1) + goto out; + + uri = nbd_get_uri (nbd[0]); + if (uri == NULL) + goto out; + + printf("Using URI: %s\n", uri); + + for (int i = 1; i < CONNECTIONS; i++) { + printf ("Connecting handle %d to URI\n", i); + + nbd[i] = nbd_create (); + if (nbd[i] == NULL) + goto out; + + if (nbd_connect_uri (nbd[i], uri) == -1) + goto out; + } + + printf ("All handles connected\n"); + result = EXIT_SUCCESS; + +out: + if (result == EXIT_FAILURE) + fprintf (stderr, "%s\n", nbd_get_error ()); + + free (uri); + + /* Closing the first connection deadlocks, so we close the additional + * connections first. */ + for (int i = CONNECTIONS - 1; i >= 0; i--) { + if (nbd[i]) { + printf ("Closing handle %d\n", i); + nbd_close (nbd[i]); + } + } + + exit (result); +} -- 2.31.1

Richard W.M. Jones

7:58 a.m.

New subject: [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation

On Sun, Nov 07, 2021 at 03:27:46PM +0200, Nir Soffer wrote:

...

The test needs to skip if nbdkit and/or the memory plugin is not available. This can be done using: #include "requires.h" ... requires ("nbdkit --version"); requires ("nbdkit memory --version");

...

+ printf("Connecting handle 0 with systemd socket actication\n"); + + nbd[0] = nbd_create (); + if (nbd[0] == NULL) + goto out; + + if (nbd_connect_systemd_socket_activation (nbd[0], args) == -1) + goto out; + + uri = nbd_get_uri (nbd[0]); + if (uri == NULL) + goto out; + + printf("Using URI: %s\n", uri);

I'm not sure what nbd_get_uri should show here - possibly it *ought* to return NULL (since there's no URI that should connect back to a server launched with systemd-socket-activation). But later ...

...

+ for (int i = 1; i < CONNECTIONS; i++) { + printf ("Connecting handle %d to URI\n", i); + + nbd[i] = nbd_create (); + if (nbd[i] == NULL) + goto out; + + if (nbd_connect_uri (nbd[i], uri) == -1) + goto out; + }

Now I'm intrigued. I'm guessing the URI exposes the internal socket we create (/tmp/libnbdXXXXXX/sock) for SSA. Which sounds like an "interesting" corner of the API. It also prevents us from changing this in future -- eg. to use socketpair or a socket in the anonymous space. Also this test needs to check for nbd_can_multi_conn >= 1 before actually doing this (although nbdkit will always return it).

...

+ printf ("All handles connected\n"); + result = EXIT_SUCCESS; + +out: + if (result == EXIT_FAILURE) + fprintf (stderr, "%s\n", nbd_get_error ()); + + free (uri); + + /* Closing the first connection deadlocks, so we close the additional + * connections first. */

And this seems like a bear-trap. I support the idea of making this work, but I'm not sure that doing it this way is a good API. Rich.

...

+ for (int i = CONNECTIONS - 1; i >= 0; i--) { + if (nbd[i]) { + printf ("Closing handle %d\n", i); + nbd_close (nbd[i]); + } + } + + exit (result); +} -- 2.31.1

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

Nir Soffer

8:52 a.m.

New subject: [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation

On Sun, Nov 7, 2021 at 3:58 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

On Sun, Nov 07, 2021 at 03:27:46PM +0200, Nir Soffer wrote: > Test connecting using systemd socket activation, and using the handle > URI to connect additional handles. > > The tests reveals a deadlock when closing the first handle owning the > nbdkit server. Closing the first handle last avoids this issue. > > Signed-off-by: Nir Soffer <nsoffer(a)redhat.com> > --- > tests/Makefile.am | 5 ++ > tests/connect-systemd-socket-activation.c | 83 +++++++++++++++++++++++ > 2 files changed, 88 insertions(+) > create mode 100644 tests/connect-systemd-socket-activation.c > > diff --git a/tests/Makefile.am b/tests/Makefile.am > index 7f00f6f..1451fb8 100644 > --- a/tests/Makefile.am > +++ b/tests/Makefile.am > @@ -192,6 +192,7 @@ check_PROGRAMS += \ > opt-list \ > opt-info \ > opt-list-meta \ > + connect-systemd-socket-activation \ > connect-unix \ > connect-tcp \ > connect-tcp6 \ > @@ -235,6 +236,7 @@ TESTS += \ > opt-list \ > opt-info \ > opt-list-meta \ > + connect-systemd-socket-activation \ > connect-unix \ > connect-tcp \ > connect-tcp6 \ > @@ -424,6 +426,9 @@ opt_info_LDADD = $(top_builddir)/lib/libnbd.la > opt_list_meta_SOURCES = opt-list-meta.c > opt_list_meta_LDADD = $(top_builddir)/lib/libnbd.la > > +connect_systemd_socket_activation_SOURCES = connect-systemd-socket-activation.c > +connect_systemd_socket_activation_LDADD = $(top_builddir)/lib/libnbd.la > + > connect_unix_SOURCES = connect-unix.c > connect_unix_LDADD = $(top_builddir)/lib/libnbd.la > > diff --git a/tests/connect-systemd-socket-activation.c b/tests/connect-systemd-socket-activation.c > new file mode 100644 > index 0000000..d64319a > --- /dev/null > +++ b/tests/connect-systemd-socket-activation.c > @@ -0,0 +1,83 @@ > +/* NBD client library in userspace > + * Copyright (C) 2021 Red Hat Inc. > + * > + * This library is free software; you can redistribute it and/or > + * modify it under the terms of the GNU Lesser General Public > + * License as published by the Free Software Foundation; either > + * version 2 of the License, or (at your option) any later version. > + * > + * This library is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + * Lesser General Public License for more details. > + * > + * You should have received a copy of the GNU Lesser General Public > + * License along with this library; if not, write to the Free Software > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA > + */ > + > +/* Test connecting using systemd socket activation. */ > + > +#include <config.h> > + > +#include <stdio.h> > +#include <stdlib.h> > + > +#include <libnbd.h> > + > +#define CONNECTIONS 4 > + > +int > +main (int argc, char *argv[]) > +{ > + char *args[] = {"nbdkit", "-f", "memory", "size=1m", NULL}; > + struct nbd_handle *nbd[CONNECTIONS] = {0}; > + char *uri = NULL; > + int result = EXIT_FAILURE; The test needs to skip if nbdkit and/or the memory plugin is not available. This can be done using: #include "requires.h" ... requires ("nbdkit --version"); requires ("nbdkit memory --version");

Thanks, I'll add it.

...

> + printf("Connecting handle 0 with systemd socket actication\n"); > + > + nbd[0] = nbd_create (); > + if (nbd[0] == NULL) > + goto out; > + > + if (nbd_connect_systemd_socket_activation (nbd[0], args) == -1) > + goto out; > + > + uri = nbd_get_uri (nbd[0]); > + if (uri == NULL) > + goto out; > + > + printf("Using URI: %s\n", uri); I'm not sure what nbd_get_uri should show here - possibly it *ought* to return NULL (since there's no URI that should connect back to a server launched with systemd-socket-activation). But later ... > + for (int i = 1; i < CONNECTIONS; i++) { > + printf ("Connecting handle %d to URI\n", i); > + > + nbd[i] = nbd_create (); > + if (nbd[i] == NULL) > + goto out; > + > + if (nbd_connect_uri (nbd[i], uri) == -1) > + goto out; > + } Now I'm intrigued. I'm guessing the URI exposes the internal socket we create (/tmp/libnbdXXXXXX/sock) for SSA.

This is the case: $ tests/connect-systemd-socket-activation Connecting handle 0 with systemd socket activation Using URI: nbd+unix:///?socket=/tmp/libnbd9BNa21/sock Connecting handle 1 to URI

...

Which sounds like an "interesting" corner of the API. It also prevents us from changing this in future -- eg. to use socketpair or a socket in the anonymous space.

Yes we don't document that socket activation uses unix socket, and using this may break in future versions. I think this the user should be in control here. For example: nbd_connect_systemd_socket_activation(h, args, LIBNBD_SA_UNIX) If the user does not care about the type of the socket: nbd_connect_systemd_socket_activation(h, args, LIBNBD_SA_AUTO)

...

Also this test needs to check for nbd_can_multi_conn >= 1 before actually doing this (although nbdkit will always return it).

Sure, although this is not an example code, just a test. We can cut corners to simplify the test.

...

> + printf ("All handles connected\n"); > + result = EXIT_SUCCESS; > + > +out: > + if (result == EXIT_FAILURE) > + fprintf (stderr, "%s\n", nbd_get_error ()); > + > + free (uri); > + > + /* Closing the first connection deadlocks, so we close the additional > + * connections first. */ And this seems like a bear-trap.

I suspect this is caused by multiple bugs: - libnbd sends SIGTERM and wait - if the server does not terminate, it will block forever. - nbdkit does not terminate if there are active connections? (I did not investigate yet) So this is not an API issue, just rough edges in libnbd.

...

I support the idea of making this work, but I'm not sure that doing it this way is a good API.

All this can be done by the user outside of libnbd, but using socket activation in linbd makes this really easy to use.

...

Rich. > + for (int i = CONNECTIONS - 1; i >= 0; i--) { > + if (nbd[i]) { > + printf ("Closing handle %d\n", i); > + nbd_close (nbd[i]); > + } > + } > + > + exit (result); > +} > -- > 2.31.1 -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

Richard W.M. Jones

Monday, 8 November Mon, 8 Nov

3:38 a.m.

New subject: [PATCH libnbd 1/2] tests: Test nbd_connect_systemd_socket_activation

I had a think about this. nbd_connect_systemd_socket_activation uses SSA to pass in a listening socket (not a connected socket), so it is expected that the child calls accept(2) at least once on the socket passed in, and presumably anticipated that the child could call accept(2) multiple times. It therefore makes sense that other clients might use the SSA socket. But using nbd_get_uri to obtain this "secret" socket is an unexpected side effect of adding that API. Since it's a listening socket, that rules out future use of socketpair(2) (because that only creates connected sockets, and as far as I can tell there is no way to pass connected sockets through SSA). We might possibly change the implementation in future to use an abstract socket (a Linux extension). Those are in theory expressible as NBD URIs, although it's unlikely that any C library can handle such URIs properly, certainly not libxml2. So I think we can do this as proposed, BUT: - It needs to be documented in the nbd_connect_systemd_socket_activation man page. - Documentation needs to state that if nbd_get_uri() returns NULL or nbd_can_multi_conn() returns <= 0, then multi-conn isn't supported. - Need to also document how to close these shared sockets without deadlocking. I'm still wondering if some kind of nbd_share_systemd_socket_activation() call might be implemented. It would return the "secret" socket path, plus perhaps set a flag on the handle to note that the handle is shared and somehow fix the shutdown path. But I'm not sure how to make that work, and if it's just a more clumsy version of the above. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

Nir Soffer

Sunday, 7 November Sun, 7 Nov

7:27 a.m.

New subject: [PATCH libnbd 2/2] socket-activation: Allow multiple connections

When using qemu-nbd with systemd socket activation, trying to connect additional clients concurrently fails with: nbd_connect_uri: connect: server backlog overflowed, see https://bugzilla.redhat.com/1925045: Resource temporarily unavailable But this bug was fixed in qemu-nbd long time ago; turns out that libnbd has the same bug, creating the server socket with: listen(s, 1); Use SOMAXCONN so we can have multiple connections for better performance. I think this can be very useful for nbdcopy. Signed-off-by: Nir Soffer <nsoffer(a)redhat.com> --- generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 800d963..2abf61b 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -147,7 +147,7 @@ STATE_MACHINE { return 0; } - if (listen (s, 1) == -1) { + if (listen (s, SOMAXCONN) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "listen"); close (s); -- 2.31.1

Richard W.M. Jones

7:59 a.m.

New subject: [PATCH libnbd 2/2] socket-activation: Allow multiple connections

On Sun, Nov 07, 2021 at 03:27:47PM +0200, Nir Soffer wrote:

...

Apart from the problems in API mentioned in the other review, this patch ordering isn't bisectable. What's the motivation for this patch series? Rich.

...

Signed-off-by: Nir Soffer <nsoffer(a)redhat.com> --- generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 800d963..2abf61b 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -147,7 +147,7 @@ STATE_MACHINE { return 0; } - if (listen (s, 1) == -1) { + if (listen (s, SOMAXCONN) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "listen"); close (s); -- 2.31.1

-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW

Nir Soffer

8:22 a.m.

New subject: [PATCH libnbd 2/2] socket-activation: Allow multiple connections

On Sun, Nov 7, 2021 at 4:00 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

On Sun, Nov 07, 2021 at 03:27:47PM +0200, Nir Soffer wrote: > When using qemu-nbd with systemd socket activation, trying to connect > additional clients concurrently fails with: > > nbd_connect_uri: connect: server backlog overflowed, see > https://bugzilla.redhat.com/1925045: Resource temporarily > unavailable > > But this bug was fixed in qemu-nbd long time ago; turns out that libnbd > has the same bug, creating the server socket with: > > listen(s, 1); > > Use SOMAXCONN so we can have multiple connections for better > performance. I think this can be very useful for nbdcopy. Apart from the problems in API mentioned in the other review, this patch ordering isn't bisectable. What's the motivation for this patch series?

Why is it not bissectable? In general, I like to add first tests before I modify code. In the best case the test also reproduces the issue. Having a failing test in the build is better than having a bug. In this case the test is too simple, we need to use multiple threads to reproduce the issue. We can switch the order if you don't like this approach. Nir

Richard W.M. Jones

8:26 a.m.

New subject: [PATCH libnbd 2/2] socket-activation: Allow multiple connections

On Sun, Nov 07, 2021 at 04:22:26PM +0200, Nir Soffer wrote:

...

On Sun, Nov 7, 2021 at 4:00 PM Richard W.M. Jones <rjones(a)redhat.com> wrote: > > On Sun, Nov 07, 2021 at 03:27:47PM +0200, Nir Soffer wrote: > > When using qemu-nbd with systemd socket activation, trying to connect > > additional clients concurrently fails with: > > > > nbd_connect_uri: connect: server backlog overflowed, see > > https://bugzilla.redhat.com/1925045: Resource temporarily > > unavailable > > > > But this bug was fixed in qemu-nbd long time ago; turns out that libnbd > > has the same bug, creating the server socket with: > > > > listen(s, 1); > > > > Use SOMAXCONN so we can have multiple connections for better > > performance. I think this can be very useful for nbdcopy. > > Apart from the problems in API mentioned in the other review, this > patch ordering isn't bisectable. > > What's the motivation for this patch series? Why is it not bissectable?

Doesn't the test fail after the first patch is applied and before the second one? Rich.

...

In general, I like to add first tests before I modify code. In the best case the test also reproduces the issue. Having a failing test in the build is better than having a bug. In this case the test is too simple, we need to use multiple threads to reproduce the issue. We can switch the order if you don't like this approach. Nir

Nir Soffer

8:54 a.m.

New subject: [PATCH libnbd 2/2] socket-activation: Allow multiple connections

On Sun, Nov 7, 2021 at 4:26 PM Richard W.M. Jones <rjones(a)redhat.com> wrote:

...

On Sun, Nov 07, 2021 at 04:22:26PM +0200, Nir Soffer wrote: > On Sun, Nov 7, 2021 at 4:00 PM Richard W.M. Jones <rjones(a)redhat.com> wrote: > > > > On Sun, Nov 07, 2021 at 03:27:47PM +0200, Nir Soffer wrote: > > > When using qemu-nbd with systemd socket activation, trying to connect > > > additional clients concurrently fails with: > > > > > > nbd_connect_uri: connect: server backlog overflowed, see > > > https://bugzilla.redhat.com/1925045: Resource temporarily > > > unavailable > > > > > > But this bug was fixed in qemu-nbd long time ago; turns out that libnbd > > > has the same bug, creating the server socket with: > > > > > > listen(s, 1); > > > > > > Use SOMAXCONN so we can have multiple connections for better > > > performance. I think this can be very useful for nbdcopy. > > > > Apart from the problems in API mentioned in the other review, this > > patch ordering isn't bisectable. > > > > What's the motivation for this patch series? > > Why is it not bissectable? Doesn't the test fail after the first patch is applied and before the second one?

No, it requires using concurrent connections to fail. I have an application using this that fails reliably without the fix.

991

days inactive

992

days old

guestfs@lists.libguestfs.org

Manage subscription

9 comments

2 participants

tags (0)

participants (2)

Nir Soffer
Richard W.M. Jones

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

[PATCH libnbd 0/2] Support concurrent connections using socket activation