On 2021/11/24 20:51, Filipe Manana wrote:
On Wed, Nov 24, 2021 at 12:28 PM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
On 2021/11/24 19:22, Filipe Manana wrote:
On Wed, Nov 24, 2021 at 9:24 AM Qu Wenruo <wqu@xxxxxxxx> wrote:
Since kernel commit d4088803f511 ("btrfs: subpage: make lzo_compress_pages()
compatible"), lzo compression no longer respects the max compressed page
limit, and can cause kernel crash.
The upstream fix is 6f019c0e0193 ("btrfs: fix a out-of-bound access in
copy_compressed_data_to_page()").
This patch will add such regression test for all possible compress-force
mount options, including lzo, zstd and zlib.
And since we're here, also make sure the content of the file matches
after a mount cycle.
Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
---
Changelog:
v2:
- Also test zlib and zstd
- Add file content verification check
---
tests/btrfs/049 | 56 +++++++++++++++++++++++++++++++++++++++++++++
tests/btrfs/049.out | 6 +++++
2 files changed, 62 insertions(+)
create mode 100755 tests/btrfs/049
diff --git a/tests/btrfs/049 b/tests/btrfs/049
new file mode 100755
index 00000000..264e576f
--- /dev/null
+++ b/tests/btrfs/049
@@ -0,0 +1,56 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2021 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 049
+#
+# Test if btrfs will crash when using compress-force mount option against
+# incompressible data
+#
+. ./common/preamble
+_begin_fstest auto quick compress dangerous
+
+# Override the default cleanup function.
+_cleanup()
+{
+ cd /
+ rm -r -f $tmp.*
+}
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+pagesize=$(get_page_size)
+workload()
+{
+ local compression
+ compression=$1
Could be shorter by doing it in one step:
local compression=$1
+
+ echo "=== Testing compress-force=$compression ==="
+ _scratch_mkfs -s "$pagesize">> $seqres.full
+ _scratch_mount -o compress-force="$compression"
+ $XFS_IO_PROG -f -c "pwrite -i /dev/urandom 0 $pagesize" \
+ "$SCRATCH_MNT/file" > /dev/null
+ md5sum "$SCRATCH_MNT/file" > "$tmp.$compression"
This doesn't really check if everything we asked to write was actually written.
pwrite(2), write(2), etc, return the number of bytes written, which
can be less than what we asked for.
And using the checksum verification in that way, we are only checking
that what we had before unmounting is the same after mounting again.
I.e. we are not checking that what was actually written is what we
have asked for.
We could do something like:
data=$(dd count=4096 bs=1 if=/dev/urandom)
echo -n "$data" > file
The reason I didn't want to use dd is it doesn't have good enough
wrapper in fstests.
(Thus I guess that's also the reason why you use above command to
workaround it)
If you're really concerned about the block size, it can be easily
changed using "-b" option of pwrite, to archive the same behavior of the
dd command.
Furthermore, since we're reading from urandom, isn't it already ensured
we won't get blocked nor get short read until we're reading very large
blocks?
I gave dd as an example, but I don't care about dd at all, it can be
xfs_io or anything else.
My whole point was about verifying that what's written to the file is
what we intended to write.
Verifying the checksum is fine when we know exactly what we asked to
write, when the test hard codes what we want to write.
In this case we're asking to write random content, so doing an md5sum
of the file after writing and
then comparing it to what we get after we unmount and mount again, is
not a safe way to test we have the
expected content.
Thus a very basic filter on the pwrite should be enough to make sure we
really got page sized data written.
It's not just about checking if the size of what was written matches
what we asked to write.
It's all about verifying the written content matches what we asked to
write in the first place (which implicitly ends up verifying the size
as well when using a checksum).
OK, then what we should do is first read 4K from urandom into some known
good place (thus I guess that's why you only use dd to read into
stdout), then do the csum, and compare it after the mount cycle?
Thanks,
Qu
Thanks,
Qu
_scratch_cycle_mount
check that the the md5sum of file is the same as: echo -n "$data" | md5sum
As it is, the test is enough to trigger the original bug, but having
such additional checks is more valuable IMO for the long run, and can
help prevent other types of regressions too.
Thanks Qu.
+
+ # When unpatched, compress-force=lzo would crash at data writeback
+ _scratch_cycle_mount
+
+ # Make sure the content matches
+ md5sum -c "$tmp.$compression" | _filter_scratch
+ _scratch_unmount
+}
+
+workload lzo
+workload zstd
+workload zlib
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/049.out b/tests/btrfs/049.out
index cb0061b3..258f3c09 100644
--- a/tests/btrfs/049.out
+++ b/tests/btrfs/049.out
@@ -1 +1,7 @@
QA output created by 049
+=== Testing compress-force=lzo ===
+SCRATCH_MNT/file: OK
+=== Testing compress-force=zstd ===
+SCRATCH_MNT/file: OK
+=== Testing compress-force=zlib ===
+SCRATCH_MNT/file: OK
--
2.34.0
