fstests: btrfs/226: fill in missing comments changes

Commit Message

Anand Jain Feb. 18, 2025, 10:35 p.m. UTC

From: Qu Wenruo <[e-mail]>

Update comments that were previously missed.

Signed-off-by: Anand Jain <[e-mail]>
---
 tests/btrfs/226 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Zorro Lang Feb. 19, 2025, 5:58 a.m. UTC | #1

On Wed, Feb 19, 2025 at 06:35:44AM +0800, Anand Jain wrote:
> From: Qu Wenruo <[e-mail]>
> 
> Update comments that were previously missed.
> 
> Signed-off-by: Anand Jain <[e-mail]>
> ---
>  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.

As a supplement patch, this is good to me.

Reviewed-by: Zorro Lang <[e-mail]>

>  _scratch_mount -o nodatasum
>  
>  # Test a write against COW file/extent - should fail with -EAGAIN. Disable the
> -- 
> 2.47.0
> 
>

Filipe Manana Feb. 21, 2025, 12:04 p.m. UTC | #2

On Tue, Feb 18, 2025 at 10:36 PM Anand Jain <[e-mail]> wrote:
>
> From: Qu Wenruo <[e-mail]>
>
> Update comments that were previously missed.
>
> Signed-off-by: Anand Jain <[e-mail]>
> ---
>  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/[e-mail]/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.



>  _scratch_mount -o nodatasum
>
>  # Test a write against COW file/extent - should fail with -EAGAIN. Disable the
> --
> 2.47.0
>
>

Zorro Lang Feb. 21, 2025, 3:03 p.m. UTC | #3

On Fri, Feb 21, 2025 at 12:04:32PM +0000, Filipe Manana wrote:
> On Tue, Feb 18, 2025 at 10:36 PM Anand Jain <[e-mail]> wrote:
> >
> > From: Qu Wenruo <[e-mail]>
> >
> > Update comments that were previously missed.
> >
> > Signed-off-by: Anand Jain <[e-mail]>
> > ---
> >  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/[e-mail]/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.

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

Anand Jain Feb. 22, 2025, 11:16 a.m. UTC | #4

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 <[e-mail]> wrote:
>>>
>>> From: Qu Wenruo <[e-mail]>
>>>
>>> Update comments that were previously missed.
>>>
>>> Signed-off-by: Anand Jain <[e-mail]>
>>> ---
>>>   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/[e-mail]/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
>>>
>>>
>>
>

Filipe Manana Feb. 22, 2025, 11:29 a.m. UTC | #5

On Sat, Feb 22, 2025 at 11:17 AM Anand Jain <[e-mail]> 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 <[e-mail]> wrote:
> >>>
> >>> From: Qu Wenruo <[e-mail]>
> >>>
> >>> Update comments that were previously missed.
> >>>
> >>> Signed-off-by: Anand Jain <[e-mail]>
> >>> ---
> >>>   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/[e-mail]/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.


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

Patch

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.
 _scratch_mount -o nodatasum
 
 # Test a write against COW file/extent - should fail with -EAGAIN. Disable the