Re: [PATCH v2] fstests: generic/018: expand write backwards sync but contiguous to test regression in btrfs
Hi, Sorry for the late reply. On Fri, Aug 14, 2015 at 10:19:16AM -0500, Eric Sandeen wrote: On 8/13/15 3:47 AM, Liu Bo wrote: Btrfs has a problem when defraging a file which has a large fragment'ed range, it'd leave the tail extent as a seperate extent instead of merging it with previous extents. This makes generic/018 recognize the above regression. Sorry for the late review, but here it is ;) In 2 years (heck, even now) we'll have no idea why this change was made. What regression is that? Can you describe it? Is there already an upstream fix/commit you can refer to? The above commit log is talking about the regression, do you mean by writing a better description of the regression than it or directly putting a comment in generic/018 case? Yes, There is a fix[1] for this, but still pending to be merged :) [1]: Btrfs: fix defrag to merge tail file extent https://patchwork.kernel.org/patch/6966631/ I see 3 changes here: 1) You change xfs_io's for loop from seq 9 -1 0 to seq 64 -1 0 - presumably this matters to btrfs. Why does this matter? This is part of reproducing the btrfs regression since it needs a fragmented range that is larger than 256K, so I choose (65 * 4)k here, will update this in the commit log if it confuses. Meanwhile, I find that in the case of 'write backwards sync but contiguous, ext4 doesn't produce fragments like btrfs and xfs, so I modify 018.out a little bit to let ext4 pass. 2) You stop expecting 10 extents initially in the backwards-write test for the above reason, I guess. I'm a little unsure about this. For me, this passes as-is. If it isn't working for you, we should understand why, instead of making the test ignore it. (And bundling this ext4 change into a btrfs-specific commit isn't great, anyway) Hmm, OK, the problem of ext4 is that with 'seq 64 -1 0' backwards-write it doesn't produce 65 extents, instead it comes to 12 extents, at least on my box and I use the default mount options. I took a look at ext4 code but didn't figure out any problems, so I'd leave it as it is so that ext4 experts can pick it up. Moreover, I follow Filipe's suggestion to filter xfs_io's output in order to check these writes actually succeed. 3) You stop redirecting xfs_io to /dev/null, and save it to the golden output file instead. Honestly, I find hundreds of extra xfs_io output lines to be rather unhelpful, because the old output file used to be quite easy to read, to see what's going on. Today it only redirects stdout: $XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ /dev/null so if a write fails, I *think* stderr will get output, and the test *should* fail as a result.[1] You could add a || _fail xfs_io failed for good measure... All right, I'll take it. Thanks for the review. Thanks, -liubo -Eric [1] oh, maybe not, I guess xfs_io is kind of notorious for not returning errors... Signed-off-by: Liu Bo bo.li@oracle.com --- v2: fix typo in title, s/expend/expand/g tests/generic/018 | 16 ++-- tests/generic/018.out | 198 +- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/tests/generic/018 b/tests/generic/018 index d97bb88..3693874 100755 --- a/tests/generic/018 +++ b/tests/generic/018 @@ -68,28 +68,24 @@ $XFS_IO_PROG -f -c truncate 1m $fragfile _defrag --before 0 --after 0 $fragfile echo Contiguous file: | tee -a $seqres.full -$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ -/dev/null +$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile | _filter_xfs_io _defrag --before 1 --after 1 $fragfile echo Write backwards sync, but contiguous - should defrag to 1 extent | tee -a $seqres.full -for i in `seq 9 -1 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null +for i in `seq 64 -1 0`; do + $XFS_IO_PROG -fd -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done -_defrag --before 10 --after 1 $fragfile +_defrag --after 1 $fragfile echo Write backwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 31 -2 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile echo Write forwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 0 2 31`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c
Re: [PATCH v2] fstests: generic/018: expand write backwards sync but contiguous to test regression in btrfs
On Fri, Aug 14, 2015 at 10:19:16AM -0500, Eric Sandeen wrote: On 8/13/15 3:47 AM, Liu Bo wrote: Btrfs has a problem when defraging a file which has a large fragment'ed range, it'd leave the tail extent as a seperate extent instead of merging it with previous extents. This makes generic/018 recognize the above regression. Sorry for the late review, but here it is ;) 3) You stop redirecting xfs_io to /dev/null, and save it to the golden output file instead. Honestly, I find hundreds of extra xfs_io output lines to be rather unhelpful, because the old output file used to be quite easy to read, to see what's going on. Yup, it should remain redirected to /dev/null. If writing a few small IOs to an otherwise empty filesystem fails, then you've got bigger problems. besides, we actually test that the writes worked by the count of extents before defrag. If any of the writes fail, we'll fail because the before extent count will be wrong. Besides, if the pwrite() fails, then all the paths in xfs_io that call do_pwrite() end up doing perror(pwrite64) and so it should output errors to stderr just fine. Today it only redirects stdout: $XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ /dev/null so if a write fails, I *think* stderr will get output, and the test *should* fail as a result.[1] You could add a || _fail xfs_io failed for good measure... _fail calls do not belong here - catch the error message, and if there isn't one on stderr then xfs_io need fixing... Cheers, Dave. -- Dave Chinner da...@fromorbit.com -- To unsubscribe from this list: send the line unsubscribe linux-btrfs in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] fstests: generic/018: expand write backwards sync but contiguous to test regression in btrfs
On 8/13/15 3:47 AM, Liu Bo wrote: Btrfs has a problem when defraging a file which has a large fragment'ed range, it'd leave the tail extent as a seperate extent instead of merging it with previous extents. This makes generic/018 recognize the above regression. Sorry for the late review, but here it is ;) In 2 years (heck, even now) we'll have no idea why this change was made. What regression is that? Can you describe it? Is there already an upstream fix/commit you can refer to? I see 3 changes here: 1) You change xfs_io's for loop from seq 9 -1 0 to seq 64 -1 0 - presumably this matters to btrfs. Why does this matter? Meanwhile, I find that in the case of 'write backwards sync but contiguous, ext4 doesn't produce fragments like btrfs and xfs, so I modify 018.out a little bit to let ext4 pass. 2) You stop expecting 10 extents initially in the backwards-write test for the above reason, I guess. I'm a little unsure about this. For me, this passes as-is. If it isn't working for you, we should understand why, instead of making the test ignore it. (And bundling this ext4 change into a btrfs-specific commit isn't great, anyway) Moreover, I follow Filipe's suggestion to filter xfs_io's output in order to check these writes actually succeed. 3) You stop redirecting xfs_io to /dev/null, and save it to the golden output file instead. Honestly, I find hundreds of extra xfs_io output lines to be rather unhelpful, because the old output file used to be quite easy to read, to see what's going on. Today it only redirects stdout: $XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ /dev/null so if a write fails, I *think* stderr will get output, and the test *should* fail as a result.[1] You could add a || _fail xfs_io failed for good measure... -Eric [1] oh, maybe not, I guess xfs_io is kind of notorious for not returning errors... Signed-off-by: Liu Bo bo.li@oracle.com --- v2: fix typo in title, s/expend/expand/g tests/generic/018 | 16 ++-- tests/generic/018.out | 198 +- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/tests/generic/018 b/tests/generic/018 index d97bb88..3693874 100755 --- a/tests/generic/018 +++ b/tests/generic/018 @@ -68,28 +68,24 @@ $XFS_IO_PROG -f -c truncate 1m $fragfile _defrag --before 0 --after 0 $fragfile echo Contiguous file: | tee -a $seqres.full -$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ - /dev/null +$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile | _filter_xfs_io _defrag --before 1 --after 1 $fragfile echo Write backwards sync, but contiguous - should defrag to 1 extent | tee -a $seqres.full -for i in `seq 9 -1 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ - /dev/null +for i in `seq 64 -1 0`; do + $XFS_IO_PROG -fd -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done -_defrag --before 10 --after 1 $fragfile +_defrag --after 1 $fragfile echo Write backwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 31 -2 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ - /dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile echo Write forwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 0 2 31`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ - /dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile diff --git a/tests/generic/018.out b/tests/generic/018.out index 5f265d1..0886a9a 100644 --- a/tests/generic/018.out +++ b/tests/generic/018.out @@ -6,14 +6,210 @@ Sparse file (no blocks): Before: 0 After: 0 Contiguous file: +wrote 16384/16384 bytes at offset 0 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Before: 1 After: 1 Write backwards sync, but contiguous - should defrag to 1 extent -Before: 10 +wrote 4096/4096 bytes at offset 262144 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 258048 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 253952 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 249856 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 245760 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset
Re: [PATCH v2] fstests: generic/018: expand write backwards sync but contiguous to test regression in btrfs
On Thu, Aug 13, 2015 at 10:43 AM, Filipe David Manana fdman...@gmail.com wrote: On Thu, Aug 13, 2015 at 9:47 AM, Liu Bo bo.li@oracle.com wrote: Btrfs has a problem when defraging a file which has a large fragment'ed range, it'd leave the tail extent as a seperate extent instead of merging it with previous extents. This makes generic/018 recognize the above regression. Meanwhile, I find that in the case of 'write backwards sync but contiguous, ext4 doesn't produce fragments like btrfs and xfs, so I modify 018.out a little bit to let ext4 pass. Moreover, I follow Filipe's suggestion to filter xfs_io's output in order to check these writes actually succeed. Signed-off-by: Liu Bo bo.li@oracle.com Reviewed-by: Filipe Manana fdman...@suse.com The lines with XFS_IO_PROG are now wider than 80 characters (the echo command lines were already wider than 80 too). But other than that all looks good to me. Test fails with the btrfs kernel fix, test passes with the fix applied Err, typo, should have been test fails without the btrfs kernel fix... and ext4/xfs continue to pass here. Thanks. --- v2: fix typo in title, s/expend/expand/g tests/generic/018 | 16 ++-- tests/generic/018.out | 198 +- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/tests/generic/018 b/tests/generic/018 index d97bb88..3693874 100755 --- a/tests/generic/018 +++ b/tests/generic/018 @@ -68,28 +68,24 @@ $XFS_IO_PROG -f -c truncate 1m $fragfile _defrag --before 0 --after 0 $fragfile echo Contiguous file: | tee -a $seqres.full -$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ -/dev/null +$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile | _filter_xfs_io _defrag --before 1 --after 1 $fragfile echo Write backwards sync, but contiguous - should defrag to 1 extent | tee -a $seqres.full -for i in `seq 9 -1 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null +for i in `seq 64 -1 0`; do + $XFS_IO_PROG -fd -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done -_defrag --before 10 --after 1 $fragfile +_defrag --after 1 $fragfile echo Write backwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 31 -2 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile echo Write forwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 0 2 31`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile diff --git a/tests/generic/018.out b/tests/generic/018.out index 5f265d1..0886a9a 100644 --- a/tests/generic/018.out +++ b/tests/generic/018.out @@ -6,14 +6,210 @@ Sparse file (no blocks): Before: 0 After: 0 Contiguous file: +wrote 16384/16384 bytes at offset 0 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Before: 1 After: 1 Write backwards sync, but contiguous - should defrag to 1 extent -Before: 10 +wrote 4096/4096 bytes at offset 262144 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 258048 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 253952 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 249856 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 245760 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 241664 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 237568 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 233472 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 229376 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 225280 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 221184 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 217088 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 212992 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 208896 +XXX Bytes, X ops;
Re: [PATCH v2] fstests: generic/018: expand write backwards sync but contiguous to test regression in btrfs
On Thu, Aug 13, 2015 at 9:47 AM, Liu Bo bo.li@oracle.com wrote: Btrfs has a problem when defraging a file which has a large fragment'ed range, it'd leave the tail extent as a seperate extent instead of merging it with previous extents. This makes generic/018 recognize the above regression. Meanwhile, I find that in the case of 'write backwards sync but contiguous, ext4 doesn't produce fragments like btrfs and xfs, so I modify 018.out a little bit to let ext4 pass. Moreover, I follow Filipe's suggestion to filter xfs_io's output in order to check these writes actually succeed. Signed-off-by: Liu Bo bo.li@oracle.com Reviewed-by: Filipe Manana fdman...@suse.com The lines with XFS_IO_PROG are now wider than 80 characters (the echo command lines were already wider than 80 too). But other than that all looks good to me. Test fails with the btrfs kernel fix, test passes with the fix applied and ext4/xfs continue to pass here. Thanks. --- v2: fix typo in title, s/expend/expand/g tests/generic/018 | 16 ++-- tests/generic/018.out | 198 +- 2 files changed, 203 insertions(+), 11 deletions(-) diff --git a/tests/generic/018 b/tests/generic/018 index d97bb88..3693874 100755 --- a/tests/generic/018 +++ b/tests/generic/018 @@ -68,28 +68,24 @@ $XFS_IO_PROG -f -c truncate 1m $fragfile _defrag --before 0 --after 0 $fragfile echo Contiguous file: | tee -a $seqres.full -$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile \ -/dev/null +$XFS_IO_PROG -f -c pwrite -b $((4 * bsize)) 0 $((4 * bsize)) $fragfile | _filter_xfs_io _defrag --before 1 --after 1 $fragfile echo Write backwards sync, but contiguous - should defrag to 1 extent | tee -a $seqres.full -for i in `seq 9 -1 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null +for i in `seq 64 -1 0`; do + $XFS_IO_PROG -fd -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done -_defrag --before 10 --after 1 $fragfile +_defrag --after 1 $fragfile echo Write backwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 31 -2 0`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile echo Write forwards sync leaving holes - defrag should do nothing | tee -a $seqres.full for i in `seq 0 2 31`; do - $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile \ -/dev/null + $XFS_IO_PROG -fs -c pwrite -b $bsize $((i * bsize)) $bsize $fragfile | _filter_xfs_io done _defrag --before 16 --after 16 $fragfile diff --git a/tests/generic/018.out b/tests/generic/018.out index 5f265d1..0886a9a 100644 --- a/tests/generic/018.out +++ b/tests/generic/018.out @@ -6,14 +6,210 @@ Sparse file (no blocks): Before: 0 After: 0 Contiguous file: +wrote 16384/16384 bytes at offset 0 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) Before: 1 After: 1 Write backwards sync, but contiguous - should defrag to 1 extent -Before: 10 +wrote 4096/4096 bytes at offset 262144 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 258048 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 253952 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 249856 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 245760 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 241664 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 237568 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 233472 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 229376 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 225280 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 221184 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 217088 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 212992 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 208896 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at offset 204800 +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +wrote 4096/4096 bytes at
