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

[Eric Blake]

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

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

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.

> 
> What does that have to do with said iscsi device, though?

Only that the unusual choice of 15M alignment and maximum discard sizing
was chosen to match that particular iscsi device, because that was the
device where the regression was first reported.

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

signature.asc
Description: OpenPGP digital signature