[PATCH 0/3] Alternate way to avoid race conditions when nbdkit exits.
This fixes the race conditions for me, using the test described here: https://www.redhat.com/archives/libguestfs/2017-September/msg00226.html Rich.
This reverts commit 63f0eb0889c8f8a82ba06a02a8a92d695902baad. --- src/internal.h | 3 --- src/main.c | 13 ------------- src/threadlocal.c | 35 ----------------------------------- 3 files changed, 51 deletions(-) diff --git a/src/internal.h b/src/internal.h index b98be3d..1fc5d69 100644 --- a/src/internal.h +++ b/src/internal.h @@ -182,9 +182,6 @@ extern size_t threadlocal_get_instance_num (void); extern void threadlocal_set_error (int err); extern int threadlocal_get_error (void); /*extern void threadlocal_get_sockaddr ();*/ -extern size_t get_running_threads (void); -extern void incr_running_threads (void); -extern void decr_running_threads (void); /* Declare program_name. */ #if HAVE_DECL_PROGRAM_INVOCATION_SHORT_NAME == 1 diff --git a/src/main.c b/src/main.c index 6ab556f..c9f08ab 100644 --- a/src/main.c +++ b/src/main.c @@ -189,7 +189,6 @@ main (int argc, char *argv[]) int option_index; int help = 0, version = 0, dump_plugin = 0; int tls_set_on_cli = 0; - size_t count; threadlocal_init (); @@ -492,12 +491,6 @@ main (int argc, char *argv[]) start_serving (); - /* Wait, but not forever, for all threads to complete. */ - debug ("waiting for running threads to complete"); - for (count = 0; count < 5 && get_running_threads (); ++count) - sleep (1); - debug ("waited %zus for running threads to complete", count); - plugin_cleanup (); free (unixsocket); @@ -643,12 +636,6 @@ start_serving (void) threadlocal_new_server_thread (); if (handle_single_connection (0, 1) == -1) exit (EXIT_FAILURE); - /* When handling a single connection we reuse the main thread as - * the server thread. We don't want to count it as a running - * thread, but it hasn't called pthread_exit(), so we need to - * explicitly decrement the running threads here. - */ - decr_running_threads (); return; } diff --git a/src/threadlocal.c b/src/threadlocal.c index cc19d48..d6e3942 100644 --- a/src/threadlocal.c +++ b/src/threadlocal.c @@ -71,8 +71,6 @@ free_threadlocal (void *threadlocalv) free (threadlocal->addr); free (threadlocal); - - decr_running_threads (); } void @@ -93,8 +91,6 @@ threadlocal_new_server_thread (void) { struct threadlocal *threadlocal; - incr_running_threads (); - threadlocal = calloc (1, sizeof *threadlocal); if (threadlocal == NULL) { perror ("malloc"); @@ -181,34 +177,3 @@ threadlocal_get_error (void) errno = err; return threadlocal ? threadlocal->err : 0; } - -/* These functions keep track of the number of running threads. */ -static pthread_mutex_t running_threads_lock = PTHREAD_MUTEX_INITIALIZER; -static size_t running_threads = 0; - -size_t -get_running_threads (void) -{ - size_t r; - - pthread_mutex_lock (&running_threads_lock); - r = running_threads; - pthread_mutex_unlock (&running_threads_lock); - return r; -} - -void -incr_running_threads (void) -{ - pthread_mutex_lock (&running_threads_lock); - running_threads++; - pthread_mutex_unlock (&running_threads_lock); -} - -void -decr_running_threads (void) -{ - pthread_mutex_lock (&running_threads_lock); - running_threads--; - pthread_mutex_unlock (&running_threads_lock); -} -- 2.13.2
There are several race conditions where nbdkit exits (calling plugin_cleanup() which unloads the plugin), while the plugin is either in a callback or in plugin.close(). Avoid these by: (1) Taking a shared lock around all plugin callbacks. (2) Taking the same as an exclusive lock in plugin_cleanup. This delays plugin_cleanup until all callbacks have finished. (3) Removing a few unnecessary ‘assert (dl)’. This is necessary because the plugin can now be unloaded while holding locks. ‘dl’ is not needed to lock or unlock the plugin so asserting it is useless. (4) Don't call plugin.close on the quit path. Another thread could be unloading the plugin so we cannot call the .close callback. --- src/connections.c | 10 ++++++++-- src/plugins.c | 20 +++++++++++++------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/src/connections.c b/src/connections.c index 46609f0..0cbf54a 100644 --- a/src/connections.c +++ b/src/connections.c @@ -224,8 +224,14 @@ free_connection (struct connection *conn) pthread_mutex_destroy (&conn->request_lock); - if (conn->handle) - plugin_close (conn); + /* Don't call the plugin again if quit has been set because the main + * thread will be in the process of unloading it. The plugin.unload + * callback should always be called. + */ + if (!quit) { + if (conn->handle) + plugin_close (conn); + } free (conn); } diff --git a/src/plugins.c b/src/plugins.c index d2d0b39..f5056d9 100644 --- a/src/plugins.c +++ b/src/plugins.c @@ -48,6 +48,7 @@ static pthread_mutex_t connection_lock = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t all_requests_lock = PTHREAD_MUTEX_INITIALIZER; +static pthread_rwlock_t unload_prevention_lock = PTHREAD_RWLOCK_INITIALIZER; /* Maximum read or write request that we will handle. */ #define MAX_REQUEST_SIZE (64 * 1024 * 1024) @@ -155,6 +156,11 @@ void plugin_cleanup (void) { if (dl) { + /* Acquiring this lock prevents any plugin callbacks from running + * simultaneously. + */ + pthread_rwlock_wrlock (&unload_prevention_lock); + debug ("%s: unload", filename); if (plugin.unload) plugin.unload (); @@ -163,6 +169,8 @@ plugin_cleanup (void) dl = NULL; free (filename); filename = NULL; + + pthread_rwlock_unlock (&unload_prevention_lock); } } @@ -299,8 +307,6 @@ plugin_config_complete (void) void plugin_lock_connection (void) { - assert (dl); - if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { debug ("%s: acquire connection lock", filename); pthread_mutex_lock (&connection_lock); @@ -310,8 +316,6 @@ plugin_lock_connection (void) void plugin_unlock_connection (void) { - assert (dl); - if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS) { debug ("%s: release connection lock", filename); pthread_mutex_unlock (&connection_lock); @@ -321,8 +325,6 @@ plugin_unlock_connection (void) void plugin_lock_request (struct connection *conn) { - assert (dl); - if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) { debug ("acquire global request lock"); pthread_mutex_lock (&all_requests_lock); @@ -332,12 +334,16 @@ plugin_lock_request (struct connection *conn) debug ("acquire per-connection request lock"); pthread_mutex_lock (connection_get_request_lock (conn)); } + + debug ("acquire unload prevention lock"); + pthread_rwlock_rdlock (&unload_prevention_lock); } void plugin_unlock_request (struct connection *conn) { - assert (dl); + debug ("release unload prevention lock"); + pthread_rwlock_unlock (&unload_prevention_lock); if (plugin._thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) { debug ("release per-connection request lock"); -- 2.13.2
On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
There are several race conditions where nbdkit exits (calling plugin_cleanup() which unloads the plugin), while the plugin is either in a callback or in plugin.close().
Avoid these by:
(1) Taking a shared lock around all plugin callbacks.
(2) Taking the same as an exclusive lock in plugin_cleanup.
This delays plugin_cleanup until all callbacks have finished.
(3) Removing a few unnecessary ‘assert (dl)’.
This is necessary because the plugin can now be unloaded while holding locks. ‘dl’ is not needed to lock or unlock the plugin so asserting it is useless.
(4) Don't call plugin.close on the quit path.
Another thread could be unloading the plugin so we cannot call the .close callback.
Yes, this looks like it solves the same race in a much nicer manner. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
This was underspecified before. --- docs/nbdkit-plugin.pod | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index ceaef1a..47a46da 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -240,6 +240,7 @@ This may be called once just before the plugin is unloaded from memory. Note that it's not guaranteed that C<.unload> will always be called (eg. the server might be killed or segfault), so you should try to make the plugin as robust as possible by not requiring cleanup. +See also L</SHUTDOWN> below. =head2 C<.config> @@ -528,6 +529,16 @@ If none of the above thread models are suitable, then use C<NBDKIT_THREAD_MODEL_PARALLEL> and implement your own locking using C<pthread_mutex_t> etc. +=head1 SHUTDOWN + +When nbdkit receives certain signals it will shut down (see +L<nbdkit(1)/SIGNALS>). The server will wait for any currently running +plugin callbacks to finish and also call the C<.unload> callback +before unloading the plugin. + +Note that it's not guaranteed this can always happen (eg. the server +might be killed by C<SIGKILL> or segfault). + =head1 PARSING SIZE PARAMETERS Use the C<nbdkit_parse_size> utility function to parse human-readable -- 2.13.2
On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
This was underspecified before. --- docs/nbdkit-plugin.pod | 11 +++++++++++ 1 file changed, 11 insertions(+)
+=head1 SHUTDOWN + +When nbdkit receives certain signals it will shut down (see +L<nbdkit(1)/SIGNALS>). The server will wait for any currently running +plugin callbacks to finish and also call the C<.unload> callback +before unloading the plugin.
Do we also guarantee that all subsequent client requests parsed off the wire in between the signal and when we finally call the .unload() callback will NOT call into the plugin, but will be immediately answered to the client with NBD_ESHUTDOWN? If not, we should. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Nov 14, 2017 at 12:18:16PM -0600, Eric Blake wrote:
On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
This was underspecified before. --- docs/nbdkit-plugin.pod | 11 +++++++++++ 1 file changed, 11 insertions(+)
+=head1 SHUTDOWN + +When nbdkit receives certain signals it will shut down (see +L<nbdkit(1)/SIGNALS>). The server will wait for any currently running +plugin callbacks to finish and also call the C<.unload> callback +before unloading the plugin.
Do we also guarantee that all subsequent client requests parsed off the wire in between the signal and when we finally call the .unload() callback will NOT call into the plugin, but will be immediately answered to the client with NBD_ESHUTDOWN? If not, we should.
It's a good question :-) At the moment, no, and yes we should. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On 11/14/2017 11:30 AM, Richard W.M. Jones wrote:
This fixes the race conditions for me, using the test described here:
https://www.redhat.com/archives/libguestfs/2017-September/msg00226.html
Running test-socket-activation in a loop, I've hit other races (some provoked a bit more easily with the code I'm working on), and will be posting some patches where I know the solution: Right now, our use of threadlocal_set_name (plugin_name ()) makes our thread-local storage point to a string in module memory. If we have any nbdkit_debug() or other statement that prints after .unload, we get a SEGV. Solution(s): initializing a plugin should strdup() the name so that plugin_name() is valid for the life of the program, rather than pointing into module memory; and threadlocal_set_name() should strdup() the name rather than relying on the lifetime of the storage of whatever the caller passed in (either fix in isolation solves that SEGV, but using both seems like a good idea). We also have a race that results in a hang in sockets.c:accept_incoming_connections(); if I understand the problem, it is something like: main signal context -------------------------------------- first iteration, finish accept_connect() checks !quit, starts second iteration SIGTERM received set quit call poll() which hangs trying to connect to the socket, compared to the usual: main signal context -------------------------------------- first iteration, finish accept_connect() SIGTERM received set quit checks !quit, ends loop or: main signal context -------------------------------------- first iteration, finish accept_connect() checks !quit, starts second iteration call poll() SIGTERM received set quit poll() fails with EINTR checks !quit, ends loop Here, I'm less sure of what would reliably solve things; maybe we want a pipe-to-self setup where our signal handler (which currently sets !quit) instead (or in addition) writes into the pipe so that our event loop (the poll() in accept_connect()) has an additional fd it can poll in parallel to all the socket fds; that way, the arrival of a signal will ensure that poll() wakes up even without any of the pending socket fds receiving another connection. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Thu, Nov 16, 2017 at 05:09:57PM -0600, Eric Blake wrote:
Here, I'm less sure of what would reliably solve things; maybe we want a pipe-to-self setup where our signal handler (which currently sets !quit) instead (or in addition) writes into the pipe so that our event loop (the poll() in accept_connect()) has an additional fd it can poll in parallel to all the socket fds; that way, the arrival of a signal will ensure that poll() wakes up even without any of the pending socket fds receiving another connection.
A pipe-to-self does sound like the better solution here. poll() will return immediately that way. However the implementation of that will be horrible since I think we'll finally need to keep track of each thread. OTOH keeping track of each thread also seems to be necessary to solve the problem you mentioned on IRC where we pass stack data to pthread_create. If you want me to look at fixing any of these things let me know, otherwise I'll just wait & review patches as posted. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
participants (2)
-
Eric Blake -
Richard W.M. Jones