[PATCH 0/8] multipath-tools: max_sectors_kb rework

[PATCH 0/8] multipath-tools: max_sectors_kb rework

[Martin Wilck]

This patch series addresses the issue described in [1].

Changing max_sectors_kb on a live multipath map is dangerous. In particular,
no path device may have a lower value than the map at any time, unless the
device is suspended and all I/O has been flushed. It is also dangerous to
read a max_sectors_kb value from sysfs and write it back, because this may
actually cause the max_sectors value in the kernel (which is in 512 byte block
units, not in kiB) to be rounded down to the next even number.

This patch set avoids both these problematic techniques. max_sectors_kb is
only changed for maps that have the respective configuration parameter set,
and only during map creation or during path addition. In the latter case,
the limit is set on the new path before actually adding it to the map, thus
avoiding a change on a live path. While it is generally recommended to set
max_sectors_kb once and for all before creating a map, the latter technique
can be used to change the value on a live map, by removing and re-adding
a path device.

A side effect of this patch set is that not necessarily all path devices of
a map that has max_sectors_kb set will have this limit configured. In
particular, if max_sectors_kb is enforced by adding a path device, only the
added path and the map itself will have the new max_sectors_kb value, because
multipathd will not attempt to change the value of the other paths, which
can therefore be higher than the configured limit. This is not a problem.
Errors arise only if the limit of the map device is higher than that of
any path device.

This patch set does not attempt to address issues between a multipath device
and block layers stacked on to of it (e.g. LVM or MD raid), as discussed in
the commit message of 8fd4868 ("libmultipath: don't set max_sectors_kb on
reloads"). I think, and my experiments have confirmed it, that for bio-based
stacked devices on top of multipath, max_sectors_kb is not a problem. bios
with a large amount of I/O are split in the kernel to fit into the lower-level
device's limits. Only request-based dm aka multipath is susceptible to errors
caused by max_sectors_kb mismatch. If I am overlooking something here, please
provide an example to prove me wrong.

https://lore.kernel.org/dm-devel/7742003e19b5a49398067dc0c59dfa8ddeffc3d7.camel@suse.com/

Martin Wilck (8):
  multipath-tools: add TGTDIR option
  libmultipath: move get_udev_for_mpp to sysfs.c
  libmultipath: add mp_find_path_by_devt()
  Revert "libmultipath: fix max_sectors_kb on adding path"
  libmultipath: Only set max_sectors_kb on map creation
  libmultipath: set max_sectors_kb in adopt_paths()
  libmultipath: add wildcard %k for printing max_sectors_kb
  multipath.conf(5): update documentation for max_sectors_kb

 .github/actions/spelling/expect.txt |  2 +
 Makefile.inc                        |  8 ++--
 README.md                           | 33 +++++++++++++++
 libmultipath/configure.c            | 63 +++--------------------------
 libmultipath/print.c                | 38 +++++++++++++++++
 libmultipath/structs.c              | 19 +++++++++
 libmultipath/structs.h              |  3 ++
 libmultipath/structs_vec.c          | 62 +++++++++++++++++++++++++---
 libmultipath/structs_vec.h          |  6 ++-
 libmultipath/sysfs.c                | 19 +++++++++
 libmultipath/sysfs.h                |  4 ++
 multipath/multipath.conf.5.in       | 33 +++++++++++++--
 multipathd/main.c                   |  6 +--
 tests/Makefile                      |  2 +-
 tests/test-lib.c                    |  9 ++++-
 15 files changed, 229 insertions(+), 78 deletions(-)

-- 
2.44.0

[PATCH 1/8] multipath-tools: add TGTDIR option

[Martin Wilck]

TGTDIR is a convenience option for developers for building multipath-tools such
that compiled-in paths of the plugins, config files, etc. match an
installation under some optional path. See README.md for instructions and
examples.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 .github/actions/spelling/expect.txt |  2 ++
 Makefile.inc                        |  8 +++----
 README.md                           | 33 +++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt
index 5906403..fc9f22f 100644
--- a/.github/actions/spelling/expect.txt
+++ b/.github/actions/spelling/expect.txt
@@ -167,6 +167,7 @@ retrigger
 rhabarber
 rootprefix
 rootprefixdir
+rpmbuild
 rport
 rtpi
 sas
@@ -200,6 +201,7 @@ tcp
 TESTDEPS
 testname
 tgill
+TGTDIR
 TIDS
 tmo
 tpg
diff --git a/Makefile.inc b/Makefile.inc
index 5668e63..81b86cd 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -101,9 +101,9 @@ WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) -Werror=implici
 		  $(WNOCLOBBERED) -Werror=cast-qual $(ERROR_DISCARDED_QUALIFIERS) $(W_URCU_TYPE_LIMITS)
 CPPFLAGS	:= $(FORTIFY_OPT) $(CPPFLAGS) $(D_URCU_VERSION) \
 		   -D_FILE_OFFSET_BITS=64 \
