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

[Alex Bligh]

Wouter,

>> 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!).
> 
> Right. I think I understand why you want this, but I think we'll have to
> agree to disagree on that one.
> 
> I'm going to veto it as a MUST. That means a server will have to do the
> processing anyway, so it doesn't matter whether the client should or
> should not do it.

Given the veto, I buy that argument. Especially given the server
should 'fail safely'. I think anyone who wrote a server that didn't
do the stuff below and relied on the client would be ... foolhardy.

>> 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'.
> 
> Right.
> 
>> 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.
> 
> Okay, I understand what you mean now.
> 
>> 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.
> 
> Right. If you have better language, we can look at that, but indeed, we
> don't seem to disagree then.

I'd *thought* that was the patch I sent through. I think I need
to emasculate^Wamend it though to delete the stuff you're not
happy with as perhaps the good parts of the changes got lost ...

> 
> [...]
>> 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.
> 
> I think there are three cases:
> 
> - After the client sent OPT_ABORT or CMD_DISC, the server may disconnect

yes

> - After either peer violated a MUST in the spec

yes

> - After the client send a valid but erroneous request (e.g.,
>  OPT_EXPORT_NAME with a name that the server isn't exporting) for which
>  the protocol doesn't provide a way out. We are in the process of
>  resolving those.

yes

There's also "the server is shutting down". It can (as
Erik suggests) error any new commands with 'ELP0ONFIRE'
or whatever, but eventually it's going to have to close
the TCP connection.

Beyond that, I don't think there are any more cases.

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

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

[Wouter Verhelst]

On Thu, Apr 14, 2016 at 12:29:09PM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 14 Apr 2016, at 07:49, Wouter Verhelst  wrote:
> 
> > On Wed, Apr 13, 2016 at 01:21:17PM +0100, Alex Bligh wrote:
> >> 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!).

Right. I think I understand why you want this, but I think we'll have to
agree to disagree on that one.

I'm going to veto it as a MUST. That means a server will have to do the
processing anyway, so it doesn't matter whether the client should or
should not do it.

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

Right.

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

Okay, I understand what you mean now.

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

Right. If you have better language, we can look at that, but indeed, we
don't seem to disagree then.

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

I think there are three cases:

- After the client sent OPT_ABORT or CMD_DISC, the server may disconnect
- After either peer violated a MUST in the spec
- After the client send a valid but erroneous request (e.g.,
  OPT_EXPORT_NAME with a name that the server isn't exporting) for which
  the protocol doesn't provide a way out. We are in the process of
  resolving those.

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

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

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

[Alex Bligh]

Wouter,

On 14 Apr 2016, at 07:49, Wouter Verhelst  wrote:

> On Wed, Apr 13, 2016 at 01:21:17PM +0100, Alex Bligh wrote:
>> Wouter,
>> 
>> On 13 Apr 2016, at 12:44, Wouter Verhelst  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 

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

[Wouter Verhelst]

On Wed, Apr 13, 2016 at 05:09:49PM +0100, Alex Bligh wrote:
[...]
> On 13 Apr 2016, at 16:39, Eric Blake  wrote:
> > and at the same time that a client SHOULD wait until
> > there are no inflight commands before sending NBD_CMD_DISC.
> 
> I like that. I think Wouter doesn't.

I'm less opposed to SHOULD than I am to MUST, but I don't think it's
necessary.

[...]

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

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

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

[Wouter Verhelst]

On Wed, Apr 13, 2016 at 01:21:17PM +0100, Alex Bligh wrote:
> Wouter,
> 
> On 13 Apr 2016, at 12:44, Wouter Verhelst  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 

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

[Alex Bligh]

On 13 Apr 2016, at 17:09, Alex Bligh  wrote:

> Here's what
> https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt
> has to say:

Aha. I found the original thread where I asked about this
(it was on linux-fsdevel):

http://www.spinics.net/lists/linux-fsdevel/msg45584.html

Specifically this from Tejun Heo who I believe is/was a block
layer / fs layer maintainer:

http://www.spinics.net/lists/linux-fsdevel/msg45616.html
> On Wed, May 25, 2011 at 5:54 PM, Alex Bligh  wrote:
> > a) If I do not complete a write command, I may avoid writing it to disk
> >  indefinitely (despite completing subsequently received FLUSH
> >  commands). The only flushes to disk that I am obliged to flush
> >  are those that I've actually told the block layer that I have done.
> 
> Yes, driver doesn't have any ordering responsibility w.r.t. FLUSH for
> writes which it hasn't declared finished yet.

