similar to: [libnbd PATCH] lib: Always return cookie once command is queued

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

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

Previously, our 'cmds_to_issue' list was at most 1 element long, because we reject all commands except from state READY, but don't get back to state READY until the issue-commands sequence has completed. However, this is not as friendly on the client - once we are in transmission phase, a client may want to queue up another command whether or not the state machine is still tied up in

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

Previously, our 'cmds_to_issue' list was at most 1 element long, because we reject all commands except from state READY, but don't get back to state READY until the issue-commands sequence has completed. However, this is not as friendly on the client - once we are in transmission phase, a client may want to queue up another command whether or not the state machine is still tied up in

On Tue, May 21, 2019 at 10:15:49PM -0500, Eric Blake wrote: > Previously, our 'cmds_to_issue' list was at most 1 element long, > because we reject all commands except from state READY, but don't get > back to state READY until the issue-commands sequence has completed. > However, this is not as friendly on the client - once we are in > transmission phase, a client may

All accesses to the h->state field are redirected through these macros (except when we create the handle). This is simple refactoring. --- generator/generator | 18 +++++++++--------- lib/connect.c | 10 +++++----- lib/disconnect.c | 8 ++++---- lib/internal.h | 3 +++ lib/is-state.c | 12 ++++++------ lib/rw.c | 4 ++-- 6 files changed, 29 insertions(+), 26

For each nbd_(unlocked_)?aio_is_* function, split it into an internal function that tests the state and a public visible API function. For calls within the library, use the internal function. This is simple refactoring. As a side effect this fixes a race condition identified by Eric Blake: Thread A Thread B (in a call that holds h->lock) (calling nbd_aio_pread)

Split h->state into: - h->state = the state on entry to the locked region - h->next_state = the current state and what the "publicly visible" state will become when we leave the locked region Some calls to get_state become calls to get_next_state depending on which of these they are trying to read. Calls to set_state become set_next_state because that is what gets

We can easily demonstrate a memory leak when repeatedly calling nbd_aio_pread from nbdsh in the wrong state. True, that isn't something a sane client would normally do, but it's worth fixing. The culprit: when nbd_aio_pread returns a cookie, we guarantee to clean up the closure, but if we fail early and never scheduled the command, nothing ever cleans up the closure. We could document

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomically, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without

On Fri, Sep 11, 2020 at 09:31:11AM -0500, Eric Blake wrote: > As mentioned in commits 176fc4ea and 609c25f0, our original plan in > adding a flags argument to nbd_shutdown was to let us specify > different behaviors at the libnbd level, rather than NBD protocol > flags (for that, the user has nbd_aio_disconnect). But when we later > parameterized OFlags to accept various bitmasks

Split h->state into: - h->public_state = the state on entry to the locked region This is also the atomicly, publicly visible state. - h->state = the real current state of the handle When we leave the locked region we update h->public_state with h->state, so that from outside the lock the handle appears to move atomically from its previous state to the final state without

As mentioned in commits 176fc4ea and 609c25f0, our original plan in adding a flags argument to nbd_shutdown was to let us specify different behaviors at the libnbd level, rather than NBD protocol flags (for that, the user has nbd_aio_disconnect). But when we later parameterized OFlags to accept various bitmasks (commit f891340b), we failed to mark nbd_shutdown as using a different bitmask than

We have a memory leak when a function with a closure exits early prior to registering that closure through some path that will guarantee cleanup. The easiest way to fix it is to guarantee that once a closure is passed into a public API, it will be cleaned regardless of whether that API succeeds or fails. But to avoid cleaning the closure more than once, we need to refactor our internal handling,

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 |

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

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 continues the changes for adding NBD_OPT_LIST support. Now, instead of libnbd malloc'ing storage itself, the user passes a callback that can handle name/description pairs however it likes, and we get rid of the artificial cap at 10000 exports. However, the user will probably end up malloc'ing a list themselves, as we can't call nbd_set_export_name, or even request NBD_OPT_INFO

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 foolproof because we can queue commands that can't be written