[PATCH] xfs/420: only check the extent layout after syncing

[PATCH] xfs/420: only check the extent layout after syncing

[Christoph Hellwig]

This tests validates the correct extent layout for some hairy reflink
related issues.  But until we called sync or fsync we have no gurantee
of any data fork layout, as only writeback moves the extents from the
COW for to the data fork.

The comparism pass before the sync might see an "error" if we use COW
fork speculative preallocations for non-overwrites, which is useful to
reduce fragmentation.

Just remove the pass of comparisms before the sync, as the one after
the sync will catch all persistent issues.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/xfs/420     | 14 --------------
 tests/xfs/420.out | 14 --------------
 2 files changed, 28 deletions(-)

diff --git a/tests/xfs/420 b/tests/xfs/420
index a083a12b..0d611fd6 100755
--- a/tests/xfs/420
+++ b/tests/xfs/420
@@ -93,20 +93,6 @@ $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file2 >> $seqres
 $XFS_IO_PROG -c "pwrite -S 0x63 0 $blksz" $testdir/file3 >> $seqres.full
 $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file3 >> $seqres.full
 
-$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file1 >> $seqres.full 2>&1
-$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file2 >> $seqres.full 2>&1
-$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file3 >> $seqres.full 2>&1
-
-echo "Seek holes and data in file1"
-$XFS_IO_PROG -c "seek -a -r 0" $testdir/file1
-echo "Seek holes and data in file2"
-$XFS_IO_PROG -c "seek -a -r 0" $testdir/file2
-
-echo "Compare files"
-md5sum $testdir/file1 | _filter_scratch
-md5sum $testdir/file2 | _filter_scratch
-md5sum $testdir/file3 | _filter_scratch
-
 echo "sync filesystem" | tee -a $seqres.full
 sync
 
diff --git a/tests/xfs/420.out b/tests/xfs/420.out
index d1b5483a..39360741 100644
--- a/tests/xfs/420.out
+++ b/tests/xfs/420.out
@@ -6,20 +6,6 @@ c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
 c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file2
 c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file3
 CoW the shared part then write into the empty part
-Seek holes and data in file1
-Whence	Result
-DATA	0
-HOLE	131072
-Seek holes and data in file2
-Whence	Result
-DATA	0
-HOLE	131072
-DATA	196608
-HOLE	262144
-Compare files
-c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
-017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file2
-017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file3
 sync filesystem
 Seek holes and data in file1
 Whence	Result
-- 
2.20.1

Re: [PATCH] xfs/420: only check the extent layout after syncing

[Eryu Guan]

On Mon, Feb 04, 2019 at 04:30:26PM +0100, Christoph Hellwig wrote:
> This tests validates the correct extent layout for some hairy reflink
> related issues.  But until we called sync or fsync we have no gurantee
> of any data fork layout, as only writeback moves the extents from the
> COW for to the data fork.
> 
> The comparism pass before the sync might see an "error" if we use COW
> fork speculative preallocations for non-overwrites, which is useful to
> reduce fragmentation.
> 
> Just remove the pass of comparisms before the sync, as the one after
> the sync will catch all persistent issues.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

I think this is fine. But I'd like the test author to take a look too.

/me looks at Darrick :)

Thanks,
Eryu

