On Tue, Jan 23, 2024 at 7:08 PM Qu Wenruo <quwenruo.btrfs@xxxxxxx> wrote:
>
>
>
> On 2024/1/24 10:21, Neal Gompa wrote:
> > On Mon, Jan 22, 2024 at 10:49 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 | 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@xxxxxxxxx>
--
真実はいつも一つ！/ Always, there's only one truth!
