28.03.2019 1:39, Eric Blake wrote:
> 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). 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
>
> But 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 for the 4.1 timeframe
> to improve the block layer to prevent the mid-block status changes
> that can be observed now with blkdebug or with a backing layer with
> smaller alignment than the active layer.
>
> Note that the iotests output changes - both pre- and post-patch, the
> server is reporting a mid-sector hole; but pre-patch, the client was
> then rounding that up to a sector boundary as a workaround, while
> post-patch the client doesn't have to round because it sees the
> server's smaller advertised block size.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> nbd/server.c | 12 +++++++-----
> tests/qemu-iotests/241.out | 3 ++-
> 2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index fd013a2817a..c76f32dbb50 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -607,13 +607,15 @@ 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);
hope it will never exceed NBD_MAX_BUFFER_SIZE ))
> + } else {
> + sizes[0] = 1;
> + }
> /* 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/241.out b/tests/qemu-iotests/241.out
> index 044afc0c6f8..a7f1e665a9a 100644
> --- a/tests/qemu-iotests/241.out
> +++ b/tests/qemu-iotests/241.out
> @@ -2,6 +2,7 @@ QA output created by 241
>
> === Exporting unaligned raw image ===
>
> -[{ "start": 0, "length": 1024, "depth": 0, "zero": false, "data": true}]
> +[{ "start": 0, "length": 1000, "depth": 0, "zero": false, "data": true},
> +{ "start": 1000, "length": 24, "depth": 0, "zero": true, "data": true}]
> 1 KiB (0x400) bytes allocated at offset 0 bytes (0x0)
> *** done
>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
