| Message ID | 7ca733f5-f33a-4b2e-a70d-4a48a4904391@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] common/rc: explicitly test for engine availability in _require_fio | expand |
On Fri, Mar 14, 2025 at 09:56:20AM -0500, Eric Sandeen wrote: > The current test in _require_fio (--warnings-fatal --showcmd) does not > fail if an invalid/unavailable io engine is specified. > > Add an explicit test that every requested io engine in the job file > is actually available. > > Remove the "ioe_e4defrag" entries in ext4 tests - an engine with this > name has seemingly never existed, but in each case later stanzas > overrode the io engine, so it did not cause problems without this > explicit parsing and checking. Well, they override it with the correct name "e4defrag": $ fio --enghelp Available IO engines: cpuio mmap sync <snip> falloc e4defrag I think the commit message should call that out specifically "...but in each case later stanzas provide the correct ioengine name 'e4defrag', so..."). > Signed-off-by: Eric Sandeen <sandeen@redhat.com> With that amended, this all looks good to me so Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> --D > --- > > V2: Be Better at Bash, thanks Zorro. Also add | sort | uniq to ensure > we only test each requested engine once. > > diff --git a/common/rc b/common/rc > index ca755055..ac443ec2 100644 > --- a/common/rc > +++ b/common/rc > @@ -3977,12 +3977,19 @@ _require_scratch_dev_pool_equal_size() > _require_fio() > { > local job=$1 > + local eng > > _require_command "$FIO_PROG" fio > if [ -z "$1" ]; then > return 1; > fi > > + # Explicitly check for every ioengine listed in the job > + for eng in `sed -n "s/ioengine=\(.*\)/\1/p" $job | sort | uniq`; do > + grep -wq $eng <($FIO_PROG --enghelp 2>/dev/null) || \ > + _notrun "fio engine $eng not available" > + done > + > $FIO_PROG --warnings-fatal --showcmd $job >> $seqres.full 2>&1 > [ $? -eq 0 ] || _notrun "$FIO_PROG too old, see $seqres.full" > } > diff --git a/tests/ext4/301 b/tests/ext4/301 > index abf47d4b..a05b8e8a 100755 > --- a/tests/ext4/301 > +++ b/tests/ext4/301 > @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > cat >$fio_config <<EOF > # Common e4defrag regression tests > [global] > -ioengine=ioe_e4defrag > iodepth=1 > directory=${SCRATCH_MNT} > filesize=${FILE_SIZE} > diff --git a/tests/ext4/302 b/tests/ext4/302 > index 87820184..e0f5f2f9 100755 > --- a/tests/ext4/302 > +++ b/tests/ext4/302 > @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > cat >$fio_config <<EOF > # Common e4defrag regression tests > [global] > -ioengine=ioe_e4defrag > iodepth=1 > directory=${SCRATCH_MNT} > filesize=${FILE_SIZE} > diff --git a/tests/ext4/303 b/tests/ext4/303 > index 2381f047..0a83e86c 100755 > --- a/tests/ext4/303 > +++ b/tests/ext4/303 > @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (3+1)))) > cat >$fio_config <<EOF > # Common e4defrag regression tests > [global] > -ioengine=ioe_e4defrag > iodepth=1 > directory=${SCRATCH_MNT} > filesize=${FILE_SIZE} > diff --git a/tests/ext4/304 b/tests/ext4/304 > index 53b522ee..5f2ae4bd 100755 > --- a/tests/ext4/304 > +++ b/tests/ext4/304 > @@ -32,7 +32,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > cat >$fio_config <<EOF > # Common e4defrag regression tests > [global] > -ioengine=ioe_e4defrag > iodepth=1 > directory=${SCRATCH_MNT} > filesize=${FILE_SIZE} > >
On 3/14/25 11:48 AM, Darrick J. Wong wrote: > On Fri, Mar 14, 2025 at 09:56:20AM -0500, Eric Sandeen wrote: >> The current test in _require_fio (--warnings-fatal --showcmd) does not >> fail if an invalid/unavailable io engine is specified. >> >> Add an explicit test that every requested io engine in the job file >> is actually available. >> >> Remove the "ioe_e4defrag" entries in ext4 tests - an engine with this >> name has seemingly never existed, but in each case later stanzas >> overrode the io engine, so it did not cause problems without this >> explicit parsing and checking. > > Well, they override it with the correct name "e4defrag": and/or with libaio - it doesn't matter what it's overridden with, just that the incorrect name in the global stanza is never used due to re-specification in each subsequent stanza. I guess I don't really understand how it helps to explicitly describe the contents of each ioengine= line that clearly still exist in each stanza, or why that's critical to the commit log... Not saying your suggested text is bad. Just saying I don't think it warrants a V3, unless that's a showstopper to get your RVB, in which case I will go back again and take the time to do so. Thanks, -Eric > $ fio --enghelp > Available IO engines: > cpuio > mmap > sync > <snip> > falloc > e4defrag > > I think the commit message should call that out specifically "...but in > each case later stanzas provide the correct ioengine name 'e4defrag', > so..."). > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > > With that amended, this all looks good to me so > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> > > --D > >> --- >> >> V2: Be Better at Bash, thanks Zorro. Also add | sort | uniq to ensure >> we only test each requested engine once. >> >> diff --git a/common/rc b/common/rc >> index ca755055..ac443ec2 100644 >> --- a/common/rc >> +++ b/common/rc >> @@ -3977,12 +3977,19 @@ _require_scratch_dev_pool_equal_size() >> _require_fio() >> { >> local job=$1 >> + local eng >> >> _require_command "$FIO_PROG" fio >> if [ -z "$1" ]; then >> return 1; >> fi >> >> + # Explicitly check for every ioengine listed in the job >> + for eng in `sed -n "s/ioengine=\(.*\)/\1/p" $job | sort | uniq`; do >> + grep -wq $eng <($FIO_PROG --enghelp 2>/dev/null) || \ >> + _notrun "fio engine $eng not available" >> + done >> + >> $FIO_PROG --warnings-fatal --showcmd $job >> $seqres.full 2>&1 >> [ $? -eq 0 ] || _notrun "$FIO_PROG too old, see $seqres.full" >> } >> diff --git a/tests/ext4/301 b/tests/ext4/301 >> index abf47d4b..a05b8e8a 100755 >> --- a/tests/ext4/301 >> +++ b/tests/ext4/301 >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) >> cat >$fio_config <<EOF >> # Common e4defrag regression tests >> [global] >> -ioengine=ioe_e4defrag >> iodepth=1 >> directory=${SCRATCH_MNT} >> filesize=${FILE_SIZE} >> diff --git a/tests/ext4/302 b/tests/ext4/302 >> index 87820184..e0f5f2f9 100755 >> --- a/tests/ext4/302 >> +++ b/tests/ext4/302 >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) >> cat >$fio_config <<EOF >> # Common e4defrag regression tests >> [global] >> -ioengine=ioe_e4defrag >> iodepth=1 >> directory=${SCRATCH_MNT} >> filesize=${FILE_SIZE} >> diff --git a/tests/ext4/303 b/tests/ext4/303 >> index 2381f047..0a83e86c 100755 >> --- a/tests/ext4/303 >> +++ b/tests/ext4/303 >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (3+1)))) >> cat >$fio_config <<EOF >> # Common e4defrag regression tests >> [global] >> -ioengine=ioe_e4defrag >> iodepth=1 >> directory=${SCRATCH_MNT} >> filesize=${FILE_SIZE} >> diff --git a/tests/ext4/304 b/tests/ext4/304 >> index 53b522ee..5f2ae4bd 100755 >> --- a/tests/ext4/304 >> +++ b/tests/ext4/304 >> @@ -32,7 +32,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) >> cat >$fio_config <<EOF >> # Common e4defrag regression tests >> [global] >> -ioengine=ioe_e4defrag >> iodepth=1 >> directory=${SCRATCH_MNT} >> filesize=${FILE_SIZE} >> >> >
On Fri, Mar 14, 2025 at 12:16:18PM -0500, Eric Sandeen wrote: > On 3/14/25 11:48 AM, Darrick J. Wong wrote: > > On Fri, Mar 14, 2025 at 09:56:20AM -0500, Eric Sandeen wrote: > >> The current test in _require_fio (--warnings-fatal --showcmd) does not > >> fail if an invalid/unavailable io engine is specified. > >> > >> Add an explicit test that every requested io engine in the job file > >> is actually available. > >> > >> Remove the "ioe_e4defrag" entries in ext4 tests - an engine with this > >> name has seemingly never existed, but in each case later stanzas > >> overrode the io engine, so it did not cause problems without this > >> explicit parsing and checking. > > > > Well, they override it with the correct name "e4defrag": > > and/or with libaio - it doesn't matter what it's overridden with, just > that the incorrect name in the global stanza is never used due to > re-specification in each subsequent stanza. > > I guess I don't really understand how it helps to explicitly describe > the contents of each ioengine= line that clearly still exist in each stanza, > or why that's critical to the commit log... > > Not saying your suggested text is bad. Just saying I don't think it warrants > a V3, unless that's a showstopper to get your RVB, in which case I will go > back again and take the time to do so. Oh, the reason I wanted the commit message to mention that the fio config files in ext4/30[1-4] eventually picks the correct engine name is that I looked at the patch, saw the removal of "ioengine=ioe_e4defrag" and wondered "Doesn't that break the test?" The patch doesn't break anything, but that's not immediately obvious from the diff context. At least it wasn't to me :P --D > Thanks, > -Eric > > > $ fio --enghelp > > Available IO engines: > > cpuio > > mmap > > sync > > <snip> > > falloc > > e4defrag > > > > I think the commit message should call that out specifically "...but in > > each case later stanzas provide the correct ioengine name 'e4defrag', > > so..."). > > > >> Signed-off-by: Eric Sandeen <sandeen@redhat.com> > > > > With that amended, this all looks good to me so > > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org> > > > > --D > > > >> --- > >> > >> V2: Be Better at Bash, thanks Zorro. Also add | sort | uniq to ensure > >> we only test each requested engine once. > >> > >> diff --git a/common/rc b/common/rc > >> index ca755055..ac443ec2 100644 > >> --- a/common/rc > >> +++ b/common/rc > >> @@ -3977,12 +3977,19 @@ _require_scratch_dev_pool_equal_size() > >> _require_fio() > >> { > >> local job=$1 > >> + local eng > >> > >> _require_command "$FIO_PROG" fio > >> if [ -z "$1" ]; then > >> return 1; > >> fi > >> > >> + # Explicitly check for every ioengine listed in the job > >> + for eng in `sed -n "s/ioengine=\(.*\)/\1/p" $job | sort | uniq`; do > >> + grep -wq $eng <($FIO_PROG --enghelp 2>/dev/null) || \ > >> + _notrun "fio engine $eng not available" > >> + done > >> + > >> $FIO_PROG --warnings-fatal --showcmd $job >> $seqres.full 2>&1 > >> [ $? -eq 0 ] || _notrun "$FIO_PROG too old, see $seqres.full" > >> } > >> diff --git a/tests/ext4/301 b/tests/ext4/301 > >> index abf47d4b..a05b8e8a 100755 > >> --- a/tests/ext4/301 > >> +++ b/tests/ext4/301 > >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > >> cat >$fio_config <<EOF > >> # Common e4defrag regression tests > >> [global] > >> -ioengine=ioe_e4defrag > >> iodepth=1 > >> directory=${SCRATCH_MNT} > >> filesize=${FILE_SIZE} > >> diff --git a/tests/ext4/302 b/tests/ext4/302 > >> index 87820184..e0f5f2f9 100755 > >> --- a/tests/ext4/302 > >> +++ b/tests/ext4/302 > >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > >> cat >$fio_config <<EOF > >> # Common e4defrag regression tests > >> [global] > >> -ioengine=ioe_e4defrag > >> iodepth=1 > >> directory=${SCRATCH_MNT} > >> filesize=${FILE_SIZE} > >> diff --git a/tests/ext4/303 b/tests/ext4/303 > >> index 2381f047..0a83e86c 100755 > >> --- a/tests/ext4/303 > >> +++ b/tests/ext4/303 > >> @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (3+1)))) > >> cat >$fio_config <<EOF > >> # Common e4defrag regression tests > >> [global] > >> -ioengine=ioe_e4defrag > >> iodepth=1 > >> directory=${SCRATCH_MNT} > >> filesize=${FILE_SIZE} > >> diff --git a/tests/ext4/304 b/tests/ext4/304 > >> index 53b522ee..5f2ae4bd 100755 > >> --- a/tests/ext4/304 > >> +++ b/tests/ext4/304 > >> @@ -32,7 +32,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) > >> cat >$fio_config <<EOF > >> # Common e4defrag regression tests > >> [global] > >> -ioengine=ioe_e4defrag > >> iodepth=1 > >> directory=${SCRATCH_MNT} > >> filesize=${FILE_SIZE} > >> > >> > > >
diff --git a/common/rc b/common/rc index ca755055..ac443ec2 100644 --- a/common/rc +++ b/common/rc @@ -3977,12 +3977,19 @@ _require_scratch_dev_pool_equal_size() _require_fio() { local job=$1 + local eng _require_command "$FIO_PROG" fio if [ -z "$1" ]; then return 1; fi + # Explicitly check for every ioengine listed in the job + for eng in `sed -n "s/ioengine=\(.*\)/\1/p" $job | sort | uniq`; do + grep -wq $eng <($FIO_PROG --enghelp 2>/dev/null) || \ + _notrun "fio engine $eng not available" + done + $FIO_PROG --warnings-fatal --showcmd $job >> $seqres.full 2>&1 [ $? -eq 0 ] || _notrun "$FIO_PROG too old, see $seqres.full" } diff --git a/tests/ext4/301 b/tests/ext4/301 index abf47d4b..a05b8e8a 100755 --- a/tests/ext4/301 +++ b/tests/ext4/301 @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) cat >$fio_config <<EOF # Common e4defrag regression tests [global] -ioengine=ioe_e4defrag iodepth=1 directory=${SCRATCH_MNT} filesize=${FILE_SIZE} diff --git a/tests/ext4/302 b/tests/ext4/302 index 87820184..e0f5f2f9 100755 --- a/tests/ext4/302 +++ b/tests/ext4/302 @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) cat >$fio_config <<EOF # Common e4defrag regression tests [global] -ioengine=ioe_e4defrag iodepth=1 directory=${SCRATCH_MNT} filesize=${FILE_SIZE} diff --git a/tests/ext4/303 b/tests/ext4/303 index 2381f047..0a83e86c 100755 --- a/tests/ext4/303 +++ b/tests/ext4/303 @@ -31,7 +31,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (3+1)))) cat >$fio_config <<EOF # Common e4defrag regression tests [global] -ioengine=ioe_e4defrag iodepth=1 directory=${SCRATCH_MNT} filesize=${FILE_SIZE} diff --git a/tests/ext4/304 b/tests/ext4/304 index 53b522ee..5f2ae4bd 100755 --- a/tests/ext4/304 +++ b/tests/ext4/304 @@ -32,7 +32,6 @@ FILE_SIZE=$((BLK_DEV_SIZE * (512 / (2 + 1)))) cat >$fio_config <<EOF # Common e4defrag regression tests [global] -ioengine=ioe_e4defrag iodepth=1 directory=${SCRATCH_MNT} filesize=${FILE_SIZE}
The current test in _require_fio (--warnings-fatal --showcmd) does not fail if an invalid/unavailable io engine is specified. Add an explicit test that every requested io engine in the job file is actually available. Remove the "ioe_e4defrag" entries in ext4 tests - an engine with this name has seemingly never existed, but in each case later stanzas overrode the io engine, so it did not cause problems without this explicit parsing and checking. Signed-off-by: Eric Sandeen <sandeen@redhat.com> --- V2: Be Better at Bash, thanks Zorro. Also add | sort | uniq to ensure we only test each requested engine once.