Re: [Qemu-block] [PATCH 2/7] nbd/server: Trace server noncompliance on unaligned requests

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> ---
>   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 <address@hidden>

-- 
Best regards,
Vladimir