Re: [Nbd] NBD_CMD_DISC

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

On Sat, Apr 09, 2016 at 12:14:02PM +0100, Alex Bligh wrote:
> If you're arguing the reply doesn't add anything, in a sense you're
> right, but only in the same way NBD_CMD_DISC itself doesn't add anything,
> as you *could* just permit straight disconnections without a command
> at all - they would be just as safe/unsafe as with NBD_CMD_DISC
> as with NBD_CMD_DISC as currently documented a legally operating
> client can actually disconnect intending to send NBD_CMD_DISC but
> with it not actually reach the server.

My point is, it's fairly easy to clarify that that is not legal. If you
*intend* to send NBD_CMD_DISC, you should ensure that you've actually done so
before dropping the connection!

[...]
> > Clients can send FLUSH if they want to be sure.
> 
> Well they can, but we could make life easier for them,

I don't see how "add another command with almost-but-not-quite the same
semantics" makes life easier for anyone...

> > Still not convinced.
> > 
> >> An alternative approach would be to load all the effort onto the client,
> >> I suppose, i.e. first wait until all inflight requests have been
> >> transmitted, then send an NBD_CMD_FLUSH, wait for the reply to that,
> >> and then send an NBD_CMD_DISC. But in that case, we don't even need
> >> NBD_CMD_DISC. The client might as well just disconnect.
> > 
> > The server may *also* want to know when a client wants to disconnect,
> > for whatever reason.
> 
> Sure, it will know when the TCP stream is terminated.

So will the client?

> That is the only notification it currently gets *now* in some
> circumstances, as the NBD_CMD_DISC gets lost (occasionally) as the
> client has exited before its TLS buffer gets flushed.

If the command gets lost, clearly the client didn't do its job
correctly...

> >> If you're concerned about saving commands, the easy answer is this:
> > 
> > I'm not concerned about saving commands so much as about complicating
> > things too much. NBD is (or at the very least, used to be ;-) a fairly
> > simple protocol to implement. That's a feature, one which I would like
> > to keep.
> 
> This is a reasonable design objective and one I am grateful for
> having implemented 2 NBD servers and worked on the reference
> implementation) :-)
> 
> > If there is a reason why we can't define that a server SHOULD behave in
> > a particular way upon CMD_DISC, then fine, we can add a CLOSE.
> > Otherwise, I think this is complicating matters for little benefit.
> 
> OK, well having thought about it I *still* think NBD_CMD_DISC as
> currently written is broken. I didn't come up with the problem as
> a pipedream, I came up with it because I couldn't help wondering
> why my server was spitting warning messages on every fifth
> 'qemu-img info' command saying the client has disconnected without
> sending an NBD_CMD_DISC.

That sounds like a bug in qemu-img then, rather than a bug in the
protocol?

> I think there are two possible courses of action.
> 
> 1. Introduce a disconnect with a reply (either NBD_CMD_CLOSE
>    or some sort of flag to indicate NBD_CMD_DISC will send
>    a reply so the client can wait for it and close the channel
>    itself).
> 
> 2. Make it legal for the client to close the communication when
>    there is nothing in flight, and after a flush (if the
>    server supports it). Make this the preferred method for
>    disconnection.

3. Clarify that NBD_CMD_DISC is an *intent* to close the connection, but
that whatever is outstanding should still be handled before doing so. If
a client sends FLUSH and DISC in rapid succession, the server should
handle the FLUSH before handling the disconnect.

That doesn't require any semantic changes (the client needs to wait for
the FLUSH response anyway), and gets you the certainty you're asking
for.

> On reflection, I think (2) is far easier. It has the following
> advantages:
> 
> * Server implementation is trivial as a client disconnect
>   needs to be monitored anyway. If the server is multithreaded
>   it can produce a warning if there are inflight commands, else
>   not.
> 
> * Client implementation is trivial too - simply don't bother
>   sending the NBD_CMD_DISC. It should send a FLUSH first (which
>   it should be doing anyway probably), and it should wait for
>   all inflight commands to complete (which it presumably should
>   be doing anyway).

Downside: servers now need to distinguish between "connection drop with
outstanding requests" and "connection drop with *no* outstanding
requests", and perhaps do different clean up in the latter case but not
the former, because the former is an unclean disconnect and the latter
isn't.

Having an explicit message to end the client side of the conversation
does away with that problem, and is why it was originally introduced;
the original version of the protocol did not, in fact, have an
NBD_CMD_DISC message.

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