Re: [Qemu-devel] [PATCH v2 21/21] iotests: Add test for different refcount widths

[Max Reitz <mreitz@redhat.com>]

On 2014-11-17 at 11:38, Max Reitz wrote:
> On 2014-11-17 at 09:34, Max Reitz wrote:
>> On 2014-11-15 at 15:50, Eric Blake wrote:
>>> On 11/14/2014 06:06 AM, Max Reitz wrote:
>>>> Add a test for conversion between different refcount widths and errors
>>>> specific to certain widths (i.e. snapshots with refcount_width=1).
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>   tests/qemu-iotests/112     | 252 
>>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>>   tests/qemu-iotests/112.out | 131 +++++++++++++++++++++++
>>>>   tests/qemu-iotests/group   |   1 +
>>>>   3 files changed, 384 insertions(+)
>>>>   create mode 100755 tests/qemu-iotests/112
>>>>   create mode 100644 tests/qemu-iotests/112.out
>>>>
>>>> +echo
>>>> +echo '=== Testing too many references for check ==='
>>>> +echo
>>>> +
>>>> +IMGOPTS="$IMGOPTS,refcount_width=1" _make_test_img 64M
>>>> +print_refcount_width
>>>> +
>>>> +# This cluster should be created at 0x50000
>>>> +$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
>>>> +# Now make the second L2 entry (the L2 table should be at 0x40000) 
>>>> point to that
>>>> +# cluster, so we have two references
>>>> +poke_file "$TEST_IMG" $((0x40008)) "\x80\x00\x00\x00\x00\x05\x00\x00"
>>>> +
>>>> +# This should say "please use amend"
>>>> +_check_test_img -r all
>>>> +
>>>> +# So we do that
>>>> +$QEMU_IMG amend -o refcount_width=2 "$TEST_IMG"
>>>> +print_refcount_width
>>>> +
>>>> +# And try again
>>>> +_check_test_img -r all
>>> I think this section also deserves a test that fuzzes an image with
>>> width=64 to intentionally set the most significant bit of one of the
>>> refcounts, and make sure that we gracefully diagnose it as invalid.
>>>
>>>> +
>>>> +echo
>>>> +echo '=== Multiple walks necessary during amend ==='
>>>> +echo
>>>> +
>>>> +IMGOPTS="$IMGOPTS,refcount_width=1,cluster_size=512" 
>>>> _make_test_img 64k
>>>> +
>>>> +# Cluster 0 is the image header, clusters 1 to 4 are used by the 
>>>> L1 table, a
>>>> +# single L2 table, the reftable and a single refblock. This 
>>>> creates 58 data
>>>> +# clusters (actually, the L2 table is created here, too), so in 
>>>> total there are
>>>> +# then 63 used clusters in the image. With a refcount width of 64, 
>>>> one refblock
>>>> +# describes 64 clusters (512 bytes / 64 bits/entry = 64 entries), 
>>>> so this will
>>>> +# make the first target refblock have exactly one free entry.
>>>> +$QEMU_IO -c "write 0 $((58 * 512))" "$TEST_IMG" | _filter_qemu_io
>>>> +
>>>> +# Now change the refcount width; since the first target refblock 
>>>> has exactly one
>>>> +# free entry, that entry will be used to store its own reference. 
>>>> No other
>>>> +# refblocks are needed, so then the new reftable will be 
>>>> allocated; since the
>>>> +# first target refblock is completely filled up, this will require 
>>>> a new
>>>> +# refblock which is why the refcount width changing function will 
>>>> need to run
>>>> +# through everything one more time until the allocations are stable.
>>>> +$QEMU_IMG amend -o refcount_width=64 "$TEST_IMG"
>>>> +print_refcount_width
>>> Umm, that sounds backwards from what you document.  It's a good test of
>>> the _new_ reftable needing a second round of allocations.  So keep it
>>> with corrected comments.  But I think you _intended_ to write a test
>>> that starts with a refcount_width=64 image and resize to a
>>> refcount_width=1, where the _old_ reftable then suffers a reallocation
>>> as part of allocating refblocks for the new table.
>>
>> That's what you intended, but that's harder to test, so I settled for 
>> this (and the comments are appropriate (note that "target refblock" 
>> refers to the refblocks after amendment); note that this does indeed 
>> fail with v1 of this series.
>>
>>> It may even help if
>>> you add a tracepoint for every iteration through the walk function
>>> callback, to prove we are indeed executing it 3 times instead of the
>>> usual 2, for these test cases.
>>
>> That's a good idea! I thought about adding some info, but totally 
>> forgot about trace points.
>
> ...On second thought, trace doesn't work so well with qemu-img. My 
> best bet would be blkdebug, but that seems kind of ugly to me...

Problem "solved": If there will be more walks than originally thought 
(3+1 instead of 2+1), progress will regress at one point. I'll just grep 
for that point and that should be enough (progress jumping from 66.67 % 
to 50.00 %).

Max