search for: nbd_cmd_disc

Once the client has requested NBD_CMD_DISC, the protocol states that it must not send any further information to the server (further writes may still be needed for a clean TLS shutdown, but that's a different matter requiring more states). Our state machine can prevent some of this if we have moved to CLOSED, but that's not foolpro...

According to the NBD spec, the server should close the connection rather than replying to NBD_CMD_DISC (after flushing any pending replies to earlier commands), and a compliant client should not send any data after that command. However, when nbdkit is running multithreaded, we already have multiple threads competing for a read lock, and each of those threads would try to read() from the socket, wh...

...we are robust to non-nbdkit partners that exhibit the same behaviors as our unpatched deadlock. Since testsuite hangs make automated testing less than useful, I'm going ahead and pushing them now. Eric Blake (2): nbd: Inform server of intent to quit nbd: Don't read after client sends NBD_CMD_DISC src/connections.c | 5 +++-- plugins/nbd/nbd.c | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) -- 2.14.3

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

...reader thread is itself blocked on a poll() that will never make additional progress. In hindsight, the problem is obvious: we used the wrong libnbd API (synchronous instead of async), which often results in: nbd .close nbd reader server nbd_shutdown poll - send NBD_CMD_DISC receive NBD_CMD_DISC shutdown(SHUT_WR) wake up on EOF nbd_aio_notify_read - move state to CLOSED nbd_aio_is_dead -&...

...till scenarios where manually retiring the command after the fact is useful (whether the return was 0 to keep the status unchanged, or -1 to alter the retirement status to *error), but by allowing a return value of 1, we can reduce the number of API calls required. We can also auto-retire the lone NBD_CMD_DISC request, reducing the amount of special casing in nbd_aio_[peek_]command_completed. Note that although this is not an API change, it does change ABI, but that's okay as we have not declared a stable interface yet. Update a couple of the tests and examples to give this coverage: examples/glib-...

...end will be received by the server, and its reply to that message will give us the POLLIN kick we need to finally process the reply that had been stuck (for a possibly long time) in the local buffer. But I _did_ come up with a problematic scenario - if the server's final two replies before our NBD_CMD_DISC occur in the same packet, we could end up with the socket closed without ever handling the second reply, giving us data loss for that reply. It's also a potential indirect deadlock: the NBD spec recommends that a client not call NBD_CMD_DISC until it has received all responses to outstanding i...

...whether to wait for the connection to have no in-flight commands, and/or to force a disconnect request to the front of the queue of commands to issue, depending on whether the client wants a fast shutdown vs. completion of outstanding requests to avoid data corruption. The NBD spec recommends that NBD_CMD_DISC not be sent while any other commands are in flight, but similarly that a server should not shut down due to NBD_CMD_DISC until all other responses have been flushed. We may also want to update the state machine to track that once NBD_CMD_DISC has been sent, it is not valid to queue or send any furt...

The next patch wants to make queuing commands smarter; rather than duplicating the logic at each command's call site, it's better to centralize things in command_common, and to also make this helper routine visible for use in NBD_CMD_DISC. --- lib/disconnect.c | 17 +++-------------- lib/internal.h | 6 ++++++ lib/rw.c | 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 @@...

...thread to additionally break out of the loop when the pipe-to-self sees EOF even when nbd_aio_is_dead() has not yet been satisfied I'm also fairly confident that I'll need to patch libnbd to call shutdown(SHUT_WR) on plaintext and gnutls_bye(GNUTLS_SHUT_WR) on TLS connections after sending NBD_CMD_DISC, as that is something we lowt when the nbd plugin switched to libnbd, even after documenting in commit 430f8141 why it is important for preventing shutdown deadlocks. Eric Blake (2): nbd: Don't reference stale errno in reader loop nbd: Reorder cleanup to avoid getting stuck in poll() plu...

...dkit -fv memory size=64M \ > --run '/path/to/qemu-img convert $nbd /var/tmp/out' > > You should see: > > nbdkit: memory.0: debug: pread count=512 offset=0 > nbdkit: memory.1: debug: extents count=67108864 offset=0 req_one=1 > nbdkit: memory.3: debug: client sent NBD_CMD_DISC, closing connection > nbdkit: memory.3: debug: exiting worker thread memory.3 > So the immediate NBD_CMD_DISC is reasonable, implying that the client flagged one of the ways you could have violated protocol (but whether qemu should just hang up, or should just ignore that particular reply b...

...;nbdkit nbd' process has cleanly exited, > things are stalled in .close where nbd is trying to pthread_join() the > reader thread, while the reader thread is itself blocked on a poll() > that will never make additional progress. Tracing the race is > difficult: nbd_shutdown() sends NBD_CMD_DISC to the server, and the > NBD protocol does not require the server to send a response but does > not forbid the server from using gnutls_bye to at least gracefully end > the TLS session. So where a plaintext server just closes the socket > (and the resulting EOF exits our poll()), it ap...

On 3/30/20 2:59 PM, Eric Blake wrote: > 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,...

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

...e client to the 'nbdkit nbd' process has cleanly exited, things are stalled in .close where nbd is trying to pthread_join() the reader thread, while the reader thread is itself blocked on a poll() that will never make additional progress. Tracing the race is difficult: nbd_shutdown() sends NBD_CMD_DISC to the server, and the NBD protocol does not require the server to send a response but does not forbid the server from using gnutls_bye to at least gracefully end the TLS session. So where a plaintext server just closes the socket (and the resulting EOF exits our poll()), it appears that with TLS,...

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 improve that logic without duplicating the fix at each caller. Fix these things by renaming the function to nbd_internal_...

...a client that legitimately sends NBD_OPT_ABORT - it exists only to enumerate the exports advertised by the server, rather than intending to connect to any of those exports. As such, we should downgrade the message for a client telling us it is going away to the same status as our debug message for NBD_CMD_DISC, given that the two have the same effect at different points in the protocol. This stops nbdkit from spamming stderr with nbdkit: file[1]: error: client sent NBD_OPT_ABORT to abort the connection messages due to a client that doesn't want to connect after all. Signed-off-by: Eric Blake <...

...+ 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; > + > + if (cmd->cb.notify (cmd->cb.opaque, cmd->handle, &error) == -1 && error) > + cmd->error = error; > + } > + if (cmd->error == 0) > + cmd->error = ENOTCO...

With this, I am finally able to get the nbd plugin to do out-of-order responses to the client. Once this series goes in, we should be ready for Rich to cut a release. Eric Blake (4): nbd: Split reading into separate thread nbd: Protect writes with mutex nbd: Enable parallel handling tests: Test parallel nbd behavior plugins/nbd/nbd.c | 217

...;%s: %d %s: %r" % (msg, status, reason, body[:200]))\n', "RuntimeError: could not flush: 403 Forbidden: b'no flush for you!'\n"] nbdkit: python[1]: debug: sending error reply: Input/output error virtual copying rate: 3992.5 M bits/sec nbdkit: python[1]: debug: client sent NBD_CMD_DISC, closing connection nbdkit: python[1]: debug: close canceling transfer a46a6646-6eb5-44c9-8234-de5b1779daa5 ## Error in pwrite() nbdkit: python[1]: debug: pwrite count=65536 offset=0 fua=0 unexpected response from imageio server: could not write sector offset 0 size 65536 403: Forbidden b'no...