> > b) If I receive a flush command, and prior to completing that flush
> >  command, I receive subsequent write commands, I may execute
> >  (and, if I like, write, to disk) write commands received AFTER that
> >  flush command. I presume if the subsequent write commands write to
> >  blocks that I am meant to be flushing, I can just forget about
> >  the blocks I am meant to be flushing (because they would be
> >  overwritten) provided *something* overwritten what was there before.
> 
> The first half is correct.  The latter half may be correct if there's
> no intervening write but _please_ don't do that.  If there's something
> to be optimized there, it should be done in upper layers.  It's
> playing with fire.



--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail
--
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

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

[Alex Bligh]

On 13 Apr 2016, at 16:39, Eric Blake  wrote:

>> 
>> 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.
> 
> We already have that constraint - commands with NBD_CMD_FLAG_FUA must be
> processed in a particular order,

No, that is not true. Commands with NBD_CMD_FUA set may be
processed in any order with respect to other commands. They just
MUST NOT reply until the data within them is written to the disk.
This is exactly per the linux block layer.

Here's what the spec says.

 "A server MUST NOT reply to a command that has NBD_CMD_FLAG_FUA set
 in its command flags until the data (if any) written by that command
 is persisted to non-volatile storage."

It is perfectly possible to reorder a FUA write to run before
a another write that is issued first, or behind another write
that is issued after.

FUA is completely independent of ordering.

> and NBD_CMD_FLUSH must be processed in
> a particular order.

No, that's not true either (but you're closer).

Here's what the spec says (again this is the same as the Linux block
layer:

"All write commands (that includes NBD_CMD_WRITE, NBD_WRITE_ZEROES
and NBD_CMD_TRIM) that the server completes (i.e. replies to) prior
to processing to a NBD_CMD_FLUSH MUST be written to non-volatile
storage prior to replying to that NBD_CMD_FLUSH"

That is NOT imposing an ordering constraint on processing commands.
It's saying that when you reply to NBD_CMD_FLUSH all the writes
that have been *replied to* must be completed. So you are at
liberty to process the flush before all your pending writes and
not flush them if you want. You can thus reorder your flushes
as much as you like; if the client doesn't want them reordered
it should not issue the flush until after the writes have
replied.

Strange but true, but this is how the Linux block layer works.

(BTW this was precisely the point I was trying to clarify
earlier on, and I had an additional 'SHOULD' behaviour to
advise against this, and people wanted it out).

Here's what
https://www.kernel.org/doc/Documentation/block/writeback_cache_control.txt
has to say:

> Explicit cache flushes
> --
> 
> The REQ_FLUSH flag can be OR ed into the r/w flags of a bio submitted from
> the filesystem and will make sure the volatile cache of the storage device
> has been flushed before the actual I/O operation is started.  This explicitly
> guarantees that previously completed write requests are on non-volatile
> storage before the flagged bio starts. In addition the REQ_FLUSH flag can be
> set on an otherwise empty bio structure, which causes only an explicit cache
> flush without any dependent I/O.  It is recommend to use
> the blkdev_issue_flush() helper for a pure cache flush.

Note that it only talks about COMPLETED writes.

There is no restriction on reordering writes.

> Requiring NBD_CMD_DISC to be processed last is not
> much different than these other two situations (well, different in that
> the other two only have to guarantee that commands _with replies_ have
> hit permanent storage, not ALL commands received).

Seems to me very different as I don't think those ordering constraints
you mention actually exist! Certainly nbd-server.c does not implement
them and neither does gonbdserver.

But:

>> 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). There are thus
>> various choices for NBD_CMD_DISC.
> 
> I think it is perfectly fair to put the requirement on the server that
> it MUST wait until all inflight commands have been responded to before
> disconnecting;

I suspect we'll end up with that.

> and at the same time that a client SHOULD wait until
> there are no inflight commands before sending NBD_CMD_DISC.

I like that. I think Wouter doesn't.

>> 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,
> 
> No, we already explicitly state that options may be replied to
> out-of-order, and that the burden is on the client to wait for
> particular replies before sending another option of the same type.

