search for: nbd_unlocked_aio_is_readi

For each nbd_(unlocked_)?aio_is_* function, split it into an internal function that tests the state and a public visible API function. For calls within the library, use the internal function. This is simple refactoring. As a side effect this fixes a race condition identified by Eric Blake: Thread A Thread B (in a call that holds h->lock) (calling nbd_aio_pread)

Patch 1/6 was posted before and I didn't change it: https://www.redhat.com/archives/libguestfs/2019-May/thread.html#00134 That doesn't necessarily mean I shouldn't change it, I'm posting it again because the other patches depend on it. The main change in this series is we add three new API functions: nbd_aio_is_created - connection has just been created

On Tue, May 21, 2019 at 10:15:49PM -0500, Eric Blake wrote: > Previously, our 'cmds_to_issue' list was at most 1 element long, > because we reject all commands except from state READY, but don't get > back to state READY until the issue-commands sequence has completed. > However, this is not as friendly on the client - once we are in > transmission phase, a client may

If not using multi-conn then obviously the synchronous connection calls ‘nbd_connect_unix’, ‘nbd_connect_tcp’ and ‘nbd_connect_command’ should only return when the (one) connection object is connected. In the multi-conn case it's not very clear what these synchronous calls should do. Previously I had it so that they would return as soon as at least one connection was connected. However this

If not using multi-conn then obviously the synchronous connection calls ‘nbd_connect_unix’, ‘nbd_connect_tcp’ and ‘nbd_connect_command’ should only return when the (one) connection object is connected. In the multi-conn case it's not very clear what these synchronous calls should do. Previously I had it so that they would return as soon as at least one connection was connected. However this

I need to think about this patch series a bit more, but it does at least pass the tests. Rich.

Although most of our synchronous commands are fine doing a round-robin for the first ready connection, and will leave the connection back in the ready state on conclusion, shutdown is a different beast. Once handshake has finished, we want a shutdown request to reach all connections, even if it has to be queued behind other commands already being processed on that connection. In the future, we

Previously, our 'cmds_to_issue' list was at most 1 element long, because we reject all commands except from state READY, but don't get back to state READY until the issue-commands sequence has completed. However, this is not as friendly on the client - once we are in transmission phase, a client may want to queue up another command whether or not the state machine is still tied up in

nbd_connect_unix|tcp had a tricky failure case. This is a consequence of allowing callers to mix synchronous and asynchronous calls, with multi-conn thrown into the mix. I think the new behaviour proposed here is better. We could do with a better way of classifying the state of connections, such as are they connectING. Rich.

On 6/5/19 6:15 AM, Richard W.M. Jones wrote: > For each nbd_(unlocked_)?aio_is_* function, split it into an internal > function that tests the state and a public visible API function. > > For calls within the library, use the internal function. > > This is simple refactoring. > > As a side effect this fixes a race condition identified by Eric Blake: > > Thread A

Simple code motion. --- lib/Makefile.am | 1 + lib/aio.c | 78 ----------------------------------- lib/is-state.c | 105 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 78 deletions(-) diff --git a/lib/Makefile.am b/lib/Makefile.am index 312545e..72d2d0b 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -42,6 +42,7 @@ libnbd_la_SOURCES = \

Since v2: - rebase to Rich's new API calls - more refactoring in patch 1 (retitled) - new patches 3 and 4 - fix data corruption in patch 6 (was 4) - more tweaks to the reproducer example (including using new API from 3) Eric Blake (7): lib: Refactor command_common() to do more common work commands: Allow for a command queue commands: Expose FIFO ordering of server completions

This works. I'm in the middle of testing whether there is any noticable benefit. Rich.

This isn't quite finished because not all of the tests or examples have been updated, but it demonstrates an idea: Should we forget about the concept of having multiple connections managed under a single handle? In this patch there is a single ‘struct nbd_handle *’ which manages a single state machine and connection (and therefore no nbd_connection). To connect to a multi-conn server you must

All accesses to the h->state field are redirected through these macros (except when we create the handle). This is simple refactoring. --- generator/generator | 18 +++++++++--------- lib/connect.c | 10 +++++----- lib/disconnect.c | 8 ++++---- lib/internal.h | 3 +++ lib/is-state.c | 12 ++++++------ lib/rw.c | 4 ++-- 6 files changed, 29 insertions(+), 26

On v1, we discussed whether cmds_to_issue needed to be a list, since it never had more than one element. I played with the idea of making it a list, and allowing the client to queue up new commands regardless of whether the state machine is currently in READY. I also polished up the tmp demo into a bit more full-fledged example file, worth including since it also let me discover a hard-to-hit race

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomically, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomicly, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without

v1 was here: https://www.redhat.com/archives/libguestfs/2019-June/thread.html#00055 v2 was here: https://www.redhat.com/archives/libguestfs/2019-June/thread.html#00067 v3: - Fix atomicly -> atomically in commit message. - Fix a comment. - Fix TOCTTOU: There is now an inline function generated called <name>_is_permitted_state, and this is called twice, first outside the

This is the bare minimum needed to allow the user to take control over the rest of option negotiating. This patch adds several new API: nbd_set_opt_mode() - called during Created to enable the new mode nbd_get_opt_mode() - query whether opt mode is enabled nbd_opt_go() - used in Negotiating state to attempt to use export nbd_opt_abort() - used in Negotiating state to skip Connected state