Re: [Qemu-devel] [PATCH v4 09/10] block/nbd-client: nbd reconnect

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

16.01.2019 20:04, Eric Blake wrote:
> On 7/31/18 12:30 PM, Vladimir Sementsov-Ogievskiy wrote:
>> Implement reconnect. To achieve this:
>>
>> 1. add new modes:
>>     connecting-wait: means, that reconnecting is in progress, and there
>>       were small number of reconnect attempts, so all requests are
>>       waiting for the connection.
>>     connecting-nowait: reconnecting is in progress, there were a lot of
>>       attempts of reconnect, all requests will return errors.
>>
>>     two old modes are used too:
>>     connected: normal state
>>     quit: exiting after fatal error or on close
> 
> What makes an error fatal?  Without reconnect, life is simple - if the
> server sends something we can't parse, we permanently turn the device
> into an error condition - because we have no way to get back in sync
> with the server for further commands.  Your patch allows reconnect
> attempts where the connection is down (we failed to send to the server
> or failed to receive the server's reply), but why can we not ALSO
> attempt to reconnect after a parse error?  A reconnect would let us get
> back in sync for attempting further commands.  You're right that the
> current command should probably fail in that case (if the server sent us
> garbage for a specific request, it will probably do so again on a repeat
> of that request; which is different than when we don't even know what
> the server would have sent because of a disconnect).

My thought was that protocol error is fatal, as it shows that server is
buggy, and it seems unsafe to communicate to buggy server..

> 
> Or, put another way, we KNOW we have (corner) cases where a mis-aligned
> image can currently cause the server to return BLOCK_STATUS replies that
> aren't aligned to the advertised minimumm block size.  Attempting to
> read the last sector of an image then causes the client to see the
> misaligned reply and complain, which we are treating as fatal.

Do we have fixes for it?

> But why
> not instead just fail that particular read, but still attempt a
> reconnect, in order to attempt further reads elsewhere in the image that
> do not trip up the server's misaligned reply?
> 

Hmm, for these cases, if we consider this errors not fatal, we don't need
even a reconnect..

If we want to consider protocol errors to be recoverable, we need reconnect only
on wrong magic and may be some kind of inconsistent variable data lenghts..

And it may need addition option, like --strict=false

>>
>> Possible transitions are:
>>
>>     * -> quit
>>     connecting-* -> connected
>>     connecting-wait -> connecting-nowait (transition is done after
>>                        reconnect-delay seconds in connecting-wait mode)
>>     connected -> connecting-wait
>>
>> 2. Implement reconnect in connection_co. So, in connecting-* mode,
>>      connection_co, tries to reconnect unlimited times.
>>
>> 3. Retry nbd queries on channel error, if we are in connecting-wait
>>      state.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd-client.h |   4 +
>>   block/nbd-client.c | 304 +++++++++++++++++++++++++++++++++++++++++++----------
>>   2 files changed, 255 insertions(+), 53 deletions(-)
>>
> 
>> @@ -781,16 +936,21 @@ static int nbd_co_request(BlockDriverState *bs, NBDRequest *request,
>>       } else {
>>           assert(request->type != NBD_CMD_WRITE);
>>       }
>> -    ret = nbd_co_send_request(bs, request, write_qiov);
>> -    if (ret < 0) {
>> -        return ret;
>> -    }
>>   
>> -    ret = nbd_co_receive_return_code(client, request->handle,
>> -                                     &request_ret, &local_err);
>> -    if (local_err) {
>> -        error_report_err(local_err);
>> -    }
>> +    do {
>> +        ret = nbd_co_send_request(bs, request, write_qiov);
>> +        if (ret < 0) {
>> +            continue;
>> +        }
>> +
>> +        ret = nbd_co_receive_return_code(client, request->handle,
>> +                                         &request_ret, &local_err);
>> +        if (local_err) {
>> +            error_report_err(local_err);
>> +            local_err = NULL;
> 
> Conflicts with the conversion to use trace points.
> 


-- 
Best regards,
Vladimir