search for: unlock_request

Patch itself is not controversial. However I do wonder if we want to change all these constructs so that instead of using #ifdef we use something like: if (HAVE_PIPE2) { // normal path } else { // fallback } (It wouldn't actually work as written above because HAVE_PIPE2 is not always defined, but you get the idea.) This would allow us to test that the fallback paths still

On Haiku which doesn't have pipe2 we were using the fallback path. However a previous change to remove the conn parameter of lock_request and unlock_request had not been made to this code and so it failed to compile. Fixes: commit 91023f269d4cea56f573a1aa0d880b12052f6e1e --- server/connections.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/connections.c b/server/connections.c index c7b55ca1..cc552b90 100644 --...

On Haiku tests/test-socket-activation failed with: nbdkit: locks.c:96:lock_request: conn != NULL called from server/sockets.c: accept_connection in the fallback path which does: lock_request (); thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL)); unlock_request () Because there is no current connection in this thread this code fails. However it should be possible to call lock_request without a connection, provided that thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS (which it is when the fallback path is engaged on Haiku). There was a regress...

...onnection (); return r; } @@ -740,12 +740,12 @@ negotiate_handshake (struct connection *conn) { int r; - plugin_lock_request (conn); + lock_request (conn); if (!newstyle) r = _negotiate_handshake_oldstyle (conn); else r = _negotiate_handshake_newstyle (conn); - plugin_unlock_request (conn); + unlock_request (conn); return r; } @@ -1057,9 +1057,9 @@ recv_request_send_reply (struct connection *conn) error = ESHUTDOWN; } else { - plugin_lock_request (conn); + lock_request (conn); error = handle_request (conn, cmd, flags, offset, count, buf); - plug...

...onnection (); return r; } @@ -740,12 +740,12 @@ negotiate_handshake (struct connection *conn) { int r; - plugin_lock_request (conn); + lock_request (conn); if (!newstyle) r = _negotiate_handshake_oldstyle (conn); else r = _negotiate_handshake_newstyle (conn); - plugin_unlock_request (conn); + unlock_request (conn); return r; } @@ -1057,9 +1057,9 @@ recv_request_send_reply (struct connection *conn) error = ESHUTDOWN; } else { - plugin_lock_request (conn); + lock_request (conn); error = handle_request (conn, cmd, flags, offset, count, buf); - plug...

The previous incomplete patch was here: https://www.redhat.com/archives/libguestfs/2019-September/msg00049.html based on earlier discussion here: https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html In v2: - The previous patch was incomplete. This version completes it by adding tests and extending nbdkit-sh-plugin. - nbdkit_export_name now returns NULL for error,

We noticed while writing various libnbd tests that when the delay filter is in use, there are scenarios where we had to resort to SIGKILL to get rid of nbdkit, because it was non-responsive to SIGINT. I'm still trying to figure out the best way to add testsuite coverage of this, but already proved to myself that it works from the command line, under two scenarios that both used to cause long

I'm fairly sure that these bugs which appear in the Python plugin: https://bugzilla.redhat.com/show_bug.cgi?id=1566516 https://bugzilla.redhat.com/show_bug.cgi?id=1566522 are really bugs in the SERIALIZE_ALL_REQUESTS thread model. See the first patch for the full explanation. The second patch is a fix for a race condition which is probably nudged into being by the first patch. Now this

...ter (size_t index, const char *filename, @@ -465,8 +446,8 @@ extern void lock_init_thread_model (void); extern const char *name_of_thread_model (int model); extern void lock_connection (void); extern void unlock_connection (void); -extern void lock_request (struct connection *conn); -extern void unlock_request (struct connection *conn); +extern void lock_request (void); +extern void unlock_request (void); extern void lock_unload (void); extern void unlock_unload (void); diff --git a/server/backend.c b/server/backend.c index 208c07b1..7a9a7ec8 100644 --- a/server/backend.c +++ b/server/backend.c @@ -1...

v1 was here: https://www.redhat.com/archives/libguestfs/2020-February/msg00081.html v2 replaces struct connection *conn = GET_CONN; with GET_CONN; which sets conn implicitly and asserts that it is non-NULL. If we actually want to test if conn is non-NULL or behave differently, then you must use threadlocal_get_conn() instead, and some existing uses do that. Rich.

The third patch is a large but mechanical change which gets rid of passing around struct connection * entirely within the server, preferring instead to reference the connection through thread-local storage. I hope this is a gateway to simplifying other parts of the code. Rich.

The messages are not really useful to us, but they do bloat the debugging output of virt-v2v massively: nbdkit: python[1]: debug: acquire global request lock nbdkit: python[1]: debug: acquire per-connection request lock nbdkit: python[1]: debug: acquire unload prevention lock nbdkit: python[1]: debug: pwrite count=2097152 offset=4628414464 fua=0 nbdkit: python[1]: debug: release unload prevention

...quire per-connection request lock"); + if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) pthread_mutex_lock (connection_get_request_lock (conn)); - } - debug ("acquire unload prevention lock"); pthread_rwlock_rdlock (&unload_prevention_lock); } void unlock_request (struct connection *conn) { - debug ("release unload prevention lock"); pthread_rwlock_unlock (&unload_prevention_lock); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) { - debug ("release per-connection request lock"); + if (thread_model <= N...

...st_lock); + if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS && + pthread_mutex_lock (&conn->request_lock)) + abort (); - pthread_rwlock_rdlock (&unload_prevention_lock); + if (pthread_rwlock_rdlock (&unload_prevention_lock)) + abort (); } void unlock_request (struct connection *conn) { - pthread_rwlock_unlock (&unload_prevention_lock); + if (pthread_rwlock_unlock (&unload_prevention_lock)) + abort (); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS) - pthread_mutex_unlock (&conn->request_lock); + if (thread_mo...

...e_connection (struct connection *conn) * thread will be in the process of unloading it. The plugin.unload * callback should always be called. */ - if (!quit && connection_get_handle (conn, 0)) { + if (!quit) { lock_request (conn); backend_close (backend, conn); unlock_request (conn); -- 2.21.0

...st-socket-activation failed with: > > nbdkit: locks.c:96:lock_request: conn != NULL > > called from server/sockets.c: accept_connection > in the fallback path which does: > lock_request (); > thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL)); > unlock_request () > > Because there is no current connection in this thread this code fails. > > However it should be possible to call lock_request without a > connection, provided that > thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS (which it is > when the fallback path is enga...

v1 -> v2: - Fixed everything mentioned in the review. Rich.

Somewhat invasive but mostly mechanical change to how plugins are called. This patch is in preparation for adding a second backend subtype for filters. Rich.

...- int thread_model = backend->thread_model (backend); - if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) { debug ("acquire global request lock"); pthread_mutex_lock (&all_requests_lock); @@ -86,8 +91,6 @@ lock_request (struct connection *conn) void unlock_request (struct connection *conn) { - int thread_model = backend->thread_model (backend); - debug ("release unload prevention lock"); pthread_rwlock_unlock (&unload_prevention_lock); diff --git a/src/main.c b/src/main.c index b3e6bad..90d464a 100644 --- a/src/main.c +++ b/src/mai...

...index 7609e9a7..0c1f2413 100644 --- a/server/connections.c +++ b/server/connections.c @@ -149,7 +149,7 @@ _handle_single_connection (int sockin, int sockout) goto done; lock_request (conn); - r = backend->open (backend, conn, readonly); + r = backend_open (backend, conn, readonly); unlock_request (conn); if (r == -1) goto done; diff --git a/server/filters.c b/server/filters.c index 0e10816f..a8d7dd7c 100644 --- a/server/filters.c +++ b/server/filters.c @@ -195,7 +195,7 @@ next_open (void *nxdata, int readonly) { struct b_conn *b_conn = nxdata; - return b_conn->b->open (b...