[PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

xfs_io pwrite issues a series of block size writes, but there is no guarantee
that the resulting extent(s) will be singular or contiguous. This behavior is
acceptable, but the test is flawed in that it expects a single extent for a
pwrite.

Modify test to accept any layout for the reflinked logical range.

Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
 tests/generic/352     | 16 +++++++++++-----
 tests/generic/352.out |  2 --
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/tests/generic/352 b/tests/generic/352
index 52ec4850..c4ee8a44 100755
--- a/tests/generic/352
+++ b/tests/generic/352
@@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
 # use reflink to create the rest of the file, whose all extents are all
 # pointing to the first extent
 for i in $(seq 1 $nr); do
-	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
+	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
 done
 
 # then call fiemap on that file to test both the shared flag and if
 # reserved extent mapping search will cause soft lockup
-$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
-cat $tmp.out >> $seqres.full
+$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
+cat $tmp2.out >> $seqres.full
 
 # refact the $LOAD_FACTOR to 1 to match the golden output
 sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
 	-e "s/$last_extent/$orig_last_extent/" \
-	-e "s/$end/$orig_end/" $tmp.out
-cat $tmp.out
+	-e "s/$end/$orig_end/" $tmp2.out
+
+cat $tmp1.out > tmp.1
+cat $tmp2.out > tmp.2
+
+diff tmp.[12]
+rm tmp.1
+rm tmp.2
 
 # success, all done
 status=0
diff --git a/tests/generic/352.out b/tests/generic/352.out
index 4ff66c21..ad90ae0d 100644
--- a/tests/generic/352.out
+++ b/tests/generic/352.out
@@ -1,5 +1,3 @@
 QA output created by 352
 wrote 131072/131072 bytes at offset 0
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-0: [0..2097151]: shared
-1: [2097152..2097407]: shared|last
-- 
2.41.0

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Darrick J. Wong]

On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> xfs_io pwrite issues a series of block size writes, but there is no guarantee
> that the resulting extent(s) will be singular or contiguous. This behavior is
> acceptable, but the test is flawed in that it expects a single extent for a
> pwrite.
> 
> Modify test to accept any layout for the reflinked logical range.
> 
> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> ---
>  tests/generic/352     | 16 +++++++++++-----
>  tests/generic/352.out |  2 --
>  2 files changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/generic/352 b/tests/generic/352
> index 52ec4850..c4ee8a44 100755
> --- a/tests/generic/352
> +++ b/tests/generic/352
> @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
>  # use reflink to create the rest of the file, whose all extents are all
>  # pointing to the first extent
>  for i in $(seq 1 $nr); do
> -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out

$tmp1 isnt defined anywhere.

>  done
>  
>  # then call fiemap on that file to test both the shared flag and if
>  # reserved extent mapping search will cause soft lockup
> -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> -cat $tmp.out >> $seqres.full
> +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> +cat $tmp2.out >> $seqres.full

Nor is $tmp2

>  
>  # refact the $LOAD_FACTOR to 1 to match the golden output
>  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
>  	-e "s/$last_extent/$orig_last_extent/" \
> -	-e "s/$end/$orig_end/" $tmp.out
> -cat $tmp.out
> +	-e "s/$end/$orig_end/" $tmp2.out
> +
> +cat $tmp1.out > tmp.1
> +cat $tmp2.out > tmp.2

Not sure why you didn't make the _reflink_range and the fiemap above
output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
default _cleanup would delete $tmp.* automatically...

> +
> +diff tmp.[12]
> +rm tmp.1
> +rm tmp.2

...and the rm here wouldn't be necessary.

Ok.  Nitpicking over.  Moving on to the weirder design questions of the
original test:

[add original test author to cc]

I don't know why $blocksize is set to 128k above.  If this test needs to
guarantee that there would only be *one* extent (and the golden output
implies this as you note), then it should have been written to say:

	blocksize=$(_get_file_block_size $SCRATCH_MNT)

But I don't know if the "btrfs soft lock up and return wrong shared
flag" behavior required sharing a (probably multi-block) 128k range, or
if that was simply what the author selected because it reproduced the
problem.

>  
>  # success, all done
>  status=0
> diff --git a/tests/generic/352.out b/tests/generic/352.out
> index 4ff66c21..ad90ae0d 100644
> --- a/tests/generic/352.out
> +++ b/tests/generic/352.out
> @@ -1,5 +1,3 @@
>  QA output created by 352
>  wrote 131072/131072 bytes at offset 0
>  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -0: [0..2097151]: shared
> -1: [2097152..2097407]: shared|last

Also I suspect from the test description that the goal here was to
detect the golden output failing because the shared flag does not get
reported correctly.

--D
> -- 
> 2.41.0
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > that the resulting extent(s) will be singular or contiguous. This behavior is
> > acceptable, but the test is flawed in that it expects a single extent for a
> > pwrite.
> > 
> > Modify test to accept any layout for the reflinked logical range.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  tests/generic/352     | 16 +++++++++++-----
> >  tests/generic/352.out |  2 --
> >  2 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/generic/352 b/tests/generic/352
> > index 52ec4850..c4ee8a44 100755
> > --- a/tests/generic/352
> > +++ b/tests/generic/352
> > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> >  # use reflink to create the rest of the file, whose all extents are all
> >  # pointing to the first extent
> >  for i in $(seq 1 $nr); do
> > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> 
> $tmp1 isnt defined anywhere.
> 
> >  done
> >  
> >  # then call fiemap on that file to test both the shared flag and if
> >  # reserved extent mapping search will cause soft lockup
> > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > -cat $tmp.out >> $seqres.full
> > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > +cat $tmp2.out >> $seqres.full
> 
> Nor is $tmp2

Do $tmp1 and $tmp2 actually need to be declared?

