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@xxxxxxxxxx>
> >
> > With that amended, this all looks good to me so
> > Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> >
> > --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}
> >>
> >>
> >
>
