Re: [Qemu-devel] [PATCH v6 10/12] nbd/client: prepare nbd_receive_reply for structured reply

[Eric Blake]

On 10/27/2017 05:40 AM, Eric Blake wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> In following patch nbd_receive_reply will be used both for simple
> and structured reply header receiving.
> NBDReply is altered into union of simple reply header and structured
> reply chunk header, simple error translation moved to block/nbd-client
> to be consistent with further structured reply error translation.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> 

> -    trace_nbd_receive_reply(magic, reply->error, 
> nbd_err_lookup(reply->error),
> -                            reply->handle);
> -    reply->error = nbd_errno_to_system_errno(reply->error);
> -
> -    if (reply->error == ESHUTDOWN) {
> -        /* This works even on mingw which lacks a native ESHUTDOWN */
> -        error_setg(errp, "server shutting down");
> +        trace_nbd_receive_simple_reply(reply->simple.error,
> +                                       nbd_err_lookup(reply->simple.error),
> +                                       reply->handle);
> +        if (reply->simple.error == NBD_ESHUTDOWN) {
> +            /* This works even on mingw which lacks a native ESHUTDOWN */
> +            error_setg(errp, "server shutting down");
> +            return -EINVAL;
> +        }
> +        break;
> +    case NBD_STRUCTURED_REPLY_MAGIC:
> +        ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, 
> errp);
> +        if (ret < 0) {
> +            break;
> +        }
> +        trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
> +                                                 reply->structured.type,
> +                                                 reply->structured.handle,
> +                                                 reply->structured.length);
> +        break;

Ouch. This change means that we now handle NBD_ESHUTDOWN differently if
it was sent as a simple message than if it is sent as a structured reply
chunk.  Furthermore, reading the NBD spec:

> On a server shutdown, the server SHOULD wait for inflight requests to be 
> serviced prior to initiating a hard disconnect. A server MAY speed this 
> process up by issuing error replies. The error value issued in respect of 
> these requests and any subsequently received requests SHOULD be ESHUTDOWN.
> 
> If the client receives an ESHUTDOWN error it MUST initiate a soft disconnect.
> 
> The client MAY issue a soft disconnect at any time, but SHOULD wait until 
> there are no inflight requests first.

We had a pre-existing bug: our behavior in the simple case is wrong - we
are causing a hard disconnect (the server kills the connection) instead
of initiating a soft disconnect (wait for all other pending replies to
come in, then send no further requests other than NBD_CMD_DISC).

A hard disconnect is not the end of the world, even if it is unclean; on
the other hand, not doing a disconnect at all means the client will
continue to see ESHUTDOWN errors until it does shutdown.  Meanwhile,
refactoring block/nbd-client.c to properly drain all existing requests
while preventing future requests seems too risky for the 2.11 timeframe.
 But I don't like the discrepancy between the two styles, so for 2.11, I
think what I will do is rip out the special-casing of NBD_ESHUTDOWN.

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

signature.asc
Description: OpenPGP digital signature