| Message ID | 20221114224655.2186173-6-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NBD spec changes for 64-bit extensions | expand |
On Mon, Nov 14, 2022 at 04:46:54PM -0600, Eric Blake wrote: > #### Simple reply message > > @@ -1232,6 +1235,19 @@ The field has the following format: > will be faster than a regular write). Clients MUST NOT set the > `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag > is set. > +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server > + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to > + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the > + server filters its response to a specific subset of negotiated > + metacontext ids passed in via a client payload, rather than the > + default of replying to all metacontext ids. Servers MUST NOT > + advertise this bit unless the client successfully negotiates > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or > + `NBD_OPT_GO` if the client does not negotiate metacontexts with > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless > + this transmission flag is set. Given that we are introducing this at the same time as the extended headers (and given that we're running out of available flags in this 16-bit field), isn't it better to make the ability to understand PAYLOAD_LEN be implied by extended headers? Or is there a use case for implementing extended headers but not a payload length on CMD_BLOCK_STATUS that I'm missing? > Clients SHOULD ignore unknown flags. > > @@ -1915,8 +1931,11 @@ valid may depend on negotiation during the handshake phase. > header. With extended headers, the flag MUST be set for > `NBD_CMD_WRITE` (as the write command always sends a payload of the > bytes to be written); for other commands, the flag will trigger an > - `NBD_EINVAL` error unless the server has advertised support for an > - extension payload form for the command. > + `NBD_EINVAL` error unless the command documents an optional payload > + form for the command and the server has implemented that form (an > + example being `NBD_CMD_BLOCK_STATUS` providing a payload form for > + restricting the response to a particular metacontext id, when the > + server advertises `NBD_FLAG_BLOCK_STATUS_PAYLOAD`). > > ##### Structured reply flags > > @@ -2464,6 +2483,23 @@ The following request types exist: > The server MAY send chunks in a different order than the context > ids were assigned in reply to `NBD_OPT_SET_META_CONTEXT`. > > + If extended headers were negotiated, a server MAY optionally > + advertise, via the transmission flag > + `NBD_FLAG_BLOCK_STATUS_PAYLOAD`, that it supports an alternative > + request form where the client sets `NBD_CMD_FLAG_PAYLOAD_LEN` in > + order to pass a payload that informs the server to limit its > + replies to the metacontext id(s) in the client's request payload, > + rather than giving an answer on all possible metacontext ids. If > + the server does not support the payload form, or detects duplicate > + or unknown metacontext ids in the client's payload, the server > + MUST gracefully consume the client's payload before failing with > + `NBD_EINVAL`. The payload form MUST occupy 8 + n*4 bytes, where n > + is the number of metacontext ids the client is interested in (as > + implied by the payload length), laid out as: > + > + 64 bits, effect length > + n * 32 bits, list of metacontext ids to use > + > The list of block status descriptors within a given status chunk > represent consecutive portions of the file starting from specified > *offset*. If the client used the `NBD_CMD_FLAG_REQ_ONE` flag,
On Wed, Feb 22, 2023 at 12:05:44PM +0200, Wouter Verhelst wrote: > On Mon, Nov 14, 2022 at 04:46:54PM -0600, Eric Blake wrote: > > #### Simple reply message > > > > @@ -1232,6 +1235,19 @@ The field has the following format: > > will be faster than a regular write). Clients MUST NOT set the > > `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag > > is set. > > +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server > > + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to > > + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the > > + server filters its response to a specific subset of negotiated > > + metacontext ids passed in via a client payload, rather than the > > + default of replying to all metacontext ids. Servers MUST NOT > > + advertise this bit unless the client successfully negotiates > > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT > > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or > > + `NBD_OPT_GO` if the client does not negotiate metacontexts with > > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the > > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless > > + this transmission flag is set. > > Given that we are introducing this at the same time as the extended > headers (and given that we're running out of available flags in this > 16-bit field), isn't it better to make the ability to understand > PAYLOAD_LEN be implied by extended headers? Or is there a use case for > implementing extended headers but not a payload length on > CMD_BLOCK_STATUS that I'm missing? In my proof of implementation, this was a distinct feature addition on top of 64-bit headers. It is easy to write a server that does not want to deal with a client payload on a block status request (for example, a server that only knows about one metacontext, and therefore never needs to deal with a client wanting a subset of negotiated metacontexts). Forcing a server to have to support a client-side filtering request on block status commands, merely because the server now supports 64-bit lengths, seemed like a stretch too far, which is why I decided to use a feature bit for this one.
On Fri, Mar 03, 2023 at 04:40:38PM -0600, Eric Blake wrote: > On Wed, Feb 22, 2023 at 12:05:44PM +0200, Wouter Verhelst wrote: > > On Mon, Nov 14, 2022 at 04:46:54PM -0600, Eric Blake wrote: > > > #### Simple reply message > > > > > > @@ -1232,6 +1235,19 @@ The field has the following format: > > > will be faster than a regular write). Clients MUST NOT set the > > > `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag > > > is set. > > > +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server > > > + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to > > > + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the > > > + server filters its response to a specific subset of negotiated > > > + metacontext ids passed in via a client payload, rather than the > > > + default of replying to all metacontext ids. Servers MUST NOT > > > + advertise this bit unless the client successfully negotiates > > > + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT > > > + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or > > > + `NBD_OPT_GO` if the client does not negotiate metacontexts with > > > + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the > > > + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless > > > + this transmission flag is set. > > > > Given that we are introducing this at the same time as the extended > > headers (and given that we're running out of available flags in this > > 16-bit field), isn't it better to make the ability to understand > > PAYLOAD_LEN be implied by extended headers? Or is there a use case for > > implementing extended headers but not a payload length on > > CMD_BLOCK_STATUS that I'm missing? > > In my proof of implementation, this was a distinct feature addition on > top of 64-bit headers. > > It is easy to write a server that does not want to deal with a client > payload on a block status request (for example, a server that only > knows about one metacontext, and therefore never needs to deal with a > client wanting a subset of negotiated metacontexts). Forcing a server > to have to support a client-side filtering request on block status > commands, merely because the server now supports 64-bit lengths, > seemed like a stretch too far, which is why I decided to use a feature > bit for this one. Okay, yes, that makes sense. Thanks.
diff --git a/doc/proto.md b/doc/proto.md index 14af48d..645a736 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -397,17 +397,20 @@ additional bytes of payload are present), or if the flag is absent (there is no payload, and *length* instead is an effect length describing how much of the image the request operates on). The command `NBD_CMD_WRITE` MUST use the flag `NBD_CMD_FLAG_PAYLOAD_LEN` -in this mode; while other commands SHOULD avoid the flag if the -server has not indicated extension suppport for payloads on that -command. A server SHOULD initiate hard disconnect if a client sets -the `NBD_CMD_FLAG_PAYLOAD_LEN` flag and uses a *length* larger than -a server's advertised or default maximum payload length (capped at -32 bits by the constraints of `NBD_INFO_BLOCK_SIZE`); in all other -cases, a server SHOULD gracefully consume *length* bytes of payload -(even if it then replies with an `NBD_EINVAL` failure because the -particular command was not expecting a payload), and proceed with -the next client command. Thus, only when *length* is used as an -effective length will it utilize a full 64-bit value. +in this mode; most other commands omit it, although some like +`NBD_CMD_BLOCK_STATUS` optionally support the flag in order to allow +the client to pass additional information in the payload (where the +command documents what the payload will contain, including the +possibility of a separate effect length). A server SHOULD initiate +hard disconnect if a client sets the `NBD_CMD_FLAG_PAYLOAD_LEN` flag +and uses a *length* larger than a server's advertised or default +maximum payload length (capped at 32 bits by the constraints of +`NBD_INFO_BLOCK_SIZE`); in all other cases, a server SHOULD gracefully +consume *length* bytes of payload (even if it then replies with an +`NBD_EINVAL` failure because the particular command was not expecting +a payload), and proceed with the next client command. Thus, only when +*length* is used as an effective length will it utilize a full 64-bit +value. #### Simple reply message @@ -1232,6 +1235,19 @@ The field has the following format: will be faster than a regular write). Clients MUST NOT set the `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag is set. +- bit 12, `NBD_FLAG_BLOCK_STATUS_PAYLOAD`: Indicates that the server + understands the use of the `NBD_CMD_FLAG_PAYLOAD_LEN` flag to + `NBD_CMD_BLOCK_STATUS` to allow the client to request that the + server filters its response to a specific subset of negotiated + metacontext ids passed in via a client payload, rather than the + default of replying to all metacontext ids. Servers MUST NOT + advertise this bit unless the client successfully negotiates + extended headers via `NBD_OPT_EXTENDED_HEADERS`, and SHOULD NOT + advertise this bit in response to `NBD_OPT_EXPORT_NAME` or + `NBD_OPT_GO` if the client does not negotiate metacontexts with + `NBD_OPT_SET_META_CONTEXT`; clients SHOULD NOT set the + `NBD_CMD_FLAG_PAYLOAD_LEN` flag for `NBD_CMD_BLOCK_STATUS` unless + this transmission flag is set. Clients SHOULD ignore unknown flags. @@ -1915,8 +1931,11 @@ valid may depend on negotiation during the handshake phase. header. With extended headers, the flag MUST be set for `NBD_CMD_WRITE` (as the write command always sends a payload of the bytes to be written); for other commands, the flag will trigger an - `NBD_EINVAL` error unless the server has advertised support for an - extension payload form for the command. + `NBD_EINVAL` error unless the command documents an optional payload + form for the command and the server has implemented that form (an + example being `NBD_CMD_BLOCK_STATUS` providing a payload form for + restricting the response to a particular metacontext id, when the + server advertises `NBD_FLAG_BLOCK_STATUS_PAYLOAD`). ##### Structured reply flags @@ -2464,6 +2483,23 @@ The following request types exist: The server MAY send chunks in a different order than the context ids were assigned in reply to `NBD_OPT_SET_META_CONTEXT`. + If extended headers were negotiated, a server MAY optionally + advertise, via the transmission flag + `NBD_FLAG_BLOCK_STATUS_PAYLOAD`, that it supports an alternative + request form where the client sets `NBD_CMD_FLAG_PAYLOAD_LEN` in + order to pass a payload that informs the server to limit its + replies to the metacontext id(s) in the client's request payload, + rather than giving an answer on all possible metacontext ids. If + the server does not support the payload form, or detects duplicate + or unknown metacontext ids in the client's payload, the server + MUST gracefully consume the client's payload before failing with + `NBD_EINVAL`. The payload form MUST occupy 8 + n*4 bytes, where n + is the number of metacontext ids the client is interested in (as + implied by the payload length), laid out as: + + 64 bits, effect length + n * 32 bits, list of metacontext ids to use + The list of block status descriptors within a given status chunk represent consecutive portions of the file starting from specified *offset*. If the client used the `NBD_CMD_FLAG_REQ_ONE` flag,
NBD_CMD_BLOCK_STATUS currently forces the server to reply to all metacontext ids that the client negotiated via NBD_OPT_SET_META_CONTEXT. But since extended headers make it easy for the client to pass command payloads, we can allow for a client to negotiate multiple metacontexts up front but express dynamic interest in varying subsets of those contexts over the life of the connection, for less wasted effort in responding to NBD_CMD_BLOCK_STATUS. This works by having the command payload supply an effect length and a list of ids the client is currently interested in. Signed-off-by: Eric Blake <eblake@redhat.com> --- doc/proto.md | 62 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 49 insertions(+), 13 deletions(-)