On 7/21/23 04:36, Jens Axboe wrote:
> On 7/19/23 7:31?PM, Damien Le Moal wrote:
>> diff --git a/engines/io_uring.c b/engines/io_uring.c
>> index f30a3c00..d55af6bc 100644
>> --- a/engines/io_uring.c
>> +++ b/engines/io_uring.c
>> @@ -157,6 +157,21 @@ static struct fio_option options[] = {
>> .category = FIO_OPT_C_ENGINE,
>> .group = FIO_OPT_G_IOURING,
>> },
>> + {
>> + .name = "cmdprio_hint",
>> + .lname = "Asynchronous I/O priority hint",
>> + .type = FIO_OPT_INT,
>> + .off1 = offsetof(struct ioring_options,
>> + cmdprio_options.hint[DDIR_READ]),
>> + .off2 = offsetof(struct ioring_options,
>> + cmdprio_options.hint[DDIR_WRITE]),
>> + .help = "Set asynchronous IO priority hint",
>> + .minval = IOPRIO_MIN_PRIO_HINT,
>> + .maxval = IOPRIO_MAX_PRIO_HINT,
>> + .interval = 1,
>> + .category = FIO_OPT_C_ENGINE,
>> + .group = FIO_OPT_G_IOURING,
>> + },
>> {
>> .name = "cmdprio",
>> .lname = "Asynchronous I/O priority level",
>> @@ -195,6 +210,12 @@ static struct fio_option options[] = {
>> .type = FIO_OPT_UNSUPPORTED,
>> .help = "Your platform does not support I/O priority classes",
>> },
>> + {
>> + .name = "cmdprio_hint",
>> + .lname = "Asynchronous I/O priority hint",
>> + .type = FIO_OPT_UNSUPPORTED,
>> + .help = "Your platform does not support I/O priority classes",
>> + },
>> {
>> .name = "cmdprio",
>> .lname = "Asynchronous I/O priority level",
>
> Since neither this nor libaio actually do anything with these values,
> why isn't this just a generic option? You could flag the two engines
> where it actually works with an engine flag, and check if it's set and
> we don't have that flag and error out in the generic code.
Hmmm. I would like to keep the async IO prio option definitions and storage
together with the cmdprio_options struct, which is common to both libaio and
iouring engines. If we move the hint option definition to be a generic one, we
cannot use that struct and would need a job field. I can make that work, but
that makes the code a little messier by spreading out the controls for async IO
prio.
I agree that we can drop the error output though as indeed the hint value is
unused on systems without hint support. But warning the user about the lack of
support is nice I think.
Checking the code again though, one thing I noticed is that iouring and libaio
cmdprio_xxx options definition is identical, modulo the group. So all these
definitions could be moved to engines/cmdprio.h as macros and IO engines
supporting these options can use these to add the options. The other option is
to move the definition of all these options to be generic ones and use the
engine flag trick you mention to indicate support.
--
Damien Le Moal
Western Digital Research
