search for: cmds_to_issue

This might not be the final solution, but it certainly seems to solve a deadlock for me that I could trigger by using 'nbdkit --filter=noparallel memory 512k' and calling nbd_aio_pread for a request larger than 256k (enough for the Linux kernel to block the server until libnbd read()s), immediately followed by nbd_aio_pwrite for a request larger than 256k (enough to block libnbd until the

...r command's reply"; + external_events = []; }; State { diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index e24ea34..3a5980d 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -25,6 +25,15 @@ assert (conn->cmds_to_issue != NULL); cmd = conn->cmds_to_issue; + /* Were we interrupted by reading a reply to an earlier command? */ + if (conn->wlen) { + if (conn->in_write_payload) + SET_NEXT_STATE(%SEND_WRITE_PAYLOAD); + else + SET_NEXT_STATE(%SEND_REQUEST); + return 0; + } + conn-&g...

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...

...r command's reply"; + external_events = []; }; State { diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index e24ea34..3a5980d 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -25,6 +25,15 @@ assert (conn->cmds_to_issue != NULL); cmd = conn->cmds_to_issue; + /* Were we interrupted by reading a reply to an earlier command? */ + if (conn->wlen) { + if (conn->in_write_payload) + SET_NEXT_STATE(%SEND_WRITE_PAYLOAD); + else + SET_NEXT_STATE(%SEND_REQUEST); + return 0; + } + conn-&g...

...ternal_events = []; + }; ] (* Receiving a reply from the server. *) diff --git a/generator/states-issue-command.c b/generator/states-issue-command.c index a57f40f..e24ea34 100644 --- a/generator/states-issue-command.c +++ b/generator/states-issue-command.c @@ -24,9 +24,6 @@ assert (conn->cmds_to_issue != NULL); cmd = conn->cmds_to_issue; - conn->cmds_to_issue = cmd->next; - cmd->next = conn->cmds_in_flight; - conn->cmds_in_flight = cmd; conn->sbuf.request.magic = htobe32 (NBD_REQUEST_MAGIC); conn->sbuf.request.flags = htobe16 (cmd->flags); @@ -40,29 +37,43...

We have no control over the user piling up lots of commands faster than the server can accept (h->cmds_to_issue), or delaying retiring those batched commands (h->cmds_in_flight), hence our use of O(n) list insertion can be noticeable, since the growth of n can be unbounded from our viewpoint. It's easy enough to track a tail pointer to keep insertion O(1), to match that these two lists are used as qu...

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 anot...

Noticed while thinking about the recent threads wondering if we need a more efficient lookup from cookie back to command. Both of these fix bugs, but are tricky enough that I'm posting for review. Eric Blake (2): lib: Decrement in_flight at response, not retirement lib: Do O(1) rather than O(n) queue insertion generator/states-issue-command.c | 2 ++ generator/states-reply.c |

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

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 anot...

...ands which send or receive data are limited to MAX_REQUEST_SIZE. */ @@ -296,8 +296,15 @@ command_common (struct nbd_connection *conn, if (conn->structured_replies && cmd->data && type == NBD_CMD_READ) memset (cmd->data, 0, cmd->count); - cmd->next = conn->cmds_to_issue; - conn->cmds_to_issue = cmd; + /* Stick the command at the end of the list */ + if (conn->cmds_to_issue != NULL) { + prev_cmd = conn->cmds_to_issue; + while (prev_cmd->next) + prev_cmd = prev_cmd->next; + prev_cmd->next = cmd; + } + else + conn->cmds_to_i...

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 w...

...+ cmd = command_common (conn, 0, NBD_CMD_DISC, 0, 0, NULL); + if (cmd == NULL) return -1; - } - cmd->flags = 0; - cmd->type = NBD_CMD_DISC; - cmd->handle = conn->h->unique++; - cmd->offset = 0; - cmd->count = 0; - cmd->data = NULL; - - cmd->next = conn->cmds_to_issue; - conn->cmds_to_issue = cmd; /* This will leave the command on the in-flight list. Is this a * problem? Probably it isn't. If it is, we could add a flag to * the command struct to tell SEND_REQUEST not to add it to the * in-flight list. */ - return nbd_internal_run...

On 5/21/19 10:09 AM, Eric Blake wrote: > A generic client exploiting multiple in-flight commands should be > prepared for out-of-order responses (and should probably ensure that > there are no overlaps between parallel in-flight commands to avoid > unspecified disk contents if the server acts on commands in an > arbitrary order or even exposing non-atomic splicing effects). But a

...DISC, 0, 0, NULL, + 0, NULL); + if (id == -1) return -1; - } - cmd->flags = 0; - cmd->type = NBD_CMD_DISC; - cmd->handle = conn->h->unique++; - cmd->offset = 0; - cmd->count = 0; - cmd->data = NULL; - - cmd->next = conn->cmds_to_issue; - conn->cmds_to_issue = cmd; /* This will leave the command on the in-flight list. Is this a * problem? Probably it isn't. If it is, we could add a flag to * the command struct to tell SEND_REQUEST not to add it to the * in-flight list. */ - return nbd_internal_run...

...+nbd_internal_abort_commands (struct nbd_handle *h, struct command **list) { struct command *next, *cmd; @@ -179,8 +179,8 @@ STATE_MACHINE { /* The caller should have used set_error() before reaching here */ assert (nbd_get_error ()); abort_option (h); - abort_commands (h, &h->cmds_to_issue); - abort_commands (h, &h->cmds_in_flight); + nbd_internal_abort_commands (h, &h->cmds_to_issue); + nbd_internal_abort_commands (h, &h->cmds_in_flight); h->in_flight = 0; if (h->sock) { h->sock->ops->close (h->sock); @@ -190,8 +190,8 @@ STATE_MAC...

..._handle *h, struct command **list) > { > struct command *next, *cmd; > > @@ -179,8 +179,8 @@ STATE_MACHINE { > /* The caller should have used set_error() before reaching here */ > assert (nbd_get_error ()); > abort_option (h); > - abort_commands (h, &h->cmds_to_issue); > - abort_commands (h, &h->cmds_in_flight); > + nbd_internal_abort_commands (h, &h->cmds_to_issue); > + nbd_internal_abort_commands (h, &h->cmds_in_flight); > h->in_flight = 0; > if (h->sock) { > h->sock->ops->close (h->sock);...

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

...nbd_internal_is_state_closed (get_state (h)); } diff --git a/lib/rw.c b/lib/rw.c index 5fe3c64..b38d95b 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -201,7 +201,7 @@ nbd_internal_command_common (struct nbd_handle *h, * be handled automatically on a future cycle around to READY. */ if (h->cmds_to_issue != NULL) { - assert (nbd_internal_is_state_processing (h->state)); + assert (nbd_internal_is_state_processing (get_state (h))); prev_cmd = h->cmds_to_issue; while (prev_cmd->next) prev_cmd = prev_cmd->next; @@ -209,7 +209,7 @@ nbd_internal_command_common (struct n...

...rn nbd_internal_is_state_closed (h->state); } diff --git a/lib/rw.c b/lib/rw.c index e3e0082..5fe3c64 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -201,7 +201,7 @@ nbd_internal_command_common (struct nbd_handle *h, * be handled automatically on a future cycle around to READY. */ if (h->cmds_to_issue != NULL) { - assert (nbd_unlocked_aio_is_processing (h)); + assert (nbd_internal_is_state_processing (h->state)); prev_cmd = h->cmds_to_issue; while (prev_cmd->next) prev_cmd = prev_cmd->next; @@ -209,7 +209,7 @@ nbd_internal_command_common (struct nbd_handle *h,...