* [PATCH 1/3] common: add _require_test_swapfile
2018-12-03 6:42 [PATCH 0/3] fstests: copy_file_range() bounds testing Dave Chinner
@ 2018-12-03 6:42 ` Dave Chinner
2018-12-03 16:43 ` Darrick J. Wong
2018-12-13 12:16 ` Xiao Yang
2018-12-03 6:42 ` [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail Dave Chinner
` (2 subsequent siblings)
3 siblings, 2 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-03 6:42 UTC (permalink / raw)
To: fstests
From: Dave Chinner <dchinner@redhat.com>
Because we can host swap files on the test device, not just the
scratch device.
Also, move the tests for the utilities needed to manipulate swap
files into the functions that test whether swap files are supported
so they are checked for existence /before/ we try to us them. This
fixes all the tests that currently check for these utilities
manually /after/ checking if swapfiles are supported.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
common/rc | 29 +++++++++++++++++++++--------
tests/generic/472 | 2 --
tests/generic/495 | 2 --
tests/generic/496 | 2 --
tests/generic/497 | 2 --
5 files changed, 21 insertions(+), 16 deletions(-)
diff --git a/common/rc b/common/rc
index ecb17380bad8..5b344b25012b 100644
--- a/common/rc
+++ b/common/rc
@@ -2214,22 +2214,35 @@ _format_swapfile() {
}
# Check that the filesystem supports swapfiles
-_require_scratch_swapfile()
+_require_swapfile()
{
- _require_scratch
+ dir=$1
- _scratch_mkfs >/dev/null
- _scratch_mount
+ # fstests also has custom binaries for mkswap/swapon
+ _require_test_program mkswap
+ _require_test_program swapon
# Minimum size for mkswap is 10 pages
- _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
+ _format_swapfile "$dir/swap" $(($(get_page_size) * 10))
- if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then
- _scratch_unmount
+ if ! swapon "$dir/swap" >/dev/null 2>&1; then
_notrun "swapfiles are not supported"
fi
- swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1
+ swapoff "$dir/swap" >/dev/null 2>&1
+}
+
+_require_test_swapfile()
+{
+ _require_swapfile $TEST_DIR
+}
+
+_require_scratch_swapfile()
+{
+ _require_scratch
+ _scratch_mkfs >/dev/null
+ _scratch_mount
+ _require_swapfile $SCRATCH_MNT
_scratch_unmount
}
diff --git a/tests/generic/472 b/tests/generic/472
index aba4a00719bc..d598eef37997 100755
--- a/tests/generic/472
+++ b/tests/generic/472
@@ -33,8 +33,6 @@ rm -f $seqres.full
_supported_fs generic
_supported_os Linux
_require_scratch_swapfile
-_require_test_program mkswap
-_require_test_program swapon
rm -f $seqres.full
_scratch_mkfs >>$seqres.full 2>&1
diff --git a/tests/generic/495 b/tests/generic/495
index 88df26c78ec2..63f45cf4b336 100755
--- a/tests/generic/495
+++ b/tests/generic/495
@@ -31,8 +31,6 @@ rm -f $seqres.full
_supported_fs generic
_supported_os Linux
_require_scratch_swapfile
-_require_test_program mkswap
-_require_test_program swapon
_scratch_mkfs >> $seqres.full 2>&1
_scratch_mount
diff --git a/tests/generic/496 b/tests/generic/496
index 3083eef0bebc..0e214909f596 100755
--- a/tests/generic/496
+++ b/tests/generic/496
@@ -34,8 +34,6 @@ rm -f $seqres.full
_supported_fs generic
_supported_os Linux
_require_scratch_swapfile
-_require_test_program mkswap
-_require_test_program swapon
_require_xfs_io_command "falloc"
rm -f $seqres.full
diff --git a/tests/generic/497 b/tests/generic/497
index 3d5502ef7c08..d9f9b7521eff 100755
--- a/tests/generic/497
+++ b/tests/generic/497
@@ -34,8 +34,6 @@ rm -f $seqres.full
_supported_fs generic
_supported_os Linux
_require_scratch_swapfile
-_require_test_program mkswap
-_require_test_program swapon
_require_xfs_io_command "fcollapse"
rm -f $seqres.full
--
2.19.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 1/3] common: add _require_test_swapfile
2018-12-03 6:42 ` [PATCH 1/3] common: add _require_test_swapfile Dave Chinner
@ 2018-12-03 16:43 ` Darrick J. Wong
2018-12-13 12:16 ` Xiao Yang
1 sibling, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2018-12-03 16:43 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Mon, Dec 03, 2018 at 05:42:54PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Because we can host swap files on the test device, not just the
> scratch device.
>
> Also, move the tests for the utilities needed to manipulate swap
> files into the functions that test whether swap files are supported
> so they are checked for existence /before/ we try to us them. This
> fixes all the tests that currently check for these utilities
> manually /after/ checking if swapfiles are supported.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Looks good,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> common/rc | 29 +++++++++++++++++++++--------
> tests/generic/472 | 2 --
> tests/generic/495 | 2 --
> tests/generic/496 | 2 --
> tests/generic/497 | 2 --
> 5 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index ecb17380bad8..5b344b25012b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2214,22 +2214,35 @@ _format_swapfile() {
> }
>
> # Check that the filesystem supports swapfiles
> -_require_scratch_swapfile()
> +_require_swapfile()
> {
> - _require_scratch
> + dir=$1
>
> - _scratch_mkfs >/dev/null
> - _scratch_mount
> + # fstests also has custom binaries for mkswap/swapon
> + _require_test_program mkswap
> + _require_test_program swapon
>
> # Minimum size for mkswap is 10 pages
> - _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> + _format_swapfile "$dir/swap" $(($(get_page_size) * 10))
>
> - if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then
> - _scratch_unmount
> + if ! swapon "$dir/swap" >/dev/null 2>&1; then
> _notrun "swapfiles are not supported"
> fi
>
> - swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1
> + swapoff "$dir/swap" >/dev/null 2>&1
> +}
> +
> +_require_test_swapfile()
> +{
> + _require_swapfile $TEST_DIR
> +}
> +
> +_require_scratch_swapfile()
> +{
> + _require_scratch
> + _scratch_mkfs >/dev/null
> + _scratch_mount
> + _require_swapfile $SCRATCH_MNT
> _scratch_unmount
> }
>
> diff --git a/tests/generic/472 b/tests/generic/472
> index aba4a00719bc..d598eef37997 100755
> --- a/tests/generic/472
> +++ b/tests/generic/472
> @@ -33,8 +33,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
>
> rm -f $seqres.full
> _scratch_mkfs >>$seqres.full 2>&1
> diff --git a/tests/generic/495 b/tests/generic/495
> index 88df26c78ec2..63f45cf4b336 100755
> --- a/tests/generic/495
> +++ b/tests/generic/495
> @@ -31,8 +31,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
>
> _scratch_mkfs >> $seqres.full 2>&1
> _scratch_mount
> diff --git a/tests/generic/496 b/tests/generic/496
> index 3083eef0bebc..0e214909f596 100755
> --- a/tests/generic/496
> +++ b/tests/generic/496
> @@ -34,8 +34,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
> _require_xfs_io_command "falloc"
>
> rm -f $seqres.full
> diff --git a/tests/generic/497 b/tests/generic/497
> index 3d5502ef7c08..d9f9b7521eff 100755
> --- a/tests/generic/497
> +++ b/tests/generic/497
> @@ -34,8 +34,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
> _require_xfs_io_command "fcollapse"
>
> rm -f $seqres.full
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/3] common: add _require_test_swapfile
2018-12-03 6:42 ` [PATCH 1/3] common: add _require_test_swapfile Dave Chinner
2018-12-03 16:43 ` Darrick J. Wong
@ 2018-12-13 12:16 ` Xiao Yang
2018-12-18 21:54 ` Dave Chinner
1 sibling, 1 reply; 19+ messages in thread
From: Xiao Yang @ 2018-12-13 12:16 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On 2018/12/03 14:42, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Because we can host swap files on the test device, not just the
> scratch device.
>
> Also, move the tests for the utilities needed to manipulate swap
> files into the functions that test whether swap files are supported
> so they are checked for existence /before/ we try to us them. This
> fixes all the tests that currently check for these utilities
> manually /after/ checking if swapfiles are supported.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> common/rc | 29 +++++++++++++++++++++--------
> tests/generic/472 | 2 --
> tests/generic/495 | 2 --
> tests/generic/496 | 2 --
> tests/generic/497 | 2 --
> 5 files changed, 21 insertions(+), 16 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index ecb17380bad8..5b344b25012b 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2214,22 +2214,35 @@ _format_swapfile() {
> }
>
> # Check that the filesystem supports swapfiles
> -_require_scratch_swapfile()
> +_require_swapfile()
> {
> - _require_scratch
> + dir=$1
>
> - _scratch_mkfs >/dev/null
> - _scratch_mount
> + # fstests also has custom binaries for mkswap/swapon
> + _require_test_program mkswap
> + _require_test_program swapon
Hi Dave,
Is it necessary to include the check for custom mkswap/swapon in _require_swapfile()?
In fact, some tests calling _require_scratch_swapfile() just use default mkswap/swapon
command(e.g. generic/356, generic/357, generic/493, generic/494). I perfer to keep it
in separate tests.
Other than that, it looks good to me.
Best Regards,
Xiao Yang
>
> # Minimum size for mkswap is 10 pages
> - _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> + _format_swapfile "$dir/swap" $(($(get_page_size) * 10))
>
> - if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then
> - _scratch_unmount
> + if ! swapon "$dir/swap" >/dev/null 2>&1; then
> _notrun "swapfiles are not supported"
> fi
>
> - swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1
> + swapoff "$dir/swap" >/dev/null 2>&1
> +}
> +
> +_require_test_swapfile()
> +{
> + _require_swapfile $TEST_DIR
> +}
> +
> +_require_scratch_swapfile()
> +{
> + _require_scratch
> + _scratch_mkfs >/dev/null
> + _scratch_mount
> + _require_swapfile $SCRATCH_MNT
> _scratch_unmount
> }
>
> diff --git a/tests/generic/472 b/tests/generic/472
> index aba4a00719bc..d598eef37997 100755
> --- a/tests/generic/472
> +++ b/tests/generic/472
> @@ -33,8 +33,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
>
> rm -f $seqres.full
> _scratch_mkfs >>$seqres.full 2>&1
> diff --git a/tests/generic/495 b/tests/generic/495
> index 88df26c78ec2..63f45cf4b336 100755
> --- a/tests/generic/495
> +++ b/tests/generic/495
> @@ -31,8 +31,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
>
> _scratch_mkfs >> $seqres.full 2>&1
> _scratch_mount
> diff --git a/tests/generic/496 b/tests/generic/496
> index 3083eef0bebc..0e214909f596 100755
> --- a/tests/generic/496
> +++ b/tests/generic/496
> @@ -34,8 +34,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
> _require_xfs_io_command "falloc"
>
> rm -f $seqres.full
> diff --git a/tests/generic/497 b/tests/generic/497
> index 3d5502ef7c08..d9f9b7521eff 100755
> --- a/tests/generic/497
> +++ b/tests/generic/497
> @@ -34,8 +34,6 @@ rm -f $seqres.full
> _supported_fs generic
> _supported_os Linux
> _require_scratch_swapfile
> -_require_test_program mkswap
> -_require_test_program swapon
> _require_xfs_io_command "fcollapse"
>
> rm -f $seqres.full
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/3] common: add _require_test_swapfile
2018-12-13 12:16 ` Xiao Yang
@ 2018-12-18 21:54 ` Dave Chinner
0 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-18 21:54 UTC (permalink / raw)
To: Xiao Yang; +Cc: fstests
On Thu, Dec 13, 2018 at 08:16:26PM +0800, Xiao Yang wrote:
> On 2018/12/03 14:42, Dave Chinner wrote:
>
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Because we can host swap files on the test device, not just the
> > scratch device.
> >
> > Also, move the tests for the utilities needed to manipulate swap
> > files into the functions that test whether swap files are supported
> > so they are checked for existence /before/ we try to us them. This
> > fixes all the tests that currently check for these utilities
> > manually /after/ checking if swapfiles are supported.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> > common/rc | 29 +++++++++++++++++++++--------
> > tests/generic/472 | 2 --
> > tests/generic/495 | 2 --
> > tests/generic/496 | 2 --
> > tests/generic/497 | 2 --
> > 5 files changed, 21 insertions(+), 16 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index ecb17380bad8..5b344b25012b 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2214,22 +2214,35 @@ _format_swapfile() {
> > }
> >
> > # Check that the filesystem supports swapfiles
> > -_require_scratch_swapfile()
> > +_require_swapfile()
> > {
> > - _require_scratch
> > + dir=$1
> >
> > - _scratch_mkfs >/dev/null
> > - _scratch_mount
> > + # fstests also has custom binaries for mkswap/swapon
> > + _require_test_program mkswap
> > + _require_test_program swapon
> Hi Dave,
>
> Is it necessary to include the check for custom mkswap/swapon in _require_swapfile()?
> In fact, some tests calling _require_scratch_swapfile() just use default mkswap/swapon
> command(e.g. generic/356, generic/357, generic/493, generic/494). I perfer to keep it
> in separate tests.
Those binaries are built by fstests, so there is absolutely no harm
in requiring them. If they didn't get built, then the fstests
install is in no shape to be run because it wasn't built correctly.
And by adding them here, we do not need to clutter up tests with
lots different requires - we just say "we require swapfile support"
and that pulls in everything swapfile related. This makes the tests
easier to review and maintain because there is less that needs to be
done to write a swapfile related test.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail
2018-12-03 6:42 [PATCH 0/3] fstests: copy_file_range() bounds testing Dave Chinner
2018-12-03 6:42 ` [PATCH 1/3] common: add _require_test_swapfile Dave Chinner
@ 2018-12-03 6:42 ` Dave Chinner
2018-12-03 7:30 ` Amir Goldstein
` (2 more replies)
2018-12-03 6:42 ` [PATCH 3/3] generic: copy_file_range bounds test Dave Chinner
2018-12-03 16:41 ` [PATCH 0/3] fstests: copy_file_range() bounds testing Darrick J. Wong
3 siblings, 3 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-03 6:42 UTC (permalink / raw)
To: fstests
From: Dave Chinner <dchinner@redhat.com>
As per the copy_file_range() man page:
EINVAL
Requested range extends beyond the end of the source file;
.....
These tests actually attempt to copy beyond the end of the source
file and so should fail with EINVAL. The kernel does not check this
and so needs fixing. These operations should fail, so fix the tests.
Also move the tests to the copy_range group so they are distinct
from the copy group which refers to xfs_copy tests.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
tests/generic/430 | 5 +----
tests/generic/430.out | 6 ++----
tests/generic/431 | 1 +
tests/generic/431.out | 1 +
tests/generic/434 | 5 +++--
tests/generic/434.out | 4 ++--
tests/generic/group | 10 +++++-----
7 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/tests/generic/430 b/tests/generic/430
index 1b11f60df059..ab15ce0e37d4 100755
--- a/tests/generic/430
+++ b/tests/generic/430
@@ -70,11 +70,8 @@ cmp -n 1000 $testdir/file $testdir/end 4000
echo "md5sums after copying end:"
md5sum $testdir/{file,end} | _filter_test_dir
-echo "Copy beyond end of original file"
+echo "Copy beyond end of original file - should fail"
$XFS_IO_PROG -f -c "copy_range -s 4000 -l 2000 $testdir/file" "$testdir/beyond"
-cmp -n 1000 $testdir/file $testdir/beyond 4000
-echo "md5sums after copying beyond:"
-md5sum $testdir/{file,beyond} | _filter_test_dir
echo "Copy creates hole in target file"
$XFS_IO_PROG -f -c "copy_range -s 1000 -l 3000 -d 1000 $testdir/file" "$testdir/hole"
diff --git a/tests/generic/430.out b/tests/generic/430.out
index 4b4ca75d5980..8dd7870658cb 100644
--- a/tests/generic/430.out
+++ b/tests/generic/430.out
@@ -15,10 +15,8 @@ Copy end of original file
md5sums after copying end:
e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
e68d4a150c4e42f4f9ea3ffe4c9cf4ed TEST_DIR/test-430/end
-Copy beyond end of original file
-md5sums after copying beyond:
-e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
-e68d4a150c4e42f4f9ea3ffe4c9cf4ed TEST_DIR/test-430/beyond
+Copy beyond end of original file - should fail
+copy_range: Invalid argument
Copy creates hole in target file
md5sums after creating hole:
e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
diff --git a/tests/generic/431 b/tests/generic/431
index f04ae2152bae..273068c78d40 100755
--- a/tests/generic/431
+++ b/tests/generic/431
@@ -52,6 +52,7 @@ $XFS_IO_PROG -f -c "copy_range -s 2 -l 1 $testdir/file" "$testdir/c"
$XFS_IO_PROG -f -c "copy_range -s 3 -l 1 $testdir/file" "$testdir/d"
$XFS_IO_PROG -f -c "copy_range -s 4 -l 1 $testdir/file" "$testdir/e"
$XFS_IO_PROG -f -c "copy_range -s 4 -l 1 -d 1 $testdir/file" "$testdir/f"
+# this should fail with EINVAL and leave an empty file behind.
$XFS_IO_PROG -f -c "copy_range -s 5 -l 1 $testdir/file" "$testdir/g"
echo -n "a" | cmp $testdir/a
echo -n "b" | cmp $testdir/b
diff --git a/tests/generic/431.out b/tests/generic/431.out
index 978c4a1b56fc..834a5db6f56f 100644
--- a/tests/generic/431.out
+++ b/tests/generic/431.out
@@ -4,6 +4,7 @@ Original md5sums:
ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/file
ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/copy
Small copies from various points in the original file
+copy_range: Invalid argument
md5sums after small copies
ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/file
0cc175b9c0f1b6a831c399e269772661 TEST_DIR/test-431/a
diff --git a/tests/generic/434 b/tests/generic/434
index 032f933dff76..4bcaf9bac04b 100755
--- a/tests/generic/434
+++ b/tests/generic/434
@@ -41,15 +41,16 @@ $XFS_IO_PROG -f -c 'pwrite -S 0x61 0 1000' $testdir/file >> $seqres.full 2>&1
mknod $testdir/dev1 c 1 3
mkfifo $testdir/fifo
-echo "Try to copy when source pos > source size"
+echo "Try to copy when source pos > source size - should fail"
$XFS_IO_PROG -f -c "copy_range -s 1000 -l 100 $testdir/file" "$testdir/copy"
-md5sum $testdir/copy | _filter_test_dir
echo "Try to copy to a read-only file"
+rm -f $testdir/copy
$XFS_IO_PROG -r -f -c "copy_range -s 0 -l 100 $testdir/file" "$testdir/copy"
md5sum $testdir/copy | _filter_test_dir
echo "Try to copy to an append-only file"
+rm -f $testdir/copy
$XFS_IO_PROG -a -f -c "copy_range -s 0 -l 100 $testdir/file" "$testdir/copy"
md5sum $testdir/copy | _filter_test_dir
diff --git a/tests/generic/434.out b/tests/generic/434.out
index 4532ebbaf864..3659592b949b 100644
--- a/tests/generic/434.out
+++ b/tests/generic/434.out
@@ -1,7 +1,7 @@
QA output created by 434
Create the original files
-Try to copy when source pos > source size
-d41d8cd98f00b204e9800998ecf8427e TEST_DIR/test-434/copy
+Try to copy when source pos > source size - should fail
+copy_range: Invalid argument
Try to copy to a read-only file
copy_range: Bad file descriptor
d41d8cd98f00b204e9800998ecf8427e TEST_DIR/test-434/copy
diff --git a/tests/generic/group b/tests/generic/group
index 58318608e7a9..cc05792ba3b6 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -432,11 +432,11 @@
427 auto quick aio rw
428 auto quick dax
429 auto encrypt
-430 auto quick copy
-431 auto quick copy
-432 auto quick copy
-433 auto quick copy
-434 auto quick copy
+430 auto quick copy_range
+431 auto quick copy_range
+432 auto quick copy_range
+433 auto quick copy_range
+434 auto quick copy_range
435 auto encrypt
436 auto quick rw
437 auto quick dax
--
2.19.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail
2018-12-03 6:42 ` [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail Dave Chinner
@ 2018-12-03 7:30 ` Amir Goldstein
2018-12-03 8:10 ` Dave Chinner
2018-12-03 16:47 ` Darrick J. Wong
2018-12-05 22:23 ` Dave Chinner
2 siblings, 1 reply; 19+ messages in thread
From: Amir Goldstein @ 2018-12-03 7:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, linux-api
On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> As per the copy_file_range() man page:
>
> EINVAL
> Requested range extends beyond the end of the source file;
> .....
>
> These tests actually attempt to copy beyond the end of the source
> file and so should fail with EINVAL. The kernel does not check this
> and so needs fixing. These operations should fail, so fix the tests.
>
[cc: linux-api]
Probably better to discuss this on the kernel patch itself, but well...
How confident are you about this change not breaking existing programs?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail
2018-12-03 7:30 ` Amir Goldstein
@ 2018-12-03 8:10 ` Dave Chinner
0 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-03 8:10 UTC (permalink / raw)
To: Amir Goldstein; +Cc: fstests, linux-api
On Mon, Dec 03, 2018 at 09:30:58AM +0200, Amir Goldstein wrote:
> On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > As per the copy_file_range() man page:
> >
> > EINVAL
> > Requested range extends beyond the end of the source file;
> > .....
> >
> > These tests actually attempt to copy beyond the end of the source
> > file and so should fail with EINVAL. The kernel does not check this
> > and so needs fixing. These operations should fail, so fix the tests.
> >
>
> [cc: linux-api]
>
> Probably better to discuss this on the kernel patch itself, but well...
I'm getting there - the problem is I need to refer to the tests in
the series description that these changes are made.
> How confident are you about this change not breaking existing programs?
My care factor is way below zero. The API implementation is so
broken right now that fixing it is almost guaranteed to break
something somewhere.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail
2018-12-03 6:42 ` [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail Dave Chinner
2018-12-03 7:30 ` Amir Goldstein
@ 2018-12-03 16:47 ` Darrick J. Wong
2018-12-05 22:23 ` Dave Chinner
2 siblings, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2018-12-03 16:47 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Mon, Dec 03, 2018 at 05:42:55PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> As per the copy_file_range() man page:
>
> EINVAL
> Requested range extends beyond the end of the source file;
> .....
>
> These tests actually attempt to copy beyond the end of the source
> file and so should fail with EINVAL. The kernel does not check this
> and so needs fixing. These operations should fail, so fix the tests.
>
> Also move the tests to the copy_range group so they are distinct
> from the copy group which refers to xfs_copy tests.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Looks ok; sorry I didn't notice this on the original patch.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> tests/generic/430 | 5 +----
> tests/generic/430.out | 6 ++----
> tests/generic/431 | 1 +
> tests/generic/431.out | 1 +
> tests/generic/434 | 5 +++--
> tests/generic/434.out | 4 ++--
> tests/generic/group | 10 +++++-----
> 7 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/tests/generic/430 b/tests/generic/430
> index 1b11f60df059..ab15ce0e37d4 100755
> --- a/tests/generic/430
> +++ b/tests/generic/430
> @@ -70,11 +70,8 @@ cmp -n 1000 $testdir/file $testdir/end 4000
> echo "md5sums after copying end:"
> md5sum $testdir/{file,end} | _filter_test_dir
>
> -echo "Copy beyond end of original file"
> +echo "Copy beyond end of original file - should fail"
> $XFS_IO_PROG -f -c "copy_range -s 4000 -l 2000 $testdir/file" "$testdir/beyond"
> -cmp -n 1000 $testdir/file $testdir/beyond 4000
> -echo "md5sums after copying beyond:"
> -md5sum $testdir/{file,beyond} | _filter_test_dir
>
> echo "Copy creates hole in target file"
> $XFS_IO_PROG -f -c "copy_range -s 1000 -l 3000 -d 1000 $testdir/file" "$testdir/hole"
> diff --git a/tests/generic/430.out b/tests/generic/430.out
> index 4b4ca75d5980..8dd7870658cb 100644
> --- a/tests/generic/430.out
> +++ b/tests/generic/430.out
> @@ -15,10 +15,8 @@ Copy end of original file
> md5sums after copying end:
> e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
> e68d4a150c4e42f4f9ea3ffe4c9cf4ed TEST_DIR/test-430/end
> -Copy beyond end of original file
> -md5sums after copying beyond:
> -e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
> -e68d4a150c4e42f4f9ea3ffe4c9cf4ed TEST_DIR/test-430/beyond
> +Copy beyond end of original file - should fail
> +copy_range: Invalid argument
> Copy creates hole in target file
> md5sums after creating hole:
> e11fbace556cba26bf0076e74cab90a3 TEST_DIR/test-430/file
> diff --git a/tests/generic/431 b/tests/generic/431
> index f04ae2152bae..273068c78d40 100755
> --- a/tests/generic/431
> +++ b/tests/generic/431
> @@ -52,6 +52,7 @@ $XFS_IO_PROG -f -c "copy_range -s 2 -l 1 $testdir/file" "$testdir/c"
> $XFS_IO_PROG -f -c "copy_range -s 3 -l 1 $testdir/file" "$testdir/d"
> $XFS_IO_PROG -f -c "copy_range -s 4 -l 1 $testdir/file" "$testdir/e"
> $XFS_IO_PROG -f -c "copy_range -s 4 -l 1 -d 1 $testdir/file" "$testdir/f"
> +# this should fail with EINVAL and leave an empty file behind.
> $XFS_IO_PROG -f -c "copy_range -s 5 -l 1 $testdir/file" "$testdir/g"
> echo -n "a" | cmp $testdir/a
> echo -n "b" | cmp $testdir/b
> diff --git a/tests/generic/431.out b/tests/generic/431.out
> index 978c4a1b56fc..834a5db6f56f 100644
> --- a/tests/generic/431.out
> +++ b/tests/generic/431.out
> @@ -4,6 +4,7 @@ Original md5sums:
> ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/file
> ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/copy
> Small copies from various points in the original file
> +copy_range: Invalid argument
> md5sums after small copies
> ab56b4d92b40713acc5af89985d4b786 TEST_DIR/test-431/file
> 0cc175b9c0f1b6a831c399e269772661 TEST_DIR/test-431/a
> diff --git a/tests/generic/434 b/tests/generic/434
> index 032f933dff76..4bcaf9bac04b 100755
> --- a/tests/generic/434
> +++ b/tests/generic/434
> @@ -41,15 +41,16 @@ $XFS_IO_PROG -f -c 'pwrite -S 0x61 0 1000' $testdir/file >> $seqres.full 2>&1
> mknod $testdir/dev1 c 1 3
> mkfifo $testdir/fifo
>
> -echo "Try to copy when source pos > source size"
> +echo "Try to copy when source pos > source size - should fail"
> $XFS_IO_PROG -f -c "copy_range -s 1000 -l 100 $testdir/file" "$testdir/copy"
> -md5sum $testdir/copy | _filter_test_dir
>
> echo "Try to copy to a read-only file"
> +rm -f $testdir/copy
> $XFS_IO_PROG -r -f -c "copy_range -s 0 -l 100 $testdir/file" "$testdir/copy"
> md5sum $testdir/copy | _filter_test_dir
>
> echo "Try to copy to an append-only file"
> +rm -f $testdir/copy
> $XFS_IO_PROG -a -f -c "copy_range -s 0 -l 100 $testdir/file" "$testdir/copy"
> md5sum $testdir/copy | _filter_test_dir
>
> diff --git a/tests/generic/434.out b/tests/generic/434.out
> index 4532ebbaf864..3659592b949b 100644
> --- a/tests/generic/434.out
> +++ b/tests/generic/434.out
> @@ -1,7 +1,7 @@
> QA output created by 434
> Create the original files
> -Try to copy when source pos > source size
> -d41d8cd98f00b204e9800998ecf8427e TEST_DIR/test-434/copy
> +Try to copy when source pos > source size - should fail
> +copy_range: Invalid argument
> Try to copy to a read-only file
> copy_range: Bad file descriptor
> d41d8cd98f00b204e9800998ecf8427e TEST_DIR/test-434/copy
> diff --git a/tests/generic/group b/tests/generic/group
> index 58318608e7a9..cc05792ba3b6 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -432,11 +432,11 @@
> 427 auto quick aio rw
> 428 auto quick dax
> 429 auto encrypt
> -430 auto quick copy
> -431 auto quick copy
> -432 auto quick copy
> -433 auto quick copy
> -434 auto quick copy
> +430 auto quick copy_range
> +431 auto quick copy_range
> +432 auto quick copy_range
> +433 auto quick copy_range
> +434 auto quick copy_range
> 435 auto encrypt
> 436 auto quick rw
> 437 auto quick dax
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail
2018-12-03 6:42 ` [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail Dave Chinner
2018-12-03 7:30 ` Amir Goldstein
2018-12-03 16:47 ` Darrick J. Wong
@ 2018-12-05 22:23 ` Dave Chinner
2 siblings, 0 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-05 22:23 UTC (permalink / raw)
To: fstests
On Mon, Dec 03, 2018 at 05:42:55PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> As per the copy_file_range() man page:
>
> EINVAL
> Requested range extends beyond the end of the source file;
> .....
>
> These tests actually attempt to copy beyond the end of the source
> file and so should fail with EINVAL. The kernel does not check this
> and so needs fixing. These operations should fail, so fix the tests.
>
> Also move the tests to the copy_range group so they are distinct
> from the copy group which refers to xfs_copy tests.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Self NACK on this - the behaviour for this corner case we've decided
on is going to be different to what the man page says, so this patch
is going to have to be reworked.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 6:42 [PATCH 0/3] fstests: copy_file_range() bounds testing Dave Chinner
2018-12-03 6:42 ` [PATCH 1/3] common: add _require_test_swapfile Dave Chinner
2018-12-03 6:42 ` [PATCH 2/3] generic/43[014]: copy_range beyond source EOF should fail Dave Chinner
@ 2018-12-03 6:42 ` Dave Chinner
2018-12-03 7:25 ` Amir Goldstein
` (2 more replies)
2018-12-03 16:41 ` [PATCH 0/3] fstests: copy_file_range() bounds testing Darrick J. Wong
3 siblings, 3 replies; 19+ messages in thread
From: Dave Chinner @ 2018-12-03 6:42 UTC (permalink / raw)
To: fstests
From: Dave Chinner <dchinner@redhat.com>
Test that copy_file_range will return the correct errors for various
error conditions and boundary constraints.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
tests/generic/530 | 165 ++++++++++++++++++++++++++++++++++++++++++
tests/generic/530.out | 62 ++++++++++++++++
tests/generic/group | 1 +
3 files changed, 228 insertions(+)
create mode 100755 tests/generic/530
create mode 100644 tests/generic/530.out
diff --git a/tests/generic/530 b/tests/generic/530
new file mode 100755
index 000000000000..42243cc70914
--- /dev/null
+++ b/tests/generic/530
@@ -0,0 +1,165 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved.
+#
+# FS QA Test No. 530
+#
+# Exercise copy_file_range() syscall error conditions.
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1 # failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 7 15
+
+_cleanup()
+{
+ cd /
+ rm -rf $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# real QA test starts here
+_supported_os Linux
+_supported_fs generic
+
+rm -f $seqres.full
+
+_require_test
+_require_scratch
+_require_xfs_io_command "copy_range"
+_require_user
+_require_test_swapfile
+
+_scratch_mkfs 2>&1 >> $seqres.full
+_scratch_mount
+
+
+testdir=$TEST_DIR/test-$seq
+rm -rf $testdir
+mkdir $testdir
+rm -f $seqres.full
+
+$XFS_IO_PROG -f -c "pwrite -S 0x61 0 128k" $testdir/file >> $seqres.full 2>&1
+chmod 777 $testdir/file
+
+echo swap files return ETXTBUSY
+_format_swapfile $testdir/swapfile 16m
+swapon $testdir/swapfile
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/file" $testdir/swapfile
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/swapfile" $testdir/copy
+swapoff $testdir/swapfile
+
+# we have to open the file to be immutable rw and hold it open over the
+# chattr command to set it immutable, otherwise we won't be able to open it for
+# writing after it's been made immutable. (i.e. would exercise file mode checks,
+# not immutable inode flag checks).
+echo
+echo immutable file returns EPERM
+$XFS_IO_PROG -f -c "pwrite -S 0x61 0 64k" -c fsync $testdir/immutable | _filter_xfs_io
+$XFS_IO_PROG -f -c "chattr +i" -c "copy_range -l 32k $testdir/file" $testdir/immutable
+$XFS_IO_PROG -f -r -c "chattr -i" $testdir/immutable
+rm -f $testdir/immutable
+
+# can't test this as root, because root is allowed to write to files do not
+# have write permission bits set.
+echo
+echo no write perms on destination returns EACCES
+chown $qa_user:12345 $testdir/copy
+su $qa_user -c "$XFS_IO_PROG -f -c 'chmod -r' -c 'copy_range -l 32k $testdir/file' $testdir/copy"
+rm -f $testdir/copy
+
+echo
+echo source range overlaps EOF returns EINVAL
+$XFS_IO_PROG -f -c "copy_range -s 112k -l 32k $testdir/file" $testdir/copy
+
+echo
+echo source range beyond EOF returns EINVAL
+$XFS_IO_PROG -f -c "copy_range -s 128k -l 32k $testdir/file" $testdir/copy
+
+echo
+echo source range overlaps destination range in same file returns EINVAL
+$XFS_IO_PROG -f -c "copy_range -s 32k -d 48k -l 32k $testdir/file" $testdir/file
+
+echo
+echo destination file O_RDONLY returns EBADF
+$XFS_IO_PROG -f -r -c "copy_range -l 32k $testdir/file" $testdir/copy
+
+echo
+echo destination file O_APPEND returns EBADF
+$XFS_IO_PROG -f -a -c "copy_range -l 32k $testdir/file" $testdir/copy
+
+echo
+echo source/destination as directory returns EISDIR
+$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir" $testdir/copy
+
+echo
+echo source/destination as blkdev returns EINVAL
+mknod $testdir/dev1 b 1 3
+$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev1
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev1" $testdir/copy
+
+echo
+echo source/destination as chardev returns EINVAL
+mknod $testdir/dev2 c 1 3
+$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev2
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev2" $testdir/copy
+
+echo
+echo source/destination as FIFO returns EINVAL
+mkfifo $testdir/fifo
+$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/fifo
+$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/fifo" $testdir/copy
+
+max_off=$((8 * 2**60 - 65536 - 1))
+min_off=65537
+
+echo
+echo length beyond 8EiB wraps around 0 returns EOVERFLOW
+$XFS_IO_PROG -f -c "copy_range -l 10e -s $max_off $testdir/file" $testdir/copy
+$XFS_IO_PROG -f -c "copy_range -l 10e -d $max_off $testdir/file" $testdir/copy
+
+echo
+echo source range beyond 8EiB returns EINVAL
+$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/file
+$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/copy
+$XFS_IO_PROG -c "copy_range -s $max_off -l $min_off -d 0 $testdir/file" $testdir/copy
+$XFS_IO_PROG -c "copy_range -s $min_off -l $max_off -d 0 $testdir/file" $testdir/copy
+
+echo
+echo destination range beyond 8TiB returns EFBIG
+$XFS_IO_PROG -c "copy_range -l $min_off -s 0 -d $max_off $testdir/file" $testdir/copy
+
+echo
+echo destination larger than rlimit returns EFBIG
+rm -f $testdir/copy
+$XFS_IO_PROG -c "truncate 128k" $testdir/file
+
+# need a wrapper so the "File size limit exceeded" error can be filtered
+do_rlimit_copy()
+{
+ $XFS_IO_PROG -f -c "copy_range -l 32k -s 0 -d 16m $testdir/file" $testdir/copy
+}
+
+ulimit -f $((8 * 1024))
+ulimit -c 0
+do_rlimit_copy 2>&1 | grep -o "File size limit exceeded"
+ulimit -f unlimited
+
+echo
+echo copy across devices
+$XFS_IO_PROG -f -c "copy_range -l 128k $testdir/file" $SCRATCH_MNT/copy
+cmp $testdir/file $SCRATCH_MNT/copy
+echo "md5sums after xdev copy:"
+md5sum $testdir/file $SCRATCH_MNT/copy | _filter_test_dir | _filter_scratch
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/530.out b/tests/generic/530.out
new file mode 100644
index 000000000000..c433fb989637
--- /dev/null
+++ b/tests/generic/530.out
@@ -0,0 +1,62 @@
+QA output created by 530
+swap files return ETXTBUSY
+copy_range: Text file busy
+copy_range: Text file busy
+
+immutable file returns EPERM
+wrote 65536/65536 bytes at offset 0
+XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+copy_range: Operation not permitted
+
+no write perms on destination returns EACCES
+copy_range: Permission denied
+
+source range overlaps EOF returns EINVAL
+copy_range: Invalid argument
+
+source range beyond EOF returns EINVAL
+copy_range: Invalid argument
+
+source range overlaps destination range in same file returns EINVAL
+copy_range: Invalid argument
+
+destination file O_RDONLY returns EBADF
+copy_range: Bad file descriptor
+
+destination file O_APPEND returns EBADF
+copy_range: Bad file descriptor
+
+source/destination as directory returns EISDIR
+copy_range: Is a directory
+copy_range: Is a directory
+
+source/destination as blkdev returns EINVAL
+copy_range: Invalid argument
+copy_range: Invalid argument
+
+source/destination as chardev returns EINVAL
+copy_range: Invalid argument
+copy_range: Invalid argument
+
+source/destination as FIFO returns EINVAL
+copy_range: Invalid argument
+copy_range: Invalid argument
+
+length beyond 8EiB wraps around 0 returns EOVERFLOW
+copy_range: Value too large for defined data type
+copy_range: Value too large for defined data type
+
+source range beyond 8EiB returns EINVAL
+copy_range: Invalid argument
+copy_range: Invalid argument
+
+destination range beyond 8TiB returns EFBIG
+copy_range: File too large
+
+destination larger than rlimit returns EFBIG
+File size limit exceeded
+
+copy across devices
+md5sums after xdev copy:
+81615449a98aaaad8dc179b3bec87f38 TEST_DIR/test-530/file
+81615449a98aaaad8dc179b3bec87f38 SCRATCH_MNT/copy
diff --git a/tests/generic/group b/tests/generic/group
index cc05792ba3b6..5ba1280aa3e6 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -523,3 +523,4 @@
517 auto quick dedupe clone
518 auto quick clone
519 auto quick
+530 auto quick copy_range
--
2.19.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 6:42 ` [PATCH 3/3] generic: copy_file_range bounds test Dave Chinner
@ 2018-12-03 7:25 ` Amir Goldstein
2018-12-03 8:17 ` Dave Chinner
2018-12-03 16:58 ` Darrick J. Wong
2019-05-21 5:33 ` Amir Goldstein
2 siblings, 1 reply; 19+ messages in thread
From: Amir Goldstein @ 2018-12-03 7:25 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Olga Kornievskaia
On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Test that copy_file_range will return the correct errors for various
> error conditions and boundary constraints.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> tests/generic/530 | 165 ++++++++++++++++++++++++++++++++++++++++++
> tests/generic/530.out | 62 ++++++++++++++++
> tests/generic/group | 1 +
> 3 files changed, 228 insertions(+)
> create mode 100755 tests/generic/530
> create mode 100644 tests/generic/530.out
>
> diff --git a/tests/generic/530 b/tests/generic/530
> new file mode 100755
> index 000000000000..42243cc70914
> --- /dev/null
> +++ b/tests/generic/530
> @@ -0,0 +1,165 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved.
> +#
> +# FS QA Test No. 530
> +#
> +# Exercise copy_file_range() syscall error conditions.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 7 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -rf $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_os Linux
> +_supported_fs generic
> +
> +rm -f $seqres.full
> +
> +_require_test
> +_require_scratch
> +_require_xfs_io_command "copy_range"
> +_require_user
> +_require_test_swapfile
> +
> +_scratch_mkfs 2>&1 >> $seqres.full
> +_scratch_mount
> +
> +
> +testdir=$TEST_DIR/test-$seq
> +rm -rf $testdir
> +mkdir $testdir
> +rm -f $seqres.full
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 128k" $testdir/file >> $seqres.full 2>&1
> +chmod 777 $testdir/file
> +
> +echo swap files return ETXTBUSY
> +_format_swapfile $testdir/swapfile 16m
> +swapon $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/file" $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/swapfile" $testdir/copy
> +swapoff $testdir/swapfile
> +
> +# we have to open the file to be immutable rw and hold it open over the
> +# chattr command to set it immutable, otherwise we won't be able to open it for
> +# writing after it's been made immutable. (i.e. would exercise file mode checks,
> +# not immutable inode flag checks).
> +echo
> +echo immutable file returns EPERM
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 64k" -c fsync $testdir/immutable | _filter_xfs_io
> +$XFS_IO_PROG -f -c "chattr +i" -c "copy_range -l 32k $testdir/file" $testdir/immutable
> +$XFS_IO_PROG -f -r -c "chattr -i" $testdir/immutable
> +rm -f $testdir/immutable
> +
> +# can't test this as root, because root is allowed to write to files do not
> +# have write permission bits set.
> +echo
> +echo no write perms on destination returns EACCES
> +chown $qa_user:12345 $testdir/copy
> +su $qa_user -c "$XFS_IO_PROG -f -c 'chmod -r' -c 'copy_range -l 32k $testdir/file' $testdir/copy"
> +rm -f $testdir/copy
> +
> +echo
> +echo source range overlaps EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 112k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 128k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range overlaps destination range in same file returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 32k -d 48k -l 32k $testdir/file" $testdir/file
> +
> +echo
> +echo destination file O_RDONLY returns EBADF
> +$XFS_IO_PROG -f -r -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo destination file O_APPEND returns EBADF
> +$XFS_IO_PROG -f -a -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source/destination as directory returns EISDIR
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir" $testdir/copy
> +
> +echo
> +echo source/destination as blkdev returns EINVAL
> +mknod $testdir/dev1 b 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev1
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev1" $testdir/copy
> +
> +echo
> +echo source/destination as chardev returns EINVAL
> +mknod $testdir/dev2 c 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev2
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev2" $testdir/copy
> +
> +echo
> +echo source/destination as FIFO returns EINVAL
> +mkfifo $testdir/fifo
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/fifo
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/fifo" $testdir/copy
> +
> +max_off=$((8 * 2**60 - 65536 - 1))
> +min_off=65537
> +
> +echo
> +echo length beyond 8EiB wraps around 0 returns EOVERFLOW
> +$XFS_IO_PROG -f -c "copy_range -l 10e -s $max_off $testdir/file" $testdir/copy
> +$XFS_IO_PROG -f -c "copy_range -l 10e -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond 8EiB returns EINVAL
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/file
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/copy
> +$XFS_IO_PROG -c "copy_range -s $max_off -l $min_off -d 0 $testdir/file" $testdir/copy
> +$XFS_IO_PROG -c "copy_range -s $min_off -l $max_off -d 0 $testdir/file" $testdir/copy
> +
> +echo
> +echo destination range beyond 8TiB returns EFBIG
> +$XFS_IO_PROG -c "copy_range -l $min_off -s 0 -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo destination larger than rlimit returns EFBIG
> +rm -f $testdir/copy
> +$XFS_IO_PROG -c "truncate 128k" $testdir/file
> +
> +# need a wrapper so the "File size limit exceeded" error can be filtered
> +do_rlimit_copy()
> +{
> + $XFS_IO_PROG -f -c "copy_range -l 32k -s 0 -d 16m $testdir/file" $testdir/copy
> +}
> +
> +ulimit -f $((8 * 1024))
> +ulimit -c 0
> +do_rlimit_copy 2>&1 | grep -o "File size limit exceeded"
> +ulimit -f unlimited
> +
> +echo
> +echo copy across devices
> +$XFS_IO_PROG -f -c "copy_range -l 128k $testdir/file" $SCRATCH_MNT/copy
> +cmp $testdir/file $SCRATCH_MNT/copy
> +echo "md5sums after xdev copy:"
> +md5sum $testdir/file $SCRATCH_MNT/copy | _filter_test_dir | _filter_scratch
> +
All the test cases above check for bugs, which I presume your kernel patch
series is aimed at fixing(?)
This one last test case tests for new functionality that is not
currently available
for any filesystem in upstream kernel.
Does your kernel patch set also add this functionality to xfs? to generic?
IMO, it would be better to split this test case for new functionality to a new
test, so that this one can pass on stable kernels once all the bug
fixes have been
applied.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 7:25 ` Amir Goldstein
@ 2018-12-03 8:17 ` Dave Chinner
2018-12-03 9:22 ` Amir Goldstein
0 siblings, 1 reply; 19+ messages in thread
From: Dave Chinner @ 2018-12-03 8:17 UTC (permalink / raw)
To: Amir Goldstein; +Cc: fstests, Olga Kornievskaia
On Mon, Dec 03, 2018 at 09:25:19AM +0200, Amir Goldstein wrote:
> On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Test that copy_file_range will return the correct errors for various
> > error conditions and boundary constraints.
....
>
> All the test cases above check for bugs, which I presume your kernel patch
> series is aimed at fixing(?)
Yes. Document the API (I have a man page patch) write the tests to
exercise correct API behaviour (these patches), fix the API
implementation until the tests start passing (still to be posted as
I wait for these to hit mailing list archives so I can point at
them).
> This one last test case tests for new functionality that is not
> currently available
> for any filesystem in upstream kernel.
Yup.
> Does your kernel patch set also add this functionality to xfs? to generic?
Yes and yes. overlay works, too, but I gave up caring about it
because it doesn't support the ioctls xfs_io uses in this test to
change open file state....
> IMO, it would be better to split this test case for new functionality to a new
> test, so that this one can pass on stable kernels once all the bug
> fixes have been
> applied.
Whatever. I'm tired, I've already put in 13 hours on this today and
I'm on the back of four 100+ hour weeks working on nothing but this
broken heap of crap.
Take it or leave it, because I'm just about burnt out on this
right now...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 8:17 ` Dave Chinner
@ 2018-12-03 9:22 ` Amir Goldstein
2018-12-03 13:15 ` Amir Goldstein
2019-05-13 6:03 ` Amir Goldstein
0 siblings, 2 replies; 19+ messages in thread
From: Amir Goldstein @ 2018-12-03 9:22 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Olga Kornievskaia
On Mon, Dec 3, 2018 at 10:17 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Mon, Dec 03, 2018 at 09:25:19AM +0200, Amir Goldstein wrote:
> > On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
> > >
> > > From: Dave Chinner <dchinner@redhat.com>
> > >
> > > Test that copy_file_range will return the correct errors for various
> > > error conditions and boundary constraints.
> ....
> >
> > All the test cases above check for bugs, which I presume your kernel patch
> > series is aimed at fixing(?)
>
> Yes. Document the API (I have a man page patch) write the tests to
> exercise correct API behaviour (these patches), fix the API
> implementation until the tests start passing (still to be posted as
> I wait for these to hit mailing list archives so I can point at
> them).
>
> > This one last test case tests for new functionality that is not
> > currently available
> > for any filesystem in upstream kernel.
>
> Yup.
>
> > Does your kernel patch set also add this functionality to xfs? to generic?
>
> Yes and yes. overlay works, too, but I gave up caring about it
> because it doesn't support the ioctls xfs_io uses in this test to
> change open file state....
Oh, not a problem. I can add FS_IOC_FS[SG]ETXATTR
to the overlayfs white list of ioctls.
That raises the issue that the test is missing:
_require_xfs_io_command "chattr" "-i"
and the chattr -i in cleanup().
>
> > IMO, it would be better to split this test case for new functionality to a new
> > test, so that this one can pass on stable kernels once all the bug
> > fixes have been
> > applied.
>
> Whatever. I'm tired, I've already put in 13 hours on this today and
> I'm on the back of four 100+ hour weeks working on nothing but this
> broken heap of crap.
>
> Take it or leave it, because I'm just about burnt out on this
> right now...
>
I hear you.
I personally have no problem with declaring the cross fs copy_range an
interface bug fix that can also go to stable. It's probably going to be harder
to get your interface fixes without it.
I will leave the call about insisting on separate test to Eryu.
If you want me to submit that separate test, I can do that.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 9:22 ` Amir Goldstein
@ 2018-12-03 13:15 ` Amir Goldstein
2019-05-13 6:03 ` Amir Goldstein
1 sibling, 0 replies; 19+ messages in thread
From: Amir Goldstein @ 2018-12-03 13:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Olga Kornievskaia
On Mon, Dec 3, 2018 at 11:22 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Dec 3, 2018 at 10:17 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Mon, Dec 03, 2018 at 09:25:19AM +0200, Amir Goldstein wrote:
> > > On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
> > > >
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > Test that copy_file_range will return the correct errors for various
> > > > error conditions and boundary constraints.
> > ....
> > >
> > > All the test cases above check for bugs, which I presume your kernel patch
> > > series is aimed at fixing(?)
> >
> > Yes. Document the API (I have a man page patch) write the tests to
> > exercise correct API behaviour (these patches), fix the API
> > implementation until the tests start passing (still to be posted as
> > I wait for these to hit mailing list archives so I can point at
> > them).
> >
> > > This one last test case tests for new functionality that is not
> > > currently available
> > > for any filesystem in upstream kernel.
> >
> > Yup.
> >
> > > Does your kernel patch set also add this functionality to xfs? to generic?
> >
> > Yes and yes. overlay works, too, but I gave up caring about it
> > because it doesn't support the ioctls xfs_io uses in this test to
> > change open file state....
>
> Oh, not a problem. I can add FS_IOC_FS[SG]ETXATTR
> to the overlayfs white list of ioctls.
>
> That raises the issue that the test is missing:
>
> _require_xfs_io_command "chattr" "-i"
>
Hmm.
_require_xfs_io_command "chattr" "+/-x"
is documented in doc/requirement-checking.txt, but that seems like
fake news.
Nevermind. I'll add support for it after implementing
FS_IOC_FS[SG]ETXATTR for overlayfs.
> and the chattr -i in cleanup().
>
That's still important of course.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 9:22 ` Amir Goldstein
2018-12-03 13:15 ` Amir Goldstein
@ 2019-05-13 6:03 ` Amir Goldstein
1 sibling, 0 replies; 19+ messages in thread
From: Amir Goldstein @ 2019-05-13 6:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests, Olga Kornievskaia, Darrick J. Wong
On Mon, Dec 3, 2018 at 11:22 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Mon, Dec 3, 2018 at 10:17 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Mon, Dec 03, 2018 at 09:25:19AM +0200, Amir Goldstein wrote:
> > > On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
> > > >
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > >
> > > > Test that copy_file_range will return the correct errors for various
> > > > error conditions and boundary constraints.
> > ....
> > >
> > > All the test cases above check for bugs, which I presume your kernel patch
> > > series is aimed at fixing(?)
> >
> > Yes. Document the API (I have a man page patch) write the tests to
> > exercise correct API behaviour (these patches), fix the API
> > implementation until the tests start passing (still to be posted as
> > I wait for these to hit mailing list archives so I can point at
> > them).
> >
> > > This one last test case tests for new functionality that is not
> > > currently available
> > > for any filesystem in upstream kernel.
> >
> > Yup.
> >
> > > Does your kernel patch set also add this functionality to xfs? to generic?
> >
> > Yes and yes. overlay works, too, but I gave up caring about it
> > because it doesn't support the ioctls xfs_io uses in this test to
> > change open file state....
>
> Oh, not a problem. I can add FS_IOC_FS[SG]ETXATTR
> to the overlayfs white list of ioctls.
>
> That raises the issue that the test is missing:
>
> _require_xfs_io_command "chattr" "-i"
>
> and the chattr -i in cleanup().
>
> >
> > > IMO, it would be better to split this test case for new functionality to a new
> > > test, so that this one can pass on stable kernels once all the bug
> > > fixes have been
> > > applied.
> >
> > Whatever. I'm tired, I've already put in 13 hours on this today and
> > I'm on the back of four 100+ hour weeks working on nothing but this
> > broken heap of crap.
> >
> > Take it or leave it, because I'm just about burnt out on this
> > right now...
> >
>
> I hear you.
>
> I personally have no problem with declaring the cross fs copy_range an
> interface bug fix that can also go to stable. It's probably going to be harder
> to get your interface fixes without it.
>
> I will leave the call about insisting on separate test to Eryu.
> If you want me to submit that separate test, I can do that.
>
Getting back to this (how time flies...)
Dave, if you don't mind I am going to re-post your tests splitting the
cross device copy test, since Darrick also echoed this request.
Cheers,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 6:42 ` [PATCH 3/3] generic: copy_file_range bounds test Dave Chinner
2018-12-03 7:25 ` Amir Goldstein
@ 2018-12-03 16:58 ` Darrick J. Wong
2019-05-21 5:33 ` Amir Goldstein
2 siblings, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2018-12-03 16:58 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Mon, Dec 03, 2018 at 05:42:56PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Test that copy_file_range will return the correct errors for various
> error conditions and boundary constraints.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> tests/generic/530 | 165 ++++++++++++++++++++++++++++++++++++++++++
> tests/generic/530.out | 62 ++++++++++++++++
> tests/generic/group | 1 +
> 3 files changed, 228 insertions(+)
> create mode 100755 tests/generic/530
> create mode 100644 tests/generic/530.out
>
> diff --git a/tests/generic/530 b/tests/generic/530
> new file mode 100755
> index 000000000000..42243cc70914
> --- /dev/null
> +++ b/tests/generic/530
> @@ -0,0 +1,165 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved.
> +#
> +# FS QA Test No. 530
> +#
> +# Exercise copy_file_range() syscall error conditions.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 7 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -rf $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_os Linux
> +_supported_fs generic
> +
> +rm -f $seqres.full
> +
> +_require_test
> +_require_scratch
> +_require_xfs_io_command "copy_range"
> +_require_user
> +_require_test_swapfile
> +
> +_scratch_mkfs 2>&1 >> $seqres.full
> +_scratch_mount
> +
> +
> +testdir=$TEST_DIR/test-$seq
> +rm -rf $testdir
> +mkdir $testdir
> +rm -f $seqres.full
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 128k" $testdir/file >> $seqres.full 2>&1
> +chmod 777 $testdir/file
> +
> +echo swap files return ETXTBUSY
> +_format_swapfile $testdir/swapfile 16m
> +swapon $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/file" $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/swapfile" $testdir/copy
> +swapoff $testdir/swapfile
> +
> +# we have to open the file to be immutable rw and hold it open over the
> +# chattr command to set it immutable, otherwise we won't be able to open it for
> +# writing after it's been made immutable. (i.e. would exercise file mode checks,
> +# not immutable inode flag checks).
> +echo
> +echo immutable file returns EPERM
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 64k" -c fsync $testdir/immutable | _filter_xfs_io
> +$XFS_IO_PROG -f -c "chattr +i" -c "copy_range -l 32k $testdir/file" $testdir/immutable
> +$XFS_IO_PROG -f -r -c "chattr -i" $testdir/immutable
> +rm -f $testdir/immutable
> +
> +# can't test this as root, because root is allowed to write to files do not
> +# have write permission bits set.
> +echo
> +echo no write perms on destination returns EACCES
> +chown $qa_user:12345 $testdir/copy
> +su $qa_user -c "$XFS_IO_PROG -f -c 'chmod -r' -c 'copy_range -l 32k $testdir/file' $testdir/copy"
> +rm -f $testdir/copy
> +
> +echo
> +echo source range overlaps EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 112k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 128k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range overlaps destination range in same file returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 32k -d 48k -l 32k $testdir/file" $testdir/file
> +
> +echo
> +echo destination file O_RDONLY returns EBADF
> +$XFS_IO_PROG -f -r -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo destination file O_APPEND returns EBADF
> +$XFS_IO_PROG -f -a -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source/destination as directory returns EISDIR
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir" $testdir/copy
> +
> +echo
> +echo source/destination as blkdev returns EINVAL
> +mknod $testdir/dev1 b 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev1
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev1" $testdir/copy
> +
> +echo
> +echo source/destination as chardev returns EINVAL
> +mknod $testdir/dev2 c 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev2
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev2" $testdir/copy
> +
> +echo
> +echo source/destination as FIFO returns EINVAL
> +mkfifo $testdir/fifo
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/fifo
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/fifo" $testdir/copy
> +
> +max_off=$((8 * 2**60 - 65536 - 1))
> +min_off=65537
> +
> +echo
> +echo length beyond 8EiB wraps around 0 returns EOVERFLOW
> +$XFS_IO_PROG -f -c "copy_range -l 10e -s $max_off $testdir/file" $testdir/copy
> +$XFS_IO_PROG -f -c "copy_range -l 10e -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond 8EiB returns EINVAL
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/file
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/copy
> +$XFS_IO_PROG -c "copy_range -s $max_off -l $min_off -d 0 $testdir/file" $testdir/copy
> +$XFS_IO_PROG -c "copy_range -s $min_off -l $max_off -d 0 $testdir/file" $testdir/copy
> +
> +echo
> +echo destination range beyond 8TiB returns EFBIG
> +$XFS_IO_PROG -c "copy_range -l $min_off -s 0 -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo destination larger than rlimit returns EFBIG
> +rm -f $testdir/copy
> +$XFS_IO_PROG -c "truncate 128k" $testdir/file
> +
> +# need a wrapper so the "File size limit exceeded" error can be filtered
> +do_rlimit_copy()
> +{
> + $XFS_IO_PROG -f -c "copy_range -l 32k -s 0 -d 16m $testdir/file" $testdir/copy
> +}
> +
> +ulimit -f $((8 * 1024))
> +ulimit -c 0
> +do_rlimit_copy 2>&1 | grep -o "File size limit exceeded"
> +ulimit -f unlimited
> +
> +echo
> +echo copy across devices
> +$XFS_IO_PROG -f -c "copy_range -l 128k $testdir/file" $SCRATCH_MNT/copy
> +cmp $testdir/file $SCRATCH_MNT/copy
> +echo "md5sums after xdev copy:"
> +md5sum $testdir/file $SCRATCH_MNT/copy | _filter_test_dir | _filter_scratch
Echoing Amir, this should be a separate test because it tests new
functionality.
Really wishing we'd put a WARN_ONCE(1, "Using EXPERIMENTAL
copy_file_range syscall"); in when this got merged. :( :(
--D
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/530.out b/tests/generic/530.out
> new file mode 100644
> index 000000000000..c433fb989637
> --- /dev/null
> +++ b/tests/generic/530.out
> @@ -0,0 +1,62 @@
> +QA output created by 530
> +swap files return ETXTBUSY
> +copy_range: Text file busy
> +copy_range: Text file busy
> +
> +immutable file returns EPERM
> +wrote 65536/65536 bytes at offset 0
> +XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +copy_range: Operation not permitted
> +
> +no write perms on destination returns EACCES
> +copy_range: Permission denied
> +
> +source range overlaps EOF returns EINVAL
> +copy_range: Invalid argument
> +
> +source range beyond EOF returns EINVAL
> +copy_range: Invalid argument
> +
> +source range overlaps destination range in same file returns EINVAL
> +copy_range: Invalid argument
> +
> +destination file O_RDONLY returns EBADF
> +copy_range: Bad file descriptor
> +
> +destination file O_APPEND returns EBADF
> +copy_range: Bad file descriptor
> +
> +source/destination as directory returns EISDIR
> +copy_range: Is a directory
> +copy_range: Is a directory
> +
> +source/destination as blkdev returns EINVAL
> +copy_range: Invalid argument
> +copy_range: Invalid argument
> +
> +source/destination as chardev returns EINVAL
> +copy_range: Invalid argument
> +copy_range: Invalid argument
> +
> +source/destination as FIFO returns EINVAL
> +copy_range: Invalid argument
> +copy_range: Invalid argument
> +
> +length beyond 8EiB wraps around 0 returns EOVERFLOW
> +copy_range: Value too large for defined data type
> +copy_range: Value too large for defined data type
> +
> +source range beyond 8EiB returns EINVAL
> +copy_range: Invalid argument
> +copy_range: Invalid argument
> +
> +destination range beyond 8TiB returns EFBIG
> +copy_range: File too large
> +
> +destination larger than rlimit returns EFBIG
> +File size limit exceeded
> +
> +copy across devices
> +md5sums after xdev copy:
> +81615449a98aaaad8dc179b3bec87f38 TEST_DIR/test-530/file
> +81615449a98aaaad8dc179b3bec87f38 SCRATCH_MNT/copy
> diff --git a/tests/generic/group b/tests/generic/group
> index cc05792ba3b6..5ba1280aa3e6 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -523,3 +523,4 @@
> 517 auto quick dedupe clone
> 518 auto quick clone
> 519 auto quick
> +530 auto quick copy_range
> --
> 2.19.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 3/3] generic: copy_file_range bounds test
2018-12-03 6:42 ` [PATCH 3/3] generic: copy_file_range bounds test Dave Chinner
2018-12-03 7:25 ` Amir Goldstein
2018-12-03 16:58 ` Darrick J. Wong
@ 2019-05-21 5:33 ` Amir Goldstein
2 siblings, 0 replies; 19+ messages in thread
From: Amir Goldstein @ 2019-05-21 5:33 UTC (permalink / raw)
To: Dave Chinner
Cc: fstests, Olga Kornievskaia, Darrick J. Wong, Christoph Hellwig
On Mon, Dec 3, 2018 at 8:43 AM Dave Chinner <david@fromorbit.com> wrote:
>
> From: Dave Chinner <dchinner@redhat.com>
>
> Test that copy_file_range will return the correct errors for various
> error conditions and boundary constraints.
Below are the changes I have made to V2 on this test in accordance to agreed
change of behavior (i.e. short copy up to EOF).
This is a heads up before posting to verify my interpretation is correct.
I still have more testing to do before posting.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> tests/generic/530 | 165 ++++++++++++++++++++++++++++++++++++++++++
> tests/generic/530.out | 62 ++++++++++++++++
> tests/generic/group | 1 +
> 3 files changed, 228 insertions(+)
> create mode 100755 tests/generic/530
> create mode 100644 tests/generic/530.out
>
> diff --git a/tests/generic/530 b/tests/generic/530
> new file mode 100755
> index 000000000000..42243cc70914
> --- /dev/null
> +++ b/tests/generic/530
> @@ -0,0 +1,165 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2018 Red Hat, Inc. All Rights Reserved.
> +#
> +# FS QA Test No. 530
> +#
> +# Exercise copy_file_range() syscall error conditions.
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 7 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -rf $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_os Linux
> +_supported_fs generic
> +
> +rm -f $seqres.full
> +
> +_require_test
> +_require_scratch
> +_require_xfs_io_command "copy_range"
> +_require_user
> +_require_test_swapfile
Testing on scratch dev, as cross-dev test was split to a new test.
> +
> +_scratch_mkfs 2>&1 >> $seqres.full
> +_scratch_mount
> +
> +
> +testdir=$TEST_DIR/test-$seq
> +rm -rf $testdir
> +mkdir $testdir
> +rm -f $seqres.full
> +
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 128k" $testdir/file >> $seqres.full 2>&1
> +chmod 777 $testdir/file
> +
> +echo swap files return ETXTBUSY
> +_format_swapfile $testdir/swapfile 16m
> +swapon $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/file" $testdir/swapfile
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/swapfile" $testdir/copy
> +swapoff $testdir/swapfile
> +
> +# we have to open the file to be immutable rw and hold it open over the
> +# chattr command to set it immutable, otherwise we won't be able to open it for
> +# writing after it's been made immutable. (i.e. would exercise file mode checks,
> +# not immutable inode flag checks).
> +echo
> +echo immutable file returns EPERM
> +$XFS_IO_PROG -f -c "pwrite -S 0x61 0 64k" -c fsync $testdir/immutable | _filter_xfs_io
> +$XFS_IO_PROG -f -c "chattr +i" -c "copy_range -l 32k $testdir/file" $testdir/immutable
> +$XFS_IO_PROG -f -r -c "chattr -i" $testdir/immutable
> +rm -f $testdir/immutable
> +
> +# can't test this as root, because root is allowed to write to files do not
> +# have write permission bits set.
> +echo
> +echo no write perms on destination returns EACCES
This test case was removed, because it does not comply with current behavior
of write(2).
> +chown $qa_user:12345 $testdir/copy
> +su $qa_user -c "$XFS_IO_PROG -f -c 'chmod -r' -c 'copy_range -l 32k $testdir/file' $testdir/copy"
> +rm -f $testdir/copy
> +
> +echo
> +echo source range overlaps EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 112k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond EOF returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 128k -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source range overlaps destination range in same file returns EINVAL
> +$XFS_IO_PROG -f -c "copy_range -s 32k -d 48k -l 32k $testdir/file" $testdir/file
> +
> +echo
> +echo destination file O_RDONLY returns EBADF
> +$XFS_IO_PROG -f -r -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo destination file O_APPEND returns EBADF
> +$XFS_IO_PROG -f -a -c "copy_range -l 32k $testdir/file" $testdir/copy
> +
> +echo
> +echo source/destination as directory returns EISDIR
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir" $testdir/copy
> +
> +echo
> +echo source/destination as blkdev returns EINVAL
> +mknod $testdir/dev1 b 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev1
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev1" $testdir/copy
> +
> +echo
> +echo source/destination as chardev returns EINVAL
> +mknod $testdir/dev2 c 1 3
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/dev2
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/dev2" $testdir/copy
> +
> +echo
> +echo source/destination as FIFO returns EINVAL
> +mkfifo $testdir/fifo
> +$XFS_IO_PROG -c "copy_range -l 32k $testdir/file" $testdir/fifo
> +$XFS_IO_PROG -f -c "copy_range -l 32k $testdir/fifo" $testdir/copy
> +
> +max_off=$((8 * 2**60 - 65536 - 1))
> +min_off=65537
> +
> +echo
> +echo length beyond 8EiB wraps around 0 returns EOVERFLOW
> +$XFS_IO_PROG -f -c "copy_range -l 10e -s $max_off $testdir/file" $testdir/copy
> +$XFS_IO_PROG -f -c "copy_range -l 10e -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo source range beyond 8EiB returns EINVAL
With accordance to implemented behavior and short copy to EOF,
this was changed to:
echo source range beyond 8TiB returns EFBIG
$XFS_IO_PROG -c "copy_range -s $max_off -l $min_off -d 0
$testdir/file" $testdir/copy
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/file
> +$XFS_IO_PROG -c "truncate $((max_off + 65536))" $testdir/copy
These ftruncates would fail with EFBIG anyway...
> +$XFS_IO_PROG -c "copy_range -s $max_off -l $min_off -d 0 $testdir/file" $testdir/copy
> +$XFS_IO_PROG -c "copy_range -s $min_off -l $max_off -d 0 $testdir/file" $testdir/copy
This second test does not fail because of short copy.
> +
> +echo
> +echo destination range beyond 8TiB returns EFBIG
> +$XFS_IO_PROG -c "copy_range -l $min_off -s 0 -d $max_off $testdir/file" $testdir/copy
> +
> +echo
> +echo destination larger than rlimit returns EFBIG
> +rm -f $testdir/copy
> +$XFS_IO_PROG -c "truncate 128k" $testdir/file
> +
> +# need a wrapper so the "File size limit exceeded" error can be filtered
> +do_rlimit_copy()
> +{
> + $XFS_IO_PROG -f -c "copy_range -l 32k -s 0 -d 16m $testdir/file" $testdir/copy
> +}
> +
> +ulimit -f $((8 * 1024))
> +ulimit -c 0
> +do_rlimit_copy 2>&1 | grep -o "File size limit exceeded"
> +ulimit -f unlimited
> +
> +echo
> +echo copy across devices
> +$XFS_IO_PROG -f -c "copy_range -l 128k $testdir/file" $SCRATCH_MNT/copy
> +cmp $testdir/file $SCRATCH_MNT/copy
> +echo "md5sums after xdev copy:"
> +md5sum $testdir/file $SCRATCH_MNT/copy | _filter_test_dir | _filter_scratch
> +
Cross-device copy test was split out to a new test, see WIP:
https://github.com/amir73il/xfstests/commits/copy_file_range
Thanks,
Amir.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/3] fstests: copy_file_range() bounds testing
2018-12-03 6:42 [PATCH 0/3] fstests: copy_file_range() bounds testing Dave Chinner
` (2 preceding siblings ...)
2018-12-03 6:42 ` [PATCH 3/3] generic: copy_file_range bounds test Dave Chinner
@ 2018-12-03 16:41 ` Darrick J. Wong
3 siblings, 0 replies; 19+ messages in thread
From: Darrick J. Wong @ 2018-12-03 16:41 UTC (permalink / raw)
To: Dave Chinner; +Cc: fstests
On Mon, Dec 03, 2018 at 05:42:53PM +1100, Dave Chinner wrote:
> Hi folks,
>
> We suck at bounds testing new system calls. This is a test that
> exercises the expected failure cases for copy_file_range(). It's
> going to fail miserably on existing kernels - I'm about to post a
> series of fixes to linux-fsdevel that make this test pass.
Can you please cc the xfs list on fstests changes that affect xfs? :)
(says a prolific patchbomber :P)
> The test is also dependent on xfs_io changes that I posted a few
> hours ago. The three (not 2!) patches can be found here:
>
> https://marc.info/?l=linux-xfs&m=154378403323889&w=2
> https://marc.info/?l=linux-xfs&m=154378403523890&w=2
> https://marc.info/?l=linux-xfs&m=154378403323888&w=2
> https://marc.info/?l=linux-xfs&m=154379644526132&w=2
>
> Comments welcome.
>
> -Dave
>
> ---
>
> As an aside, one thing that I've discovered in writing this test is
> that certain things we do to test certain file conditions are not
> being tested corectly. e.g. immutable files.
>
> When we set a file as immutable and then go to modify it with xfs_io
> like this:
>
> # xfs_io -c "chattr +i" test_file
> # xfs_io -c "pwrite 0 4k" test_file
>
> We are not actually testing whether the pwrite() syscall detected
> that it can't write to an immutable file. xfs_io actually fails when
> the open(O_RDWR) syscall fails because we can't open an immutable
> file for writing. IOWs, it's not testing pwrite() at all.
>
> Hence tests like generic/159 and generic/160 are not actually
> testing whether cloning/deduping files fails on immutable files.
>
> Instead, what we need to do is open the file O_RDWR, then set the
> file immutable, then perform the modification operation. i.e. this:
>
> # xfs_io -c "chattr +i" -c "pwrite 0 4k" test_file
Ok, I'll post a change to 159/160 to fix that. I don't think btrfs got
this right (inode_permission vs. security_file_permission) either, which
is why the hoisted code and tests (mis)behave the way they do.
> Will exercise the pwrite() syscall hitting an immutable file. A
> similar thing happens with trying to write/modify to read only files
> - the open() call fails, not the call that we want to test. That's
> why I added the "chmod" operation to xfs_io, to allow us to open a
> file for write, then turn it read only while we still have a
> writeable fd open. This then exercises trying to write/modify a
> read-only file.
>
> I'm sure there's lots of tests that have these problems. I don't
> have time to audit them right now, but I'm bringing it up so that
> people are aware of the issue and at least catch problems like this
> in new tests....
--D
^ permalink raw reply [flat|nested] 19+ messages in thread