search for: send_write_payload

...gine, which means we are unlikely to be unblocked for > + * writes yet; we want to advance back to the correct state but > + * without trying a send_from_wbuf that will likely return 1. > + */ > if (h->wlen) { > if (h->in_write_payload) > - SET_NEXT_STATE(%SEND_WRITE_PAYLOAD); > + *next_state = %SEND_WRITE_PAYLOAD; > else > - SET_NEXT_STATE(%SEND_REQUEST); > + *next_state = %SEND_REQUEST; It would be nice to do this without fiddling with essentially an internal detail of the generated code. Could we add another macro, something like &q...

...events = [ NotifyWrite, "" ]; }; + State { + default_state with + name = "PREPARE_WRITE_PAYLOAD"; + comment = "Prepare the write payload to send to the remote server"; + external_events = []; + }; + State { default_state with name = "SEND_WRITE_PAYLOAD"; comment = "Sending the write payload to the remote server"; external_events = [ NotifyWrite, "" ]; }; + +State { + default_state with + name = "FINISH"; + comment = "Finish issuing a command"; + external_events = []; + }; ]...

...not on the hot-path (after all, we can't progress >> until data arrives from the server), it's slightly cleaner if we >> instead declare that we are already blocked. >> >> if (h->wlen) { >> if (h->in_write_payload) >> - SET_NEXT_STATE(%SEND_WRITE_PAYLOAD); >> + *next_state = %SEND_WRITE_PAYLOAD; >> else >> - SET_NEXT_STATE(%SEND_REQUEST); >> + *next_state = %SEND_REQUEST; > > It would be nice to do this without fiddling with essentially an > internal detail of the generated code. > > Coul...

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

Let's check for a quite a few more errors. Among other things, this adds some coverage for a few things I've patched recently. --- And these enhancements set me up for my next fix: making NBD_CMD_DISC prevent future commands. tests/errors.c | 167 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 154 insertions(+), 13 deletions(-) diff --git a/tests/errors.c

...unt through + * the REPLY engine, which means we are unlikely to be unblocked for + * writes yet; we want to advance back to the correct state but + * without trying a send_from_wbuf that will likely return 1. + */ if (h->wlen) { if (h->in_write_payload) - SET_NEXT_STATE(%SEND_WRITE_PAYLOAD); + *next_state = %SEND_WRITE_PAYLOAD; else - SET_NEXT_STATE(%SEND_REQUEST); + *next_state = %SEND_REQUEST; return 0; } -- 2.20.1

...bug: nbd_aio_pwrite: transition: ISSUE_COMMAND.START -> ISSUE_COMMAND. SEND_REQUEST libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_REQUEST -> ISSUE_C OMMAND.PREPARE_WRITE_PAYLOAD libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD - > ISSUE_COMMAND.SEND_WRITE_PAYLOAD libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_WRITE_PAYLOAD -> I SSUE_COMMAND.FINISH libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.FINISH -> READY /home/rjones/d/libnbd/tests/.libs/lt-errors: test failed: expect to be blocked on write It seems as if this is caused...

...default_state with + name = "PAUSE_SEND_REQUEST"; + comment = "Interrupt send request to receive an earlier command's reply"; + external_events = []; }; State { @@ -648,7 +656,15 @@ and issue_command_state_machine = [ default_state with name = "SEND_WRITE_PAYLOAD"; comment = "Sending the write payload to the remote server"; - external_events = [ NotifyWrite, "" ]; + external_events = [ NotifyWrite, ""; + NotifyRead, "PAUSE_WRITE_PAYLOAD" ]; + }; + +State { + default_state wit...

...default_state with + name = "PAUSE_SEND_REQUEST"; + comment = "Interrupt send request to receive an earlier command's reply"; + external_events = []; }; State { @@ -648,7 +650,15 @@ and issue_command_state_machine = [ default_state with name = "SEND_WRITE_PAYLOAD"; comment = "Sending the write payload to the remote server"; - external_events = [ NotifyWrite, "" ]; + external_events = [ NotifyWrite, ""; + NotifyRead, "PAUSE_WRITE_PAYLOAD" ]; + }; + +State { + default_state wit...

...nsition: ISSUE_COMMAND.START -> ISSUE_COMMAND. > SEND_REQUEST > libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_REQUEST -> ISSUE_C > OMMAND.PREPARE_WRITE_PAYLOAD > libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.PREPARE_WRITE_PAYLOAD - >> ISSUE_COMMAND.SEND_WRITE_PAYLOAD > libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.SEND_WRITE_PAYLOAD -> I > SSUE_COMMAND.FINISH > libnbd: debug: nbd_aio_pwrite: transition: ISSUE_COMMAND.FINISH -> READY > /home/rjones/d/libnbd/tests/.libs/lt-errors: test failed: expect to be blocked on write > >...

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

I'm not sure if this is worth pursuing. On paper, it makes sense (if we know we have multiple commands batched to send over the wire, AND those commands are short in length, we might as well use MSG_MORE), but the measurement numbers with it applied might just be in the noise. Eric Blake (2): examples: Enhance access patterns of threaded-reads-and-writes states: Another use for MSG_MORE

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

When processing a server reply within the REPLY subgroup, we will often hit a situation where recv() requires us to block until the next NotifyRead. But since NotifyRead is the only permitted external action while in this group, we are effectively blocking CmdIssue and NotifyWrite events from happening until the server finishes the in-progress reply, even though those events have no strict

...= MSG_MORE; SET_NEXT_STATE (%SEND_REQUEST); return 0; @@ -70,6 +70,8 @@ if (cmd->type == NBD_CMD_WRITE) { h->wbuf = cmd->data; h->wlen = cmd->count; + if (cmd->next && cmd->count < 64 * 1024) + h->wflags = MSG_MORE; SET_NEXT_STATE (%SEND_WRITE_PAYLOAD); } else -- 2.20.1

nbdkit 1.2 as a server waits for read() to see EOF, even after the client has sent NBD_CMD_DISC. That was fixed in mbdkit 1.4; and most modern NBD servers are smarter than this (they close() the write end of their traffic soon after NBD_CMD_DISC). But it's easy enough to revert nbdkit commit c70616f8 to get back to a server with the same behavior as the older nbdkit, at which point both

...larger buffers: nbdkit -U - --filter=noparallel memory 512k --run './deadlock $unixsocket' Post-patch, you can see the state machine now shift through ISSUE_COMMAND.PAUSE_WRITE_PAYLOAD through an entire REPLY.START..REPLY.FINISH sequence, and then resume in the middle of the ISSUE_COMMAND.SEND_WRITE_PAYLOAD. .gitignore | 1 + examples/Makefile.am | 10 +++ examples/deadlock.c | 200 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 211 insertions(+) create mode 100644 examples/deadlock.c diff --git a/.gitignore b/.gitignore index 66ff811..c135c26 100644 --- a/.gitignore ++...

...-issue-command.c @@ -54,7 +54,7 @@ ISSUE_COMMAND.SEND_REQUEST: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%PREPARE_WRITE_PAYLOAD); } return 0; @@ -85,7 +85,7 @@ ISSUE_COMMAND.SEND_WRITE_PAYLOAD: switch (send_from_wbuf (h)) { - case -1: SET_NEXT_STATE (%.DEAD); return -1; + case -1: SET_NEXT_STATE (%.DEAD); return 0; case 0: SET_NEXT_STATE (%FINISH); } return 0; diff --git a/generator/states-magic.c b/generator/states-magic.c index 93c92fc..de8d235 100644 --- a/generator/sta...

This is a continuation of a discussion we were having on IRC. The problems with IRC are it's not recorded and it's hard to have deep technical conversations. I hope this is a decent summary. Problem simply stated: Certain NBD servers (qemu-nbd in particular) are able to simultaneously read and write on a socket. ie. They can be simultaneously reading a request and writing the reply to

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