-		   -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(plugindir)\" \
-		   -DRUNTIME_DIR=\"$(runtimedir)\" -DCONFIG_DIR=\"$(configdir)\" \
-		   -DDEFAULT_CONFIGFILE=\"$(configfile)\" -DSTATE_DIR=\"$(statedir)\" \
+		   -DBIN_DIR=\"$(bindir)\" -DMULTIPATH_DIR=\"$(TGTDIR)$(plugindir)\" \
+		   -DRUNTIME_DIR=\"$(runtimedir)\" -DCONFIG_DIR=\"$(TGTDIR)$(configdir)\" \
+		   -DDEFAULT_CONFIGFILE=\"$(TGTDIR)$(configfile)\" -DSTATE_DIR=\"$(TGTDIR)$(statedir)\" \
 		   -DEXTRAVERSION=\"$(EXTRAVERSION)\" -MMD -MP
 CFLAGS		:= -std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe
 BIN_CFLAGS	:= -fPIE -DPIE
@@ -149,4 +149,4 @@ NV_VERSION_SCRIPT = $(DEVLIB:%.so=%-nv.version)
 
 %:	%.in
 	@echo creating $@
-	$(Q)sed 's:@CONFIGFILE@:'$(configfile)':g;s:@CONFIGDIR@:'$(configdir)':g;s:@STATE_DIR@:'$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g;s:@BINDIR@:'$(bindir)':g' $< >$@
+	$(Q)sed 's:@CONFIGFILE@:'$(TGTDIR)$(configfile)':g;s:@CONFIGDIR@:'$(TGTDIR)$(configdir)':g;s:@STATE_DIR@:'$(TGTDIR)$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g;s:@BINDIR@:'$(bindir)':g' $< >$@
diff --git a/README.md b/README.md
index d4f35f5..7207b14 100644
--- a/README.md
+++ b/README.md
@@ -151,6 +151,39 @@ sufficient control. See `Makefile.inc` for even more fine-grained control.
 	On such distributions, override `unitdir` and `libudevdir` to use systemd's
    `rootprefix`: `make libudevdir=/lib/udev unitdir=/lib/systemd/system`
 
+### prefix, DESTDIR and TGTDIR
+
+`prefix` and related variables are included in compiled-in paths like
+`plugindir` and are used by `make install`. Using `prefix` is useful if
+multipath-tools is built locally on the same host where it's supposed to be
+installed.
+
+By convention, the `DESTDIR` variable is prepended to all paths by `make
+install`, but not to any compiled-in paths.
+It is useful if the software is built on one system (build host) but intended
+to be run on another system (installation host). This is typically used in build
+systems like *rpmbuild* to set a root directory for all the installed
+files.
+
+On the contrary, the `TGTDIR` variable is used for compiled-in paths only, and
+ignored by `make install`. It is useful for running multipath-tools in a separate
+subdirectory in the installation host, mostly for testing / development
+purposes.
+
+For example,
+
+    make prefix=/opt DESTDIR=/build TGTDIR=/test install
+
+will install plugins into `/build/opt/lib64/multipath` on the build
+host. On the installation host, the plugins will be expected to be found under
+`/test/opt/lib64/multipath`. If the developer runs
+
+    rsync -a $BUILD_HOST:$DESTDIR/ $INSTALL_HOST:$TGTDIR/
+	
+and adds `$TGTDIR/lib64` to `LD_LIBRARY_PATH` on the installation host, the
+multipath binaries installed under `$TGTDIR` will find their plugins and
+configuration files in the expected compiled-in paths.
+
 ### Compiler Options
 
 Use `OPTFLAGS` to change optimization-related compiler options;
