diff mbox

generic: test Btrfs delalloc accounting overflow

Message ID 49a0c7551c337566be29b2c073ba0be57779d321.1496391726.git.osandov@fb.com (mailing list archive)
State New, archived
Headers show

Commit Message

Omar Sandoval June 2, 2017, 8:23 a.m. UTC 
From: Omar Sandoval <osandov@fb.com>

This is a regression test for "[PATCH] Btrfs: fix delalloc accounting
leak caused by u32 overflow". It creates a bunch of delalloc extents and
merges them together to make sure the accounting is done right.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 tests/generic/438     | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 tests/generic/438.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 80 insertions(+)
 create mode 100755 tests/generic/438
 create mode 100644 tests/generic/438.out

Comments

Nikolay Borisov June 2, 2017, 9:07 a.m. UTC | #1 
On  2.06.2017 11:23, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This is a regression test for "[PATCH] Btrfs: fix delalloc accounting
> leak caused by u32 overflow". It creates a bunch of delalloc extents and
> merges them together to make sure the accounting is done right.
> 
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
>  tests/generic/438     | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/438.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 80 insertions(+)
>  create mode 100755 tests/generic/438
>  create mode 100644 tests/generic/438.out
> 
> diff --git a/tests/generic/438 b/tests/generic/438
> new file mode 100755
> index 00000000..8edac03d
> --- /dev/null
> +++ b/tests/generic/438
> @@ -0,0 +1,77 @@
> +#! /bin/bash
> +# FS QA Test 438
> +#
> +# Test delayed allocation with a large number of extents that are merged.
> +# Regression test for patch "Btrfs: fix delalloc accounting leak caused by u32
> +# overflow".
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Facebook.  All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> +#-----------------------------------------------------------------------
> +#
> +
> +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
> +
> +test_file="$TEST_DIR/$seq"
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.* "$test_file"
> +}
> +
> +. ./common/rc
> +. ./common/filter
> +
> +rm -f $seqres.full
> +
> +_supported_fs generic
> +_supported_os Linux
> +_require_test
> +
> +# Create 32k extents. All of these extents will be accounted as outstanding and
> +# reserved.
> +for ((i = 0; i < 32 * 1024; i++)); do
> +	xfs_io -f -c "pwrite $((2 * 4096 * i)) 4096" "$test_file" >>"$seqres.full"
> +done
> +
> +# Fill in the gaps between the created extents. The outstanding extents will
> +# all be merged into 1, but there will still be 32k reserved.
> +for ((i = 0; i < 32 * 1024; i++)); do
> +	xfs_io -f -c "pwrite $((2 * 4096 * i + 1)) 4096" "$test_file" >>"$seqres.full"
> +done
> +
> +# Flush the delayed allocations.
> +sync
> +
> +# Make sure that we didn't leak any metadata space.
> +if [[ $FSTYP = btrfs ]]; then
> +	uuid="$(findmnt -n -o UUID "$TEST_DIR")"

if we are on btrfs and we don't have findmnt this test will likely fail.
Perhaps include a _require_command findmnt

> +	cd "/sys/fs/btrfs/$uuid/allocation"
> +	echo "$(($(cat metadata/bytes_may_use) - $(cat global_rsv_reserved))) bytes leaked" | grep -v '^0 '
> +fi
> +
> +echo "Silence is golden"
> +
> +status=0
> +exit
> diff --git a/tests/generic/438.out b/tests/generic/438.out
> new file mode 100644
> index 00000000..4968f4d7
> --- /dev/null
> +++ b/tests/generic/438.out
> @@ -0,0 +1,2 @@
> +QA output created by 438
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index 438c2990..c804b05d 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -440,3 +440,4 @@
>  435 auto encrypt
>  436 auto quick rw
>  437 auto quick
> +438 auto
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba June 2, 2017, 12:46 p.m. UTC | #2 
On Fri, Jun 02, 2017 at 12:07:37PM +0300, Nikolay Borisov wrote:
> > +# Make sure that we didn't leak any metadata space.
> > +if [[ $FSTYP = btrfs ]]; then
> > +	uuid="$(findmnt -n -o UUID "$TEST_DIR")"
> 
> if we are on btrfs and we don't have findmnt this test will likely fail.
> Perhaps include a _require_command findmnt

I think utilities like findmnt should be checked at the beginning of the
whole testuiste, not in each test that uses them. As findmnt is part of
util-linux, missing it would also mean that eg 'mount' is missing.
Highly unlikely.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan June 3, 2017, 7:01 a.m. UTC | #3 
On Fri, Jun 02, 2017 at 02:46:52PM +0200, David Sterba wrote:
> On Fri, Jun 02, 2017 at 12:07:37PM +0300, Nikolay Borisov wrote:
> > > +# Make sure that we didn't leak any metadata space.
> > > +if [[ $FSTYP = btrfs ]]; then
> > > +	uuid="$(findmnt -n -o UUID "$TEST_DIR")"
> > 
> > if we are on btrfs and we don't have findmnt this test will likely fail.
> > Perhaps include a _require_command findmnt
> 
> I think utilities like findmnt should be checked at the beginning of the
> whole testuiste, not in each test that uses them. As findmnt is part of

