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

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

Hi Alex,

On Wed, Apr 13, 2016 at 11:25:02AM +0100, Alex Bligh wrote:
> Wouter,
> 
> >> +A client MAY use a soft disconnect to terminate the session
> >> +whenever it wishes, provided that there are no outstanding
> >> +replies to options.
> > 
> > NAK. A client MAY use a soft disconnect *at any time*, but the server
> > MUST NOT act upon it until there are no outstanding replies, and the
> > client MUST NOT send any further options after sending NBD_OPT_ABORT.
> > 
> > (same for CMD_DISC)
> 
> This gets to the root of the unresolved issues I think. I suspect
> the answer may be different for NBD_OPT_ABORT and NBD_CMD_DISC.
> 
> NBD commands are asynchronous and can be replied and acted on
> in any order (so, from the document):
>  
>   "The server MAY process commands out of order, and MAY reply
>    out of order"
> 
> I wouldn't want to loose that. So if the client sends NBD_CMD_DISC
> without waiting for all his inflight commands to complete, those
> inflight commands may not be executed at all, because the server
> is free to process commands in any order. It's going to make
> server design very awkward if you can only process /some/ commands
> out of order.

It's actually fairly easy. Current nbd-server does this
(mainloop_threaded):

                if(req->type == NBD_CMD_DISC) {
                        g_thread_pool_free(tpool, FALSE, TRUE);
                        return 0;
                }

The "return 0" causes it to exit mainloop_threaded, which causes the
server to do its final cleanup and exit.

The second argument is "immediate", which is documented like so:

  If immediate is TRUE, no new task is processed for pool. Otherwise pool is
  not freed before the last task is processed. Note however, that no thread of
  this pool is interrupted while processing a task. Instead at least all still
  running threads can finish their tasks before the pool is freed.

IOW (since we use FALSE), we finish whatever outstanding requests are
queued, and then close the TCP connection.

That's all the handling that NBD_CMD_DISC (currently) requires.

What's awkward about that?

Note that NBD_CMD_DISC is the *only* command for which I think it makes
sense to have ordering requirements. Everything else should be fair
game. We could perhaps make that more explicit in the "Ordering of
messages and writes" section?

> Another alternative would be to make the server
> wait for all commands to complete before acting on the disconnect
> (as opposed to or in addition to making the client wait to send
> it). I'm reasonably relaxed about which one we do, but I think
> we should do one or the other (or at least say that if the
> client sends NBD_CMD_DISC without waiting for commands to complete
> then those commands must not be executed).

I think that is obviously wrong, and we should not say that.

> There are thus various choices for NBD_CMD_DISC.
> 
> I think the option haggling phase is different (or rather need
> not be the same). Fundamentally options MUST be processed in
> the order they are issued, and there is only ever one in
> flight at a time. My understanding is (though perhaps this
> not explicit in the document) that the client should not be
> sending ANY option until it has got a reply to the last one.

Well, the text already says

  As there is no unique number for client requests, clients who want to
  differentiate between answers to two instances of the same option
  during any negotiation must make sure they've seen the answer to an
  outstanding request before sending the next one of the same type. The
  server MAY send replies in the order that the requests were received,
  but is not required to.

So no, you're wrong here, too ;-)

Since the option reply contains the request type, too, clients can
differentiate between two requests of different type, but not two
requests of the same type.

> Certainly I know of no servers which can process options in
> parallel, and as we don't have an 'Id' field to line up
> replies they would have to be processed sequentially anyway.
> I think we should document (somewhere) that the client MUST NOT
> send an option until it has received a final reply to the previous
> option. If we don't do that, we should document how concurrency
> with options is meant to work.

We already do :)

> So for this case I think it is completely correct that NBD_OPT_ABORT
> must not (like any other option) be sent until there are no
> outstanding *option* replies.
> 
> Let's see if we can resolve this one on list.

I think the current text is clear enough, and we don't need to resolve
anything (although clarifying some things might make sense).

> >> +terminate the session. In the client's case, if it wishes to
> >> +do so it MUST use soft disconnect. In the server's case it
> >> +MUST (save where set out above) simply error inbound options until
> >> +the client gets the hint that it is unwelcome.
> > 
> > It might be good to add a "NBD_REP_ERR_NOSERVICE" error, for "server
> > shutting down"?
> 
> I think that's probably a good idea. I suspect I'm going to go
> and remove the bit that prescribes exactly what errors are permitted
> for NBD_OPT_STARTTLS etc. as in the past 2 days we've now found two more
> (NBD_REP_ERR_PLATFORM and this)!

That might be a good idea.

[...]
> >> -Clients MUST NOT set any other flags; the server MUST drop the
> >> +Clients MUST NOT set any other flags; the server MUST drop the TCP
> > 
> > initiate a hard disconnect?
> 
> My thought was that we aren't (yet) in the option haggling phase,
> so should not use option haggling terminology.

OIC. Hrm. Perhaps.

-- 
< 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