-- 
2.44.0

[PATCH 2/8] libmultipath: move get_udev_for_mpp to sysfs.c

[Martin Wilck]

No functional changes, just code movement.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 20 --------------------
 libmultipath/sysfs.c     | 19 +++++++++++++++++++
 libmultipath/sysfs.h     |  4 ++++
 3 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 4ecf6ba..89ac03d 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -453,26 +453,6 @@ pgcmp (struct multipath * mpp, struct multipath * cmpp)
 	return r;
 }
 
-static struct udev_device *
-get_udev_for_mpp(const struct multipath *mpp)
-{
-	dev_t devnum;
-	struct udev_device *udd;
-
-	if (!mpp || !has_dm_info(mpp)) {
-		condlog(1, "%s called with empty mpp", __func__);
-		return NULL;
-	}
-
-	devnum = makedev(mpp->dmi.major, mpp->dmi.minor);
-	udd = udev_device_new_from_devnum(udev, 'b', devnum);
-	if (!udd) {
-		condlog(1, "failed to get udev device for %s", mpp->alias);
-		return NULL;
-	}
-	return udd;
-}
-
 void trigger_partitions_udev_change(struct udev_device *dev,
 				    const char *action, int len)
 {
diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index 328951e..afe9de9 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -339,3 +339,22 @@ bool sysfs_is_multipathed(struct path *pp, bool set_wwid)
 
 	return found;
 }
+
+struct udev_device *get_udev_for_mpp(const struct multipath *mpp)
+{
+	dev_t devnum;
+	struct udev_device *udd;
+
+	if (!mpp || !has_dm_info(mpp)) {
+		condlog(1, "%s called with empty mpp", __func__);
+		return NULL;
+	}
+
+	devnum = makedev(mpp->dmi.major, mpp->dmi.minor);
+	udd = udev_device_new_from_devnum(udev, 'b', devnum);
+	if (!udd) {
+		condlog(1, "failed to get udev device for %s", mpp->alias);
+		return NULL;
+	}
+	return udd;
+}
diff --git a/libmultipath/sysfs.h b/libmultipath/sysfs.h
index 2db86bd..3be3f66 100644
--- a/libmultipath/sysfs.h
+++ b/libmultipath/sysfs.h
@@ -39,4 +39,8 @@ do {									\
 int sysfs_get_size (struct path *pp, unsigned long long * size);
 int sysfs_check_holders(char * check_devt, char * new_devt);
 bool sysfs_is_multipathed(struct path *pp, bool set_wwid);
+
+struct multipath;
+struct udev_device *get_udev_for_mpp(const struct multipath *mpp);
+
 #endif
-- 
2.44.0

[PATCH 3/8] libmultipath: add mp_find_path_by_devt()

[Martin Wilck]

A helper function that searches a struct multipath by dev_t, and
works whether or not mpp->paths is currently available.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/structs.c | 19 +++++++++++++++++++
 libmultipath/structs.h |  3 +++
 2 files changed, 22 insertions(+)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 74e31a1..e248fb5 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -526,6 +526,25 @@ find_path_by_devt (const struct _vector *pathvec, const char * dev_t)
 	return NULL;
 }
 