> 
> >  
> >  # refact the $LOAD_FACTOR to 1 to match the golden output
> >  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> >  	-e "s/$last_extent/$orig_last_extent/" \
> > -	-e "s/$end/$orig_end/" $tmp.out
> > -cat $tmp.out
> > +	-e "s/$end/$orig_end/" $tmp2.out
> > +
> > +cat $tmp1.out > tmp.1
> > +cat $tmp2.out > tmp.2
> 
> Not sure why you didn't make the _reflink_range and the fiemap above
> output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> default _cleanup would delete $tmp.* automatically...
> 
> > +
> > +diff tmp.[12]
> > +rm tmp.1
> > +rm tmp.2
> 
> ...and the rm here wouldn't be necessary.

But how to [diff $tmp.out1 $tmp.out2]? I don't see that working.

> 
> Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> original test:
> 
> [add original test author to cc]
> 
> I don't know why $blocksize is set to 128k above.  If this test needs to
> guarantee that there would only be *one* extent (and the golden output
> implies this as you note), then it should have been written to say:
> 
> 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> 
> But I don't know if the "btrfs soft lock up and return wrong shared
> flag" behavior required sharing a (probably multi-block) 128k range, or
> if that was simply what the author selected because it reproduced the
> problem.
> 
> >  
> >  # success, all done
> >  status=0
> > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > index 4ff66c21..ad90ae0d 100644
> > --- a/tests/generic/352.out
> > +++ b/tests/generic/352.out
> > @@ -1,5 +1,3 @@
> >  QA output created by 352
> >  wrote 131072/131072 bytes at offset 0
> >  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > -0: [0..2097151]: shared
> > -1: [2097152..2097407]: shared|last
> 
> Also I suspect from the test description that the goal here was to
> detect the golden output failing because the shared flag does not get
> reported correctly.

That's my interpretation as well.

Thanks-
Bill


> 
> --D
> > -- 
> > 2.41.0
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Wed, Aug 23, 2023 at 02:55:50PM -0500, Bill O'Donnell wrote:
> On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> > On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > > that the resulting extent(s) will be singular or contiguous. This behavior is
> > > acceptable, but the test is flawed in that it expects a single extent for a
> > > pwrite.
> > > 
> > > Modify test to accept any layout for the reflinked logical range.
> > > 
> > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > > ---
> > >  tests/generic/352     | 16 +++++++++++-----
> > >  tests/generic/352.out |  2 --
> > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tests/generic/352 b/tests/generic/352
> > > index 52ec4850..c4ee8a44 100755
> > > --- a/tests/generic/352
> > > +++ b/tests/generic/352
> > > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> > >  # use reflink to create the rest of the file, whose all extents are all
> > >  # pointing to the first extent
> > >  for i in $(seq 1 $nr); do
> > > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> > 
> > $tmp1 isnt defined anywhere.
> > 
> > >  done
> > >  
> > >  # then call fiemap on that file to test both the shared flag and if
> > >  # reserved extent mapping search will cause soft lockup
> > > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > > -cat $tmp.out >> $seqres.full
> > > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > > +cat $tmp2.out >> $seqres.full
> > 
> > Nor is $tmp2
> 
> Do $tmp1 and $tmp2 actually need to be declared?
> 
> > 
> > >  
> > >  # refact the $LOAD_FACTOR to 1 to match the golden output
> > >  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> > >  	-e "s/$last_extent/$orig_last_extent/" \
> > > -	-e "s/$end/$orig_end/" $tmp.out
> > > -cat $tmp.out
> > > +	-e "s/$end/$orig_end/" $tmp2.out
> > > +
> > > +cat $tmp1.out > tmp.1
> > > +cat $tmp2.out > tmp.2
> > 
> > Not sure why you didn't make the _reflink_range and the fiemap above
> > output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> > default _cleanup would delete $tmp.* automatically...
> > 
> > > +
> > > +diff tmp.[12]
> > > +rm tmp.1
> > > +rm tmp.2
> > 
> > ...and the rm here wouldn't be necessary.
> 
> But how to [diff $tmp.out1 $tmp.out2]? I don't see that working.

Oh, I suppose this would work:
diff <(cat $tmp.out1) <(cat $tmp.out2)

> 
> > 
> > Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> > original test:
> > 
> > [add original test author to cc]
> > 
> > I don't know why $blocksize is set to 128k above.  If this test needs to
> > guarantee that there would only be *one* extent (and the golden output
> > implies this as you note), then it should have been written to say:
> > 
> > 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> > 
> > But I don't know if the "btrfs soft lock up and return wrong shared
> > flag" behavior required sharing a (probably multi-block) 128k range, or
> > if that was simply what the author selected because it reproduced the
> > problem.
> > 
> > >  
> > >  # success, all done
> > >  status=0
> > > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > > index 4ff66c21..ad90ae0d 100644
> > > --- a/tests/generic/352.out
> > > +++ b/tests/generic/352.out
> > > @@ -1,5 +1,3 @@
> > >  QA output created by 352
> > >  wrote 131072/131072 bytes at offset 0
> > >  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > -0: [0..2097151]: shared
> > > -1: [2097152..2097407]: shared|last
> > 
> > Also I suspect from the test description that the goal here was to
> > detect the golden output failing because the shared flag does not get
> > reported correctly.
> 
> That's my interpretation as well.
> 
> Thanks-
> Bill
> 
> 
> > 
> > --D
> > > -- 
> > > 2.41.0
> > > 
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Wed, Aug 23, 2023 at 03:42:10PM -0500, Bill O'Donnell wrote:
> On Wed, Aug 23, 2023 at 02:55:50PM -0500, Bill O'Donnell wrote:
> > On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> > > On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > > > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > > > that the resulting extent(s) will be singular or contiguous. This behavior is
> > > > acceptable, but the test is flawed in that it expects a single extent for a
> > > > pwrite.
> > > > 
> > > > Modify test to accept any layout for the reflinked logical range.
> > > > 
> > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > > > ---
> > > >  tests/generic/352     | 16 +++++++++++-----
> > > >  tests/generic/352.out |  2 --
> > > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/tests/generic/352 b/tests/generic/352
> > > > index 52ec4850..c4ee8a44 100755
> > > > --- a/tests/generic/352
> > > > +++ b/tests/generic/352
> > > > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> > > >  # use reflink to create the rest of the file, whose all extents are all
> > > >  # pointing to the first extent
> > > >  for i in $(seq 1 $nr); do
> > > > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > > > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> > > 
> > > $tmp1 isnt defined anywhere.
> > > 
> > > >  done
> > > >  
> > > >  # then call fiemap on that file to test both the shared flag and if
> > > >  # reserved extent mapping search will cause soft lockup
> > > > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > > > -cat $tmp.out >> $seqres.full
> > > > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > > > +cat $tmp2.out >> $seqres.full
> > > 
> > > Nor is $tmp2
> > 
> > Do $tmp1 and $tmp2 actually need to be declared?
> > 
> > > 
> > > >  
> > > >  # refact the $LOAD_FACTOR to 1 to match the golden output
> > > >  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> > > >  	-e "s/$last_extent/$orig_last_extent/" \
> > > > -	-e "s/$end/$orig_end/" $tmp.out
> > > > -cat $tmp.out
> > > > +	-e "s/$end/$orig_end/" $tmp2.out
> > > > +
> > > > +cat $tmp1.out > tmp.1
> > > > +cat $tmp2.out > tmp.2
> > > 
> > > Not sure why you didn't make the _reflink_range and the fiemap above
> > > output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> > > default _cleanup would delete $tmp.* automatically...
> > > 
> > > > +
> > > > +diff tmp.[12]
> > > > +rm tmp.1
> > > > +rm tmp.2
> > > 
> > > ...and the rm here wouldn't be necessary.
> > 
> > But how to [diff $tmp.out1 $tmp.out2]? I don't see that working.
> 
> Oh, I suppose this would work:
> diff <(cat $tmp.out1) <(cat $tmp.out2)

