Re: [PATCH] fstests: btrfs/226: fill in missing comments changes

[Anand Jain <anand.jain@oracle.com>]

On 22/2/25 19:29, Filipe Manana wrote:
> On Sat, Feb 22, 2025 at 11:17 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>>
>>
>> On 21/2/25 23:03, Zorro Lang wrote:
>>> On Fri, Feb 21, 2025 at 12:04:32PM +0000, Filipe Manana wrote:
>>>> On Tue, Feb 18, 2025 at 10:36 PM Anand Jain <anand.jain@oracle.com> wrote:
>>>>>
>>>>> From: Qu Wenruo <wqu@suse.com>
>>>>>
>>>>> Update comments that were previously missed.
>>>>>
>>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>>> ---
>>>>>    tests/btrfs/226 | 6 ++----
>>>>>    1 file changed, 2 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/tests/btrfs/226 b/tests/btrfs/226
>>>>> index 359813c4f394..ce53b7d48c49 100755
>>>>> --- a/tests/btrfs/226
>>>>> +++ b/tests/btrfs/226
>>>>> @@ -22,10 +22,8 @@ _require_xfs_io_command fpunch
>>>>>
>>>>>    _scratch_mkfs >>$seqres.full 2>&1
>>>>>
>>>>> -# This test involves RWF_NOWAIT direct IOs, but for inodes with data checksum,
>>>>> -# btrfs will fall back to buffered IO unconditionally to prevent data checksum
>>>>> -# mimsatch, and that will break RWF_NOWAIT with -EAGAIN.
>>>>> -# So here we have to go with nodatasum mount option.
>>>>> +# RWF_NOWAIT works only with direct I/O and requires an inode with nodatasum
>>>>> +# to avoid checksum mismatches. Otherwise, it falls back to buffered I/O.
>>>>
>>>> Btw, this is different from what I suggested before here:
>>>>
>>>> https://lore.kernel.org/fstests/68aa436b-4ddd-4ee7-ad5a-8eca55aae176@oracle.com/T/#mb2369802d2e33c9778c62fcb3c0ee47de28b773b
>>>>
>>>> Which is:
>>>>
>>>> # RWF_NOWAIT only works with direct IO, which requires an inode with
>>>> nodatasum (otherwise it falls back to buffered IO).
>>>>
>>>> What is being added in this patch:
>>>>
>>>> +# RWF_NOWAIT works only with direct I/O and requires an inode with nodatasum
>>>> +# to avoid checksum mismatches. Otherwise, it falls back to buffered I/O.
>>>>
>>>> Is confusing because:
>>>>
>>>> 1) It gives the suggestion RWF_NOWAIT requires nodatasum.
>>>>
>>>> 2) The part that says "to avoid checksum mismatches", that's not
>>>> related to RWF_NOWAIT at all.
>>>>       That's the reason why direct IO writes against inodes without
>>>> nodatasum fallback to buffered IO.
>>>>       We don't have to explain that - this is not a test to exercise the
>>>> fallback after all, all we have to say
>>>>       is that RWF_NOWAIT needs direct IO and direct IO can only be done
>>>> against inodes with nodatasum.
>>>>
>>>> So you didn't pick my suggestion after all, you just added your own
>>>> rephrasing which IMO is confusing.
>>>
>>
>> Your sentence missed the consequence part (checksum mismatches) that
>> Qu's sentence included.
> 
> And that's totally irrelevant to this test case.
> 
> Preventing checksum mismatches is why direct IO writes fallback to
> buffered IO if the inode doesn't have the nodatasum flag - it has
> nothing to do with RWF_NOWAIT writes.
> Besides that, such mismatches only happen for cases where an app
> writes to the write buffer while the direct IO write is in progress -
> which is not the case of this test case.
> 
>>
>> How about,
>>
>> # RWF_NOWAIT only works with direct IO, which requires an inode with
>> nodatasum to avoid checksum-mismatches (otherwise it falls back to
>> buffered IO).
> 
> Just stick it to the original - simple and to the point.
> 
> Thanks.

Done. Thx.
Anand

> 
>>
>> Thx, Anand
>>
>>> Hi Anand, please talk with Filipe (or more btrfs folks) and make a final
>>> decision about how to write this comment. I'll drop this patch from
>>> patches-in-queue branch temporarily, until you reach a consensus :)
>>>
>>> Thanks,
>>> Zorro
>>>
>>>>
>>>>
>>>>
>>>>>    _scratch_mount -o nodatasum
>>>>>
>>>>>    # Test a write against COW file/extent - should fail with -EAGAIN. Disable the
>>>>> --
>>>>> 2.47.0
>>>>>
>>>>>
>>>>
>>>
>>