Re: [PATCH v2 3/6] spec: Add NBD_OPT_EXTENDED_HEADERS

[Eric Blake <eblake@redhat.com>]

On Wed, Feb 22, 2023 at 11:49:18AM +0200, Wouter Verhelst wrote:
> On Mon, Nov 14, 2022 at 04:46:52PM -0600, Eric Blake wrote:
> [...]
> > @@ -1370,9 +1475,10 @@ of the newstyle negotiation.
> >      Return a list of `NBD_REP_META_CONTEXT` replies, one per context,
> >      followed by an `NBD_REP_ACK` or an error.
> > 
> > -    This option SHOULD NOT be requested unless structured replies have
> > -    been negotiated first. If a client attempts to do so, a server
> > -    MAY send `NBD_REP_ERR_INVALID`.
> > +    This option SHOULD NOT be requested unless structured replies or
> > +    extended headers have been negotiated first. If a client attempts
> > +    to do so, a server MAY send `NBD_REP_ERR_INVALID` or
> > +    `NBD_REP_ERR_EXT_HEADER_REQD`.
> 
> Is it the intent that NBD_REP_ERR_EXT_HEADER_REQD means structured
> replies are not supported by this server? I think that could be
> clarified here.

My intent here was that a newer server that ONLY wants to talk to
clients that understand extended headers can use
NBD_REP_ERR_EXT_HEADER_REQD as its error message to clients that have
not requested extended headers yet.  Older clients may not know what
that error code means, but our spec already has reasonable constraints
that the client should at least recognize it as an error code.

That way, a server only has to implement extended headers, rather than
maintaining the structured header code in parallel.

> 
> (this occurs twice)
> 
> [...]
> > +* `NBD_OPT_EXTENDED_HEADERS` (11)
> > +
> > +    The client wishes to use extended headers during the transmission
> > +    phase.  The client MUST NOT send any additional data with the
> > +    option, and the server SHOULD reject a request that includes data
> > +    with `NBD_REP_ERR_INVALID`.
> > +
> > +    When successful, this option takes precedence over structured
> > +    replies.  A client MAY request structured replies first, although
> > +    a server SHOULD support this option even if structured replies are
> > +    not negotiated.
> > +
> > +    It is envisioned that future extensions will add other new
> > +    requests that support a data payload in the request or reply.  A
> > +    server that supports such extensions SHOULD NOT advertise those
> > +    extensions until the client has negotiated extended headers; and a
> > +    client MUST NOT make use of those extensions without first
> > +    enabling support for reply payloads.
> > +
> > +    The server replies with the following, or with an error permitted
> > +    elsewhere in this document:
> > +
> > +    - `NBD_REP_ACK`: Extended headers have been negotiated; the client
> > +      MUST use the 32-byte extended request header, with proper use of
> > +      `NBD_CMD_FLAG_PAYLOAD_LEN` for all commands sending a payload;
> > +      and the server MUST use the 32-byte extended reply header.
> > +    - For backwards compatibility, clients SHOULD be prepared to also
> > +      handle `NBD_REP_ERR_UNSUP`; in this case, only the compact
> > +      transmission headers will be used.
> > +
> > +    Note that a response of `NBD_REP_ERR_BLOCK_SIZE_REQD` does not
> > +    make sense in response to this command, but a server MAY fail with
> > +    that error for a later `NBD_OPT_GO` without a client request for
> > +    `NBD_INFO_BLOCK_SIZE`, since the use of extended headers provides
> > +    more incentive for a client to promise to obey block size
> > +    constraints.
> > +
> > +    If the client requests `NBD_OPT_STARTTLS` after this option, it
> > +    MUST renegotiate extended headers.
> > +
> 
> Does it make sense here to also forbid use of NBD_OPT_EXPORT_NAME? I
> think the sooner we get rid of that, the better ;-)

I hadn't thought of that, but it does indeed sound desirable.  I can
touch up the spec to state that NBD_OPT_EXPORT_NAME MUST NOT be used
by a client that requested extended headers.

> 
> [...]
> > @@ -1746,13 +1914,15 @@ unrecognized flags.
> > 
> >  #### Structured reply types
> > 
> > -These values are used in the "type" field of a structured reply.
> > -Some chunk types can additionally be categorized by role, such as
> > -*error chunks* or *content chunks*.  Each type determines how to
> > -interpret the "length" bytes of payload.  If the client receives
> > -an unknown or unexpected type, other than an *error chunk*, it
> > -MUST initiate a hard disconnect.  A server MUST NOT send a chunk
> > -larger than any advertised maximum block payload size.
> > +These values are used in the "type" field of a structured reply.  Some
> > +chunk types can additionally be categorized by role, such as *error
> > +chunks*, *content chunks*, or *status chunks*.  Each type determines
> > +how to interpret the "length" bytes of payload.  If the client
> > +receives an unknown or unexpected type, other than an *error chunk*,
> > +it MAY initiate a hard disconnect on the grounds that the client is
> > +uncertain whether the server handled the request as desired.  A server
> > +MUST NOT send a chunk larger than any advertised maximum block payload
> > +size.
> 
> Why do we make this a MAY rather than a MUST?

