yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-nbd-server-Advertise-actual-minimum-block-size.patch

b38b0f
From 1832a90928232cb91a8542613b754079fd1f0f0e Mon Sep 17 00:00:00 2001
b38b0f
From: Max Reitz <mreitz@redhat.com>
b38b0f
Date: Tue, 23 Jul 2019 14:45:46 +0100
b38b0f
Subject: [PATCH 08/14] nbd/server: Advertise actual minimum block size
b38b0f
b38b0f
RH-Author: Max Reitz <mreitz@redhat.com>
b38b0f
Message-id: <20190723144546.23701-8-mreitz@redhat.com>
b38b0f
Patchwork-id: 89652
b38b0f
O-Subject: [RHEL-8.1.0 qemu-kvm PATCH 7/7] nbd/server: Advertise actual minimum block size
b38b0f
Bugzilla: 1678979
b38b0f
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
b38b0f
RH-Acked-by: Stefano Garzarella <sgarzare@redhat.com>
b38b0f
RH-Acked-by: John Snow <jsnow@redhat.com>
b38b0f
b38b0f
From: Eric Blake <eblake@redhat.com>
b38b0f
b38b0f
Both NBD_CMD_BLOCK_STATUS and structured NBD_CMD_READ will split their
b38b0f
reply according to bdrv_block_status() boundaries. If the block device
b38b0f
has a request_alignment smaller than 512, but we advertise a block
b38b0f
alignment of 512 to the client, then this can result in the server
b38b0f
reply violating client expectations by reporting a smaller region of
b38b0f
the export than what the client is permitted to address (although this
b38b0f
is less of an issue for qemu 4.0 clients, given recent client patches
b38b0f
to overlook our non-compliance at EOF).  Since it's always better to
b38b0f
be strict in what we send, it is worth advertising the actual minimum
b38b0f
block limit rather than blindly rounding it up to 512.
b38b0f
b38b0f
Note that this patch is not foolproof - it is still possible to
b38b0f
provoke non-compliant server behavior using:
b38b0f
b38b0f
$ qemu-nbd --image-opts driver=blkdebug,align=512,image.driver=file,image.filename=/path/to/non-aligned-file
b38b0f
b38b0f
That is arguably a bug in the blkdebug driver (it should never pass
b38b0f
back block status smaller than its alignment, even if it has to make
b38b0f
multiple bdrv_get_status calls and determine the
b38b0f
least-common-denominator status among the group to return). It may
b38b0f
also be possible to observe issues with a backing layer with smaller
b38b0f
alignment than the active layer, although so far I have been unable to
b38b0f
write a reliable iotest for that scenario (but again, an issue like
b38b0f
that could be argued to be a bug in the block layer, or something
b38b0f
where we need a flag to bdrv_block_status() to state whether the
b38b0f
result must be aligned to the current layer's limits or can be
b38b0f
subdivided for accuracy when chasing backing files).
b38b0f
b38b0f
Anyways, as blkdebug is not normally used, and as this patch makes our
b38b0f
server more interoperable with qemu 3.1 clients, it is worth applying
b38b0f
now, even while we still work on a larger patch series for the 4.1
b38b0f
timeframe to have byte-accurate file lengths.
b38b0f
b38b0f
Note that the iotests output changes - for 223 and 233, we can see the
b38b0f
server's better granularity advertisement; and for 241, the three test
b38b0f
cases have the following effects:
b38b0f
- natural alignment: the server's smaller alignment is now advertised,
b38b0f
and the hole reported at EOF is now the right result; we've gotten rid
b38b0f
of the server's non-compliance
b38b0f
- forced server alignment: the server still advertises 512 bytes, but
b38b0f
still sends a mid-sector hole. This is still a server compliance bug,
b38b0f
which needs to be fixed in the block layer in a later patch; output
b38b0f
does not change because the client is already being tolerant of the
b38b0f
non-compliance
b38b0f
- forced client alignment: the server's smaller alignment means that
b38b0f
the client now sees the server's status change mid-sector without any
b38b0f
protocol violations, but the fact that the map shows an unaligned
b38b0f
mid-sector hole is evidence of the block layer problems with aligned
b38b0f
block status, to be fixed in a later patch
b38b0f
b38b0f
Signed-off-by: Eric Blake <eblake@redhat.com>
b38b0f
Message-Id: <20190329042750.14704-7-eblake@redhat.com>
b38b0f
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
b38b0f
[eblake: rebase to enhanced iotest 241 coverage]
b38b0f
(cherry picked from commit b0245d6478ea5906e3d7a542244d5c015fd47bc7)
b38b0f
b38b0f
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
b38b0f
b38b0f
Conflicts:
b38b0f
- tests/qemu-iotests/223.out: We are missing
b38b0f
  ddd09448fd833d646952c769ae9ce3d39bee989f downstream, which adds
b38b0f
  qemu-nbd --list tests to 223.  (qemu-nbd --list does not exist
b38b0f
  downstream.)
b38b0f
b38b0f
- tests/qemu-iotests/233.out: Does not exist downstream.
b38b0f
b38b0f
- tests/qemu-iotests/241.out: Does not exist downstream, because it
b38b0f
  would require qemu-nbd --list.
b38b0f
b38b0f
Signed-off-by: Max Reitz <mreitz@redhat.com>
b38b0f
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
b38b0f
---
b38b0f
 nbd/server.c | 13 ++++++++-----
b38b0f
 1 file changed, 8 insertions(+), 5 deletions(-)
b38b0f
b38b0f
diff --git a/nbd/server.c b/nbd/server.c
b38b0f
index e094300..96b6631 100644
b38b0f
--- a/nbd/server.c
b38b0f
+++ b/nbd/server.c
b38b0f
@@ -608,13 +608,16 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
b38b0f
     /* Send NBD_INFO_BLOCK_SIZE always, but tweak the minimum size
b38b0f
      * according to whether the client requested it, and according to
b38b0f
      * whether this is OPT_INFO or OPT_GO. */
b38b0f
-    /* minimum - 1 for back-compat, or 512 if client is new enough.
b38b0f
-     * TODO: consult blk_bs(blk)->bl.request_alignment? */
b38b0f
-    sizes[0] =
b38b0f
-            (client->opt == NBD_OPT_INFO || blocksize) ? BDRV_SECTOR_SIZE : 1;
b38b0f
+    /* minimum - 1 for back-compat, or actual if client will obey it. */
b38b0f
+    if (client->opt == NBD_OPT_INFO || blocksize) {
b38b0f
+        sizes[0] = blk_get_request_alignment(exp->blk);
b38b0f
+    } else {
b38b0f
+        sizes[0] = 1;
b38b0f
+    }
b38b0f
+    assert(sizes[0] <= NBD_MAX_BUFFER_SIZE);
b38b0f
     /* preferred - Hard-code to 4096 for now.
b38b0f
      * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
b38b0f
-    sizes[1] = 4096;
b38b0f
+    sizes[1] = MAX(4096, sizes[0]);
b38b0f
     /* maximum - At most 32M, but smaller as appropriate. */
b38b0f
     sizes[2] = MIN(blk_get_max_transfer(exp->blk), NBD_MAX_BUFFER_SIZE);
b38b0f
     trace_nbd_negotiate_handle_info_block_size(sizes[0], sizes[1], sizes[2]);
b38b0f
-- 
b38b0f
1.8.3.1
b38b0f