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

[Eric Blake]

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 <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 ))

I can add an assert.

> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

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