[PATCH] fstests: btrfs: Add regression test to check if btrfs can handle high devid

[PATCH] fstests: btrfs: Add regression test to check if btrfs can handle high devid

[Qu Wenruo]

Add a regression test to check if btrfs can handle high devid.

The test will add and remove devices to a btrfs fs, so that the devid
will increase to uncommon but still valid values.

The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
tree-checker: Verify dev item").
The fix is titled "btrfs: tree-checker: Fix wrong check on max devid".

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/btrfs/194     | 73 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/194.out |  2 ++
 tests/btrfs/group   |  1 +
 3 files changed, 76 insertions(+)
 create mode 100755 tests/btrfs/194
 create mode 100644 tests/btrfs/194.out

diff --git a/tests/btrfs/194 b/tests/btrfs/194
new file mode 100755
index 00000000..7a52ed86
--- /dev/null
+++ b/tests/btrfs/194
@@ -0,0 +1,73 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2019 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 194
+#
+# Test if btrfs can handle large device ids.
+#
+# The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
+# tree-checker: Verify dev item").
+# The fix is titlted: "btrfs: tree-checker: Fix wrong check on max devid"
+#
+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 btrfs
+_supported_os Linux
+_require_scratch_dev_pool 2
+_scratch_dev_pool_get 2
+
+# The wrong check limit is based on node size, to reduce runtime, we only
+# support 4K page size system for now
+if [ $(get_page_size) != 4096 ]; then
+	_notrun "This test need 4k page size"
+fi
+
+device_1=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
+device_2=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
+
+echo device_1=$device_1 device_2=$device_2 >> $seqres.full
+
+# Use 4K nodesize to reduce runtime
+_scratch_mkfs -n 4k >> $seqres.full
+_scratch_mount
+
+# Add and remove device in a loop, one loop will increase devid by 2.
+# for 4k nodesize, the wrong check will be triggered at devid 123.
+# here 64 is enough to trigger such regression
+for (( i = 0; i < 64; i++ )); do
+	$BTRFS_UTIL_PROG device add -f $device_2 $SCRATCH_MNT
+	$BTRFS_UTIL_PROG device del $device_1 $SCRATCH_MNT
+	$BTRFS_UTIL_PROG device add -f $device_1 $SCRATCH_MNT
+	$BTRFS_UTIL_PROG device del $device_2 $SCRATCH_MNT
+done
+_scratch_dev_pool_put
+
+echo "Silence is golden"
+
+# success, all done
+status=0
+exit
diff --git a/tests/btrfs/194.out b/tests/btrfs/194.out
new file mode 100644
index 00000000..7bfd50ff
--- /dev/null
+++ b/tests/btrfs/194.out
@@ -0,0 +1,2 @@
+QA output created by 194
+Silence is golden
diff --git a/tests/btrfs/group b/tests/btrfs/group
index b92cb12c..ef1f0e1b 100644
--- a/tests/btrfs/group
+++ b/tests/btrfs/group
@@ -196,3 +196,4 @@
 191 auto quick send dedupe
 192 auto replay snapshot stress
 193 auto quick qgroup enospc limit
+194 auto
-- 
2.22.0

Re: [PATCH] fstests: btrfs: Add regression test to check if btrfs can handle high devid

[Anand Jain]

On 9/30/19 4:37 PM, Qu Wenruo wrote:
> Add a regression test to check if btrfs can handle high devid.
> 
> The test will add and remove devices to a btrfs fs, so that the devid
> will increase to uncommon but still valid values.
> 
> The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
> tree-checker: Verify dev item").
> The fix is titled "btrfs: tree-checker: Fix wrong check on max devid".
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Suggested-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Anand Jain <anand.jain@oracle.com>

one nit below.

> ---
>   tests/btrfs/194     | 73 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/btrfs/194.out |  2 ++
>   tests/btrfs/group   |  1 +
>   3 files changed, 76 insertions(+)
>   create mode 100755 tests/btrfs/194
>   create mode 100644 tests/btrfs/194.out
> 
> diff --git a/tests/btrfs/194 b/tests/btrfs/194
> new file mode 100755
> index 00000000..7a52ed86
> --- /dev/null
> +++ b/tests/btrfs/194
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2019 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 194
> +#
> +# Test if btrfs can handle large device ids.
> +#
> +# The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
> +# tree-checker: Verify dev item").
> +# The fix is titlted: "btrfs: tree-checker: Fix wrong check on max devid"
> +#
> +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 btrfs
> +_supported_os Linux
> +_require_scratch_dev_pool 2
> +_scratch_dev_pool_get 2
> +
> +# The wrong check limit is based on node size, to reduce runtime, we only
> +# support 4K page size system for now
> +if [ $(get_page_size) != 4096 ]; then
> +	_notrun "This test need 4k page size"
> +fi
> +
> +device_1=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
> +device_2=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
> +
> +echo device_1=$device_1 device_2=$device_2 >> $seqres.full
> +
> +# Use 4K nodesize to reduce runtime
> +_scratch_mkfs -n 4k >> $seqres.full

  This init part should be _scratch_mkfs on only one device right?

