diff --git a/SOURCES/0052-libmultipath-check-for-null-wwid-before-strcmp.patch b/SOURCES/0052-libmultipath-check-for-null-wwid-before-strcmp.patch new file mode 100644 index 0000000..fe33783 --- /dev/null +++ b/SOURCES/0052-libmultipath-check-for-null-wwid-before-strcmp.patch @@ -0,0 +1,34 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 4 Jan 2021 21:59:54 -0600 +Subject: [PATCH] libmultipath: check for null wwid before strcmp + +Commit 749aabd0 (libmultipath: ignore multipaths sections without wwid +option) removed all mpentries with a NULL wwid, but didn't stop strcmp() +from being run on them in merge_mptable(). The result of strcmp() with +a NULL parameter is undefined, so fix that. + +Signed-off-by: Benjamin Marzinski + +Reviewed-by: Martin Wilck +Signed-off-by: Benjamin Marzinski +--- + libmultipath/config.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/libmultipath/config.c b/libmultipath/config.c +index dc81c994..dd645f17 100644 +--- a/libmultipath/config.c ++++ b/libmultipath/config.c +@@ -453,7 +453,7 @@ void merge_mptable(vector mptable) + } + j = i + 1; + vector_foreach_slot_after(mptable, mp2, j) { +- if (strcmp(mp1->wwid, mp2->wwid)) ++ if (!mp2->wwid || strcmp(mp1->wwid, mp2->wwid)) + continue; + condlog(1, "%s: duplicate multipath config section for %s", + __func__, mp1->wwid); +-- +2.17.2 + diff --git a/SOURCES/0053-libmultipath-make-find_err_path_by_dev-static.patch b/SOURCES/0053-libmultipath-make-find_err_path_by_dev-static.patch new file mode 100644 index 0000000..8375b10 --- /dev/null +++ b/SOURCES/0053-libmultipath-make-find_err_path_by_dev-static.patch @@ -0,0 +1,27 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 14 Jan 2021 20:20:22 -0600 +Subject: [PATCH] libmultipath: make find_err_path_by_dev() static + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index 1b9cd6c0..449760a0 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -89,7 +89,7 @@ static void rcu_unregister(__attribute__((unused)) void *param) + rcu_unregister_thread(); + } + +-struct io_err_stat_path *find_err_path_by_dev(vector pathvec, char *dev) ++static struct io_err_stat_path *find_err_path_by_dev(vector pathvec, char *dev) + { + int i; + struct io_err_stat_path *pp; +-- +2.17.2 + diff --git a/SOURCES/0054-multipathd-avoid-io_err_stat-crash-during-shutdown.patch b/SOURCES/0054-multipathd-avoid-io_err_stat-crash-during-shutdown.patch new file mode 100644 index 0000000..67aa4e2 --- /dev/null +++ b/SOURCES/0054-multipathd-avoid-io_err_stat-crash-during-shutdown.patch @@ -0,0 +1,291 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 14 Jan 2021 20:20:23 -0600 +Subject: [PATCH] multipathd: avoid io_err_stat crash during shutdown + +The checker thread is reponsible for enqueueing paths for the +io_err_stat thread to check. During shutdown, the io_err_stat thread is +shut down and cleaned up before the checker thread. There is no code +to make sure that the checker thread isn't accessing the io_err_stat +pathvec or its mutex while they are being freed, which can lead to +memory corruption crashes. + +To solve this, get rid of the io_err_stat_pathvec structure, and +statically define the mutex. This means that the mutex is always valid +to access, and the io_err_stat pathvec can only be accessed while +holding it. If the io_err_stat thread has already been cleaned up +when the checker tries to access the pathvec, it will be NULL, and the +checker will simply fail to enqueue the path. + +This change also fixes a bug in free_io_err_pathvec(), which previously +only attempted to free the pathvec if it was not set, instead of when it +was set. + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 112 +++++++++++++++---------------------- + libmultipath/util.c | 5 ++ + libmultipath/util.h | 1 + + 3 files changed, 50 insertions(+), 68 deletions(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index 449760a0..f6c564f0 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -34,6 +34,7 @@ + #include "lock.h" + #include "time-util.h" + #include "io_err_stat.h" ++#include "util.h" + + #define IOTIMEOUT_SEC 60 + #define TIMEOUT_NO_IO_NSEC 10000000 /*10ms = 10000000ns*/ +@@ -46,12 +47,6 @@ + #define io_err_stat_log(prio, fmt, args...) \ + condlog(prio, "io error statistic: " fmt, ##args) + +- +-struct io_err_stat_pathvec { +- pthread_mutex_t mutex; +- vector pathvec; +-}; +- + struct dio_ctx { + struct timespec io_starttime; + unsigned int blksize; +@@ -76,9 +71,10 @@ pthread_attr_t io_err_stat_attr; + + static pthread_mutex_t io_err_thread_lock = PTHREAD_MUTEX_INITIALIZER; + static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER; ++static pthread_mutex_t io_err_pathvec_lock = PTHREAD_MUTEX_INITIALIZER; + static int io_err_thread_running = 0; + +-static struct io_err_stat_pathvec *paths; ++static vector io_err_pathvec; + struct vectors *vecs; + io_context_t ioctx; + +@@ -208,46 +204,23 @@ static void free_io_err_stat_path(struct io_err_stat_path *p) + FREE(p); + } + +-static struct io_err_stat_pathvec *alloc_pathvec(void) +-{ +- struct io_err_stat_pathvec *p; +- int r; +- +- p = (struct io_err_stat_pathvec *)MALLOC(sizeof(*p)); +- if (!p) +- return NULL; +- p->pathvec = vector_alloc(); +- if (!p->pathvec) +- goto out_free_struct_pathvec; +- r = pthread_mutex_init(&p->mutex, NULL); +- if (r) +- goto out_free_member_pathvec; +- +- return p; +- +-out_free_member_pathvec: +- vector_free(p->pathvec); +-out_free_struct_pathvec: +- FREE(p); +- return NULL; +-} +- +-static void free_io_err_pathvec(struct io_err_stat_pathvec *p) ++static void free_io_err_pathvec(void) + { + struct io_err_stat_path *path; + int i; + +- if (!p) +- return; +- pthread_mutex_destroy(&p->mutex); +- if (!p->pathvec) { +- vector_foreach_slot(p->pathvec, path, i) { +- destroy_directio_ctx(path); +- free_io_err_stat_path(path); +- } +- vector_free(p->pathvec); ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ pthread_cleanup_push(cleanup_mutex, &io_err_pathvec_lock); ++ if (!io_err_pathvec) ++ goto out; ++ vector_foreach_slot(io_err_pathvec, path, i) { ++ destroy_directio_ctx(path); ++ free_io_err_stat_path(path); + } +- FREE(p); ++ vector_free(io_err_pathvec); ++ io_err_pathvec = NULL; ++out: ++ pthread_cleanup_pop(1); + } + + /* +@@ -259,13 +232,13 @@ static int enqueue_io_err_stat_by_path(struct path *path) + { + struct io_err_stat_path *p; + +- pthread_mutex_lock(&paths->mutex); +- p = find_err_path_by_dev(paths->pathvec, path->dev); ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ p = find_err_path_by_dev(io_err_pathvec, path->dev); + if (p) { +- pthread_mutex_unlock(&paths->mutex); ++ pthread_mutex_unlock(&io_err_pathvec_lock); + return 0; + } +- pthread_mutex_unlock(&paths->mutex); ++ pthread_mutex_unlock(&io_err_pathvec_lock); + + p = alloc_io_err_stat_path(); + if (!p) +@@ -277,18 +250,18 @@ static int enqueue_io_err_stat_by_path(struct path *path) + + if (setup_directio_ctx(p)) + goto free_ioerr_path; +- pthread_mutex_lock(&paths->mutex); +- if (!vector_alloc_slot(paths->pathvec)) ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ if (!vector_alloc_slot(io_err_pathvec)) + goto unlock_destroy; +- vector_set_slot(paths->pathvec, p); +- pthread_mutex_unlock(&paths->mutex); ++ vector_set_slot(io_err_pathvec, p); ++ pthread_mutex_unlock(&io_err_pathvec_lock); + + io_err_stat_log(2, "%s: enqueue path %s to check", + path->mpp->alias, path->dev); + return 0; + + unlock_destroy: +- pthread_mutex_unlock(&paths->mutex); ++ pthread_mutex_unlock(&io_err_pathvec_lock); + destroy_directio_ctx(p); + free_ioerr_path: + free_io_err_stat_path(p); +@@ -421,9 +394,9 @@ static int delete_io_err_stat_by_addr(struct io_err_stat_path *p) + { + int i; + +- i = find_slot(paths->pathvec, p); ++ i = find_slot(io_err_pathvec, p); + if (i != -1) +- vector_del_slot(paths->pathvec, i); ++ vector_del_slot(io_err_pathvec, i); + + destroy_directio_ctx(p); + free_io_err_stat_path(p); +@@ -594,7 +567,7 @@ static void poll_async_io_timeout(void) + + if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0) + return; +- vector_foreach_slot(paths->pathvec, pp, i) { ++ vector_foreach_slot(io_err_pathvec, pp, i) { + for (j = 0; j < CONCUR_NR_EVENT; j++) { + rc = try_to_cancel_timeout_io(pp->dio_ctx_array + j, + &curr_time, pp->devname); +@@ -640,7 +613,7 @@ static void handle_async_io_done_event(struct io_event *io_evt) + int rc = PATH_UNCHECKED; + int i, j; + +- vector_foreach_slot(paths->pathvec, pp, i) { ++ vector_foreach_slot(io_err_pathvec, pp, i) { + for (j = 0; j < CONCUR_NR_EVENT; j++) { + ct = pp->dio_ctx_array + j; + if (&ct->io == io_evt->obj) { +@@ -674,19 +647,14 @@ static void service_paths(void) + struct io_err_stat_path *pp; + int i; + +- pthread_mutex_lock(&paths->mutex); +- vector_foreach_slot(paths->pathvec, pp, i) { ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ vector_foreach_slot(io_err_pathvec, pp, i) { + send_batch_async_ios(pp); + process_async_ios_event(TIMEOUT_NO_IO_NSEC, pp->devname); + poll_async_io_timeout(); + poll_io_err_stat(vecs, pp); + } +- pthread_mutex_unlock(&paths->mutex); +-} +- +-static void cleanup_unlock(void *arg) +-{ +- pthread_mutex_unlock((pthread_mutex_t*) arg); ++ pthread_mutex_unlock(&io_err_pathvec_lock); + } + + static void cleanup_exited(__attribute__((unused)) void *arg) +@@ -744,12 +712,17 @@ int start_io_err_stat_thread(void *data) + io_err_stat_log(4, "io_setup failed"); + return 1; + } +- paths = alloc_pathvec(); +- if (!paths) ++ ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ io_err_pathvec = vector_alloc(); ++ if (!io_err_pathvec) { ++ pthread_mutex_unlock(&io_err_pathvec_lock); + goto destroy_ctx; ++ } ++ pthread_mutex_unlock(&io_err_pathvec_lock); + + pthread_mutex_lock(&io_err_thread_lock); +- pthread_cleanup_push(cleanup_unlock, &io_err_thread_lock); ++ pthread_cleanup_push(cleanup_mutex, &io_err_thread_lock); + + ret = pthread_create(&io_err_stat_thr, &io_err_stat_attr, + io_err_stat_loop, data); +@@ -769,7 +742,10 @@ int start_io_err_stat_thread(void *data) + return 0; + + out_free: +- free_io_err_pathvec(paths); ++ pthread_mutex_lock(&io_err_pathvec_lock); ++ vector_free(io_err_pathvec); ++ io_err_pathvec = NULL; ++ pthread_mutex_unlock(&io_err_pathvec_lock); + destroy_ctx: + io_destroy(ioctx); + io_err_stat_log(0, "failed to start io_error statistic thread"); +@@ -785,6 +761,6 @@ void stop_io_err_stat_thread(void) + pthread_cancel(io_err_stat_thr); + + pthread_join(io_err_stat_thr, NULL); +- free_io_err_pathvec(paths); ++ free_io_err_pathvec(); + io_destroy(ioctx); + } +diff --git a/libmultipath/util.c b/libmultipath/util.c +index 51c38c87..dd30a46e 100644 +--- a/libmultipath/util.c ++++ b/libmultipath/util.c +@@ -469,3 +469,8 @@ void close_fd(void *arg) + { + close((long)arg); + } ++ ++void cleanup_mutex(void *arg) ++{ ++ pthread_mutex_unlock(arg); ++} +diff --git a/libmultipath/util.h b/libmultipath/util.h +index 56bd78c7..ce277680 100644 +--- a/libmultipath/util.h ++++ b/libmultipath/util.h +@@ -44,6 +44,7 @@ void set_max_fds(rlim_t max_fds); + pthread_cleanup_push(((void (*)(void *))&f), (arg)) + + void close_fd(void *arg); ++void cleanup_mutex(void *arg); + + struct scandir_result { + struct dirent **di; +-- +2.17.2 + diff --git a/SOURCES/0055-multipathd-avoid-io_err_stat-ABBA-deadlock.patch b/SOURCES/0055-multipathd-avoid-io_err_stat-ABBA-deadlock.patch new file mode 100644 index 0000000..8fd1605 --- /dev/null +++ b/SOURCES/0055-multipathd-avoid-io_err_stat-ABBA-deadlock.patch @@ -0,0 +1,146 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 14 Jan 2021 20:20:24 -0600 +Subject: [PATCH] multipathd: avoid io_err_stat ABBA deadlock + +When the checker thread enqueues paths for the io_err_stat thread to +check, it calls enqueue_io_err_stat_by_path() with the vecs lock held. +start_io_err_stat_thread() is also called with the vecs lock held. +These two functions both lock io_err_pathvec_lock. When the io_err_stat +thread updates the paths in vecs->pathvec in poll_io_err_stat(), it has +the io_err_pathvec_lock held, and then locks the vecs lock. This can +cause an ABBA deadlock. + +To solve this, service_paths() no longer updates the paths in +vecs->pathvec with the io_err_pathvec_lock held. It does this by moving +the io_err_stat_path from io_err_pathvec to a local vector when it needs +to update the path. After releasing the io_err_pathvec_lock, it goes +through this temporary vector, updates the paths with the vecs lock +held, and then frees everything. + +This change fixes a bug in service_paths() where elements were being +deleted from io_err_pathvec, without the index being decremented, +causing the loop to skip elements. Also, service_paths() could be +cancelled while holding the io_err_pathvec_lock, so it should have a +cleanup handler. + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 56 ++++++++++++++++++++++---------------- + 1 file changed, 32 insertions(+), 24 deletions(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index f6c564f0..63ee2e07 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -390,20 +390,6 @@ recover: + return 0; + } + +-static int delete_io_err_stat_by_addr(struct io_err_stat_path *p) +-{ +- int i; +- +- i = find_slot(io_err_pathvec, p); +- if (i != -1) +- vector_del_slot(io_err_pathvec, i); +- +- destroy_directio_ctx(p); +- free_io_err_stat_path(p); +- +- return 0; +-} +- + static void account_async_io_state(struct io_err_stat_path *pp, int rc) + { + switch (rc) { +@@ -420,17 +406,26 @@ static void account_async_io_state(struct io_err_stat_path *pp, int rc) + } + } + +-static int poll_io_err_stat(struct vectors *vecs, struct io_err_stat_path *pp) ++static int io_err_stat_time_up(struct io_err_stat_path *pp) + { + struct timespec currtime, difftime; +- struct path *path; +- double err_rate; + + if (clock_gettime(CLOCK_MONOTONIC, &currtime) != 0) +- return 1; ++ return 0; + timespecsub(&currtime, &pp->start_time, &difftime); + if (difftime.tv_sec < pp->total_time) + return 0; ++ return 1; ++} ++ ++static void end_io_err_stat(struct io_err_stat_path *pp) ++{ ++ struct timespec currtime; ++ struct path *path; ++ double err_rate; ++ ++ if (clock_gettime(CLOCK_MONOTONIC, &currtime) != 0) ++ currtime = pp->start_time; + + io_err_stat_log(4, "%s: check end", pp->devname); + +@@ -469,10 +464,6 @@ static int poll_io_err_stat(struct vectors *vecs, struct io_err_stat_path *pp) + pp->devname); + } + lock_cleanup_pop(vecs->lock); +- +- delete_io_err_stat_by_addr(pp); +- +- return 0; + } + + static int send_each_async_io(struct dio_ctx *ct, int fd, char *dev) +@@ -632,6 +623,7 @@ static void process_async_ios_event(int timeout_nsecs, char *dev) + struct timespec timeout = { .tv_nsec = timeout_nsecs }; + + errno = 0; ++ pthread_testcancel(); + n = io_getevents(ioctx, 1L, CONCUR_NR_EVENT, events, &timeout); + if (n < 0) { + io_err_stat_log(3, "%s: async io events returned %d (errno=%s)", +@@ -644,17 +636,33 @@ static void process_async_ios_event(int timeout_nsecs, char *dev) + + static void service_paths(void) + { ++ struct _vector _pathvec = {0}; ++ /* avoid gcc warnings that &_pathvec will never be NULL in vector ops */ ++ struct _vector * const tmp_pathvec = &_pathvec; + struct io_err_stat_path *pp; + int i; + + pthread_mutex_lock(&io_err_pathvec_lock); ++ pthread_cleanup_push(cleanup_mutex, &io_err_pathvec_lock); + vector_foreach_slot(io_err_pathvec, pp, i) { + send_batch_async_ios(pp); + process_async_ios_event(TIMEOUT_NO_IO_NSEC, pp->devname); + poll_async_io_timeout(); +- poll_io_err_stat(vecs, pp); ++ if (io_err_stat_time_up(pp)) { ++ if (!vector_alloc_slot(tmp_pathvec)) ++ continue; ++ vector_del_slot(io_err_pathvec, i--); ++ vector_set_slot(tmp_pathvec, pp); ++ } + } +- pthread_mutex_unlock(&io_err_pathvec_lock); ++ pthread_cleanup_pop(1); ++ vector_foreach_slot_backwards(tmp_pathvec, pp, i) { ++ end_io_err_stat(pp); ++ vector_del_slot(tmp_pathvec, i); ++ destroy_directio_ctx(pp); ++ free_io_err_stat_path(pp); ++ } ++ vector_reset(tmp_pathvec); + } + + static void cleanup_exited(__attribute__((unused)) void *arg) +-- +2.17.2 + diff --git a/SOURCES/0056-multipathd-use-get_monotonic_time-in-io_err_stat-cod.patch b/SOURCES/0056-multipathd-use-get_monotonic_time-in-io_err_stat-cod.patch new file mode 100644 index 0000000..c6ebd39 --- /dev/null +++ b/SOURCES/0056-multipathd-use-get_monotonic_time-in-io_err_stat-cod.patch @@ -0,0 +1,111 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 14 Jan 2021 20:20:25 -0600 +Subject: [PATCH] multipathd: use get_monotonic_time() in io_err_stat code + +Instead of calling clock_gettime(), and dealing with failure +conditions, just call get_monotonic_time(). + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 34 +++++++++++----------------------- + 1 file changed, 11 insertions(+), 23 deletions(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index 63ee2e07..3389d693 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -305,8 +305,7 @@ int io_err_stat_handle_pathfail(struct path *path) + * the repeated count threshold and time frame, we assume a path + * which fails at least twice within 60 seconds is flaky. + */ +- if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0) +- return 1; ++ get_monotonic_time(&curr_time); + if (path->io_err_pathfail_cnt == 0) { + path->io_err_pathfail_cnt++; + path->io_err_pathfail_starttime = curr_time.tv_sec; +@@ -362,9 +361,9 @@ int need_io_err_check(struct path *pp) + } + if (pp->io_err_pathfail_cnt != PATH_IO_ERR_WAITING_TO_CHECK) + return 1; +- if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0 || +- (curr_time.tv_sec - pp->io_err_dis_reinstate_time) > +- pp->mpp->marginal_path_err_recheck_gap_time) { ++ get_monotonic_time(&curr_time); ++ if ((curr_time.tv_sec - pp->io_err_dis_reinstate_time) > ++ pp->mpp->marginal_path_err_recheck_gap_time) { + io_err_stat_log(4, "%s: reschedule checking after %d seconds", + pp->dev, + pp->mpp->marginal_path_err_recheck_gap_time); +@@ -410,8 +409,7 @@ static int io_err_stat_time_up(struct io_err_stat_path *pp) + { + struct timespec currtime, difftime; + +- if (clock_gettime(CLOCK_MONOTONIC, &currtime) != 0) +- return 0; ++ get_monotonic_time(&currtime); + timespecsub(&currtime, &pp->start_time, &difftime); + if (difftime.tv_sec < pp->total_time) + return 0; +@@ -424,8 +422,7 @@ static void end_io_err_stat(struct io_err_stat_path *pp) + struct path *path; + double err_rate; + +- if (clock_gettime(CLOCK_MONOTONIC, &currtime) != 0) +- currtime = pp->start_time; ++ get_monotonic_time(&currtime); + + io_err_stat_log(4, "%s: check end", pp->devname); + +@@ -474,11 +471,7 @@ static int send_each_async_io(struct dio_ctx *ct, int fd, char *dev) + ct->io_starttime.tv_sec == 0) { + struct iocb *ios[1] = { &ct->io }; + +- if (clock_gettime(CLOCK_MONOTONIC, &ct->io_starttime) != 0) { +- ct->io_starttime.tv_sec = 0; +- ct->io_starttime.tv_nsec = 0; +- return rc; +- } ++ get_monotonic_time(&ct->io_starttime); + io_prep_pread(&ct->io, fd, ct->buf, ct->blksize, 0); + if (io_submit(ioctx, 1, ios) != 1) { + io_err_stat_log(5, "%s: io_submit error %i", +@@ -497,8 +490,7 @@ static void send_batch_async_ios(struct io_err_stat_path *pp) + struct dio_ctx *ct; + struct timespec currtime, difftime; + +- if (clock_gettime(CLOCK_MONOTONIC, &currtime) != 0) +- return; ++ get_monotonic_time(&currtime); + /* + * Give a free time for all IO to complete or timeout + */ +@@ -513,11 +505,8 @@ static void send_batch_async_ios(struct io_err_stat_path *pp) + if (!send_each_async_io(ct, pp->fd, pp->devname)) + pp->io_nr++; + } +- if (pp->start_time.tv_sec == 0 && pp->start_time.tv_nsec == 0 && +- clock_gettime(CLOCK_MONOTONIC, &pp->start_time)) { +- pp->start_time.tv_sec = 0; +- pp->start_time.tv_nsec = 0; +- } ++ if (pp->start_time.tv_sec == 0 && pp->start_time.tv_nsec == 0) ++ get_monotonic_time(&pp->start_time); + } + + static int try_to_cancel_timeout_io(struct dio_ctx *ct, struct timespec *t, +@@ -556,8 +545,7 @@ static void poll_async_io_timeout(void) + int rc = PATH_UNCHECKED; + int i, j; + +- if (clock_gettime(CLOCK_MONOTONIC, &curr_time) != 0) +- return; ++ get_monotonic_time(&curr_time); + vector_foreach_slot(io_err_pathvec, pp, i) { + for (j = 0; j < CONCUR_NR_EVENT; j++) { + rc = try_to_cancel_timeout_io(pp->dio_ctx_array + j, +-- +2.17.2 + diff --git a/SOURCES/0057-multipathd-combine-free_io_err_stat_path-and-destroy.patch b/SOURCES/0057-multipathd-combine-free_io_err_stat_path-and-destroy.patch new file mode 100644 index 0000000..c3573c7 --- /dev/null +++ b/SOURCES/0057-multipathd-combine-free_io_err_stat_path-and-destroy.patch @@ -0,0 +1,101 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 14 Jan 2021 20:20:26 -0600 +Subject: [PATCH] multipathd: combine free_io_err_stat_path and + destroy_directio_ctx + +destroy_directio_ctx() is only called from free_io_err_stat_path(), and +free_io_err_stat_path() is very short, so combine them. + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 24 ++++++++++-------------- + 1 file changed, 10 insertions(+), 14 deletions(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index 3389d693..bf1d3910 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -163,12 +163,15 @@ fail_close: + return 1; + } + +-static void destroy_directio_ctx(struct io_err_stat_path *p) ++static void free_io_err_stat_path(struct io_err_stat_path *p) + { + int i; + +- if (!p || !p->dio_ctx_array) ++ if (!p) + return; ++ if (!p->dio_ctx_array) ++ goto free_path; ++ + cancel_inflight_io(p); + + for (i = 0; i < CONCUR_NR_EVENT; i++) +@@ -177,6 +180,8 @@ static void destroy_directio_ctx(struct io_err_stat_path *p) + + if (p->fd > 0) + close(p->fd); ++free_path: ++ FREE(p); + } + + static struct io_err_stat_path *alloc_io_err_stat_path(void) +@@ -199,11 +204,6 @@ static struct io_err_stat_path *alloc_io_err_stat_path(void) + return p; + } + +-static void free_io_err_stat_path(struct io_err_stat_path *p) +-{ +- FREE(p); +-} +- + static void free_io_err_pathvec(void) + { + struct io_err_stat_path *path; +@@ -213,10 +213,8 @@ static void free_io_err_pathvec(void) + pthread_cleanup_push(cleanup_mutex, &io_err_pathvec_lock); + if (!io_err_pathvec) + goto out; +- vector_foreach_slot(io_err_pathvec, path, i) { +- destroy_directio_ctx(path); ++ vector_foreach_slot(io_err_pathvec, path, i) + free_io_err_stat_path(path); +- } + vector_free(io_err_pathvec); + io_err_pathvec = NULL; + out: +@@ -252,7 +250,7 @@ static int enqueue_io_err_stat_by_path(struct path *path) + goto free_ioerr_path; + pthread_mutex_lock(&io_err_pathvec_lock); + if (!vector_alloc_slot(io_err_pathvec)) +- goto unlock_destroy; ++ goto unlock_pathvec; + vector_set_slot(io_err_pathvec, p); + pthread_mutex_unlock(&io_err_pathvec_lock); + +@@ -260,9 +258,8 @@ static int enqueue_io_err_stat_by_path(struct path *path) + path->mpp->alias, path->dev); + return 0; + +-unlock_destroy: ++unlock_pathvec: + pthread_mutex_unlock(&io_err_pathvec_lock); +- destroy_directio_ctx(p); + free_ioerr_path: + free_io_err_stat_path(p); + +@@ -647,7 +644,6 @@ static void service_paths(void) + vector_foreach_slot_backwards(tmp_pathvec, pp, i) { + end_io_err_stat(pp); + vector_del_slot(tmp_pathvec, i); +- destroy_directio_ctx(pp); + free_io_err_stat_path(pp); + } + vector_reset(tmp_pathvec); +-- +2.17.2 + diff --git a/SOURCES/0058-multipathd-cleanup-logging-for-marginal-paths.patch b/SOURCES/0058-multipathd-cleanup-logging-for-marginal-paths.patch new file mode 100644 index 0000000..2983712 --- /dev/null +++ b/SOURCES/0058-multipathd-cleanup-logging-for-marginal-paths.patch @@ -0,0 +1,110 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 18 Jan 2021 22:46:04 -0600 +Subject: [PATCH] multipathd: cleanup logging for marginal paths + +io_err_stat logged at level 2 whenever it enqueued a path to check, +which could happen multiple times while a path was marginal. On the +other hand if marginal_pathgroups wasn't set, multipathd didn't log when +paths were set to marginal. Now io_err_stat only logs at level 2 when +something unexpected happens, but multipathd will always log when a +path switches its marginal state. + +This patch also fixes an issue where paths in the delayed state could +get set to the pending state if they could not be checked in time. +Aside from going against the idea the paths should not be set to pending +if they already have a valid state, this caused multipathd to log a +message whenever the path state switched to from delayed to pending and +then back. + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmultipath/io_err_stat.c | 7 +++---- + multipathd/main.c | 20 +++++++++++++------- + 2 files changed, 16 insertions(+), 11 deletions(-) + +diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c +index bf1d3910..ee711f7f 100644 +--- a/libmultipath/io_err_stat.c ++++ b/libmultipath/io_err_stat.c +@@ -254,7 +254,7 @@ static int enqueue_io_err_stat_by_path(struct path *path) + vector_set_slot(io_err_pathvec, p); + pthread_mutex_unlock(&io_err_pathvec_lock); + +- io_err_stat_log(2, "%s: enqueue path %s to check", ++ io_err_stat_log(3, "%s: enqueue path %s to check", + path->mpp->alias, path->dev); + return 0; + +@@ -353,7 +353,7 @@ int need_io_err_check(struct path *pp) + if (uatomic_read(&io_err_thread_running) == 0) + return 0; + if (count_active_paths(pp->mpp) <= 0) { +- io_err_stat_log(2, "%s: recover path early", pp->dev); ++ io_err_stat_log(2, "%s: no paths. recovering early", pp->dev); + goto recover; + } + if (pp->io_err_pathfail_cnt != PATH_IO_ERR_WAITING_TO_CHECK) +@@ -371,8 +371,7 @@ int need_io_err_check(struct path *pp) + * Or else, return 1 to set path state to PATH_SHAKY + */ + if (r == 1) { +- io_err_stat_log(3, "%s: enqueue fails, to recover", +- pp->dev); ++ io_err_stat_log(2, "%s: enqueue failed. recovering early", pp->dev); + goto recover; + } else + pp->io_err_pathfail_cnt = PATH_IO_ERR_IN_CHECKING; +diff --git a/multipathd/main.c b/multipathd/main.c +index 1d0579e9..cc1aeea2 100644 +--- a/multipathd/main.c ++++ b/multipathd/main.c +@@ -2041,8 +2041,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) + pathinfo(pp, conf, 0); + pthread_cleanup_pop(1); + return 1; +- } else if ((newstate != PATH_UP && newstate != PATH_GHOST) && +- (pp->state == PATH_DELAYED)) { ++ } else if ((newstate != PATH_UP && newstate != PATH_GHOST && ++ newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) { + /* If path state become failed again cancel path delay state */ + pp->state = newstate; + return 1; +@@ -2104,8 +2104,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) + if ((newstate == PATH_UP || newstate == PATH_GHOST) && + (san_path_check_enabled(pp->mpp) || + marginal_path_check_enabled(pp->mpp))) { +- int was_marginal = pp->marginal; + if (should_skip_path(pp)) { ++ if (!pp->marginal && pp->state != PATH_DELAYED) ++ condlog(2, "%s: path is now marginal", pp->dev); + if (!marginal_pathgroups) { + if (marginal_path_check_enabled(pp->mpp)) + /* to reschedule as soon as possible, +@@ -2115,13 +2116,18 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) + pp->state = PATH_DELAYED; + return 1; + } +- if (!was_marginal) { ++ if (!pp->marginal) { + pp->marginal = 1; + marginal_changed = 1; + } +- } else if (marginal_pathgroups && was_marginal) { +- pp->marginal = 0; +- marginal_changed = 1; ++ } else { ++ if (pp->marginal || pp->state == PATH_DELAYED) ++ condlog(2, "%s: path is no longer marginal", ++ pp->dev); ++ if (marginal_pathgroups && pp->marginal) { ++ pp->marginal = 0; ++ marginal_changed = 1; ++ } + } + } + +-- +2.17.2 + diff --git a/SOURCES/0059-libmpathpersist-fix-thread-safety-of-default-functio.patch b/SOURCES/0059-libmpathpersist-fix-thread-safety-of-default-functio.patch new file mode 100644 index 0000000..5bbd255 --- /dev/null +++ b/SOURCES/0059-libmpathpersist-fix-thread-safety-of-default-functio.patch @@ -0,0 +1,277 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 25 Jan 2021 23:31:04 -0600 +Subject: [PATCH] libmpathpersist: fix thread safety of default functions + +commit a839e39e ("libmpathpersist: factor out initialization and +teardown") made mpath_presistent_reserve_{in,out} use share variables +for curmp and pathvec. There are users of this library that call these +functions in a multi-threaded process, and this change causes their +application to crash. config and udev are also shared variables, but +libmpathpersist doesn't write to the config in +mpath_presistent_reserve_{in,out}, and looking into the libudev code, I +don't see any place where libmpathpersist uses the udev object in a way +that isn't thread-safe. + +This patch makes mpath_presistent_reserve_{in,out} go back to using +local variables for curmp and pathvec, so that multiple threads won't +be operating on these variables at the same time. + +Fixes: a839e39e ("libmpathpersist: factor out initialization and teardown") +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + libmpathpersist/mpath_persist.c | 116 +++++++++++++++++++++----------- + libmpathpersist/mpath_persist.h | 24 +++++-- + 2 files changed, 94 insertions(+), 46 deletions(-) + +diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c +index a01dfb0b..07a5f17f 100644 +--- a/libmpathpersist/mpath_persist.c ++++ b/libmpathpersist/mpath_persist.c +@@ -147,72 +147,60 @@ mpath_prin_activepath (struct multipath *mpp, int rq_servact, + return ret; + } + +-int mpath_persistent_reserve_in (int fd, int rq_servact, +- struct prin_resp *resp, int noisy, int verbose) +-{ +- int ret = mpath_persistent_reserve_init_vecs(verbose); +- +- if (ret != MPATH_PR_SUCCESS) +- return ret; +- ret = __mpath_persistent_reserve_in(fd, rq_servact, resp, noisy); +- mpath_persistent_reserve_free_vecs(); +- return ret; +-} +- +-int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, +- unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose) +-{ +- int ret = mpath_persistent_reserve_init_vecs(verbose); +- +- if (ret != MPATH_PR_SUCCESS) +- return ret; +- ret = __mpath_persistent_reserve_out(fd, rq_servact, rq_scope, rq_type, +- paramp, noisy); +- mpath_persistent_reserve_free_vecs(); +- return ret; +-} +- + static vector curmp; + static vector pathvec; + +-void mpath_persistent_reserve_free_vecs(void) ++static void __mpath_persistent_reserve_free_vecs(vector curmp, vector pathvec) + { + free_multipathvec(curmp, KEEP_PATHS); + free_pathvec(pathvec, FREE_PATHS); ++} ++ ++void mpath_persistent_reserve_free_vecs(void) ++{ ++ __mpath_persistent_reserve_free_vecs(curmp, pathvec); + curmp = pathvec = NULL; + } + +-int mpath_persistent_reserve_init_vecs(int verbose) ++static int __mpath_persistent_reserve_init_vecs(vector *curmp_p, ++ vector *pathvec_p, int verbose) + { + struct config *conf = get_multipath_config(); + + conf->verbosity = verbose; + put_multipath_config(conf); + +- if (curmp) ++ if (*curmp_p) + return MPATH_PR_SUCCESS; + /* + * allocate core vectors to store paths and multipaths + */ +- curmp = vector_alloc (); +- pathvec = vector_alloc (); ++ *curmp_p = vector_alloc (); ++ *pathvec_p = vector_alloc (); + +- if (!curmp || !pathvec){ ++ if (!*curmp_p || !*pathvec_p){ + condlog (0, "vector allocation failed."); + goto err; + } + +- if (dm_get_maps(curmp)) ++ if (dm_get_maps(*curmp_p)) + goto err; + + return MPATH_PR_SUCCESS; + + err: +- mpath_persistent_reserve_free_vecs(); ++ __mpath_persistent_reserve_free_vecs(*curmp_p, *pathvec_p); ++ *curmp_p = *pathvec_p = NULL; + return MPATH_PR_DMMP_ERROR; + } + +-static int mpath_get_map(int fd, char **palias, struct multipath **pmpp) ++int mpath_persistent_reserve_init_vecs(int verbose) ++{ ++ return __mpath_persistent_reserve_init_vecs(&curmp, &pathvec, verbose); ++} ++ ++static int mpath_get_map(vector curmp, vector pathvec, int fd, char **palias, ++ struct multipath **pmpp) + { + int ret = MPATH_PR_DMMP_ERROR; + struct stat info; +@@ -272,13 +260,13 @@ out: + return ret; + } + +-int __mpath_persistent_reserve_in (int fd, int rq_servact, +- struct prin_resp *resp, int noisy) ++static int do_mpath_persistent_reserve_in (vector curmp, vector pathvec, ++ int fd, int rq_servact, struct prin_resp *resp, int noisy) + { + struct multipath *mpp; + int ret; + +- ret = mpath_get_map(fd, NULL, &mpp); ++ ret = mpath_get_map(curmp, pathvec, fd, NULL, &mpp); + if (ret != MPATH_PR_SUCCESS) + return ret; + +@@ -287,8 +275,17 @@ int __mpath_persistent_reserve_in (int fd, int rq_servact, + return ret; + } + +-int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, +- unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy) ++ ++int __mpath_persistent_reserve_in (int fd, int rq_servact, ++ struct prin_resp *resp, int noisy) ++{ ++ return do_mpath_persistent_reserve_in(curmp, pathvec, fd, rq_servact, ++ resp, noisy); ++} ++ ++static int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd, ++ int rq_servact, int rq_scope, unsigned int rq_type, ++ struct prout_param_descriptor *paramp, int noisy) + { + struct multipath *mpp; + char *alias; +@@ -296,7 +293,7 @@ int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, + uint64_t prkey; + struct config *conf; + +- ret = mpath_get_map(fd, &alias, &mpp); ++ ret = mpath_get_map(curmp, pathvec, fd, &alias, &mpp); + if (ret != MPATH_PR_SUCCESS) + return ret; + +@@ -366,6 +363,45 @@ out1: + return ret; + } + ++ ++int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, ++ unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy) ++{ ++ return do_mpath_persistent_reserve_out(curmp, pathvec, fd, rq_servact, ++ rq_scope, rq_type, paramp, ++ noisy); ++} ++ ++int mpath_persistent_reserve_in (int fd, int rq_servact, ++ struct prin_resp *resp, int noisy, int verbose) ++{ ++ vector curmp = NULL, pathvec; ++ int ret = __mpath_persistent_reserve_init_vecs(&curmp, &pathvec, ++ verbose); ++ ++ if (ret != MPATH_PR_SUCCESS) ++ return ret; ++ ret = do_mpath_persistent_reserve_in(curmp, pathvec, fd, rq_servact, ++ resp, noisy); ++ __mpath_persistent_reserve_free_vecs(curmp, pathvec); ++ return ret; ++} ++ ++int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, ++ unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose) ++{ ++ vector curmp = NULL, pathvec; ++ int ret = __mpath_persistent_reserve_init_vecs(&curmp, &pathvec, ++ verbose); ++ ++ if (ret != MPATH_PR_SUCCESS) ++ return ret; ++ ret = do_mpath_persistent_reserve_out(curmp, pathvec, fd, rq_servact, ++ rq_scope, rq_type, paramp, noisy); ++ __mpath_persistent_reserve_free_vecs(curmp, pathvec); ++ return ret; ++} ++ + int + get_mpvec (vector curmp, vector pathvec, char * refwwid) + { +diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h +index 7cf4faf9..0e4e0e53 100644 +--- a/libmpathpersist/mpath_persist.h ++++ b/libmpathpersist/mpath_persist.h +@@ -215,9 +215,13 @@ extern int mpath_persistent_reserve_in (int fd, int rq_servact, struct prin_resp + + /* + * DESCRIPTION : +- * This function is like mpath_persistent_reserve_in(), except that it doesn't call +- * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs() +- * before and after the actual PR call. ++ * This function is like mpath_persistent_reserve_in(), except that it ++ * requires mpath_persistent_reserve_init_vecs() to be called before the ++ * PR call to set up internal variables. These must later be cleanup up ++ * by calling mpath_persistent_reserve_free_vecs(). ++ * ++ * RESTRICTIONS: ++ * This function uses static internal variables, and is not thread-safe. + */ + extern int __mpath_persistent_reserve_in(int fd, int rq_servact, + struct prin_resp *resp, int noisy); +@@ -249,9 +253,13 @@ extern int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, + int verbose); + /* + * DESCRIPTION : +- * This function is like mpath_persistent_reserve_out(), except that it doesn't call +- * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs() +- * before and after the actual PR call. ++ * This function is like mpath_persistent_reserve_out(), except that it ++ * requires mpath_persistent_reserve_init_vecs() to be called before the ++ * PR call to set up internal variables. These must later be cleanup up ++ * by calling mpath_persistent_reserve_free_vecs(). ++ * ++ * RESTRICTIONS: ++ * This function uses static internal variables, and is not thread-safe. + */ + extern int __mpath_persistent_reserve_out( int fd, int rq_servact, int rq_scope, + unsigned int rq_type, struct prout_param_descriptor *paramp, +@@ -265,6 +273,7 @@ extern int __mpath_persistent_reserve_out( int fd, int rq_servact, int rq_scope, + * @verbose: Set verbosity level. Input argument. value:0 to 3. 0->disabled, 3->Max verbose + * + * RESTRICTIONS: ++ * This function uses static internal variables, and is not thread-safe. + * + * RETURNS: MPATH_PR_SUCCESS if successful else returns any of the status specified + * above in RETURN_STATUS. +@@ -275,6 +284,9 @@ int mpath_persistent_reserve_init_vecs(int verbose); + * DESCRIPTION : + * This function frees data structures allocated by + * mpath_persistent_reserve_init_vecs(). ++ * ++ * RESTRICTIONS: ++ * This function uses static internal variables, and is not thread-safe. + */ + void mpath_persistent_reserve_free_vecs(void); + +-- +2.17.2 + diff --git a/SPECS/device-mapper-multipath.spec b/SPECS/device-mapper-multipath.spec index f73dc9f..8f4feda 100644 --- a/SPECS/device-mapper-multipath.spec +++ b/SPECS/device-mapper-multipath.spec @@ -1,7 +1,7 @@ Summary: Tools to manage multipath devices using device-mapper Name: device-mapper-multipath Version: 0.8.4 -Release: 7%{?dist} +Release: 8%{?dist} License: GPLv2 Group: System Environment/Base URL: http://christophe.varoqui.free.fr/ @@ -62,6 +62,14 @@ Patch00048: 0048-mpathpersist-Fix-Register-and-Ignore-with-0x00-SARK.patch Patch00049: 0049-mpathpersist-update-prkeys-file-on-changing-registra.patch Patch00050: 0050-multipathd-Fix-multipathd-stopping-on-shutdown.patch Patch00051: 0051-multipath.conf.5-Improve-checker_timeout-description.patch +Patch00052: 0052-libmultipath-check-for-null-wwid-before-strcmp.patch +Patch00053: 0053-libmultipath-make-find_err_path_by_dev-static.patch +Patch00054: 0054-multipathd-avoid-io_err_stat-crash-during-shutdown.patch +Patch00055: 0055-multipathd-avoid-io_err_stat-ABBA-deadlock.patch +Patch00056: 0056-multipathd-use-get_monotonic_time-in-io_err_stat-cod.patch +Patch00057: 0057-multipathd-combine-free_io_err_stat_path-and-destroy.patch +Patch00058: 0058-multipathd-cleanup-logging-for-marginal-paths.patch +Patch00059: 0059-libmpathpersist-fix-thread-safety-of-default-functio.patch # runtime Requires: %{name}-libs = %{version}-%{release} @@ -263,6 +271,20 @@ fi %{_pkgconfdir}/libdmmp.pc %changelog +* Wed Jan 27 2021 Benjamin Marzinski 0.8.4-7 +- Add 0052-libmultipath-check-for-null-wwid-before-strcmp.patch + * Missing part of fix for bz #1897815 +- Add 0053-libmultipath-make-find_err_path_by_dev-static.patch +- Add 0054-multipathd-avoid-io_err_stat-crash-during-shutdown.patch +- Add 0055-multipathd-avoid-io_err_stat-ABBA-deadlock.patch +- Add 0056-multipathd-use-get_monotonic_time-in-io_err_stat-cod.patch +- Add 0057-multipathd-combine-free_io_err_stat_path-and-destroy.patch +- Add 0058-multipathd-cleanup-logging-for-marginal-paths.patch + * The above 6 patches fix bz #1920795 +- Add 0059-libmpathpersist-fix-thread-safety-of-default-functio.patch + * Fixes bz #1913549 +- Resolves: bz #1897815, #1913549, #1920795 + * Tue Dec 15 2020 Benjamin Marzinski 0.8.4-6 - Add 0050-multipathd-Fix-multipathd-stopping-on-shutdown.patch * Fixes bz #1892496