libaio,io_uring: make it possible to cleanup cmdprio malloced data

The way that fio currently handles engine options:
options_free() will call free() only for options that have the type
FIO_OPT_STR_STORE. This means that any option that has a pointer in
either td->o or td->eo, which is not of type FIO_OPT_STR_STORE will
leak memory. This is true even for numjobs == 1.

When running with numjobs > 1, fio_options_mem_dupe() will memcpy
td->eo into the new td. Since off1 of the pointers in the first td
has already been set, the pointers in the new td will point to the
same data. (Regardless, options_free() will never try to free the
memory, for neither td.) Neither can we manually free the memory in
cleanup(), since the other td will still point to the same memory,
so this would lead to a double free.

These memory leaks are reported by e.g. valgrind.

The most obvious way to solve this is to put dynamically allocated
memory in {ioring,libaio}_data instead of {ioring,libaio}_options.

This solves the problem since {ioring,libaio}_data is dynamically
allocated by each td during the ioengine init callback, and is freed
when the ioengine cleanup callback for that td is called.

The downside of this is that the parsing has to be done in
fio_cmdprio_init() instead of in the option .cb callback, since the
.cb callback is called before {ioring,libaio}_data is available.

This patch keeps the static cmdprio options in
{ioring,libaio}_options, but moves the dynamically allocated memory
needed by cmdprio to {ioring,libaio}_data.

No cmdprio related memory leaks are reported after this patch.

Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Link: https://lore.kernel.org/r/20211112095428.158300-9-Niklas.Cassel@wdc.com
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/engines/cmdprio.c b/engines/cmdprio.c
index 8c52dabdf3e86d671f35b825ea9a0807f36cb413..92b752aecd413d5085249bd880502ab49e6609f6 100644 (file)
--- a/engines/cmdprio.c
+++ b/engines/cmdprio.c
@@ -46,22 +46,15 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
                              struct cmdprio *cmdprio)
 {
        char *str, *p;
-       int i, ret = 0;
+       int ret = 0;
 
        p = str = strdup(input);
 
        strip_blank_front(&str);
        strip_blank_end(str);
 
-       ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio, false);
-
-       if (parse_dryrun()) {
-               for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
-                       free(cmdprio->bssplit[i]);
-                       cmdprio->bssplit[i] = NULL;
-                       cmdprio->bssplit_nr[i] = 0;
-               }
-       }
+       ret = str_split_parse(td, str, fio_cmdprio_bssplit_ddir, cmdprio,
+                             false);
 
        free(p);
        return ret;