Thanks, Anand

> +_scratch_mount
> +
> +# Add and remove device in a loop, one loop will increase devid by 2.
> +# for 4k nodesize, the wrong check will be triggered at devid 123.
> +# here 64 is enough to trigger such regression



> +for (( i = 0; i < 64; i++ )); do
> +	$BTRFS_UTIL_PROG device add -f $device_2 $SCRATCH_MNT
> +	$BTRFS_UTIL_PROG device del $device_1 $SCRATCH_MNT
> +	$BTRFS_UTIL_PROG device add -f $device_1 $SCRATCH_MNT
> +	$BTRFS_UTIL_PROG device del $device_2 $SCRATCH_MNT
> +done


> +_scratch_dev_pool_put
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/194.out b/tests/btrfs/194.out
> new file mode 100644
> index 00000000..7bfd50ff
> --- /dev/null
> +++ b/tests/btrfs/194.out
> @@ -0,0 +1,2 @@
> +QA output created by 194
> +Silence is golden
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index b92cb12c..ef1f0e1b 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -196,3 +196,4 @@
>   191 auto quick send dedupe
>   192 auto replay snapshot stress
>   193 auto quick qgroup enospc limit
> +194 auto
> 

Re: [PATCH] fstests: btrfs: Add regression test to check if btrfs can handle high devid

[Filipe Manana]

On Mon, Sep 30, 2019 at 9:39 AM Qu Wenruo <wqu@suse.com> wrote:
>
> Add a regression test to check if btrfs can handle high devid.
>
> The test will add and remove devices to a btrfs fs, so that the devid
> will increase to uncommon but still valid values.
>
> The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
> tree-checker: Verify dev item").
> The fix is titled "btrfs: tree-checker: Fix wrong check on max devid".
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  tests/btrfs/194     | 73 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/194.out |  2 ++
>  tests/btrfs/group   |  1 +
>  3 files changed, 76 insertions(+)
>  create mode 100755 tests/btrfs/194
>  create mode 100644 tests/btrfs/194.out
>
> diff --git a/tests/btrfs/194 b/tests/btrfs/194
> new file mode 100755
> index 00000000..7a52ed86
> --- /dev/null
> +++ b/tests/btrfs/194
> @@ -0,0 +1,73 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (C) 2019 SUSE Linux Products GmbH. All Rights Reserved.
> +#
> +# FS QA Test 194
> +#
> +# Test if btrfs can handle large device ids.
> +#
> +# The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
> +# tree-checker: Verify dev item").
> +# The fix is titlted: "btrfs: tree-checker: Fix wrong check on max devid"

type, titlted -> titled

> +#
> +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 btrfs
> +_supported_os Linux
> +_require_scratch_dev_pool 2
> +_scratch_dev_pool_get 2
> +
> +# The wrong check limit is based on node size, to reduce runtime, we only
> +# support 4K page size system for now
> +if [ $(get_page_size) != 4096 ]; then
> +       _notrun "This test need 4k page size"
> +fi
> +
> +device_1=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
> +device_2=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
> +
> +echo device_1=$device_1 device_2=$device_2 >> $seqres.full
> +
> +# Use 4K nodesize to reduce runtime

How does the node size reduces runtime?
It's obvious when one wants to create deeper/larger trees for some
purpose, but this test doesn't populate the filesystem, it uses an
empty filesystem.
So that deserves an explanation of how the node size influences the
test's running time.

> +_scratch_mkfs -n 4k >> $seqres.full
> +_scratch_mount
> +
> +# Add and remove device in a loop, one loop will increase devid by 2.

" ... one loop will ..." -> "... each iteration will ..."

> +# for 4k nodesize, the wrong check will be triggered at devid 123.

Why 123? It's not clear to me why, the test case doesn't explain and
neither does the btrfs patch that fixes the regression.
If it's related to the value of the constants/functions
BTRFS_MAX_DEVS and BTRFS_MAX_DEVS_SYS_CHUNK, it should be mentioned
here.

> +# here 64 is enough to trigger such regression
> +for (( i = 0; i < 64; i++ )); do
> +       $BTRFS_UTIL_PROG device add -f $device_2 $SCRATCH_MNT
> +       $BTRFS_UTIL_PROG device del $device_1 $SCRATCH_MNT
> +       $BTRFS_UTIL_PROG device add -f $device_1 $SCRATCH_MNT
> +       $BTRFS_UTIL_PROG device del $device_2 $SCRATCH_MNT
> +done
> +_scratch_dev_pool_put
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/194.out b/tests/btrfs/194.out
> new file mode 100644
> index 00000000..7bfd50ff
> --- /dev/null
> +++ b/tests/btrfs/194.out
> @@ -0,0 +1,2 @@
> +QA output created by 194
> +Silence is golden
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index b92cb12c..ef1f0e1b 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -196,3 +196,4 @@
>  191 auto quick send dedupe
>  192 auto replay snapshot stress
>  193 auto quick qgroup enospc limit
> +194 auto

