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 <ebl...@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 <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
