Re: [Libguestfs] [libnbd PATCH] states: Never block state machine inside REPLY

On Wed, Jun 19, 2019 at 01:18:01PM -0500, Eric Blake wrote: > Oddly enough, I am not getting any measurable performance difference > with this patch applied and using examples/threaded-reads-and-writes > coupled with nbdkit. My explanation is that in the common case, once > a server has something to send, it is going to send the entire reply > as fast as it can, rather than sending a partial reply and waiting; > and our attempts to keep up to 64 commands in flight mean that our > real bottleneck is not sending our next command (even if we have to > wait for the server's reply to finish), but at the server (if we are > saturating the server's amount of in-flight requests, the server won't > read our next request until it has finished its current reply). > Still, I'm sure that it is possible to construct corner cases where > this shows more of an effect. What I'm going to say is I think stating the obvious, but my intuition is there are going to be two types of load. In the first and easiest case you want to read or write sequentially over the whole or a large section of the image (think: qemu-img convert). In this case you know well in advance what parts of the image you want to read/write and can keep the in flight queue full at all times. This is what threaded-reads-and-writes actually tests. The harder case is random access (think: qemu with a database guest). There is a short queue, for example because of data dependencies between the requests, so the issued commands buffer is always short. Also there may be server latency because of the random access pattern. This kind of workload should show the benefit of this commit, but we don't really have a test for this kind of workload.

> /* STATE MACHINE */ { > REPLY.START: > + /* If rlen is non-zero, we are resuming an earlier reply cycle. */ > + if (h->rlen > 0) { > + if (h->reply_cmd) { > + assert (nbd_internal_is_state_processing (h->reply_cmd->state)); > + SET_NEXT_STATE (h->reply_cmd->state); This is essentially the "stack of states" idea that I had early on, but with a maximum stack depth of 1. I originally rejected the idea then because it means that the generator would not longer have a complete view of all state transitions in the state machine, and thus wouldn't be able to enforce invariants such as not jumping to a state in the middle of a state group. (In fact with this patch that is now the case -- I thought the generator would give an error about this, but maybe my test is wrong). Nevertheless given that we need to do this, maybe it's better to drop the idea that the generator needs to have a complete view. In my original plan we would have had "push state" and "pop state" operations (pop state is a general jump to "any state", which is why it breaks the assumption in the generator).

> +++ b/lib/internal.h > @@ -253,6 +253,7 @@ struct command_in_flight { > uint32_t count; > void *data; /* Buffer for read/write */ > struct command_cb cb; > + enum state state; /* State to resume with on next POLLIN */ > bool data_seen; /* For read, true if at least one data chunk seen */ > uint32_t error; /* Local errno value */ > }; The patch seems reasonable. Does this obviate any need to split the state machine?

ACK