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@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}
>>
>>
>
