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

[Max Reitz]

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. It's a good test and you should keep it but
you should probably change the comment about the iSCSI device because as
far as I can see, this has nothing to do with it.

In said iSCSI case, the block driver limiting the alignment wouldn't be
the format block driver or blkdebug, but the protocol driver (i.e.
iscsi). As I said in my reply to patch 5, though, I don't think that
your series changes behavior in that case.

Old behavior: The block layer cuts off head and tail from the discard
request before sending it to the iscsi driver. That driver then asserts
that the request is aligned and proceeds.

New behavior: The block layer sends head and tail separately from the
rest. The iscsi driver discards head and tail, though, because they are
not aligned, and thus again only sends the aligned part of the request
to the device.

> Maybe I should tweak the number to not be a multiple of 512, to doubly
> make sure that the double-alignment code in io.c 5/9 is doing the right
> job (I didn't even check that 80000000 is indeed a multiple of 512).

I was a bit surprised myself. :-)

> You're right that it doesn't tickle iscsi code, so much as tickling the
> block layer code that was previously throwing away the unaligned
> portions rather than passing them on through anyways.  The test was
> inspired because of the iscsi regression, but I was able to rework it
> into something that reproducibly failed even without iscsi in the mix,
> until the block layer was fixed.  But if that means cleaning up the
> comment on respin, I'm fine with that.

Yes, that would be very much fine with me.

Max

signature.asc
Description: OpenPGP digital signature