search for: cmds_in_flight

...tor/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 @@ return 0; ISSUE_COMMAND.SEND_REQUEST: - struct command_in_flight *cmd; - switch (send_from_wbuf (conn)) { case...

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 queues; cmds_to_issue is always consumed with O(1) lookup/deletion, an...

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 |

...ommand_in_flight *cmd; + struct command_in_flight *cmd = h->reply_cmd; uint32_t error; uint64_t handle; error = be32toh (h->sbuf.simple_reply.error); handle = be64toh (h->sbuf.simple_reply.handle); - /* Find the command amongst the commands in flight. */ - for (cmd = h->cmds_in_flight; cmd != NULL; cmd = cmd->next) { - if (cmd->handle == handle) - break; - } - if (cmd == NULL) { - SET_NEXT_STATE (%.READY); - set_error (0, "no matching handle found for server reply, " - "this is probably a bug in the server"); - return -1...

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

...| 3 ++- 5 files changed, 17 insertions(+), 10 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 09adfed..389317e 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -194,7 +194,7 @@ save_reply_state (struct nbd_handle *h) h->cmds_in_flight = cmd->next; cmd->next = NULL; if (retire) - nbd_internal_retire_and_free_command (cmd); + nbd_internal_retire_and_free_command (h, cmd); else { if (h->cmds_done_tail != NULL) h->cmds_done_tail = h->cmds_done_tail->next = cmd; diff --git a/generator/stat...

...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_MACHINE { CLOSED: abort_option (h); - abort_co...

...depend on commands being processed in strict FIFO order, and we should not get in the way of that. When adding commands to be issued, and when moving a server's reply into commands to inform the client about, we need to insert at the end rather than the head of the appropriate list. Only the cmds_in_flight list does not have to care about maintaining FIFO ordering. --- generator/states-reply.c | 13 ++++++++++--- lib/rw.c | 13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 93f6cda..45362d4 10064...

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

...uct 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_MACHINE { > >...

...being processed in strict FIFO > order, and we should not get in the way of that. When adding commands > to be issued, and when moving a server's reply into commands to inform > the client about, we need to insert at the end rather than the head of > the appropriate list. Only the cmds_in_flight list does not have to > care about maintaining FIFO ordering. > --- > generator/states-reply.c | 13 ++++++++++--- > lib/rw.c | 13 ++++++++++--- > 2 files changed, 20 insertions(+), 6 deletions(-) If O(n) traversal through the list is painful, we could instead twea...

...100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -103,13 +103,20 @@ } assert (cmd != NULL); - /* Move it to the cmds_done list. */ + /* Move it to the end of the cmds_done list. */ if (prev_cmd != NULL) prev_cmd->next = cmd->next; else conn->cmds_in_flight = cmd->next; - cmd->next = conn->cmds_done; - conn->cmds_done = cmd; + cmd->next = NULL; + if (conn->cmds_done) { + prev_cmd = conn->cmds_done; + while (prev_cmd->next) + prev_cmd = prev_cmd->next; + prev_cmd->next = cmd; + } + else + conn->cm...

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

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

...-- End of prologue. -----*/ /* STATE MACHINE */ { @@ -127,6 +152,8 @@ send_from_wbuf (struct nbd_handle *h) DEAD: /* The caller should have used set_error() before reaching here */ assert (nbd_get_error ()); + abort_commands (h, &h->cmds_to_issue); + abort_commands (h, &h->cmds_in_flight); if (h->sock) { h->sock->ops->close (h->sock); h->sock = NULL; @@ -134,6 +161,8 @@ send_from_wbuf (struct nbd_handle *h) return -1; CLOSED: + abort_commands (h, &h->cmds_to_issue); + abort_commands (h, &h->cmds_in_flight); if (h->sock) {...

I've mentioned this topic before (in fact, the idea of adding NBD_CMD_FLAG_DF was first mentioned at [1]), but finally finished enough of an implementation to feel confident in posting it. I'd still like to add something under examples/ that uses the new API to implement strict checking of a server's structured replies read implementation (ensure that a server never sends data after

...-flight list. Is this a * problem? Probably it isn't. If it is, we could add a flag to diff --git a/lib/internal.h b/lib/internal.h index 88ad703..11e0db6 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -191,6 +191,8 @@ struct nbd_handle { struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done; /* Current command during a REPLY cycle */ struct command_in_flight *reply_cmd; + + bool disconnect_request; /* True if we've sent NBD_CMD_DISC */ }; struct meta_context { diff --git a/lib/rw.c b/lib/rw.c index 2dc60de..6b57f11 100644 --- a/lib/rw.c +++ b/lib/rw.c @@...

...or/states.c @@ -191,8 +191,6 @@ STATE_MACHINE { return 0; DEAD: - /* The caller should have used set_error() before reaching here */ - assert (nbd_get_error ()); abort_option (h); nbd_internal_abort_commands (h, &h->cmds_to_issue); nbd_internal_abort_commands (h, &h->cmds_in_flight); diff --git a/lib/errors.c b/lib/errors.c index 8b77650ef3..9142a0843e 100644 --- a/lib/errors.c +++ b/lib/errors.c @@ -93,7 +93,10 @@ allocate_last_error_on_demand (void) last_error = calloc (1, sizeof *last_error); if (last_error) { int err = pthread_setspecific (errors_key, las...

...trict FIFO > > order, and we should not get in the way of that. When adding commands > > to be issued, and when moving a server's reply into commands to inform > > the client about, we need to insert at the end rather than the head of > > the appropriate list. Only the cmds_in_flight list does not have to > > care about maintaining FIFO ordering. > > --- > > generator/states-reply.c | 13 ++++++++++--- > > lib/rw.c | 13 ++++++++++--- > > 2 files changed, 20 insertions(+), 6 deletions(-) > > If O(n) traversal through the lis...

...connect_command. At the moment these functions move the handle to the DEAD state if they fail, but that's not really necessary in all cases. > >--- a/lib/handle.c > >+++ b/lib/handle.c > >@@ -129,6 +129,16 @@ nbd_close (struct nbd_handle *h) > > free_cmd_list (h->cmds_in_flight); > > free_cmd_list (h->cmds_done); > > nbd_internal_free_string_list (h->argv); > >+ if (h->sa_sockpath) { > >+ if (h->pid > 0) > >+ kill (h->pid, SIGTERM); > >+ unlink (h->sa_sockpath); > >+ free (h->sa_sockpat...