qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3
@ 2019-04-08 19:02 Eric Blake
  2019-04-08 19:02 ` Eric Blake
                   ` (5 more replies)
  0 siblings, 6 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel

The following changes since commit 2c573106279495795449b0d0373464b597dfe316:

  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2019-04-08' into staging (2019-04-08 15:21:11 +0100)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-04-08

for you to fetch changes up to e53f88df77e70350b0eda92a2e5e39f67792008f:

  nbd/client: Fix error message for server with unusable sizing (2019-04-08 13:51:25 -0500)

----------------------------------------------------------------
nbd patches for 2019-04-08

- Fix minor issues in recent alignment patches

----------------------------------------------------------------
Eric Blake (4):
      nbd/server: Fix blockstatus trace
      nbd/server: Trace client noncompliance on unaligned requests
      nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
      nbd/client: Fix error message for server with unusable sizing

 nbd/client.c     |  2 +-
 nbd/server.c     | 39 +++++++++++++++++++++++++++------------
 nbd/trace-events |  1 +
 3 files changed, 29 insertions(+), 13 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
@ 2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02 ` [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace Eric Blake
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel

The following changes since commit 2c573106279495795449b0d0373464b597dfe316:

  Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2019-04-08' into staging (2019-04-08 15:21:11 +0100)

are available in the Git repository at:

  https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-04-08

for you to fetch changes up to e53f88df77e70350b0eda92a2e5e39f67792008f:

  nbd/client: Fix error message for server with unusable sizing (2019-04-08 13:51:25 -0500)

----------------------------------------------------------------
nbd patches for 2019-04-08

- Fix minor issues in recent alignment patches

----------------------------------------------------------------
Eric Blake (4):
      nbd/server: Fix blockstatus trace
      nbd/server: Trace client noncompliance on unaligned requests
      nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
      nbd/client: Fix error message for server with unusable sizing

 nbd/client.c     |  2 +-
 nbd/server.c     | 39 +++++++++++++++++++++++++++------------
 nbd/trace-events |  1 +
 3 files changed, 29 insertions(+), 13 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
  2019-04-08 19:02 ` Eric Blake
@ 2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02   ` Eric Blake
  2019-04-08 19:02 ` [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests Eric Blake
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

Don't increment remaining_bytes until we know that we will actually be
including the current block status extent in the reply; otherwise, the
value traced will include a bytes value that is oversized by the
length of the next block status extent which did not get sent because
it instead ended the loop.

Fixes: fb7afc79
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 218a2aa5e65..1b8c8619896 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1880,17 +1880,12 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,

         flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
                 (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0);
-        offset += num;
-        remaining_bytes -= num;

         if (first_extent) {
             extent->flags = flags;
             extent->length = num;
             first_extent = false;
-            continue;
-        }
-
-        if (flags == extent->flags) {
+        } else if (flags == extent->flags) {
             /* extend current extent */
             extent->length += num;
         } else {
@@ -1903,6 +1898,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
             extent->flags = flags;
             extent->length = num;
         }
+        offset += num;
+        remaining_bytes -= num;
     }

     extents_end = extent + 1;
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace
  2019-04-08 19:02 ` [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace Eric Blake
@ 2019-04-08 19:02   ` Eric Blake
  0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

Don't increment remaining_bytes until we know that we will actually be
including the current block status extent in the reply; otherwise, the
value traced will include a bytes value that is oversized by the
length of the next block status extent which did not get sent because
it instead ended the loop.

Fixes: fb7afc79
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 218a2aa5e65..1b8c8619896 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1880,17 +1880,12 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,

         flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
                 (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0);
-        offset += num;
-        remaining_bytes -= num;

         if (first_extent) {
             extent->flags = flags;
             extent->length = num;
             first_extent = false;
-            continue;
-        }
-
-        if (flags == extent->flags) {
+        } else if (flags == extent->flags) {
             /* extend current extent */
             extent->length += num;
         } else {
@@ -1903,6 +1898,8 @@ static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
             extent->flags = flags;
             extent->length = num;
         }