Yeah so Wouter pointed out :-(

>> and there is only ever one in
>> flight at a time.
> 
> No, a client can batch send a bunch of options before waiting for any
> replies.

Yeah so Wouter pointed out :-(

>>> 
>>> It might be good 

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

[Alex Bligh]

Wouter,

On 13 Apr 2016, at 12:44, Wouter Verhelst  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 

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

[Wouter Verhelst]

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

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

[Alex Bligh]

Eric,

Agree with the nits - many of them were from the mailing list
message which of course I then didn't check before copying
into the commit message.

Re the substance:

>> +* Transmission mode can be entered (by the client sending
>> +  `NBD_OPT_EXPORT_NAME` or by the server responding to an
>> +  `NBD_OPT_GO` with `NBD_REP_ACK`). This is documented
> 
> s/ACK/SERVER/

yep

> (although I may bite the bullet and create a new NBD_REP_INFO if we want
> the name to be optional, since NBD_OPT_[INFO/GO] is still experimental,
> as part of my rework on block size information)

indeed, but that's orthogonal.

>> +A client MAY use a soft disconnect to terminate the session
>> +whenever it wishes, provided that there are no outstanding
>> +replies to options.
> 
> Why the disclaimer on no outstanding replies?

See reply to Wouter who made the same point. Let's handle that
there.

>> +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.
> 
> so basically wait for either the client to give up and close first, or
> for the client to do something that is provably in violation of a MUST
> in the protocol so the server can close the connection.  Can a malicious
> client abuse this requirement to tie up a server as a denial of service?

Good point. I think we should give the server the right to disconnect
in a DoS situation. This is a bit like DoS protection for TCP violating
the TCP spec though.

>> +On a server shutdown, the server SHOULD wait for inflight
>> +requests to be serviced prior to initiating a hard disconnect.
> 
> Maybe a mention that the server MAY use error replies to speed up the
> processing of those requests, even if the command would normally succeed
> if termination weren't pending?

+1, and as Wouter suggested, use a different reply.

>> +The client MAY issue a soft disconnect at any time, but
>> +MUST wait until there are no inflight requests first.
> 
> Why MUST and not SHOULD?  Didn't Wouter have an example of a client that
> batches up its entire request sequence, including NBD_CMD_DISC, and
> sends that in bulk before waiting for any server replies?  I thought the
> goal was that the server MUST NOT react to NBD_CMD_DISC until all other
> pending requests have been dealt with, but don't necessarily see the
> reason why the client MUST NOT send NBD_CMD_DISC while requests are
> inflight.

The issue is that the server MAY process requests out of order. I thus
think such a client is foolhardy as the server MAY process the NBD_CMD_DISC
first. It depends whether that 'processing' includes 'waiting for all
the other commands'. Again, see my reply to Wouter - this is definitely
an area we need to sort out.

I agree though that MUST is too strong. I think I perhaps I'd say
it may send it if it wishes, but should remember the server can process
replies out of order.

>> - `NBD_OPT_ABORT` (2)
>> 
>> -The client desires to abort the negotiation and close the
>> -connection.
>> +The client desires to abort the negotiation and terminate the
>> +session. The server MUST reply with `NBD_REP_ACK`.
> 
> Maybe explicitly mention that the client MAY disconnect immediately
> rather than waiting to receive the response?

I'm saying the client MUST wait. But if it doesn't (meaning only
clients will be non-conformant) nothing is lost, particular as
this is only at option haggling stage. So it's more (as Wouter
said) "be aware that there may old non-compliant clients that
will not wait".

--
Alex Bligh






signature.asc
Description: Message signed with OpenPGP using GPGMail
--
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

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

[Wouter Verhelst]

On Tue, Apr 12, 2016 at 08:31:33PM +0100, Alex Bligh wrote:
> Improve the documentation as per the mailing list discussion.
> Here's what we deciced (broadly).
> 
> * One side MAY drop the connection if the other end violates a
>  MUST condition.
> 
> * The server MUST drop the connection in the 'no way out' situations
>  during the negotiation phase (error on NBD_OPT_EXPORT_NAME, error
>  in negotiating text).
> 
> * The server SHOULD NOT otherwise drop the connection. It can wait
>  and error the next command. Clearly there are situations where
>  this is going to happen (e.g. server shutdown).
> 
> * If the server does need to drop the connection, it SHOULD wait
>  until there are no commands in-flight in transmission mode,
>  it possible.
> 
> * If he client is going to drop the the connection, then other
>  than in the event of a protocol violation or a 'no way out'
>  situation (e.g. TLS negotiation fails), it MUST use NBD_CMD_DISC
>  or NBD_OPT_ABORT
> 
> * We should tidy up the semantics and descriptions of NBD_CMD_DISC
>  and NBD_OPT_ABORT, viz replies or not to the latter, shutting
>  down TLS properly etc.
> 
> Other changes:
> 
> * Added a reply to NBD_OPT_ABORT. No harm if the client closes
>   the connection anyway.
> 
> * Said the offset and length fields in NBD_CMD_DISC MUST be zero.
>   Not doing so is a protocol violation and would only lead to ...
>   the connection being closed, so this is a useful tidy up.
> 
> * Introduced consistent terminology for disconnection throughout.
> 
> This patch applies on top of:
>   0001-docs-proto.md-Clarify-SHOULD-MUST-MAY-etc v7
> 
> Signed-off-by: Alex Bligh 
> ---
>  doc/proto.md | 143 
> ---
>  1 file changed, 107 insertions(+), 36 deletions(-)
> 
> diff --git a/doc/proto.md b/doc/proto.md
> index b88e054..db6b96d 100644
> --- a/doc/proto.md
> +++ b/doc/proto.md
> @@ -122,7 +122,7 @@ C: 32 bits, flags
>  This completes the initial phase of negotiation; the client and server
>  now both know they understand the first version of the newstyle
>  handshake, with no options. The client SHOULD ignore any handshake flags
> -it does not recognize, while the server MUST close the connection if
> +it does not recognize, while the server MUST close the TCP connection if
>  it does not recognize the client's flags.  What follows is a repeating
>  group of options. In non-fixed newstyle only one option can be set
>  (`NBD_OPT_EXPORT_NAME`), and it is not optional.
> @@ -150,8 +150,8 @@ S: 16 bits, transmission flags
>  S: 124 bytes, zeroes (reserved) (unless `NBD_FLAG_C_NO_ZEROES` was
> negotiated by the client)  
>  
> -If the server is unwilling to allow the export, it SHOULD close the
> -connection.
> +If the server is unwilling to allow the export, it MUST terminate
> +the session.
>  
>  The reason that the flags field is 16 bits large and not 32 as in the
>  oldstyle negotiation is that there are now 16 bits of transmission flags,
> @@ -201,22 +201,60 @@ 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.
>  
> + Termination of the session during option haggling
> +
> +There are three possible mechanisms to end option haggling:
> +
> +* Transmission mode can be entered (by the client sending
> +  `NBD_OPT_EXPORT_NAME` or by the server responding to an
> +  `NBD_OPT_GO` with `NBD_REP_ACK`). This is documented
> +  elsewhere.
> +
> +* The client can send (and the server can reply to) an
> +  `NBD_OPT_ABORT`. This MUST be followed by the client
> +  shutting down TLS (if it is running), and the client
> +  dropping the connection. This is referred to as
> +  'initiating a soft disconnect'; soft disconnects can
> +  only be initiated by the client.
> +
> +* The client or the server can disconnect the TCP session
> +  without activity at the NBD protocol level. If TLS is
> +  negotiated, the party intitiating the transaction SHOULD
> +  shutdown TLS first if it is running. This is referrred
> +  to as 'initiating a hard disconnect'.
> +
> +This section concerns the second and third of these, together
> +called 'terminating the session', and under which circumstances
> +they are valid.
> +
> +If either the client or the server detects a violation of a
> +mandatory condition ('MUST' etc.) by the other party, it MAY
> +initiate a hard discconect.
> +
> +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)

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

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

