Re: [Nbd] NBD_CMD_DISC

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

On 9 Apr 2016, at 11:28, Wouter Verhelst <w@...112...> wrote:

>>> This will be effectively the same thing. The reply to NBD_CMD_DISC could
>>> even have the FIN flag set, too, resulting in no net benefit to the
>>> above.
>> 
>> I can't remember now exactly what Eric was suggesting (I think it was
>> a flag that effectively made NBD_CMD_DISC have the roughly the same
>> semantics as a close, i.e. gave a reply).
>> 
>> The space of available flags is limited. Why use one up? Before you
>> say 'well you're using a flag bit anyway', I'm using a flag bit
>> from the server to say it supports this, which we'd need on Eric's
>> proposal too (I think he converted over to the command approach).
> 
> I meant the TCP FIN flag (as in, FIN, FIN+ACK, ACK).

That's up to the TCP implementation. But I don't understand the above.

One problem I'm addressing is that the server doesn't know that the
client has done a clean close. This is because several clients do this

   write(NBD_CMD_DISC) ...
   // oh I've done, don't have to wait for reply and I wouldn't
   // know how long to wait
   close(fd) // if you're lucky - often this doesn't wait for TLS flush
   exit(0) // now all data in TLS buffers is lost.

What that does is not give the TLS layer the chance to send the packet
at all. Many TLS implementations (e.g. golang) don't give you the chance
to do a flush. And it isn't clear from the spec how long the client
should wait anyway.

The above looks to the server just like a dropped connection.

So better:
   write (NBD_CMD_DISC)
   read(REPLY)
   close(fd)
   exit(0).

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.

>>>> In other words, I'm not seeing what value added we have in either choice
>>>> 2 or choice 3 (what behavior did we guarantee by extending the
>>>> handshake, that we cannot already get by specifying best practice that
>>>> server MUST reply, and client SHOULD wait for server to close connection
>>>> but SHOULD NOT expect the reply due to back-compat?)
>>> 
>>> Not much. Even the reply doesn't seem necessary to me.
>> 
>> Without the reply, the client has no guarantee that the server has
>> run through the a safe disconnect.
> 
> So say that the server should not close the connection until it's
> flushed everything?

With NBD_CMD_CLOSE, yes.

> Clients can send FLUSH if they want to be sure.

Well they can, but we could make life easier for them, and for the
server that doesn't want to support NBD_CMD_FLUSH in general (for
instance because FLUSH is disproportionately slow).

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

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.

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

-- 
Alex Bligh