All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] generic: test fiemap offsets and < 512 byte ranges
@ 2021-04-06 22:47 Boris Burkov
  2021-04-06 22:54 ` [PATCH v2] " Boris Burkov
  0 siblings, 1 reply; 8+ messages in thread
From: Boris Burkov @ 2021-04-06 22:47 UTC (permalink / raw)
  To: linux-btrfs, kernel-team, fstests

btrfs trims fiemap extents to the inputted offset, which leads to
inconsistent results for most inputs, and downright bizarre outputs like
[7..6] when the trimmed extent is at the end of an extent and shorter
than 512 bytes.

This test covers a bunch of cases like that and ensures that file
systems always return the full extent without trimming it.

I also ran it under ext2, ext3, ext4, f2fs, and xfs successfully, but I
suppose it's no guarantee that every file system will store a 4k synced
write in a single extent. For that reason, this might be a bit fragile.

This test is fixed for btrfs by:
btrfs: return whole extents in fiemap
(https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)

Signed-off-by: Boris Burkov <boris@bur.io>
---
 tests/generic/623     | 93 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/623.out |  2 +
 tests/generic/group   |  1 +
 3 files changed, 96 insertions(+)
 create mode 100755 tests/generic/623
 create mode 100644 tests/generic/623.out

diff --git a/tests/generic/623 b/tests/generic/623
new file mode 100755
index 00000000..d399c9f0
--- /dev/null
+++ b/tests/generic/623
@@ -0,0 +1,93 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 YOUR NAME HERE.  All Rights Reserved.
+#
+# FS QA Test 623
+#
+# what am I here for?
+#
+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 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_test
+_require_scratch
+_require_xfs_io_command "fiemap"
+
+rm -f $seqres.full
+
+_do_fiemap() {
+	off=$1
+	len=$2
+	$XFS_IO_PROG -c "fiemap $off $len" $SCRATCH_MNT/foo
+}
+
+_check_fiemap() {
+	off=$1
+	len=$2
+	actual=$(_do_fiemap $off $len | tee -a $seqres.full)
+	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
+}
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+# write a file with one extent
+$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 4K" $SCRATCH_MNT/foo >/dev/null
+
+# since the exact extent location is unpredictable especially when
+# varying file systems, just test that they are all equal, which is
+# what we really expect.
+expected=$(_do_fiemap)
+
+# start to mid-extent
+_check_fiemap 0 2048
+# start to end
+_check_fiemap 0 4096
+# start to past-end
+_check_fiemap 0 4097
+# mid-extent to mid-extent
+_check_fiemap 1024 2048
+# mid-extent to end
+_check_fiemap 2048 4096
+# mid-extent to past-end
+_check_fiemap 2048 4097
+
+# to end; len < 512
+_check_fiemap 4091 5
+# to end; len == 512
+_check_fiemap 3584 512
+# past end; len < 512
+_check_fiemap 4091 500
+# past end; len == 512
+_check_fiemap 4091 512
+
+_scratch_unmount
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/623.out b/tests/generic/623.out
new file mode 100644
index 00000000..6f774f19
--- /dev/null
+++ b/tests/generic/623.out
@@ -0,0 +1,2 @@
+QA output created by 623
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index b10fdea4..39e02383 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -625,3 +625,4 @@
 620 auto mount quick
 621 auto quick encrypt
 622 auto shutdown metadata atime
+623 auto quick
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2] generic: test fiemap offsets and < 512 byte ranges
  2021-04-06 22:47 [PATCH] generic: test fiemap offsets and < 512 byte ranges Boris Burkov
@ 2021-04-06 22:54 ` Boris Burkov
  2021-04-07 16:10   ` Darrick J. Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Boris Burkov @ 2021-04-06 22:54 UTC (permalink / raw)
  To: linux-btrfs, kernel-team, fstests

btrfs trims fiemap extents to the inputted offset, which leads to
inconsistent results for most inputs, and downright bizarre outputs like
[7..6] when the trimmed extent is at the end of an extent and shorter
than 512 bytes.

This test covers a bunch of cases like that and ensures that file
systems always return the full extent without trimming it.

I also ran it under ext2, ext3, ext4, f2fs, and xfs successfully, but I
suppose it's no guarantee that every file system will store a 4k synced
write in a single extent. For that reason, this might be a bit fragile.

This test is fixed for btrfs by:
btrfs: return whole extents in fiemap
(https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)

Signed-off-by: Boris Burkov <boris@bur.io>

--
v2: fill out copyright and test description

---
 tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/623.out |  2 +
 tests/generic/group   |  1 +
 3 files changed, 97 insertions(+)
 create mode 100755 tests/generic/623
 create mode 100644 tests/generic/623.out

diff --git a/tests/generic/623 b/tests/generic/623
new file mode 100755
index 00000000..85ef68f6
--- /dev/null
+++ b/tests/generic/623
@@ -0,0 +1,94 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Facebook.  All Rights Reserved.
+#
+# FS QA Test 623
+#
+# Test fiemaps with offsets into small parts of extents.
+# Expect to get the whole extent, anyway.
+#
+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 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_test
+_require_scratch
+_require_xfs_io_command "fiemap"
+
+rm -f $seqres.full
+
+_do_fiemap() {
+	off=$1
+	len=$2
+	$XFS_IO_PROG -c "fiemap $off $len" $SCRATCH_MNT/foo
+}
+
+_check_fiemap() {
+	off=$1
+	len=$2
+	actual=$(_do_fiemap $off $len | tee -a $seqres.full)
+	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
+}
+
+_scratch_mkfs >>$seqres.full 2>&1
+_scratch_mount
+
+# write a file with one extent
+$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 4K" $SCRATCH_MNT/foo >/dev/null
+
+# since the exact extent location is unpredictable especially when
+# varying file systems, just test that they are all equal, which is
+# what we really expect.
+expected=$(_do_fiemap)
+
+# start to mid-extent
+_check_fiemap 0 2048
+# start to end
+_check_fiemap 0 4096
+# start to past-end
+_check_fiemap 0 4097
+# mid-extent to mid-extent
+_check_fiemap 1024 2048
+# mid-extent to end
+_check_fiemap 2048 4096
+# mid-extent to past-end
+_check_fiemap 2048 4097
+
+# to end; len < 512
+_check_fiemap 4091 5
+# to end; len == 512
+_check_fiemap 3584 512
+# past end; len < 512
+_check_fiemap 4091 500
+# past end; len == 512
+_check_fiemap 4091 512
+
+_scratch_unmount
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/623.out b/tests/generic/623.out
new file mode 100644
index 00000000..6f774f19
--- /dev/null
+++ b/tests/generic/623.out
@@ -0,0 +1,2 @@
+QA output created by 623
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index b10fdea4..39e02383 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -625,3 +625,4 @@
 620 auto mount quick
 621 auto quick encrypt
 622 auto shutdown metadata atime
+623 auto quick
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] generic: test fiemap offsets and < 512 byte ranges
  2021-04-06 22:54 ` [PATCH v2] " Boris Burkov
