diff mbox

[1/2] common/rc: Add syncfs check and a helper _scratch_shutdown()

Message ID 2D8A66F2-6AB9-4B7D-90B0-D3B6807FB353@icloud.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chengguang Xu Dec. 5, 2017, 3:50 p.m. UTC
1. Add a check case in _require_xfs_io_command() to support syncfs.
2. Introduce a helper to support scratch shutdown for overlayfs.

Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
---
common/rc | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)

Comments

Eryu Guan Dec. 5, 2017, 5:14 p.m. UTC | #1
On Tue, Dec 05, 2017 at 06:13:35PM +0200, Amir Goldstein wrote:
> On Tue, Dec 5, 2017 at 5:50 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> > 1. Add a check case in _require_xfs_io_command() to support syncfs.
> > 2. Introduce a helper to support scratch shutdown for overlayfs.
> >
> > Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
> > ---
> > common/rc | 22 ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/common/rc b/common/rc
> > index 4c053a5..e36ee24 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -669,6 +669,21 @@ _scratch_cleanup_files()
> >         esac
> > }
> >
> > +_scratch_shutdown()
> > +{
> > +
> > +       case $FSTYP in
> > +       overlay)
> 
> Looks good,
> but first you need to check for [ -z "$OVL_BASE_SCRATCH_MNT" ]
> meaning that tester is using "old" overlay config and we are not allowed to
> mess with base fs.

Agreed, we don't want to shutdown the root fs accidently.

> 
> > +               src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
> > +                       || _notrun "Underlying filesystem does not support shutdown"
> > +               ;;
> > +       *)
> > +               src/godown -f $SCRATCH_MNT 2>&1 \
> > +                       || _notrun "$FSTYP does not support shutdown"
> > +               ;;
> > +       esac
> > +}

I think this _scratch_shutdown() should be folded into
_require_scratch_shutdown(). The name _scratch_shutdown indicates it
shuts down the scratch device, and call _notrun from it would be a
surprise to users.

> > +
> > _scratch_mkfs()
> > {
> >         local mkfs_cmd=""
> > @@ -2087,6 +2102,10 @@ _require_xfs_io_command()
> >         "utimes" )
> >                 testio=`$XFS_IO_PROG -f -c "utimes" 0 0 0 0 $testfile 2>&1`
> >                 ;;
> > +       "syncfs")
> > +               touch $testfile
> > +               testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
> > +               ;;
> >         *)

This part should be in a separate patch.

Thanks,
Eryu

> >                 testio=`$XFS_IO_PROG -c "help $command" 2>&1`
> >         esac
> > @@ -2908,8 +2927,7 @@ _require_scratch_shutdown()
> >
> >         _scratch_mkfs > /dev/null 2>&1
> >         _scratch_mount
> > -       src/godown -f $SCRATCH_MNT 2>&1 \
> > -               || _notrun "$FSTYP does not support shutdown"
> > +       _scratch_shutdown
> >         _scratch_unmount
> > }
> >
> > --
> > 1.8.3.1
--
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
Chengguang Xu Dec. 6, 2017, 12:59 a.m. UTC | #2
> 在 2017年12月6日,上午1:14,Eryu Guan <eguan@redhat.com> 写道:
> 
> On Tue, Dec 05, 2017 at 06:13:35PM +0200, Amir Goldstein wrote:
>> On Tue, Dec 5, 2017 at 5:50 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>> 1. Add a check case in _require_xfs_io_command() to support syncfs.
>>> 2. Introduce a helper to support scratch shutdown for overlayfs.
>>> 
>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>>> ---
>>> common/rc | 22 ++++++++++++++++++++--
>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/common/rc b/common/rc
>>> index 4c053a5..e36ee24 100644
>>> --- a/common/rc
>>> +++ b/common/rc
>>> @@ -669,6 +669,21 @@ _scratch_cleanup_files()
>>>        esac
>>> }
>>> 
>>> +_scratch_shutdown()
>>> +{
>>> +
>>> +       case $FSTYP in
>>> +       overlay)
>> 
>> Looks good,
>> but first you need to check for [ -z "$OVL_BASE_SCRATCH_MNT" ]
>> meaning that tester is using "old" overlay config and we are not allowed to
>> mess with base fs.
> 
> Agreed, we don't want to shutdown the root fs accidently.

If "$OVL_BASE_SCRATCH_MNT” is nothing, godown command should fail for improper param, right?
I don’t clearly know how it makes rootfs shutdown.
you mean I need to check for [ "$OVL_BASE_SCRATCH_MNT” = “/“ ] ?

> 
>> 
>>> +               src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
>>> +                       || _notrun "Underlying filesystem does not support shutdown"
>>> +               ;;
>>> +       *)
>>> +               src/godown -f $SCRATCH_MNT 2>&1 \
>>> +                       || _notrun "$FSTYP does not support shutdown"
>>> +               ;;
>>> +       esac
>>> +}
> 
> I think this _scratch_shutdown() should be folded into
> _require_scratch_shutdown(). The name _scratch_shutdown indicates it
> shuts down the scratch device, and call _notrun from it would be a
> surprise to users.
> 
>>> +
>>> _scratch_mkfs()
>>> {
>>>        local mkfs_cmd=""
>>> @@ -2087,6 +2102,10 @@ _require_xfs_io_command()
>>>        "utimes" )
>>>                testio=`$XFS_IO_PROG -f -c "utimes" 0 0 0 0 $testfile 2>&1`
>>>                ;;
>>> +       "syncfs")
>>> +               touch $testfile
>>> +               testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
>>> +               ;;
>>>        *)
> 
> This part should be in a separate patch.
> 
> Thanks,
> Eryu
> 
>>>                testio=`$XFS_IO_PROG -c "help $command" 2>&1`
>>>        esac
>>> @@ -2908,8 +2927,7 @@ _require_scratch_shutdown()
>>> 
>>>        _scratch_mkfs > /dev/null 2>&1
>>>        _scratch_mount
>>> -       src/godown -f $SCRATCH_MNT 2>&1 \
>>> -               || _notrun "$FSTYP does not support shutdown"
>>> +       _scratch_shutdown
>>>        _scratch_unmount
>>> }
>>> 
>>> —
>>> 1.8.3.1

Thanks,
cgxu



--
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
Amir Goldstein Dec. 6, 2017, 6:40 a.m. UTC | #3
On Wed, Dec 6, 2017 at 2:59 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
>
>> 在 2017年12月6日,上午1:14,Eryu Guan <eguan@redhat.com> 写道:
>>
>> On Tue, Dec 05, 2017 at 06:13:35PM +0200, Amir Goldstein wrote:
>>> On Tue, Dec 5, 2017 at 5:50 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>> 1. Add a check case in _require_xfs_io_command() to support syncfs.
>>>> 2. Introduce a helper to support scratch shutdown for overlayfs.
>>>>
>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>>>> ---
>>>> common/rc | 22 ++++++++++++++++++++--
>>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/common/rc b/common/rc
>>>> index 4c053a5..e36ee24 100644
>>>> --- a/common/rc
>>>> +++ b/common/rc
>>>> @@ -669,6 +669,21 @@ _scratch_cleanup_files()
>>>>        esac
>>>> }
>>>>
>>>> +_scratch_shutdown()
>>>> +{
>>>> +
>>>> +       case $FSTYP in
>>>> +       overlay)
>>>
>>> Looks good,
>>> but first you need to check for [ -z "$OVL_BASE_SCRATCH_MNT" ]
>>> meaning that tester is using "old" overlay config and we are not allowed to
>>> mess with base fs.
>>
>> Agreed, we don't want to shutdown the root fs accidently.
>
> If "$OVL_BASE_SCRATCH_MNT” is nothing, godown command should fail for improper param, right?
> I don’t clearly know how it makes rootfs shutdown.
> you mean I need to check for [ "$OVL_BASE_SCRATCH_MNT” = “/“ ] ?
>

Sorry, I meant you need to check if OVL_BASE_SCRATCH_DEV is not empty.
OVL_BASE_SCRATCH_MNT will always be valid dir, but either user set it
with old configuration, e.g.:
SCRATCH_DEV=/my/ovl/root
FSTYP=overlay

OR xfstests mounted a base fs with new overlay configuration, e.g.:
SCRATCH_DEV=/dev/my-ovl-bdev
SCRATCH_MNT=/my/ovl/root
FSTYP=xfs

If you shutdown OVL_BASE_SCRATCH_MNT in the first case, you may
shutdown the fs of the host running the test.

Amir.
--
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
Chengguang Xu Dec. 6, 2017, 12:26 p.m. UTC | #4
> 
> 在 2017年12月6日,下午2:40,Amir Goldstein <amir73il@gmail.com> 写道:
> 
> On Wed, Dec 6, 2017 at 2:59 AM, Chengguang Xu <cgxu519@icloud.com> wrote:
>> 
>>> 在 2017年12月6日,上午1:14,Eryu Guan <eguan@redhat.com> 写道:
>>> 
>>> On Tue, Dec 05, 2017 at 06:13:35PM +0200, Amir Goldstein wrote:
>>>> On Tue, Dec 5, 2017 at 5:50 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
>>>>> 1. Add a check case in _require_xfs_io_command() to support syncfs.
>>>>> 2. Introduce a helper to support scratch shutdown for overlayfs.
>>>>> 
>>>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com>
>>>>> ---
>>>>> common/rc | 22 ++++++++++++++++++++--
>>>>> 1 file changed, 20 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/common/rc b/common/rc
>>>>> index 4c053a5..e36ee24 100644
>>>>> --- a/common/rc
>>>>> +++ b/common/rc
>>>>> @@ -669,6 +669,21 @@ _scratch_cleanup_files()
>>>>>       esac
>>>>> }
>>>>> 
>>>>> +_scratch_shutdown()
>>>>> +{
>>>>> +
>>>>> +       case $FSTYP in
>>>>> +       overlay)
>>>> 
>>>> Looks good,
>>>> but first you need to check for [ -z "$OVL_BASE_SCRATCH_MNT" ]
>>>> meaning that tester is using "old" overlay config and we are not allowed to
>>>> mess with base fs.
>>> 
>>> Agreed, we don't want to shutdown the root fs accidently.
>> 
>> If "$OVL_BASE_SCRATCH_MNT” is nothing, godown command should fail for improper param, right?
>> I don’t clearly know how it makes rootfs shutdown.
>> you mean I need to check for [ "$OVL_BASE_SCRATCH_MNT” = “/“ ] ?
>> 
> 
> Sorry, I meant you need to check if OVL_BASE_SCRATCH_DEV is not empty.
> OVL_BASE_SCRATCH_MNT will always be valid dir, but either user set it
> with old configuration, e.g.:
> SCRATCH_DEV=/my/ovl/root
> FSTYP=overlay
> 
> OR xfstests mounted a base fs with new overlay configuration, e.g.:
> SCRATCH_DEV=/dev/my-ovl-bdev
> SCRATCH_MNT=/my/ovl/root
> FSTYP=xfs
> 
> If you shutdown OVL_BASE_SCRATCH_MNT in the first case, you may
> shutdown the fs of the host running the test.

I’ve got your point now, thanks.


Thanks,
Chengguang.--
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
diff mbox

Patch

diff --git a/common/rc b/common/rc
index 4c053a5..e36ee24 100644
--- a/common/rc
+++ b/common/rc
@@ -669,6 +669,21 @@  _scratch_cleanup_files()
	esac
}

+_scratch_shutdown()
+{
+
+	case $FSTYP in
+	overlay)
+		src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \
+			|| _notrun "Underlying filesystem does not support shutdown"
+		;;
+	*)
+		src/godown -f $SCRATCH_MNT 2>&1 \
+			|| _notrun "$FSTYP does not support shutdown"
+		;;
+	esac
+}
+
_scratch_mkfs()
{
	local mkfs_cmd=""
@@ -2087,6 +2102,10 @@  _require_xfs_io_command()
	"utimes" )
		testio=`$XFS_IO_PROG -f -c "utimes" 0 0 0 0 $testfile 2>&1`
		;;
+	"syncfs")
+		touch $testfile
+		testio=`$XFS_IO_PROG -c "syncfs" $testfile 2>&1`
+		;;
	*)
		testio=`$XFS_IO_PROG -c "help $command" 2>&1`
	esac
@@ -2908,8 +2927,7 @@  _require_scratch_shutdown()

	_scratch_mkfs > /dev/null 2>&1
	_scratch_mount
-	src/godown -f $SCRATCH_MNT 2>&1 \
-		|| _notrun "$FSTYP does not support shutdown"
+	_scratch_shutdown
	_scratch_unmount
}