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

[Eric Blake]

On 5/22/19 4:23 AM, Richard W.M. Jones wrote:
> 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.

Indeed.

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

Yes, that flow looks sane.

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

Agreed. v3 will be along those lines, and your added helper functions
are indeed nicer for the setup.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190522/cb5000ad/attachment.sig>