Hmm, now I'm trying to remember why I relaxed this from MUST to MAY on
the client side.  Keeping it at MUST makes sense, because a
well-formed server won't be sending unknown reply types.

> 
> Also, should this section say "structured or extended reply"? We use the
> same types for both.

Makes sense.

> 
> [...]
> > +* `NBD_REPLY_TYPE_BLOCK_STATUS_EXT` (6)
> > +
> > +  This chunk type is in the status chunk category.  *length* MUST be
> > +  8 + (a positive multiple of 16).  The semantics of this chunk mirror
> > +  those of `NBD_REPLY_TYPE_BLOCK_STATUS`, other than the use of a
> > +  larger *extent length* field, added padding in each descriptor to
> > +  ease alignment, and the addition of a *descriptor count* field that
> > +  can be used for easier client processing.  This chunk type MUST NOT
> > +  be used unless extended headers were negotiated with
> > +  `NBD_OPT_EXTENDED_HEADERS`.
> > +
> > +  If the *descriptor count* field contains 0, the number of subsequent
> > +  descriptors is determined solely by the *length* field of the reply
> > +  header.  However, the server MAY populate the *descriptor count*
> > +  field with the number of descriptors that follow; when doing this,
> > +  the server MUST ensure that the header *length* is equal to
> > +  *descriptor count* * 16 + 8.
> > +
> > +  The payload starts with:
> > +
> > +  32 bits, metadata context ID  
> > +  32 bits, descriptor count  
> > +
> > +  and is followed by a list of one or more descriptors, each with this
> > +  layout:
> > +
> > +  64 bits, length of the extent to which the status below
> > +     applies (unsigned, MUST be nonzero)  
> > +  32 bits, padding (MUST be zero)  
> > +  32 bits, status flags  
> > +
> > +  Note that even when extended headers are in use, the client MUST be
> > +  prepared for the server to use either the compact or extended chunk
> > +  type, regardless of whether the client's hinted effect length was
> > +  more or less than 32 bits; but the server MUST use exactly one of
> > +  the two chunk types per negotiated metacontext ID.
> 
> Is this last paragraph really a good idea? I would think it makes more
> sense to require the new format if we're already required to support it
> on both sides anyway.

My proof of implementation was easier to code when I didn't have to
resize the block status reply sizing in the same patch as adding the
64-bit headers.  But if you think requiring the 64-bit reply type
always (and forbidding the 32-bit reply) when extended headers are in
force, that's also possible.
> 
> [...]
> > -    The list of block status descriptors within the
> > -    `NBD_REPLY_TYPE_BLOCK_STATUS` chunk represent consecutive portions
> > -    of the file starting from specified *offset*.  If the client used
> 
> I know this was in the old text (hence me mentioning it here), but this
> should probably say "export" rarher than "file". NBD does not deal
> (conceptually) with files...

Agreed - we have been fairly consistent on the term 'export' (whether
that export be a file or some other source of data).  Will fix
(perhaps separately and just push it as trivial).

> 
> > -    the `NBD_CMD_FLAG_REQ_ONE` flag, each chunk contains exactly one
> > -    descriptor where the *length* of the descriptor MUST NOT be
> > -    greater than the *length* of the request; otherwise, a chunk MAY
> > -    contain multiple descriptors, and the final descriptor MAY extend
> > -    beyond the original requested size if the server can determine a
> > -    larger length without additional effort.  On the other hand, the
> > -    server MAY return less data than requested.  In particular, a
> > -    server SHOULD NOT send more than 2^20 status descriptors in a
> > -    single chunk.  However the server MUST return at least one status
> > -    descriptor, and since each status descriptor has a non-zero
> > -    length, a client can always make progress on a successful return.
> > +    The list of block status descriptors within a given status chunk
> > +    represent consecutive portions of the file starting from specified
> > +    *offset*.  If the client used the `NBD_CMD_FLAG_REQ_ONE` flag,
> > +    each chunk contains exactly one descriptor where the *length* of
> > +    the descriptor MUST NOT be greater than the *length* of the
> > +    request; otherwise, a chunk MAY contain multiple descriptors, and
> > +    the final descriptor MAY extend beyond the original requested size
> > +    if the server can determine a larger length without additional
> > +    effort.  On the other hand, the server MAY return less data than
> > +    requested.  In particular, a server SHOULD NOT send more than 2^20
> > +    status descriptors in a single chunk.  However the server MUST
> > +    return at least one status descriptor, and since each status
> > +    descriptor has a non-zero length, a client can always make
> > +    progress on a successful return.
> 
> Other than that, no comments on this one.
> 
> -- 
>      w@uter.{be,co.za}
> wouter@{grep.be,fosdem.org,debian.org}
> 
> I will have a Tin-Actinium-Potassium mixture, thanks.
> 

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