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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3779 bytes --]

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

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

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]