Re: [PATCH v2] nbd/server: Suppress Broken pipe errors on abrupt disconnection

[Vladimir Sementsov-Ogievskiy]

14.09.2021 17:52, Richard W.M. Jones wrote:
> On Tue, Sep 14, 2021 at 05:40:59PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 13.09.2021 18:19, Richard W.M. Jones wrote:
> > > $ rm -f /tmp/sock /tmp/pid
> > > $ qemu-img create -f qcow2 /tmp/disk.qcow2 1M
> > > $ qemu-nbd -t --format=qcow2 --socket=/tmp/sock --pid-file=/tmp/pid /tmp/disk.qcow2 
> > > &
> > > $ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()'
> > > qemu-nbd: Disconnect client, due to: Failed to send reply: Unable to write to 
> > > socket: Broken pipe
> > > $ killall qemu-nbd
> > >
> > > nbdsh is abruptly dropping the NBD connection here which is a valid
> > > way to close the connection.  It seems unnecessary to print an error
> > > in this case so this commit suppresses it.
> > >
> > > Note that if you call the nbdsh h.shutdown() method then the message
> > > was not printed:
> > >
> > > $ nbdsh -u 'nbd+unix:///?socket=/tmp/sock' -c 'h.get_size()' -c 'h.shutdown()'
> >
> > My personal opinion, is that this warning doesn't hurt in general. I
> > think in production tools should gracefully shutdown any connection,
> > and abrupt shutdown is a sign of something wrong - i.e., worth
> > warning.
> >
> > Shouldn't nbdsh do graceful shutdown by default?
>
> On the client side the only difference is that nbd_shutdown sends
> NBD_CMD_DISC to the server (versus simply closing the socket).

Qemu as NBD client always send NBD_CMD_DISC on close() (if io channel is alive).

>  On the
> server side when the server receives NBD_CMD_DISC it must complete any
> in-flight requests, but there's no requirement for the server to
> commit anything to disk.  IOW you can still lose data even though you
> took the time to disconnect.
>
> So I don't think there's any reason for libnbd to always gracefully

Hmm. Me go to NBD spec :)

I think, there is a reason:

"The client and the server MUST NOT initiate any form of disconnect other than in 
one of the above circumstances."

And the only possibility for client to initiate a hard disconnect listed above is 
"if it detects a violation by the other party of a mandatory condition within this 
document".

So at least, nbdsh violates NBD protocol. May be spec should be updated to 
satisfy your needs.

> shut down (especially in this case where there are no in-flight
> requests), and anyway it would break ABI to make that change and slow
> down the client in cases when there's nothing to clean up.

Which ABI will it break?

> > > Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
> > > ---
> > >   nbd/server.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/nbd/server.c b/nbd/server.c
> > > index 3927f7789d..e0a43802b2 100644
> > > --- a/nbd/server.c
> > > +++ b/nbd/server.c
> > > @@ -2669,7 +2669,12 @@ static coroutine_fn void nbd_trip(void *opaque)
> > >           ret = nbd_handle_request(client, &request, req->data, &local_err);
> > >       }
> > >       if (ret < 0) {
> > > -        error_prepend(&local_err, "Failed to send reply: ");
> > > +        if (errno != EPIPE) {
> >
> > Both nbd_handle_request() and nbd_send_generic_reply() declares that
> > they return -errno on failure in communication with client. I think,
> > you should use ret here: if (ret != -EPIPE). It's safer: who knows,
> > does errno really set on all error paths of called functions? If
> > not, we may see here errno of some another previous operation.
>
> Should we set errno = 0 earlier in nbd_trip()?  I don't really know
> how coroutines in qemu interact with thread-local variables though.
>
> Rich.
>
> > > +            error_prepend(&local_err, "Failed to send reply: ");
> > > +        } else {
> > > +            error_free(local_err);
> > > +            local_err = NULL;
> > > +        }
> > >           goto disconnect;
> > >       }
> >
> >
> > --
> > Best regards,
> > Vladimir


--
Best regards,
Vladimir