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

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

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.

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).

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
>>>
>>>
>>
>