| Message ID | 20170629132947.29939-1-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 09:29:47AM -0400, jlayton@kernel.org wrote: > From: Jeff Layton <jlayton@redhat.com> > > Currently we just have this test run on a whitelist of filesystems, but > it would be best to be able to run it on all of them. The problem is > that a lot of filesystems basically shut down once they hit metadata > errors. > > Allow the fsync-err testcase to operate in two different modes. One > mode just does basic testing to ensure that we get an error back on > all fd's when we fsync. The other does a more thorough test to ensure > that we get back 0 on subsequent fsyncs when there hasn't been any > write activity. > > For now, we just opt-in to the more thorough testing on certain > filesystems: xfs, ext3 and ext4 on the generic test. All other > filesystems will run in simple mode. > > Signed-off-by: Jeff Layton <jlayton@redhat.com> > --- > common/rc | 9 +++++++++ > src/fsync-err.c | 51 +++++++++++++++++++++++++++++++++------------------ > tests/generic/441 | 22 +++++++++++++++------- > 3 files changed, 57 insertions(+), 25 deletions(-) > > diff --git a/common/rc b/common/rc > index 2972f89e9527..83675364cf24 100644 > --- a/common/rc > +++ b/common/rc > @@ -1738,6 +1738,15 @@ _require_test() > touch ${RESULT_DIR}/require_test > } > > +_has_logdev() > +{ > + ret=0 > + [ -z "$SCRATCH_LOGDEV" -o ! -b "$SCRATCH_LOGDEV" ] && > + [ "$USE_EXTERNAL" != yes ] && ret=1 This doesn't seem right. _has_logdev = ($SCRATCH_LOGDEV is block device) && ($USE_EXTERNAL set to yes), so we should set ret=1 if any of these conditions is not met. But this code returns 0 (true) if I only have SCRATCH_LOGDEV set but not USE_EXTERNAL. And once we have _has_logdev, I think we can refactor _require_logdev to use _has_logdev too. > + > + return $ret > +} > + > # this test needs a logdev > # > _require_logdev() > diff --git a/src/fsync-err.c b/src/fsync-err.c > index 5b3bdd3ada07..4b0205cf2fd4 100644 > --- a/src/fsync-err.c > +++ b/src/fsync-err.c > @@ -13,6 +13,7 @@ > #include <string.h> > #include <unistd.h> > #include <getopt.h> > +#include <stdbool.h> > > /* > * btrfs has a fixed stripewidth of 64k, so we need to write enough data to > @@ -25,7 +26,7 @@ > > static void usage() > { > - printf("Usage: fsync-err [ -b bufsize ] [ -n num_fds ] -d dmerror path <filename>\n"); > + printf("Usage: fsync-err [ -b bufsize ] [ -n num_fds ] [ -s ] -d dmerror path <filename>\n"); > } > > int main(int argc, char **argv) > @@ -35,8 +36,9 @@ int main(int argc, char **argv) > char *dmerror_path = NULL; > char *cmdbuf; > size_t cmdsize, bufsize = DEFAULT_BUFSIZE; > + bool simple_mode = false; > > - while ((i = getopt(argc, argv, "b:d:n:")) != -1) { > + while ((i = getopt(argc, argv, "b:d:n:s")) != -1) { > switch (i) { > case 'b': > bufsize = strtol(optarg, &buf, 0); > @@ -55,6 +57,15 @@ int main(int argc, char **argv) > return 1; > } > break; > + case 's': > + /* > + * Many filesystems will continue to throw errors after > + * fsync has already advanced to the current error, > + * due to metadata writeback failures or other > + * issues. Allow those fs' to opt out of more thorough > + * testing. > + */ > + simple_mode = true; > } > } > > @@ -154,16 +165,18 @@ int main(int argc, char **argv) > } > } > > - for (i = 0; i < numfds; ++i) { > - ret = fsync(fd[i]); > - if (ret < 0) { > - /* > - * We did a failed write and fsync on each fd before. > - * Now the error should be clear since we've not done > - * any writes since then. > - */ > - printf("Third fsync on fd[%d] failed: %m\n", i); > - return 1; > + if (!simple_mode) { > + for (i = 0; i < numfds; ++i) { > + ret = fsync(fd[i]); > + if (ret < 0) { > + /* > + * We did a failed write and fsync on each fd before. > + * Now the error should be clear since we've not done > + * any writes since then. > + */ > + printf("Third fsync on fd[%d] failed: %m\n", i); > + return 1; > + } > } > } > > @@ -185,12 +198,14 @@ int main(int argc, char **argv) > return 1; > } > > - for (i = 0; i < numfds; ++i) { > - ret = fsync(fd[i]); > - if (ret < 0) { > - /* The error should still be clear */ > - printf("fsync after healing device on fd[%d] failed: %m\n", i); > - return 1; > + if (!simple_mode) { > + for (i = 0; i < numfds; ++i) { > + ret = fsync(fd[i]); > + if (ret < 0) { > + /* The error should still be clear */ > + printf("fsync after healing device on fd[%d] failed: %m\n", i); > + return 1; > + } > } > } > > diff --git a/tests/generic/441 b/tests/generic/441 > index 2215b64db9a7..e8244224e097 100755 > --- a/tests/generic/441 > +++ b/tests/generic/441 > @@ -45,15 +45,23 @@ _cleanup() > . ./common/dmerror > > # real QA test starts here > -_supported_fs ext2 ext3 ext4 xfs > _supported_os Linux > _require_scratch > > -# Generally, we want to avoid journal errors in this test. Ensure that > -# journalled fs' have a logdev. > -if [ "$FSTYP" != "ext2" ]; then > - _require_logdev > -fi > +# Generally, we want to avoid journal errors on the extended testcase. Only > +# set the -r flag if we have a logdev > +sflag='-s' This part seems confusing, comments said "set the -r flag" but '-s' is actually used. > +case $FSTYP in > + btrfs) > + _notrun "btrfs has a specialized test for this" > + ;; > + ext3|ext4|xfs) > + # Do the more thorough test if we have a logdev > + _has_logdev && sflag='' > + ;; > + *) > + ;; > +esac > > _require_dm_target error > _require_test_program fsync-err > @@ -70,7 +78,7 @@ _require_fs_space $SCRATCH_MNT 65536 > > testfile=$SCRATCH_MNT/fsync-err-test > > -$here/src/fsync-err -d $here/src/dmerror $testfile > +$here/src/fsync-err $sflag -d $here/src/dmerror $testfile Better to append this command to $seqres.full too for debug purpose, so we know if we're running with or without '-s' option. Thanks, Eryu > > # success, all done > _dmerror_load_working_table > -- > 2.13.0 > -- To unsubscribe from this list: send the line "unsubscribe fstests" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/common/rc b/common/rc index 2972f89e9527..83675364cf24 100644 --- a/common/rc +++ b/common/rc @@ -1738,6 +1738,15 @@ _require_test() touch ${RESULT_DIR}/require_test } +_has_logdev() +{ + ret=0 + [ -z "$SCRATCH_LOGDEV" -o ! -b "$SCRATCH_LOGDEV" ] && + [ "$USE_EXTERNAL" != yes ] && ret=1 + + return $ret +} + # this test needs a logdev # _require_logdev() diff --git a/src/fsync-err.c b/src/fsync-err.c index 5b3bdd3ada07..4b0205cf2fd4 100644 --- a/src/fsync-err.c +++ b/src/fsync-err.c @@ -13,6 +13,7 @@ #include <string.h> #include <unistd.h> #include <getopt.h> +#include <stdbool.h> /* * btrfs has a fixed stripewidth of 64k, so we need to write enough data to @@ -25,7 +26,7 @@ static void usage() { - printf("Usage: fsync-err [ -b bufsize ] [ -n num_fds ] -d dmerror path <filename>\n"); + printf("Usage: fsync-err [ -b bufsize ] [ -n num_fds ] [ -s ] -d dmerror path <filename>\n"); } int main(int argc, char **argv) @@ -35,8 +36,9 @@ int main(int argc, char **argv) char *dmerror_path = NULL; char *cmdbuf; size_t cmdsize, bufsize = DEFAULT_BUFSIZE; + bool simple_mode = false; - while ((i = getopt(argc, argv, "b:d:n:")) != -1) { + while ((i = getopt(argc, argv, "b:d:n:s")) != -1) { switch (i) { case 'b': bufsize = strtol(optarg, &buf, 0); @@ -55,6 +57,15 @@ int main(int argc, char **argv) return 1; } break; + case 's': + /* + * Many filesystems will continue to throw errors after + * fsync has already advanced to the current error, + * due to metadata writeback failures or other + * issues. Allow those fs' to opt out of more thorough + * testing. + */ + simple_mode = true; } } @@ -154,16 +165,18 @@ int main(int argc, char **argv) } } - for (i = 0; i < numfds; ++i) { - ret = fsync(fd[i]); - if (ret < 0) { - /* - * We did a failed write and fsync on each fd before. - * Now the error should be clear since we've not done - * any writes since then. - */ - printf("Third fsync on fd[%d] failed: %m\n", i); - return 1; + if (!simple_mode) { + for (i = 0; i < numfds; ++i) { + ret = fsync(fd[i]); + if (ret < 0) { + /* + * We did a failed write and fsync on each fd before. + * Now the error should be clear since we've not done + * any writes since then. + */ + printf("Third fsync on fd[%d] failed: %m\n", i); + return 1; + } } } @@ -185,12 +198,14 @@ int main(int argc, char **argv) return 1; } - for (i = 0; i < numfds; ++i) { - ret = fsync(fd[i]); - if (ret < 0) { - /* The error should still be clear */ - printf("fsync after healing device on fd[%d] failed: %m\n", i); - return 1; + if (!simple_mode) { + for (i = 0; i < numfds; ++i) { + ret = fsync(fd[i]); + if (ret < 0) { + /* The error should still be clear */ + printf("fsync after healing device on fd[%d] failed: %m\n", i); + return 1; + } } } diff --git a/tests/generic/441 b/tests/generic/441 index 2215b64db9a7..e8244224e097 100755 --- a/tests/generic/441 +++ b/tests/generic/441 @@ -45,15 +45,23 @@ _cleanup() . ./common/dmerror # real QA test starts here -_supported_fs ext2 ext3 ext4 xfs _supported_os Linux _require_scratch -# Generally, we want to avoid journal errors in this test. Ensure that -# journalled fs' have a logdev. -if [ "$FSTYP" != "ext2" ]; then - _require_logdev -fi +# Generally, we want to avoid journal errors on the extended testcase. Only +# set the -r flag if we have a logdev +sflag='-s' +case $FSTYP in + btrfs) + _notrun "btrfs has a specialized test for this" + ;; + ext3|ext4|xfs) + # Do the more thorough test if we have a logdev + _has_logdev && sflag='' + ;; + *) + ;; +esac _require_dm_target error _require_test_program fsync-err @@ -70,7 +78,7 @@ _require_fs_space $SCRATCH_MNT 65536 testfile=$SCRATCH_MNT/fsync-err-test -$here/src/fsync-err -d $here/src/dmerror $testfile +$here/src/fsync-err $sflag -d $here/src/dmerror $testfile # success, all done _dmerror_load_working_table