* [PATCH] xfs: Regression test for invalid sb_logsunit @ 2018-01-11 8:25 xiao yang 2018-01-11 18:07 ` Darrick J. Wong 0 siblings, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-11 8:25 UTC (permalink / raw) To: fstests; +Cc: darrick.wong, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/437.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 76 insertions(+) create mode 100755 tests/xfs/437 create mode 100644 tests/xfs/437.out diff --git a/tests/xfs/437 b/tests/xfs/437 new file mode 100755 index 0000000..96cec25 --- /dev/null +++ b/tests/xfs/437 @@ -0,0 +1,73 @@ +#! /bin/bash +# FS QA Test No. 437 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment and checks +. ./common/rc + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_db -x -c "sb 0" -c "p blocksize") +_scratch_xfs_db -x -c "sb 0" -c "write logsunit $((blksz - 1))" \ + >> $seqres.full 2>&1 || _notrun "Failed to set sb_logsunit" + +# Mount and writing log may trigger a crash +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + touch ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/437.out b/tests/xfs/437.out new file mode 100644 index 0000000..4dcb607 --- /dev/null +++ b/tests/xfs/437.out @@ -0,0 +1,2 @@ +QA output created by 437 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index d230060..35d1b03 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -434,3 +434,4 @@ 434 auto quick clone fsr 435 auto quick clone 436 auto quick clone fsr +437 auto quick log dangerous -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH] xfs: Regression test for invalid sb_logsunit 2018-01-11 8:25 [PATCH] xfs: Regression test for invalid sb_logsunit xiao yang @ 2018-01-11 18:07 ` Darrick J. Wong 2018-01-12 1:58 ` Xiao Yang ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Darrick J. Wong @ 2018-01-11 18:07 UTC (permalink / raw) To: xiao yang; +Cc: fstests On Thu, Jan 11, 2018 at 04:25:08PM +0800, xiao yang wrote: > If log stripe unit isn't a multiple of the fs blocksize and mounting, > the invalid sb_logsunit leads to crash as soon as we try to write to > the log. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/437.out | 2 ++ > tests/xfs/group | 1 + > 3 files changed, 76 insertions(+) > create mode 100755 tests/xfs/437 > create mode 100644 tests/xfs/437.out > > diff --git a/tests/xfs/437 b/tests/xfs/437 > new file mode 100755 > index 0000000..96cec25 > --- /dev/null > +++ b/tests/xfs/437 > @@ -0,0 +1,73 @@ > +#! /bin/bash > +# FS QA Test No. 437 > +# > +# Regression test for commit: > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > +# > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > +# the invalid sb_logsunit leads to crash as soon as we try to write to > +# the log. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it would be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write the Free Software Foundation, > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > +#----------------------------------------------------------------------- > + > +seq=`basename "$0"` > +seqres="$RESULT_DIR/$seq" > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +_cleanup() > +{ > + rm -rf $tmp.* > +} > + > +# get standard environment and checks > +. ./common/rc > + > +# real QA test starts here > +_supported_os Linux > +_supported_fs xfs > +_require_scratch > + > +rm -f "$seqres.full" > + > +# Format > +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" > + > +# Set logsunit to a value which is not a multiple of the fs blocksize > +blksz=$(_scratch_xfs_db -x -c "sb 0" -c "p blocksize") _scratch_xfs_get_metadata_field ? (Or, _scratch_xfs_get_sb_field if Hou Tao's dquot unmount tests go in) > +_scratch_xfs_db -x -c "sb 0" -c "write logsunit $((blksz - 1))" \ > + >> $seqres.full 2>&1 || _notrun "Failed to set sb_logsunit" _scratch_xfs_set_metadata_field ? Otherwise this looks decent. --D > + > +# Mount and writing log may trigger a crash > +if _scratch_mount >> $seqres.full 2>&1; then > + for i in $(seq 1 1000); do > + touch ${SCRATCH_MNT}/$i > + done > + _scratch_unmount > +fi > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/xfs/437.out b/tests/xfs/437.out > new file mode 100644 > index 0000000..4dcb607 > --- /dev/null > +++ b/tests/xfs/437.out > @@ -0,0 +1,2 @@ > +QA output created by 437 > +Silence is golden > diff --git a/tests/xfs/group b/tests/xfs/group > index d230060..35d1b03 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -434,3 +434,4 @@ > 434 auto quick clone fsr > 435 auto quick clone > 436 auto quick clone fsr > +437 auto quick log dangerous > -- > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH] xfs: Regression test for invalid sb_logsunit 2018-01-11 18:07 ` Darrick J. Wong @ 2018-01-12 1:58 ` Xiao Yang 2018-01-12 6:14 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory xiao yang 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2 siblings, 0 replies; 42+ messages in thread From: Xiao Yang @ 2018-01-12 1:58 UTC (permalink / raw) To: Darrick J. Wong; +Cc: fstests Hi Darrick, Thanks for your comment, i will send v2 patch based on _scratch_xfs_[get|set]_sb_field helper. :-) On 2018/01/12 2:07, Darrick J. Wong wrote: > On Thu, Jan 11, 2018 at 04:25:08PM +0800, xiao yang wrote: >> If log stripe unit isn't a multiple of the fs blocksize and mounting, >> the invalid sb_logsunit leads to crash as soon as we try to write to >> the log. >> >> Signed-off-by: xiao yang<yangx.jy@cn.fujitsu.com> >> --- >> tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> tests/xfs/437.out | 2 ++ >> tests/xfs/group | 1 + >> 3 files changed, 76 insertions(+) >> create mode 100755 tests/xfs/437 >> create mode 100644 tests/xfs/437.out >> >> diff --git a/tests/xfs/437 b/tests/xfs/437 >> new file mode 100755 >> index 0000000..96cec25 >> --- /dev/null >> +++ b/tests/xfs/437 >> @@ -0,0 +1,73 @@ >> +#! /bin/bash >> +# FS QA Test No. 437 >> +# >> +# Regression test for commit: >> +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") >> +# >> +# If log stripe unit isn't a multiple of the fs blocksize and mounting, >> +# the invalid sb_logsunit leads to crash as soon as we try to write to >> +# the log. >> +# >> +#----------------------------------------------------------------------- >> +# Copyright (c) 2018 FUJITSU. All Rights Reserved. >> +# Author: Xiao Yang<yangx.jy@cn.fujitsu.com> >> +# >> +# This program is free software; you can redistribute it and/or >> +# modify it under the terms of the GNU General Public License as >> +# published by the Free Software Foundation. >> +# >> +# This program is distributed in the hope that it would be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program; if not, write the Free Software Foundation, >> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >> +#----------------------------------------------------------------------- >> + >> +seq=`basename "$0"` >> +seqres="$RESULT_DIR/$seq" >> +echo "QA output created by $seq" >> + >> +here=`pwd` >> +tmp=/tmp/$$ >> +status=1 # failure is the default! >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +_cleanup() >> +{ >> + rm -rf $tmp.* >> +} >> + >> +# get standard environment and checks >> +. ./common/rc >> + >> +# real QA test starts here >> +_supported_os Linux >> +_supported_fs xfs >> +_require_scratch >> + >> +rm -f "$seqres.full" >> + >> +# Format >> +_scratch_mkfs> $seqres.full 2>&1 || _fail "mkfs failed" >> + >> +# Set logsunit to a value which is not a multiple of the fs blocksize >> +blksz=$(_scratch_xfs_db -x -c "sb 0" -c "p blocksize") > _scratch_xfs_get_metadata_field ? > > (Or, _scratch_xfs_get_sb_field if Hou Tao's dquot unmount tests go in) Agreed. Use _scratch_xfs_get_sb_field here. >> +_scratch_xfs_db -x -c "sb 0" -c "write logsunit $((blksz - 1))" \ >> + >> $seqres.full 2>&1 || _notrun "Failed to set sb_logsunit" > _scratch_xfs_set_metadata_field ? Agreed. Use _scratch_xfs_set_sb_field here. Thanks, Xiao Yang > Otherwise this looks decent. > > --D >> + >> +# Mount and writing log may trigger a crash >> +if _scratch_mount>> $seqres.full 2>&1; then >> + for i in $(seq 1 1000); do >> + touch ${SCRATCH_MNT}/$i >> + done >> + _scratch_unmount >> +fi >> + >> +echo "Silence is golden" >> + >> +# success, all done >> +status=0 >> +exit >> diff --git a/tests/xfs/437.out b/tests/xfs/437.out >> new file mode 100644 >> index 0000000..4dcb607 >> --- /dev/null >> +++ b/tests/xfs/437.out >> @@ -0,0 +1,2 @@ >> +QA output created by 437 >> +Silence is golden >> diff --git a/tests/xfs/group b/tests/xfs/group >> index d230060..35d1b03 100644 >> --- a/tests/xfs/group >> +++ b/tests/xfs/group >> @@ -434,3 +434,4 @@ >> 434 auto quick clone fsr >> 435 auto quick clone >> 436 auto quick clone fsr >> +437 auto quick log dangerous >> -- >> 1.8.3.1 >> >> >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory 2018-01-11 18:07 ` Darrick J. Wong 2018-01-12 1:58 ` Xiao Yang @ 2018-01-12 6:14 ` xiao yang 2018-01-12 6:14 ` [PATCH v2] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-12 6:19 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory Xiao Yang 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2 siblings, 2 replies; 42+ messages in thread From: xiao yang @ 2018-01-12 6:14 UTC (permalink / raw) To: fstests; +Cc: darrick.wong, eguan, xiao yang 1) on some distros(e.g. RHEL6), memory cgroup was supported and mounted on /cgroup/memory by default, but the test was skipped if /sys/fs/cgroup/memory did not exist. 2) We got the following error if memory cgroup wasn't mounted on /sys/fs/cgroup/memory: ------------------------------------------------------------ safe_macros.c:169: BROK: madvise09.c:175: mkdir(/sys/fs/cgroup/memory/ltp_madvise09_16386/,0777) failed: EROFS ------------------------------------------------------------ We use custom mount point and mount memory cgroup on it manually to fix these issues. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- testcases/kernel/syscalls/madvise/madvise09.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/testcases/kernel/syscalls/madvise/madvise09.c b/testcases/kernel/syscalls/madvise/madvise09.c index f744405..25cf81f 100644 --- a/testcases/kernel/syscalls/madvise/madvise09.c +++ b/testcases/kernel/syscalls/madvise/madvise09.c @@ -53,12 +53,14 @@ #include <errno.h> #include <stdio.h> #include <ctype.h> +#include <sys/mount.h> #include "tst_test.h" #include "lapi/mmap.h" -#define MEMCG_PATH "/sys/fs/cgroup/memory/" +#define MEMCG_PATH "/dev/memcg_madvise09/" +static int memcg_mounted; static char cgroup_path[PATH_MAX]; static char tasks_path[PATH_MAX]; static char limit_in_bytes_path[PATH_MAX]; @@ -277,6 +279,15 @@ static void cleanup(void) { if (cgroup_path[0] && !access(cgroup_path, F_OK)) rmdir(cgroup_path); + + if (memcg_mounted) { + tst_res(TINFO, "Umount memory cgroup after testing"); + SAFE_UMOUNT(MEMCG_PATH); + memcg_mounted = 0; + } + + if (!access(MEMCG_PATH, F_OK) && rmdir(MEMCG_PATH)) + tst_res(TWARN | TERRNO, "Rmdir %s failed", MEMCG_PATH); } static void run(void) @@ -316,10 +327,17 @@ static void setup(void) { long int swap_total; - if (access(MEMCG_PATH, F_OK)) { - tst_brk(TCONF, "'" MEMCG_PATH - "' not present, CONFIG_MEMCG missing?"); + SAFE_MKDIR(MEMCG_PATH, 0777); + + tst_res(TINFO, "Mount memory cgroup on %s", MEMCG_PATH); + if (mount("memcg", MEMCG_PATH, "cgroup", 0, "memory") == -1) { + if (errno == ENODEV) { + tst_brk(TCONF, + "Memory cgroup was not configured in kernel"); + } + tst_brk(TBROK | TERRNO, "Failed to mount memory cgroup"); } + memcg_mounted = 1; if (!access(MEMCG_PATH "memory.memsw.limit_in_bytes", F_OK)) swap_accounting_enabled = 1; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2] xfs: Regression test for invalid sb_logsunit 2018-01-12 6:14 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory xiao yang @ 2018-01-12 6:14 ` xiao yang 2018-01-12 6:19 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory Xiao Yang 1 sibling, 0 replies; 42+ messages in thread From: xiao yang @ 2018-01-12 6:14 UTC (permalink / raw) To: fstests; +Cc: darrick.wong, eguan, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/437.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 76 insertions(+) create mode 100755 tests/xfs/437 create mode 100644 tests/xfs/437.out diff --git a/tests/xfs/437 b/tests/xfs/437 new file mode 100755 index 0000000..f2b84ad --- /dev/null +++ b/tests/xfs/437 @@ -0,0 +1,73 @@ +#! /bin/bash +# FS QA Test No. 437 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment and checks +. ./common/rc + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 \ + || _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + touch ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/437.out b/tests/xfs/437.out new file mode 100644 index 0000000..4dcb607 --- /dev/null +++ b/tests/xfs/437.out @@ -0,0 +1,2 @@ +QA output created by 437 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index d230060..35d1b03 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -434,3 +434,4 @@ 434 auto quick clone fsr 435 auto quick clone 436 auto quick clone fsr +437 auto quick log dangerous -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory 2018-01-12 6:14 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory xiao yang 2018-01-12 6:14 ` [PATCH v2] xfs: Regression test for invalid sb_logsunit xiao yang @ 2018-01-12 6:19 ` Xiao Yang 1 sibling, 0 replies; 42+ messages in thread From: Xiao Yang @ 2018-01-12 6:19 UTC (permalink / raw) To: xiao yang; +Cc: fstests, darrick.wong, eguan Hi, Sorry, please ignore the wrong patchset, i will resend the v2 patchset. Thanks, Xiao Yang On 2018/01/12 14:14, xiao yang wrote: > 1) on some distros(e.g. RHEL6), memory cgroup was supported and > mounted on /cgroup/memory by default, but the test was skipped > if /sys/fs/cgroup/memory did not exist. > > 2) We got the following error if memory cgroup wasn't mounted > on /sys/fs/cgroup/memory: > ------------------------------------------------------------ > safe_macros.c:169: BROK: madvise09.c:175: mkdir(/sys/fs/cgroup/memory/ltp_madvise09_16386/,0777) failed: EROFS > ------------------------------------------------------------ > > We use custom mount point and mount memory cgroup on it manually > to fix these issues. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > testcases/kernel/syscalls/madvise/madvise09.c | 26 ++++++++++++++++++++++---- > 1 file changed, 22 insertions(+), 4 deletions(-) > > diff --git a/testcases/kernel/syscalls/madvise/madvise09.c b/testcases/kernel/syscalls/madvise/madvise09.c > index f744405..25cf81f 100644 > --- a/testcases/kernel/syscalls/madvise/madvise09.c > +++ b/testcases/kernel/syscalls/madvise/madvise09.c > @@ -53,12 +53,14 @@ > #include <errno.h> > #include <stdio.h> > #include <ctype.h> > +#include <sys/mount.h> > > #include "tst_test.h" > #include "lapi/mmap.h" > > -#define MEMCG_PATH "/sys/fs/cgroup/memory/" > +#define MEMCG_PATH "/dev/memcg_madvise09/" > > +static int memcg_mounted; > static char cgroup_path[PATH_MAX]; > static char tasks_path[PATH_MAX]; > static char limit_in_bytes_path[PATH_MAX]; > @@ -277,6 +279,15 @@ static void cleanup(void) > { > if (cgroup_path[0] && !access(cgroup_path, F_OK)) > rmdir(cgroup_path); > + > + if (memcg_mounted) { > + tst_res(TINFO, "Umount memory cgroup after testing"); > + SAFE_UMOUNT(MEMCG_PATH); > + memcg_mounted = 0; > + } > + > + if (!access(MEMCG_PATH, F_OK) && rmdir(MEMCG_PATH)) > + tst_res(TWARN | TERRNO, "Rmdir %s failed", MEMCG_PATH); > } > > static void run(void) > @@ -316,10 +327,17 @@ static void setup(void) > { > long int swap_total; > > - if (access(MEMCG_PATH, F_OK)) { > - tst_brk(TCONF, "'" MEMCG_PATH > - "' not present, CONFIG_MEMCG missing?"); > + SAFE_MKDIR(MEMCG_PATH, 0777); > + > + tst_res(TINFO, "Mount memory cgroup on %s", MEMCG_PATH); > + if (mount("memcg", MEMCG_PATH, "cgroup", 0, "memory") == -1) { > + if (errno == ENODEV) { > + tst_brk(TCONF, > + "Memory cgroup was not configured in kernel"); > + } > + tst_brk(TBROK | TERRNO, "Failed to mount memory cgroup"); > } > + memcg_mounted = 1; > > if (!access(MEMCG_PATH "memory.memsw.limit_in_bytes", F_OK)) > swap_accounting_enabled = 1; ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-11 18:07 ` Darrick J. Wong 2018-01-12 1:58 ` Xiao Yang 2018-01-12 6:14 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory xiao yang @ 2018-01-12 6:16 ` xiao yang 2018-01-12 6:16 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit xiao yang ` (2 more replies) 2 siblings, 3 replies; 42+ messages in thread From: xiao yang @ 2018-01-12 6:16 UTC (permalink / raw) To: fstests; +Cc: darrick.wong, eguan, xiao yang Make sure _scratch_xfs_set_metadata_field() can be used on an old xfsprogs-dev(e.g. v3.1.1). The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). The special argument "--" is only used to end option-scanning in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/xfs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/common/xfs b/common/xfs index 45b84a0..59d6dd3 100644 --- a/common/xfs +++ b/common/xfs @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() for arg in "$@"; do cmds+=("-c" "${arg}") done - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" - echo + + local wr_cmd="write" + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- $value" + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" } _scratch_xfs_get_sb_field() -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang @ 2018-01-12 6:16 ` xiao yang 2018-01-12 7:49 ` Eryu Guan 2018-01-12 7:44 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db Eryu Guan 2018-01-12 16:43 ` Darrick J. Wong 2 siblings, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-12 6:16 UTC (permalink / raw) To: fstests; +Cc: darrick.wong, eguan, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/437.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 76 insertions(+) create mode 100755 tests/xfs/437 create mode 100644 tests/xfs/437.out diff --git a/tests/xfs/437 b/tests/xfs/437 new file mode 100755 index 0000000..f2b84ad --- /dev/null +++ b/tests/xfs/437 @@ -0,0 +1,73 @@ +#! /bin/bash +# FS QA Test No. 437 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment and checks +. ./common/rc + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 \ + || _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + touch ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/437.out b/tests/xfs/437.out new file mode 100644 index 0000000..4dcb607 --- /dev/null +++ b/tests/xfs/437.out @@ -0,0 +1,2 @@ +QA output created by 437 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index d230060..35d1b03 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -434,3 +434,4 @@ 434 auto quick clone fsr 435 auto quick clone 436 auto quick clone fsr +437 auto quick log dangerous -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 6:16 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit xiao yang @ 2018-01-12 7:49 ` Eryu Guan 2018-01-12 8:36 ` Dave Chinner 0 siblings, 1 reply; 42+ messages in thread From: Eryu Guan @ 2018-01-12 7:49 UTC (permalink / raw) To: xiao yang; +Cc: fstests, darrick.wong On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > If log stripe unit isn't a multiple of the fs blocksize and mounting, > the invalid sb_logsunit leads to crash as soon as we try to write to > the log. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/437.out | 2 ++ > tests/xfs/group | 1 + > 3 files changed, 76 insertions(+) > create mode 100755 tests/xfs/437 > create mode 100644 tests/xfs/437.out > > diff --git a/tests/xfs/437 b/tests/xfs/437 > new file mode 100755 > index 0000000..f2b84ad > --- /dev/null > +++ b/tests/xfs/437 > @@ -0,0 +1,73 @@ > +#! /bin/bash > +# FS QA Test No. 437 > +# > +# Regression test for commit: > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > +# > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > +# the invalid sb_logsunit leads to crash as soon as we try to write to > +# the log. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it would be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write the Free Software Foundation, > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > +#----------------------------------------------------------------------- > + > +seq=`basename "$0"` > +seqres="$RESULT_DIR/$seq" > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +_cleanup() > +{ > + rm -rf $tmp.* > +} > + > +# get standard environment and checks > +. ./common/rc > + > +# real QA test starts here > +_supported_os Linux > +_supported_fs xfs > +_require_scratch This test triggers ASSERT failure and warning on debug build, thus failed _dmesg_check, I think we need _disable_dmesg_check (and some comments) too. [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > + > +rm -f "$seqres.full" > + > +# Format > +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" > + > +# Set logsunit to a value which is not a multiple of the fs blocksize > +blksz=$(_scratch_xfs_get_sb_field blocksize) > +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 \ > + || _notrun "failed to set sb_logsunit" It seems xfs_db doesn't return non-zero on failure, so _notrun won'be be triggered, you may want to read the logsuint value again to see if it's set correctly. > + > +# Mount and writing log may trigger a crash > +if _scratch_mount >> $seqres.full 2>&1; then Better to mention that with the fix applied kernel refuses to mount, so we know a mount failure is expected. > + for i in $(seq 1 1000); do > + touch ${SCRATCH_MNT}/$i echo > ${SCRATCH_MNT}/$i might be faster Thanks, Eryu > + done > + _scratch_unmount > +fi > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/xfs/437.out b/tests/xfs/437.out > new file mode 100644 > index 0000000..4dcb607 > --- /dev/null > +++ b/tests/xfs/437.out > @@ -0,0 +1,2 @@ > +QA output created by 437 > +Silence is golden > diff --git a/tests/xfs/group b/tests/xfs/group > index d230060..35d1b03 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -434,3 +434,4 @@ > 434 auto quick clone fsr > 435 auto quick clone > 436 auto quick clone fsr > +437 auto quick log dangerous > -- > 1.8.3.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 7:49 ` Eryu Guan @ 2018-01-12 8:36 ` Dave Chinner 2018-01-12 8:50 ` Eryu Guan 0 siblings, 1 reply; 42+ messages in thread From: Dave Chinner @ 2018-01-12 8:36 UTC (permalink / raw) To: Eryu Guan; +Cc: xiao yang, fstests, darrick.wong On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > the invalid sb_logsunit leads to crash as soon as we try to write to > > the log. > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > --- > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/xfs/437.out | 2 ++ > > tests/xfs/group | 1 + > > 3 files changed, 76 insertions(+) > > create mode 100755 tests/xfs/437 > > create mode 100644 tests/xfs/437.out > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > new file mode 100755 > > index 0000000..f2b84ad > > --- /dev/null > > +++ b/tests/xfs/437 > > @@ -0,0 +1,73 @@ > > +#! /bin/bash > > +# FS QA Test No. 437 > > +# > > +# Regression test for commit: > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > +# > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > +# the log. > > +# > > +#----------------------------------------------------------------------- > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > +# > > +# This program is free software; you can redistribute it and/or > > +# modify it under the terms of the GNU General Public License as > > +# published by the Free Software Foundation. > > +# > > +# This program is distributed in the hope that it would be useful, > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > +# GNU General Public License for more details. > > +# > > +# You should have received a copy of the GNU General Public License > > +# along with this program; if not, write the Free Software Foundation, > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > +#----------------------------------------------------------------------- > > + > > +seq=`basename "$0"` > > +seqres="$RESULT_DIR/$seq" > > +echo "QA output created by $seq" > > + > > +here=`pwd` > > +tmp=/tmp/$$ > > +status=1 # failure is the default! > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > + > > +_cleanup() > > +{ > > + rm -rf $tmp.* > > +} > > + > > +# get standard environment and checks > > +. ./common/rc > > + > > +# real QA test starts here > > +_supported_os Linux > > +_supported_fs xfs > > +_require_scratch > > This test triggers ASSERT failure and warning on debug build, thus > failed _dmesg_check, I think we need _disable_dmesg_check (and some > comments) too. > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] If it triggers debug asserts which have been put there to catch bugs in other utilities (like mkfs) on a current upstream debug kernel, then the test should not be part of the auto and quick groups. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 8:36 ` Dave Chinner @ 2018-01-12 8:50 ` Eryu Guan 2018-01-12 16:41 ` Darrick J. Wong 2018-01-13 2:23 ` Dave Chinner 0 siblings, 2 replies; 42+ messages in thread From: Eryu Guan @ 2018-01-12 8:50 UTC (permalink / raw) To: Dave Chinner; +Cc: xiao yang, fstests, darrick.wong On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > the log. > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > --- > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > tests/xfs/437.out | 2 ++ > > > tests/xfs/group | 1 + > > > 3 files changed, 76 insertions(+) > > > create mode 100755 tests/xfs/437 > > > create mode 100644 tests/xfs/437.out > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > new file mode 100755 > > > index 0000000..f2b84ad > > > --- /dev/null > > > +++ b/tests/xfs/437 > > > @@ -0,0 +1,73 @@ > > > +#! /bin/bash > > > +# FS QA Test No. 437 > > > +# > > > +# Regression test for commit: > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > +# > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > +# the log. > > > +# > > > +#----------------------------------------------------------------------- > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > +# > > > +# This program is free software; you can redistribute it and/or > > > +# modify it under the terms of the GNU General Public License as > > > +# published by the Free Software Foundation. > > > +# > > > +# This program is distributed in the hope that it would be useful, > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > +# GNU General Public License for more details. > > > +# > > > +# You should have received a copy of the GNU General Public License > > > +# along with this program; if not, write the Free Software Foundation, > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > +#----------------------------------------------------------------------- > > > + > > > +seq=`basename "$0"` > > > +seqres="$RESULT_DIR/$seq" > > > +echo "QA output created by $seq" > > > + > > > +here=`pwd` > > > +tmp=/tmp/$$ > > > +status=1 # failure is the default! > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > + > > > +_cleanup() > > > +{ > > > + rm -rf $tmp.* > > > +} > > > + > > > +# get standard environment and checks > > > +. ./common/rc > > > + > > > +# real QA test starts here > > > +_supported_os Linux > > > +_supported_fs xfs > > > +_require_scratch > > > > This test triggers ASSERT failure and warning on debug build, thus > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > comments) too. > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > If it triggers debug asserts which have been put there to catch bugs > in other utilities (like mkfs) on a current upstream debug kernel, > then the test should not be part of the auto and quick groups. This test corrupts the filesystem on purpose (I didn't make this clear in my last reply, sorry about that), and xfs detects the corruption & refuses the mount instead of crashing. So I think the ASSERT failure is expected on debug build and we just need to ignore it. And the test reproduces the crash quickly and reliably on buggy kernel and passes (if we ignore the ASSERT failure) on patched kernel, it serves as a good 'auto' and 'quick' regression test to me. Thanks, Eryu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 8:50 ` Eryu Guan @ 2018-01-12 16:41 ` Darrick J. Wong 2018-01-13 2:23 ` Dave Chinner 1 sibling, 0 replies; 42+ messages in thread From: Darrick J. Wong @ 2018-01-12 16:41 UTC (permalink / raw) To: Eryu Guan; +Cc: Dave Chinner, xiao yang, fstests On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > the log. > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > --- > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/xfs/437.out | 2 ++ > > > > tests/xfs/group | 1 + > > > > 3 files changed, 76 insertions(+) > > > > create mode 100755 tests/xfs/437 > > > > create mode 100644 tests/xfs/437.out > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > new file mode 100755 > > > > index 0000000..f2b84ad > > > > --- /dev/null > > > > +++ b/tests/xfs/437 > > > > @@ -0,0 +1,73 @@ > > > > +#! /bin/bash > > > > +# FS QA Test No. 437 > > > > +# > > > > +# Regression test for commit: > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > +# > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > +# the log. > > > > +# > > > > +#----------------------------------------------------------------------- > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > +# > > > > +# This program is free software; you can redistribute it and/or > > > > +# modify it under the terms of the GNU General Public License as > > > > +# published by the Free Software Foundation. > > > > +# > > > > +# This program is distributed in the hope that it would be useful, > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > +# GNU General Public License for more details. > > > > +# > > > > +# You should have received a copy of the GNU General Public License > > > > +# along with this program; if not, write the Free Software Foundation, > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > +#----------------------------------------------------------------------- > > > > + > > > > +seq=`basename "$0"` > > > > +seqres="$RESULT_DIR/$seq" > > > > +echo "QA output created by $seq" > > > > + > > > > +here=`pwd` > > > > +tmp=/tmp/$$ > > > > +status=1 # failure is the default! > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > + > > > > +_cleanup() > > > > +{ > > > > + rm -rf $tmp.* > > > > +} > > > > + > > > > +# get standard environment and checks > > > > +. ./common/rc > > > > + > > > > +# real QA test starts here > > > > +_supported_os Linux > > > > +_supported_fs xfs > > > > +_require_scratch > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > comments) too. > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > If it triggers debug asserts which have been put there to catch bugs > > in other utilities (like mkfs) on a current upstream debug kernel, > > then the test should not be part of the auto and quick groups. This particular assert triggers because we rejected the log geometry options thta we just read off the disk. I don't think this scenario is that much different from a block verifier rejecting a block (two printks and an assert(0) for a corrupt field seem a bit much to me but clearly someone wanted that to fail noisily), so I think it better just to filter this particular assertion. Frankly I think I'd just as soon get rid of the ASSERT since I've been moving the disk verifiers away from ASSERT->BUGing on garbage disk contents. > This test corrupts the filesystem on purpose (I didn't make this clear > in my last reply, sorry about that), and xfs detects the corruption & > refuses the mount instead of crashing. So I think the ASSERT failure is > expected on debug build and we just need to ignore it. > > And the test reproduces the crash quickly and reliably on buggy kernel > and passes (if we ignore the ASSERT failure) on patched kernel, it > serves as a good 'auto' and 'quick' regression test to me. It ought to be in 'fuzzers' too... --D > Thanks, > Eryu > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-12 8:50 ` Eryu Guan 2018-01-12 16:41 ` Darrick J. Wong @ 2018-01-13 2:23 ` Dave Chinner 2018-01-15 6:29 ` Eryu Guan 2018-01-15 12:45 ` [PATCH v2 2/2] " Brian Foster 1 sibling, 2 replies; 42+ messages in thread From: Dave Chinner @ 2018-01-13 2:23 UTC (permalink / raw) To: Eryu Guan; +Cc: xiao yang, fstests, darrick.wong On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > the log. > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > --- > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/xfs/437.out | 2 ++ > > > > tests/xfs/group | 1 + > > > > 3 files changed, 76 insertions(+) > > > > create mode 100755 tests/xfs/437 > > > > create mode 100644 tests/xfs/437.out > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > new file mode 100755 > > > > index 0000000..f2b84ad > > > > --- /dev/null > > > > +++ b/tests/xfs/437 > > > > @@ -0,0 +1,73 @@ > > > > +#! /bin/bash > > > > +# FS QA Test No. 437 > > > > +# > > > > +# Regression test for commit: > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > +# > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > +# the log. > > > > +# > > > > +#----------------------------------------------------------------------- > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > +# > > > > +# This program is free software; you can redistribute it and/or > > > > +# modify it under the terms of the GNU General Public License as > > > > +# published by the Free Software Foundation. > > > > +# > > > > +# This program is distributed in the hope that it would be useful, > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > +# GNU General Public License for more details. > > > > +# > > > > +# You should have received a copy of the GNU General Public License > > > > +# along with this program; if not, write the Free Software Foundation, > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > +#----------------------------------------------------------------------- > > > > + > > > > +seq=`basename "$0"` > > > > +seqres="$RESULT_DIR/$seq" > > > > +echo "QA output created by $seq" > > > > + > > > > +here=`pwd` > > > > +tmp=/tmp/$$ > > > > +status=1 # failure is the default! > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > + > > > > +_cleanup() > > > > +{ > > > > + rm -rf $tmp.* > > > > +} > > > > + > > > > +# get standard environment and checks > > > > +. ./common/rc > > > > + > > > > +# real QA test starts here > > > > +_supported_os Linux > > > > +_supported_fs xfs > > > > +_require_scratch > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > comments) too. > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > If it triggers debug asserts which have been put there to catch bugs > > in other utilities (like mkfs) on a current upstream debug kernel, > > then the test should not be part of the auto and quick groups. > > This test corrupts the filesystem on purpose (I didn't make this clear > in my last reply, sorry about that), and xfs detects the corruption & > refuses the mount instead of crashing. So I think the ASSERT failure is > expected on debug build and we just need to ignore it. Except that after that assert, the block device is now busy and can't be released, so we can't use the scratch device anymore. All tests after this assert has been triggered are going to fail because the block device is busy.... That's the whole point of adding debug asserts in cases like this - they are supposed to stop test execution in it's tracks and leave a corpse to analyse. The auto group regression tests are not supposed to take the machine down on normal test configs (i.e. CONFIG_XFS_DEBUG=y). > And the test reproduces the crash quickly and reliably on buggy kernel > and passes (if we ignore the ASSERT failure) on patched kernel, it > serves as a good 'auto' and 'quick' regression test to me. Sure, I'm not saying that it doesn't work as a regression test. I'm saying that it's a regression test that *should not be run by everyone*. It's classified as dangerous which means - by definition - it should not be part of the quick and auto groups.... If you want to test dangerous regression tests, then you need to *opt-in* by adding "-g dangerous", not force everyone else to opt-out by having to run "-g auto -x dangerous". Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-13 2:23 ` Dave Chinner @ 2018-01-15 6:29 ` Eryu Guan 2018-01-15 7:48 ` [PATCH v3 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-15 12:45 ` [PATCH v2 2/2] " Brian Foster 1 sibling, 1 reply; 42+ messages in thread From: Eryu Guan @ 2018-01-15 6:29 UTC (permalink / raw) To: Dave Chinner; +Cc: xiao yang, fstests, darrick.wong On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > the log. > > > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > > --- > > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/xfs/437.out | 2 ++ > > > > > tests/xfs/group | 1 + > > > > > 3 files changed, 76 insertions(+) > > > > > create mode 100755 tests/xfs/437 > > > > > create mode 100644 tests/xfs/437.out > > > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > > new file mode 100755 > > > > > index 0000000..f2b84ad > > > > > --- /dev/null > > > > > +++ b/tests/xfs/437 > > > > > @@ -0,0 +1,73 @@ > > > > > +#! /bin/bash > > > > > +# FS QA Test No. 437 > > > > > +# > > > > > +# Regression test for commit: > > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > > +# > > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > +# the log. > > > > > +# > > > > > +#----------------------------------------------------------------------- > > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > > +# > > > > > +# This program is free software; you can redistribute it and/or > > > > > +# modify it under the terms of the GNU General Public License as > > > > > +# published by the Free Software Foundation. > > > > > +# > > > > > +# This program is distributed in the hope that it would be useful, > > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > > +# GNU General Public License for more details. > > > > > +# > > > > > +# You should have received a copy of the GNU General Public License > > > > > +# along with this program; if not, write the Free Software Foundation, > > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > > +#----------------------------------------------------------------------- > > > > > + > > > > > +seq=`basename "$0"` > > > > > +seqres="$RESULT_DIR/$seq" > > > > > +echo "QA output created by $seq" > > > > > + > > > > > +here=`pwd` > > > > > +tmp=/tmp/$$ > > > > > +status=1 # failure is the default! > > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > + > > > > > +_cleanup() > > > > > +{ > > > > > + rm -rf $tmp.* > > > > > +} > > > > > + > > > > > +# get standard environment and checks > > > > > +. ./common/rc > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_os Linux > > > > > +_supported_fs xfs > > > > > +_require_scratch > > > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > > comments) too. > > > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > > > If it triggers debug asserts which have been put there to catch bugs > > > in other utilities (like mkfs) on a current upstream debug kernel, > > > then the test should not be part of the auto and quick groups. > > > > This test corrupts the filesystem on purpose (I didn't make this clear > > in my last reply, sorry about that), and xfs detects the corruption & > > refuses the mount instead of crashing. So I think the ASSERT failure is > > expected on debug build and we just need to ignore it. > > Except that after that assert, the block device is now busy and can't > be released, so we can't use the scratch device anymore. All tests > after this assert has been triggered are going to fail because > the block device is busy.... > > That's the whole point of adding debug asserts in cases like this - > they are supposed to stop test execution in it's tracks and leave a > corpse to analyse. The auto group regression tests are not supposed > to take the machine down on normal test configs (i.e. > CONFIG_XFS_DEBUG=y). You're right, my test kernel was built with XFS_WARN not XFS_DEBUG, I missed that the test would crash kernel even on patched kernel on XFS_DEBUG builds. Thanks for pointing this out! Eryu ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-15 6:29 ` Eryu Guan @ 2018-01-15 7:48 ` xiao yang 2018-01-15 7:48 ` [PATCH v3 2/3] common/filter: factor out expected XFS warnings for mount xiao yang 2018-01-15 7:48 ` [PATCH v3 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 0 siblings, 2 replies; 42+ messages in thread From: xiao yang @ 2018-01-15 7:48 UTC (permalink / raw) To: eguan; +Cc: david, darrick.wong, fstests, xiao yang Make sure _scratch_xfs_set_metadata_field() can be used on an old xfsprogs-dev(e.g. v3.1.1). The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). The special argument "--" is only used to end option-scanning in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/xfs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/common/xfs b/common/xfs index 45b84a0..ab58364 100644 --- a/common/xfs +++ b/common/xfs @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() for arg in "$@"; do cmds+=("-c" "${arg}") done - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" - echo + + local wr_cmd="write" + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- ${value}" + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" } _scratch_xfs_get_sb_field() -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v3 2/3] common/filter: factor out expected XFS warnings for mount 2018-01-15 7:48 ` [PATCH v3 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang @ 2018-01-15 7:48 ` xiao yang 2018-01-15 7:48 ` [PATCH v3 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 1 sibling, 0 replies; 42+ messages in thread From: xiao yang @ 2018-01-15 7:48 UTC (permalink / raw) To: eguan; +Cc: david, darrick.wong, fstests, xiao yang 1) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename it as _filter_mount_dmesg. 2) filter out another expected warning on CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) build. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/filter | 11 +++++++++++ tests/xfs/098 | 16 ++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/common/filter b/common/filter index 8e1fdb4..71b8c92 100644 --- a/common/filter +++ b/common/filter @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() -e "s#$warn9#Intentional warnings in dio_complete#" } +# We generate mount related WARNINGs on purpose and make sure test doesn't fail +# because of these warnings. This is a helper for _check_dmesg() to filter out +# them. +_filter_mount_dmesg() +{ + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" + sed -e "s#$warn1#Intentional warnings in asswarn#" \ + -e "s#$warn2#Intentional warnings in assfail#" +} + # make sure this script returns success /bin/true diff --git a/tests/xfs/098 b/tests/xfs/098 index 9bcd94b..bf7f89a 100755 --- a/tests/xfs/098 +++ b/tests/xfs/098 @@ -56,16 +56,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" rm -f $seqres.full -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check -# which could miss other potential bugs, so filter out the intentional WARNINGs, -# make sure test doesn't fail because of this warning and fails on other WARNINGs. -filter_xfs_dmesg() -{ - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" - sed -e "s#$warn#Intentional warnings in asswarn#" -} - TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" @@ -107,8 +97,10 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" echo "+ repair fs" _repair_scratch_fs >> $seqres.full 2>&1 -# mount may trigger related WARNINGs, so filter them. -_check_dmesg filter_xfs_dmesg +# We may trigger mounted related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) +# build, so filter them. +_check_dmesg _filter_mount_dmesg echo "+ mount image (2)" _scratch_mount -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v3 3/3] xfs: Regression test for invalid sb_logsunit 2018-01-15 7:48 ` [PATCH v3 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-15 7:48 ` [PATCH v3 2/3] common/filter: factor out expected XFS warnings for mount xiao yang @ 2018-01-15 7:48 ` xiao yang 1 sibling, 0 replies; 42+ messages in thread From: xiao yang @ 2018-01-15 7:48 UTC (permalink / raw) To: eguan; +Cc: david, darrick.wong, fstests, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/439 | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/439.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 87 insertions(+) create mode 100755 tests/xfs/439 create mode 100644 tests/xfs/439.out diff --git a/tests/xfs/439 b/tests/xfs/439 new file mode 100755 index 0000000..49d8074 --- /dev/null +++ b/tests/xfs/439 @@ -0,0 +1,84 @@ +#! /bin/bash +# FS QA Test No. 439 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 + +# Check if logsunit is set correctly +lsunit=$(_scratch_xfs_get_sb_field logsunit) +[ $lsunit -ne $((blksz - 1)) ] && _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash on buggy kernel +# The fix applied kernel refuses to mount, so a mount failure is +# expected +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + echo > ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +# Mount may trigger ASSERT failure and warnings on the fix applied +# kernel if logsunit is invalid on CONFIG_XFS_WARN or CONFIG_XFS_DEBUG +# (without CONFIG_XFS_ASSERT_FATAL) build, so filter them. +_check_dmesg _filter_mount_dmesg + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/439.out b/tests/xfs/439.out new file mode 100644 index 0000000..9712295 --- /dev/null +++ b/tests/xfs/439.out @@ -0,0 +1,2 @@ +QA output created by 439 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index 04a63b7..130f4e7 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -436,3 +436,4 @@ 436 auto quick clone fsr 437 auto quick other 438 auto quick quota dangerous +439 dangerous_fuzzers log -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-13 2:23 ` Dave Chinner 2018-01-15 6:29 ` Eryu Guan @ 2018-01-15 12:45 ` Brian Foster 2018-01-15 21:03 ` Dave Chinner 1 sibling, 1 reply; 42+ messages in thread From: Brian Foster @ 2018-01-15 12:45 UTC (permalink / raw) To: Dave Chinner; +Cc: Eryu Guan, xiao yang, fstests, darrick.wong On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > the log. > > > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > > --- > > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/xfs/437.out | 2 ++ > > > > > tests/xfs/group | 1 + > > > > > 3 files changed, 76 insertions(+) > > > > > create mode 100755 tests/xfs/437 > > > > > create mode 100644 tests/xfs/437.out > > > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > > new file mode 100755 > > > > > index 0000000..f2b84ad > > > > > --- /dev/null > > > > > +++ b/tests/xfs/437 > > > > > @@ -0,0 +1,73 @@ > > > > > +#! /bin/bash > > > > > +# FS QA Test No. 437 > > > > > +# > > > > > +# Regression test for commit: > > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > > +# > > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > +# the log. > > > > > +# > > > > > +#----------------------------------------------------------------------- > > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > > +# > > > > > +# This program is free software; you can redistribute it and/or > > > > > +# modify it under the terms of the GNU General Public License as > > > > > +# published by the Free Software Foundation. > > > > > +# > > > > > +# This program is distributed in the hope that it would be useful, > > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > > +# GNU General Public License for more details. > > > > > +# > > > > > +# You should have received a copy of the GNU General Public License > > > > > +# along with this program; if not, write the Free Software Foundation, > > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > > +#----------------------------------------------------------------------- > > > > > + > > > > > +seq=`basename "$0"` > > > > > +seqres="$RESULT_DIR/$seq" > > > > > +echo "QA output created by $seq" > > > > > + > > > > > +here=`pwd` > > > > > +tmp=/tmp/$$ > > > > > +status=1 # failure is the default! > > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > + > > > > > +_cleanup() > > > > > +{ > > > > > + rm -rf $tmp.* > > > > > +} > > > > > + > > > > > +# get standard environment and checks > > > > > +. ./common/rc > > > > > + > > > > > +# real QA test starts here > > > > > +_supported_os Linux > > > > > +_supported_fs xfs > > > > > +_require_scratch > > > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > > comments) too. > > > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > > > If it triggers debug asserts which have been put there to catch bugs > > > in other utilities (like mkfs) on a current upstream debug kernel, > > > then the test should not be part of the auto and quick groups. > > > > This test corrupts the filesystem on purpose (I didn't make this clear > > in my last reply, sorry about that), and xfs detects the corruption & > > refuses the mount instead of crashing. So I think the ASSERT failure is > > expected on debug build and we just need to ignore it. > > Except that after that assert, the block device is now busy and can't > be released, so we can't use the scratch device anymore. All tests > after this assert has been triggered are going to fail because > the block device is busy.... > > That's the whole point of adding debug asserts in cases like this - > they are supposed to stop test execution in it's tracks and leave a > corpse to analyse. The auto group regression tests are not supposed > to take the machine down on normal test configs (i.e. > CONFIG_XFS_DEBUG=y). > FWIW, my understanding of the dangerous group has always been that it's for tests that when they trigger a regression, forcibly affect the entire system as such (lockup, hang, crash, etc.). IMO, a test that intentionally generates an XFS assert doesn't really follow the spirit of that categorization, even though technically an assert can cause a BUG. This is why we have CONFIG_XFS_ASSERT_FATAL now, because unless that is disabled any auto test that reproduces a regression (assuming we have broad/robust assert coverage) can very easily BUG() and have the associated effect on the system. The flipside of course is that one should generally be able to run xfstests against a FATAL_ASSERTS=y kernel that otherwise has no regressions and not bork the system because a test generates a BUG() as part of a successful run. Perhaps the root problem here is that the kernel generates an assert from a codepath that "successfully" handles the erroneous situation. We already spit out several error messages and fail the mount, so my .02 (fwiw) would be to leave the test as auto+dangerous, delete the useless assert and let people affected by older kernels (w/ FATAL_ASSERT behavior) filter out the test as appropriate in those environments. (Looking back, I see Darrick already suggested to delete the assert as well...). > > And the test reproduces the crash quickly and reliably on buggy kernel > > and passes (if we ignore the ASSERT failure) on patched kernel, it > > serves as a good 'auto' and 'quick' regression test to me. > > Sure, I'm not saying that it doesn't work as a regression test. I'm > saying that it's a regression test that *should not be run by > everyone*. It's classified as dangerous which means - by definition > - it should not be part of the quick and auto groups.... > > If you want to test dangerous regression tests, then you need to > *opt-in* by adding "-g dangerous", not force everyone else to > opt-out by having to run "-g auto -x dangerous". > I often explicitly filter out the dangerous group as above because we already have more than a handful of tests that are in both groups. I don't think they've historically been mutually exclusive. Brian > 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 [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-15 12:45 ` [PATCH v2 2/2] " Brian Foster @ 2018-01-15 21:03 ` Dave Chinner 2018-01-16 4:02 ` Eryu Guan 2018-01-16 13:58 ` Brian Foster 0 siblings, 2 replies; 42+ messages in thread From: Dave Chinner @ 2018-01-15 21:03 UTC (permalink / raw) To: Brian Foster; +Cc: Eryu Guan, xiao yang, fstests, darrick.wong On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > > > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > > the log. > > > > > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > > > --- > > > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > tests/xfs/437.out | 2 ++ > > > > > > tests/xfs/group | 1 + > > > > > > 3 files changed, 76 insertions(+) > > > > > > create mode 100755 tests/xfs/437 > > > > > > create mode 100644 tests/xfs/437.out > > > > > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > > > new file mode 100755 > > > > > > index 0000000..f2b84ad > > > > > > --- /dev/null > > > > > > +++ b/tests/xfs/437 > > > > > > @@ -0,0 +1,73 @@ > > > > > > +#! /bin/bash > > > > > > +# FS QA Test No. 437 > > > > > > +# > > > > > > +# Regression test for commit: > > > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > > > +# > > > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > > +# the log. > > > > > > +# > > > > > > +#----------------------------------------------------------------------- > > > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > > > +# > > > > > > +# This program is free software; you can redistribute it and/or > > > > > > +# modify it under the terms of the GNU General Public License as > > > > > > +# published by the Free Software Foundation. > > > > > > +# > > > > > > +# This program is distributed in the hope that it would be useful, > > > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > > > +# GNU General Public License for more details. > > > > > > +# > > > > > > +# You should have received a copy of the GNU General Public License > > > > > > +# along with this program; if not, write the Free Software Foundation, > > > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > > > +#----------------------------------------------------------------------- > > > > > > + > > > > > > +seq=`basename "$0"` > > > > > > +seqres="$RESULT_DIR/$seq" > > > > > > +echo "QA output created by $seq" > > > > > > + > > > > > > +here=`pwd` > > > > > > +tmp=/tmp/$$ > > > > > > +status=1 # failure is the default! > > > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > > + > > > > > > +_cleanup() > > > > > > +{ > > > > > > + rm -rf $tmp.* > > > > > > +} > > > > > > + > > > > > > +# get standard environment and checks > > > > > > +. ./common/rc > > > > > > + > > > > > > +# real QA test starts here > > > > > > +_supported_os Linux > > > > > > +_supported_fs xfs > > > > > > +_require_scratch > > > > > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > > > comments) too. > > > > > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > > > > > If it triggers debug asserts which have been put there to catch bugs > > > > in other utilities (like mkfs) on a current upstream debug kernel, > > > > then the test should not be part of the auto and quick groups. > > > > > > This test corrupts the filesystem on purpose (I didn't make this clear > > > in my last reply, sorry about that), and xfs detects the corruption & > > > refuses the mount instead of crashing. So I think the ASSERT failure is > > > expected on debug build and we just need to ignore it. > > > > Except that after that assert, the block device is now busy and can't > > be released, so we can't use the scratch device anymore. All tests > > after this assert has been triggered are going to fail because > > the block device is busy.... > > > > That's the whole point of adding debug asserts in cases like this - > > they are supposed to stop test execution in it's tracks and leave a > > corpse to analyse. The auto group regression tests are not supposed > > to take the machine down on normal test configs (i.e. > > CONFIG_XFS_DEBUG=y). > > > > FWIW, my understanding of the dangerous group has always been that it's > for tests that when they trigger a regression, forcibly affect the > entire system as such (lockup, hang, crash, etc.). IMO, a test that > intentionally generates an XFS assert doesn't really follow the spirit > of that categorization, even though technically an assert can cause a > BUG. This is why we have CONFIG_XFS_ASSERT_FATAL now, because unless > that is disabled any auto test that reproduces a regression (assuming we > have broad/robust assert coverage) can very easily BUG() and have the > associated effect on the system. And this is exactly why I opposed the CONFIG_XFS_ASSERT_FATAL inclusion. It started us down the slippery slope of allowing people to ignore asserts that indicate an impossible condition had occurred. This was a bug found via fuzzing, not user problems in the wild. The assert had functioned perfectly well up to this point as a mechanism for preventing the XFS tools from creating a corrupt log stripe unit, and it still does.... That's the point here - a corrupt log stripe unit *should never happen* because it should trigger a CRC validation failure long before we get to parsing it. If we've got a corrupt value with a correct CRC, then we damn well need to understand the cause *immediately* because something else has gone very, very wrong. > The flipside of course is that one should generally be able to run > xfstests against a FATAL_ASSERTS=y kernel that otherwise has no > regressions and not bork the system because a test generates a BUG() as > part of a successful run. That is the historical behaviour of CONFIG_XFS_DEBUG=y and the historical policy for xfstests. Adding CONFIG_XFS_ASSERT_FATAL does not change that because (at least) some of use still treat asserts as fatal. > Perhaps the root problem here is that the > kernel generates an assert from a codepath that "successfully" handles > the erroneous situation. We do that all over the place - the assert is there because it indicates a fatal problem has occurred that should never happen. For production systems, we still have to handle that gracefully because people do fuzz testing of "should never happen" conditions and then get upset we fail to detect this. That does not mean the ASSERT is wrong - it makes it even more important that developers know when their code hits these cases.... If we really want to test these "should not ever happen" conditions that trigger asserts on debug kernels as "everyone always runs" regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y kernels. fstests already knows that it's running on a debug kernel, so adding a _requires_production_kernel check may be the way around this being considered a dangerous test. > We already spit out several error messages and > fail the mount, so my .02 (fwiw) would be to leave the test as > auto+dangerous, delete the useless assert and let people affected by > older kernels (w/ FATAL_ASSERT behavior) filter out the test as > appropriate in those environments. (Looking back, I see Darrick already > suggested to delete the assert as well...). ... and this specific assert caught multiple bugs I introduced while refactoring mkfs recently. That's it's purpose, and it works for that purpose very well. > > If you want to test dangerous regression tests, then you need to > > *opt-in* by adding "-g dangerous", not force everyone else to > > opt-out by having to run "-g auto -x dangerous". > > > > I often explicitly filter out the dangerous group as above because we > already have more than a handful of tests that are in both groups. I > don't think they've historically been mutually exclusive. Which means we've already deviated from the historic policy. That doesn't mean it the right direction to take. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-15 21:03 ` Dave Chinner @ 2018-01-16 4:02 ` Eryu Guan 2018-01-16 6:41 ` Xiao Yang ` (2 more replies) 2018-01-16 13:58 ` Brian Foster 1 sibling, 3 replies; 42+ messages in thread From: Eryu Guan @ 2018-01-16 4:02 UTC (permalink / raw) To: Dave Chinner, Brian Foster; +Cc: xiao yang, fstests, darrick.wong On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: > On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > > On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > > > > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > > > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: > > > > > > > If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > > > the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > > > the log. > > > > > > > > > > > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > > > > > > --- > > > > > > > tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > tests/xfs/437.out | 2 ++ > > > > > > > tests/xfs/group | 1 + > > > > > > > 3 files changed, 76 insertions(+) > > > > > > > create mode 100755 tests/xfs/437 > > > > > > > create mode 100644 tests/xfs/437.out > > > > > > > > > > > > > > diff --git a/tests/xfs/437 b/tests/xfs/437 > > > > > > > new file mode 100755 > > > > > > > index 0000000..f2b84ad > > > > > > > --- /dev/null > > > > > > > +++ b/tests/xfs/437 > > > > > > > @@ -0,0 +1,73 @@ > > > > > > > +#! /bin/bash > > > > > > > +# FS QA Test No. 437 > > > > > > > +# > > > > > > > +# Regression test for commit: > > > > > > > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > > > > > > > +# > > > > > > > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > > > > > > > +# the invalid sb_logsunit leads to crash as soon as we try to write to > > > > > > > +# the log. > > > > > > > +# > > > > > > > +#----------------------------------------------------------------------- > > > > > > > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > > > > > > > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > > > > > > > +# > > > > > > > +# This program is free software; you can redistribute it and/or > > > > > > > +# modify it under the terms of the GNU General Public License as > > > > > > > +# published by the Free Software Foundation. > > > > > > > +# > > > > > > > +# This program is distributed in the hope that it would be useful, > > > > > > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > > > > > > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > > > > > +# GNU General Public License for more details. > > > > > > > +# > > > > > > > +# You should have received a copy of the GNU General Public License > > > > > > > +# along with this program; if not, write the Free Software Foundation, > > > > > > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > > > > > > > +#----------------------------------------------------------------------- > > > > > > > + > > > > > > > +seq=`basename "$0"` > > > > > > > +seqres="$RESULT_DIR/$seq" > > > > > > > +echo "QA output created by $seq" > > > > > > > + > > > > > > > +here=`pwd` > > > > > > > +tmp=/tmp/$$ > > > > > > > +status=1 # failure is the default! > > > > > > > +trap "_cleanup; exit \$status" 0 1 2 3 15 > > > > > > > + > > > > > > > +_cleanup() > > > > > > > +{ > > > > > > > + rm -rf $tmp.* > > > > > > > +} > > > > > > > + > > > > > > > +# get standard environment and checks > > > > > > > +. ./common/rc > > > > > > > + > > > > > > > +# real QA test starts here > > > > > > > +_supported_os Linux > > > > > > > +_supported_fs xfs > > > > > > > +_require_scratch > > > > > > > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > > > > comments) too. > > > > > > > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > > > > > > > If it triggers debug asserts which have been put there to catch bugs > > > > > in other utilities (like mkfs) on a current upstream debug kernel, > > > > > then the test should not be part of the auto and quick groups. > > > > > > > > This test corrupts the filesystem on purpose (I didn't make this clear > > > > in my last reply, sorry about that), and xfs detects the corruption & > > > > refuses the mount instead of crashing. So I think the ASSERT failure is > > > > expected on debug build and we just need to ignore it. > > > > > > Except that after that assert, the block device is now busy and can't > > > be released, so we can't use the scratch device anymore. All tests > > > after this assert has been triggered are going to fail because > > > the block device is busy.... > > > > > > That's the whole point of adding debug asserts in cases like this - > > > they are supposed to stop test execution in it's tracks and leave a > > > corpse to analyse. The auto group regression tests are not supposed > > > to take the machine down on normal test configs (i.e. > > > CONFIG_XFS_DEBUG=y). > > > > > > > FWIW, my understanding of the dangerous group has always been that it's > > for tests that when they trigger a regression, forcibly affect the > > entire system as such (lockup, hang, crash, etc.). IMO, a test that I had the same understanding of dangerous group. And I recommended the usage of "-g auto -x dangerous" before[1], and Dave acknowledged this dangerous group usage[2] :) [1] https://www.spinics.net/lists/linux-btrfs/msg57312.html [2] https://www.spinics.net/lists/linux-btrfs/msg57330.html > > intentionally generates an XFS assert doesn't really follow the spirit > > of that categorization, even though technically an assert can cause a > > BUG. This is why we have CONFIG_XFS_ASSERT_FATAL now, because unless > > that is disabled any auto test that reproduces a regression (assuming we > > have broad/robust assert coverage) can very easily BUG() and have the > > associated effect on the system. > > And this is exactly why I opposed the CONFIG_XFS_ASSERT_FATAL > inclusion. It started us down the slippery slope of allowing people > to ignore asserts that indicate an impossible condition had > occurred. > > This was a bug found via fuzzing, not user problems in the wild. The > assert had functioned perfectly well up to this point as a mechanism > for preventing the XFS tools from creating a corrupt log stripe > unit, and it still does.... > > That's the point here - a corrupt log stripe unit *should never > happen* because it should trigger a CRC validation failure long > before we get to parsing it. If we've got a corrupt value with a > correct CRC, then we damn well need to understand the cause > *immediately* because something else has gone very, very wrong. > > > The flipside of course is that one should generally be able to run > > xfstests against a FATAL_ASSERTS=y kernel that otherwise has no > > regressions and not bork the system because a test generates a BUG() as > > part of a successful run. > > That is the historical behaviour of CONFIG_XFS_DEBUG=y and the > historical policy for xfstests. Adding CONFIG_XFS_ASSERT_FATAL does > not change that because (at least) some of use still treat asserts > as fatal. > > > Perhaps the root problem here is that the > > kernel generates an assert from a codepath that "successfully" handles > > the erroneous situation. > > We do that all over the place - the assert is there because it > indicates a fatal problem has occurred that should never happen. > For production systems, we still have to handle that gracefully > because people do fuzz testing of "should never happen" conditions > and then get upset we fail to detect this. That does not mean > the ASSERT is wrong - it makes it even more important that > developers know when their code hits these cases.... > > If we really want to test these "should not ever happen" conditions > that trigger asserts on debug kernels as "everyone always runs" > regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y > kernels. fstests already knows that it's running on a debug kernel, > so adding a _requires_production_kernel check may be the way around > this being considered a dangerous test. This reminds that we already have a _require_no_xfs_debug rule, as used in xfs/115, which is known to trigger ASSERT failure on debug build. So we can do the same in this new test. Thanks, Eryu > > > We already spit out several error messages and > > fail the mount, so my .02 (fwiw) would be to leave the test as > > auto+dangerous, delete the useless assert and let people affected by > > older kernels (w/ FATAL_ASSERT behavior) filter out the test as > > appropriate in those environments. (Looking back, I see Darrick already > > suggested to delete the assert as well...). > > ... and this specific assert caught multiple bugs I introduced while > refactoring mkfs recently. That's it's purpose, and it works for > that purpose very well. > > > > If you want to test dangerous regression tests, then you need to > > > *opt-in* by adding "-g dangerous", not force everyone else to > > > opt-out by having to run "-g auto -x dangerous". > > > > > > > I often explicitly filter out the dangerous group as above because we > > already have more than a handful of tests that are in both groups. I > > don't think they've historically been mutually exclusive. > > Which means we've already deviated from the historic policy. That > doesn't mean it the right direction to take. > > 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 [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-16 4:02 ` Eryu Guan @ 2018-01-16 6:41 ` Xiao Yang 2018-01-16 7:26 ` [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-16 8:50 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit Dave Chinner 2 siblings, 0 replies; 42+ messages in thread From: Xiao Yang @ 2018-01-16 6:41 UTC (permalink / raw) To: Eryu Guan; +Cc: Dave Chinner, Brian Foster, fstests, darrick.wong On 2018/01/16 12:02, Eryu Guan wrote: > On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: >> On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: >>> On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: >>>> On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: >>>>> On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: >>>>>> On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: >>>>>>> On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: >>>>>>>> If log stripe unit isn't a multiple of the fs blocksize and mounting, >>>>>>>> the invalid sb_logsunit leads to crash as soon as we try to write to >>>>>>>> the log. >>>>>>>> >>>>>>>> Signed-off-by: xiao yang<yangx.jy@cn.fujitsu.com> >>>>>>>> --- >>>>>>>> tests/xfs/437 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> tests/xfs/437.out | 2 ++ >>>>>>>> tests/xfs/group | 1 + >>>>>>>> 3 files changed, 76 insertions(+) >>>>>>>> create mode 100755 tests/xfs/437 >>>>>>>> create mode 100644 tests/xfs/437.out >>>>>>>> >>>>>>>> diff --git a/tests/xfs/437 b/tests/xfs/437 >>>>>>>> new file mode 100755 >>>>>>>> index 0000000..f2b84ad >>>>>>>> --- /dev/null >>>>>>>> +++ b/tests/xfs/437 >>>>>>>> @@ -0,0 +1,73 @@ >>>>>>>> +#! /bin/bash >>>>>>>> +# FS QA Test No. 437 >>>>>>>> +# >>>>>>>> +# Regression test for commit: >>>>>>>> +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") >>>>>>>> +# >>>>>>>> +# If log stripe unit isn't a multiple of the fs blocksize and mounting, >>>>>>>> +# the invalid sb_logsunit leads to crash as soon as we try to write to >>>>>>>> +# the log. >>>>>>>> +# >>>>>>>> +#----------------------------------------------------------------------- >>>>>>>> +# Copyright (c) 2018 FUJITSU. All Rights Reserved. >>>>>>>> +# Author: Xiao Yang<yangx.jy@cn.fujitsu.com> >>>>>>>> +# >>>>>>>> +# This program is free software; you can redistribute it and/or >>>>>>>> +# modify it under the terms of the GNU General Public License as >>>>>>>> +# published by the Free Software Foundation. >>>>>>>> +# >>>>>>>> +# This program is distributed in the hope that it would be useful, >>>>>>>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >>>>>>>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>>>>>> +# GNU General Public License for more details. >>>>>>>> +# >>>>>>>> +# You should have received a copy of the GNU General Public License >>>>>>>> +# along with this program; if not, write the Free Software Foundation, >>>>>>>> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA >>>>>>>> +#----------------------------------------------------------------------- >>>>>>>> + >>>>>>>> +seq=`basename "$0"` >>>>>>>> +seqres="$RESULT_DIR/$seq" >>>>>>>> +echo "QA output created by $seq" >>>>>>>> + >>>>>>>> +here=`pwd` >>>>>>>> +tmp=/tmp/$$ >>>>>>>> +status=1 # failure is the default! >>>>>>>> +trap "_cleanup; exit \$status" 0 1 2 3 15 >>>>>>>> + >>>>>>>> +_cleanup() >>>>>>>> +{ >>>>>>>> + rm -rf $tmp.* >>>>>>>> +} >>>>>>>> + >>>>>>>> +# get standard environment and checks >>>>>>>> +. ./common/rc >>>>>>>> + >>>>>>>> +# real QA test starts here >>>>>>>> +_supported_os Linux >>>>>>>> +_supported_fs xfs >>>>>>>> +_require_scratch >>>>>>> This test triggers ASSERT failure and warning on debug build, thus >>>>>>> failed _dmesg_check, I think we need _disable_dmesg_check (and some >>>>>>> comments) too. >>>>>>> >>>>>>> [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size >>>>>>> [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! >>>>>>> [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 >>>>>>> [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] >>>>>> If it triggers debug asserts which have been put there to catch bugs >>>>>> in other utilities (like mkfs) on a current upstream debug kernel, >>>>>> then the test should not be part of the auto and quick groups. >>>>> This test corrupts the filesystem on purpose (I didn't make this clear >>>>> in my last reply, sorry about that), and xfs detects the corruption& >>>>> refuses the mount instead of crashing. So I think the ASSERT failure is >>>>> expected on debug build and we just need to ignore it. >>>> Except that after that assert, the block device is now busy and can't >>>> be released, so we can't use the scratch device anymore. All tests >>>> after this assert has been triggered are going to fail because >>>> the block device is busy.... >>>> >>>> That's the whole point of adding debug asserts in cases like this - >>>> they are supposed to stop test execution in it's tracks and leave a >>>> corpse to analyse. The auto group regression tests are not supposed >>>> to take the machine down on normal test configs (i.e. >>>> CONFIG_XFS_DEBUG=y). >>>> >>> FWIW, my understanding of the dangerous group has always been that it's >>> for tests that when they trigger a regression, forcibly affect the >>> entire system as such (lockup, hang, crash, etc.). IMO, a test that > I had the same understanding of dangerous group. And I recommended the > usage of "-g auto -x dangerous" before[1], and Dave acknowledged this > dangerous group usage[2] :) > > [1] https://www.spinics.net/lists/linux-btrfs/msg57312.html > [2] https://www.spinics.net/lists/linux-btrfs/msg57330.html > >>> intentionally generates an XFS assert doesn't really follow the spirit >>> of that categorization, even though technically an assert can cause a >>> BUG. This is why we have CONFIG_XFS_ASSERT_FATAL now, because unless >>> that is disabled any auto test that reproduces a regression (assuming we >>> have broad/robust assert coverage) can very easily BUG() and have the >>> associated effect on the system. >> And this is exactly why I opposed the CONFIG_XFS_ASSERT_FATAL >> inclusion. It started us down the slippery slope of allowing people >> to ignore asserts that indicate an impossible condition had >> occurred. >> >> This was a bug found via fuzzing, not user problems in the wild. The >> assert had functioned perfectly well up to this point as a mechanism >> for preventing the XFS tools from creating a corrupt log stripe >> unit, and it still does.... >> >> That's the point here - a corrupt log stripe unit *should never >> happen* because it should trigger a CRC validation failure long >> before we get to parsing it. If we've got a corrupt value with a >> correct CRC, then we damn well need to understand the cause >> *immediately* because something else has gone very, very wrong. >> >>> The flipside of course is that one should generally be able to run >>> xfstests against a FATAL_ASSERTS=y kernel that otherwise has no >>> regressions and not bork the system because a test generates a BUG() as >>> part of a successful run. >> That is the historical behaviour of CONFIG_XFS_DEBUG=y and the >> historical policy for xfstests. Adding CONFIG_XFS_ASSERT_FATAL does >> not change that because (at least) some of use still treat asserts >> as fatal. >> >>> Perhaps the root problem here is that the >>> kernel generates an assert from a codepath that "successfully" handles >>> the erroneous situation. >> We do that all over the place - the assert is there because it >> indicates a fatal problem has occurred that should never happen. >> For production systems, we still have to handle that gracefully >> because people do fuzz testing of "should never happen" conditions >> and then get upset we fail to detect this. That does not mean >> the ASSERT is wrong - it makes it even more important that >> developers know when their code hits these cases.... >> >> If we really want to test these "should not ever happen" conditions >> that trigger asserts on debug kernels as "everyone always runs" >> regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y >> kernels. fstests already knows that it's running on a debug kernel, >> so adding a _requires_production_kernel check may be the way around >> this being considered a dangerous test. > This reminds that we already have a _require_no_xfs_debug rule, as used > in xfs/115, which is known to trigger ASSERT failure on debug build. So > we can do the same in this new test. Agreed. This new test can be skipped by _require_no_xfs_debug rule on debug build, and is considered as a 'auto' and 'quick' regression test, so i wil send v4 patch in this way. Thanks, Xiao Yang > Thanks, > Eryu > >>> We already spit out several error messages and >>> fail the mount, so my .02 (fwiw) would be to leave the test as >>> auto+dangerous, delete the useless assert and let people affected by >>> older kernels (w/ FATAL_ASSERT behavior) filter out the test as >>> appropriate in those environments. (Looking back, I see Darrick already >>> suggested to delete the assert as well...). >> ... and this specific assert caught multiple bugs I introduced while >> refactoring mkfs recently. That's it's purpose, and it works for >> that purpose very well. >> >>>> If you want to test dangerous regression tests, then you need to >>>> *opt-in* by adding "-g dangerous", not force everyone else to >>>> opt-out by having to run "-g auto -x dangerous". >>>> >>> I often explicitly filter out the dangerous group as above because we >>> already have more than a handful of tests that are in both groups. I >>> don't think they've historically been mutually exclusive. >> Which means we've already deviated from the historic policy. That >> doesn't mean it the right direction to take. >> >> 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 [flat|nested] 42+ messages in thread
* [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-16 4:02 ` Eryu Guan 2018-01-16 6:41 ` Xiao Yang @ 2018-01-16 7:26 ` xiao yang 2018-01-16 7:26 ` [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount xiao yang 2018-01-16 7:26 ` [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-16 8:50 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit Dave Chinner 2 siblings, 2 replies; 42+ messages in thread From: xiao yang @ 2018-01-16 7:26 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang Make sure _scratch_xfs_set_metadata_field() can be used on an old xfsprogs-dev(e.g. v3.1.1). The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). The special argument "--" is only used to end option-scanning in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/xfs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/common/xfs b/common/xfs index 45b84a0..ab58364 100644 --- a/common/xfs +++ b/common/xfs @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() for arg in "$@"; do cmds+=("-c" "${arg}") done - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" - echo + + local wr_cmd="write" + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- ${value}" + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" } _scratch_xfs_get_sb_field() -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount 2018-01-16 7:26 ` [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang @ 2018-01-16 7:26 ` xiao yang 2018-01-18 8:48 ` Eryu Guan 2018-01-16 7:26 ` [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 1 sibling, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-16 7:26 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang 1) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename it as _filter_assert_dmesg. 2) Add _require_no_xfs_debug helper to skip xfs/098 if it is tested on debug build. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/filter | 9 +++++++++ tests/xfs/098 | 18 ++++++------------ 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/common/filter b/common/filter index 8e1fdb4..8f90b01 100644 --- a/common/filter +++ b/common/filter @@ -570,5 +570,14 @@ _filter_aiodio_dmesg() -e "s#$warn9#Intentional warnings in dio_complete#" } +# We generate assert WARNINGs on CONFIG_XFS_WARN build as expected and make +# sure test doesn't fail because of these warnings. This is a helper for +# _check_dmesg() to filter out them. +_filter_assert_dmesg() +{ + local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" + sed -e "s#$warn#Intentional warnings in asswarn#" +} + # make sure this script returns success /bin/true diff --git a/tests/xfs/098 b/tests/xfs/098 index 9bcd94b..f924b32 100755 --- a/tests/xfs/098 +++ b/tests/xfs/098 @@ -47,6 +47,9 @@ _cleanup() _supported_fs xfs _supported_os Linux +# We corrupt XFS on purpose, and debug built XFS would crash due to +# assert failure, so skip if we're testing on a debug built XFS +_require_no_xfs_debug _require_scratch test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc _require_attrs @@ -56,16 +59,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" rm -f $seqres.full -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check -# which could miss other potential bugs, so filter out the intentional WARNINGs, -# make sure test doesn't fail because of this warning and fails on other WARNINGs. -filter_xfs_dmesg() -{ - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" - sed -e "s#$warn#Intentional warnings in asswarn#" -} - TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" @@ -107,8 +100,9 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" echo "+ repair fs" _repair_scratch_fs >> $seqres.full 2>&1 -# mount may trigger related WARNINGs, so filter them. -_check_dmesg filter_xfs_dmesg +# We may trigger mounted related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN build, so filter them. +_check_dmesg _filter_assert_dmesg echo "+ mount image (2)" _scratch_mount -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount 2018-01-16 7:26 ` [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount xiao yang @ 2018-01-18 8:48 ` Eryu Guan 2018-01-18 8:56 ` Xiao Yang 0 siblings, 1 reply; 42+ messages in thread From: Eryu Guan @ 2018-01-18 8:48 UTC (permalink / raw) To: xiao yang; +Cc: david, bfoster, fstests, darrick.wong On Tue, Jan 16, 2018 at 03:26:56PM +0800, xiao yang wrote: > 1) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename > it as _filter_assert_dmesg. > > 2) Add _require_no_xfs_debug helper to skip xfs/098 if it is tested > on debug build. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > common/filter | 9 +++++++++ > tests/xfs/098 | 18 ++++++------------ > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/common/filter b/common/filter > index 8e1fdb4..8f90b01 100644 > --- a/common/filter > +++ b/common/filter > @@ -570,5 +570,14 @@ _filter_aiodio_dmesg() > -e "s#$warn9#Intentional warnings in dio_complete#" > } > > +# We generate assert WARNINGs on CONFIG_XFS_WARN build as expected and make > +# sure test doesn't fail because of these warnings. This is a helper for > +# _check_dmesg() to filter out them. > +_filter_assert_dmesg() > +{ > + local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" When bug_on_assert=0, the "asswarn" can be "assfail" too, we should filter out both cases. > + sed -e "s#$warn#Intentional warnings in asswarn#" > +} > + > # make sure this script returns success > /bin/true > diff --git a/tests/xfs/098 b/tests/xfs/098 > index 9bcd94b..f924b32 100755 > --- a/tests/xfs/098 > +++ b/tests/xfs/098 > @@ -47,6 +47,9 @@ _cleanup() > _supported_fs xfs > _supported_os Linux > > +# We corrupt XFS on purpose, and debug built XFS would crash due to > +# assert failure, so skip if we're testing on a debug built XFS > +_require_no_xfs_debug Same here, _require_no_xfs_bug_on_assert if we all agree to do so. Thanks, Eryu > _require_scratch > test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc > _require_attrs > @@ -56,16 +59,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" > > rm -f $seqres.full > > -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related > -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check > -# which could miss other potential bugs, so filter out the intentional WARNINGs, > -# make sure test doesn't fail because of this warning and fails on other WARNINGs. > -filter_xfs_dmesg() > -{ > - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" > - sed -e "s#$warn#Intentional warnings in asswarn#" > -} > - > TESTDIR="${SCRATCH_MNT}/scratchdir" > TESTFILE="${TESTDIR}/testfile" > > @@ -107,8 +100,9 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" > echo "+ repair fs" > _repair_scratch_fs >> $seqres.full 2>&1 > > -# mount may trigger related WARNINGs, so filter them. > -_check_dmesg filter_xfs_dmesg > +# We may trigger mounted related WARNINGs if we corrupt log on a > +# CONFIG_XFS_WARN build, so filter them. > +_check_dmesg _filter_assert_dmesg > > echo "+ mount image (2)" > _scratch_mount > -- > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount 2018-01-18 8:48 ` Eryu Guan @ 2018-01-18 8:56 ` Xiao Yang 0 siblings, 0 replies; 42+ messages in thread From: Xiao Yang @ 2018-01-18 8:56 UTC (permalink / raw) To: Eryu Guan; +Cc: david, bfoster, fstests, darrick.wong On 2018/01/18 16:48, Eryu Guan wrote: > On Tue, Jan 16, 2018 at 03:26:56PM +0800, xiao yang wrote: >> 1) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename >> it as _filter_assert_dmesg. >> >> 2) Add _require_no_xfs_debug helper to skip xfs/098 if it is tested >> on debug build. >> >> Signed-off-by: xiao yang<yangx.jy@cn.fujitsu.com> >> --- >> common/filter | 9 +++++++++ >> tests/xfs/098 | 18 ++++++------------ >> 2 files changed, 15 insertions(+), 12 deletions(-) >> >> diff --git a/common/filter b/common/filter >> index 8e1fdb4..8f90b01 100644 >> --- a/common/filter >> +++ b/common/filter >> @@ -570,5 +570,14 @@ _filter_aiodio_dmesg() >> -e "s#$warn9#Intentional warnings in dio_complete#" >> } >> >> +# We generate assert WARNINGs on CONFIG_XFS_WARN build as expected and make >> +# sure test doesn't fail because of these warnings. This is a helper for >> +# _check_dmesg() to filter out them. >> +_filter_assert_dmesg() >> +{ >> + local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" > When bug_on_assert=0, the "asswarn" can be "assfail" too, we should > filter out both cases. Hi Eryu, Agreed. >> + sed -e "s#$warn#Intentional warnings in asswarn#" >> +} >> + >> # make sure this script returns success >> /bin/true >> diff --git a/tests/xfs/098 b/tests/xfs/098 >> index 9bcd94b..f924b32 100755 >> --- a/tests/xfs/098 >> +++ b/tests/xfs/098 >> @@ -47,6 +47,9 @@ _cleanup() >> _supported_fs xfs >> _supported_os Linux >> >> +# We corrupt XFS on purpose, and debug built XFS would crash due to >> +# assert failure, so skip if we're testing on a debug built XFS >> +_require_no_xfs_debug > Same here, _require_no_xfs_bug_on_assert if we all agree to do so. OK, i will add a new _require_no_xfs_bug_on_assert and update the whole patch set. Thanks, Xiao Yang. > Thanks, > Eryu > >> _require_scratch >> test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc >> _require_attrs >> @@ -56,16 +59,6 @@ test -z "${FUZZ_ARGS}"&& FUZZ_ARGS="-n 8 -3" >> >> rm -f $seqres.full >> >> -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related >> -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check >> -# which could miss other potential bugs, so filter out the intentional WARNINGs, >> -# make sure test doesn't fail because of this warning and fails on other WARNINGs. >> -filter_xfs_dmesg() >> -{ >> - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" >> - sed -e "s#$warn#Intentional warnings in asswarn#" >> -} >> - >> TESTDIR="${SCRATCH_MNT}/scratchdir" >> TESTFILE="${TESTDIR}/testfile" >> >> @@ -107,8 +100,9 @@ _scratch_mount 2>/dev/null&& _fail "mount should not succeed" >> echo "+ repair fs" >> _repair_scratch_fs>> $seqres.full 2>&1 >> >> -# mount may trigger related WARNINGs, so filter them. >> -_check_dmesg filter_xfs_dmesg >> +# We may trigger mounted related WARNINGs if we corrupt log on a >> +# CONFIG_XFS_WARN build, so filter them. >> +_check_dmesg _filter_assert_dmesg >> >> echo "+ mount image (2)" >> _scratch_mount >> -- >> 1.8.3.1 >> >> >> > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit 2018-01-16 7:26 ` [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-16 7:26 ` [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount xiao yang @ 2018-01-16 7:26 ` xiao yang 2018-01-18 8:46 ` Eryu Guan 1 sibling, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-16 7:26 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/439 | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/439.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 89 insertions(+) create mode 100755 tests/xfs/439 create mode 100644 tests/xfs/439.out diff --git a/tests/xfs/439 b/tests/xfs/439 new file mode 100755 index 0000000..5dfed1a --- /dev/null +++ b/tests/xfs/439 @@ -0,0 +1,86 @@ +#! /bin/bash +# FS QA Test No. 439 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch +# We corrupt XFS on purpose, and debug built XFS would crash due to +# assert failure, so skip if we're testing on a debug built XFS +_require_no_xfs_debug + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 + +# Check if logsunit is set correctly +lsunit=$(_scratch_xfs_get_sb_field logsunit) +[ $lsunit -ne $((blksz - 1)) ] && _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash on buggy kernel +# The fix applied kernel refuses to mount, so a mount failure is +# expected +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + echo > ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +# Mount may trigger ASSERT failure and warnings on the fix applied +# kernel if logsunit is invalid on CONFIG_XFS_WARN build, so filter them. +_check_dmesg _filter_assert_dmesg + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/439.out b/tests/xfs/439.out new file mode 100644 index 0000000..9712295 --- /dev/null +++ b/tests/xfs/439.out @@ -0,0 +1,2 @@ +QA output created by 439 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index 04a63b7..0b281a8 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -436,3 +436,4 @@ 436 auto quick clone fsr 437 auto quick other 438 auto quick quota dangerous +439 auto quick dangerous_fuzzers log -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit 2018-01-16 7:26 ` [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit xiao yang @ 2018-01-18 8:46 ` Eryu Guan 2018-01-18 10:49 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 0 siblings, 1 reply; 42+ messages in thread From: Eryu Guan @ 2018-01-18 8:46 UTC (permalink / raw) To: xiao yang; +Cc: david, bfoster, fstests, darrick.wong On Tue, Jan 16, 2018 at 03:26:57PM +0800, xiao yang wrote: > If log stripe unit isn't a multiple of the fs blocksize and mounting, > the invalid sb_logsunit leads to crash as soon as we try to write to > the log. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > tests/xfs/439 | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/xfs/439.out | 2 ++ > tests/xfs/group | 1 + > 3 files changed, 89 insertions(+) > create mode 100755 tests/xfs/439 > create mode 100644 tests/xfs/439.out > > diff --git a/tests/xfs/439 b/tests/xfs/439 > new file mode 100755 > index 0000000..5dfed1a > --- /dev/null > +++ b/tests/xfs/439 > @@ -0,0 +1,86 @@ > +#! /bin/bash > +# FS QA Test No. 439 > +# > +# Regression test for commit: > +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") > +# > +# If log stripe unit isn't a multiple of the fs blocksize and mounting, > +# the invalid sb_logsunit leads to crash as soon as we try to write to > +# the log. > +# > +#----------------------------------------------------------------------- > +# Copyright (c) 2018 FUJITSU. All Rights Reserved. > +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> > +# > +# This program is free software; you can redistribute it and/or > +# modify it under the terms of the GNU General Public License as > +# published by the Free Software Foundation. > +# > +# This program is distributed in the hope that it would be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program; if not, write the Free Software Foundation, > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA > +#----------------------------------------------------------------------- > + > +seq=`basename "$0"` > +seqres="$RESULT_DIR/$seq" > +echo "QA output created by $seq" > + > +here=`pwd` > +tmp=/tmp/$$ > +status=1 # failure is the default! > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +_cleanup() > +{ > + rm -rf $tmp.* > +} > + > +# get standard environment, filters and checks > +. ./common/rc > +. ./common/filter > + > +# real QA test starts here > +_supported_os Linux > +_supported_fs xfs > +_require_scratch > +# We corrupt XFS on purpose, and debug built XFS would crash due to > +# assert failure, so skip if we're testing on a debug built XFS > +_require_no_xfs_debug If we agree to introduce a new _require_no_xfs_bug_on_assert, the comments and require rule here can be updated too. > + > +rm -f "$seqres.full" > + > +# Format > +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" > + > +# Set logsunit to a value which is not a multiple of the fs blocksize > +blksz=$(_scratch_xfs_get_sb_field blocksize) > +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 > + > +# Check if logsunit is set correctly > +lsunit=$(_scratch_xfs_get_sb_field logsunit) > +[ $lsunit -ne $((blksz - 1)) ] && _notrun "failed to set sb_logsunit" > + > +# Mount and writing log may trigger a crash on buggy kernel > +# The fix applied kernel refuses to mount, so a mount failure is > +# expected > +if _scratch_mount >> $seqres.full 2>&1; then > + for i in $(seq 1 1000); do > + echo > ${SCRATCH_MNT}/$i > + done > + _scratch_unmount > +fi > + > +# Mount may trigger ASSERT failure and warnings on the fix applied > +# kernel if logsunit is invalid on CONFIG_XFS_WARN build, so filter them. and when bug_on_assert=0 :) Thanks, Eryu > +_check_dmesg _filter_assert_dmesg > + > +echo "Silence is golden" > + > +# success, all done > +status=0 > +exit > diff --git a/tests/xfs/439.out b/tests/xfs/439.out > new file mode 100644 > index 0000000..9712295 > --- /dev/null > +++ b/tests/xfs/439.out > @@ -0,0 +1,2 @@ > +QA output created by 439 > +Silence is golden > diff --git a/tests/xfs/group b/tests/xfs/group > index 04a63b7..0b281a8 100644 > --- a/tests/xfs/group > +++ b/tests/xfs/group > @@ -436,3 +436,4 @@ > 436 auto quick clone fsr > 437 auto quick other > 438 auto quick quota dangerous > +439 auto quick dangerous_fuzzers log > -- > 1.8.3.1 > > > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-18 8:46 ` Eryu Guan @ 2018-01-18 10:49 ` xiao yang 2018-01-18 10:49 ` [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert xiao yang ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: xiao yang @ 2018-01-18 10:49 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang Make sure _scratch_xfs_set_metadata_field() can be used on an old xfsprogs-dev(e.g. v3.1.1). The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). The special argument "--" is only used to end option-scanning in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/xfs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/common/xfs b/common/xfs index 45b84a0..ab58364 100644 --- a/common/xfs +++ b/common/xfs @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() for arg in "$@"; do cmds+=("-c" "${arg}") done - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" - echo + + local wr_cmd="write" + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- ${value}" + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" } _scratch_xfs_get_sb_field() -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert 2018-01-18 10:49 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang @ 2018-01-18 10:49 ` xiao yang 2018-01-18 18:29 ` Darrick J. Wong 2018-01-18 10:49 ` [PATCH v5 " xiao yang 2018-01-18 18:19 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db Darrick J. Wong 2 siblings, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-18 10:49 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang 1) Introduce _require_no_xfs_bug_on_assert helper to check if XFS is built with CONFIG_XFS_ASSERT_FATAL, and just call _require_no_xfs_debug if /sys/fs/xfs/debug/bug_on_assert is not available. 2) Apply _require_no_xfs_bug_on_assert in xfs/098 and xfs/115. 3) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename it as _filter_assert_dmesg. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/filter | 11 +++++++++++ common/xfs | 12 ++++++++++++ tests/xfs/098 | 20 ++++++++------------ tests/xfs/115 | 7 ++++--- 4 files changed, 35 insertions(+), 15 deletions(-) diff --git a/common/filter b/common/filter index 8e1fdb4..53874a0 100644 --- a/common/filter +++ b/common/filter @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() -e "s#$warn9#Intentional warnings in dio_complete#" } +# We generate assert related WARNINGs on purpose and make sure test doesn't fail +# because of these warnings. This is a helper for _check_dmesg() to filter out +# them. +_filter_assert_dmesg() +{ + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" + sed -e "s#$warn1#Intentional warnings in asswarn#" \ + -e "s#$warn2#Intentional warnings in assfail#" +} + # make sure this script returns success /bin/true diff --git a/common/xfs b/common/xfs index ab58364..a0e03b6 100644 --- a/common/xfs +++ b/common/xfs @@ -625,6 +625,18 @@ _require_no_xfs_debug() fi } +# Check if XFS is built with CONFIG_XFS_ASSERT_FATAL +_require_no_xfs_bug_on_assert() +{ + if [ -f /sys/fs/xfs/debug/bug_on_assert ]; then + grep -q "1" /sys/fs/xfs/debug/bug_on_assert && \ + _notrun "Require XFS built without CONFIG_XFS_ASSERT_FATAL" + else + # Call _require_no_xfs_debug if bug_on_assert is not available + _require_no_xfs_debug + fi +} + # Get a metadata field # The first arg is the field name # The rest of the arguments are xfs_db commands to find the metadata. diff --git a/tests/xfs/098 b/tests/xfs/098 index 9bcd94b..ebb7ca7 100755 --- a/tests/xfs/098 +++ b/tests/xfs/098 @@ -47,6 +47,10 @@ _cleanup() _supported_fs xfs _supported_os Linux +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS +# would crash due to assert failure, so skip if we're testing on a +# CONFIG_XFS_ASSERT_FATAL built XFS. +_require_no_xfs_bug_on_assert _require_scratch test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc _require_attrs @@ -56,16 +60,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" rm -f $seqres.full -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check -# which could miss other potential bugs, so filter out the intentional WARNINGs, -# make sure test doesn't fail because of this warning and fails on other WARNINGs. -filter_xfs_dmesg() -{ - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" - sed -e "s#$warn#Intentional warnings in asswarn#" -} - TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" @@ -107,8 +101,10 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" echo "+ repair fs" _repair_scratch_fs >> $seqres.full 2>&1 -# mount may trigger related WARNINGs, so filter them. -_check_dmesg filter_xfs_dmesg +# We may trigger assert related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) +# build, so filter them. +_check_dmesg _filter_assert_dmesg echo "+ mount image (2)" _scratch_mount diff --git a/tests/xfs/115 b/tests/xfs/115 index 0e62628..5fe1fd5 100755 --- a/tests/xfs/115 +++ b/tests/xfs/115 @@ -52,9 +52,10 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch_nocheck -# we corrupt XFS on purpose, and debug built XFS would crash due to assert -# failure, so skip if we're testing on a debug built XFS -_require_no_xfs_debug +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS +# would crash due to assert failure, so skip if we're testing on a +# CONFIG_XFS_ASSERT_FATAL built XFS. +_require_no_xfs_bug_on_assert _disable_dmesg_check # Make sure we disable finobt if the filesystem supports it, otherwise, just -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert 2018-01-18 10:49 ` [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert xiao yang @ 2018-01-18 18:29 ` Darrick J. Wong 2018-01-19 2:51 ` Eryu Guan ` (2 more replies) 0 siblings, 3 replies; 42+ messages in thread From: Darrick J. Wong @ 2018-01-18 18:29 UTC (permalink / raw) To: xiao yang; +Cc: eguan, david, bfoster, fstests On Thu, Jan 18, 2018 at 06:49:54PM +0800, xiao yang wrote: > 1) Introduce _require_no_xfs_bug_on_assert helper to check if XFS is built > with CONFIG_XFS_ASSERT_FATAL, and just call _require_no_xfs_debug if > /sys/fs/xfs/debug/bug_on_assert is not available. > > 2) Apply _require_no_xfs_bug_on_assert in xfs/098 and xfs/115. > > 3) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename > it as _filter_assert_dmesg. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > common/filter | 11 +++++++++++ > common/xfs | 12 ++++++++++++ > tests/xfs/098 | 20 ++++++++------------ > tests/xfs/115 | 7 ++++--- > 4 files changed, 35 insertions(+), 15 deletions(-) > > diff --git a/common/filter b/common/filter > index 8e1fdb4..53874a0 100644 > --- a/common/filter > +++ b/common/filter > @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() > -e "s#$warn9#Intentional warnings in dio_complete#" > } > > +# We generate assert related WARNINGs on purpose and make sure test doesn't fail > +# because of these warnings. This is a helper for _check_dmesg() to filter out > +# them. > +_filter_assert_dmesg() > +{ > + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" > + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" > + sed -e "s#$warn1#Intentional warnings in asswarn#" \ > + -e "s#$warn2#Intentional warnings in assfail#" > +} > + > # make sure this script returns success > /bin/true > diff --git a/common/xfs b/common/xfs > index ab58364..a0e03b6 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -625,6 +625,18 @@ _require_no_xfs_debug() > fi > } > > +# Check if XFS is built with CONFIG_XFS_ASSERT_FATAL > +_require_no_xfs_bug_on_assert() > +{ > + if [ -f /sys/fs/xfs/debug/bug_on_assert ]; then > + grep -q "1" /sys/fs/xfs/debug/bug_on_assert && \ > + _notrun "Require XFS built without CONFIG_XFS_ASSERT_FATAL" This knob can be written, so perhaps we'd rather just set it to zero instead of _notrun? > + else > + # Call _require_no_xfs_debug if bug_on_assert is not available That much is obvious from the code, but the comment doesn't say much about why we shift to _require_no_xfs_debug. How about: # Require that assertions will not hang the system. # # Note: Prior to the creation of CONFIG_XFS_ASSERT_FATAL (and the sysfs # knob bug_on_assert), assertions would always crash the system if XFS # debug was enabled (CONFIG_XFS_DEBUG=y). If a test is designed to # trigger an assertion and the test designer does not want to hang # fstests, skip the test. --D > + _require_no_xfs_debug > + fi > +} > + > # Get a metadata field > # The first arg is the field name > # The rest of the arguments are xfs_db commands to find the metadata. > diff --git a/tests/xfs/098 b/tests/xfs/098 > index 9bcd94b..ebb7ca7 100755 > --- a/tests/xfs/098 > +++ b/tests/xfs/098 > @@ -47,6 +47,10 @@ _cleanup() > _supported_fs xfs > _supported_os Linux > > +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS > +# would crash due to assert failure, so skip if we're testing on a > +# CONFIG_XFS_ASSERT_FATAL built XFS. > +_require_no_xfs_bug_on_assert > _require_scratch > test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc > _require_attrs > @@ -56,16 +60,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" > > rm -f $seqres.full > > -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related > -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check > -# which could miss other potential bugs, so filter out the intentional WARNINGs, > -# make sure test doesn't fail because of this warning and fails on other WARNINGs. > -filter_xfs_dmesg() > -{ > - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" > - sed -e "s#$warn#Intentional warnings in asswarn#" > -} > - > TESTDIR="${SCRATCH_MNT}/scratchdir" > TESTFILE="${TESTDIR}/testfile" > > @@ -107,8 +101,10 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" > echo "+ repair fs" > _repair_scratch_fs >> $seqres.full 2>&1 > > -# mount may trigger related WARNINGs, so filter them. > -_check_dmesg filter_xfs_dmesg > +# We may trigger assert related WARNINGs if we corrupt log on a > +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) > +# build, so filter them. > +_check_dmesg _filter_assert_dmesg > > echo "+ mount image (2)" > _scratch_mount > diff --git a/tests/xfs/115 b/tests/xfs/115 > index 0e62628..5fe1fd5 100755 > --- a/tests/xfs/115 > +++ b/tests/xfs/115 > @@ -52,9 +52,10 @@ rm -f $seqres.full > _supported_fs generic > _supported_os Linux > _require_scratch_nocheck > -# we corrupt XFS on purpose, and debug built XFS would crash due to assert > -# failure, so skip if we're testing on a debug built XFS > -_require_no_xfs_debug > +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS > +# would crash due to assert failure, so skip if we're testing on a > +# CONFIG_XFS_ASSERT_FATAL built XFS. > +_require_no_xfs_bug_on_assert > _disable_dmesg_check > > # Make sure we disable finobt if the filesystem supports it, otherwise, just > -- > 1.8.3.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert 2018-01-18 18:29 ` Darrick J. Wong @ 2018-01-19 2:51 ` Eryu Guan 2018-01-19 4:04 ` Xiao Yang 2018-01-19 5:38 ` [PATCH v6 2/3] common: Add _require_no_xfs_bug_on_assert && Factor out filter_xfs_dmesg xiao yang 2 siblings, 0 replies; 42+ messages in thread From: Eryu Guan @ 2018-01-19 2:51 UTC (permalink / raw) To: Darrick J. Wong; +Cc: xiao yang, david, bfoster, fstests On Thu, Jan 18, 2018 at 10:29:15AM -0800, Darrick J. Wong wrote: > On Thu, Jan 18, 2018 at 06:49:54PM +0800, xiao yang wrote: > > 1) Introduce _require_no_xfs_bug_on_assert helper to check if XFS is built > > with CONFIG_XFS_ASSERT_FATAL, and just call _require_no_xfs_debug if > > /sys/fs/xfs/debug/bug_on_assert is not available. > > > > 2) Apply _require_no_xfs_bug_on_assert in xfs/098 and xfs/115. > > > > 3) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename > > it as _filter_assert_dmesg. > > > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > > --- > > common/filter | 11 +++++++++++ > > common/xfs | 12 ++++++++++++ > > tests/xfs/098 | 20 ++++++++------------ > > tests/xfs/115 | 7 ++++--- > > 4 files changed, 35 insertions(+), 15 deletions(-) > > > > diff --git a/common/filter b/common/filter > > index 8e1fdb4..53874a0 100644 > > --- a/common/filter > > +++ b/common/filter > > @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() > > -e "s#$warn9#Intentional warnings in dio_complete#" > > } > > > > +# We generate assert related WARNINGs on purpose and make sure test doesn't fail > > +# because of these warnings. This is a helper for _check_dmesg() to filter out > > +# them. > > +_filter_assert_dmesg() > > +{ > > + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" > > + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" > > + sed -e "s#$warn1#Intentional warnings in asswarn#" \ > > + -e "s#$warn2#Intentional warnings in assfail#" > > +} > > + > > # make sure this script returns success > > /bin/true > > diff --git a/common/xfs b/common/xfs > > index ab58364..a0e03b6 100644 > > --- a/common/xfs > > +++ b/common/xfs > > @@ -625,6 +625,18 @@ _require_no_xfs_debug() > > fi > > } > > > > +# Check if XFS is built with CONFIG_XFS_ASSERT_FATAL > > +_require_no_xfs_bug_on_assert() > > +{ > > + if [ -f /sys/fs/xfs/debug/bug_on_assert ]; then > > + grep -q "1" /sys/fs/xfs/debug/bug_on_assert && \ > > + _notrun "Require XFS built without CONFIG_XFS_ASSERT_FATAL" > > This knob can be written, so perhaps we'd rather just set it to zero > instead of _notrun? I thought about it too, in the end I think it's better to not change the behavior in test, this simplies the code a bit (if we change it, do we need to change it back after test?) and gives the testers full control on assert behavior (tester would want a crash in debugging?). Perhaps we could update the _notrun message a bit to indicate that bug_on_assert could be turned off if you want test to run. Thanks, Eryu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert 2018-01-18 18:29 ` Darrick J. Wong 2018-01-19 2:51 ` Eryu Guan @ 2018-01-19 4:04 ` Xiao Yang 2018-01-19 5:38 ` [PATCH v6 2/3] common: Add _require_no_xfs_bug_on_assert && Factor out filter_xfs_dmesg xiao yang 2 siblings, 0 replies; 42+ messages in thread From: Xiao Yang @ 2018-01-19 4:04 UTC (permalink / raw) To: Darrick J. Wong; +Cc: eguan, david, bfoster, fstests On 2018/01/19 2:29, Darrick J. Wong wrote: > On Thu, Jan 18, 2018 at 06:49:54PM +0800, xiao yang wrote: >> 1) Introduce _require_no_xfs_bug_on_assert helper to check if XFS is built >> with CONFIG_XFS_ASSERT_FATAL, and just call _require_no_xfs_debug if >> /sys/fs/xfs/debug/bug_on_assert is not available. >> >> 2) Apply _require_no_xfs_bug_on_assert in xfs/098 and xfs/115. >> >> 3) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename >> it as _filter_assert_dmesg. >> >> Signed-off-by: xiao yang<yangx.jy@cn.fujitsu.com> >> --- >> common/filter | 11 +++++++++++ >> common/xfs | 12 ++++++++++++ >> tests/xfs/098 | 20 ++++++++------------ >> tests/xfs/115 | 7 ++++--- >> 4 files changed, 35 insertions(+), 15 deletions(-) >> >> diff --git a/common/filter b/common/filter >> index 8e1fdb4..53874a0 100644 >> --- a/common/filter >> +++ b/common/filter >> @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() >> -e "s#$warn9#Intentional warnings in dio_complete#" >> } >> >> +# We generate assert related WARNINGs on purpose and make sure test doesn't fail >> +# because of these warnings. This is a helper for _check_dmesg() to filter out >> +# them. >> +_filter_assert_dmesg() >> +{ >> + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" >> + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" >> + sed -e "s#$warn1#Intentional warnings in asswarn#" \ >> + -e "s#$warn2#Intentional warnings in assfail#" >> +} >> + >> # make sure this script returns success >> /bin/true >> diff --git a/common/xfs b/common/xfs >> index ab58364..a0e03b6 100644 >> --- a/common/xfs >> +++ b/common/xfs >> @@ -625,6 +625,18 @@ _require_no_xfs_debug() >> fi >> } >> >> +# Check if XFS is built with CONFIG_XFS_ASSERT_FATAL >> +_require_no_xfs_bug_on_assert() >> +{ >> + if [ -f /sys/fs/xfs/debug/bug_on_assert ]; then >> + grep -q "1" /sys/fs/xfs/debug/bug_on_assert&& \ >> + _notrun "Require XFS built without CONFIG_XFS_ASSERT_FATAL" > This knob can be written, so perhaps we'd rather just set it to zero > instead of _notrun? Hi Darrick, I think we can keep it and update the _notrun message as Eryu suggested. >> + else >> + # Call _require_no_xfs_debug if bug_on_assert is not available > That much is obvious from the code, but the comment doesn't say much > about why we shift to _require_no_xfs_debug. How about: > > # Require that assertions will not hang the system. > # > # Note: Prior to the creation of CONFIG_XFS_ASSERT_FATAL (and the sysfs > # knob bug_on_assert), assertions would always crash the system if XFS > # debug was enabled (CONFIG_XFS_DEBUG=y). If a test is designed to > # trigger an assertion and the test designer does not want to hang > # fstests, skip the test. This comment looks better to me. Thanks, Xiao Yang > --D > >> + _require_no_xfs_debug >> + fi >> +} >> + >> # Get a metadata field >> # The first arg is the field name >> # The rest of the arguments are xfs_db commands to find the metadata. >> diff --git a/tests/xfs/098 b/tests/xfs/098 >> index 9bcd94b..ebb7ca7 100755 >> --- a/tests/xfs/098 >> +++ b/tests/xfs/098 >> @@ -47,6 +47,10 @@ _cleanup() >> _supported_fs xfs >> _supported_os Linux >> >> +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS >> +# would crash due to assert failure, so skip if we're testing on a >> +# CONFIG_XFS_ASSERT_FATAL built XFS. >> +_require_no_xfs_bug_on_assert >> _require_scratch >> test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc >> _require_attrs >> @@ -56,16 +60,6 @@ test -z "${FUZZ_ARGS}"&& FUZZ_ARGS="-n 8 -3" >> >> rm -f $seqres.full >> >> -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related >> -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check >> -# which could miss other potential bugs, so filter out the intentional WARNINGs, >> -# make sure test doesn't fail because of this warning and fails on other WARNINGs. >> -filter_xfs_dmesg() >> -{ >> - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" >> - sed -e "s#$warn#Intentional warnings in asswarn#" >> -} >> - >> TESTDIR="${SCRATCH_MNT}/scratchdir" >> TESTFILE="${TESTDIR}/testfile" >> >> @@ -107,8 +101,10 @@ _scratch_mount 2>/dev/null&& _fail "mount should not succeed" >> echo "+ repair fs" >> _repair_scratch_fs>> $seqres.full 2>&1 >> >> -# mount may trigger related WARNINGs, so filter them. >> -_check_dmesg filter_xfs_dmesg >> +# We may trigger assert related WARNINGs if we corrupt log on a >> +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) >> +# build, so filter them. >> +_check_dmesg _filter_assert_dmesg >> >> echo "+ mount image (2)" >> _scratch_mount >> diff --git a/tests/xfs/115 b/tests/xfs/115 >> index 0e62628..5fe1fd5 100755 >> --- a/tests/xfs/115 >> +++ b/tests/xfs/115 >> @@ -52,9 +52,10 @@ rm -f $seqres.full >> _supported_fs generic >> _supported_os Linux >> _require_scratch_nocheck >> -# we corrupt XFS on purpose, and debug built XFS would crash due to assert >> -# failure, so skip if we're testing on a debug built XFS >> -_require_no_xfs_debug >> +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS >> +# would crash due to assert failure, so skip if we're testing on a >> +# CONFIG_XFS_ASSERT_FATAL built XFS. >> +_require_no_xfs_bug_on_assert >> _disable_dmesg_check >> >> # Make sure we disable finobt if the filesystem supports it, otherwise, just >> -- >> 1.8.3.1 >> >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe fstests" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v6 2/3] common: Add _require_no_xfs_bug_on_assert && Factor out filter_xfs_dmesg 2018-01-18 18:29 ` Darrick J. Wong 2018-01-19 2:51 ` Eryu Guan 2018-01-19 4:04 ` Xiao Yang @ 2018-01-19 5:38 ` xiao yang 2018-01-19 5:38 ` [PATCH v6 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 2 siblings, 1 reply; 42+ messages in thread From: xiao yang @ 2018-01-19 5:38 UTC (permalink / raw) To: darrick.wong, eguan; +Cc: david, bfoster, fstests, xiao yang 1) Introduce _require_no_xfs_bug_on_assert helper to check if XFS is built with CONFIG_XFS_ASSERT_FATAL, and call _require_no_xfs_debug if bug_on_assert is not available. 2) Apply _require_no_xfs_bug_on_assert in xfs/098 and xfs/115. 3) Move filter_xfs_dmesg from xfs/098 to common/filter, and rename it as _filter_assert_dmesg. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- common/filter | 11 +++++++++++ common/xfs | 22 ++++++++++++++++++++++ tests/xfs/098 | 18 ++++++------------ tests/xfs/115 | 5 ++--- 4 files changed, 41 insertions(+), 15 deletions(-) diff --git a/common/filter b/common/filter index 8e1fdb4..53874a0 100644 --- a/common/filter +++ b/common/filter @@ -570,5 +570,16 @@ _filter_aiodio_dmesg() -e "s#$warn9#Intentional warnings in dio_complete#" } +# We generate assert related WARNINGs on purpose and make sure test doesn't fail +# because of these warnings. This is a helper for _check_dmesg() to filter out +# them. +_filter_assert_dmesg() +{ + local warn1="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" + local warn2="WARNING:.*fs/xfs/xfs_message\.c:.*assfail.*" + sed -e "s#$warn1#Intentional warnings in asswarn#" \ + -e "s#$warn2#Intentional warnings in assfail#" +} + # make sure this script returns success /bin/true diff --git a/common/xfs b/common/xfs index ab58364..72ad64d 100644 --- a/common/xfs +++ b/common/xfs @@ -625,6 +625,28 @@ _require_no_xfs_debug() fi } +# Require that assertions will not crash the system. +_require_no_xfs_bug_on_assert() +{ + # Assertions would always crash the system if XFS assert fatal was + # enabled (CONFIG_XFS_ASSERT_FATAL=y). If a test is designed to + # trigger an assertion, skip the test on a CONFIG_XFS_ASSERT_FATAL + # built XFS by default. + # Note: CONFIG_XFS_ASSERT_FATAL can be disabled by setting bug_on_assert + # to zero if we want test to run. + if [ -f /sys/fs/xfs/debug/bug_on_assert ]; then + grep -q "1" /sys/fs/xfs/debug/bug_on_assert && \ + _notrun "CONFIG_XFS_ASSERT_FATAL is enabled, but setting bug_on_assert can disable it at runtime" + else + # Note: Prior to the creation of CONFIG_XFS_ASSERT_FATAL (and the + # sysfs knob bug_on_assert), assertions would always crash the system + # if XFS debug was enabled (CONFIG_XFS_DEBUG=y). If a test is designed + # to trigger an assertion and the test designer does not want to hang + # fstests, skip the test. + _require_no_xfs_debug + fi +} + # Get a metadata field # The first arg is the field name # The rest of the arguments are xfs_db commands to find the metadata. diff --git a/tests/xfs/098 b/tests/xfs/098 index 9bcd94b..24ce1d5 100755 --- a/tests/xfs/098 +++ b/tests/xfs/098 @@ -47,6 +47,8 @@ _cleanup() _supported_fs xfs _supported_os Linux +# We corrupt XFS on purpose, and check if assert failures would crash system. +_require_no_xfs_bug_on_assert _require_scratch test -n "${FORCE_FUZZ}" || _require_scratch_xfs_crc _require_attrs @@ -56,16 +58,6 @@ test -z "${FUZZ_ARGS}" && FUZZ_ARGS="-n 8 -3" rm -f $seqres.full -# If we corrupt log on a CONFIG_XFS_WARN build, there will be mount related -# WARNINGs in dmesg as expected. We don't want to simply _disable_dmesg_check -# which could miss other potential bugs, so filter out the intentional WARNINGs, -# make sure test doesn't fail because of this warning and fails on other WARNINGs. -filter_xfs_dmesg() -{ - local warn="WARNING:.*fs/xfs/xfs_message\.c:.*asswarn.*" - sed -e "s#$warn#Intentional warnings in asswarn#" -} - TESTDIR="${SCRATCH_MNT}/scratchdir" TESTFILE="${TESTDIR}/testfile" @@ -107,8 +99,10 @@ _scratch_mount 2>/dev/null && _fail "mount should not succeed" echo "+ repair fs" _repair_scratch_fs >> $seqres.full 2>&1 -# mount may trigger related WARNINGs, so filter them. -_check_dmesg filter_xfs_dmesg +# We may trigger assert related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG (CONFIG_XFS_ASSERT_FATAL is +# disabled) build, so filter them. +_check_dmesg _filter_assert_dmesg echo "+ mount image (2)" _scratch_mount diff --git a/tests/xfs/115 b/tests/xfs/115 index 0e62628..f77d0dd 100755 --- a/tests/xfs/115 +++ b/tests/xfs/115 @@ -52,9 +52,8 @@ rm -f $seqres.full _supported_fs generic _supported_os Linux _require_scratch_nocheck -# we corrupt XFS on purpose, and debug built XFS would crash due to assert -# failure, so skip if we're testing on a debug built XFS -_require_no_xfs_debug +# We corrupt XFS on purpose, and check if assert failures would crash system. +_require_no_xfs_bug_on_assert _disable_dmesg_check # Make sure we disable finobt if the filesystem supports it, otherwise, just -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v6 3/3] xfs: Regression test for invalid sb_logsunit 2018-01-19 5:38 ` [PATCH v6 2/3] common: Add _require_no_xfs_bug_on_assert && Factor out filter_xfs_dmesg xiao yang @ 2018-01-19 5:38 ` xiao yang 0 siblings, 0 replies; 42+ messages in thread From: xiao yang @ 2018-01-19 5:38 UTC (permalink / raw) To: darrick.wong, eguan; +Cc: david, bfoster, fstests, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/439 | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/439.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 89 insertions(+) create mode 100755 tests/xfs/439 create mode 100644 tests/xfs/439.out diff --git a/tests/xfs/439 b/tests/xfs/439 new file mode 100755 index 0000000..dbf654a --- /dev/null +++ b/tests/xfs/439 @@ -0,0 +1,86 @@ +#! /bin/bash +# FS QA Test No. 439 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch +# We corrupt XFS on purpose, and check if assert failures would crash system. +_require_no_xfs_bug_on_assert + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 + +# Check if logsunit is set correctly +lsunit=$(_scratch_xfs_get_sb_field logsunit) +[ $lsunit -ne $((blksz - 1)) ] && _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash on buggy kernel +# The fix applied kernel refuses to mount, so a mount failure is +# expected +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + echo > ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +# We may trigger assert related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG (CONFIG_XFS_ASSERT_FATAL is +# disabled) build, so filter them. +_check_dmesg _filter_assert_dmesg + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/439.out b/tests/xfs/439.out new file mode 100644 index 0000000..9712295 --- /dev/null +++ b/tests/xfs/439.out @@ -0,0 +1,2 @@ +QA output created by 439 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index 04a63b7..0b281a8 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -436,3 +436,4 @@ 436 auto quick clone fsr 437 auto quick other 438 auto quick quota dangerous +439 auto quick dangerous_fuzzers log -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH v5 3/3] xfs: Regression test for invalid sb_logsunit 2018-01-18 10:49 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-18 10:49 ` [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert xiao yang @ 2018-01-18 10:49 ` xiao yang 2018-01-18 18:19 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db Darrick J. Wong 2 siblings, 0 replies; 42+ messages in thread From: xiao yang @ 2018-01-18 10:49 UTC (permalink / raw) To: eguan; +Cc: david, bfoster, fstests, darrick.wong, xiao yang If log stripe unit isn't a multiple of the fs blocksize and mounting, the invalid sb_logsunit leads to crash as soon as we try to write to the log. Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> --- tests/xfs/439 | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tests/xfs/439.out | 2 ++ tests/xfs/group | 1 + 3 files changed, 91 insertions(+) create mode 100755 tests/xfs/439 create mode 100644 tests/xfs/439.out diff --git a/tests/xfs/439 b/tests/xfs/439 new file mode 100755 index 0000000..02814ee --- /dev/null +++ b/tests/xfs/439 @@ -0,0 +1,88 @@ +#! /bin/bash +# FS QA Test No. 439 +# +# Regression test for commit: +# 9c92ee2 ("xfs: validate sb_logsunit is a multiple of the fs blocksize") +# +# If log stripe unit isn't a multiple of the fs blocksize and mounting, +# the invalid sb_logsunit leads to crash as soon as we try to write to +# the log. +# +#----------------------------------------------------------------------- +# Copyright (c) 2018 FUJITSU. All Rights Reserved. +# Author: Xiao Yang <yangx.jy@cn.fujitsu.com> +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation. +# +# This program is distributed in the hope that it would be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write the Free Software Foundation, +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA +#----------------------------------------------------------------------- + +seq=`basename "$0"` +seqres="$RESULT_DIR/$seq" +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + rm -rf $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# real QA test starts here +_supported_os Linux +_supported_fs xfs +_require_scratch +# We corrupt XFS on purpose, and CONFIG_XFS_ASSERT_FATAL built XFS +# would crash due to assert failure, so skip if we're testing on a +# CONFIG_XFS_ASSERT_FATAL built XFS. +_require_no_xfs_bug_on_assert + +rm -f "$seqres.full" + +# Format +_scratch_mkfs > $seqres.full 2>&1 || _fail "mkfs failed" + +# Set logsunit to a value which is not a multiple of the fs blocksize +blksz=$(_scratch_xfs_get_sb_field blocksize) +_scratch_xfs_set_sb_field logsunit $((blksz - 1)) >> $seqres.full 2>&1 + +# Check if logsunit is set correctly +lsunit=$(_scratch_xfs_get_sb_field logsunit) +[ $lsunit -ne $((blksz - 1)) ] && _notrun "failed to set sb_logsunit" + +# Mount and writing log may trigger a crash on buggy kernel +# The fix applied kernel refuses to mount, so a mount failure is +# expected +if _scratch_mount >> $seqres.full 2>&1; then + for i in $(seq 1 1000); do + echo > ${SCRATCH_MNT}/$i + done + _scratch_unmount +fi + +# We may trigger assert related WARNINGs if we corrupt log on a +# CONFIG_XFS_WARN or CONFIG_XFS_DEBUG(without CONFIG_XFS_ASSERT_FATAL) +# build, so filter them. +_check_dmesg _filter_assert_dmesg + +echo "Silence is golden" + +# success, all done +status=0 +exit diff --git a/tests/xfs/439.out b/tests/xfs/439.out new file mode 100644 index 0000000..9712295 --- /dev/null +++ b/tests/xfs/439.out @@ -0,0 +1,2 @@ +QA output created by 439 +Silence is golden diff --git a/tests/xfs/group b/tests/xfs/group index 04a63b7..0b281a8 100644 --- a/tests/xfs/group +++ b/tests/xfs/group @@ -436,3 +436,4 @@ 436 auto quick clone fsr 437 auto quick other 438 auto quick quota dangerous +439 auto quick dangerous_fuzzers log -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-18 10:49 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-18 10:49 ` [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert xiao yang 2018-01-18 10:49 ` [PATCH v5 " xiao yang @ 2018-01-18 18:19 ` Darrick J. Wong 2 siblings, 0 replies; 42+ messages in thread From: Darrick J. Wong @ 2018-01-18 18:19 UTC (permalink / raw) To: xiao yang; +Cc: eguan, david, bfoster, fstests On Thu, Jan 18, 2018 at 06:49:53PM +0800, xiao yang wrote: > Make sure _scratch_xfs_set_metadata_field() can be used on an > old xfsprogs-dev(e.g. v3.1.1). > > The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit > 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). > > The special argument "--" is only used to end option-scanning > in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by > commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > common/xfs | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/common/xfs b/common/xfs > index 45b84a0..ab58364 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() > for arg in "$@"; do > cmds+=("-c" "${arg}") > done > - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" > - echo > + > + local wr_cmd="write" > + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- ${value}" > + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" > + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" > } > > _scratch_xfs_get_sb_field() > -- > 1.8.3.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-16 4:02 ` Eryu Guan 2018-01-16 6:41 ` Xiao Yang 2018-01-16 7:26 ` [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang @ 2018-01-16 8:50 ` Dave Chinner 2018-01-16 14:09 ` Brian Foster 2 siblings, 1 reply; 42+ messages in thread From: Dave Chinner @ 2018-01-16 8:50 UTC (permalink / raw) To: Eryu Guan; +Cc: Brian Foster, xiao yang, fstests, darrick.wong On Tue, Jan 16, 2018 at 12:02:54PM +0800, Eryu Guan wrote: > On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: > > On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > > > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > > > That's the whole point of adding debug asserts in cases like this - > > > > they are supposed to stop test execution in it's tracks and leave a > > > > corpse to analyse. The auto group regression tests are not supposed > > > > to take the machine down on normal test configs (i.e. > > > > CONFIG_XFS_DEBUG=y). > > > > > > > > > > FWIW, my understanding of the dangerous group has always been that it's > > > for tests that when they trigger a regression, forcibly affect the > > > entire system as such (lockup, hang, crash, etc.). IMO, a test that > > I had the same understanding of dangerous group. And I recommended the > usage of "-g auto -x dangerous" before[1], and Dave acknowledged this > dangerous group usage[2] :) > > [1] https://www.spinics.net/lists/linux-btrfs/msg57312.html > [2] https://www.spinics.net/lists/linux-btrfs/msg57330.html Context is important. The context that the above links were talking about filtering the dangerous group was for older kernels that don't have the fixes for the bugs that crash the kernel. This particular test fails this auto group criteria in the case of people using CONFIG_XFS_DEBUG=y: " - it passes on current upstream kernels, if it fails, it's likely to be resolved in forseeable future [2] " It fails, and isn't likely to ever work, because the assert needs to remain there to catch userspace tool screwups.... > > If we really want to test these "should not ever happen" conditions > > that trigger asserts on debug kernels as "everyone always runs" > > regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y > > kernels. fstests already knows that it's running on a debug kernel, > > so adding a _requires_production_kernel check may be the way around > > this being considered a dangerous test. > > This reminds that we already have a _require_no_xfs_debug rule, as used > in xfs/115, which is known to trigger ASSERT failure on debug build. So > we can do the same in this new test. Ok, good! And it appears to be addressing the same "intentional corruption triggers failures" case as we are discussing here: # we corrupt XFS on purpose, and debug built XFS would crash due to assert # failure, so skip if we're testing on a debug built XFS _require_no_xfs_debug Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-16 8:50 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit Dave Chinner @ 2018-01-16 14:09 ` Brian Foster 2018-01-18 8:44 ` Eryu Guan 0 siblings, 1 reply; 42+ messages in thread From: Brian Foster @ 2018-01-16 14:09 UTC (permalink / raw) To: Dave Chinner; +Cc: Eryu Guan, xiao yang, fstests, darrick.wong On Tue, Jan 16, 2018 at 07:50:23PM +1100, Dave Chinner wrote: > On Tue, Jan 16, 2018 at 12:02:54PM +0800, Eryu Guan wrote: > > On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: > > > On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > > > > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > > > > That's the whole point of adding debug asserts in cases like this - > > > > > they are supposed to stop test execution in it's tracks and leave a > > > > > corpse to analyse. The auto group regression tests are not supposed > > > > > to take the machine down on normal test configs (i.e. > > > > > CONFIG_XFS_DEBUG=y). > > > > > > > > > > > > > FWIW, my understanding of the dangerous group has always been that it's > > > > for tests that when they trigger a regression, forcibly affect the > > > > entire system as such (lockup, hang, crash, etc.). IMO, a test that > > > > I had the same understanding of dangerous group. And I recommended the > > usage of "-g auto -x dangerous" before[1], and Dave acknowledged this > > dangerous group usage[2] :) > > > > [1] https://www.spinics.net/lists/linux-btrfs/msg57312.html > > [2] https://www.spinics.net/lists/linux-btrfs/msg57330.html > > Context is important. The context that the above links were talking > about filtering the dangerous group was for older kernels that don't > have the fixes for the bugs that crash the kernel. > > This particular test fails this auto group criteria in the case of > people using CONFIG_XFS_DEBUG=y: > > " > - it passes on current upstream kernels, if it fails, it's > likely to be resolved in forseeable future [2] > " > > It fails, and isn't likely to ever work, because the assert needs to > remain there to catch userspace tool screwups.... > > > > If we really want to test these "should not ever happen" conditions > > > that trigger asserts on debug kernels as "everyone always runs" > > > regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y > > > kernels. fstests already knows that it's running on a debug kernel, > > > so adding a _requires_production_kernel check may be the way around > > > this being considered a dangerous test. > > > > This reminds that we already have a _require_no_xfs_debug rule, as used > > in xfs/115, which is known to trigger ASSERT failure on debug build. So > > we can do the same in this new test. > > Ok, good! And it appears to be addressing the same "intentional > corruption triggers failures" case as we are discussing here: > My only suggestion (re: my previous comment) is to consider using a new check that looks at bug_on_assert when the knob is available for tests that are expected to generate asserts as such. The test stil has to filter the assert itself to cover the WARN=1 case, right? > # we corrupt XFS on purpose, and debug built XFS would crash due to assert > # failure, so skip if we're testing on a debug built XFS > _require_no_xfs_debug > Only with CONFIG_XFS_ASSERT_FATAL=y! :) Brian > 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 [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-16 14:09 ` Brian Foster @ 2018-01-18 8:44 ` Eryu Guan 0 siblings, 0 replies; 42+ messages in thread From: Eryu Guan @ 2018-01-18 8:44 UTC (permalink / raw) To: Brian Foster; +Cc: Dave Chinner, xiao yang, fstests, darrick.wong On Tue, Jan 16, 2018 at 09:09:14AM -0500, Brian Foster wrote: > On Tue, Jan 16, 2018 at 07:50:23PM +1100, Dave Chinner wrote: > > On Tue, Jan 16, 2018 at 12:02:54PM +0800, Eryu Guan wrote: > > > On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: > > > > On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > > > > > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > > > > > That's the whole point of adding debug asserts in cases like this - > > > > > > they are supposed to stop test execution in it's tracks and leave a > > > > > > corpse to analyse. The auto group regression tests are not supposed > > > > > > to take the machine down on normal test configs (i.e. > > > > > > CONFIG_XFS_DEBUG=y). > > > > > > > > > > > > > > > > FWIW, my understanding of the dangerous group has always been that it's > > > > > for tests that when they trigger a regression, forcibly affect the > > > > > entire system as such (lockup, hang, crash, etc.). IMO, a test that > > > > > > I had the same understanding of dangerous group. And I recommended the > > > usage of "-g auto -x dangerous" before[1], and Dave acknowledged this > > > dangerous group usage[2] :) > > > > > > [1] https://www.spinics.net/lists/linux-btrfs/msg57312.html > > > [2] https://www.spinics.net/lists/linux-btrfs/msg57330.html > > > > Context is important. The context that the above links were talking > > about filtering the dangerous group was for older kernels that don't > > have the fixes for the bugs that crash the kernel. > > > > This particular test fails this auto group criteria in the case of > > people using CONFIG_XFS_DEBUG=y: > > > > " > > - it passes on current upstream kernels, if it fails, it's > > likely to be resolved in forseeable future [2] > > " > > > > It fails, and isn't likely to ever work, because the assert needs to > > remain there to catch userspace tool screwups.... > > > > > > If we really want to test these "should not ever happen" conditions > > > > that trigger asserts on debug kernels as "everyone always runs" > > > > regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y > > > > kernels. fstests already knows that it's running on a debug kernel, > > > > so adding a _requires_production_kernel check may be the way around > > > > this being considered a dangerous test. > > > > > > This reminds that we already have a _require_no_xfs_debug rule, as used > > > in xfs/115, which is known to trigger ASSERT failure on debug build. So > > > we can do the same in this new test. > > > > Ok, good! And it appears to be addressing the same "intentional > > corruption triggers failures" case as we are discussing here: > > > > My only suggestion (re: my previous comment) is to consider using a new > check that looks at bug_on_assert when the knob is available for tests > that are expected to generate asserts as such. The test stil has to Looks like we need a new _require_no_xfs_bug_on_assert, which looks at /sys/fs/xfs/debug/bug_on_assert value if it's available, and just calls _require_no_xfs_debug if it's not available. > filter the assert itself to cover the WARN=1 case, right? I think so (and to cover the bug_on_assert=0 case). > > > # we corrupt XFS on purpose, and debug built XFS would crash due to assert > > # failure, so skip if we're testing on a debug built XFS > > _require_no_xfs_debug > > > > Only with CONFIG_XFS_ASSERT_FATAL=y! :) XFS_ASSERT_FATAL and _require_no_xfs_debug were introduced closely in time. XFS_ASSERT_FATAL was there first, and it was too new to be noticed when we then introduced _require_no_xfs_debug.. Thanks, Eryu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit 2018-01-15 21:03 ` Dave Chinner 2018-01-16 4:02 ` Eryu Guan @ 2018-01-16 13:58 ` Brian Foster 1 sibling, 0 replies; 42+ messages in thread From: Brian Foster @ 2018-01-16 13:58 UTC (permalink / raw) To: Dave Chinner; +Cc: Eryu Guan, xiao yang, fstests, darrick.wong On Tue, Jan 16, 2018 at 08:03:52AM +1100, Dave Chinner wrote: > On Mon, Jan 15, 2018 at 07:45:23AM -0500, Brian Foster wrote: > > On Sat, Jan 13, 2018 at 01:23:53PM +1100, Dave Chinner wrote: > > > On Fri, Jan 12, 2018 at 04:50:05PM +0800, Eryu Guan wrote: > > > > On Fri, Jan 12, 2018 at 07:36:05PM +1100, Dave Chinner wrote: > > > > > On Fri, Jan 12, 2018 at 03:49:21PM +0800, Eryu Guan wrote: > > > > > > On Fri, Jan 12, 2018 at 02:16:23PM +0800, xiao yang wrote: ... > > > > > > > > > > > > This test triggers ASSERT failure and warning on debug build, thus > > > > > > failed _dmesg_check, I think we need _disable_dmesg_check (and some > > > > > > comments) too. > > > > > > > > > > > > [1809960.157615] XFS (sda6): log stripe unit 4095 bytes must be a multiple of block size > > > > > > [1809960.159228] XFS (sda6): AAIEEE! Log failed size checks. Abort! > > > > > > [1809960.160534] XFS: Assertion failed: 0, file: fs/xfs/xfs_log.c, line: 679 > > > > > > [1809960.162298] WARNING: CPU: 1 PID: 2186 at fs/xfs/xfs_message.c:105 asswarn+0x1e/0x30 [xfs] > > > > > > > > > > If it triggers debug asserts which have been put there to catch bugs > > > > > in other utilities (like mkfs) on a current upstream debug kernel, > > > > > then the test should not be part of the auto and quick groups. > > > > > > > > This test corrupts the filesystem on purpose (I didn't make this clear > > > > in my last reply, sorry about that), and xfs detects the corruption & > > > > refuses the mount instead of crashing. So I think the ASSERT failure is > > > > expected on debug build and we just need to ignore it. > > > > > > Except that after that assert, the block device is now busy and can't > > > be released, so we can't use the scratch device anymore. All tests > > > after this assert has been triggered are going to fail because > > > the block device is busy.... > > > > > > That's the whole point of adding debug asserts in cases like this - > > > they are supposed to stop test execution in it's tracks and leave a > > > corpse to analyse. The auto group regression tests are not supposed > > > to take the machine down on normal test configs (i.e. > > > CONFIG_XFS_DEBUG=y). > > > > > > > FWIW, my understanding of the dangerous group has always been that it's > > for tests that when they trigger a regression, forcibly affect the > > entire system as such (lockup, hang, crash, etc.). IMO, a test that > > intentionally generates an XFS assert doesn't really follow the spirit > > of that categorization, even though technically an assert can cause a > > BUG. This is why we have CONFIG_XFS_ASSERT_FATAL now, because unless > > that is disabled any auto test that reproduces a regression (assuming we > > have broad/robust assert coverage) can very easily BUG() and have the > > associated effect on the system. > > And this is exactly why I opposed the CONFIG_XFS_ASSERT_FATAL > inclusion. It started us down the slippery slope of allowing people > to ignore asserts that indicate an impossible condition had > occurred. > I thought you opposed the unconditional change from BUG() to WARN() but supported the config option, but I could be misremembering. > This was a bug found via fuzzing, not user problems in the wild. The > assert had functioned perfectly well up to this point as a mechanism > for preventing the XFS tools from creating a corrupt log stripe > unit, and it still does.... > > That's the point here - a corrupt log stripe unit *should never > happen* because it should trigger a CRC validation failure long > before we get to parsing it. If we've got a corrupt value with a > correct CRC, then we damn well need to understand the cause > *immediately* because something else has gone very, very wrong. > I'm not following the argument. What do we really lose by removing an assert that is accompanied by mount failure and several other context specific error messages? > > The flipside of course is that one should generally be able to run > > xfstests against a FATAL_ASSERTS=y kernel that otherwise has no > > regressions and not bork the system because a test generates a BUG() as > > part of a successful run. > > That is the historical behaviour of CONFIG_XFS_DEBUG=y and the > historical policy for xfstests. Adding CONFIG_XFS_ASSERT_FATAL does > not change that because (at least) some of use still treat asserts > as fatal. > Agree (I thought that's exactly what I said above..? ;)). > > Perhaps the root problem here is that the > > kernel generates an assert from a codepath that "successfully" handles > > the erroneous situation. > > We do that all over the place - the assert is there because it > indicates a fatal problem has occurred that should never happen. > For production systems, we still have to handle that gracefully > because people do fuzz testing of "should never happen" conditions > and then get upset we fail to detect this. That does not mean > the ASSERT is wrong - it makes it even more important that > developers know when their code hits these cases.... > I'm not suggesting the assert is wrong, that we don't do this all over the place already, or that we should never do the ASSERT(0) thing. I'm suggesting that in this specific case, the most practical thing may be just to delete the assert rather than categorize this new test as dangerous. > If we really want to test these "should not ever happen" conditions > that trigger asserts on debug kernels as "everyone always runs" > regression testing, then these need to _notrun on CONFIG_XFS_DEBUG=y > kernels. fstests already knows that it's running on a debug kernel, > so adding a _requires_production_kernel check may be the way around > this being considered a dangerous test. > I think that's overkill for the time being, but I suppose is a reasonable option in the long term as we end up with more of this particular situation (fuzzer-like tests vs. ASSERT()). I think more broadly useful would be to use the bug_on_assert sysfs value where it's available, then fall back to DEBUG=1 if necessary. > > We already spit out several error messages and > > fail the mount, so my .02 (fwiw) would be to leave the test as > > auto+dangerous, delete the useless assert and let people affected by > > older kernels (w/ FATAL_ASSERT behavior) filter out the test as > > appropriate in those environments. (Looking back, I see Darrick already > > suggested to delete the assert as well...). > > ... and this specific assert caught multiple bugs I introduced while > refactoring mkfs recently. That's it's purpose, and it works for > that purpose very well. > Maybe I'm missing something.. the mount would have still failed and spit out the error message as well, right? > > > If you want to test dangerous regression tests, then you need to > > > *opt-in* by adding "-g dangerous", not force everyone else to > > > opt-out by having to run "-g auto -x dangerous". > > > > > > > I often explicitly filter out the dangerous group as above because we > > already have more than a handful of tests that are in both groups. I > > don't think they've historically been mutually exclusive. > > Which means we've already deviated from the historic policy. That > doesn't mean it the right direction to take. > I've taken this approach with xfstests for as long as I can remember (running the dangerous tests separately at least, so as to not interrupt a longer running auto run). This doesn't have anything to do with the whole FATAL_ASSERT thing because most of those dangerous tests weren't considered dangerous just because they triggered an assert. They were dangerous because they hung or caused bad memory accesses or some such on kernels without the associated fix. That actually makes me wonder whether we should have some kind of dangerous expiration policy where the tag could be removed after some period of time (where fixes have had plenty of time to trickle into stable kernels), but that's a separate topic. The FATAL_ASSERT thing was more about the ability to enable all of the additional DEBUG mode code we have (things like sparse inode injection, etc., that simply cannot be enabled on DEBUG=0) without also having to crash the box if an assertion fails. Brian > 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 [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-12 6:16 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit xiao yang @ 2018-01-12 7:44 ` Eryu Guan 2018-01-12 16:43 ` Darrick J. Wong 2 siblings, 0 replies; 42+ messages in thread From: Eryu Guan @ 2018-01-12 7:44 UTC (permalink / raw) To: xiao yang; +Cc: fstests, darrick.wong On Fri, Jan 12, 2018 at 02:16:22PM +0800, xiao yang wrote: > Make sure _scratch_xfs_set_metadata_field() can be used on an > old xfsprogs-dev(e.g. v3.1.1). > > The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit > 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). > > The special argument "--" is only used to end option-scanning > in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by > commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > common/xfs | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/common/xfs b/common/xfs > index 45b84a0..59d6dd3 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() > for arg in "$@"; do > cmds+=("-c" "${arg}") > done > - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" > - echo I'm not sure if any golden image depends on this extra newline.. > + > + local wr_cmd="write" > + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- $value" > + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" > + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" This looks OK, but I'd like Darrick to help review too. Thanks, Eryu > } > > _scratch_xfs_get_sb_field() > -- > 1.8.3.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-12 6:16 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-12 7:44 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db Eryu Guan @ 2018-01-12 16:43 ` Darrick J. Wong 2 siblings, 0 replies; 42+ messages in thread From: Darrick J. Wong @ 2018-01-12 16:43 UTC (permalink / raw) To: xiao yang; +Cc: fstests, eguan On Fri, Jan 12, 2018 at 02:16:22PM +0800, xiao yang wrote: > Make sure _scratch_xfs_set_metadata_field() can be used on an > old xfsprogs-dev(e.g. v3.1.1). > > The "-d" option was introduced since xfsprogs-dev v4.7.0 by commit > 86769b3 ("xfs_db: allow recalculating CRCs on invalid metadata"). > > The special argument "--" is only used to end option-scanning > in getopt(). getopt() was introduced since xfsprogs-dev v3.2.3 by > commit c9f5e3d ("xfs_db: Allow writes of corrupted data")'. > > Signed-off-by: xiao yang <yangx.jy@cn.fujitsu.com> > --- > common/xfs | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/common/xfs b/common/xfs > index 45b84a0..59d6dd3 100644 > --- a/common/xfs > +++ b/common/xfs > @@ -658,8 +658,11 @@ _scratch_xfs_set_metadata_field() > for arg in "$@"; do > cmds+=("-c" "${arg}") > done > - _scratch_xfs_db -x "${cmds[@]}" -c "write -d ${key} -- ${value}" > - echo > + > + local wr_cmd="write" > + _scratch_xfs_db -x -c "help write" | egrep -q "(-c|-d)" && value="-- $value" "${value}" at the end of this line, for consistency within the function? Looks ok, Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > + _scratch_xfs_db -x -c "help write" | egrep -q "(-d)" && wr_cmd="${wr_cmd} -d" > + _scratch_xfs_db -x "${cmds[@]}" -c "${wr_cmd} ${key} ${value}" > } > > _scratch_xfs_get_sb_field() > -- > 1.8.3.1 > > > > -- > To unsubscribe from this list: send the line "unsubscribe fstests" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2018-01-19 5:46 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-11 8:25 [PATCH] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-11 18:07 ` Darrick J. Wong 2018-01-12 1:58 ` Xiao Yang 2018-01-12 6:14 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory xiao yang 2018-01-12 6:14 ` [PATCH v2] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-12 6:19 ` [PATCH v2] syscalls/madvise09.c: Use custom mount point instead of /sys/fs/cgroup/memory Xiao Yang 2018-01-12 6:16 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-12 6:16 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-12 7:49 ` Eryu Guan 2018-01-12 8:36 ` Dave Chinner 2018-01-12 8:50 ` Eryu Guan 2018-01-12 16:41 ` Darrick J. Wong 2018-01-13 2:23 ` Dave Chinner 2018-01-15 6:29 ` Eryu Guan 2018-01-15 7:48 ` [PATCH v3 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-15 7:48 ` [PATCH v3 2/3] common/filter: factor out expected XFS warnings for mount xiao yang 2018-01-15 7:48 ` [PATCH v3 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-15 12:45 ` [PATCH v2 2/2] " Brian Foster 2018-01-15 21:03 ` Dave Chinner 2018-01-16 4:02 ` Eryu Guan 2018-01-16 6:41 ` Xiao Yang 2018-01-16 7:26 ` [PATCH v4 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-16 7:26 ` [PATCH v4 2/3] common/filter: factor out expected XFS warnings for mount xiao yang 2018-01-18 8:48 ` Eryu Guan 2018-01-18 8:56 ` Xiao Yang 2018-01-16 7:26 ` [PATCH v4 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-18 8:46 ` Eryu Guan 2018-01-18 10:49 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db xiao yang 2018-01-18 10:49 ` [PATCH v5 2/3] common/filter: Factor out expected XFS warnings for assert xiao yang 2018-01-18 18:29 ` Darrick J. Wong 2018-01-19 2:51 ` Eryu Guan 2018-01-19 4:04 ` Xiao Yang 2018-01-19 5:38 ` [PATCH v6 2/3] common: Add _require_no_xfs_bug_on_assert && Factor out filter_xfs_dmesg xiao yang 2018-01-19 5:38 ` [PATCH v6 3/3] xfs: Regression test for invalid sb_logsunit xiao yang 2018-01-18 10:49 ` [PATCH v5 " xiao yang 2018-01-18 18:19 ` [PATCH v5 1/3] common/xfs: Check if write supports [-c|-d] option in xfs_db Darrick J. Wong 2018-01-16 8:50 ` [PATCH v2 2/2] xfs: Regression test for invalid sb_logsunit Dave Chinner 2018-01-16 14:09 ` Brian Foster 2018-01-18 8:44 ` Eryu Guan 2018-01-16 13:58 ` Brian Foster 2018-01-12 7:44 ` [PATCH v2 1/2] common/xfs: Check if write supports [-c|-d] option in xfs_db Eryu Guan 2018-01-12 16:43 ` Darrick J. Wong
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.