> ---
>  tests/xfs/420     | 14 --------------
>  tests/xfs/420.out | 14 --------------
>  2 files changed, 28 deletions(-)
> 
> diff --git a/tests/xfs/420 b/tests/xfs/420
> index a083a12b..0d611fd6 100755
> --- a/tests/xfs/420
> +++ b/tests/xfs/420
> @@ -93,20 +93,6 @@ $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file2 >> $seqres
>  $XFS_IO_PROG -c "pwrite -S 0x63 0 $blksz" $testdir/file3 >> $seqres.full
>  $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file3 >> $seqres.full
>  
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file1 >> $seqres.full 2>&1
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file2 >> $seqres.full 2>&1
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file3 >> $seqres.full 2>&1
> -
> -echo "Seek holes and data in file1"
> -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file1
> -echo "Seek holes and data in file2"
> -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file2
> -
> -echo "Compare files"
> -md5sum $testdir/file1 | _filter_scratch
> -md5sum $testdir/file2 | _filter_scratch
> -md5sum $testdir/file3 | _filter_scratch
> -
>  echo "sync filesystem" | tee -a $seqres.full
>  sync
>  
> diff --git a/tests/xfs/420.out b/tests/xfs/420.out
> index d1b5483a..39360741 100644
> --- a/tests/xfs/420.out
> +++ b/tests/xfs/420.out
> @@ -6,20 +6,6 @@ c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
>  c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file2
>  c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file3
>  CoW the shared part then write into the empty part
> -Seek holes and data in file1
> -Whence	Result
> -DATA	0
> -HOLE	131072
> -Seek holes and data in file2
> -Whence	Result
> -DATA	0
> -HOLE	131072
> -DATA	196608
> -HOLE	262144
> -Compare files
> -c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
> -017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file2
> -017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file3
>  sync filesystem
>  Seek holes and data in file1
>  Whence	Result
> -- 
> 2.20.1
> 

Re: [PATCH] xfs/420: only check the extent layout after syncing

[Darrick J. Wong]

On Mon, Feb 04, 2019 at 04:30:26PM +0100, Christoph Hellwig wrote:
> This tests validates the correct extent layout for some hairy reflink
> related issues.  But until we called sync or fsync we have no gurantee
> of any data fork layout, as only writeback moves the extents from the
> COW for to the data fork.

Hmm.  Looking at my notes for xfs/420, the goal was to make sure that if
userspace writes to offsets X and Y, an immediately subsequent SEEK_DATA
returns X and Y as data, both before and after an fsync.

The twist for this test is that offset Y didn't have any block mapped
before the COW requirement was added to the file, which means that this
test is making sure that we can't miss a piece of data that will be COWd
into place in the future but isn't currently mapped into the data fork.

It also checks that the file contents and SEEK_DATA results remain the
same after an fsync.

> The comparism pass before the sync might see an "error" if we use COW
> fork speculative preallocations for non-overwrites, which is useful to
> reduce fragmentation.

What error do you see?

> Just remove the pass of comparisms before the sync, as the one after
> the sync will catch all persistent issues.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  tests/xfs/420     | 14 --------------
>  tests/xfs/420.out | 14 --------------
>  2 files changed, 28 deletions(-)
> 
> diff --git a/tests/xfs/420 b/tests/xfs/420
> index a083a12b..0d611fd6 100755
> --- a/tests/xfs/420
> +++ b/tests/xfs/420
> @@ -93,20 +93,6 @@ $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file2 >> $seqres
>  $XFS_IO_PROG -c "pwrite -S 0x63 0 $blksz" $testdir/file3 >> $seqres.full
>  $XFS_IO_PROG -c "pwrite -S 0x63 $((blksz * 3)) $blksz" $testdir/file3 >> $seqres.full
>  
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file1 >> $seqres.full 2>&1
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file2 >> $seqres.full 2>&1
> -$XFS_IO_PROG -c "bmap -ev" -c "bmap -cv" $testdir/file3 >> $seqres.full 2>&1
> -
> -echo "Seek holes and data in file1"
> -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file1
> -echo "Seek holes and data in file2"
> -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file2

This removed code tests that the earlier write of 64k of data into file2
between 192k and 256k can be found by SEEK_DATA before file2 gets
sync'd to disk.

> -echo "Compare files"
> -md5sum $testdir/file1 | _filter_scratch
> -md5sum $testdir/file2 | _filter_scratch
> -md5sum $testdir/file3 | _filter_scratch

And this removed code checks that the page cache contents remain stable
and correct even for a write that goes through the COW mechanism.

I don't see why it's advantageous to remove this part of the test?

--D

> -
>  echo "sync filesystem" | tee -a $seqres.full
>  sync
>  
> diff --git a/tests/xfs/420.out b/tests/xfs/420.out
> index d1b5483a..39360741 100644
> --- a/tests/xfs/420.out
> +++ b/tests/xfs/420.out
> @@ -6,20 +6,6 @@ c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
>  c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file2
>  c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file3
>  CoW the shared part then write into the empty part
> -Seek holes and data in file1
> -Whence	Result
> -DATA	0
> -HOLE	131072
> -Seek holes and data in file2
> -Whence	Result
> -DATA	0
> -HOLE	131072
> -DATA	196608
> -HOLE	262144
> -Compare files
> -c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
> -017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file2
> -017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file3
>  sync filesystem
>  Seek holes and data in file1
>  Whence	Result
> -- 
> 2.20.1
> 

