Re: [Qemu-devel] [PATCH] doc: Permit BLOCK_STATUS reply to extend beyond request

Re: [Qemu-devel] [PATCH] doc: Permit BLOCK_STATUS reply to extend beyond request

[Vladimir Sementsov-Ogievskiy]

ping.

Finally, Qemu 3.0 released, and it is incompatible with current NBD 
protocol. Please, commit this patch.

13.08.2018 15:02, Vladimir Sementsov-Ogievskiy wrote:
> ping
>
> Qemu 3.0 is near to release, and it is not correspond to current 
> version of spec, it corresponds to this patch. Can we push it?
>
> 30.05.2018 03:35, Eric Blake wrote:
>> When the NBD_CMD_BLOCK_STATUS extension was first discussed, the
>> idea of having the client's length be a hint was proposed, where
>> the server could reply beyond the client's request in order to
>> allow for fewer transactions when querying the entire disk. The
>> portion beyond the client's original request can only occur in
>> the final extent for a given context, and only if the additional
>> length matches the type given for the last byte actually requested
>> by the client.
>>
>> In the meantime, qemu 2.12 was released as a first client
>> implementation of NBD_CMD_BLOCK_STATUS, which always sends the
>> NBD_CMD_FLAG_REQ_ONE flag, and which disconnects from the server
>> if the server's length exceeds the client request.  This was
>> relaxed for subsequent qemu, but it means that we have to be
>> explicit that a server should not send extra length except when
>> the client is not limiting its request to exactly one extent.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>
>> I brought this topic up a few days back, but realized I never
>> actually posted a patch to go along with it.
>>
>>   doc/proto.md | 25 ++++++++++++++++++-------
>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/doc/proto.md b/doc/proto.md
>> index 4b73e0b..bd5f61e 100644
>> --- a/doc/proto.md
>> +++ b/doc/proto.md
>> @@ -1709,8 +1709,8 @@ MUST initiate a hard disconnect.
>>
>>     *length* MUST be 4 + (a positive integer multiple of 8). This reply
>>     represents a series of consecutive block descriptors where the sum
>> -  of the length fields within the descriptors MUST not be greater than
>> -  the length of the original request. This chunk type MUST appear
>> +  of the length fields within the descriptors are subject to further
>> +  constraints documented below. This chunk type MUST appear
>>     exactly once per metadata ID in a structured reply.
>>
>>     The payload starts with:
>> @@ -1725,7 +1725,14 @@ MUST initiate a hard disconnect.
>>     32 bits, status flags
>>
>>     If the client used the `NBD_CMD_FLAG_REQ_ONE` flag in the request,
>> -  then every reply chunk MUST NOT contain more than one descriptor.
>> +  then every reply chunk MUST NOT contain more than one descriptor,
>> +  and the descriptor MUST NOT exceed the *length* of the request.
>> +  Otherwise, when the server returns N extents for a given context,
>> +  the sum of the *length* fields of the first N-1 extents MUST be
>> +  smaller than the overall *length* of the client's request, but the
>> +  final extent MAY exceed the requested length if the server has that
>> +  information anyway as a side effect of reporting the status of the
>> +  requested region.
>>
>>     Even if the client did not use the `NBD_CMD_FLAG_REQ_ONE` flag in
>>     its request, the server MAY return fewer descriptors in the reply
>> @@ -2037,10 +2044,14 @@ The following request types exist:
>>
>>       The list of block status descriptors within the
>>       `NBD_REPLY_TYPE_BLOCK_STATUS` chunk represent consecutive portions
>> -    of the file starting from specified *offset*, and the sum of the
>> -    *length* fields of each descriptor MUST not be greater than the
>> -    overall *length* of the request. This means that the server MAY
>> -    return less data than required. However the server MUST return at
>> +    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. 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 server SHOULD use different *status*
>
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH] doc: Permit BLOCK_STATUS reply to extend beyond request

[Eric Blake]

On 08/15/2018 04:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> ping.
> 
> Finally, Qemu 3.0 released, and it is incompatible with current NBD 
> protocol. Please, commit this patch.

Indeed, qemu 3.0 with the qemu:dirty-bitmap:NAME context does exceed the 
final length when REQ_ONE is not in force.

As I recall, there was no complaint about the concept, only the wording; 
so I think this updated wording satisfies everyone:


    If the client used the `NBD_CMD_FLAG_REQ_ONE` flag in the request,
-  then every reply chunk MUST NOT contain more than one descriptor.
+  then every reply chunk MUST contain exactly one descriptor, and that
+  descriptor MUST NOT exceed the *length* of the original request.  If
+  the client did not use the flag, and the server replies with N
+  extents, then the sum of the *length* fields of the first N-1
+  extents (if any) MUST be less than the requested length, while the
+  *length* of the final extent MAY result in a sum larger than the
+  original requested length, if the server has that information anyway
+  as a side effect of reporting the status of the requested region.

I've gone ahead and pushed this.

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