On 9 Apr 2016, at 11:28, Wouter Verhelst <[email protected]> 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 ------------------------------------------------------------------------------ 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! http://pubads.g.doubleclick.net/ gampad/clk?id=1444514301&iu=/ca-pub-7940484522588532 _______________________________________________ Nbd-general mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/nbd-general
