Re: [PATCH v2 1/2] btrfs: add a test case for btrfs seed device delete

[Filipe Manana <fdmanana@gmail.com>]

On Sat, Sep 5, 2020 at 12:25 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> This is a regression test for the issue fixed by the kernel patch
>    btrfs: fix put of uninitialized kobject after seed device delete

Now that the patch is in Linus' tree, we could have the commit id as well.
Just a few comments below.

>
> In this test case, we verify the seed device delete on a sprouted
> filesystem.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2 drop the sysfs layout check as it breaks the test-case backward
> compatibility.
>
>  tests/btrfs/219     | 83 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/btrfs/219.out | 15 ++++++++
>  tests/btrfs/group   |  1 +
>  3 files changed, 99 insertions(+)
>  create mode 100755 tests/btrfs/219
>  create mode 100644 tests/btrfs/219.out
>
> diff --git a/tests/btrfs/219 b/tests/btrfs/219
> new file mode 100755
> index 000000000000..86f2a6991bd7
> --- /dev/null
> +++ b/tests/btrfs/219
> @@ -0,0 +1,83 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Oracle. All Rights Reserved.
> +#
> +# FS QA Test 219
> +#
> +# Test for seed device-delete on a sprouted FS.
> +# Requires kernel patch
> +#    btrfs: fix put of uninitialized kobject after seed device delete
> +#
> +# Steps:
> +#  Create a seed FS. Add a RW device to make it sprout FS and then delete
> +#  the seed device.
> +
> +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

s/generic/btrfs

> +_supported_os Linux

This should go away, _supported_os is gone now.

> +_require_test
> +_require_scratch_dev_pool 2
> +
> +_scratch_dev_pool_get 2
> +
> +seed=$(echo $SCRATCH_DEV_POOL | awk '{print $1}')
> +sprout=$(echo $SCRATCH_DEV_POOL | awk '{print $2}')

$AWK_PROG should be used instead.

> +
> +_mkfs_dev $seed
> +_mount $seed $SCRATCH_MNT
> +
> +$XFS_IO_PROG -f -d -c "pwrite -S 0xab 0 1M" $SCRATCH_MNT/foo > /dev/null

Why the direct IO write? Why not buffered IO?
I just tried the test, and it passes too with a buffered write (no -d).
If there's any reason for using direct IO, it should be mentioned in a
comment, and _require_odirect added at the top.

> +_scratch_unmount
> +$BTRFS_TUNE_PROG -S 1 $seed
> +
> +# Mount the seed device and add the rw device
> +_mount -o ro $seed $SCRATCH_MNT
> +$BTRFS_UTIL_PROG device add -f $sprout $SCRATCH_MNT
> +_scratch_unmount
> +
> +# Now remount
> +_mount $sprout $SCRATCH_MNT
> +$XFS_IO_PROG -f -d -c "pwrite -S 0xcd 0 1M" $SCRATCH_MNT/bar > /dev/null

Same comment here regarding the use of direct IO.

> +
> +echo --- before delete ----
> +od -x $SCRATCH_MNT/foo
> +od -x $SCRATCH_MNT/bar
> +
> +$BTRFS_UTIL_PROG device delete $seed $SCRATCH_MNT
> +_scratch_unmount
> +_btrfs_forget_or_module_reload
> +_mount $sprout $SCRATCH_MNT
> +
> +echo --- after delete ----
> +od -x $SCRATCH_MNT/foo
> +od -x $SCRATCH_MNT/bar
> +
> +_scratch_dev_pool_put
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/219.out b/tests/btrfs/219.out
> new file mode 100644
> index 000000000000..d39e0d8ffafd
> --- /dev/null
> +++ b/tests/btrfs/219.out
> @@ -0,0 +1,15 @@
> +QA output created by 219
> +--- before delete ----
> +0000000 abab abab abab abab abab abab abab abab
> +*
> +4000000
> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> +*
> +4000000
> +--- after delete ----
> +0000000 abab abab abab abab abab abab abab abab
> +*
> +4000000
> +0000000 cdcd cdcd cdcd cdcd cdcd cdcd cdcd cdcd
> +*
> +4000000
> diff --git a/tests/btrfs/group b/tests/btrfs/group
> index 3295856d0c8c..3633fa66abe4 100644
> --- a/tests/btrfs/group
> +++ b/tests/btrfs/group
> @@ -221,3 +221,4 @@
>  216 auto quick seed
>  217 auto quick trim dangerous
>  218 auto quick volume
> +219 auto quick volume seed

New tests were added in the meanwhile.
For the next version don't forget to renumber the test to 224.

Other than those minor comments, it looks fine and it works.

Thanks.

> --
> 2.25.1
>


-- 
Filipe David Manana,

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