search for: cmds_done

...ur 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, and whether cmds_done is used via O(1) or O(n) lookup depends on whether the client uses nbd_aio_peek_command_completed. What's more, we documented that nbd_aio_peek_command_completed can be used to process commands strictly in the order that the server responded - but violated that promise if we strand any command...

...", { default_call with args = []; ret = RConstString; diff --git a/generator/states-reply.c b/generator/states-reply.c index 93f6cda..45362d4 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) { +...

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 |

...13 ++++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 93f6cda..45362d4 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) { +...

...+ > + if (cmd->cb.notify (cmd->cb.opaque, cmd->handle, &error) == -1 && error) > + cmd->error = error; > + } > + if (cmd->error == 0) > + cmd->error = ENOTCONN; > + } > + if (prev_cmd) { > + prev_cmd->next = h->cmds_done; > + h->cmds_done = *list; > + *list = NULL; > + } This inserts the list to the head of cmds_done, which breaks its use as a FIFO queue for clients using nbd_aio_peek_command_completed to process messages in server order. I'll post a fix that keeps things in order. -- Eric...

...r; + + assert (cmd->type != NBD_CMD_DISC); + switch (cmd->cb.callback (cmd->cb.user_data, cookie, &error)) { + case -1: + if (error) + cmd->error = error; + break; + case 1: + retire = true; + break; + } + } /* Move it to the end of the cmds_done list. */ if (prev_cmd != NULL) @@ -171,23 +189,19 @@ save_reply_state (struct nbd_handle *h) else h->cmds_in_flight = cmd->next; cmd->next = NULL; - if (h->cmds_done_tail != NULL) - h->cmds_done_tail = h->cmds_done_tail->next = cmd; + if (retire) + free (c...

...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/states.c b/generator/states.c index 9ed57ae..a11c1d1 100644 --- a/generator/states.c +++ b/generator/states.c @@ -142,7 +142,7 @@ void abort_commands (struct nbd_handle *h, if (cmd->error...

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

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

...+++++++--- 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/generator/states-reply.c b/generator/states-reply.c index 88274bc..c5cd790 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -180,6 +180,14 @@ save_reply_state (struct nbd_handle *h) else h->cmds_done = cmd; + /* Notify the user */ + if (cmd->cb.notify) { + int error = cmd->error; + + if (cmd->cb.notify (cmd->cb.opaque, handle, &error) == -1 && error) + cmd->error = error; + } + SET_NEXT_STATE (%.READY); return 0; diff --git a/generator/states.c b...

...;cmds_to_issue != NULL); cmd = conn->cmds_to_issue; conn->cmds_to_issue = cmd->next; diff --git a/generator/states-reply.c b/generator/states-reply.c index 45362d4..6bb503a 100644 --- a/generator/states-reply.c +++ b/generator/states-reply.c @@ -118,7 +118,10 @@ else conn->cmds_done = cmd; - SET_NEXT_STATE (%.READY); + if (conn->cmds_to_issue) + SET_NEXT_STATE (%^ISSUE_COMMAND.START); + else + SET_NEXT_STATE (%.READY); return 0; } /* END STATE MACHINE */ diff --git a/lib/internal.h b/lib/internal.h index 3f2b729..466af9d 100644 --- a/lib/internal.h +++ b/lib/...

...ure when the handle is closed. But if you had submitted thousands > of asynchronous commands you would end up registering thousands of > close callbacks. > > +++ b/lib/aio.c > @@ -90,6 +90,17 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, > else > h->cmds_done = cmd->next; > > + /* Free the callbacks. */ > + if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent) > + cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data, > + NULL, 0, NULL, 0, NULL); > + if (cmd->type == NBD_CMD...

I still need to wire in the use of *_notify functions into nbdkit to prove whether it makes the code any faster or easier to maintain, but at least the added example shows one good use case for the new API. Eric Blake (6): api: Add nbd_aio_in_flight generator: Allow DEAD state actions to run generator: Allow Int64 in callbacks states: Prepare for aio notify callback api: Add new

...-1; + } + h->reply_cmd = cmd; + return 0; + REPLY.FINISH_COMMAND: struct command_in_flight *prev_cmd, *cmd; uint64_t handle; @@ -102,6 +127,8 @@ break; } assert (cmd != NULL); + assert (h->reply_cmd == cmd); + h->reply_cmd = NULL; /* Move it to the end of the cmds_done list. */ if (prev_cmd != NULL) diff --git a/lib/internal.h b/lib/internal.h index 9404d65..6fde06c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -189,6 +189,8 @@ struct nbd_handle { * acknowledge them. */ struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done; + /...

...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 @@ -163,6 +163...

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

....c @@ -23,6 +23,7 @@ #include <stdbool.h> #include <errno.h> #include <inttypes.h> +#include <assert.h> #include "internal.h" @@ -84,6 +85,8 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, prev_cmd->next = cmd->next; else h->cmds_done = cmd->next; + h->in_flight--; + assert (h->in_flight >= 0); free (cmd); @@ -110,3 +113,9 @@ nbd_unlocked_aio_peek_command_completed (struct nbd_handle *h) set_error (EINVAL, "no commands are in flight"); return -1; } + +int +nbd_unlocked_aio_in_flight (struct nb...

...e 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_sockpath); > >+ } > >+ if (h->sa_tmp...

...ue; conn->cmds_to_issue = cmd->next; diff --git a/lib/internal.h b/lib/internal.h index 2d6ad9d..91c056c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -185,6 +185,7 @@ struct nbd_connection { * acknowledge them. */ struct command_in_flight *cmds_to_issue, *cmds_in_flight, *cmds_done; + bool in_write_payload; }; struct meta_context { -- 2.20.1

...l_run (h, cmd_connect_sa); > +} > diff --git a/lib/handle.c b/lib/handle.c > index 2af25fe..a7f2c79 100644 > --- 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_sockpath); > + } > + if (h->sa_tmpdir) { > + rmdir (h->sa...