[PATCH 0/3] fstests: fix io_uring testing

[PATCH 0/3] fstests: fix io_uring testing

[Zorro Lang]

According to the manual of io_uring_queue_init, it doesn't set errno
but return the -errno on failure. So we should check the return value
of io_uring_queue_init, to make sure if the io_uring is supported by
kernel. We've left this problem in xfstests/ltp/fsstress.c long time.
(Refer to PATCH 1/3)

And besides kernel build without CONFIG_IO_URING, a system can disable
the io_uring supporting manually, by set sysctl kernel.io_uring_disabled=2.
The former cause io_uring_queue_init return ENOSYS, but the latter will
return EPERM. So I let fsstress to deal with both situations.
(Refer to PATCH 2/3)

A question is if we should do "sysctl -w kernel.io_uring_disabled=0 &> /dev/null"
at the beginning of each test case (e.g. do that in common/config ?), or leave
this decision to the testers (in their test wrapper). Now I only do that
in _require_io_uring(). If anyone has any opinions, feel free to reply.
(Refer to PATCH 3/3)

Thanks,
Zorro

[PATCH 1/3] fsstress: check io_uring_queue_init errno properly

[Zorro Lang]

As the manual of io_uring_queue_init says "io_uring_queue_init(3)
returns 0 on success and -errno on failure". We should check if the
return value is -ENOSYS, not the errno.

Fixes: d15b1721f284 ("ltp/fsstress: don't fail on io_uring ENOSYS")
Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 ltp/fsstress.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 63c75767..482395c4 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -763,8 +763,8 @@ int main(int argc, char **argv)
 #ifdef URING
 			have_io_uring = true;
 			/* If ENOSYS, just ignore uring, other errors are fatal. */
