| Message ID | 169335095953.3534600.16325849760213190849.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: io_uring tweaks | expand |
On Tue, Aug 29, 2023 at 04:15:59PM -0700, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@kernel.org> > > Don't enable io_uring in fsstress unless someone asks for it explicitly, > just like fsx. I think both tools should require explicit opt-in to > facilitate A/B testing between the old IO paths and this new one. > > While I was playing with fstests+io_uring, I noticed quite a few > regressions in fstests, which fell into two classes: > > The first class is umount failing with EBUSY. Apparently this is due to > the kernel uring code hanging on to file references even after the > userspace program exits. Tests that run fsstress and immediately > unmount now fail sporadically due to the EBUSY. Unfortunately, the > metadata update stress tests, the recovery loop tests, the xfs online > fsck functional tests, and the xfs fuzz tests make heavy use of > "fsstress; umount" and they fail all over the place now. > > Something's broken, Jens and Christian said it should get fixed, but in > the meantime this is getting in the way of me testing my own code. I'm not seeing regular problems with io_uring on my test machines. Occasionally there will be a filesystem unmount issue, but that's not causing anything but a single test here or there to fail. It's not a big deal. > The second problem I noticed is that fsstress now lodges complaints > about sporadic heap corruption. I /think/ this is due to some kind of > memory mishandling bug when uring is active but IO requests fail, but I > haven't had the time to go figure out what's up with that. Yes, I've seen that happen in ~6.4 kernels, but current TOT doesn't seem to do that anymore on my test machines. Regardless, I don't think turning off io_uring support by default is the right thing to do. That's just shooting the messenger. We really do need this code to be exercised as much as possible because it is so full of bugs. Sure, add a flag to turn it off if you need it off (and add it to FSSTRESS_AVOID for your test environments), but otherwise we really should be exercising io_uring. Ignorance doesn't prevent bugs or CVEs.... Realistically, what we actually need is to require io_uring developers to focus on testing io_uring functionality with filesystems and fsstress and *to fix the regressions* rather than endlessly adding more features and complexity that create more bugs. Turning the code off certainly won't help us acheive that.... Cheers, Dave.
On Thu, Aug 31, 2023 at 09:10:00AM +1000, Dave Chinner wrote: > On Tue, Aug 29, 2023 at 04:15:59PM -0700, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > Don't enable io_uring in fsstress unless someone asks for it explicitly, > > just like fsx. I think both tools should require explicit opt-in to > > facilitate A/B testing between the old IO paths and this new one. > > > > While I was playing with fstests+io_uring, I noticed quite a few > > regressions in fstests, which fell into two classes: > > > > The first class is umount failing with EBUSY. Apparently this is due to > > the kernel uring code hanging on to file references even after the > > userspace program exits. Tests that run fsstress and immediately > > unmount now fail sporadically due to the EBUSY. Unfortunately, the > > metadata update stress tests, the recovery loop tests, the xfs online > > fsck functional tests, and the xfs fuzz tests make heavy use of > > "fsstress; umount" and they fail all over the place now. > > > > Something's broken, Jens and Christian said it should get fixed, but in > > the meantime this is getting in the way of me testing my own code. > > I'm not seeing regular problems with io_uring on my test machines. Me neither. > Occasionally there will be a filesystem unmount issue, but that's > not causing anything but a single test here or there to fail. It's > not a big deal. > > > The second problem I noticed is that fsstress now lodges complaints > > about sporadic heap corruption. I /think/ this is due to some kind of > > memory mishandling bug when uring is active but IO requests fail, but I > > haven't had the time to go figure out what's up with that. > > Yes, I've seen that happen in ~6.4 kernels, but current TOT doesn't > seem to do that anymore on my test machines. > > Regardless, I don't think turning off io_uring support by default is > the right thing to do. That's just shooting the messenger. We really Agree, we'd better to give io_uring a test by default. I've found several regression issues on io_uring by fsstress. If someone feels io_uring breaks his testing, remove the liburing and liburing-devel package, then fsstress won't build io_uring things. Or export FSSTRESS_AVOID="-f uring_read=0 -f uring_write=0". Sometimes, I even removed the IO_URING kernel config then rebuild kernel, to avoid the effection of io_uring code totally. Thanks, Zorro > do need this code to be exercised as much as possible because it is > so full of bugs. Sure, add a flag to turn it off if you need it off > (and add it to FSSTRESS_AVOID for your test environments), but > otherwise we really should be exercising io_uring. Ignorance doesn't > prevent bugs or CVEs.... > > Realistically, what we actually need is to require io_uring > developers to focus on testing io_uring functionality with > filesystems and fsstress and *to fix the regressions* rather than > endlessly adding more features and complexity that create more bugs. > Turning the code off certainly won't help us acheive that.... > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com >
diff --git a/ltp/fsstress.c b/ltp/fsstress.c index abe2874253..f8bb166646 100644 --- a/ltp/fsstress.c +++ b/ltp/fsstress.c @@ -339,8 +339,8 @@ struct opdesc ops[OP_LAST] = { [OP_TRUNCATE] = {"truncate", truncate_f, 2, 1 }, [OP_UNLINK] = {"unlink", unlink_f, 1, 1 }, [OP_UNRESVSP] = {"unresvsp", unresvsp_f, 1, 1 }, - [OP_URING_READ] = {"uring_read", uring_read_f, 1, 0 }, - [OP_URING_WRITE] = {"uring_write", uring_write_f, 1, 1 }, + [OP_URING_READ] = {"uring_read", uring_read_f, -1, 0 }, + [OP_URING_WRITE] = {"uring_write", uring_write_f, -1, 1 }, [OP_WRITE] = {"write", write_f, 4, 1 }, [OP_WRITEV] = {"writev", writev_f, 4, 1 }, [OP_XCHGRANGE] = {"xchgrange", xchgrange_f, 2, 1 }, @@ -507,7 +507,7 @@ int main(int argc, char **argv) xfs_error_injection_t err_inj; struct sigaction action; int loops = 1; - const char *allopts = "cd:e:f:i:l:m:M:n:o:p:rRs:S:vVwx:X:zH"; + const char *allopts = "cd:e:f:i:l:m:M:n:o:p:rRs:S:UvVwx:X:zH"; long long duration; errrange = errtag = 0; @@ -603,6 +603,12 @@ int main(int argc, char **argv) printf("\n"); nousage=1; break; + case 'U': + if (ops[OP_URING_READ].freq == -1) + ops[OP_URING_READ].freq = 1; + if (ops[OP_URING_WRITE].freq == -1) + ops[OP_URING_WRITE].freq = 1; + break; case 'V': verifiable_log = 1; break; @@ -640,6 +646,11 @@ int main(int argc, char **argv) } } + if (ops[OP_URING_READ].freq == -1) + ops[OP_URING_READ].freq = 0; + if (ops[OP_URING_WRITE].freq == -1) + ops[OP_URING_WRITE].freq = 0; + if (!dirname) { /* no directory specified */ if (!nousage) usage(); diff --git a/tests/generic/1220 b/tests/generic/1220 new file mode 100755 index 0000000000..ec8cafba71 --- /dev/null +++ b/tests/generic/1220 @@ -0,0 +1,43 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2017 Oracle, Inc. All Rights Reserved. +# +# FS QA Test No. 1220 +# +# Run an all-writes fsstress run with multiple threads and io_uring to shake +# out bugs in the write path. +# +. ./common/preamble +_begin_fstest auto rw long_rw stress soak smoketest + +# Override the default cleanup function. +_cleanup() +{ + cd / + rm -f $tmp.* + $KILLALL_PROG -9 fsstress > /dev/null 2>&1 +} + +# Import common functions. + +# Modify as appropriate. +_supported_fs generic + +_require_scratch +_require_command "$KILLALL_PROG" "killall" + +echo "Silence is golden." + +_scratch_mkfs > $seqres.full 2>&1 +_scratch_mount >> $seqres.full 2>&1 + +nr_cpus=$((LOAD_FACTOR * 4)) +nr_ops=$((25000 * nr_cpus * TIME_FACTOR)) +fsstress_args=(-w -d $SCRATCH_MNT -n $nr_ops -p $nr_cpus -U) +test -n "$SOAK_DURATION" && fsstress_args+=(--duration="$SOAK_DURATION") + +$FSSTRESS_PROG $FSSTRESS_AVOID "${fsstress_args[@]}" >> $seqres.full + +# success, all done +status=0 +exit diff --git a/tests/generic/1220.out b/tests/generic/1220.out new file mode 100644 index 0000000000..2583a6bf73 --- /dev/null +++ b/tests/generic/1220.out @@ -0,0 +1,2 @@ +QA output created by 1220 +Silence is golden.