@ 2021-04-07 16:10   ` Darrick J. Wong
  2021-04-07 20:13     ` [PATCH v3] " Boris Burkov
  2021-04-07 20:27     ` [PATCH v2] " Boris Burkov
  0 siblings, 2 replies; 8+ messages in thread
From: Darrick J. Wong @ 2021-04-07 16:10 UTC (permalink / raw)
  To: Boris Burkov; +Cc: linux-btrfs, kernel-team, fstests

On Tue, Apr 06, 2021 at 03:54:29PM -0700, Boris Burkov wrote:
> btrfs trims fiemap extents to the inputted offset, which leads to
> inconsistent results for most inputs, and downright bizarre outputs like
> [7..6] when the trimmed extent is at the end of an extent and shorter
> than 512 bytes.
> 
> This test covers a bunch of cases like that and ensures that file
> systems always return the full extent without trimming it.
> 
> I also ran it under ext2, ext3, ext4, f2fs, and xfs successfully, but I
> suppose it's no guarantee that every file system will store a 4k synced
> write in a single extent. For that reason, this might be a bit fragile.

Does it work with 64k fs blocks?  Or 512b blocks? :)

Also... is there an xfs_io fix to go with this?

> 
> This test is fixed for btrfs by:
> btrfs: return whole extents in fiemap
> (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> 
> --
> v2: fill out copyright and test description
> 
> ---
>  tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/623.out |  2 +
>  tests/generic/group   |  1 +
>  3 files changed, 97 insertions(+)
>  create mode 100755 tests/generic/623
>  create mode 100644 tests/generic/623.out
> 
> diff --git a/tests/generic/623 b/tests/generic/623
> new file mode 100755
> index 00000000..85ef68f6
> --- /dev/null
> +++ b/tests/generic/623
> @@ -0,0 +1,94 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Facebook.  All Rights Reserved.
> +#
> +# FS QA Test 623
> +#
> +# Test fiemaps with offsets into small parts of extents.
> +# Expect to get the whole extent, anyway.
> +#
> +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 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_test
> +_require_scratch
> +_require_xfs_io_command "fiemap"
> +
> +rm -f $seqres.full
> +
> +_do_fiemap() {
> +	off=$1
> +	len=$2
> +	$XFS_IO_PROG -c "fiemap $off $len" $SCRATCH_MNT/foo
> +}
> +
> +_check_fiemap() {

Only helper functions in common/ need to be prefixed with "_".

> +	off=$1
> +	len=$2
> +	actual=$(_do_fiemap $off $len | tee -a $seqres.full)
> +	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
> +}
> +
> +_scratch_mkfs >>$seqres.full 2>&1
> +_scratch_mount

You could probably accomplish this by creating a file on the test
device, which means this test would run on configurations where there's
no scratch device; and run faster since there's no longer any need for
mkfs.

--D

> +
> +# write a file with one extent
> +$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 4K" $SCRATCH_MNT/foo >/dev/null
> +
> +# since the exact extent location is unpredictable especially when
> +# varying file systems, just test that they are all equal, which is
> +# what we really expect.
> +expected=$(_do_fiemap)
> +
> +# start to mid-extent
> +_check_fiemap 0 2048
> +# start to end
> +_check_fiemap 0 4096
> +# start to past-end
> +_check_fiemap 0 4097
> +# mid-extent to mid-extent
> +_check_fiemap 1024 2048
> +# mid-extent to end
> +_check_fiemap 2048 4096
> +# mid-extent to past-end
> +_check_fiemap 2048 4097
> +
> +# to end; len < 512
> +_check_fiemap 4091 5
> +# to end; len == 512
> +_check_fiemap 3584 512
> +# past end; len < 512
> +_check_fiemap 4091 500
> +# past end; len == 512
> +_check_fiemap 4091 512
> +
> +_scratch_unmount
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/623.out b/tests/generic/623.out
> new file mode 100644
> index 00000000..6f774f19
> --- /dev/null
> +++ b/tests/generic/623.out
> @@ -0,0 +1,2 @@
> +QA output created by 623
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index b10fdea4..39e02383 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -625,3 +625,4 @@
>  620 auto mount quick
>  621 auto quick encrypt
>  622 auto shutdown metadata atime
> +623 auto quick
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v3] generic: test fiemap offsets and < 512 byte ranges
  2021-04-07 16:10   ` Darrick J. Wong
