From 569674a3b855f516a8bec22ca365fc7614639ce6 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 23 Jul 2019 14:45:42 +0100 Subject: [PATCH 04/14] nbd/client: Lower min_block for block-status, unaligned size RH-Author: Max Reitz Message-id: <20190723144546.23701-4-mreitz@redhat.com> Patchwork-id: 89650 O-Subject: [RHEL-8.1.0 qemu-kvm PATCH 3/7] nbd/client: Lower min_block for block-status, unaligned size Bugzilla: 1678979 RH-Acked-by: Kevin Wolf RH-Acked-by: Stefano Garzarella RH-Acked-by: John Snow From: 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 Signed-off-by: Eric Blake Message-Id: <20190329042750.14704-3-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy Tested-by: Richard W.M. Jones (cherry picked from commit 7da537f70d929800ba9c657b8a47a7b827695ccc) Signed-off-by: Max Reitz Signed-off-by: Danilo C. L. de Paula --- block/nbd.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/block/nbd.c b/block/nbd.c index f29c10f..3d642cd 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -473,7 +473,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; -- 1.8.3.1