[dm-devel] [PATCH 1/4] libmultipath: don't print garbage keywords
If snprint_keyword() failed to correctly set up sbuf, don't print it. Instead, return an error. Signed-off-by: Benjamin Marzinski --- libmpathutil/parser.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c index 014d9b83..8d3ac53a 100644 --- a/libmpathutil/parser.c +++ b/libmpathutil/parser.c @@ -152,7 +152,7 @@ int snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw, const void *data) { - int r; + int r = 0; char *f; struct config *conf; STRBUF_ON_STACK(sbuf); @@ -190,8 +190,10 @@ snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw, } } while (*fmt++); out: - return __append_strbuf_str(buff, get_strbuf_str(), - get_strbuf_len()); + if (r >= 0) + r = __append_strbuf_str(buff, get_strbuf_str(), + get_strbuf_len()); + return r; } static const char quote_marker[] = { '\0', '"', '\0' }; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/4] libmultipath: avoid STRBUF_ON_STACK with cancellation points
STRBUF_ON_STACK() uses the cleanup __attribute__, which doesn't get run if a thread is cancelled. condlog() will call fprintf() when run under systemd, which is a cancellation point. The snprint function for the generic mutipath and generic path operations both call cancellation points. Also, the keyword print functions can call cancellation points. Because of all this, I did not see any safe uses of STRBUF_ON_STACK() outside of the unit tests. Replace them all with pthread cleanup handlers. Signed-off-by: Benjamin Marzinski --- libmpathutil/parser.c| 5 +- libmpathutil/strbuf.h| 4 +- libmultipath/alias.c | 59 +++--- libmultipath/blacklist.c | 4 +- libmultipath/discovery.c | 34 +++ libmultipath/dmparser.c | 21 +++ libmultipath/foreign.c | 4 +- libmultipath/generic.c | 10 +++- libmultipath/print.c | 50 ++-- libmultipath/prioritizers/weightedpath.c | 16 ++--- libmultipath/propsel.c | 76 ++-- libmultipath/sysfs.h | 11 +--- libmultipath/uevent.c| 6 +- 13 files changed, 195 insertions(+), 105 deletions(-) diff --git a/libmpathutil/parser.c b/libmpathutil/parser.c index 8d3ac53a..04c65b51 100644 --- a/libmpathutil/parser.c +++ b/libmpathutil/parser.c @@ -25,6 +25,7 @@ #include "parser.h" #include "debug.h" #include "strbuf.h" +#include "util.h" /* local vars */ static int sublevel = 0; @@ -155,11 +156,12 @@ snprint_keyword(struct strbuf *buff, const char *fmt, struct keyword *kw, int r = 0; char *f; struct config *conf; - STRBUF_ON_STACK(sbuf); + struct strbuf sbuf = STRBUF_INIT; if (!kw || !kw->print) return 0; + pthread_cleanup_push_cast(reset_strbuf, ); do { f = strchr(fmt, '%'); if (f == NULL) { @@ -193,6 +195,7 @@ out: if (r >= 0) r = __append_strbuf_str(buff, get_strbuf_str(), get_strbuf_len()); + pthread_cleanup_pop(1); return r; } diff --git a/libmpathutil/strbuf.h b/libmpathutil/strbuf.h index ae863417..5aa54677 100644 --- a/libmpathutil/strbuf.h +++ b/libmpathutil/strbuf.h @@ -42,7 +42,9 @@ void free_strbuf(struct strbuf *buf); * macro: STRBUF_ON_STACK * * Define and initialize a local struct @strbuf to be cleaned up when - * the current scope is left + * the current scope is left. This can only be used in non-threaded + * programs, or if there are no pthread cancellation points in the + * current scope, after the buffer could first be used. */ #define STRBUF_ON_STACK(__x) \ struct strbuf __attribute__((cleanup(reset_strbuf))) (__x) = STRBUF_INIT; diff --git a/libmultipath/alias.c b/libmultipath/alias.c index 05201224..3a81a7ad 100644 --- a/libmultipath/alias.c +++ b/libmultipath/alias.c @@ -112,12 +112,14 @@ scan_devname(const char *alias, const char *prefix) static int id_already_taken(int id, const char *prefix, const char *map_wwid) { - STRBUF_ON_STACK(buf); + struct strbuf buf = STRBUF_INIT; const char *alias; + int r = 0; + pthread_cleanup_push_cast(reset_strbuf, ); if (append_strbuf_str(, prefix) < 0 || format_devname(, id) < 0) - return 0; + goto out; alias = get_strbuf_str(); if (dm_map_present(alias)) { @@ -126,11 +128,13 @@ id_already_taken(int id, const char *prefix, const char *map_wwid) /* 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; + goto out; condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", map_wwid, alias); - return 1; + r = 1; } - return 0; +out: + pthread_cleanup_pop(1); + return r; } @@ -277,10 +281,12 @@ rlookup_binding(FILE *f, char *buff, const char *map_alias) static char * allocate_binding(int fd, const char *wwid, int id, const char *prefix) { - STRBUF_ON_STACK(buf); + struct strbuf buf = STRBUF_INIT; off_t offset; ssize_t len; - char *alias, *c; + char *alias = NULL; + const char *str; + int end; if (id <= 0) { condlog(0, "%s: cannot allocate new binding for id %d", @@ -288,38 +294,41 @@ allocate_binding(int fd, const char *wwid, int id, const char *prefix) return NULL; } + pthread_cleanup_push_cast(reset_s
[dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses
the cleanup __attribute__ is only run when a variable goes out of scope normally. It is not run on pthread cancellation. This means that multipathd could leak whatever resources were supposed to be cleaned up if the thread was cancelled in a function using variables with the cleanup __attribute__. This patchset removes all these uses in cases where the code is run by multipathd and includes a cancellation point in the variables scope (usually condlog(), which calls fprintf(), a cancellation point, the way multipathd is usually run). Benjamin Marzinski (4): libmultipath: don't print garbage keywords libmultipath: avoid STRBUF_ON_STACK with cancellation points libmultipath: use regular array for field widths libmultipath: avoid cleanup __attribute__ with cancellation points libmpathutil/parser.c| 13 ++-- libmpathutil/strbuf.h| 4 +- libmultipath/alias.c | 59 ++--- libmultipath/blacklist.c | 4 +- libmultipath/configure.c | 6 +- libmultipath/discovery.c | 34 ++ libmultipath/dmparser.c | 23 +++ libmultipath/foreign.c | 9 +-- libmultipath/generic.c | 14 ++-- libmultipath/libmultipath.version| 4 +- libmultipath/print.c | 82 ++-- libmultipath/print.h | 4 +- libmultipath/prioritizers/weightedpath.c | 22 --- libmultipath/propsel.c | 76 -- libmultipath/sysfs.h | 11 +--- libmultipath/uevent.c| 6 +- multipath/main.c | 5 +- multipathd/cli_handlers.c| 52 +++ multipathd/main.c| 49 -- 19 files changed, 286 insertions(+), 191 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 8/8] libmultipath: enforce queue_mode bio for nmve:tcp paths
nvme:tcp devices set BLK_MQ_F_BLOCKING (they are the only block devices which multipath supports that do so), meaning that block_mq expects that they can block at certain points while servicing a request. However, due to the way device-mapper sets up its queue, it is not able to set BLK_MQ_F_BLOCKING when it includes paths that set this flag. Patches were written to address this issue but they were rejected upstream https://lore.kernel.org/linux-block/ych%2fe4jnag0qy...@infradead.org/T/#t The proposed solution was to have multipath use the bio queue_mode for multipath devices that include nvme:tcp paths. Multipath devices now automatically add the "queue_mode bio" feature if they include nvme:tcp paths. If a multipath devices was created with "queue_mode rq", it will disallow the addition of nvme:tcp paths. Signed-off-by: Benjamin Marzinski --- libmultipath/configure.c | 17 - libmultipath/structs_vec.c | 7 +++ multipath/multipath.conf.5 | 4 +++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 7c7babd9..e5249fc1 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -260,6 +260,7 @@ int rr_optimize_path_order(struct pathgroup *pgp) int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) { struct pathgroup * pgp; + struct path *pp; struct config *conf; int i, marginal_pathgroups; char *save_attr; @@ -275,6 +276,14 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) if (mpp->disable_queueing && VECTOR_SIZE(mpp->paths) != 0) mpp->disable_queueing = 0; + /* Force QUEUE_MODE_BIO for maps with nvme:tcp paths */ + vector_foreach_slot(mpp->paths, pp, i) { + if (pp->bus == SYSFS_BUS_NVME && + pp->sg_id.proto_id == NVME_PROTOCOL_TCP) { + mpp->queue_mode = QUEUE_MODE_BIO; + break; + } + } /* * If this map was created with add_map_without_path(), * mpp->hwe might not be set yet. @@ -1148,6 +1157,13 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid, continue; } + cmpp = find_mp_by_wwid(curmp, pp1->wwid); + if (cmpp && cmpp->queue_mode == QUEUE_MODE_RQ && + pp1->bus == SYSFS_BUS_NVME && pp1->sg_id.proto_id == + NVME_PROTOCOL_TCP) { + orphan_path(pp1, "nvme:tcp path not allowed with request queue_mode multipath device"); + continue; + } /* * at this point, we know we really got a new mp */ @@ -1186,7 +1202,6 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid, } verify_paths(mpp); - cmpp = find_mp_by_wwid(curmp, mpp->wwid); if (cmpp) mpp->queue_mode = cmpp->queue_mode; if (setup_map(mpp, , vecs)) { diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 645896c6..5a618767 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -264,6 +264,13 @@ int adopt_paths(vector pathvec, struct multipath *mpp) } if (pp->initialized == INIT_REMOVED) continue; + if (mpp->queue_mode == QUEUE_MODE_RQ && + pp->bus == SYSFS_BUS_NVME && + pp->sg_id.proto_id == NVME_PROTOCOL_TCP) { + condlog(2, "%s: mulitpath device %s created with request queue_mode. Unable to add nvme:tcp paths", + pp->dev, mpp->alias); + continue; + } if (!mpp->paths && !(mpp->paths = vector_alloc())) goto err; diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 07476497..1fea9d5a 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -463,7 +463,9 @@ Before kernel 4.20 The default depends on the kernel parameter \fBdm_mod.use_blk_mq\fR. It is \fImq\fR if the latter is set, and \fIrq\fR otherwise. Since kernel 4.20, \fIrq\fR and \fImq\fR both correspond to block-multiqueue. Once a multipath device has been created, its queue_mode -cannot be changed. +cannot be changed. \fInvme:tcp\fR paths are only supported in multipath +devices with queue_mode set to \fIbio\fR. multipath will automatically +set this when creating a device with \fInvme:tcp\fR paths. .TP The default is: \fB\fR .RE -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 4/8] libmultipath: fix queue_mode feature handling
device-mapper is not able to change the queue_mode on a table reload. Make sure that when multipath sets up the map, both on regular reloads and reconfigures, it keeps the queue_mode the same. Signed-off-by: Benjamin Marzinski --- libmultipath/configure.c | 4 +++ libmultipath/dmparser.c| 2 ++ libmultipath/propsel.c | 55 ++ libmultipath/structs.h | 7 + multipath/multipath.conf.5 | 7 +++-- 5 files changed, 73 insertions(+), 2 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 8af7cd79..41641e30 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -1075,6 +1075,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid, struct config *conf = NULL; int allow_queueing; struct bitfield *size_mismatch_seen; + struct multipath * cmpp; /* ignore refwwid if it's empty */ if (refwwid && !strlen(refwwid)) @@ -1184,6 +1185,9 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid, } verify_paths(mpp); + cmpp = find_mp_by_wwid(curmp, mpp->wwid); + if (cmpp) + mpp->queue_mode = cmpp->queue_mode; if (setup_map(mpp, , vecs)) { remove_map(mpp, vecs->pathvec, NULL); continue; diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c index 50d13c08..3b37a926 100644 --- a/libmultipath/dmparser.c +++ b/libmultipath/dmparser.c @@ -151,6 +151,8 @@ int disassemble_map(const struct _vector *pathvec, free(word); } + mpp->queue_mode = strstr(mpp->features, "queue_mode bio") ? + QUEUE_MODE_BIO : QUEUE_MODE_RQ; /* * hwhandler diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index 98e3aad1..d4f19897 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -26,6 +26,7 @@ #include "strbuf.h" #include #include +#include pgpolicyfn *pgpolicies[] = { NULL, @@ -413,6 +414,59 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa } } +static void reconcile_features_with_queue_mode(struct multipath *mp) +{ + char *space = NULL, *val = NULL, *mode_str = NULL, *feat; + int features_mode = QUEUE_MODE_UNDEF; + + if (!mp->features) + return; + + pthread_cleanup_push(cleanup_free_ptr, ); + pthread_cleanup_push(cleanup_free_ptr, ); + pthread_cleanup_push(cleanup_free_ptr, _str); + + if (!(feat = strstr(mp->features, "queue_mode")) || + feat == mp->features || !isspace(*(feat - 1)) || + sscanf(feat, "queue_mode%m[ \f\n\r\t\v]%ms", , ) != 2) + goto sync_mode; + if (asprintf(_str, "queue_mode%s%s", space, val) < 0) { + condlog(1, "failed to allocate space for queue_mode feature string"); + mode_str = NULL; /* value undefined on failure */ + goto exit; + } + + if (!strcmp(val, "rq") || !strcmp(val, "mq")) + features_mode = QUEUE_MODE_RQ; + else if (!strcmp(val, "bio")) + features_mode = QUEUE_MODE_BIO; + if (features_mode == QUEUE_MODE_UNDEF) { + condlog(2, "%s: ignoring invalid feature '%s'", + mp->alias, mode_str); + goto sync_mode; + } + + if (mp->queue_mode == QUEUE_MODE_UNDEF) + mp->queue_mode = features_mode; + if (mp->queue_mode == features_mode) + goto exit; + + condlog(2, + "%s: ignoring feature '%s' because queue_mode is set to '%s'", + mp->alias, mode_str, + (mp->queue_mode == QUEUE_MODE_RQ)? "rq" : "bio"); + +sync_mode: + if (mode_str) + remove_feature(>features, mode_str); + if (mp->queue_mode == QUEUE_MODE_BIO) + add_feature(>features, "queue_mode bio"); +exit: + pthread_cleanup_pop(1); + pthread_cleanup_pop(1); + pthread_cleanup_pop(1); +} + int select_features(struct config *conf, struct multipath *mp) { const char *origin; @@ -428,6 +482,7 @@ out: reconcile_features_with_options(mp->alias, >features, >no_path_retry, >retain_hwhandler); + reconcile_features_with_queue_mode(mp); condlog(3, "%s: features = \"%s\" %s", mp->alias, mp->features, origin); return 0; } diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 5a713d46..129bdf0e 100644 --- a/libmultipath/structs.h +++ b/libmulti
[dm-devel] [PATCH 7/8] libmultipath: get nvme path transport protocol
Read the transport protocol from /sys/block/nvmeXnY/device/transport. Update protocol_name[] and bus_protocol_id() to store the nvme protocol names after the scsi protocol names. Signed-off-by: Benjamin Marzinski --- libmultipath/discovery.c | 18 -- libmultipath/structs.c | 22 +- libmultipath/structs.h | 33 + multipath/multipath.conf.5 | 8 ++-- 4 files changed, 60 insertions(+), 21 deletions(-) diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index e937f090..f3fccedd 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -1539,6 +1539,7 @@ nvme_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable) struct udev_device *parent; const char *attr_path = NULL; const char *attr; + int i; if (pp->udev) attr_path = udev_device_get_sysname(pp->udev); @@ -1561,6 +1562,18 @@ nvme_sysfs_pathinfo (struct path *pp, const struct _vector *hwtable) attr = udev_device_get_sysattr_value(parent, "cntlid"); pp->sg_id.channel = attr ? atoi(attr) : 0; + attr = udev_device_get_sysattr_value(parent, "transport"); + if (attr) { + for (i = 0; i < NVME_PROTOCOL_UNSPEC; i++){ + if (protocol_name[SYSFS_BUS_NVME + i] && + !strcmp(attr, + protocol_name[SYSFS_BUS_NVME + i] + 5)) { + pp->sg_id.proto_id = i; + break; + } + } + } + snprintf(pp->vendor_id, SCSI_VENDOR_SIZE, "NVME"); snprintf(pp->product_id, PATH_PRODUCT_SIZE, "%s", udev_device_get_sysattr_value(parent, "model")); @@ -1815,9 +1828,10 @@ sysfs_pathinfo(struct path *pp, const struct _vector *hwtable) pp->bus = SYSFS_BUS_SCSI; pp->sg_id.proto_id = SCSI_PROTOCOL_UNSPEC; } - if (!strncmp(pp->dev,"nvme", 4)) + if (!strncmp(pp->dev,"nvme", 4)) { pp->bus = SYSFS_BUS_NVME; - + pp->sg_id.proto_id = NVME_PROTOCOL_UNSPEC; + } switch (pp->bus) { case SYSFS_BUS_SCSI: return scsi_sysfs_pathinfo(pp, hwtable); diff --git a/libmultipath/structs.c b/libmultipath/structs.c index fb44cd64..7a2ff589 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -25,7 +25,6 @@ const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { [SYSFS_BUS_UNDEF] = "undef", [SYSFS_BUS_CCW] = "ccw", [SYSFS_BUS_CCISS] = "cciss", - [SYSFS_BUS_NVME] = "nvme", [SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp", [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi", [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa", @@ -37,6 +36,13 @@ const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata", [SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb", [SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec", + [SYSFS_BUS_NVME + NVME_PROTOCOL_PCIE] = "nvme:pcie", + [SYSFS_BUS_NVME + NVME_PROTOCOL_RDMA] = "nvme:rdma", + [SYSFS_BUS_NVME + NVME_PROTOCOL_FC] = "nvme:fc", + [SYSFS_BUS_NVME + NVME_PROTOCOL_TCP] = "nvme:tcp", + [SYSFS_BUS_NVME + NVME_PROTOCOL_LOOP] = "nvme:loop", + [SYSFS_BUS_NVME + NVME_PROTOCOL_APPLE_NVME] = "nvme:apple-nvme", + [SYSFS_BUS_NVME + NVME_PROTOCOL_UNSPEC] = "nvme:unspec", }; struct adapter_group * @@ -746,11 +752,17 @@ out: } unsigned int bus_protocol_id(const struct path *pp) { - if (!pp || pp->bus < 0 || pp->bus > SYSFS_BUS_SCSI) + if (!pp || pp->bus < 0 || pp->bus > SYSFS_BUS_NVME) return SYSFS_BUS_UNDEF; - if (pp->bus != SYSFS_BUS_SCSI) + if (pp->bus != SYSFS_BUS_SCSI && pp->bus != SYSFS_BUS_NVME) return pp->bus; - if ((int)pp->sg_id.proto_id < 0 || pp->sg_id.proto_id > SCSI_PROTOCOL_UNSPEC) + if (pp->sg_id.proto_id < 0) return SYSFS_BUS_UNDEF; - return SYSFS_BUS_SCSI + pp->sg_id.proto_id; + if (pp->bus == SYSFS_BUS_SCSI && + pp->sg_id.proto_id > SCSI_PROTOCOL_UNSPEC) + return SYSFS_BUS_UNDEF; + if (pp->bus == SYSFS_BUS_NVME && + pp->sg_id.proto_id > NVME_PROTOCOL_UNSPEC) + return SYSFS_BUS_UNDEF; + return pp->bus + pp->sg_id.proto_id; } diff --git a/libmultipath/structs.h b/libmultipath/st
[dm-devel] [PATCH 5/8] multipath tests: tests for reconcile_features_with_queue_mode
Signed-off-by: Benjamin Marzinski --- tests/Makefile | 2 + tests/features.c | 232 ++- 2 files changed, 233 insertions(+), 1 deletion(-) diff --git a/tests/Makefile b/tests/Makefile index ccdfbd15..bc9380f8 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -36,6 +36,7 @@ ifneq ($(DIO_TEST_DEV),) directio-test_FLAGS := -DDIO_TEST_DEV=\"$(DIO_TEST_DEV)\" endif mpathvalid-test_FLAGS := -I$(mpathvaliddir) +features-test_FLAGS := -I$(multipathdir)/nvme # test-specific linker flags # XYZ-test_TESTDEPS: test libraries containing __wrap_xyz functions @@ -73,6 +74,7 @@ strbuf-test_OBJDEPS := $(mpathutildir)/strbuf.o sysfs-test_TESTDEPS := test-log.o sysfs-test_OBJDEPS := $(multipathdir)/sysfs.o $(mpathutildir)/util.o sysfs-test_LIBDEPS := -ludev -lpthread -ldl +features-test_LIBDEPS := -ludev -lpthread %.o: %.c $(CC) $(CPPFLAGS) $(CFLAGS) $($*-test_FLAGS) -c -o $@ $< diff --git a/tests/features.c b/tests/features.c index 4d8f0860..31f978fd 100644 --- a/tests/features.c +++ b/tests/features.c @@ -1,9 +1,10 @@ +#define _GNU_SOURCE #include #include #include #include -#include "structs.h" +#include "../libmultipath/propsel.c" #include "globals.c" static void test_af_null_features_ptr(void **state) @@ -307,6 +308,234 @@ static int test_remove_features(void) return cmocka_run_group_tests(tests, NULL, NULL); } +static void test_cf_null_features(void **state) +{ + struct multipath mp = { + .alias = "test", + }; + reconcile_features_with_queue_mode(); + assert_null(mp.features); +} + +static void cf_helper(const char *features_start, const char *features_end, + int queue_mode_start, int queue_mode_end) +{ + struct multipath mp = { + .alias = "test", + .features = strdup(features_start), + .queue_mode = queue_mode_start, + }; + char *orig = mp.features; + + assert_non_null(orig); + reconcile_features_with_queue_mode(); + if (!features_end) + assert_ptr_equal(orig, mp.features); + else + assert_string_equal(mp.features, features_end); + free(mp.features); + assert_int_equal(mp.queue_mode, queue_mode_end); +} + +static void test_cf_unset_unset1(void **state) +{ + cf_helper("0", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_UNDEF); +} + +static void test_cf_unset_unset2(void **state) +{ + cf_helper("1 queue_mode", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_UNDEF); +} + +static void test_cf_unset_unset3(void **state) +{ + cf_helper("queue_mode", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_UNDEF); +} + +static void test_cf_unset_unset4(void **state) +{ + cf_helper("2 queue_model bio", NULL, QUEUE_MODE_UNDEF, + QUEUE_MODE_UNDEF); +} + +static void test_cf_unset_unset5(void **state) +{ + cf_helper("1 queue_if_no_path", NULL, QUEUE_MODE_UNDEF, + QUEUE_MODE_UNDEF); +} + +static void test_cf_invalid_unset1(void **state) +{ + cf_helper("2 queue_mode biop", "0", QUEUE_MODE_UNDEF, QUEUE_MODE_UNDEF); +} + +static void test_cf_invalid_unset2(void **state) +{ + cf_helper("3 queue_mode rqs queue_if_no_path", "1 queue_if_no_path", + QUEUE_MODE_UNDEF, QUEUE_MODE_UNDEF); +} + +static void test_cf_rq_unset1(void **state) +{ + cf_helper("2 queue_mode rq", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_RQ); +} + +static void test_cf_rq_unset2(void **state) +{ + cf_helper("2 queue_mode mq", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_RQ); +} + +static void test_cf_bio_unset(void **state) +{ + cf_helper("2 queue_mode bio", NULL, QUEUE_MODE_UNDEF, QUEUE_MODE_BIO); +} + +static void test_cf_unset_bio1(void **state) +{ + cf_helper("1 queue_if_no_path", "3 queue_if_no_path queue_mode bio", + QUEUE_MODE_BIO, QUEUE_MODE_BIO); +} + +static void test_cf_unset_bio2(void **state) +{ + cf_helper("0", "2 queue_mode bio", QUEUE_MODE_BIO, QUEUE_MODE_BIO); +} + +static void test_cf_unset_bio3(void **state) +{ + cf_helper("2 pg_init_retries 50", "4 pg_init_retries 50 queue_mode bio", + QUEUE_MODE_BIO, QUEUE_MODE_BIO); +} + +static void test_cf_invalid_bio1(void **state) +{ + cf_helper("2 queue_mode bad", "2 queue_mode bio", + QUEUE_MODE_BIO, QUEUE_MODE_BIO); +} + +static void test_cf_invalid_bio2(void **state) +{ + cf_helper("3 queue_if_no_path queue_mode\tbad", "3 queue_if_no_path queue_mode bio", + QUEUE_MODE_BIO, QUEUE_MODE_BIO); +} + +static void test_cf_bio_bio1(void **state) +{ + cf_helper("2 que
[dm-devel] [PATCH 6/8] libmultipath: prepare proto_id for use by non-scsi devivces
Make sure that when we are checking for a scsi protocol, we are first checking that we are working with a scsi path. Signed-off-by: Benjamin Marzinski --- libmultipath/configure.c | 9 + libmultipath/discovery.c | 13 - libmultipath/print.c | 6 -- libmultipath/structs.c | 2 +- libmultipath/structs.h | 4 +++- multipathd/fpin_handlers.c | 2 +- 6 files changed, 22 insertions(+), 14 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 41641e30..7c7babd9 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -218,10 +218,11 @@ int rr_optimize_path_order(struct pathgroup *pgp) total_paths = VECTOR_SIZE(pgp->paths); vector_foreach_slot(pgp->paths, pp, i) { - if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP && - pp->sg_id.proto_id != SCSI_PROTOCOL_SAS && - pp->sg_id.proto_id != SCSI_PROTOCOL_ISCSI && - pp->sg_id.proto_id != SCSI_PROTOCOL_SRP) { + if (pp->bus != SYSFS_BUS_SCSI || + (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP && +pp->sg_id.proto_id != SCSI_PROTOCOL_SAS && +pp->sg_id.proto_id != SCSI_PROTOCOL_ISCSI && +pp->sg_id.proto_id != SCSI_PROTOCOL_SRP)) { /* return success as default path order * is maintained in path group */ diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 15560f8c..e937f090 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -504,10 +504,11 @@ int sysfs_get_host_adapter_name(const struct path *pp, char *adapter_name) proto_id = pp->sg_id.proto_id; - if (proto_id != SCSI_PROTOCOL_FCP && - proto_id != SCSI_PROTOCOL_SAS && - proto_id != SCSI_PROTOCOL_ISCSI && - proto_id != SCSI_PROTOCOL_SRP) { + if (pp->bus != SYSFS_BUS_SCSI || + (proto_id != SCSI_PROTOCOL_FCP && +proto_id != SCSI_PROTOCOL_SAS && +proto_id != SCSI_PROTOCOL_ISCSI && +proto_id != SCSI_PROTOCOL_SRP)) { return 1; } /* iscsi doesn't have adapter info in sysfs @@ -1810,8 +1811,10 @@ sysfs_pathinfo(struct path *pp, const struct _vector *hwtable) pp->bus = SYSFS_BUS_CCISS; if (!strncmp(pp->dev,"dasd", 4)) pp->bus = SYSFS_BUS_CCW; - if (!strncmp(pp->dev,"sd", 2)) + if (!strncmp(pp->dev,"sd", 2)) { pp->bus = SYSFS_BUS_SCSI; + pp->sg_id.proto_id = SCSI_PROTOCOL_UNSPEC; + } if (!strncmp(pp->dev,"nvme", 4)) pp->bus = SYSFS_BUS_NVME; diff --git a/libmultipath/print.c b/libmultipath/print.c index 68a793e7..d7d522c8 100644 --- a/libmultipath/print.c +++ b/libmultipath/print.c @@ -650,7 +650,8 @@ snprint_host_attr (struct strbuf *buff, const struct path * pp, char *attr) const char *value = NULL; int ret; - if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP) + if (pp->bus != SYSFS_BUS_SCSI || + pp->sg_id.proto_id != SCSI_PROTOCOL_FCP) return append_strbuf_str(buff, "[undef]"); sprintf(host_id, "host%d", pp->sg_id.host_no); host_dev = udev_device_new_from_subsystem_sysname(udev, "fc_host", @@ -689,7 +690,8 @@ snprint_tgt_wwpn (struct strbuf *buff, const struct path * pp) const char *value = NULL; int ret; - if (pp->sg_id.proto_id != SCSI_PROTOCOL_FCP) + if (pp->bus != SYSFS_BUS_SCSI || + pp->sg_id.proto_id != SCSI_PROTOCOL_FCP) return append_strbuf_str(buff, "[undef]"); sprintf(rport_id, "rport-%d:%d-%d", pp->sg_id.host_no, pp->sg_id.channel, pp->sg_id.transport_id); diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 0f16c071..fb44cd64 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -116,7 +116,7 @@ alloc_path (void) pp->sg_id.channel = -1; pp->sg_id.scsi_id = -1; pp->sg_id.lun = SCSI_INVALID_LUN; - pp->sg_id.proto_id = SCSI_PROTOCOL_UNSPEC; + pp->sg_id.proto_id = PROTOCOL_UNSET; pp->fd = -1; pp->tpgs = TPGS_UNDEF; pp->priority = PRIO_UNDEF; diff --git a/libmultipath/structs.h b/libmultipath/structs.h index 129bdf0e..d3054662 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -176,6 +176,8 @@ enum queue_mode_states { QUEUE_MODE_RQ, }; +#define PROTOCOL_UNSET -1 + enum
[dm-devel] [PATCH 3/8] multipath tests: tests for adding and removing features
Signed-off-by: Benjamin Marzinski --- tests/Makefile | 2 +- tests/features.c | 319 +++ 2 files changed, 320 insertions(+), 1 deletion(-) create mode 100644 tests/features.c diff --git a/tests/Makefile b/tests/Makefile index 109ea75b..ccdfbd15 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -16,7 +16,7 @@ CFLAGS += $(BIN_CFLAGS) -Wno-unused-parameter $(W_MISSING_INITIALIZERS) LIBDEPS += -L. -L $(mpathutildir) -L$(mpathcmddir) -lmultipath -lmpathutil -lmpathcmd -lcmocka TESTS := uevent parser util dmevents hwtable blacklist unaligned vpd pgpolicy \ -alias directio valid devt mpathvalid strbuf sysfs +alias directio valid devt mpathvalid strbuf sysfs features HELPERS := test-lib.o test-log.o .SILENT: $(TESTS:%=%.o) diff --git a/tests/features.c b/tests/features.c new file mode 100644 index ..4d8f0860 --- /dev/null +++ b/tests/features.c @@ -0,0 +1,319 @@ +#include +#include +#include +#include + +#include "structs.h" +#include "globals.c" + +static void test_af_null_features_ptr(void **state) +{ + assert_int_equal(add_feature(NULL, "test"), 1); +} + +static void af_helper(const char *features_start, const char *addition, + const char *features_end, int result) +{ + char *f = NULL, *orig = NULL; + + if (features_start) { + f = strdup(features_start); + assert_non_null(f); + orig = f; + } + assert_int_equal(add_feature(, addition), result); + if (result != 0 || features_end == NULL) + assert_ptr_equal(orig, f); + else + assert_string_equal(f, features_end); + free(f); +} + +static void test_af_null_addition1(void **state) +{ + af_helper("0", NULL, NULL, 0); +} + +static void test_af_null_addition2(void **state) +{ + af_helper("1 queue_if_no_path", NULL, NULL, 0); +} + +static void test_af_empty_addition(void **state) +{ + af_helper("2 pg_init_retries 5", "", NULL, 0); +} + +static void test_af_invalid_addition1(void **state) +{ + af_helper("2 pg_init_retries 5", " ", NULL, 1); +} + +static void test_af_invalid_addition2(void **state) +{ + af_helper("2 pg_init_retries 5", "\tbad", NULL, 1); +} + +static void test_af_invalid_addition3(void **state) +{ + af_helper("2 pg_init_retries 5", "bad ", NULL, 1); +} + +static void test_af_invalid_addition4(void **state) +{ + af_helper("2 pg_init_retries 5", " bad ", NULL, 1); +} + +static void test_af_null_features1(void **state) +{ + af_helper(NULL, "test", "1 test", 0); +} + +static void test_af_null_features2(void **state) +{ + af_helper(NULL, "test\t more", "2 test\t more", 0); +} + +static void test_af_null_features3(void **state) +{ + af_helper(NULL, "test\neven\tmore", "3 test\neven\tmore", 0); +} + +static void test_af_already_exists1(void **state) +{ + af_helper("4 this is a test", "test", NULL, 0); +} + +static void test_af_already_exists2(void **state) +{ + af_helper("5 contest testy intestine test retest", "test", NULL, 0); +} + +static void test_af_almost_exists(void **state) +{ + af_helper("3 contest testy intestine", "test", + "4 contest testy intestine test", 0); +} + +static void test_af_bad_features1(void **state) +{ + af_helper("bad", "test", NULL, 1); +} + +static void test_af_bad_features2(void **state) +{ + af_helper("1bad", "test", NULL, 1); +} + +static void test_af_add1(void **state) +{ + af_helper("0", "test", "1 test", 0); +} + +static void test_af_add2(void **state) +{ + af_helper("0", "this is a test", "4 this is a test", 0); +} + +static void test_af_add3(void **state) +{ + af_helper("1 features", "more values", "3 features more values", 0); +} + +static void test_af_add4(void **state) +{ + af_helper("2 one\ttwo", "three\t four", "4 one\ttwo three\t four", 0); +} + +static int test_add_features(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_af_null_features_ptr), + cmocka_unit_test(test_af_null_addition1), + cmocka_unit_test(test_af_null_addition2), + cmocka_unit_test(test_af_empty_addition), + cmocka_unit_test(test_af_invalid_addition1), + cmocka_unit_test(test_af_invalid_addition2), + cmocka_unit_test(test_af_invalid_addition3), + cmocka_unit_test(test_af_invalid_addition4), + cmoc
[dm-devel] [PATCH 1/8] libmultipath: cleanup remove_feature
remove_feature() didn't correctly handle feature strings that used whitespace other than spaces, which the kernel allows. It also didn't check if the feature string to be removed was part of a larger feature token. Finally, it did a lot of unnecessary work. By failing if the feature string to be removed contains leading or trailing whitespace, the function can be significanly simplified. Signed-off-by: Benjamin Marzinski --- libmultipath/structs.c | 82 +++--- 1 file changed, 29 insertions(+), 53 deletions(-) diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 49621cb3..1f9945e0 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -6,6 +6,7 @@ #include #include #include +#include #include "checkers.h" #include "vector.h" @@ -663,7 +664,7 @@ int add_feature(char **f, const char *n) int remove_feature(char **f, const char *o) { - int c = 0, d, l; + int c = 0, d; char *e, *p, *n; const char *q; @@ -674,33 +675,35 @@ int remove_feature(char **f, const char *o) if (!o || *o == '\0') return 0; - /* Check if not present */ - if (!strstr(*f, o)) + d = strlen(o); + if (isspace(*o) || isspace(*(o + d - 1))) { + condlog(0, "internal error: feature \"%s\" has leading or trailing spaces", o); + return 1; + } + + /* Check if present and not part of a larger feature token*/ + p = *f + 1; /* the size must be at the start of the features string */ + while ((p = strstr(p, o)) != NULL) { + if (isspace(*(p - 1)) && + (isspace(*(p + d)) || *(p + d) == '\0')) + break; + p += d; + } + if (!p) return 0; /* Get feature count */ c = strtoul(*f, , 10); - if (*f == e) - /* parse error */ + if (*f == e || !isspace(*e)) { + condlog(0, "parse error in feature string \"%s\"", *f); return 1; - - /* Normalize features */ - while (*o == ' ') { - o++; } - /* Just spaces, return */ - if (*o == '\0') - return 0; - q = o + strlen(o); - while (*q == ' ') - q--; - d = (int)(q - o); /* Update feature count */ c--; q = o; - while (q[0] != '\0') { - if (q[0] == ' ' && q[1] != ' ' && q[1] != '\0') + while (*q != '\0') { + if (isspace(*q) && !isspace(*(q + 1)) && *(q + 1) != '\0') c--; q++; } @@ -714,15 +717,8 @@ int remove_feature(char **f, const char *o) goto out; } - /* Search feature to be removed */ - e = strstr(*f, o); - if (!e) - /* Not found, return */ - return 0; - /* Update feature count space */ - l = strlen(*f) - d; - n = malloc(l + 1); + n = malloc(strlen(*f) - d + 1); if (!n) return 1; @@ -732,36 +728,16 @@ int remove_feature(char **f, const char *o) * Copy existing features up to the feature * about to be removed */ - p = strchr(*f, ' '); - if (!p) { - /* Internal error, feature string inconsistent */ - free(n); - return 1; - } - while (*p == ' ') - p++; - p--; - if (e != p) { - do { - e--; - d++; - } while (*e == ' '); - e++; d--; - strncat(n, p, (size_t)(e - p)); - p += (size_t)(e - p); - } + strncat(n, e, (size_t)(p - e)); /* Skip feature to be removed */ p += d; - /* Copy remaining features */ - if (strlen(p)) { - while (*p == ' ') - p++; - if (strlen(p)) { - p--; - strcat(n, p); - } - } + while (isspace(*p)) + p++; + if (*p != '\0') + strcat(n, p); + else + strchop(n); out: free(*f); -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 0/8] mutipath: handle nvme:tcp paths better
nvme:tcp devices set BLK_MQ_F_BLOCKING (they are the only block devices which multipath supports that do so), meaning that block_mq expects that they can block at certain points while servicing a request. However, due to the way device-mapper sets up its queue, it is not able to set BLK_MQ_F_BLOCKING when it includes paths that set this flag. Patches were written to address this issue but they were rejected upstream https://lore.kernel.org/linux-block/ych%2fe4jnag0qy...@infradead.org/T/#t The proposed solution was to have multipath use the bio queue_mode for multipath devices that include nvme:tcp paths. Multipathd currently doesn't handle queue_mode as well as it could. Once a multipath device is created, queue_mode cannot be changed, but multipath doesn't enforce this. This patchset improves multipath's handling of the queue_mode feature, and then makes sure that if a multipath device includes a nvme:tcp path, it will have queue_mode set to bio. Benjamin Marzinski (8): libmultipath: cleanup remove_feature libmultipath: cleanup add_feature multipath tests: tests for adding and removing features libmultipath: fix queue_mode feature handling multipath tests: tests for reconcile_features_with_queue_mode libmultipath: prepare proto_id for use by non-scsi devivces libmultipath: get nvme path transport protocol libmultipath: enforce queue_mode bio for nmve:tcp paths libmultipath/configure.c | 28 +- libmultipath/discovery.c | 31 ++- libmultipath/dmparser.c| 2 + libmultipath/print.c | 6 +- libmultipath/propsel.c | 55 libmultipath/structs.c | 155 +-- libmultipath/structs.h | 44 ++- libmultipath/structs_vec.c | 7 + multipath/multipath.conf.5 | 17 +- multipathd/fpin_handlers.c | 2 +- tests/Makefile | 4 +- tests/features.c | 549 + 12 files changed, 784 insertions(+), 116 deletions(-) create mode 100644 tests/features.c -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/8] libmultipath: cleanup add_feature
add_feature() didn't correctly handle feature strings that used whitespace other than spaces, which the kernel allows. It also didn't allow adding features with multiple tokens. When it looked to see if the feature string to be added already existed, it didn't check if the match was part of a larger token. Finally, it did unnecessary work. By using asprintf() to create the string, the function can be signifcantly simplified. Signed-off-by: Benjamin Marzinski --- libmultipath/structs.c | 49 +- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 1f9945e0..0f16c071 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -602,23 +602,33 @@ int add_feature(char **f, const char *n) { int c = 0, d, l; char *e, *t; + const char *p; if (!f) return 1; /* Nothing to do */ - if (!n || *n == '0') + if (!n || *n == '\0') return 0; - if (strchr(n, ' ') != NULL) { - condlog(0, "internal error: feature \"%s\" contains spaces", n); + l = strlen(n); + if (isspace(*n) || isspace(*(n + l - 1))) { + condlog(0, "internal error: feature \"%s\" has leading or trailing spaces", n); return 1; } + p = n; + d = 1; + while (*p != '\0') { + if (isspace(*p) && !isspace(*(p + 1)) && *(p + 1) != '\0') + d++; + p++; + } + /* default feature is null */ if(!*f) { - l = asprintf(, "1 %s", n); + l = asprintf(, "%0d %s", d, n); if(l == -1) return 1; @@ -627,35 +637,24 @@ int add_feature(char **f, const char *n) } /* Check if feature is already present */ - if (strstr(*f, n)) - return 0; + e = *f; + while ((e = strstr(e, n)) != NULL) { + if (isspace(*(e - 1)) && + (isspace(*(e + l)) || *(e + l) == '\0')) + return 0; + e += l; + } /* Get feature count */ c = strtoul(*f, , 10); - if (*f == e || (*e != ' ' && *e != '\0')) { + if (*f == e || (!isspace(*e) && *e != '\0')) { condlog(0, "parse error in feature string \"%s\"", *f); return 1; } - - /* Add 1 digit and 1 space */ - l = strlen(e) + strlen(n) + 2; - - c++; - /* Check if we need more digits for feature count */ - for (d = c; d >= 10; d /= 10) - l++; - - t = calloc(1, l + 1); - if (!t) + c += d; + if (asprintf(, "%0d%s %s", c, e, n) < 0) return 1; - /* e: old feature string with leading space, or "" */ - if (*e == ' ') - while (*(e + 1) == ' ') - e++; - - snprintf(t, l + 1, "%0d%s %s", c, e, n); - free(*f); *f = t; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH V2 0/6] allowing path checking to be interrupted.
On Thu, Sep 15, 2022 at 02:56:36PM +0800, Wu Guanghao wrote: > Sorry for the late feedback. > > The version we are currently testing is 0.8.4, so we only merge the > first 3 patches in this series of patches. Then after the actual test, > it was found that the effect improvement is not very obvious. > > Test environment: 1000 multipath devices, 16 paths per device > Test command: Aggregate multipath devices using multipathd add path > Time consuming (time for 16 paths to aggregate 1 multipath device): > 1. Original: > < 4s: 76.9%; > 4s~10s: 18.4%; > >10s:4.7%; > 2. After using the patches: > < 4s: 79.1%; > 4s~10s: 18.2%; > >10s:2.7%; > > >From the results, the time-consuming improvement of the patch is not obvious, > so we made a few changes to the patch and it worked as expected. The > modification > of the patch is as follows: > > Paths_checked is changed to configurable, current setting n = 2. > Removed judgment on diff_time. > Sleep change from 10us to 5ms I worry that this is giving too much deference to the uevents. If there is a uevent storm, checking will stop for 5ms every 2 paths checked. I'm worried that this will make it take significantly longer for the the path checker to complete a cycle. Making paths_checked configurable, so that this doesn't trigger too often does help to avoid the issue that checking the time from chk_start_time was dealing with, and makes the mechanics of this simpler. But 5ms seems like a long time to have to wait, just to make sure that another process had the time to grab the vecs lock. Perhaps it would be better to sleep for a shorter length of time, but in a loop, where we can check to see if progress has been made, perhaps by checking some counter of events and user commands serviced. This way, we aren't sleeping too long for no good reason. > if (++paths_checked % n == 0 && > lock_has_waiters(>lock)) { > get_monotonic_time(_time); > timespecsub(_time, _start_time, > _time); > if (diff_time.tv_sec > 0) // Delete the code, goto directly > goto unlock; > } > > if (checker_state != CHECKER_FINISHED) { > /* Yield to waiters */ > //struct timespec wait = { .tv_nsec = 1, }; > //nanosleep(, NULL); > usleep(5000); // sleep change from 10us to 5ms > } > > Time consuming(After making the above changes to the patch): > < 4s: 99.5%; > = 4s: 0.5%; > > 4s: 0%; Since I believe that this is likely causing a real impact on the checker speed, I'm not sure that we're looking for results like this. Are you seeing That "path checkers took longer than ..." message? How long does it say the checker is taking (and what do you have max_polling_interval set to)? -Ben > 在 2022/8/18 4:48, Benjamin Marzinski 写道: > > When there are a huge number of paths (> 1) The amount of time that > > the checkerloop can hold the vecs lock for while checking the paths can > > get to be large enough that it starves other vecs lock users. If path > > checking takes long enough, it's possible that uxlsnr threads will never > > run. To deal with this, this patchset makes it possible to drop the vecs > > lock while checking the paths, and then reacquire it and continue with > > the next path to check. > > > > My choice of only checking if there are waiters every 128 paths checked > > and only interrupting if path checking has taken more than a second are > > arbitrary. I didn't want to slow down path checking in the common case > > where this isn't an issue, and I wanted to avoid path checking getting > > starved by other vecs->lock users. Having the checkerloop wait for 1 > > nsec was based on my own testing with a setup using 4K multipath devies > > with 4 paths each. This was almost always long enough for the uevent or > > uxlsnr client to grab the vecs lock, but I'm not sure how dependent this > > is on details of the system. For instance with my setup in never took > > more than 20 seconds to check the paths. and usually, a looping through > > all the paths took well under 10 seconds, most often under 5. I would > > only occasionally run into situations where a uxlsnr client would time > > out. > > > > V2 Changes: > > 0003: Switched to a simpler method of determining the path to continue > > checking at, as suggested by Martin Wilck. Also fixed a bug when > > the previous index was larger than the current vector size. > > > > Benjamin Marzinski (6): > > multipathd: Use regular pthread_mutex_t for waiter_lock > > multipathd: track
[dm-devel] [PATCH] multipathd: fix read past end of of buffer in sending reply
Reduce the send() length for the uxlsnr reply, by the number of bytes already sent Signed-off-by: Benjamin Marzinski --- multipathd/uxlsnr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c index 23cb123d..238744b7 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -541,7 +541,7 @@ static int client_state_machine(struct client *c, struct vectors *vecs, if (c->len < c->cmd_len) { const char *buf = get_strbuf_str(>reply); - n = send(c->fd, buf + c->len, c->cmd_len, MSG_NOSIGNAL); + n = send(c->fd, buf + c->len, c->cmd_len - c->len, MSG_NOSIGNAL); if (n == -1) { if (!(errno == EAGAIN || errno == EINTR)) c->error = -ECONNRESET; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/3] multipath-tools documentation fixes
On Sat, Sep 03, 2022 at 01:11:25PM +0200, mwi...@suse.com wrote: > From: Martin Wilck For the set: Reviewed-by: Benjamin Marzinski > > Martin Wilck (3): > multipathd: add multipathc.8 manual page > multipathd.8: remove misleading paragraph about multipath > multipathd.8: Fix "SEE ALSO" section > > multipathd/multipathc.8 | 64 + > multipathd/multipathd.8 | 41 ++ > 2 files changed, 93 insertions(+), 12 deletions(-) > create mode 100644 multipathd/multipathc.8 > > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 00/16] multipath-tools: minor fixes and build improvements
On Thu, Sep 01, 2022 at 06:09:36PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Hi Ben, hi Christophe, > > here's a set of minor fixes for multipath-tools. For the set: Reviewed-by: Benjamin Marzinski > > Regards > Martin > > Martin Wilck (16): > multipath-tools: Makefile: remove useless .PHONY targets > multipath-tools: Makefile: fix install->all dependency > multipath-tools: Makefile: remove dependency test -> test-progs > multipath-tools: Makefile: run abidiff with --redundant flag > libdmmp: Makefile: create man3dir > tests/Makefile: use $(multipathdir) > tests/Makefile: add library dependencies > tests/Makefile: use symbolic links under tests/lib > tests/Makefile: redirect program output into output file > GitHub workflows: package shared objects in artifact > libmultipath: replace close_fd() with cleanup_fd_ptr() > libmultipath: cleanup_free_ptr(): avoid double free > multipath: find_multipaths_check_timeout(): no need for pthread > cleanup > multipathd: fix segfault in cli_list_map_fmt() > multipathd: fix broken pthread cleanup in > fpin_fabric_notification_receiver() > multipathd: Fix command completion in interactive mode > > .github/workflows/foreign.yaml| 2 ++ > Makefile | 8 > libdmmp/Makefile | 1 + > libmpathutil/libmpathutil.version | 6 +- > libmpathutil/util.c | 19 +-- > libmpathutil/util.h | 2 +- > libmultipath/alias.c | 4 ++-- > libmultipath/foreign/nvme.c | 4 ++-- > libmultipath/sysfs.c | 12 ++-- > libmultipath/wwids.c | 8 > multipath/main.c | 8 +++- > multipathd/cli.c | 2 ++ > multipathd/cli_handlers.c | 2 +- > multipathd/fpin_handlers.c| 9 + > multipathd/main.c | 1 - > tests/Makefile| 26 +- > 16 files changed, 64 insertions(+), 50 deletions(-) > > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 10/12] multipathd: exec multipathc in interactive mode
On Tue, Aug 30, 2022 at 09:27:11PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > A previous patch disabled interactive mode in multipathd, because > uxclnt() would return immediately without an input command. > > With this patch, we reinstate interactive mode for "multipath -k", > by just exec()ing the multipathc client. > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > multipathd/Makefile | 3 ++- > multipathd/main.c | 15 +-- > multipathd/multipathc.c | 25 + > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/multipathd/Makefile b/multipathd/Makefile > index 19ab2e9..8a56304 100644 > --- a/multipathd/Makefile > +++ b/multipathd/Makefile > @@ -17,7 +17,8 @@ endif > > CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathpersistdir) > -I$(mpathcmddir) -I$(thirdpartydir) \ > $(shell $(PKGCONFIG) --modversion liburcu 2>/dev/null | \ > - awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + > $$2) + $$3); }') > + awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + > $$2) + $$3); }') \ > + -DBINDIR='"$(bindir)"' > CFLAGS += $(BIN_CFLAGS) > LDFLAGS += $(BIN_LDFLAGS) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 5a40894..66177cd 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -3616,7 +3616,7 @@ main (int argc, char *argv[]) > extern char *optarg; > extern int optind; > int arg; > - int err; > + int err = 0; > int foreground = 0; > struct config *conf; > char *opt_k_arg = NULL; > @@ -3710,7 +3710,18 @@ main (int argc, char *argv[]) > } > c += snprintf(c, s + CMDSIZE - c, "\n"); > } > - err = uxclnt(s, uxsock_timeout + 100); > + if (!s) { > + char tmo_buf[16]; > + > + snprintf(tmo_buf, sizeof(tmo_buf), "%d", > + uxsock_timeout + 100); > + if (execl(BINDIR "/multipathc", "multipathc", > + tmo_buf, NULL) == -1) { > + condlog(0, "ERROR: failed to execute > multipathc: %m"); > + err = 1; > + } > + } else > + err = uxclnt(s, uxsock_timeout + 100); > free_config(conf); > return err; > } > diff --git a/multipathd/multipathc.c b/multipathd/multipathc.c > index a4f9023..9d49655 100644 > --- a/multipathd/multipathc.c > +++ b/multipathd/multipathc.c > @@ -246,14 +246,31 @@ static void process(int fd, unsigned int timeout) > } > } > > -int main (void) > +int main (int argc, const char * const argv[]) > { > - int fd = mpath_connect(); > + int fd; > + int tmo = DEFAULT_REPLY_TIMEOUT + 100; > + char *ep; > > - if (fd == -1) > + if (argc > 2) { > + fprintf(stderr, "Usage: %s [timeout]\n", argv[0]); > return 1; > + } > + if (argc == 2) { > + tmo = strtol(argv[1], , 10); > + if (*argv[1] == '\0' || *ep != '\0' || tmo < 0) { > + fprintf(stderr, "ERROR: invalid timeout value\n"); > + return 1; > + } > + } > > - process(fd, DEFAULT_REPLY_TIMEOUT + 100); > + fd = mpath_connect(); > + if (fd == -1) { > + fprintf(stderr, "ERROR: failed to connect to multipathd\n"); > + return 1; > + } > + > + process(fd, tmo); > mpath_disconnect(fd); > return 0; > } > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 12/12] multipathd: fix use-after-free in handle_path_wwid_change()
On Tue, Aug 30, 2022 at 09:27:13PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Found by coverity (defect #380536). > > Fixes: b4eb57e ("libmultipath, multipathd: log failure setting sysfs > attributes") Reviewed-by: Benjamin Marzinski > --- > multipathd/main.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 66177cd..2d0a7bc 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -951,10 +951,12 @@ handle_path_wwid_change(struct path *pp, struct vectors > *vecs) > struct udev_device *udd; > static const char add[] = "add"; > ssize_t ret; > + char dev[FILE_NAME_SIZE]; > > if (!pp || !pp->udev) > return; > > + strlcpy(dev, pp->dev, sizeof(dev)); > udd = udev_device_ref(pp->udev); > if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) && pp->mpp) { > pp->dmstate = PSTATE_FAILED; > @@ -965,8 +967,7 @@ handle_path_wwid_change(struct path *pp, struct vectors > *vecs) > udev_device_unref(udd); > if (ret != sizeof(add) - 1) > log_sysfs_attr_set_value(1, ret, > - "%s: failed to trigger add event", > - pp->dev); > + "%s: failed to trigger add event", > dev); > } > > bool > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 09/12] multipathc: add new interactive client program
On Tue, Aug 30, 2022 at 09:27:10PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Add a new small program, multipathc, that acts as interactive > uxsock client for multipathd. multipathc is the only program > that has an interactive mode and can thus link to either libreadline > or libedit for command line history. All code depending on libreadline > is moved from uxclnt.c and cli.c to multipathc.c. > > This patch breaks multipathd's interactive mode. It will be restored > in the next patch. > > As multipathc doesn't link to libmultipath, it can link to libreadline > without license conflict. > > Signed-off-by: Martin Wilck > Reviewed-by: Benjamin Marzinski Reviewed-by: Benjamin Marzinski > --- > .gitignore | 1 + > multipathd/Makefile | 24 ++-- > multipathd/cli.c| 130 ++-- > multipathd/cli.h| 7 +- > multipathd/multipathc.c | 259 > multipathd/uxclnt.c | 131 +--- > 6 files changed, 299 insertions(+), 253 deletions(-) > create mode 100644 multipathd/multipathc.c > > diff --git a/.gitignore b/.gitignore > index b88608c..821c3e6 100644 > --- a/.gitignore > +++ b/.gitignore > @@ -13,6 +13,7 @@ cscope.out > kpartx/kpartx > multipath/multipath > multipathd/multipathd > +multipathd/multipathc > mpathpersist/mpathpersist > abi.tar.gz > abi > diff --git a/multipathd/Makefile b/multipathd/Makefile > index 7128510..19ab2e9 100644 > --- a/multipathd/Makefile > +++ b/multipathd/Makefile > @@ -27,12 +27,12 @@ LIBDEPS += -L$(multipathdir) -lmultipath > -L$(mpathpersistdir) -lmpathpersist \ > > > ifeq ($(READLINE),libedit) > -CPPFLAGS += -DUSE_LIBEDIT > -LIBDEPS += -ledit > +RL_CPPFLAGS = -DUSE_LIBEDIT > +RL_LIBDEPS += -ledit > endif > ifeq ($(READLINE),libreadline) > -CPPFLAGS += -DUSE_LIBREADLINE > -LIBDEPS += -lreadline > +RL_CPPFLAGS += -DUSE_LIBREADLINE > +RL_LIBDEPS += -lreadline > endif > > ifdef SYSTEMD > @@ -50,6 +50,8 @@ endif > OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o waiter.o \ > dmevents.o init_unwinder.o > > +CLI_OBJS = multipathc.o cli.o > + > ifeq ($(FPIN_SUPPORT),1) > OBJS += fpin_handlers.o > endif > @@ -57,18 +59,26 @@ endif > > > EXEC = multipathd > +CLI = multipathc > > -all : $(EXEC) > +all : $(EXEC) $(CLI) > > $(EXEC): $(OBJS) $(multipathdir)/libmultipath.so > $(mpathcmddir)/libmpathcmd.so > $(CC) $(CFLAGS) $(OBJS) $(LDFLAGS) -o $(EXEC) $(LIBDEPS) > > +multipathc.o:multipathc.c > + $(CC) $(CPPFLAGS) $(RL_CPPFLAGS) $(CFLAGS) -Wno-unused-parameter -c -o > $@ $< > + > +$(CLI): $(CLI_OBJS) > + $(CC) $(CFLAGS) $(CLI_OBJS) $(LDFLAGS) -o $@ $(CLI_LIBDEPS) > $(RL_LIBDEPS) > + > cli_handlers.o: cli_handlers.c > $(CC) $(CPPFLAGS) $(CFLAGS) -Wno-unused-parameter -c -o $@ $< > > install: > $(INSTALL_PROGRAM) -d $(DESTDIR)$(bindir) > $(INSTALL_PROGRAM) -m 755 $(EXEC) $(DESTDIR)$(bindir) > + $(INSTALL_PROGRAM) -m 755 $(CLI) $(DESTDIR)$(bindir) > ifdef SYSTEMD > $(INSTALL_PROGRAM) -d $(DESTDIR)$(unitdir) > $(INSTALL_PROGRAM) -m 644 $(EXEC).service $(DESTDIR)$(unitdir) > @@ -78,13 +88,13 @@ endif > $(INSTALL_PROGRAM) -m 644 $(EXEC).8 $(DESTDIR)$(man8dir) > > uninstall: > - $(RM) $(DESTDIR)$(bindir)/$(EXEC) > + $(RM) $(DESTDIR)$(bindir)/$(EXEC) $(DESTDIR)$(bindir)/$(CLI) > $(RM) $(DESTDIR)$(man8dir)/$(EXEC).8 > $(RM) $(DESTDIR)$(unitdir)/$(EXEC).service > $(RM) $(DESTDIR)$(unitdir)/$(EXEC).socket > > clean: dep_clean > - $(RM) core *.o $(EXEC) > + $(RM) core *.o $(EXEC) $(CLI) > > include $(wildcard $(OBJS:.o=.d)) > > diff --git a/multipathd/cli.c b/multipathd/cli.c > index cc56950..d1bfeee 100644 > --- a/multipathd/cli.c > +++ b/multipathd/cli.c > @@ -11,12 +11,6 @@ > #include "parser.h" > #include "util.h" > #include "version.h" > -#ifdef USE_LIBEDIT > -#include > -#endif > -#ifdef USE_LIBREADLINE > -#include > -#endif > > #include "mpath_cmd.h" > #include "cli.h" > @@ -26,6 +20,16 @@ > static vector keys; > static vector handlers; > > +vector get_keys(void) > +{ > + return keys; > +} > + > +vector get_handlers(void) > +{ > + return handlers; > +} > + > static struct key * > alloc_key (void) > { > @@ -225,8 +229,7 @@ load_keys (void) > return 0; > } > > -static struct key * > -find_key (const char * str) > +struct key *find_key (const char * str
Re: [dm-devel] [PATCH v4 01/12] multipathd: replace libreadline with getline()
On Tue, Aug 30, 2022 at 09:27:02PM +0200, mwi...@suse.com wrote: > From: Hannes Reinecke > > libreadline changed the license to be incompatible with multipath-tools > usage, so replace it with a simple getline(). > > mwilck: Make this the default option via Makefile.inc; it is used if > READLINE is unset. Compiling with READLINE=libreadline or READLINE=libedit > remains possible. > > Signed-off-by: Hannes Reinecke > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > Makefile.inc| 4 ++-- > multipathd/cli.c| 2 ++ > multipathd/cli.h| 4 +++- > multipathd/uxclnt.c | 49 + > 4 files changed, 43 insertions(+), 16 deletions(-) > > diff --git a/Makefile.inc b/Makefile.inc > index ad7afd0..4b32fa7 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -9,10 +9,10 @@ > # Uncomment to disable dmevents polling support > # ENABLE_DMEVENTS_POLL = 0 > # > -# Readline library to use, libedit or libreadline > +# Readline library to use, libedit, libreadline, or empty > # Caution: Using libreadline may make the multipathd binary undistributable, > # see https://github.com/opensvc/multipath-tools/issues/36 > -READLINE = libedit > +READLINE := > > # List of scsi device handler modules to load on boot, e.g. > # SCSI_DH_MODULES_PRELOAD := scsi_dh_alua scsi_dh_rdac > diff --git a/multipathd/cli.c b/multipathd/cli.c > index fa482a6..cc56950 100644 > --- a/multipathd/cli.c > +++ b/multipathd/cli.c > @@ -459,6 +459,7 @@ void cli_exit(void) > keys = NULL; > } > > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > static int > key_match_fingerprint (struct key * kw, uint64_t fp) > { > @@ -564,3 +565,4 @@ key_generator (const char * str, int state) >*/ > return ((char *)NULL); > } > +#endif > diff --git a/multipathd/cli.h b/multipathd/cli.h > index a6082ac..2a0c102 100644 > --- a/multipathd/cli.h > +++ b/multipathd/cli.h > @@ -151,6 +151,8 @@ void free_keys (vector vec); > void free_handlers (void); > int cli_init (void); > void cli_exit(void); > -char * key_generator (const char * str, int state); > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > +char *key_generator (const char * str, int state); > +#endif > > #endif /* _CLI_H_ */ > diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c > index 251e7d7..deff565 100644 > --- a/multipathd/uxclnt.c > +++ b/multipathd/uxclnt.c > @@ -30,6 +30,7 @@ > #include "defaults.h" > > #include "vector.h" > +#include "util.h" > #include "cli.h" > #include "uxclnt.h" > > @@ -77,35 +78,57 @@ static int need_quit(char *str, size_t len) > */ > static void process(int fd, unsigned int timeout) > { > - char *line; > - char *reply; > - int ret; > > - cli_init(); > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > rl_readline_name = "multipathd"; > rl_completion_entry_function = key_generator; > - while ((line = readline("multipathd> "))) { > - size_t llen = strlen(line); > +#endif > > - if (!llen) { > - free(line); > + cli_init(); > + for(;;) > + { > + char *line __attribute__((cleanup(cleanup_charp))) = NULL; > + char *reply __attribute__((cleanup(cleanup_charp))) = NULL; > + ssize_t llen; > + int ret; > + > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > + line = readline("multipathd> "); > + if (!line) > + break; > + llen = strlen(line); > + if (!llen) > continue; > +#else > + size_t lsize = 0; > + > + fputs("multipathd> ", stdout); > + errno = 0; > + llen = getline(, , stdin); > + if (llen == -1) { > + if (errno != 0) > + fprintf(stderr, "Error in getline: %m"); > + break; > } > + if (!llen || !strcmp(line, "\n")) > + continue; > +#endif > > if (need_quit(line, llen)) > break; > > - if (send_packet(fd, line) != 0) break; > + if (send_packet(fd, line) != 0) > + break; > ret = recv_packet(fd, , timeout); > - if (ret != 0) break; > + if (ret != 0) > + break; > > print_reply(reply); > > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > if (line && *line) > add_history(line); > - > - free(line); > - free(reply); > +#endif > } > } > > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v4 0/5] multipath: optimizations for large mptable
On Fri, Aug 26, 2022 at 08:05:51PM +0200, mwi...@suse.com wrote: > From: Martin Wilck For the set: Reviewed-by: Benjamin Marzinski > > We observe that multipath operations take a long time if the multipaths > section in multipath.conf contains a lot of alias settings > (1+ in our case). This hurts in particular in udev rules, when > multipath -u or multipath -U is invoked, but also for command line > invocations like "multipath -ll". > > This series provides a few optimizations for this use case. It speeds > up simple multipath operations in the test case by a factor of 20. > > Changes v3->v4: > > 02: more compilation fixes for msort.c to make it pass CI > (only re-posting this patch) > > Changes v2->v3, after discussion with Benjamin Marzinski: > > 01, 02: added msort.c from glibc and adapted to our needs. > Numbering changes accordingly. > 03, 04: (was 01, 02): remove pointer comparisons from v2 again, this was a > dumb idea. Use the stable msort algorithm instead. > > Changes wrt v1, after suggestions from Benjamin Marzinski: > > 01, 02: Use pointer comparisons to achieve stable sorting with qsort > 02: Fix return without popping the cleanup handler. The way I fixed this > leaves the possibility that some memory won't be freed if a thread is > killed while executing vector_convert(). I think this is acceptible; > avoiding it would complicate the code, with very small benefit. > 02: Remove unnecessary checks and break loop if alias==NULL is encountered. > > Martin Wilck (5): > libmultipath: add msort.c from glibc > libmultipath: modifications for msort.c > libmultipath: merge_mptable(): sort table by wwid > libmultipath: check_alias_settings(): pre-sort mptable by alias > multipath: optimize program startup for frequent invocations > > libmultipath/Makefile | 2 +- > libmultipath/alias.c | 37 +- > libmultipath/config.c | 15 ++- > libmultipath/msort.c | 271 ++ > libmultipath/msort.h | 6 + > libmultipath/vector.c | 9 ++ > libmultipath/vector.h | 1 + > multipath/main.c | 33 ++--- > 8 files changed, 352 insertions(+), 22 deletions(-) > create mode 100644 libmultipath/msort.c > create mode 100644 libmultipath/msort.h > > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] multipathd: check for command overflow
The code to build the command string calls snprintf() in a loop, adding the return value to the start pointer, c. Since snprintf() can return more characters than it actually printed if it runs out of space, c can end up pointing past the end of the command string buffer on sebsequent loops. Since the size argument to snprintf() is unsigned, after an overflow it will be a huge number, instead of a negative one, meaning that it will continue printing past the end of the buffer. Check for overflow after each snprintf() to avoid this. Signed-off-by: Benjamin Marzinski --- multipathd/main.c | 4 1 file changed, 4 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index 5a408945..1032fb2a 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -3707,6 +3707,10 @@ main (int argc, char *argv[]) c += snprintf(c, s + CMDSIZE - c, "%s ", argv[optind]); optind++; + if (c >= s + CMDSIZE) { + fprintf(stderr, "multipathd command too large\n"); + exit(1); + } } c += snprintf(c, s + CMDSIZE - c, "\n"); } -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v3 00/11] Split libmultipath and libmpathutil
On Mon, Aug 22, 2022 at 11:22:49PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > The CI found a few more glitches in my v2 patch set. Not reposting > the entire set here, just the changed patches. For all except 01/11 & 10/11: Reviewed-by: Benjamin Marzinski > > Changes v2->v3: > fix compilation errors on Alpine an CI errors. > > > Hannes Reinecke (1): > multipathd: replace libreadline with getline() > > Martin Wilck (10): > .gitignore: ignore generated ABI files > libmultipath: move all reservation key functions to prkey.c > libmultipath: always set _GNU_SOURCE > multipath-tools: Makefile: fix dependencies for "install" target > libmultipath checkers/prioritizers: search for includes in > libmultipath > libmultipath: remove weak attribute for {get,put}_multipath_config > libmultipath: split off libmpathutil > multipathc: add new interactive client program > multipathd: exec multipathc in interactive mode > multipathd: fix incompatible pointer type error with libedit > > .gitignore | 3 + > Makefile | 8 +- > Makefile.inc | 5 +- > libmpathpersist/Makefile | 6 +- > libmpathutil/Makefile| 70 + > {libmultipath => libmpathutil}/debug.c | 0 > {libmultipath => libmpathutil}/debug.h | 0 > libmpathutil/globals.c | 12 + > libmpathutil/globals.h | 39 +++ > libmpathutil/libmpathutil.version| 122 + > {libmultipath => libmpathutil}/log.c | 0 > {libmultipath => libmpathutil}/log.h | 0 > {libmultipath => libmpathutil}/log_pthread.c | 0 > {libmultipath => libmpathutil}/log_pthread.h | 0 > {libmultipath => libmpathutil}/parser.c | 0 > {libmultipath => libmpathutil}/parser.h | 2 +- > {libmultipath => libmpathutil}/strbuf.c | 0 > {libmultipath => libmpathutil}/strbuf.h | 0 > {libmultipath => libmpathutil}/time-util.c | 0 > {libmultipath => libmpathutil}/time-util.h | 0 > {libmultipath => libmpathutil}/util.c| 32 --- > {libmultipath => libmpathutil}/util.h| 2 - > {libmultipath => libmpathutil}/uxsock.c | 0 > {libmultipath => libmpathutil}/uxsock.h | 0 > {libmultipath => libmpathutil}/vector.c | 0 > {libmultipath => libmpathutil}/vector.h | 0 > libmpathvalid/Makefile | 6 +- > libmultipath/Makefile| 17 +- > libmultipath/checkers/Makefile | 7 +- > libmultipath/checkers/directio.c | 4 +- > libmultipath/checkers/emc_clariion.c | 2 +- > libmultipath/checkers/hp_sw.c| 4 +- > libmultipath/checkers/rdac.c | 2 +- > libmultipath/checkers/tur.c | 8 +- > libmultipath/config.c| 4 +- > libmultipath/config.h| 14 +- > libmultipath/dict.c | 16 +- > libmultipath/dict.h | 2 - > libmultipath/foreign/Makefile| 6 +- > libmultipath/libmultipath.version| 41 --- > libmultipath/prioritizers/Makefile | 8 +- > libmultipath/prioritizers/alua_rtpg.c| 2 +- > libmultipath/prkey.c | 49 +++- > libmultipath/prkey.h | 3 + > mpathpersist/Makefile| 4 +- > multipath/Makefile | 6 +- > multipathd/Makefile | 43 ++- > multipathd/cli.c | 128 + > multipathd/cli.h | 5 +- > multipathd/main.c| 15 +- > multipathd/multipathc.c | 271 +++ > multipathd/uxclnt.c | 108 +--- > tests/Makefile | 12 +- > 53 files changed, 693 insertions(+), 395 deletions(-) > create mode 100644 libmpathutil/Makefile > rename {libmultipath => libmpathutil}/debug.c (100%) > rename {libmultipath => libmpathutil}/debug.h (100%) > create mode 100644 libmpathutil/globals.c > create mode 100644 libmpathutil/globals.h > create mode 100644 libmpathutil/libmpathutil.version > rename {libmultipath => libmpathutil}/log.c (100%) > rename {libmultipath => libmpathutil}/log.h (100%) > rename {libmultipath => libmpathutil}/log_pthread.c (100%) > rename {libmultipath => libmpathutil}/log_pthread.h (100%)
Re: [dm-devel] [PATCH v2 10/11] multipathd: exec multipathc in interactive mode
On Mon, Aug 22, 2022 at 10:41:18PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > A previous patch disabled interactive mode in multipathd, because > uxclnt() would return immediately without an input command > > With this patch, we reinstate interactive mode for "multipath -k", > by just exec()ing the multipathc client. > > Signed-off-by: Martin Wilck > --- > multipathd/Makefile | 3 ++- > multipathd/main.c | 15 +-- > multipathd/multipathc.c | 25 + > 3 files changed, 36 insertions(+), 7 deletions(-) > > diff --git a/multipathd/Makefile b/multipathd/Makefile > index 19ab2e9..8a56304 100644 > --- a/multipathd/Makefile > +++ b/multipathd/Makefile > @@ -17,7 +17,8 @@ endif > > CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathpersistdir) > -I$(mpathcmddir) -I$(thirdpartydir) \ > $(shell $(PKGCONFIG) --modversion liburcu 2>/dev/null | \ > - awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + > $$2) + $$3); }') > + awk -F. '{ printf("-DURCU_VERSION=0x%06x", 256 * ( 256 * $$1 + > $$2) + $$3); }') \ > + -DBINDIR='"$(bindir)"' > CFLAGS += $(BIN_CFLAGS) > LDFLAGS += $(BIN_LDFLAGS) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 5a40894..63df643 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -3616,7 +3616,7 @@ main (int argc, char *argv[]) > extern char *optarg; > extern int optind; > int arg; > - int err; > + int err = 0; > int foreground = 0; > struct config *conf; > char *opt_k_arg = NULL; > @@ -3710,7 +3710,18 @@ main (int argc, char *argv[]) > } > c += snprintf(c, s + CMDSIZE - c, "\n"); > } > - err = uxclnt(s, uxsock_timeout + 100); > + if (!s) { > + char tmo_buf[16]; > + > + snprintf(tmo_buf, sizeof(tmo_buf), "%d", > + uxsock_timeout + 100); > + if (execl(BINDIR "/multipathc", "multipathc", > + tmo_buf, NULL) == -1) { > + condlog(0, "ERROR: failed to execute > multipathc"); We should probably print the error code here. > + err = 1; > + } > + } else > + err = uxclnt(s, uxsock_timeout + 100); > free_config(conf); > return err; > } > diff --git a/multipathd/multipathc.c b/multipathd/multipathc.c > index 571a182..323bd78 100644 > --- a/multipathd/multipathc.c > +++ b/multipathd/multipathc.c > @@ -241,14 +241,31 @@ static void process(int fd, unsigned int timeout) > } > } > > -int main (void) > +int main (int argc, const char * const argv[]) > { > - int fd = mpath_connect(); > + int fd; > + int tmo = DEFAULT_REPLY_TIMEOUT + 100; > + char *ep; > > - if (fd == -1) > + if (argc > 2) { > + fprintf(stderr, "Usage: %s [timeout]\n", argv[0]); > return 1; > + } > + if (argc == 2) { > + tmo = strtol(argv[1], , 10); strtol() can return a negative number, but process() takes an unsigned int. We should probably either use strtoul() or check for negative tmo, and error. -Ben > + if (*argv[1] == '\0' || *ep != '\0') { > + fprintf(stderr, "ERROR: invalid timeout value\n"); > + return 1; > + } > + } > > - process(fd, DEFAULT_REPLY_TIMEOUT + 100); > + fd = mpath_connect(); > + if (fd == -1) { > + fprintf(stderr, "ERROR: failed to connect to multipathd\n"); > + return 1; > + } > + > + process(fd, tmo); > mpath_disconnect(fd); > return 0; > } > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 01/11] multipathd: replace libreadline with getline()
On Mon, Aug 22, 2022 at 10:41:09PM +0200, mwi...@suse.com wrote: > From: Hannes Reinecke > > libreadline changed the license to be incompatible with multipath-tools > usage, so replace it with a simple getline(). > > mwilck: Make this the default option via Makefile.inc; it is used if > READLINE is unset. Compiling with READLINE=libreadline or READLINE=libedit > remains possible. > > Signed-off-by: Hannes Reinecke > Signed-off-by: Martin Wilck > --- > Makefile.inc| 4 ++-- > multipathd/cli.c| 2 ++ > multipathd/uxclnt.c | 50 ++--- > 3 files changed, 38 insertions(+), 18 deletions(-) > > diff --git a/Makefile.inc b/Makefile.inc > index ad7afd0..0653d21 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -9,10 +9,10 @@ > # Uncomment to disable dmevents polling support > # ENABLE_DMEVENTS_POLL = 0 > # > -# Readline library to use, libedit or libreadline > +# Readline library to use, libedit, libreadline, or empty > # Caution: Using libreadline may make the multipathd binary undistributable, > # see https://github.com/opensvc/multipath-tools/issues/36 > -READLINE = libedit > +READLINE := Trailing whitespace nitpick. > > # List of scsi device handler modules to load on boot, e.g. > # SCSI_DH_MODULES_PRELOAD := scsi_dh_alua scsi_dh_rdac > diff --git a/multipathd/cli.c b/multipathd/cli.c > index fa482a6..cc56950 100644 > --- a/multipathd/cli.c > +++ b/multipathd/cli.c > @@ -459,6 +459,7 @@ void cli_exit(void) > keys = NULL; > } > > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > static int > key_match_fingerprint (struct key * kw, uint64_t fp) > { > @@ -564,3 +565,4 @@ key_generator (const char * str, int state) >*/ > return ((char *)NULL); > } > +#endif It's kind of odd to not define key_generator(), but leave it defined in cli.h if no library is defined. > diff --git a/multipathd/uxclnt.c b/multipathd/uxclnt.c > index 251e7d7..b817bea 100644 > --- a/multipathd/uxclnt.c > +++ b/multipathd/uxclnt.c > @@ -30,6 +30,7 @@ > #include "defaults.h" > > #include "vector.h" > +#include "util.h" > #include "cli.h" > #include "uxclnt.h" > > @@ -77,35 +78,52 @@ static int need_quit(char *str, size_t len) > */ > static void process(int fd, unsigned int timeout) > { > - char *line; > - char *reply; > - int ret; > > - cli_init(); > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > rl_readline_name = "multipathd"; > rl_completion_entry_function = key_generator; > - while ((line = readline("multipathd> "))) { > - size_t llen = strlen(line); > +#endif > > - if (!llen) { > - free(line); > + cli_init(); > + for(;;) > + { > + char *line __attribute__((cleanup(cleanup_charp))) = NULL; > + char *reply __attribute__((cleanup(cleanup_charp))) = NULL; > + ssize_t llen; > + int ret; > + > +#if defined(USE_LIBREADLINE) || defined(USE_LIBEDIT) > + line = readline("multipathd> "); > + if (!line) > + break; > + llen = strlen(line); > + if (!llen) > continue; > +#else > + size_t lsize = 0; > + > + fputs("multipathd> ", stdout); > + errno = 0; > + llen = getline(, , stdin); > + if (llen == -1) { > + if (errno != 0) > + fprintf(stderr, "Error in getline: %m"); > + break; > } > + if (!llen || !strcmp(line, "\n")) > + continue; > +#endif > > if (need_quit(line, llen)) > break; > > - if (send_packet(fd, line) != 0) break; > + if (send_packet(fd, line) != 0) > + break; > ret = recv_packet(fd, , timeout); > - if (ret != 0) break; > + if (ret != 0) > + break; > > print_reply(reply); > - > - if (line && *line) > - add_history(line); > - > - free(line); > - free(reply); Why drop add_history() support even if a library is defined? -Ben > } > } > > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 1/3] libmultipath: merge_mptable(): sort table by wwid
On Thu, Aug 25, 2022 at 09:44:22AM +0200, Martin Wilck wrote: > On Wed, 2022-08-24 at 19:02 +0200, Martin Wilck wrote: > > On Wed, 2022-08-24 at 11:16 -0500, Benjamin Marzinski wrote: > > > On Wed, Aug 24, 2022 at 09:07:56AM +0200, Martin Wilck wrote: > > > > On Tue, 2022-08-23 at 15:48 -0500, Benjamin Marzinski wrote: > > > > > On Thu, Aug 18, 2022 at 11:06:28PM +0200, > > > > > mwi...@suse.com wrote: > > > > > > From: Martin Wilck > > > > > > > > > > > > If the mptable is very large (for example, in a configuration > > > > > > with > > > > > > lots of maps assigned individual aliases), merge_mptable may > > > > > > get > > > > > > very slow because it needs to make O(n^2) string comparisons > > > > > > (with > > > > > > n being the number of mptable entries). WWID strings often > > > > > > differ > > > > > > only in the last few bytes, causing a lot of CPU time spent > > > > > > in > > > > > > strcmp(). > > > > > > > > > > > > merge_mptable is executed whenever multipath or multipathd is > > > > > > started, that > > > > > > is, also for "multipath -u" and "multipath -U" invocations > > > > > > from > > > > > > udev rules. > > > > > > Optimize it by sorting the mptable before merging. This way > > > > > > we > > > > > > don't need > > > > > > to iterate towards the end of the vector searching for > > > > > > duplicates. > > > > > > > > > > > > Signed-off-by: Martin Wilck > > > > > > --- > > > > > > libmultipath/config.c | 15 +-- > > > > > > libmultipath/vector.c | 8 > > > > > > libmultipath/vector.h | 1 + > > > > > > 3 files changed, 22 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/libmultipath/config.c b/libmultipath/config.c > > > > > > index ab8b26e..a2c79a4 100644 > > > > > > --- a/libmultipath/config.c > > > > > > +++ b/libmultipath/config.c > > > > > > @@ -520,6 +520,14 @@ merge_mpe(struct mpentry *dst, struct > > > > > > mpentry > > > > > > *src) > > > > > > merge_num(mode); > > > > > > } > > > > > > > > > > > > +static int wwid_compar(const void *p1, const void *p2) > > > > > > +{ > > > > > > + const char *wwid1 = (*(struct mpentry * const *)p1)- > > > > > > > wwid; > > > > > > + const char *wwid2 = (*(struct mpentry * const *)p2)- > > > > > > > wwid; > > > > > > + > > > > > > + return strcmp(wwid1, wwid2); > > > > > > +} > > > > > > + > > > > > > void merge_mptable(vector mptable) > > > > > > { > > > > > > struct mpentry *mp1, *mp2; > > > > > > @@ -533,10 +541,13 @@ void merge_mptable(vector mptable) > > > > > > free_mpe(mp1); > > > > > > continue; > > > > > > } > > > > > > + } > > > > > > + vector_sort(mptable, wwid_compar); > > > > > > + vector_foreach_slot(mptable, mp1, i) { > > > > > > j = i + 1; > > > > > > vector_foreach_slot_after(mptable, mp2, j) { > > > > > > - if (!mp2->wwid || strcmp(mp1->wwid, > > > > > > mp2- > > > > > > > wwid)) > > > > > > - continue; > > > > > > + if (strcmp(mp1->wwid, mp2->wwid)) > > > > > > + break; > > > > > > condlog(1, "%s: duplicate multipath > > > > > > config > > > > > > section for %s", > > > > > > __func__, mp1->wwid); > > > > > > merge_mpe(mp2, mp1); > > > > > > > > > > The way merge_mpe() works, valu
Re: [dm-devel] [PATCH 1/3] libmultipath: merge_mptable(): sort table by wwid
On Wed, Aug 24, 2022 at 09:07:56AM +0200, Martin Wilck wrote: > On Tue, 2022-08-23 at 15:48 -0500, Benjamin Marzinski wrote: > > On Thu, Aug 18, 2022 at 11:06:28PM +0200, mwi...@suse.com wrote: > > > From: Martin Wilck > > > > > > If the mptable is very large (for example, in a configuration with > > > lots of maps assigned individual aliases), merge_mptable may get > > > very slow because it needs to make O(n^2) string comparisons (with > > > n being the number of mptable entries). WWID strings often differ > > > only in the last few bytes, causing a lot of CPU time spent in > > > strcmp(). > > > > > > merge_mptable is executed whenever multipath or multipathd is > > > started, that > > > is, also for "multipath -u" and "multipath -U" invocations from > > > udev rules. > > > Optimize it by sorting the mptable before merging. This way we > > > don't need > > > to iterate towards the end of the vector searching for duplicates. > > > > > > Signed-off-by: Martin Wilck > > > --- > > > libmultipath/config.c | 15 +-- > > > libmultipath/vector.c | 8 > > > libmultipath/vector.h | 1 + > > > 3 files changed, 22 insertions(+), 2 deletions(-) > > > > > > diff --git a/libmultipath/config.c b/libmultipath/config.c > > > index ab8b26e..a2c79a4 100644 > > > --- a/libmultipath/config.c > > > +++ b/libmultipath/config.c > > > @@ -520,6 +520,14 @@ merge_mpe(struct mpentry *dst, struct mpentry > > > *src) > > > merge_num(mode); > > > } > > > > > > +static int wwid_compar(const void *p1, const void *p2) > > > +{ > > > + const char *wwid1 = (*(struct mpentry * const *)p1)->wwid; > > > + const char *wwid2 = (*(struct mpentry * const *)p2)->wwid; > > > + > > > + return strcmp(wwid1, wwid2); > > > +} > > > + > > > void merge_mptable(vector mptable) > > > { > > > struct mpentry *mp1, *mp2; > > > @@ -533,10 +541,13 @@ void merge_mptable(vector mptable) > > > free_mpe(mp1); > > > continue; > > > } > > > + } > > > + vector_sort(mptable, wwid_compar); > > > + vector_foreach_slot(mptable, mp1, i) { > > > j = i + 1; > > > vector_foreach_slot_after(mptable, mp2, j) { > > > - if (!mp2->wwid || strcmp(mp1->wwid, mp2- > > > >wwid)) > > > - continue; > > > + if (strcmp(mp1->wwid, mp2->wwid)) > > > + break; > > > condlog(1, "%s: duplicate multipath config > > > section for %s", > > > __func__, mp1->wwid); > > > merge_mpe(mp2, mp1); > > > > The way merge_mpe() works, values are only copied from mp1's > > variables > > if the corresponding variable is unset in mp2. This requires the > > original order of mp1 and mp2 to be unchanged by the sorting > > algorithm, > > but according to the qsort() man page "If two members compare as > > equal, > > their order in the sorted array is undefined." > > > > One quick and dirty way we could fix this is to add a variable to > > struct > > mptable called config_idx, which is its index in the config file. If > > the wwids are equal, you compare that. > > Thanks for pointing this out. I believe it's easier than that: as we're > passed the offsets into the array (aka struct mpentry **), we can > simply compare p1 and p2 if the strings are equal. > > Agree? I don't think so. Comparing the array offsets assumes that two mpentries are still ordered correctly when they are compared against each other. But the way quick sort works, array elements can get swapped around multiple times, based on whether they are larger or smaller than some pivot point. It's possible that the two mpentries already switched order before they were compared. Essentially, all comparing the offset does is force qsort to not switch the place of two otherwise equal entries. But for speed reasons alone, there is no reason why qsort would bother to swap the location of two equal entries. Here's an example of what could go wrong: Say you start with the array (I'm also tracking the mpentry's original config index) array offset: 0 1 2
Re: [dm-devel] [PATCH 3/3] multipath: optimize program startup for frequent invocations
On Thu, Aug 18, 2022 at 11:06:30PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Neither "multipath -u" nor "multipath -U" need initialization of the > prioritizers, checkers, and foreign libraries. Also, these commands > need not fail if the bindings file is inconsistent. Move these > possibly slow initialization steps after these special command > invocations. > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > multipath/main.c | 33 + > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/multipath/main.c b/multipath/main.c > index 034dd2f..8e5154a 100644 > --- a/multipath/main.c > +++ b/multipath/main.c > @@ -957,11 +957,6 @@ main (int argc, char *argv[]) > exit(RTVL_FAIL); > } > > - if (check_alias_settings(conf)) { > - fprintf(stderr, "fatal configuration error, aborting"); > - exit(RTVL_FAIL); > - } > - > if (optind < argc) { > dev = calloc(1, FILE_NAME_SIZE); > > @@ -988,20 +983,9 @@ main (int argc, char *argv[]) > > libmp_udev_set_sync_support(1); > > - if (init_checkers()) { > - condlog(0, "failed to initialize checkers"); > - goto out; > - } > - if (init_prio()) { > - condlog(0, "failed to initialize prioritizers"); > - goto out; > - } > - > if ((cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) && enable_foreign) > conf->enable_foreign = strdup(""); > > - /* Failing here is non-fatal */ > - init_foreign(conf->enable_foreign); > if (cmd == CMD_USABLE_PATHS) { > r = check_usable_paths(conf, dev, dev_type) ? > RTVL_FAIL : RTVL_OK; > @@ -1036,6 +1020,23 @@ main (int argc, char *argv[]) > break; > } > > + if (check_alias_settings(conf)) { > + fprintf(stderr, "fatal configuration error, aborting"); > + exit(RTVL_FAIL); > + } > + > + if (init_checkers()) { > + condlog(0, "failed to initialize checkers"); > + goto out; > + } > + if (init_prio()) { > + condlog(0, "failed to initialize prioritizers"); > + goto out; > + } > + > + /* Failing here is non-fatal */ > + init_foreign(conf->enable_foreign); > + > if (cmd == CMD_RESET_WWIDS) { > struct multipath * mpp; > int i; > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 2/3] libmultipath: check_alias_settings(): pre-sort mptable by alias
On Thu, Aug 18, 2022 at 11:06:29PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > add_binding() contains an optimization; it assumes that the list of > bindings is alphabetically sorted by alias, and tries to maintain > this order. > > But conf->mptable is sorted by wwid. Therefore check_alias_settings() may take > a long time if the mptable is large. > > Create a copy of the mptable, sorted by alias, and use it for add_bindings(). > This speeds up check_alias_settings by roughly a factor of 10 (0.1s vs. 1s) > for my test setup with 1 entries in the mptable. > > Signed-off-by: Martin Wilck > --- > libmultipath/alias.c | 29 - > 1 file changed, 28 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > index 548a118..60428fe 100644 > --- a/libmultipath/alias.c > +++ b/libmultipath/alias.c > @@ -672,6 +672,26 @@ static void cleanup_fclose(void *p) > fclose(p); > } > > +static int alias_compar(const void *p1, const void *p2) > +{ > + const char *alias1 = (*(struct mpentry * const *)p1)->alias; > + const char *alias2 = (*(struct mpentry * const *)p2)->alias; > + > + if (!alias1 && !alias2) > + return 0; > + if (!alias1) > + return 1; > + if (!alias2) > + return -1; > + return strcmp(alias1, alias2); > +} > + > +static void cleanup_vector_free(void *arg) > +{ > + if (arg) > + vector_free((vector)arg); > +} > + > /* > * check_alias_settings(): test for inconsistent alias configuration > * > @@ -693,10 +713,16 @@ int check_alias_settings(const struct config *conf) > int can_write; > int rc = 0, i, fd; > Bindings bindings = {.allocated = 0, }; > + vector mptable = NULL; > struct mpentry *mpe; > > pthread_cleanup_push_cast(free_bindings, ); > - vector_foreach_slot(conf->mptable, mpe, i) { > + pthread_cleanup_push(cleanup_vector_free, mptable); > + mptable = vector_convert(NULL, conf->mptable, struct mpentry *, > identity); > + if (!mptable) > + return -1; Are there other places in the code where we return without popping the cleanup handler? According to the man page "POSIX.1 says that the effect of using return, break, continue, or goto to prematurely leave a block bracketed pthread_cleanup_push() and pthread_cleanup_pop() is undefined. Portable applications should avoid doing this." It also says that linux implements these as macros that expand to create code blocks. So, I'm pretty sure this is safe in linux, but if we aren't already doing it, we probably shouldn't start, especially since vector_convert() doesn't have any pthread cancellation points, so we can just move the push until after we are sure we successfully copied the vector. > + vector_sort(mptable, alias_compar); > + vector_foreach_slot(mptable, mpe, i) { > if (!mpe->wwid || !mpe->alias) Unless I'm missing something, merge_mptable() should have already guaranteed that mpe->wwid must be non-NULL. Also, since mptable has all of the entries with a NULL alias sorted to the end, as soon as we hit one, we can stop checking. -Ben > continue; > if (add_binding(, mpe->alias, mpe->wwid) == > @@ -710,6 +736,7 @@ int check_alias_settings(const struct config *conf) > } > /* This clears the bindings */ > pthread_cleanup_pop(1); > + pthread_cleanup_pop(1); > > pthread_cleanup_push_cast(free_bindings, ); > fd = open_file(conf->bindings_file, _write, BINDINGS_FILE_HEADER); > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 1/3] libmultipath: merge_mptable(): sort table by wwid
On Thu, Aug 18, 2022 at 11:06:28PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > If the mptable is very large (for example, in a configuration with > lots of maps assigned individual aliases), merge_mptable may get > very slow because it needs to make O(n^2) string comparisons (with > n being the number of mptable entries). WWID strings often differ > only in the last few bytes, causing a lot of CPU time spent in > strcmp(). > > merge_mptable is executed whenever multipath or multipathd is started, that > is, also for "multipath -u" and "multipath -U" invocations from udev rules. > Optimize it by sorting the mptable before merging. This way we don't need > to iterate towards the end of the vector searching for duplicates. > > Signed-off-by: Martin Wilck > --- > libmultipath/config.c | 15 +-- > libmultipath/vector.c | 8 > libmultipath/vector.h | 1 + > 3 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index ab8b26e..a2c79a4 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -520,6 +520,14 @@ merge_mpe(struct mpentry *dst, struct mpentry *src) > merge_num(mode); > } > > +static int wwid_compar(const void *p1, const void *p2) > +{ > + const char *wwid1 = (*(struct mpentry * const *)p1)->wwid; > + const char *wwid2 = (*(struct mpentry * const *)p2)->wwid; > + > + return strcmp(wwid1, wwid2); > +} > + > void merge_mptable(vector mptable) > { > struct mpentry *mp1, *mp2; > @@ -533,10 +541,13 @@ void merge_mptable(vector mptable) > free_mpe(mp1); > continue; > } > + } > + vector_sort(mptable, wwid_compar); > + vector_foreach_slot(mptable, mp1, i) { > j = i + 1; > vector_foreach_slot_after(mptable, mp2, j) { > - if (!mp2->wwid || strcmp(mp1->wwid, mp2->wwid)) > - continue; > + if (strcmp(mp1->wwid, mp2->wwid)) > + break; > condlog(1, "%s: duplicate multipath config section for > %s", > __func__, mp1->wwid); > merge_mpe(mp2, mp1); The way merge_mpe() works, values are only copied from mp1's variables if the corresponding variable is unset in mp2. This requires the original order of mp1 and mp2 to be unchanged by the sorting algorithm, but according to the qsort() man page "If two members compare as equal, their order in the sorted array is undefined." One quick and dirty way we could fix this is to add a variable to struct mptable called config_idx, which is its index in the config file. If the wwids are equal, you compare that. Something like (only compile tested): diff --git a/libmultipath/config.c b/libmultipath/config.c index a2c79a48..a8e2620b 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -522,10 +522,15 @@ merge_mpe(struct mpentry *dst, struct mpentry *src) static int wwid_compar(const void *p1, const void *p2) { + int r; const char *wwid1 = (*(struct mpentry * const *)p1)->wwid; const char *wwid2 = (*(struct mpentry * const *)p2)->wwid; + int idx1 = (*(struct mpentry * const *)p1)->config_idx; + int idx2 = (*(struct mpentry * const *)p2)->config_idx; - return strcmp(wwid1, wwid2); + if ((r = strcmp(wwid1, wwid2)) != 0) + return r; + return idx1 - idx2; } void merge_mptable(vector mptable) @@ -541,6 +546,7 @@ void merge_mptable(vector mptable) free_mpe(mp1); continue; } + mp1->config_idx = i; } vector_sort(mptable, wwid_compar); vector_foreach_slot(mptable, mp1, i) { diff --git a/libmultipath/config.h b/libmultipath/config.h index fdcdff0a..fc44914c 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -133,6 +133,7 @@ struct mpentry { uid_t uid; gid_t gid; mode_t mode; + int config_idx; }; struct config { -Ben > diff --git a/libmultipath/vector.c b/libmultipath/vector.c > index e2d1ec9..0befe71 100644 > --- a/libmultipath/vector.c > +++ b/libmultipath/vector.c > @@ -208,3 +208,11 @@ int vector_find_or_add_slot(vector v, void *value) > vector_set_slot(v, value); > return VECTOR_SIZE(v) - 1; > } > + > +void vector_sort(vector v, int (*compar)(const void *, const void *)) > +{ > + if (!v || !v->slot || !v->allocated) > + return; > + return qsort((void *)v->slot, v->allocated, sizeof(void *), compar); > + > +} > diff --git a/libmultipath/vector.h b/libmultipath/vector.h > index 2862dc2..c0b09cb 100644 > --- a/libmultipath/vector.h > +++ b/libmultipath/vector.h > @@ -89,4 +89,5 @@ extern void vector_repack(vector v); > extern void vector_dump(vector v); > extern void dump_strvec(vector
Re: [dm-devel] [PATCH V2 3/6] multipathd: Occasionally allow waiters to interrupt checking paths
On Mon, Aug 22, 2022 at 04:15:01PM +, Martin Wilck wrote: > On Wed, 2022-08-17 at 15:48 -0500, Benjamin Marzinski wrote: > > If there are a very large number of paths that all get checked at the > > same time, it is possible for the path checkers to starve out other > > vecs->lock users, like uevent processing. To avoid this, if the path > > checkers are taking a long time, checkerloop now occasionally unlocks > > and allows other vecs->lock waiters to run. > > > > In order to make sure that path checking is always making real > > progress, > > checkerloop() only checks if it should unlock every 128 path checks. > > To > > check, it sees if there are waiters and more than a second has passed > > since it acquired the vecs->lock. If both of these are true, it drops > > the lock and calls nanosleep() to schedule. > > > > When checkerloop() reacquires the lock, it may no longer be at the > > correct place in the vector. While paths can be deleted from any > > point > > of the pathvec, they can only be added at the end. This means that > > the > > next path to check must be between its previous location and the > > start > > of the vector. To help determine the correct starting place, > > checkerloop() marks each path as not checked at the start of each > > checker loop. New paths start in the unchecked state. As paths are > > checked, they are marked as such. If the checker loop is interrupted, > > when it reacquires the lock, it will iterate backwards from the last > > location in checked to the start of the vector. The first path it > > finds > > that is marked as checked must be the last path it checked. > > > > Signed-off-by: Benjamin Marzinski > > --- > > libmultipath/structs.h | 1 + > > multipathd/main.c | 82 +--- > > -- > > 2 files changed, 67 insertions(+), 16 deletions(-) > > > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > > index a6a09441..9d4fb9c8 100644 > > --- a/libmultipath/structs.h > > +++ b/libmultipath/structs.h > > @@ -351,6 +351,7 @@ struct path { > > int fast_io_fail; > > unsigned int dev_loss; > > int eh_deadline; > > + bool is_checked; > > /* configlet pointers */ > > vector hwe; > > struct gen_path generic_path; > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 71079113..37c04018 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -2548,6 +2548,11 @@ check_path (struct vectors * vecs, struct path > > * pp, unsigned int ticks) > > } > > return 1; > > } > > +enum checker_state { > > + CHECKER_STARTING, > > + CHECKER_RUNNING, > > + CHECKER_FINISHED, > > +}; > > > > static void * > > checkerloop (void *ap) > > @@ -2555,7 +2560,6 @@ checkerloop (void *ap) > > struct vectors *vecs; > > struct path *pp; > > int count = 0; > > - unsigned int i; > > struct timespec last_time; > > struct config *conf; > > int foreign_tick = 0; > > @@ -2581,8 +2585,9 @@ checkerloop (void *ap) > > > > while (1) { > > struct timespec diff_time, start_time, end_time; > > - int num_paths = 0, strict_timing, rc = 0; > > + int num_paths = 0, strict_timing, rc = 0, i = 0; > > unsigned int ticks = 0; > > + enum checker_state checker_state = CHECKER_STARTING; > > > > if (set_config_state(DAEMON_RUNNING) != > > DAEMON_RUNNING) > > /* daemon shutdown */ > > @@ -2603,22 +2608,67 @@ checkerloop (void *ap) > > if (use_watchdog) > > sd_notify(0, "WATCHDOG=1"); > > #endif > > + while (checker_state != CHECKER_FINISHED) { > > + unsigned int paths_checked = 0; > > + struct timespec chk_start_time; > > > > - pthread_cleanup_push(cleanup_lock, >lock); > > - lock(>lock); > > - pthread_testcancel(); > > - vector_foreach_slot (vecs->pathvec, pp, i) { > > - rc = check_path(vecs, pp, ticks); > > - if (rc < 0) { > > - condlog(1, "%s: check_path() failed, > > removing", > > -
[dm-devel] [PATCH 3/3] multipathd: Handle losing all path in update_map
Its possible that when a multipath device is being updated, it will end up that all the paths for it are gone. This can happen if paths are added and then removed again before multipathd processes the uevent for the newly created multipath device. In this case multipathd wasn't taking the proper action for the case where all the paths had been removed. If flush_on_last_del was set, multipathd wasn't disabling flushing and if deferred_remove was set, it wasn't doing a deferred remove. Multipathd should call flush_map_nopaths(), just like ev_remove_path() does when the last path is removed. Signed-off-by: Benjamin Marzinski --- multipathd/main.c | 4 1 file changed, 4 insertions(+) diff --git a/multipathd/main.c b/multipathd/main.c index 1380dd8b..f2890842 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -602,6 +602,10 @@ retry: goto fail; } verify_paths(mpp); + if (VECTOR_SIZE(mpp->paths) == 0 && + flush_map_nopaths(mpp, vecs)) + return 1; + mpp->action = ACT_RELOAD; if (mpp->prflag) { -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 0/3] mulitpathd: Handle losing all paths in update_map
When users rapidly add and remove paths, it's possible for multipathd to delay updating the multipath device, and when if finally calls update_map(), find out that no paths remain for the device. When this happens, multipathd should act just like all the paths had been removed, attempting to remove the device, while respecting the flush_on_last_del and deferred_remove settings. Instead multipathd was simply reloading the map with no paths. This patchset fixes this. Benjamin Marzinski (3): multipathd: factor out the code to flush a map with no paths libmultipath: return success if we raced to remove a map and lost multipathd: Handle losing all path in update_map libmultipath/devmapper.c | 4 +++ multipathd/main.c| 64 +--- 2 files changed, 37 insertions(+), 31 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/3] multipathd: factor out the code to flush a map with no paths
The code to flush a multipath device because all of its paths have been removed will be used by another caller, so factor it out of ev_remove_path(). Signed-off-by: Benjamin Marzinski --- multipathd/main.c | 56 --- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 5c2a7272..325f8cd4 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -561,6 +561,30 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset) return 0; } +static bool +flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) { + char alias[WWID_SIZE]; + + /* +* flush_map will fail if the device is open +*/ + strlcpy(alias, mpp->alias, WWID_SIZE); + if (mpp->flush_on_last_del == FLUSH_ENABLED) { + condlog(2, "%s Last path deleted, disabling queueing", + mpp->alias); + mpp->retry_tick = 0; + mpp->no_path_retry = NO_PATH_RETRY_FAIL; + mpp->disable_queueing = 1; + mpp->stat_map_failures++; + dm_queue_if_no_path(mpp->alias, 0); + } + if (!flush_map(mpp, vecs, 1)) { + condlog(2, "%s: removed map after removing all paths", alias); + return true; + } + return false; +} + static int update_map (struct multipath *mpp, struct vectors *vecs, int new_map) { @@ -1351,34 +1375,12 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map) vector_del_slot(mpp->paths, i); /* -* remove the map IF removing the last path +* remove the map IF removing the last path. If +* flush_map_nopaths succeeds, the path has been removed. */ - if (VECTOR_SIZE(mpp->paths) == 0) { - char alias[WWID_SIZE]; - - /* -* flush_map will fail if the device is open -*/ - strlcpy(alias, mpp->alias, WWID_SIZE); - if (mpp->flush_on_last_del == FLUSH_ENABLED) { - condlog(2, "%s Last path deleted, disabling queueing", mpp->alias); - mpp->retry_tick = 0; - mpp->no_path_retry = NO_PATH_RETRY_FAIL; - mpp->disable_queueing = 1; - mpp->stat_map_failures++; - dm_queue_if_no_path(mpp->alias, 0); - } - if (!flush_map(mpp, vecs, 1)) { - condlog(2, "%s: removed map after" - " removing all paths", - alias); - /* flush_map() has freed the path */ - goto out; - } - /* -* Not an error, continue -*/ - } + if (VECTOR_SIZE(mpp->paths) == 0 && + flush_map_nopaths(mpp, vecs)) + goto out; if (setup_map(mpp, , vecs)) { condlog(0, "%s: failed to setup map for" -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/3] libmultipath: return success if we raced to remove a map and lost
_dm_flush_map() was returning failure if it failed to remove a map, even if that was because the map had already been removed. Return success in this case. Signed-off-by: Benjamin Marzinski --- libmultipath/devmapper.c | 4 multipathd/main.c| 4 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c index 1748d258..a49db3b0 100644 --- a/libmultipath/devmapper.c +++ b/libmultipath/devmapper.c @@ -,6 +,10 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove, } condlog(4, "multipath map %s removed", mapname); return 0; + } else if (dm_is_mpath(mapname) != 1) { + condlog(4, "multipath map %s removed externally", + mapname); + return 0; /*we raced with someone else removing it */ } else { condlog(2, "failed to remove multipath map %s", mapname); diff --git a/multipathd/main.c b/multipathd/main.c index 325f8cd4..1380dd8b 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -758,10 +758,6 @@ flush_map(struct multipath * mpp, struct vectors * vecs, int nopaths) * the spurious uevent we may generate with the dm_flush_map call below */ if (r) { - /* -* May not really be an error -- if the map was already flushed -* from the device mapper by dmsetup(8) for instance. -*/ if (r == 1) condlog(0, "%s: can't flush", mpp->alias); else { -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [RFC PATCH] multipath: fix systemd timers in the initramfs
The systemd timers created for "find_multipaths smart" conflict with shutdown.target, but not with initrd-cleanup.service. This can make these timers trigger after the inirtd has started shutting down, restarting multipathd (which then stops initrd-cleanup.service, since it conflicts). To avoid this, make sure the timers and the unit they trigger conflict with inird-cleanup.service. Also don't make them start multipathd. "multipath -u" will not return "maybe" if multipathd isn't running or set to run, and since we no longer wait for udev-settle, multipathd starts up pretty quickly, so it shouldn't be a problem to not trigger it here. Signed-off-by: Benjamin Marzinski --- Notes: I haven't seen this, but I do worry that path uevents could come in after initrd-cleanup.service has started. In this case the timers themselves could stop initrd-cleanup.service, since they conflict with it. I'm not sure if there is any way to see where we are in the bootup/shutdown process and not start up the timers if we are already cleaning up. Also, I don't know of any way to set a service so that if it conflicts with another service, it is always the one that gets stopped. A safe option would be to simply make the timers not start multipathd, without adding the conflicts. multipath/multipath.rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/multipath/multipath.rules b/multipath/multipath.rules index 9df11a95..f993d996 100644 --- a/multipath/multipath.rules +++ b/multipath/multipath.rules @@ -71,7 +71,7 @@ ENV{.SAVED_FM_WAIT_UNTIL}=="?*", GOTO="pretend_mpath" # # We must trigger an "add" event because LVM2 will only act on those. -RUN+="/usr/bin/systemd-run --unit=cancel-multipath-wait-$kernel --description 'cancel waiting for multipath siblings of $kernel' --no-block --timer-property DefaultDependencies=no --timer-property Conflicts=shutdown.target --timer-property Before=shutdown.target --timer-property AccuracySec=500ms --property DefaultDependencies=no --property Conflicts=shutdown.target --property Before=shutdown.target --property Wants=multipathd.service --property After=multipathd.service --on-active=$env{FIND_MULTIPATHS_WAIT_UNTIL} /usr/bin/udevadm trigger --action=add $sys$devpath" +RUN+="/usr/bin/systemd-run --unit=cancel-multipath-wait-$kernel --description 'cancel waiting for multipath siblings of $kernel' --no-block --timer-property DefaultDependencies=no --timer-property Conflicts=shutdown.target --timer-property Before=shutdown.target --timer-property Conflicts=initrd-cleanup.service --timer-property Before=initrd-cleanup.service --timer-property AccuracySec=500ms --property DefaultDependencies=no --property Conflicts=shutdown.target --property Before=shutdown.target --property Conflicts=initrd-cleanup.service --property Before=initrd-cleanup.service --on-active=$env{FIND_MULTIPATHS_WAIT_UNTIL} /usr/bin/udevadm trigger --action=add $sys$devpath" LABEL="pretend_mpath" ENV{DM_MULTIPATH_DEVICE_PATH}="1" -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/6] multipathd: Use regular pthread_mutex_t for waiter_lock
waiter_lock doesn't need any special lock handing. Also make it static. Signed-off-by: Benjamin Marzinski --- multipathd/waiter.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/multipathd/waiter.c b/multipathd/waiter.c index 2a221465..52793698 100644 --- a/multipathd/waiter.c +++ b/multipathd/waiter.c @@ -24,7 +24,7 @@ #include "main.h" pthread_attr_t waiter_attr; -struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER }; +static pthread_mutex_t waiter_lock = PTHREAD_MUTEX_INITIALIZER; static struct event_thread *alloc_waiter (void) { @@ -65,11 +65,11 @@ void stop_waiter_thread (struct multipath *mpp) (unsigned long)mpp->waiter); thread = mpp->waiter; mpp->waiter = (pthread_t)0; - pthread_cleanup_push(cleanup_lock, _lock); - lock(_lock); + pthread_cleanup_push(cleanup_mutex, _lock); + pthread_mutex_lock(_lock); pthread_kill(thread, SIGUSR2); pthread_cancel(thread); - lock_cleanup_pop(_lock); + pthread_cleanup_pop(1); } /* @@ -126,10 +126,10 @@ static int waiteventloop (struct event_thread *waiter) waiter->dmt = NULL; if (!r) { /* wait interrupted by signal. check for cancellation */ - pthread_cleanup_push(cleanup_lock, _lock); - lock(_lock); + pthread_cleanup_push(cleanup_mutex, _lock); + pthread_mutex_lock(_lock); pthread_testcancel(); - lock_cleanup_pop(_lock); + pthread_cleanup_pop(1); return 1; /* If we weren't cancelled, just reschedule */ } -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [RFC PATCH 0/6] allowing path checking to be interrupted.
When there are a huge number of paths (> 1) The about of time that the checkerloop can hold the vecs lock for while checking the paths can get to be large enough that it starves other vecs lock users. If path checking takes long enough, it's possible that uxlsnr threads will never run. To deal with this, this patchset makes it possible to drop the vecs lock while checking the paths, and then reacquire it and continue with the next path to check. My choice of only checking if there are waiters every 128 paths checked and only interrupting if path checking has taken more than a second are arbitrary. I didn't want to slow down path checking in the common case where this isn't an issue, and I wanted to avoid path checking getting starved by other vecs->lock users. Having the checkerloop wait for 1 nsec was based on my own testing with a setup using 4K multipath devies with 4 paths each. This was almost always long enough for the uevent or uxlsnr client to grab the vecs lock, but I'm not sure how dependent this is on details of the system. For instance with my setup in never took more than 20 seconds to check the paths. and usually, a looping through all the paths took well under 10 seconds, most often under 5. I would only occasionally run into situations where a uxlsnr client would time out. Benjamin Marzinski (6): multipathd: Use regular pthread_mutex_t for waiter_lock multipathd: track waiters for mutex_lock multipathd: Occasionally allow waiters to interrupt checking paths multipathd: allow uxlsnr clients to interrupt checking paths multipathd: fix uxlsnr timeout multipathd: Don't check if timespec.tv_sec is zero libmultipath/lock.h| 16 + libmultipath/structs.h | 1 + multipathd/main.c | 144 + multipathd/uxlsnr.c| 23 +-- multipathd/uxlsnr.h| 1 + multipathd/waiter.c| 14 ++-- 6 files changed, 132 insertions(+), 67 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 4/6] multipathd: allow uxlsnr clients to interrupt checking paths
The uxlsnr clients never block waiting on the vecs->lock. Instead they register to get woken up and call trylock() when the lock is dropped. Add code to track when they are registered to get woken up. The checkerloop now checks if there are waiting uxlsnr clients as well. Signed-off-by: Benjamin Marzinski --- multipathd/main.c | 3 ++- multipathd/uxlsnr.c | 20 +++- multipathd/uxlsnr.h | 1 + 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 73c95806..78374377 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2647,7 +2647,8 @@ checkerloop (void *ap) num_paths += rc; if (++paths_checked % 128 == 0 && check_id < INT_MAX && - lock_has_waiters(>lock)) { + (lock_has_waiters(>lock) || +waiting_clients())) { get_monotonic_time(_time); timespecsub(_time, _start_time, _time); diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c index 645e356c..04bcd020 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -91,6 +91,7 @@ static LIST_HEAD(clients); static struct pollfd *polls; static int notify_fd = -1; static int idle_fd = -1; +static bool clients_need_lock = false; static bool _socket_client_is_root(int fd) { @@ -309,15 +310,22 @@ static struct timespec *get_soonest_timeout(struct timespec *ts) return ts; } -static bool need_vecs_lock(void) +bool waiting_clients(void) +{ + return clients_need_lock; +} + +static void check_for_locked_work(struct client *skip) { struct client *c; list_for_each_entry(c, , node) { - if (c->state == CLT_LOCKED_WORK) - return true; + if (c != skip && c->state == CLT_LOCKED_WORK) { + clients_need_lock = true; + return; + } } - return false; + clients_need_lock = false; } static int parse_cmd(struct client *c) @@ -494,6 +502,7 @@ static int client_state_machine(struct client *c, struct vectors *vecs, /* don't use cleanup_lock(), lest we wakeup ourselves */ pthread_cleanup_push_cast(__unlock, >lock); c->error = execute_handler(c, vecs); + check_for_locked_work(c); pthread_cleanup_pop(1); condlog(4, "%s: cli[%d] grabbed lock", __func__, c->fd); free_keys(c->cmdvec); @@ -661,7 +670,8 @@ void *uxsock_listen(long ux_sock, void *trigger_data) polls[POLLFD_NOTIFY].events = POLLIN; polls[POLLFD_IDLE].fd = idle_fd; - if (need_vecs_lock()) + check_for_locked_work(NULL); + if (clients_need_lock) polls[POLLFD_IDLE].events = POLLIN; else polls[POLLFD_IDLE].events = 0; diff --git a/multipathd/uxlsnr.h b/multipathd/uxlsnr.h index 60c3a2c7..3e45930b 100644 --- a/multipathd/uxlsnr.h +++ b/multipathd/uxlsnr.h @@ -3,6 +3,7 @@ #include +bool waiting_clients(void); void uxsock_cleanup(void *arg); void *uxsock_listen(long ux_sock, void * trigger_data); -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 5/6] multipathd: fix uxlsnr timeout
check_timeout() is called whenever it's time to handle a client, and if it detects a timeout, it will switch to the CLT_SEND state. However it may already be in the CLT_SEND state, and may have already sent the length, and possibly some data. Restarting the CLT_SEND state will cause it to restart sending the length, messing up communication with the client. If we are already sending a reply to the client, we should just finish the send. Disable timeouts in the CLT_SEND state. Signed-off-by: Benjamin Marzinski --- multipathd/uxlsnr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c index 04bcd020..23cb123d 100644 --- a/multipathd/uxlsnr.c +++ b/multipathd/uxlsnr.c @@ -405,10 +405,11 @@ static void set_client_state(struct client *c, int state) case CLT_RECV: reset_strbuf(>reply); memset(c->cmd, '\0', sizeof(c->cmd)); - c->expires = ts_zero; c->error = 0; /* fallthrough */ case CLT_SEND: + /* no timeout while waiting for the client or sending a reply */ + c->expires = ts_zero; /* reuse these fields for next data transfer */ c->len = c->cmd_len = 0; break; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 6/6] multipathd: Don't check if timespec.tv_sec is zero
checking if tv_sec is 0 is a holdover from before we had get_monotonic_time(), when we used to zero out tv_sec on failure. Also, use normalize_timespec() to simplify setting the sleep time. Signed-off-by: Benjamin Marzinski --- multipathd/main.c | 62 --- 1 file changed, 26 insertions(+), 36 deletions(-) diff --git a/multipathd/main.c b/multipathd/main.c index 78374377..5c2a7272 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2589,16 +2589,11 @@ checkerloop (void *ap) break; get_monotonic_time(_time); - if (start_time.tv_sec && last_time.tv_sec) { - timespecsub(_time, _time, _time); - condlog(4, "tick (%ld.%06lu secs)", - (long)diff_time.tv_sec, diff_time.tv_nsec / 1000); - last_time = start_time; - ticks = diff_time.tv_sec; - } else { - ticks = 1; - condlog(4, "tick (%d ticks)", ticks); - } + timespecsub(_time, _time, _time); + condlog(4, "tick (%ld.%06lu secs)", + (long)diff_time.tv_sec, diff_time.tv_nsec / 1000); + last_time = start_time; + ticks = diff_time.tv_sec; #ifdef USE_SYSTEMD if (use_watchdog) sd_notify(0, "WATCHDOG=1"); @@ -2688,26 +2683,23 @@ unlock: lock_cleanup_pop(vecs->lock); } - diff_time.tv_nsec = 0; - if (start_time.tv_sec) { - get_monotonic_time(_time); - timespecsub(_time, _time, _time); - if (num_paths) { - unsigned int max_checkint; - - condlog(4, "checked %d path%s in %ld.%06lu secs", - num_paths, num_paths > 1 ? "s" : "", - (long)diff_time.tv_sec, - diff_time.tv_nsec / 1000); - conf = get_multipath_config(); - max_checkint = conf->max_checkint; - put_multipath_config(conf); - if (diff_time.tv_sec > (time_t)max_checkint) - condlog(1, "path checkers took longer " - "than %ld seconds, consider " - "increasing max_polling_interval", - (long)diff_time.tv_sec); - } + get_monotonic_time(_time); + timespecsub(_time, _time, _time); + if (num_paths) { + unsigned int max_checkint; + + condlog(4, "checked %d path%s in %ld.%06lu secs", + num_paths, num_paths > 1 ? "s" : "", + (long)diff_time.tv_sec, + diff_time.tv_nsec / 1000); + conf = get_multipath_config(); + max_checkint = conf->max_checkint; + put_multipath_config(conf); + if (diff_time.tv_sec > (time_t)max_checkint) + condlog(1, "path checkers took longer " + "than %ld seconds, consider " + "increasing max_polling_interval", + (long)diff_time.tv_sec); } if (foreign_tick == 0) { @@ -2725,12 +2717,10 @@ unlock: if (!strict_timing) sleep(1); else { - if (diff_time.tv_nsec) { - diff_time.tv_sec = 0; - diff_time.tv_nsec = -1000UL * 1000 * 1000 - diff_time.tv_nsec; - } else - diff_time.tv_sec = 1; + diff_time.tv_sec = 0; + diff_time.tv_nsec = +1000UL * 1000 * 1000 - diff_time.tv_nsec; + normalize_timespec(_time); condlog(3, "waiting for %ld.%06lu secs", (long)diff_time.tv_sec, -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 3/6] multipathd: Occasionally allow waiters to interrupt checking paths
If there are a very large number of paths that all get checked at the same time, it is possible for the path checkers to starve out other vecs->lock users, like uevent processing. To avoid this, if the path checkers are taking a long time, checkerloop now occasionally unlocks and allows other vecs->lock waiters to run. In order to make sure that path checking is always making real progress, checkerloop() only checks if it should unlock every 128 path checks. To check, it sees if there are waiters and more than a second has passed since it acquired the vecs->lock. If both of these are true, it drops the lock and calls nanosleep() to schedule. When checkerloop() reacquires the lock, it may no longer be at the correct place in the vector. While paths can be deleted from any point of the pathvec, they can only be added at the end. This means that the next path to check must be between its previous location and the start of the vector. To help determine the correct starting place, checkerloop() assigns a check_id, starting from 1, to paths as it checks them. The paths save this id. Newly added paths start with a special id of 0. As paths are deleted, later paths, with higher ids, are shifted towards the start of the vector. checkerloop() just needs to check backwards from the array index where it was at when in dropped the lock until it finds the first path with a check_id greater than zero and smaller than the last one it gave out. This will be the last path it checked. For this to work, the check_ids must always increase as you go through the pathvec array. To guarantee this, checkloop() cannot drop the lock unless it can guarantee that no matter what happens before it regains the lock, it will have enough ids to not roll over before it hits the end of the pathvec (check_id must be less than INT_MAX). Signed-off-by: Benjamin Marzinski --- libmultipath/structs.h | 1 + multipathd/main.c | 79 +- 2 files changed, 63 insertions(+), 17 deletions(-) diff --git a/libmultipath/structs.h b/libmultipath/structs.h index a6a09441..47358091 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -351,6 +351,7 @@ struct path { int fast_io_fail; unsigned int dev_loss; int eh_deadline; + unsigned int check_id; /* configlet pointers */ vector hwe; struct gen_path generic_path; diff --git a/multipathd/main.c b/multipathd/main.c index 71079113..73c95806 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2555,7 +2555,6 @@ checkerloop (void *ap) struct vectors *vecs; struct path *pp; int count = 0; - unsigned int i; struct timespec last_time; struct config *conf; int foreign_tick = 0; @@ -2581,8 +2580,9 @@ checkerloop (void *ap) while (1) { struct timespec diff_time, start_time, end_time; - int num_paths = 0, strict_timing, rc = 0; - unsigned int ticks = 0; + int num_paths = 0, strict_timing, rc = 0, i = 0; + unsigned int ticks = 0, check_id = 0; + bool more_paths = true; if (set_config_state(DAEMON_RUNNING) != DAEMON_RUNNING) /* daemon shutdown */ @@ -2604,21 +2604,66 @@ checkerloop (void *ap) sd_notify(0, "WATCHDOG=1"); #endif - pthread_cleanup_push(cleanup_lock, >lock); - lock(>lock); - pthread_testcancel(); - vector_foreach_slot (vecs->pathvec, pp, i) { - rc = check_path(vecs, pp, ticks); - if (rc < 0) { - condlog(1, "%s: check_path() failed, removing", - pp->dev); - vector_del_slot(vecs->pathvec, i); - free_path(pp); - i--; - } else - num_paths += rc; + while (more_paths) { + unsigned int paths_checked = 0; + struct timespec chk_start_time; + + pthread_cleanup_push(cleanup_lock, >lock); + lock(>lock); + pthread_testcancel(); + get_monotonic_time(_start_time); + /* +* Paths could have been removed since we dropped +* the lock. Find the path to continue checking at. +* Paths added since we last checked will always have +* pp->check_id == 0 Otherwise, pp->check_id will +* always be increasing, and always greater than a +* path's array index. Thus, checking backwards, the +* first non-new path wi
[dm-devel] [PATCH 2/6] multipathd: track waiters for mutex_lock
use the uatomic operations to track how many threads are waiting in lock() for mutex_locks. This will be used by a later patch. Signed-off-by: Benjamin Marzinski --- libmultipath/lock.h | 16 multipathd/main.c | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/libmultipath/lock.h b/libmultipath/lock.h index d7b779e7..20ca77e6 100644 --- a/libmultipath/lock.h +++ b/libmultipath/lock.h @@ -2,17 +2,28 @@ #define _LOCK_H #include +#include +#include typedef void (wakeup_fn)(void); struct mutex_lock { pthread_mutex_t mutex; wakeup_fn *wakeup; + int waiters; /* uatomic access only */ }; +static inline void init_lock(struct mutex_lock *a) +{ + pthread_mutex_init(>mutex, NULL); + uatomic_set(>waiters, 0); +} + static inline void lock(struct mutex_lock *a) { + uatomic_inc(>waiters); pthread_mutex_lock(>mutex); + uatomic_dec(>waiters); } static inline int trylock(struct mutex_lock *a) @@ -30,6 +41,11 @@ static inline void __unlock(struct mutex_lock *a) pthread_mutex_unlock(>mutex); } +static inline bool lock_has_waiters(struct mutex_lock *a) +{ + return (uatomic_read(>waiters) > 0); +} + #define lock_cleanup_pop(a) pthread_cleanup_pop(1) void cleanup_lock (void * data); diff --git a/multipathd/main.c b/multipathd/main.c index 2f2b9d4c..71079113 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -2940,7 +2940,7 @@ init_vecs (void) if (!vecs) return NULL; - pthread_mutex_init(>lock.mutex, NULL); + init_lock(>lock); return vecs; } -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: remove list of rebranded arrays vendors from man page
On Thu, Jul 21, 2022 at 07:22:04PM +0200, Xose Vazquez Perez wrote: > It does not provide useful info, and it is incomplete. > Reviewed-by: Benjamin Marzinski > Cc: Martin Wilck > Cc: Benjamin Marzinski > Cc: Christophe Varoqui > Cc: DM-DEVEL ML > Signed-off-by: Xose Vazquez Perez > --- > multipath/multipath.conf.5 | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 > index c2d34f18..d5506d99 100644 > --- a/multipath/multipath.conf.5 > +++ b/multipath/multipath.conf.5 > @@ -315,12 +315,12 @@ accepts the optional prio_arg \fIexclusive_pref_bit\fR. > .TP > .I ontap > (Hardware-dependent) > -Generate the path priority for NetApp ONTAP class and OEM arrays as IBM > NSeries. > +Generate the path priority for NetApp ONTAP class, and rebranded arrays. > .TP > .I rdac > (Hardware-dependent) > Generate the path priority for LSI/Engenio/NetApp RDAC class as NetApp > SANtricity > -E/EF Series, and OEM arrays from IBM DELL SGI STK and SUN. > +E/EF Series, and rebranded arrays. > .TP > .I hp_sw > (Hardware-dependent) > @@ -496,7 +496,7 @@ Active/Standby mode exclusively. > .I rdac > (Hardware-dependent) > Check the path state for LSI/Engenio/NetApp RDAC class as NetApp SANtricity > E/EF > -Series, and OEM arrays from IBM DELL SGI STK and SUN. > +Series, and rebranded arrays. > .TP > .I directio > Read the first sector with direct I/O. This checker could cause spurious path > @@ -1568,7 +1568,7 @@ families. > .I 1 rdac > (Hardware-dependent) > Hardware handler for LSI/Engenio/NetApp RDAC class as NetApp SANtricity E/EF > -Series, and OEM arrays from IBM DELL SGI STK and SUN. > +Series, and rebranded arrays. > .TP > .I 1 hp_sw > (Hardware-dependent) > -- > 2.37.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [Question] multipathd add/remove paths takes a long time
On Tue, Jul 19, 2022 at 08:13:39PM +0800, Wu Guanghao wrote: > The system has 1K multipath devices, each device has 16 paths. > Execute multipathd add/multipathd remove or uev_add_path/ > uev_remove_path to add/remove paths, which takes over 20s. > What's more, the second checkloop may be execed immediately > after finishing first checkloop. It's too long. > > We found that time was mostly spent waiting for locks. > > checkerloop(){ > ... > lock(>lock); > vector_foreach_slot (vecs->pathvec, pp, i) { > rc = check_path(...); // Too many paths, it takes a long time > ... > } > lock_cleanup_pop(vecs->lock); > ... > } > > Can the range of vecs->lock locks be adjusted to reduce the time consuming > when adding/removing paths? As long as we make sure not to skip any paths or double-check any paths, we don't need to hold the vecs->lock between checking paths. There is certainly some optimization that could get done here. could you post the output of: # multipath -l # multipathd show config local -Ben -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 08/14] libmultipath: sysfs_attr_set_value(): don't return 0 on partial write
On Wed, Jul 13, 2022 at 10:20:43AM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > sysfs_attr_set_value() returned 0 if not all requested bytes were written. > Change this to return the number of bytes written. Error checking is now > somewhat more involved; provide a helper macro for it. > Reviewed-by: Benjamin Marzinski > Signed-off-by: Martin Wilck > --- > libmultipath/configure.c | 10 -- > libmultipath/discovery.c | 74 +--- > libmultipath/sysfs.c | 6 ++-- > libmultipath/sysfs.h | 10 ++ > 4 files changed, 66 insertions(+), 34 deletions(-) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index 467bbaa..0607dba 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -568,6 +568,7 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int > is_reload) > struct pathgroup * pgp; > struct path *pp; > char buff[11]; > + ssize_t len; > int i, j, ret, err = 0; > struct udev_device *udd; > int max_sectors_kb; > @@ -600,14 +601,17 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int > is_reload) > } > } > snprintf(buff, 11, "%d", max_sectors_kb); > + len = strlen(buff); > > vector_foreach_slot (mpp->pg, pgp, i) { > vector_foreach_slot(pgp->paths, pp, j) { > ret = sysfs_attr_set_value(pp->udev, > "queue/max_sectors_kb", > -buff, strlen(buff)); > - if (ret < 0) { > - condlog(1, "failed setting max_sectors_kb on %s > : %s", pp->dev, strerror(-ret)); > +buff, len); > + if (ret != len) { > + log_sysfs_attr_set_value(1, ret, > + "failed setting max_sectors_kb on %s", > + pp->dev); > err = 1; > } > } > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 54b1caf..ee29009 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -598,13 +598,15 @@ sysfs_set_eh_deadline(struct path *pp) > len = sprintf(value, "%d", pp->eh_deadline); > > ret = sysfs_attr_set_value(hostdev, "eh_deadline", > -value, len + 1); > +value, len); > /* >* not all scsi drivers support setting eh_deadline, so failing >* is totally reasonable >*/ > - if (ret <= 0) > - condlog(3, "%s: failed to set eh_deadline to %s, error %d", > udev_device_get_sysname(hostdev), value, -ret); > + if (ret != len) > + log_sysfs_attr_set_value(3, ret, > + "%s: failed to set eh_deadline to %s", > + udev_device_get_sysname(hostdev), value); > > udev_device_unref(hostdev); > return (ret <= 0); > @@ -667,19 +669,22 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path > *pp) > pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { > /* Check if we need to temporarily increase dev_loss_tmo */ > if ((unsigned int)pp->fast_io_fail >= tmo) { > + ssize_t len; > + > /* Increase dev_loss_tmo temporarily */ > snprintf(value, sizeof(value), "%u", >(unsigned int)pp->fast_io_fail + 1); > + len = strlen(value); > ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", > -value, strlen(value)); > - if (ret <= 0) { > +value, len); > + if (ret != len) { > if (ret == -EBUSY) > condlog(3, "%s: rport blocked", > rport_id); > else > - condlog(0, "%s: failed to set " > - "dev_loss_tmo to %s, error %d", > - rport_id, value, -ret); > + log_sysfs_attr_set_value(0, ret, > +
[dm-devel] [PATCH] libmultipath: fix find_multipaths_timeout for unknown hardware
pp->hwe is now a vector that will always be allocated for all path devices. Instead of checking if it is NULL, check if it is empty. Signed-off-by: Benjamin Marzinski --- libmultipath/propsel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index 50d0b5c8..f782f251 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -1293,7 +1293,7 @@ out: */ if (pp->find_multipaths_timeout < 0) { pp->find_multipaths_timeout = -pp->find_multipaths_timeout; - if (!pp->hwe) { + if (VECTOR_SIZE(pp->hwe) == 0) { pp->find_multipaths_timeout = DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT; origin = "(default for unknown hardware)"; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/2] update hwtable
On Sun, Jul 10, 2022 at 01:07:07AM +0200, Xose Vazquez Perez wrote: > Xose Vazquez Perez (2): > multipath-tools: update Huawei OceanStor NVMe vendor id > multipath-tools: update "Generic NVMe" vendor regex in hwtable For the set Reviewed-by: Benjamin Marzinski > > libmultipath/hwtable.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Cc: > Cc: > Cc: > Cc: Zhouweigang (Jack) > Cc: Zou Ming > Cc: Martin Wilck > Cc: Benjamin Marzinski > Cc: Christophe Varoqui > Cc: DM-DEVEL ML > Signed-off-by: Xose Vazquez Perez > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 00/14] multipath: fixes for sysfs accessors
On Wed, Jul 06, 2022 at 04:38:08PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > This set fixes some strangeness in our sysfs accessors which I > found while looking at > https://github.com/opensvc/multipath-tools/issues/35#issuecomment-1175901745. > (The patches don't fix this issue, which seems to be related to > Debian's initramfs setup). > > Most importantly, sysfs_attr_get_value() and sysfs_attr_set_value() > would return 0 if the number of bytes read/written was different from > the expected value, which is non-standard and unexpected. This > series changes the return value semantics of these functions: > > - in sysfs_attr_get_value(), if a read buffer is too small to hold >the string read plus a terminating 0 byte, the return value >equals the buffer size. > - in sysfs_bin_attr_get_value(), no 0 bytes are appended. It's not >an error if the read buffer is completely filled, and no warning >is printed in this case. > - sysfs_attr_set_value() always returns the number of bytes written >unless an error occured in open() or write(). > > Tests for the new semantics are added. Moreover, the sysfs.c code > is slightly refactored to avoid code duplication. For all except 8/14: Reviewed-by: Benjamin Marzinski > > Martin Wilck (14): > libmultipath: alua: remove get_sysfs_pg83() > libmultipath: remove sysfs_get_binary() > libmultipath: sysfs_bin_attr_get_value(): no error if buffer is filled > libmultipath: common code path for sysfs_attr_get_value() > libmultipath: sanitize error checking in sysfs accessors > libmultipath: get rid of PATH_SIZE > libmultipath: sysfs_attr_get_value(): don't return 0 if buffer too > small > libmultipath: sysfs_attr_set_value(): don't return 0 on partial write > libmultipath: sysfs: cleanup file descriptors on pthread_cancel() > libmultipath, multipathd: log failure setting sysfs attributes > multipath tests: expect_condlog: skip depending on verbosity > multipath tests: __wrap_dlog: print log message > multipath tests: add sysfs test > libmultipath.version: bump version for sysfs accessors > > libmultipath/configure.c | 30 +- > libmultipath/discovery.c | 120 +++ > libmultipath/libmultipath.version | 8 +- > libmultipath/prioritizers/alua_rtpg.c | 57 +-- > libmultipath/propsel.c| 6 +- > libmultipath/structs.h| 3 - > libmultipath/sysfs.c | 191 -- > libmultipath/sysfs.h | 23 ++ > libmultipath/util.c | 8 +- > multipathd/cli_handlers.c | 2 +- > multipathd/fpin_handlers.c| 11 +- > multipathd/main.c | 40 ++- > tests/Makefile| 5 +- > tests/sysfs.c | 494 ++ > tests/test-lib.c | 1 - > tests/test-log.c | 5 + > 16 files changed, 751 insertions(+), 253 deletions(-) > create mode 100644 tests/sysfs.c > > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 08/14] libmultipath: sysfs_attr_set_value(): don't return 0 on partial write
On Wed, Jul 06, 2022 at 04:38:16PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > sysfs_attr_set_value() returned 0 if not all requested bytes were written. > Change this to return the number of bytes written. Error checking is now > somewhat more involved; provide a helper macro fo it. > > Signed-off-by: Martin Wilck > --- > libmultipath/configure.c | 10 -- > libmultipath/discovery.c | 70 ++-- > libmultipath/sysfs.c | 6 ++-- > libmultipath/sysfs.h | 10 ++ > 4 files changed, 64 insertions(+), 32 deletions(-) > > diff --git a/libmultipath/configure.c b/libmultipath/configure.c > index 467bbaa..0607dba 100644 > --- a/libmultipath/configure.c > +++ b/libmultipath/configure.c > @@ -568,6 +568,7 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int > is_reload) > struct pathgroup * pgp; > struct path *pp; > char buff[11]; > + ssize_t len; > int i, j, ret, err = 0; > struct udev_device *udd; > int max_sectors_kb; > @@ -600,14 +601,17 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int > is_reload) > } > } > snprintf(buff, 11, "%d", max_sectors_kb); > + len = strlen(buff); > > vector_foreach_slot (mpp->pg, pgp, i) { > vector_foreach_slot(pgp->paths, pp, j) { > ret = sysfs_attr_set_value(pp->udev, > "queue/max_sectors_kb", > -buff, strlen(buff)); > - if (ret < 0) { > - condlog(1, "failed setting max_sectors_kb on %s > : %s", pp->dev, strerror(-ret)); > +buff, len); > + if (ret != len) { > + log_sysfs_attr_set_value(1, ret, > + "failed setting max_sectors_kb on %s", > + pp->dev); > err = 1; > } > } > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 54b1caf..1137629 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -603,8 +603,10 @@ sysfs_set_eh_deadline(struct path *pp) >* not all scsi drivers support setting eh_deadline, so failing >* is totally reasonable >*/ > - if (ret <= 0) > - condlog(3, "%s: failed to set eh_deadline to %s, error %d", > udev_device_get_sysname(hostdev), value, -ret); > + if (ret != len + 1) I know that this was originally my error, but I don't see any reason why we should check (or write) the NULL byte here. I can fix this is in a separate patch, or you can fix it along with my other issue for this patch. > + log_sysfs_attr_set_value(3, ret, > + "%s: failed to set eh_deadline to %s", > + udev_device_get_sysname(hostdev), value); > > udev_device_unref(hostdev); > return (ret <= 0); > @@ -667,19 +669,22 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path > *pp) > pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { > /* Check if we need to temporarily increase dev_loss_tmo */ > if ((unsigned int)pp->fast_io_fail >= tmo) { > + ssize_t len; > + > /* Increase dev_loss_tmo temporarily */ > snprintf(value, sizeof(value), "%u", >(unsigned int)pp->fast_io_fail + 1); > + len = strlen(value); > ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", > -value, strlen(value)); > - if (ret <= 0) { > +value, len); > + if (ret != len) { > if (ret == -EBUSY) > condlog(3, "%s: rport blocked", > rport_id); > else > - condlog(0, "%s: failed to set " > - "dev_loss_tmo to %s, error %d", > - rport_id, value, -ret); > + log_sysfs_attr_set_value(0, ret, > + "%s: failed to set dev_loss_tmo > to %s", > + rport_id, value); > goto out; > } > } > @@ -691,32 +696,39 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path > *pp) > pp->dev_loss = DEFAULT_DEV_LOSS_TMO; > } > if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET) { > + ssize_t len; > + > if (pp->fast_io_fail == MP_FAST_IO_FAIL_OFF) >
Re: [dm-devel] [PATCH 05/14] libmultipath: sanitize error checking in sysfs accessors
On Wed, Jul 06, 2022 at 04:38:13PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > udev_device_get_syspath() may return NULL; check for it, and check > for pathname overflow. Disallow a zero buffer length. The fstat() > calls were superfluous, as a read() or write() on the fd would > return the respective error codes depending on file type or permissions, > the extra system call and code complexity adds no value. > > Log levels should be moderate in sysfs.c, because it depends > on the caller whether errors getting/setting certain attributes are > fatal. > > Signed-off-by: Martin Wilck > --- > libmultipath/sysfs.c | 94 ++-- > 1 file changed, 39 insertions(+), 55 deletions(-) > > diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c > index 4db911c..1f0f207 100644 > --- a/libmultipath/sysfs.c > +++ b/libmultipath/sysfs.c > @@ -47,46 +47,38 @@ > static ssize_t __sysfs_attr_get_value(struct udev_device *dev, const char > *attr_name, > char *value, size_t value_len, bool > binary) > { > + const char *syspath; > char devpath[PATH_SIZE]; > - struct stat statbuf; > int fd; > ssize_t size = -1; > > - if (!dev || !attr_name || !value) > - return 0; > + if (!dev || !attr_name || !value || !value_len) { > + condlog(1, "%s: invalid parameters", __func__); > + return -EINVAL; > + } > > - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), > - attr_name); > + syspath = udev_device_get_syspath(dev); > + if (!syspath) { > + condlog(3, "%s: invalid udevice", __func__); > + return -EINVAL; > + } > + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { > + condlog(3, "%s: devpath overflow", __func__); > + return -EOVERFLOW; > + } > condlog(4, "open '%s'", devpath); > /* read attribute value */ > fd = open(devpath, O_RDONLY); > if (fd < 0) { > - condlog(4, "attribute '%s' can not be opened: %s", > - devpath, strerror(errno)); > + condlog(3, "%s: attribute '%s' can not be opened: %s", > + __func__, devpath, strerror(errno)); > return -errno; > } > - if (fstat(fd, ) < 0) { > - condlog(4, "stat '%s' failed: %s", devpath, strerror(errno)); > - close(fd); > - return -ENXIO; > - } > - /* skip directories */ > - if (S_ISDIR(statbuf.st_mode)) { > - condlog(4, "%s is a directory", devpath); > - close(fd); > - return -EISDIR; > - } > - /* skip non-writeable files */ > - if ((statbuf.st_mode & S_IRUSR) == 0) { > - condlog(4, "%s is not readable", devpath); > - close(fd); > - return -EPERM; > - } > - > size = read(fd, value, value_len); > if (size < 0) { > - condlog(4, "read from %s failed: %s", devpath, strerror(errno)); > size = -errno; > + condlog(3, "%s: read from %s failed: %s", __func__, devpath, > + strerror(errno)); > if (!binary) > value[0] = '\0'; > } else if (!binary && size == (ssize_t)value_len) { > @@ -119,51 +111,43 @@ ssize_t sysfs_bin_attr_get_value(struct udev_device > *dev, const char *attr_name, > ssize_t sysfs_attr_set_value(struct udev_device *dev, const char *attr_name, >const char * value, size_t value_len) > { > + const char *syspath; > char devpath[PATH_SIZE]; > - struct stat statbuf; > int fd; > ssize_t size = -1; > > - if (!dev || !attr_name || !value || !value_len) > - return 0; > + if (!dev || !attr_name || !value || !value_len) { > + condlog(1, "%s: invalid parameters", __func__); > + return -EINVAL; > + } > + > + syspath = udev_device_get_syspath(dev); > + if (!syspath) { > + condlog(3, "%s: invalid udevice", __func__); > + return -EINVAL; > + } > + if (safe_sprintf(devpath, "%s/%s", syspath, attr_name)) { > + condlog(3, "%s: devpath overflow", __func__); > + return -EOVERFLOW; > + } > > - snprintf(devpath, PATH_SIZE, "%s/%s", udev_device_get_syspath(dev), > - attr_name); > condlog(4, "open '%s'", devpath); > /* write attribute value */ > fd = open(devpath, O_WRONLY); > if (fd < 0) { > - condlog(4, "attribute '%s' can not be opened: %s", > - devpath, strerror(errno)); > + condlog(2, "%s: attribute '%s' can not be opened: %s", > + __func__, devpath, strerror(errno)); You log at loglevel 2 here, but at 3 for an open() failure in __sysfs_attr_get_value(). However I notice that this gets resolved in
[dm-devel] [PATCH] dm: fix possible race in dm_start_io_acct
After commit 82f6cdcc3676c ("dm: switch dm_io booleans over to proper flags") dm_start_io_acct stopped atomically checking and setting was_accounted, which turned into the DM_IO_ACCOUNTED flag. This opened the possibility for a race where IO accounting is started twice for duplicate bios. To remove the race, check the flag while holding the io->lock. Fixes: 82f6cdcc3676c ("dm: switch dm_io booleans over to proper flags") Signed-off-by: Benjamin Marzinski --- drivers/md/dm.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/md/dm.c b/drivers/md/dm.c index d8f16183bf27..c7d2dbf03ccb 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -544,17 +544,20 @@ static void dm_start_io_acct(struct dm_io *io, struct bio *clone) { /* * Ensure IO accounting is only ever started once. +* Expect no possibility for race unless DM_TIO_IS_DUPLICATE_BIO. */ - if (dm_io_flagged(io, DM_IO_ACCOUNTED)) - return; - - /* Expect no possibility for race unless DM_TIO_IS_DUPLICATE_BIO. */ if (!clone || likely(dm_tio_is_normal(clone_to_tio(clone { + if (WARN_ON_ONCE(dm_io_flagged(io, DM_IO_ACCOUNTED))) + return; dm_io_set_flag(io, DM_IO_ACCOUNTED); } else { unsigned long flags; /* Can afford locking given DM_TIO_IS_DUPLICATE_BIO */ spin_lock_irqsave(>lock, flags); + if (dm_io_flagged(io, DM_IO_ACCOUNTED)) { + spin_unlock_irqrestore(>lock, flags); + return; + } dm_io_set_flag(io, DM_IO_ACCOUNTED); spin_unlock_irqrestore(>lock, flags); } -- 2.30.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: unset detect_checker for clariion / Unity arrays
On Wed, Jun 08, 2022 at 04:47:37PM +, Martin Wilck wrote: > On Wed, 2022-06-08 at 09:40 -0500, Benjamin Marzinski wrote: > > On Wed, Jun 08, 2022 at 07:56:27AM +, Martin Wilck wrote: > > > On Tue, 2022-06-07 at 17:45 -0500, Benjamin Marzinski wrote: > > > > Dell EMC would like to always use the emc_clariion checker. > > > > Currently > > > > detect_checker will switch the checker to TUR for Unity arrays. > > > > This can cause problems on some setups with replicated Unity > > > > LUNs, > > > > which are handled correctly the the emc_checker, but not the TUR > > > > checker. > > > > > > > > Cc: vincent.ch...@dell.com > > > > Signed-off-by: Benjamin Marzinski > > > > > > This points us to a flaw in our logic. > > > > > > It was wrong in the first place to have autodetection take > > > precedence, > > > even over "overrides". The effect for users is that whenever > > > "path_checker" is set, "detect_checker no" must also be set, which > > > is highly surprising and adds no benefit. > > > > > > We should assume that if a device has an explicit checker > > > configured > > > either in the device configuration, overrides, or the hwtable, this > > > checker must be used, and fall back to autodetection only if this > > > is > > > not the case. > > > > > > So while this patch is alright, I'd prefer a patch that fixes the > > > logic. > > > > I'm not sure that this is a good idea. IIRC, the current method was > > an > > intentional choice. The idea was that if a checker was autodetected, > > we > > would use that. If not, we would fall back to the user defined > > checker. > > For the most part this is useful for arrays that could be run in ALUA > > or > > non-alua mode. Changing the priority of detect_checker will suddenly > > change how these arrays are configured. Users that configured a > > failback checker for cases where their arrays weren't in ALUA mode > > would > > suddenly find that their arrays were always using the fallback > > method. > > > Now I'm not saying that the original logic was the best option. But I > > am > > saying that it hasn't caused many issues over the years that it's > > been > > in existance. There are a number of arrays in the builtin hardware > > table > > that define checkers. I assume that they either don't support ALUA, > > or > > they are happy with only using their configured checker when > > their > arrays aren't in ALUA mode. > > Most of the built-in hwtable entries set the RDAC checker. For these, > the result will be unchanged if we lower the precedence of > detect_checker. There are two entries setting DIRECTIO for non-SCSI > devices (DASD and Huawei NVMe): no regression risk there. Then there's > Clariion CX/AX/VNX, for which the change in behavior is intended. > Finally, there are two entries for ancient HPE devices using HP_SW. If > we change the precedence, these devices might switch from TUR to HP_SW > (if they support ALUA, dunno). As well as any user configurations that set this. I realize that it has been a while since the days when most of the default device configs explicitly set a checker, but I don't have any idea how many users are still running with a user config like that. > > I would rather fix the remaining cases where > > the existing priority gives the wrong answer than suddenly change how > > things work, in a way that could suddenly break things for an unknown > > (but quite possibly large) number of existing users. > > According to the above, only people who'd use very old HPE storage > arrays with latest upstream multipath-tools would be affected. > > Remain those users that you mentioned — people who operate in ALUA mode > but keep some explicit multipath.conf settings around as fallback "when > their arrays aren't in ALUA mode" (for whatever reason). These users > would now observe that their arrays operate in fallback mode, even if > ALUA was enabled. > > I'm not sure if this matters much. OTOH, it's rather common for people > to forget to set "detect_checker no" and wonder why their > multipath.conf settings don't take effect. But like my patch showed, it's a simple fix. If users wanted to define a fallback method for arrays that optionally support ALUA (and I admit that I have no idea how many arrays like this are still being used) it's a lot tricker if the devices section path_checker takes precedence. They can't set
Re: [dm-devel] [PATCH] multipath-tools: tests: fix hwtable test
On Thu, Jun 09, 2022 at 12:13:55PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > After "libmultipath: hwtable: new defaults for NVMe", we need to fix > the unit tests. > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > tests/hwtable.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/hwtable.c b/tests/hwtable.c > index e60e914..bfaf613 100644 > --- a/tests/hwtable.c > +++ b/tests/hwtable.c > @@ -573,7 +573,7 @@ static void test_internal_nvme(const struct hwt_state > *hwt) > assert_ptr_not_equal(mp, NULL); > TEST_PROP(checker_name(>checker), NONE); > TEST_PROP(pp->uid_attribute, DEFAULT_NVME_UID_ATTRIBUTE); > - assert_int_equal(mp->pgpolicy, DEFAULT_PGPOLICY); > + assert_int_equal(mp->pgpolicy, GROUP_BY_PRIO); > assert_int_equal(mp->no_path_retry, DEFAULT_NO_PATH_RETRY); > assert_int_equal(mp->retain_hwhandler, RETAIN_HWHANDLER_OFF); > > @@ -586,7 +586,7 @@ static void test_internal_nvme(const struct hwt_state > *hwt) > assert_ptr_not_equal(mp, NULL); > TEST_PROP(checker_name(>checker), NONE); > TEST_PROP(pp->uid_attribute, "ID_WWN"); > - assert_int_equal(mp->pgpolicy, MULTIBUS); > + assert_int_equal(mp->pgpolicy, GROUP_BY_PRIO); > assert_int_equal(mp->no_path_retry, NO_PATH_RETRY_QUEUE); > assert_int_equal(mp->retain_hwhandler, RETAIN_HWHANDLER_OFF); > } > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: hwtable: remove ETERNUS AB/HB NVMe
On Thu, Jun 09, 2022 at 11:05:54AM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > The previous patch "libmultipath: hwtable: remove obsolete NVMe entries" > removed the entry for NetApp E-Series NVMe, but not the corresponding OEM > entry for ETERNUS AB/HB NVMe. Fix it. > > Cc: Steven Schremmer > Cc: Xose Vazquez Perez > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > libmultipath/hwtable.c | 6 -- > 1 file changed, 6 deletions(-) > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > index fd4573f..513fa67 100644 > --- a/libmultipath/hwtable.c > +++ b/libmultipath/hwtable.c > @@ -468,12 +468,6 @@ static struct hwentry default_hw[] = { > .pgfailback= -FAILBACK_IMMEDIATE, > .no_path_retry = 30, > }, > - { > - /* ETERNUS AB/HB NVMe */ > - .vendor= "NVME", > - .product = "Fujitsu ETERNUS AB/HB Series", > - .no_path_retry = 30, > - }, > /* >* Hitachi Vantara >* > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath, kpartx: fix callers of dm_get_next_target()
On Tue, May 31, 2022 at 12:24:45PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > dm_get_next_target() may return NULL for target_type and params > in some situations. Follows the same idea as the previous patch > "dm_get_map: fix segfault when can't found target" by Wu Guanghao. > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > kpartx/devmapper.c | 6 -- > libmultipath/devmapper.c | 10 ++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/kpartx/devmapper.c b/kpartx/devmapper.c > index 49ffd31..bf14c78 100644 > --- a/kpartx/devmapper.c > +++ b/kpartx/devmapper.c > @@ -412,8 +412,10 @@ dm_get_map(const char *mapname, char * outparams) > goto out; > > /* Fetch 1st target */ > - dm_get_next_target(dmt, NULL, , , > -_type, ); > + if (dm_get_next_target(dmt, NULL, , , > +_type, ) != NULL || !params) > + /* more than one target or not found target */ > + goto out; > > if (snprintf(outparams, PARAMS_SIZE, "%s", params) <= PARAMS_SIZE) > r = 0; > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 9819e29..1ada3fb 100644 > --- a/libmultipath/devmapper.c > +++ b/libmultipath/devmapper.c > @@ -1715,6 +1715,16 @@ int dm_reassign_table(const char *name, char *old, > char *new) > do { > next = dm_get_next_target(dmt, next, , , > , ); > + if (!target || !params) { > + /* > + * We can't call dm_task_add_target() with > + * invalid parameters. But simply dropping this > + * target feels wrong, too. Abort and warn. > + */ > + condlog(1, "%s: invalid target found in map %s", > + __func__, name); > + goto out_reload; > + } > buff = strdup(params); > if (!buff) { > condlog(3, "%s: failed to replace target %s, " > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [RFC PATCH 0/2] multipath-tools: simplify defaults for NVMe
On Wed, Jun 01, 2022 at 10:26:26PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Using dm-multipath with NVMe devices is increasingly becoming a niche > configuration, as it's recommended against by the kernel community and > various vendors. Some vendors would prefer not to see their devices listed > in the libmultipath hwtable to avoid the impression on users that this > setup was supported by the vendor. > > This RFC patch set makes it possible to remove multiple recently added > hwtable entries by changing the generic NVMe defaults. The only > device-specific settings that remain are those for no_path_retry. For the set: Reviewed-by: Benjamin Marzinski > > Martin Wilck (2): > libmultipath: hwtable: new defaults for NVMe > libmultipath: hwtable: remove obsolete NVMe entries > > libmultipath/hwtable.c | 30 ++ > 1 file changed, 2 insertions(+), 28 deletions(-) > > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] libmultipath: unset detect_checker for clariion / Unity arrays
On Wed, Jun 08, 2022 at 07:56:27AM +, Martin Wilck wrote: > On Tue, 2022-06-07 at 17:45 -0500, Benjamin Marzinski wrote: > > Dell EMC would like to always use the emc_clariion checker. Currently > > detect_checker will switch the checker to TUR for Unity arrays. > > This can cause problems on some setups with replicated Unity LUNs, > > which are handled correctly the the emc_checker, but not the TUR > > checker. > > > > Cc: vincent.ch...@dell.com > > Signed-off-by: Benjamin Marzinski > > This points us to a flaw in our logic. > > It was wrong in the first place to have autodetection take precedence, > even over "overrides". The effect for users is that whenever > "path_checker" is set, "detect_checker no" must also be set, which > is highly surprising and adds no benefit. > > We should assume that if a device has an explicit checker configured > either in the device configuration, overrides, or the hwtable, this > checker must be used, and fall back to autodetection only if this is > not the case. > > So while this patch is alright, I'd prefer a patch that fixes the > logic. I'm not sure that this is a good idea. IIRC, the current method was an intentional choice. The idea was that if a checker was autodetected, we would use that. If not, we would fall back to the user defined checker. For the most part this is useful for arrays that could be run in ALUA or non-alua mode. Changing the priority of detect_checker will suddenly change how these arrays are configured. Users that configured a failback checker for cases where their arrays weren't in ALUA mode would suddenly find that their arrays were always using the fallback method. Now I'm not saying that the original logic was the best option. But I am saying that it hasn't caused many issues over the years that it's been in existance. There are a number of arrays in the builtin hardware table that define checkers. I assume that they either don't support ALUA, or they are happy with only using their configured checker when their arrays aren't in ALUA mode. I would rather fix the remaining cases where the existing priority gives the wrong answer than suddenly change how things work, in a way that could suddenly break things for an unknown (but quite possibly large) number of existing users. > (The same could be said for detect_prio, but I don't want to make > outrageous demands). The above arguments apply here, only moreso. -Ben > Martin > > > > > > > --- > > libmultipath/hwtable.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > > index 39daadc2..415bf683 100644 > > --- a/libmultipath/hwtable.c > > +++ b/libmultipath/hwtable.c > > @@ -350,6 +350,7 @@ static struct hwentry default_hw[] = { > > .no_path_retry = (300 / DEFAULT_CHECKINT), > > .checker_name = EMC_CLARIION, > > .prio_name = PRIO_EMC, > > + .detect_checker = DETECT_CHECKER_OFF, > > }, > > { > > /* Invista / VPLEX */ -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: unset detect_checker for clariion / Unity arrays
Dell EMC would like to always use the emc_clariion checker. Currently detect_checker will switch the checker to TUR for Unity arrays. This can cause problems on some setups with replicated Unity LUNs, which are handled correctly the the emc_checker, but not the TUR checker. Cc: vincent.ch...@dell.com Signed-off-by: Benjamin Marzinski --- libmultipath/hwtable.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c index 39daadc2..415bf683 100644 --- a/libmultipath/hwtable.c +++ b/libmultipath/hwtable.c @@ -350,6 +350,7 @@ static struct hwentry default_hw[] = { .no_path_retry = (300 / DEFAULT_CHECKINT), .checker_name = EMC_CLARIION, .prio_name = PRIO_EMC, + .detect_checker = DETECT_CHECKER_OFF, }, { /* Invista / VPLEX */ -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath.conf(5): add disclaimer about vendor support
On Tue, May 31, 2022 at 10:39:05PM +0200, mwi...@suse.com wrote: > From: Martin Wilck Reviewed-by: Benjamin Marzinski > > Add a disclaimer to clarify that entries in the hwtable don't imply any > obligations on the vendor's part. > --- > multipath/multipath.conf.5 | 12 > 1 file changed, 12 insertions(+) > > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 > index d57c810..c2d34f1 100644 > --- a/multipath/multipath.conf.5 > +++ b/multipath/multipath.conf.5 > @@ -1490,6 +1490,18 @@ section: > .SH "devices section" > .\" > > . > +.TP 4 > +.B Important: > +The built-in hardware device table of > +.I multipath-tools > +is created by members of the Linux community in the hope that it will be > useful. > +The existence of an entry for a given storage product in the hardware table > +.B does not imply > +that the product vendor supports, or has tested, the product with > +.I multipath-tools > +in any way. > +.B Always consult the vendor\(aqs official documentation for support-related > information. > +.PP > \fImultipath-tools\fR have a built-in device table with reasonable defaults > for more than 100 known multipath-capable storage devices. The devices > section > can be used to override these settings. If there are multiple matches for a > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 1/2] libmultipath: hwtable: new defaults for NVMe
On Wed, Jun 01, 2022 at 10:26:27PM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > So far we were using the general defaults (pgpolicy = FAILOVER, > pgfailback = -FAILBACK_MANUAL). Xosé's late patches were setting this to > either MULTIBUS or ANA, and -FAILBACK_IMMEDIATE, respectively > for most specific arrays. At the same time, some vendors don't like > seeing their NVMe arrays listed specifically in the multipath-tools > hwtable. > > IMO it makes sense to change the general defaults. > > detect_prio is the default, and we probe for ANA support. Thus prio > will be "ana" for arrays that support it, and "const" otherwise. > With "const", GROUP_BY_PRIO degenerates to MULTIBUS, and pgfailback > won't happen anyway. This way, our defaults match most Xosé's new entries. The > only devices for which this patch changes behavior (from FAILOVER to MULTIBUS) > are those generic devices that aren't listed, and don't support ANA. > > I considered changing the default for no_path_retry, too, but decided against > it. The default is "fail", and users who dislike that will need to change it. > no_path_retry is more a policy setting than a hardware property, anyway. I agree that these new defaults are sensible, but this patch will cause some user's arrays to change configuration when they update. I'm not against doing this at all, but this is one of those patches that distributions need to take some care with, so that they can make this change at a sensible time. So, unless there are other objections, I'm O.k. with this patch set, I just wanted to point this out. -Ben > --- > libmultipath/hwtable.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > index 39daadc..e0dce84 100644 > --- a/libmultipath/hwtable.c > +++ b/libmultipath/hwtable.c > @@ -90,7 +90,8 @@ static struct hwentry default_hw[] = { > .product = ".*", > .uid_attribute = DEFAULT_NVME_UID_ATTRIBUTE, > .checker_name = NONE, > - .retain_hwhandler = RETAIN_HWHANDLER_OFF, > + .pgpolicy = GROUP_BY_PRIO, > + .pgfailback= -FAILBACK_IMMEDIATE, > }, > /* >* Apple > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 4/9] multipath-tools: add NetApp E-Series NVMe to hardware table
On Tue, May 31, 2022 at 10:43:01AM +0200, Martin Wilck wrote: > Hi Steve, > > On Thu, 2022-05-26 at 20:10 +, Schremmer, Steven wrote: > > > From: Martin Wilck > > > Sent: Thursday, May 19, 2022 9:47 AM > > > Hi Steve, > > > > > > On Thu, 2022-05-19 at 13:18 +, Schremmer, Steven wrote: > > > > Martin W, > > > > > > > > > Steve, > > > > > > > > > > On Wed, 2022-05-18 at 20:24 +, Schremmer, Steven wrote: > > > > > > > > > > > > Nak. NetApp E-Series only supports these settings in certain > > > > > > configurations, and we prefer to handle it via our > > > > > > installation > > > > > > documentation. > > > > > > > > > > > > > > > > I don't follow. What harm is done to Netapp if these settings > > > > > are > > > > > included? People can still follow your documentation, the end > > > > > result > > > > > will be the same... no? > > > > > > > > > > AFAICS, the only setting above that would only be supported in > > > > > certain > > > > > configurations is PRIO_ANA, without which GROUP_BY_PRIO doesn't > > > > > make > > > > > much sense. If the array is configured not to support ANA, this > > > > > configuration would lead to error messages and PRIO_UNDEF for > > > > > all > > > > > paths, and thus "imply" multibus topology. Not beautiful, but > > > > > also > > > > > no > > > > > big harm done, IMO. > > > > > > > > > > If it's that you're concerned about, please provide the set of > > > > > defaults > > > > > you prefer for E-Series, or explictly state that you prefer to > > > > > run > > > > > with > > > > > the generic NVMe defaults (const prio, failover policy). > > > > > > > > > > In general, if vendor-recommended settings are strongly > > > > > dependent > > > > > on > > > > > storage configuration, host-side software defaults must try to > > > > > match > > > > > the storage array's defaults. We shoud do this for E-Series, > > > > > too. > > > > > If > > > > > ANA needs to be explicitly enabled on the array by the admin, > > > > > we > > > > > shouldn't enable it by default; but otherwise, we should. > > > > > > > > > > Martin > > > > > > > > NVMe-attached E-Series is moving towards only using the native > > > > NVMe > > > > multipathing in the kernel rather than DM-MP with NVMe. At some > > > > point > > > > we will stop interoperability testing with NVMe DM-MP and not > > > > certify > > > > new > > > > solutions with it. > > > > > > > > The set of defaults listed for NVMe E-Series are the correct > > > > ones, > > > > but I'm not sure > > > > they should be included if we aren't going to continue to test > > > > the > > > > interoperability > > > > of NVMe-attached E-Series and DM-MP. > > > > > > Thanks for the explanation. > > > > > > I believe everyone understands that the fact that the built-in > > > hwtable > > > in multipath-tools contains sensible defaults for a given storage > > > array > > > does *not* imply that this array has been tested or officially > > > released > > > by Netapp (or any storage vendor). If you want, we can add a > > > statement > > > of this kind (vendor-neutral) to our man page and/or README. > > > > > > It's also understood that Netapp, like the kernel community, > > > recommends > > > native multipathing for NVMe, and discourages use of device-mapper > > > multipath for NVMe devices. Native multipathing is the kernel > > > default, > > > and must be explicitly disabled either at build time or on the > > > kernel > > > command line before dm-multipath would even see the devices. IMO it > > > can > > > be assumed that a user who employs such a setup knows what she's > > > doing, > > > and is aware that the setup doesn't comply with common > > > recommendations. > > > > > > Netapp currently publishes configuration recommendations for > > > multipath- > > > tools with E-Series and NVMe. Xose's patch simply changes the > > > built-in > > > defaults to match these recommendations. We have been doing this > > > for > > > years with the intention to improve the "out of the box" > > > experience, > > > and it's a good thing. > > > > > > If we didn't take this patch, we'd knowingly force suboptimal > > > default > > > settings on (admittedly few) users. Who would benefit from that? > > > > > > We want to ship multipath-tools with the most reasonable defaults > > > that > > > we are aware of. Xose's continued efforts in this area have been > > > immensely useful for the community of dm-multipath users. I don't > > > think > > > we should give this up. I'd like to encourage everyone to continue > > > submitting improvements for the hardware table. > > > > > > Regards, > > > Martin > > > > > > > Martin, > > > > Sorry for being slow to respond to this. NetApp publishes settings > > for > > multipath-tools for NVMe-attach E-Series for specific distribution > > versions > > that we have qualified. Anyone using these settings outside of these > > versions would NOT be in a valid system configuration for NetApp > > support. > > Anyone using wrong or
Re: [dm-devel] [PATCH]dm_get_map: fix segfault when can't found target
On Fri, May 27, 2022 at 10:27:37AM +0800, Wu Guanghao wrote: > We got a segfault when we test multipath + iscsi. > > (gdb) bt > #0 __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:74 > #1 0x7f694306cd23 in __GI___strdup (s=0x0) at strdup.c:41 > #2 0x7f69433a147c in dm_get_map (name=0x55d4bc090320 > "3600140537cde137ea8c43d8a971462c7", > size=size@entry=0x55d4bc0270d8, outparams=outparams@entry=0x7f6941add640) > at devmapper.c:688 > #3 0x7f69433cbbdf in update_multipath_table > (mpp=mpp@entry=0x55d4bc026f30, > pathvec=pathvec@entry=0x55d4bc063990, flags=flags@entry=0) at > structs_vec.c:426 > #4 0x7f69433cbfe5 in update_multipath_strings (mpp=0x55d4bc026f30, > pathvec=0x55d4bc063990) > at structs_vec.c:526 > #5 0x55d4bb52e03e in check_path (vecs=0x55d4bbfad760, pp=0x7f692402d270, > ticks=) > at main.c:2280 > #6 0x55d4bb52f3e2 in checkerloop (ap=0x55d4bbfad760) at main.c:2542 > #7 0x7f694305b3ba in start_thread (arg=) at > pthread_create.c:443 > #8 0x7f69430ddb40 in clone3 () at > ../sysdeps/unix/sysv/linux/x86_64/clone3.S:81 > (gdb) f 2 > #2 0x7f69433a147c in dm_get_map (name=0x55d4bc090320 > "3600140537cde137ea8c43d8a971462c7", > size=size@entry=0x55d4bc0270d8, outparams=outparams@entry=0x7f6941add640) > at devmapper.c:688 > 688 *outparams = strdup(params); > (gdb) l > 683 *size = length; > 684 > 685 if (!outparams) > 686 r = DMP_OK; > 687 else { > 688 *outparams = strdup(params); > 689 r = *outparams ? DMP_OK : DMP_ERR; > 690 } > 691 > 692 out: > (gdb) p params > $1 = 0x0 > > If can't found target, we should goto out > > Signed-off-by: Wu Guanghao Reviewed-by: Benjamin Marzinski > --- > libmultipath/devmapper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c > index 2507f77f..450b17ef 100644 > --- a/libmultipath/devmapper.c > +++ b/libmultipath/devmapper.c > @@ -682,8 +682,8 @@ int dm_get_map(const char *name, unsigned long long > *size, char **outparams) > r = DMP_NOT_FOUND; > /* Fetch 1st target */ > if (dm_get_next_target(dmt, NULL, , , > - _type, ) != NULL) > - /* more than one target */ > + _type, ) != NULL || !params) > + /* more than one target or not found target */ > goto out; > > if (size) > -- > 2.27.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/9] add new devices to hw table
On Sun, May 15, 2022 at 01:04:31AM +0200, Xose Vazquez Perez wrote: > Xose Vazquez Perez (9): > multipath-tools: fix misspellings > multipath-tools: add HPE Alletra 9000 NVMe to hardware table > multipath-tools: delete redundant ONTAP NVMe comment > multipath-tools: add NetApp E-Series NVMe to hardware table > multipath-tools: add Huawei OceanStor NVMe to hardware table > multipath-tools: add IBM FlashSystem(TMS RamSan) NVMe to hardware table > multipath-tools: add IBM FlashSystem(Storwize/SVC) NVMe to hardware table > multipath-tools: add Pure FlashArray NVMe to hardware table > multipath-tools: add Dell EMC PowerStore NVMe to hardware table For the set: Reviewed-by: Benjamin Marzinski > > README.md | 2 +- > libmultipath/checkers/rdac.c| 2 +- > libmultipath/hwtable.c | 60 ++--- > libmultipath/prioritizers/iet.c | 2 +- > multipath/multipath.conf.5 | 2 +- > tests/directio.c| 2 +- > 6 files changed, 60 insertions(+), 10 deletions(-) > > Cc: NetApp RDAC team > Cc: Uday Shankar > Cc: Brian Bunker > Cc: Zhouweigang (Jack) > Cc: Zou Ming > Cc: Martin Wilck > Cc: Benjamin Marzinski > Cc: Christophe Varoqui > Cc: DM-DEVEL ML > -- > 2.36.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: Makefile.inc: add test for -D_FORTIFY_SOURCE=3
On Mon, May 09, 2022 at 05:21:27PM +0200, mwi...@suse.com wrote: > From: Martin Wilck Reviewed-by: Benjamin Marzinski > > 6186645 ("Fix possibility to redefine -D_FORTIFY_SOURCE macro.") > does not work as-is, because OPTFLAGS can't be used to override CPPFLAGS. > Instead, add a test for support of -D_FORTIFY_SOURCE=3, and use it > automatically if supported. The test uses similar logic as e.g. > https://sourceware.org/git/?p=elfutils.git;a=commit;h=29859f2e79ef3c650ee9712cae990c6a7f787a7d > > This test works in environments with glibc 2.33 or newer. On older > distributions, > -D_FORTIFY_SOURCE=3 does not cause an error, and will thus be used. In this > case, it has the same effect as -D_FORTIFY_SOURCE=2. On alpine Linux (musl > libc), -D_FORTIFY_SOURCE=3 generates an error. > > Fixes: 6186645 ("Fix possibility to redefine -D_FORTIFY_SOURCE macro.") > Signed-off-by: Martin Wilck > --- > Makefile.inc | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/Makefile.inc b/Makefile.inc > index cef7a06..b915c06 100644 > --- a/Makefile.inc > +++ b/Makefile.inc > @@ -117,6 +117,20 @@ TEST_CC_OPTION = $(shell \ > echo "$(2)"; \ > fi) > > +# "make" on some distros will fail on explicit '#' or '\#' in the program > text below > +__HASH__ := \# > +# Check if _DFORTIFY_SOURCE=3 is supported. > +# On some distros (e.g. Debian Buster) it will be falsely reported as > supported > +# but it doesn't seem to make a difference wrt the compilation result. > +FORTIFY_OPT := $(shell \ > + if /bin/echo -e '$(__HASH__)include \nint main(void) { return > 0; }' | \ > + $(CC) -o /dev/null -c -O2 -Werror -D_FORTIFY_SOURCE=3 -xc - > 2>/dev/null; \ > + then \ > + echo "-D_FORTIFY_SOURCE=3"; \ > + else \ > + echo "-D_FORTIFY_SOURCE=2"; \ > + fi) > + > STACKPROT := $(call > TEST_CC_OPTION,-fstack-protector-strong,-fstack-protector) > ERROR_DISCARDED_QUALIFIERS := $(call > TEST_CC_OPTION,-Werror=discarded-qualifiers,) > WNOCLOBBERED := $(call TEST_CC_OPTION,-Wno-clobbered -Wno-error=clobbered,) > @@ -126,7 +140,7 @@ OPTFLAGS := -O2 -g $(STACKPROT) --param=ssp-buffer-size=4 > WARNFLAGS:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) > -Werror=implicit-int \ > -Werror=implicit-function-declaration -Werror=format-security > \ > $(WNOCLOBBERED) -Werror=cast-qual > $(ERROR_DISCARDED_QUALIFIERS) > -CPPFLAGS := -D_FORTIFY_SOURCE=2 > +CPPFLAGS := $(FORTIFY_OPT) > CFLAGS := --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe > \ > -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" > -DRUN_DIR=\"${RUN}\" \ > -DCONFIG_DIR=\"$(configdir)\" > -DEXTRAVERSION=\"$(EXTRAVERSION)\" -MMD -MP > -- > 2.36.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/7] multipath-tools: fix unit test breakage
On Wed, May 04, 2022 at 12:06:39AM +0200, mwi...@suse.com wrote: > From: Martin Wilck > > Hi Christophe, hi Ben, > > The previous patches > > af15832 multipath-tools: make multipath_dir a compiled-in option > 1fc7c4d multipath-tools: make config_dir a compiled-in option > 21b3d6b multipath-tools: stop supporting getuid_callout > > have caused breakage in the unit tests. Not so much because they'd > actually break the tests, but because they broke the compilation and > the search paths for the configuration files (for the former two commits) > and because the hwtable test relied on being able to the retrieve > the now removed "getuid_callout" property from config files. > > Fix it. As always, comments welcome. For the set, Reviewed-by: Benjamin Marzinski > > Regards > Martin > > Martin Wilck (7): > multipath-tools: enable local configdir and plugindir for unit tests > tests/mpathvalid: fix check for DEFAULT_CONFIG_FILE > multipath-tools tests: hwtable: adapt after removal of getuid > tests/hwtable: fix test_regex_2_strings_hwe_dir > libmultipath: fix valgrind_test > tests/Makefile: make TESTDIR configurable > github workflows: foreign.yaml: fix config dir in run environment > > .github/workflows/foreign.yaml | 7 +- > libmultipath/Makefile | 24 -- > libmultipath/config.c | 3 + > tests/Makefile | 12 ++- > tests/hwtable.c| 151 - > tests/mpathvalid.c | 2 +- > tests/test-lib.c | 89 ++- > tests/test-lib.h | 2 +- > 8 files changed, 180 insertions(+), 110 deletions(-) > > -- > 2.36.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] update HPE MSA builtin config
On Mon, May 02, 2022 at 11:01:40AM -0500, Benjamin Marzinski wrote: > On Mon, May 02, 2022 at 10:31:36AM +, Martin Wilck wrote: > > On Fri, 2022-04-29 at 17:09 -0500, Benjamin Marzinski wrote: > > > Make the config better align to MSA 4th, 5th and 6th Generation > > > systems. > > > > > > Suggested-by: Jon Paul > > > Signed-off-by: Benjamin Marzinski > > > --- > > > libmultipath/hwtable.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > > > index 0e1c0a41..0f0795c3 100644 > > > --- a/libmultipath/hwtable.c > > > +++ b/libmultipath/hwtable.c > > > @@ -182,8 +182,8 @@ static struct hwentry default_hw[] = { > > > }, > > > { > > > /* MSA 1040, 1050, 1060, 2040, 2050 and 2060 families > > > */ > > > - .vendor = "HP", > > > - .product = "MSA [12]0[456]0 SA[NS]", > > > + .vendor = "(HP|HPE)", > > > > This changes nothing unless you use "^(HP|HPE)$", as our regex matching > > is by substring. > > Fair enough. I'm just passing along the config I got. I'm not sure if > they what it to be more immediately obvious to people looking at the > configs that this applies to their device (which could have a vendor > string of "HPE"). I'll leave it to HP to decide what, if anything, they > want to change here. I've talked to HP, and they would like to keep (HP|HPE), since they feel that it's more obvious to readers. They are fine with whatever anchors we feel should be added to the regular expression. I personally don't think it matters much. Most of our vendor strings are unanchored and since the product string must also match, it seems unlikely to cause issues. Do you have a strong feeling on this? Otherwise, I'm fine with the config as they gave it to me. -Ben > > -Ben > > > Regards, > > Martin > > > > > > > + .product = "MSA [12]0[456]0 > > > (SAN|SAS|FC|iSCSI)", > > > .pgpolicy = GROUP_BY_PRIO, > > > .pgfailback = -FAILBACK_IMMEDIATE, > > > .no_path_retry = 18, > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://listman.redat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] update HPE MSA builtin config
On Mon, May 02, 2022 at 10:31:36AM +, Martin Wilck wrote: > On Fri, 2022-04-29 at 17:09 -0500, Benjamin Marzinski wrote: > > Make the config better align to MSA 4th, 5th and 6th Generation > > systems. > > > > Suggested-by: Jon Paul > > Signed-off-by: Benjamin Marzinski > > --- > > libmultipath/hwtable.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c > > index 0e1c0a41..0f0795c3 100644 > > --- a/libmultipath/hwtable.c > > +++ b/libmultipath/hwtable.c > > @@ -182,8 +182,8 @@ static struct hwentry default_hw[] = { > > }, > > { > > /* MSA 1040, 1050, 1060, 2040, 2050 and 2060 families > > */ > > - .vendor = "HP", > > - .product = "MSA [12]0[456]0 SA[NS]", > > + .vendor = "(HP|HPE)", > > This changes nothing unless you use "^(HP|HPE)$", as our regex matching > is by substring. Fair enough. I'm just passing along the config I got. I'm not sure if they what it to be more immediately obvious to people looking at the configs that this applies to their device (which could have a vendor string of "HPE"). I'll leave it to HP to decide what, if anything, they want to change here. -Ben > Regards, > Martin > > > > + .product = "MSA [12]0[456]0 > > (SAN|SAS|FC|iSCSI)", > > .pgpolicy = GROUP_BY_PRIO, > > .pgfailback = -FAILBACK_IMMEDIATE, > > .no_path_retry = 18, > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH] multipath-tools: prevent a kernel upgrade leading to all paths lost
On Mon, Apr 18, 2022 at 02:54:15PM -0700, Brian Bunker wrote: > With the kernel commit 268940b80fa4096397fd0a28e6ad807e64120215, the > handling of ALUA state transitioning state changed. It used to be that > paths which returned with ALUA transitioning state would not result in > those paths being failed. With this patch, that state returned will > result in the path being failed. The result of this will result in all > paths down events when a configuration has the no_path_retry set to 0. > Before this kernel change that same configuration would not reach all > paths down. > > If the kernel is not changed back to the previous behavior, the > multipath-tools have to protect against this condition or > configurations which were working correctly prior to the kernel change > will lead to an unexpected failure. Unless I'm missing something, this patch isn't going to be helpful. It doesn't configure the multipath device to queue IOs before the failure. When the kernel runs out of paths, it will see that the mutltipath device has no paths and isn't set to queue, and it will fail the IOs. After the paths come back up again, multipath will enable queueing in update_queue_mode_add_path(). But it's too late by then. The IOs will have already been failed. Another issue is that the patch is doing scsi specific work in code meant for general block devices. Finally, It's seems pointless to set the device to queue when all the paths go away and the turn off queueing when the paths come back. no_path_retry doesn't do anything when there are paths, so if it's necessary to set when the paths are gone, it won't hurt anything to be set when the paths are present. I understand that you only want to do this for devices in the ALUA transtitioning state, but the only why to make this actually help is if multipathd enables queueing before the device ever gets into that state. The only really safe thing to do would be to set no_path_retry to something other than 0 or NO_PATH_RETRY_QUEUE. This can already be done in the configuration files. But I agree that failing in IOs because the device is transitioning seems wrong in the first place, and fixing this in the kernel makes sense. > See the kernel discussions here: > https://marc.info/?l=linux-scsi=162636826305740=2 > https://marc.info/?l=linux-scsi=164987222322261=2 > > Signed-off-by: br...@purestorage.com > Reviewed-by: scon...@purestorage.com > ___ > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index d94f93a0..0af7e598 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -370,6 +370,7 @@ struct multipath { > int failback_tick; > > int no_path_retry; /* number of retries after all paths are down */ > + int no_path_retry_configured; /* value in config */ > int retry_tick;/* remaining times for retries */ > int disable_queueing; > int minio; > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index 6c23df86..3db4e6f7 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -780,8 +780,15 @@ void update_queue_mode_add_path(struct multipath *mpp) > { > int active = count_active_paths(mpp); > > - if (active > 0) > + if (active > 0) { > leave_recovery_mode(mpp); > + if (mpp->no_path_retry != mpp->no_path_retry_configured) { > + condlog(2, "%s: return no path retry to %d > from %d", mpp->alias, > + mpp->no_path_retry_configured, mpp->no_path_retry); > + mpp->no_path_retry = mpp->no_path_retry_configured; > + } > + } > + > condlog(2, "%s: remaining active paths: %d", mpp->alias, active); > } > > diff --git a/multipathd/main.c b/multipathd/main.c > index f2c0b280..92841d13 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -524,7 +524,7 @@ int update_multipath (struct vectors *vecs, char > *mapname, int reset) > struct multipath *mpp; > struct pathgroup *pgp; > struct path *pp; > - int i, j; > + int i, j, tpg; > > mpp = find_mp_by_alias(vecs->mpvec, mapname); > > @@ -553,6 +553,15 @@ int update_multipath (struct vectors *vecs, char > *mapname, int reset) > checkint = conf->checkint; > put_multipath_config(conf); > condlog(2, "%s: mark as failed", pp->dev); > + tpg = get_target_port_group(pp, DEF_TIMEOUT); > + if ((tpg >= 0) && > + (mpp->no_path_retry == 0) && > + (get_asymmetric_access_state(pp, > tpg, DEF_TIMEOUT) == AAS_TRANSITIONING)) { > + mpp->no_path_retry_configured > = mpp->no_path_retry; > + mpp->no_path_retry = (60 / checkint); > +
[dm-devel] [PATCH] update HPE MSA builtin config
Make the config better align to MSA 4th, 5th and 6th Generation systems. Suggested-by: Jon Paul Signed-off-by: Benjamin Marzinski --- libmultipath/hwtable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c index 0e1c0a41..0f0795c3 100644 --- a/libmultipath/hwtable.c +++ b/libmultipath/hwtable.c @@ -182,8 +182,8 @@ static struct hwentry default_hw[] = { }, { /* MSA 1040, 1050, 1060, 2040, 2050 and 2060 families */ - .vendor= "HP", - .product = "MSA [12]0[456]0 SA[NS]", + .vendor= "(HP|HPE)", + .product = "MSA [12]0[456]0 (SAN|SAS|FC|iSCSI)", .pgpolicy = GROUP_BY_PRIO, .pgfailback= -FAILBACK_IMMEDIATE, .no_path_retry = 18, -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/7] multipath-tools: remove deprecated options
On Thu, Apr 14, 2022 at 03:18:04PM +0200, mwi...@suse.com wrote: For the set: Reviewed-by: Benjamin Marzinski -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH] libmultipath: use symbolic value for invalid pcentry
Suggested-by: Martin Wilck Signed-off-by: Benjamin Marzinski --- libmultipath/config.c | 4 ++-- libmultipath/config.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index c3cfa119..54c28d24 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -383,7 +383,7 @@ alloc_pce (void) { struct pcentry *pce = (struct pcentry *) calloc(1, sizeof(struct pcentry)); - pce->type = -1; + pce->type = PCE_INVALID; return pce; } @@ -642,7 +642,7 @@ validate_pctable(struct hwentry *ovr, int idx, const char *table_desc) return; vector_foreach_slot_after(ovr->pctable, pce, idx) { - if (pce->type < 0) { + if (pce->type == PCE_INVALID) { condlog(0, "protocol section in %s missing type", table_desc); vector_del_slot(ovr->pctable, idx--); diff --git a/libmultipath/config.h b/libmultipath/config.h index 4f1a18a1..45cf9b35 100644 --- a/libmultipath/config.h +++ b/libmultipath/config.h @@ -40,6 +40,7 @@ enum force_reload_types { FORCE_RELOAD_WEAK, }; +#define PCE_INVALID -1 struct pcentry { int type; int fast_io_fail; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 6/7] libmultipath: fix eh_deadline documentation
Signed-off-by: Benjamin Marzinski --- multipath/multipath.conf.5 | 4 1 file changed, 4 insertions(+) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 605b46e0..97695da4 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -1636,6 +1636,8 @@ section: .TP .B dev_loss_tmo .TP +.B eh_deadline +.TP .B flush_on_last_del .TP .B user_friendly_names @@ -1722,6 +1724,8 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections: .TP .B dev_loss_tmo .TP +.B eh_deadline +.TP .B user_friendly_names .TP .B retain_attached_hw_handler -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 7/7] libmultipath: Add documentation for the protocol subsection
Signed-off-by: Benjamin Marzinski --- multipath/multipath.conf.5 | 32 1 file changed, 32 insertions(+) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 97695da4..198a79dd 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -1764,6 +1764,38 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections: .RE .PD .LP +The overrides section also recognizes the optional \fIprotocol\fR subsection, +and can contain multiple protocol subsections. Path devices are matched against +the protocol subsection using the mandatory \fItype\fR attribute. Attributes +in a matching protocol subsection take precedence over attributes in the rest +of the overrides section. If there are multiple matching protocol subsections, +later entries take precedence. +.TP +.B protocol subsection +The protocol subsection recognizes the following mandatory attribute: +.RS +.TP +.B type +The protocol string of the path device. The possible values are \fIscsi:fcp\fR, +\fIscsi:spi\fR, \fIscsi:ssa\fR, \fIscsi:sbp\fR, \fIscsi:srp\fR, +\fIscsi:iscsi\fR, \fIscsi:sas\fR, \fIscsi:adt\fR, \fIscsi:ata\fR, +\fIscsi:unspec\fR, \fIccw\fR, \fIcciss\fR, \fInvme\fR, and \fIundef\fR. This is +\fBnot\fR a regular expression. the path device protcol string must match +exactly. The protocol that a path is using can be viewed by running +\fBmultipathd show paths format "%d %P"\fR +.LP +The following attributes are optional; if not set, the default values are taken +from the \fIoverrides\fR, \fIdevices\fR, or \fIdefaults\fR section: +.sp 1 +.PD .1v +.RS +.TP +.B fast_io_fail_tmo +.TP +.B dev_loss_tmo +.TP +.B eh_deadline +.PD . . .\" -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 3/7] libmultipath: add a protocol subsection to multipath.conf
Some storage arrays can be accessed using multiple protocols at the same time. In these cases, users may want to set path attributes differently, depending on the protocol that the path is using. To allow this, add a protocol subsection to the overrides section in multipath.conf, which allows select path-specific options to be set. This commit simply adds the subsection, and handles merging matching entries. Future patches will make use of the section. Signed-off-by: Benjamin Marzinski --- libmultipath/config.c | 83 libmultipath/config.h | 10 + libmultipath/dict.c | 99 +++ libmultipath/print.c | 50 ++ 4 files changed, 242 insertions(+) diff --git a/libmultipath/config.c b/libmultipath/config.c index 612941b8..5fe71562 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -237,6 +237,18 @@ const char *get_mpe_wwid(const struct _vector *mptable, const char *alias) return NULL; } +static void +free_pctable (vector pctable) +{ + int i; + struct pcentry *pce; + + vector_foreach_slot(pctable, pce, i) + free(pce); + + vector_free(pctable); +} + void free_hwe (struct hwentry * hwe) { @@ -282,6 +294,9 @@ free_hwe (struct hwentry * hwe) if (hwe->bl_product) free(hwe->bl_product); + if (hwe->pctable) + free_pctable(hwe->pctable); + free(hwe); } @@ -363,6 +378,15 @@ alloc_hwe (void) return hwe; } +struct pcentry * +alloc_pce (void) +{ + struct pcentry *pce = (struct pcentry *) + calloc(1, sizeof(struct pcentry)); + pce->type = -1; + return pce; +} + static char * set_param_str(const char * str) { @@ -396,6 +420,13 @@ set_param_str(const char * str) if (!dst->s && src->s) \ dst->s = src->s +static void +merge_pce(struct pcentry *dst, struct pcentry *src) +{ + merge_num(fast_io_fail); + merge_num(dev_loss); + merge_num(eh_deadline); +} static void merge_hwe (struct hwentry * dst, struct hwentry * src) @@ -602,6 +633,51 @@ out: return 1; } +static void +validate_pctable(struct hwentry *ovr, int idx, const char *table_desc) +{ + struct pcentry *pce; + + if (!ovr || !ovr->pctable) + return; + + vector_foreach_slot_after(ovr->pctable, pce, idx) { + if (pce->type < 0) { + condlog(0, "protocol section in %s missing type", + table_desc); + vector_del_slot(ovr->pctable, idx--); + free(pce); + } + } + + if (VECTOR_SIZE(ovr->pctable) == 0) { + vector_free(ovr->pctable); + ovr->pctable = NULL; + } +} + +static void +merge_pctable(struct hwentry *ovr) +{ + struct pcentry *pce1, *pce2; + int i, j; + + if (!ovr || !ovr->pctable) + return; + + vector_foreach_slot(ovr->pctable, pce1, i) { + j = i + 1; + vector_foreach_slot_after(ovr->pctable, pce2, j) { + if (pce1->type != pce2->type) + continue; + merge_pce(pce2,pce1); + vector_del_slot(ovr->pctable, i--); + free(pce1); + break; + } + } +} + static void factorize_hwtable (vector hw, int n, const char *table_desc) { @@ -750,6 +826,7 @@ process_config_dir(struct config *conf, char *dir) int i, n; char path[LINE_MAX]; int old_hwtable_size; + int old_pctable_size = 0; if (dir[0] != '/') { condlog(1, "config_dir '%s' must be a fully qualified path", @@ -776,11 +853,15 @@ process_config_dir(struct config *conf, char *dir) continue; old_hwtable_size = VECTOR_SIZE(conf->hwtable); + old_pctable_size = conf->overrides ? + VECTOR_SIZE(conf->overrides->pctable) : 0; snprintf(path, LINE_MAX, "%s/%s", dir, namelist[i]->d_name); path[LINE_MAX-1] = '\0'; process_file(conf, path); factorize_hwtable(conf->hwtable, old_hwtable_size, namelist[i]->d_name); + validate_pctable(conf->overrides, old_pctable_size, +namelist[i]->d_name); } pthread_cleanup_pop(1); } @@ -888,6 +969,7 @@ int _init_config (const char *file, struct config *conf) goto out; } factorize_hwtable(conf->hwtable, builtin_hwtable_size, file); + validate_pctabl
[dm-devel] [PATCH v2 4/7] libmultipath: Set the scsi timeout parameters by path
Instead of dev_loss, fast_io_fail, and eh_deadline belonging to the multipath structure, have them belong to the path structure. This means that they are selected per path, and that sysfs_set_scsi_tmo() doesn't assume that all paths of a multipath device will have the same value. Currently they will all be the same, but a future patch will make it possible for paths to have different values based on their protocol. Signed-off-by: Benjamin Marzinski --- libmultipath/configure.c | 5 +- libmultipath/discovery.c | 174 ++- libmultipath/discovery.h | 2 +- libmultipath/propsel.c | 42 +- libmultipath/propsel.h | 6 +- libmultipath/structs.c | 1 - libmultipath/structs.h | 6 +- 7 files changed, 127 insertions(+), 109 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index eca11ba0..09ae708d 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -329,9 +329,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) select_mode(conf, mpp); select_uid(conf, mpp); 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); @@ -347,7 +344,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) select_ghost_delay(conf, mpp); select_flush_on_last_del(conf, mpp); - sysfs_set_scsi_tmo(mpp, conf->checkint); + sysfs_set_scsi_tmo(conf, mpp); marginal_pathgroups = conf->marginal_pathgroups; pthread_cleanup_pop(1); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index b969fba1..c6ba1967 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -598,13 +598,13 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen) } static int -sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp) +sysfs_set_eh_deadline(struct path *pp) { struct udev_device *hostdev; char host_name[HOST_NAME_LEN], value[16]; int ret, len; - if (mpp->eh_deadline == EH_DEADLINE_UNSET) + if (pp->eh_deadline == EH_DEADLINE_UNSET) return 0; sprintf(host_name, "host%d", pp->sg_id.host_no); @@ -613,12 +613,12 @@ sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp) if (!hostdev) return 1; - if (mpp->eh_deadline == EH_DEADLINE_OFF) + if (pp->eh_deadline == EH_DEADLINE_OFF) len = sprintf(value, "off"); - else if (mpp->eh_deadline == EH_DEADLINE_ZERO) + else if (pp->eh_deadline == EH_DEADLINE_ZERO) len = sprintf(value, "0"); else - len = sprintf(value, "%d", mpp->eh_deadline); + len = sprintf(value, "%d", pp->eh_deadline); ret = sysfs_attr_set_value(hostdev, "eh_deadline", value, len + 1); @@ -642,8 +642,8 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp) unsigned int tmo; int ret; - if (mpp->dev_loss == DEV_LOSS_TMO_UNSET && - mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET) + if (pp->dev_loss == DEV_LOSS_TMO_UNSET && + pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET) return; sprintf(rport_id, "rport-%d:%d-%d", @@ -685,14 +685,14 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp) * then set fast_io_fail, and _then_ set dev_loss_tmo * to the correct value. */ - if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET && - mpp->fast_io_fail != MP_FAST_IO_FAIL_ZERO && - mpp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { + if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET && + pp->fast_io_fail != MP_FAST_IO_FAIL_ZERO && + pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { /* Check if we need to temporarily increase dev_loss_tmo */ - if ((unsigned int)mpp->fast_io_fail >= tmo) { + if ((unsigned int)pp->fast_io_fail >= tmo) { /* Increase dev_loss_tmo temporarily */ snprintf(value, sizeof(value), "%u", -(unsigned int)mpp->fast_io_fail + 1); +(unsigned int)pp->fast_io_fail + 1); ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", value, strlen(value)); if (ret <= 0) { @@ -706,20 +706,20 @@ sysfs_set_rport_tm
[dm-devel] [PATCH v2 5/7] libmultipath: check the overrides pctable for path variables
Paths will now also check the pctable when checking for attribtes that exists in both the overrides section and the protocol subsection. Values in a matching pcentry will be used in preference to values in the overrides hwentry. Signed-off-by: Benjamin Marzinski --- libmultipath/propsel.c | 29 ++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index d2d70090..72b42991 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -79,6 +79,8 @@ static const char conf_origin[] = "(setting: multipath.conf defaults/devices section)"; static const char overrides_origin[] = "(setting: multipath.conf overrides section)"; +static const char overrides_pce_origin[] = + "(setting: multipath.conf overrides protocol section)"; static const char cmdline_origin[] = "(setting: multipath command line [-p] flag)"; static const char autodetect_origin[] = @@ -146,6 +148,27 @@ do { \ } \ } while (0) +#define pp_set_ovr_pce(var)\ +do { \ + struct pcentry *_pce; \ + int _i; \ + \ + if (conf->overrides) { \ + vector_foreach_slot(conf->overrides->pctable, _pce, _i) { \ + if (_pce->type == (int)bus_protocol_id(pp) && _pce->var) { \ + pp->var = _pce->var;\ + origin = overrides_pce_origin; \ + goto out; \ + } \ + } \ + if (conf->overrides->var) { \ + pp->var = conf->overrides->var; \ + origin = overrides_origin; \ + goto out; \ + } \ + } \ +} while (0) + int select_mode(struct config *conf, struct multipath *mp) { const char *origin; @@ -774,7 +797,7 @@ int select_fast_io_fail(struct config *conf, struct path *pp) const char *origin; STRBUF_ON_STACK(buff); - pp_set_ovr(fast_io_fail); + pp_set_ovr_pce(fast_io_fail); pp_set_hwe(fast_io_fail); pp_set_conf(fast_io_fail); pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL); @@ -790,7 +813,7 @@ int select_dev_loss(struct config *conf, struct path *pp) const char *origin; STRBUF_ON_STACK(buff); - pp_set_ovr(dev_loss); + pp_set_ovr_pce(dev_loss); pp_set_hwe(dev_loss); pp_set_conf(dev_loss); pp->dev_loss = DEV_LOSS_TMO_UNSET; @@ -807,7 +830,7 @@ int select_eh_deadline(struct config *conf, struct path *pp) const char *origin; STRBUF_ON_STACK(buff); - pp_set_ovr(eh_deadline); + pp_set_ovr_pce(eh_deadline); pp_set_hwe(eh_deadline); pp_set_conf(eh_deadline); pp->eh_deadline = EH_DEADLINE_UNSET; -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 2/7] libmultipath: make protocol_name global
Future patches will make use of this, so move it out of snprint_path_protocol() Signed-off-by: Benjamin Marzinski Reviewed-by: Martin Wilck --- libmultipath/print.c | 17 - libmultipath/structs.c | 18 ++ libmultipath/structs.h | 1 + 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/libmultipath/print.c b/libmultipath/print.c index bf88f301..27c2cf1a 100644 --- a/libmultipath/print.c +++ b/libmultipath/print.c @@ -754,23 +754,6 @@ snprint_path_failures(struct strbuf *buff, const struct path * pp) int snprint_path_protocol(struct strbuf *buff, const struct path * pp) { - static const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { - [SYSFS_BUS_UNDEF] = "undef", - [SYSFS_BUS_CCW] = "ccw", - [SYSFS_BUS_CCISS] = "cciss", - [SYSFS_BUS_NVME] = "nvme", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec", - }; const char *pn = protocol_name[bus_protocol_id(pp)]; assert(pn != NULL); diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 4b62da54..04cfdcdc 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -20,6 +20,24 @@ #include "dm-generic.h" #include "devmapper.h" +const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { + [SYSFS_BUS_UNDEF] = "undef", + [SYSFS_BUS_CCW] = "ccw", + [SYSFS_BUS_CCISS] = "cciss", + [SYSFS_BUS_NVME] = "nvme", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec", +}; + struct adapter_group * alloc_adaptergroup(void) { diff --git a/libmultipath/structs.h b/libmultipath/structs.h index d94f93a0..3722e31b 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -192,6 +192,7 @@ enum scsi_protocol { */ #define LAST_BUS_PROTOCOL_ID (SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC) unsigned int bus_protocol_id(const struct path *pp); +extern const char * const protocol_name[]; #define SCSI_INVALID_LUN ~0ULL -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 1/7] libmultipath: steal the src string pointer in merge_str()
Instead of allocating a copy when the original string is going to be freed right after the merge, just steal the pointer. Also, merge_mpe() can't get called with NULL arguments. Signed-off-by: Benjamin Marzinski Reviewed-by: Martin Wilck --- libmultipath/config.c | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index c595e768..612941b8 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -387,9 +387,9 @@ set_param_str(const char * str) } #define merge_str(s) \ - if (!dst->s && src->s) { \ - if (!(dst->s = set_param_str(src->s))) \ - return 1; \ + if (!dst->s && src->s && strlen(src->s)) { \ + dst->s = src->s; \ + src->s = NULL; \ } #define merge_num(s) \ @@ -397,7 +397,7 @@ set_param_str(const char * str) dst->s = src->s -static int +static void merge_hwe (struct hwentry * dst, struct hwentry * src) { char id[SCSI_VENDOR_SIZE+PATH_PRODUCT_SIZE]; @@ -449,15 +449,11 @@ merge_hwe (struct hwentry * dst, struct hwentry * src) reconcile_features_with_options(id, >features, >no_path_retry, >retain_hwhandler); - return 0; } -static int +static void merge_mpe(struct mpentry *dst, struct mpentry *src) { - if (!dst || !src) - return 1; - merge_str(alias); merge_str(uid_attribute); merge_str(getuid); @@ -499,8 +495,6 @@ merge_mpe(struct mpentry *dst, struct mpentry *src) merge_num(uid); merge_num(gid); merge_num(mode); - - return 0; } void merge_mptable(vector mptable) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH v2 0/7] Add protocol specific config subsection
Some storage arrays can be accessed using multiple protocols at the same time. I've have customers request the ability to set different values for the path specific timeouts, like fast_io_fail_tmo, based on the protocol used to access the path. In order to make this possible, this patchset adds a new protocol subsection to the overrides section. This allows users to set a device config's path specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and eh_deadline on a per-protocol basis. Changes from v1 (all based on suggestions from Martin Wilck): 0003: Adds the protocol subsection to the overrides section instead of the device subsection, pulling in code from original patch 0007 0005: Checks the pctable of the overrides section instead of the hwes, pulling in code from original patch 0007 Original patches 0006 and 0007 have been removed. 0007: (original patch 0009) modified the man page to no longer reference the protocol subsection under the device subsection Benjamin Marzinski (7): libmultipath: steal the src string pointer in merge_str() libmultipath: make protocol_name global libmultipath: add a protocol subsection to multipath.conf libmultipath: Set the scsi timeout parameters by path libmultipath: check the overrides pctable for path variables libmultipath: fix eh_deadline documentation libmultipath: Add documentation for the protocol subsection libmultipath/config.c | 99 ++--- libmultipath/config.h | 10 +++ libmultipath/configure.c | 5 +- libmultipath/dict.c| 99 + libmultipath/discovery.c | 174 + libmultipath/discovery.h | 2 +- libmultipath/print.c | 67 ++ libmultipath/propsel.c | 65 +- libmultipath/propsel.h | 6 +- libmultipath/structs.c | 19 +++- libmultipath/structs.h | 7 +- multipath/multipath.conf.5 | 36 12 files changed, 452 insertions(+), 137 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection
On Tue, Apr 12, 2022 at 08:47:38PM +, Martin Wilck wrote: > On Tue, 2022-04-12 at 13:47 -0500, Benjamin Marzinski wrote: > > On Tue, Apr 12, 2022 at 10:31:50AM +, Martin Wilck wrote: > > > > > I agree that setting fast_io_fail_tmo to different values based on > > both > > array and protocol is usually overkill. > > If we doubt that there is a serious use case for this level of > differentiation, I think we shouldn't implement it. Doing that would > violate the KISS principle. > > > > > > > What about adding simply adding "protocols" as a new top section in > > > the > > > conf file, and have the per-protocol settings override built-in > > > hwtable > > > settings for any array, but not explicit storage-device settings in > > > the > > > conf file? > > > > I'm not really enamored with the idea of only working on the built-in > > hwtable. > > I feel like the people that want to tune things based on the > > protocol are the same sort of people that want tune things per array. > > Hm, maybe we are misunderstanding each other. Let me give an example > what I meant. We've got dev_loss = inifinity for ONTAP in the builtin > hwtable. My idea would be that users could override this in the > protocols section for all devices using a certain protocol: > > protocols { > protocol { > type scsi:fcp > dev_loss_tmo 120 > } > } > > This would take precedence over the built-in hwtable, but not over an > explicit config file entry: > > devices { > device { > vendor NETAPP > product LUN > # With the dev_loss_tmo line below, RDAC would use 300 for > # every protocol. Without it, it would use 120 for FC and > # "infinity" for other protocols. > dev_loss_tmo 300 > } > } > > Admittedly, the problem on the implementation side is that we don't > distinguish between built-in hwentries and those from explicit > configuration. Changing this would be considerable effort; perhaps more > effort than what you did in your patch set. I haven't thought it > through. > > > More importantly, we don't have anything else in multipath that only > > applies to some of the device config entries. That change seems more > > confusing to me than adding a new subsection. > > The main change would be to differentiate between built-in and user- > configured hardware properties. I hope most users would be able to > understand the concept. > > > The protocol subsection is > > visually part of the device config it is modifying. A separate > > protocol > > section that only modified some of the device configs > > seems less > > obvious. Plus we don't have a good way to code that. Also, what > > happens > > to merged configs, where some of the timeouts came from a builtin > > config, and some came from the user config. > > To clarify once more: this is what I meant, built-in configs would be > overridden, user configs wouldn't be. This is different from > "defaults", as "defaults" don't override hardware-specific built-ins. But what do you call a device config that is the result of merging (via merge_hwe()) a built-in and a non-built-in config. Do we really want to track that some of the values of this merged config need to check the protocol section, and some don't? We could remove merging identical configs, but that simply makes it harder for users to figure out how their device will be configured from the configuration output. I understand your idea. I'd just rather that it worked on all the device configs, instead of only the built-in ones. I think overriding only the built-in configs is needlessly complicated, both from a coding and from an explaining point of view. > > If you are really agains the subsection idea, > > I would rather have the protocol section override > > all the device configs. Users would need to be o.k. with picking a > > protocol for which all their arrays have the same timeout values. But > > again, that should be the common case. > > The question is whether a "protocol" entry should override device > settings, or vice versa (as in my hypothetical ONTAP example above). We > seem to agree that that device subsection would implement a level of > complexity that hardly any user needs. If this is the case, we'd just > need to determine the precedence between "devices" and "protocols" > sections. If we determine that "protocols" should always take > precedence, we might as well allow it under "overrides" only. We'd need > no &
Re: [dm-devel] [PATCH 0/9] Add protocol specific config subsection
On Tue, Apr 12, 2022 at 10:31:50AM +, Martin Wilck wrote: > On Mon, 2022-04-11 at 20:59 -0500, Benjamin Marzinski wrote: > > Some storage arrays can be accessed using multiple protocols at the > > same > > time. I've have customers request the ability to set different > > values > > for the path specific timeouts, like fast_io_fail_tmo, based on the > > protocol used to access the path. In order to make this possible, > > this > > patchset adds a new protocol subsection to the device subsection and > > the > > overrides section. This allows users to set a device config's path > > specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and > > eh_deadline on a per-protocol basis. > > Sigh... I am not happy about this amount of additional complexity in > the multipath configuration. It is already so complicated that hardly > anyone really understands how it works. > > I fully agree that some properties, in particular fast_io_fail_tmo [1], > are rather properties of protocol or fabrics than a storage array. > But do we really need to differentiate by both "device" and "protocol"? > IOW, do users need different fast_io_fail_tmo value for the same > protocol, but different arrays? My feeling is that making this property > depend on a 2-D matrix of (protocol x storage) is overcomplicating > matters. And _if_ this is necessary, what do we gain by adding an > "overrides" on top? [2] I agree that setting fast_io_fail_tmo to different values based on both array and protocol is usually overkill. This is why I added it to the overrides section as well. If you just put it there, it will work for all devices. I assumed that would be the common case. It wouldn't make sense to have it be in the defaults section and override things in devices section, but it's a natural fit for the overrides section. Plus, since I added the protocol table to the hwentry, enabling it in the overrides section wasn't much new code. > What about adding simply adding "protocols" as a new top section in the > conf file, and have the per-protocol settings override built-in hwtable > settings for any array, but not explicit storage-device settings in the > conf file? I'm not really enamored with the idea of only working on the built-in hwtable. I feel like the people that want to tune things based on the protocol are the same sort of people that want tune things per array. More importantly, we don't have anything else in multipath that only applies to some of the device config entries. That change seems more confusing to me than adding a new subsection. The protocol subsection is visually part of the device config it is modifying. A separate protocol section that only modified some of the device configs seems less obvious. Plus we don't have a good way to code that. Also, what happens to merged configs, where some of the timeouts came from a builtin config, and some came from the user config. If you are really against the subsection idea, I would rather have the protocol section override all the device configs. Users would need to be o.k. with picking a protocol for which all their arrays have the same timeout values. But again, that should be the common case. > Regards, > Martin > > [1]: wrt dev_loss_tmo, I tend to think that the best value must be > found based on neither protocol nor array, but data center staff. I do agree that fast_io_fail_tmo and eh_deadline make more sense than dev_loss_tmo (especially since FC/iSCSI setups seem the most common, and iSCSI doesn't support dev_loss_tmo), but since the section is there, and dev_loss_tmo is a per-path timeout setting, I put it in. I thought about letting people change the checker type per protocol, but figured that could wait till someone asked for it. This would make less sense if we had a seperate top level protocol section. So would things like per-protocol manginal path handling, and other path specific things which could reasonably go in a protocol subsection of a device config. > [2]: I strongly dislike "overrides", in general. I wonder what we need > it for, except for quick experiments where people are too lazy to add > explicit settings for devices and/or multipaths. The biggest reason is that some of the builtin device configs do things like set no_path_retry to "queue". I know people have used it to override dev_loss_tmo and fast_io_fail_tmo, but the big one is no_path_retry. But in general, some builtin device configs make choices that aren't applicable for all environments, but are what the vendors have validated and want for the default. While you can call it lazy, it's always possible that a new builtin config will be added, or and existing one changed, and suddenly your devices are hanging instead of failing like expected when all the paths go d
[dm-devel] [PATCH 9/9] libmultipath: Add documentation for the protocol subsection
Signed-off-by: Benjamin Marzinski --- multipath/multipath.conf.5 | 38 ++ 1 file changed, 38 insertions(+) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 97695da4..dab1d58f 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -1678,6 +1678,41 @@ section: .RE .PD .LP +The device subsection also recognizes the optional \fIprotocol\fR subsection. A +single device subsection can contain multiple protocol subsections. Devices are +matched against the protocol subsection using the mandatory \fItype\fR +attribute. Attributes in a matching protocol subsection take precedence over +attributes in the containing device subsection. If there are multiple matching +protocol subsections within a device subsection, later entries take precedence. +If there are multiple matching device subsections, attributes in later device +subsections still take precedence over attributes in a protocol subsection of +an earlier device subsection. +.TP +.B protocol subsection +The protocol subsection recognizes the following mandatory attribute: +.RS +.TP +.B type +The protocol string of the path device. The possible values are \fIscsi:fcp\fR, +\fIscsi:spi\fR, \fIscsi:ssa\fR, \fIscsi:sbp\fR, \fIscsi:srp\fR, +\fIscsi:iscsi\fR, \fIscsi:sas\fR, \fIscsi:adt\fR, \fIscsi:ata\fR, +\fIscsi:unspec\fR, \fIccw\fR, \fIcciss\fR, \fInvme\fR, and \fIundef\fR. This is +\fBnot\fR a regular expression. the path device protcol string must match +exactly. The protocol that a path is using can be viewed by running +\fBmultipathd show paths format "%d %P"\fR +.LP +The following attributes are optional; if not set, the default values are taken +from the containing \fIdevice\fR subsection: +.sp 1 +.PD .1v +.RS +.TP +.B fast_io_fail_tmo +.TP +.B dev_loss_tmo +.TP +.B eh_deadline +.PD . . .\" @@ -1764,6 +1799,9 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections: .RE .PD .LP +The overrides section also recognizes the optional \fIprotocol\fR subsection +and can contain multiple protocol subsections. For a full description of this +subsection please see its description in the \fIdevices\fR section. . . .\" -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 8/9] libmultipath: fix eh_deadline documentation
Signed-off-by: Benjamin Marzinski --- multipath/multipath.conf.5 | 4 1 file changed, 4 insertions(+) diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 index 605b46e0..97695da4 100644 --- a/multipath/multipath.conf.5 +++ b/multipath/multipath.conf.5 @@ -1636,6 +1636,8 @@ section: .TP .B dev_loss_tmo .TP +.B eh_deadline +.TP .B flush_on_last_del .TP .B user_friendly_names @@ -1722,6 +1724,8 @@ the values are taken from the \fIdevices\fR or \fIdefaults\fR sections: .TP .B dev_loss_tmo .TP +.B eh_deadline +.TP .B user_friendly_names .TP .B retain_attached_hw_handler -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 7/9] libmultipath: add procotol subsection to overrides
Add a procotol subsection to the overrides section, just like the one in the device subsection. This allows users to a protocol specific parameters to all device configurations. Signed-off-by: Benjamin Marzinski --- libmultipath/dict.c| 76 -- libmultipath/print.c | 5 +++ libmultipath/propsel.c | 29 ++-- 3 files changed, 90 insertions(+), 20 deletions(-) diff --git a/libmultipath/dict.c b/libmultipath/dict.c index 04d86ee3..4923b8d2 100644 --- a/libmultipath/dict.c +++ b/libmultipath/dict.c @@ -386,6 +386,20 @@ snprint_ovr_ ## option (struct config *conf, struct strbuf *buff, \ return function (buff, conf->overrides->option);\ } +#define declare_ovr_pc_handler(option, function) \ +static int \ +ovr_pc_ ## option ## _handler (struct config *conf, vector strvec, \ + const char *file, int line_nr) \ +{ \ + struct pcentry *pce;\ + if (!conf->overrides || !conf->overrides->pctable) \ + return 1; \ + pce = VECTOR_LAST_SLOT(conf->overrides->pctable); \ + if (!pce) \ + return 1; \ + return function (strvec, >option, file, line_nr); \ +} + #define declare_mp_handler(option, function) \ static int \ mp_ ## option ## _handler (struct config *conf, vector strvec, \ @@ -417,10 +431,10 @@ snprint_mp_ ## option (struct config *conf, struct strbuf *buff, \ return function(buff, mpe->option); \ } -#define declare_pc_handler(option, function) \ +#define declare_hw_pc_handler(option, function) \ static int \ -pc_ ## option ## _handler (struct config *conf, vector strvec, \ - const char *file, int line_nr) \ +hw_pc_ ## option ## _handler (struct config *conf, vector strvec, \ + const char *file, int line_nr) \ { \ struct pcentry *pce;\ struct hwentry * hwe = VECTOR_LAST_SLOT(conf->hwtable); \ @@ -1070,7 +1084,8 @@ 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) -declare_pc_handler(fast_io_fail, set_undef_off_zero) +declare_hw_pc_handler(fast_io_fail, set_undef_off_zero) +declare_ovr_pc_handler(fast_io_fail, set_undef_off_zero) declare_pc_snprint(fast_io_fail, print_undef_off_zero) static int @@ -1109,7 +1124,8 @@ declare_ovr_handler(dev_loss, set_dev_loss) 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_pc_handler(dev_loss, set_dev_loss) +declare_hw_pc_handler(dev_loss, set_dev_loss) +declare_ovr_pc_handler(dev_loss, set_dev_loss) declare_pc_snprint(dev_loss, print_dev_loss) declare_def_handler(eh_deadline, set_undef_off_zero) @@ -1118,7 +1134,8 @@ 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) -declare_pc_handler(eh_deadline, set_undef_off_zero) +declare_hw_pc_handler(eh_deadline, set_undef_off_zero) +declare_ovr_pc_handler(eh_deadline, set_undef_off_zero) declare_pc_snprint(eh_deadline, print_undef_off_zero) static int @@ -1929,13 +1946,9 @@ declare_mp_snprint(alias, print_str) static int -protocol_handler(struct config *conf, vector strvec, const char *file, - int line_nr) +_protocol_handler(struct hwentry *hwe) { struct pcentry *pce; - struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable); - if (!hwe) - return 1; if (!hwe->pctable && !(hwe->pctable = vector_alloc())) return 1; @@ -1952,6 +1965,27 @@ protocol_handler(struct config *conf, vector strvec, const char *file, return 0; } +static int +hw_protocol_handler(struct config *conf, vector strvec, const char *file, + int line_nr) +{ + struct hwentry *hwe = VECTOR_LAST_SLOT(conf->hwtable); + if (!hwe) +
[dm-devel] [PATCH 3/9] libmultipath: add a protocol subsection to multipath.conf
Some storage arrays can be accessed using multiple protocols at the same time. In these cases, users may want to set path attributes differently, depending on the protocol that the path is using. To allow this, add a protocol subsection to the device subsection in multipath.conf, which allows select path-specific options to be set. This commit simply adds the subsection, and handles merging matching entries, both within a hwentry, and when hwentries are merged. Future patches will make use of the section. Signed-off-by: Benjamin Marzinski --- libmultipath/config.c | 102 ++ libmultipath/config.h | 10 + libmultipath/dict.c | 99 libmultipath/print.c | 44 ++ 4 files changed, 255 insertions(+) diff --git a/libmultipath/config.c b/libmultipath/config.c index 612941b8..61d3c182 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -237,6 +237,18 @@ const char *get_mpe_wwid(const struct _vector *mptable, const char *alias) return NULL; } +static void +free_pctable (vector pctable) +{ + int i; + struct pcentry *pce; + + vector_foreach_slot(pctable, pce, i) + free(pce); + + vector_free(pctable); +} + void free_hwe (struct hwentry * hwe) { @@ -282,6 +294,9 @@ free_hwe (struct hwentry * hwe) if (hwe->bl_product) free(hwe->bl_product); + if (hwe->pctable) + free_pctable(hwe->pctable); + free(hwe); } @@ -363,6 +378,15 @@ alloc_hwe (void) return hwe; } +struct pcentry * +alloc_pce (void) +{ + struct pcentry *pce = (struct pcentry *) + calloc(1, sizeof(struct pcentry)); + pce->type = -1; + return pce; +} + static char * set_param_str(const char * str) { @@ -396,6 +420,41 @@ set_param_str(const char * str) if (!dst->s && src->s) \ dst->s = src->s +static void +merge_pce(struct pcentry *dst, struct pcentry *src) +{ + merge_num(fast_io_fail); + merge_num(dev_loss); + merge_num(eh_deadline); +} + +static int +merge_pctable(vector d_pctable, vector s_pctable) +{ + struct pcentry *d_pce, *s_pce; + int i, j, start = 0; + + vector_foreach_slot_backwards(s_pctable, s_pce, i) { + bool found = false; + j = start; + vector_foreach_slot_after(d_pctable, d_pce, j) { + if (s_pce->type != d_pce->type) + continue; + found = true; + merge_pce(d_pce, s_pce); + vector_del_slot(s_pctable, i); + free(s_pce); + break; + } + if (found) + continue; + if (!vector_insert_slot(d_pctable, 0, s_pce)) + return 1; + vector_del_slot(s_pctable, i); + start++; + } + return 0; +} static void merge_hwe (struct hwentry * dst, struct hwentry * src) @@ -445,6 +504,13 @@ merge_hwe (struct hwentry * dst, struct hwentry * src) merge_num(marginal_path_err_recheck_gap_time); merge_num(marginal_path_double_failed_time); + if (src->pctable) { + if (!dst->pctable) + dst->pctable = steal_ptr(src->pctable); + else + merge_pctable(dst->pctable, src->pctable); + } + snprintf(id, sizeof(id), "%s/%s", dst->vendor, dst->product); reconcile_features_with_options(id, >features, >no_path_retry, @@ -602,6 +668,41 @@ out: return 1; } +static void +factorize_pctable(struct hwentry *hwe, const char *table_desc) +{ + struct pcentry *pce1, *pce2; + int i, j; + + if (!hwe->pctable) + return; + + vector_foreach_slot(hwe->pctable, pce1, i) { + if (pce1->type < 0) { + condlog(0, "protocol section from %s:%s in %s missing type", + hwe->vendor, hwe->product, table_desc); + vector_del_slot(hwe->pctable, i--); + free(pce1); + continue; + } + j = i + 1; + vector_foreach_slot_after(hwe->pctable, pce2, j) { + if (pce1->type != pce2->type) + continue; + merge_pce(pce2,pce1); + vector_del_slot(hwe->pctable, i--); + free(pce1); + break; + } + + } + + if (VECTOR_SIZE(hwe->pctable) == 0) { + vector_free(hwe->pctable); +
[dm-devel] [PATCH 5/9] libmultipath: check the hwentry pctable for path variables
For the config values that exist in the proctol subsection of the device configs, paths will now also check the pctable when checking a hwentry for a value. Values in a matching pcentry will be used in preference to values in that hwentry. Signed-off-by: Benjamin Marzinski --- libmultipath/propsel.c | 30 +++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c index d2d70090..762b7fcb 100644 --- a/libmultipath/propsel.c +++ b/libmultipath/propsel.c @@ -73,6 +73,8 @@ do { \ static const char default_origin[] = "(setting: multipath internal)"; static const char hwe_origin[] = "(setting: storage device configuration)"; +static const char hwe_pce_origin[] = + "(setting: storage device procotol section)"; static const char multipaths_origin[] = "(setting: multipath.conf multipaths section)"; static const char conf_origin[] = @@ -146,6 +148,28 @@ do { \ } \ } while (0) +#define pp_set_hwe_pce(var)\ +do { \ + struct hwentry *_hwe; \ + struct pcentry *_pce; \ + int _i, _j; \ + \ + vector_foreach_slot(pp->hwe, _hwe, _i) {\ + vector_foreach_slot(_hwe->pctable, _pce, _j) { \ + if (_pce->type == (int)bus_protocol_id(pp) && _pce->var) { \ + pp->var = _pce->var;\ + origin = hwe_pce_origin;\ + goto out; \ + } \ + } \ + if (_hwe->var) {\ + pp->var = _hwe->var;\ + origin = hwe_origin;\ + goto out; \ + } \ + } \ +} while (0) + int select_mode(struct config *conf, struct multipath *mp) { const char *origin; @@ -775,7 +799,7 @@ int select_fast_io_fail(struct config *conf, struct path *pp) STRBUF_ON_STACK(buff); pp_set_ovr(fast_io_fail); - pp_set_hwe(fast_io_fail); + pp_set_hwe_pce(fast_io_fail); pp_set_conf(fast_io_fail); pp_set_default(fast_io_fail, DEFAULT_FAST_IO_FAIL); out: @@ -791,7 +815,7 @@ int select_dev_loss(struct config *conf, struct path *pp) STRBUF_ON_STACK(buff); pp_set_ovr(dev_loss); - pp_set_hwe(dev_loss); + pp_set_hwe_pce(dev_loss); pp_set_conf(dev_loss); pp->dev_loss = DEV_LOSS_TMO_UNSET; return 0; @@ -808,7 +832,7 @@ int select_eh_deadline(struct config *conf, struct path *pp) STRBUF_ON_STACK(buff); pp_set_ovr(eh_deadline); - pp_set_hwe(eh_deadline); + pp_set_hwe_pce(eh_deadline); pp_set_conf(eh_deadline); pp->eh_deadline = EH_DEADLINE_UNSET; /* not changing sysfs in default cause, so don't print anything */ -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 6/9] libmultipath: make snprint_pctable indent a variable amount
Instead of always indenting two tabs, which is what is needed in for the protocol subsection of the device subsection, indent a variable amount. This will be needed in a future patch. Signed-off-by: Benjamin Marzinski --- libmultipath/print.c | 28 +--- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/libmultipath/print.c b/libmultipath/print.c index 46d231ed..093e43aa 100644 --- a/libmultipath/print.c +++ b/libmultipath/print.c @@ -1407,28 +1407,42 @@ int snprint_multipath_topology_json (struct strbuf *buff, } static int -snprint_pcentry (struct strbuf *buff, const struct pcentry *pce, +snprint_pcentry (struct strbuf *buff, int indent, const struct pcentry *pce, const struct keyword *rootkw) { int i, rc; struct keyword *kw; size_t initial_len = get_strbuf_len(buff); + STRBUF_ON_STACK(fmt); - if ((rc = append_strbuf_str(buff, "\t\tprotocol {\n")) < 0) + for (i = 0; i < indent; i++) + if ((rc = append_strbuf_str(, "\t")) < 0) + return rc; + if ((rc = append_strbuf_str(, "\t%k %v\n")) < 0) + return rc; + + for (i = 0; i < indent; i++) + if ((rc = append_strbuf_str(buff, "\t")) < 0) + return rc; + if ((rc = append_strbuf_str(buff, "protocol {\n")) < 0) return rc; iterate_sub_keywords(rootkw, kw, i) { - if ((rc = snprint_keyword(buff, "\t\t\t%k %v\n", kw, pce)) < 0) + if ((rc = snprint_keyword(buff, get_strbuf_str(), kw, + pce)) < 0) return rc; } - if ((rc = append_strbuf_str(buff, "\t\t}\n")) < 0) + for (i = 0; i < indent; i++) + if ((rc = append_strbuf_str(buff, "\t")) < 0) + return rc; + if ((rc = append_strbuf_str(buff, "}\n")) < 0) return rc; return get_strbuf_len(buff) - initial_len; } static int -snprint_pctable (const struct config *conf, struct strbuf *buff, +snprint_pctable (const struct config *conf, struct strbuf *buff, int indent, const struct _vector *pctable, const struct keyword *rootkw) { int i, rc; @@ -1439,7 +1453,7 @@ snprint_pctable (const struct config *conf, struct strbuf *buff, assert(rootkw); vector_foreach_slot(pctable, pce, i) { - if ((rc = snprint_pcentry(buff, pce, rootkw)) < 0) + if ((rc = snprint_pcentry(buff, indent, pce, rootkw)) < 0) return rc; } return get_strbuf_len(buff) - initial_len; @@ -1468,7 +1482,7 @@ snprint_hwentry (const struct config *conf, } if (hwe->pctable && - (rc = snprint_pctable(conf, buff, hwe->pctable, rootkw)) < 0) + (rc = snprint_pctable(conf, buff, 2, hwe->pctable, rootkw)) < 0) return rc; if ((rc = append_strbuf_str(buff, "\t}\n")) < 0) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 2/9] libmultipath: make protocol_name global
Future patches will make use of this, so move it out of snprint_path_protocol() Signed-off-by: Benjamin Marzinski --- libmultipath/print.c | 17 - libmultipath/structs.c | 18 ++ libmultipath/structs.h | 1 + 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/libmultipath/print.c b/libmultipath/print.c index bf88f301..27c2cf1a 100644 --- a/libmultipath/print.c +++ b/libmultipath/print.c @@ -754,23 +754,6 @@ snprint_path_failures(struct strbuf *buff, const struct path * pp) int snprint_path_protocol(struct strbuf *buff, const struct path * pp) { - static const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { - [SYSFS_BUS_UNDEF] = "undef", - [SYSFS_BUS_CCW] = "ccw", - [SYSFS_BUS_CCISS] = "cciss", - [SYSFS_BUS_NVME] = "nvme", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb", - [SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec", - }; const char *pn = protocol_name[bus_protocol_id(pp)]; assert(pn != NULL); diff --git a/libmultipath/structs.c b/libmultipath/structs.c index 4b62da54..04cfdcdc 100644 --- a/libmultipath/structs.c +++ b/libmultipath/structs.c @@ -20,6 +20,24 @@ #include "dm-generic.h" #include "devmapper.h" +const char * const protocol_name[LAST_BUS_PROTOCOL_ID + 1] = { + [SYSFS_BUS_UNDEF] = "undef", + [SYSFS_BUS_CCW] = "ccw", + [SYSFS_BUS_CCISS] = "cciss", + [SYSFS_BUS_NVME] = "nvme", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_FCP] = "scsi:fcp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SPI] = "scsi:spi", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SSA] = "scsi:ssa", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SBP] = "scsi:sbp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SRP] = "scsi:srp", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ISCSI] = "scsi:iscsi", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_SAS] = "scsi:sas", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ADT] = "scsi:adt", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_ATA] = "scsi:ata", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_USB] = "scsi:usb", + [SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC] = "scsi:unspec", +}; + struct adapter_group * alloc_adaptergroup(void) { diff --git a/libmultipath/structs.h b/libmultipath/structs.h index d94f93a0..3722e31b 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -192,6 +192,7 @@ enum scsi_protocol { */ #define LAST_BUS_PROTOCOL_ID (SYSFS_BUS_SCSI + SCSI_PROTOCOL_UNSPEC) unsigned int bus_protocol_id(const struct path *pp); +extern const char * const protocol_name[]; #define SCSI_INVALID_LUN ~0ULL -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 1/9] libmultipath: steal the src string pointer in merge_str()
Instead of allocating a copy when the original string is going to be freed right after the merge, just steal the pointer. Also, merge_mpe() can't get called with NULL arguments. Signed-off-by: Benjamin Marzinski --- libmultipath/config.c | 16 +--- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/libmultipath/config.c b/libmultipath/config.c index c595e768..612941b8 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -387,9 +387,9 @@ set_param_str(const char * str) } #define merge_str(s) \ - if (!dst->s && src->s) { \ - if (!(dst->s = set_param_str(src->s))) \ - return 1; \ + if (!dst->s && src->s && strlen(src->s)) { \ + dst->s = src->s; \ + src->s = NULL; \ } #define merge_num(s) \ @@ -397,7 +397,7 @@ set_param_str(const char * str) dst->s = src->s -static int +static void merge_hwe (struct hwentry * dst, struct hwentry * src) { char id[SCSI_VENDOR_SIZE+PATH_PRODUCT_SIZE]; @@ -449,15 +449,11 @@ merge_hwe (struct hwentry * dst, struct hwentry * src) reconcile_features_with_options(id, >features, >no_path_retry, >retain_hwhandler); - return 0; } -static int +static void merge_mpe(struct mpentry *dst, struct mpentry *src) { - if (!dst || !src) - return 1; - merge_str(alias); merge_str(uid_attribute); merge_str(getuid); @@ -499,8 +495,6 @@ merge_mpe(struct mpentry *dst, struct mpentry *src) merge_num(uid); merge_num(gid); merge_num(mode); - - return 0; } void merge_mptable(vector mptable) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
[dm-devel] [PATCH 4/9] libmultipath: Set the scsi timeout parameters by path
Instead of dev_loss, fast_io_fail, and eh_deadline belonging to the multipath structure, have them belong to the path structure. This means that they are selected per path, and that sysfs_set_scsi_tmo() doesn't assume that all paths of a multipath device will have the same value. Currently they will all be the same, but a future patch will make it possible for paths to have different values based on their protocol. Signed-off-by: Benjamin Marzinski --- libmultipath/configure.c | 5 +- libmultipath/discovery.c | 174 ++- libmultipath/discovery.h | 2 +- libmultipath/propsel.c | 42 +- libmultipath/propsel.h | 6 +- libmultipath/structs.c | 1 - libmultipath/structs.h | 6 +- 7 files changed, 127 insertions(+), 109 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index eca11ba0..09ae708d 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -329,9 +329,6 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) select_mode(conf, mpp); select_uid(conf, mpp); 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); @@ -347,7 +344,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs) select_ghost_delay(conf, mpp); select_flush_on_last_del(conf, mpp); - sysfs_set_scsi_tmo(mpp, conf->checkint); + sysfs_set_scsi_tmo(conf, mpp); marginal_pathgroups = conf->marginal_pathgroups; pthread_cleanup_pop(1); diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index b969fba1..c6ba1967 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -598,13 +598,13 @@ sysfs_get_asymmetric_access_state(struct path *pp, char *buff, int buflen) } static int -sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp) +sysfs_set_eh_deadline(struct path *pp) { struct udev_device *hostdev; char host_name[HOST_NAME_LEN], value[16]; int ret, len; - if (mpp->eh_deadline == EH_DEADLINE_UNSET) + if (pp->eh_deadline == EH_DEADLINE_UNSET) return 0; sprintf(host_name, "host%d", pp->sg_id.host_no); @@ -613,12 +613,12 @@ sysfs_set_eh_deadline(struct multipath *mpp, struct path *pp) if (!hostdev) return 1; - if (mpp->eh_deadline == EH_DEADLINE_OFF) + if (pp->eh_deadline == EH_DEADLINE_OFF) len = sprintf(value, "off"); - else if (mpp->eh_deadline == EH_DEADLINE_ZERO) + else if (pp->eh_deadline == EH_DEADLINE_ZERO) len = sprintf(value, "0"); else - len = sprintf(value, "%d", mpp->eh_deadline); + len = sprintf(value, "%d", pp->eh_deadline); ret = sysfs_attr_set_value(hostdev, "eh_deadline", value, len + 1); @@ -642,8 +642,8 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp) unsigned int tmo; int ret; - if (mpp->dev_loss == DEV_LOSS_TMO_UNSET && - mpp->fast_io_fail == MP_FAST_IO_FAIL_UNSET) + if (pp->dev_loss == DEV_LOSS_TMO_UNSET && + pp->fast_io_fail == MP_FAST_IO_FAIL_UNSET) return; sprintf(rport_id, "rport-%d:%d-%d", @@ -685,14 +685,14 @@ sysfs_set_rport_tmo(struct multipath *mpp, struct path *pp) * then set fast_io_fail, and _then_ set dev_loss_tmo * to the correct value. */ - if (mpp->fast_io_fail != MP_FAST_IO_FAIL_UNSET && - mpp->fast_io_fail != MP_FAST_IO_FAIL_ZERO && - mpp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { + if (pp->fast_io_fail != MP_FAST_IO_FAIL_UNSET && + pp->fast_io_fail != MP_FAST_IO_FAIL_ZERO && + pp->fast_io_fail != MP_FAST_IO_FAIL_OFF) { /* Check if we need to temporarily increase dev_loss_tmo */ - if ((unsigned int)mpp->fast_io_fail >= tmo) { + if ((unsigned int)pp->fast_io_fail >= tmo) { /* Increase dev_loss_tmo temporarily */ snprintf(value, sizeof(value), "%u", -(unsigned int)mpp->fast_io_fail + 1); +(unsigned int)pp->fast_io_fail + 1); ret = sysfs_attr_set_value(rport_dev, "dev_loss_tmo", value, strlen(value)); if (ret <= 0) { @@ -706,20 +706,20 @@ sysfs_set_rport_tm
[dm-devel] [PATCH 0/9] Add protocol specific config subsection
Some storage arrays can be accessed using multiple protocols at the same time. I've have customers request the ability to set different values for the path specific timeouts, like fast_io_fail_tmo, based on the protocol used to access the path. In order to make this possible, this patchset adds a new protocol subsection to the device subsection and the overrides section. This allows users to set a device config's path specific timeouts, such as dev_loss_tmo, fast_io_fail_tmo, and eh_deadline on a per-protocol basis. Benjamin Marzinski (9): libmultipath: steal the src string pointer in merge_str() libmultipath: make protocol_name global libmultipath: add a protocol subsection to multipath.conf libmultipath: Set the scsi timeout parameters by path libmultipath: check the hwentry pctable for path variables libmultipath: make snprint_pctable indent a variable amount libmultipath: add procotol subsection to overrides libmultipath: fix eh_deadline documentation libmultipath: Add documentation for the protocol subsection libmultipath/config.c | 116 ++--- libmultipath/config.h | 10 +++ libmultipath/configure.c | 5 +- libmultipath/dict.c| 141 ++ libmultipath/discovery.c | 174 + libmultipath/discovery.h | 2 +- libmultipath/print.c | 80 + libmultipath/propsel.c | 89 ++- libmultipath/propsel.h | 6 +- libmultipath/structs.c | 19 +++- libmultipath/structs.h | 7 +- multipath/multipath.conf.5 | 42 + 12 files changed, 555 insertions(+), 136 deletions(-) -- 2.17.2 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 15/15] libmultipath: avoid memory leak with uid_attrs
On Mon, Apr 04, 2022 at 07:04:57PM +0200, mwi...@suse.com wrote: > Free all vector elements when freeing uid_attrs. > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > libmultipath/config.c | 5 + > libmultipath/dict.c | 5 + > 2 files changed, 10 insertions(+) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index 7346c37..bfaf041 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -656,6 +656,9 @@ static struct config *alloc_config (void) > > static void _uninit_config(struct config *conf) > { > + void *ptr; > + int i; > + > if (!conf) > conf = &__internal_config; > > @@ -668,6 +671,8 @@ static void _uninit_config(struct config *conf) > if (conf->uid_attribute) > free(conf->uid_attribute); > > + vector_foreach_slot(>uid_attrs, ptr, i) > + free(ptr); > vector_reset(>uid_attrs); > > if (conf->getuid) > diff --git a/libmultipath/dict.c b/libmultipath/dict.c > index 26cbe78..3b99296 100644 > --- a/libmultipath/dict.c > +++ b/libmultipath/dict.c > @@ -597,8 +597,13 @@ static int uid_attrs_handler(struct config *conf, vector > strvec, >const char *file, int line_nr) > { > char *val; > + void *ptr; > + int i; > > + vector_foreach_slot(>uid_attrs, ptr, i) > + free(ptr); > vector_reset(>uid_attrs); > + > val = set_value(strvec); > if (!val) > return 1; > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 14/15] libmultipath: apply_format(): prevent buffer overflow
On Mon, Apr 04, 2022 at 07:04:56PM +0200, mwi...@suse.com wrote: > Potential overflow found by coverity (CID 376918). Reviewed-by: Benjamin Marzinski > --- > libmultipath/callout.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libmultipath/callout.c b/libmultipath/callout.c > index dac088c..57c3481 100644 > --- a/libmultipath/callout.c > +++ b/libmultipath/callout.c > @@ -160,7 +160,7 @@ int apply_format(char * string, char * cmd, struct path * > pp) > myfree = CALLOUT_MAX_SIZE; > > if (!pos) { > - strcpy(dst, string); > + strlcpy(dst, string, CALLOUT_MAX_SIZE); > return 0; > } > > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 05/15] multipathd: reconfigure: disallow changing uid_attrs
On Mon, Apr 04, 2022 at 07:04:47PM +0200, mwi...@suse.com wrote: > uevent merging by WWID relies on the uid_attrs config option. As we > drop struct config between calls to uevent_merge(), we must be sure > that the WWID is not changed in reconfigure(). > > Signed-off-by: Martin Wilck Reviewed-by: Benjamin Marzinski > --- > multipath/multipath.conf.5 | 2 ++ > multipathd/main.c | 59 -- > 2 files changed, 52 insertions(+), 9 deletions(-) > > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 > index 605b46e..a9cd776 100644 > --- a/multipath/multipath.conf.5 > +++ b/multipath/multipath.conf.5 > @@ -264,6 +264,8 @@ If this option is configured and matches the device > node name of a device, it overrides any other configured methods for > determining the WWID for this device. > .PP > +This option cannot be changed during runtime with the multipathd > \fBreconfigure\fR command. > +.PP > The default is: \fB\fR. To enable uevent merging, set it e.g. to > \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq. > .RE > diff --git a/multipathd/main.c b/multipathd/main.c > index 13b1948..6808ad1 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2835,11 +2835,58 @@ void rcu_free_config(struct rcu_head *head) > free_config(conf); > } > > +static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs, > + const struct _vector *new_attrs) > +{ > + int i; > + char *old; > + > + if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs)) > + return true; > + > + vector_foreach_slot(old_attrs, old, i) { > + char *new = VECTOR_SLOT(new_attrs, i); > + > + if (strcmp(old, new)) > + return true; > + } > + > + return false; > +} > + > +static void reconfigure_check(struct config *old, struct config *new) > +{ > + int old_marginal_pathgroups; > + > + old_marginal_pathgroups = old->marginal_pathgroups; > + if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) != > + (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) { > + condlog(1, "multipathd must be restarted to turn %s fpin > marginal paths", > + (old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)? > + "off" : "on"); > + new->marginal_pathgroups = old_marginal_pathgroups; > + } > + > + if (reconfigure_check_uid_attrs(>uid_attrs, >uid_attrs)) { > + int i; > + void *ptr; > + > + condlog(1, "multipathd must be restarted to change uid_attrs, > keeping old values"); > + vector_foreach_slot(>uid_attrs, ptr, i) > + free(ptr); > + vector_reset(>uid_attrs); > + new->uid_attrs = old->uid_attrs; > + > + /* avoid uid_attrs being freed in rcu_free_config() */ > + old->uid_attrs.allocated = 0; > + old->uid_attrs.slot = NULL; > + } > +} > + > static int > reconfigure (struct vectors *vecs, enum force_reload_types reload_type) > { > struct config * old, *conf; > - int old_marginal_pathgroups; > > conf = load_config(DEFAULT_CONFIGFILE); > if (!conf) > @@ -2870,14 +2917,8 @@ reconfigure (struct vectors *vecs, enum > force_reload_types reload_type) > uxsock_timeout = conf->uxsock_timeout; > > old = rcu_dereference(multipath_conf); > - old_marginal_pathgroups = old->marginal_pathgroups; > - if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) != > - (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) { > - condlog(1, "multipathd must be restarted to turn %s fpin > marginal paths", > - (old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)? > - "off" : "on"); > - conf->marginal_pathgroups = old_marginal_pathgroups; > - } > + reconfigure_check(old, conf); > + > conf->sequence_nr = old->sequence_nr + 1; > rcu_assign_pointer(multipath_conf, conf); > call_rcu(>rcu, rcu_free_config); > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH v2 00/15] Rework uevent filtering and merging
On Mon, Apr 04, 2022 at 07:04:42PM +0200, mwi...@suse.com wrote: FYI, these are still not coming through as plain-text messages. See https://listman.redhat.com/archives/dm-devel/2022-April/050155.html -Ben -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 00/14] Rework uevent filtering and merging
On Thu, Mar 31, 2022 at 12:14:56AM +0200, mwi...@suse.com wrote: > Hi Ben, hi Christophe, hi Tang Junhui, > > the bulk of this patch set (3-8) is a rework of the uevent filtering and > merging logic introduced in commit eef ("multipath-tools: improve > processing efficiency for addition and deletion of multipath devices"), > by Tang Junhui. > > The rationale is explained in detail in the commit message in patch 08/14. > TL;DR: The previous approach delayed uevent handling, possibly a lot, which > is often undesirable. The new logic passes events to the dispatcher > immediately, but if they queue up (because the dispatcher can't keep > up with the rate at which events arrive, or is blocked e.g. by the > path checker), the dispatcher will apply filtering and merging > between servicing individual events. This worked well in my own testing, > but I'd appreciate if ZTE could give it a shot in their special test > environment. > > Patch 9-14 add some more improvements to the uevent handling code, and > improve logging. The first 2 patches are unrelated fixes. > > Comments welcome, > > Martin For everything except 04, 05, and 06 Reviewed-by: Benjamin Marzinski BTW, for some reason, these emails didn't come through a plain test for me. > > Martin Wilck (14): > multipathd: allow adding git rev as version suffix > multipathd: don't switch to DAEMON_IDLE during startup > uevent_dispatch(): use while in wait loop > libmultipath: uevent_dispatch(): process uevents one by one > libmultipath: uevent_dispatch(): only filter/merge new uevents > multipathd: reconfigure: disallow changing uid_attrs > libmultipath: microoptimize uevent filtering and merging > libmultipath: uevent_listen(): don't delay uevents > libmultipath: uevent: use struct to pass parameters around > libmultipath: uevent: log statistics about filtering and merging > libmultipath: merge_uevq(): filter first, then merge > libmultipath: uevent_filter(): filter previously merged events > libmultipath: uevent: improve log messages > libmultipath: uevent: add debug messages for event queue > > Makefile.inc | 3 +- > libmultipath/config.c | 6 +- > libmultipath/config.h | 4 +- > libmultipath/discovery.c | 2 +- > libmultipath/list.h| 53 + > libmultipath/structs.h | 2 +- > libmultipath/uevent.c | 407 ++--- > libmultipath/uevent.h | 3 +- > multipath/multipath.conf.5 | 2 + > multipathd/main.c | 59 -- > tests/uevent.c | 2 +- > 11 files changed, 354 insertions(+), 189 deletions(-) > > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 12/14] libmultipath: uevent_filter(): filter previously merged events
On Thu, Mar 31, 2022 at 12:15:08AM +0200, mwi...@suse.com wrote: > With the new list-appending logic, it can happen that previously > merged events can now be filtered. Do it. > > Signed-off-by: Martin Wilck > --- > libmultipath/uevent.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index eb900ec..809c74c 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -305,7 +305,7 @@ static void uevent_delete_from_list(struct uevent > *to_delete, >* for the anchor), "old_tail" must be moved. It can happen that >* "old_tail" ends up pointing at the anchor. >*/ > - if (*old_tail == _delete->node) > + if (old_tail && *old_tail == _delete->node) > *old_tail = to_delete->node.prev; > > list_del_init(_delete->node); > @@ -360,6 +360,20 @@ uevent_filter(struct uevent *later, struct > uevent_filter_state *st) >* filter unnessary earlier uevents >* by the later uevent >*/ > + if (!list_empty(>merge_node)) { > + struct uevent *mn, *t; > + > + list_for_each_entry_reverse_safe(mn, t, > >merge_node, node) { > + if (uevent_can_filter(mn, later)) { > + condlog(4, "uevent: \"%s %s\" (merged > into \"%s %s\") filtered by \"%s %s\"", > + mn->action, mn->kernel, > + earlier->action, > earlier->kernel, > + later->action, later->kernel); > + uevent_delete_from_list(mn, , NULL); Just like with 05/14, you could just use a much simpler delete function here, since moving old_tail and merged nodes is unnecessary. I guess I don't care that much, if you'd rather just have one function, so Reviewed-by: Benjamin Marzinski > + st->filtered++; > + } > + } > + } > if (uevent_can_filter(earlier, later)) { > condlog(3, "uevent: %s-%s has filtered by uevent: > %s-%s", > earlier->kernel, earlier->action, > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs
> On Thu, Mar 31, 2022 at 12:15:02AM +0200, mwi...@suse.com wrote: > uevent merging by WWID relies on the uid_attrs config option. As we > drop struct config between calls to uevent_merge(), we must be sure > that the WWID is not changed in reconfigure(). > > Signed-off-by: Martin Wilck > --- > multipath/multipath.conf.5 | 2 ++ > multipathd/main.c | 53 +++--- > 2 files changed, 46 insertions(+), 9 deletions(-) > > diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5 > index 605b46e..a9cd776 100644 > --- a/multipath/multipath.conf.5 > +++ b/multipath/multipath.conf.5 > @@ -264,6 +264,8 @@ If this option is configured and matches the device > node name of a device, it overrides any other configured methods for > determining the WWID for this device. > .PP > +This option cannot be changed during runtime with the multipathd > \fBreconfigure\fR command. > +.PP > The default is: \fB\fR. To enable uevent merging, set it e.g. to > \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq. > .RE > diff --git a/multipathd/main.c b/multipathd/main.c > index 13b1948..f514b32 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head) > free_config(conf); > } > > +static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs, > + const struct _vector *new_attrs) > +{ > + int i; > + char *old; > + > + if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs)) > + return true; > + > + vector_foreach_slot(old_attrs, old, i) { > + char *new = VECTOR_SLOT(new_attrs, i); > + > + if (strcmp(old, new)) > + return true; > + } > + > + return false; > +} > + > +static void reconfigure_check(struct config *old, struct config *new) > +{ > + int old_marginal_pathgroups; > + > + old_marginal_pathgroups = old->marginal_pathgroups; > + if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) != > + (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) { > + condlog(1, "multipathd must be restarted to turn %s fpin > marginal paths", > + (old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)? > + "off" : "on"); > + new->marginal_pathgroups = old_marginal_pathgroups; > + } > + > + if (reconfigure_check_uid_attrs(>uid_attrs, >uid_attrs)) { > + struct _vector v = new->uid_attrs; > + > + condlog(1, "multipathd must be restarted to change uid_attrs, > keeping old values"); > + new->uid_attrs = old->uid_attrs; > + vector_reset(); This leaks the strings that v points to, right? This also can happen in uid_attrs_handler(), I just noticed. -Ben > + old->uid_attrs = v; > + } > +} > + > static int > reconfigure (struct vectors *vecs, enum force_reload_types reload_type) > { > struct config * old, *conf; > - int old_marginal_pathgroups; > > conf = load_config(DEFAULT_CONFIGFILE); > if (!conf) > @@ -2870,14 +2911,8 @@ reconfigure (struct vectors *vecs, enum > force_reload_types reload_type) > uxsock_timeout = conf->uxsock_timeout; > > old = rcu_dereference(multipath_conf); > - old_marginal_pathgroups = old->marginal_pathgroups; > - if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) != > - (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) { > - condlog(1, "multipathd must be restarted to turn %s fpin > marginal paths", > - (old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)? > - "off" : "on"); > - conf->marginal_pathgroups = old_marginal_pathgroups; > - } > + reconfigure_check(old, conf); > + > conf->sequence_nr = old->sequence_nr + 1; > rcu_assign_pointer(multipath_conf, conf); > call_rcu(>rcu, rcu_free_config); > -- > 2.35.1 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 05/14] libmultipath: uevent_dispatch(): only filter/merge new uevents
> On Thu, Mar 31, 2022 at 12:15:01AM +0200, mwi...@suse.com wrote: > When uevq_work is non-empty and we append a list of new events, > we don't need to check the entire list for filterable and mergeable > uevents. uevq_work had been filtered and merged already. So we just > need to check the newly appended events. These must of course be > checked for merges with earlier events, too. > > We must deal with some special cases here, like previously merged > uevents being filtered later. > > Signed-off-by: Martin Wilck > --- > libmultipath/list.h | 24 ++ > libmultipath/uevent.c | 77 +-- > 2 files changed, 83 insertions(+), 18 deletions(-) > > diff --git a/libmultipath/list.h b/libmultipath/list.h > index ddea99f..248f72b 100644 > --- a/libmultipath/list.h > +++ b/libmultipath/list.h > @@ -363,6 +363,30 @@ static inline struct list_head *list_pop(struct > list_head *head) >>member != (head);\ >pos = n, n = list_entry(n->member.prev, typeof(*n), member)) > > +/** > + * list_for_some_entry - iterate list from the given begin node to the given > end node > + * @pos: the type * to use as a loop counter. > + * @from:the begin node of the iteration. > + * @to: the end node of the iteration. > + * @member: the name of the list_struct within the struct. > + */ > +#define list_for_some_entry(pos, from, to, member) \ > + for (pos = list_entry((from)->next, typeof(*pos), member); \ > + >member != (to); \ > + pos = list_entry(pos->member.next, typeof(*pos), member)) > + > +/** > + * list_for_some_entry_reverse - iterate backwards list from the given begin > node to the given end node > + * @pos: the type * to use as a loop counter. > + * @from:the begin node of the iteration. > + * @to: the end node of the iteration. > + * @member: the name of the list_struct within the struct. > + */ > +#define list_for_some_entry_reverse(pos, from, to, member) \ > + for (pos = list_entry((from)->prev, typeof(*pos), member); \ > + >member != (to); \ > + pos = list_entry(pos->member.prev, typeof(*pos), member)) > + > /** > * list_for_some_entry_safe - iterate list from the given begin node to the > given end node safe against removal of list entry > * @pos: the type * to use as a loop counter. > diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c > index 602eccb..2779703 100644 > --- a/libmultipath/uevent.c > +++ b/libmultipath/uevent.c > @@ -306,17 +306,49 @@ uevent_can_merge(struct uevent *earlier, struct uevent > *later) > return false; > } > > +static void uevent_delete_from_list(struct uevent *to_delete, > + struct uevent **previous, > + struct list_head **old_tail) > +{ > + /* > + * "old_tail" is the list_head before the last list element to which > + * the caller iterates (the list anchor if the caller iterates over > + * the entire list). If this element is removed (which can't happen > + * for the anchor), "old_tail" must be moved. It can happen that > + * "old_tail" ends up pointing at the anchor. > + */ > + if (*old_tail == _delete->node) > + *old_tail = to_delete->node.prev; > + > + list_del_init(_delete->node); > + > + /* > + * The "to_delete" uevent has been merged with other uevents > + * previously. Re-insert them into the list, at the point we're > + * currently at. This must be done after the list_del_init() above, > + * otherwise previous->next would still point to to_delete. > + */ > + if (!list_empty(_delete->merge_node)) { > + struct uevent *last = list_entry(to_delete->merge_node.prev, > + typeof(*last), node); > + > + list_splice(_delete->merge_node, &(*previous)->node); > + *previous = last; > + } > + if (to_delete->udev) > + udev_device_unref(to_delete->udev); > + > + free(to_delete); > +} > + > static void > -uevent_prepare(struct list_head *tmpq) > +uevent_prepare(struct list_head *tmpq, struct list_head **stop) > { > struct uevent *uev, *tmp; > > - list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) { > + list_for_some_entry_reverse_safe(uev, tmp, tmpq, *stop, node) { > if (uevent_can_discard(uev)) { > - list_del_init(>node); > - if (uev->udev) > - udev_device_unref(uev->udev); > - free(uev); > + uevent_delete_from_list(uev, , stop); You are only running this on new events, so they can't possibly have any merged uevents, and can't possibly be equal to stop, so the old,