@ 2021-04-07 20:13     ` Boris Burkov
  2021-04-11 14:03       ` Eryu Guan
  2021-04-07 20:27     ` [PATCH v2] " Boris Burkov
  1 sibling, 1 reply; 8+ messages in thread
From: Boris Burkov @ 2021-04-07 20:13 UTC (permalink / raw)
  To: Darrick J. Wong, linux-btrfs, kernel-team, fstests

btrfs trims fiemap extents to the inputted offset, which leads to
inconsistent results for most inputs, and downright bizarre outputs like
[7..6] when the trimmed extent is at the end of an extent and shorter
than 512 bytes.

The test writes out one extent of the file system's block size and tries
fiemaps at various offsets. It expects that all the fiemaps return the
full single extent.

I ran it under the following fs, block size combinations:
ext2: 1024, 2048, 4096
ext3: 1024, 2048, 4096
ext4: 1024, 2048, 4096
xfs: 512, 1024, 2048, 4096
f2fs: 4096
btrfs: 4096

This test is fixed for btrfs by:
btrfs: return whole extents in fiemap
(https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)

Signed-off-by: Boris Burkov <boris@bur.io>
---
v3: make the block size more generic, use test dev instead of scratch,
cleanup style issues.
v2: fill out copyright and test description
---
 tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/623.out |  2 +
 tests/generic/group   |  1 +
 3 files changed, 97 insertions(+)
 create mode 100755 tests/generic/623
 create mode 100644 tests/generic/623.out

diff --git a/tests/generic/623 b/tests/generic/623
new file mode 100755
index 00000000..a5ef369a
--- /dev/null
+++ b/tests/generic/623
@@ -0,0 +1,94 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Facebook.  All Rights Reserved.
+#
+# FS QA Test 623
+#
+# Test fiemaps with offsets into small parts of extents.
+# Expect to get the whole extent, anyway.
+#
+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 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+	rm -f $fiemap_file
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+
+# Modify as appropriate.
+_supported_fs generic
+_require_test
+_require_xfs_io_command "fiemap"
+
+rm -f $seqres.full
+
+fiemap_file=$TEST_DIR/foo.$$
+
+do_fiemap() {
+	off=$1
+	len=$2
+	echo $off $len >> $seqres.full
+	$XFS_IO_PROG -c "fiemap $off $len" $fiemap_file | tee -a $seqres.full
+}
+
+check_fiemap() {
+	off=$1
+	len=$2
+	actual=$(do_fiemap $off $len)
+	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
+}
+
+# write a file with one extent
+block_size=$(_get_block_size $TEST_DIR)
+$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 $block_size" $fiemap_file >/dev/null
+
+# since the exact extent location is unpredictable especially when
+# varying file systems, just test that they are all equal, which is
+# what we really expect.
+expected=$(do_fiemap)
+
+mid=$((block_size / 2))
+almost=$((block_size - 5))
+past=$((block_size + 1))
+
+check_fiemap 0 $mid
+check_fiemap 0 $block_size
+check_fiemap 0 $past
+check_fiemap $mid $almost
+check_fiemap $mid $block_size
+check_fiemap $mid $past
+check_fiemap $almost 5
+check_fiemap $almost 6
+
+# fiemap output explicitly deals in 512 byte increments,
+# so exercise some cases where len is 512.
+# Naturally, some of these can't work if block size is 512.
+one_short=$((block_size - 512))
+check_fiemap 0 512
+check_fiemap $one_short 512
+check_fiemap $almost 512
+
+_test_unmount
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/623.out b/tests/generic/623.out
new file mode 100644
index 00000000..6f774f19
--- /dev/null
+++ b/tests/generic/623.out
@@ -0,0 +1,2 @@
+QA output created by 623
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index b10fdea4..39e02383 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -625,3 +625,4 @@
 620 auto mount quick
 621 auto quick encrypt
 622 auto shutdown metadata atime
+623 auto quick
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] generic: test fiemap offsets and < 512 byte ranges
  2021-04-07 16:10   ` Darrick J. Wong
  2021-04-07 20:13     ` [PATCH v3] " Boris Burkov
@ 2021-04-07 20:27     ` Boris Burkov
  1 sibling, 0 replies; 8+ messages in thread