-			if (io_uring_queue_init(URING_ENTRIES, &ring, 0)) {
-				if (errno == ENOSYS) {
+			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
+				if (c == -ENOSYS) {
 					have_io_uring = false;
 				} else {
 					fprintf(stderr, "io_uring_queue_init failed\n");
-- 
2.43.0

Re: [PATCH 1/3] fsstress: check io_uring_queue_init errno properly

[Darrick J. Wong]

On Wed, Mar 06, 2024 at 05:19:33PM +0800, Zorro Lang wrote:
> As the manual of io_uring_queue_init says "io_uring_queue_init(3)
> returns 0 on success and -errno on failure". We should check if the
> return value is -ENOSYS, not the errno.

/me checks liburing source code and sees that the library returns a
negative error code without touching errno (the semi global error code
variable) at all.  That's an unfortunate quirk of the manpage, but this
code here is correct...

> Fixes: d15b1721f284 ("ltp/fsstress: don't fail on io_uring ENOSYS")
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  ltp/fsstress.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 63c75767..482395c4 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -763,8 +763,8 @@ int main(int argc, char **argv)
>  #ifdef URING
>  			have_io_uring = true;
>  			/* If ENOSYS, just ignore uring, other errors are fatal. */
> -			if (io_uring_queue_init(URING_ENTRIES, &ring, 0)) {
> -				if (errno == ENOSYS) {
> +			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
> +				if (c == -ENOSYS) {
>  					have_io_uring = false;
>  				} else {
>  					fprintf(stderr, "io_uring_queue_init failed\n");

But why not:

			c = io_uring_queue_init(...);
			switch (c) {
			case 0:
				have_io_uring = true;
				break;
			case -ENOSYS:
				have_io_uring = false;
				break;
			default:
				fprintf(stderr, "io_uring_queue_init failed\n");
				break;
			}

Especially since you add another case in the next patch?

I'll leave the style nits up to you though:
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


--D

> -- 
> 2.43.0
> 
> 

Re: [PATCH 1/3] fsstress: check io_uring_queue_init errno properly

[Zorro Lang]

On Wed, Mar 06, 2024 at 07:53:57AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 06, 2024 at 05:19:33PM +0800, Zorro Lang wrote:
> > As the manual of io_uring_queue_init says "io_uring_queue_init(3)
> > returns 0 on success and -errno on failure". We should check if the
> > return value is -ENOSYS, not the errno.
> 
> /me checks liburing source code and sees that the library returns a
> negative error code without touching errno (the semi global error code
> variable) at all.  That's an unfortunate quirk of the manpage, but this
> code here is correct...

Yeah, that confuse me too, especially some io_uring functions set errno,
some return -errno ...

> 
> > Fixes: d15b1721f284 ("ltp/fsstress: don't fail on io_uring ENOSYS")
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  ltp/fsstress.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > index 63c75767..482395c4 100644
> > --- a/ltp/fsstress.c
> > +++ b/ltp/fsstress.c
> > @@ -763,8 +763,8 @@ int main(int argc, char **argv)
> >  #ifdef URING
> >  			have_io_uring = true;
> >  			/* If ENOSYS, just ignore uring, other errors are fatal. */
> > -			if (io_uring_queue_init(URING_ENTRIES, &ring, 0)) {
> > -				if (errno == ENOSYS) {
> > +			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
> > +				if (c == -ENOSYS) {
> >  					have_io_uring = false;
> >  				} else {
> >  					fprintf(stderr, "io_uring_queue_init failed\n");
> 
> But why not:
> 
> 			c = io_uring_queue_init(...);
> 			switch (c) {
> 			case 0:
> 				have_io_uring = true;
> 				break;
> 			case -ENOSYS:
> 				have_io_uring = false;
> 				break;
> 			default:
> 				fprintf(stderr, "io_uring_queue_init failed\n");
> 				break;
> 			}
> 
> Especially since you add another case in the next patch?

Sure, that looks more clearly, I'll change this part. Thanks!

> 
> I'll leave the style nits up to you though:
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> 
> --D
> 
> 
> --D
> 
> > -- 
> > 2.43.0
> > 
> > 
> 

Re: [PATCH 1/3] fsstress: check io_uring_queue_init errno properly

[Jeff Moyer]

Zorro Lang <zlang@kernel.org> writes:

> As the manual of io_uring_queue_init says "io_uring_queue_init(3)
> returns 0 on success and -errno on failure". We should check if the
> return value is -ENOSYS, not the errno.
>
> Fixes: d15b1721f284 ("ltp/fsstress: don't fail on io_uring ENOSYS")
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  ltp/fsstress.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 63c75767..482395c4 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -763,8 +763,8 @@ int main(int argc, char **argv)
>  #ifdef URING
>  			have_io_uring = true;
>  			/* If ENOSYS, just ignore uring, other errors are fatal. */
> -			if (io_uring_queue_init(URING_ENTRIES, &ring, 0)) {
> -				if (errno == ENOSYS) {
> +			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
> +				if (c == -ENOSYS) {
>  					have_io_uring = false;
>  				} else {
>  					fprintf(stderr, "io_uring_queue_init failed\n");

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

[PATCH 2/3] fsstress: bypass io_uring testing if io_uring_queue_init returns EPERM

[Zorro Lang]

I found the io_uring testing still fails as:
  io_uring_queue_init failed
even if kernel supports io_uring feature.

That because of the /proc/sys/kernel/io_uring_disabled isn't 0.

Different value means:
  0 All processes can create io_uring instances as normal.
  1 io_uring creation is disabled (io_uring_setup() will fail with
    -EPERM) for unprivileged processes not in the io_uring_group
    group. Existing io_uring instances can still be used.  See the
    documentation for io_uring_group for more information.
  2 io_uring creation is disabled for all processes. io_uring_setup()
    always fails with -EPERM. Existing io_uring instances can still
    be used.

So besides the CONFIG_IO_URING kernel config, there's another switch
can on or off the io_uring supporting. And the "2" or "1" might be
the default on some systems.

On this situation the io_uring_queue_init returns -EPERM, so I change
the fsstress to ignore io_uring testing if io_uring_queue_init returns
-ENOSYS or -EPERM. And print different verbose message for debug.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 ltp/fsstress.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 482395c4..9c75f27b 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -762,12 +762,23 @@ int main(int argc, char **argv)
 #endif
 #ifdef URING
 			have_io_uring = true;
-			/* If ENOSYS, just ignore uring, other errors are fatal. */
+			/*
+			 * If ENOSYS, just ignore uring, due to kernel doesn't support it.
+			 * If EPERM, might due to sysctl kernel.io_uring_disabled isn't 0,
+			 *           or some selinux policies, etc.
+			 * Other errors are fatal.
+			 */
 			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
 				if (c == -ENOSYS) {
 					have_io_uring = false;
+					if (verbose)
+						printf("io_uring isn't supported by kernel\n");
+				} else if (c == -EPERM) {
+					have_io_uring = false;
+					if (verbose)
+						printf("io_uring isn't allowed, check io_uring_disabled sysctl or selinux policy\n");
 				} else {
-					fprintf(stderr, "io_uring_queue_init failed\n");
+					fprintf(stderr, "io_uring_queue_init failed, errno=%d\n", c);
 					exit(1);
 				}
 			}
-- 
2.43.0

Re: [PATCH 2/3] fsstress: bypass io_uring testing if io_uring_queue_init returns EPERM

[Darrick J. Wong]

On Wed, Mar 06, 2024 at 05:19:34PM +0800, Zorro Lang wrote:
> I found the io_uring testing still fails as:
>   io_uring_queue_init failed
> even if kernel supports io_uring feature.
> 
> That because of the /proc/sys/kernel/io_uring_disabled isn't 0.
> 
> Different value means:
>   0 All processes can create io_uring instances as normal.
>   1 io_uring creation is disabled (io_uring_setup() will fail with
>     -EPERM) for unprivileged processes not in the io_uring_group
>     group. Existing io_uring instances can still be used.  See the
>     documentation for io_uring_group for more information.
>   2 io_uring creation is disabled for all processes. io_uring_setup()
>     always fails with -EPERM. Existing io_uring instances can still
>     be used.
> 
> So besides the CONFIG_IO_URING kernel config, there's another switch
> can on or off the io_uring supporting. And the "2" or "1" might be
> the default on some systems.
> 
> On this situation the io_uring_queue_init returns -EPERM, so I change
> the fsstress to ignore io_uring testing if io_uring_queue_init returns
> -ENOSYS or -EPERM. And print different verbose message for debug.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  ltp/fsstress.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 482395c4..9c75f27b 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -762,12 +762,23 @@ int main(int argc, char **argv)
>  #endif
>  #ifdef URING
>  			have_io_uring = true;
> -			/* If ENOSYS, just ignore uring, other errors are fatal. */
> +			/*
> +			 * If ENOSYS, just ignore uring, due to kernel doesn't support it.
> +			 * If EPERM, might due to sysctl kernel.io_uring_disabled isn't 0,

"might be due to..."

With that fixed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +			 *           or some selinux policies, etc.
> +			 * Other errors are fatal.
> +			 */
>  			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
>  				if (c == -ENOSYS) {
>  					have_io_uring = false;
> +					if (verbose)
> +						printf("io_uring isn't supported by kernel\n");
> +				} else if (c == -EPERM) {
> +					have_io_uring = false;
> +					if (verbose)
> +						printf("io_uring isn't allowed, check io_uring_disabled sysctl or selinux policy\n");
>  				} else {
> -					fprintf(stderr, "io_uring_queue_init failed\n");
> +					fprintf(stderr, "io_uring_queue_init failed, errno=%d\n", c);
>  					exit(1);
>  				}
>  			}
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/3] fsstress: bypass io_uring testing if io_uring_queue_init returns EPERM

[Zorro Lang]

On Wed, Mar 06, 2024 at 07:55:26AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 06, 2024 at 05:19:34PM +0800, Zorro Lang wrote:
> > I found the io_uring testing still fails as:
> >   io_uring_queue_init failed
> > even if kernel supports io_uring feature.
> > 
> > That because of the /proc/sys/kernel/io_uring_disabled isn't 0.
> > 
> > Different value means:
> >   0 All processes can create io_uring instances as normal.
> >   1 io_uring creation is disabled (io_uring_setup() will fail with
> >     -EPERM) for unprivileged processes not in the io_uring_group
> >     group. Existing io_uring instances can still be used.  See the
> >     documentation for io_uring_group for more information.
> >   2 io_uring creation is disabled for all processes. io_uring_setup()
> >     always fails with -EPERM. Existing io_uring instances can still
> >     be used.
> > 
> > So besides the CONFIG_IO_URING kernel config, there's another switch
> > can on or off the io_uring supporting. And the "2" or "1" might be
> > the default on some systems.
> > 
> > On this situation the io_uring_queue_init returns -EPERM, so I change
> > the fsstress to ignore io_uring testing if io_uring_queue_init returns
> > -ENOSYS or -EPERM. And print different verbose message for debug.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  ltp/fsstress.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > index 482395c4..9c75f27b 100644
> > --- a/ltp/fsstress.c
> > +++ b/ltp/fsstress.c
> > @@ -762,12 +762,23 @@ int main(int argc, char **argv)
> >  #endif
> >  #ifdef URING
> >  			have_io_uring = true;
> > -			/* If ENOSYS, just ignore uring, other errors are fatal. */
> > +			/*
> > +			 * If ENOSYS, just ignore uring, due to kernel doesn't support it.
> > +			 * If EPERM, might due to sysctl kernel.io_uring_disabled isn't 0,
> 
> "might be due to..."

Hahaha, as native english speaker so :)

> 
> With that fixed,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks, I'll change below if...else... to switch... as you suggested.

> 
> --D
> 
> > +			 *           or some selinux policies, etc.
> > +			 * Other errors are fatal.
> > +			 */
> >  			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
> >  				if (c == -ENOSYS) {
> >  					have_io_uring = false;
> > +					if (verbose)
> > +						printf("io_uring isn't supported by kernel\n");
> > +				} else if (c == -EPERM) {
> > +					have_io_uring = false;
> > +					if (verbose)
> > +						printf("io_uring isn't allowed, check io_uring_disabled sysctl or selinux policy\n");
> >  				} else {
> > -					fprintf(stderr, "io_uring_queue_init failed\n");
> > +					fprintf(stderr, "io_uring_queue_init failed, errno=%d\n", c);
> >  					exit(1);
> >  				}
> >  			}
> > -- 
> > 2.43.0
> > 
> > 
> 

Re: [PATCH 2/3] fsstress: bypass io_uring testing if io_uring_queue_init returns EPERM

[Jeff Moyer]

Zorro Lang <zlang@kernel.org> writes:

> I found the io_uring testing still fails as:
>   io_uring_queue_init failed
> even if kernel supports io_uring feature.
>
> That because of the /proc/sys/kernel/io_uring_disabled isn't 0.
>
> Different value means:
>   0 All processes can create io_uring instances as normal.
>   1 io_uring creation is disabled (io_uring_setup() will fail with
>     -EPERM) for unprivileged processes not in the io_uring_group
>     group. Existing io_uring instances can still be used.  See the
>     documentation for io_uring_group for more information.
>   2 io_uring creation is disabled for all processes. io_uring_setup()
>     always fails with -EPERM. Existing io_uring instances can still
>     be used.
>
> So besides the CONFIG_IO_URING kernel config, there's another switch
> can on or off the io_uring supporting. And the "2" or "1" might be
> the default on some systems.
>
> On this situation the io_uring_queue_init returns -EPERM, so I change
> the fsstress to ignore io_uring testing if io_uring_queue_init returns
> -ENOSYS or -EPERM. And print different verbose message for debug.
>
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  ltp/fsstress.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> index 482395c4..9c75f27b 100644
> --- a/ltp/fsstress.c
> +++ b/ltp/fsstress.c
> @@ -762,12 +762,23 @@ int main(int argc, char **argv)
>  #endif
>  #ifdef URING
>  			have_io_uring = true;
> -			/* If ENOSYS, just ignore uring, other errors are fatal. */
> +			/*
> +			 * If ENOSYS, just ignore uring, due to kernel doesn't support it.
> +			 * If EPERM, might due to sysctl kernel.io_uring_disabled isn't 0,
> +			 *           or some selinux policies, etc.
> +			 * Other errors are fatal.
> +			 */
>  			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
>  				if (c == -ENOSYS) {
>  					have_io_uring = false;
> +					if (verbose)
> +						printf("io_uring isn't supported by kernel\n");
> +				} else if (c == -EPERM) {
> +					have_io_uring = false;
> +					if (verbose)
> +						printf("io_uring isn't allowed, check io_uring_disabled sysctl or selinux policy\n");
>  				} else {
> -					fprintf(stderr, "io_uring_queue_init failed\n");
> +					fprintf(stderr, "io_uring_queue_init failed, errno=%d\n", c);

I think you want to use -c here, right?

Other than that:

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Re: [PATCH 2/3] fsstress: bypass io_uring testing if io_uring_queue_init returns EPERM

[Zorro Lang]

On Wed, Mar 06, 2024 at 10:57:08AM -0500, Jeff Moyer wrote:
> Zorro Lang <zlang@kernel.org> writes:
> 
> > I found the io_uring testing still fails as:
> >   io_uring_queue_init failed
> > even if kernel supports io_uring feature.
> >
> > That because of the /proc/sys/kernel/io_uring_disabled isn't 0.
> >
> > Different value means:
> >   0 All processes can create io_uring instances as normal.
> >   1 io_uring creation is disabled (io_uring_setup() will fail with
> >     -EPERM) for unprivileged processes not in the io_uring_group
> >     group. Existing io_uring instances can still be used.  See the
> >     documentation for io_uring_group for more information.
> >   2 io_uring creation is disabled for all processes. io_uring_setup()
> >     always fails with -EPERM. Existing io_uring instances can still
> >     be used.
> >
> > So besides the CONFIG_IO_URING kernel config, there's another switch
> > can on or off the io_uring supporting. And the "2" or "1" might be
> > the default on some systems.
> >
> > On this situation the io_uring_queue_init returns -EPERM, so I change
> > the fsstress to ignore io_uring testing if io_uring_queue_init returns
> > -ENOSYS or -EPERM. And print different verbose message for debug.
> >
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  ltp/fsstress.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/ltp/fsstress.c b/ltp/fsstress.c
> > index 482395c4..9c75f27b 100644
> > --- a/ltp/fsstress.c
> > +++ b/ltp/fsstress.c
> > @@ -762,12 +762,23 @@ int main(int argc, char **argv)
> >  #endif
> >  #ifdef URING
> >  			have_io_uring = true;
> > -			/* If ENOSYS, just ignore uring, other errors are fatal. */
> > +			/*
> > +			 * If ENOSYS, just ignore uring, due to kernel doesn't support it.
> > +			 * If EPERM, might due to sysctl kernel.io_uring_disabled isn't 0,
> > +			 *           or some selinux policies, etc.
> > +			 * Other errors are fatal.
> > +			 */
> >  			if ((c = io_uring_queue_init(URING_ENTRIES, &ring, 0)) != 0) {
> >  				if (c == -ENOSYS) {
> >  					have_io_uring = false;
> > +					if (verbose)
> > +						printf("io_uring isn't supported by kernel\n");
> > +				} else if (c == -EPERM) {
> > +					have_io_uring = false;
> > +					if (verbose)
> > +						printf("io_uring isn't allowed, check io_uring_disabled sysctl or selinux policy\n");
> >  				} else {
> > -					fprintf(stderr, "io_uring_queue_init failed\n");
> > +					fprintf(stderr, "io_uring_queue_init failed, errno=%d\n", c);
> 
> I think you want to use -c here, right?

Sure, will change that, thanks!

> 
> Other than that:
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
> 
> 

[PATCH 3/3] common/rc: force enable io_uring in _require_io_uring

[Zorro Lang]

If kernel supports io_uring, userspace still can/might disable that
supporting by set /proc/sys/kernel/io_uring_disabled=2. Let's set
it to 0, to always enable io_uring (ignore error if there's not
that file).

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 common/rc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/common/rc b/common/rc
index 50dde313..966c92e3 100644
--- a/common/rc
+++ b/common/rc
@@ -2317,6 +2317,9 @@ _require_aiodio()
 # this test requires that the kernel supports IO_URING
 _require_io_uring()
 {
+	# Force enable io_uring if kernel supports it
+	sysctl -w kernel.io_uring_disabled=0 &> /dev/null
+
 	$here/src/feature -R
 	case $? in
 	0)
-- 
2.43.0

Re: [PATCH 3/3] common/rc: force enable io_uring in _require_io_uring

[Darrick J. Wong]

On Wed, Mar 06, 2024 at 05:19:35PM +0800, Zorro Lang wrote:
> If kernel supports io_uring, userspace still can/might disable that
> supporting by set /proc/sys/kernel/io_uring_disabled=2. Let's set
> it to 0, to always enable io_uring (ignore error if there's not
> that file).
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  common/rc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/common/rc b/common/rc
> index 50dde313..966c92e3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2317,6 +2317,9 @@ _require_aiodio()
>  # this test requires that the kernel supports IO_URING
>  _require_io_uring()
>  {
> +	# Force enable io_uring if kernel supports it
> +	sysctl -w kernel.io_uring_disabled=0 &> /dev/null

_require_XXX functions are supposed to be predicates that _notrun the
test if XXX isn't configured or available.  Shouldn't this be:

	local io_uring_knob="$(sysctl --values kernel.io_uring_disabled)"
	test "$io_uring_knob" -ne 0 && _notrun "io_uring disabled by admin"

Alternately -- if it _is_ ok to turn this knob, then there should be a
cleanup method to put it back after the test.

--D

> +
>  	$here/src/feature -R
>  	case $? in
>  	0)
> -- 
> 2.43.0
> 
> 

Re: [PATCH 3/3] common/rc: force enable io_uring in _require_io_uring

[Jeff Moyer]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Wed, Mar 06, 2024 at 05:19:35PM +0800, Zorro Lang wrote:
>> If kernel supports io_uring, userspace still can/might disable that
>> supporting by set /proc/sys/kernel/io_uring_disabled=2. Let's set
>> it to 0, to always enable io_uring (ignore error if there's not
>> that file).
>> 
>> Signed-off-by: Zorro Lang <zlang@kernel.org>
>> ---
>>  common/rc | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/common/rc b/common/rc
>> index 50dde313..966c92e3 100644
>> --- a/common/rc
>> +++ b/common/rc
>> @@ -2317,6 +2317,9 @@ _require_aiodio()
>>  # this test requires that the kernel supports IO_URING
>>  _require_io_uring()
>>  {
>> +	# Force enable io_uring if kernel supports it
>> +	sysctl -w kernel.io_uring_disabled=0 &> /dev/null
>
> _require_XXX functions are supposed to be predicates that _notrun the
> test if XXX isn't configured or available.  Shouldn't this be:
>
> 	local io_uring_knob="$(sysctl --values kernel.io_uring_disabled)"
> 	test "$io_uring_knob" -ne 0 && _notrun "io_uring disabled by admin"

That sounds like a good option to me.

> Alternately -- if it _is_ ok to turn this knob, then there should be a
> cleanup method to put it back after the test.

I think it would be better not to change the setting, especially if the
admin had disabled it.

Cheers,
Jeff

>
> --D
>
>> +
>>  	$here/src/feature -R
>>  	case $? in
>>  	0)
>> -- 
>> 2.43.0
>> 
>> 

Re: [PATCH 3/3] common/rc: force enable io_uring in _require_io_uring

[Zorro Lang]

On Wed, Mar 06, 2024 at 10:59:25AM -0500, Jeff Moyer wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> 
> > On Wed, Mar 06, 2024 at 05:19:35PM +0800, Zorro Lang wrote:
> >> If kernel supports io_uring, userspace still can/might disable that
> >> supporting by set /proc/sys/kernel/io_uring_disabled=2. Let's set
> >> it to 0, to always enable io_uring (ignore error if there's not
> >> that file).
> >> 
> >> Signed-off-by: Zorro Lang <zlang@kernel.org>
> >> ---
> >>  common/rc | 3 +++
> >>  1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/common/rc b/common/rc
> >> index 50dde313..966c92e3 100644
> >> --- a/common/rc
> >> +++ b/common/rc
> >> @@ -2317,6 +2317,9 @@ _require_aiodio()
> >>  # this test requires that the kernel supports IO_URING
> >>  _require_io_uring()
> >>  {
> >> +	# Force enable io_uring if kernel supports it
> >> +	sysctl -w kernel.io_uring_disabled=0 &> /dev/null
> >
> > _require_XXX functions are supposed to be predicates that _notrun the
> > test if XXX isn't configured or available.  Shouldn't this be:
> >
> > 	local io_uring_knob="$(sysctl --values kernel.io_uring_disabled)"
> > 	test "$io_uring_knob" -ne 0 && _notrun "io_uring disabled by admin"
> 
> That sounds like a good option to me.
> 
> > Alternately -- if it _is_ ok to turn this knob, then there should be a
> > cleanup method to put it back after the test.
> 
> I think it would be better not to change the setting, especially if the
> admin had disabled it.

As a testing environment/system, even if fstests doesn't touch the io_uring_disabled
sysctl, the testers should care about that before doing his test. So the fstests
users might need to change the io_uring_disabled=0 manually, or do it in their CI
test scripts. Maybe we should leave this job to the users.

Thanks,
Zorro

> 
> Cheers,
> Jeff
> 
> >
> > --D
> >
> >> +
> >>  	$here/src/feature -R
> >>  	case $? in
> >>  	0)
> >> -- 
> >> 2.43.0
> >> 
> >> 
> 
> 

Re: [PATCH 3/3] common/rc: force enable io_uring in _require_io_uring

[Zorro Lang]

On Wed, Mar 06, 2024 at 07:43:24AM -0800, Darrick J. Wong wrote:
> On Wed, Mar 06, 2024 at 05:19:35PM +0800, Zorro Lang wrote:
> > If kernel supports io_uring, userspace still can/might disable that
> > supporting by set /proc/sys/kernel/io_uring_disabled=2. Let's set
> > it to 0, to always enable io_uring (ignore error if there's not
> > that file).
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> >  common/rc | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/common/rc b/common/rc
> > index 50dde313..966c92e3 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2317,6 +2317,9 @@ _require_aiodio()
> >  # this test requires that the kernel supports IO_URING
> >  _require_io_uring()
> >  {
> > +	# Force enable io_uring if kernel supports it
> > +	sysctl -w kernel.io_uring_disabled=0 &> /dev/null
> 
> _require_XXX functions are supposed to be predicates that _notrun the
> test if XXX isn't configured or available.  Shouldn't this be:

Yeah, that makes sense, I tried to find a place to force enable io_uring,
forgot it's not proper for _require_XXX function.

> 
> 	local io_uring_knob="$(sysctl --values kernel.io_uring_disabled)"
> 	test "$io_uring_knob" -ne 0 && _notrun "io_uring disabled by admin"

And I think I need to change the src/feature.c:check_uring_support() too,
check -EPERM as I did in fsstress.c.

> 
> Alternately -- if it _is_ ok to turn this knob, then there should be a
> cleanup method to put it back after the test.

Yeah, I'm still wondering if we should let fstests touch/set the io_uring_disabled
sysctl. Or we leave this job to the fstests user/wrapper script, fstests just
_notrun if io_uring is off.

Thanks,
Zorro

> 
> --D
> 
> > +
> >  	$here/src/feature -R
> >  	case $? in
> >  	0)
> > -- 
> > 2.43.0
> > 
> > 
>