Re: [PATCH 7/7] iotests/271: check disk usage on subcluster-based discard/unmap

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

On 11/3/23 17:59, Hanna Czenczek wrote:
> On 03.11.23 16:51, Hanna Czenczek wrote:
>> On 20.10.23 23:56, Andrey Drobyshev wrote: 
> 
> [...]
> 
>>> @@ -528,6 +543,14 @@ for use_backing_file in yes no; do
>>>       else
>>>           _make_test_img -o extended_l2=on 1M
>>>       fi
>>> +    # Write cluster #0 and discard its subclusters #0-#3
>>> +    $QEMU_IO -c 'write -q 0 64k' "$TEST_IMG"
>>> +    before=$(disk_usage "$TEST_IMG")
>>> +    $QEMU_IO -c 'discard -q 0 8k' "$TEST_IMG"
>>> +    after=$(disk_usage "$TEST_IMG")
>>> +    _verify_du_delta $before $after 8192
>>> +    alloc="$(seq 4 31)"; zero="$(seq 0 3)"
>>> +    _verify_l2_bitmap 0
>>>       # Write clusters #0-#2 and then discard them
>>>       $QEMU_IO -c 'write -q 0 128k' "$TEST_IMG"
>>>       $QEMU_IO -c 'discard -q 0 128k' "$TEST_IMG"
>>
>> Similarly to above, I think it would be good if we combined this
>> following case with the one you added, i.e. to write 128k from the
>> beginning, drop the write here, and change the discard to be “discard
>> -q 8k 120k”, i.e. skip the subclusters we have already discarded, to
>> see that this is still combined to discard the whole first cluster.
>>
>> ...Ah, see, and when I try this, the following assertion fails:
>>
>> qemu-io: ../block/qcow2-cache.c:156: qcow2_cache_destroy: Assertion
>> `c->entries[i].ref == 0' failed.
>> ./common.rc: line 220: 128894 Aborted                 (core dumped) (
>> VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec
>> "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" )
>>
>> Looks like an L2 table is leaked somewhere.  That’s why SCRI should be
>> a g_auto()-able type.
> 
> Forgot to add: This single test case here is the only place where we
> test the added functionality.  I think there should be more cases. It
> doesn’t really make sense now that 271 has so many cases for writing
> zeroes, but so few for discarding, now that discarding works on
> subclusters.  Most of them should at least be considered whether we can
> run them for discard as well.
> 
> I didn’t want to push for such an extensive set of tests, but, well, now
> it turned out I overlooked a bug in patch 4, and only found it because I
> thought “this place might also make a nice test case for this series”.
> 
> Hanna
> 

I agree that more coverage should be added.  Based on the previous
email, I see the following cases:

1. Direct 'discard' on the subclusters range (discard_l2_subclusters()).
2. Direct 'discard' on the subclusters range, complementary to an
unallocated range (i.e. discard_l2_subclusters() -> discard_in_l2_slice()).
3. 'write -u -z' on the subclusters range (zero_l2_subclusters() ->
discard_l2_subclusters()).
4. 'write -u -z' on the subclusters range, complementary to an
unallocated range (zero_l2_subclusters() -> discard_l2_subclusters() ->
discard_in_l2_slice()).

Would also be nice to test the zero_l2_subclusters() ->
zero_in_l2_slice() path, but we'd have to somehow check the refcount
table for that since the L2 bitmap doesn't change.

Please let me know if you can think of anything else.