[Qemu-devel] [PULL 00/14] NBD patches for 4.0-rc2

[Qemu-devel] [PULL 00/14] NBD patches for 4.0-rc2

[Eric Blake]

The following changes since commit 230ce19814ecc6bff8edac3b5b86e7c82f422c6c:

  Merge remote-tracking branch 'remotes/rth/tags/pull-axp-20190325' into staging (2019-03-29 19:29:00 +0000)

are available in the Git repository at:

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

for you to fetch changes up to 75d34eb98ca0bb2f49d607fcaefd9c482e56b15d:

  nbd/client: Trace server noncompliance on structured reads (2019-04-01 08:58:04 -0500)

----------------------------------------------------------------
nbd patches for 2019-04-01

- Better behavior of qemu-img map on NBD images
- Fixes for NBD protocol alignment corner cases:
 - the server has fewer places where it sends reads or block status
   not aligned to its advertised block size
 - the client has more cases where it can work around server
   non-compliance present in qemu 3.1
 - the client now avoids non-compliant requests when interoperating
   with nbdkit or other servers not advertising block size

----------------------------------------------------------------
Eric Blake (14):
      qemu-img: Report bdrv_block_status failures
      nbd: Tolerate some server non-compliance in NBD_CMD_BLOCK_STATUS
      nbd: Don't lose server's error to NBD_CMD_BLOCK_STATUS
      nbd: Permit simple error to NBD_CMD_BLOCK_STATUS
      qemu-img: Gracefully shutdown when map can't finish
      nbd-client: Work around server BLOCK_STATUS misalignment at EOF
      iotests: Add 241 to test NBD on unaligned images
      nbd/client: Lower min_block for block-status, unaligned size
      nbd/client: Report offsets in bdrv_block_status
      nbd/client: Reject inaccessible tail of inconsistent server
      nbd/client: Support qemu-img convert from unaligned size
      block: Add bdrv_get_request_alignment()
      nbd/server: Advertise actual minimum block size
      nbd/client: Trace server noncompliance on structured reads

 include/sysemu/block-backend.h |   1 +
 block/block-backend.c          |   7 +++
 block/nbd-client.c             | 124 +++++++++++++++++++++++++++++++++--------
 block/nbd.c                    |  19 ++++++-
 nbd/client.c                   |   8 +++
 nbd/server.c                   |  13 +++--
 qemu-img.c                     |  16 ++++--
 block/trace-events             |   2 +
 tests/qemu-iotests/209.out     |   4 +-
 tests/qemu-iotests/223.out     |  22 ++++----
 tests/qemu-iotests/233.out     |   2 +-
 tests/qemu-iotests/241         | 100 +++++++++++++++++++++++++++++++++
 tests/qemu-iotests/241.out     |  28 ++++++++++
 tests/qemu-iotests/group       |   1 +
 14 files changed, 299 insertions(+), 48 deletions(-)
 create mode 100755 tests/qemu-iotests/241
 create mode 100644 tests/qemu-iotests/241.out

-- 
2.20.1

[Qemu-devel] [PULL 01/14] qemu-img: Report bdrv_block_status failures

[Eric Blake]

If bdrv_block_status_above() fails, we are aborting the convert
process but failing to print an error message.  Broken in commit
690c7301 (v2.4) when rewriting convert's logic.

Discovered when teaching nbdkit to support NBD_CMD_BLOCK_STATUS, and
accidentally violating the protocol by returning more than one extent
in spite of qemu asking for NBD_CMD_FLAG_REQ_ONE.  The qemu NBD code
should probably handle the server's non-compliance more gracefully
than failing with EINVAL, but qemu-img shouldn't be silently
squelching any block status failures. It doesn't help that qemu 3.1
masks the qemu-img bug with extra noise that the nbd code is dumping
to stderr (that noise was cleaned up in d8b4bad8).

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190323212639.579-2-eblake@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qemu-img.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/qemu-img.c b/qemu-img.c
index 8ee63daeaeb..03a9a10dec1 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1630,6 +1630,8 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
                                           count, &count, NULL, NULL);
         }
         if (ret < 0) {
+            error_report("error while reading block status of sector %" PRId64
+                         ": %s", sector_num, strerror(-ret));
             return ret;
         }
         n = DIV_ROUND_UP(count, BDRV_SECTOR_SIZE);
-- 
2.20.1

[Qemu-devel] [PULL 02/14] nbd: Tolerate some server non-compliance in NBD_CMD_BLOCK_STATUS

[Eric Blake]

The NBD spec states that NBD_CMD_FLAG_REQ_ONE (which we currently
always use) should not reply with an extent larger than our request,
and that the server's response should be exactly one extent. Right
now, that means that if a server sends more than one extent, we treat
the server as broken, fail the block status request, and disconnect,
which prevents all further use of the block device. But while good
software should be strict in what it sends, it should be tolerant in
what it receives.

While trying to implement NBD_CMD_BLOCK_STATUS in nbdkit, we
temporarily had a non-compliant server sending too many extents in
spite of REQ_ONE. Oddly enough, 'qemu-img convert' with qemu 3.1
failed with a somewhat useful message:
  qemu-img: Protocol error: invalid payload for NBD_REPLY_TYPE_BLOCK_STATUS

which then disappeared with commit d8b4bad8, on the grounds that an
error message flagged only at the time of coroutine teardown is
pointless, and instead we should rely on the actual failed API to
report an error - in other words, the 3.1 behavior was masking the
fact that qemu-img was not reporting an error. That has since been
fixed in the previous patch, where qemu-img convert now fails with:
  qemu-img: error while reading block status of sector 0: Invalid argument

