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

From: Eric Blake <eblake@redhat.com>
To: qemu-devel@nongnu.org
Cc: jsnow@redhat.com, vsementsov@virtuozzo.com, qemu-block@nongnu.org
Subject: [Qemu-devel] [PATCH 2/7] nbd/server: Trace server noncompliance on unaligned requests
Date: Tue,  2 Apr 2019 22:05:21 -0500	[thread overview]
Message-ID: <20190403030526.12258-3-eblake@redhat.com> (raw)
In-Reply-To: <20190403030526.12258-1-eblake@redhat.com>

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;
         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"
-- 
2.20.1
