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);
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.
> + break;
> + default:
> + log_err("fio: invalid cmdprio_bssplit format\n");
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Returns a negative integer if the first argument should be before the second
> + * argument in the sorted list. A positive integer if the first argument should
> + * be after the second argument in the sorted list. A zero if they are equal.
> + */
> +static int fio_split_prio_cmp(const void *p1, const void *p2)
> +{
> + const struct split_prio *tmp1 = p1;
> + const struct split_prio *tmp2 = p2;
> +
> + if (tmp1->bs > tmp2->bs)
> + return 1;
> + if (tmp1->bs < tmp2->bs)
> + return -1;
> + return 0;
> +}
> +
> +int split_parse_prio_ddir(struct thread_options *o, struct split_prio **entries,
> + int *nr_entries, char *str)
> +{
> + struct split_prio *tmp_entries;
> + unsigned int nr_bssplits;
> + char *str_cpy, *p, *fname;
> +
> + /* strsep modifies the string, dup it so that we can use strsep twice */
> + p = str_cpy = strdup(str);
> + if (!p)
> + return 1;
> +
> + nr_bssplits = 0;
> + while ((fname = strsep(&str_cpy, ":")) != NULL) {
> + if (!strlen(fname))
> + break;
> + nr_bssplits++;
> + }
> + free(p);
> +
> + if (nr_bssplits > BSSPLIT_MAX) {
> + log_err("fio: too many cmdprio_bssplit entries\n");
> + return 1;
> + }
> +
> + tmp_entries = calloc(nr_bssplits, sizeof(*tmp_entries));
> + if (!tmp_entries)
> + return 1;
> +
> + nr_bssplits = 0;
> + while ((fname = strsep(&str, ":")) != NULL) {
> + struct split_prio *entry;
> +
> + if (!strlen(fname))
> + break;
> +
> + entry = &tmp_entries[nr_bssplits];
> +
> + if (parse_cmdprio_bssplit_entry(o, entry, fname)) {
> + log_err("fio: failed to parse cmdprio_bssplit entry\n");
> + free(tmp_entries);
> + return 1;
> + }
> +
> + /* skip zero perc entries, they provide no useful information */
> + if (entry->perc)
> + nr_bssplits++;
> + }
> +
> + qsort(tmp_entries, nr_bssplits, sizeof(*tmp_entries),
> + fio_split_prio_cmp);
> +
> + *entries = tmp_entries;
> + *nr_entries = nr_bssplits;
> +
> + return 0;
> +}
> +
> static int str2error(char *str)
> {
> const char *err[] = { "EPERM", "ENOENT", "ESRCH", "EINTR", "EIO",
> diff --git a/thread_options.h b/thread_options.h
> index 8f4c8a59..ace658b7 100644
> --- a/thread_options.h
> +++ b/thread_options.h
> @@ -50,6 +50,12 @@ struct split {
> unsigned long long val2[ZONESPLIT_MAX];
> };
>
> +struct split_prio {
> + uint64_t bs;
> + int32_t prio;
> + uint32_t perc;
> +};
> +
> struct bssplit {
> uint64_t bs;
> uint32_t perc;
> @@ -702,4 +708,8 @@ extern int str_split_parse(struct thread_data *td, char *str,
> extern int split_parse_ddir(struct thread_options *o, struct split *split,
> char *str, bool absolute, unsigned int max_splits);
>
> +extern int split_parse_prio_ddir(struct thread_options *o,
> + struct split_prio **entries, int *nr_entries,
> + char *str);
> +
> #endif
--
Damien Le Moal
Western Digital Research
