On 03/30/2016 02:44 PM, Wouter Verhelst wrote: >> >> @@ -579,7 +602,7 @@ To remedy this, a `SELECT` extension is envisioned. This extension adds >> two option requests and one error reply type, and extends one existing >> option reply type. >> >> -* `NBD_OPT_SELECT` >> +* `NBD_OPT_SELECT` (6) > > NAK. The spec is currently consistent in associating numbers to > constants in only *one* place. This is no accident, and I'd like to keep > it that way. > > (at least I think it is; if not, that's a bug) I didn't realize it was intentional; I was trying to be consistent in the text I add, but I can be consistent in the opposite manner (only tie it to a value in one place). The spec does mention that the "server will set bit 0"; that might read better as "server will set the global flag `NBD_FLAG_FIXED_NEWSTYLE`" >> + The client wishes to use structured reads during the transmission >> + phase. The option request has no additional data. >> + >> + The server replies with one of the following: >> + >> + - `NBD_REP_ACK`: Structured reads have been negotiated; the server >> + MUST use structured replies to `NBD_CMD_READ` >> + - `NBD_REP_UNSUP`: Structured reads are not available; the transmission > ^ ERR_ > > (however, see below ;-) > >> + phase MUST remain the same as if the client had not attempted >> + `NBD_OPT_STRUCTURED_READ` > > This makes it seem as though NBD_REP_ERR_UNSUP has a different meaning > here than it usually does. I think the spec should just say that the > server should reply with NBD_REP_ACK, and then mention that for > backwards compatibility the client should be prepared to handle > NBD_REP_UNSUP too (as is done elsewhere). > > That is, if the server implements structured replies, it should allow it > (it makes no sense for the server to disallow structured reads if it > knows about them) Works for me. >> +* `NBD_CMD_FLAG_DF` (bit 1) >> + >> + Valid during `NBD_CMD_READ`. SHOULD be set to 1 if the client >> + requires the server to send at most one data chunk in reply. MUST >> + NOT be set unless the client negotiated Structured Reads with the >> + server. > > I realize I'm flip-flopping on whether or not to use a flag bit for > this, but bear with me on this one for a moment. > > There is an ioctl NBD_SET_FLAGS which just expects the per-export flags > to be passed to the kernel. By reusing that, there is no need for an > extra kernel call to specify the options the kernel-side client can use. > That still leaves the ability for userspace to detect whether the client > can interpret structured replies, but this could easily be signalled > using a sysfs flag (or similar). > > If the client can do something along the lines of: > > check sysfs (or whatever) > send NBD_OPT_STRUCTURED_READ to server > get flags from server > call NBD_SET_FLAGS ioctl > > and that signals *everything* to both the kernel and the server, then we > don't need any extra uapi calls to be defined. This is probably a good > thing. > > However, in order for that to be possible, the per-export flags field > needs to have a bit set to signal the server's understanding of, and > client's permission to use, NBD_CMD_FLAG_DF. As such, I think we need an > extra flag in the per-export flags field of the protocol, even though > we've already negotiated structured reads and I expressed the preference > that this shouldn't be decoupled. > > Yes, that's slightly ugly. So to recap, IF the client negotiates structured replies, then the server MUST set the export flag NBD_FLAG_SEND_DF. The user-space client then passes this flag to the kernel, and the kernel instantly knows that structured replies will be arriving, and that the kernel may use NBD_CMD_FLAG_DF on reads. > > Thinking about this, I suppose it makes sense to rename the "global" > flags field as the "negotiation flags field" which signals incompatible Maybe "handshake flags" instead, since we document handshake/transmission phases? > changes in the negotiation phase, and the "per-export" flags field as > the "transmission flags field" which signals features the client can use > during transmission, or something along those lines. Thoughts? The rename doesn't hurt anything (it is a document artifact only), and makes enough sense that we could do it as a pre-req patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature