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 <eblake@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);
+    } 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
-- 
2.20.1

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 <eblake@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 <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

On 3/28/19 3:46 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <eblake@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 ))

I can add an assert.

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks. I've got more patches coming, so I think I'll respin this series
into v3 with my other patches included. I'm still aiming to get this
into 4.2-rc2 though.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org