Re: [Qemu-devel] [Nbd] [PATCH 3/1] doc: Propose Structured Replies extension

[Wouter Verhelst <w@uter.be>]

[-- Attachment #1: Type: text/plain, Size: 5090 bytes --]

On Tue, Mar 29, 2016 at 09:12:02AM -0600, Eric Blake wrote:
> On 03/29/2016 08:37 AM, Alex Bligh wrote:
> > Eric,
> > 
> >> I guess what I need to add is that in transmission phase, most commands
> >> have exactly one response per request; but commands may document
> >> scenarios where there will be multiple responses to a single request.
> >> NBD_CMD_READ uses the multiple responses to make partial read and error
> >> handling possible
> > 
> > Yes, this.
> > 
> 
> >> Are you arguing that there should be a flag that controls whether reads
> >> must be in-order vs. reassembled?  Reassembly must happen either way,
> >> the question is whether having a way to allow out-of-order for
> >> efficiency, vs. defaulting to in-order for easier computation, is worth it.
> > 
> > No, that sounds overengineered.
> > 
> > More a way of guaranteeing avoiding a fragmentation on 'simple' reads.
> > Perhaps a 'DF' bit (don't fragment)! If the server doesn't like it, it
> > can always error the command.
> 
> Okay, that makes sense.  Does reusing NBD_CMD_FLAG_FUA sound reasonable
> for this purpose, or should it be a new flag?  I guess from the
> standpoint of client/server negotiation, we want to support this
> don't-fragment request even if NBD_FLAG_SEND_FUA was not listed in
> export flags, so it sounds like a new flag is best.  Next, should it be
> independently negotiated, or implied by negotiating
> NBD_FLAG_C_STRUCTURED_REPLIES?  I'm leaning towards implied - it's

I think it should be implied, yes.

Having said that,

There's only a limited number of flag bits available. We can obviously
always add more flags by adding in a second flags field, but that
introduces more backwards compatibility issues (would require another
global flag to say "we support extended flags", which the client then
has to ack, too, etc). As such, not using flag bits when we don't
strictly need them is a feature.

I'm not sure if this really needs to be negotiated using a flag bit. The
NO_ZEROES thing was negotiated using a flag because NBD_OPT_EXPORT_NAME
can't be upgraded, but that isn't the case here. This could easily be
negotiated using some NBD_OPT thing:

client->kernel: check whether structured replies are supported
(if yes:)
client->server: NBD_OPT_STRUCTURED_REPLIES
server->client: NBD_REP_ACK (if supported, or NBD_REP_UNSUP if not)

At this point, the server can send structured replies at leisure. We
could also set a "support don't fragment" flag bit in the per-export
flags field (which is a larger flags field than the global one), so that
the client kernel knows it can request non-fragmented replies without
requiring an extra kernel API (since per-export flags are passed to the
kernel by way of the NBD_SET_FLAGS ioctl).

(the spec can then possibly also say that the kernel MAY send structured
replies without sending the "support don't fragment" bit, but that it
then MUST NOT send fragmented replies -- although I'm not too sure
whether that's a good idea)

This would also get it more in line with the way the CMD_TRIM and
CMD_FLUSH commands are negotiated (by way of a per-export flag).

> all-or-none for use of the improved read structures.  I'm also leaning
> towards the name NBD_FLAG_C_STRUCTURED_READ, since elsewhere I'm

That's probably a good idea too, yes (with obvious s/FLAG_C/OPT/
change as per above).

> documenting that negotiating this particular global flag affects only
> the replies to NBD_CMD_READ (other commands may use structured replies,
> but those commands will be independently negotiated).

Right.

> >> Sure.  But keep in mind that if (when?) we add a flag for allowing the
> >> server to skip read chunks on holes, we'll have to tweak the wording to
> >> allow the server to send fewer chunks than the client's length, where
> >> the client must then assume zeroes for all chunks not received.
> > 
> > Or alternatively a chunk representing a hole. I wonder whether you
> > might be better to extend the chunk structure by 4 bytes to allow for
> > future modifications like this (e.g. NBD_CHUNK_FLAG_HOLE means
> > the chunk is full of zeroes and has no payload).
> 
> Seems reasonable (then I need to reword everything from len-8 to instead
> be len-12 when dealing with data size within the len bytes of payload).
> Network traffic-wise, I think it's better to always send the chunk
> flags, than it would be to try and make the sending of the chunk flags
> dependent on the client's choice of command flags (that is, we already
> argued that wire format should never be changed based merely on command
> flags, as it makes the server stream harder to decipher in isolation).
> 
> Thanks for the good feedback, by the way; it will make v2 better.

Regards,

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]