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

[Alex Bligh <alex@...872...>]

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.

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

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

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

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

>> 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.
Personally I would favour (at least) the server MUST reply to options
in the order they were sent.

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.

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

Indeed.

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

Yep, missed that, and don't like it!

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

-- 
Alex Bligh