But even that is harsh.  Since we already partially relaxed things in
commit acfd8f7a to tolerate a server that exceeds the cap (although
that change was made prior to the NBD spec actually putting a cap on
the extent length during REQ_ONE - in fact, the NBD spec change was
BECAUSE of the qemu behavior prior to that commit), it's not that much
harder to argue that we should also tolerate a server that sends too
many extents.  But at the same time, it's nice to trace when we are
being tolerant of server non-compliance, in order to help server
writers fix their implementations to be more portable (if they refer
to our traces, rather than just stderr).

Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190323212639.579-3-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 21 ++++++++++++++++-----
 block/trace-events |  1 +
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index bfbaf7ebe94..8fe660b6093 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -240,8 +240,8 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
 }

 /* nbd_parse_blockstatus_payload
- * support only one extent in reply and only for
- * base:allocation context
+ * Based on our request, we expect only one extent in reply, for the
+ * base:allocation context.
  */
 static int nbd_parse_blockstatus_payload(NBDClientSession *client,
                                          NBDStructuredReplyChunk *chunk,
@@ -250,7 +250,8 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,
 {
     uint32_t context_id;

-    if (chunk->length != sizeof(context_id) + sizeof(*extent)) {
+    /* The server succeeded, so it must have sent [at least] one extent */
+    if (chunk->length < sizeof(context_id) + sizeof(*extent)) {
         error_setg(errp, "Protocol error: invalid payload for "
                          "NBD_REPLY_TYPE_BLOCK_STATUS");
         return -EINVAL;
@@ -276,10 +277,20 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,
         return -EINVAL;
     }

-    /* The server is allowed to send us extra information on the final
-     * extent; just clamp it to the length we requested. */
+    /*
+     * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have
+     * sent us any more than one extent, nor should it have included
+     * status beyond our request in that extent. However, it's easy
+     * enough to ignore the server's noncompliance without killing the
+     * connection; just ignore trailing extents, and clamp things to
+     * the length of our request.
+     */
+    if (chunk->length > sizeof(context_id) + sizeof(*extent)) {
+        trace_nbd_parse_blockstatus_compliance("more than one extent");
+    }
     if (extent->length > orig_length) {
         extent->length = orig_length;
+        trace_nbd_parse_blockstatus_compliance("extent length too large");
     }

     return 0;
diff --git a/block/trace-events b/block/trace-events
index e6bb5a8f05c..debb25c0ac8 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -157,6 +157,7 @@ nvme_cmd_map_qiov_iov(void *s, int i, void *page, int pages) "s %p iov[%d] %p pa
 iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, uint64_t bytes, int ret) "src_lun %p offset %"PRIu64" dst_lun %p offset %"PRIu64" bytes %"PRIu64" ret %d"

 # nbd-client.c
+nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from non-compliant server: %s"
 nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
 nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"

-- 
2.20.1

[Qemu-devel] [PULL 03/14] nbd: Don't lose server's error to NBD_CMD_BLOCK_STATUS

[Eric Blake]

When the server replies with a (structured [*]) error to
NBD_CMD_BLOCK_STATUS, without any extent information sent first, the
client code was blindly throwing away the server's error code and
instead telling the caller that EIO occurred.  This has been broken
since its introduction in 78a33ab5 (v2.12, where we should have called:
   error_setg(&local_err, "Server did not reply with any status extents");
   nbd_iter_error(&iter, false, -EIO, &local_err);
to declare the situation as a non-fatal error if no earlier error had
already been flagged, rather than just blindly slamming iter.err and
iter.ret), although it is more noticeable since commit 7f86068d, which
actually tries hard to preserve the server's code thanks to a separate
iter.request_ret.

[*] The spec is clear that the server is also permitted to reply with
a simple error, but that's a separate fix.

I was able to provoke this scenario with a hack to the server, then
seeing whether ENOMEM makes it back to the caller:

| diff --git a/nbd/server.c b/nbd/server.c
| index fd013a2817a..29c7995de02 100644
| --- a/nbd/server.c
| +++ b/nbd/server.c
| @@ -2269,6 +2269,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
|                                        "discard failed", errp);
|
|      case NBD_CMD_BLOCK_STATUS:
| +        return nbd_send_generic_reply(client, request->handle, -ENOMEM,
| +                                      "no status for you today", errp);
|          if (!request->len) {
|              return nbd_send_generic_reply(client, request->handle, -EINVAL,
|                                            "need non-zero length", errp);
| --

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

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 8fe660b6093..b37a5963013 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -769,12 +769,9 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
         payload = NULL;
     }

-    if (!extent->length && !iter.err) {
-        error_setg(&iter.err,
-                   "Server did not reply with any status extents");
-        if (!iter.ret) {
-            iter.ret = -EIO;
-        }
+    if (!extent->length && !iter.request_ret) {
+        error_setg(&local_err, "Server did not reply with any status extents");
+        nbd_iter_channel_error(&iter, -EIO, &local_err);
     }

     error_propagate(errp, iter.err);
-- 
2.20.1

[Qemu-devel] [PULL 04/14] nbd: Permit simple error to NBD_CMD_BLOCK_STATUS

[Eric Blake]

The NBD spec is clear that when structured replies are active, a
simple error reply is acceptable to any command except for
NBD_CMD_READ.  However, we were mistakenly requiring structured errors
for NBD_CMD_BLOCK_STATUS, and hanging up on a server that gave a
simple error (since qemu does not behave as such a server, we didn't
notice the problem until now).  Broken since its introduction in
commit 78a33ab5 (v2.12).

Noticed while debugging a separate failure reported by nbdkit while
working out its initial implementation of BLOCK_STATUS, although it
turns out that nbdkit also chose to send structured error replies for
BLOCK_STATUS, so I had to manually provoke the situation by hacking
qemu's server to send a simple error reply:

| diff --git i/nbd/server.c w/nbd/server.c
| index fd013a2817a..833288d7c45 100644
| 00--- i/nbd/server.c
| +++ w/nbd/server.c
| @@ -2269,6 +2269,8 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
|                                        "discard failed", errp);
|
|      case NBD_CMD_BLOCK_STATUS:
| +        return nbd_co_send_simple_reply(client, request->handle, ENOMEM,
| +                                        NULL, 0, errp);
|          if (!request->len) {
|              return nbd_send_generic_reply(client, request->handle, -EINVAL,
|                                            "need non-zero length", errp);
|

Signed-off-by: Eric Blake <eblake@redhat.com>
Acked-by: Richard W.M. Jones <rjones@redhat.com>
Message-Id: <20190325190104.30213-3-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index b37a5963013..a3b70d14004 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -729,9 +729,7 @@ static int nbd_co_receive_blockstatus_reply(NBDClientSession *s,
     bool received = false;

     assert(!extent->length);
-    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
-                            NULL, &reply, &payload)
-    {
+    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
         int ret;
         NBDStructuredReplyChunk *chunk = &reply.structured;

-- 
2.20.1

[Qemu-devel] [PULL 05/14] qemu-img: Gracefully shutdown when map can't finish

[Eric Blake]

Trying 'qemu-img map -f raw nbd://localhost:10809' causes the
NBD server to output a scary message:

qemu-nbd: Disconnect client, due to: Failed to read request: Unexpected end-of-file before all bytes were read

This is because the NBD client, being remote, has no way to expose a
human-readable map (the --output=json data is fine, however). But
because we exit(1) right after the message, causing the client to
bypass all block cleanup, the server sees the abrupt exit and warns,
whereas it would be silent had the client had a chance to send
NBD_CMD_DISC. Other protocols may have similar cleanup issues, where
failure to blk_unref() could cause unintended effects.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190326184043.7544-1-eblake@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-img.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 03a9a10dec1..76a961df824 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2736,14 +2736,14 @@ static int img_info(int argc, char **argv)
     return 0;
 }

-static void dump_map_entry(OutputFormat output_format, MapEntry *e,
-                           MapEntry *next)
+static int dump_map_entry(OutputFormat output_format, MapEntry *e,
+                          MapEntry *next)
 {
     switch (output_format) {
     case OFORMAT_HUMAN:
         if (e->data && !e->has_offset) {
             error_report("File contains external, encrypted or compressed clusters.");
-            exit(1);
+            return -1;
         }
         if (e->data && !e->zero) {
             printf("%#-16"PRIx64"%#-16"PRIx64"%#-16"PRIx64"%s\n",
@@ -2776,6 +2776,7 @@ static void dump_map_entry(OutputFormat output_format, MapEntry *e,
         }
         break;
     }
+    return 0;
 }

 static int get_block_status(BlockDriverState *bs, int64_t offset,
@@ -2968,12 +2969,15 @@ static int img_map(int argc, char **argv)
         }

         if (curr.length > 0) {
-            dump_map_entry(output_format, &curr, &next);
+            ret = dump_map_entry(output_format, &curr, &next);
+            if (ret < 0) {
+                goto out;
+            }
         }
         curr = next;
     }

-    dump_map_entry(output_format, &curr, NULL);
+    ret = dump_map_entry(output_format, &curr, NULL);

 out:
     blk_unref(blk);
-- 
2.20.1

[Qemu-devel] [PULL 06/14] nbd-client: Work around server BLOCK_STATUS misalignment at EOF

[Eric Blake]

The NBD spec is clear that a server that advertises a minimum block
size should reply to NBD_CMD_BLOCK_STATUS with extents aligned
accordingly. However, we know that the qemu NBD server implementation
has had a corner-case bug where it is not compliant with the spec,
present since the introduction of NBD_CMD_BLOCK_STATUS in qemu 2.12
(and unlikely to be patched in time for 4.0). Namely, when qemu is
serving a file that is not a multiple of 512 bytes, it rounds the size
advertised over NBD up to the next sector boundary (someday, I'd like
to fix that to be byte-accurate, but it's a much bigger audit not
appropriate for this release); yet if the final sector contains data
prior to EOF, lseek(SEEK_HOLE) will point to the implicit hole
mid-sector which qemu then reported over NBD.

We are well within our rights to hang up on a server that can't follow
the spec, but it is more useful to try and keep the connection alive
in spite of the problem. Do so by tracing a message about the problem,
and then either truncating the request back to an aligned boundary (if
it covered more than the final sector) or widening it out to the full
boundary with a forced status of data (since truncating would result
in 0 bytes, but we have to make progress, and valid since data is a
default-safe answer). And in practice, since the problem only happens
on a sector that starts with data and ends with a hole, we are going
to want to read that full sector anyway (where qemu as the server
fills in the tail beyond EOF with appropriate NUL bytes).

Easy reproduction:
$ printf %1000d 1 > file
$ qemu-nbd -f raw -t file & pid=$!
$ qemu-img map --output=json -f raw nbd://localhost:10809
qemu-img: Could not read file metadata: Invalid argument
$ kill $pid

where the patched version instead succeeds with:
[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190326171317.4036-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index a3b70d14004..150af9cc46f 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -269,14 +269,36 @@ static int nbd_parse_blockstatus_payload(NBDClientSession *client,
     extent->length = payload_advance32(&payload);
     extent->flags = payload_advance32(&payload);

-    if (extent->length == 0 ||
-        (client->info.min_block && !QEMU_IS_ALIGNED(extent->length,
-                                                    client->info.min_block))) {
+    if (extent->length == 0) {
         error_setg(errp, "Protocol error: server sent status chunk with "
-                   "invalid length");
+                   "zero length");
         return -EINVAL;
     }

+    /*
+     * A server sending unaligned block status is in violation of the
+     * protocol, but as qemu-nbd 3.1 is such a server (at least for
+     * POSIX files that are not a multiple of 512 bytes, since qemu
+     * rounds files up to 512-byte multiples but lseek(SEEK_HOLE)
+     * still sees an implicit hole beyond the real EOF), it's nicer to
+     * work around the misbehaving server. If the request included
+     * more than the final unaligned block, truncate it back to an
+     * aligned result; if the request was only the final block, round
+     * up to the full block and change the status to fully-allocated
+     * (always a safe status, even if it loses information).
+     */
+    if (client->info.min_block && !QEMU_IS_ALIGNED(extent->length,
+                                                   client->info.min_block)) {
+        trace_nbd_parse_blockstatus_compliance("extent length is unaligned");
+        if (extent->length > client->info.min_block) {
+            extent->length = QEMU_ALIGN_DOWN(extent->length,
+                                             client->info.min_block);
+        } else {
+            extent->length = client->info.min_block;
+            extent->flags = 0;
+        }
+    }
+
     /*
      * We used NBD_CMD_FLAG_REQ_ONE, so the server should not have
      * sent us any more than one extent, nor should it have included
-- 
2.20.1

[Qemu-devel] [PULL 07/14] iotests: Add 241 to test NBD on unaligned images

[Eric Blake]

Add a test for the NBD client workaround in the previous patch.  It's
not really feasible for an iotest to assume a specific tracing engine,
so we can't really probe trace_nbd_parse_blockstatus_compliance to see
if the server was fixed vs. whether the client just worked around the
server (other than by rearranging order between code patches and this
test). But having a successful exchange sure beats the previous state
of an error message. Since format probing can change alignment, we can
use that as an easy way to test several configurations.

Not tested yet, but worth adding to this test in future patches: an
NBD server that can advertise a non-sector-aligned size (such as
nbdkit) causes qemu as the NBD client to misbehave when it rounds the
size up and accesses beyond the advertised size. Qemu as NBD server
never advertises a non-sector-aligned size (since bdrv_getlength()
currently rounds up to sector boundaries); until qemu can act as such
a server, testing that flaw will have to rely on external binaries.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329042750.14704-2-eblake@redhat.com>
Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: add forced-512 alignment, and nbdkit reproducer comment]
---
 tests/qemu-iotests/241     | 100 +++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/241.out |  26 ++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 127 insertions(+)
 create mode 100755 tests/qemu-iotests/241
 create mode 100644 tests/qemu-iotests/241.out

diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
new file mode 100755
index 00000000000..4b196857387
--- /dev/null
+++ b/tests/qemu-iotests/241
@@ -0,0 +1,100 @@
+#!/bin/bash
+#
+# Test qemu-nbd vs. unaligned images
+#
+# Copyright (C) 2018-2019 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1 # failure is the default!
+
+nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
+
+_cleanup()
+{
+    _cleanup_test_img
+    nbd_server_stop
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.nbd
+
+_supported_fmt raw
+_supported_proto nbd
+_supported_os Linux
+_require_command QEMU_NBD
+
+# can't use _make_test_img, because qemu-img rounds image size up,
+# and because we want to use Unix socket rather than TCP port. Likewise,
+# we have to redirect TEST_IMG to our server.
+# This tests that we can deal with the hole at the end of an unaligned
+# raw file (either because the server doesn't advertise alignment too
+# large, or because the client ignores the server's noncompliance - even
+# though we can't actually wire iotests into checking trace messages).
+printf %01000d 0 > "$TEST_IMG_FILE"
+TEST_IMG="nbd:unix:$nbd_unix_socket"
+
+echo
+echo "=== Exporting unaligned raw image, natural alignment ==="
+echo
+
+nbd_server_start_unix_socket -f $IMGFMT "$TEST_IMG_FILE"
+
+$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
+$QEMU_IMG map -f raw --output=json "$TEST_IMG" | _filter_qemu_img_map
+$QEMU_IO -f raw -c map "$TEST_IMG"
+nbd_server_stop
+
+echo
+echo "=== Exporting unaligned raw image, forced server sector alignment ==="
+echo
+
+# Intentionally omit '-f' to force image probing, which in turn forces
+# sector alignment, here at the server.
+nbd_server_start_unix_socket "$TEST_IMG_FILE"
+
+$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
+$QEMU_IMG map -f raw --output=json "$TEST_IMG" | _filter_qemu_img_map
+$QEMU_IO -f raw -c map "$TEST_IMG"
+nbd_server_stop
+
+echo
+echo "=== Exporting unaligned raw image, forced client sector alignment ==="
+echo
+
+# Now force sector alignment at the client.
+nbd_server_start_unix_socket -f $IMGFMT "$TEST_IMG_FILE"
+
+$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+$QEMU_IO -c map "$TEST_IMG"
+nbd_server_stop
+
+# Not tested yet: we also want to ensure that qemu as NBD client does
+# not access beyond the end of a server's advertised unaligned size:
+#  nbdkit -U - memory size=513 --run 'qemu-io -f raw -c "r 512 512" $nbd'
+# However, since qemu as server always rounds up to a sector alignment,
+# we would have to use nbdkit to provoke the current client failures.
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
new file mode 100644
index 00000000000..b76a6234d72
--- /dev/null
+++ b/tests/qemu-iotests/241.out
@@ -0,0 +1,26 @@
+QA output created by 241
+
+=== Exporting unaligned raw image, natural alignment ===
+
+  size:  1024
+  min block: 512
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
+
+=== Exporting unaligned raw image, forced server sector alignment ===
+
+WARNING: Image format was not specified for '/home/eblake/qemu/tests/qemu-iotests/scratch/t.raw' and probing guessed raw.
+         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
+         Specify the 'raw' format explicitly to remove the restrictions.
+  size:  1024
+  min block: 512
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
+
+=== Exporting unaligned raw image, forced client sector alignment ===
+
+  size:  1024
+  min block: 512
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 41da10c6cf5..bae77183809 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -240,6 +240,7 @@
 238 auto quick
 239 rw auto quick
 240 auto quick
+241 rw auto quick
 242 rw auto quick
 243 rw auto quick
 244 rw auto quick
-- 
2.20.1

Re: [Qemu-devel] [Qemu-block] [PULL 07/14] iotests: Add 241 to test NBD on unaligned images

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 7617 bytes --]

On 01.04.19 16:08, Eric Blake wrote:
> Add a test for the NBD client workaround in the previous patch.  It's
> not really feasible for an iotest to assume a specific tracing engine,
> so we can't really probe trace_nbd_parse_blockstatus_compliance to see
> if the server was fixed vs. whether the client just worked around the
> server (other than by rearranging order between code patches and this
> test). But having a successful exchange sure beats the previous state
> of an error message. Since format probing can change alignment, we can
> use that as an easy way to test several configurations.
> 
> Not tested yet, but worth adding to this test in future patches: an
> NBD server that can advertise a non-sector-aligned size (such as
> nbdkit) causes qemu as the NBD client to misbehave when it rounds the
> size up and accesses beyond the advertised size. Qemu as NBD server
> never advertises a non-sector-aligned size (since bdrv_getlength()
> currently rounds up to sector boundaries); until qemu can act as such
> a server, testing that flaw will have to rely on external binaries.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20190329042750.14704-2-eblake@redhat.com>
> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> [eblake: add forced-512 alignment, and nbdkit reproducer comment]
> ---
>  tests/qemu-iotests/241     | 100 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/241.out |  26 ++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 127 insertions(+)
>  create mode 100755 tests/qemu-iotests/241
>  create mode 100644 tests/qemu-iotests/241.out
> 
> diff --git a/tests/qemu-iotests/241 b/tests/qemu-iotests/241
> new file mode 100755
> index 00000000000..4b196857387
> --- /dev/null
> +++ b/tests/qemu-iotests/241
> @@ -0,0 +1,100 @@
> +#!/bin/bash
> +#
> +# Test qemu-nbd vs. unaligned images
> +#
> +# Copyright (C) 2018-2019 Red Hat, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +seq="$(basename $0)"
> +echo "QA output created by $seq"
> +
> +status=1 # failure is the default!
> +
> +nbd_unix_socket=$TEST_DIR/test_qemu_nbd_socket
> +
> +_cleanup()
> +{
> +    _cleanup_test_img
> +    nbd_server_stop
> +}
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +. ./common.nbd
> +
> +_supported_fmt raw
> +_supported_proto nbd
> +_supported_os Linux
> +_require_command QEMU_NBD
> +
> +# can't use _make_test_img, because qemu-img rounds image size up,
> +# and because we want to use Unix socket rather than TCP port. Likewise,
> +# we have to redirect TEST_IMG to our server.
> +# This tests that we can deal with the hole at the end of an unaligned
> +# raw file (either because the server doesn't advertise alignment too
> +# large, or because the client ignores the server's noncompliance - even
> +# though we can't actually wire iotests into checking trace messages).
> +printf %01000d 0 > "$TEST_IMG_FILE"
> +TEST_IMG="nbd:unix:$nbd_unix_socket"
> +
> +echo
> +echo "=== Exporting unaligned raw image, natural alignment ==="
> +echo
> +
> +nbd_server_start_unix_socket -f $IMGFMT "$TEST_IMG_FILE"
> +
> +$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
> +$QEMU_IMG map -f raw --output=json "$TEST_IMG" | _filter_qemu_img_map
> +$QEMU_IO -f raw -c map "$TEST_IMG"
> +nbd_server_stop
> +
> +echo
> +echo "=== Exporting unaligned raw image, forced server sector alignment ==="
> +echo
> +
> +# Intentionally omit '-f' to force image probing, which in turn forces
> +# sector alignment, here at the server.
> +nbd_server_start_unix_socket "$TEST_IMG_FILE"
> +
> +$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
> +$QEMU_IMG map -f raw --output=json "$TEST_IMG" | _filter_qemu_img_map
> +$QEMU_IO -f raw -c map "$TEST_IMG"
> +nbd_server_stop
> +
> +echo
> +echo "=== Exporting unaligned raw image, forced client sector alignment ==="
> +echo
> +
> +# Now force sector alignment at the client.
> +nbd_server_start_unix_socket -f $IMGFMT "$TEST_IMG_FILE"
> +
> +$QEMU_NBD_PROG --list -k $nbd_unix_socket | grep '\(size\|min\)'
> +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
> +$QEMU_IO -c map "$TEST_IMG"
> +nbd_server_stop
> +
> +# Not tested yet: we also want to ensure that qemu as NBD client does
> +# not access beyond the end of a server's advertised unaligned size:
> +#  nbdkit -U - memory size=513 --run 'qemu-io -f raw -c "r 512 512" $nbd'
> +# However, since qemu as server always rounds up to a sector alignment,
> +# we would have to use nbdkit to provoke the current client failures.
> +
> +# success, all done
> +echo '*** done'
> +rm -f $seq.full
> +status=0
> diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
> new file mode 100644
> index 00000000000..b76a6234d72
> --- /dev/null
> +++ b/tests/qemu-iotests/241.out
> @@ -0,0 +1,26 @@
> +QA output created by 241
> +
> +=== Exporting unaligned raw image, natural alignment ===
> +
> +  size:  1024
> +  min block: 512
> +[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> +1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
> +
> +=== Exporting unaligned raw image, forced server sector alignment ===
> +
> +WARNING: Image format was not specified for '/home/eblake/qemu/tests/qemu-iotests/scratch/t.raw' and probing guessed raw.
> +         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
> +         Specify the 'raw' format explicitly to remove the restrictions.

Is there a patch for this already?

Also, I planned to reply to the original patch – but where is it?  The
latest version I can see on the list is v3, and that only has seven
lines in the reference output.  Well, the difference would explain why
Vladimir would give a Tested-by to a test that clearly will not run on
his machine.

Max

> +  size:  1024
> +  min block: 512
> +[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> +1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
> +
> +=== Exporting unaligned raw image, forced client sector alignment ===
> +
> +  size:  1024
> +  min block: 512
> +[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> +1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
> +*** done
> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
> index 41da10c6cf5..bae77183809 100644
> --- a/tests/qemu-iotests/group
> +++ b/tests/qemu-iotests/group
> @@ -240,6 +240,7 @@
>  238 auto quick
>  239 rw auto quick
>  240 auto quick
> +241 rw auto quick
>  242 rw auto quick
>  243 rw auto quick
>  244 rw auto quick
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-block] [PULL 07/14] iotests: Add 241 to test NBD on unaligned images

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2528 bytes --]

On 4/10/19 12:45 PM, Max Reitz wrote:
> On 01.04.19 16:08, Eric Blake wrote:
>> Add a test for the NBD client workaround in the previous patch.  It's
>> not really feasible for an iotest to assume a specific tracing engine,
>> so we can't really probe trace_nbd_parse_blockstatus_compliance to see
>> if the server was fixed vs. whether the client just worked around the
>> server (other than by rearranging order between code patches and this
>> test). But having a successful exchange sure beats the previous state
>> of an error message. Since format probing can change alignment, we can
>> use that as an easy way to test several configurations.
>>
>> Not tested yet, but worth adding to this test in future patches: an
>> NBD server that can advertise a non-sector-aligned size (such as
>> nbdkit) causes qemu as the NBD client to misbehave when it rounds the
>> size up and accesses beyond the advertised size. Qemu as NBD server
>> never advertises a non-sector-aligned size (since bdrv_getlength()
>> currently rounds up to sector boundaries); until qemu can act as such
>> a server, testing that flaw will have to rely on external binaries.
>>

>> +++ b/tests/qemu-iotests/241.out

>> +=== Exporting unaligned raw image, forced server sector alignment ===
>> +
>> +WARNING: Image format was not specified for '/home/eblake/qemu/tests/qemu-iotests/scratch/t.raw' and probing guessed raw.
>> +         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
>> +         Specify the 'raw' format explicitly to remove the restrictions.
> 
> Is there a patch for this already?

Oh shoot. I saw the complaint about the hard-coded path needing a
filter, and still failed to fix it in time for 4.0-rc3. Not a driver for
-rc4 on its own, but if we have something else pop up that mandates
-rc4, I'll get that fix in.

Will post shortly...

> 
> Also, I planned to reply to the original patch – but where is it?  The
> latest version I can see on the list is v3, and that only has seven
> lines in the reference output.  Well, the difference would explain why
> Vladimir would give a Tested-by to a test that clearly will not run on
> his machine.

Yeah, this line in the commit message is evidence of my botching things:

> [eblake: add forced-512 alignment, and nbdkit reproducer comment]

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PULL 08/14] nbd/client: Lower min_block for block-status, unaligned size

[Eric Blake]

We have a latent bug in our NBD client code, tickled by the brand new
nbdkit 1.11.10 block status support:

$ nbdkit --filter=log --filter=truncate -U - \
           data data="1" size=511 truncate=64K logfile=/dev/stdout \
           --run 'qemu-img convert $nbd /var/tmp/out'
...
qemu-img: block/io.c:2122: bdrv_co_block_status: Assertion `*pnum && QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset' failed.

The culprit? Our implementation of .bdrv_co_block_status can return
unaligned block status for any server that operates with a lower
actual alignment than what we tell the block layer in
request_alignment, in violation of the block layer's constraints. To
date, we've been unable to trip the bug, because qemu as NBD server
always advertises block sizing (at which point it is a server bug if
the server sends unaligned status - although qemu 3.1 is such a server
and I've sent separate patches for 4.0 both to get the server to obey
the spec, and to let the client to tolerate server oddities at EOF).

But nbdkit does not (yet) advertise block sizing, and therefore is not
in violation of the spec for returning block status at whatever
boundaries it wants, and those unaligned results can occur anywhere
rather than just at EOF. While we are still wise to avoid sending
sub-sector read/write requests to a server of unknown origin, we MUST
consider that a server telling us block status without an advertised
block size is correct.  So, we either have to munge unaligned answers
from the server into aligned ones that we hand back to the block
layer, or we have to tell the block layer about a smaller alignment.

Similarly, if the server advertises an image size that is not
sector-aligned, we might as well assume that the server intends to let
us access those tail bytes, and therefore supports a minimum block
size of 1, regardless of whether the server supports block status
(although we still need more patches to fix the problem that with an
unaligned image, we can send read or block status requests that exceed
EOF to the server). Again, qemu as server cannot trip this problem
(because it rounds images to sector alignment), but nbdkit advertised
unaligned size even before it gained block status support.

Solve both alignment problems at once by using better heuristics on
what alignment to report to the block layer when the server did not
give us something to work with. Note that very few NBD servers
implement block status (to date, only qemu and nbdkit are known to do
so); and as the NBD spec mentioned block sizing constraints prior to
documenting block status, it can be assumed that any future
implementations of block status are aware that they must advertise
block size if they want a minimum size other than 1.

We've had a long history of struggles with picking the right alignment
to use in the block layer, as evidenced by the commit message of
fd8d372d (v2.12) that introduced the current choice of forced 512-byte
alignment.

There is no iotest coverage for this fix, because qemu can't provoke
it, and I didn't want to make test 241 dependent on nbdkit.

Fixes: fd8d372d
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329042750.14704-3-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
---
 block/nbd.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/block/nbd.c b/block/nbd.c
index 2e72df528ac..208be596027 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -437,7 +437,24 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
     uint32_t min = s->info.min_block;
     uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block);

-    bs->bl.request_alignment = min ? min : BDRV_SECTOR_SIZE;
+    /*
+     * If the server did not advertise an alignment:
+     * - a size that is not sector-aligned implies that an alignment
+     *   of 1 can be used to access those tail bytes
+     * - advertisement of block status requires an alignment of 1, so
+     *   that we don't violate block layer constraints that block
+     *   status is always aligned (as we can't control whether the
+     *   server will report sub-sector extents, such as a hole at EOF
+     *   on an unaligned POSIX file)
+     * - otherwise, assume the server is so old that we are safer avoiding
+     *   sub-sector requests
+     */
+    if (!min) {
+        min = (!QEMU_IS_ALIGNED(s->info.size, BDRV_SECTOR_SIZE) ||
+               s->info.base_allocation) ? 1 : BDRV_SECTOR_SIZE;
+    }
+
+    bs->bl.request_alignment = min;
     bs->bl.max_pdiscard = max;
     bs->bl.max_pwrite_zeroes = max;
     bs->bl.max_transfer = max;
-- 
2.20.1

[Qemu-devel] [PULL 09/14] nbd/client: Report offsets in bdrv_block_status

[Eric Blake]

It is desirable for 'qemu-img map' to have the same output for a file
whether it is served over file or nbd protocols. However, ever since
we implemented block status for NBD (2.12), the NBD protocol forgot to
inform the block layer that as the final layer in the chain, the
offset is valid; without an offset, the human-readable form of
qemu-img map gives up with the unhelpful:

$ nbdkit -U - data data="1" size=512 --run 'qemu-img map $nbd'
Offset          Length          Mapped to       File
qemu-img: File contains external, encrypted or compressed clusters.

The --output=json form always works, because it is reporting the
lower-level bdrv_block_status results directly rather than trying to
filter out sparse ranges for human consumption - but now it also
shows the offset member.

With this patch, the human output changes to:

Offset          Length          Mapped to       File
0               0x200           0               nbd+unix://?socket=/tmp/nbdkitOxeoLa/socket

This change is observable to several iotests.

Fixes: 78a33ab5
Reported-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329042750.14704-4-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c         |  9 +++++++--
 tests/qemu-iotests/209.out |  4 ++--
 tests/qemu-iotests/223.out | 18 +++++++++---------
 tests/qemu-iotests/241.out |  6 +++---
 4 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 150af9cc46f..3edb508f668 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -972,7 +972,9 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,

     if (!client->info.base_allocation) {
         *pnum = bytes;
-        return BDRV_BLOCK_DATA;
+        *map = offset;
+        *file = bs;
+        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
     }

     ret = nbd_co_send_request(bs, &request, NULL);
@@ -995,8 +997,11 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,

     assert(extent.length);
     *pnum = extent.length;
+    *map = offset;
+    *file = bs;
     return (extent.flags & NBD_STATE_HOLE ? 0 : BDRV_BLOCK_DATA) |
-           (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0);
+        (extent.flags & NBD_STATE_ZERO ? BDRV_BLOCK_ZERO : 0) |
+        BDRV_BLOCK_OFFSET_VALID;
 }

 void nbd_client_detach_aio_context(BlockDriverState *bs)
diff --git a/tests/qemu-iotests/209.out b/tests/qemu-iotests/209.out
index 0d29724e84a..214e27bfcec 100644
--- a/tests/qemu-iotests/209.out
+++ b/tests/qemu-iotests/209.out
@@ -1,2 +1,2 @@
-[{ "start": 0, "length": 524288, "depth": 0, "zero": false, "data": true},
-{ "start": 524288, "length": 524288, "depth": 0, "zero": true, "data": false}]
+[{ "start": 0, "length": 524288, "depth": 0, "zero": false, "data": true, "offset": 0},
+{ "start": 524288, "length": 524288, "depth": 0, "zero": true, "data": false, "offset": 524288}]
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 95c40a17ad7..7828a447392 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -67,18 +67,18 @@ read 1048576/1048576 bytes at offset 1048576
 1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 2097152/2097152 bytes at offset 2097152
 2 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true},
-{ "start": 4096, "length": 1044480, "depth": 0, "zero": true, "data": false},
-{ "start": 1048576, "length": 3145728, "depth": 0, "zero": false, "data": true}]
+[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 4096, "length": 1044480, "depth": 0, "zero": true, "data": false, "offset": OFFSET},
+{ "start": 1048576, "length": 3145728, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
 [{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false},
-{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true},
+{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]

 === Contrast to small granularity dirty-bitmap ===

-[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true},
+[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
-{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
+{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]

 === End qemu NBD server ===
@@ -94,10 +94,10 @@ read 2097152/2097152 bytes at offset 2097152
 === Use qemu-nbd as server ===

 [{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": false},
-{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true},
+{ "start": 65536, "length": 2031616, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
-[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true},
+[{ "start": 0, "length": 512, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 512, "length": 512, "depth": 0, "zero": false, "data": false},
-{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true},
+{ "start": 1024, "length": 2096128, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
 { "start": 2097152, "length": 2097152, "depth": 0, "zero": false, "data": false}]
 *** done
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
index b76a6234d72..f22eabbf324 100644
--- a/tests/qemu-iotests/241.out
+++ b/tests/qemu-iotests/241.out
@@ -4,7 +4,7 @@ QA output created by 241

   size:  1024
   min block: 512
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)

 === Exporting unaligned raw image, forced server sector alignment ===
@@ -14,13 +14,13 @@ WARNING: Image format was not specified for '/home/eblake/qemu/tests/qemu-iotest
          Specify the 'raw' format explicitly to remove the restrictions.
   size:  1024
   min block: 512
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)

 === Exporting unaligned raw image, forced client sector alignment ===

   size:  1024
   min block: 512
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
+[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
 *** done
-- 
2.20.1

[Qemu-devel] [PULL 10/14] nbd/client: Reject inaccessible tail of inconsistent server

[Eric Blake]

The NBD spec suggests that a server should never advertise a size
inconsistent with its minimum block alignment, as that tail is
effectively inaccessible to a compliant client obeying those block
constraints. Since we have a habit of rounding up rather than
truncating, to avoid losing the last few bytes of user input, and we
cannot access the tail when the server advertises bogus block sizing,
abort the connection to alert the server to fix their bug.  And
rejecting such servers matches what we already did for a min_block
that was not a power of 2 or which was larger than max_block.

Does not impact either qemu (which always sends properly aligned
sizes) or nbdkit (which does not send minimum block requirements yet);
so this is mostly aimed at new NBD server implementations, and ensures
that the rest of our code can assume the size is aligned.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190330155704.24191-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/nbd/client.c b/nbd/client.c
index de7da48246b..427980bdd22 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -426,6 +426,14 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
                 nbd_send_opt_abort(ioc);
                 return -1;
             }
+            if (info->min_block &&
+                !QEMU_IS_ALIGNED(info->size, info->min_block)) {
+                error_setg(errp, "export size %" PRIu64 "is not multiple of "
+                           "minimum block size %" PRIu32, info->size,
+                           info->min_block);
+                nbd_send_opt_abort(ioc);
+                return -1;
+            }
             trace_nbd_receive_negotiate_size_flags(info->size, info->flags);
             break;

-- 
2.20.1

Re: [Qemu-devel] [PULL 10/14] nbd/client: Reject inaccessible tail of inconsistent server

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]

On 4/1/19 9:08 AM, Eric Blake wrote:
> The NBD spec suggests that a server should never advertise a size
> inconsistent with its minimum block alignment, as that tail is
> effectively inaccessible to a compliant client obeying those block
> constraints. Since we have a habit of rounding up rather than
> truncating, to avoid losing the last few bytes of user input, and we
> cannot access the tail when the server advertises bogus block sizing,
> abort the connection to alert the server to fix their bug.  And
> rejecting such servers matches what we already did for a min_block
> that was not a power of 2 or which was larger than max_block.
> 
> Does not impact either qemu (which always sends properly aligned
> sizes) or nbdkit (which does not send minimum block requirements yet);
> so this is mostly aimed at new NBD server implementations, and ensures
> that the rest of our code can assume the size is aligned.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20190330155704.24191-1-eblake@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  nbd/client.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index de7da48246b..427980bdd22 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -426,6 +426,14 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
>                  nbd_send_opt_abort(ioc);
>                  return -1;
>              }
> +            if (info->min_block &&
> +                !QEMU_IS_ALIGNED(info->size, info->min_block)) {
> +                error_setg(errp, "export size %" PRIu64 "is not multiple of "

Missing a space in the error message. If I have any other patches worthy
of an -rc3 pull request, I'll do the obvious fix then.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[Qemu-devel] [PULL 11/14] nbd/client: Support qemu-img convert from unaligned size

[Eric Blake]

If an NBD server advertises a size that is not a multiple of a sector,
the block layer rounds up that size, even though we set info.size to
the exact byte value sent by the server. The block layer then proceeds
to let us read or query block status on the hole that it added past
EOF, which the NBD server is unlikely to be happy with. Fortunately,
qemu as a server never advertizes an unaligned size, so we generally
don't run into this problem; but the nbdkit server makes it easy to
test:

$ printf %1000d 1 > f1
$ ~/nbdkit/nbdkit -fv file f1 & pid=$!
$ qemu-img convert -f raw nbd://localhost:10809 f2
$ kill $pid
$ qemu-img compare f1 f2

Pre-patch, the server attempts a 1024-byte read, which nbdkit
rightfully rejects as going beyond its advertised 1000 byte size; the
conversion fails and the output files differ (not even the first
sector is copied, because qemu-img does not follow ddrescue's habit of
trying smaller reads to get as much information as possible in spite
of errors). Post-patch, the client's attempts to read (and query block
status, for new enough nbdkit) are properly truncated to the server's
length, with sane handling of the hole the block layer forced on
us. Although f2 ends up as a larger file (1024 bytes instead of 1000),
qemu-img compare shows the two images to have identical contents for
display to the guest.

I didn't add iotests coverage since I didn't want to add a dependency
on nbdkit in iotests. I also did NOT patch write, trim, or write
zeroes - these commands continue to fail (usually with ENOSPC, but
whatever the server chose), because we really can't write to the end
of the file, and because 'qemu-img convert' is the most common case
where we care about being tolerant (which is read-only). Perhaps we
could truncate the request if the client is writing zeros to the tail,
but that seems like more work, especially if the block layer is fixed
in 4.1 to track byte-accurate sizing (in which case this patch would
be reverted as unnecessary).

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329042750.14704-5-eblake@redhat.com>
Tested-by: Richard W.M. Jones <rjones@redhat.com>
---
 block/nbd-client.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 3edb508f668..409c2171bc3 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -848,6 +848,25 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
     if (!bytes) {
         return 0;
     }
+    /*
+     * Work around the fact that the block layer doesn't do
+     * byte-accurate sizing yet - if the read exceeds the server's
+     * advertised size because the block layer rounded size up, then
+     * truncate the request to the server and tail-pad with zero.
+     */
+    if (offset >= client->info.size) {
+        assert(bytes < BDRV_SECTOR_SIZE);
+        qemu_iovec_memset(qiov, 0, 0, bytes);
+        return 0;
+    }
+    if (offset + bytes > client->info.size) {
+        uint64_t slop = offset + bytes - client->info.size;
+
+        assert(slop < BDRV_SECTOR_SIZE);
+        qemu_iovec_memset(qiov, bytes - slop, 0, slop);
+        request.len -= slop;
+    }
+
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         return ret;
@@ -966,7 +985,8 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
         .from = offset,
         .len = MIN(MIN_NON_ZERO(QEMU_ALIGN_DOWN(INT_MAX,
                                                 bs->bl.request_alignment),
-                                client->info.max_block), bytes),
+                                client->info.max_block),
+                   MIN(bytes, client->info.size - offset)),
         .flags = NBD_CMD_FLAG_REQ_ONE,
     };

@@ -977,6 +997,23 @@ int coroutine_fn nbd_client_co_block_status(BlockDriverState *bs,
         return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
     }

+    /*
+     * Work around the fact that the block layer doesn't do
+     * byte-accurate sizing yet - if the status request exceeds the
+     * server's advertised size because the block layer rounded size
+     * up, we truncated the request to the server (above), or are
+     * called on just the hole.
+     */
+    if (offset >= client->info.size) {
+        *pnum = bytes;
+        assert(bytes < BDRV_SECTOR_SIZE);
+        /* Intentionally don't report offset_valid for the hole */
+        return BDRV_BLOCK_ZERO;
+    }
+
+    if (client->info.min_block) {
+        assert(QEMU_IS_ALIGNED(request.len, client->info.min_block));
+    }
     ret = nbd_co_send_request(bs, &request, NULL);
     if (ret < 0) {
         return ret;
-- 
2.20.1

[Qemu-devel] [PULL 12/14] block: Add bdrv_get_request_alignment()

[Eric Blake]

The next patch needs access to a device's minimum permitted
alignment, since NBD wants to advertise this to clients. Add
an accessor function, borrowing from blk_get_max_transfer()
for accessing a backend's block limits.

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20190329042750.14704-6-eblake@redhat.com>
---
 include/sysemu/block-backend.h | 1 +
 block/block-backend.c          | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index e2066eb06b2..3be05c2d689 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -177,6 +177,7 @@ bool blk_is_available(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
+uint32_t blk_get_request_alignment(BlockBackend *blk);
 uint32_t blk_get_max_transfer(BlockBackend *blk);
 int blk_get_max_iov(BlockBackend *blk);
 void blk_set_guest_block_size(BlockBackend *blk, int align);
diff --git a/block/block-backend.c b/block/block-backend.c
index edad02a0f2a..f78e82a707f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1764,6 +1764,13 @@ int blk_get_flags(BlockBackend *blk)
     }
 }

+/* Returns the minimum request alignment, in bytes; guaranteed nonzero */
+uint32_t blk_get_request_alignment(BlockBackend *blk)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    return bs ? bs->bl.request_alignment : BDRV_SECTOR_SIZE;
+}
+
 /* Returns the maximum transfer length, in bytes; guaranteed nonzero */
 uint32_t blk_get_max_transfer(BlockBackend *blk)
 {
-- 
2.20.1

[Qemu-devel] [PULL 13/14] nbd/server: Advertise actual minimum block size

[Eric Blake]

Both NBD_CMD_BLOCK_STATUS and structured NBD_CMD_READ will split their
reply according to bdrv_block_status() boundaries. If the block device
has a request_alignment smaller than 512, but we advertise a block
alignment of 512 to the client, then this can result in the server
reply violating client expectations by reporting a smaller region of
the export than what the client is permitted to address (although this
is less of an issue for qemu 4.0 clients, given recent client patches
to overlook our non-compliance at EOF).  Since it's always better to
be strict in what we send, it is worth advertising the actual minimum
block limit rather than blindly rounding it up to 512.

Note that this patch is not foolproof - it is still possible to
provoke non-compliant server behavior using:

$ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file

That is arguably a bug in the blkdebug driver (it should never pass
back block status smaller than its alignment, even if it has to make
multiple bdrv_get_status calls and determine the
least-common-denominator status among the group to return). It may
also be possible to observe issues with a backing layer with smaller
alignment than the active layer, although so far I have been unable to
write a reliable iotest for that scenario (but again, an issue like
that could be argued to be a bug in the block layer, or something
where we need a flag to bdrv_block_status() to state whether the
result must be aligned to the current layer's limits or can be
subdivided for accuracy when chasing backing files).

Anyways, as blkdebug is not normally used, and as this patch makes our
server more interoperable with qemu 3.1 clients, it is worth applying
now, even while we still work on a larger patch series for the 4.1
timeframe to have byte-accurate file lengths.

Note that the iotests output changes - for 223 and 233, we can see the
server's better granularity advertisement; and for 241, the three test
cases have the following effects:
- natural alignment: the server's smaller alignment is now advertised,
and the hole reported at EOF is now the right result; we've gotten rid
of the server's non-compliance
- forced server alignment: the server still advertises 512 bytes, but
still sends a mid-sector hole. This is still a server compliance bug,
which needs to be fixed in the block layer in a later patch; output
does not change because the client is already being tolerant of the
non-compliance
- forced client alignment: the server's smaller alignment means that
the client now sees the server's status change mid-sector without any
protocol violations, but the fact that the map shows an unaligned
mid-sector hole is evidence of the block layer problems with aligned
block status, to be fixed in a later patch

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190329042750.14704-7-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: rebase to enhanced iotest 241 coverage]
---
 nbd/server.c               | 13 ++++++++-----
 tests/qemu-iotests/223.out |  4 ++--
 tests/qemu-iotests/233.out |  2 +-
 tests/qemu-iotests/241.out | 10 ++++++----
 4 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index fd013a2817a..218a2aa5e65 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -607,13 +607,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
     /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size
      * according to whether the client requested it, and according to
      * whether this is OPT_INFO or OPT_GO. */
-    /* minimum - 1 for back-compat, or 512 if client is new enough.
-     * TODO: consult blk_bs(blk)->bl.request_alignment? */
-    sizes[0] =
-            (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
+    /* 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);
+    } else {
+        sizes[0] = 1;
+    }
+    assert(sizes[0] <= NBD_MAX_BUFFER_SIZE);
     /* preferred - Hard-code to 4096 for now.
      * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
-    sizes[1] = 4096;
+    sizes[1] = MAX(4096, sizes[0]);
     /* maximum - At most 32M, but smaller as appropriate. */
     sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE);
     trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]);
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 7828a447392..d5201b2356a 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -41,7 +41,7 @@ exports available: 2
  export: 'n'
   size:  4194304
   flags: 0x4ef ( readonly flush fua trim zeroes df cache )
-  min block: 512
+  min block: 1
   opt block: 4096
   max block: 33554432
   available meta contexts: 2
@@ -50,7 +50,7 @@ exports available: 2
  export: 'n2'
   size:  4194304
   flags: 0x4ed ( flush fua trim zeroes df cache )
-  min block: 512
+  min block: 1
   opt block: 4096
   max block: 33554432
   available meta contexts: 2
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 5acbc13b54a..9511b6ea658 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -38,7 +38,7 @@ exports available: 1
  export: ''
   size:  67108864
   flags: 0x4ed ( flush fua trim zeroes df cache )
-  min block: 512
+  min block: 1
   opt block: 4096
   max block: 33554432
   available meta contexts: 1
diff --git a/tests/qemu-iotests/241.out b/tests/qemu-iotests/241.out
index f22eabbf324..f481074a02e 100644
--- a/tests/qemu-iotests/241.out
+++ b/tests/qemu-iotests/241.out
@@ -3,8 +3,9 @@ QA output created by 241
 === Exporting unaligned raw image, natural alignment ===

   size:  1024
-  min block: 512
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+  min block: 1
+[{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, "offset": OFFSET}]
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)

 === Exporting unaligned raw image, forced server sector alignment ===
