* [PATCH v2 0/5] re-enable tests that require scratch dev on NFS
@ 2014-10-31 17:03 Eryu Guan
2014-10-31 17:03 ` [PATCH v2 1/5] common: " Eryu Guan
` (5 more replies)
0 siblings, 6 replies; 31+ messages in thread
From: Eryu Guan @ 2014-10-31 17:03 UTC (permalink / raw)
To: fstests; +Cc: linux-nfs, Eryu Guan
The commit below disables tests requires scratch dev running on NFS
c041421 xfstests: stop special casing nfs and udf
Now re-enable them to get a larger test coverage on NFS.
Also do more updates to avoid unnecessary failures on NFS.
I tested against NFSv3 NFSv4.0 NFSv4.1 (both server and client are linux
running 3.18-rc1 kernel), the results look good.
Failures on NFSv3:
generic/035 generic/089 generic/258 generic/294
Failures on NFSv4.0:
generic/035 generic/169 generic/294
Failures on NFSv4.1:
I hit kernel BUG_ON when testing on NFSv4.1 in generic/285
SEEK_DATA/SEEK_HOLE test. I think there's already a patch to fix it.
http://www.spinics.net/lists/linux-nfs/msg47359.html
Note that generic/294 does remount,ro on SCRATCH_DEV, but TEST_DEV is
affected too, so some tests after generic/294 fail because of EROFS.
Run the failed tests separately and they all passed.
I'm not sure if it's a bug, but I filed one, please see
https://bugzilla.redhat.com/show_bug.cgi?id=1158046
The third patch disables all atime related tests on NFS, and Christoph
starts a discussion about NFS atime handling issue in v1 thread. Before
there's a conclusion I keep the patch as it is, and we can update it
later when we decide to test atime on NFS again.
v2:
- introduce _scratch_cleanup_files helper to remove all files on
$SCRATCH_MNT, for later CIFS use. (Christoph Hellwig)
- split _require_relatime change into another patch (Christoph Hellwig)
v1: http://www.spinics.net/lists/linux-nfs/msg47423.html
Thanks,
Eryu
---
Eryu Guan (5):
common: re-enable tests that require scratch dev on NFS
common: add _require_block_device() helper
common: skip atime related tests on NFS
common: use _scratch_mount helper in _require_relatime()
generic/277: add _require_attrs
common/rc | 46 ++++++++++++++++++++++++++++++++++++++++++----
tests/generic/003 | 1 +
tests/generic/076 | 1 +
tests/generic/192 | 1 +
tests/generic/277 | 2 ++
5 files changed, 47 insertions(+), 4 deletions(-)
--
1.9.3
^ permalink raw reply [flat|nested] 31+ messages in thread* [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan @ 2014-10-31 17:03 ` Eryu Guan 2014-11-10 2:12 ` Dave Chinner 2014-11-12 18:36 ` Steve French 2014-10-31 17:03 ` [PATCH v2 2/5] common: add _require_block_device() helper Eryu Guan ` (4 subsequent siblings) 5 siblings, 2 replies; 31+ messages in thread From: Eryu Guan @ 2014-10-31 17:03 UTC (permalink / raw) To: fstests; +Cc: linux-nfs, Eryu Guan This commit disables tests requires scratch dev running on NFS c041421 xfstests: stop special casing nfs and udf Now re-enable them to get a larger test coverage on NFS. Signed-off-by: Eryu Guan <eguan@redhat.com> --- common/rc | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/common/rc b/common/rc index 747cf72..ae03712 100644 --- a/common/rc +++ b/common/rc @@ -551,6 +551,14 @@ _mkfs_dev() rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd } +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS +_scratch_cleanup_files() +{ + _scratch_mount + rm -rf $SCRATCH_MNT/* + _scratch_unmount +} + _scratch_mkfs() { case $FSTYP in @@ -558,7 +566,9 @@ _scratch_mkfs() _scratch_mkfs_xfs $* ;; nfs*) - # do nothing for nfs + # unable to re-create NFS, just remove all files in $SCRATCH_MNT to + # avoid EEXIST caused by the leftover files created in previous runs + _scratch_cleanup_files ;; cifs) # do nothing for cifs @@ -1032,8 +1042,14 @@ _require_scratch_nocheck() { case "$FSTYP" in nfs*) - _notrun "requires a scratch device" - ;; + echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1 + if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then + _notrun "this test requires a valid \$SCRATCH_DEV" + fi + if [ ! -d "$SCRATCH_MNT" ]; then + _notrun "this test requires a valid \$SCRATCH_MNT" + fi + ;; cifs) _notrun "requires a scratch device" ;; -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-10-31 17:03 ` [PATCH v2 1/5] common: " Eryu Guan @ 2014-11-10 2:12 ` Dave Chinner 2014-11-10 4:05 ` Eryu Guan 2014-11-12 18:36 ` Steve French 1 sibling, 1 reply; 31+ messages in thread From: Dave Chinner @ 2014-11-10 2:12 UTC (permalink / raw) To: Eryu Guan; +Cc: fstests, linux-nfs On Sat, Nov 01, 2014 at 01:03:56AM +0800, Eryu Guan wrote: > This commit disables tests requires scratch dev running on NFS > > c041421 xfstests: stop special casing nfs and udf > > Now re-enable them to get a larger test coverage on NFS. > > Signed-off-by: Eryu Guan <eguan@redhat.com> I'll commit this as is, but can you send another patch to wire this same functionality up for CIFS? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-11-10 2:12 ` Dave Chinner @ 2014-11-10 4:05 ` Eryu Guan 0 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-11-10 4:05 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests, linux-nfs On Mon, Nov 10, 2014 at 01:12:48PM +1100, Dave Chinner wrote: > On Sat, Nov 01, 2014 at 01:03:56AM +0800, Eryu Guan wrote: > > This commit disables tests requires scratch dev running on NFS > > > > c041421 xfstests: stop special casing nfs and udf > > > > Now re-enable them to get a larger test coverage on NFS. > > > > Signed-off-by: Eryu Guan <eguan@redhat.com> > > I'll commit this as is, but can you send another patch to wire this > same functionality up for CIFS? Sure, will do that. Thanks, Eryu ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-12 18:36 ` Steve French 0 siblings, 0 replies; 31+ messages in thread From: Steve French @ 2014-11-12 18:36 UTC (permalink / raw) To: Eryu Guan Cc: fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, dros There should be a check to make sure SCRATCH_MNT exists before you wipe the whole disk .... +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS +_scratch_cleanup_files() +{ + _scratch_mount + rm -rf $SCRATCH_MNT/* + _scratch_unmount +} + so if no SCRATCH_MNT then this does rm -rf/* right ... (and wipes out your whole system ...) On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > This commit disables tests requires scratch dev running on NFS > > c041421 xfstests: stop special casing nfs and udf > > Now re-enable them to get a larger test coverage on NFS. > > Signed-off-by: Eryu Guan <eguan@redhat.com> > --- > common/rc | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/common/rc b/common/rc > index 747cf72..ae03712 100644 > --- a/common/rc > +++ b/common/rc > @@ -551,6 +551,14 @@ _mkfs_dev() > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > } > > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > +_scratch_cleanup_files() > +{ > + _scratch_mount > + rm -rf $SCRATCH_MNT/* > + _scratch_unmount > +} > + > _scratch_mkfs() > { > case $FSTYP in > @@ -558,7 +566,9 @@ _scratch_mkfs() > _scratch_mkfs_xfs $* > ;; > nfs*) > - # do nothing for nfs > + # unable to re-create NFS, just remove all files in $SCRATCH_MNT to > + # avoid EEXIST caused by the leftover files created in previous runs > + _scratch_cleanup_files > ;; > cifs) > # do nothing for cifs > @@ -1032,8 +1042,14 @@ _require_scratch_nocheck() > { > case "$FSTYP" in > nfs*) > - _notrun "requires a scratch device" > - ;; > + echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1 > + if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then > + _notrun "this test requires a valid \$SCRATCH_DEV" > + fi > + if [ ! -d "$SCRATCH_MNT" ]; then > + _notrun "this test requires a valid \$SCRATCH_MNT" > + fi > + ;; > cifs) > _notrun "requires a scratch device" > ;; > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thanks, Steve ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-12 18:36 ` Steve French 0 siblings, 0 replies; 31+ messages in thread From: Steve French @ 2014-11-12 18:36 UTC (permalink / raw) To: Eryu Guan Cc: fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dros-7I+n7zu2hftEKMMhf/gKZA There should be a check to make sure SCRATCH_MNT exists before you wipe the whole disk .... +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS +_scratch_cleanup_files() +{ + _scratch_mount + rm -rf $SCRATCH_MNT/* + _scratch_unmount +} + so if no SCRATCH_MNT then this does rm -rf/* right ... (and wipes out your whole system ...) On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > This commit disables tests requires scratch dev running on NFS > > c041421 xfstests: stop special casing nfs and udf > > Now re-enable them to get a larger test coverage on NFS. > > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > --- > common/rc | 22 +++++++++++++++++++--- > 1 file changed, 19 insertions(+), 3 deletions(-) > > diff --git a/common/rc b/common/rc > index 747cf72..ae03712 100644 > --- a/common/rc > +++ b/common/rc > @@ -551,6 +551,14 @@ _mkfs_dev() > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > } > > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > +_scratch_cleanup_files() > +{ > + _scratch_mount > + rm -rf $SCRATCH_MNT/* > + _scratch_unmount > +} > + > _scratch_mkfs() > { > case $FSTYP in > @@ -558,7 +566,9 @@ _scratch_mkfs() > _scratch_mkfs_xfs $* > ;; > nfs*) > - # do nothing for nfs > + # unable to re-create NFS, just remove all files in $SCRATCH_MNT to > + # avoid EEXIST caused by the leftover files created in previous runs > + _scratch_cleanup_files > ;; > cifs) > # do nothing for cifs > @@ -1032,8 +1042,14 @@ _require_scratch_nocheck() > { > case "$FSTYP" in > nfs*) > - _notrun "requires a scratch device" > - ;; > + echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1 > + if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then > + _notrun "this test requires a valid \$SCRATCH_DEV" > + fi > + if [ ! -d "$SCRATCH_MNT" ]; then > + _notrun "this test requires a valid \$SCRATCH_MNT" > + fi > + ;; > cifs) > _notrun "requires a scratch device" > ;; > -- > 1.9.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thanks, Steve ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-13 3:33 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-13 3:33 UTC (permalink / raw) To: Steve French Cc: Eryu Guan, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, dros On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > > This commit disables tests requires scratch dev running on NFS > > > > c041421 xfstests: stop special casing nfs and udf > > > > Now re-enable them to get a larger test coverage on NFS. > > > > Signed-off-by: Eryu Guan <eguan@redhat.com> > > --- > > common/rc | 22 +++++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index 747cf72..ae03712 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -551,6 +551,14 @@ _mkfs_dev() > > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > } > > > > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > +_scratch_cleanup_files() > > +{ > > + _scratch_mount > > + rm -rf $SCRATCH_MNT/* > > + _scratch_unmount > > +} > > There should be a check to make sure SCRATCH_MNT exists before you > wipe the whole disk .... > > so if no SCRATCH_MNT then this does rm -rf/* > right ... (and wipes out your whole system ...) You can't get to that function until after all the checks that SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that is only called in tests after all the startup checks validate devices and mounts exist. i.e. see common/config::get_next_config() Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-13 3:33 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-13 3:33 UTC (permalink / raw) To: Steve French Cc: Eryu Guan, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dros-7I+n7zu2hftEKMMhf/gKZA On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > This commit disables tests requires scratch dev running on NFS > > > > c041421 xfstests: stop special casing nfs and udf > > > > Now re-enable them to get a larger test coverage on NFS. > > > > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > > --- > > common/rc | 22 +++++++++++++++++++--- > > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index 747cf72..ae03712 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -551,6 +551,14 @@ _mkfs_dev() > > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > } > > > > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > +_scratch_cleanup_files() > > +{ > > + _scratch_mount > > + rm -rf $SCRATCH_MNT/* > > + _scratch_unmount > > +} > > There should be a check to make sure SCRATCH_MNT exists before you > wipe the whole disk .... > > so if no SCRATCH_MNT then this does rm -rf/* > right ... (and wipes out your whole system ...) You can't get to that function until after all the checks that SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that is only called in tests after all the startup checks validate devices and mounts exist. i.e. see common/config::get_next_config() Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-11-13 3:33 ` Dave Chinner @ 2014-11-14 17:02 ` Steve French -1 siblings, 0 replies; 31+ messages in thread From: Steve French @ 2014-11-14 17:02 UTC (permalink / raw) To: Dave Chinner Cc: Eryu Guan, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: >> > This commit disables tests requires scratch dev running on NFS >> > >> > c041421 xfstests: stop special casing nfs and udf >> > >> > Now re-enable them to get a larger test coverage on NFS. >> > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> >> > --- >> > common/rc | 22 +++++++++++++++++++--- >> > 1 file changed, 19 insertions(+), 3 deletions(-) >> > >> > diff --git a/common/rc b/common/rc >> > index 747cf72..ae03712 100644 >> > --- a/common/rc >> > +++ b/common/rc >> > @@ -551,6 +551,14 @@ _mkfs_dev() >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd >> > } >> > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS >> > +_scratch_cleanup_files() >> > +{ >> > + _scratch_mount >> > + rm -rf $SCRATCH_MNT/* >> > + _scratch_unmount >> > +} >> >> There should be a check to make sure SCRATCH_MNT exists before you >> wipe the whole disk .... >> >> so if no SCRATCH_MNT then this does rm -rf/* >> right ... (and wipes out your whole system ...) > > You can't get to that function until after all the checks that > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > is only called in tests after all the startup checks validate > devices and mounts exist. i.e. see common/config::get_next_config() Well, I reproduced it easily enough again today (after taking a snapshot of the VM) by simply running generic/120 against NFS with SCRATCH_MNT not specified in local.config Dros also ran into this problem. The patch below fixes it for me but it wasn't immediately obvious how to best return info to the user (ie print messages that make sense here - "echo" seems to be supressed in common/rc and notrun exits and logs it to a file but not to the screen in this case) sfrench@ubuntu:~/xfstests$ git diff -a diff --git a/common/rc b/common/rc index d5e3aff..866244b 100644 --- a/common/rc +++ b/common/rc @@ -555,6 +555,9 @@ _mkfs_dev() _scratch_cleanup_files() { _scratch_mount + if ! [ "$SCRATCH_MNT" ]; then + _notrun "this test requires a \$SCRATCH_MNT to be specified" + fi rm -rf $SCRATCH_MNT/* _scratch_unmount } -- Thanks, Steve ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-14 17:02 ` Steve French 0 siblings, 0 replies; 31+ messages in thread From: Steve French @ 2014-11-14 17:02 UTC (permalink / raw) To: Dave Chinner Cc: Eryu Guan, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: >> > This commit disables tests requires scratch dev running on NFS >> > >> > c041421 xfstests: stop special casing nfs and udf >> > >> > Now re-enable them to get a larger test coverage on NFS. >> > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> >> > --- >> > common/rc | 22 +++++++++++++++++++--- >> > 1 file changed, 19 insertions(+), 3 deletions(-) >> > >> > diff --git a/common/rc b/common/rc >> > index 747cf72..ae03712 100644 >> > --- a/common/rc >> > +++ b/common/rc >> > @@ -551,6 +551,14 @@ _mkfs_dev() >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd >> > } >> > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS >> > +_scratch_cleanup_files() >> > +{ >> > + _scratch_mount >> > + rm -rf $SCRATCH_MNT/* >> > + _scratch_unmount >> > +} >> >> There should be a check to make sure SCRATCH_MNT exists before you >> wipe the whole disk .... >> >> so if no SCRATCH_MNT then this does rm -rf/* >> right ... (and wipes out your whole system ...) > > You can't get to that function until after all the checks that > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > is only called in tests after all the startup checks validate > devices and mounts exist. i.e. see common/config::get_next_config() Well, I reproduced it easily enough again today (after taking a snapshot of the VM) by simply running generic/120 against NFS with SCRATCH_MNT not specified in local.config Dros also ran into this problem. The patch below fixes it for me but it wasn't immediately obvious how to best return info to the user (ie print messages that make sense here - "echo" seems to be supressed in common/rc and notrun exits and logs it to a file but not to the screen in this case) sfrench@ubuntu:~/xfstests$ git diff -a diff --git a/common/rc b/common/rc index d5e3aff..866244b 100644 --- a/common/rc +++ b/common/rc @@ -555,6 +555,9 @@ _mkfs_dev() _scratch_cleanup_files() { _scratch_mount + if ! [ "$SCRATCH_MNT" ]; then + _notrun "this test requires a \$SCRATCH_MNT to be specified" + fi rm -rf $SCRATCH_MNT/* _scratch_unmount } -- Thanks, Steve ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-15 5:35 ` Eryu Guan 0 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-11-15 5:35 UTC (permalink / raw) To: Steve French Cc: Dave Chinner, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > >> > This commit disables tests requires scratch dev running on NFS > >> > > >> > c041421 xfstests: stop special casing nfs and udf > >> > > >> > Now re-enable them to get a larger test coverage on NFS. > >> > > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> > >> > --- > >> > common/rc | 22 +++++++++++++++++++--- > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/common/rc b/common/rc > >> > index 747cf72..ae03712 100644 > >> > --- a/common/rc > >> > +++ b/common/rc > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > >> > } > >> > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > >> > +_scratch_cleanup_files() > >> > +{ > >> > + _scratch_mount > >> > + rm -rf $SCRATCH_MNT/* > >> > + _scratch_unmount > >> > +} > >> > >> There should be a check to make sure SCRATCH_MNT exists before you > >> wipe the whole disk .... > >> > >> so if no SCRATCH_MNT then this does rm -rf/* > >> right ... (and wipes out your whole system ...) > > > > You can't get to that function until after all the checks that > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > is only called in tests after all the startup checks validate > > devices and mounts exist. i.e. see common/config::get_next_config() > > Well, I reproduced it easily enough again today (after taking a > snapshot of the VM) > by simply running generic/120 against NFS with SCRATCH_MNT not > specified in local.config > Dros also ran into this problem. You're right, I missed that _scratch_mkfs is also called by ./check, and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is set, _scratch_mkfs can be dangerous. [root@hp-dl388eg8-01 xfstests]# ./check -nfs generic/001 FSTYP -- nfs PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc4+ MKFS_OPTIONS -- -bsize=4096 localhost:/mnt/nfsscratch MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 localhost:/mnt/nfsscratch our local _scratch_mkfs routine ... mount: can't find localhost:/mnt/nfsscratch in /etc/fstab rm -rf /* umount: localhost:/mnt/nfsscratch: mountpoint not found check: failed to mkfs $SCRATCH_DEV using specified options Passed all 0 tests (I replaced the real "rm -rf ..." by "echo 'rm -rf ...' in above test) > > The patch below fixes it for me but it wasn't immediately obvious how to best > return info to the user (ie print messages that make sense here - > "echo" seems to be supressed > in common/rc and notrun exits and logs it to a file but not to the > screen in this case) > > sfrench@ubuntu:~/xfstests$ git diff -a > diff --git a/common/rc b/common/rc > index d5e3aff..866244b 100644 > --- a/common/rc > +++ b/common/rc > @@ -555,6 +555,9 @@ _mkfs_dev() > _scratch_cleanup_files() > { > _scratch_mount > + if ! [ "$SCRATCH_MNT" ]; then > + _notrun "this test requires a \$SCRATCH_MNT to be specified" > + fi > rm -rf $SCRATCH_MNT/* > _scratch_unmount > } I propose this patch, which skips _scratch_cleanup_files if called by check [root@hp-dl388eg8-01 xfstests]# git diff diff --git a/common/rc b/common/rc index 435f74f..254fb66 100644 --- a/common/rc +++ b/common/rc @@ -554,6 +554,11 @@ _mkfs_dev() # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS _scratch_cleanup_files() { + # do nothing if called by check, variables are not fully valided yet + # SCRATCH_MNT can be empty, which is dangerous + if [ "$iam" == "check" ]; then + return + fi _scratch_mount rm -rf $SCRATCH_MNT/* _scratch_unmount Thanks, Eryu ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-15 5:35 ` Eryu Guan 0 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-11-15 5:35 UTC (permalink / raw) To: Steve French Cc: Dave Chinner, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > >> > This commit disables tests requires scratch dev running on NFS > >> > > >> > c041421 xfstests: stop special casing nfs and udf > >> > > >> > Now re-enable them to get a larger test coverage on NFS. > >> > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > >> > --- > >> > common/rc | 22 +++++++++++++++++++--- > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/common/rc b/common/rc > >> > index 747cf72..ae03712 100644 > >> > --- a/common/rc > >> > +++ b/common/rc > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > >> > } > >> > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > >> > +_scratch_cleanup_files() > >> > +{ > >> > + _scratch_mount > >> > + rm -rf $SCRATCH_MNT/* > >> > + _scratch_unmount > >> > +} > >> > >> There should be a check to make sure SCRATCH_MNT exists before you > >> wipe the whole disk .... > >> > >> so if no SCRATCH_MNT then this does rm -rf/* > >> right ... (and wipes out your whole system ...) > > > > You can't get to that function until after all the checks that > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > is only called in tests after all the startup checks validate > > devices and mounts exist. i.e. see common/config::get_next_config() > > Well, I reproduced it easily enough again today (after taking a > snapshot of the VM) > by simply running generic/120 against NFS with SCRATCH_MNT not > specified in local.config > Dros also ran into this problem. You're right, I missed that _scratch_mkfs is also called by ./check, and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is set, _scratch_mkfs can be dangerous. [root@hp-dl388eg8-01 xfstests]# ./check -nfs generic/001 FSTYP -- nfs PLATFORM -- Linux/x86_64 hp-dl388eg8-01 3.18.0-rc4+ MKFS_OPTIONS -- -bsize=4096 localhost:/mnt/nfsscratch MOUNT_OPTIONS -- -o context=system_u:object_r:nfs_t:s0 localhost:/mnt/nfsscratch our local _scratch_mkfs routine ... mount: can't find localhost:/mnt/nfsscratch in /etc/fstab rm -rf /* umount: localhost:/mnt/nfsscratch: mountpoint not found check: failed to mkfs $SCRATCH_DEV using specified options Passed all 0 tests (I replaced the real "rm -rf ..." by "echo 'rm -rf ...' in above test) > > The patch below fixes it for me but it wasn't immediately obvious how to best > return info to the user (ie print messages that make sense here - > "echo" seems to be supressed > in common/rc and notrun exits and logs it to a file but not to the > screen in this case) > > sfrench@ubuntu:~/xfstests$ git diff -a > diff --git a/common/rc b/common/rc > index d5e3aff..866244b 100644 > --- a/common/rc > +++ b/common/rc > @@ -555,6 +555,9 @@ _mkfs_dev() > _scratch_cleanup_files() > { > _scratch_mount > + if ! [ "$SCRATCH_MNT" ]; then > + _notrun "this test requires a \$SCRATCH_MNT to be specified" > + fi > rm -rf $SCRATCH_MNT/* > _scratch_unmount > } I propose this patch, which skips _scratch_cleanup_files if called by check [root@hp-dl388eg8-01 xfstests]# git diff diff --git a/common/rc b/common/rc index 435f74f..254fb66 100644 --- a/common/rc +++ b/common/rc @@ -554,6 +554,11 @@ _mkfs_dev() # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS _scratch_cleanup_files() { + # do nothing if called by check, variables are not fully valided yet + # SCRATCH_MNT can be empty, which is dangerous + if [ "$iam" == "check" ]; then + return + fi _scratch_mount rm -rf $SCRATCH_MNT/* _scratch_unmount Thanks, Eryu ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 5:41 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 5:41 UTC (permalink / raw) To: Eryu Guan Cc: Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > > >> > This commit disables tests requires scratch dev running on NFS > > >> > > > >> > c041421 xfstests: stop special casing nfs and udf > > >> > > > >> > Now re-enable them to get a larger test coverage on NFS. > > >> > > > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> > > >> > --- > > >> > common/rc | 22 +++++++++++++++++++--- > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > >> > > > >> > diff --git a/common/rc b/common/rc > > >> > index 747cf72..ae03712 100644 > > >> > --- a/common/rc > > >> > +++ b/common/rc > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > >> > } > > >> > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > >> > +_scratch_cleanup_files() > > >> > +{ > > >> > + _scratch_mount > > >> > + rm -rf $SCRATCH_MNT/* > > >> > + _scratch_unmount > > >> > +} > > >> > > >> There should be a check to make sure SCRATCH_MNT exists before you > > >> wipe the whole disk .... > > >> > > >> so if no SCRATCH_MNT then this does rm -rf/* > > >> right ... (and wipes out your whole system ...) > > > > > > You can't get to that function until after all the checks that > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > is only called in tests after all the startup checks validate > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > Well, I reproduced it easily enough again today (after taking a > > snapshot of the VM) > > by simply running generic/120 against NFS with SCRATCH_MNT not > > specified in local.config > > Dros also ran into this problem. > > You're right, I missed that _scratch_mkfs is also called by ./check, > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > set, _scratch_mkfs can be dangerous. As I asked earlier in this thread: why isn't get_next_config() catching this? > I propose this patch, which skips _scratch_cleanup_files if called by check > > [root@hp-dl388eg8-01 xfstests]# git diff > diff --git a/common/rc b/common/rc > index 435f74f..254fb66 100644 > --- a/common/rc > +++ b/common/rc > @@ -554,6 +554,11 @@ _mkfs_dev() > # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > _scratch_cleanup_files() > { > + # do nothing if called by check, variables are not fully valided yet > + # SCRATCH_MNT can be empty, which is dangerous > + if [ "$iam" == "check" ]; then > + return > + fi Again, this is the wrong place to try to fix this - we haven't fixed the landmine that has left us running with an invalid config. IOWs, by the time _scratch_mkfs is called from *anywhere* we should have fully validated the environment to be correct and valid. Parsing and validating the config we have loaded from the config file is the job of get_next_config(), yes? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 5:41 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 5:41 UTC (permalink / raw) To: Eryu Guan Cc: Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > >> > This commit disables tests requires scratch dev running on NFS > > >> > > > >> > c041421 xfstests: stop special casing nfs and udf > > >> > > > >> > Now re-enable them to get a larger test coverage on NFS. > > >> > > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > > >> > --- > > >> > common/rc | 22 +++++++++++++++++++--- > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > >> > > > >> > diff --git a/common/rc b/common/rc > > >> > index 747cf72..ae03712 100644 > > >> > --- a/common/rc > > >> > +++ b/common/rc > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > >> > } > > >> > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > >> > +_scratch_cleanup_files() > > >> > +{ > > >> > + _scratch_mount > > >> > + rm -rf $SCRATCH_MNT/* > > >> > + _scratch_unmount > > >> > +} > > >> > > >> There should be a check to make sure SCRATCH_MNT exists before you > > >> wipe the whole disk .... > > >> > > >> so if no SCRATCH_MNT then this does rm -rf/* > > >> right ... (and wipes out your whole system ...) > > > > > > You can't get to that function until after all the checks that > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > is only called in tests after all the startup checks validate > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > Well, I reproduced it easily enough again today (after taking a > > snapshot of the VM) > > by simply running generic/120 against NFS with SCRATCH_MNT not > > specified in local.config > > Dros also ran into this problem. > > You're right, I missed that _scratch_mkfs is also called by ./check, > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > set, _scratch_mkfs can be dangerous. As I asked earlier in this thread: why isn't get_next_config() catching this? > I propose this patch, which skips _scratch_cleanup_files if called by check > > [root@hp-dl388eg8-01 xfstests]# git diff > diff --git a/common/rc b/common/rc > index 435f74f..254fb66 100644 > --- a/common/rc > +++ b/common/rc > @@ -554,6 +554,11 @@ _mkfs_dev() > # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > _scratch_cleanup_files() > { > + # do nothing if called by check, variables are not fully valided yet > + # SCRATCH_MNT can be empty, which is dangerous > + if [ "$iam" == "check" ]; then > + return > + fi Again, this is the wrong place to try to fix this - we haven't fixed the landmine that has left us running with an invalid config. IOWs, by the time _scratch_mkfs is called from *anywhere* we should have fully validated the environment to be correct and valid. Parsing and validating the config we have loaded from the config file is the job of get_next_config(), yes? Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-11-17 5:41 ` Dave Chinner @ 2014-11-17 6:06 ` Eryu Guan -1 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-11-17 6:06 UTC (permalink / raw) To: Dave Chinner Cc: Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote: > On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > > > >> > This commit disables tests requires scratch dev running on NFS > > > >> > > > > >> > c041421 xfstests: stop special casing nfs and udf > > > >> > > > > >> > Now re-enable them to get a larger test coverage on NFS. > > > >> > > > > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> > > > >> > --- > > > >> > common/rc | 22 +++++++++++++++++++--- > > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > > >> > > > > >> > diff --git a/common/rc b/common/rc > > > >> > index 747cf72..ae03712 100644 > > > >> > --- a/common/rc > > > >> > +++ b/common/rc > > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > > >> > } > > > >> > > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > > >> > +_scratch_cleanup_files() > > > >> > +{ > > > >> > + _scratch_mount > > > >> > + rm -rf $SCRATCH_MNT/* > > > >> > + _scratch_unmount > > > >> > +} > > > >> > > > >> There should be a check to make sure SCRATCH_MNT exists before you > > > >> wipe the whole disk .... > > > >> > > > >> so if no SCRATCH_MNT then this does rm -rf/* > > > >> right ... (and wipes out your whole system ...) > > > > > > > > You can't get to that function until after all the checks that > > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > > is only called in tests after all the startup checks validate > > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > > > Well, I reproduced it easily enough again today (after taking a > > > snapshot of the VM) > > > by simply running generic/120 against NFS with SCRATCH_MNT not > > > specified in local.config > > > Dros also ran into this problem. > > > > You're right, I missed that _scratch_mkfs is also called by ./check, > > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > > set, _scratch_mkfs can be dangerous. > > As I asked earlier in this thread: why isn't get_next_config() > catching this? I think I see the problem, this patch catches the empty SCRATCH_MNT and works for me: eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff diff --git a/common/config b/common/config index 1cb08c0..409f1b8 100644 --- a/common/config +++ b/common/config @@ -464,7 +464,7 @@ get_next_config() { exit 1 fi - if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then + if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory" exit 1 fi If it looks sane I will send out a patch. > > > I propose this patch, which skips _scratch_cleanup_files if called by check > > > > [root@hp-dl388eg8-01 xfstests]# git diff > > diff --git a/common/rc b/common/rc > > index 435f74f..254fb66 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -554,6 +554,11 @@ _mkfs_dev() > > # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > _scratch_cleanup_files() > > { > > + # do nothing if called by check, variables are not fully valided yet > > + # SCRATCH_MNT can be empty, which is dangerous > > + if [ "$iam" == "check" ]; then > > + return > > + fi > > Again, this is the wrong place to try to fix this - we haven't fixed > the landmine that has left us running with an invalid config. IOWs, > by the time _scratch_mkfs is called from *anywhere* we should have > fully validated the environment to be correct and valid. Parsing and > validating the config we have loaded from the config file is the job > of get_next_config(), yes? Yes, that makes more sense, thanks for the explanation! Thanks, Eryu > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com > -- > 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 6:06 ` Eryu Guan 0 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-11-17 6:06 UTC (permalink / raw) To: Dave Chinner Cc: Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote: > On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > > >> > This commit disables tests requires scratch dev running on NFS > > > >> > > > > >> > c041421 xfstests: stop special casing nfs and udf > > > >> > > > > >> > Now re-enable them to get a larger test coverage on NFS. > > > >> > > > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > > > >> > --- > > > >> > common/rc | 22 +++++++++++++++++++--- > > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > > >> > > > > >> > diff --git a/common/rc b/common/rc > > > >> > index 747cf72..ae03712 100644 > > > >> > --- a/common/rc > > > >> > +++ b/common/rc > > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > > >> > } > > > >> > > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > > >> > +_scratch_cleanup_files() > > > >> > +{ > > > >> > + _scratch_mount > > > >> > + rm -rf $SCRATCH_MNT/* > > > >> > + _scratch_unmount > > > >> > +} > > > >> > > > >> There should be a check to make sure SCRATCH_MNT exists before you > > > >> wipe the whole disk .... > > > >> > > > >> so if no SCRATCH_MNT then this does rm -rf/* > > > >> right ... (and wipes out your whole system ...) > > > > > > > > You can't get to that function until after all the checks that > > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > > is only called in tests after all the startup checks validate > > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > > > Well, I reproduced it easily enough again today (after taking a > > > snapshot of the VM) > > > by simply running generic/120 against NFS with SCRATCH_MNT not > > > specified in local.config > > > Dros also ran into this problem. > > > > You're right, I missed that _scratch_mkfs is also called by ./check, > > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > > set, _scratch_mkfs can be dangerous. > > As I asked earlier in this thread: why isn't get_next_config() > catching this? I think I see the problem, this patch catches the empty SCRATCH_MNT and works for me: eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff diff --git a/common/config b/common/config index 1cb08c0..409f1b8 100644 --- a/common/config +++ b/common/config @@ -464,7 +464,7 @@ get_next_config() { exit 1 fi - if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then + if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then echo "common/config: Error: \$SCRATCH_MNT ($SCRATCH_MNT) is not a directory" exit 1 fi If it looks sane I will send out a patch. > > > I propose this patch, which skips _scratch_cleanup_files if called by check > > > > [root@hp-dl388eg8-01 xfstests]# git diff > > diff --git a/common/rc b/common/rc > > index 435f74f..254fb66 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -554,6 +554,11 @@ _mkfs_dev() > > # remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > _scratch_cleanup_files() > > { > > + # do nothing if called by check, variables are not fully valided yet > > + # SCRATCH_MNT can be empty, which is dangerous > > + if [ "$iam" == "check" ]; then > > + return > > + fi > > Again, this is the wrong place to try to fix this - we haven't fixed > the landmine that has left us running with an invalid config. IOWs, > by the time _scratch_mkfs is called from *anywhere* we should have > fully validated the environment to be correct and valid. Parsing and > validating the config we have loaded from the config file is the job > of get_next_config(), yes? Yes, that makes more sense, thanks for the explanation! Thanks, Eryu > > Cheers, > > Dave. > -- > Dave Chinner > david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 6:54 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 6:54 UTC (permalink / raw) To: Eryu Guan Cc: Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 02:06:03PM +0800, Eryu Guan wrote: > On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote: > > On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > > > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > > > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > > > > >> > This commit disables tests requires scratch dev running on NFS > > > > >> > > > > > >> > c041421 xfstests: stop special casing nfs and udf > > > > >> > > > > > >> > Now re-enable them to get a larger test coverage on NFS. > > > > >> > > > > > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> > > > > >> > --- > > > > >> > common/rc | 22 +++++++++++++++++++--- > > > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > >> > > > > > >> > diff --git a/common/rc b/common/rc > > > > >> > index 747cf72..ae03712 100644 > > > > >> > --- a/common/rc > > > > >> > +++ b/common/rc > > > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > > > >> > } > > > > >> > > > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > > > >> > +_scratch_cleanup_files() > > > > >> > +{ > > > > >> > + _scratch_mount > > > > >> > + rm -rf $SCRATCH_MNT/* > > > > >> > + _scratch_unmount > > > > >> > +} > > > > >> > > > > >> There should be a check to make sure SCRATCH_MNT exists before you > > > > >> wipe the whole disk .... > > > > >> > > > > >> so if no SCRATCH_MNT then this does rm -rf/* > > > > >> right ... (and wipes out your whole system ...) > > > > > > > > > > You can't get to that function until after all the checks that > > > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > > > is only called in tests after all the startup checks validate > > > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > > > > > Well, I reproduced it easily enough again today (after taking a > > > > snapshot of the VM) > > > > by simply running generic/120 against NFS with SCRATCH_MNT not > > > > specified in local.config > > > > Dros also ran into this problem. > > > > > > You're right, I missed that _scratch_mkfs is also called by ./check, > > > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > > > set, _scratch_mkfs can be dangerous. > > > > As I asked earlier in this thread: why isn't get_next_config() > > catching this? > > I think I see the problem, this patch catches the empty SCRATCH_MNT > and works for me: > > eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff > diff --git a/common/config b/common/config > index 1cb08c0..409f1b8 100644 > --- a/common/config > +++ b/common/config > @@ -464,7 +464,7 @@ get_next_config() { > exit 1 > fi > > - if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then > + if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then "if (not an empty string OR not a directory)" That doesn't work if you define $SCRATCH_MNT - the directory check fails on an empty string. As it is, this isn't solving the underlying problem - that "empty string" check is to avoid failing configs that are not configured with a scratch device. i.e. if you define SCRATCH_MNT, it must be a directory. However, there's no check that if you define SCRATCH_DEV you've also defined a SCRATCH_MNT... IOWs, the underlying problem here is that SCRATCH_DEV is defined, but SCRATCH_MNT is not and we are not catching that issue. FYI, the setup script is used for checking these sorts of things. e.g. with an empty config: $ sudo ./setup Warning: need to define parameters for host test2 or set variables: TEST_DIR TEST_DEV $ And you'll note that it catches when we only have one of the two necessary TEST_D* variables set. However, even with XFS as the probe fstyp, it doesn't error out if I don't define a SCRATCH_MNT: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./setup TEST: DIR=/mnt/test DEV=/dev/vda rt=[] log=[] TAPE: dev=[] rmt=[] rmtirix=[@] SCRATCH: MNT= DEV=/dev/vdb rt=[/dev/vdc] log=[/dev/vdd] VARIABLES: external=no largeblk=no fstyp=xfs large_scratch_dev=no attrsecure=no $ But it should fail with a sane error message indicating that we caught the invalid config and didn't leave check to fail randomly wiht some downstream error like: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./check generic/120 .... check: failed to mount $SCRATCH_DEV using specified options Passed all 0 tests $ Another FYI: we can actually test any filesystem without a scratch device configured: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 FSTYP -- xfs (non-debug) PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV Not run: generic/120 Passed all 0 tests $ So, really, all we need to do is ensure that SCRATCH_DEV/SCRATCH_MNT are sanely configured... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 6:54 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 6:54 UTC (permalink / raw) To: Eryu Guan Cc: Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 02:06:03PM +0800, Eryu Guan wrote: > On Mon, Nov 17, 2014 at 04:41:41PM +1100, Dave Chinner wrote: > > On Sat, Nov 15, 2014 at 01:35:33PM +0800, Eryu Guan wrote: > > > On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > > > > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > > > > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > > > > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > > > > >> > This commit disables tests requires scratch dev running on NFS > > > > >> > > > > > >> > c041421 xfstests: stop special casing nfs and udf > > > > >> > > > > > >> > Now re-enable them to get a larger test coverage on NFS. > > > > >> > > > > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > > > > >> > --- > > > > >> > common/rc | 22 +++++++++++++++++++--- > > > > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > > > > >> > > > > > >> > diff --git a/common/rc b/common/rc > > > > >> > index 747cf72..ae03712 100644 > > > > >> > --- a/common/rc > > > > >> > +++ b/common/rc > > > > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > > > > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > > > > >> > } > > > > >> > > > > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > > > > >> > +_scratch_cleanup_files() > > > > >> > +{ > > > > >> > + _scratch_mount > > > > >> > + rm -rf $SCRATCH_MNT/* > > > > >> > + _scratch_unmount > > > > >> > +} > > > > >> > > > > >> There should be a check to make sure SCRATCH_MNT exists before you > > > > >> wipe the whole disk .... > > > > >> > > > > >> so if no SCRATCH_MNT then this does rm -rf/* > > > > >> right ... (and wipes out your whole system ...) > > > > > > > > > > You can't get to that function until after all the checks that > > > > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > > > > is only called in tests after all the startup checks validate > > > > > devices and mounts exist. i.e. see common/config::get_next_config() > > > > > > > > Well, I reproduced it easily enough again today (after taking a > > > > snapshot of the VM) > > > > by simply running generic/120 against NFS with SCRATCH_MNT not > > > > specified in local.config > > > > Dros also ran into this problem. > > > > > > You're right, I missed that _scratch_mkfs is also called by ./check, > > > and if there's no SCRATCH_MNT set in local.config, only SCRATCH_DEV is > > > set, _scratch_mkfs can be dangerous. > > > > As I asked earlier in this thread: why isn't get_next_config() > > catching this? > > I think I see the problem, this patch catches the empty SCRATCH_MNT > and works for me: > > eguan@dhcp-13-216:~/workspace/src/xfstests$ git diff > diff --git a/common/config b/common/config > index 1cb08c0..409f1b8 100644 > --- a/common/config > +++ b/common/config > @@ -464,7 +464,7 @@ get_next_config() { > exit 1 > fi > > - if [ ! -z "$SCRATCH_MNT" -a ! -d "$SCRATCH_MNT" ]; then > + if [ ! -z "$SCRATCH_MNT" -o ! -d "$SCRATCH_MNT" ]; then "if (not an empty string OR not a directory)" That doesn't work if you define $SCRATCH_MNT - the directory check fails on an empty string. As it is, this isn't solving the underlying problem - that "empty string" check is to avoid failing configs that are not configured with a scratch device. i.e. if you define SCRATCH_MNT, it must be a directory. However, there's no check that if you define SCRATCH_DEV you've also defined a SCRATCH_MNT... IOWs, the underlying problem here is that SCRATCH_DEV is defined, but SCRATCH_MNT is not and we are not catching that issue. FYI, the setup script is used for checking these sorts of things. e.g. with an empty config: $ sudo ./setup Warning: need to define parameters for host test2 or set variables: TEST_DIR TEST_DEV $ And you'll note that it catches when we only have one of the two necessary TEST_D* variables set. However, even with XFS as the probe fstyp, it doesn't error out if I don't define a SCRATCH_MNT: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./setup TEST: DIR=/mnt/test DEV=/dev/vda rt=[] log=[] TAPE: dev=[] rmt=[] rmtirix=[@] SCRATCH: MNT= DEV=/dev/vdb rt=[/dev/vdc] log=[/dev/vdd] VARIABLES: external=no largeblk=no fstyp=xfs large_scratch_dev=no attrsecure=no $ But it should fail with a sane error message indicating that we caught the invalid config and didn't leave check to fail randomly wiht some downstream error like: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test SCRATCH_DEV=/dev/vdb ./check generic/120 .... check: failed to mount $SCRATCH_DEV using specified options Passed all 0 tests $ Another FYI: we can actually test any filesystem without a scratch device configured: $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 FSTYP -- xfs (non-debug) PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV Not run: generic/120 Passed all 0 tests $ So, really, all we need to do is ensure that SCRATCH_DEV/SCRATCH_MNT are sanely configured... Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-11-17 6:54 ` Dave Chinner @ 2014-11-17 14:53 ` Omer Zilberberg -1 siblings, 0 replies; 31+ messages in thread From: Omer Zilberberg @ 2014-11-17 14:53 UTC (permalink / raw) To: Dave Chinner, Eryu Guan Cc: Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson > Another FYI: we can actually test any filesystem without a scratch > device configured: > > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 > FSTYP -- xfs (non-debug) > PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ > > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV > Not run: generic/120 > Passed all 0 tests > $ Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 FSTYP -- ext4 PLATFORM -- Linux/x86_64 testvm 3.17.0 generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 @@ -1,4 +1,5 @@ QA output created by 001 +Usage: _is_block_dev dev cleanup setup .................................... iter 1 chain ... check .................................... ... (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) Ran: generic/001 Failures: generic/001 Failed 1 of 1 tests Here's a patch to fix that: ---- Subject: [PATCH] _required_test: removed unneeded test for scratch_dev testing for scratch_dev in _required_test is unnecessary, since it is not required for these tests. Furthermore, if a scratch_dev is not given, all tests which are supposed to pass on test_dev fail, because _is_block_dev is given an empty string as scratch_dev, and prints usage. Signed-off-by: Omer Zilberberg <omzg@plexistor.com> --- common/rc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/rc b/common/rc index d5e3aff..b863808 100644 --- a/common/rc +++ b/common/rc @@ -1104,7 +1104,7 @@ _require_scratch() } -# this test needs a test partition - check we're ok & unmount it +# this test needs a test partition - check we're ok & mount if necessary # _require_test() { @@ -1138,10 +1138,6 @@ _require_test() then _notrun "this test requires a valid \$TEST_DEV" fi - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] - then - _notrun "this test requires a valid \$TEST_DEV" - fi if [ ! -d "$TEST_DIR" ] then _notrun "this test requires a valid \$TEST_DIR" -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 14:53 ` Omer Zilberberg 0 siblings, 0 replies; 31+ messages in thread From: Omer Zilberberg @ 2014-11-17 14:53 UTC (permalink / raw) To: Dave Chinner, Eryu Guan Cc: Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson > Another FYI: we can actually test any filesystem without a scratch > device configured: > > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 > FSTYP -- xfs (non-debug) > PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ > > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV > Not run: generic/120 > Passed all 0 tests > $ Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 FSTYP -- ext4 PLATFORM -- Linux/x86_64 testvm 3.17.0 generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 @@ -1,4 +1,5 @@ QA output created by 001 +Usage: _is_block_dev dev cleanup setup .................................... iter 1 chain ... check .................................... ... (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) Ran: generic/001 Failures: generic/001 Failed 1 of 1 tests Here's a patch to fix that: ---- Subject: [PATCH] _required_test: removed unneeded test for scratch_dev testing for scratch_dev in _required_test is unnecessary, since it is not required for these tests. Furthermore, if a scratch_dev is not given, all tests which are supposed to pass on test_dev fail, because _is_block_dev is given an empty string as scratch_dev, and prints usage. Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org> --- common/rc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/rc b/common/rc index d5e3aff..b863808 100644 --- a/common/rc +++ b/common/rc @@ -1104,7 +1104,7 @@ _require_scratch() } -# this test needs a test partition - check we're ok & unmount it +# this test needs a test partition - check we're ok & mount if necessary # _require_test() { @@ -1138,10 +1138,6 @@ _require_test() then _notrun "this test requires a valid \$TEST_DEV" fi - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] - then - _notrun "this test requires a valid \$TEST_DEV" - fi if [ ! -d "$TEST_DIR" ] then _notrun "this test requires a valid \$TEST_DIR" -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 20:22 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 20:22 UTC (permalink / raw) To: Omer Zilberberg Cc: Eryu Guan, Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote: > > Another FYI: we can actually test any filesystem without a scratch > > device configured: > > > > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 > > FSTYP -- xfs (non-debug) > > PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ > > > > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV > > Not run: generic/120 > > Passed all 0 tests > > $ > > Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: > $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 testvm 3.17.0 > > generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) > --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 > +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 > @@ -1,4 +1,5 @@ > QA output created by 001 > +Usage: _is_block_dev dev > cleanup > setup .................................... > iter 1 chain ... check .................................... > ... > (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) > Ran: generic/001 > Failures: generic/001 > Failed 1 of 1 tests > > Here's a patch to fix that: > ---- > Subject: [PATCH] _required_test: removed unneeded test for scratch_dev > > testing for scratch_dev in _required_test is unnecessary, since it is > not required for these tests. Furthermore, if a scratch_dev is not given, > all tests which are supposed to pass on test_dev fail, because > _is_block_dev is given an empty string as scratch_dev, and prints usage. I'm beginning to sound like a broken record. Why isn't this *invalid config check* being done in the config parsing rather than on every test that requires a scratch or test device? i.e. removing the check is not the correct fix - checking it during config parsing means none of these checks need to be done in _require_test, nor in _require_scratch_nocheck. i.e. in get_next_config() we already know the FSTYP, so we should be checking that: a) TEST_DEV is valid for fstyp b) SCRATCH_DEV (if configured) is valid for fstyp c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp indicates they should be block devices And then we can pretty much assume that they are valid everywhere else, and _require_scratch_nocheck() simply needs to check for a non-empty string... > Signed-off-by: Omer Zilberberg <omzg@plexistor.com> > --- > common/rc | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/common/rc b/common/rc > index d5e3aff..b863808 100644 > --- a/common/rc > +++ b/common/rc > @@ -1104,7 +1104,7 @@ _require_scratch() > } > > > -# this test needs a test partition - check we're ok & unmount it > +# this test needs a test partition - check we're ok & mount if necessary > # > _require_test() > { > @@ -1138,10 +1138,6 @@ _require_test() > then > _notrun "this test requires a valid \$TEST_DEV" > fi > - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] > - then > - _notrun "this test requires a valid \$TEST_DEV" > - fi > if [ ! -d "$TEST_DIR" ] > then > _notrun "this test requires a valid \$TEST_DIR" The patch is whitespace damaged. Please read: https://www.kernel.org/doc/Documentation/email-clients.txt and resend. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 20:22 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 20:22 UTC (permalink / raw) To: Omer Zilberberg Cc: Eryu Guan, Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote: > > Another FYI: we can actually test any filesystem without a scratch > > device configured: > > > > $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 > > FSTYP -- xfs (non-debug) > > PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ > > > > generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV > > Not run: generic/120 > > Passed all 0 tests > > $ > > Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: > $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 > FSTYP -- ext4 > PLATFORM -- Linux/x86_64 testvm 3.17.0 > > generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) > --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 > +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 > @@ -1,4 +1,5 @@ > QA output created by 001 > +Usage: _is_block_dev dev > cleanup > setup .................................... > iter 1 chain ... check .................................... > ... > (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) > Ran: generic/001 > Failures: generic/001 > Failed 1 of 1 tests > > Here's a patch to fix that: > ---- > Subject: [PATCH] _required_test: removed unneeded test for scratch_dev > > testing for scratch_dev in _required_test is unnecessary, since it is > not required for these tests. Furthermore, if a scratch_dev is not given, > all tests which are supposed to pass on test_dev fail, because > _is_block_dev is given an empty string as scratch_dev, and prints usage. I'm beginning to sound like a broken record. Why isn't this *invalid config check* being done in the config parsing rather than on every test that requires a scratch or test device? i.e. removing the check is not the correct fix - checking it during config parsing means none of these checks need to be done in _require_test, nor in _require_scratch_nocheck. i.e. in get_next_config() we already know the FSTYP, so we should be checking that: a) TEST_DEV is valid for fstyp b) SCRATCH_DEV (if configured) is valid for fstyp c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp indicates they should be block devices And then we can pretty much assume that they are valid everywhere else, and _require_scratch_nocheck() simply needs to check for a non-empty string... > Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org> > --- > common/rc | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/common/rc b/common/rc > index d5e3aff..b863808 100644 > --- a/common/rc > +++ b/common/rc > @@ -1104,7 +1104,7 @@ _require_scratch() > } > > > -# this test needs a test partition - check we're ok & unmount it > +# this test needs a test partition - check we're ok & mount if necessary > # > _require_test() > { > @@ -1138,10 +1138,6 @@ _require_test() > then > _notrun "this test requires a valid \$TEST_DEV" > fi > - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] > - then > - _notrun "this test requires a valid \$TEST_DEV" > - fi > if [ ! -d "$TEST_DIR" ] > then > _notrun "this test requires a valid \$TEST_DIR" The patch is whitespace damaged. Please read: https://www.kernel.org/doc/Documentation/email-clients.txt and resend. Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS 2014-11-17 20:22 ` Dave Chinner @ 2014-11-18 16:16 ` Omer Zilberberg -1 siblings, 0 replies; 31+ messages in thread From: Omer Zilberberg @ 2014-11-18 16:16 UTC (permalink / raw) To: Dave Chinner Cc: Eryu Guan, Steve French, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On 11/17/2014 10:22 PM, Dave Chinner wrote: > On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote: >>> Another FYI: we can actually test any filesystem without a scratch >>> device configured: >>> >>> $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 >>> FSTYP -- xfs (non-debug) >>> PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ >>> >>> generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV >>> Not run: generic/120 >>> Passed all 0 tests >>> $ >> >> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: >> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 >> FSTYP -- ext4 >> PLATFORM -- Linux/x86_64 testvm 3.17.0 >> >> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) >> --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 >> +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 >> @@ -1,4 +1,5 @@ >> QA output created by 001 >> +Usage: _is_block_dev dev >> cleanup >> setup .................................... >> iter 1 chain ... check .................................... >> ... >> (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) >> Ran: generic/001 >> Failures: generic/001 >> Failed 1 of 1 tests >> >> Here's a patch to fix that: >> ---- >> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev >> >> testing for scratch_dev in _required_test is unnecessary, since it is >> not required for these tests. Furthermore, if a scratch_dev is not given, >> all tests which are supposed to pass on test_dev fail, because >> _is_block_dev is given an empty string as scratch_dev, and prints usage. > > I'm beginning to sound like a broken record. Why isn't this *invalid > config check* being done in the config parsing rather than on every > test that requires a scratch or test device? > > i.e. removing the check is not the correct fix - checking it during > config parsing means none of these checks need to be done in > _require_test, nor in _require_scratch_nocheck. > > i.e. in get_next_config() we already know the FSTYP, so we should > be checking that: > > a) TEST_DEV is valid for fstyp > b) SCRATCH_DEV (if configured) is valid for fstyp > c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp > indicates they should be block devices > > And then we can pretty much assume that they are valid everywhere > else, and _require_scratch_nocheck() simply needs to check for a > non-empty string... > >> Signed-off-by: Omer Zilberberg <omzg@plexistor.com> >> --- >> common/rc | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index d5e3aff..b863808 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -1104,7 +1104,7 @@ _require_scratch() >> } >> >> >> -# this test needs a test partition - check we're ok & unmount it >> +# this test needs a test partition - check we're ok & mount if necessary >> # >> _require_test() >> { >> @@ -1138,10 +1138,6 @@ _require_test() >> then >> _notrun "this test requires a valid \$TEST_DEV" >> fi >> - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] >> - then >> - _notrun "this test requires a valid \$TEST_DEV" >> - fi >> if [ ! -d "$TEST_DIR" ] >> then >> _notrun "this test requires a valid \$TEST_DIR" > > The patch is whitespace damaged. Please read: > > https://www.kernel.org/doc/Documentation/email-clients.txt Here's the patch resent with whitespace fixed, at least as a temporary solution... ---- Subject: [PATCH] _required_test: removed unneeded test for scratch_dev testing for scratch_dev in _required_test is unnecessary, since it is not required for these tests. Furthermore, if a scratch_dev is not given, all tests which are supposed to pass on test_dev fail, because _is_block_dev is given an empty string as scratch_dev, and prints usage. Signed-off-by: Omer Zilberberg <omzg@plexistor.com> --- common/rc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/rc b/common/rc index d5e3aff..b863808 100644 --- a/common/rc +++ b/common/rc @@ -1104,7 +1104,7 @@ _require_scratch() } -# this test needs a test partition - check we're ok & unmount it +# this test needs a test partition - check we're ok & mount if necessary # _require_test() { @@ -1138,10 +1138,6 @@ _require_test() then _notrun "this test requires a valid \$TEST_DEV" fi - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] - then - _notrun "this test requires a valid \$TEST_DEV" - fi if [ ! -d "$TEST_DIR" ] then _notrun "this test requires a valid \$TEST_DIR" ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-18 16:16 ` Omer Zilberberg 0 siblings, 0 replies; 31+ messages in thread From: Omer Zilberberg @ 2014-11-18 16:16 UTC (permalink / raw) To: Dave Chinner Cc: Eryu Guan, Steve French, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On 11/17/2014 10:22 PM, Dave Chinner wrote: > On Mon, Nov 17, 2014 at 04:53:50PM +0200, Omer Zilberberg wrote: >>> Another FYI: we can actually test any filesystem without a scratch >>> device configured: >>> >>> $ sudo TEST_DEV=/dev/vda TEST_DIR=/mnt/test ./check generic/120 >>> FSTYP -- xfs (non-debug) >>> PLATFORM -- Linux/x86_64 test2 3.18.0-rc2-dgc+ >>> >>> generic/120 16s ... [not run] this test requires a valid $SCRATCH_DEV >>> Not run: generic/120 >>> Passed all 0 tests >>> $ >> >> Please note that since commit 83ef157d, that is no longer true, because _require_test calls _is_block_dev with empty parameter, which prints usage: >> $ sudo TEST_DEV=/dev/sda TEST_DIR=/mnt/dev0 ./check generic/001 >> FSTYP -- ext4 >> PLATFORM -- Linux/x86_64 testvm 3.17.0 >> >> generic/001 7s ... - output mismatch (see /opt/xfstests/results//generic/001.out.bad) >> --- tests/generic/001.out 2014-09-10 11:04:44.249185592 +0300 >> +++ /opt/xfstests/results//generic/001.out.bad 2014-11-17 15:14:12.380061760 +0200 >> @@ -1,4 +1,5 @@ >> QA output created by 001 >> +Usage: _is_block_dev dev >> cleanup >> setup .................................... >> iter 1 chain ... check .................................... >> ... >> (Run 'diff -u tests/generic/001.out /opt/xfstests/results//generic/001.out.bad' to see the entire diff) >> Ran: generic/001 >> Failures: generic/001 >> Failed 1 of 1 tests >> >> Here's a patch to fix that: >> ---- >> Subject: [PATCH] _required_test: removed unneeded test for scratch_dev >> >> testing for scratch_dev in _required_test is unnecessary, since it is >> not required for these tests. Furthermore, if a scratch_dev is not given, >> all tests which are supposed to pass on test_dev fail, because >> _is_block_dev is given an empty string as scratch_dev, and prints usage. > > I'm beginning to sound like a broken record. Why isn't this *invalid > config check* being done in the config parsing rather than on every > test that requires a scratch or test device? > > i.e. removing the check is not the correct fix - checking it during > config parsing means none of these checks need to be done in > _require_test, nor in _require_scratch_nocheck. > > i.e. in get_next_config() we already know the FSTYP, so we should > be checking that: > > a) TEST_DEV is valid for fstyp > b) SCRATCH_DEV (if configured) is valid for fstyp > c) TEST_DEV != SCRATCH_DEV if they both exist and fstyp > indicates they should be block devices > > And then we can pretty much assume that they are valid everywhere > else, and _require_scratch_nocheck() simply needs to check for a > non-empty string... > >> Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org> >> --- >> common/rc | 6 +----- >> 1 file changed, 1 insertion(+), 5 deletions(-) >> >> diff --git a/common/rc b/common/rc >> index d5e3aff..b863808 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -1104,7 +1104,7 @@ _require_scratch() >> } >> >> >> -# this test needs a test partition - check we're ok & unmount it >> +# this test needs a test partition - check we're ok & mount if necessary >> # >> _require_test() >> { >> @@ -1138,10 +1138,6 @@ _require_test() >> then >> _notrun "this test requires a valid \$TEST_DEV" >> fi >> - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] >> - then >> - _notrun "this test requires a valid \$TEST_DEV" >> - fi >> if [ ! -d "$TEST_DIR" ] >> then >> _notrun "this test requires a valid \$TEST_DIR" > > The patch is whitespace damaged. Please read: > > https://www.kernel.org/doc/Documentation/email-clients.txt Here's the patch resent with whitespace fixed, at least as a temporary solution... ---- Subject: [PATCH] _required_test: removed unneeded test for scratch_dev testing for scratch_dev in _required_test is unnecessary, since it is not required for these tests. Furthermore, if a scratch_dev is not given, all tests which are supposed to pass on test_dev fail, because _is_block_dev is given an empty string as scratch_dev, and prints usage. Signed-off-by: Omer Zilberberg <omzg-/8YdC2HfS5554TAoqtyWWQ@public.gmane.org> --- common/rc | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/rc b/common/rc index d5e3aff..b863808 100644 --- a/common/rc +++ b/common/rc @@ -1104,7 +1104,7 @@ _require_scratch() } -# this test needs a test partition - check we're ok & unmount it +# this test needs a test partition - check we're ok & mount if necessary # _require_test() { @@ -1138,10 +1138,6 @@ _require_test() then _notrun "this test requires a valid \$TEST_DEV" fi - if [ "`_is_block_dev $SCRATCH_DEV`" = "`_is_block_dev $TEST_DEV`" ] - then - _notrun "this test requires a valid \$TEST_DEV" - fi if [ ! -d "$TEST_DIR" ] then _notrun "this test requires a valid \$TEST_DIR" -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 5:34 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 5:34 UTC (permalink / raw) To: Steve French Cc: Eryu Guan, fstests, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org, Weston Andros Adamson On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david@fromorbit.com> wrote: > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan@redhat.com> wrote: > >> > This commit disables tests requires scratch dev running on NFS > >> > > >> > c041421 xfstests: stop special casing nfs and udf > >> > > >> > Now re-enable them to get a larger test coverage on NFS. > >> > > >> > Signed-off-by: Eryu Guan <eguan@redhat.com> > >> > --- > >> > common/rc | 22 +++++++++++++++++++--- > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/common/rc b/common/rc > >> > index 747cf72..ae03712 100644 > >> > --- a/common/rc > >> > +++ b/common/rc > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > >> > } > >> > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > >> > +_scratch_cleanup_files() > >> > +{ > >> > + _scratch_mount > >> > + rm -rf $SCRATCH_MNT/* > >> > + _scratch_unmount > >> > +} > >> > >> There should be a check to make sure SCRATCH_MNT exists before you > >> wipe the whole disk .... > >> > >> so if no SCRATCH_MNT then this does rm -rf/* > >> right ... (and wipes out your whole system ...) > > > > You can't get to that function until after all the checks that > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > is only called in tests after all the startup checks validate > > devices and mounts exist. i.e. see common/config::get_next_config() > > Well, I reproduced it easily enough again today (after taking a > snapshot of the VM) > by simply running generic/120 against NFS with SCRATCH_MNT not > specified in local.config > Dros also ran into this problem. tests/generic/120 does this: .... _require_scratch _scratch_mkfs .... So we call: _require_scratch _require_scratch_nocheck case "$FSTYP" in nfs*) echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1 if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then _notrun "this test requires a valid \$SCRATCH_DEV" fi if [ ! -d "$SCRATCH_MNT" ]; then _notrun "this test requires a valid \$SCRATCH_MNT" fi If $SCRATCH_MNT is empty, then it should fail right there. An empty string gives: $ if [ ! -d "" ]; then > echo foo > fi foo $ before it gets anywhere near _scratch_mkfs. So, why isn't generic/120 aborting during the _require_scratch check? Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH v2 1/5] common: re-enable tests that require scratch dev on NFS @ 2014-11-17 5:34 ` Dave Chinner 0 siblings, 0 replies; 31+ messages in thread From: Dave Chinner @ 2014-11-17 5:34 UTC (permalink / raw) To: Steve French Cc: Eryu Guan, fstests-u79uwXL29TY76Z2rM5mHXA, linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Weston Andros Adamson On Fri, Nov 14, 2014 at 11:02:50AM -0600, Steve French wrote: > On Wed, Nov 12, 2014 at 9:33 PM, Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> wrote: > > On Wed, Nov 12, 2014 at 12:36:13PM -0600, Steve French wrote: > >> On Fri, Oct 31, 2014 at 12:03 PM, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote: > >> > This commit disables tests requires scratch dev running on NFS > >> > > >> > c041421 xfstests: stop special casing nfs and udf > >> > > >> > Now re-enable them to get a larger test coverage on NFS. > >> > > >> > Signed-off-by: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> > >> > --- > >> > common/rc | 22 +++++++++++++++++++--- > >> > 1 file changed, 19 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/common/rc b/common/rc > >> > index 747cf72..ae03712 100644 > >> > --- a/common/rc > >> > +++ b/common/rc > >> > @@ -551,6 +551,14 @@ _mkfs_dev() > >> > rm -f $tmp_dir.mkfserr $tmp_dir.mkfsstd > >> > } > >> > > >> > +# remove all files in $SCRATCH_MNT, useful when testing on NFS/CIFS > >> > +_scratch_cleanup_files() > >> > +{ > >> > + _scratch_mount > >> > + rm -rf $SCRATCH_MNT/* > >> > + _scratch_unmount > >> > +} > >> > >> There should be a check to make sure SCRATCH_MNT exists before you > >> wipe the whole disk .... > >> > >> so if no SCRATCH_MNT then this does rm -rf/* > >> right ... (and wipes out your whole system ...) > > > > You can't get to that function until after all the checks that > > SCRATCH_MNT exists. i.e. this happens during _scratch_mkfs, and that > > is only called in tests after all the startup checks validate > > devices and mounts exist. i.e. see common/config::get_next_config() > > Well, I reproduced it easily enough again today (after taking a > snapshot of the VM) > by simply running generic/120 against NFS with SCRATCH_MNT not > specified in local.config > Dros also ran into this problem. tests/generic/120 does this: .... _require_scratch _scratch_mkfs .... So we call: _require_scratch _require_scratch_nocheck case "$FSTYP" in nfs*) echo $SCRATCH_DEV | grep -q ":/" > /dev/null 2>&1 if [ -z "$SCRATCH_DEV" -o "$?" != "0" ]; then _notrun "this test requires a valid \$SCRATCH_DEV" fi if [ ! -d "$SCRATCH_MNT" ]; then _notrun "this test requires a valid \$SCRATCH_MNT" fi If $SCRATCH_MNT is empty, then it should fail right there. An empty string gives: $ if [ ! -d "" ]; then > echo foo > fi foo $ before it gets anywhere near _scratch_mkfs. So, why isn't generic/120 aborting during the _require_scratch check? Cheers, Dave. -- Dave Chinner david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v2 2/5] common: add _require_block_device() helper 2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan 2014-10-31 17:03 ` [PATCH v2 1/5] common: " Eryu Guan @ 2014-10-31 17:03 ` Eryu Guan 2014-10-31 17:03 ` [PATCH v2 3/5] common: skip atime related tests on NFS Eryu Guan ` (3 subsequent siblings) 5 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-10-31 17:03 UTC (permalink / raw) To: fstests; +Cc: linux-nfs, Eryu Guan Add _require_block_device() helper and use it in _require_dm_flakey() and generic/076. _require_dm_flakey() assumes $SCRATCH_DEV is a block device, now it can also be a NFS export. generic/076 does "cat $SCRATCH_DEV" which will fail when testing on NFS. Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Eryu Guan <eguan@redhat.com> --- common/rc | 15 +++++++++++++++ tests/generic/076 | 1 + 2 files changed, 16 insertions(+) diff --git a/common/rc b/common/rc index ae03712..ca8c4a2 100644 --- a/common/rc +++ b/common/rc @@ -1243,10 +1243,25 @@ _require_command() [ -n "$1" -a -x "$1" ] || _notrun "$_cmd utility required, skipped this test" } +# this test requires the device to be valid block device +# $1 - device +_require_block_device() +{ + if [ -z "$1" ]; then + echo "Usage: _require_block_device <dev>" 1>&2 + exit 1 + fi + if [ "`_is_block_dev $SCRATCH_DEV`" == "" ]; then + _notrun "require $1 to be valid block disk" + fi +} + # this test requires the device mapper flakey target # _require_dm_flakey() { + # require SCRATCH_DEV to be a valid block device + _require_block_device $SCRATCH_DEV _require_command $DMSETUP_PROG modprobe dm-flakey >/dev/null 2>&1 diff --git a/tests/generic/076 b/tests/generic/076 index 02af762..aa0aae0 100755 --- a/tests/generic/076 +++ b/tests/generic/076 @@ -56,6 +56,7 @@ _supported_fs generic _supported_os IRIX Linux _require_scratch +_require_block_device $SCRATCH_DEV echo "*** init fs" -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 3/5] common: skip atime related tests on NFS 2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan 2014-10-31 17:03 ` [PATCH v2 1/5] common: " Eryu Guan 2014-10-31 17:03 ` [PATCH v2 2/5] common: add _require_block_device() helper Eryu Guan @ 2014-10-31 17:03 ` Eryu Guan 2014-10-31 17:03 ` [PATCH v2 4/5] common: use _scratch_mount helper in _require_relatime() Eryu Guan ` (2 subsequent siblings) 5 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-10-31 17:03 UTC (permalink / raw) To: fstests; +Cc: linux-nfs, Eryu Guan >From nfs(5) we can know that atime related mount options have no effect on NFS mounts, so add _require_atime() helper to skip atime tests on NFS Signed-off-by: Eryu Guan <eguan@redhat.com> --- common/rc | 7 +++++++ tests/generic/003 | 1 + tests/generic/192 | 1 + 3 files changed, 9 insertions(+) diff --git a/common/rc b/common/rc index ca8c4a2..2d2b40f 100644 --- a/common/rc +++ b/common/rc @@ -2385,6 +2385,13 @@ _verify_reflink() || echo "$1 and $2 are not reflinks: different extents" } +_require_atime() +{ + if [ "$FSTYP" == "nfs" ]; then + _notrun "atime related mount options have no effect on NFS" + fi +} + _require_relatime() { _scratch_mkfs > /dev/null 2>&1 diff --git a/tests/generic/003 b/tests/generic/003 index 83d6f90..7ffd09a 100755 --- a/tests/generic/003 +++ b/tests/generic/003 @@ -47,6 +47,7 @@ _cleanup() _supported_fs generic _supported_os Linux _require_scratch +_require_atime _require_relatime rm -f $seqres.full diff --git a/tests/generic/192 b/tests/generic/192 index b2da358..5b6cfbc 100755 --- a/tests/generic/192 +++ b/tests/generic/192 @@ -54,6 +54,7 @@ is_noatime_set() { _supported_fs generic _supported_os Linux _require_test +_require_atime #delay=150 #delay=75 #delay=60 -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 4/5] common: use _scratch_mount helper in _require_relatime() 2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan ` (2 preceding siblings ...) 2014-10-31 17:03 ` [PATCH v2 3/5] common: skip atime related tests on NFS Eryu Guan @ 2014-10-31 17:03 ` Eryu Guan 2014-10-31 17:04 ` [PATCH v2 5/5] generic/277: add _require_attrs Eryu Guan [not found] ` <1414775040-4051-1-git-send-email-eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 5 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-10-31 17:03 UTC (permalink / raw) To: fstests; +Cc: linux-nfs, Eryu Guan Change the way how _require_relatime() mount $SCRATCH_DEV, use _scratch_mount helper so $SCRATCH_DEV is mounted with selinux context, to avoid "same superblock, different selinux context" failure. Signed-off-by: Eryu Guan <eguan@redhat.com> --- common/rc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/rc b/common/rc index 2d2b40f..b495ec1 100644 --- a/common/rc +++ b/common/rc @@ -2395,7 +2395,7 @@ _require_atime() _require_relatime() { _scratch_mkfs > /dev/null 2>&1 - _mount -t $FSTYP -o relatime $SCRATCH_DEV $SCRATCH_MNT || \ + _scratch_mount -o relatime || \ _notrun "relatime not supported by the current kernel" _scratch_unmount } -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH v2 5/5] generic/277: add _require_attrs 2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan ` (3 preceding siblings ...) 2014-10-31 17:03 ` [PATCH v2 4/5] common: use _scratch_mount helper in _require_relatime() Eryu Guan @ 2014-10-31 17:04 ` Eryu Guan [not found] ` <1414775040-4051-1-git-send-email-eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> 5 siblings, 0 replies; 31+ messages in thread From: Eryu Guan @ 2014-10-31 17:04 UTC (permalink / raw) To: fstests; +Cc: linux-nfs, Eryu Guan NFS doesn't support attr yet, add _require_attrs in generic/277 to avoid failure when testing on NFS. Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Eryu Guan <eguan@redhat.com> --- tests/generic/277 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/generic/277 b/tests/generic/277 index 8461ad9..39ebdc3 100755 --- a/tests/generic/277 +++ b/tests/generic/277 @@ -38,11 +38,13 @@ trap "_cleanup ; exit \$status" 0 1 2 3 15 # get standard environment, filters and checks . ./common/rc . ./common/filter +. ./common/attr # real QA test starts here _supported_fs generic _supported_os Linux _require_scratch +_require_attrs _scratch_mkfs > /dev/null 2>&1 _scratch_mount -- 1.9.3 ^ permalink raw reply related [flat|nested] 31+ messages in thread
[parent not found: <1414775040-4051-1-git-send-email-eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]
* Fwd: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS [not found] ` <1414775040-4051-1-git-send-email-eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> @ 2014-11-02 20:52 ` Steve French 0 siblings, 0 replies; 31+ messages in thread From: Steve French @ 2014-11-02 20:52 UTC (permalink / raw) To: Pavel Shilovsky, linux-cifs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Has anyone tried these newly enabled xfstests on cifs? ---------- Forwarded message ---------- From: Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Date: Fri, Oct 31, 2014 at 12:03 PM Subject: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS To: fstests-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Eryu Guan <eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> The commit below disables tests requires scratch dev running on NFS c041421 xfstests: stop special casing nfs and udf Now re-enable them to get a larger test coverage on NFS. Also do more updates to avoid unnecessary failures on NFS. I tested against NFSv3 NFSv4.0 NFSv4.1 (both server and client are linux running 3.18-rc1 kernel), the results look good. Failures on NFSv3: generic/035 generic/089 generic/258 generic/294 Failures on NFSv4.0: generic/035 generic/169 generic/294 Failures on NFSv4.1: I hit kernel BUG_ON when testing on NFSv4.1 in generic/285 SEEK_DATA/SEEK_HOLE test. I think there's already a patch to fix it. http://www.spinics.net/lists/linux-nfs/msg47359.html Note that generic/294 does remount,ro on SCRATCH_DEV, but TEST_DEV is affected too, so some tests after generic/294 fail because of EROFS. Run the failed tests separately and they all passed. I'm not sure if it's a bug, but I filed one, please see https://bugzilla.redhat.com/show_bug.cgi?id=1158046 The third patch disables all atime related tests on NFS, and Christoph starts a discussion about NFS atime handling issue in v1 thread. Before there's a conclusion I keep the patch as it is, and we can update it later when we decide to test atime on NFS again. v2: - introduce _scratch_cleanup_files helper to remove all files on $SCRATCH_MNT, for later CIFS use. (Christoph Hellwig) - split _require_relatime change into another patch (Christoph Hellwig) v1: http://www.spinics.net/lists/linux-nfs/msg47423.html Thanks, Eryu --- Eryu Guan (5): common: re-enable tests that require scratch dev on NFS common: add _require_block_device() helper common: skip atime related tests on NFS common: use _scratch_mount helper in _require_relatime() generic/277: add _require_attrs common/rc | 46 ++++++++++++++++++++++++++++++++++++++++++---- tests/generic/003 | 1 + tests/generic/076 | 1 + tests/generic/192 | 1 + tests/generic/277 | 2 ++ 5 files changed, 47 insertions(+), 4 deletions(-) -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Thanks, Steve ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2014-11-18 16:16 UTC | newest]
Thread overview: 31+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-10-31 17:03 [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Eryu Guan
2014-10-31 17:03 ` [PATCH v2 1/5] common: " Eryu Guan
2014-11-10 2:12 ` Dave Chinner
2014-11-10 4:05 ` Eryu Guan
2014-11-12 18:36 ` Steve French
2014-11-12 18:36 ` Steve French
2014-11-13 3:33 ` Dave Chinner
2014-11-13 3:33 ` Dave Chinner
2014-11-14 17:02 ` Steve French
2014-11-14 17:02 ` Steve French
2014-11-15 5:35 ` Eryu Guan
2014-11-15 5:35 ` Eryu Guan
2014-11-17 5:41 ` Dave Chinner
2014-11-17 5:41 ` Dave Chinner
2014-11-17 6:06 ` Eryu Guan
2014-11-17 6:06 ` Eryu Guan
2014-11-17 6:54 ` Dave Chinner
2014-11-17 6:54 ` Dave Chinner
2014-11-17 14:53 ` Omer Zilberberg
2014-11-17 14:53 ` Omer Zilberberg
2014-11-17 20:22 ` Dave Chinner
2014-11-17 20:22 ` Dave Chinner
2014-11-18 16:16 ` Omer Zilberberg
2014-11-18 16:16 ` Omer Zilberberg
2014-11-17 5:34 ` Dave Chinner
2014-11-17 5:34 ` Dave Chinner
2014-10-31 17:03 ` [PATCH v2 2/5] common: add _require_block_device() helper Eryu Guan
2014-10-31 17:03 ` [PATCH v2 3/5] common: skip atime related tests on NFS Eryu Guan
2014-10-31 17:03 ` [PATCH v2 4/5] common: use _scratch_mount helper in _require_relatime() Eryu Guan
2014-10-31 17:04 ` [PATCH v2 5/5] generic/277: add _require_attrs Eryu Guan
[not found] ` <1414775040-4051-1-git-send-email-eguan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
2014-11-02 20:52 ` Fwd: [PATCH v2 0/5] re-enable tests that require scratch dev on NFS Steve French
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.