As explained in the previous commit, setting max_sectors_kb for paths that are members of a live map is dangerous. But as long as a path is not a member of a map, setting max_sectors_kb poses no risk. Set the parameter in adopt_paths(), for paths that aren't members of the map yet. In order to determine whether or not a path is currently member of the map, adopt_paths() needs to passed the current member list. If (and only if) it's called from coalesce_paths(), the passed struct multipath does not represent the current state of the map; adopt_paths() must therefore get a new argument current_mpp that represents the kernel state.
We must still call sysfs_set_max_sectors_kb() from domap() in the ACT_CREATE case, because when adopt_paths is called from coalesce_paths() for a map that doesn't exist yet, mpp->max_sectors_kb is not set. Signed-off-by: Martin Wilck <[email protected]> --- libmultipath/configure.c | 2 +- libmultipath/structs_vec.c | 62 ++++++++++++++++++++++++++++++++++---- libmultipath/structs_vec.h | 6 ++-- multipathd/main.c | 6 ++-- tests/Makefile | 2 +- tests/test-lib.c | 9 +++++- 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/libmultipath/configure.c b/libmultipath/configure.c index 8cbb2a8..b5c701f 100644 --- a/libmultipath/configure.c +++ b/libmultipath/configure.c @@ -1105,7 +1105,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid, /* * at this point, we know we really got a new mp */ - mpp = add_map_with_path(vecs, pp1, 0); + mpp = add_map_with_path(vecs, pp1, 0, cmpp); if (!mpp) { orphan_path(pp1, "failed to create multipath device"); continue; diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index 0fc608c..c0c5cc9 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -242,7 +242,38 @@ static bool update_pathvec_from_dm(vector pathvec, struct multipath *mpp, return must_reload; } -int adopt_paths(vector pathvec, struct multipath *mpp) +static bool set_path_max_sectors_kb(const struct path *pp, int max_sectors_kb) +{ + char buf[11]; + ssize_t len; + int ret, current; + bool rc = false; + + if (max_sectors_kb == MAX_SECTORS_KB_UNDEF) + return rc; + + if (sysfs_attr_get_value(pp->udev, "queue/max_sectors_kb", buf, sizeof(buf)) <= 0 + || sscanf(buf, "%d\n", ¤t) != 1) + current = MAX_SECTORS_KB_UNDEF; + if (current == max_sectors_kb) + return rc; + + len = snprintf(buf, sizeof(buf), "%d", max_sectors_kb); + ret = sysfs_attr_set_value(pp->udev, "queue/max_sectors_kb", buf, len); + if (ret != len) + log_sysfs_attr_set_value(3, ret, + "failed setting max_sectors_kb on %s", + pp->dev); + else { + condlog(3, "%s: set max_sectors_kb to %d for %s", __func__, + max_sectors_kb, pp->dev); + rc = true; + } + return rc; +} + +int adopt_paths(vector pathvec, struct multipath *mpp, + const struct multipath *current_mpp) { int i, ret; struct path * pp; @@ -285,9 +316,28 @@ int adopt_paths(vector pathvec, struct multipath *mpp) continue; } - if (!find_path_by_devt(mpp->paths, pp->dev_t) && - store_path(mpp->paths, pp)) - goto err; + if (!find_path_by_devt(mpp->paths, pp->dev_t)) { + + if (store_path(mpp->paths, pp)) + goto err; + /* + * Setting max_sectors_kb on live paths is dangerous. + * But we can do it here on a path that isn't yet part + * of the map. If this value is lower than the current + * max_sectors_kb and the map is reloaded, the map's + * max_sectors_kb will be safely adjusted by the kernel. + * + * We must make sure that the path is not part of the + * map yet. Normally we can check this in mpp->paths. + * But if adopt_paths is called from coalesce_paths, + * we need to check the separate struct multipath that + * has been obtained from map_discovery(). + */ + if (!current_mpp || + !mp_find_path_by_devt(current_mpp, pp->dev_t)) + mpp->need_reload = mpp->need_reload || + set_path_max_sectors_kb(pp, mpp->max_sectors_kb); + } pp->mpp = mpp; condlog(3, "%s: ownership set to %s", @@ -693,7 +743,7 @@ find_existing_alias (struct multipath * mpp, } struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp, - int add_vec) + int add_vec, const struct multipath *current_mpp) { struct multipath * mpp; struct config *conf = NULL; @@ -721,7 +771,7 @@ struct multipath *add_map_with_path(struct vectors *vecs, struct path *pp, goto out; mpp->size = pp->size; - if (adopt_paths(vecs->pathvec, mpp) || pp->mpp != mpp || + if (adopt_paths(vecs->pathvec, mpp, current_mpp) || pp->mpp != mpp || find_slot(mpp->paths, pp) == -1) goto out; diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h index 3253f1b..dbc4305 100644 --- a/libmultipath/structs_vec.h +++ b/libmultipath/structs_vec.h @@ -13,7 +13,8 @@ struct vectors { void set_no_path_retry(struct multipath *mpp); -int adopt_paths (vector pathvec, struct multipath * mpp); +int adopt_paths (vector pathvec, struct multipath *mpp, + const struct multipath *current_mpp); void orphan_path (struct path * pp, const char *reason); void set_path_removed(struct path *pp); @@ -28,7 +29,8 @@ void remove_maps (struct vectors * vecs); void sync_map_state (struct multipath *); struct multipath * add_map_with_path (struct vectors * vecs, - struct path * pp, int add_vec); + struct path * pp, int add_vec, + const struct multipath *current_mpp); void update_queue_mode_del_path(struct multipath *mpp); void update_queue_mode_add_path(struct multipath *mpp); int update_multipath_table (struct multipath *mpp, vector pathvec, int flags); diff --git a/multipathd/main.c b/multipathd/main.c index dd17d5c..d8518a9 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -636,7 +636,7 @@ update_map (struct multipath *mpp, struct vectors *vecs, int new_map) retry: condlog(4, "%s: updating new map", mpp->alias); - if (adopt_paths(vecs->pathvec, mpp)) { + if (adopt_paths(vecs->pathvec, mpp, NULL)) { condlog(0, "%s: failed to adopt paths for new map update", mpp->alias); retries = -1; @@ -1231,7 +1231,7 @@ rescan: if (mpp) { condlog(4,"%s: adopting all paths for path %s", mpp->alias, pp->dev); - if (adopt_paths(vecs->pathvec, mpp) || pp->mpp != mpp || + if (adopt_paths(vecs->pathvec, mpp, NULL) || pp->mpp != mpp || find_slot(mpp->paths, pp) == -1) goto fail; /* leave path added to pathvec */ @@ -1245,7 +1245,7 @@ rescan: return 0; } condlog(4,"%s: creating new map", pp->dev); - if ((mpp = add_map_with_path(vecs, pp, 1))) { + if ((mpp = add_map_with_path(vecs, pp, 1, NULL))) { mpp->action = ACT_CREATE; /* * We don't depend on ACT_CREATE, as domap will diff --git a/tests/Makefile b/tests/Makefile index d1d52dd..4005204 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -45,7 +45,7 @@ dmevents-test_OBJDEPS = $(multipathdir)/devmapper.o dmevents-test_LIBDEPS = -lpthread -ldevmapper -lurcu hwtable-test_TESTDEPS := test-lib.o hwtable-test_OBJDEPS := $(multipathdir)/discovery.o $(multipathdir)/blacklist.o \ - $(multipathdir)/structs.o $(multipathdir)/propsel.o + $(multipathdir)/structs_vec.o $(multipathdir)/structs.o $(multipathdir)/propsel.o hwtable-test_LIBDEPS := -ludev -lpthread -ldl blacklist-test_TESTDEPS := test-log.o blacklist-test_LIBDEPS := -ludev diff --git a/tests/test-lib.c b/tests/test-lib.c index cdb2780..88f35e9 100644 --- a/tests/test-lib.c +++ b/tests/test-lib.c @@ -152,6 +152,13 @@ int __wrap_sysfs_get_size(struct path *pp, unsigned long long *sz) return 0; } +int __wrap_sysfs_attr_set_value(struct udev_device *dev, const char *attr_name, + const char * value, size_t value_len) +{ + condlog(5, "%s: %s", __func__, value); + return value_len; +} + void *__wrap_udev_device_get_parent_with_subsystem_devtype( struct udev_device *ud, const char *subsys, char *type) { @@ -400,7 +407,7 @@ struct multipath *__mock_multipath(struct vectors *vecs, struct path *pp) /* pathinfo() call in adopt_paths */ mock_pathinfo(DI_CHECKER|DI_PRIO, &mop); - mp = add_map_with_path(vecs, pp, 1); + mp = add_map_with_path(vecs, pp, 1, NULL); assert_ptr_not_equal(mp, NULL); /* TBD: mock setup_map() ... */ -- 2.44.0
