xfs: limit xfs_growfs size if test with --large-fs

PATCH In review Zorro Lang 4 participants 9 messages Apr 27, 2018 – May 12, 2018

Share & tools

Zorro Lang

author

2018-04-27 08:22 UTC #0 lore raw

PATCH 1/1 xfs: limit xfs_growfs size if test with --large-fs

When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:

1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.

Signed-off-by Zorro Lang <[email protected]>

---
 common/xfs    | 34 ++++++++++++++++++++++++++++++++++
 tests/xfs/002 |  2 +-
 tests/xfs/127 |  2 +-
 tests/xfs/233 |  2 +-
 4 files changed, 37 insertions(+), 3 deletions(-)

common/xfs +34 -0

index e0bc3f43..6200297c 100644
@@ -721,3 +721,37 @@ _require_xfs_db_write_array()
 	rm -f $TEST_DIR/$seq.img
 	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
 }
+
+# If test on large device or a limit size is specified, this helper make sure
+# xfs_growfs won't beyond this limit (try to grow 10 times current fs size by
+# default).
+#   usage: _scratch_xfs_growfs_limited [size_by_byte]
+_scratch_xfs_growfs_limited()
+{
+
+	local limit_size="$1"
+	local option=""
+
+	if [ "$LARGE_SCRATCH_DEV" = "yes" -o -n "$limit_size" ]; then
+		local tmp=`mktemp -u`
+		xfs_info $SCRATCH_MNT | _filter_mkfs > /dev/null 2>$tmp.info
+		. $tmp.info
+		rm -f $tmp.info
+
+		local fs_size=$((dbsize * dblocks))
+		local dev_size_kb=`_get_device_size $SCRATCH_DEV`
+
+		# default limit_size is 10 times current fs size.
+		if [ -z "$limit_size" ]; then
+			limit_size=$((fs_size * 10))
+		fi
+		# don't limit growfs size if device size is smaller
+		if [ $((dev_size_kb * 1024)) -gt $limit_size ]; then
+			option="-D $((limit_size / dbsize))"
+		else
+			option=""
+		fi
+	fi
+
+	$XFS_GROWFS_PROG $option $SCRATCH_MNT
+}

tests/xfs/002 +1 -1

index 741117be..42d2e2d7 100755
@@ -68,7 +68,7 @@ _scratch_xfs_db -x -c "sb 2" -c "type data" -c "write fill 0xff 224 4"
 _scratch_mount
 
 # This should pass
-$XFS_GROWFS_PROG $SCRATCH_MNT >> $seqres.full 2>&1 || _fail "growfs failed"
+_scratch_xfs_growfs_limited >> $seqres.full 2>&1 || _fail "growfs failed"
 
 # success, all done
 status=0

tests/xfs/127 +1 -1

index 9df99904..c05fcee9 100755
@@ -61,7 +61,7 @@ _cp_reflink $testdir/original $testdir/copy1
 _cp_reflink $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 | _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more reflink copies"

tests/xfs/233 +1 -1

index e61c444d..a0dd42e7 100755
@@ -59,7 +59,7 @@ cp -p $testdir/original $testdir/copy1
 cp -p $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 |  _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more copies"

Signature · 1 line

2.14.3

Eryu Guan

2018-05-09 16:02 UTC #1 lore raw

[cc linux-xfs list for xfs test]

Quoted 9 lines · Zorro Lang

When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>

Sorry for the late review..

This looks fine to me, but I'd like to let XFS developers to take a look too, I'm not sure if "10 times larger" is a sane default value.

BTW, the subject doesn't quite describe what the patch does:

"xfs: limit xfs_growfs size if test with --large-fs"

The patch also limits the growfs size based on the user-specified fs size.

Quoted 12 lines

---
 common/xfs    | 34 ++++++++++++++++++++++++++++++++++
 tests/xfs/002 |  2 +-
 tests/xfs/127 |  2 +-
 tests/xfs/233 |  2 +-
 4 files changed, 37 insertions(+), 3 deletions(-)

common/xfs +0 -0

index e0bc3f43..6200297c 100644

common/xfs _require_xfs_db_write_array() · line 729

Quoted 9 lines

common/xfs +6 -0

@@ -721,3 +721,37 @@ _require_xfs_db_write_array()
 	rm -f $TEST_DIR/$seq.img
 	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
 }
