| Message ID | 20240127204417.11880-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] fstests: btrfs: verify the read behavior of compressed inline extent | expand |
On Sat, Jan 27, 2024 at 8:44 PM Qu Wenruo <wqu@suse.com> 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@suse.com> > --- > 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... > +# 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@gompa.dev>
On 2024/1/29 01:53, Neal Gompa wrote: > On Sat, Jan 27, 2024 at 8:44 PM Qu Wenruo <wqu@suse.com> 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@suse.com> >> --- >> 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@gompa.dev> > >
On 1/28/24 04:44, 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@suse.com> Reviewed-by: Anand Jain <anand.jain@oracle.com>
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\"" + +# 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
[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@suse.com> --- 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.