On 1/25/24 05:00, Qu Wenruo wrote:
On 2024/1/24 22:40, Anand Jain wrote:
On 1/23/24 11:49, Qu Wenruo wrote:
[BUG]
There is a report about reading a zstd compressed inline file extent
would lead to either a VM_BUG_ON() crash, or lead to incorrect file
content.
[CAUSE]
The root cause is a incorrect memcpy_to_page() call, which uses
incorrect page offset, and can lead to either the VM_BUG_ON() as we may
write beyond the page boundary, or writes into the incorrect offset of
the page.
[TEST CASE]
The test case would:
- Mount with the specified compress algorithm
- Create a 4K file
- Verify the 4K file is all inlined and compressed
- Verify the content of the initial write
- Cycle mount to drop all the page cache
- Verify the content of the file again
- Unmount and fsck the fs
This workload would be applied to all supported compression algorithms.
And it can catch the problem correctly by triggering VM_BUG_ON(), as our
workload would result decompressed extent size to be 4K, and would
trigger the VM_BUG_ON() 100%.
And with the revert or the new fix, the test case can pass safely.
Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
---
tests/btrfs/310 | 81 +++++++++++++++++++++++++++++++++++++++++++++
tests/btrfs/310.out | 2 ++
2 files changed, 83 insertions(+)
create mode 100755 tests/btrfs/310
create mode 100644 tests/btrfs/310.out
diff --git a/tests/btrfs/310 b/tests/btrfs/310
new file mode 100755
index 00000000..b514a8bc
--- /dev/null
+++ b/tests/btrfs/310
@@ -0,0 +1,81 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2024 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 310
+#
+# Make sure reading on an compressed inline extent is behaving
correctly
+#
+. ./common/preamble
+_begin_fstest auto quick compress
+
+# Import common functions.
+# . ./common/filter
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs btrfs
+_require_scratch
+
+# This test require inlined compressed extents creation, and all the
writes
+# are designed for 4K sector size.
+_require_btrfs_inline_extents_creation
+_require_btrfs_support_sectorsize 4096
+
+_fixed_by_kernel_commit e01a83e12604 \
+ "Revert \"btrfs: zstd: fix and simplify the inline extent
decompression\""
+
+# The correct md5 for the correct 4K file filled with "0xcd"
+md5sum_correct="5fed275e7617a806f94c173746a2a723"
+
+workload()
+{
+ local algo="$1"
+
+ echo "=== Testing compression algorithm ${algo} ===" >>
$seqres.full
+ _scratch_mkfs >> $seqres.full
+ _scratch_mount -o compress=${algo}
+
+ _pwrite_byte 0xcd 0 4k "$SCRATCH_MNT/inline_file" > /dev/null
+ result=$(_md5_checksum "$SCRATCH_MNT/inline_file")
+ echo "after initial write, md5sum=${result}" >> $seqres.full
+ if [ "$result" != "$md5sum_correct" ]; then
+ _fail "initial write results incorrect content for \"$algo\""
+ fi
General rule of thumb is where possible use stdout and compare it with
the golden output.
So something like the below shall suffice.
echo "after initial write with alog=$algo"
_md5_checksum "$SCRATCH_MNT/inline_file"
Also, helps quick debug, when fails we have the diff.
Nope, for this particular case, golden output is not suitable.
As the workload is dependent on the support compression algorithm, we
can not reply on golden output to cover all algorithms.
That's a fair reason not to use golden output.
Looks like you missed more comments below.
+ sync
Generally, we need comments to explain why sync is necessary.
Needs a comment for calling sync.
+
+ $XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/inline_file | tail -n 1
> $tmp.fiemap
+ cat $tmp.fiemap >> $seqres.full
+ # Make sure we got an inlined compressed file extent.
+ # 0x200 means inlined, 0x100 means not block aligned, 0x8 means
encoded
+ # (compressed in this case), and 0x1 means the last extent.
+ if ! grep -q "0x309" $tmp.fiemap; then
+ rm -f -- $tmp.fiemap
+ _notrun "No compressed inline extent created, maybe subpage?"
workload() is called for each compress algo. If we fail
for one of the algo then notrun is not a good option here.
There is no good alternative, lets keep the notrun for now.
Thanks, Anand
IMO, stdout (with filters?) and comparing it with golden output
is better.
+ fi
+ rm -f -- $tmp.fiemap
+
+ # Unmount to clear the page cache.
+ _scratch_cycle_mount
+
+ # For v6.8-rc1 without the revert or the newer fix, this can
+ # crash or lead to incorrect contents for zstd.
+ result=$(_md5_checksum "$SCRATCH_MNT/inline_file")
+ echo "after cycle mount, md5sum=${result}" >> $seqres.full
+ if [ "$result" != "$md5sum_correct" ]; then
+ _fail "read for compressed inline extent failed for \"$algo\""
+ fi
Here too, same as above, golden output to compare can be done.
And remove _fail.
Thanks, Anand
+ _scratch_unmount
+ _check_scratch_fs
+}
+
+algo_list=($(_btrfs_compression_algos))
+for algo in ${algo_list[@]}; do
+ workload $algo
+done
+
+echo "Silence is golden"
+
+status=0
+exit
diff --git a/tests/btrfs/310.out b/tests/btrfs/310.out
new file mode 100644
index 00000000..7b9eaf78
--- /dev/null
+++ b/tests/btrfs/310.out
@@ -0,0 +1,2 @@
+QA output created by 310
+Silence is golden