Wrong :/

> 
> > 
> > > 
> > > Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> > > original test:
> > > 
> > > [add original test author to cc]
> > > 
> > > I don't know why $blocksize is set to 128k above.  If this test needs to
> > > guarantee that there would only be *one* extent (and the golden output
> > > implies this as you note), then it should have been written to say:
> > > 
> > > 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> > > 
> > > But I don't know if the "btrfs soft lock up and return wrong shared
> > > flag" behavior required sharing a (probably multi-block) 128k range, or
> > > if that was simply what the author selected because it reproduced the
> > > problem.
> > > 
> > > >  
> > > >  # success, all done
> > > >  status=0
> > > > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > > > index 4ff66c21..ad90ae0d 100644
> > > > --- a/tests/generic/352.out
> > > > +++ b/tests/generic/352.out
> > > > @@ -1,5 +1,3 @@
> > > >  QA output created by 352
> > > >  wrote 131072/131072 bytes at offset 0
> > > >  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > > -0: [0..2097151]: shared
> > > > -1: [2097152..2097407]: shared|last
> > > 
> > > Also I suspect from the test description that the goal here was to
> > > detect the golden output failing because the shared flag does not get
> > > reported correctly.
> > 
> > That's my interpretation as well.
> > 
> > Thanks-
> > Bill
> > 
> > 
> > > 
> > > --D
> > > > -- 
> > > > 2.41.0
> > > > 
> > > 
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > that the resulting extent(s) will be singular or contiguous. This behavior is
> > acceptable, but the test is flawed in that it expects a single extent for a
> > pwrite.
> > 
> > Modify test to accept any layout for the reflinked logical range.
> > 
> > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > ---
> >  tests/generic/352     | 16 +++++++++++-----
> >  tests/generic/352.out |  2 --
> >  2 files changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/generic/352 b/tests/generic/352
> > index 52ec4850..c4ee8a44 100755
> > --- a/tests/generic/352
> > +++ b/tests/generic/352
> > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> >  # use reflink to create the rest of the file, whose all extents are all
> >  # pointing to the first extent
> >  for i in $(seq 1 $nr); do
> > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> 
> $tmp1 isnt defined anywhere.
> 
> >  done
> >  
> >  # then call fiemap on that file to test both the shared flag and if
> >  # reserved extent mapping search will cause soft lockup
> > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > -cat $tmp.out >> $seqres.full
> > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > +cat $tmp2.out >> $seqres.full
> 
> Nor is $tmp2
> 
> >  
> >  # refact the $LOAD_FACTOR to 1 to match the golden output
> >  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> >  	-e "s/$last_extent/$orig_last_extent/" \
> > -	-e "s/$end/$orig_end/" $tmp.out
> > -cat $tmp.out
> > +	-e "s/$end/$orig_end/" $tmp2.out
> > +
> > +cat $tmp1.out > tmp.1
> > +cat $tmp2.out > tmp.2
> 
> Not sure why you didn't make the _reflink_range and the fiemap above
> output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> default _cleanup would delete $tmp.* automatically...
> 
> > +
> > +diff tmp.[12]
> > +rm tmp.1
> > +rm tmp.2
> 
> ...and the rm here wouldn't be necessary.
> 
> Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> original test:
> 
> [add original test author to cc]

Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
would know what btrfs intent was?

-Bill


> 
> I don't know why $blocksize is set to 128k above.  If this test needs to
> guarantee that there would only be *one* extent (and the golden output
> implies this as you note), then it should have been written to say:
> 
> 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> 
> But I don't know if the "btrfs soft lock up and return wrong shared
> flag" behavior required sharing a (probably multi-block) 128k range, or
> if that was simply what the author selected because it reproduced the
> problem.
> 
> >  
> >  # success, all done
> >  status=0
> > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > index 4ff66c21..ad90ae0d 100644
> > --- a/tests/generic/352.out
> > +++ b/tests/generic/352.out
> > @@ -1,5 +1,3 @@
> >  QA output created by 352
> >  wrote 131072/131072 bytes at offset 0
> >  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > -0: [0..2097151]: shared
> > -1: [2097152..2097407]: shared|last
> 
> Also I suspect from the test description that the goal here was to
> detect the golden output failing because the shared flag does not get
> reported correctly.
> 
> --D
> > -- 
> > 2.41.0
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Darrick J. Wong]