@@ -20,7 +21,8 @@ WARNING: Image format was not specified for '/home/eblake/qemu/tests/qemu-iotest
 === Exporting unaligned raw image, forced client sector alignment ===

   size:  1024
-  min block: 512
-[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true, "offset": OFFSET}]
+  min block: 1
+[{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true, "offset": OFFSET},
+{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true, "offset": OFFSET}]
 1 KiB (0x400) bytes     allocated at offset 0 bytes (0x0)
 *** done
-- 
2.20.1

[Qemu-devel] [PULL 14/14] nbd/client: Trace server noncompliance on structured reads

[Eric Blake]

Just as we recently added a trace for a server sending block status
that doesn't match the server's advertised minimum block alignment,
let's do the same for read chunks.  But since qemu 3.1 is such a
server (because it advertised 512-byte alignment, but when serving a
file that ends in data but is not sector-aligned, NBD_CMD_READ would
detect a mid-sector change between data and hole at EOF and the
resulting read chunks are unaligned), we don't want to change our
behavior of otherwise tolerating unaligned reads.

Note that even though we fixed the server for 4.0 to advertise an
actual block alignment (which gets rid of the unaligned reads at EOF
for posix files), we can still trigger it via other means:

$ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file

Arguably, that is a bug in the blkdebug block status function, for
leaking a block status that is not aligned. It may also be possible to
observe issues with a backing layer with smaller alignment than the
active layer, although so far I have been unable to write a reliable
iotest for that scenario.

Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190330165349.32256-1-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd-client.c | 12 ++++++++++--
 block/trace-events |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 409c2171bc3..790ecc1ee1c 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -211,7 +211,8 @@ static inline uint64_t payload_advance64(uint8_t **payload)
     return ldq_be_p(*payload - 8);
 }

