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

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

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

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

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

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.

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.

>> 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 think it boils down to the same behaviour, but is explained
in a different way. I think my way is clearer.

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

I'm not proposing changing it in this change set. You are right
this is permitted by the current standard (even if I don't like
it).

I just didn't realise that and may need to adjust the patch occasionally.

This is a discussion for another time.

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

Indeed.

> 
> [...]
>>>> 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*.

So it seems (+/- the question on *client* behaviour) I wasn't
trying to change the semantics for NBD_CMD_DISC, I was trying to
clarify them.

I was trying to change the semantics for NBD_OPT_ABORT (by
adding a reply) - which I think comes under this category:

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

... and I was also trying to document some stuff that simply
wasn't handled at all (dropping the connection, especially
w.r.t. TLS handling, when it can be done etc).

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.

-- 
Alex Bligh