Re: [PATCH] generic: add a test case for writes with prealloc extents beyond i_size

在 2026/5/29 09:22, Anand Jain 写道:
> On 28/5/26 19:30, Filipe Manana wrote:
>> On Thu, May 28, 2026 at 12:23 PM Qu Wenruo <[email protected]> wrote:
>>>
>>>
>>>
>>> 在 2026/5/28 19:57, [email protected] 写道:
>>>> From: Filipe Manana <[email protected]>
>>>>
>>>> Test writing into a file range containing prealloc extents beyond the current
>>>> i_size, with an unmount and mount after fallocate and the write, to verify
>>>> that the file data, size and extent layout were not lost.
>>>>
>>>> This used to fail on btrfs when not using the no-holes feature (which is
>>>> a default since btrfs-progs 5.15) before this recent kernel fix:
>>>>
>>>>      080ecbd05432 ("btrfs: mark file extent range dirty after converting prealloc extents")
>>>>
>>>> So in order to reproduce the failure when using an unpatched kernel and
>>>> a btrfs-progs >= 5.15, one must run the test with:
>>>>
>>>>      MKFS_OPTIONS="-O ^no-holes"
>>>>
>>>> Signed-off-by: Filipe Manana <[email protected]>
>>>
>>> Reviewed-by: Qu Wenruo <[email protected]>
>>>
>>>
>>> Just one question related to the specified mkfs option.
>>>
>>> As you mentioned, this bug is only affecting ^no-holes, thus default
>>> mount option runs won't trigger it.
>>>
>>> This makes me wondering, should we have a dedicated btrfs test case just
>>> exercising the ^no-holes path.
>>>
>>> I know this will cause duplication, thus will not be a good idea to
>>> maintain, but we also have dedicated test cases utilizing specified
>>> mount option/mkfs options already.
>>>
>>> So what is the prefer method here?
>>
> 
> There will be many testcase and option combinations,
> which makes this harder to scale and increases the
> chances of missing a testcase + option combination
> across federated teams.
> 
> I think it makes sense to maintain a standard testing config
> (not sure what to call it, maybe a Test-Profile?).
> 
> V2 was sent here:
> 
> https://lore.kernel.org/fstests/cfb8c19533ac3c764edc1fe62b7fde75e76579a4.1743137470.git.anand.jain@oracle.com/
> 
> Any thoughts? I'm ok to revisit to send v3 if needed.

There is no "nodatasum" mount option to verify the behavior of zero-copy 
direct writes.
Although I hope the incoming IOMAP_DIO_BOUNCE flag usage will get rid of 
the special nodatasum requirement for dio.

Nor "flushoncommit" mount option which recently exposed a deadlock.

And I'm not sure if you should put "rst" mkfs option as default, it's 
still very experimental.

Not to mention there will be more and more sectorsize/nodesize 
combination to come with bs > ps support, or on 64K page sizes systems.
Meanwhile the coverage itself may not be improved that much with extra 
sectorsizes...

Finally you're pushing for a lot of compression runs (3 out of 8), but 
IIRC there are some known failures and some ENOSPC runs will take way 
too longer than regular runs.

Even with a 24x7 VM running tests, the generated false alerts still will 
take a lot of human time to review and fix.

