[PATCHSET 0/2] fstests: io_uring tweaks

[PATCHSET 0/2] fstests: io_uring tweaks

[Darrick J. Wong]

Hi all,

I recently added io_uring to my main testing configuration, and a couple
of things stood out to me: First, generic/616 and generic/617 are long
soak test cases specifically for io_uring.  Therefore, we ought to allow
control via SOAK_DURATION.

The second problem is nastier.  If liburing is present, fsstress will
/always/ try to use it, unlike fsx, which requires a -U argument.  I
think both tools should require explicit opt-in to facilitate A/B
testing between the old IO paths and this new one, so I added a similar
-U switch to fsstress and a new testcase for it.

While I was doing that, 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 due to the EBUSY.  As a
result, nearly all of the online fsck functional test cases and the
metadata update stress tests now exhibit sporadic failures.  Requiring
explicit opt-in to uring makes that go away.

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 where that happens.

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been running on the djcloud for months with no problems.  Enjoy!
Comments and questions are, as always, welcome.

--D

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=uring-tweaks
---
 ltp/fsstress.c         |   17 ++++++++++++++---
 tests/generic/1220     |   43 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/1220.out |    2 ++
 tests/generic/616      |    1 +
 tests/generic/617      |    1 +
 5 files changed, 61 insertions(+), 3 deletions(-)
 create mode 100755 tests/generic/1220
 create mode 100644 tests/generic/1220.out

[PATCH 1/2] generic/61[67]: support SOAK_DURATION

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Now that I've finally gotten liburing installed on my test machine, I
can actually test io_uring.  Adapt these two tests to support
SOAK_DURATION so I can add it to that too.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/generic/616 |    1 +
 tests/generic/617 |    1 +
 2 files changed, 2 insertions(+)


diff --git a/tests/generic/616 b/tests/generic/616
index 538b480ba7..729898fded 100755
--- a/tests/generic/616
+++ b/tests/generic/616
@@ -33,6 +33,7 @@ fsx_args+=(-N $nr_ops)
 fsx_args+=(-p $((nr_ops / 100)))
 fsx_args+=(-o $op_sz)
 fsx_args+=(-l $file_sz)
+test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
 
 run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
 
diff --git a/tests/generic/617 b/tests/generic/617
index 3bb3112e99..f0fd1feb2e 100755
--- a/tests/generic/617
+++ b/tests/generic/617
@@ -39,6 +39,7 @@ fsx_args+=(-r $min_dio_sz)
 fsx_args+=(-t $min_dio_sz)
 fsx_args+=(-w $min_dio_sz)
 fsx_args+=(-Z)
+test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
 
 run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
 

Re: [PATCH 1/2] generic/61[67]: support SOAK_DURATION

[Zorro Lang]

On Tue, Aug 29, 2023 at 04:15:53PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that I've finally gotten liburing installed on my test machine, I
> can actually test io_uring.  Adapt these two tests to support
> SOAK_DURATION so I can add it to that too.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

This makes sense to me, but how these two cases into "soak" group. I'd like to
add all cases which supports "SOAK_DURATION" into "soak" group at least. Then
we can do soak tests better.

Thanks,
Zorro

>  tests/generic/616 |    1 +
>  tests/generic/617 |    1 +
>  2 files changed, 2 insertions(+)
> 
> 
> diff --git a/tests/generic/616 b/tests/generic/616
> index 538b480ba7..729898fded 100755
> --- a/tests/generic/616
> +++ b/tests/generic/616
> @@ -33,6 +33,7 @@ fsx_args+=(-N $nr_ops)
>  fsx_args+=(-p $((nr_ops / 100)))
>  fsx_args+=(-o $op_sz)
>  fsx_args+=(-l $file_sz)
> +test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
>  
>  run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
>  
> diff --git a/tests/generic/617 b/tests/generic/617
> index 3bb3112e99..f0fd1feb2e 100755
> --- a/tests/generic/617
> +++ b/tests/generic/617
> @@ -39,6 +39,7 @@ fsx_args+=(-r $min_dio_sz)
>  fsx_args+=(-t $min_dio_sz)
>  fsx_args+=(-w $min_dio_sz)
>  fsx_args+=(-Z)
> +test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
>  
>  run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
>  
> 

[PATCH v2 1/2] generic/61[67]: support SOAK_DURATION

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Now that I've finally gotten liburing installed on my test machine, I
can actually test io_uring.  Adapt these two tests to support
SOAK_DURATION so I can add it to that too.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
v2: add to soak group
---
 tests/generic/616 |    3 ++-
 tests/generic/617 |    3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/generic/616 b/tests/generic/616
