Re: [Nbd] [PATCH] Docs: improve description of disconnection methods

[Wouter Verhelst <w@...112...>]

On Thu, Apr 14, 2016 at 12:29:09PM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 14 Apr 2016, at 07:49, Wouter Verhelst <w@...112...> wrote:
> 
> > On Wed, Apr 13, 2016 at 01:21:17PM +0100, Alex Bligh wrote:
> >> Secondly I think you are doing exactly what I said below. You
> >> are processing it immediately, but waiting for all commands
> >> to complete - "no thread of this pool is interrupted before
> >> processing a task". I can't recall whether in nbd-server
> >> that actually means the replies are sent - if I remember it
> >> has a mutex on the socket for that, rather than a separate
> >> non-pooled reply sending thread (which is what I have).
> > 
> > Okay, I'm confused now. I think it is okay for NBD_CMD_DISC to be sent
> > before all outstanding requests have been handled, but that the server
> > MUST ensure to NOT process that request (and drop the connection) until
> > that is the case.
> > 
> > If you disagree with that, please explain how. If you don't disagree
> > with that, I think your language is wrong and we need to clarify it.
> 
> There are two issues here.
> 
> Firstly, I would prefer that there is at least a SHOULD NOT condition
> against sending a disconnect before the client has received replies.
> You don't agree with that, and that's fine, I'm going with your
> view (at least for the purposes of this reply!).

Right. I think I understand why you want this, but I think we'll have to
agree to disagree on that one.

I'm going to veto it as a MUST. That means a server will have to do the
processing anyway, so it doesn't matter whether the client should or
should not do it.

> Secondly, I think there is another difference, which is down to
> semantics, and possibly a ambiguities in English (which
> means it does need clarifying).
> 
> I think by "process" you mean "finish processing" and I mean "start
> processing". Ditto with s/process/handle/.
> 
> You are saying (I think) that the server cannot process an NBD_CMD_DISC
> it until all requests are cleared. By 'process' you do not mean
> 'decode', you mean 'perform the disconnect'.

Right.

> I am saying that the server can process the NBD_CMD_DISC at any
> time, provided that the disconnect does not actually occur until
> all inflight requests have been handled.
> 
> IE in my language, 'process' includes the act of decoding, and processing
> *includes* waiting. This is probably because both the threaded
> servers I've written take commands off the wire, decode them, then
> send them to an arbitrary thread for 'processing', then indirect
> that 'processing' somehow. By the time a thread has discovered it
> has an NBD_CMD_DISC it's already 'processing' it in my language.

Okay, I understand what you mean now.

> I am also not keen on changing the simplicity of saying you can
> 'process' (meaning here 'start to process') the commands in any order,
> though there are constraints in what one can do within 'processing'.
> 
> I think here we're not arguing about two different behaviours (save
> for the bit re the client which we disagree on but never mind about
> that). I think we are arguing about how we express the correct
> behaviour clearly.
> 
> I would be worried about diluting the 'can process commands in
> any order'. I didn't realise that the when I was writing my
> kernel block driver, made the same mistake when writing my
> first NBD server, nearly made the same mistake when writing
> my second, and Eric's just shown how easy it is to make the
> mistake too.

Right. If you have better language, we can look at that, but indeed, we
don't seem to disagree then.

[...]
> Currently it just is not clear when the client and server are
> permitted to drop a connection without the use of NBD_CMD_DISC
> or NBD_OPT_ABORT. So I've had to invent some semantics there.
> Apart from some specific instances, there is currently no
> general rule for this.

I think there are three cases:

- After the client sent OPT_ABORT or CMD_DISC, the server may disconnect
- After either peer violated a MUST in the spec
- After the client send a valid but erroneous request (e.g.,
  OPT_EXPORT_NAME with a name that the server isn't exporting) for which
  the protocol doesn't provide a way out. We are in the process of
  resolving those.

-- 
< ron> I mean, the main *practical* problem with C++, is there's like a dozen
       people in the world who think they really understand all of its rules,
       and pretty much all of them are just lying to themselves too.
 -- #debian-devel, OFTC, 2016-02-12