@@ -70,11 +63,12 @@ int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
 static int fio_cmdprio_percentage(struct cmdprio *cmdprio, struct io_u *io_u)
 {
        enum fio_ddir ddir = io_u->ddir;
+       struct cmdprio_options *options = cmdprio->options;
        int i;
 
        switch (cmdprio->mode) {
        case CMDPRIO_MODE_PERC:
-               return cmdprio->percentage[ddir];
+               return options->percentage[ddir];
        case CMDPRIO_MODE_BSSPLIT:
                for (i = 0; i < cmdprio->bssplit_nr[ddir]; i++) {
                        if (cmdprio->bssplit[ddir][i].bs == io_u->buflen)
@@ -107,9 +101,10 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
                            struct io_u *io_u)
 {
        enum fio_ddir ddir = io_u->ddir;
+       struct cmdprio_options *options = cmdprio->options;
        unsigned int p;
        unsigned int cmdprio_value =
-               ioprio_value(cmdprio->class[ddir], cmdprio->level[ddir]);
+               ioprio_value(options->class[ddir], options->level[ddir]);
 
        p = fio_cmdprio_percentage(cmdprio, io_u);
        if (p && rand_between(&td->prio_state, 0, 99) < p) {
@@ -138,30 +133,91 @@ bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
        return false;
 }
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
+static int fio_cmdprio_parse_and_gen_bssplit(struct thread_data *td,
+                                            struct cmdprio *cmdprio)
 {
-       struct thread_options *to = &td->o;
-       bool has_cmdprio_percentage = false;
-       bool has_cmdprio_bssplit = false;
-       int i;
+       struct cmdprio_options *options = cmdprio->options;
+       int ret;
+
+       ret = fio_cmdprio_bssplit_parse(td, options->bssplit_str, cmdprio);
+       if (ret)
+               goto err;
+
+       return 0;
+
+err:
+       fio_cmdprio_cleanup(cmdprio);
+
+       return ret;
+}
+
+static int fio_cmdprio_parse_and_gen(struct thread_data *td,
+                                    struct cmdprio *cmdprio)
+{
+       struct cmdprio_options *options = cmdprio->options;
+       int i, ret;
+
+       switch (cmdprio->mode) {
+       case CMDPRIO_MODE_BSSPLIT:
+               ret = fio_cmdprio_parse_and_gen_bssplit(td, cmdprio);
+               break;
+       case CMDPRIO_MODE_PERC:
+               ret = 0;
+               break;
+       default:
+               assert(0);
+               return 1;
+       }
 
        /*
         * If cmdprio_percentage/cmdprio_bssplit is set and cmdprio_class
         * is not set, default to RT priority class.
         */
        for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
-               if (cmdprio->percentage[i]) {
-                       if (!cmdprio->class[i])
-                               cmdprio->class[i] = IOPRIO_CLASS_RT;
-                       has_cmdprio_percentage = true;
-               }
-               if (cmdprio->bssplit_nr[i]) {
-                       if (!cmdprio->class[i])
-                               cmdprio->class[i] = IOPRIO_CLASS_RT;
-                       has_cmdprio_bssplit = true;
+               if (options->percentage[i] || cmdprio->bssplit_nr[i]) {
+                       if (!options->class[i])
+                               options->class[i] = IOPRIO_CLASS_RT;
                }
        }
 
+       return ret;
+}
+
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio)
+{
+       int ddir;
+
+       for (ddir = 0; ddir < CMDPRIO_RWDIR_CNT; ddir++) {
+               free(cmdprio->bssplit[ddir]);
+               cmdprio->bssplit[ddir] = NULL;
+               cmdprio->bssplit_nr[ddir] = 0;
+       }
+
+       /*
+        * options points to a cmdprio_options struct that is part of td->eo.
+        * td->eo itself will be freed by free_ioengine().
+        */
+       cmdprio->options = NULL;
+}
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+                    struct cmdprio_options *options)
+{
+       struct thread_options *to = &td->o;
+       bool has_cmdprio_percentage = false;
+       bool has_cmdprio_bssplit = false;
+       int i;
+
+       cmdprio->options = options;
+
+       if (options->bssplit_str && strlen(options->bssplit_str))
+               has_cmdprio_bssplit = true;
+
+       for (i = 0; i < CMDPRIO_RWDIR_CNT; i++) {
+               if (options->percentage[i])
+                       has_cmdprio_percentage = true;
+       }
+
        /*
         * Check for option conflicts
         */
@@ -179,5 +235,9 @@ int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio)
        else
                cmdprio->mode = CMDPRIO_MODE_NONE;
 
-       return 0;
+       /* Nothing left to do if cmdprio is not used */
+       if (cmdprio->mode == CMDPRIO_MODE_NONE)
+               return 0;
+
+       return fio_cmdprio_parse_and_gen(td, cmdprio);
 }

diff --git a/engines/cmdprio.h b/engines/cmdprio.h
index 7e4fcf6c58e4113436b19cc8cf3b1965889c6b0e..0c7bd6cf4b92222189ff3bc897c4446f895f9847 100644 (file)
--- a/engines/cmdprio.h
+++ b/engines/cmdprio.h
@@ -17,21 +17,26 @@ enum {
        CMDPRIO_MODE_BSSPLIT,
 };
 
