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.
