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
