| Message ID | 20190403030526.12258-3-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Final round of NBD alignment fixes | expand |
03.04.2019 6:05, Eric Blake wrote: > We've recently added traces for clients to flag server non-compliance; > let's do the same for servers to flag client non-compliance. According > to the spec, if the client requests NBD_INFO_BLOCK_SIZE, it is > promising to send all requests aligned to those boundaries. Of > course, if the client does not request NBD_INFO_BLOCK_SIZE, then it > made no promises so we shouldn't flag anything; and because we are > willing to handle clients that made no promises (the spec allows us to > use NBD_REP_ERR_BLOCK_SIZE_REQD if we had been unwilling), we already > have to handle unaligned requests (which the block layer already does > on our behalf). So even though the spec allows us to return EINVAL > for clients that promised to behave, it's easier to always answer > unaligned requests. Still, flagging non-compliance can be useful in > debugging a client that is trying to be maximally portable. > > Qemu as client used to have one spot where it sent non-compliant > requests: if the server sends an unaligned reply to > NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire > disk, the next request would start at that unaligned point; this was > fixed in commit a39286dd when the client was taught to work around > server non-compliance; but is equally fixed if the server is patched > to not send unaligned replies in the first place (the next few patches > will address that). Fortunately, I did not find any more spots where > qemu as client was non-compliant. I was able to test the patch by > using the following hack to convince qemu-io to run various unaligned > commands, coupled with serving 512-byte alignment by intentionally > omitting '-f raw' on the server while viewing server traces. > > | diff --git i/nbd/client.c w/nbd/client.c > | index 427980bdd22..1858b2aac35 100644 > | --- i/nbd/client.c > | +++ w/nbd/client.c > | @@ -449,6 +449,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, > | nbd_send_opt_abort(ioc); > | return -1; > | } > | + info->min_block = 1;//hack > | if (!is_power_of_2(info->min_block)) { > | error_setg(errp, "server minimum block size %" PRIu32 > | " is not a power of two", info->min_block); > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > nbd/server.c | 12 ++++++++++++ > nbd/trace-events | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/nbd/server.c b/nbd/server.c > index 1b8c8619896..3040ceb5606 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -124,6 +124,8 @@ struct NBDClient { > int nb_requests; > bool closing; > > + uint32_t check_align; /* If non-zero, check for aligned client requests */ > + > bool structured_reply; > NBDExportMetaContexts export_meta; > > @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, > > if (client->opt == NBD_OPT_GO) { > client->exp = exp; > + client->check_align = blocksize ? > + blk_get_request_alignment(exp->blk) : 0; I think better set in same place, where sizes[0] set, or use sizes[0] here or add separate local varibale for it, so that, if "sizes[0] =" changes somehow, we will not forget to fix this place too. > QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); > nbd_export_get(client->exp); > nbd_check_meta_export(client); > @@ -2126,6 +2130,14 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, > return (request->type == NBD_CMD_WRITE || > request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; > } > + if (client->check_align && !QEMU_IS_ALIGNED(request->from | request->len, > + client->check_align)) { > + /* > + * The block layer gracefully handles unaligned requests, but > + * it's still worth tracing client non-compliance > + */ > + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); > + } > valid_flags = NBD_CMD_FLAG_FUA; > if (request->type == NBD_CMD_READ && client->structured_reply) { > valid_flags |= NBD_CMD_FLAG_DF; > diff --git a/nbd/trace-events b/nbd/trace-events > index a6cca8fdf83..87a2b896c35 100644 > --- a/nbd/trace-events > +++ b/nbd/trace-events > @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t > nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" > nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" > nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 > +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request" Don't you want print request->from, request->len and client->check_align as well? > nbd_trip(void) "Reading request" > Patch seems correct anyway, so if you are in a hurry, it's OK as is: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote: > 03.04.2019 6:05, Eric Blake wrote: >> We've recently added traces for clients to flag server non-compliance; >> let's do the same for servers to flag client non-compliance. According Thus, s/Trace server/Trace client/ in the subject line. >> >> Qemu as client used to have one spot where it sent non-compliant >> requests: if the server sends an unaligned reply to >> NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire >> disk, the next request would start at that unaligned point; this was >> fixed in commit a39286dd when the client was taught to work around >> server non-compliance; but is equally fixed if the server is patched >> to not send unaligned replies in the first place (the next few patches >> will address that). I'll have to reword this now that we know 4.0 will still have some of those server bugs in place (as the tail of this series is deferred to 4.1). >> @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, >> >> if (client->opt == NBD_OPT_GO) { >> client->exp = exp; >> + client->check_align = blocksize ? >> + blk_get_request_alignment(exp->blk) : 0; > > I think better set in same place, where sizes[0] set, or use sizes[0] here or add separate local > varibale for it, so that, if "sizes[0] =" changes somehow, we will not forget to fix this place too. I don't want to set client->check_align for NBD_OPT_INFO; but I can indeed use a temporary variable or hoist the computation so that it is all in one spot. >> +++ b/nbd/trace-events >> @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t >> nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" >> nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" >> nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 >> +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request" > > Don't you want print request->from, request->len and client->check_align as well? Wouldn't hurt. > >> nbd_trip(void) "Reading request" >> > > Patch seems correct anyway, so if you are in a hurry, it's OK as is: > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Here's what I'll squash in; I think it is obvious enough to still keep your R-b, if I send the pull request before you reply back. diff --git i/nbd/server.c w/nbd/server.c index 3040ceb5606..cb38dfe6902 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -535,6 +535,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, bool blocksize = false; uint32_t sizes[3]; char buf[sizeof(uint64_t) + sizeof(uint16_t)]; + uint32_t check_align = 0; /* Client sends: 4 bytes: L, name length (can be 0) @@ -611,7 +612,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, * whether this is OPT_INFO or OPT_GO. */ /* minimum - 1 for back-compat, or actual if client will obey it. */ if (client->opt == NBD_OPT_INFO || blocksize) { - sizes[0] = blk_get_request_alignment(exp->blk); + check_align = sizes[0] = blk_get_request_alignment(exp->blk); } else { sizes[0] = 1; } @@ -662,8 +663,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, if (client->opt == NBD_OPT_GO) { client->exp = exp; - client->check_align = blocksize ? - blk_get_request_alignment(exp->blk) : 0; + client->check_align = check_align; QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); nbd_export_get(client->exp); nbd_check_meta_export(client); @@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, * The block layer gracefully handles unaligned requests, but * it's still worth tracing client non-compliance */ - trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type, + request->from, + request->len, + client->check_align)); } valid_flags = NBD_CMD_FLAG_FUA; if (request->type == NBD_CMD_READ && client->structured_reply) { diff --git i/nbd/trace-events w/nbd/trace-events index 87a2b896c35..ec2d46c9247 100644 --- i/nbd/trace-events +++ w/nbd/trace-events @@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 -nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request" +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%x, align=0x%x" nbd_trip(void) "Reading request"
05.04.2019 23:04, Eric Blake wrote: > On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote: >> 03.04.2019 6:05, Eric Blake wrote: >>> We've recently added traces for clients to flag server non-compliance; >>> let's do the same for servers to flag client non-compliance. According > > Thus, s/Trace server/Trace client/ in the subject line. > >>> >>> Qemu as client used to have one spot where it sent non-compliant >>> requests: if the server sends an unaligned reply to >>> NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire >>> disk, the next request would start at that unaligned point; this was >>> fixed in commit a39286dd when the client was taught to work around >>> server non-compliance; but is equally fixed if the server is patched >>> to not send unaligned replies in the first place (the next few patches >>> will address that). > > I'll have to reword this now that we know 4.0 will still have some of > those server bugs in place (as the tail of this series is deferred to 4.1). > > >>> @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, >>> >>> if (client->opt == NBD_OPT_GO) { >>> client->exp = exp; >>> + client->check_align = blocksize ? >>> + blk_get_request_alignment(exp->blk) : 0; >> >> I think better set in same place, where sizes[0] set, or use sizes[0] here or add separate local >> varibale for it, so that, if "sizes[0] =" changes somehow, we will not forget to fix this place too. > > I don't want to set client->check_align for NBD_OPT_INFO; but I can > indeed use a temporary variable or hoist the computation so that it is > all in one spot. > > >>> +++ b/nbd/trace-events >>> @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t >>> nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" >>> nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" >>> nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 >>> +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request" >> >> Don't you want print request->from, request->len and client->check_align as well? > > Wouldn't hurt. > >> >>> nbd_trip(void) "Reading request" >>> >> >> Patch seems correct anyway, so if you are in a hurry, it's OK as is: >> >> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> > > Here's what I'll squash in; I think it is obvious enough to still keep > your R-b, if I send the pull request before you reply back. > > diff --git i/nbd/server.c w/nbd/server.c > index 3040ceb5606..cb38dfe6902 100644 > --- i/nbd/server.c > +++ w/nbd/server.c > @@ -535,6 +535,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > bool blocksize = false; > uint32_t sizes[3]; > char buf[sizeof(uint64_t) + sizeof(uint16_t)]; > + uint32_t check_align = 0; > > /* Client sends: > 4 bytes: L, name length (can be 0) > @@ -611,7 +612,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > * whether this is OPT_INFO or OPT_GO. */ > /* minimum - 1 for back-compat, or actual if client will obey it. */ > if (client->opt == NBD_OPT_INFO || blocksize) { > - sizes[0] = blk_get_request_alignment(exp->blk); > + check_align = sizes[0] = blk_get_request_alignment(exp->blk); > } else { > sizes[0] = 1; > } > @@ -662,8 +663,7 @@ static int nbd_negotiate_handle_info(NBDClient > *client, uint16_t myflags, > > if (client->opt == NBD_OPT_GO) { > client->exp = exp; > - client->check_align = blocksize ? > - blk_get_request_alignment(exp->blk) : 0; > + client->check_align = check_align; > QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); > nbd_export_get(client->exp); > nbd_check_meta_export(client); > @@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData > *req, NBDRequest *request, > * The block layer gracefully handles unaligned requests, but > * it's still worth tracing client non-compliance > */ > - > trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); > + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type, > + request->from, > + request->len, > + > client->check_align)); something strange here with brackets. > } > valid_flags = NBD_CMD_FLAG_FUA; > if (request->type == NBD_CMD_READ && client->structured_reply) { > diff --git i/nbd/trace-events w/nbd/trace-events > index 87a2b896c35..ec2d46c9247 100644 > --- i/nbd/trace-events > +++ w/nbd/trace-events > @@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int > extents, uint32_t id, uint64_t > nbd_co_send_structured_error(uint64_t handle, int err, const char > *errname, const char *msg) "Send structured error reply: handle = %" > PRIu64 ", error = %d (%s), msg = '%s'" > nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, > const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 > " (%s)" > nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) > "Payload received: handle = %" PRIu64 ", len = %" PRIu32 > -nbd_co_receive_align_compliance(const char *op) "client sent > non-compliant unaligned %s request" > +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t > len, uint32_t align) "client sent non-compliant unaligned %s request: > from=0x%" PRIx64 ", len=0x%x, align=0x%x" %x or % PRIx32 - doesn't matter? > nbd_trip(void) "Reading request" > > > OK for me, thanks.
On 4/8/19 7:14 AM, Vladimir Sementsov-Ogievskiy wrote: > 05.04.2019 23:04, Eric Blake wrote: >> On 4/5/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote: >>> 03.04.2019 6:05, Eric Blake wrote: >>>> We've recently added traces for clients to flag server non-compliance; >>>> let's do the same for servers to flag client non-compliance. According >> >> Thus, s/Trace server/Trace client/ in the subject line. >> >>> Patch seems correct anyway, so if you are in a hurry, it's OK as is: >>> >>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> >> >> Here's what I'll squash in; I think it is obvious enough to still keep >> your R-b, if I send the pull request before you reply back. >> >> @@ -2136,7 +2136,10 @@ static int nbd_co_receive_request(NBDRequestData >> *req, NBDRequest *request, >> * The block layer gracefully handles unaligned requests, but >> * it's still worth tracing client non-compliance >> */ >> - >> trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); >> + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type, >> + request->from, >> + request->len, >> + >> client->check_align)); > > something strange here with brackets. Yeah, I grabbed the diff before compile-testing, but fixed it before pushing to my branch. >> +++ w/nbd/trace-events >> @@ -71,5 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int >> extents, uint32_t id, uint64_t >> nbd_co_send_structured_error(uint64_t handle, int err, const char >> *errname, const char *msg) "Send structured error reply: handle = %" >> PRIu64 ", error = %d (%s), msg = '%s'" >> nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, >> const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 >> " (%s)" >> nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) >> "Payload received: handle = %" PRIu64 ", len = %" PRIu32 >> -nbd_co_receive_align_compliance(const char *op) "client sent >> non-compliant unaligned %s request" >> +nbd_co_receive_align_compliance(const char *op, uint64_t from, uint32_t >> len, uint32_t align) "client sent non-compliant unaligned %s request: >> from=0x%" PRIx64 ", len=0x%x, align=0x%x" > > %x or % PRIx32 - doesn't matter? Will use PRIx32 for consistency. > >> nbd_trip(void) "Reading request" >> >> >> > > OK for me, thanks. Thanks for a second look. Pull request will be out later today.
diff --git a/nbd/server.c b/nbd/server.c index 1b8c8619896..3040ceb5606 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -124,6 +124,8 @@ struct NBDClient { int nb_requests; bool closing; + uint32_t check_align; /* If non-zero, check for aligned client requests */ + bool structured_reply; NBDExportMetaContexts export_meta; @@ -660,6 +662,8 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, if (client->opt == NBD_OPT_GO) { client->exp = exp; + client->check_align = blocksize ? + blk_get_request_alignment(exp->blk) : 0; QTAILQ_INSERT_TAIL(&client->exp->clients, client, next); nbd_export_get(client->exp); nbd_check_meta_export(client); @@ -2126,6 +2130,14 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request, return (request->type == NBD_CMD_WRITE || request->type == NBD_CMD_WRITE_ZEROES) ? -ENOSPC : -EINVAL; } + if (client->check_align && !QEMU_IS_ALIGNED(request->from | request->len, + client->check_align)) { + /* + * The block layer gracefully handles unaligned requests, but + * it's still worth tracing client non-compliance + */ + trace_nbd_co_receive_align_compliance(nbd_cmd_lookup(request->type)); + } valid_flags = NBD_CMD_FLAG_FUA; if (request->type == NBD_CMD_READ && client->structured_reply) { valid_flags |= NBD_CMD_FLAG_DF; diff --git a/nbd/trace-events b/nbd/trace-events index a6cca8fdf83..87a2b896c35 100644 --- a/nbd/trace-events +++ b/nbd/trace-events @@ -71,4 +71,5 @@ nbd_co_send_extents(uint64_t handle, unsigned int extents, uint32_t id, uint64_t nbd_co_send_structured_error(uint64_t handle, int err, const char *errname, const char *msg) "Send structured error reply: handle = %" PRIu64 ", error = %d (%s), msg = '%s'" nbd_co_receive_request_decode_type(uint64_t handle, uint16_t type, const char *name) "Decoding type: handle = %" PRIu64 ", type = %" PRIu16 " (%s)" nbd_co_receive_request_payload_received(uint64_t handle, uint32_t len) "Payload received: handle = %" PRIu64 ", len = %" PRIu32 +nbd_co_receive_align_compliance(const char *op) "client sent non-compliant unaligned %s request" nbd_trip(void) "Reading request"
We've recently added traces for clients to flag server non-compliance; let's do the same for servers to flag client non-compliance. According to the spec, if the client requests NBD_INFO_BLOCK_SIZE, it is promising to send all requests aligned to those boundaries. Of course, if the client does not request NBD_INFO_BLOCK_SIZE, then it made no promises so we shouldn't flag anything; and because we are willing to handle clients that made no promises (the spec allows us to use NBD_REP_ERR_BLOCK_SIZE_REQD if we had been unwilling), we already have to handle unaligned requests (which the block layer already does on our behalf). So even though the spec allows us to return EINVAL for clients that promised to behave, it's easier to always answer unaligned requests. Still, flagging non-compliance can be useful in debugging a client that is trying to be maximally portable. Qemu as client used to have one spot where it sent non-compliant requests: if the server sends an unaligned reply to NBD_CMD_BLOCK_STATUS, and the client was iterating over the entire disk, the next request would start at that unaligned point; this was fixed in commit a39286dd when the client was taught to work around server non-compliance; but is equally fixed if the server is patched to not send unaligned replies in the first place (the next few patches will address that). Fortunately, I did not find any more spots where qemu as client was non-compliant. I was able to test the patch by using the following hack to convince qemu-io to run various unaligned commands, coupled with serving 512-byte alignment by intentionally omitting '-f raw' on the server while viewing server traces. | diff --git i/nbd/client.c w/nbd/client.c | index 427980bdd22..1858b2aac35 100644 | --- i/nbd/client.c | +++ w/nbd/client.c | @@ -449,6 +449,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt, | nbd_send_opt_abort(ioc); | return -1; | } | + info->min_block = 1;//hack | if (!is_power_of_2(info->min_block)) { | error_setg(errp, "server minimum block size %" PRIu32 | " is not a power of two", info->min_block); Signed-off-by: Eric Blake <eblake@redhat.com> --- nbd/server.c | 12 ++++++++++++ nbd/trace-events | 1 + 2 files changed, 13 insertions(+)