Re: [PATCH] generic: add test for missing btrfs csums in log when doing async on subpage vol

[Mark Harmstone <maharmstone@meta.com>]

On 16/10/24 12:09, Filipe Manana wrote:
> > 
> On Tue, Oct 15, 2024 at 4:42 PM Mark Harmstone <maharmstone@fb.com> wrote:
>>
>> Adds a test for a bug we encountered on Linux 6.4 on aarch64, where a
>> race could mean that csums weren't getting written to the log tree,
>> leading to corruption when it was replayed.
>>
>> The patches to detect log this tree corruption are in btrfs-progs 6.11.
> 
> This shouldn't be needed right?
> Because after log replay the csums are missing and 'btrfs check'
> detects (IIRC) missing csums for extents referred by file extent items
> in a subvolume tree - if it doesn't then it should be improved.

Yes, but we're not mounting it in the tests between the log_writes 
calls, so the log isn't getting replayed. The patches to btrfs check 
make it so that it identifies filesystems that would get corrupted as 
soon as they're next mounted.

> 
>>
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>> ---
>> This is a genericized version of the test I originally proposed as
>> btrfs/333.
>>
>>   tests/generic/757     | 71 +++++++++++++++++++++++++++++++++++++++++++
>>   tests/generic/757.out |  2 ++
>>   2 files changed, 73 insertions(+)
>>   create mode 100755 tests/generic/757
>>   create mode 100644 tests/generic/757.out
>>
>> diff --git a/tests/generic/757 b/tests/generic/757
>> new file mode 100755
>> index 00000000..6ad3d01e
>> --- /dev/null
>> +++ b/tests/generic/757
>> @@ -0,0 +1,71 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# FS QA Test 757
>> +#
>> +# Test async dio with fsync to test a btrfs bug where a race meant that csums
>> +# weren't getting written to the log tree, causing corruptions on remount.
>> +# This can be seen on subpage FSes on Linux 6.4.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick metadata log recoveryloop
>> +
>> +_fixed_by_kernel_commit e917ff56c8e7 \
>> +       "btrfs: determine synchronous writers from bio or writeback control"
> 
> For generic tests what we do is:
> 
> [ $FSTYP == "btrfs" ] && _fixed_by_kernel_commit .....
> 
> As long as the failure has not been observed and fixed on other filesystems.
> In case one day a regression happens in another fs, we just add a
> corresponding line using the same logic.
> 
> Otherwise if the test one days fails on another fs and fstests
> suggests that that commit is missing, it would be odd.
> 
> Everything else looks good, so with that fixed (maybe Zorro can change
> that when picking the patch):
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 

Thanks Filipe. Zorro, let me know if you're happy making this change, or 
otherwise I'll resubmit.

Mark