On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
> On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> > On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > > that the resulting extent(s) will be singular or contiguous. This behavior is
> > > acceptable, but the test is flawed in that it expects a single extent for a
> > > pwrite.
> > > 
> > > Modify test to accept any layout for the reflinked logical range.
> > > 
> > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > > ---
> > >  tests/generic/352     | 16 +++++++++++-----
> > >  tests/generic/352.out |  2 --
> > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/tests/generic/352 b/tests/generic/352
> > > index 52ec4850..c4ee8a44 100755
> > > --- a/tests/generic/352
> > > +++ b/tests/generic/352
> > > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> > >  # use reflink to create the rest of the file, whose all extents are all
> > >  # pointing to the first extent
> > >  for i in $(seq 1 $nr); do
> > > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> > 
> > $tmp1 isnt defined anywhere.
> > 
> > >  done
> > >  
> > >  # then call fiemap on that file to test both the shared flag and if
> > >  # reserved extent mapping search will cause soft lockup
> > > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > > -cat $tmp.out >> $seqres.full
> > > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > > +cat $tmp2.out >> $seqres.full
> > 
> > Nor is $tmp2
> > 
> > >  
> > >  # refact the $LOAD_FACTOR to 1 to match the golden output
> > >  sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> > >  	-e "s/$last_extent/$orig_last_extent/" \
> > > -	-e "s/$end/$orig_end/" $tmp.out
> > > -cat $tmp.out
> > > +	-e "s/$end/$orig_end/" $tmp2.out
> > > +
> > > +cat $tmp1.out > tmp.1
> > > +cat $tmp2.out > tmp.2
> > 
> > Not sure why you didn't make the _reflink_range and the fiemap above
> > output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> > default _cleanup would delete $tmp.* automatically...
> > 
> > > +
> > > +diff tmp.[12]
> > > +rm tmp.1
> > > +rm tmp.2
> > 
> > ...and the rm here wouldn't be necessary.
> > 
> > Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> > original test:
> > 
> > [add original test author to cc]
> 
> Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
> would know what btrfs intent was?

...or I guess I could have used the current email addr instead of the
one on the commit. :(

Qu: Question for you:

> > I don't know why $blocksize is set to 128k above.  If this test needs to
> > guarantee that there would only be *one* extent (and the golden output
> > implies this as you note), then it should have been written to say:
> > 
> > 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> > 
> > But I don't know if the "btrfs soft lock up and return wrong shared
> > flag" behavior required sharing a (probably multi-block) 128k range, or
> > if that was simply what the author selected because it reproduced the
> > problem.

Any thoughts?

--D

> > 
> > >  
> > >  # success, all done
> > >  status=0
> > > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > > index 4ff66c21..ad90ae0d 100644
> > > --- a/tests/generic/352.out
> > > +++ b/tests/generic/352.out
> > > @@ -1,5 +1,3 @@
> > >  QA output created by 352
> > >  wrote 131072/131072 bytes at offset 0
> > >  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > -0: [0..2097151]: shared
> > > -1: [2097152..2097407]: shared|last
> > 
> > Also I suspect from the test description that the goal here was to
> > detect the golden output failing because the shared flag does not get
> > reported correctly.
> > 
> > --D
> > > -- 
> > > 2.41.0
> > > 
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Qu Wenruo]

On 2023/8/24 06:27, Darrick J. Wong wrote:
> On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
>> On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
>>> On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
>>>> xfs_io pwrite issues a series of block size writes, but there is no guarantee
>>>> that the resulting extent(s) will be singular or contiguous.

However this doesn't make much difference, at least for btrfs.

Btrfs would do the merging emitting the fiemap entry, thus even if the
write didn't result a singular extent, as long as they are contiguous
(under most cases they are) the fiemap result would still be a single one.

>>>> This behavior is
>>>> acceptable, but the test is flawed in that it expects a single extent for a
>>>> pwrite.

I'm more interested in if you're hitting any test failure?