+struct path *mp_find_path_by_devt(const struct multipath *mpp, const char *devt)
+{
+	struct path *pp;
+	struct pathgroup *pgp;
+	unsigned int i, j;
+
+	pp = find_path_by_devt(mpp->paths, devt);
+	if (pp)
+		return pp;
+
+	vector_foreach_slot (mpp->pg, pgp, i){
+		vector_foreach_slot (pgp->paths, pp, j){
+			if (!strcmp(pp->dev_t, devt))
+				return pp;
+		}
+	}
+	return NULL;
+}
+
 static int do_pathcount(const struct multipath *mpp, const int *states,
 			unsigned int nr_states)
 {
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 734905e..a25eb9d 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -568,6 +568,9 @@ struct path * find_path_by_devt (const struct _vector *pathvec, const char *devt
 struct path * find_path_by_dev (const struct _vector *pathvec, const char *dev);
 struct path * first_path (const struct multipath *mpp);
 
+struct path *mp_find_path_by_devt(const struct multipath *mpp, const char *devt);
+
+
 int pathcount (const struct multipath *, int);
 int count_active_paths(const struct multipath *);
 int count_active_pending_paths(const struct multipath *);
-- 
2.44.0

[PATCH 4/8] Revert "libmultipath: fix max_sectors_kb on adding path"

[Martin Wilck]

This reverts commit bbb77f318ee483292f50a7782aecaecc7e60f727.
---
 libmultipath/configure.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 89ac03d..13602f3 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -575,12 +575,11 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 	ssize_t len;
 	int i, j, ret, err = 0;
 	struct udev_device *udd;
-	int max_sectors_kb = mpp->max_sectors_kb;
+	int max_sectors_kb;
 
-	/* by default, do not initialize max_sectors_kb on the device */
-	if (max_sectors_kb == MAX_SECTORS_KB_UNDEF && !is_reload)
+	if (mpp->max_sectors_kb == MAX_SECTORS_KB_UNDEF)
 		return 0;
-	/* on reload, re-apply the user tuning on all the path devices */
+	max_sectors_kb = mpp->max_sectors_kb;
 	if (is_reload) {
 		if (!has_dm_info(mpp) &&
 		    dm_get_info(mpp->alias, &mpp->dmi) != 0) {
-- 
2.44.0

[PATCH 5/8] libmultipath: Only set max_sectors_kb on map creation

[Martin Wilck]

Changing max_sectors_kb on a live map is dangerous. When I/O occurs while the
max_sectors_kb value of a map is larger than that of any of its paths, I/O
errors like this may result:

  blk_insert_cloned_request: over max size limit. (127 > 126)

This situation must be strictly avoided. The kernel makes sure that the
map's limits match those of the paths when the map is created. But setting
max_sectors_kb on path devices before reloading is dangerous, even if we
read the value from the map's max_sectors_kb beforehand. The reason for
this is that the sysfs value max_sectors_kb is the kernel's max_sectors
divided by 2, and user space has no way to figure out if the kernel-internal
value is odd or even. Thus by writing back the value just read to
max_sectors_kb, one might actually decrease the kernel value by one.

The only safe way to set max_sectors_kb on a live map would be to suspend
it, flushing all outstanding IO, then the path max_sectors_kb, reload,
and resume.

Since commit 8fd4868 ("libmultipath: don't set max_sectors_kb on reloads"),
we don't set the configured max_sectors_kb any more. But as shown above,
this is not safe. So really only set max_sectors_kb when creating a map.

Users who have stacked block devices on top of multipath, as described in the
commit message of 8fd4868 must make sure that the max_sectors_kb setting is
correct when the multipath map is first created. Decreasing the map's
max_sectors_kb value (without touching the paths) should actually be possible
in this situation, because stacked block devices are usually bio-based, and
bio-based IO (in contrast to request-based) can be split if the lower-level
device has can't handle the size of the I/O.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 38 ++++----------------------------------
 1 file changed, 4 insertions(+), 34 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 13602f3..8cbb2a8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -566,46 +566,18 @@ trigger_paths_udev_change(struct multipath *mpp, bool is_mpath)
 	mpp->needs_paths_uevent = 0;
 }
 
-static int
-sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
+static int sysfs_set_max_sectors_kb(struct multipath *mpp)
 {
 	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;
 
 	if (mpp->max_sectors_kb == MAX_SECTORS_KB_UNDEF)
 		return 0;
-	max_sectors_kb = mpp->max_sectors_kb;
-	if (is_reload) {
-		if (!has_dm_info(mpp) &&
-		    dm_get_info(mpp->alias, &mpp->dmi) != 0) {
-			condlog(1, "failed to get dm info for %s", mpp->alias);
-			return 1;
-		}
-		udd = get_udev_for_mpp(mpp);
-		if (!udd) {
-			condlog(1, "failed to get udev device to set max_sectors_kb for %s", mpp->alias);
-			return 1;
-		}
-		ret = sysfs_attr_get_value(udd, "queue/max_sectors_kb", buff,
-					   sizeof(buff));
-		udev_device_unref(udd);
-		if (!sysfs_attr_value_ok(ret, sizeof(buff))) {
-			condlog(1, "failed to get current max_sectors_kb from %s", mpp->alias);
-			return 1;
-		}
-		if (sscanf(buff, "%u\n", &max_sectors_kb) != 1) {
-			condlog(1, "can't parse current max_sectors_kb from %s",
-				mpp->alias);
-			return 1;
-		}
-	}
-	snprintf(buff, 11, "%d", max_sectors_kb);
-	len = strlen(buff);
+
+	len = snprintf(buff, sizeof(buff), "%d", mpp->max_sectors_kb);
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot(pgp->paths, pp, j) {
@@ -923,7 +895,7 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 			return DOMAP_RETRY;
 		}
 
-		sysfs_set_max_sectors_kb(mpp, 0);
+		sysfs_set_max_sectors_kb(mpp);
 		if (is_daemon && mpp->ghost_delay > 0 && count_active_paths(mpp) &&
 		    pathcount(mpp, PATH_UP) == 0)
 			mpp->ghost_delay_tick = mpp->ghost_delay;
@@ -934,7 +906,6 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 
 	case ACT_RELOAD:
 	case ACT_RELOAD_RENAME:
-		sysfs_set_max_sectors_kb(mpp, 1);
 		if (mpp->ghost_delay_tick > 0 && pathcount(mpp, PATH_UP))
 			mpp->ghost_delay_tick = 0;
 		r = dm_addmap_reload(mpp, params, 0);
@@ -942,7 +913,6 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
 
 	case ACT_RESIZE:
 	case ACT_RESIZE_RENAME:
-		sysfs_set_max_sectors_kb(mpp, 1);
 		if (mpp->ghost_delay_tick > 0 && pathcount(mpp, PATH_UP))
 			mpp->ghost_delay_tick = 0;
 		r = dm_addmap_reload(mpp, params, 1);
-- 
2.44.0

[PATCH 6/8] libmultipath: set max_sectors_kb in adopt_paths()

[Martin Wilck]

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 <mwilck@suse.com>
---
 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", &current) != 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

[PATCH 7/8] libmultipath: add wildcard %k for printing max_sectors_kb

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/print.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/libmultipath/print.c b/libmultipath/print.c
index 360308d..efd5a16 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -32,6 +32,7 @@
 #include "util.h"
 #include "foreign.h"
 #include "strbuf.h"
+#include "sysfs.h"
 
 #define PRINT_PATH_LONG      "%w %i %d %D %p %t %T %s %o"
 #define PRINT_PATH_INDENT    "%i %d %D %t %T %o"
@@ -431,6 +432,27 @@ snprint_multipath_vpd_data(struct strbuf *buff,
 	return append_strbuf_str(buff, "[undef]");
 }
 
+static void cleanup_udev_device(struct udev_device **udd)
+{
+	if (*udd)
+		udev_device_unref(*udd);
+}
+
+static int
+snprint_multipath_max_sectors_kb(struct strbuf *buff, const struct multipath *mpp)
+{
+	char buf[11];
+	int max_sectors_kb;
+	struct udev_device *udd __attribute__((cleanup(cleanup_udev_device)))
+		= get_udev_for_mpp(mpp);
+
+	if (!udd ||
+	    sysfs_attr_get_value(udd, "queue/max_sectors_kb", buf, sizeof(buf)) <= 0 ||
+	    sscanf(buf, "%d\n", &max_sectors_kb) != 1)
+		return print_strbuf(buff, "n/a");
+	return print_strbuf(buff, "%d", max_sectors_kb);
+}
+
 /*
  * path info printing functions
  */
@@ -790,6 +812,20 @@ snprint_alua_tpg(struct strbuf *buff, const struct path * pp)
 	return print_strbuf(buff, "0x%04x", pp->tpg_id);
 }
 
+static int
+snprint_path_max_sectors_kb(struct strbuf *buff, const struct path *pp)
+{
+	char buf[11];
+	int max_sectors_kb;
+
+	if (!pp->udev ||
+	    sysfs_attr_get_value(pp->udev, "queue/max_sectors_kb", 
+				 buf, sizeof(buf)) <= 0 ||
+	    sscanf(buf, "%d\n", &max_sectors_kb) != 1)
+		return print_strbuf(buff, "n/a");
+	return print_strbuf(buff, "%d", max_sectors_kb);
+}
+
 static const struct multipath_data mpd[] = {
 	{'n', "name",          snprint_name},
 	{'w', "uuid",          snprint_multipath_uuid},
@@ -815,6 +851,7 @@ static const struct multipath_data mpd[] = {
 	{'e', "rev",           snprint_multipath_rev},
 	{'G', "foreign",       snprint_multipath_foreign},
 	{'g', "vpd page data", snprint_multipath_vpd_data},
+	{'k', "max_sectors_kb",snprint_multipath_max_sectors_kb},
 };
 
 static const struct path_data pd[] = {
@@ -845,6 +882,7 @@ static const struct path_data pd[] = {
 	{'I', "init_st",       snprint_initialized},
 	{'L', "LUN hex",       snprint_path_lunhex},
 	{'A', "TPG",           snprint_alua_tpg},
+	{'k', "max_sectors_kb",snprint_path_max_sectors_kb},
 };
 
 static const struct pathgroup_data pgd[] = {
-- 
2.44.0

[PATCH 8/8] multipath.conf(5): update documentation for max_sectors_kb

[Martin Wilck]

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.conf.5.in | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/multipath/multipath.conf.5.in b/multipath/multipath.conf.5.in
index c788c18..b29a75f 100644
--- a/multipath/multipath.conf.5.in
+++ b/multipath/multipath.conf.5.in
@@ -1311,11 +1311,36 @@ The default is: \fB0\fR
 .
 .TP
 .B max_sectors_kb
-Sets the max_sectors_kb device parameter on all path devices and the multipath
-device to the specified value.
+Sets the \fImax_sectors_kb\fR device parameter on some path devices and the multipath
+device to the specified value. \fImax_sectors_kb\fR is the largest I/O size, in units
+of 1024 bytes, that the kernel allows for a single I/O request. For hardware devices
+like SCSI disks, this value is limited by the capabilities of the hardware.
+It is crucial that the value of a multipath map is never higher than the minimum value of
+of all its path devices. This is ensured by the kernel when a multipath map
+is loaded, but manipulating the values of a map or either of its paths while the
+map is live can cause race conditions and I/O errors. Therefore this value is only
+enforced by multipathd when a multipath map is first created, or when a path device
+is added to a map. In both cases, race conditions are avoided by the kernel.
 .RS
-.TP
-The default is: in \fB/sys/block/<dev>/queue/max_sectors_kb\fR
+.PP
+Setting \fImax_sectors_kb\fR does not guarantee that all path devices will have this
+value set. It is not an error if the value of a path device is higher than that of
+the containing multipath map. It is also not an error if the actual limit of a map is
+lower than the value in \fI@CONFIGFILE@\fR. This can happen if the hardware limits of one
+or more path devices are lower than the configured value.
+.PP
+Normally the kernel and its device drivers take care of the maximum
+I/O size, and administrators do not need to bother about \fImax_sectors_kb\fR.
+But some hardware devices may report incorrect I/O size limits, or other components
+in the environment (e.g. the fabric) may impose constraints that the kernel cannot
+detect. In such cases setting \fImax_sectors_kb\fR makes sense. It should be set when
+maps are first created, and not be changed thereafter.
+If the setting \fBmust\fR be changed for a live map, set the
+value in \fI@CONFIGFILE@\fR, run \fBmultipathd reconfigure\fR, and use
+\fBmultipathd del path <path>\fR and \fBmultipathd add path <path>\fR to
+delete and re-add the same path device.
+.LP
+The default is: \fBundefined\fR.
 .RE
 .
 .
-- 
2.44.0

Re: [PATCH 0/8] multipath-tools: max_sectors_kb rework

[Benjamin Marzinski]

On Wed, Apr 17, 2024 at 08:46:36PM +0200, Martin Wilck wrote:
> This patch series addresses the issue described in [1].

For the Set:
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>


> Changing max_sectors_kb on a live multipath map is dangerous. In particular,
> no path device may have a lower value than the map at any time, unless the
> device is suspended and all I/O has been flushed. It is also dangerous to
> read a max_sectors_kb value from sysfs and write it back, because this may
> actually cause the max_sectors value in the kernel (which is in 512 byte block
> units, not in kiB) to be rounded down to the next even number.
> 
> This patch set avoids both these problematic techniques. max_sectors_kb is
> only changed for maps that have the respective configuration parameter set,
> and only during map creation or during path addition. In the latter case,
> the limit is set on the new path before actually adding it to the map, thus
> avoiding a change on a live path. While it is generally recommended to set
> max_sectors_kb once and for all before creating a map, the latter technique
> can be used to change the value on a live map, by removing and re-adding
> a path device.
> 
> A side effect of this patch set is that not necessarily all path devices of
> a map that has max_sectors_kb set will have this limit configured. In
> particular, if max_sectors_kb is enforced by adding a path device, only the
> added path and the map itself will have the new max_sectors_kb value, because
> multipathd will not attempt to change the value of the other paths, which
> can therefore be higher than the configured limit. This is not a problem.
> Errors arise only if the limit of the map device is higher than that of
> any path device.
> 
> This patch set does not attempt to address issues between a multipath device
> and block layers stacked on to of it (e.g. LVM or MD raid), as discussed in
> the commit message of 8fd4868 ("libmultipath: don't set max_sectors_kb on
> reloads"). I think, and my experiments have confirmed it, that for bio-based
> stacked devices on top of multipath, max_sectors_kb is not a problem. bios
> with a large amount of I/O are split in the kernel to fit into the lower-level
> device's limits. Only request-based dm aka multipath is susceptible to errors
> caused by max_sectors_kb mismatch. If I am overlooking something here, please
> provide an example to prove me wrong.
> 
> https://lore.kernel.org/dm-devel/7742003e19b5a49398067dc0c59dfa8ddeffc3d7.camel@suse.com/

I reran the bug reproducer for original issue that inspired 8fd4868,
after manually increasing max_sectors_kb of the device on top of
multipathd, and it works fine on recent kernels.
 
> Martin Wilck (8):
>   multipath-tools: add TGTDIR option
>   libmultipath: move get_udev_for_mpp to sysfs.c
>   libmultipath: add mp_find_path_by_devt()
>   Revert "libmultipath: fix max_sectors_kb on adding path"
>   libmultipath: Only set max_sectors_kb on map creation
>   libmultipath: set max_sectors_kb in adopt_paths()
>   libmultipath: add wildcard %k for printing max_sectors_kb
>   multipath.conf(5): update documentation for max_sectors_kb
> 
>  .github/actions/spelling/expect.txt |  2 +
>  Makefile.inc                        |  8 ++--
>  README.md                           | 33 +++++++++++++++
>  libmultipath/configure.c            | 63 +++--------------------------
>  libmultipath/print.c                | 38 +++++++++++++++++
>  libmultipath/structs.c              | 19 +++++++++
>  libmultipath/structs.h              |  3 ++
>  libmultipath/structs_vec.c          | 62 +++++++++++++++++++++++++---
>  libmultipath/structs_vec.h          |  6 ++-
>  libmultipath/sysfs.c                | 19 +++++++++
>  libmultipath/sysfs.h                |  4 ++
>  multipath/multipath.conf.5.in       | 33 +++++++++++++--
>  multipathd/main.c                   |  6 +--
>  tests/Makefile                      |  2 +-
>  tests/test-lib.c                    |  9 ++++-
>  15 files changed, 229 insertions(+), 78 deletions(-)
> 
> -- 
> 2.44.0