> 
> Thanks, Anand
> 
> 
>> So in the past I attempted tests like that, making them btrfs specific
>> and forcing a mount option.
>> Some people (non-btrfs people, I don't recall exactly who, to be
>> honest) disagreed with the claims that the test was actually generic,
>> and that exercising the bug should be done by setting MKFS_OPTIONS in
>> the command line.
>>
>> I'm assuming people and our automations run tests with -O ^no-holes. I
>> do it frequently in my test vms.
>>
>>>
>>> Thanks,
>>> Qu
>>>> ---
>>>>    tests/generic/796     | 54 +++++++++++++++++++++++++++++++++++++++++++
>>>>    tests/generic/796.out | 10 ++++++++
>>>>    2 files changed, 64 insertions(+)
>>>>    create mode 100755 tests/generic/796
>>>>    create mode 100644 tests/generic/796.out
>>>>
>>>> diff --git a/tests/generic/796 b/tests/generic/796
>>>> new file mode 100755
>>>> index 00000000..c42a4722
>>>> --- /dev/null
>>>> +++ b/tests/generic/796
>>>> @@ -0,0 +1,54 @@
>>>> +#! /bin/bash
>>>> +# SPDX-License-Identifier: GPL-2.0
>>>> +# Copyright (c) 2026 SUSE S.A.  All Rights Reserved.
>>>> +#
>>>> +# FS QA Test 796
>>>> +#
>>>> +# Test writing into a file range containing prealloc extents beyond the current
>>>> +# i_size, with an unmount and mount after fallocate and the write, to verify
>>>> +# that the file data, size and extent layout were not lost.
>>>> +#
>>>> +. ./common/preamble
>>>> +_begin_fstest auto quick prealloc preallocrw fiemap
>>>> +
>>>> +. ./common/filter
>>>> +. ./common/punch # for _filter_fiemap
>>>> +
>>>> +_require_scratch
>>>> +_require_xfs_io_command "falloc" "-k"
>>>> +_require_xfs_io_command "fiemap"
>>>> +
>>>> +_fixed_by_fs_commit btrfs 080ecbd05432 \
>>>> +     "btrfs: mark file extent range dirty after converting prealloc extents"
>>>> +
>>>> +_scratch_mkfs >>$seqres.full 2>&1
>>>> +_scratch_mount
>>>> +
>>>> +# The fiemap results in the golden output requires file allocations to align to
>>>> +# 1M boundaries.
>>>> +_require_congruent_file_oplen $SCRATCH_MNT 1048576
>>>> +
>>>> +# Create our file with a size of 0 and a prealloc extent in the range [0, 2M].
>>>> +$XFS_IO_PROG -f -c "falloc -k 0 2M" $SCRATCH_MNT/foo
>>>> +
>>>> +# Unmount and mount again to remove any in memory state of the inode. We will
>>>> +# verify later that neither metadata nor extents were lost during unmount.
>>>> +_scratch_cycle_mount
>>>> +
>>>> +# Write into the [0, 1M] range, which increases the inode's i_size.
>>>> +$XFS_IO_PROG -c "pwrite -S 0xab -b 1M 0 1M" $SCRATCH_MNT/foo | _filter_xfs_io
>>>> +
>>>> +# Unmount and mount again to remove any in memory state of the inode. We will
>>>> +# verify later that neither metadata nor extents were lost during unmount.
>>>> +_scratch_cycle_mount
>>>> +
>>>> +# Check file data (and size).
>>>> +echo "File data:"
>>>> +_hexdump $SCRATCH_MNT/foo
>>>> +
>>>> +# Check we have unwritten extents in range [1M, 2M].
>>>> +echo "Fiemap output:"
>>>> +$XFS_IO_PROG -c "fiemap -v" $SCRATCH_MNT/foo | _filter_fiemap
>>>> +
>>>> +# Success, all done.
>>>> +_exit 0
>>>> diff --git a/tests/generic/796.out b/tests/generic/796.out
>>>> new file mode 100644
>>>> index 00000000..c6c6e6a8
>>>> --- /dev/null
>>>> +++ b/tests/generic/796.out
>>>> @@ -0,0 +1,10 @@
>>>> +QA output created by 796
>>>> +wrote 1048576/1048576 bytes at offset 0
>>>> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>> +File data:
>>>> +000000 ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab ab  >................<
>>>> +*
>>>> +100000
>>>> +Fiemap output:
>>>> +0: [0..2047]: data
>>>> +1: [2048..4095]: unwritten
>>>
> 
>