| Message ID | 1513048179-151065-1-git-send-email-cgxu519@icloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 12, 2017 at 5:09 AM, Chengguang Xu <cgxu519@icloud.com> wrote: > OverlayFS overrides SCRATCH_MNT variable to it's own, > so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) > when running shutdown on overlayfs. > > Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > --- Maybe "check -overlay overrides SCRATCH_MNT... Otherwise, looks good. 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
On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: > OverlayFS overrides SCRATCH_MNT variable to it's own, > so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) > when running shutdown on overlayfs. > > Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > --- > > Changes since v2: > 1. Make option for _scratch_shutdown(), so caller can > specify option when calling. > 2. Add comment for why overlay requires special handling. > 3. Add commit log. > > Changes since v1: > _scratch_shutdown() does not call notrun. > > common/rc | 37 +++++++++++++++++++++++++++++++++++-- > 1 file changed, 35 insertions(+), 2 deletions(-) > > diff --git a/common/rc b/common/rc > index 4c053a5..a5c4ace 100644 > --- a/common/rc > +++ b/common/rc > @@ -382,6 +382,24 @@ _scratch_cycle_mount() > _scratch_mount "$opts" > } > > +_scratch_shutdown() > +{ > + if [ $FSTYP = "overlay" ]; then > + # In lagacy overlay usage, it may specify directory as Trailing whitespace in above line. > + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV > + # will be null, so check OVL_BASE_SCRATCH_DEV before > + # running shutdown to avoid shutting down base fs accidently. > + if [ -z $OVL_BASE_SCRATCH_DEV ]; then > + echo -n "Ignore shutdown because " > + echo "$SCRATCH_DEV is not a block device" Hmm, I think a '_fail "<message>"' would be more appropriate here, so that test fails and exits immediately. Test should call _require_scratch_shutdown first before calling _scratch_shutdown, if not, it indicates a useage error of the helper, we need to prompt the test writer to call _require_scratch_shutdown first in error message. > + else > + src/godown $* $OVL_BASE_SCRATCH_MNT > + fi > + else > + src/godown $* $SCRATCH_MNT > + fi > +} > + > _test_mount() > { > if [ "$FSTYP" == "overlay" ]; then > @@ -2908,8 +2926,23 @@ _require_scratch_shutdown() > > _scratch_mkfs > /dev/null 2>&1 > _scratch_mount > - src/godown -f $SCRATCH_MNT 2>&1 \ > - || _notrun "$FSTYP does not support shutdown" > + > + if [ $FSTYP = "overlay" ]; then > + if [ -z $OVL_BASE_SCRATCH_DEV ]; then > + # In lagacy overlay usage, it may specify directory as Trailing whitespace in above line. > + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV > + # will be null, so check OVL_BASE_SCRATCH_DEV before > + # running shutdown to avoid shutting down base fs accidently. > + _notrun "$SCRATCH_DEV is not a block device" > + else > + src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \ > + || _notrun "Underlying filesystem does not support shutdown" > + fi > + else > + src/godown -f $SCRATCH_MNT 2>&1 \ > + || _notrun "$FSTYP does not support shutdown" > + fi > + > _scratch_unmount > } Sorry for not bringing this up earlier, but I think we need to convert all existing direct callers of 'godown' to use this new helper, otherwise tests will call godown on an overlay merged dir and result in test failure as: +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device You can go through all tests in 'shutdown' group in generic dir. But I noticed that not all tests in 'shutdown' group can be converted to _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050, they require $SCRATCH_DEV to be a local device file (either block device or char device (ubifs uses char device)) rather than a dir or an NFS export, we need an additional _require rule in both of the tests, so that they won't run on overlay: _require_local_device $SCRATCH_DEV Other tests in 'shutdown' group can be converted easily. Thanks, Eryu > > -- > 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
> > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： > > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: >> OverlayFS overrides SCRATCH_MNT variable to it's own, >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) >> when running shutdown on overlayfs. >> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >> --- >> >> Changes since v2: >> 1. Make option for _scratch_shutdown(), so caller can >> specify option when calling. >> 2. Add comment for why overlay requires special handling. >> 3. Add commit log. >> >> Changes since v1: >> _scratch_shutdown() does not call notrun. >> >> common/rc | 37 +++++++++++++++++++++++++++++++++++-- >> 1 file changed, 35 insertions(+), 2 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index 4c053a5..a5c4ace 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -382,6 +382,24 @@ _scratch_cycle_mount() >> _scratch_mount "$opts" >> } >> >> +_scratch_shutdown() >> +{ >> + if [ $FSTYP = "overlay" ]; then >> + # In lagacy overlay usage, it may specify directory as > > Trailing whitespace in above line. > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >> + # will be null, so check OVL_BASE_SCRATCH_DEV before >> + # running shutdown to avoid shutting down base fs accidently. >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >> + echo -n "Ignore shutdown because " >> + echo "$SCRATCH_DEV is not a block device" > > Hmm, I think a '_fail "<message>"' would be more appropriate here, so > that test fails and exits immediately. Test should call > _require_scratch_shutdown first before calling _scratch_shutdown, if > not, it indicates a useage error of the helper, we need to prompt the > test writer to call _require_scratch_shutdown first in error message. > >> + else >> + src/godown $* $OVL_BASE_SCRATCH_MNT >> + fi >> + else >> + src/godown $* $SCRATCH_MNT >> + fi >> +} >> + >> _test_mount() >> { >> if [ "$FSTYP" == "overlay" ]; then >> @@ -2908,8 +2926,23 @@ _require_scratch_shutdown() >> >> _scratch_mkfs > /dev/null 2>&1 >> _scratch_mount >> - src/godown -f $SCRATCH_MNT 2>&1 \ >> - || _notrun "$FSTYP does not support shutdown" >> + >> + if [ $FSTYP = "overlay" ]; then >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >> + # In lagacy overlay usage, it may specify directory as > > Trailing whitespace in above line. > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >> + # will be null, so check OVL_BASE_SCRATCH_DEV before >> + # running shutdown to avoid shutting down base fs accidently. >> + _notrun "$SCRATCH_DEV is not a block device" >> + else >> + src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \ >> + || _notrun "Underlying filesystem does not support shutdown" >> + fi >> + else >> + src/godown -f $SCRATCH_MNT 2>&1 \ >> + || _notrun "$FSTYP does not support shutdown" >> + fi >> + >> _scratch_unmount >> } > > Sorry for not bringing this up earlier, but I think we need to convert > all existing direct callers of 'godown' to use this new helper, > otherwise tests will call godown on an overlay merged dir and result in > test failure as: > > +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device > > You can go through all tests in 'shutdown' group in generic dir. But I > noticed that not all tests in 'shutdown' group can be converted to > _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050, > they require $SCRATCH_DEV to be a local device file (either block device > or char device (ubifs uses char device)) rather than a dir or an NFS > export, we need an additional _require rule in both of the tests, so > that they won't run on overlay: > > _require_local_device $SCRATCH_DEV > > Other tests in 'shutdown' group can be converted easily. We should fix the improper godown calling for overlayfs in generic test cases, but it’s not directly caused by this patch, so I suggest merging this patch first, then we can make another patch to fix godown problem for overlayfs. What do you think? 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
On Tue, Dec 12, 2017 at 06:20:51PM +0800, Chengguang Xu wrote: > > > > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： > > > > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: > >> OverlayFS overrides SCRATCH_MNT variable to it's own, > >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) > >> when running shutdown on overlayfs. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > >> --- > >> > >> Changes since v2: > >> 1. Make option for _scratch_shutdown(), so caller can > >> specify option when calling. > >> 2. Add comment for why overlay requires special handling. > >> 3. Add commit log. > >> > >> Changes since v1: > >> _scratch_shutdown() does not call notrun. > >> > >> common/rc | 37 +++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 35 insertions(+), 2 deletions(-) > >> > >> diff --git a/common/rc b/common/rc > >> index 4c053a5..a5c4ace 100644 > >> --- a/common/rc > >> +++ b/common/rc > >> @@ -382,6 +382,24 @@ _scratch_cycle_mount() > >> _scratch_mount "$opts" > >> } > >> > >> +_scratch_shutdown() > >> +{ > >> + if [ $FSTYP = "overlay" ]; then > >> + # In lagacy overlay usage, it may specify directory as > > > > Trailing whitespace in above line. > > > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV > >> + # will be null, so check OVL_BASE_SCRATCH_DEV before > >> + # running shutdown to avoid shutting down base fs accidently. > >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then > >> + echo -n "Ignore shutdown because " > >> + echo "$SCRATCH_DEV is not a block device" > > > > Hmm, I think a '_fail "<message>"' would be more appropriate here, so > > that test fails and exits immediately. Test should call > > _require_scratch_shutdown first before calling _scratch_shutdown, if > > not, it indicates a useage error of the helper, we need to prompt the > > test writer to call _require_scratch_shutdown first in error message. > > > >> + else > >> + src/godown $* $OVL_BASE_SCRATCH_MNT > >> + fi > >> + else > >> + src/godown $* $SCRATCH_MNT > >> + fi > >> +} > >> + > >> _test_mount() > >> { > >> if [ "$FSTYP" == "overlay" ]; then > >> @@ -2908,8 +2926,23 @@ _require_scratch_shutdown() > >> > >> _scratch_mkfs > /dev/null 2>&1 > >> _scratch_mount > >> - src/godown -f $SCRATCH_MNT 2>&1 \ > >> - || _notrun "$FSTYP does not support shutdown" > >> + > >> + if [ $FSTYP = "overlay" ]; then > >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then > >> + # In lagacy overlay usage, it may specify directory as > > > > Trailing whitespace in above line. > > > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV > >> + # will be null, so check OVL_BASE_SCRATCH_DEV before > >> + # running shutdown to avoid shutting down base fs accidently. > >> + _notrun "$SCRATCH_DEV is not a block device" > >> + else > >> + src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \ > >> + || _notrun "Underlying filesystem does not support shutdown" > >> + fi > >> + else > >> + src/godown -f $SCRATCH_MNT 2>&1 \ > >> + || _notrun "$FSTYP does not support shutdown" > >> + fi > >> + > >> _scratch_unmount > >> } > > > > Sorry for not bringing this up earlier, but I think we need to convert > > all existing direct callers of 'godown' to use this new helper, > > otherwise tests will call godown on an overlay merged dir and result in > > test failure as: > > > > +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device > > > > You can go through all tests in 'shutdown' group in generic dir. But I > > noticed that not all tests in 'shutdown' group can be converted to > > _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050, > > they require $SCRATCH_DEV to be a local device file (either block device > > or char device (ubifs uses char device)) rather than a dir or an NFS > > export, we need an additional _require rule in both of the tests, so > > that they won't run on overlay: > > > > _require_local_device $SCRATCH_DEV > > > > Other tests in 'shutdown' group can be converted easily. > > We should fix the improper godown calling for overlayfs in generic test cases, > but it’s not directly caused by this patch, so I suggest merging this patch IMO, it's just this patch that causes these kind of false failures in other generic shutdown tests. It's this patch that makes generic shutdown tests running on overlay possible, thus causes false failures. We should fix all these problems in one patch, otherwise anyone tests overlay using xfs/ext4 (or other filesystems that support shutdown) as backing filesystems could see these failures. OTOH, the overlay support in _require_metadata_journaling issue is a relative corner case, I think it's safe to fix that in a separate patch. Thanks, Eryu > first, then we can make another patch to fix godown problem for overlayfs. > What do you think? > > 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
> > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： > > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: >> OverlayFS overrides SCRATCH_MNT variable to it's own, >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) >> when running shutdown on overlayfs. >> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >> --- >> >> Changes since v2: >> 1. Make option for _scratch_shutdown(), so caller can >> specify option when calling. >> 2. Add comment for why overlay requires special handling. >> 3. Add commit log. >> >> Changes since v1: >> _scratch_shutdown() does not call notrun. >> >> common/rc | 37 +++++++++++++++++++++++++++++++++++-- >> 1 file changed, 35 insertions(+), 2 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index 4c053a5..a5c4ace 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -382,6 +382,24 @@ _scratch_cycle_mount() >> _scratch_mount "$opts" >> } >> >> +_scratch_shutdown() >> +{ >> + if [ $FSTYP = "overlay" ]; then >> + # In lagacy overlay usage, it may specify directory as > > Trailing whitespace in above line. > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >> + # will be null, so check OVL_BASE_SCRATCH_DEV before >> + # running shutdown to avoid shutting down base fs accidently. >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >> + echo -n "Ignore shutdown because " >> + echo "$SCRATCH_DEV is not a block device" > > Hmm, I think a '_fail "<message>"' would be more appropriate here, so > that test fails and exits immediately. Test should call > _require_scratch_shutdown first before calling _scratch_shutdown, if > not, it indicates a useage error of the helper, we need to prompt the > test writer to call _require_scratch_shutdown first in error message. The additional check I added here, is mainly for avoiding accident improper shutdown. If you think we need to prompt message to indicate should call _require_scratch_shutdown first, I think we had better modify godown to implement this function. What do you think? 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
On Thu, Dec 14, 2017 at 11:35:04AM +0800, Chengguang Xu wrote: > > > > > 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： > > > > On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: > >> OverlayFS overrides SCRATCH_MNT variable to it's own, > >> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) > >> when running shutdown on overlayfs. > >> > >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > >> --- > >> > >> Changes since v2: > >> 1. Make option for _scratch_shutdown(), so caller can > >> specify option when calling. > >> 2. Add comment for why overlay requires special handling. > >> 3. Add commit log. > >> > >> Changes since v1: > >> _scratch_shutdown() does not call notrun. > >> > >> common/rc | 37 +++++++++++++++++++++++++++++++++++-- > >> 1 file changed, 35 insertions(+), 2 deletions(-) > >> > >> diff --git a/common/rc b/common/rc > >> index 4c053a5..a5c4ace 100644 > >> --- a/common/rc > >> +++ b/common/rc > >> @@ -382,6 +382,24 @@ _scratch_cycle_mount() > >> _scratch_mount "$opts" > >> } > >> > >> +_scratch_shutdown() > >> +{ > >> + if [ $FSTYP = "overlay" ]; then > >> + # In lagacy overlay usage, it may specify directory as > > > > Trailing whitespace in above line. > > > >> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV > >> + # will be null, so check OVL_BASE_SCRATCH_DEV before > >> + # running shutdown to avoid shutting down base fs accidently. > >> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then > >> + echo -n "Ignore shutdown because " > >> + echo "$SCRATCH_DEV is not a block device" > > > > Hmm, I think a '_fail "<message>"' would be more appropriate here, so > > that test fails and exits immediately. Test should call > > _require_scratch_shutdown first before calling _scratch_shutdown, if > > not, it indicates a useage error of the helper, we need to prompt the > > test writer to call _require_scratch_shutdown first in error message. > > > The additional check I added here, is mainly for avoiding accident improper shutdown. Yeah, I understand, and the _fail call I suggested is to exit the test immediately with a proper error message when such an accident usage is detected, while current code just prints a message and continues the rest of the test, which is unnecessary. > If you think we need to prompt message to indicate should call _require_scratch_shutdown first, > I think we had better modify godown to implement this function. What do you think? I just want to see a more specific & clearer error message on such a usage failure, so people know what to do to correct this error. Similar to what we do in common/rc::_require_command (_fail "usage: _require_command <command> [<name>]"). Thanks, Eryu -- 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
> > 在 2017年12月14日，下午12:32，Eryu Guan <eguan@redhat.com> 写道： > > On Thu, Dec 14, 2017 at 11:35:04AM +0800, Chengguang Xu wrote: >> >>> >>> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： >>> >>> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: >>>> OverlayFS overrides SCRATCH_MNT variable to it's own, >>>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) >>>> when running shutdown on overlayfs. >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>>> --- >>>> >>>> Changes since v2: >>>> 1. Make option for _scratch_shutdown(), so caller can >>>> specify option when calling. >>>> 2. Add comment for why overlay requires special handling. >>>> 3. Add commit log. >>>> >>>> Changes since v1: >>>> _scratch_shutdown() does not call notrun. >>>> >>>> common/rc | 37 +++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 35 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index 4c053a5..a5c4ace 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -382,6 +382,24 @@ _scratch_cycle_mount() >>>> _scratch_mount "$opts" >>>> } >>>> >>>> +_scratch_shutdown() >>>> +{ >>>> + if [ $FSTYP = "overlay" ]; then >>>> + # In lagacy overlay usage, it may specify directory as >>> >>> Trailing whitespace in above line. >>> >>>> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >>>> + # will be null, so check OVL_BASE_SCRATCH_DEV before >>>> + # running shutdown to avoid shutting down base fs accidently. >>>> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >>>> + echo -n "Ignore shutdown because " >>>> + echo "$SCRATCH_DEV is not a block device" >>> >>> Hmm, I think a '_fail "<message>"' would be more appropriate here, so >>> that test fails and exits immediately. Test should call >>> _require_scratch_shutdown first before calling _scratch_shutdown, if >>> not, it indicates a useage error of the helper, we need to prompt the >>> test writer to call _require_scratch_shutdown first in error message. >> >> >> The additional check I added here, is mainly for avoiding accident improper shutdown. > > Yeah, I understand, and the _fail call I suggested is to exit the test > immediately with a proper error message when such an accident usage is > detected, while current code just prints a message and continues the > rest of the test, which is unnecessary. > >> If you think we need to prompt message to indicate should call _require_scratch_shutdown first, >> I think we had better modify godown to implement this function. What do you think? > > I just want to see a more specific & clearer error message on such a > usage failure, so people know what to do to correct this error. Similar > to what we do in common/rc::_require_command (_fail "usage: > _require_command <command> [<name>]”). I agree exit the test immediately if you get into this condition. But for error message let me keep it as what it is, because the reason of the failure is actually config problem not improper command usage. 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
> 在 2017年12月12日，下午9:39，Eryu Guan <eguan@redhat.com> 写道： > > On Tue, Dec 12, 2017 at 06:20:51PM +0800, Chengguang Xu wrote: >>> >>> 在 2017年12月12日，下午5:16，Eryu Guan <eguan@redhat.com> 写道： >>> >>> On Tue, Dec 12, 2017 at 11:09:37AM +0800, Chengguang Xu wrote: >>>> OverlayFS overrides SCRATCH_MNT variable to it's own, >>>> so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) >>>> when running shutdown on overlayfs. >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>>> --- >>>> >>>> Changes since v2: >>>> 1. Make option for _scratch_shutdown(), so caller can >>>> specify option when calling. >>>> 2. Add comment for why overlay requires special handling. >>>> 3. Add commit log. >>>> >>>> Changes since v1: >>>> _scratch_shutdown() does not call notrun. >>>> >>>> common/rc | 37 +++++++++++++++++++++++++++++++++++-- >>>> 1 file changed, 35 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index 4c053a5..a5c4ace 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -382,6 +382,24 @@ _scratch_cycle_mount() >>>> _scratch_mount "$opts" >>>> } >>>> >>>> +_scratch_shutdown() >>>> +{ >>>> + if [ $FSTYP = "overlay" ]; then >>>> + # In lagacy overlay usage, it may specify directory as >>> >>> Trailing whitespace in above line. >>> >>>> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >>>> + # will be null, so check OVL_BASE_SCRATCH_DEV before >>>> + # running shutdown to avoid shutting down base fs accidently. >>>> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >>>> + echo -n "Ignore shutdown because " >>>> + echo "$SCRATCH_DEV is not a block device" >>> >>> Hmm, I think a '_fail "<message>"' would be more appropriate here, so >>> that test fails and exits immediately. Test should call >>> _require_scratch_shutdown first before calling _scratch_shutdown, if >>> not, it indicates a useage error of the helper, we need to prompt the >>> test writer to call _require_scratch_shutdown first in error message. >>> >>>> + else >>>> + src/godown $* $OVL_BASE_SCRATCH_MNT >>>> + fi >>>> + else >>>> + src/godown $* $SCRATCH_MNT >>>> + fi >>>> +} >>>> + >>>> _test_mount() >>>> { >>>> if [ "$FSTYP" == "overlay" ]; then >>>> @@ -2908,8 +2926,23 @@ _require_scratch_shutdown() >>>> >>>> _scratch_mkfs > /dev/null 2>&1 >>>> _scratch_mount >>>> - src/godown -f $SCRATCH_MNT 2>&1 \ >>>> - || _notrun "$FSTYP does not support shutdown" >>>> + >>>> + if [ $FSTYP = "overlay" ]; then >>>> + if [ -z $OVL_BASE_SCRATCH_DEV ]; then >>>> + # In lagacy overlay usage, it may specify directory as >>> >>> Trailing whitespace in above line. >>> >>>> + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV >>>> + # will be null, so check OVL_BASE_SCRATCH_DEV before >>>> + # running shutdown to avoid shutting down base fs accidently. >>>> + _notrun "$SCRATCH_DEV is not a block device" >>>> + else >>>> + src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \ >>>> + || _notrun "Underlying filesystem does not support shutdown" >>>> + fi >>>> + else >>>> + src/godown -f $SCRATCH_MNT 2>&1 \ >>>> + || _notrun "$FSTYP does not support shutdown" >>>> + fi >>>> + >>>> _scratch_unmount >>>> } >>> >>> Sorry for not bringing this up earlier, but I think we need to convert >>> all existing direct callers of 'godown' to use this new helper, >>> otherwise tests will call godown on an overlay merged dir and result in >>> test failure as: >>> >>> +/root/xfstests/src/godown: error on xfsctl(GOINGDOWN) of "/mnt/scratch/ovl-mnt": Inappropriate ioctl for device >>> >>> You can go through all tests in 'shutdown' group in generic dir. But I >>> noticed that not all tests in 'shutdown' group can be converted to >>> _scratch_shutdown straightforwardly, e.g. generic/042 and generic/050, >>> they require $SCRATCH_DEV to be a local device file (either block device >>> or char device (ubifs uses char device)) rather than a dir or an NFS >>> export, we need an additional _require rule in both of the tests, so >>> that they won't run on overlay: >>> >>> _require_local_device $SCRATCH_DEV >>> >>> Other tests in 'shutdown' group can be converted easily. >> >> We should fix the improper godown calling for overlayfs in generic test cases, >> but it’s not directly caused by this patch, so I suggest merging this patch > > IMO, it's just this patch that causes these kind of false failures in > other generic shutdown tests. It's this patch that makes generic > shutdown tests running on overlay possible, thus causes false failures. > > We should fix all these problems in one patch, otherwise anyone tests > overlay using xfs/ext4 (or other filesystems that support shutdown) as > backing filesystems could see these failures. OTOH, the overlay support > in _require_metadata_journaling issue is a relative corner case, I think > it's safe to fix that in a separate patch. In order to add overlay support in some requirement checks like _require_metadata_journaling, I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based on it. Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it? 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
On Wed, Jan 3, 2018 at 2:44 PM, Chengguang Xu <cgxu519@icloud.com> wrote: > [...] > In order to add overlay support in some requirement checks like _require_metadata_journaling, > I think it’s better save underlying filesystem type to $OVL_BASE_FSTYP and doing proper check based > on it. > > Currently $OVL_BASE_FSTYP and $FSTYP all set to “overlay”, Is there any specific reason for it? > In _overlay_config_override(): # Config file may specify base fs type, but we obay -overlay flag export OVL_BASE_FSTYP="$FSTYP" export FSTYP=overlay So either your setup is wrong or there is a bug. $OVL_BASE_FSTYP *should* contain the base fs type, but only if you defined FSTYPE in your config file when running ./check -overlay Cheers, 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
diff --git a/common/rc b/common/rc index 4c053a5..a5c4ace 100644 --- a/common/rc +++ b/common/rc @@ -382,6 +382,24 @@ _scratch_cycle_mount() _scratch_mount "$opts" } +_scratch_shutdown() +{ + if [ $FSTYP = "overlay" ]; then + # In lagacy overlay usage, it may specify directory as + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV + # will be null, so check OVL_BASE_SCRATCH_DEV before + # running shutdown to avoid shutting down base fs accidently. + if [ -z $OVL_BASE_SCRATCH_DEV ]; then + echo -n "Ignore shutdown because " + echo "$SCRATCH_DEV is not a block device" + else + src/godown $* $OVL_BASE_SCRATCH_MNT + fi + else + src/godown $* $SCRATCH_MNT + fi +} + _test_mount() { if [ "$FSTYP" == "overlay" ]; then @@ -2908,8 +2926,23 @@ _require_scratch_shutdown() _scratch_mkfs > /dev/null 2>&1 _scratch_mount - src/godown -f $SCRATCH_MNT 2>&1 \ - || _notrun "$FSTYP does not support shutdown" + + if [ $FSTYP = "overlay" ]; then + if [ -z $OVL_BASE_SCRATCH_DEV ]; then + # In lagacy overlay usage, it may specify directory as + # SCRATCH_DEV, in this case OVL_BASE_SCRATCH_DEV + # will be null, so check OVL_BASE_SCRATCH_DEV before + # running shutdown to avoid shutting down base fs accidently. + _notrun "$SCRATCH_DEV is not a block device" + else + src/godown -f $OVL_BASE_SCRATCH_MNT 2>&1 \ + || _notrun "Underlying filesystem does not support shutdown" + fi + else + src/godown -f $SCRATCH_MNT 2>&1 \ + || _notrun "$FSTYP does not support shutdown" + fi + _scratch_unmount }
OverlayFS overrides SCRATCH_MNT variable to it's own, so adjust target to underlying filesystem(OVL_BASE_SCRATCH_MNT) when running shutdown on overlayfs. Signed-off-by: Chengguang Xu <cgxu519@icloud.com> --- Changes since v2: 1. Make option for _scratch_shutdown(), so caller can specify option when calling. 2. Add comment for why overlay requires special handling. 3. Add commit log. Changes since v1: _scratch_shutdown() does not call notrun. common/rc | 37 +++++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-)