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

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

On Wed, Apr 13, 2016 at 01:21:17PM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 13 Apr 2016, at 12:44, Wouter Verhelst <w@...112...> wrote:
> 
> > 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?
> 
> Well, firstly not everyone uses your threading mechanism.

Sure.

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

> > 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?
> 
> I think that would be pretty foul. Especially as you actually
> seem to do exactly what I suggest below!
> 
> >> 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).
> 
> So isn't this what you are doing? Waiting (in g_thread_pool_free)
> for the existing requests to finish?

Yes.

> That's far better than saying mucking around with the
> statement on ordering.

Isn't that the same thing?

> >> 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.
> 
> I meant that if it was permitted behaviour, we should document
> it. I agree that I'd like it not to be permitted behaviour!

Good :-)

> >> 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 ;-)
> 
> Yuck! I'd much rather this was a serial process (or at least lock-step).
> I don't know of any server that doesn't implement it that way.
> Basically the current semantics say "well, you can send what you like
> but unless you wait for a reply first, the replies may be disordered
> and you may not be able to disambiguate them". That's not very pretty.

If it's a problem for a client, it should just not send things until
it's seen the relevant reply...

> Personally I would favour (at least) the server MUST reply to options
> in the order they were sent.

This works for today's option requests, but I can imagine a future
wherein some option might require a *lot* of processing to be handled,
and that in that case it might be more useful for a client to do
something like

send NBD_OPT_EXPENSIVE
send NBD_OPT_X
send NBD_OPT_Y

with the expectation that the server would handle NBD_OPT_EXPENSIVE in
the background while handling NBD_OPT_X and NBD_OPT_Y immediately.

If we declare now that the server may not do that under any
circumstance, we impede ourselves for future possibilities.

> Whatever, I maintain we don't necessarily need to end up with the
> sane answer for NBD_OPT_ABORT as NBD_OPT_DISC. Another reason is
> that in the option haggling phase, no data can be at risk.

I agree that we don't necessarily need the same semantics for OPT_ABORT
as for CMD_DISC. But that doesn't mean it's bad if they're similar.

[...]
> >> 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).
> 
> You mean I should drop the patch entirely? I thought you
> wanted it clarified?

I mean we don't need to modify the semantics of the commands; they are
clear, and don't need 'resolving'. I don't mind updating the text to
clarify what the semantics are, but I don't think they need to be
*changed*.

Of course, that doesn't mean I can't be convinced otherwise by good
arguments, but that's a different matter.

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