| Message ID | 20230608135653.2918540-14-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu patches for 64-bit NBD extensions | expand |
On Thu, Jun 08, 2023 at 08:56:42AM -0500, Eric Blake wrote: > Upcoming additions to support NBD 64-bit effect lengths allow for the > possibility to distinguish between payload length (capped at 32M) and > effect length (up to 63 bits). Without that extension, only the Technically, the NBD spec does not (yet) have a documented cap of a 63-bit size limit; although I should probably propose a patch upstream that does that (I had one in my draft work, but it hasn't yet made it upstream) > NBD_CMD_WRITE request has a payload; but with the extension, it makes > sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload > and effect length (where the payload is a limited-size struct that in > turns gives the real effect length as well as a subset of known ids > for which status is requested). Other future NBD commands may also > have a request payload, so the 64-bit extension introduces a new > NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header > length is a payload length or an effect length, rather than > hard-coding the decision based on the command. Note that we do not > support the payload version of BLOCK_STATUS yet. > > For this patch, no semantic change is intended for a compliant client. > For a non-compliant client, it is possible that the error behavior > changes (a different message, a change on whether the connection is > killed or remains alive for the next command, or so forth), in part > because req->complete is set later on some paths, but all errors > should still be handled gracefully. > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > v4: less indentation on several 'if's [Vladimir] > --- > nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------ > nbd/trace-events | 1 + > 2 files changed, 49 insertions(+), 28 deletions(-) > > diff --git a/nbd/server.c b/nbd/server.c > index 4ac05d0cd7b..d7dc29f0445 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -2329,6 +2329,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * > Error **errp) > { > NBDClient *client = req->client; > + bool extended_with_payload; > + unsigned payload_len = 0; > int valid_flags; > int ret; > > @@ -2342,48 +2344,63 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * > trace_nbd_co_receive_request_decode_type(request->cookie, request->type, > nbd_cmd_lookup(request->type)); > > - if (request->type != NBD_CMD_WRITE) { > - /* No payload, we are ready to read the next request. */ > - req->complete = true; > - } > - > if (request->type == NBD_CMD_DISC) { > /* Special case: we're going to disconnect without a reply, > * whether or not flags, from, or len are bogus */ > + req->complete = true; > return -EIO; > } > > - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || > - request->type == NBD_CMD_CACHE) > - { > - if (request->len > NBD_MAX_BUFFER_SIZE) { > - error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", > - request->len, NBD_MAX_BUFFER_SIZE); > - return -EINVAL; > + /* Payload and buffer handling. */ > + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && > + (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN); > + if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || > + request->type == NBD_CMD_CACHE || extended_with_payload) && > + request->len > NBD_MAX_BUFFER_SIZE) { I'm still debating about rewriting this series of slightly-different 'if' into a single switch (request->type) block; the benefit is that all actions for one command will be localized instead of split over multiple lines of if, the drawback is that some code will now have to be duplicated across commands (such as the buffer allocation code for CMD_READ and CMD_WRITE).
On 08.06.23 21:29, Eric Blake wrote: > On Thu, Jun 08, 2023 at 08:56:42AM -0500, Eric Blake wrote: >> Upcoming additions to support NBD 64-bit effect lengths allow for the >> possibility to distinguish between payload length (capped at 32M) and >> effect length (up to 63 bits). Without that extension, only the > > Technically, the NBD spec does not (yet) have a documented cap of a > 63-bit size limit; although I should probably propose a patch upstream > that does that (I had one in my draft work, but it hasn't yet made it > upstream) > >> NBD_CMD_WRITE request has a payload; but with the extension, it makes >> sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload >> and effect length (where the payload is a limited-size struct that in >> turns gives the real effect length as well as a subset of known ids >> for which status is requested). Other future NBD commands may also >> have a request payload, so the 64-bit extension introduces a new >> NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header >> length is a payload length or an effect length, rather than >> hard-coding the decision based on the command. Note that we do not >> support the payload version of BLOCK_STATUS yet. >> >> For this patch, no semantic change is intended for a compliant client. >> For a non-compliant client, it is possible that the error behavior >> changes (a different message, a change on whether the connection is >> killed or remains alive for the next command, or so forth), in part >> because req->complete is set later on some paths, but all errors >> should still be handled gracefully. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> --- >> >> v4: less indentation on several 'if's [Vladimir] >> --- >> nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------ >> nbd/trace-events | 1 + >> 2 files changed, 49 insertions(+), 28 deletions(-) >> >> diff --git a/nbd/server.c b/nbd/server.c >> index 4ac05d0cd7b..d7dc29f0445 100644 >> --- a/nbd/server.c >> +++ b/nbd/server.c >> @@ -2329,6 +2329,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * >> Error **errp) >> { >> NBDClient *client = req->client; >> + bool extended_with_payload; >> + unsigned payload_len = 0; >> int valid_flags; >> int ret; >> >> @@ -2342,48 +2344,63 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * >> trace_nbd_co_receive_request_decode_type(request->cookie, request->type, >> nbd_cmd_lookup(request->type)); >> >> - if (request->type != NBD_CMD_WRITE) { >> - /* No payload, we are ready to read the next request. */ >> - req->complete = true; >> - } >> - >> if (request->type == NBD_CMD_DISC) { >> /* Special case: we're going to disconnect without a reply, >> * whether or not flags, from, or len are bogus */ >> + req->complete = true; >> return -EIO; >> } >> >> - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || >> - request->type == NBD_CMD_CACHE) >> - { >> - if (request->len > NBD_MAX_BUFFER_SIZE) { >> - error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", >> - request->len, NBD_MAX_BUFFER_SIZE); >> - return -EINVAL; >> + /* Payload and buffer handling. */ >> + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && >> + (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN); >> + if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || >> + request->type == NBD_CMD_CACHE || extended_with_payload) && >> + request->len > NBD_MAX_BUFFER_SIZE) { > > I'm still debating about rewriting this series of slightly-different > 'if' into a single switch (request->type) block; the benefit is that I vote for switch!) Really, seems it would be a lot simpler to read. > all actions for one command will be localized instead of split over > multiple lines of if, the drawback is that some code will now have to > be duplicated across commands (such as the buffer allocation code for > CMD_READ and CMD_WRITE). >
diff --git a/nbd/server.c b/nbd/server.c index 4ac05d0cd7b..d7dc29f0445 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2329,6 +2329,8 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * Error **errp) { NBDClient *client = req->client; + bool extended_with_payload; + unsigned payload_len = 0; int valid_flags; int ret; @@ -2342,48 +2344,63 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * trace_nbd_co_receive_request_decode_type(request->cookie, request->type, nbd_cmd_lookup(request->type)); - if (request->type != NBD_CMD_WRITE) { - /* No payload, we are ready to read the next request. */ - req->complete = true; - } - if (request->type == NBD_CMD_DISC) { /* Special case: we're going to disconnect without a reply, * whether or not flags, from, or len are bogus */ + req->complete = true; return -EIO; } - if (request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || - request->type == NBD_CMD_CACHE) - { - if (request->len > NBD_MAX_BUFFER_SIZE) { - error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", - request->len, NBD_MAX_BUFFER_SIZE); - return -EINVAL; + /* Payload and buffer handling. */ + extended_with_payload = client->mode >= NBD_MODE_EXTENDED && + (request->flags & NBD_CMD_FLAG_PAYLOAD_LEN); + if ((request->type == NBD_CMD_READ || request->type == NBD_CMD_WRITE || + request->type == NBD_CMD_CACHE || extended_with_payload) && + request->len > NBD_MAX_BUFFER_SIZE) { + error_setg(errp, "len (%" PRIu64" ) is larger than max len (%u)", + request->len, NBD_MAX_BUFFER_SIZE); + return -EINVAL; + } + + if (request->type == NBD_CMD_WRITE || extended_with_payload) { + payload_len = request->len; + if (request->type != NBD_CMD_WRITE) { + /* + * For now, we don't support payloads on other + * commands; but we can keep the connection alive. + */ + request->len = 0; + } else if (client->mode >= NBD_MODE_EXTENDED && + !extended_with_payload) { + /* The client is noncompliant. Trace it, but proceed. */ + trace_nbd_co_receive_ext_payload_compliance(request->from, + request->len); } + } - if (request->type != NBD_CMD_CACHE) { - req->data = blk_try_blockalign(client->exp->common.blk, - request->len); - if (req->data == NULL) { - error_setg(errp, "No memory"); - return -ENOMEM; - } + if (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_READ) { + req->data = blk_try_blockalign(client->exp->common.blk, + request->len); + if (req->data == NULL) { + error_setg(errp, "No memory"); + return -ENOMEM; } } - if (request->type == NBD_CMD_WRITE) { - assert(request->len <= NBD_MAX_BUFFER_SIZE); - if (nbd_read(client->ioc, req->data, request->len, "CMD_WRITE data", - errp) < 0) - { + if (payload_len) { + if (req->data) { + ret = nbd_read(client->ioc, req->data, payload_len, + "CMD_WRITE data", errp); + } else { + ret = nbd_drop(client->ioc, payload_len, errp); + } + if (ret < 0) { return -EIO; } - req->complete = true; - trace_nbd_co_receive_request_payload_received(request->cookie, - request->len); + payload_len); } + req->complete = true; /* Sanity checks. */ if (client->exp->nbdflags & NBD_FLAG_READ_ONLY && @@ -2413,7 +2430,10 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, NBDRequest * client->check_align); } valid_flags = NBD_CMD_FLAG_FUA; - if (request->type == NBD_CMD_READ && client->mode >= NBD_MODE_STRUCTURED) { + if (request->type == NBD_CMD_WRITE && client->mode >= NBD_MODE_EXTENDED) { + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } else if (request->type == NBD_CMD_READ && + client->mode >= NBD_MODE_STRUCTURED) { valid_flags |= NBD_CMD_FLAG_DF; } else if (request->type == NBD_CMD_WRITE_ZEROES) { valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO; diff --git a/nbd/trace-events b/nbd/trace-events index 3338da2be2a..6a34d7f027a 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -71,6 +71,7 @@ nbd_co_send_extents(uint64_t cookie, unsigned int extents, uint32_t id, uint64_t nbd_co_send_chunk_error(uint64_t cookie, int err, const char *errname, const char *msg) "Send structured error reply: cookie = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t cookie, uint16_t type, const char *name) "Decoding type: cookie = %" PRIu64 ", type = %" PRIu16 " (%s)" nbd_co_receive_request_payload_received(uint64_t cookie, uint64_t len) "Payload received: cookie = %" PRIu64 ", len = %" PRIu64 +nbd_co_receive_ext_payload_compliance(uint64_t from, uint64_t len) "client sent non-compliant write without payload flag: from=0x%" PRIx64 ", len=0x%" PRIx64 nbd_co_receive_align_compliance(const char *op, uint64_t from, uint64_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx64 ", align=0x%" PRIx32 nbd_trip(void) "Reading request"
Upcoming additions to support NBD 64-bit effect lengths allow for the possibility to distinguish between payload length (capped at 32M) and effect length (up to 63 bits). Without that extension, only the NBD_CMD_WRITE request has a payload; but with the extension, it makes sense to allow at least NBD_CMD_BLOCK_STATUS to have both a payload and effect length (where the payload is a limited-size struct that in turns gives the real effect length as well as a subset of known ids for which status is requested). Other future NBD commands may also have a request payload, so the 64-bit extension introduces a new NBD_CMD_FLAG_PAYLOAD_LEN that distinguishes between whether the header length is a payload length or an effect length, rather than hard-coding the decision based on the command. Note that we do not support the payload version of BLOCK_STATUS yet. For this patch, no semantic change is intended for a compliant client. For a non-compliant client, it is possible that the error behavior changes (a different message, a change on whether the connection is killed or remains alive for the next command, or so forth), in part because req->complete is set later on some paths, but all errors should still be handled gracefully. Signed-off-by: Eric Blake <eblake@redhat.com> --- v4: less indentation on several 'if's [Vladimir] --- nbd/server.c | 76 ++++++++++++++++++++++++++++++------------------ nbd/trace-events | 1 + 2 files changed, 49 insertions(+), 28 deletions(-)