+
+# If test on large device or a limit size is specified, this helper make sure
+# xfs_growfs won't beyond this limit (try to grow 10 times current fs size by
+# default).
+#   usage: _scratch_xfs_growfs_limited [size_by_byte]
+_scratch_xfs_growfs_limited()

I think we could just name it as _scratch_xfs_growfs().

Thanks,
Eryu

Quoted 74 lines

common/xfs +28 -0

+{
+
+	local limit_size="$1"
+	local option=""
+
+	if [ "$LARGE_SCRATCH_DEV" = "yes" -o -n "$limit_size" ]; then
+		local tmp=`mktemp -u`
+		xfs_info $SCRATCH_MNT | _filter_mkfs > /dev/null 2>$tmp.info
+		. $tmp.info
+		rm -f $tmp.info
+
+		local fs_size=$((dbsize * dblocks))
+		local dev_size_kb=`_get_device_size $SCRATCH_DEV`
+
+		# default limit_size is 10 times current fs size.
+		if [ -z "$limit_size" ]; then
+			limit_size=$((fs_size * 10))
+		fi
+		# don't limit growfs size if device size is smaller
+		if [ $((dev_size_kb * 1024)) -gt $limit_size ]; then
+			option="-D $((limit_size / dbsize))"
+		else
+			option=""
+		fi
+	fi
+
+	$XFS_GROWFS_PROG $option $SCRATCH_MNT
+}

tests/xfs/002 +1 -1

index 741117be..42d2e2d7 100755
@@ -68,7 +68,7 @@ _scratch_xfs_db -x -c "sb 2" -c "type data" -c "write fill 0xff 224 4"
 _scratch_mount
 
 # This should pass
-$XFS_GROWFS_PROG $SCRATCH_MNT >> $seqres.full 2>&1 || _fail "growfs failed"
+_scratch_xfs_growfs_limited >> $seqres.full 2>&1 || _fail "growfs failed"
 
 # success, all done
 status=0

tests/xfs/127 +1 -1

index 9df99904..c05fcee9 100755
@@ -61,7 +61,7 @@ _cp_reflink $testdir/original $testdir/copy1
 _cp_reflink $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 | _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more reflink copies"

tests/xfs/233 +1 -1

index e61c444d..a0dd42e7 100755
@@ -59,7 +59,7 @@ cp -p $testdir/original $testdir/copy1
 cp -p $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 |  _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more copies"

Signature · 6 lines

2.14.3

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

2018-05-09 16:23 UTC #2 lore raw

Quoted 18 lines · Eryu Guan

[cc linux-xfs list for xfs test]
Quoted 9 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>
Sorry for the late review..
This looks fine to me, but I'd like to let XFS developers to take a look too, I'm not sure if "10 times larger" is a sane default value.

Quoted 6 lines

BTW, the subject doesn't quite describe what the patch does:
"xfs: limit xfs_growfs size if test with --large-fs"
The patch also limits the growfs size based on the user-specified fs size.

Agreed.

Quoted 7 lines

---
 common/xfs    | 34 ++++++++++++++++++++++++++++++++++
 tests/xfs/002 |  2 +-
 tests/xfs/127 |  2 +-
 tests/xfs/233 |  2 +-
 4 files changed, 37 insertions(+), 3 deletions(-)

Quoted 16 lines

Quoted 5 lines

common/xfs +0 -0

index e0bc3f43..6200297c 100644

common/xfs _require_xfs_db_write_array() · line 729

Quoted 9 lines

common/xfs +6 -0

@@ -721,3 +721,37 @@ _require_xfs_db_write_array()
 	rm -f $TEST_DIR/$seq.img
 	[ $supported -eq 0 ] && _notrun "xfs_db write can't support array"
 }
+
+# If test on large device or a limit size is specified, this helper make sure
+# xfs_growfs won't beyond this limit (try to grow 10 times current fs size by
+# default).
+#   usage: _scratch_xfs_growfs_limited [size_by_byte]
+_scratch_xfs_growfs_limited()

I think we could just name it as _scratch_xfs_growfs().

And tweak the documentation to make its behavior clearer:

"Grows the mounted scratch filesystem.

"If max_size_in_bytes is specified, the filesystem will not be grown larger than that size. Otherwise, if LARGE_SCRATCH_DEV == yes, the filesystem will not be grown larger than 10x the current size. If max_size_in_bytes is not specified and LARGE_SCRATCH_DEV != yes, the scratch filesystem will be expanded to fit the scratch device."

Assuming that's the intended behavior.

--D

Quoted 82 lines

Thanks,
Eryu

Quoted 74 lines