Re: [PATCH] xfs/420: only check the extent layout after syncing

[Christoph Hellwig]

On Mon, Feb 11, 2019 at 03:51:44PM -0800, Darrick J. Wong wrote:
> > The comparism pass before the sync might see an "error" if we use COW
> > fork speculative preallocations for non-overwrites, which is useful to
> > reduce fragmentation.
> 
> What error do you see?

-- /root/xfstests/tests/xfs/420.out	2019-02-12 15:41:12.202606228 +0000
+++ /root/xfstests/results//xfs/420.out.bad	2019-02-12 18:58:06.158426573
+0000
@@ -14,8 +14,6 @@
 Whence	Result
 DATA	0
 HOLE	131072
-DATA	196608
-HOLE	262144
 Compare files
 c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
 017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file2


> > -echo "Seek holes and data in file1"
> > -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file1
> > -echo "Seek holes and data in file2"
> > -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file2
> 
> This removed code tests that the earlier write of 64k of data into file2
> between 192k and 256k can be found by SEEK_DATA before file2 gets
> sync'd to disk.

Well, and it might not be able to be found if it is in the COW fork..

> And this removed code checks that the page cache contents remain stable
> and correct even for a write that goes through the COW mechanism.
> 
> I don't see why it's advantageous to remove this part of the test?

Last time I send a patch to just add a sync and got the recommendation
to just remove the double tests before and after the sync..

Re: [PATCH] xfs/420: only check the extent layout after syncing

[Darrick J. Wong]

On Tue, Feb 12, 2019 at 08:00:22PM +0100, Christoph Hellwig wrote:
> On Mon, Feb 11, 2019 at 03:51:44PM -0800, Darrick J. Wong wrote:
> > > The comparism pass before the sync might see an "error" if we use COW
> > > fork speculative preallocations for non-overwrites, which is useful to
> > > reduce fragmentation.
> > 
> > What error do you see?
> 
> -- /root/xfstests/tests/xfs/420.out	2019-02-12 15:41:12.202606228 +0000
> +++ /root/xfstests/results//xfs/420.out.bad	2019-02-12 18:58:06.158426573
> +0000
> @@ -14,8 +14,6 @@
>  Whence	Result
>  DATA	0
>  HOLE	131072
> -DATA	196608
> -HOLE	262144
>  Compare files
>  c2803804acc9936eef8aab42c119bfac  SCRATCH_MNT/test-420/file1
>  017c08a9320aad844ce86aa9631afb98  SCRATCH_MNT/test-420/file2
> 
> 
> > > -echo "Seek holes and data in file1"
> > > -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file1
> > > -echo "Seek holes and data in file2"
> > > -$XFS_IO_PROG -c "seek -a -r 0" $testdir/file2
> > 
> > This removed code tests that the earlier write of 64k of data into file2
> > between 192k and 256k can be found by SEEK_DATA before file2 gets
> > sync'd to disk.
> 
> Well, and it might not be able to be found if it is in the COW fork..

But why would data only be able to be found in the COW fork?

Does this test failure happen for you on a regular xfs filesystem, or
does it only happen with the alwayscow patchset?

If yes, are unfsync'd O_ATOMIC writes to holes invisible to seek_data
until fsync finishes?

> > And this removed code checks that the page cache contents remain stable
> > and correct even for a write that goes through the COW mechanism.
> > 
> > I don't see why it's advantageous to remove this part of the test?
> 
> Last time I send a patch to just add a sync and got the recommendation
> to just remove the double tests before and after the sync..

Yes, and I think Eryu is wrong.  We wrote data into a hole between 192k
and 256k, and SEEK_DATA should be able to find that data (it's readable
from the page cache, after all) even if we haven't yet fsync'd the file.

--D