Maybe volume group as well.

Thanks Qu.

> --
> 2.22.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH] fstests: btrfs: Add regression test to check if btrfs can handle high devid

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4984 bytes --]



On 2019/9/30 下午6:33, Filipe Manana wrote:
> On Mon, Sep 30, 2019 at 9:39 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Add a regression test to check if btrfs can handle high devid.
>>
>> The test will add and remove devices to a btrfs fs, so that the devid
>> will increase to uncommon but still valid values.
>>
>> The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
>> tree-checker: Verify dev item").
>> The fix is titled "btrfs: tree-checker: Fix wrong check on max devid".
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  tests/btrfs/194     | 73 +++++++++++++++++++++++++++++++++++++++++++++
>>  tests/btrfs/194.out |  2 ++
>>  tests/btrfs/group   |  1 +
>>  3 files changed, 76 insertions(+)
>>  create mode 100755 tests/btrfs/194
>>  create mode 100644 tests/btrfs/194.out
>>
>> diff --git a/tests/btrfs/194 b/tests/btrfs/194
>> new file mode 100755
>> index 00000000..7a52ed86
>> --- /dev/null
>> +++ b/tests/btrfs/194
>> @@ -0,0 +1,73 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2019 SUSE Linux Products GmbH. All Rights Reserved.
>> +#
>> +# FS QA Test 194
>> +#
>> +# Test if btrfs can handle large device ids.
>> +#
>> +# The regression is introduced by kernel commit ab4ba2e13346 ("btrfs:
>> +# tree-checker: Verify dev item").
>> +# The fix is titlted: "btrfs: tree-checker: Fix wrong check on max devid"
> 
> type, titlted -> titled
> 
>> +#
>> +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 btrfs
>> +_supported_os Linux
>> +_require_scratch_dev_pool 2
>> +_scratch_dev_pool_get 2
>> +
>> +# The wrong check limit is based on node size, to reduce runtime, we only
>> +# support 4K page size system for now
>> +if [ $(get_page_size) != 4096 ]; then
>> +       _notrun "This test need 4k page size"
>> +fi
>> +
>> +device_1=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
>> +device_2=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')
>> +
>> +echo device_1=$device_1 device_2=$device_2 >> $seqres.full
>> +
>> +# Use 4K nodesize to reduce runtime
> 
> How does the node size reduces runtime?

The old wrong check is based on how large a chunk item can be.
The macro we use is BTRFS_MAX_DEVS() which takes fs_info->nodesize to
calculate.

The largest chunk item (or any item) is based on nodesize.
Thus nodesize will affect the runtime.

> It's obvious when one wants to create deeper/larger trees for some
> purpose, but this test doesn't populate the filesystem, it uses an
> empty filesystem.
> So that deserves an explanation of how the node size influences the
> test's running time.

Indeed.

> 
>> +_scratch_mkfs -n 4k >> $seqres.full
>> +_scratch_mount
>> +
>> +# Add and remove device in a loop, one loop will increase devid by 2.
> 
> " ... one loop will ..." -> "... each iteration will ..."
> 
>> +# for 4k nodesize, the wrong check will be triggered at devid 123.
> 
> Why 123? It's not clear to me why, the test case doesn't explain and
> neither does the btrfs patch that fixes the regression.
> If it's related to the value of the constants/functions
> BTRFS_MAX_DEVS and BTRFS_MAX_DEVS_SYS_CHUNK, it should be mentioned
> here.

OK, I'll mention
> 
>> +# here 64 is enough to trigger such regression
>> +for (( i = 0; i < 64; i++ )); do
>> +       $BTRFS_UTIL_PROG device add -f $device_2 $SCRATCH_MNT
>> +       $BTRFS_UTIL_PROG device del $device_1 $SCRATCH_MNT
>> +       $BTRFS_UTIL_PROG device add -f $device_1 $SCRATCH_MNT
>> +       $BTRFS_UTIL_PROG device del $device_2 $SCRATCH_MNT
>> +done
>> +_scratch_dev_pool_put
>> +
>> +echo "Silence is golden"
>> +
>> +# success, all done
>> +status=0
>> +exit
>> diff --git a/tests/btrfs/194.out b/tests/btrfs/194.out
>> new file mode 100644
>> index 00000000..7bfd50ff
>> --- /dev/null
>> +++ b/tests/btrfs/194.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 194
>> +Silence is golden
>> diff --git a/tests/btrfs/group b/tests/btrfs/group
>> index b92cb12c..ef1f0e1b 100644
>> --- a/tests/btrfs/group
>> +++ b/tests/btrfs/group
>> @@ -196,3 +196,4 @@
>>  191 auto quick send dedupe
>>  192 auto replay snapshot stress
>>  193 auto quick qgroup enospc limit
>> +194 auto
> 
> Maybe volume group as well.

Thanks for the hint, I was looking into the groups but can't find a good
candidate.

At least volume makes more sense.

Thanks,
Qu
> 
> Thanks Qu.
> 
>> --
>> 2.22.0
>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]