common/xfs +28 -0

+{
+
+	local limit_size="$1"
+	local option=""
+
+	if [ "$LARGE_SCRATCH_DEV" = "yes" -o -n "$limit_size" ]; then
+		local tmp=`mktemp -u`
+		xfs_info $SCRATCH_MNT | _filter_mkfs > /dev/null 2>$tmp.info
+		. $tmp.info
+		rm -f $tmp.info
+
+		local fs_size=$((dbsize * dblocks))
+		local dev_size_kb=`_get_device_size $SCRATCH_DEV`
+
+		# default limit_size is 10 times current fs size.
+		if [ -z "$limit_size" ]; then
+			limit_size=$((fs_size * 10))
+		fi
+		# don't limit growfs size if device size is smaller
+		if [ $((dev_size_kb * 1024)) -gt $limit_size ]; then
+			option="-D $((limit_size / dbsize))"
+		else
+			option=""
+		fi
+	fi
+
+	$XFS_GROWFS_PROG $option $SCRATCH_MNT
+}

tests/xfs/002 +1 -1

index 741117be..42d2e2d7 100755
@@ -68,7 +68,7 @@ _scratch_xfs_db -x -c "sb 2" -c "type data" -c "write fill 0xff 224 4"
 _scratch_mount
 
 # This should pass
-$XFS_GROWFS_PROG $SCRATCH_MNT >> $seqres.full 2>&1 || _fail "growfs failed"
+_scratch_xfs_growfs_limited >> $seqres.full 2>&1 || _fail "growfs failed"
 
 # success, all done
 status=0

tests/xfs/127 +1 -1

index 9df99904..c05fcee9 100755
@@ -61,7 +61,7 @@ _cp_reflink $testdir/original $testdir/copy1
 _cp_reflink $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 | _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more reflink copies"

tests/xfs/233 +1 -1

index e61c444d..a0dd42e7 100755
@@ -59,7 +59,7 @@ cp -p $testdir/original $testdir/copy1
 cp -p $testdir/copy1 $testdir/copy2
 
 echo "Grow fs"
-$XFS_GROWFS_PROG $SCRATCH_MNT 2>&1 |  _filter_growfs >> $seqres.full
+_scratch_xfs_growfs_limited 2>&1 |  _filter_growfs >> $seqres.full
 _scratch_cycle_mount
 
 echo "Create more copies"

Signature · 6 lines

2.14.3

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dave Chinner

2018-05-10 22:19 UTC #3 lore raw

In reply to Zorro Lang

Quoted 9 lines · Zorro Lang

When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>

I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Zorro Lang

author

2018-05-11 03:41 UTC #4 lore raw

Quoted 5 lines · Dave Chinner

Quoted 4 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:

Quoted 8 lines

Quoted 5 lines
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>
I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....

Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ...

Dave: require_no_largefs is better.
Darrick: nearly ack this patch.
Eric:
2018-04-27 04:03 < sandeen> [15:01] <zoro> [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' 2018-04-27 04:03 < sandeen> just limit growfs to something smaller.

What should I do next?

Thanks,
Zorro

Quoted 8 lines

Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]

Dave Chinner

2018-05-11 23:29 UTC #5 lore raw

Quoted 10 lines · Zorro Lang

Quoted 9 lines · Dave Chinner
Quoted 8 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Zorro Lang <[email protected]>
I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....
Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ...

I'm not a maintainer, I'm just the guy who added this functionality to xfstests originally. Deciding what is to be done needs to start from an understanding of the criteria I used for skipping tests on large devices. In this case, I never intended to have multiple order magnitude growfs tests run on large scratch devices.

When I added large device support, I tried to avoid tests that we already had substantial coverage for. i.e. if inreasing the space used by the test doesn't increase test coverage but only increased test runtime, then I skipped it. In this case, we already test small to large size growfs via loopback devices on small scratch devices (e.g. xfs/078), so doing it on extremely large scratch devices doesn't reallycover any new code or error conditions.

Hence, based on my original criteria for deciding what tests to run on large filesystems, I would have skipped this test if it caused excessive runtime. I was testing on sparse devices on SSDs, so seek times for growfs did not impact performance, hence I probably didn't skip it...

Quoted 7 lines