[Eric Blake]

On 04/12/2016 01:31 PM, Alex Bligh wrote:
> Improve the documentation as per the mailing list discussion.
> Here's what we deciced (broadly).

s/deciced/decided/

> 
> * One side MAY drop the connection if the other end violates a
>  MUST condition.
> 
> * The server MUST drop the connection in the 'no way out' situations
>  during the negotiation phase (error on NBD_OPT_EXPORT_NAME, error
>  in negotiating text).

negotiating text, or negotiating TLS?

> 
> * The server SHOULD NOT otherwise drop the connection. It can wait
>  and error the next command. Clearly there are situations where
>  this is going to happen (e.g. server shutdown).
> 
> * If the server does need to drop the connection, it SHOULD wait
>  until there are no commands in-flight in transmission mode,
>  it possible.
> 
> * If he client is going to drop the the connection, then other

s/he/the/
s/the the/the/

>  than in the event of a protocol violation or a 'no way out'
>  situation (e.g. TLS negotiation fails), it MUST use NBD_CMD_DISC
>  or NBD_OPT_ABORT
> 
> * We should tidy up the semantics and descriptions of NBD_CMD_DISC
>  and NBD_OPT_ABORT, viz replies or not to the latter, shutting
>  down TLS properly etc.
> 
> Other changes:
> 
> * Added a reply to NBD_OPT_ABORT. No harm if the client closes
>   the connection anyway.
> 
> * Said the offset and length fields in NBD_CMD_DISC MUST be zero.
>   Not doing so is a protocol violation and would only lead to ...
>   the connection being closed, so this is a useful tidy up.
> 
> * Introduced consistent terminology for disconnection throughout.
> 
> This patch applies on top of:
>   0001-docs-proto.md-Clarify-SHOULD-MUST-MAY-etc v7
> 
> Signed-off-by: Alex Bligh 
> ---
>  doc/proto.md | 143 
> ---
>  1 file changed, 107 insertions(+), 36 deletions(-)
> 