From: Boris Burkov @ 2021-04-07 20:27 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: linux-btrfs, kernel-team, fstests

On Wed, Apr 07, 2021 at 09:10:46AM -0700, Darrick J. Wong wrote:
> On Tue, Apr 06, 2021 at 03:54:29PM -0700, Boris Burkov wrote:
> > btrfs trims fiemap extents to the inputted offset, which leads to
> > inconsistent results for most inputs, and downright bizarre outputs like
> > [7..6] when the trimmed extent is at the end of an extent and shorter
> > than 512 bytes.
> > 
> > This test covers a bunch of cases like that and ensures that file
> > systems always return the full extent without trimming it.
> > 
> > I also ran it under ext2, ext3, ext4, f2fs, and xfs successfully, but I
> > suppose it's no guarantee that every file system will store a 4k synced
> > write in a single extent. For that reason, this might be a bit fragile.
> 
> Does it work with 64k fs blocks?  Or 512b blocks? :)
> 

I don't have easy access to a means of testing with 64k blocks, but I
did test the more flexible v3 on 512, 1024, 2048, and 4096 byte blocks.

> Also... is there an xfs_io fix to go with this?
> 

Since the xfs_bmap manpage says that it outputs in 512 byte chunks and
the xfs_io manpage says fiemap should output the full extent, I figured
this was a case of garbage-in-garbage-out.

Do you think xfs_io fiemap should handle these <=512 byte cases more
permissively? With xfs and -b size=512, I got:
0: [0..0]: 11..11
which might really be a bug.

