Wouter,
On 13 Apr 2016, at 12:44, Wouter Verhelst <w...@uter.be> 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
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Nbd-general mailing list
Nbd-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nbd-general
