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

[Vladimir Sementsov-Ogievskiy]

15.09.2021 12:00, Richard W.M. Jones wrote:
> On Wed, Sep 15, 2021 at 10:15:20AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > 14.09.2021 19:32, Richard W.M. Jones wrote:
> > > On Tue, Sep 14, 2021 at 06:21:58PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > 14.09.2021 17:52, Richard W.M. Jones wrote:
> > > > > 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.
> > >
> > > I would say the spec is at best contradictory, but if you read other
> > > parts of the spec, then it's pretty clear that we're allowed to drop
> > > the connection whenever we like.  This section says as much:
> > >
> > > https://github.com/NetworkBlockDevice/nbd/blob/5805b25ad3da96e7c0b3160cda51ea19eb518d5b/doc/proto.md#terminating-the-transmission-phase
> >
> > Hmm, that was exactly the section I read and quoted :)
> >
> > >    There are two methods of terminating the transmission phase:
> > >    ...
> > >    "The client or the server drops the TCP session (in which case it
> > >    SHOULD shut down the TLS session first). This is referred to as
> > >    'initiating a hard disconnect'."
> >
> > Right. Here the method is defined, no word that client can do it at any time.
>
> I don't read this as a definition.

If so, next paragraphs that specify client behavior doesn't make sense.

> But we should probably clarify the spec to note that dropping the
> connection is fine, because it is - no one has made any argument so
> far that there's an actual reason to waste time on NBD_CMD_DISC.

I agree that it's safe enough..

Hmm. If we update the spec to clarify that sending DISC is not necessary, is 
there any reason to send it at all? We can stop sending it in Qemu as well.


> > Next, in same section:
> >
> >     Either side MAY initiate a hard disconnect if it detects a violation by the 
> > other party of a mandatory condition within this document.
> >
> > Next
> >     The client MAY issue a soft disconnect at any time
> >
> > And finally:
> >
> >     The client and the server MUST NOT initiate any form of disconnect other 
> > than in one of the above circumstances.
> >
> >
> > Or do you mean some other spec section I missed?
> >
> > > Anyway we're dropping the TCP connection because sometimes we are just
> > > interrogating an NBD server eg to find out what it supports, and doing
> > > a graceful shutdown is a waste of time and internet.
> > >
> > > > > 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?
> > >
> > > Our contract with callers using nbd_close(3), if nbd_shutdown(3) is
> > > not called beforehand.
> > > https://libguestfs.org/nbd_shutdown.3.html
> > > https://libguestfs.org/nbd_create.3.html (really nbd_close ...)
> > >
> > > Rich.
> >
> >
> > --
> > Best regards,
> > Vladimir


--
Best regards,
Vladimir