Re: [Qemu-devel] [PATCHv3] Improve the documentation of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA.

[Eric Blake]

On 04/05/2016 06:07 AM, Alex Bligh wrote:
> Improve the documentation of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA. Specifically
> the latter may be set on any command, and its semantics on commands other
> than NBD_CMD_WRITE need explaining. Further, explain how these relate to
> reordering of commands.
> 
> Signed-off-by: Alex Bligh <address@hidden>
> ---
>  doc/proto.md | 51 +++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 41 insertions(+), 10 deletions(-)
> 

close, but needs a v4

> +++ b/doc/proto.md
> @@ -217,6 +217,33 @@ handle as was sent by the client in the corresponding 
> request.  In
>  this way, the client can correlate which request is receiving a
>  response.
>  
> +#### Ordering of messages and writes
> +
> +The server MAY process commands out of order, and MAY reply out of
> +order, save that:

maybe s/save/except/

> +
> +* All write commands (that includes both `NBD_CMD_WRITE` and
> +  `NBD_CMD_TRIM`) that the server completes (i.e. replies to)

should we also list  NBD_CMD_WRITE_ZEROES?

> +  prior to processing to a `NBD_CMD_FLUSH` MUST be written to non-volatile
> +  storage prior to replying to that `NBD_CMD_FLUSH`. This
> +  paragraph only applies if `NBD_FLAG_SEND_FLUSH` is set within
> +  the transmission flags, as otherwise `NBD_CMD_FLUSH` will never
> +  be sent by the client to the server.
> +
> +* A server MUST NOT reply to a command that has `NBD_CMD_FLAG_FUA` set
> +  in its command flags until the data (if any) written by that command
> +  is persisted to non-volatile storage. This only applies if
> +  `NBD_FLAG_SEND_FLUSH` is set within the transmission flags, as otherwise

s/FLUSH/FUA/

> +  `NBD_CMD_FLAG_FUA` will not be set on any commands sent to the server
> +  by the client.
> +
> +`NBD_CMD_FLUSH` is modelled on the Linux kernel empty bio with
> +`REQ_FLUSH` set. `NBD_CMD_FLAG_FUA` is modelled on the Linux
> +kernel bio with `REQ_FUA` set. In case of ambiguity in this
> +specification, the
> +[kernel 
> documentation](https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt)
> +may be useful.
> +
>  #### Request message
>  
>  The request message, sent by the client, looks as follows:
> @@ -483,10 +510,20 @@ affects a particular command.  Clients MUST NOT set a 
> command flag bit
>  that is not documented for the particular command; and whether a flag is
>  valid may depend on negotiation during the handshake phase.
>  
> -- bit 0, `NBD_CMD_FLAG_FUA`; valid during `NBD_CMD_WRITE` and
> -  `NBD_CMD_WRITE_ZEROES` commands.  SHOULD be set to 1 if the client requires
> -  "Force Unit Access" mode of operation.  MUST NOT be set unless transmission
> -  flags included `NBD_FLAG_SEND_FUA`.
> +- bit 0, `NBD_CMD_FLAG_FUA`; This flag is valid for all commands provided

s/commands/commands,/

> +  `NBD_FLAG_SEND_FUA` has been negotiated, in which case the server MUST
> +  accept all commands with this bit set (even by ignoring the bit). The
> +  client SHOULD NOT set this bit unless the command has the potential of
> +  writing data (current commands are `NBD_CMD_WRITE`, `NBD_CMD_WRITE_ZEROES`
> +  and `NBD_CMD_TRIM`); existing clients are known to set this bit on

maybe: s/existing/but existing/

> +  other commands; subject to that, provided `NBD_FLAG_SEND_FUA` is
> +  negotiated, the client MAY set this bit as it wishes. If the server

Sounds redundant.  I'd strike 'subject to that, ... as it wishes'.

> +  receives a command with `NBD_CMD_FLAG_FUA` set it MUST NOT send its
> +  reply to that command until all write operations (if any) associated with
> +  that command command have been completed and persisted to non-volatile
> +  storage. If the command does not in fact write data (for instance on an
> +  `NBD_CMD_TRIM` which does is ignored), the server MAY ignore this bit

s/does //

> +  being set on such a command.
>  - bit 1, `NBD_CMD_NO_HOLE`; defined by the experimental `WRITE_ZEROES`
>    extension; see below.
>  - bit 2, `NBD_CMD_FLAG_DF`; defined by the experimental `STRUCTURED_REPLY`
> @@ -535,12 +572,6 @@ The following request types exist:
>      message. The server MAY send the reply message before the data has
>      reached permanent storage.
>  
> -    If the `NBD_FLAG_SEND_FUA` flag ("Force Unit Access") was set in the
> -    transmission flags field, the client MAY set the flag `NBD_CMD_FLAG_FUA` 
> in
> -    the command flags field. If this flag was set, the server MUST NOT send
> -    the reply until it has ensured that the newly-written data has reached
> -    permanent storage.
> -

You should drop this paragraph from both NBD_CMD_WRITE and
NBD_CMD_WRITE_ZEROES, now that the flag is globally described (here, you
only dropped it from one of the two).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature