diff mbox

fstests: btrfs, add missing umount for raid5 tests 124 and 125

Message ID baee4697-7fee-e739-ad34-c4d8a45c453f@oracle.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Anand Jain Dec. 2, 2016, 1:14 a.m. UTC 
Hi,

I didn't add umount at end of the test because...
_check_btrfs_filesystem() does it, which gets called as this test does 
not specify _require_scratch_nocheck

----------------
  _check_btrfs_filesystem()
{
     device=$1

     # If type is set, we're mounted
     type=`_fs_type $device`
     ok=1

     if [ "$type" = "$FSTYP" ]
     then
         # mounted ...
         mountpoint=`_umount_or_remount_ro $device`  <----
     fi

     btrfsck $device >$tmp.fsck 2>&1
----------------


I faced the similar problem on some other tests and I found
adding the delay is the right approach. for eg:

--------------------------
  # success, all done
  status=0
--------------------------


HTH
-Anand



On 11/24/16 14:25, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> The tests mount the second device in the device pool but never unmount
> it, causing the next test to fail.
>
> Example:
>
> $ cat local.config
> export TEST_DEV=/dev/sdb
> export TEST_DIR=/home/fdmanana/btrfs-tests/dev
> export SCRATCH_MNT="/home/fdmanana/btrfs-tests/scratch_1"
> export SCRATCH_DEV_POOL="/dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg"
> export FSTYP=btrfs
>
> $ ./check btrfs/125 btrfs/126
> FSTYP         -- btrfs
> PLATFORM      -- Linux/x86_64 debian3 4.8.0-rc8-btrfs-next-35+
> MKFS_OPTIONS  -- /dev/sdc
> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>
> btrfs/125 23s ... 22s
> btrfs/126 1s ... - output mismatch (see /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad)
>     --- tests/btrfs/126.out	2016-11-24 06:11:42.048372385 +0000
>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad	2016-11-24 06:16:35.987988895 +0000
>     @@ -1,2 +1,5 @@
>      QA output created by 126
>     -pwrite: Disk quota exceeded
>     +ERROR: /dev/sdc is mounted
>     +mount: /dev/sdc is already mounted or /home/fdmanana/btrfs-tests/scratch_1 busy
>     +       /dev/sdc is already mounted on /home/fdmanana/btrfs-tests/scratch_1
>     +/home/fdmanana/btrfs-tests/scratch_1/test_file: Disk quota exceeded
>     ...
>     (Run 'diff -u tests/btrfs/126.out /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad'  to see the entire diff)
> Ran: btrfs/125 btrfs/126
> Failures: btrfs/126
> Failed 1 of 2 tests
>
> So just make sure those test unmount the device before they finish.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  tests/btrfs/124 | 1 +
>  tests/btrfs/125 | 1 +
>  2 files changed, 2 insertions(+)
>
> diff --git a/tests/btrfs/124 b/tests/btrfs/124
> index 2618a26..7206094 100755
> --- a/tests/btrfs/124
> +++ b/tests/btrfs/124
> @@ -159,6 +159,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>  	echo "Inital sum does not match with data on dev2 written by balance"
>  fi
>
> +$UMOUNT_PROG $dev2
>  _scratch_dev_pool_put
>  _test_mount
>
> diff --git a/tests/btrfs/125 b/tests/btrfs/125
> index 1062b87..91aa8d8 100755
> --- a/tests/btrfs/125
> +++ b/tests/btrfs/125
> @@ -175,6 +175,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>  	echo "Inital sum does not match with data on dev2 written by balance"
>  fi
>
> +$UMOUNT_PROG $dev2
>  _scratch_dev_pool_put
>  _test_mount
>
>
--
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

Comments

Filipe Manana Dec. 2, 2016, 10:54 a.m. UTC | #1 
On Fri, Dec 2, 2016 at 1:14 AM, Anand Jain <anand.jain@oracle.com> wrote:
>
> Hi,
>
> I didn't add umount at end of the test because...
> _check_btrfs_filesystem() does it, which gets called as this test does not

Nop it doesn't.

See common/check:

_check_filesystems()
{
   if [ -f ${RESULT_DIR}/require_test ]; then
      _check_test_fs || err=true
   rm -f ${RESULT_DIR}/require_test
   fi
   if [ -f ${RESULT_DIR}/require_scratch ]; then
     _check_scratch_fs || err=true
     rm -f ${RESULT_DIR}/require_scratch
   fi
}

Those files are created by the _require_scratch and _require_test
functions, which are not called by btrfs/12[45].


> specify _require_scratch_nocheck
>
> ----------------
>  _check_btrfs_filesystem()
> {
>     device=$1
>
>     # If type is set, we're mounted
>     type=`_fs_type $device`
>     ok=1
>
>     if [ "$type" = "$FSTYP" ]
>     then
>         # mounted ...
>         mountpoint=`_umount_or_remount_ro $device`  <----
>     fi
>
>     btrfsck $device >$tmp.fsck 2>&1
> ----------------
>
>
> I faced the similar problem on some other tests and I found
> adding the delay is the right approach. for eg:
>
> --------------------------
> diff --git a/tests/generic/298 b/tests/generic/298
> index e85db1266fa9..4092efa6b961 100755
> --- a/tests/generic/298
> +++ b/tests/generic/298
> @@ -92,7 +92,7 @@ echo "reflink of $n bytes took $delta seconds" >>
> $seqres.full
>  test $delta -gt $timeout && _fail "reflink didn't stop in time, n=$n
> t=$delta"
>
>  echo "Check scratch fs"
> -sleep 2                # give it a few seconds to actually die...
> +sleep 40               # give it a few seconds to actually die...


That might work for that test, but certainly not for btrfs/12[45] as
mentioned above.
Besides, that's a terrible solution. What guarantees you have that 40
seconds is enough for  everyone...

thanks

>
>  # success, all done
>  status=0
> --------------------------
>
>
> HTH
> -Anand
>
>
>
>
> On 11/24/16 14:25, fdmanana@kernel.org wrote:
>>
>> From: Filipe Manana <fdmanana@suse.com>
>>
>> The tests mount the second device in the device pool but never unmount
>> it, causing the next test to fail.
>>
>> Example:
>>
>> $ cat local.config
>> export TEST_DEV=/dev/sdb
>> export TEST_DIR=/home/fdmanana/btrfs-tests/dev
>> export SCRATCH_MNT="/home/fdmanana/btrfs-tests/scratch_1"
>> export SCRATCH_DEV_POOL="/dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg"
>> export FSTYP=btrfs
>>
>> $ ./check btrfs/125 btrfs/126
>> FSTYP         -- btrfs
>> PLATFORM      -- Linux/x86_64 debian3 4.8.0-rc8-btrfs-next-35+
>> MKFS_OPTIONS  -- /dev/sdc
>> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>>
>> btrfs/125 23s ... 22s
>> btrfs/126 1s ... - output mismatch (see
>> /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad)
>>     --- tests/btrfs/126.out     2016-11-24 06:11:42.048372385 +0000
>>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad
>> 2016-11-24 06:16:35.987988895 +0000
>>     @@ -1,2 +1,5 @@
>>      QA output created by 126
>>     -pwrite: Disk quota exceeded
>>     +ERROR: /dev/sdc is mounted
>>     +mount: /dev/sdc is already mounted or
>> /home/fdmanana/btrfs-tests/scratch_1 busy
>>     +       /dev/sdc is already mounted on
>> /home/fdmanana/btrfs-tests/scratch_1
>>     +/home/fdmanana/btrfs-tests/scratch_1/test_file: Disk quota exceeded
>>     ...
>>     (Run 'diff -u tests/btrfs/126.out
>> /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad'  to see the
>> entire diff)
>> Ran: btrfs/125 btrfs/126
>> Failures: btrfs/126
>> Failed 1 of 2 tests
>>
>> So just make sure those test unmount the device before they finish.
>>
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>> ---
>>  tests/btrfs/124 | 1 +
>>  tests/btrfs/125 | 1 +
>>  2 files changed, 2 insertions(+)
>>
>> diff --git a/tests/btrfs/124 b/tests/btrfs/124
>> index 2618a26..7206094 100755
>> --- a/tests/btrfs/124
>> +++ b/tests/btrfs/124
>> @@ -159,6 +159,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>>         echo "Inital sum does not match with data on dev2 written by
>> balance"
>>  fi
>>
>> +$UMOUNT_PROG $dev2
>>  _scratch_dev_pool_put
>>  _test_mount
>>
>> diff --git a/tests/btrfs/125 b/tests/btrfs/125
>> index 1062b87..91aa8d8 100755
>> --- a/tests/btrfs/125
>> +++ b/tests/btrfs/125
>> @@ -175,6 +175,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>>         echo "Inital sum does not match with data on dev2 written by
>> balance"
>>  fi
>>
>> +$UMOUNT_PROG $dev2
>>  _scratch_dev_pool_put
>>  _test_mount
>>
>>
>
--
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
Anand Jain Dec. 2, 2016, 11:56 a.m. UTC | #2 
On 12/02/16 18:54, Filipe Manana wrote:
> On Fri, Dec 2, 2016 at 1:14 AM, Anand Jain <anand.jain@oracle.com> wrote:
>>
>> Hi,
>>
>> I didn't add umount at end of the test because...
>> _check_btrfs_filesystem() does it, which gets called as this test does not
>
> Nop it doesn't.
>
> See common/check:
>
> _check_filesystems()
> {
>    if [ -f ${RESULT_DIR}/require_test ]; then
>       _check_test_fs || err=true
>    rm -f ${RESULT_DIR}/require_test
>    fi
>    if [ -f ${RESULT_DIR}/require_scratch ]; then
>      _check_scratch_fs || err=true
>      rm -f ${RESULT_DIR}/require_scratch
>    fi
> }
>
> Those files are created by the _require_scratch and _require_test
> functions, which are not called by btrfs/12[45].
>

  Ah yes. you are right.

>> specify _require_scratch_nocheck
>>
>> ----------------
>>  _check_btrfs_filesystem()
>> {
>>     device=$1
>>
>>     # If type is set, we're mounted
>>     type=`_fs_type $device`
>>     ok=1
>>
>>     if [ "$type" = "$FSTYP" ]
>>     then
>>         # mounted ...
>>         mountpoint=`_umount_or_remount_ro $device`  <----
>>     fi
>>
>>     btrfsck $device >$tmp.fsck 2>&1
>> ----------------
>>
>>
>> I faced the similar problem on some other tests and I found
>> adding the delay is the right approach. for eg:
>>
>> --------------------------
>> diff --git a/tests/generic/298 b/tests/generic/298
>> index e85db1266fa9..4092efa6b961 100755
>> --- a/tests/generic/298
>> +++ b/tests/generic/298
>> @@ -92,7 +92,7 @@ echo "reflink of $n bytes took $delta seconds" >>
>> $seqres.full
>>  test $delta -gt $timeout && _fail "reflink didn't stop in time, n=$n
>> t=$delta"
>>
>>  echo "Check scratch fs"
>> -sleep 2                # give it a few seconds to actually die...
>> +sleep 40               # give it a few seconds to actually die...
>
>
> That might work for that test, but certainly not for btrfs/12[45] as
> mentioned above.
> Besides, that's a terrible solution. What guarantees you have that 40
> seconds is enough for  everyone...

  yep. I agree.

thanks,
Anand


> thanks
>
>>
>>  # success, all done
>>  status=0
>> --------------------------
>>
>>
>> HTH
>> -Anand
>>
>>
>>
>>
>> On 11/24/16 14:25, fdmanana@kernel.org wrote:
>>>
>>> From: Filipe Manana <fdmanana@suse.com>
>>>
>>> The tests mount the second device in the device pool but never unmount
>>> it, causing the next test to fail.
>>>
>>> Example:
>>>
>>> $ cat local.config
>>> export TEST_DEV=/dev/sdb
>>> export TEST_DIR=/home/fdmanana/btrfs-tests/dev
>>> export SCRATCH_MNT="/home/fdmanana/btrfs-tests/scratch_1"
>>> export SCRATCH_DEV_POOL="/dev/sdc /dev/sdd /dev/sde /dev/sdf /dev/sdg"
>>> export FSTYP=btrfs
>>>
>>> $ ./check btrfs/125 btrfs/126
>>> FSTYP         -- btrfs
>>> PLATFORM      -- Linux/x86_64 debian3 4.8.0-rc8-btrfs-next-35+
>>> MKFS_OPTIONS  -- /dev/sdc
>>> MOUNT_OPTIONS -- /dev/sdc /home/fdmanana/btrfs-tests/scratch_1
>>>
>>> btrfs/125 23s ... 22s
>>> btrfs/126 1s ... - output mismatch (see
>>> /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad)
>>>     --- tests/btrfs/126.out     2016-11-24 06:11:42.048372385 +0000
>>>     +++ /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad
>>> 2016-11-24 06:16:35.987988895 +0000
>>>     @@ -1,2 +1,5 @@
>>>      QA output created by 126
>>>     -pwrite: Disk quota exceeded
>>>     +ERROR: /dev/sdc is mounted
>>>     +mount: /dev/sdc is already mounted or
>>> /home/fdmanana/btrfs-tests/scratch_1 busy
>>>     +       /dev/sdc is already mounted on
>>> /home/fdmanana/btrfs-tests/scratch_1
>>>     +/home/fdmanana/btrfs-tests/scratch_1/test_file: Disk quota exceeded
>>>     ...
>>>     (Run 'diff -u tests/btrfs/126.out
>>> /home/fdmanana/git/hub/xfstests/results//btrfs/126.out.bad'  to see the
>>> entire diff)
>>> Ran: btrfs/125 btrfs/126
>>> Failures: btrfs/126
>>> Failed 1 of 2 tests
>>>
>>> So just make sure those test unmount the device before they finish.
>>>
>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>> ---
>>>  tests/btrfs/124 | 1 +
>>>  tests/btrfs/125 | 1 +
>>>  2 files changed, 2 insertions(+)
>>>
>>> diff --git a/tests/btrfs/124 b/tests/btrfs/124
>>> index 2618a26..7206094 100755
>>> --- a/tests/btrfs/124
>>> +++ b/tests/btrfs/124
>>> @@ -159,6 +159,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>>>         echo "Inital sum does not match with data on dev2 written by
>>> balance"
>>>  fi
>>>
>>> +$UMOUNT_PROG $dev2
>>>  _scratch_dev_pool_put
>>>  _test_mount
>>>
>>> diff --git a/tests/btrfs/125 b/tests/btrfs/125
>>> index 1062b87..91aa8d8 100755
>>> --- a/tests/btrfs/125
>>> +++ b/tests/btrfs/125
>>> @@ -175,6 +175,7 @@ if [ "$checkpoint1" != "$checkpoint3" ]; then
>>>         echo "Inital sum does not match with data on dev2 written by
>>> balance"
>>>  fi
>>>
>>> +$UMOUNT_PROG $dev2
>>>  _scratch_dev_pool_put
>>>  _test_mount
>>>
>>>
>>
> --
> 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
>
--
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/298 b/tests/generic/298
index e85db1266fa9..4092efa6b961 100755
--- a/tests/generic/298
+++ b/tests/generic/298
@@ -92,7 +92,7 @@  echo "reflink of $n bytes took $delta seconds" >> 
$seqres.full
  test $delta -gt $timeout && _fail "reflink didn't stop in time, n=$n 
t=$delta"

  echo "Check scratch fs"
-sleep 2		# give it a few seconds to actually die...
+sleep 40		# give it a few seconds to actually die...