>>>>
>>>> Modify test to accept any layout for the reflinked logical range.
>>>>
>>>> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
>>>> ---
>>>>   tests/generic/352     | 16 +++++++++++-----
>>>>   tests/generic/352.out |  2 --
>>>>   2 files changed, 11 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/tests/generic/352 b/tests/generic/352
>>>> index 52ec4850..c4ee8a44 100755
>>>> --- a/tests/generic/352
>>>> +++ b/tests/generic/352
>>>> @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
>>>>   # use reflink to create the rest of the file, whose all extents are all
>>>>   # pointing to the first extent
>>>>   for i in $(seq 1 $nr); do
>>>> -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
>>>> +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
>>>
>>> $tmp1 isnt defined anywhere.
>>>
>>>>   done
>>>>
>>>>   # then call fiemap on that file to test both the shared flag and if
>>>>   # reserved extent mapping search will cause soft lockup
>>>> -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
>>>> -cat $tmp.out >> $seqres.full
>>>> +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
>>>> +cat $tmp2.out >> $seqres.full
>>>
>>> Nor is $tmp2
>>>
>>>>
>>>>   # refact the $LOAD_FACTOR to 1 to match the golden output
>>>>   sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
>>>>   	-e "s/$last_extent/$orig_last_extent/" \
>>>> -	-e "s/$end/$orig_end/" $tmp.out
>>>> -cat $tmp.out
>>>> +	-e "s/$end/$orig_end/" $tmp2.out
>>>> +
>>>> +cat $tmp1.out > tmp.1
>>>> +cat $tmp2.out > tmp.2
>>>
>>> Not sure why you didn't make the _reflink_range and the fiemap above
>>> output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
>>> default _cleanup would delete $tmp.* automatically...
>>>
>>>> +
>>>> +diff tmp.[12]
>>>> +rm tmp.1
>>>> +rm tmp.2
>>>
>>> ...and the rm here wouldn't be necessary.
>>>
>>> Ok.  Nitpicking over.  Moving on to the weirder design questions of the
>>> original test:
>>>
>>> [add original test author to cc]
>>
>> Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
>> would know what btrfs intent was?
>
> ...or I guess I could have used the current email addr instead of the
> one on the commit. :(
>
> Qu: Question for you:

Thanks a lot for referring it to me.

>
>>> I don't know why $blocksize is set to 128k above.  If this test needs to
>>> guarantee that there would only be *one* extent (and the golden output
>>> implies this as you note), then it should have been written to say:
>>>
>>> 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
>>>
>>> But I don't know if the "btrfs soft lock up and return wrong shared
>>> flag" behavior required sharing a (probably multi-block) 128k range, or
>>> if that was simply what the author selected because it reproduced the
>>> problem.

It's quite sometime ago, thus my memory may not be reliable, but IIRC
the blocksize has no specific requirement other than allowing all
possible blocksize (4K to 64K).

And at that time, at least I was preferring to use golden output to
detect errors, thus I choose a larger blocksize to allow all blocksizes
to work.

>
> Any thoughts?
>
> --D
>
>>>
>>>>
>>>>   # success, all done
>>>>   status=0
>>>> diff --git a/tests/generic/352.out b/tests/generic/352.out
>>>> index 4ff66c21..ad90ae0d 100644
>>>> --- a/tests/generic/352.out
>>>> +++ b/tests/generic/352.out
>>>> @@ -1,5 +1,3 @@
>>>>   QA output created by 352
>>>>   wrote 131072/131072 bytes at offset 0
>>>>   XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>> -0: [0..2097151]: shared
>>>> -1: [2097152..2097407]: shared|last
>>>
>>> Also I suspect from the test description that the goal here was to
>>> detect the golden output failing because the shared flag does not get
>>> reported correctly.

Could explain more on why the shared flag detection is not correct here?

If a file extent is shared, no matter if it's shared by another inode or
not, shouldn't it be marked with SHARED flag?

Thanks,
Qu

>>>
>>> --D
>>>> --
>>>> 2.41.0
>>>>
>>>
>>

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Thu, Aug 24, 2023 at 08:03:23AM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/8/24 06:27, Darrick J. Wong wrote:
> > On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
> > > On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> > > > On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > > > > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > > > > that the resulting extent(s) will be singular or contiguous.
> 
> However this doesn't make much difference, at least for btrfs.
> 
> Btrfs would do the merging emitting the fiemap entry, thus even if the
> write didn't result a singular extent, as long as they are contiguous
> (under most cases they are) the fiemap result would still be a single one.
> 
> > > > > This behavior is
> > > > > acceptable, but the test is flawed in that it expects a single extent for a
> > > > > pwrite.
> 
> I'm more interested in if you're hitting any test failure?

Yes we are.

thanks-
Bill


> 
> > > > > 
> > > > > Modify test to accept any layout for the reflinked logical range.
> > > > > 
> > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > > > > ---
> > > > >   tests/generic/352     | 16 +++++++++++-----
> > > > >   tests/generic/352.out |  2 --
> > > > >   2 files changed, 11 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/tests/generic/352 b/tests/generic/352
> > > > > index 52ec4850..c4ee8a44 100755
> > > > > --- a/tests/generic/352
> > > > > +++ b/tests/generic/352
> > > > > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> > > > >   # use reflink to create the rest of the file, whose all extents are all
> > > > >   # pointing to the first extent
> > > > >   for i in $(seq 1 $nr); do
> > > > > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > > > > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> > > > 
> > > > $tmp1 isnt defined anywhere.
> > > > 
> > > > >   done
> > > > > 
> > > > >   # then call fiemap on that file to test both the shared flag and if
> > > > >   # reserved extent mapping search will cause soft lockup
> > > > > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > > > > -cat $tmp.out >> $seqres.full
> > > > > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > > > > +cat $tmp2.out >> $seqres.full
> > > > 
> > > > Nor is $tmp2
> > > > 
> > > > > 
> > > > >   # refact the $LOAD_FACTOR to 1 to match the golden output
> > > > >   sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> > > > >   	-e "s/$last_extent/$orig_last_extent/" \
> > > > > -	-e "s/$end/$orig_end/" $tmp.out
> > > > > -cat $tmp.out
> > > > > +	-e "s/$end/$orig_end/" $tmp2.out
> > > > > +
> > > > > +cat $tmp1.out > tmp.1
> > > > > +cat $tmp2.out > tmp.2
> > > > 
> > > > Not sure why you didn't make the _reflink_range and the fiemap above
> > > > output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> > > > default _cleanup would delete $tmp.* automatically...
> > > > 
> > > > > +
> > > > > +diff tmp.[12]
> > > > > +rm tmp.1
> > > > > +rm tmp.2
> > > > 
> > > > ...and the rm here wouldn't be necessary.
> > > > 
> > > > Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> > > > original test:
> > > > 
> > > > [add original test author to cc]
> > > 
> > > Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
> > > would know what btrfs intent was?
> > 
> > ...or I guess I could have used the current email addr instead of the
> > one on the commit. :(
> > 
> > Qu: Question for you:
> 
> Thanks a lot for referring it to me.
> 
> > 
> > > > I don't know why $blocksize is set to 128k above.  If this test needs to
> > > > guarantee that there would only be *one* extent (and the golden output
> > > > implies this as you note), then it should have been written to say:
> > > > 
> > > > 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> > > > 
> > > > But I don't know if the "btrfs soft lock up and return wrong shared
> > > > flag" behavior required sharing a (probably multi-block) 128k range, or
> > > > if that was simply what the author selected because it reproduced the
> > > > problem.
> 
> It's quite sometime ago, thus my memory may not be reliable, but IIRC
> the blocksize has no specific requirement other than allowing all
> possible blocksize (4K to 64K).
> 
> And at that time, at least I was preferring to use golden output to
> detect errors, thus I choose a larger blocksize to allow all blocksizes
> to work.
> 
> > 
> > Any thoughts?
> > 
> > --D
> > 
> > > > 
> > > > > 
> > > > >   # success, all done
> > > > >   status=0
> > > > > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > > > > index 4ff66c21..ad90ae0d 100644
> > > > > --- a/tests/generic/352.out
> > > > > +++ b/tests/generic/352.out
> > > > > @@ -1,5 +1,3 @@
> > > > >   QA output created by 352
> > > > >   wrote 131072/131072 bytes at offset 0
> > > > >   XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > > > -0: [0..2097151]: shared
> > > > > -1: [2097152..2097407]: shared|last
> > > > 
> > > > Also I suspect from the test description that the goal here was to
> > > > detect the golden output failing because the shared flag does not get
> > > > reported correctly.
> 
> Could explain more on why the shared flag detection is not correct here?
> 
> If a file extent is shared, no matter if it's shared by another inode or
> not, shouldn't it be marked with SHARED flag?
> 
> Thanks,
> Qu
> 
> > > > 
> > > > --D
> > > > > --
> > > > > 2.41.0
> > > > > 
> > > > 
> > > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Qu Wenruo]

On 2023/8/24 11:38, Bill O'Donnell wrote:
> On Thu, Aug 24, 2023 at 08:03:23AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2023/8/24 06:27, Darrick J. Wong wrote:
>>> On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
>>>> On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
>>>>> On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
>>>>>> xfs_io pwrite issues a series of block size writes, but there is no guarantee
>>>>>> that the resulting extent(s) will be singular or contiguous.
>>
>> However this doesn't make much difference, at least for btrfs.
>>
>> Btrfs would do the merging emitting the fiemap entry, thus even if the
>> write didn't result a singular extent, as long as they are contiguous
>> (under most cases they are) the fiemap result would still be a single one.
>>
>>>>>> This behavior is
>>>>>> acceptable, but the test is flawed in that it expects a single extent for a
>>>>>> pwrite.
>>
>> I'm more interested in if you're hitting any test failure?
>
> Yes we are.

I guess it's using XFS?

Then the idea of using proper block size looks good to me.

And since we're getting rid of the golden output, it's also a good
timing to get rid of the refactoring to match the golden output completely.

Thanks,
Qu
>
> thanks-
> Bill
>
>
>>
>>>>>>
>>>>>> Modify test to accept any layout for the reflinked logical range.
>>>>>>
>>>>>> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
>>>>>> ---
>>>>>>    tests/generic/352     | 16 +++++++++++-----
>>>>>>    tests/generic/352.out |  2 --
>>>>>>    2 files changed, 11 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/generic/352 b/tests/generic/352
>>>>>> index 52ec4850..c4ee8a44 100755
>>>>>> --- a/tests/generic/352
>>>>>> +++ b/tests/generic/352
>>>>>> @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
>>>>>>    # use reflink to create the rest of the file, whose all extents are all
>>>>>>    # pointing to the first extent
>>>>>>    for i in $(seq 1 $nr); do
>>>>>> -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
>>>>>> +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
>>>>>
>>>>> $tmp1 isnt defined anywhere.
>>>>>
>>>>>>    done
>>>>>>
>>>>>>    # then call fiemap on that file to test both the shared flag and if
>>>>>>    # reserved extent mapping search will cause soft lockup
>>>>>> -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
>>>>>> -cat $tmp.out >> $seqres.full
>>>>>> +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
>>>>>> +cat $tmp2.out >> $seqres.full
>>>>>
>>>>> Nor is $tmp2
>>>>>
>>>>>>
>>>>>>    # refact the $LOAD_FACTOR to 1 to match the golden output
>>>>>>    sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
>>>>>>    	-e "s/$last_extent/$orig_last_extent/" \
>>>>>> -	-e "s/$end/$orig_end/" $tmp.out
>>>>>> -cat $tmp.out
>>>>>> +	-e "s/$end/$orig_end/" $tmp2.out
>>>>>> +
>>>>>> +cat $tmp1.out > tmp.1
>>>>>> +cat $tmp2.out > tmp.2
>>>>>
>>>>> Not sure why you didn't make the _reflink_range and the fiemap above
>>>>> output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
>>>>> default _cleanup would delete $tmp.* automatically...
>>>>>
>>>>>> +
>>>>>> +diff tmp.[12]
>>>>>> +rm tmp.1
>>>>>> +rm tmp.2
>>>>>
>>>>> ...and the rm here wouldn't be necessary.
>>>>>
>>>>> Ok.  Nitpicking over.  Moving on to the weirder design questions of the
>>>>> original test:
>>>>>
>>>>> [add original test author to cc]
>>>>
>>>> Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
>>>> would know what btrfs intent was?
>>>
>>> ...or I guess I could have used the current email addr instead of the
>>> one on the commit. :(
>>>
>>> Qu: Question for you:
>>
>> Thanks a lot for referring it to me.
>>
>>>
>>>>> I don't know why $blocksize is set to 128k above.  If this test needs to
>>>>> guarantee that there would only be *one* extent (and the golden output
>>>>> implies this as you note), then it should have been written to say:
>>>>>
>>>>> 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
>>>>>
>>>>> But I don't know if the "btrfs soft lock up and return wrong shared
>>>>> flag" behavior required sharing a (probably multi-block) 128k range, or
>>>>> if that was simply what the author selected because it reproduced the
>>>>> problem.
>>
>> It's quite sometime ago, thus my memory may not be reliable, but IIRC
>> the blocksize has no specific requirement other than allowing all
>> possible blocksize (4K to 64K).
>>
>> And at that time, at least I was preferring to use golden output to
>> detect errors, thus I choose a larger blocksize to allow all blocksizes
>> to work.
>>
>>>
>>> Any thoughts?
>>>
>>> --D
>>>
>>>>>
>>>>>>
>>>>>>    # success, all done
>>>>>>    status=0
>>>>>> diff --git a/tests/generic/352.out b/tests/generic/352.out
>>>>>> index 4ff66c21..ad90ae0d 100644
>>>>>> --- a/tests/generic/352.out
>>>>>> +++ b/tests/generic/352.out
>>>>>> @@ -1,5 +1,3 @@
>>>>>>    QA output created by 352
>>>>>>    wrote 131072/131072 bytes at offset 0
>>>>>>    XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>>> -0: [0..2097151]: shared
>>>>>> -1: [2097152..2097407]: shared|last
>>>>>
>>>>> Also I suspect from the test description that the goal here was to
>>>>> detect the golden output failing because the shared flag does not get
>>>>> reported correctly.
>>
>> Could explain more on why the shared flag detection is not correct here?
>>
>> If a file extent is shared, no matter if it's shared by another inode or
>> not, shouldn't it be marked with SHARED flag?
>>
>> Thanks,
>> Qu
>>
>>>>>
>>>>> --D
>>>>>> --
>>>>>> 2.41.0
>>>>>>
>>>>>
>>>>
>>
>

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Bill O'Donnell]

On Thu, Aug 24, 2023 at 02:46:10PM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/8/24 11:38, Bill O'Donnell wrote:
> > On Thu, Aug 24, 2023 at 08:03:23AM +0800, Qu Wenruo wrote:
> > > 
> > > 
> > > On 2023/8/24 06:27, Darrick J. Wong wrote:
> > > > On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
> > > > > On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
> > > > > > On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
> > > > > > > xfs_io pwrite issues a series of block size writes, but there is no guarantee
> > > > > > > that the resulting extent(s) will be singular or contiguous.
> > > 
> > > However this doesn't make much difference, at least for btrfs.
> > > 
> > > Btrfs would do the merging emitting the fiemap entry, thus even if the
> > > write didn't result a singular extent, as long as they are contiguous
> > > (under most cases they are) the fiemap result would still be a single one.
> > > 
> > > > > > > This behavior is
> > > > > > > acceptable, but the test is flawed in that it expects a single extent for a
> > > > > > > pwrite.
> > > 
> > > I'm more interested in if you're hitting any test failure?
> > 
> > Yes we are.
> 
> I guess it's using XFS?

Yes. The problem begins with the initial pwrite, with unpredictable mapping.
It's happening under the following configuration:

export TEST_DEV=/dev/pmem0
export TEST_DIR=/mnt/test
export SCRATCH_DEV=/dev/pmem0.1
export SCRATCH_MNT=/mnt/scratch
export MKFS_OPTIONS="-m reflink=1 -m rmapbt=0 -b size=4096"
export MOUNT_OPTIONS="-o dax=always"
export TEST_FS_MOUNT_OPTS="-o dax=always"

-Bill

> 
> Then the idea of using proper block size looks good to me.
> 
> And since we're getting rid of the golden output, it's also a good
> timing to get rid of the refactoring to match the golden output completely.
> 
> Thanks,
> Qu
> > 
> > thanks-
> > Bill
> > 
> > 
> > > 
> > > > > > > 
> > > > > > > Modify test to accept any layout for the reflinked logical range.
> > > > > > > 
> > > > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
> > > > > > > ---
> > > > > > >    tests/generic/352     | 16 +++++++++++-----
> > > > > > >    tests/generic/352.out |  2 --
> > > > > > >    2 files changed, 11 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/generic/352 b/tests/generic/352
> > > > > > > index 52ec4850..c4ee8a44 100755
> > > > > > > --- a/tests/generic/352
> > > > > > > +++ b/tests/generic/352
> > > > > > > @@ -48,19 +48,25 @@ _pwrite_byte 0xcdcdcdcd 0 $blocksize $file | _filter_xfs_io
> > > > > > >    # use reflink to create the rest of the file, whose all extents are all
> > > > > > >    # pointing to the first extent
> > > > > > >    for i in $(seq 1 $nr); do
> > > > > > > -	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > /dev/null
> > > > > > > +	_reflink_range $file 0 $file $(($i * $blocksize)) $blocksize > $tmp1.out
> > > > > > 
> > > > > > $tmp1 isnt defined anywhere.
> > > > > > 
> > > > > > >    done
> > > > > > > 
> > > > > > >    # then call fiemap on that file to test both the shared flag and if
> > > > > > >    # reserved extent mapping search will cause soft lockup
> > > > > > > -$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp.out
> > > > > > > -cat $tmp.out >> $seqres.full
> > > > > > > +$XFS_IO_PROG -c "fiemap -v" $file | _filter_fiemap_flags > $tmp2.out
> > > > > > > +cat $tmp2.out >> $seqres.full
> > > > > > 
> > > > > > Nor is $tmp2
> > > > > > 
> > > > > > > 
> > > > > > >    # refact the $LOAD_FACTOR to 1 to match the golden output
> > > > > > >    sed -i -e "s/$(($last_extent - 1))/$(($orig_last_extent - 1))/" \
> > > > > > >    	-e "s/$last_extent/$orig_last_extent/" \
> > > > > > > -	-e "s/$end/$orig_end/" $tmp.out
> > > > > > > -cat $tmp.out
> > > > > > > +	-e "s/$end/$orig_end/" $tmp2.out
> > > > > > > +
> > > > > > > +cat $tmp1.out > tmp.1
> > > > > > > +cat $tmp2.out > tmp.2
> > > > > > 
> > > > > > Not sure why you didn't make the _reflink_range and the fiemap above
> > > > > > output to $tmp.out1 and $tmp.out2, respectively.  If you had, then the
> > > > > > default _cleanup would delete $tmp.* automatically...
> > > > > > 
> > > > > > > +
> > > > > > > +diff tmp.[12]
> > > > > > > +rm tmp.1
> > > > > > > +rm tmp.2
> > > > > > 
> > > > > > ...and the rm here wouldn't be necessary.
> > > > > > 
> > > > > > Ok.  Nitpicking over.  Moving on to the weirder design questions of the
> > > > > > original test:
> > > > > > 
> > > > > > [add original test author to cc]
> > > > > 
> > > > > Emails to quwenruo@cn.fujitsu.com seem to be undeliverable. Maybe Joseph
> > > > > would know what btrfs intent was?
> > > > 
> > > > ...or I guess I could have used the current email addr instead of the
> > > > one on the commit. :(
> > > > 
> > > > Qu: Question for you:
> > > 
> > > Thanks a lot for referring it to me.
> > > 
> > > > 
> > > > > > I don't know why $blocksize is set to 128k above.  If this test needs to
> > > > > > guarantee that there would only be *one* extent (and the golden output
> > > > > > implies this as you note), then it should have been written to say:
> > > > > > 
> > > > > > 	blocksize=$(_get_file_block_size $SCRATCH_MNT)
> > > > > > 
> > > > > > But I don't know if the "btrfs soft lock up and return wrong shared
> > > > > > flag" behavior required sharing a (probably multi-block) 128k range, or
> > > > > > if that was simply what the author selected because it reproduced the
> > > > > > problem.
> > > 
> > > It's quite sometime ago, thus my memory may not be reliable, but IIRC
> > > the blocksize has no specific requirement other than allowing all
> > > possible blocksize (4K to 64K).
> > > 
> > > And at that time, at least I was preferring to use golden output to
> > > detect errors, thus I choose a larger blocksize to allow all blocksizes
> > > to work.
> > > 
> > > > 
> > > > Any thoughts?
> > > > 
> > > > --D
> > > > 
> > > > > > 
> > > > > > > 
> > > > > > >    # success, all done
> > > > > > >    status=0
> > > > > > > diff --git a/tests/generic/352.out b/tests/generic/352.out
> > > > > > > index 4ff66c21..ad90ae0d 100644
> > > > > > > --- a/tests/generic/352.out
> > > > > > > +++ b/tests/generic/352.out
> > > > > > > @@ -1,5 +1,3 @@
> > > > > > >    QA output created by 352
> > > > > > >    wrote 131072/131072 bytes at offset 0
> > > > > > >    XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> > > > > > > -0: [0..2097151]: shared
> > > > > > > -1: [2097152..2097407]: shared|last
> > > > > > 
> > > > > > Also I suspect from the test description that the goal here was to
> > > > > > detect the golden output failing because the shared flag does not get
> > > > > > reported correctly.
> > > 
> > > Could explain more on why the shared flag detection is not correct here?
> > > 
> > > If a file extent is shared, no matter if it's shared by another inode or
> > > not, shouldn't it be marked with SHARED flag?
> > > 
> > > Thanks,
> > > Qu
> > > 
> > > > > > 
> > > > > > --D
> > > > > > > --
> > > > > > > 2.41.0
> > > > > > > 
> > > > > > 
> > > > > 
> > > 
> > 
> 

Re: [PATCH] fstests: generic/352 should accomodate other pwrite behaviors

[Eric Sandeen]

On 8/23/23 7:03 PM, Qu Wenruo wrote:
> 
> 
> On 2023/8/24 06:27, Darrick J. Wong wrote:
>> On Wed, Aug 23, 2023 at 05:18:02PM -0500, Bill O'Donnell wrote:
>>> On Wed, Aug 23, 2023 at 09:46:41AM -0700, Darrick J. Wong wrote:
>>>> On Wed, Aug 23, 2023 at 10:43:50AM -0500, Bill O'Donnell wrote:
>>>>> xfs_io pwrite issues a series of block size writes, but there is no
>>>>> guarantee
>>>>> that the resulting extent(s) will be singular or contiguous.
> 
> However this doesn't make much difference, at least for btrfs.
> 
> Btrfs would do the merging emitting the fiemap entry, thus even if the
> write didn't result a singular extent, as long as they are contiguous
> (under most cases they are) the fiemap result would still be a single one.

btrfs may merge physically adjacent extents in fiemap output, but if the
write happened to create physically discontiguous extents for the
logical range for the single pwrite, I think you would run into the same
problem as Bill has described (golden output expects a single extent
from fiemap), yes?

"under most cases" is the problem here, and results in a flakey test. :)

>>>>> This behavior is
>>>>> acceptable, but the test is flawed in that it expects a single
>>>>> extent for a
>>>>> pwrite.
> 
> I'm more interested in if you're hitting any test failure?

As Bill noted, yes - under some circumstances the pwrite on XFS creates
physically discontiguous extents for the range.  (Why it does so is also
an interesting question, for later...)

<snip>

>> Qu: Question for you:
> 
> Thanks a lot for referring it to me.
> 
>>
>>>> I don't know why $blocksize is set to 128k above.  If this test
>>>> needs to
>>>> guarantee that there would only be *one* extent (and the golden output
>>>> implies this as you note), then it should have been written to say:
>>>>
>>>>     blocksize=$(_get_file_block_size $SCRATCH_MNT)
>>>>
>>>> But I don't know if the "btrfs soft lock up and return wrong shared
>>>> flag" behavior required sharing a (probably multi-block) 128k range, or
>>>> if that was simply what the author selected because it reproduced the
>>>> problem.
> 
> It's quite sometime ago, thus my memory may not be reliable, but IIRC
> the blocksize has no specific requirement other than allowing all
> possible blocksize (4K to 64K).
> 
> And at that time, at least I was preferring to use golden output to
> detect errors, thus I choose a larger blocksize to allow all blocksizes
> to work.

Ok, if it was simply "accomodate maximum blocksize" then I like
Darrick's suggestion to simply get the actual filesystem block size, and
use that.

>>
>> Any thoughts?
>>
>> --D
>>
>>>>
>>>>>
>>>>>   # success, all done
>>>>>   status=0
>>>>> diff --git a/tests/generic/352.out b/tests/generic/352.out
>>>>> index 4ff66c21..ad90ae0d 100644
>>>>> --- a/tests/generic/352.out
>>>>> +++ b/tests/generic/352.out
>>>>> @@ -1,5 +1,3 @@
>>>>>   QA output created by 352
>>>>>   wrote 131072/131072 bytes at offset 0
>>>>>   XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>>>> -0: [0..2097151]: shared
>>>>> -1: [2097152..2097407]: shared|last
>>>>
>>>> Also I suspect from the test description that the goal here was to
>>>> detect the golden output failing because the shared flag does not get
>>>> reported correctly.
> 
> Could explain more on why the shared flag detection is not correct here?
> 
> If a file extent is shared, no matter if it's shared by another inode or
> not, shouldn't it be marked with SHARED flag?

I think that what Darrick meant was that Bill's patch removed "shared"
from the golden output, and if the entire point of the test was to
detect proper sharing:

# This test case will check if reserved extent map searching go
# without problem and return correct SHARED flag.
# Which btrfs will soft lock up and return wrong shared flag.

then it was no longer testing as originally designed.

-Eric

> Thanks,
> Qu