On 10/27/2017 05:40 AM, Eric Blake wrote: > From: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> > > 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 <vsement...@virtuozzo.com> > Signed-off-by: Eric Blake <ebl...@redhat.com> >
> - 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