Dave: require_no_largefs is better.
Darrick: nearly ack this patch.
Eric:
2018-04-27 04:03 < sandeen> [15:01] <zoro> [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' 2018-04-27 04:03 < sandeen> just limit growfs to something smaller.
What should I do next?

Make your own decision about how best to proceed based on the feedback you've received. Or ask the fstests maintainer to decide what is best.... :P

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Darrick J. Wong

2018-05-12 00:17 UTC #6 lore raw

Quoted 44 lines · Dave Chinner

Quoted 10 lines · Zorro Lang
Quoted 9 lines · Dave Chinner
Quoted 8 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Signed-off-by Zorro Lang <[email protected]>
I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....
Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ...
I'm not a maintainer, I'm just the guy who added this functionality to xfstests originally. Deciding what is to be done needs to start from an understanding of the criteria I used for skipping tests on large devices. In this case, I never intended to have multiple order magnitude growfs tests run on large scratch devices.
When I added large device support, I tried to avoid tests that we already had substantial coverage for. i.e. if inreasing the space used by the test doesn't increase test coverage but only increased test runtime, then I skipped it. In this case, we already test small to large size growfs via loopback devices on small scratch devices (e.g. xfs/078), so doing it on extremely large scratch devices doesn't reallycover any new code or error conditions.
Hence, based on my original criteria for deciding what tests to run on large filesystems, I would have skipped this test if it caused excessive runtime. I was testing on sparse devices on SSDs, so seek times for growfs did not impact performance, hence I probably didn't skip it...
Quoted 6 lines
Dave: require_no_largefs is better.
Darrick: nearly ack this patch.
Eric:
2018-04-27 04:03 < sandeen> [15:01] <zoro> [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' 2018-04-27 04:03 < sandeen> just limit growfs to something smaller.

Quoted 5 lines

Quoted 1 line
What should I do next?
Make your own decision about how best to proceed based on the feedback you've received. Or ask the fstests maintainer to decide what is best.... :P

No, don'... :) jk

Yes, it's worth asking Eryu. I'm ok with either resolution (_require_no_large_scratch_dev or just constrict it to 10x growfs).

--D

Quoted 10 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eryu Guan

2018-05-12 05:53 UTC #7 lore raw

In reply to Zorro Lang

Quoted 17 lines · Zorro Lang

Quoted 5 lines · Dave Chinner
Quoted 4 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
Quoted 8 lines
Quoted 5 lines
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>
I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....
Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ...

Quoted 7 lines

Dave: require_no_largefs is better.
Darrick: nearly ack this patch.
Eric:
2018-04-27 04:03 < sandeen> [15:01] <zoro> [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' 2018-04-27 04:03 < sandeen> just limit growfs to something smaller.
What should I do next?

If testing on large device won't add more test coverage but only test runtime, I'd like to just skip the tests.

Would you please send a new version of the patch that adds _require_no_large_scratch_dev to the affected tests?

And thanks all for the comments!

Eryu

Quoted 16 lines

Thanks,
Zorro
Quoted 8 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Zorro Lang

author

2018-05-12 13:19 UTC #8 lore raw

Quoted 24 lines · Eryu Guan

Quoted 23 lines · Zorro Lang
Quoted 5 lines · Dave Chinner
Quoted 4 lines · Zorro Lang
When test on large SCRATCH_DEV, grow a small XFS to huge size is a horrible thing (e.g grow 128m to 500T). So add a helper named _scratch_xfs_growfs_limited() to do below things:
Quoted 8 lines
Quoted 5 lines
1) If --large-fs is used, limit growfs size.
2) If a limit size parameter is specified, make sure growfs won't beyond this size.
Signed-off-by Zorro Lang <[email protected]>
I think I originally just didn't run growfs tests like this on large filesystems. i.e. require_no_largefs....
Hmm... Sorry, am I facing different review-points from 3 different XFS maintainers? ...
Dave: require_no_largefs is better.
Darrick: nearly ack this patch.
Eric:
2018-04-27 04:03 < sandeen> [15:01] <zoro> [00:55:47] I think maybe use _require_no_large_scratch_dev for xfs/002 will be better. Grow a 128M XFS to large size is 'horrible' 2018-04-27 04:03 < sandeen> just limit growfs to something smaller.

Quoted 7 lines

Quoted 1 line
What should I do next?
If testing on large device won't add more test coverage but only test runtime, I'd like to just skip the tests.
Would you please send a new version of the patch that adds _require_no_large_scratch_dev to the affected tests?

Sure, going to do that.

Quoted 21 lines

And thanks all for the comments!
Eryu
Quoted 16 lines
Thanks,
Zorro
Quoted 8 lines
Cheers,
Dave.
Signature · 2 lines
Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html