* [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors
@ 2023-09-01 16:18 Bill O'Donnell
2023-09-08 13:42 ` Bill O'Donnell
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Bill O'Donnell @ 2023-09-01 16:18 UTC (permalink / raw)
To: fstests; +Cc: zlang, 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 use actual blocksize for pwrite and reflink. Also
modify it to accommodate pwrite and reflink that produce different
mapping results.
Signed-off-by: Bill O'Donnell <bodonnel@redhat.com>
---
tests/generic/353 | 29 ++++++++++++++++-------------
tests/generic/353.out | 15 +--------------
2 files changed, 17 insertions(+), 27 deletions(-)
diff --git a/tests/generic/353 b/tests/generic/353
index 9a1471bd..c5639725 100755
--- a/tests/generic/353
+++ b/tests/generic/353
@@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap"
_scratch_mkfs > /dev/null 2>&1
_scratch_mount
-blocksize=64k
+blocksize=$(_get_file_block_size $SCRATCH_MNT)
+
file1="$SCRATCH_MNT/file1"
file2="$SCRATCH_MNT/file2"
+extmap1="$SCRATCH_MNT/extmap1"
+extmap2="$SCRATCH_MNT/extmap2"
# write the initial file
-_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io
+_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null
# reflink initial file
-_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io
+_reflink_range $file1 0 $file2 0 $blocksize > /dev/null
# check their fiemap to make sure it's correct
-echo "before sync:"
-echo "$file1" | _filter_scratch
-$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags
-echo "$file2" | _filter_scratch
-$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags
+$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1
+$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2
+
+cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync"
# sync and recheck, to make sure the fiemap doesn't change just
# due to sync
sync
-echo "after sync:"
-echo "$file1" | _filter_scratch
-$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags
-echo "$file2" | _filter_scratch
-$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags
+$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1
+$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2
+
+cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync"
+
+echo "Silence is golden"
# success, all done
status=0
diff --git a/tests/generic/353.out b/tests/generic/353.out
index 4f6e0b92..16ba4f1f 100644
--- a/tests/generic/353.out
+++ b/tests/generic/353.out
@@ -1,15 +1,2 @@
QA output created by 353
-wrote 65536/65536 bytes at offset 0
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-linked 65536/65536 bytes at offset 0
-XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-before sync:
-SCRATCH_MNT/file1
-0: [0..127]: shared|last
-SCRATCH_MNT/file2
-0: [0..127]: shared|last
-after sync:
-SCRATCH_MNT/file1
-0: [0..127]: shared|last
-SCRATCH_MNT/file2
-0: [0..127]: shared|last
+Silence is golden
--
2.41.0
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-01 16:18 [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors Bill O'Donnell @ 2023-09-08 13:42 ` Bill O'Donnell 2023-09-21 22:30 ` Bill O'Donnell ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Bill O'Donnell @ 2023-09-08 13:42 UTC (permalink / raw) To: Bill O'Donnell; +Cc: fstests, zlang, quwenruo.btrfs cc quwenruo.btrfs@gmx.com On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > modify it to accommodate pwrite and reflink that produce different > mapping results. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- > tests/generic/353 | 29 ++++++++++++++++------------- > tests/generic/353.out | 15 +-------------- > 2 files changed, 17 insertions(+), 27 deletions(-) > > diff --git a/tests/generic/353 b/tests/generic/353 > index 9a1471bd..c5639725 100755 > --- a/tests/generic/353 > +++ b/tests/generic/353 > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > _scratch_mkfs > /dev/null 2>&1 > _scratch_mount > > -blocksize=64k > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > + > file1="$SCRATCH_MNT/file1" > file2="$SCRATCH_MNT/file2" > +extmap1="$SCRATCH_MNT/extmap1" > +extmap2="$SCRATCH_MNT/extmap2" > > # write the initial file > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > # reflink initial file > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > # check their fiemap to make sure it's correct > -echo "before sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > # sync and recheck, to make sure the fiemap doesn't change just > # due to sync > sync > -echo "after sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > + > +echo "Silence is golden" > > # success, all done > status=0 > diff --git a/tests/generic/353.out b/tests/generic/353.out > index 4f6e0b92..16ba4f1f 100644 > --- a/tests/generic/353.out > +++ b/tests/generic/353.out > @@ -1,15 +1,2 @@ > QA output created by 353 > -wrote 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -linked 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -before sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > -after sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > +Silence is golden > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-01 16:18 [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors Bill O'Donnell 2023-09-08 13:42 ` Bill O'Donnell @ 2023-09-21 22:30 ` Bill O'Donnell 2023-09-25 20:37 ` Bill O'Donnell 2023-09-29 5:06 ` Zorro Lang 3 siblings, 0 replies; 9+ messages in thread From: Bill O'Donnell @ 2023-09-21 22:30 UTC (permalink / raw) To: Bill O'Donnell; +Cc: fstests, zlang Gentle ping. On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > modify it to accommodate pwrite and reflink that produce different > mapping results. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- > tests/generic/353 | 29 ++++++++++++++++------------- > tests/generic/353.out | 15 +-------------- > 2 files changed, 17 insertions(+), 27 deletions(-) > > diff --git a/tests/generic/353 b/tests/generic/353 > index 9a1471bd..c5639725 100755 > --- a/tests/generic/353 > +++ b/tests/generic/353 > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > _scratch_mkfs > /dev/null 2>&1 > _scratch_mount > > -blocksize=64k > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > + > file1="$SCRATCH_MNT/file1" > file2="$SCRATCH_MNT/file2" > +extmap1="$SCRATCH_MNT/extmap1" > +extmap2="$SCRATCH_MNT/extmap2" > > # write the initial file > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > # reflink initial file > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > # check their fiemap to make sure it's correct > -echo "before sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > # sync and recheck, to make sure the fiemap doesn't change just > # due to sync > sync > -echo "after sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > + > +echo "Silence is golden" > > # success, all done > status=0 > diff --git a/tests/generic/353.out b/tests/generic/353.out > index 4f6e0b92..16ba4f1f 100644 > --- a/tests/generic/353.out > +++ b/tests/generic/353.out > @@ -1,15 +1,2 @@ > QA output created by 353 > -wrote 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -linked 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -before sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > -after sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > +Silence is golden > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-01 16:18 [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors Bill O'Donnell 2023-09-08 13:42 ` Bill O'Donnell 2023-09-21 22:30 ` Bill O'Donnell @ 2023-09-25 20:37 ` Bill O'Donnell 2023-09-29 5:06 ` Zorro Lang 3 siblings, 0 replies; 9+ messages in thread From: Bill O'Donnell @ 2023-09-25 20:37 UTC (permalink / raw) To: fstests; +Cc: zlang, quwenruo.btrfs, esandeen widen cc-list On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > modify it to accommodate pwrite and reflink that produce different > mapping results. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- > tests/generic/353 | 29 ++++++++++++++++------------- > tests/generic/353.out | 15 +-------------- > 2 files changed, 17 insertions(+), 27 deletions(-) > > diff --git a/tests/generic/353 b/tests/generic/353 > index 9a1471bd..c5639725 100755 > --- a/tests/generic/353 > +++ b/tests/generic/353 > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > _scratch_mkfs > /dev/null 2>&1 > _scratch_mount > > -blocksize=64k > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > + > file1="$SCRATCH_MNT/file1" > file2="$SCRATCH_MNT/file2" > +extmap1="$SCRATCH_MNT/extmap1" > +extmap2="$SCRATCH_MNT/extmap2" > > # write the initial file > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > # reflink initial file > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > # check their fiemap to make sure it's correct > -echo "before sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > # sync and recheck, to make sure the fiemap doesn't change just > # due to sync > sync > -echo "after sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > + > +echo "Silence is golden" > > # success, all done > status=0 > diff --git a/tests/generic/353.out b/tests/generic/353.out > index 4f6e0b92..16ba4f1f 100644 > --- a/tests/generic/353.out > +++ b/tests/generic/353.out > @@ -1,15 +1,2 @@ > QA output created by 353 > -wrote 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -linked 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -before sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > -after sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > +Silence is golden > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-01 16:18 [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors Bill O'Donnell ` (2 preceding siblings ...) 2023-09-25 20:37 ` Bill O'Donnell @ 2023-09-29 5:06 ` Zorro Lang 2023-09-29 13:42 ` Bill O'Donnell 3 siblings, 1 reply; 9+ messages in thread From: Zorro Lang @ 2023-09-29 5:06 UTC (permalink / raw) To: Bill O'Donnell; +Cc: fstests On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > modify it to accommodate pwrite and reflink that produce different > mapping results. > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > --- This patch makes sense to me, but this case looks like a regression test test for a known btrfs issue. But this test case doesn't point out which bug/fix does it test for. So I don't know if the 64k blocksize is a necessary condition to reproduce the bug. If we can prove it still can reproduce that bug with this patch at least, then it's good to me to merge it. Thanks, Zorro > tests/generic/353 | 29 ++++++++++++++++------------- > tests/generic/353.out | 15 +-------------- > 2 files changed, 17 insertions(+), 27 deletions(-) > > diff --git a/tests/generic/353 b/tests/generic/353 > index 9a1471bd..c5639725 100755 > --- a/tests/generic/353 > +++ b/tests/generic/353 > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > _scratch_mkfs > /dev/null 2>&1 > _scratch_mount > > -blocksize=64k > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > + > file1="$SCRATCH_MNT/file1" > file2="$SCRATCH_MNT/file2" > +extmap1="$SCRATCH_MNT/extmap1" > +extmap2="$SCRATCH_MNT/extmap2" > > # write the initial file > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > # reflink initial file > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > # check their fiemap to make sure it's correct > -echo "before sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > # sync and recheck, to make sure the fiemap doesn't change just > # due to sync > sync > -echo "after sync:" > -echo "$file1" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > -echo "$file2" | _filter_scratch > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > + > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > + > +echo "Silence is golden" > > # success, all done > status=0 > diff --git a/tests/generic/353.out b/tests/generic/353.out > index 4f6e0b92..16ba4f1f 100644 > --- a/tests/generic/353.out > +++ b/tests/generic/353.out > @@ -1,15 +1,2 @@ > QA output created by 353 > -wrote 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -linked 65536/65536 bytes at offset 0 > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > -before sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > -after sync: > -SCRATCH_MNT/file1 > -0: [0..127]: shared|last > -SCRATCH_MNT/file2 > -0: [0..127]: shared|last > +Silence is golden > -- > 2.41.0 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-29 5:06 ` Zorro Lang @ 2023-09-29 13:42 ` Bill O'Donnell 2023-10-23 22:40 ` Bill O'Donnell 0 siblings, 1 reply; 9+ messages in thread From: Bill O'Donnell @ 2023-09-29 13:42 UTC (permalink / raw) To: Zorro Lang; +Cc: fstests, quwenruo.btrfs, esandeen On Fri, Sep 29, 2023 at 01:06:51PM +0800, Zorro Lang wrote: > On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > > modify it to accommodate pwrite and reflink that produce different > > mapping results. > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > > --- > > This patch makes sense to me, but this case looks like a regression test > test for a known btrfs issue. But this test case doesn't point out which > bug/fix does it test for. So I don't know if the 64k blocksize is a > necessary condition to reproduce the bug. > > If we can prove it still can reproduce that bug with this patch at least, > then it's good to me to merge it. I'd like Qu to weigh in on this from the btrfs standpoint. Thanks- Bill > > Thanks, > Zorro > > > tests/generic/353 | 29 ++++++++++++++++------------- > > tests/generic/353.out | 15 +-------------- > > 2 files changed, 17 insertions(+), 27 deletions(-) > > > > diff --git a/tests/generic/353 b/tests/generic/353 > > index 9a1471bd..c5639725 100755 > > --- a/tests/generic/353 > > +++ b/tests/generic/353 > > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > > _scratch_mkfs > /dev/null 2>&1 > > _scratch_mount > > > > -blocksize=64k > > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > > + > > file1="$SCRATCH_MNT/file1" > > file2="$SCRATCH_MNT/file2" > > +extmap1="$SCRATCH_MNT/extmap1" > > +extmap2="$SCRATCH_MNT/extmap2" > > > > # write the initial file > > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > > > # reflink initial file > > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > > > # check their fiemap to make sure it's correct > > -echo "before sync:" > > -echo "$file1" | _filter_scratch > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > -echo "$file2" | _filter_scratch > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > + > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > > > # sync and recheck, to make sure the fiemap doesn't change just > > # due to sync > > sync > > -echo "after sync:" > > -echo "$file1" | _filter_scratch > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > -echo "$file2" | _filter_scratch > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > + > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > > + > > +echo "Silence is golden" > > > > # success, all done > > status=0 > > diff --git a/tests/generic/353.out b/tests/generic/353.out > > index 4f6e0b92..16ba4f1f 100644 > > --- a/tests/generic/353.out > > +++ b/tests/generic/353.out > > @@ -1,15 +1,2 @@ > > QA output created by 353 > > -wrote 65536/65536 bytes at offset 0 > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > -linked 65536/65536 bytes at offset 0 > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > -before sync: > > -SCRATCH_MNT/file1 > > -0: [0..127]: shared|last > > -SCRATCH_MNT/file2 > > -0: [0..127]: shared|last > > -after sync: > > -SCRATCH_MNT/file1 > > -0: [0..127]: shared|last > > -SCRATCH_MNT/file2 > > -0: [0..127]: shared|last > > +Silence is golden > > -- > > 2.41.0 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-09-29 13:42 ` Bill O'Donnell @ 2023-10-23 22:40 ` Bill O'Donnell 2023-10-24 5:21 ` Zorro Lang 0 siblings, 1 reply; 9+ messages in thread From: Bill O'Donnell @ 2023-10-23 22:40 UTC (permalink / raw) To: Bill O'Donnell; +Cc: Zorro Lang, fstests, quwenruo.btrfs, esandeen ping? On Fri, Sep 29, 2023 at 08:42:26AM -0500, Bill O'Donnell wrote: > On Fri, Sep 29, 2023 at 01:06:51PM +0800, Zorro Lang wrote: > > On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > > > modify it to accommodate pwrite and reflink that produce different > > > mapping results. > > > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > > > --- > > > > This patch makes sense to me, but this case looks like a regression test > > test for a known btrfs issue. But this test case doesn't point out which > > bug/fix does it test for. So I don't know if the 64k blocksize is a > > necessary condition to reproduce the bug. > > > > If we can prove it still can reproduce that bug with this patch at least, > > then it's good to me to merge it. > > I'd like Qu to weigh in on this from the btrfs standpoint. > > Thanks- > Bill > > > > > Thanks, > > Zorro > > > > > tests/generic/353 | 29 ++++++++++++++++------------- > > > tests/generic/353.out | 15 +-------------- > > > 2 files changed, 17 insertions(+), 27 deletions(-) > > > > > > diff --git a/tests/generic/353 b/tests/generic/353 > > > index 9a1471bd..c5639725 100755 > > > --- a/tests/generic/353 > > > +++ b/tests/generic/353 > > > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > > > _scratch_mkfs > /dev/null 2>&1 > > > _scratch_mount > > > > > > -blocksize=64k > > > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > > > + > > > file1="$SCRATCH_MNT/file1" > > > file2="$SCRATCH_MNT/file2" > > > +extmap1="$SCRATCH_MNT/extmap1" > > > +extmap2="$SCRATCH_MNT/extmap2" > > > > > > # write the initial file > > > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > > > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > > > > > # reflink initial file > > > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > > > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > > > > > # check their fiemap to make sure it's correct > > > -echo "before sync:" > > > -echo "$file1" | _filter_scratch > > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > > -echo "$file2" | _filter_scratch > > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > > + > > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > > > > > # sync and recheck, to make sure the fiemap doesn't change just > > > # due to sync > > > sync > > > -echo "after sync:" > > > -echo "$file1" | _filter_scratch > > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > > -echo "$file2" | _filter_scratch > > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > > + > > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > > > + > > > +echo "Silence is golden" > > > > > > # success, all done > > > status=0 > > > diff --git a/tests/generic/353.out b/tests/generic/353.out > > > index 4f6e0b92..16ba4f1f 100644 > > > --- a/tests/generic/353.out > > > +++ b/tests/generic/353.out > > > @@ -1,15 +1,2 @@ > > > QA output created by 353 > > > -wrote 65536/65536 bytes at offset 0 > > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > > -linked 65536/65536 bytes at offset 0 > > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > > -before sync: > > > -SCRATCH_MNT/file1 > > > -0: [0..127]: shared|last > > > -SCRATCH_MNT/file2 > > > -0: [0..127]: shared|last > > > -after sync: > > > -SCRATCH_MNT/file1 > > > -0: [0..127]: shared|last > > > -SCRATCH_MNT/file2 > > > -0: [0..127]: shared|last > > > +Silence is golden > > > -- > > > 2.41.0 > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-10-23 22:40 ` Bill O'Donnell @ 2023-10-24 5:21 ` Zorro Lang 2023-10-24 7:03 ` Qu Wenruo 0 siblings, 1 reply; 9+ messages in thread From: Zorro Lang @ 2023-10-24 5:21 UTC (permalink / raw) To: Bill O'Donnell; +Cc: fstests, quwenruo.btrfs, esandeen, linux-btrfs On Mon, Oct 23, 2023 at 05:40:31PM -0500, Bill O'Donnell wrote: > ping? Hi Bill, > > On Fri, Sep 29, 2023 at 08:42:26AM -0500, Bill O'Donnell wrote: > > On Fri, Sep 29, 2023 at 01:06:51PM +0800, Zorro Lang wrote: > > > On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also > > > > modify it to accommodate pwrite and reflink that produce different > > > > mapping results. > > > > > > > > Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> > > > > --- > > > > > > This patch makes sense to me, but this case looks like a regression test > > > test for a known btrfs issue. But this test case doesn't point out which > > > bug/fix does it test for. So I don't know if the 64k blocksize is a > > > necessary condition to reproduce the bug. > > > > > > If we can prove it still can reproduce that bug with this patch at least, > > > then it's good to me to merge it. > > > > I'd like Qu to weigh in on this from the btrfs standpoint. If there's still not any review points from Qu or other btrfs devels, I'll merge this patch in next release. Better to make sure if the 64k (multi-blocks) is a necessary condition for this case. What's the bug fix this case trying to cover? Thanks, Zorro > > > > Thanks- > > Bill > > > > > > > > Thanks, > > > Zorro > > > > > > > tests/generic/353 | 29 ++++++++++++++++------------- > > > > tests/generic/353.out | 15 +-------------- > > > > 2 files changed, 17 insertions(+), 27 deletions(-) > > > > > > > > diff --git a/tests/generic/353 b/tests/generic/353 > > > > index 9a1471bd..c5639725 100755 > > > > --- a/tests/generic/353 > > > > +++ b/tests/generic/353 > > > > @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" > > > > _scratch_mkfs > /dev/null 2>&1 > > > > _scratch_mount > > > > > > > > -blocksize=64k > > > > +blocksize=$(_get_file_block_size $SCRATCH_MNT) > > > > + > > > > file1="$SCRATCH_MNT/file1" > > > > file2="$SCRATCH_MNT/file2" > > > > +extmap1="$SCRATCH_MNT/extmap1" > > > > +extmap2="$SCRATCH_MNT/extmap2" > > > > > > > > # write the initial file > > > > -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io > > > > +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null > > > > > > > > # reflink initial file > > > > -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io > > > > +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null > > > > > > > > # check their fiemap to make sure it's correct > > > > -echo "before sync:" > > > > -echo "$file1" | _filter_scratch > > > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > > > -echo "$file2" | _filter_scratch > > > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > > > + > > > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" > > > > > > > > # sync and recheck, to make sure the fiemap doesn't change just > > > > # due to sync > > > > sync > > > > -echo "after sync:" > > > > -echo "$file1" | _filter_scratch > > > > -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > > > > -echo "$file2" | _filter_scratch > > > > -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > > > > +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 > > > > +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 > > > > + > > > > +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" > > > > + > > > > +echo "Silence is golden" > > > > > > > > # success, all done > > > > status=0 > > > > diff --git a/tests/generic/353.out b/tests/generic/353.out > > > > index 4f6e0b92..16ba4f1f 100644 > > > > --- a/tests/generic/353.out > > > > +++ b/tests/generic/353.out > > > > @@ -1,15 +1,2 @@ > > > > QA output created by 353 > > > > -wrote 65536/65536 bytes at offset 0 > > > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > > > -linked 65536/65536 bytes at offset 0 > > > > -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > > > > -before sync: > > > > -SCRATCH_MNT/file1 > > > > -0: [0..127]: shared|last > > > > -SCRATCH_MNT/file2 > > > > -0: [0..127]: shared|last > > > > -after sync: > > > > -SCRATCH_MNT/file1 > > > > -0: [0..127]: shared|last > > > > -SCRATCH_MNT/file2 > > > > -0: [0..127]: shared|last > > > > +Silence is golden > > > > -- > > > > 2.41.0 > > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors 2023-10-24 5:21 ` Zorro Lang @ 2023-10-24 7:03 ` Qu Wenruo 0 siblings, 0 replies; 9+ messages in thread From: Qu Wenruo @ 2023-10-24 7:03 UTC (permalink / raw) To: Zorro Lang, Bill O'Donnell Cc: fstests, quwenruo.btrfs, esandeen, linux-btrfs On 2023/10/24 15:51, Zorro Lang wrote: > On Mon, Oct 23, 2023 at 05:40:31PM -0500, Bill O'Donnell wrote: >> ping? > > Hi Bill, > >> >> On Fri, Sep 29, 2023 at 08:42:26AM -0500, Bill O'Donnell wrote: >>> On Fri, Sep 29, 2023 at 01:06:51PM +0800, Zorro Lang wrote: >>>> On Fri, Sep 01, 2023 at 11:18:16AM -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 use actual blocksize for pwrite and reflink. Also >>>>> modify it to accommodate pwrite and reflink that produce different >>>>> mapping results. >>>>> >>>>> Signed-off-by: Bill O'Donnell <bodonnel@redhat.com> >>>>> --- >>>> >>>> This patch makes sense to me, but this case looks like a regression test >>>> test for a known btrfs issue. But this test case doesn't point out which >>>> bug/fix does it test for. So I don't know if the 64k blocksize is a >>>> necessary condition to reproduce the bug. >>>> >>>> If we can prove it still can reproduce that bug with this patch at least, >>>> then it's good to me to merge it. >>> >>> I'd like Qu to weigh in on this from the btrfs standpoint. Weird, this is the first time this mail shows up in my suse.com mailbox, but not in my gmx one, maybe it's something wrong with my filter. Anyway the patch looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> > > If there's still not any review points from Qu or other btrfs devels, I'll > merge this patch in next release. > Better to make sure if the 64k (multi-blocks) > is a necessary condition for this case. What's the bug fix this case trying to > cover? 64K is not a necessary condition, it's just here to allow all btrfs sector sizes (4K to 64K) to be covered. In fact, 64K blocksize on a btrfs with 4K sectorsize can lead to false alerts, as there is no guarantee that 64K buffered write would lead to one single extent. The initial bugfix is just a bug inside btrfs' SHARED flag detection code, and is completely blocksize unrelated. So we're safe to change the IO blocksize to ensure the buffered writes only lead to a single extent. Sorry for the late reply (and possibly bad filter setup) Thanks, Qu > > Thanks, > Zorro > >>> >>> Thanks- >>> Bill >>> >>>> >>>> Thanks, >>>> Zorro >>>> >>>>> tests/generic/353 | 29 ++++++++++++++++------------- >>>>> tests/generic/353.out | 15 +-------------- >>>>> 2 files changed, 17 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/tests/generic/353 b/tests/generic/353 >>>>> index 9a1471bd..c5639725 100755 >>>>> --- a/tests/generic/353 >>>>> +++ b/tests/generic/353 >>>>> @@ -29,31 +29,34 @@ _require_xfs_io_command "fiemap" >>>>> _scratch_mkfs > /dev/null 2>&1 >>>>> _scratch_mount >>>>> >>>>> -blocksize=64k >>>>> +blocksize=$(_get_file_block_size $SCRATCH_MNT) >>>>> + >>>>> file1="$SCRATCH_MNT/file1" >>>>> file2="$SCRATCH_MNT/file2" >>>>> +extmap1="$SCRATCH_MNT/extmap1" >>>>> +extmap2="$SCRATCH_MNT/extmap2" >>>>> >>>>> # write the initial file >>>>> -_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 | _filter_xfs_io >>>>> +_pwrite_byte 0xcdcdcdcd 0 $blocksize $file1 > /dev/null >>>>> >>>>> # reflink initial file >>>>> -_reflink_range $file1 0 $file2 0 $blocksize | _filter_xfs_io >>>>> +_reflink_range $file1 0 $file2 0 $blocksize > /dev/null >>>>> >>>>> # check their fiemap to make sure it's correct >>>>> -echo "before sync:" >>>>> -echo "$file1" | _filter_scratch >>>>> -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags >>>>> -echo "$file2" | _filter_scratch >>>>> -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags >>>>> +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 >>>>> +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 >>>>> + >>>>> +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps before sync" >>>>> >>>>> # sync and recheck, to make sure the fiemap doesn't change just >>>>> # due to sync >>>>> sync >>>>> -echo "after sync:" >>>>> -echo "$file1" | _filter_scratch >>>>> -$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags >>>>> -echo "$file2" | _filter_scratch >>>>> -$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags >>>>> +$XFS_IO_PROG -c "fiemap -v" $file1 | _filter_fiemap_flags > $extmap1 >>>>> +$XFS_IO_PROG -c "fiemap -v" $file2 | _filter_fiemap_flags > $extmap2 >>>>> + >>>>> +cmp -s $extmap1 $extmap2 || echo "mismatched extent maps after sync" >>>>> + >>>>> +echo "Silence is golden" >>>>> >>>>> # success, all done >>>>> status=0 >>>>> diff --git a/tests/generic/353.out b/tests/generic/353.out >>>>> index 4f6e0b92..16ba4f1f 100644 >>>>> --- a/tests/generic/353.out >>>>> +++ b/tests/generic/353.out >>>>> @@ -1,15 +1,2 @@ >>>>> QA output created by 353 >>>>> -wrote 65536/65536 bytes at offset 0 >>>>> -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >>>>> -linked 65536/65536 bytes at offset 0 >>>>> -XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >>>>> -before sync: >>>>> -SCRATCH_MNT/file1 >>>>> -0: [0..127]: shared|last >>>>> -SCRATCH_MNT/file2 >>>>> -0: [0..127]: shared|last >>>>> -after sync: >>>>> -SCRATCH_MNT/file1 >>>>> -0: [0..127]: shared|last >>>>> -SCRATCH_MNT/file2 >>>>> -0: [0..127]: shared|last >>>>> +Silence is golden >>>>> -- >>>>> 2.41.0 >>>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-10-24 7:03 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-01 16:18 [PATCH v2] fstests: generic/353 should accomodate other pwrite behaviors Bill O'Donnell 2023-09-08 13:42 ` Bill O'Donnell 2023-09-21 22:30 ` Bill O'Donnell 2023-09-25 20:37 ` Bill O'Donnell 2023-09-29 5:06 ` Zorro Lang 2023-09-29 13:42 ` Bill O'Donnell 2023-10-23 22:40 ` Bill O'Donnell 2023-10-24 5:21 ` Zorro Lang 2023-10-24 7:03 ` Qu Wenruo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox