[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



Eric,

On 26 Apr 2016, at 20:06, Eric Blake <eblake@...696...> wrote:
> 
>> +
>> +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/

thanks - was in the original text.

> 
>> +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

thx

> 
>> +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

Well I deliberately put both in so we can introduce additional parameters
later as non-breaking changes. At some point NBT_OPT_INFO will
be promoted, in which case we can't muck about with it any more
in non-backwards compatible ways.

> 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))?

I suppose I was trying to make it easier for protocol analysers,
not that multiplying by 2 is particularly hard. But yeah I'll
change that one.

>> +    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 )

thx

> 
>> +    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"?

Yeah ok. Or out of band ones.

I'll send through a v4.

--
Alex Bligh




Attachment: signature.asc
Description: Message signed with OpenPGP using GPGMail


Reply to: