[PATCH] fstests: generic/362: remove the old file to reflect new mount options

[PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Qu Wenruo]

[HIDDEN BUG]
There is a btrfs bug that will only trigger on newly formated TEST_DEV,
with mount option "nodatasum":

 FSTYP         -- btrfs
 PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
 MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
 MOUNT_OPTIONS -- -o nodatasum /dev/mapper/test-scratch1 /mnt/scratch

 generic/362  0s ... - output mismatch (see /home/adam/xfstests/results//generic/362.out.bad)
     --- tests/generic/362.out	2024-08-24 15:31:37.200000000 +0930
     +++ /home/adam/xfstests/results//generic/362.out.bad	2026-05-26 16:24:40.201201849 +0930
     @@ -1,2 +1,3 @@
      QA output created by 362
     +First write failed: Input/output error
      Silence is golden
     ...
     (Run 'diff -u /home/adam/xfstests/tests/generic/362.out /home/adam/xfstests/results//generic/362.out.bad'  to see the entire diff)

But if one has formated TEST_DEV, run test with default mount option,
then change the mount option to "nodatasum", the test will not fail
anymore:

 FSTYP         -- btrfs
 PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
 MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
 MOUNT_OPTIONS -- /dev/mapper/test-scratch1 /mnt/scratch

 generic/362  0s ...  0s
 Ran: generic/362
 Passed all 1 tests

 FSTYP         -- btrfs
 PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
 MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
 MOUNT_OPTIONS -- -o nodatasum /dev/mapper/test-scratch1 /mnt/scratch

 generic/362  0s ...  0s
 Ran: generic/362
 Passed all 1 tests

[CAUSE]
Btrfs' nodatasum mount option only affect new files, but the test case
itself is using TEST_DEV, and never delete the file
"$TEST_DIR/dio-append-buf-fault"

So if the file is created with default mount option, then all later
"nodatasum" mount option will not affect that file, thus hide the test
failure.

[FIX]
Always delete the target file "$TEST_DIR/dio-append-buf-fault" before
running dio-append-buf-fault command.

So that the new target file is always newly created and will follow
btrfs' new mount option and expose the failure for nodatasum.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
I'm already looking into the test failure, because it's mostly hidden by
the btrfs' falling back to buffered IO behavior.

With the incoming IOMAP_DIO_BOUNCE usage inside btrfs, it will expose
the failure unconditionally.
---
 tests/generic/362 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/generic/362 b/tests/generic/362
index 0cfaa726..86191e57 100755
--- a/tests/generic/362
+++ b/tests/generic/362
@@ -20,6 +20,10 @@ _require_test_program dio-append-buf-fault
 _fixed_by_fs_commit btrfs 939b656bc8ab \
 	"btrfs: fix corruption after buffer fault in during direct IO append write"
 
+# Remove the existing file, so a new inode can be created, and will be
+# affected by changed mount options.
+rm -rf $TEST_DIR/dio-append-buf-fault
+
 # On error the test program writes messages to stderr, causing a golden output
 # mismatch and making the test fail.
 $here/src/dio-append-buf-fault $TEST_DIR/dio-append-buf-fault
-- 
2.51.2

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Christoph Hellwig]

On Tue, May 26, 2026 at 04:30:55PM +0930, Qu Wenruo wrote:
>       Silence is golden
>      ...
>      (Run 'diff -u /home/adam/xfstests/tests/generic/362.out /home/adam/xfstests/results//generic/362.out.bad'  to see the entire diff)
> 
> But if one has formated TEST_DEV, run test with default mount option,
> then change the mount option to "nodatasum", the test will not fail
> anymore:

> [FIX]
> Always delete the target file "$TEST_DIR/dio-append-buf-fault" before
> running dio-append-buf-fault command.
> 
> So that the new target file is always newly created and will follow
> btrfs' new mount option and expose the failure for nodatasum.

I'd rather add a copy of the test that runs on the scratch device
that can control the environment.  Or maybe just change the test
to use the scratch device?

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Qu Wenruo]

在 2026/5/27 16:04, Christoph Hellwig 写道:
> On Tue, May 26, 2026 at 04:30:55PM +0930, Qu Wenruo wrote:
>>        Silence is golden
>>       ...
>>       (Run 'diff -u /home/adam/xfstests/tests/generic/362.out /home/adam/xfstests/results//generic/362.out.bad'  to see the entire diff)
>>
>> But if one has formated TEST_DEV, run test with default mount option,
>> then change the mount option to "nodatasum", the test will not fail
>> anymore:
> 
>> [FIX]
>> Always delete the target file "$TEST_DIR/dio-append-buf-fault" before
>> running dio-append-buf-fault command.
>>
>> So that the new target file is always newly created and will follow
>> btrfs' new mount option and expose the failure for nodatasum.
> 
> I'd rather add a copy of the test that runs on the scratch device
> that can control the environment.  Or maybe just change the test
> to use the scratch device?

I'm completely happy with using scratch device instead of test dev.

If no special reason to use test dev, the next update will go scratch 
dev instead.

Thanks,
Qu

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Christoph Hellwig]

On Wed, May 27, 2026 at 04:20:38PM +0930, Qu Wenruo wrote:
> > I'd rather add a copy of the test that runs on the scratch device
> > that can control the environment.  Or maybe just change the test
> > to use the scratch device?
> 
> I'm completely happy with using scratch device instead of test dev.
> 
> If no special reason to use test dev, the next update will go scratch dev
> instead.

I went back and re-read this.  What is the special mount option you
change?   If you want to always exercise this as nocow, maybe add a new
btrfs test that is a copy and uses the scratch device?  Or am I
misunderstanding something?

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Filipe Manana]

On Tue, May 26, 2026 at 8:02 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [HIDDEN BUG]
> There is a btrfs bug that will only trigger on newly formated TEST_DEV,
> with mount option "nodatasum":
>
>  FSTYP         -- btrfs
>  PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
>  MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
>  MOUNT_OPTIONS -- -o nodatasum /dev/mapper/test-scratch1 /mnt/scratch
>
>  generic/362  0s ... - output mismatch (see /home/adam/xfstests/results//generic/362.out.bad)
>      --- tests/generic/362.out  2024-08-24 15:31:37.200000000 +0930
>      +++ /home/adam/xfstests/results//generic/362.out.bad       2026-05-26 16:24:40.201201849 +0930
>      @@ -1,2 +1,3 @@
>       QA output created by 362
>      +First write failed: Input/output error
>       Silence is golden
>      ...
>      (Run 'diff -u /home/adam/xfstests/tests/generic/362.out /home/adam/xfstests/results//generic/362.out.bad'  to see the entire diff)
>
> But if one has formated TEST_DEV, run test with default mount option,
> then change the mount option to "nodatasum", the test will not fail
> anymore:
>
>  FSTYP         -- btrfs
>  PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
>  MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
>  MOUNT_OPTIONS -- /dev/mapper/test-scratch1 /mnt/scratch
>
>  generic/362  0s ...  0s
>  Ran: generic/362
>  Passed all 1 tests
>
>  FSTYP         -- btrfs
>  PLATFORM      -- Linux/x86_64 btrfs-vm 7.1.0-rc4-custom+ #381 SMP PREEMPT_DYNAMIC Tue May 26 10:47:14 ACST 2026
>  MKFS_OPTIONS  -- -O bgt -K /dev/mapper/test-scratch1
>  MOUNT_OPTIONS -- -o nodatasum /dev/mapper/test-scratch1 /mnt/scratch
>
>  generic/362  0s ...  0s
>  Ran: generic/362
>  Passed all 1 tests
>
> [CAUSE]
> Btrfs' nodatasum mount option only affect new files, but the test case
> itself is using TEST_DEV, and never delete the file
> "$TEST_DIR/dio-append-buf-fault"
>
> So if the file is created with default mount option, then all later
> "nodatasum" mount option will not affect that file, thus hide the test
> failure.
>
> [FIX]
> Always delete the target file "$TEST_DIR/dio-append-buf-fault" before
> running dio-append-buf-fault command.
>
> So that the new target file is always newly created and will follow
> btrfs' new mount option and expose the failure for nodatasum.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> I'm already looking into the test failure, because it's mostly hidden by
> the btrfs' falling back to buffered IO behavior.
>
> With the incoming IOMAP_DIO_BOUNCE usage inside btrfs, it will expose
> the failure unconditionally.
> ---
>  tests/generic/362 | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/tests/generic/362 b/tests/generic/362
> index 0cfaa726..86191e57 100755
> --- a/tests/generic/362
> +++ b/tests/generic/362
> @@ -20,6 +20,10 @@ _require_test_program dio-append-buf-fault
>  _fixed_by_fs_commit btrfs 939b656bc8ab \
>         "btrfs: fix corruption after buffer fault in during direct IO append write"
>
> +# Remove the existing file, so a new inode can be created, and will be
> +# affected by changed mount options.
> +rm -rf $TEST_DIR/dio-append-buf-fault

We can just remove the file in a _cleanup function - that's what most
other tests that exercise faults during direct IO do, like
generic/647.
Some tests like this one are missing that, and there's at least one
more: generic/708. We had several similar tests, and many were copied
from another test.

Thanks.

> +
>  # On error the test program writes messages to stderr, causing a golden output
>  # mismatch and making the test fail.
>  $here/src/dio-append-buf-fault $TEST_DIR/dio-append-buf-fault
> --
> 2.51.2
>
>

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Qu Wenruo]

在 2026/5/27 22:33, Christoph Hellwig 写道:
> On Wed, May 27, 2026 at 04:20:38PM +0930, Qu Wenruo wrote:
>>> I'd rather add a copy of the test that runs on the scratch device
>>> that can control the environment.  Or maybe just change the test
>>> to use the scratch device?
>>
>> I'm completely happy with using scratch device instead of test dev.
>>
>> If no special reason to use test dev, the next update will go scratch dev
>> instead.
> 
> I went back and re-read this.  What is the special mount option you
> change?   If you want to always exercise this as nocow, maybe add a new
> btrfs test that is a copy and uses the scratch device?  Or am I
> misunderstanding something?
> 

My point is, the current script prevent the test case to respect certain 
mount options, thus reduce the coverage.

E.g. in a multi-section fstests setup, default and nodatasum mount 
options are defined in different sections, and default mount option will 
be run first.

Then the next nodatasum section will still utilize the old inode created 
with regular default options, thus reduce the coverage.


Sure, a dedicated btrfs copy will help, but that will introduce duplication.
Not sure what is the proper way to handle such situation.


For now I'll follow Filipe's review to use _cleanup() to delete those 
involves test cases.

Thanks,
Qu

Re: [PATCH] fstests: generic/362: remove the old file to reflect new mount options

[Zorro Lang]

On Thu, May 28, 2026 at 07:12:24AM +0930, Qu Wenruo wrote:
> 
> 
> 在 2026/5/27 22:33, Christoph Hellwig 写道:
> > On Wed, May 27, 2026 at 04:20:38PM +0930, Qu Wenruo wrote:
> > > > I'd rather add a copy of the test that runs on the scratch device
> > > > that can control the environment.  Or maybe just change the test
> > > > to use the scratch device?
> > > 
> > > I'm completely happy with using scratch device instead of test dev.
> > > 
> > > If no special reason to use test dev, the next update will go scratch dev
> > > instead.
> > 
> > I went back and re-read this.  What is the special mount option you
> > change?   If you want to always exercise this as nocow, maybe add a new
> > btrfs test that is a copy and uses the scratch device?  Or am I
> > misunderstanding something?
> > 
> 
> My point is, the current script prevent the test case to respect certain
> mount options, thus reduce the coverage.
> 
> E.g. in a multi-section fstests setup, default and nodatasum mount options
> are defined in different sections, and default mount option will be run
> first.
> 
> Then the next nodatasum section will still utilize the old inode created
> with regular default options, thus reduce the coverage.
> 
> 
> Sure, a dedicated btrfs copy will help, but that will introduce duplication.
> Not sure what is the proper way to handle such situation.

If there is a specific btrfs bug (commit) that can be 100% reproduced by
slightly modifying this case (e.g. use a specific mkfs or mount option),
I think it’s worth splitting it into a separate test case for better
test coverage and known issue tracking.

Thanks,
Zorro

> 
> 
> For now I'll follow Filipe's review to use _cleanup() to delete those
> involves test cases.
> 
> Thanks,
> Qu
>