> > 
> > This test is fixed for btrfs by:
> > btrfs: return whole extents in fiemap
> > (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > 
> > --
> > v2: fill out copyright and test description
> > 
> > ---
> >  tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/623.out |  2 +
> >  tests/generic/group   |  1 +
> >  3 files changed, 97 insertions(+)
> >  create mode 100755 tests/generic/623
> >  create mode 100644 tests/generic/623.out
> > 
> > diff --git a/tests/generic/623 b/tests/generic/623
> > new file mode 100755
> > index 00000000..85ef68f6
> > --- /dev/null
> > +++ b/tests/generic/623
> > @@ -0,0 +1,94 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021 Facebook.  All Rights Reserved.
> > +#
> > +# FS QA Test 623
> > +#
> > +# Test fiemaps with offsets into small parts of extents.
> > +# Expect to get the whole extent, anyway.
> > +#
> > +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 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_test
> > +_require_scratch
> > +_require_xfs_io_command "fiemap"
> > +
> > +rm -f $seqres.full
> > +
> > +_do_fiemap() {
> > +	off=$1
> > +	len=$2
> > +	$XFS_IO_PROG -c "fiemap $off $len" $SCRATCH_MNT/foo
> > +}
> > +
> > +_check_fiemap() {
> 
> Only helper functions in common/ need to be prefixed with "_".
> 
> > +	off=$1
> > +	len=$2
> > +	actual=$(_do_fiemap $off $len | tee -a $seqres.full)
> > +	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
> > +}
> > +
> > +_scratch_mkfs >>$seqres.full 2>&1
> > +_scratch_mount
> 
> You could probably accomplish this by creating a file on the test
> device, which means this test would run on configurations where there's
> no scratch device; and run faster since there's no longer any need for
> mkfs.
> 
> --D
> 

Thanks for the review,
Boris

> > +
> > +# write a file with one extent
> > +$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 4K" $SCRATCH_MNT/foo >/dev/null
> > +
> > +# since the exact extent location is unpredictable especially when
> > +# varying file systems, just test that they are all equal, which is
> > +# what we really expect.
> > +expected=$(_do_fiemap)
> > +
> > +# start to mid-extent
> > +_check_fiemap 0 2048
> > +# start to end
> > +_check_fiemap 0 4096
> > +# start to past-end
> > +_check_fiemap 0 4097
> > +# mid-extent to mid-extent
> > +_check_fiemap 1024 2048
> > +# mid-extent to end
> > +_check_fiemap 2048 4096
> > +# mid-extent to past-end
> > +_check_fiemap 2048 4097
> > +
> > +# to end; len < 512
> > +_check_fiemap 4091 5
> > +# to end; len == 512
> > +_check_fiemap 3584 512
> > +# past end; len < 512
> > +_check_fiemap 4091 500
> > +# past end; len == 512
> > +_check_fiemap 4091 512
> > +
> > +_scratch_unmount
> > +
> > +echo "Silence is golden"
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/623.out b/tests/generic/623.out
> > new file mode 100644
> > index 00000000..6f774f19
> > --- /dev/null
> > +++ b/tests/generic/623.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 623
> > +Silence is golden
> > diff --git a/tests/generic/group b/tests/generic/group
> > index b10fdea4..39e02383 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -625,3 +625,4 @@
> >  620 auto mount quick
> >  621 auto quick encrypt
> >  622 auto shutdown metadata atime
> > +623 auto quick
> > -- 
> > 2.30.2
> > 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] generic: test fiemap offsets and < 512 byte ranges
  2021-04-07 20:13     ` [PATCH v3] " Boris Burkov
@ 2021-04-11 14:03       ` Eryu Guan
  2021-04-21 19:13         ` Boris Burkov
  0 siblings, 1 reply; 8+ messages in thread
From: Eryu Guan @ 2021-04-11 14:03 UTC (permalink / raw)
  To: Boris Burkov; +Cc: Darrick J. Wong, linux-btrfs, kernel-team, fstests

On Wed, Apr 07, 2021 at 01:13:26PM -0700, Boris Burkov wrote:
> btrfs trims fiemap extents to the inputted offset, which leads to
> inconsistent results for most inputs, and downright bizarre outputs like
> [7..6] when the trimmed extent is at the end of an extent and shorter
> than 512 bytes.
> 
> The test writes out one extent of the file system's block size and tries
> fiemaps at various offsets. It expects that all the fiemaps return the
> full single extent.
> 
> I ran it under the following fs, block size combinations:
> ext2: 1024, 2048, 4096
> ext3: 1024, 2048, 4096
> ext4: 1024, 2048, 4096
> xfs: 512, 1024, 2048, 4096
> f2fs: 4096
> btrfs: 4096
> 
> This test is fixed for btrfs by:
> btrfs: return whole extents in fiemap
> (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)
> 
> Signed-off-by: Boris Burkov <boris@bur.io>

generic/473, which tests fiemap, has been marked as broken, as fiemap
behavior is not consistent across filesystems, and the specific behavior
tested by generic/473 is not defined and filesystems could have
different implementations.

I'm not sure if this test fits into the undefined-behavior fiemap
categary. I think it's fine if it tests a well-defined & consistent
behavior.

> ---
> v3: make the block size more generic, use test dev instead of scratch,
> cleanup style issues.
> v2: fill out copyright and test description
> ---
>  tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/623.out |  2 +
>  tests/generic/group   |  1 +
>  3 files changed, 97 insertions(+)
>  create mode 100755 tests/generic/623
>  create mode 100644 tests/generic/623.out
> 
> diff --git a/tests/generic/623 b/tests/generic/623
> new file mode 100755
> index 00000000..a5ef369a
> --- /dev/null
> +++ b/tests/generic/623
> @@ -0,0 +1,94 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Facebook.  All Rights Reserved.
> +#
> +# FS QA Test 623
> +#
> +# Test fiemaps with offsets into small parts of extents.
> +# Expect to get the whole extent, anyway.
> +#
> +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 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +	rm -f $fiemap_file
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_require_test
> +_require_xfs_io_command "fiemap"
> +
> +rm -f $seqres.full
> +
> +fiemap_file=$TEST_DIR/foo.$$
> +
> +do_fiemap() {
> +	off=$1
> +	len=$2
> +	echo $off $len >> $seqres.full
> +	$XFS_IO_PROG -c "fiemap $off $len" $fiemap_file | tee -a $seqres.full
> +}
> +
> +check_fiemap() {
> +	off=$1
> +	len=$2
> +	actual=$(do_fiemap $off $len)
> +	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
> +}
> +
> +# write a file with one extent
> +block_size=$(_get_block_size $TEST_DIR)
> +$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 $block_size" $fiemap_file >/dev/null
> +
> +# since the exact extent location is unpredictable especially when
> +# varying file systems, just test that they are all equal, which is
> +# what we really expect.
> +expected=$(do_fiemap)
> +
> +mid=$((block_size / 2))
> +almost=$((block_size - 5))
> +past=$((block_size + 1))
> +
> +check_fiemap 0 $mid
> +check_fiemap 0 $block_size
> +check_fiemap 0 $past
> +check_fiemap $mid $almost
> +check_fiemap $mid $block_size
> +check_fiemap $mid $past
> +check_fiemap $almost 5
> +check_fiemap $almost 6
> +
> +# fiemap output explicitly deals in 512 byte increments,
> +# so exercise some cases where len is 512.
> +# Naturally, some of these can't work if block size is 512.
> +one_short=$((block_size - 512))
> +check_fiemap 0 512
> +check_fiemap $one_short 512
> +check_fiemap $almost 512
> +
> +_test_unmount

Any reason to umount TEST_DEV?

Thanks,
Eryu

> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/623.out b/tests/generic/623.out
> new file mode 100644
> index 00000000..6f774f19
> --- /dev/null
> +++ b/tests/generic/623.out
> @@ -0,0 +1,2 @@
> +QA output created by 623
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index b10fdea4..39e02383 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -625,3 +625,4 @@
>  620 auto mount quick
>  621 auto quick encrypt
>  622 auto shutdown metadata atime
> +623 auto quick
> -- 
> 2.30.2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] generic: test fiemap offsets and < 512 byte ranges
  2021-04-11 14:03       ` Eryu Guan
