| Message ID | 20240123034908.25415-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: btrfs: verify the read behavior of compressed inline extent | expand |
On Tue, 23 Jan 2024 14:19:08 +1030, 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> > --- > 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 Hopefully coverage can be expanded in future to cover some other sector sizes. This looks fine for now: Reviewed-by: David Disseldorp <ddiss@suse.de>
On Mon, Jan 22, 2024 at 10:49 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 | 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 > + 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 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 > + _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 > > This looks reasonable to me, but how is $_btrfs_compression_algos defined? Does it include all the algorithm options supported in Btrfs?
On 2024/1/24 10:21, Neal Gompa wrote: > On Mon, Jan 22, 2024 at 10:49 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 | 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 >> + 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 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 >> + _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 >> >> > > This looks reasonable to me, but how is $_btrfs_compression_algos > defined? Does it include all the algorithm options supported in Btrfs? It fetches all the supported compression algo through the sysfs interfaces: /sys/fs/btrfs/features/compress_* Along with the default supported zlib compression. Thanks, Qu > >
On Tue, Jan 23, 2024 at 7:08 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > On 2024/1/24 10:21, Neal Gompa wrote: > > On Mon, Jan 22, 2024 at 10:49 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 | 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 > >> + 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 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 > >> + _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 > >> > >> > > > > This looks reasonable to me, but how is $_btrfs_compression_algos > > defined? Does it include all the algorithm options supported in Btrfs? > > It fetches all the supported compression algo through the sysfs interfaces: > > /sys/fs/btrfs/features/compress_* > > Along with the default supported zlib compression. > Cool then. Reviewed-by: Neal Gompa <neal@gompa.dev>
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@suse.com> > --- > 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. > + sync Generally, we need comments to explain why sync is necessary. > + > + $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. 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
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@suse.com> >> --- >> 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. Or it would always fail for older kernels without zstd, or for newer kernel with newer algorithm. That's why I personally don't believe golden output is always the best way to go. Thanks, Qu > > >> + sync > > Generally, we need comments to explain why sync is necessary. > >> + >> + $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. > > 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 > >
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@suse.com> >>> --- >>> 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 >> >>
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 + 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 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 + _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 | 81 +++++++++++++++++++++++++++++++++++++++++++++ tests/btrfs/310.out | 2 ++ 2 files changed, 83 insertions(+) create mode 100755 tests/btrfs/310 create mode 100644 tests/btrfs/310.out