-struct cmdprio {
+struct cmdprio_options {
        unsigned int percentage[CMDPRIO_RWDIR_CNT];
        unsigned int class[CMDPRIO_RWDIR_CNT];
        unsigned int level[CMDPRIO_RWDIR_CNT];
+       char *bssplit_str;
+};
+
+struct cmdprio {
+       struct cmdprio_options *options;
        unsigned int bssplit_nr[CMDPRIO_RWDIR_CNT];
        struct bssplit *bssplit[CMDPRIO_RWDIR_CNT];
        unsigned int mode;
 };
 
-int fio_cmdprio_bssplit_parse(struct thread_data *td, const char *input,
-                             struct cmdprio *cmdprio);
-
 bool fio_cmdprio_set_ioprio(struct thread_data *td, struct cmdprio *cmdprio,
                            struct io_u *io_u);
 
-int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio);
+void fio_cmdprio_cleanup(struct cmdprio *cmdprio);
+
+int fio_cmdprio_init(struct thread_data *td, struct cmdprio *cmdprio,
+                    struct cmdprio_options *options);
 
 #endif

diff --git a/engines/io_uring.c b/engines/io_uring.c
index 1f55ad1756af2e2bf43d6297fe67790c3a9bb73b..8b8f35f1e6644b4cf4481956684a46b32de2a8f0 100644 (file)
--- a/engines/io_uring.c
+++ b/engines/io_uring.c
@@ -68,12 +68,14 @@ struct ioring_data {
        int prepped;
 
        struct ioring_mmap mmap[3];
+
+       struct cmdprio cmdprio;
 };
 
 struct ioring_options {
        struct thread_data *td;
        unsigned int hipri;
-       struct cmdprio cmdprio;
+       struct cmdprio_options cmdprio_options;
        unsigned int fixedbufs;
        unsigned int registerfiles;
        unsigned int sqpoll_thread;
@@ -104,15 +106,6 @@ static int fio_ioring_sqpoll_cb(void *data, unsigned long long *val)
        return 0;
 }
 
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
-       struct ioring_options *o = data;
-       struct thread_data *td = o->td;
-       struct cmdprio *cmdprio = &o->cmdprio;
-
-       return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
 static struct fio_option options[] = {
        {
                .name   = "hipri",
@@ -129,9 +122,9 @@ static struct fio_option options[] = {
                .lname  = "high priority percentage",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct ioring_options,
-                                  cmdprio.percentage[DDIR_READ]),
+                                  cmdprio_options.percentage[DDIR_READ]),
                .off2   = offsetof(struct ioring_options,
-                                  cmdprio.percentage[DDIR_WRITE]),
+                                  cmdprio_options.percentage[DDIR_WRITE]),
                .minval = 0,
                .maxval = 100,
                .help   = "Send high priority I/O this percentage of the time",
@@ -143,9 +136,9 @@ static struct fio_option options[] = {
                .lname  = "Asynchronous I/O priority class",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct ioring_options,
-                                  cmdprio.class[DDIR_READ]),
+                                  cmdprio_options.class[DDIR_READ]),
                .off2   = offsetof(struct ioring_options,
-                                  cmdprio.class[DDIR_WRITE]),
+                                  cmdprio_options.class[DDIR_WRITE]),
                .help   = "Set asynchronous IO priority class",
                .minval = IOPRIO_MIN_PRIO_CLASS + 1,
                .maxval = IOPRIO_MAX_PRIO_CLASS,
@@ -158,9 +151,9 @@ static struct fio_option options[] = {
                .lname  = "Asynchronous I/O priority level",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct ioring_options,
-                                  cmdprio.level[DDIR_READ]),
+                                  cmdprio_options.level[DDIR_READ]),
                .off2   = offsetof(struct ioring_options,
-                                  cmdprio.level[DDIR_WRITE]),
+                                  cmdprio_options.level[DDIR_WRITE]),
                .help   = "Set asynchronous IO priority level",
                .minval = IOPRIO_MIN_PRIO,
                .maxval = IOPRIO_MAX_PRIO,
