yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone

Blame SOURCES/kvm-nbd-client-Lower-min_block-for-block-status-unaligne.patch

b38b0f
From 569674a3b855f516a8bec22ca365fc7614639ce6 Mon Sep 17 00:00:00 2001
b38b0f
From: Max Reitz <mreitz@redhat.com>
b38b0f
Date: Tue, 23 Jul 2019 14:45:42 +0100
b38b0f
Subject: [PATCH 04/14] nbd/client: Lower min_block for block-status, unaligned
b38b0f
 size
b38b0f
b38b0f
RH-Author: Max Reitz <mreitz@redhat.com>
b38b0f
Message-id: <20190723144546.23701-4-mreitz@redhat.com>
b38b0f
Patchwork-id: 89650
b38b0f
O-Subject: [RHEL-8.1.0 qemu-kvm PATCH 3/7] nbd/client: Lower min_block for block-status, unaligned 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
We have a latent bug in our NBD client code, tickled by the brand new
b38b0f
nbdkit 1.11.10 block status support:
b38b0f
b38b0f
$ nbdkit --filter=log --filter=truncate -U - \
b38b0f
           data data="1" size=511 truncate=64K logfile=/dev/stdout \
b38b0f
           --run 'qemu-img convert $nbd /var/tmp/out'
b38b0f
...
b38b0f
qemu-img: block/io.c:2122: bdrv_co_block_status: Assertion `*pnum && QEMU_IS_ALIGNED(*pnum, align) && align > offset - aligned_offset' failed.
b38b0f
b38b0f
The culprit? Our implementation of .bdrv_co_block_status can return
b38b0f
unaligned block status for any server that operates with a lower
b38b0f
actual alignment than what we tell the block layer in
b38b0f
request_alignment, in violation of the block layer's constraints. To
b38b0f
date, we've been unable to trip the bug, because qemu as NBD server
b38b0f
always advertises block sizing (at which point it is a server bug if
b38b0f
the server sends unaligned status - although qemu 3.1 is such a server
b38b0f
and I've sent separate patches for 4.0 both to get the server to obey
b38b0f
the spec, and to let the client to tolerate server oddities at EOF).
b38b0f
b38b0f
But nbdkit does not (yet) advertise block sizing, and therefore is not
b38b0f
in violation of the spec for returning block status at whatever
b38b0f
boundaries it wants, and those unaligned results can occur anywhere
b38b0f
rather than just at EOF. While we are still wise to avoid sending
b38b0f
sub-sector read/write requests to a server of unknown origin, we MUST
b38b0f
consider that a server telling us block status without an advertised
b38b0f
block size is correct.  So, we either have to munge unaligned answers
b38b0f
from the server into aligned ones that we hand back to the block
b38b0f
layer, or we have to tell the block layer about a smaller alignment.
b38b0f
b38b0f
Similarly, if the server advertises an image size that is not
b38b0f
sector-aligned, we might as well assume that the server intends to let
b38b0f
us access those tail bytes, and therefore supports a minimum block
b38b0f
size of 1, regardless of whether the server supports block status
b38b0f
(although we still need more patches to fix the problem that with an
b38b0f
unaligned image, we can send read or block status requests that exceed
b38b0f
EOF to the server). Again, qemu as server cannot trip this problem
b38b0f
(because it rounds images to sector alignment), but nbdkit advertised
b38b0f
unaligned size even before it gained block status support.
b38b0f
b38b0f
Solve both alignment problems at once by using better heuristics on
b38b0f
what alignment to report to the block layer when the server did not
b38b0f
give us something to work with. Note that very few NBD servers
b38b0f
implement block status (to date, only qemu and nbdkit are known to do
b38b0f
so); and as the NBD spec mentioned block sizing constraints prior to
b38b0f
documenting block status, it can be assumed that any future
b38b0f
implementations of block status are aware that they must advertise
b38b0f
block size if they want a minimum size other than 1.
b38b0f
b38b0f
We've had a long history of struggles with picking the right alignment
b38b0f
to use in the block layer, as evidenced by the commit message of
b38b0f
fd8d372d (v2.12) that introduced the current choice of forced 512-byte
b38b0f
alignment.
b38b0f
b38b0f
There is no iotest coverage for this fix, because qemu can't provoke
b38b0f
it, and I didn't want to make test 241 dependent on nbdkit.
b38b0f
b38b0f
Fixes: fd8d372d
b38b0f
Reported-by: Richard W.M. Jones <rjones@redhat.com>
b38b0f
Signed-off-by: Eric Blake <eblake@redhat.com>
b38b0f
Message-Id: <20190329042750.14704-3-eblake@redhat.com>
b38b0f
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
b38b0f
Tested-by: Richard W.M. Jones <rjones@redhat.com>
b38b0f
(cherry picked from commit 7da537f70d929800ba9c657b8a47a7b827695ccc)
b38b0f
Signed-off-by: Max Reitz <mreitz@redhat.com>
b38b0f
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
b38b0f
---
b38b0f
 block/nbd.c | 19 ++++++++++++++++++-
b38b0f
 1 file changed, 18 insertions(+), 1 deletion(-)
b38b0f
b38b0f
diff --git a/block/nbd.c b/block/nbd.c
b38b0f
index f29c10f..3d642cd 100644
b38b0f
--- a/block/nbd.c
b38b0f
+++ b/block/nbd.c
b38b0f
@@ -473,7 +473,24 @@ static void nbd_refresh_limits(BlockDriverState *bs, Error **errp)
b38b0f
     uint32_t min = s->info.min_block;
b38b0f
     uint32_t max = MIN_NON_ZERO(NBD_MAX_BUFFER_SIZE, s->info.max_block);
b38b0f
 
b38b0f
-    bs->bl.request_alignment = min ? min : BDRV_SECTOR_SIZE;
b38b0f
+    /*
b38b0f
+     * If the server did not advertise an alignment:
b38b0f
+     * - a size that is not sector-aligned implies that an alignment
b38b0f
+     *   of 1 can be used to access those tail bytes
b38b0f
+     * - advertisement of block status requires an alignment of 1, so
b38b0f
+     *   that we don't violate block layer constraints that block
b38b0f
+     *   status is always aligned (as we can't control whether the
b38b0f
+     *   server will report sub-sector extents, such as a hole at EOF
b38b0f
+     *   on an unaligned POSIX file)
b38b0f
+     * - otherwise, assume the server is so old that we are safer avoiding
b38b0f
+     *   sub-sector requests
b38b0f
+     */
b38b0f
+    if (!min) {
b38b0f
+        min = (!QEMU_IS_ALIGNED(s->info.size, BDRV_SECTOR_SIZE) ||
b38b0f
+               s->info.base_allocation) ? 1 : BDRV_SECTOR_SIZE;
b38b0f
+    }
b38b0f
+
b38b0f
+    bs->bl.request_alignment = min;
b38b0f
     bs->bl.max_pdiscard = max;
b38b0f
     bs->bl.max_pwrite_zeroes = max;
b38b0f
     bs->bl.max_transfer = max;
b38b0f
-- 
b38b0f
1.8.3.1
b38b0f