@ 2021-04-21 19:13         ` Boris Burkov
  2021-04-25  5:14           ` Eryu Guan
  0 siblings, 1 reply; 8+ messages in thread
From: Boris Burkov @ 2021-04-21 19:13 UTC (permalink / raw)
  To: Eryu Guan; +Cc: Darrick J. Wong, linux-btrfs, kernel-team, fstests

On Sun, Apr 11, 2021 at 10:03:59PM +0800, Eryu Guan wrote:
> On Wed, Apr 07, 2021 at 01:13:26PM -0700, Boris Burkov wrote:
> > btrfs trims fiemap extents to the inputted offset, which leads to
> > inconsistent results for most inputs, and downright bizarre outputs like
> > [7..6] when the trimmed extent is at the end of an extent and shorter
> > than 512 bytes.
> > 
> > The test writes out one extent of the file system's block size and tries
> > fiemaps at various offsets. It expects that all the fiemaps return the
> > full single extent.
> > 
> > I ran it under the following fs, block size combinations:
> > ext2: 1024, 2048, 4096
> > ext3: 1024, 2048, 4096
> > ext4: 1024, 2048, 4096
> > xfs: 512, 1024, 2048, 4096
> > f2fs: 4096
> > btrfs: 4096
> > 
> > This test is fixed for btrfs by:
> > btrfs: return whole extents in fiemap
> > (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> 
> generic/473, which tests fiemap, has been marked as broken, as fiemap
> behavior is not consistent across filesystems, and the specific behavior
> tested by generic/473 is not defined and filesystems could have
> different implementations.
> 
> I'm not sure if this test fits into the undefined-behavior fiemap
> categary. I think it's fine if it tests a well-defined & consistent
> behavior.
> 

Interesting, I didn't know about that test being marked as broken.

I was worried about this problem to some extent and attempted to
mitigate it by only requiring that all the output be the same, rather
than matching some specific standard.

Thinking about it further, I think this test is portable only so long as
the step where it writes a file with one extent is portable.

If "pwrite 0 block-size" ends up as a file with multiple extents, then
it is possible one of the partial fiemaps will only intersect with a
subset of the extents and rightly return those. In fact, that was broken
in the original version of the test which explicitly used 4096 instead of
being detecting the block size.

I do think it is nice to have this as a regression test for btrfs, since
we have pretty complicated logic for fiemap and it was so broken in this
case. If you prefer, I can make this a btrfs specific test.

Thanks for the review,
Boris

> > ---
> > v3: make the block size more generic, use test dev instead of scratch,
> > cleanup style issues.
> > v2: fill out copyright and test description
> > ---
> >  tests/generic/623     | 94 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/623.out |  2 +
> >  tests/generic/group   |  1 +
> >  3 files changed, 97 insertions(+)
> >  create mode 100755 tests/generic/623
> >  create mode 100644 tests/generic/623.out
> > 
> > diff --git a/tests/generic/623 b/tests/generic/623
> > new file mode 100755
> > index 00000000..a5ef369a
> > --- /dev/null
> > +++ b/tests/generic/623
> > @@ -0,0 +1,94 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021 Facebook.  All Rights Reserved.
> > +#
> > +# FS QA Test 623
> > +#
> > +# Test fiemaps with offsets into small parts of extents.
> > +# Expect to get the whole extent, anyway.
> > +#
> > +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 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +	rm -f $fiemap_file
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +
> > +# Modify as appropriate.
> > +_supported_fs generic
> > +_require_test
> > +_require_xfs_io_command "fiemap"
> > +
> > +rm -f $seqres.full
> > +
> > +fiemap_file=$TEST_DIR/foo.$$
> > +
> > +do_fiemap() {
> > +	off=$1
> > +	len=$2
> > +	echo $off $len >> $seqres.full
> > +	$XFS_IO_PROG -c "fiemap $off $len" $fiemap_file | tee -a $seqres.full
> > +}
> > +
> > +check_fiemap() {
> > +	off=$1
> > +	len=$2
> > +	actual=$(do_fiemap $off $len)
> > +	[ "$actual" == "$expected" ] || _fail "unexpected fiemap on $off $len"
> > +}
> > +
> > +# write a file with one extent
> > +block_size=$(_get_block_size $TEST_DIR)
> > +$XFS_IO_PROG -f -s -c "pwrite -S 0xcf 0 $block_size" $fiemap_file >/dev/null
> > +
> > +# since the exact extent location is unpredictable especially when
> > +# varying file systems, just test that they are all equal, which is
> > +# what we really expect.
> > +expected=$(do_fiemap)
> > +
> > +mid=$((block_size / 2))
> > +almost=$((block_size - 5))
> > +past=$((block_size + 1))
> > +
> > +check_fiemap 0 $mid
> > +check_fiemap 0 $block_size
> > +check_fiemap 0 $past
> > +check_fiemap $mid $almost
> > +check_fiemap $mid $block_size
> > +check_fiemap $mid $past
> > +check_fiemap $almost 5
> > +check_fiemap $almost 6
> > +
> > +# fiemap output explicitly deals in 512 byte increments,
> > +# so exercise some cases where len is 512.
> > +# Naturally, some of these can't work if block size is 512.
> > +one_short=$((block_size - 512))
> > +check_fiemap 0 512
> > +check_fiemap $one_short 512
> > +check_fiemap $almost 512
> > +
> > +_test_unmount
> 
> Any reason to umount TEST_DEV?
> 
> Thanks,
> Eryu
> 
> > +
> > +echo "Silence is golden"
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/623.out b/tests/generic/623.out
> > new file mode 100644
> > index 00000000..6f774f19
> > --- /dev/null
> > +++ b/tests/generic/623.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 623
> > +Silence is golden
> > diff --git a/tests/generic/group b/tests/generic/group
> > index b10fdea4..39e02383 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -625,3 +625,4 @@
> >  620 auto mount quick
> >  621 auto quick encrypt
> >  622 auto shutdown metadata atime
> > +623 auto quick
> > -- 
> > 2.30.2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3] generic: test fiemap offsets and < 512 byte ranges
  2021-04-21 19:13         ` Boris Burkov
@ 2021-04-25  5:14           ` Eryu Guan
  0 siblings, 0 replies; 8+ messages in thread
