search for: nbd_unlocked_aio_is_ready

..._text permitted_states); pr " return %s;\n" errcode; pr " }\n"; diff --git a/lib/connect.c b/lib/connect.c index 50ec58a..63d2234 100644 --- a/lib/connect.c +++ b/lib/connect.c @@ -38,16 +38,16 @@ static int error_unless_ready (struct nbd_handle *h) { - if (nbd_unlocked_aio_is_ready (h)) + if (nbd_internal_is_state_ready (h->state)) return 0; /* Why did it fail? */ - if (nbd_unlocked_aio_is_closed (h)) { + if (nbd_internal_is_state_closed (h->state)) { set_error (0, "connection is closed"); return -1; } - if (nbd_unlocked_aio_is_dea...

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

...ut not in the READY state) I will post these later this morning. I believe they will let you get rid of the reached_ready flag, as well as making this patch more correct because you probably want to avoid issuing commands on a connection which is closed or dead. You'd use a test like: if (nbd_unlocked_aio_is_ready (conn)) { // call nbd_internal_run } else if (nbd_unlocked_aio_is_processing (conn)) { // can't call nbd_internal_run because we're in the // wrong state, but we can enqueue the command and it // will be processed next time we get to READY } else { // this is an...

..._connected (struct nbd_handle *h) +wait_all_connected (struct nbd_handle *h) { size_t i; for (;;) { - bool connected = false; + bool all_connected = true; - /* Are any connected? */ + /* Are any not yet connected? */ for (i = 0; i < h->multi_conn; ++i) { - if (nbd_unlocked_aio_is_ready (h->conns[i])) { - connected = true; + if (nbd_unlocked_aio_is_closed (h->conns[i])) { + set_error (0, "connection is closed"); + return -1; + } + if (nbd_unlocked_aio_is_dead (h->conns[i])) { + /* Don't set the error here, keep the...

..._connected (struct nbd_handle *h) +wait_all_connected (struct nbd_handle *h) { size_t i; for (;;) { - bool connected = false; + bool all_connected = true; - /* Are any connected? */ + /* Are any not yet connected? */ for (i = 0; i < h->multi_conn; ++i) { - if (nbd_unlocked_aio_is_ready (h->conns[i])) { - connected = true; + if (nbd_unlocked_aio_is_closed (h->conns[i])) { + set_error (0, "connection is closed"); + return -1; + } + if (nbd_unlocked_aio_is_dead (h->conns[i])) { + /* Don't set the error here, keep the...

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

...changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/disconnect.c b/lib/disconnect.c index 9706835..0b8fea0 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -31,7 +31,8 @@ nbd_unlocked_shutdown (struct nbd_handle *h) size_t i; for (i = 0; i < h->multi_conn; ++i) { - if (nbd_unlocked_aio_is_ready (h->conns[i])) { + if (nbd_unlocked_aio_is_ready (h->conns[i]) || + nbd_unlocked_aio_is_processing (h->conns[i])) { if (nbd_unlocked_aio_disconnect (h->conns[i]) == -1) return -1; } -- 2.20.1

...7,7 +248,15 @@ nbd_internal_command_common (struct nbd_connection *conn, uint64_t offset, uint64_t count, void *data, int64_t id, extent_fn extent) { - struct command_in_flight *cmd; + struct command_in_flight *cmd, *prev_cmd; + + if (!nbd_unlocked_aio_is_ready (conn) && + !nbd_unlocked_aio_is_processing (conn)) { + set_error (0, "command request %s is invalid in state %s", + nbd_internal_name_of_nbd_cmd (type), + nbd_internal_state_short_string (conn->state)); + return -1; + } switch (type)...

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.

...essing (state)" Yes, this fixes the race: this code is executed outside the lock, so it must read h->state exactly once before using it in multiple spots. > +++ b/lib/connect.c > @@ -38,16 +38,16 @@ > static int > error_unless_ready (struct nbd_handle *h) > { > - if (nbd_unlocked_aio_is_ready (h)) > + if (nbd_internal_is_state_ready (h->state)) > return 0; > > /* Why did it fail? */ > - if (nbd_unlocked_aio_is_closed (h)) { > + if (nbd_internal_is_state_closed (h->state)) { > set_error (0, "connection is closed"); > return -...

...- } -} - -/* NB: is_locked = false, may_set_error = false. */ -int -nbd_unlocked_aio_is_connecting (struct nbd_handle *h) -{ - enum state_group group = nbd_internal_state_group (h->state); - - return is_connecting_group (group); -} - -/* NB: is_locked = false, may_set_error = false. */ -int -nbd_unlocked_aio_is_ready (struct nbd_handle *h) -{ - return h->state == STATE_READY; -} - -static int -is_processing_group (enum state_group group) -{ - switch (group) { - case GROUP_TOP: - return 0; - case GROUP_ISSUE_COMMAND: - case GROUP_REPLY: - return 1; - default: - return is_processing_group (nbd_i...

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

.../* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_connecting (struct nbd_handle *h) { - return nbd_internal_is_state_connecting (h->state); + return nbd_internal_is_state_connecting (get_state (h)); } /* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_ready (struct nbd_handle *h) { - return nbd_internal_is_state_ready (h->state); + return nbd_internal_is_state_ready (get_state (h)); } /* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_processing (struct nbd_handle *h) { - return nbd_internal_is_state_processing (...

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

...is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_connecting (struct nbd_handle *h) { - return nbd_internal_is_state_connecting (get_state (h)); + return nbd_internal_is_state_connecting (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_ready (struct nbd_handle *h) { - return nbd_internal_is_state_ready (get_state (h)); + return nbd_internal_is_state_ready (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_processing (struct nbd_handle *h) { - return nbd_internal_is_state_pro...

...is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_connecting (struct nbd_handle *h) { - return nbd_internal_is_state_connecting (get_state (h)); + return nbd_internal_is_state_connecting (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_ready (struct nbd_handle *h) { - return nbd_internal_is_state_ready (get_state (h)); + return nbd_internal_is_state_ready (get_public_state (h)); } -/* NB: is_locked = false, may_set_error = false. */ int nbd_unlocked_aio_is_processing (struct nbd_handle *h) { - return nbd_internal_is_state_pro...

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

...state) { @@ -120,6 +126,12 @@ nbd_unlocked_aio_is_connecting (struct nbd_handle *h) return nbd_internal_is_state_connecting (get_public_state (h)); } +int +nbd_unlocked_aio_is_negotiating (struct nbd_handle *h) +{ + return nbd_internal_is_state_negotiating (get_public_state (h)); +} + int nbd_unlocked_aio_is_ready (struct nbd_handle *h) { diff --git a/lib/Makefile.am b/lib/Makefile.am index 1c46c54..9fd6331 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -1,5 +1,5 @@ # nbd client library in userspace -# Copyright (C) 2013-2019 Red Hat Inc. +# Copyright (C) 2013-2020 Red Hat Inc. # # This library is...