Re: [PATCH v3 2/6] spec: Change maximum block size to maximum payload size

[Wouter Verhelst]

Yes, this will work.

Thanks.

(I think I already gave Reviewed-By for this one, but you can add it if I 
didn't ;-)

Eric Blake <eblake@redhat.com> schreef op 18 april 2023 17:24:48 CEST:
>On Tue, Apr 18, 2023 at 11:26:40AM +0200, Wouter Verhelst wrote:
>> On Thu, Apr 13, 2023 at 05:02:37PM -0500, Eric Blake wrote:
>> > Commit 9f30fedb improved the spec to allow non-payload requests like
>> > NBD_CMD_TRIM that exceed any advertised maximum block size.  Take this
>> > one step further by documenting that the server may use NBD_EOVERFLOW
>> > as a hint to the client when a non-payload request is oversize (while
>> > permitting NBD_EINVAL for back-compat), and by rewording the text to
>> > explicitly call out that what is being advertised is the maximum
>> > payload length, not maximum block size.  Furthermore, favor the term
>> > 'maximum payload size' instead of 'maximum block size', as the real
>> > limitation here is how many bytes are sent in one direction as part of
>> > the command (a maximum payload size may be related to maximum block
>> > size, but existing implementations of both servers and clients that
>> > actually implement NBD_INFO_BLOCK_SIZE have generally been advertising
>> > things like a 32M or 64M data cap, and not an underlying block size
>> > constraint).
>> > 
>....
>> > @@ -747,8 +747,8 @@ text unless the client insists on TLS.
>> > 
>> >  During transmission phase, several operations are constrained by the
>> >  export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`,
>> > -as well as by three block size constraints defined here (minimum,
>> > -preferred, and maximum).
>> > +as well as by three block size constraints defined here (minimum
>> > +block, preferred block, and maximum payload).
>> 
>> I think this may be reworded as:
>> 
>> "as well as by three size constraint defined here"
>> 
>> as they're now no longer all block size constraints.
>> 
>> (this occurs more below)
>
>Concur; how about squashing in:
>
>diff --git i/doc/proto.md w/doc/proto.md
>index 9098c42..7918179 100644
>--- i/doc/proto.md
>+++ w/doc/proto.md
>@@ -409,7 +409,7 @@ cases, a server SHOULD gracefully consume *length* bytes 
>of payload
> (even if it then replies with an `NBD_EINVAL` failure because the
> particular command was not expecting a payload), and proceed with
> the next client command.  Thus, only when *length* is used as an
>-effective length will it utilize a full 64-bit value.
>+effect length will it utilize a full 64-bit value.
>
> #### Simple reply message
>
>@@ -841,24 +841,24 @@ exports. It is not possible to avoid downgrade attacks
> on exports which may be served either via TLS or in plain
> text unless the client insists on TLS.
>
>-## Block size constraints
>+## Size constraints
>
> During transmission phase, several operations are constrained by the
> export size sent by the final `NBD_OPT_EXPORT_NAME` or `NBD_OPT_GO`,
>-as well as by three block size constraints defined here (minimum
>+as well as by three size constraints defined here (minimum
> block, preferred block, and maximum payload).
>
>-If a client can honour server block size constraints (as set out below
>+If a client can honour server size constraints (as set out below
> and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the
> handshake phase by using `NBD_OPT_GO` (and `NBD_OPT_INFO` if used) with
> an `NBD_INFO_BLOCK_SIZE` information request, and MUST use `NBD_OPT_GO`
> rather than `NBD_OPT_EXPORT_NAME` (except in the case of a fallback
> where the server did not support `NBD_OPT_INFO` or `NBD_OPT_GO`).
>
>-A server with block size constraints other than the default SHOULD
>-advertise the block size constraints during handshake phase via
>+A server with size constraints other than the default SHOULD
>+advertise the size constraints during handshake phase via
> `NBD_INFO_BLOCK_SIZE` in response to `NBD_OPT_INFO` or `NBD_OPT_GO`,
>-and MUST do so unless it has agreed on block size constraints via out
>+and MUST do so unless it has agreed on size constraints via out
> of band means.
>
> Some servers are able to make optimizations, such as opening files
>@@ -866,11 +866,11 @@ with `O_DIRECT`, if they know that the client will obey 
>a particular
> minimum block size, where it must fall back to safer but slower code
> if the client might send unaligned requests. For that reason, if a
> client issues an `NBD_OPT_GO` including an `NBD_INFO_BLOCK_SIZE`
>-information request, it MUST abide by the block size constraints it
>+information request, it MUST abide by the size constraints it
> receives. Clients MAY issue `NBD_OPT_INFO` with `NBD_INFO_BLOCK_SIZE` to
> learn the server's constraints without committing to them.
>
>-If block size constraints have not been advertised or agreed on
>+If size constraints have not been advertised or agreed on
> externally, then a server SHOULD support a default minimum block size
> of 1, a preferred block size of 2^12 (4,096), and a maximum
> payload size that is at least 2^25 (33,554,432) (even if the export
>@@ -886,12 +886,12 @@ a hard disconnect) or which uses `NBD_OPT_GO` without 
>requesting
> that do not request sizing information when the server supports
> default sizing or where sizing constraints can be agreed on
> externally.  When allowing clients that did not negotiate sizing via
>-NBD, a server that enforces stricter block size constraints than the
>+NBD, a server that enforces stricter size constraints than the
> defaults MUST cleanly error commands that fall outside the constraints
> without corrupting data; even so, enforcing constraints in this manner
> may limit interoperability.
>
>-A client MAY choose to operate as if tighter block size constraints
>+A client MAY choose to operate as if tighter size constraints
> had been specified (for example, even when the server advertises the
> default minimum block size of 1, a client may safely use a minimum
> block size of 2^9 (512)).
>@@ -1392,13 +1392,13 @@ of the newstyle negotiation.
>       `NBD_REP_INFO` replies, but a SELECTIVETLS server MAY do so if
>       this is a TLS-only export.
>     - `NBD_REP_ERR_BLOCK_SIZE_REQD`: The server requires the client to
>-      request block size constraints using `NBD_INFO_BLOCK_SIZE` prior
>+      request size constraints using `NBD_INFO_BLOCK_SIZE` prior
>       to entering transmission phase, because the server will be using
>       non-default block sizes constraints. The server MUST NOT send this
>-      error if block size constraints were requested with
>+      error if size constraints were requested with
>       `NBD_INFO_BLOCK_SIZE` with the `NBD_OPT_INFO` or `NBD_OPT_GO`
>       request. The server SHOULD NOT send this error if it is using
>-      default block size constraints or block size constraints
>+      default size constraints or size constraints
>       negotiated out of band. A server sending an
>       `NBD_REP_ERR_BLOCK_SIZE_REQD` error SHOULD ensure it first
>       sends an `NBD_INFO_BLOCK_SIZE` information reply in order
>@@ -1748,15 +1748,15 @@ during option haggling in the fixed newstyle 
>negotiation.
>
>     * `NBD_INFO_BLOCK_SIZE` (3)
>
>-      Represents the server's advertised block size constraints; see the
>-      "Block size constraints" section for more details on what these
>+      Represents the server's advertised size constraints; see the
>+      "Size constraints" section for more details on what these
>       values represent, and on constraints on their values.  The server
>       MUST send this info if it is requested and it intends to enforce
>-      block size constraints other than the defaults. After
>+      size constraints other than the defaults. After
>       sending this information in response to an `NBD_OPT_GO` in which
>       the client specifically requested `NBD_INFO_BLOCK_SIZE`, the server
>       can legitimately assume that any client that continues the session
>-      will support the block size constraints supplied (note that this
>+      will support the size constraints supplied (note that this
>       assumption cannot be made solely on the basis of an `NBD_OPT_INFO`
>       with an `NBD_INFO_BLOCK_SIZE` request, or an `NBD_OPT_GO` without
>       an explicit `NBD_INFO_BLOCK_SIZE` request). The *length* MUST be 14,
>@@ -2644,7 +2644,7 @@ implement the following features:
> - Servers that implement block constraints through `NBD_INFO_BLOCK_SIZE`
>   and desire maximum interoperability SHOULD NOT require them.
>   Similarly, clients that desire maximum interoperability SHOULD
>-  implement querying for block size constraints. Since some clients
>+  implement querying for size constraints. Since some clients
>   default to a block size of 512 bytes, implementations desiring maximum
>   interoperability MAY default to that size.
> - Clients or servers that desire interoperability with older
>@@ -2652,7 +2652,7 @@ implement the following features:
>   addition to `NBD_OPT_INFO` and `NBD_OPT_GO`.
> - For data safety, implementing `NBD_CMD_FLUSH` and the
>   `NBD_CMD_FLAG_FUA` flag to `NBD_CMD_WRITE` is strongly recommended.
>-  Clients that do not implement querying for block size constraints
>+  Clients that do not implement querying for size constraints
>   SHOULD abide by the rules laid out in the section "Block size
>   constraints", above.
> - Servers that implement extended headers but desire interoperability
>

-- 
Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.