Re: [Qemu-devel] [PATCH v2 3/3] nbd/server: Advertise actual minimum block size

[Vladimir Sementsov-Ogievskiy]

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 <address@hidden>
> ---
>   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 <address@hidden>

-- 
Best regards,
Vladimir