index 538b480ba7..5b0b02c5e4 100755
--- a/tests/generic/616
+++ b/tests/generic/616
@@ -8,7 +8,7 @@
 # fsx ops to limit the testing time to be an auto group test.
 #
 . ./common/preamble
-_begin_fstest auto rw io_uring stress
+_begin_fstest auto rw io_uring stress soak
 
 # Import common functions.
 . ./common/filter
@@ -33,6 +33,7 @@ fsx_args+=(-N $nr_ops)
 fsx_args+=(-p $((nr_ops / 100)))
 fsx_args+=(-o $op_sz)
 fsx_args+=(-l $file_sz)
+test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
 
 run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
 
diff --git a/tests/generic/617 b/tests/generic/617
index 3bb3112e99..a977870023 100755
--- a/tests/generic/617
+++ b/tests/generic/617
@@ -8,7 +8,7 @@
 # fsx ops to limit the testing time to be an auto group test.
 #
 . ./common/preamble
-_begin_fstest auto rw io_uring stress
+_begin_fstest auto rw io_uring stress soak
 
 # Import common functions.
 . ./common/filter
@@ -39,6 +39,7 @@ fsx_args+=(-r $min_dio_sz)
 fsx_args+=(-t $min_dio_sz)
 fsx_args+=(-w $min_dio_sz)
 fsx_args+=(-Z)
+test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
 
 run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
 

Re: [PATCH v2 1/2] generic/61[67]: support SOAK_DURATION

[Zorro Lang]

On Fri, Sep 01, 2023 at 07:53:31AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Now that I've finally gotten liburing installed on my test machine, I
> can actually test io_uring.  Adapt these two tests to support
> SOAK_DURATION so I can add it to that too.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> v2: add to soak group
> ---

Thanks! This version looks good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/generic/616 |    3 ++-
>  tests/generic/617 |    3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/616 b/tests/generic/616
> index 538b480ba7..5b0b02c5e4 100755
> --- a/tests/generic/616
> +++ b/tests/generic/616
> @@ -8,7 +8,7 @@
>  # fsx ops to limit the testing time to be an auto group test.
>  #
>  . ./common/preamble
> -_begin_fstest auto rw io_uring stress
> +_begin_fstest auto rw io_uring stress soak
>  
>  # Import common functions.
>  . ./common/filter
> @@ -33,6 +33,7 @@ fsx_args+=(-N $nr_ops)
>  fsx_args+=(-p $((nr_ops / 100)))
>  fsx_args+=(-o $op_sz)
>  fsx_args+=(-l $file_sz)
> +test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
>  
>  run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
>  
> diff --git a/tests/generic/617 b/tests/generic/617
> index 3bb3112e99..a977870023 100755
> --- a/tests/generic/617
> +++ b/tests/generic/617
> @@ -8,7 +8,7 @@
>  # fsx ops to limit the testing time to be an auto group test.
>  #
>  . ./common/preamble
> -_begin_fstest auto rw io_uring stress
> +_begin_fstest auto rw io_uring stress soak
>  
>  # Import common functions.
>  . ./common/filter
> @@ -39,6 +39,7 @@ fsx_args+=(-r $min_dio_sz)
>  fsx_args+=(-t $min_dio_sz)
>  fsx_args+=(-w $min_dio_sz)
>  fsx_args+=(-Z)
> +test -n "$SOAK_DURATION" && fsx_args+=(--duration="$SOAK_DURATION")
>  
>  run_fsx "${fsx_args[@]}" | sed -e '/^fsx.*/d'
>  
> 

[PATCH 2/2] generic: only enable io_uring in fsstress explicitly

[Darrick J. Wong]

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.

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.

Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wj8RuUosugVZk+iqCAq7x6rs=7C-9sUXcO2heu4dCuOVw@mail.gmail.com/
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 ltp/fsstress.c         |   17 ++++++++++++++---
 tests/generic/1220     |   43 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/1220.out |    2 ++
 3 files changed, 59 insertions(+), 3 deletions(-)
 create mode 100755 tests/generic/1220
 create mode 100644 tests/generic/1220.out


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.

Re: [PATCH 2/2] generic: only enable io_uring in fsstress explicitly

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] generic: only enable io_uring in fsstress explicitly

[Zorro Lang]

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
>