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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

02.08.2018 17:48, 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.  Thus, it is imperative that we
> advertise the actual minimum block limit, rather than blindly
> rounding it up to 512 (bdrv_block_status() cannot return status
> aligned any smaller than request_alignment).
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>   nbd/server.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/nbd/server.c b/nbd/server.c
> index ea5fe0eb336..cd3c41f895b 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -608,10 +608,12 @@ 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);
> +    } else {
> +        sizes[0] = 1;
> +    }
>       /* preferred - Hard-code to 4096 for now.
>        * TODO: is blk_bs(blk)->bl.opt_transfer appropriate? */
>       sizes[1] = 4096;

Here, what about dirty bitmap export through BLOCK_STATUS? Could it's 
granularity be less than request_alignment? Shouldn't we handle it somehow?

-- 
Best regards,
Vladimir