From: Eryu Guan @ 2021-04-25  5:14 UTC (permalink / raw)
  To: Boris Burkov; +Cc: Darrick J. Wong, linux-btrfs, kernel-team, fstests

On Wed, Apr 21, 2021 at 12:13:33PM -0700, Boris Burkov wrote:
> On Sun, Apr 11, 2021 at 10:03:59PM +0800, Eryu Guan wrote:
> > On Wed, Apr 07, 2021 at 01:13:26PM -0700, Boris Burkov wrote:
> > > btrfs trims fiemap extents to the inputted offset, which leads to
> > > inconsistent results for most inputs, and downright bizarre outputs like
> > > [7..6] when the trimmed extent is at the end of an extent and shorter
> > > than 512 bytes.
> > > 
> > > The test writes out one extent of the file system's block size and tries
> > > fiemaps at various offsets. It expects that all the fiemaps return the
> > > full single extent.
> > > 
> > > I ran it under the following fs, block size combinations:
> > > ext2: 1024, 2048, 4096
> > > ext3: 1024, 2048, 4096
> > > ext4: 1024, 2048, 4096
> > > xfs: 512, 1024, 2048, 4096
> > > f2fs: 4096
> > > btrfs: 4096
> > > 
> > > This test is fixed for btrfs by:
> > > btrfs: return whole extents in fiemap
> > > (https://lore.kernel.org/linux-btrfs/274e5bcebdb05a8969fc300b4802f33da2fbf218.1617746680.git.boris@bur.io/)
> > > 
> > > Signed-off-by: Boris Burkov <boris@bur.io>
> > 
> > generic/473, which tests fiemap, has been marked as broken, as fiemap
> > behavior is not consistent across filesystems, and the specific behavior
> > tested by generic/473 is not defined and filesystems could have
> > different implementations.
> > 
> > I'm not sure if this test fits into the undefined-behavior fiemap
> > categary. I think it's fine if it tests a well-defined & consistent
> > behavior.
> > 
> 
> Interesting, I didn't know about that test being marked as broken.
> 
> I was worried about this problem to some extent and attempted to
> mitigate it by only requiring that all the output be the same, rather
> than matching some specific standard.
> 
> Thinking about it further, I think this test is portable only so long as
> the step where it writes a file with one extent is portable.
> 
> If "pwrite 0 block-size" ends up as a file with multiple extents, then
> it is possible one of the partial fiemaps will only intersect with a
> subset of the extents and rightly return those. In fact, that was broken
> in the original version of the test which explicitly used 4096 instead of
> being detecting the block size.
> 
> I do think it is nice to have this as a regression test for btrfs, since
> we have pretty complicated logic for fiemap and it was so broken in this
> case. If you prefer, I can make this a btrfs specific test.

Yeah, a btrfs specific test seems safer, and we could move it to generic
later if the behavior is well defined.

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-25  5:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 22:47 [PATCH] generic: test fiemap offsets and < 512 byte ranges Boris Burkov
2021-04-06 22:54 ` [PATCH v2] " Boris Burkov
2021-04-07 16:10   ` Darrick J. Wong
2021-04-07 20:13     ` [PATCH v3] " Boris Burkov
2021-04-11 14:03       ` Eryu Guan
2021-04-21 19:13         ` Boris Burkov
2021-04-25  5:14           ` Eryu Guan
2021-04-07 20:27     ` [PATCH v2] " Boris Burkov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.