Re: [Qemu-devel] [PATCH v2 9/9] tests: Add coverage for recent block geometry fixes

[Max Reitz]

On 19.11.2016 22:45, Max Reitz wrote:
> On 18.11.2016 02:28, Eric Blake wrote:
>> On 11/17/2016 05:42 PM, Max Reitz wrote:
>>> On 17.11.2016 21:14, Eric Blake wrote:
>>>> Use blkdebug's new geometry constraints to emulate setups that
>>>> have caused recent regression fixes: write zeroes asserting
>>>> when running through a loopback block device with max-transfer
>>>> smaller than cluster size, and discard rounding away requests
>>>> that were not aligned to power-of-two boundaries.  Also, add
>>>> coverage that the block layer is honoring max transfer limits.
>>>>
>>>> For now, a single iotest performs all actions, with the idea
>>>> that we can add future blkdebug constraint test cases in the
>>>> same file; but it can be split into multiple iotests if we find
>>>> reason to run one portion of the test in more setups than what
>>>> are possible in the other.
>>>>
>>
>>>
>>>> +# Dell Equallogic iSCSI device is unusual with its 15M page size
>>>> +echo
>>>> +echo "== non-power-of-2 write zeroes =="
>>>> +
>>>> +limits=opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
>>>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
>>>> +         -c "write -z 32M 32M" | _filter_qemu_io
>>>> +
>>>> +echo
>>>> +echo "== non-power-of-2 discard =="
>>>> +
>>>> +limits=opt-write-zero=15M,max-write-zero=15M,opt-discard=15M,max-discard=15M
>>>> +$QEMU_IO -c "open -o $options,$limits blkdebug::$TEST_IMG" \
>>>> +         -c "discard 80000000 30M" | _filter_qemu_io
>>>
>>> Question: What does this test has to do with iscsi?
>>>
>>> The first case just tests that we fall back to writing the head and tail
>>> as full zeroes when the driver returns -ENOTSUP.
>>
>> The first one isn't all that interesting, so much as making sure we
>> don't regress. I couldn't make it fail, pre-patch.  The real test is the
>> second one...
>>
>>>
>>> The second test, as far as I can see, just gives some discard request to
>>> blkdebug (split up into head, mid and tail), which blkdebug just passes
>>> on (because 80000000 is a multiple of 512). qcow2 then discards part of
>>> that and drops the head and tail of the request it receives (but head
>>> and tail are now calculated based on qcow2's 64k limit).
>>
>> Thanks to the opt-discard=15M, the blkdebug layer is forcing the block
>> layer to break it into head/middle/tail on 15M boundaries, but throwing
>> away the head and tail without giving blkdebug a chance, so it only
>> zeroed 90-105M.  Then, with the block layer fixed to pass the head on
>> through anyways, but without patch 2/9, the qcow2 code was seeing that
>> the start offset was not cluster-aligned ($TEST_IMG has 1M clusters),
>> and with patch 4/9 that was making qcow2 return -ENOTSUP, and still
>> ignoring everything up to 90M.  It took all of 2, 4, and 5 before the
>> discard finally affected the range 77M-90M (since 80000000 is just
>> before 77M).
> 
> OK, thank you for the explanation, but again, I don't know how that is
> related to the iscsi case.

And now with my last response to patch 5, I do know. So because the
"align" option isn't set, the request_alignment defaults to 512.
blkdebug will accept requests and pass them on even if they're not
aligned to pdiscard_alignment, so that's similar to how iscsi only drops
discard requests that are not aligned to request_alignment but passes
everything on regardless of whether it's aligned to pdiscard_alignment.

I'd be fine with either adjusting the "Dell..." comment to be something
entirely iscsi-unrelated, or with a description of the complete picture
here.

Max

signature.asc
Description: OpenPGP digital signature