+        offset += num;
+        remaining_bytes -= num;
     }

     extents_end = extent + 1;
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
  2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02 ` [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace Eric Blake
@ 2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02   ` Eric Blake
  2019-04-08 19:02 ` [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources Eric Blake
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

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 (yes, qemu 4.0 as
server still has few such bugs, although they will be patched in
4.1). 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>
Message-Id: <20190403030526.12258-3-eblake@redhat.com>
[eblake: address minor review nits]
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c     | 17 ++++++++++++++++-
 nbd/trace-events |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 1b8c8619896..1c4c5474ad4 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;

@@ -533,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)
@@ -609,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;
     }
@@ -660,6 +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 = check_align;
         QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
         nbd_export_get(client->exp);
         nbd_check_meta_export(client);
@@ -2126,6 +2130,17 @@ 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),
+                                              request->from,
+                                              request->len,
+                                              client->check_align);
+    }
     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..7ab6b3788cb 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, uint64_t from, uint32_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx32 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests
  2019-04-08 19:02 ` [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests Eric Blake
@ 2019-04-08 19:02   ` Eric Blake
  0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

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 (yes, qemu 4.0 as
server still has few such bugs, although they will be patched in
4.1). 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>
Message-Id: <20190403030526.12258-3-eblake@redhat.com>
[eblake: address minor review nits]
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c     | 17 ++++++++++++++++-
 nbd/trace-events |  1 +
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/nbd/server.c b/nbd/server.c
index 1b8c8619896..1c4c5474ad4 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;

@@ -533,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)
@@ -609,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;
     }
@@ -660,6 +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 = check_align;
         QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
         nbd_export_get(client->exp);
         nbd_check_meta_export(client);
@@ -2126,6 +2130,17 @@ 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),
+                                              request->from,
+                                              request->len,
+                                              client->check_align);
+    }
     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..7ab6b3788cb 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, uint64_t from, uint32_t len, uint32_t align) "client sent non-compliant unaligned %s request: from=0x%" PRIx64 ", len=0x%" PRIx32 ", align=0x%" PRIx32
 nbd_trip(void) "Reading request"
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
                   ` (2 preceding siblings ...)
  2019-04-08 19:02 ` [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests Eric Blake
@ 2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02   ` Eric Blake
  2019-04-08 19:02 ` [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing Eric Blake
  2019-04-09  9:02 ` [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Peter Maydell
  5 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

In commit 0c1d50bd, I added a couple of TODO comments about whether we
consult bl.request_alignment when responding to NBD_OPT_INFO. At the
time, qemu as server was hard-coding an advertised alignment of 512 to
clients that promised to obey constraints, and there was no function
for getting at a device's preferred alignment. But in hindsight,
advertising 512 when the block device prefers 1 caused other
compliance problems, and commit b0245d64 changed one of the two TODO
comments to advertise a more accurate alignment. Time to fix the other
TODO.  Doesn't really impact qemu as client (our normal client doesn't
use NBD_OPT_INFO, and qemu-nbd --list promises to obey block sizes),
but it might prove useful to other clients.

Fixes: b0245d64
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-4-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 1c4c5474ad4..e21bd501dc6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -643,11 +643,14 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
         return rc;
     }

