On 2024/1/29 01:53, Neal Gompa wrote:
On Sat, Jan 27, 2024 at 8:44 PM Qu Wenruo <wqu@xxxxxxxx> 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 | 83 +++++++++++++++++++++++++++++++++++++++++++++
tests/btrfs/310.out | 2 ++
2 files changed, 85 insertions(+)
create mode 100755 tests/btrfs/310
create mode 100644 tests/btrfs/310.out
---
Changelog:
v2:
- Add a comment on why a "sync" is needed
- Update the failure case comment
The specific design of the inline extent size is ensured to trigger
VM_BUG_ON(), thus remove the data corruption case.
diff --git a/tests/btrfs/310 b/tests/btrfs/310
new file mode 100755
index 00000000..507485a4
--- /dev/null
+++ b/tests/btrfs/310
@@ -0,0 +1,83 @@
+#! /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\""
+
I assume that this will be updated once we land a fixed commit? We
should ensure we keep track of those things...
Not exactly, in fact the test case only verify the very basic of inline
compressed extent read, thus this fix is correct.
For the reflinking of inline compressed extent, the behavior is tested
in another test case.
Thanks,
Qu
+# 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
+ # Writeback data to get correct fiemap result, or we got FIEMAP_DEALLOC
+ # without compression/inline flags.
+ 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?"
+ 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 would
+ # lead to VM_BUG_ON() thus crash
+ 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
+ _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
--
2.42.0
Otherwise, LGTM.
Reviewed-by: Neal Gompa <neal@xxxxxxxxx>
