On 04/04/2016 04:40 PM, Wouter Verhelst wrote:
Hi,
Need to look into this in some detail, for which I don't have the time
(or the non-tiredness ;-) right now, but these two caught my eye:
+ The payload is structured as a list of one or more descriptors,
+ each with this layout:
+
+ * 32 bits, length (unsigned, MUST NOT be zero)
+ * 32 bits, status flags
+
+ The definition of the status flags is determined based on the
+ flags present in the original request.
Might be a good idea to specify what a client can do with flags it
doesn't know about; ignore them, probably?
Sounds correct - so a server can subdivide into more descriptors with
additional status bits, and clients just have to ignore those extra bits
and coalesce things itself when dealing with the bits it cares about.
[...]
+The extension adds the following new command flag:
+
+- `NBD_CMD_FLAG_STATUS_DIRTY`; valid during `NBD_CMD_BLOCK_STATUS`.
+ SHOULD be set to 1 if the client wants to request dirtiness status
+ rather than provisioning status.
Why only one flag here? I could imagine a client might want to query for
both states at the same time. Obviously that means a client needs to
query for *at least* one of the statuses, otherwise the server should
reply with EINVAL.
Though I'm undecided on whether a bit being set to 0 should mean "give
me that information" or whether 1 should.
Hmm. Based on that reading, maybe we want TWO flags:
NBD_CMD_FLAG_STATUS_ALLOCATED
NBD_CMD_FLAG_STATUS_DIRTY
and require that the client MUST set at least one flag (setting no flags
at all is boring), but similarly allow that the server MAY reject
attempts to set multiple flags with EINVAL (if there is no efficient way
to provide the information for all flags on a single pass), in which
case clients have to fall back to querying one flag at a time.
But while Alex and Denis were arguing that no one would ever query both
things at once (and therefore, it might be better to make
NBD_STATUS_HOLE and NBD_STATUS_CLEAN both be bit 0), your approach of
having two separate request flags and allowing both at once would mean
we do need to keep the status information separate (NBD_STATUS_HOLE is
bit 0, NBD_STATUS_CLEAN is bit 2).
I also see that I failed to reserve a bit for NBD_CMD_FLAG_STATUS_DIRTY.
Looks like there will still be some more conversation and at least a v3
needed, but I'll wait a couple days for that to happen so that more
reviewers can chime in, without being too tired during the review process.