From: Anand Jain <anand.jain@oracle.com>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>, Qu Wenruo <wqu@suse.com>,
linux-btrfs@vger.kernel.org, fstests@vger.kernel.org
Subject: Re: [PATCH] fstests: btrfs: verify the read behavior of compressed inline extent
Date: Sat, 27 Jan 2024 16:38:22 +0800 [thread overview]
Message-ID: <72d39266-3ec6-462a-9ac2-02040c6c37e0@oracle.com> (raw)
In-Reply-To: <9baa2a24-a44a-4f5e-95d7-23509ea450e4@gmx.com>
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
>>
>>
prev parent reply other threads:[~2024-01-27 8:38 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-23 3:49 [PATCH] fstests: btrfs: verify the read behavior of compressed inline extent Qu Wenruo
2024-01-23 4:57 ` David Disseldorp
2024-01-23 23:51 ` Neal Gompa
2024-01-24 0:08 ` Qu Wenruo
2024-01-24 1:06 ` Neal Gompa
2024-01-24 12:10 ` Anand Jain
2024-01-24 21:00 ` Qu Wenruo
2024-01-27 8:38 ` Anand Jain [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=72d39266-3ec6-462a-9ac2-02040c6c37e0@oracle.com \
--to=anand.jain@oracle.com \
--cc=fstests@vger.kernel.org \
--cc=linux-btrfs@vger.kernel.org \
--cc=quwenruo.btrfs@gmx.com \
--cc=wqu@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).