[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: [Nbd] [PATCH/RFCv3] Remove NBD_OPT_BLOCK_SIZE; add specific requests to NBD_OPT_INFO



On 04/26/2016 11:50 AM, Alex Bligh wrote:
> Allow a client to requests specific bits of information using
> NBD_OPT_INFO and thereby signal to the server that it supports them.
> This makes it easier for the server to know whether the information
> it is providing will be used / respected.
> 
> Now a server can determine whether a client supports block sizes by
> the fact it uses NBD_OPT_INFO or NBD_OPT_GO with an NBD_INFO_BLOCK_SIZE,
> request as these are part of the same extension, so there is no
> need for NBD_OPT_BLOCK_SIZE.
> 
> Signed-off-by: Alex Bligh <alex@...872...>
> ---
>  doc/proto.md | 99 +++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 52 insertions(+), 47 deletions(-)
> 
> Changes from v2:
> 
> * Add a list of information required to NBD_OPT_INFO
> 
> * Change language for signalling support of blocksizes to
>   NBD_INFO_BLOCK_SIZE being requested by the client.
> 
> * Highlight that a server shouldn't be using NBD_ERR_BLK_SIZE_REQD if
>   it supports the default block sizes.
> 

> +
> +If a client can honor server block size constraints (as set out below
> +and under `NBD_INFO_BLOCK_SIZE`), it SHOULD announce this during the
> +handshake phase by using `NBD_OPT_INFO` and/or `NBD_OPT_GO` with
> +an `NBD_INFO_BLOCK_SIZE` information request, and SHOULD
> +use `NBD_OPT_GO` rather than `NBD_OPT_EXPORT_NAME`.  A server with block
> +size constraints other than the default SHOULD advertise the block size
> +contraints during handshake phase via `NBD_INFO_BLOCK_SIZE` in response

s/contraints/constraints/

> +to `NBD_OPT_INFO` or `NBD_OPT_GO`, and MUST do so unless it
> +has agreed on block size constraints via out of band means.
> +
> +Some servers are able to make optimizations, such as opening files
> +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. A server is entitled to
> +assume that a client that issues `NBD_OPT_INFO` or `NBD_OPT_GO`
> +including in each case an `NBD_INFO_BLOCK_SIZE` information request,
> +will either support the block size constraints it has supplied using
> +`NBD_INFO_BLOCK_SIZE`, or will not continue the session.
>  
>  If block sizes have not been advertised or agreed on externally, then
>  a client SHOULD assume a default minimum block size of 1, a preferred
> @@ -655,9 +669,9 @@ the export size or 0xffffffff (effectively unlimited).  A server that
>  wants to enforce block sizes other than the defaults specified here
>  MAY refuse to go into transmission phase with a client that uses
>  `NBD_OPT_EXPORT_NAME` (via a hard disconnect) or which fails to use
> -`NBD_OPT_BLOCK_SIZE` (where the server uses
> -`NBD_REP_ERR_BLOCK_SIZE_REQD` in response to `NBD_OPT_GO`), although a
> -server SHOULD permit such clients if block sizes can be agreed on
> +`NBD_INFO_BLOCK_SIZE` with `NBD_OPT_GO` (where the server uses
> +NBD_REP_ERR_BLOCK_SIZE_REQD), although a server SHOULD permit such

Missing `` formatting

> +clients if block size constraints are the default or can be agreed on
>  externally.  When allowing such clients, the server MUST cleanly error
>  commands that fall outside block size parameters without corrupting
>  data; even so, this may limit interoperability.
> @@ -883,9 +897,27 @@ of the newstyle negotiation.
>  
>      Data (both commands):
>  
> +    - 32 bits, length of requested information list in bytes; MUST be
> +      no larger than the option data length - 8
> +    - 16 bits x n - list of `NBD_INFO` information requests
> +    - 32 bits, length of name (unsigned); MUST be no larger than the
> +      option data length - 8
>      - String: name of the export (as with `NBD_OPT_EXPORT_NAME`, the length
>        comes from the option header).

Either we only need one secondary length (the length of the '16 bits x
n' array') and the remaining length is implied by the header, or we need
to reword the text about the string length coming from the option header
(as it would now come from your second secondary length).  Personally, I
prefer only ONE secondary length, not two, as there are fewer things
that have to be checked for consistency, as in:

32 bits, length of 'NBD_INFO' request array in bytes
16 bits x n, list of n `NBD_INFO` requests, according to previous length
String, length is option header - 4 - NBD_INFO length

Also, since NBD_INFO requests are capped at 16 bits each, you can
request at most 64k NBD_INFO (or 128k bytes for the request array).
Would it be any easier to state that the first length is the number of
NBD_INFO requests (rather than the size in bytes), at which point it is
then sufficient to have only 16 bits rather than 32 bits for the sizing
of the request (and my above formula for the string length becomes
option header - 4 - (2*length))?

>  
> +    The client MAY list one or more items of specific information it
> +    is seeking in the list of information requests, or it MAY specify
> +    an empty list. The server MUST ignore any information requests
> +    it does not understand. The server MAY ignore information requests
> +    that it does not wish to supply for policy reasons (other than
> +    `NBD_INFO_EXPORT`. Equally the client MAY refuse to negotiate

Missing )

> +    if not supplied information it has requested. The server MAY send
> +    information requests back which are not explicitly requested, but
> +    the server MUST NOT assume that such information requests are
> +    understood and respected by the client unless the client explicitly
> +    asked for them. The client MUST ignore information replies it
> +    does not understand.
> +
>      If no name is specified (i.e. a zero length string is provided),
>      this specifies the default export (if any), as with
>      `NBD_OPT_EXPORT_NAME`.
> @@ -908,12 +940,11 @@ 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
> -      negotiate `NBD_OPT_BLOCK_SIZE` prior to entering transmission
> -      phase, because the server will be using non-default block sizes.
> -      In this case, the server SHOULD first send at least an
> -      `NBD_REP_INFO` reply with `NBD_INFO_BLOCK_SIZE` data.  This
> -      error MUST NOT be used if the client has already negotiated
> -      `NBD_OPT_BLOCK_SIZE`.
> +      request block size constraints using `NBD_INFO_BLOCK_SIZE` prior
> +      to entering transmission phase, because the server will be using
> +      non-default block sizes. The server MUST NOT send this error
> +      if block size constraints were requested with `NBD_INFO_BLOCK_SIZE`
> +      with the `NBD_OPT_INFO` request.

Do we also want "the server SHOULD NOT sent this error if it is using
default block sizes"?

>  
>        Represents the server's advertised block sizes; see the "Block
>        sizes" section for more details on what these values represent,
> -      and on constraints on their values.  The server MAY send this
> -      info whether or not the client has negotiated
> -      `NBD_OPT_BLOCK_SIZE`, and SHOULD send this info if it intends to
> -      enforce block sizes other than the defaults. The *length* MUST
> -      be 14, and the reply payload is interpreted as:
> +      and on constraints on their values.  The server MUST send this info if
> +      it intends to enforce block sizes other than the defaults. After
> +      sending this information, the server can legitimately assume that
> +      any client that continues the session will support the block size
> +      constraints supplied. The *length* MUST be 14, and the reply payload
> +      is interpreted as:
>  
>        - 16 bits, `NBD_INFO_BLOCK_SIZE`  
>        - 32 bits, minimum block size  
> 

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

Attachment: signature.asc
Description: OpenPGP digital signature


Reply to: