Re: [Libguestfs] [libnbd PATCH v2 2/5] commands: Allow for a command queue

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 want to queue up another command > whether or not the state machine is still tied up in processing a > previous command. We still want to reject commands sent before the > first time we reach READY, as well as keep the cmd_issue event for > kicking the state machine into action when there is no previous > command being worked on, but otherwise, the state machine itself can > recognize when the command queue needs draining. I have a queued series which adds some new calls that may make this patch simpler:

I believe they will let you get rid of the reached_ready flag, as well as making this patch more correct because you probably want to avoid issuing commands on a connection which is closed or dead.

You'd use a test like: if (nbd_unlocked_aio_is_ready (conn)) { // call nbd_internal_run } else if (nbd_unlocked_aio_is_processing (conn)) { // can't call nbd_internal_run because we're in the // wrong state, but we can enqueue the command and it // will be processed next time we get to READY } else { // this is an error }

> @@ -296,9 +297,24 @@ command_common (struct nbd_connection *conn, > if (conn->structured_replies && cmd->data && type == NBD_CMD_READ) > memset (cmd->data, 0, cmd->count); > > - cmd->next = conn->cmds_to_issue; > - conn->cmds_to_issue = cmd; > - if (nbd_internal_run (conn->h, conn, cmd_issue) == -1) > + /* If we have not reached READY yet, sending the event lets the > + * state machine fail for requesting a command too early. If there > + * are no queued commands and we are already in state READY, send an > + * event to kick the state machine. In all other cases, append our > + * command to the end of the queue, and the state machine will > + * eventually get to it when it cycles back to READY. > + */ I think this test is wrong. We don't know what the failure was, it might have failed for a variety of reasons. It's better to simple test if nbd_unlocked_aio_is_ready, then call nbd_internal_run (if true), otherwise queue the command.