-static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
+static int nbd_parse_offset_hole_payload(NBDClientSession *client,
+                                         NBDStructuredReplyChunk *chunk,
                                          uint8_t *payload, uint64_t orig_offset,
                                          QEMUIOVector *qiov, Error **errp)
 {
@@ -233,6 +234,10 @@ static int nbd_parse_offset_hole_payload(NBDStructuredReplyChunk *chunk,
                          " region");
         return -EINVAL;
     }
+    if (client->info.min_block &&
+        !QEMU_IS_ALIGNED(hole_size, client->info.min_block)) {
+        trace_nbd_structured_read_compliance("hole");
+    }

     qemu_iovec_memset(qiov, offset - orig_offset, 0, hole_size);

@@ -390,6 +395,9 @@ static int nbd_co_receive_offset_data_payload(NBDClientSession *s,
                          " region");
         return -EINVAL;
     }
+    if (s->info.min_block && !QEMU_IS_ALIGNED(data_size, s->info.min_block)) {
+        trace_nbd_structured_read_compliance("data");
+    }

     qemu_iovec_init(&sub_qiov, qiov->niov);
     qemu_iovec_concat(&sub_qiov, qiov, offset - orig_offset, data_size);
@@ -712,7 +720,7 @@ static int nbd_co_receive_cmdread_reply(NBDClientSession *s, uint64_t handle,
              * in qiov */
             break;
         case NBD_REPLY_TYPE_OFFSET_HOLE:
-            ret = nbd_parse_offset_hole_payload(&reply.structured, payload,
+            ret = nbd_parse_offset_hole_payload(s, &reply.structured, payload,
                                                 offset, qiov, &local_err);
             if (ret < 0) {
                 s->quit = true;
diff --git a/block/trace-events b/block/trace-events
index debb25c0ac8..7335a425404 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -158,6 +158,7 @@ iscsi_xcopy(void *src_lun, uint64_t src_off, void *dst_lun, uint64_t dst_off, ui

 # nbd-client.c
 nbd_parse_blockstatus_compliance(const char *err) "ignoring extra data from non-compliant server: %s"
+nbd_structured_read_compliance(const char *type) "server sent non-compliant unaligned read %s chunk"
 nbd_read_reply_entry_fail(int ret, const char *err) "ret = %d, err: %s"
 nbd_co_request_fail(uint64_t from, uint32_t len, uint64_t handle, uint16_t flags, uint16_t type, const char *name, int ret, const char *err) "Request failed { .from = %" PRIu64", .len = %" PRIu32 ", .handle = %" PRIu64 ", .flags = 0x%" PRIx16 ", .type = %" PRIu16 " (%s) } ret = %d, err: %s"

-- 
2.20.1

Re: [Qemu-devel] [PULL 00/14] NBD patches for 4.0-rc2

[Peter Maydell]

On Mon, 1 Apr 2019 at 21:14, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit 230ce19814ecc6bff8edac3b5b86e7c82f422c6c:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-axp-20190325' into staging (2019-03-29 19:29:00 +0000)
>
> are available in the Git repository at:
>
>   https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-04-01
>
> for you to fetch changes up to 75d34eb98ca0bb2f49d607fcaefd9c482e56b15d:
>
>   nbd/client: Trace server noncompliance on structured reads (2019-04-01 08:58:04 -0500)
>
> ----------------------------------------------------------------
> nbd patches for 2019-04-01
>
> - Better behavior of qemu-img map on NBD images
> - Fixes for NBD protocol alignment corner cases:
>  - the server has fewer places where it sends reads or block status
>    not aligned to its advertised block size
>  - the client has more cases where it can work around server
>    non-compliance present in qemu 3.1
>  - the client now avoids non-compliant requests when interoperating
>    with nbdkit or other servers not advertising block size

Applied, thanks.

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

-- PMM