@@ -171,9 +164,9 @@ static struct fio_option options[] = {
        {
                .name   = "cmdprio_bssplit",
                .lname  = "Priority percentage block size split",
-               .type   = FIO_OPT_STR_ULL,
-               .cb     = str_cmdprio_bssplit_cb,
-               .off1   = offsetof(struct ioring_options, cmdprio.bssplit),
+               .type   = FIO_OPT_STR_STORE,
+               .off1   = offsetof(struct ioring_options,
+                                  cmdprio_options.bssplit_str),
                .help   = "Set priority percentages for different block sizes",
                .category = FIO_OPT_C_ENGINE,
                .group  = FIO_OPT_G_IOURING,
@@ -458,8 +451,7 @@ static inline void fio_ioring_cmdprio_prep(struct thread_data *td,
                                           struct io_u *io_u)
 {
        struct ioring_data *ld = td->io_ops_data;
-       struct ioring_options *o = td->eo;
-       struct cmdprio *cmdprio = &o->cmdprio;
+       struct cmdprio *cmdprio = &ld->cmdprio;
 
        if (fio_cmdprio_set_ioprio(td, cmdprio, io_u))
                ld->sqes[io_u->index].ioprio = io_u->ioprio;
@@ -470,7 +462,6 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
 {
        struct ioring_data *ld = td->io_ops_data;
        struct io_sq_ring *ring = &ld->sq_ring;
-       struct ioring_options *o = td->eo;
        unsigned tail, next_tail;
 
        fio_ro_check(td, io_u);
@@ -493,7 +484,7 @@ static enum fio_q_status fio_ioring_queue(struct thread_data *td,
        if (next_tail == atomic_load_acquire(ring->head))
                return FIO_Q_BUSY;
 
-       if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+       if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
                fio_ioring_cmdprio_prep(td, io_u);
 
        ring->array[tail & ld->sq_ring_mask] = io_u->index;
@@ -599,6 +590,7 @@ static void fio_ioring_cleanup(struct thread_data *td)
                if (!(td->flags & TD_F_CHILD))
                        fio_ioring_unmap(ld);
 
+               fio_cmdprio_cleanup(&ld->cmdprio);
                free(ld->io_u_index);
                free(ld->iovecs);
                free(ld->fds);
@@ -805,7 +797,6 @@ static int fio_ioring_init(struct thread_data *td)
 {
        struct ioring_options *o = td->eo;
        struct ioring_data *ld;
-       struct cmdprio *cmdprio = &o->cmdprio;
        int ret;
 
        /* sqthread submission requires registered files */
@@ -830,7 +821,7 @@ static int fio_ioring_init(struct thread_data *td)
 
        td->io_ops_data = ld;
 
-       ret = fio_cmdprio_init(td, cmdprio);
+       ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
        if (ret) {
                td_verror(td, EINVAL, "fio_ioring_init");
                return 1;

diff --git a/engines/libaio.c b/engines/libaio.c
index 53fe7428a010cfa07140c108760a8005094a2c7e..9c278d060b218617d0ab89aa5285a1e010507661 100644 (file)
--- a/engines/libaio.c
+++ b/engines/libaio.c
@@ -51,24 +51,17 @@ struct libaio_data {
        unsigned int queued;
        unsigned int head;
        unsigned int tail;
+
+       struct cmdprio cmdprio;
 };
 
 struct libaio_options {
        struct thread_data *td;
        unsigned int userspace_reap;
-       struct cmdprio cmdprio;
+       struct cmdprio_options cmdprio_options;
        unsigned int nowait;
 };
 
-static int str_cmdprio_bssplit_cb(void *data, const char *input)
-{
-       struct libaio_options *o = data;
-       struct thread_data *td = o->td;
-       struct cmdprio *cmdprio = &o->cmdprio;
-
-       return fio_cmdprio_bssplit_parse(td, input, cmdprio);
-}
-
 static struct fio_option options[] = {
        {
                .name   = "userspace_reap",
@@ -85,9 +78,9 @@ static struct fio_option options[] = {
                .lname  = "high priority percentage",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct libaio_options,
-                                  cmdprio.percentage[DDIR_READ]),
+                                  cmdprio_options.percentage[DDIR_READ]),
                .off2   = offsetof(struct libaio_options,
-                                  cmdprio.percentage[DDIR_WRITE]),
+                                  cmdprio_options.percentage[DDIR_WRITE]),
                .minval = 0,
                .maxval = 100,
                .help   = "Send high priority I/O this percentage of the time",
@@ -99,9 +92,9 @@ static struct fio_option options[] = {
                .lname  = "Asynchronous I/O priority class",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct libaio_options,
-                                  cmdprio.class[DDIR_READ]),
+                                  cmdprio_options.class[DDIR_READ]),
                .off2   = offsetof(struct libaio_options,
-                                  cmdprio.class[DDIR_WRITE]),
+                                  cmdprio_options.class[DDIR_WRITE]),
                .help   = "Set asynchronous IO priority class",
                .minval = IOPRIO_MIN_PRIO_CLASS + 1,
                .maxval = IOPRIO_MAX_PRIO_CLASS,
@@ -114,9 +107,9 @@ static struct fio_option options[] = {
                .lname  = "Asynchronous I/O priority level",
                .type   = FIO_OPT_INT,
                .off1   = offsetof(struct libaio_options,
-                                  cmdprio.level[DDIR_READ]),
+                                  cmdprio_options.level[DDIR_READ]),
                .off2   = offsetof(struct libaio_options,
-                                  cmdprio.level[DDIR_WRITE]),
+                                  cmdprio_options.level[DDIR_WRITE]),
                .help   = "Set asynchronous IO priority level",
                .minval = IOPRIO_MIN_PRIO,
                .maxval = IOPRIO_MAX_PRIO,
@@ -127,9 +120,9 @@ static struct fio_option options[] = {
        {
                .name   = "cmdprio_bssplit",
                .lname  = "Priority percentage block size split",
-               .type   = FIO_OPT_STR_ULL,
-               .cb     = str_cmdprio_bssplit_cb,
-               .off1   = offsetof(struct libaio_options, cmdprio.bssplit),
+               .type   = FIO_OPT_STR_STORE,
+               .off1   = offsetof(struct libaio_options,
+                                  cmdprio_options.bssplit_str),
                .help   = "Set priority percentages for different block sizes",
                .category = FIO_OPT_C_ENGINE,
                .group  = FIO_OPT_G_LIBAIO,
@@ -206,8 +199,8 @@ static int fio_libaio_prep(struct thread_data *td, struct io_u *io_u)
 static inline void fio_libaio_cmdprio_prep(struct thread_data *td,
                                           struct io_u *io_u)
 {
-       struct libaio_options *o = td->eo;
-       struct cmdprio *cmdprio = &o->cmdprio;
+       struct libaio_data *ld = td->io_ops_data;
+       struct cmdprio *cmdprio = &ld->cmdprio;
 
        if (fio_cmdprio_set_ioprio(td, cmdprio, io_u)) {
                io_u->iocb.aio_reqprio = io_u->ioprio;
@@ -318,7 +311,6 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
                                          struct io_u *io_u)
 {
        struct libaio_data *ld = td->io_ops_data;
-       struct libaio_options *o = td->eo;
 
        fio_ro_check(td, io_u);
 
@@ -349,7 +341,7 @@ static enum fio_q_status fio_libaio_queue(struct thread_data *td,
                return FIO_Q_COMPLETED;
        }
 
-       if (o->cmdprio.mode != CMDPRIO_MODE_NONE)
+       if (ld->cmdprio.mode != CMDPRIO_MODE_NONE)
                fio_libaio_cmdprio_prep(td, io_u);
 
        ld->iocbs[ld->head] = &io_u->iocb;
@@ -468,6 +460,8 @@ static void fio_libaio_cleanup(struct thread_data *td)
                 */
                if (!(td->flags & TD_F_CHILD))
                        io_destroy(ld->aio_ctx);
+
+               fio_cmdprio_cleanup(&ld->cmdprio);
                free(ld->aio_events);
                free(ld->iocbs);
                free(ld->io_us);
@@ -493,7 +487,6 @@ static int fio_libaio_init(struct thread_data *td)
 {
        struct libaio_data *ld;
        struct libaio_options *o = td->eo;
-       struct cmdprio *cmdprio = &o->cmdprio;
        int ret;
 
        ld = calloc(1, sizeof(*ld));
@@ -506,7 +499,7 @@ static int fio_libaio_init(struct thread_data *td)
 
        td->io_ops_data = ld;
 
-       ret = fio_cmdprio_init(td, cmdprio);
+       ret = fio_cmdprio_init(td, &ld->cmdprio, &o->cmdprio_options);
        if (ret) {
                td_verror(td, EINVAL, "fio_libaio_init");
                return 1;