> + Termination of the session during option haggling
> +
> +There are three possible mechanisms to end option haggling:
> +
> +* Transmission mode can be entered (by the client sending
> +  `NBD_OPT_EXPORT_NAME` or by the server responding to an
> +  `NBD_OPT_GO` with `NBD_REP_ACK`). This is documented

s/ACK/SERVER/

(although I may bite the bullet and create a new NBD_REP_INFO if we want
the name to be optional, since NBD_OPT_[INFO/GO] is still experimental,
as part of my rework on block size information)

> +  elsewhere.
> +
> +* The client can send (and the server can reply to) an
> +  `NBD_OPT_ABORT`. This MUST be followed by the client
> +  shutting down TLS (if it is running), and the client
> +  dropping the connection. This is referred to as
> +  'initiating a soft disconnect'; soft disconnects can
> +  only be initiated by the client.
> +
> +* The client or the server can disconnect the TCP session
> +  without activity at the NBD protocol level. If TLS is
> +  negotiated, the party intitiating the transaction SHOULD

s/intitiating/initiating

> +  shutdown TLS first if it is running. This is referrred

s/referrred/referred/

> +  to as 'initiating a hard disconnect'.
> +
> +This section concerns the second and third of these, together
> +called 'terminating the session', and under which circumstances
> +they are valid.
> +
> +If either the client or the server detects a violation of a
> +mandatory condition ('MUST' etc.) by the other party, it MAY
> +initiate a hard discconect.

s/discconect/disconnect/

> +
> +A client MAY use a soft disconnect to terminate the session
> +whenever it wishes, provided that there are no outstanding
> +replies to options.

Why the disclaimer on no outstanding replies?

> +
> +A party that is mandated by this document to terminate the
> +session MUST initiate a hard disconnect if it is not possible
> +to use a soft disconnect. Such circumstances include: where
> +that party is the server and it cannot return an error
> +(e.g. after an `NBD_OPT_EXPORT_NAME` it cannot satisfy),
> +and where that party is the client following a failed TLS
> +negotiation.
> +
> +A party MUST NOT initiate a hard disconnect save where set out
> +in this section. Therefore, unless a client's situation falls
> +within the provisions of the previous paragraph, it MUST NOT
> +use a hard disconnect, and hence its only option to terminate
> +the session is via a soft disconnect.
> +
>  There is no requirement for the client or server to complete a
>  negotiation if it does not wish to do so. Either end MAY simply
> -close the TCP connection (though see below regarding prior use
> -of NBD_OPT_ABORT). Under certain circumstances either
> -the client or the server may be required by this document to close
> -the TCP connection. In each case, this is referred to as 'terminate
> -the session'.
> -
> -If the client wishes to terminate the session in the negotiation
> -phase, and is not doing so because it is required to do so
> -by this document, it SHOULD send NBD_OPT_ABORT first if the protocol
> -permits. There are instances where this is