diff --git a/SOURCES/0039-libmultipath-factor-out-code-to-get-vpd-page-data.patch b/SOURCES/0039-libmultipath-factor-out-code-to-get-vpd-page-data.patch new file mode 100644 index 0000000..9862e22 --- /dev/null +++ b/SOURCES/0039-libmultipath-factor-out-code-to-get-vpd-page-data.patch @@ -0,0 +1,54 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Tue, 3 Nov 2020 14:27:58 -0600 +Subject: [PATCH] libmultipath: factor out code to get vpd page data + +A future patch will reuse the code to get the vpd page data, so factor +it out from get_vpd_sgio(). + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/discovery.c | 19 +++++++++++++++---- + 1 file changed, 15 insertions(+), 4 deletions(-) + +diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c +index aa5942c3..eb1e735d 100644 +--- a/libmultipath/discovery.c ++++ b/libmultipath/discovery.c +@@ -1239,11 +1239,10 @@ get_vpd_sysfs (struct udev_device *parent, int pg, char * str, int maxlen) + return len; + } + +-int +-get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen) ++static int ++fetch_vpd_page(int fd, int pg, unsigned char *buff) + { +- int len, buff_len; +- unsigned char buff[4096]; ++ int buff_len; + + memset(buff, 0x0, 4096); + if (sgio_get_vpd(buff, 4096, fd, pg) < 0) { +@@ -1264,6 +1263,18 @@ get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen) + condlog(3, "vpd pg%02x page truncated", pg); + buff_len = 4096; + } ++ return buff_len; ++} ++ ++int ++get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen) ++{ ++ int len, buff_len; ++ unsigned char buff[4096]; ++ ++ buff_len = fetch_vpd_page(fd, pg, buff); ++ if (buff_len < 0) ++ return buff_len; + if (pg == 0x80) + len = parse_vpd_pg80(buff, str, maxlen); + else if (pg == 0x83) +-- +2.17.2 + diff --git a/SOURCES/0040-libmultipath-limit-reading-0xc9-vpd-page.patch b/SOURCES/0040-libmultipath-limit-reading-0xc9-vpd-page.patch new file mode 100644 index 0000000..d23bdaf --- /dev/null +++ b/SOURCES/0040-libmultipath-limit-reading-0xc9-vpd-page.patch @@ -0,0 +1,98 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Wed, 7 Oct 2020 21:43:02 -0500 +Subject: [PATCH] libmultipath: limit reading 0xc9 vpd page + +Only rdac arrays support 0xC9 vpd page inquiries. All other arrays will +return a failure. Only do the rdac inquiry when detecting array +capabilities if the array's path checker is explicitly set to rdac, or +the path checker is not set, and the array reports that it supports vpd +page 0xC9 in the Supported VPD Pages (0x00) vpd page. + +Multipath was doing the check if either the path checker was set to +rdac, or no path checker was set. This means that for almost all +non-rdac arrays, multipath was issuing a bad inquiry. This was annoying +users. + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/discovery.c | 25 +++++++++++++++++++++++++ + libmultipath/discovery.h | 1 + + libmultipath/propsel.c | 10 ++++++---- + 3 files changed, 32 insertions(+), 4 deletions(-) + +diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c +index eb1e735d..01aadba9 100644 +--- a/libmultipath/discovery.c ++++ b/libmultipath/discovery.c +@@ -1266,6 +1266,31 @@ fetch_vpd_page(int fd, int pg, unsigned char *buff) + return buff_len; + } + ++/* heavily based on sg_inq.c from sg3_utils */ ++bool ++is_vpd_page_supported(int fd, int pg) ++{ ++ int i, len, buff_len; ++ unsigned char buff[4096]; ++ ++ buff_len = fetch_vpd_page(fd, 0x00, buff); ++ if (buff_len < 0) ++ return false; ++ if (buff_len < 4) { ++ condlog(3, "VPD page 00h too short"); ++ return false; ++ } ++ ++ len = buff[3] + 4; ++ if (len > buff_len) ++ condlog(3, "vpd page 00h trucated, expected %d, have %d", ++ len, buff_len); ++ for (i = 4; i < len; ++i) ++ if (buff[i] == pg) ++ return true; ++ return false; ++} ++ + int + get_vpd_sgio (int fd, int pg, int vend_id, char * str, int maxlen) + { +diff --git a/libmultipath/discovery.h b/libmultipath/discovery.h +index 6444887d..d3193daf 100644 +--- a/libmultipath/discovery.h ++++ b/libmultipath/discovery.h +@@ -56,6 +56,7 @@ int sysfs_get_asymmetric_access_state(struct path *pp, + char *buff, int buflen); + int get_uid(struct path * pp, int path_state, struct udev_device *udev, + int allow_fallback); ++bool is_vpd_page_supported(int fd, int pg); + + /* + * discovery bitmask +diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c +index d362beb4..d7febec6 100644 +--- a/libmultipath/propsel.c ++++ b/libmultipath/propsel.c +@@ -496,13 +496,15 @@ check_rdac(struct path * pp) + { + int len; + char buff[44]; +- const char *checker_name; ++ const char *checker_name = NULL; + + if (pp->bus != SYSFS_BUS_SCSI) + return 0; +- /* Avoid ioctl if this is likely not an RDAC array */ +- if (__do_set_from_hwe(checker_name, pp, checker_name) && +- strcmp(checker_name, RDAC)) ++ /* Avoid checking 0xc9 if this is likely not an RDAC array */ ++ if (!__do_set_from_hwe(checker_name, pp, checker_name) && ++ !is_vpd_page_supported(pp->fd, 0xC9)) ++ return 0; ++ if (checker_name && strcmp(checker_name, RDAC)) + return 0; + len = get_vpd_sgio(pp->fd, 0xC9, 0, buff, 44); + if (len <= 0) +-- +2.17.2 + diff --git a/SOURCES/0041-libmultipath-add-device-to-hwtable.c.patch b/SOURCES/0041-libmultipath-add-device-to-hwtable.c.patch new file mode 100644 index 0000000..9d5e780 --- /dev/null +++ b/SOURCES/0041-libmultipath-add-device-to-hwtable.c.patch @@ -0,0 +1,44 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Steve Schremmer +Date: Mon, 6 Jul 2020 20:22:35 +0000 +Subject: [PATCH] libmultipath: add device to hwtable.c + +Add FUJITSU ETERNUS_AHB defaults. + +Signed-off-by: Steve Schremmer +Reviewed-by: Martin Wilck +Signed-off-by: Benjamin Marzinski +--- + libmultipath/hwtable.c | 16 ++++++++++++++++ + 1 file changed, 16 insertions(+) + +diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c +index d1fcfdb3..d680bdfc 100644 +--- a/libmultipath/hwtable.c ++++ b/libmultipath/hwtable.c +@@ -428,6 +428,22 @@ static struct hwentry default_hw[] = { + .pgpolicy = MULTIBUS, + .no_path_retry = 10, + }, ++ { ++ /* ++ * ETERNUS AB/HB ++ * Maintainer: NetApp RDAC team ++ */ ++ .vendor = "FUJITSU", ++ .product = "ETERNUS_AHB", ++ .bl_product = "Universal Xport", ++ .pgpolicy = GROUP_BY_PRIO, ++ .checker_name = RDAC, ++ .features = "2 pg_init_retries 50", ++ .hwhandler = "1 rdac", ++ .prio_name = PRIO_RDAC, ++ .pgfailback = -FAILBACK_IMMEDIATE, ++ .no_path_retry = 30, ++ }, + /* + * Hitachi Vantara + * +-- +2.17.2 + diff --git a/SOURCES/0042-libmultipath-move-fast_io_fail-defines-to-structs.h.patch b/SOURCES/0042-libmultipath-move-fast_io_fail-defines-to-structs.h.patch new file mode 100644 index 0000000..0cd6997 --- /dev/null +++ b/SOURCES/0042-libmultipath-move-fast_io_fail-defines-to-structs.h.patch @@ -0,0 +1,159 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 12 Oct 2020 16:06:11 -0500 +Subject: [PATCH] libmultipath: move fast_io_fail defines to structs.h + +Since fast_io_fail is part of the multipath struct, its symbolic values +belong in structs.h. Also, make it an instance of a general enum, which +will be used again in future patches, and change the set/print functions +which use it to use the general enum instead. + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/config.h | 8 -------- + libmultipath/dict.c | 30 +++++++++++++++--------------- + libmultipath/dict.h | 2 +- + libmultipath/propsel.c | 2 +- + libmultipath/structs.h | 17 +++++++++++++++++ + 5 files changed, 34 insertions(+), 25 deletions(-) + +diff --git a/libmultipath/config.h b/libmultipath/config.h +index 160867cd..f38c7639 100644 +--- a/libmultipath/config.h ++++ b/libmultipath/config.h +@@ -11,14 +11,6 @@ + #define ORIGIN_CONFIG 1 + #define ORIGIN_NO_CONFIG 2 + +-/* +- * In kernel, fast_io_fail == 0 means immediate failure on rport delete. +- * OTOH '0' means not-configured in various places in multipath-tools. +- */ +-#define MP_FAST_IO_FAIL_UNSET (0) +-#define MP_FAST_IO_FAIL_OFF (-1) +-#define MP_FAST_IO_FAIL_ZERO (-2) +- + enum devtypes { + DEV_NONE, + DEV_DEVT, +diff --git a/libmultipath/dict.c b/libmultipath/dict.c +index 184d4b22..ce8e1cda 100644 +--- a/libmultipath/dict.c ++++ b/libmultipath/dict.c +@@ -834,7 +834,7 @@ declare_mp_attr_handler(gid, set_gid) + declare_mp_attr_snprint(gid, print_gid) + + static int +-set_fast_io_fail(vector strvec, void *ptr) ++set_undef_off_zero(vector strvec, void *ptr) + { + char * buff; + int *int_ptr = (int *)ptr; +@@ -844,36 +844,36 @@ set_fast_io_fail(vector strvec, void *ptr) + return 1; + + if (strcmp(buff, "off") == 0) +- *int_ptr = MP_FAST_IO_FAIL_OFF; ++ *int_ptr = UOZ_OFF; + else if (sscanf(buff, "%d", int_ptr) != 1 || +- *int_ptr < MP_FAST_IO_FAIL_ZERO) +- *int_ptr = MP_FAST_IO_FAIL_UNSET; ++ *int_ptr < UOZ_ZERO) ++ *int_ptr = UOZ_UNDEF; + else if (*int_ptr == 0) +- *int_ptr = MP_FAST_IO_FAIL_ZERO; ++ *int_ptr = UOZ_ZERO; + + FREE(buff); + return 0; + } + + int +-print_fast_io_fail(char * buff, int len, long v) ++print_undef_off_zero(char * buff, int len, long v) + { +- if (v == MP_FAST_IO_FAIL_UNSET) ++ if (v == UOZ_UNDEF) + return 0; +- if (v == MP_FAST_IO_FAIL_OFF) ++ if (v == UOZ_OFF) + return snprintf(buff, len, "\"off\""); +- if (v == MP_FAST_IO_FAIL_ZERO) ++ if (v == UOZ_ZERO) + return snprintf(buff, len, "0"); + return snprintf(buff, len, "%ld", v); + } + +-declare_def_handler(fast_io_fail, set_fast_io_fail) +-declare_def_snprint_defint(fast_io_fail, print_fast_io_fail, ++declare_def_handler(fast_io_fail, set_undef_off_zero) ++declare_def_snprint_defint(fast_io_fail, print_undef_off_zero, + DEFAULT_FAST_IO_FAIL) +-declare_ovr_handler(fast_io_fail, set_fast_io_fail) +-declare_ovr_snprint(fast_io_fail, print_fast_io_fail) +-declare_hw_handler(fast_io_fail, set_fast_io_fail) +-declare_hw_snprint(fast_io_fail, print_fast_io_fail) ++declare_ovr_handler(fast_io_fail, set_undef_off_zero) ++declare_ovr_snprint(fast_io_fail, print_undef_off_zero) ++declare_hw_handler(fast_io_fail, set_undef_off_zero) ++declare_hw_snprint(fast_io_fail, print_undef_off_zero) + + static int + set_dev_loss(vector strvec, void *ptr) +diff --git a/libmultipath/dict.h b/libmultipath/dict.h +index a40ac66f..a917e1ca 100644 +--- a/libmultipath/dict.h ++++ b/libmultipath/dict.h +@@ -13,7 +13,7 @@ int print_rr_weight(char *buff, int len, long v); + int print_pgfailback(char *buff, int len, long v); + int print_pgpolicy(char *buff, int len, long v); + int print_no_path_retry(char *buff, int len, long v); +-int print_fast_io_fail(char *buff, int len, long v); ++int print_undef_off_zero(char *buff, int len, long v); + int print_dev_loss(char *buff, int len, unsigned long v); + int print_reservation_key(char * buff, int len, struct be64 key, uint8_t + flags, int source); +diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c +index d7febec6..725db2b1 100644 +--- a/libmultipath/propsel.c ++++ b/libmultipath/propsel.c +@@ -755,7 +755,7 @@ int select_fast_io_fail(struct config *conf, struct multipath *mp) + mp_set_conf(fast_io_fail); + mp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL); + out: +- print_fast_io_fail(buff, 12, mp->fast_io_fail); ++ print_undef_off_zero(buff, 12, mp->fast_io_fail); + condlog(3, "%s: fast_io_fail_tmo = %s %s", mp->alias, buff, origin); + return 0; + } +diff --git a/libmultipath/structs.h b/libmultipath/structs.h +index 8e78b8c0..29209984 100644 +--- a/libmultipath/structs.h ++++ b/libmultipath/structs.h +@@ -229,6 +229,23 @@ enum vpd_vendor_ids { + VPD_VP_ARRAY_SIZE, /* This must remain the last entry */ + }; + ++/* ++ * Multipath treats 0 as undefined for optional config parameters. ++ * Use this for cases where 0 is a valid option for systems multipath ++ * is communicating with ++ */ ++enum undefined_off_zero { ++ UOZ_UNDEF = 0, ++ UOZ_OFF = -1, ++ UOZ_ZERO = -2, ++}; ++ ++enum fast_io_fail_states { ++ MP_FAST_IO_FAIL_UNSET = UOZ_UNDEF, ++ MP_FAST_IO_FAIL_OFF = UOZ_OFF, ++ MP_FAST_IO_FAIL_ZERO = UOZ_ZERO, ++}; ++ + struct vpd_vendor_page { + int pg; + const char *name; +-- +2.17.2 + diff --git a/SOURCES/0043-libmultipath-add-eh_deadline-multipath.conf-paramete.patch b/SOURCES/0043-libmultipath-add-eh_deadline-multipath.conf-paramete.patch new file mode 100644 index 0000000..a8ebb2c --- /dev/null +++ b/SOURCES/0043-libmultipath-add-eh_deadline-multipath.conf-paramete.patch @@ -0,0 +1,295 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Wed, 14 Oct 2020 18:38:20 -0500 +Subject: [PATCH] libmultipath: add eh_deadline multipath.conf parameter + +There are times a fc rport is never lost, meaning that fast_io_fail_tmo +and dev_loss_tmo never trigger, but scsi commands still hang. This can +cause problems in cases where users have string timing requirements, and +the easiest way to solve these issues is to set eh_deadline. Since it's +already possible to set fast_io_fail_tmo and dev_loss_tmo from +multipath.conf, and have multipath take care of setting it correctly for +the scsi devices in sysfs, it makes sense to allow users to set +eh_deadline here as well. + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/config.c | 2 ++ + libmultipath/config.h | 2 ++ + libmultipath/configure.c | 1 + + libmultipath/dict.c | 10 +++++++ + libmultipath/discovery.c | 58 +++++++++++++++++++++++++++++++++----- + libmultipath/propsel.c | 17 +++++++++++ + libmultipath/propsel.h | 1 + + libmultipath/structs.h | 7 +++++ + multipath/multipath.conf.5 | 16 +++++++++++ + 9 files changed, 107 insertions(+), 7 deletions(-) + +diff --git a/libmultipath/config.c b/libmultipath/config.c +index 26f8e050..a71db2d0 100644 +--- a/libmultipath/config.c ++++ b/libmultipath/config.c +@@ -359,6 +359,7 @@ merge_hwe (struct hwentry * dst, struct hwentry * src) + merge_num(flush_on_last_del); + merge_num(fast_io_fail); + merge_num(dev_loss); ++ merge_num(eh_deadline); + merge_num(user_friendly_names); + merge_num(retain_hwhandler); + merge_num(detect_prio); +@@ -514,6 +515,7 @@ store_hwe (vector hwtable, struct hwentry * dhwe) + hwe->flush_on_last_del = dhwe->flush_on_last_del; + hwe->fast_io_fail = dhwe->fast_io_fail; + hwe->dev_loss = dhwe->dev_loss; ++ hwe->eh_deadline = dhwe->eh_deadline; + hwe->user_friendly_names = dhwe->user_friendly_names; + hwe->retain_hwhandler = dhwe->retain_hwhandler; + hwe->detect_prio = dhwe->detect_prio; +diff --git a/libmultipath/config.h b/libmultipath/config.h +index f38c7639..a22c1b4e 100644 +--- a/libmultipath/config.h ++++ b/libmultipath/config.h +@@ -64,6 +64,7 @@ struct hwentry { + int flush_on_last_del; + int fast_io_fail; + unsigned int dev_loss; ++ int eh_deadline; + int user_friendly_names; + int retain_hwhandler; + int detect_prio; +@@ -149,6 +150,7 @@ struct config { + int attribute_flags; + int fast_io_fail; + unsigned int dev_loss; ++ int eh_deadline; + int log_checker_err; + int allow_queueing; + int find_multipaths; +diff --git a/libmultipath/configure.c b/libmultipath/configure.c +index 96c79610..b7113291 100644 +--- a/libmultipath/configure.c ++++ b/libmultipath/configure.c +@@ -340,6 +340,7 @@ int setup_map(struct multipath *mpp, char *params, int params_size, + select_gid(conf, mpp); + select_fast_io_fail(conf, mpp); + select_dev_loss(conf, mpp); ++ select_eh_deadline(conf, mpp); + select_reservation_key(conf, mpp); + select_deferred_remove(conf, mpp); + select_marginal_path_err_sample_time(conf, mpp); +diff --git a/libmultipath/dict.c b/libmultipath/dict.c +index ce8e1cda..8fd91d8c 100644 +--- a/libmultipath/dict.c ++++ b/libmultipath/dict.c +@@ -911,6 +911,13 @@ declare_ovr_snprint(dev_loss, print_dev_loss) + declare_hw_handler(dev_loss, set_dev_loss) + declare_hw_snprint(dev_loss, print_dev_loss) + ++declare_def_handler(eh_deadline, set_undef_off_zero) ++declare_def_snprint(eh_deadline, print_undef_off_zero) ++declare_ovr_handler(eh_deadline, set_undef_off_zero) ++declare_ovr_snprint(eh_deadline, print_undef_off_zero) ++declare_hw_handler(eh_deadline, set_undef_off_zero) ++declare_hw_snprint(eh_deadline, print_undef_off_zero) ++ + static int + set_pgpolicy(vector strvec, void *ptr) + { +@@ -1776,6 +1783,7 @@ init_keywords(vector keywords) + install_keyword("gid", &def_gid_handler, &snprint_def_gid); + install_keyword("fast_io_fail_tmo", &def_fast_io_fail_handler, &snprint_def_fast_io_fail); + install_keyword("dev_loss_tmo", &def_dev_loss_handler, &snprint_def_dev_loss); ++ install_keyword("eh_deadline", &def_eh_deadline_handler, &snprint_def_eh_deadline); + install_keyword("bindings_file", &def_bindings_file_handler, &snprint_def_bindings_file); + install_keyword("wwids_file", &def_wwids_file_handler, &snprint_def_wwids_file); + install_keyword("prkeys_file", &def_prkeys_file_handler, &snprint_def_prkeys_file); +@@ -1885,6 +1893,7 @@ init_keywords(vector keywords) + install_keyword("flush_on_last_del", &hw_flush_on_last_del_handler, &snprint_hw_flush_on_last_del); + install_keyword("fast_io_fail_tmo", &hw_fast_io_fail_handler, &snprint_hw_fast_io_fail); + install_keyword("dev_loss_tmo", &hw_dev_loss_handler, &snprint_hw_dev_loss); ++ install_keyword("eh_deadline", &hw_eh_deadline_handler, &snprint_hw_eh_deadline); + install_keyword("user_friendly_names", &hw_user_friendly_names_handler, &snprint_hw_user_friendly_names); + install_keyword("retain_attached_hw_handler", &hw_retain_hwhandler_handler, &snprint_hw_retain_hwhandler); + install_keyword("detect_prio", &hw_detect_prio_handler, &snprint_hw_detect_prio); +@@ -1925,6 +1934,7 @@ init_keywords(vector keywords) + install_keyword("flush_on_last_del", &ovr_flush_on_last_del_handler, &snprint_ovr_flush_on_last_del); + install_keyword("fast_io_fail_tmo", &ovr_fast_io_fail_handler, &snprint_ovr_fast_io_fail); + install_keyword("dev_loss_tmo", &ovr_dev_loss_handler, &snprint_ovr_dev_loss); ++ install_keyword("eh_deadline", &ovr_eh_deadline_handler, &snprint_ovr_eh_deadline); + install_keyword("user_friendly_names", &ovr_user_friendly_names_handler, &snprint_ovr_user_friendly_names); + install_keyword("retain_attached_hw_handler", &ovr_retain_hwhandler_handler, &snprint_ovr_retain_hwhandler); + install_keyword("detect_prio", &ovr_detect_prio_handler, &snprint_ovr_detect_prio); +diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c +index 01aadba9..a328aafa 100644 +--- a/libmultipath/discovery.c ++++ b/libmultipath/discovery.c +@@ -577,6 +577,42 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen) + return !!preferred; + } + ++static int ++sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp) ++{ ++ struct udev_device *hostdev; ++ char host_name[HOST_NAME_LEN], value[16]; ++ int ret; ++ ++ if (mpp->eh_deadline == EH_DEADLINE_UNSET) ++ return 0; ++ ++ sprintf(host_name, "host%d", pp->sg_id.host_no); ++ hostdev = udev_device_new_from_subsystem_sysname(udev, ++ "scsi_host", host_name); ++ if (!hostdev) ++ return 1; ++ ++ if (mpp->eh_deadline == EH_DEADLINE_OFF) ++ sprintf(value, "off"); ++ else if (mpp->eh_deadline == EH_DEADLINE_ZERO) ++ sprintf(value, "0"); ++ else ++ snprintf(value, 16, "%u", mpp->eh_deadline); ++ ++ ret = sysfs_attr_set_value(hostdev, "eh_deadline", ++ value, strlen(value)); ++ /* ++ * not all scsi drivers support setting eh_deadline, so failing ++ * is totally reasonable ++ */ ++ if (ret <= 0) ++ condlog(4, "%s: failed to set eh_deadline to %s, error %d", udev_device_get_sysname(hostdev), value, -ret); ++ ++ udev_device_unref(hostdev); ++ return (ret <= 0); ++} ++ + static void + sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp) + { +@@ -787,16 +823,24 @@ sysfs_set_scsi_tmo (struct multipath *mpp, unsigned int checkint) + mpp->alias, mpp->fast_io_fail); + mpp->fast_io_fail = MP_FAST_IO_FAIL_OFF; + } +- if (!mpp->dev_loss && mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET) ++ if (!mpp->dev_loss && mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET && ++ mpp->eh_deadline == EH_DEADLINE_UNSET) + return 0; + + vector_foreach_slot(mpp->paths, pp, i) { +- if (pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) +- sysfs_set_rport_tmo(mpp, pp); +- if (pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI) +- sysfs_set_session_tmo(mpp, pp); +- if (pp->sg_id.proto_id == SCSI_PROTOCOL_SAS) +- sysfs_set_nexus_loss_tmo(mpp, pp); ++ if (pp->bus != SYSFS_BUS_SCSI) ++ continue; ++ ++ if (mpp->dev_loss || ++ mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) { ++ if (pp->sg_id.proto_id == SCSI_PROTOCOL_FCP) ++ sysfs_set_rport_tmo(mpp, pp); ++ else if (pp->sg_id.proto_id == SCSI_PROTOCOL_ISCSI) ++ sysfs_set_session_tmo(mpp, pp); ++ else if (pp->sg_id.proto_id == SCSI_PROTOCOL_SAS) ++ sysfs_set_nexus_loss_tmo(mpp, pp); ++ } ++ sysfs_set_eh_deadline(mpp, pp); + } + return 0; + } +diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c +index 725db2b1..1150cfe8 100644 +--- a/libmultipath/propsel.c ++++ b/libmultipath/propsel.c +@@ -776,6 +776,23 @@ out: + return 0; + } + ++int select_eh_deadline(struct config *conf, struct multipath *mp) ++{ ++ const char *origin; ++ char buff[12]; ++ ++ mp_set_ovr(eh_deadline); ++ mp_set_hwe(eh_deadline); ++ mp_set_conf(eh_deadline); ++ mp->eh_deadline = EH_DEADLINE_UNSET; ++ /* not changing sysfs in default cause, so don't print anything */ ++ return 0; ++out: ++ print_undef_off_zero(buff, 12, mp->eh_deadline); ++ condlog(3, "%s: eh_deadline = %s %s", mp->alias, buff, origin); ++ return 0; ++} ++ + int select_flush_on_last_del(struct config *conf, struct multipath *mp) + { + const char *origin; +diff --git a/libmultipath/propsel.h b/libmultipath/propsel.h +index 3d6edd8a..a68bacf0 100644 +--- a/libmultipath/propsel.h ++++ b/libmultipath/propsel.h +@@ -17,6 +17,7 @@ int select_uid(struct config *conf, struct multipath *mp); + int select_gid(struct config *conf, struct multipath *mp); + int select_fast_io_fail(struct config *conf, struct multipath *mp); + int select_dev_loss(struct config *conf, struct multipath *mp); ++int select_eh_deadline(struct config *conf, struct multipath *mp); + int select_reservation_key(struct config *conf, struct multipath *mp); + int select_retain_hwhandler (struct config *conf, struct multipath * mp); + int select_detect_prio(struct config *conf, struct path * pp); +diff --git a/libmultipath/structs.h b/libmultipath/structs.h +index 29209984..65542dea 100644 +--- a/libmultipath/structs.h ++++ b/libmultipath/structs.h +@@ -246,6 +246,12 @@ enum fast_io_fail_states { + MP_FAST_IO_FAIL_ZERO = UOZ_ZERO, + }; + ++enum eh_deadline_states { ++ EH_DEADLINE_UNSET = UOZ_UNDEF, ++ EH_DEADLINE_OFF = UOZ_OFF, ++ EH_DEADLINE_ZERO = UOZ_ZERO, ++}; ++ + struct vpd_vendor_page { + int pg; + const char *name; +@@ -366,6 +372,7 @@ struct multipath { + int ghost_delay; + int ghost_delay_tick; + unsigned int dev_loss; ++ int eh_deadline; + uid_t uid; + gid_t gid; + mode_t mode; +diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 +index 6dc26f10..60954574 100644 +--- a/multipath/multipath.conf.5 ++++ b/multipath/multipath.conf.5 +@@ -700,6 +700,22 @@ The default is: \fB600\fR + . + . + .TP ++.B eh_deadline ++Specify the maximum number of seconds the SCSI layer will spend doing error ++handling when scsi devices fail. After this timeout the scsi layer will perform ++a full HBA reset. Setting this may be necessary in cases where the rport is ++never lost, so \fIfast_io_fail_tmo\fR and \fIdev_loss_tmo\fR will never ++trigger, but (frequently do to load) scsi commands still hang. \fBNote:\fR when ++the scsi error handler performs the HBA reset, all target paths on that HBA ++will be affected. eh_deadline should only be set in cases where all targets on ++the affected HBAs are multipathed. ++.RS ++.TP ++The default is: \fB\fR ++.RE ++. ++. ++.TP + .B bindings_file + The full pathname of the binding file to be used when the user_friendly_names + option is set. +-- +2.17.2 + diff --git a/SOURCES/0044-libmultipath-don-t-dlclose-tur-checker-DSO.patch b/SOURCES/0044-libmultipath-don-t-dlclose-tur-checker-DSO.patch new file mode 100644 index 0000000..18ed578 --- /dev/null +++ b/SOURCES/0044-libmultipath-don-t-dlclose-tur-checker-DSO.patch @@ -0,0 +1,89 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Fri, 23 Oct 2020 11:38:24 -0500 +Subject: [PATCH] libmultipath: don't dlclose tur checker DSO + +The multipathd tur checker thread is designed to be able to finish at +any time, even after the tur checker itself has been freed. The +multipathd shutdown code makes sure all the checkers have been freed +before freeing the checker_class and calling dlclose() to unload the +DSO, but this doesn't guarantee that the checker threads have finished. +If one hasn't, the DSO will get unloaded while the thread still running +code from it, causing a segfault. Unfortunately, it's not possible to be +sure that all tur checker threads have ended during shutdown, without +making them joinable. + +However, since libmultipath will never be reinitialized after it has +been uninitialzed, not dlclosing the tur checker DSO once a thread is +started has minimal cost (keeping the DSO code around until the program +exits, which usually happens right after freeing the checkers). + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/checkers.c | 10 +++++++++- + libmultipath/checkers.h | 1 + + libmultipath/checkers/tur.c | 1 + + 3 files changed, 11 insertions(+), 1 deletion(-) + +diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c +index 8d2be8a9..6359e5d8 100644 +--- a/libmultipath/checkers.c ++++ b/libmultipath/checkers.c +@@ -21,6 +21,7 @@ struct checker_class { + void (*reset)(void); /* to reset the global variables */ + const char **msgtable; + short msgtable_size; ++ int keep_dso; + }; + + char *checker_state_names[] = { +@@ -69,7 +70,7 @@ void free_checker_class(struct checker_class *c) + list_del(&c->node); + if (c->reset) + c->reset(); +- if (c->handle) { ++ if (c->handle && !c->keep_dso) { + if (dlclose(c->handle) != 0) { + condlog(0, "Cannot unload checker %s: %s", + c->name, dlerror()); +@@ -192,6 +193,13 @@ out: + return NULL; + } + ++void checker_keep_dso(struct checker * c) ++{ ++ if (!c || !c->cls) ++ return; ++ c->cls->keep_dso = 1; ++} ++ + void checker_set_fd (struct checker * c, int fd) + { + if (!c) +diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h +index b458118d..f183cff9 100644 +--- a/libmultipath/checkers.h ++++ b/libmultipath/checkers.h +@@ -145,6 +145,7 @@ void checker_reset (struct checker *); + void checker_set_sync (struct checker *); + void checker_set_async (struct checker *); + void checker_set_fd (struct checker *, int); ++void checker_keep_dso(struct checker *c); + void checker_enable (struct checker *); + void checker_disable (struct checker *); + int checker_check (struct checker *, int); +diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c +index e886fcf8..fd58d62a 100644 +--- a/libmultipath/checkers/tur.c ++++ b/libmultipath/checkers/tur.c +@@ -394,6 +394,7 @@ int libcheck_check(struct checker * c) + uatomic_set(&ct->running, 1); + tur_set_async_timeout(c); + setup_thread_attr(&attr, 32 * 1024, 1); ++ checker_keep_dso(c); + r = pthread_create(&ct->thread, &attr, tur_thread, ct); + pthread_attr_destroy(&attr); + if (r) { +-- +2.17.2 + diff --git a/SOURCES/0045-libmultipath-warn-about-missing-braces-at-end-of-mul.patch b/SOURCES/0045-libmultipath-warn-about-missing-braces-at-end-of-mul.patch new file mode 100644 index 0000000..f8f45f8 --- /dev/null +++ b/SOURCES/0045-libmultipath-warn-about-missing-braces-at-end-of-mul.patch @@ -0,0 +1,41 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 16 Nov 2020 13:52:09 -0600 +Subject: [PATCH] libmultipath: warn about missing braces at end of + multipath.conf + +Multipath doesn't warn when multipath.conf is missing closing braces at +the end of the file. This has confused people about the correct config +file syntax, so add a warning. + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/parser.c | 5 +++-- + 1 file changed, 3 insertions(+), 2 deletions(-) + +diff --git a/libmultipath/parser.c b/libmultipath/parser.c +index a7285a35..48b54e87 100644 +--- a/libmultipath/parser.c ++++ b/libmultipath/parser.c +@@ -544,7 +544,7 @@ process_stream(struct config *conf, FILE *stream, vector keywords, char *file) + if (!strcmp(str, EOB)) { + if (kw_level > 0) { + free_strvec(strvec); +- break; ++ goto out; + } + condlog(0, "unmatched '%s' at line %d of %s", + EOB, line_nr, file); +@@ -583,7 +583,8 @@ process_stream(struct config *conf, FILE *stream, vector keywords, char *file) + + free_strvec(strvec); + } +- ++ if (kw_level == 1) ++ condlog(1, "missing '%s' at end of %s", EOB, file); + out: + FREE(buf); + free_uniques(uniques); +-- +2.17.2 + diff --git a/SOURCES/0046-libmultipath-ignore-multipaths-sections-without-wwid.patch b/SOURCES/0046-libmultipath-ignore-multipaths-sections-without-wwid.patch new file mode 100644 index 0000000..3ccf175 --- /dev/null +++ b/SOURCES/0046-libmultipath-ignore-multipaths-sections-without-wwid.patch @@ -0,0 +1,36 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 16 Nov 2020 16:38:21 -0600 +Subject: [PATCH] libmultipath: ignore multipaths sections without wwid option + +"multipathd show config local" was crashing in find_mp_by_wwid() if +the multipath configuration included a multipaths section that did +not set a wwid option. There is no reason to keep a mpentry that +didn't set its wwid. Remove it in merge_mptable(). + +Signed-off-by: Benjamin Marzinski +--- + libmultipath/config.c | 7 +++++++ + 1 file changed, 7 insertions(+) + +diff --git a/libmultipath/config.c b/libmultipath/config.c +index a71db2d0..dc81c994 100644 +--- a/libmultipath/config.c ++++ b/libmultipath/config.c +@@ -444,6 +444,13 @@ void merge_mptable(vector mptable) + int i, j; + + vector_foreach_slot(mptable, mp1, i) { ++ /* drop invalid multipath configs */ ++ if (!mp1->wwid) { ++ condlog(0, "multipaths config section missing wwid"); ++ vector_del_slot(mptable, i--); ++ free_mpe(mp1); ++ continue; ++ } + j = i + 1; + vector_foreach_slot_after(mptable, mp2, j) { + if (strcmp(mp1->wwid, mp2->wwid)) +-- +2.17.2 + diff --git a/SOURCES/0047-tests-fix-missing-priority-blacklist-test.patch b/SOURCES/0047-tests-fix-missing-priority-blacklist-test.patch new file mode 100644 index 0000000..a73ffe6 --- /dev/null +++ b/SOURCES/0047-tests-fix-missing-priority-blacklist-test.patch @@ -0,0 +1,25 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 16 Nov 2020 23:21:41 -0600 +Subject: [PATCH] tests: fix missing priority blacklist test + +Signed-off-by: Benjamin Marzinski +--- + tests/blacklist.c | 1 - + 1 file changed, 1 deletion(-) + +diff --git a/tests/blacklist.c b/tests/blacklist.c +index d20e97af..401600d9 100644 +--- a/tests/blacklist.c ++++ b/tests/blacklist.c +@@ -378,7 +378,6 @@ static void test_property_missing(void **state) + { + static struct udev_device udev = { "sdb", { "ID_FOO", "ID_BAZ", "ID_BAR", "ID_SERIAL", NULL } }; + conf.blist_property = blist_property_wwn; +- expect_condlog(3, "sdb: blacklisted, udev property missing\n"); + assert_int_equal(filter_property(&conf, &udev, 3, "ID_SERIAL"), + MATCH_NOTHING); + assert_int_equal(filter_property(&conf, &udev, 3, "ID_BLAH"), +-- +2.17.2 + diff --git a/SOURCES/0048-mpathpersist-Fix-Register-and-Ignore-with-0x00-SARK.patch b/SOURCES/0048-mpathpersist-Fix-Register-and-Ignore-with-0x00-SARK.patch new file mode 100644 index 0000000..fa9d703 --- /dev/null +++ b/SOURCES/0048-mpathpersist-Fix-Register-and-Ignore-with-0x00-SARK.patch @@ -0,0 +1,33 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 5 Nov 2020 09:15:43 -0600 +Subject: [PATCH] mpathpersist: Fix Register and Ignore with 0x00 SARK + +When the Register and Ignore command is run with sg_persist, if a 0x00 +Service Action Reservation Key is given or the --param-sark option is +not used at all, sg_persist will clear the registration. mpathpersist +will fail with an error. This patch fixes mpathpersist to work like +sg_persist in this case. + +Signed-off-by: Benjamin Marzinski +--- + libmpathpersist/mpath_persist.c | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c +index 3da7a6cf..aa196008 100644 +--- a/libmpathpersist/mpath_persist.c ++++ b/libmpathpersist/mpath_persist.c +@@ -321,7 +321,8 @@ int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, + } + + if (memcmp(paramp->key, &mpp->reservation_key, 8) && +- memcmp(paramp->sa_key, &mpp->reservation_key, 8)) { ++ memcmp(paramp->sa_key, &mpp->reservation_key, 8) && ++ (prkey || rq_servact != MPATH_PROUT_REG_IGN_SA)) { + condlog(0, "%s: configured reservation key doesn't match: 0x%" PRIx64, alias, get_be64(mpp->reservation_key)); + ret = MPATH_PR_SYNTAX_ERROR; + goto out1; +-- +2.17.2 + diff --git a/SOURCES/0049-mpathpersist-update-prkeys-file-on-changing-registra.patch b/SOURCES/0049-mpathpersist-update-prkeys-file-on-changing-registra.patch new file mode 100644 index 0000000..0cd0ab2 --- /dev/null +++ b/SOURCES/0049-mpathpersist-update-prkeys-file-on-changing-registra.patch @@ -0,0 +1,38 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 23 Nov 2020 20:45:50 -0600 +Subject: [PATCH] mpathpersist: update prkeys file on changing registrations + +When the "reservation_key" option is set to "file" and Register command +is run with both the current Reservation Key and a new Service Action +Reservation Key, mpathpersist will change the registration, but will not +update the prkeys file. This means that future paths that come online +will not be able to register, since multipathd is still using the old +reservation key. Fix this. + +Signed-off-by: Benjamin Marzinski +--- + libmpathpersist/mpath_persist.c | 7 ++++--- + 1 file changed, 4 insertions(+), 3 deletions(-) + +diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c +index aa196008..a01dfb0b 100644 +--- a/libmpathpersist/mpath_persist.c ++++ b/libmpathpersist/mpath_persist.c +@@ -307,9 +307,10 @@ int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope, + + memcpy(&prkey, paramp->sa_key, 8); + if (mpp->prkey_source == PRKEY_SOURCE_FILE && prkey && +- ((!get_be64(mpp->reservation_key) && +- rq_servact == MPATH_PROUT_REG_SA) || +- rq_servact == MPATH_PROUT_REG_IGN_SA)) { ++ (rq_servact == MPATH_PROUT_REG_IGN_SA || ++ (rq_servact == MPATH_PROUT_REG_SA && ++ (!get_be64(mpp->reservation_key) || ++ memcmp(paramp->key, &mpp->reservation_key, 8) == 0)))) { + memcpy(&mpp->reservation_key, paramp->sa_key, 8); + if (update_prkey_flags(alias, get_be64(mpp->reservation_key), + paramp->sa_flags)) { +-- +2.17.2 + diff --git a/SOURCES/0050-multipathd-Fix-multipathd-stopping-on-shutdown.patch b/SOURCES/0050-multipathd-Fix-multipathd-stopping-on-shutdown.patch new file mode 100644 index 0000000..a9f8969 --- /dev/null +++ b/SOURCES/0050-multipathd-Fix-multipathd-stopping-on-shutdown.patch @@ -0,0 +1,37 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 14 Dec 2020 14:16:29 -0600 +Subject: [PATCH] multipathd: Fix multipathd stopping on shutdown + +According to man "systemd.special" + +"shutdown.target: ... Services that shall be terminated on system +shutdown shall add Conflicts= and Before= dependencies to this unit for +their service unit, which is implicitly done when +DefaultDependencies=yes is set (the default)." + +multipathd.service sets DefaultDependencies=no and includes the +Conflits= dependency, but not the Before= one. This can cause multipathd +to continue running past when it is supposed to during shutdown. + +Signed-off-by: Benjamin Marzinski +--- + multipathd/multipathd.service | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/multipathd/multipathd.service b/multipathd/multipathd.service +index 0fbcc46b..bc8fa07a 100644 +--- a/multipathd/multipathd.service ++++ b/multipathd/multipathd.service +@@ -2,7 +2,7 @@ + Description=Device-Mapper Multipath Device Controller + Wants=systemd-udev-trigger.service systemd-udev-settle.service + Before=iscsi.service iscsid.service lvm2-activation-early.service +-Before=local-fs-pre.target blk-availability.service ++Before=local-fs-pre.target blk-availability.service shutdown.target + After=multipathd.socket systemd-udev-trigger.service systemd-udev-settle.service + ConditionPathExists=/etc/multipath.conf + DefaultDependencies=no +-- +2.17.2 + diff --git a/SOURCES/0051-multipath.conf.5-Improve-checker_timeout-description.patch b/SOURCES/0051-multipath.conf.5-Improve-checker_timeout-description.patch new file mode 100644 index 0000000..1e1f249 --- /dev/null +++ b/SOURCES/0051-multipath.conf.5-Improve-checker_timeout-description.patch @@ -0,0 +1,36 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Tue, 15 Dec 2020 12:47:14 -0600 +Subject: [PATCH] multipath.conf.5: Improve checker_timeout description + +I was asked to explain how checker_timeout works for checkers like +directio, that don't issue scsi commands with an explicit timeout + +Signed-off-by: Benjamin Marzinski +--- + multipath/multipath.conf.5 | 9 +++++++-- + 1 file changed, 7 insertions(+), 2 deletions(-) + +diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 +index 60954574..a5686090 100644 +--- a/multipath/multipath.conf.5 ++++ b/multipath/multipath.conf.5 +@@ -634,8 +634,13 @@ The default is: \fBno\fR + . + .TP + .B checker_timeout +-Specify the timeout to use for path checkers and prioritizers that issue SCSI +-commands with an explicit timeout, in seconds. ++Specify the timeout to use for path checkers and prioritizers, in seconds. ++Only prioritizers that issue scsi commands use checker_timeout. Checkers ++that support an asynchronous mode (\fItur\fR and \fIdirectio\fR), will ++return shortly after being called by multipathd, regardless of whether the ++storage array responds. If the storage array hasn't responded, mulitpathd will ++check for a response every second, until \fIchecker_timeout\fR seconds have ++elapsed. + .RS + .TP + The default is: in \fB/sys/block/sd/device/timeout\fR +-- +2.17.2 + 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/SOURCES/0060-kpartx-free-loop-device-after-listing-partitions.patch b/SOURCES/0060-kpartx-free-loop-device-after-listing-partitions.patch new file mode 100644 index 0000000..c50bbc0 --- /dev/null +++ b/SOURCES/0060-kpartx-free-loop-device-after-listing-partitions.patch @@ -0,0 +1,61 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Tue, 9 Feb 2021 17:16:04 -0600 +Subject: [PATCH] kpartx: free loop device after listing partitions + +If "kpartx -l" is run on a file that doesn't already have a loop device +associated with it, it will create a loop device to run the command. +Starting with da59d15c6 ("Fix loopback file with kpartx -av"), it will +not free the loop device when exitting. This is because it checks if the +the file it stat()ed is a regular file, before freeing the loop device. +However, after da59d15c6, stat() is rerun on the loop device itself, so +the check fails. There is no need to check this, if loopcreated is +true, then the file will be a kpartx created loop device, and should be +freed. + +Also, keep kpartx from printing that the loop device has been removed +at normal verbosity. + +Fixes: da59d15c6 ("Fix loopback file with kpartx -av") +Signed-off-by: Benjamin Marzinski +--- + kpartx/kpartx.c | 9 +++++---- + 1 file changed, 5 insertions(+), 4 deletions(-) + +diff --git a/kpartx/kpartx.c b/kpartx/kpartx.c +index 653ce0c8..a337a07b 100644 +--- a/kpartx/kpartx.c ++++ b/kpartx/kpartx.c +@@ -407,7 +407,7 @@ main(int argc, char **argv){ + fprintf(stderr, "can't del loop : %s\n", + loopdev); + r = 1; +- } else ++ } else if (verbose) + fprintf(stderr, "loop deleted : %s\n", loopdev); + } + goto end; +@@ -649,16 +649,17 @@ main(int argc, char **argv){ + if (n > 0) + break; + } +- if (what == LIST && loopcreated && S_ISREG (buf.st_mode)) { ++ if (what == LIST && loopcreated) { + if (fd != -1) + close(fd); + if (del_loop(device)) { + if (verbose) +- printf("can't del loop : %s\n", ++ fprintf(stderr, "can't del loop : %s\n", + device); + exit(1); + } +- printf("loop deleted : %s\n", device); ++ if (verbose) ++ fprintf(stderr, "loop deleted : %s\n", device); + } + + end: +-- +2.17.2 + diff --git a/SOURCES/0061-RH-fix-find_multipaths-in-mpathconf.patch b/SOURCES/0061-RH-fix-find_multipaths-in-mpathconf.patch new file mode 100644 index 0000000..ea9d530 --- /dev/null +++ b/SOURCES/0061-RH-fix-find_multipaths-in-mpathconf.patch @@ -0,0 +1,134 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Wed, 10 Feb 2021 15:42:42 -0600 +Subject: [PATCH] RH: fix find_multipaths in mpathconf + +mpathconf wasn't correctly dealing with the new rhel-8 values for +find_multipaths + +Signed-off-by: Benjamin Marzinski +--- + multipath/mpathconf | 38 +++++++++++++++++++------------------- + multipath/mpathconf.8 | 14 +++++++------- + 2 files changed, 26 insertions(+), 26 deletions(-) + +diff --git a/multipath/mpathconf b/multipath/mpathconf +index f34003c9..2f4f3eaf 100644 +--- a/multipath/mpathconf ++++ b/multipath/mpathconf +@@ -54,7 +54,7 @@ function usage + echo "Disable: --disable" + echo "Only allow certain wwids (instead of enable): --allow " + echo "Set user_friendly_names (Default y): --user_friendly_names " +- echo "Set find_multipaths (Default y): --find_multipaths " ++ echo "Set find_multipaths (Default y): --find_multipaths " + echo "Set default property blacklist (Default y): --property_blacklist " + echo "Set enable_foreign to show foreign devices (Default n): --enable_foreign " + echo "Load the dm-multipath modules on enable (Default y): --with_module " +@@ -224,8 +224,12 @@ function validate_args + echo "--user_friendly_names must be either 'y' or 'n'" + exit 1 + fi +- if [ -n "$FIND" ] && [ "$FIND" != "y" -a "$FIND" != "n" ]; then +- echo "--find_multipaths must be either 'y' or 'n'" ++ if [ "$FIND" = "y" ]; then ++ FIND="yes" ++ elif [ "$FIND" = "n" ]; then ++ FIND="no" ++ elif [ -n "$FIND" ] && [ "$FIND" != "yes" -a "$FIND" != "no" -a "$FIND" != "strict" -a "$FIND" != "greedy" -a "$FIND" != "smart" ]; then ++ echo "--find_multipaths must be one of 'yes' 'no' 'strict' 'greedy' or 'smart'" + exit 1 + fi + if [ -n "$PROPERTY" ] && [ "$PROPERTY" != "y" -a "$PROPERTY" != "n" ]; then +@@ -327,10 +331,11 @@ if [ "$HAVE_BLACKLIST" = "1" ]; then + fi + + if [ "$HAVE_DEFAULTS" = "1" ]; then +- if sed -n '/^defaults[[:space:]]*{/,/^}/ p' $TMPFILE | grep -q "^[[:space:]]*find_multipaths[[:space:]]*\(yes\|1\)" ; then +- HAVE_FIND=1 +- elif sed -n '/^defaults[[:space:]]*{/,/^}/ p' $TMPFILE | grep -q "^[[:space:]]*find_multipaths[[:space:]]*\(no\|0\)" ; then +- HAVE_FIND=0 ++ HAVE_FIND=`sed -n '/^defaults[[:space:]]*{/,/^}/ p' $TMPFILE | sed -n 's/^[[:blank:]]*find_multipaths[[:blank:]]*\([^[:blank:]]*\).*$/\1/p' | sed -n 1p` ++ if [ "$HAVE_FIND" = "1" ]; then ++ HAVE_FIND="yes" ++ elif [ "$HAVE_FIND" = "0" ]; then ++ HAVE_FIND="no" + fi + if sed -n '/^defaults[[:space:]]*{/,/^}/ p' $TMPFILE | grep -q "^[[:space:]]*user_friendly_names[[:space:]]*\(yes\|1\)" ; then + HAVE_FRIENDLY=1 +@@ -360,10 +365,10 @@ if [ -n "$SHOW_STATUS" ]; then + else + echo "multipath is disabled" + fi +- if [ -z "$HAVE_FIND" -o "$HAVE_FIND" = 0 ]; then +- echo "find_multipaths is disabled" ++ if [ -z "$HAVE_FIND" ]; then ++ echo "find_multipaths is no" + else +- echo "find_multipaths is enabled" ++ echo "find_multipaths is $HAVE_FIND" + fi + if [ -z "$HAVE_FRIENDLY" -o "$HAVE_FRIENDLY" = 0 ]; then + echo "user_friendly_names is disabled" +@@ -455,19 +460,14 @@ elif [ "$ENABLE" = 0 ]; then + fi + fi + +-if [ "$FIND" = "n" ]; then +- if [ "$HAVE_FIND" = 1 ]; then +- sed -i '/^defaults[[:space:]]*{/,/^}/ s/^[[:space:]]*find_multipaths[[:space:]]*\(yes\|1\)/ find_multipaths no/' $TMPFILE +- CHANGED_CONFIG=1 +- fi +-elif [ "$FIND" = "y" ]; then ++if [ -n "$FIND" ]; then + if [ -z "$HAVE_FIND" ]; then + sed -i '/^defaults[[:space:]]*{/ a\ +- find_multipaths yes ++ find_multipaths '"$FIND"' + ' $TMPFILE + CHANGED_CONFIG=1 +- elif [ "$HAVE_FIND" = 0 ]; then +- sed -i '/^defaults[[:space:]]*{/,/^}/ s/^[[:space:]]*find_multipaths[[:space:]]*\(no\|0\)/ find_multipaths yes/' $TMPFILE ++ elif [ "$FIND" != "$HAVE_FIND" ]; then ++ sed -i '/^defaults[[:space:]]*{/,/^}/ s/^[[:blank:]]*find_multipaths[[:blank:]]*[^[:blank:]]*/ find_multipaths '"$FIND"'/' $TMPFILE + CHANGED_CONFIG=1 + fi + fi +diff --git a/multipath/mpathconf.8 b/multipath/mpathconf.8 +index b82961d6..83515eb4 100644 +--- a/multipath/mpathconf.8 ++++ b/multipath/mpathconf.8 +@@ -38,9 +38,9 @@ If + already exists, mpathconf will edit it. If it does not exist, mpathconf will + create a default file with + .B user_friendly_names +-and ++set and + .B find_multipaths +-set. To disable these, use the ++set to \fByes\fP. To disable these, use the + .B --user_friendly_names n + and + .B --find_multipaths n +@@ -77,13 +77,13 @@ to the + defaults section. If set to \fBn\fP, this removes the line, if present. This + command can be used along with any other command. + .TP +-.B --find_multipaths\fP { \fBy\fP | \fBn\fP } +-If set to \fBy\fP, this adds the line +-.B find_multipaths yes ++.B --find_multipaths\fP { \fByes\fP | \fBno\fP | \fBstrict\fP | \fBgreedy\fP | \fBsmart\fP } ++If set to \fB\fP, this adds the line ++.B find_multipaths + to the + .B /etc/multipath.conf +-defaults section. If set to \fBn\fP, this removes the line, if present. This +-command can be used along with any other command. ++defaults section. This command can be used along with any other command. ++\fBy\fP and \fBn\fP can be used instead of \fByes\fP and \fBno\fP. + .TP + .B --property_blacklist \fP { \fBy\fP | \fBn\fP } + If set to \fBy\fP, this adds the line +-- +2.17.2 + diff --git a/SOURCES/0062-libmultipath-select_action-don-t-drop-map-if-alias-c.patch b/SOURCES/0062-libmultipath-select_action-don-t-drop-map-if-alias-c.patch new file mode 100644 index 0000000..d34e9e3 --- /dev/null +++ b/SOURCES/0062-libmultipath-select_action-don-t-drop-map-if-alias-c.patch @@ -0,0 +1,45 @@ +From 7a7b96246b84ccf533a6f4dc0424830792fdb96a Mon Sep 17 00:00:00 2001 +From: Martin Wilck +Date: Fri, 3 Jul 2020 15:17:09 +0200 +Subject: [PATCH] libmultipath: select_action(): don't drop map if alias + clashes + +If for a given map, if we find that the requested alias is already +used by a map with different WWID, while the map's own WWID is +not used yet, give up the alias and use the WWID instead. This +is safer than trying to destroy the existing map, which is likely +to fail. + +This allows us to make use const for the "curmp" parameter. + +Reviewed-by: Benjamin Marzinski +--- + libmultipath/configure.c | 11 ++++++----- + 1 file changed, 6 insertions(+), 5 deletions(-) + +diff --git a/libmultipath/configure.c b/libmultipath/configure.c +index b7113291..2e8f34f9 100644 +--- a/libmultipath/configure.c ++++ b/libmultipath/configure.c +@@ -690,12 +690,13 @@ select_action (struct multipath * mpp, vector curmp, int force_reload) + } + + if (!cmpp) { +- condlog(2, "%s: remove (wwid changed)", mpp->alias); +- dm_flush_map(mpp->alias); +- strlcpy(cmpp_by_name->wwid, mpp->wwid, WWID_SIZE); +- drop_multipath(curmp, cmpp_by_name->wwid, KEEP_PATHS); ++ condlog(1, "%s: can't use alias \"%s\" used by %s, falling back to WWID", ++ mpp->wwid, mpp->alias, cmpp_by_name->wwid); ++ /* We can do this because wwid wasn't found */ ++ free(mpp->alias); ++ mpp->alias = strdup(mpp->wwid); + mpp->action = ACT_CREATE; +- condlog(3, "%s: set ACT_CREATE (map wwid change)", ++ condlog(3, "%s: set ACT_CREATE (map does not exist, name changed)", + mpp->alias); + return; + } +-- +2.17.2 + diff --git a/SOURCES/0063-libmultipath-check-if-user_friendly_name-is-in-use.patch b/SOURCES/0063-libmultipath-check-if-user_friendly_name-is-in-use.patch new file mode 100644 index 0000000..81947be --- /dev/null +++ b/SOURCES/0063-libmultipath-check-if-user_friendly_name-is-in-use.patch @@ -0,0 +1,231 @@ +From e714eb26fddc8768a8de279d1de3ffedab35929e Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Fri, 5 Mar 2021 21:40:57 -0600 +Subject: [PATCH] libmultipath: check if user_friendly_name is in use + +If there are multipath devices that have user_friendly_names but do not +have their bindings in the bindings_file, get_user_friendly_alias() can +currently give out those names again. This can result in an incorrect +entry in the bindings file, and a device that gets created with a WWID +alias instead of a user_friendly_name. This situation can happen after +the pivot root, if a multipath device is created in the initramfs. If +this device doesn't have a binding in the regular filesystem +bindings_file and a new multipath device is created before it can add +its binding, the new device can steal that user_friendly_name during +multipathd's initial configure. + +To solve this, get_user_friendly_alias() now calls lookup_binding() with +a new paramter, telling it to check if the id it found is already in use +by a diffent device. If so, lookup_binding() will continue to check open +ids, until it finds one that it not currently in use by a dm device. +--- + libmultipath/alias.c | 48 +++++++++++++++++++++++++++++++++++++++++--- + tests/alias.c | 22 ++++++++++---------- + 2 files changed, 56 insertions(+), 14 deletions(-) + +diff --git a/libmultipath/alias.c b/libmultipath/alias.c +index 14401cae..01f737f4 100644 +--- a/libmultipath/alias.c ++++ b/libmultipath/alias.c +@@ -17,6 +17,7 @@ + #include "vector.h" + #include "checkers.h" + #include "structs.h" ++#include "devmapper.h" + + + /* +@@ -104,6 +105,28 @@ scan_devname(const char *alias, const char *prefix) + return n; + } + ++static int ++id_already_taken(int id, const char *prefix, const char *map_wwid) ++{ ++ char alias[LINE_MAX]; ++ ++ if (format_devname(alias, id, LINE_MAX, prefix) < 0) ++ return 0; ++ ++ if (dm_map_present(alias)) { ++ char wwid[WWID_SIZE]; ++ ++ /* If both the name and the wwid match, then it's fine.*/ ++ if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 && ++ strncmp(map_wwid, wwid, sizeof(wwid)) == 0) ++ return 0; ++ condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", map_wwid, alias); ++ return 1; ++ } ++ return 0; ++} ++ ++ + /* + * Returns: 0 if matching entry in WWIDs file found + * -1 if an error occurs +@@ -113,7 +136,7 @@ scan_devname(const char *alias, const char *prefix) + */ + static int + lookup_binding(FILE *f, const char *map_wwid, char **map_alias, +- const char *prefix) ++ const char *prefix, int check_if_taken) + { + char buf[LINE_MAX]; + unsigned int line_nr = 0; +@@ -168,12 +191,31 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias, + return 0; + } + } ++ if (!prefix && check_if_taken) ++ id = -1; + if (id >= smallest_bigger_id) { + if (biggest_id < INT_MAX) + id = biggest_id + 1; + else + id = -1; + } ++ if (id > 0 && check_if_taken) { ++ while(id_already_taken(id, prefix, map_wwid)) { ++ if (id == INT_MAX) { ++ id = -1; ++ break; ++ } ++ id++; ++ if (id == smallest_bigger_id) { ++ if (biggest_id == INT_MAX) { ++ id = -1; ++ break; ++ } ++ if (biggest_id >= smallest_bigger_id) ++ id = biggest_id + 1; ++ } ++ } ++ } + if (id < 0) { + condlog(0, "no more available user_friendly_names"); + return -1; +@@ -316,7 +358,7 @@ use_existing_alias (const char *wwid, const char *file, const char *alias_old, + goto out; + } + +- id = lookup_binding(f, wwid, &alias, NULL); ++ id = lookup_binding(f, wwid, &alias, NULL, 0); + if (alias) { + condlog(3, "Use existing binding [%s] for WWID [%s]", + alias, wwid); +@@ -373,7 +415,7 @@ get_user_friendly_alias(const char *wwid, const char *file, const char *prefix, + return NULL; + } + +- id = lookup_binding(f, wwid, &alias, prefix); ++ id = lookup_binding(f, wwid, &alias, prefix, 1); + if (id < 0) { + fclose(f); + return NULL; +diff --git a/tests/alias.c b/tests/alias.c +index 30414db0..ab1a9325 100644 +--- a/tests/alias.c ++++ b/tests/alias.c +@@ -356,7 +356,7 @@ static void lb_empty(void **state) + + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID0", &alias, NULL); ++ rc = lookup_binding(NULL, "WWID0", &alias, NULL, 0); + assert_int_equal(rc, 1); + assert_ptr_equal(alias, NULL); + } +@@ -369,7 +369,7 @@ static void lb_match_a(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + expect_condlog(3, "Found matching wwid [WWID0] in bindings file." + " Setting alias to MPATHa\n"); +- rc = lookup_binding(NULL, "WWID0", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 0); + assert_int_equal(rc, 0); + assert_ptr_not_equal(alias, NULL); + assert_string_equal(alias, "MPATHa"); +@@ -384,7 +384,7 @@ static void lb_nomatch_a(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID1] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID1", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 0); + assert_int_equal(rc, 2); + assert_ptr_equal(alias, NULL); + } +@@ -398,7 +398,7 @@ static void lb_match_c(void **state) + will_return(__wrap_fgets, "MPATHc WWID1\n"); + expect_condlog(3, "Found matching wwid [WWID1] in bindings file." + " Setting alias to MPATHc\n"); +- rc = lookup_binding(NULL, "WWID1", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 0); + assert_int_equal(rc, 0); + assert_ptr_not_equal(alias, NULL); + assert_string_equal(alias, "MPATHc"); +@@ -414,7 +414,7 @@ static void lb_nomatch_a_c(void **state) + will_return(__wrap_fgets, "MPATHc WWID1\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, 2); + assert_ptr_equal(alias, NULL); + } +@@ -428,7 +428,7 @@ static void lb_nomatch_c_a(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, 2); + assert_ptr_equal(alias, NULL); + } +@@ -443,7 +443,7 @@ static void lb_nomatch_a_b(void **state) + will_return(__wrap_fgets, "MPATHb WWID1\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, 3); + assert_ptr_equal(alias, NULL); + } +@@ -459,7 +459,7 @@ static void lb_nomatch_a_b_bad(void **state) + will_return(__wrap_fgets, NULL); + expect_condlog(3, "Ignoring malformed line 3 in bindings file\n"); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, 3); + assert_ptr_equal(alias, NULL); + } +@@ -474,7 +474,7 @@ static void lb_nomatch_b_a(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, 27); + assert_ptr_equal(alias, NULL); + } +@@ -490,7 +490,7 @@ static void lb_nomatch_int_max(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(0, "no more available user_friendly_names\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, -1); + assert_ptr_equal(alias, NULL); + } +@@ -505,7 +505,7 @@ static void lb_nomatch_int_max_m1(void **state) + will_return(__wrap_fgets, "MPATHa WWID0\n"); + will_return(__wrap_fgets, NULL); + expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n"); +- rc = lookup_binding(NULL, "WWID2", &alias, "MPATH"); ++ rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0); + assert_int_equal(rc, INT_MAX); + assert_ptr_equal(alias, NULL); + } +-- +2.17.2 + diff --git a/SPECS/device-mapper-multipath.spec b/SPECS/device-mapper-multipath.spec index e479429..5104f8f 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: 5%{?dist} +Release: 10%{?dist} License: GPLv2 Group: System Environment/Base URL: http://christophe.varoqui.free.fr/ @@ -49,6 +49,31 @@ Patch00035: 0035-Makefile.inc-trim-extra-information-from-systemd-ver.patch Patch00036: 0036-kpartx-fix-Wsign-compare-error.patch Patch00037: 0037-libmultipath-count-pending-paths-as-active-on-loads.patch Patch00038: 0038-multipath-deal-with-failures-flushing-maps.patch +Patch00039: 0039-libmultipath-factor-out-code-to-get-vpd-page-data.patch +Patch00040: 0040-libmultipath-limit-reading-0xc9-vpd-page.patch +Patch00041: 0041-libmultipath-add-device-to-hwtable.c.patch +Patch00042: 0042-libmultipath-move-fast_io_fail-defines-to-structs.h.patch +Patch00043: 0043-libmultipath-add-eh_deadline-multipath.conf-paramete.patch +Patch00044: 0044-libmultipath-don-t-dlclose-tur-checker-DSO.patch +Patch00045: 0045-libmultipath-warn-about-missing-braces-at-end-of-mul.patch +Patch00046: 0046-libmultipath-ignore-multipaths-sections-without-wwid.patch +Patch00047: 0047-tests-fix-missing-priority-blacklist-test.patch +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 +Patch00060: 0060-kpartx-free-loop-device-after-listing-partitions.patch +Patch00061: 0061-RH-fix-find_multipaths-in-mpathconf.patch +Patch00062: 0062-libmultipath-select_action-don-t-drop-map-if-alias-c.patch +Patch00063: 0063-libmultipath-check-if-user_friendly_name-is-in-use.patch # runtime Requires: %{name}-libs = %{version}-%{release} @@ -250,6 +275,72 @@ fi %{_pkgconfdir}/libdmmp.pc %changelog +* Sun Mar 7 2021 Benjamin Marzinski 0.8.4-10 +- Add 0062-libmultipath-select_action-don-t-drop-map-if-alias-c.patch + * Fall back to WWID names instead of removing existing device +- Add 0063-libmultipath-check-if-user_friendly_name-is-in-use.patch + * make sure to choose a user_friendly_name that isn't in use + * Fixes bz #1923777 +- Add alias_clash CI test +- Resolves: bz #1923777 + +* Wed Feb 10 2021 Benjamin Marzinski 0.8.4-9 +- Add 0060-kpartx-free-loop-device-after-listing-partitions.patch + * Fixes bz #1925490 +- Add 0061-RH-fix-find_multipaths-in-mpathconf.patch + * Fixes bz #1921651 +- Resolves: bz #1921651, #1925490 + +* 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 +- Add 0051-multipath.conf.5-Improve-checker_timeout-description.patch + * Fixes bz #1906073 +- Resolves: bz #1892496, #1906073 + +* Tue Nov 10 2020 Benjamin Marzinski 0.8.4-6 +- Add 0039-libmultipath-factor-out-code-to-get-vpd-page-data.patch +- Add 0040-libmultipath-limit-reading-0xc9-vpd-page.patch + * Only check for rdac support on configured devices, or devices where + vpd page 0x00 indicates support for vpd page 0xc9 + * Fixes bz #1877415 +- Add 0041-libmultipath-add-device-to-hwtable.c.patch + * Add support for Fujitsu Eternus AB/HB + * Fixes bz #1857429 +- Add 0042-libmultipath-move-fast_io_fail-defines-to-structs.h.patch +- Add 0043-libmultipath-add-eh_deadline-multipath.conf-paramete.patch + * Fixes bz #1819897 +- Add 0044-libmultipath-don-t-dlclose-tur-checker-DSO.patch + * Fixes bz #1875384 +- Add 0045-libmultipath-warn-about-missing-braces-at-end-of-mul.patch + * Fixes bz #1897530 +- Add 0046-libmultipath-ignore-multipaths-sections-without-wwid.patch + * Fixes bz #1897815 +- Add 0047-tests-fix-missing-priority-blacklist-test.patch +- Add 0048-mpathpersist-Fix-Register-and-Ignore-with-0x00-SARK.patch + * Make mpathpersist Register and Ignore command clear the registration + with a 0x0 SARK, like sg_persist does. + * Fixes bz #1894103 +- Add 0049-mpathpersist-update-prkeys-file-on-changing-registra.patch + * Fixes bz #1900522 + * The above 11 patches have all been submitted upstream +- Resolves: bz #1819897, #1857429, #1875384, #1877415, #1894103, #1897530 +- Resolves: bz #1897815, #1900522 + * Mon Jul 13 2020 Benjamin Marzinski 0.8.4-5 - Add 0038-multipath-deal-with-failures-flushing-maps.patch * Don't print "fail" if multipath needs to failback to removing