On Thu, Feb 03, 2022 at 10:04:38AM +0900, Damien Le Moal wrote:
> On 2/1/22 06:13, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@xxxxxxx>
> >
> > The cmdprio_bssplit ioengine option for io_uring/libaio is currently parsed
> > using split_parse_ddir(). While this function works fine for parsing the
> > existing cmdprio_bssplit entry format, it forces every cmdprio_bssplit
> > entry to use the priority defined by cmdprio and cmdprio_class. This means
> > that there will only ever be at most two different priority values used in
> > the job.
> >
> > To enable us to use more than two different priority values, add a new
> > parsing function, split_parse_prio_ddir(), that will support parsing the
> > existing cmdprio_bssplit entry format (blocksize/percentage), and a new
> > cmdprio_bssplit entry format (blocksize/percentage/prioclass/priolevel).
> >
> > Since IO engines can be compiled as plugins, having the parse function in
> > options.c avoids potential problems with ioengines having different
> > versions of the same parsing function.
> >
> > A follow up patch will change to the new parsing function.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > ---
> > options.c | 126 +++++++++++++++++++++++++++++++++++++++++++++++
> > thread_options.h | 10 ++++
> > 2 files changed, 136 insertions(+)
> >
> > diff --git a/options.c b/options.c
> > index 102bcf56..4f2ce0d0 100644
> > --- a/options.c
> > +++ b/options.c
> > @@ -278,6 +278,132 @@ static int str_bssplit_cb(void *data, const char *input)
> > return ret;
> > }
> >
> > +static int parse_cmdprio_bssplit_entry(struct thread_options *o,
> > + struct split_prio *entry, char *str)
> > +{
> > + int matches = 0;
> > + char *bs_str = NULL;
> > + long long bs_val;
> > + unsigned int perc = 0, class, level;
> > +
> > + /*
> > + * valid entry formats:
> > + * bs/ - %s/ - set perc to 0, prio to -1.
> > + * bs/perc - %s/%u - set prio to -1.
> > + * bs/perc/class/level - %s/%u/%u/%u
> > + */
> > + matches = sscanf(str, "%m[^/]/%u/%u/%u", &bs_str, &perc, &class, &level);
> > + if (matches < 1) {
> > + log_err("fio: invalid cmdprio_bssplit format\n");
> > + return 1;
> > + }
> > +
> > + if (str_to_decimal(bs_str, &bs_val, 1, o, 0, 0)) {
> > + log_err("fio: split conversion failed\n");
> > + free(bs_str);
> > + return 1;
> > + }
> > + free(bs_str);
> > +
> > + entry->bs = bs_val;
> > + entry->perc = min(perc, 100u);
> > + entry->prio = -1;
> > + switch (matches) {
> > + case 1: /* bs/ case */
> > + case 2: /* bs/perc case */
> > + break;
> > + case 4: /* bs/perc/class/level case */
> > +#ifdef FIO_HAVE_IOPRIO_CLASS
> > + class = min(class, (unsigned int) IOPRIO_MAX_PRIO_CLASS);
> > +#endif
> > +#ifdef FIO_HAVE_IOPRIO
> > + level = min(level, (unsigned int) IOPRIO_MAX_PRIO);
> > +#endif
> > + entry->prio = ioprio_value(class, level);
>
Hello Damien,
First off, thank you for the review.
> For the !FIO_HAVE_IOPRIO_CLASS and/or !FIO_HAVE_IOPRIO cases, class and
> level will be used as-is... I wonder if they should be set to 0 ?
> Or may be more simply, in os/os.h, conditionally define
> IOPRIO_MAX_PRIO_CLASS to 0 if FIO_HAVE_IOPRIO_CLASS is false. Same for
> IOPRIO_MAX_PRIO. That will avoid the #ifdef here.
Sure, that sounds like a nice cleanup.
I will add it as a new patch in the series.
Kind regards,
Niklas
