* [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
* [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
* 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
* 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: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
* 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
end of thread, other threads:[~2014-11-18 16:21 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.