Re: [Qemu-block] [PATCH v2 7/9] blkdebug: Add pass-through write_zero and discard support

[Eric Blake]

On 11/17/2016 04:47 PM, Max Reitz wrote:
> On 17.11.2016 21:14, Eric Blake wrote:
>> In order to test the effects of artificial geometry constraints
>> on operations like write zero or discard, we first need blkdebug
>> to manage these actions.  Ideally, it would be nice to let these
>> operations also react to injected errors like read/write/flush,
>> but it is not trivial to turn bdrv_aio error injection (where
>> we return BlockAIOCB*) into bdrv_co (where we return int), not
>> to mention the fact that I don't want to conflict with Kevin's
>> concurrent work on refactoring away from bdrv_aio.  So for now,
>> the operations merely have a TODO comment for adding error
>> injection.
>>
>> However, one thing we CAN test is the contract promised by the
>> block layer; namely, if a device has specified limits on
>> alignment or maximum size, then those limits must be obeyed (for
>> now, the blkdebug driver merely inherits limits from whatever it
>> is wrapping, but the next patch will further enhance it to allow
>> specific limit overrides).
>>
>> Tested by setting up an NBD server with export 'foo', then invoking:
>> $ ./qemu-io
>> qemu-io> open -o driver=blkdebug blkdebug::nbd://localhost:10809/foo
>> qemu-io> d 0 15M
>> qemu-io> w -z 0 15M
>>
>> Pre-patch, the server never sees the discard (it was silently
>> eaten by the block layer); post-patch it is passed across the
>> wire.  Likewise, pre-patch the write is always passed with
>> NBD_WRITE (with 15M of zeroes on the wire), while post-patch
>> it can utilize NBD_WRITE_ZEROES (for less traffic).
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>  block/blkdebug.c | 61 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 61 insertions(+)
>>
>> diff --git a/block/blkdebug.c b/block/blkdebug.c
>> index 0a47977..d45826d 100644
>> --- a/block/blkdebug.c
>> +++ b/block/blkdebug.c
> 
> [...]
> 
>> @@ -522,6 +528,59 @@ static BlockAIOCB *blkdebug_aio_flush(BlockDriverState 
>> *bs,
>>  }
>>
>>
>> +static int coroutine_fn blkdebug_co_pwrite_zeroes(BlockDriverState *bs,
>> +                                                  int64_t offset, int count,
>> +                                                  BdrvRequestFlags flags)
>> +{
>> +    uint32_t align = MAX(bs->bl.request_alignment,
>> +                         bs->bl.pwrite_zeroes_alignment);
>> +
>> +    /* Regardless of whether the lower layer has a finer granularity,
>> +     * we want to treat any unaligned request as unsupported, and
> 
> Why?

Hmm, at the moment, I'm having a hard time coming up with a strong
reason why I did that. I'll retest without it, and see if it still picks
up the regression fixed by 3/5; if so I'll drop it as part of the respin
(since I still have the iotest to fix); if not, I'll have a good reason
why and include it in the commit message.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature