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 <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 ))I can add an assert. > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@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
signature.asc
Description: OpenPGP digital signature