-    /* If the client is just asking for NBD_OPT_INFO, but forgot to
-     * request block sizes, return an error.
-     * TODO: consult blk_bs(blk)->request_align, and only error if it
-     * is not 1? */
-    if (client->opt == NBD_OPT_INFO && !blocksize) {
+    /*
+     * If the client is just asking for NBD_OPT_INFO, but forgot to
+     * request block sizes in a situation that would impact
+     * performance, then return an error. But for NBD_OPT_GO, we
+     * tolerate all clients, regardless of alignments.
+     */
+    if (client->opt == NBD_OPT_INFO && !blocksize &&
+        blk_get_request_alignment(exp->blk) > 1) {
         return nbd_negotiate_send_rep_err(client,
                                           NBD_REP_ERR_BLOCK_SIZE_REQD,
                                           errp,
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
  2019-04-08 19:02 ` [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources Eric Blake
@ 2019-04-08 19:02   ` Eric Blake
  0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Vladimir Sementsov-Ogievskiy, open list:Network Block Dev...

In commit 0c1d50bd, I added a couple of TODO comments about whether we
consult bl.request_alignment when responding to NBD_OPT_INFO. At the
time, qemu as server was hard-coding an advertised alignment of 512 to
clients that promised to obey constraints, and there was no function
for getting at a device's preferred alignment. But in hindsight,
advertising 512 when the block device prefers 1 caused other
compliance problems, and commit b0245d64 changed one of the two TODO
comments to advertise a more accurate alignment. Time to fix the other
TODO.  Doesn't really impact qemu as client (our normal client doesn't
use NBD_OPT_INFO, and qemu-nbd --list promises to obey block sizes),
but it might prove useful to other clients.

Fixes: b0245d64
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190403030526.12258-4-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 1c4c5474ad4..e21bd501dc6 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -643,11 +643,14 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
         return rc;
     }

-    /* If the client is just asking for NBD_OPT_INFO, but forgot to
-     * request block sizes, return an error.
-     * TODO: consult blk_bs(blk)->request_align, and only error if it
-     * is not 1? */
-    if (client->opt == NBD_OPT_INFO && !blocksize) {
+    /*
+     * If the client is just asking for NBD_OPT_INFO, but forgot to
+     * request block sizes in a situation that would impact
+     * performance, then return an error. But for NBD_OPT_GO, we
+     * tolerate all clients, regardless of alignments.
+     */
+    if (client->opt == NBD_OPT_INFO && !blocksize &&
+        blk_get_request_alignment(exp->blk) > 1) {
         return nbd_negotiate_send_rep_err(client,
                                           NBD_REP_ERR_BLOCK_SIZE_REQD,
                                           errp,
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
                   ` (3 preceding siblings ...)
  2019-04-08 19:02 ` [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources Eric Blake
@ 2019-04-08 19:02 ` Eric Blake
  2019-04-08 19:02   ` Eric Blake
  2019-04-09  9:02 ` [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Peter Maydell
  5 siblings, 1 reply; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, open list:Network Block Dev...

Add a missing space to the error message used when giving up on a
server that insists on an alignment which renders the last few bytes
of the export unreadable.

Fixes: 3add3ab78
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190404145226.32649-1-eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 nbd/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client.c b/nbd/client.c
index 427980bdd22..4de30630c73 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -428,7 +428,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
             }
             if (info->min_block &&
                 !QEMU_IS_ALIGNED(info->size, info->min_block)) {
-                error_setg(errp, "export size %" PRIu64 "is not multiple of "
+                error_setg(errp, "export size %" PRIu64 " is not multiple of "
                            "minimum block size %" PRIu32, info->size,
                            info->min_block);
                 nbd_send_opt_abort(ioc);
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing
  2019-04-08 19:02 ` [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing Eric Blake
@ 2019-04-08 19:02   ` Eric Blake
  0 siblings, 0 replies; 12+ messages in thread
From: Eric Blake @ 2019-04-08 19:02 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, open list:Network Block Dev...

Add a missing space to the error message used when giving up on a
server that insists on an alignment which renders the last few bytes
of the export unreadable.

Fixes: 3add3ab78
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190404145226.32649-1-eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 nbd/client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/nbd/client.c b/nbd/client.c
index 427980bdd22..4de30630c73 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -428,7 +428,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
             }
             if (info->min_block &&
                 !QEMU_IS_ALIGNED(info->size, info->min_block)) {
-                error_setg(errp, "export size %" PRIu64 "is not multiple of "
+                error_setg(errp, "export size %" PRIu64 " is not multiple of "
                            "minimum block size %" PRIu32, info->size,
                            info->min_block);
                 nbd_send_opt_abort(ioc);
-- 
2.20.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3
  2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
                   ` (4 preceding siblings ...)
  2019-04-08 19:02 ` [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing Eric Blake
@ 2019-04-09  9:02 ` Peter Maydell
  2019-04-09  9:02   ` Peter Maydell
  5 siblings, 1 reply; 12+ messages in thread
From: Peter Maydell @ 2019-04-09  9:02 UTC (permalink / raw)
  To: Eric Blake; +Cc: QEMU Developers

On Mon, 8 Apr 2019 at 20:04, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit 2c573106279495795449b0d0373464b597dfe316:
>
>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2019-04-08' into staging (2019-04-08 15:21:11 +0100)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-04-08
>
> for you to fetch changes up to e53f88df77e70350b0eda92a2e5e39f67792008f:
>
>   nbd/client: Fix error message for server with unusable sizing (2019-04-08 13:51:25 -0500)
>
> ----------------------------------------------------------------
> nbd patches for 2019-04-08
>
> - Fix minor issues in recent alignment patches
>
> ----------------------------------------------------------------
> Eric Blake (4):
>       nbd/server: Fix blockstatus trace
>       nbd/server: Trace client noncompliance on unaligned requests
>       nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
>       nbd/client: Fix error message for server with unusable sizing
>
>  nbd/client.c     |  2 +-
>  nbd/server.c     | 39 +++++++++++++++++++++++++++------------
>  nbd/trace-events |  1 +
>  3 files changed, 29 insertions(+), 13 deletions(-)
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3
  2019-04-09  9:02 ` [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Peter Maydell
@ 2019-04-09  9:02   ` Peter Maydell
  0 siblings, 0 replies; 12+ messages in thread
From: Peter Maydell @ 2019-04-09  9:02 UTC (permalink / raw)
  To: Eric Blake; +Cc: QEMU Developers

On Mon, 8 Apr 2019 at 20:04, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit 2c573106279495795449b0d0373464b597dfe316:
>
>   Merge remote-tracking branch 'remotes/huth-gitlab/tags/pull-request-2019-04-08' into staging (2019-04-08 15:21:11 +0100)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-04-08
>
> for you to fetch changes up to e53f88df77e70350b0eda92a2e5e39f67792008f:
>
>   nbd/client: Fix error message for server with unusable sizing (2019-04-08 13:51:25 -0500)
>
> ----------------------------------------------------------------
> nbd patches for 2019-04-08
>
> - Fix minor issues in recent alignment patches
>
> ----------------------------------------------------------------
> Eric Blake (4):
>       nbd/server: Fix blockstatus trace
>       nbd/server: Trace client noncompliance on unaligned requests
>       nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources
>       nbd/client: Fix error message for server with unusable sizing
>
>  nbd/client.c     |  2 +-
>  nbd/server.c     | 39 +++++++++++++++++++++++++++------------
>  nbd/trace-events |  1 +
>  3 files changed, 29 insertions(+), 13 deletions(-)
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/4.0
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2019-04-09  9:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-08 19:02 [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Eric Blake
2019-04-08 19:02 ` Eric Blake
2019-04-08 19:02 ` [Qemu-devel] [PULL 1/4] nbd/server: Fix blockstatus trace Eric Blake
2019-04-08 19:02   ` Eric Blake
2019-04-08 19:02 ` [Qemu-devel] [PULL 2/4] nbd/server: Trace client noncompliance on unaligned requests Eric Blake
2019-04-08 19:02   ` Eric Blake
2019-04-08 19:02 ` [Qemu-devel] [PULL 3/4] nbd/server: Don't fail NBD_OPT_INFO for byte-aligned sources Eric Blake
2019-04-08 19:02   ` Eric Blake
2019-04-08 19:02 ` [Qemu-devel] [PULL 4/4] nbd/client: Fix error message for server with unusable sizing Eric Blake
2019-04-08 19:02   ` Eric Blake
2019-04-09  9:02 ` [Qemu-devel] [PULL 0/4] NBD patches for 4.0-rc3 Peter Maydell
2019-04-09  9:02   ` Peter Maydell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).