search for: command_in_flight

...insertions(+), 64 deletions(-) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7e5340c..12536e0 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -20,24 +20,15 @@ /* STATE MACHINE */ { REPLY.SIMPLE_REPLY.START: - struct command_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;...

Our construction of struct command_in_flight was ad hoc; most parameters were set in command_common(), but extent callbacks were done after the fact, and NBD_CMD_DISC was open-coding things. Furthermore, every caller was triggering nbd_internal_run() for the cmd_issue event; doing that in a central place makes it easier for the next patch to...

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

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

...ates.c > @@ -111,6 +111,31 @@ send_from_wbuf (struct nbd_handle *h) > return 0; /* move to next state */ > } > > +/* Forcefully fail any remaining in-flight commands in list */ > +void abort_commands (struct nbd_handle *h, > + struct command_in_flight **list) > +{ > + struct command_in_flight *prev_cmd, *cmd; > + > + for (cmd = *list, prev_cmd = NULL; > + cmd != NULL; > + prev_cmd = cmd, cmd = cmd->next) { > + if (cmd->cb.notify && cmd->type != NBD_CMD_DISC) { > + int error = cmd-&gt...

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

..._t *entries, size_t nr_entries); +typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset, + uint32_t *entries, size_t nr_entries); + +struct command_cb { + void *opaque; + union { + extent_fn extent; + /* More to come */ + } fn; +}; struct command_in_flight { struct command_in_flight *next; @@ -240,8 +249,8 @@ struct command_in_flight { uint64_t handle; uint64_t offset; uint32_t count; - void *data; /* Buffer for read/write, opaque for block status */ - extent_fn extent_fn; + void *data; /* Buffer for read/write */ + struct command_cb...

...| 19 +++---------------- 3 files changed, 12 insertions(+), 30 deletions(-) diff --git a/lib/disconnect.c b/lib/disconnect.c index bc43b4c..26d7298 100644 --- a/lib/disconnect.c +++ b/lib/disconnect.c @@ -62,25 +62,14 @@ nbd_unlocked_aio_disconnect (struct nbd_connection *conn) { struct command_in_flight *cmd; - cmd = malloc (sizeof *cmd); - if (cmd == NULL) { - set_error (errno, "malloc"); + 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->uniq...

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

...ds_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 -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: - assert (conn->cmds_in_flight != NULL); - cmd = conn->cmds_in_flight; - assert (cmd->handle == be64toh (conn->sbuf.request.handle)); - if (cmd->type == NBD_CMD_WRITE) {...

...D_REPLY_FLAG_DONE must be set in NBD_REPLY_TYPE_NONE"); return -1; } - SET_NEXT_STATE (%^FINISH_COMMAND); + SET_NEXT_STATE (%FINISH); return 0; } else if (type == NBD_REPLY_TYPE_OFFSET_DATA) { @@ -225,7 +225,6 @@ REPLY.STRUCTURED_REPLY.RECV_ERROR_TAIL: struct command_in_flight *cmd; - uint16_t flags; uint64_t handle; uint32_t error; uint64_t offset; @@ -234,7 +233,6 @@ switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; case 0: - flags = be16toh (h->sbuf.sr.structured_reply.flags); handle = be64toh (h->sbuf.sr.struc...

...(cmd->type); @@ -43,12 +52,18 @@ } return 0; + ISSUE_COMMAND.PAUSE_SEND_REQUEST: + assert (conn->wlen); + assert (conn->cmds_to_issue != NULL); + conn->in_write_payload = false; + SET_NEXT_STATE (%^REPLY.START); + return 0; + ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD: struct command_in_flight *cmd; assert (conn->cmds_to_issue != NULL); cmd = conn->cmds_to_issue; - assert (cmd->handle == be64toh (conn->sbuf.request.handle)); if (cmd->type == NBD_CMD_WRITE) { conn->wbuf = cmd->data; conn->wlen = cmd->count; @@ -65,9 +80,17 @@ } return...

...(cmd->type); @@ -43,12 +52,18 @@ } return 0; + ISSUE_COMMAND.PAUSE_SEND_REQUEST: + assert (conn->wlen); + assert (conn->cmds_to_issue != NULL); + conn->in_write_payload = false; + SET_NEXT_STATE (%^REPLY.START); + return 0; + ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD: struct command_in_flight *cmd; assert (conn->cmds_to_issue != NULL); cmd = conn->cmds_to_issue; - assert (cmd->handle == be64toh (conn->sbuf.request.handle)); if (cmd->type == NBD_CMD_WRITE) { conn->wbuf = cmd->data; conn->wlen = cmd->count; @@ -65,9 +80,17 @@ } return...

...nerator/states.c +++ b/generator/states.c @@ -111,6 +111,31 @@ send_from_wbuf (struct nbd_handle *h) return 0; /* move to next state */ } +/* Forcefully fail any remaining in-flight commands in list */ +void abort_commands (struct nbd_handle *h, + struct command_in_flight **list) +{ + struct command_in_flight *prev_cmd, *cmd; + + for (cmd = *list, prev_cmd = NULL; + cmd != NULL; + prev_cmd = cmd, cmd = cmd->next) { + if (cmd->cb.notify && cmd->type != NBD_CMD_DISC) { + int error = cmd->error ? cmd->error : ENOTCONN; + +...

...ADY); return 0; diff --git a/lib/rw.c b/lib/rw.c index 9dfce97..fa7dc52 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -246,7 +246,7 @@ command_common (struct nbd_connection *conn, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, void *data) { - struct command_in_flight *cmd; + struct command_in_flight *cmd, *prev_cmd; switch (type) { /* Commands 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_...

...rrno.h> +#include <assert.h> #include "internal.h" @@ -247,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), + nb...

...contains commands diff --git a/lib/rw.c b/lib/rw.c index a7587e9..ebd4ff9 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -246,7 +246,8 @@ command_common (struct nbd_connection *conn, uint16_t flags, uint16_t type, uint64_t offset, uint64_t count, void *data) { - struct command_in_flight *cmd; + struct command_in_flight *cmd, *prev_cmd; + bool event = !conn->reached_ready; switch (type) { /* Commands which send or receive data are limited to MAX_REQUEST_SIZE. */ @@ -296,9 +297,24 @@ command_common (struct nbd_connection *conn, if (conn->structured_replies &&...

...-----------------------------*) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 935f6d2..ddc91ce 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -49,9 +49,22 @@ return 0; REPLY.SIMPLE_REPLY.RECV_READ_PAYLOAD: + struct command_in_flight *cmd = h->reply_cmd; + switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: SET_NEXT_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { + assert (cmd->error == 0); + errno = 0;...

...-----------------------------*) diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 935f6d2..87622a0 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -49,9 +49,22 @@ return 0; REPLY.SIMPLE_REPLY.RECV_READ_PAYLOAD: + struct command_in_flight *cmd = h->reply_cmd; + switch (recv_into_rbuf (h)) { case -1: SET_NEXT_STATE (%.DEAD); return -1; - case 0: SET_NEXT_STATE (%^FINISH_COMMAND); + case 0: + /* guaranteed by START */ + assert (cmd); + if (cmd->cb.fn.read) { + assert (cmd->error == 0); + errno = 0;...