Re: [Qemu-devel] [PATCH 07/18] nbd: Minimal structured read for client

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

07.02.2017 23:14, Eric Blake wrote:
> On 02/03/2017 09:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Minimal implementation: always send DF flag, to not deal with fragmented
>> replies.
> This works well with your minimal server implementation, but I worry
> that it will cause us to fall over when talking to a fully-compliant
> server that chooses to send EOVERFLOW errors for any request larger than
> 64k when DF is set; it also makes it impossible to benefit from sparse
> reads.  I guess that means we need to start thinking about followup
> patches to flush out our implementation.  But maybe I can live with this
> patch as is, since the goal of your series was not so much the full
> power of structured reads, but getting to a point where we could use
> structured reply for block status, even if it means your client can only
> communicate with qemu-nbd as server for now, as long as we do get to the
> rest of the patches for a full-blown structured read.
>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd-client.c  |  47 +++++++++++----
>>   block/nbd-client.h  |   2 +
>>   include/block/nbd.h |  15 +++--
>>   nbd/client.c        | 170 ++++++++++++++++++++++++++++++++++++++++++++++------
>>   qemu-nbd.c          |   2 +-
>>   5 files changed, 203 insertions(+), 33 deletions(-)
> Hmm - no change to the testsuite. Structured reads seems like the sort
> of thing that it would be nice to test with some canned server replies,
> particularly with server behavior that is permitted by the NBD protocol
> but does not happen by default in qemu-nbd.
>
>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>> index 3779c6c999..ff96bd1635 100644
>> --- a/block/nbd-client.c
>> +++ b/block/nbd-client.c
>> @@ -180,13 +180,20 @@ static void nbd_co_receive_reply(NBDClientSession *s,
>>       *reply = s->reply;
>>       if (reply->handle != request->handle ||
>>           !s->ioc) {
>> +        reply->simple = true;
>>           reply->error = EIO;
> I don't think this is quite right - by setting reply->simple to true,
> you are forcing the caller to treat this as the final packet related to
> this request->handle, even though that might not be the case.
>
> As it is, I wonder if this code is correct, even before your patch - the
> server is allowed to give responses out-of-order (if we request multiple
> reads without waiting for the first response) - I don't see how setting
> reply->error to EIO if the request->handle indicates that we are
> receiving an out-of-order response to some other packet, but that our
> request is still awaiting traffic.

Hmm, looks like it should initiate disconnect instead of just reporting 
error to io operation caller.


-- 
Best regards,
Vladimir