Agreed. I think we can define a FINDMNT_PROG in common/config and refuse
to run any test if it's mising, as what we did to $MOUNT_PROG and other
must-have commands. (There's already a bare call to findmnt in
common/rc, change it to call $FINDMNT_PROG too).

We can do this in a separate patch.

Thanks,
Eryu

> util-linux, missing it would also mean that eg 'mount' is missing.
> Highly unlikely.
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Christoph Hellwig June 3, 2017, 7:37 a.m. UTC | #4 
This looks like a btrfs-specific test, and not like a generic one
to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval June 7, 2017, 12:03 a.m. UTC | #5 
On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> This looks like a btrfs-specific test, and not like a generic one
> to me.

Nothing about the workload itself is btrfs-specific, we just have the
extra check at the end. But I don't really care, I can make it a btrfs
test unless Eryu has already applied it.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eryu Guan June 7, 2017, 3:08 a.m. UTC | #6 
On Tue, Jun 06, 2017 at 05:03:05PM -0700, Omar Sandoval wrote:
> On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> > This looks like a btrfs-specific test, and not like a generic one
> > to me.
> 
> Nothing about the workload itself is btrfs-specific, we just have the
> extra check at the end. But I don't really care, I can make it a btrfs
> test unless Eryu has already applied it.

What's not very clear to me is that how do we check the accountings for
non-btrfs filesystems, i.e. what would cause a test failure on non-btrfs
filesystems besides an xfs_io write failure? I'd prefer a btrfs-specific
test if there's no good way to do the check. Or if we want to keep it a
generic test, some comments on the non-btrfs case would be good. I'm
fine with either way.

Thanks,
Eryu
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval June 7, 2017, 3:12 a.m. UTC | #7 
On Wed, Jun 07, 2017 at 11:08:01AM +0800, Eryu Guan wrote:
> On Tue, Jun 06, 2017 at 05:03:05PM -0700, Omar Sandoval wrote:
> > On Sat, Jun 03, 2017 at 12:37:00AM -0700, Christoph Hellwig wrote:
> > > This looks like a btrfs-specific test, and not like a generic one
> > > to me.
> > 
> > Nothing about the workload itself is btrfs-specific, we just have the
> > extra check at the end. But I don't really care, I can make it a btrfs
> > test unless Eryu has already applied it.
> 
> What's not very clear to me is that how do we check the accountings for
> non-btrfs filesystems, i.e. what would cause a test failure on non-btrfs
> filesystems besides an xfs_io write failure? I'd prefer a btrfs-specific
> test if there's no good way to do the check. Or if we want to keep it a
> generic test, some comments on the non-btrfs case would be good. I'm
> fine with either way.
> 
> Thanks,
> Eryu

The only thing I can imagine other filesystems hitting would be an early
enospc or some sort of kernel warning, so I'll resend it as a btrfs test.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tests/generic/438 b/tests/generic/438
new file mode 100755
index 00000000..8edac03d
--- /dev/null
+++ b/tests/generic/438
@@ -0,0 +1,77 @@ 
+#! /bin/bash
+# FS QA Test 438
+#
+# Test delayed allocation with a large number of extents that are merged.
+# Regression test for patch "Btrfs: fix delalloc accounting leak caused by u32
+# overflow".
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2017 Facebook.  All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#-----------------------------------------------------------------------
+#
+
+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
+
+test_file="$TEST_DIR/$seq"
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.* "$test_file"
+}
+
+. ./common/rc
+. ./common/filter
+
+rm -f $seqres.full
+
+_supported_fs generic
+_supported_os Linux
+_require_test
+
+# Create 32k extents. All of these extents will be accounted as outstanding and
+# reserved.
+for ((i = 0; i < 32 * 1024; i++)); do
+	xfs_io -f -c "pwrite $((2 * 4096 * i)) 4096" "$test_file" >>"$seqres.full"
+done
+
+# Fill in the gaps between the created extents. The outstanding extents will
+# all be merged into 1, but there will still be 32k reserved.
+for ((i = 0; i < 32 * 1024; i++)); do
+	xfs_io -f -c "pwrite $((2 * 4096 * i + 1)) 4096" "$test_file" >>"$seqres.full"
+done
+
+# Flush the delayed allocations.
+sync
+
+# Make sure that we didn't leak any metadata space.
+if [[ $FSTYP = btrfs ]]; then
+	uuid="$(findmnt -n -o UUID "$TEST_DIR")"
+	cd "/sys/fs/btrfs/$uuid/allocation"
+	echo "$(($(cat metadata/bytes_may_use) - $(cat global_rsv_reserved))) bytes leaked" | grep -v '^0 '
+fi
+
+echo "Silence is golden"
+
+status=0
+exit
diff --git a/tests/generic/438.out b/tests/generic/438.out
new file mode 100644
index 00000000..4968f4d7
--- /dev/null
+++ b/tests/generic/438.out
@@ -0,0 +1,2 @@ 
+QA output created by 438
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index 438c2990..c804b05d 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -440,3 +440,4 @@ 
 435 auto encrypt
 436 auto quick rw
 437 auto quick
+438 auto