| Message ID | 20240306091935.4090399-4-zlang@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: fix io_uring testing | expand |
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 > >
"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 >> >>
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 > > > > >
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 > >> > >> > >
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)
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(+)