[PATCH v2 00/20] path checker refactor and misc fixes

[PATCH v2 00/20] path checker refactor and misc fixes

[Benjamin Marzinski]

This patchset is based on discussions I had with Martin Wilck about my
last, partially applied patchset. It's based on top of his mwilck-tip
branch, 4ef98926 "fixup! Makefile: add targets for test-progs.cpio and
test-progs.tar". These patches are mostly orthogonal to the
libmp_mapinfo() work. I'll rebase them if further changes to that
patchset stop them form applying cleanly.

The first two patches are reposts of patches from my earlier patchset,
redone to work with the new libmp_mapinfo() API. They make it possible
to add maps by WWID with "multipathd add map".

The rest of the patches are work and bugfixes related to refactoring
checkerloop so that

1. multipath devices get resynced with the kernel occasionally, even if
they have not paths

2. If multiple paths from a multipath device are checked in the same
loop, the multipath device will only get resynced once.

3. All the paths of a multipath device will converge to being checked at
the same time (at least as much as their differing checkint values will
allow).

4. The times for checking the paths of each multipath device will spread
out as much as possible so multipathd doesn't do all of it's checking in
a burst.

5. path checking is done my multipath device (for initialized paths,
the uninitialized paths are handled after all the adopted paths are
handled).

Items 1 & 2 are handled by patch 10 and preceding patches.

Items 3 & 4 and handled by patch 17 and preceding patches.

Item 5 is handled by patch 18 and 19.

Changes in v2 (old patch number in quotes):
01: change from returning minor number to dm info (Martin)
02: adapt to change in patch 01
05 (05-07): squash into one commit (Martin)
08 (10): fix commit message (Matin)
10 (12): rename do_check_mpp amd check_mpp
11 (13): move the code that saved the path name to right before it was
         removed (Martin)
14 (16): use symbolic return codes (Martin)
15 (17): set path state to PATH_UNCHECKED when orphaned, and skip paths
         in INIT_REMOVED in sync_map_state() (Martin)
17 (19): clear pp->pending_ticks in check_path() if the path is delayed
         instead of pending. Use a different equation to check if we
         need to modify the ticks, that can adjust the ticks on every
         check. adapt to change in patch 14
18 (20): adapt to change in patch 14
19 (21): adapt to change in patch 10 and 14
20 (22): Add fixes trailer

Benjamin Marzinski (20):
  libmultipath: rename dm_map_present_by_wwid() and add outputs
  multipathd: make cli_add_map() handle adding maps by WWID correctly
  multipathd: remove checker restart optimization
  multipathd: refactor path state getting code into a helper
  multipathd: handle uninitialized paths in new function
  multipathd: check paths immediately after failing udev initialization
  multipathd: set pp->tick = max_checkint in handle_uninitialized_path
  multipathd: return 0 from check_path() if that path wasn't checked
  multipathd: reorder path state checks
  multipathd: adjust when mpp is synced with the kernel
  multipathd: resync map after setup_map in ev_remove_path
  multipathd: resync map after setup_map in resize_map
  multipathd: always resync map in reload_and_sync_map
  multipathd: correctly handle paths removed for a wwid change
  multipathd: handle changed wwid when adding partial path
  multipathd: don't read conf->checkint twice in check_path
  multipathd: make multipath devices manage their path check times
  multipathd: factor out actual path checking loop from checkerloop
  multipathd: check paths in order by mpp
  multipathd: clean up the correct wwid in check_path_wwid_change

 libmultipath/config.c             |  12 +
 libmultipath/config.h             |   1 +
 libmultipath/configure.c          |   1 +
 libmultipath/devmapper.c          |   5 +-
 libmultipath/devmapper.h          |   2 +-
 libmultipath/libmultipath.version |   5 +
 libmultipath/structs.c            |   2 +
 libmultipath/structs.h            |   3 +
 libmultipath/structs_vec.c        |   9 +-
 libmultipath/valid.c              |   2 +-
 multipathd/cli_handlers.c         |  66 ++--
 multipathd/main.c                 | 582 +++++++++++++++++++-----------
 multipathd/main.h                 |   2 +-
 tests/valid.c                     |  13 +-
 14 files changed, 432 insertions(+), 273 deletions(-)

-- 
2.45.0

[PATCH v2 01/20] libmultipath: rename dm_map_present_by_wwid() and add outputs

[Benjamin Marzinski]

add arguments to dm_map_present_by_wwid() to allow optionally fetching
the device name and dm info for the devices found by WWID.  These will
be used by a later patch. Since it can now also be used to get the name
and dm info of a device from its WWID, rename it to
dm_find_map_by_wwid()

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c |  5 +++--
 libmultipath/devmapper.h |  2 +-
 libmultipath/valid.c     |  2 +-
 tests/valid.c            | 13 +++++++------
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3d256d38..9af0a80e 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -874,7 +874,8 @@ int dm_is_mpath(const char *name)
 	}
 }
 
-int dm_map_present_by_wwid(const char *wwid)
+/* if name is non-NULL, it must point to an array of WWID_SIZE bytes */
+int dm_find_map_by_wwid(const char *wwid, char *name, struct dm_info *dmi)
 {
 	char tmp[DM_UUID_LEN];
 
@@ -883,7 +884,7 @@ int dm_map_present_by_wwid(const char *wwid)
 
 	return libmp_mapinfo(DM_MAP_BY_UUID,
 			     (mapid_t) { .str = tmp },
-			     (mapinfo_t) { .name = NULL });
+			     (mapinfo_t) { .name = name, .dmi = dmi });
 }
 
 static int dm_dev_t (const char *mapname, char *dev_t, int len)
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index f6d0017e..a26dc1e5 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -132,7 +132,7 @@ int dm_simplecmd_flush (int task, const char *name, uint16_t udev_flags);
 int dm_simplecmd_noflush (int task, const char *name, uint16_t udev_flags);
 int dm_addmap_create (struct multipath *mpp, char *params);
 int dm_addmap_reload (struct multipath *mpp, char *params, int flush);
-int dm_map_present_by_wwid(const char *uuid);
+int dm_find_map_by_wwid(const char *wwid, char *name, struct dm_info *dmi);
 
 enum {
 	DM_IS_MPATH_NO,
diff --git a/libmultipath/valid.c b/libmultipath/valid.c
index 9267cef9..b7e0cc9b 100644
--- a/libmultipath/valid.c
+++ b/libmultipath/valid.c
@@ -360,7 +360,7 @@ is_path_valid(const char *name, struct config *conf, struct path *pp,
 	if (check_wwids_file(pp->wwid, 0) == 0)
 		return PATH_IS_VALID_NO_CHECK;
 
-	if (dm_map_present_by_wwid(pp->wwid) == DMP_OK)
+	if (dm_find_map_by_wwid(pp->wwid, NULL, NULL) == DMP_OK)
 		return PATH_IS_VALID;
 
 	/* all these act like FIND_MULTIPATHS_STRICT for finding if a
diff --git a/tests/valid.c b/tests/valid.c
index a93bbe50..ecaeb467 100644
--- a/tests/valid.c
+++ b/tests/valid.c
@@ -189,10 +189,11 @@ int __wrap_check_wwids_file(char *wwid, int write_wwid)
 		return -1;
 }
 
-int __wrap_dm_map_present_by_wwid(const char *uuid)
+int __wrap_dm_find_map_by_wwid(const char *wwid, char *name,
+			       struct dm_info *dmi)
 {
 	int ret = mock_type(int);
-	assert_string_equal(uuid, mock_ptr_type(char *));
+	assert_string_equal(wwid, mock_ptr_type(char *));
 	return ret;
 }
 
@@ -271,8 +272,8 @@ static void setup_passing(char *name, char *wwid, unsigned int check_multipathd,
 	will_return(__wrap_check_wwids_file, wwid);
 	if (stage == STAGE_CHECK_WWIDS)
 		return;
-	will_return(__wrap_dm_map_present_by_wwid, 0);
-	will_return(__wrap_dm_map_present_by_wwid, wwid);
+	will_return(__wrap_dm_find_map_by_wwid, 0);
+	will_return(__wrap_dm_find_map_by_wwid, wwid);
 }
 
 static void test_bad_arguments(void **state)
@@ -516,8 +517,8 @@ static void test_check_uuid_present(void **state)
 	memset(&pp, 0, sizeof(pp));
 	conf.find_multipaths = FIND_MULTIPATHS_STRICT;
 	setup_passing(name, wwid, CHECK_MPATHD_RUNNING, STAGE_CHECK_WWIDS);
-	will_return(__wrap_dm_map_present_by_wwid, 1);
-	will_return(__wrap_dm_map_present_by_wwid, wwid);
+	will_return(__wrap_dm_find_map_by_wwid, 1);
+	will_return(__wrap_dm_find_map_by_wwid, wwid);
 	assert_int_equal(is_path_valid(name, &conf, &pp, true),
 			 PATH_IS_VALID);
 	assert_string_equal(pp.dev, name);
-- 
2.45.0

[PATCH v2 02/20] multipathd: make cli_add_map() handle adding maps by WWID correctly

[Benjamin Marzinski]

cli_add_map() runs filter_wwid() on the input param as if it were a
WWID, but dm_get_major_minor() will never find the multipath device if
the user actually passes in a WWID. To handle this case, call
get_refwwid() early in the function, and use dm_find_map_by_wwid() to
check if the map exists, and find its alias and minor number.

Also, the do/while loop is unnecessarily confusing and only avoids one
repeated function call. Remove it to simplify cli_add_map().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/libmultipath.version |  5 +++
 multipathd/cli_handlers.c         | 60 +++++++++++--------------------
 2 files changed, 25 insertions(+), 40 deletions(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 959f675e..d074032b 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -242,3 +242,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_25.1.0 {
+global:
+	dm_find_map_by_wwid;
+} LIBMULTIPATH_25.0.0;
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 0106213e..772a45d0 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -697,57 +697,37 @@ cli_add_map (void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, KEY_MAP);
-	int major = -1, minor = -1;
 	char dev_path[FILE_NAME_SIZE];
-	char *refwwid;
-	char *alias __attribute__((cleanup(cleanup_charp))) = NULL;
-	int rc, count = 0;
-	struct config *conf;
-	int invalid = 0;
+	char *refwwid __attribute__((cleanup(cleanup_charp))) = NULL;
+	char alias[WWID_SIZE];
+	int rc;
+	struct dm_info dmi;
 
 	param = convert_dev(param, 0);
 	condlog(2, "%s: add map (operator)", param);
 
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
-	if (filter_wwid(conf->blist_wwid, conf->elist_wwid, param, NULL) > 0)
-		invalid = 1;
-	pthread_cleanup_pop(1);
-	if (invalid) {
+	if (get_refwwid(CMD_NONE, param, DEV_DEVMAP, vecs->pathvec, &refwwid) ==
+	    PATHINFO_SKIPPED) {
 		append_strbuf_str(reply, "blacklisted\n");
 		condlog(2, "%s: map blacklisted", param);
 		return 1;
+	} else if (!refwwid) {
+		condlog(2, "%s: unknown map.", param);
+		return -ENODEV;
 	}
-	do {
-		if (dm_get_major_minor(param, &major, &minor) < 0)
-			condlog(count ? 2 : 3,
-				"%s: not a device mapper table", param);
-		else {
-			sprintf(dev_path, "dm-%d", minor);
-			alias = dm_mapname(major, minor);
+	if (dm_find_map_by_wwid(refwwid, alias, &dmi) != DMP_OK) {
+		condlog(3, "%s: map not present. creating", param);
+		if (coalesce_paths(vecs, NULL, refwwid, FORCE_RELOAD_NONE,
+				   CMD_NONE) != CP_OK) {
+			condlog(2, "%s: coalesce_paths failed", param);
+			return 1;
+		}
+		if (dm_find_map_by_wwid(refwwid, alias, &dmi) != DMP_OK) {
+			condlog(2, "%s: failed getting map", param);
+			return 1;
 		}
-		/*if there is no mapname found, we first create the device*/
-		if (!alias && !count) {
-			condlog(3, "%s: mapname not found for %d:%d",
-				param, major, minor);
-			get_refwwid(CMD_NONE, param, DEV_DEVMAP,
-				    vecs->pathvec, &refwwid);
-			if (refwwid) {
-				if (coalesce_paths(vecs, NULL, refwwid,
-						   FORCE_RELOAD_NONE, CMD_NONE)
-				    != CP_OK)
-					condlog(2, "%s: coalesce_paths failed",
-									param);
-				free(refwwid);
-			}
-		} /*we attempt to create device only once*/
-		count++;
-	} while (!alias && (count < 2));
-
-	if (!alias) {
-		condlog(2, "%s: add map failed", param);
-		return 1;
 	}
+	sprintf(dev_path, "dm-%u", dmi.minor);
 	rc = ev_add_map(dev_path, alias, vecs);
 	return rc;
 }
-- 
2.45.0

[PATCH v2 03/20] multipathd: remove checker restart optimization

[Benjamin Marzinski]

Future commits will make this optimization unusable.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 13ed6d05..eaef505a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2704,7 +2704,7 @@ checkerloop (void *ap)
 
 	while (1) {
 		struct timespec diff_time, start_time, end_time;
-		int num_paths = 0, strict_timing, rc = 0, i = 0;
+		int num_paths = 0, strict_timing, rc = 0;
 		unsigned int ticks = 0;
 		enum checker_state checker_state = CHECKER_STARTING;
 
@@ -2723,7 +2723,7 @@ checkerloop (void *ap)
 			sd_notify(0, "WATCHDOG=1");
 #endif
 		while (checker_state != CHECKER_FINISHED) {
-			unsigned int paths_checked = 0;
+			unsigned int paths_checked = 0, i;
 			struct timespec chk_start_time;
 
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
@@ -2733,28 +2733,11 @@ checkerloop (void *ap)
 			if (checker_state == CHECKER_STARTING) {
 				vector_foreach_slot(vecs->pathvec, pp, i)
 					pp->is_checked = false;
-				i = 0;
 				checker_state = CHECKER_RUNNING;
-			} else {
-				/*
-				 * Paths could have been removed since we
-				 * dropped the lock. Find the path to continue
-				 * checking at. Since paths can be removed from
-				 * anywhere in the vector, but can only be added
-				 * at the end, the last checked path must be
-				 * between its old location, and the start or
-				 * the vector.
-				 */
-				if (i >= VECTOR_SIZE(vecs->pathvec))
-					i = VECTOR_SIZE(vecs->pathvec) - 1;
-				while ((pp = VECTOR_SLOT(vecs->pathvec, i))) {
-					if (pp->is_checked == true)
-						break;
-					i--;
-				}
-				i++;
 			}
-			vector_foreach_slot_after (vecs->pathvec, pp, i) {
+			vector_foreach_slot(vecs->pathvec, pp, i) {
+				if (pp->is_checked)
+					continue;
 				pp->is_checked = true;
 				rc = check_path(vecs, pp, ticks);
 				if (rc < 0) {
-- 
2.45.0

Re: [PATCH v2 03/20] multipathd: remove checker restart optimization

[Martin Wilck]

On Wed, 2024-07-17 at 14:10 -0400, Benjamin Marzinski wrote:
> Future commits will make this optimization unusable.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 27 +++++----------------------
>  1 file changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 13ed6d05..eaef505a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2704,7 +2704,7 @@ checkerloop (void *ap)
>  
>  	while (1) {
>  		struct timespec diff_time, start_time, end_time;
> -		int num_paths = 0, strict_timing, rc = 0, i = 0;
> +		int num_paths = 0, strict_timing, rc = 0;
>  		unsigned int ticks = 0;
>  		enum checker_state checker_state = CHECKER_STARTING;
>  
> @@ -2723,7 +2723,7 @@ checkerloop (void *ap)
>  			sd_notify(0, "WATCHDOG=1");
>  #endif
>  		while (checker_state != CHECKER_FINISHED) {
> -			unsigned int paths_checked = 0;
> +			unsigned int paths_checked = 0, i;

Nit: To avoid a coverity warning for the "i--" statement below, you
need to use a signed int for "i".

I can change this later on.

Martin

>  			struct timespec chk_start_time;
>  
>  			pthread_cleanup_push(cleanup_lock, &vecs-
> >lock);
> @@ -2733,28 +2733,11 @@ checkerloop (void *ap)
>  			if (checker_state == CHECKER_STARTING) {
>  				vector_foreach_slot(vecs->pathvec,
> pp, i)
>  					pp->is_checked = false;
> -				i = 0;
>  				checker_state = CHECKER_RUNNING;
> -			} else {
> -				/*
> -				 * Paths could have been removed
> since we
> -				 * dropped the lock. Find the path
> to continue
> -				 * checking at. Since paths can be
> removed from
> -				 * anywhere in the vector, but can
> only be added
> -				 * at the end, the last checked path
> must be
> -				 * between its old location, and the
> start or
> -				 * the vector.
> -				 */
> -				if (i >= VECTOR_SIZE(vecs->pathvec))
> -					i = VECTOR_SIZE(vecs-
> >pathvec) - 1;
> -				while ((pp = VECTOR_SLOT(vecs-
> >pathvec, i))) {
> -					if (pp->is_checked == true)
> -						break;
> -					i--;
> -				}
> -				i++;
>  			}
> -			vector_foreach_slot_after (vecs->pathvec,
> pp, i) {
> +			vector_foreach_slot(vecs->pathvec, pp, i) {
> +				if (pp->is_checked)
> +					continue;
>  				pp->is_checked = true;
>  				rc = check_path(vecs, pp, ticks);
>  				if (rc < 0) {

[PATCH v2 04/20] multipathd: refactor path state getting code into a helper

[Benjamin Marzinski]

Pull the code that gets the new path state out into a helper function
named check_path_state(), in preparation for splittig check_path()
into two functions.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 64 ++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index eaef505a..37e6bd18 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2305,6 +2305,43 @@ should_skip_path(struct path *pp){
 	return 0;
 }
 
+static int
+check_path_state(struct path *pp)
+{
+	int newstate;
+	struct config *conf;
+
+	newstate = path_offline(pp);
+	if (newstate == PATH_UP) {
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
+		newstate = get_state(pp, conf, 1, newstate);
+		pthread_cleanup_pop(1);
+	} else {
+		checker_clear_message(&pp->checker);
+		condlog(3, "%s: state %s, checker not called",
+			pp->dev, checker_state_name(newstate));
+	}
+	/*
+	 * Wait for uevent for removed paths;
+	 * some LLDDs like zfcp keep paths unavailable
+	 * without sending uevents.
+	 */
+	if (newstate == PATH_REMOVED)
+		newstate = PATH_DOWN;
+
+	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
+		condlog(2, "%s: unusable path (%s) - checker failed",
+			pp->dev, checker_state_name(newstate));
+		LOG_MSG(2, pp);
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
+		pathinfo(pp, conf, 0);
+		pthread_cleanup_pop(1);
+	}
+	return newstate;
+}
+
 /*
  * Returns '1' if the path has been checked, '-1' if it was blacklisted
  * and '0' otherwise
@@ -2384,33 +2421,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	 */
 	pp->tick = checkint;
 
-	newstate = path_offline(pp);
-	if (newstate == PATH_UP) {
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
-		newstate = get_state(pp, conf, 1, newstate);
-		pthread_cleanup_pop(1);
-	} else {
-		checker_clear_message(&pp->checker);
-		condlog(3, "%s: state %s, checker not called",
-			pp->dev, checker_state_name(newstate));
-	}
-	/*
-	 * Wait for uevent for removed paths;
-	 * some LLDDs like zfcp keep paths unavailable
-	 * without sending uevents.
-	 */
-	if (newstate == PATH_REMOVED)
-		newstate = PATH_DOWN;
-
+	newstate = check_path_state(pp);
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
-		condlog(2, "%s: unusable path (%s) - checker failed",
-			pp->dev, checker_state_name(newstate));
-		LOG_MSG(2, pp);
-		conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
-		pathinfo(pp, conf, 0);
-		pthread_cleanup_pop(1);
 		return 1;
 	} else if ((newstate != PATH_UP && newstate != PATH_GHOST &&
 		    newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
-- 
2.45.0

[PATCH v2 05/20] multipathd: handle uninitialized paths in new function

[Benjamin Marzinski]

Pull the code to handle uninitialized paths out of check_path() and into
a new function called handle_uninitialized_paths(). This should cause
no functional changes. Also make check_path() static.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 168 ++++++++++++++++++++++++++++------------------
 1 file changed, 101 insertions(+), 67 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 37e6bd18..2331f18e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2343,10 +2343,9 @@ check_path_state(struct path *pp)
 }
 
 /*
- * Returns '1' if the path has been checked, '-1' if it was blacklisted
- * and '0' otherwise
+ * Returns '1' if the path has been checked and '0' otherwise
  */
-int
+static int
 check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 {
 	int newstate;
@@ -2354,16 +2353,13 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int chkr_new_path_up = 0;
 	int disable_reinstate = 0;
 	int oldchkrstate = pp->chkrstate;
-	int retrigger_tries;
 	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
 	int ret;
 	bool need_reload;
 
-	if (((pp->initialized == INIT_OK || pp->initialized == INIT_PARTIAL ||
-	      pp->initialized == INIT_REQUESTED_UDEV) && !pp->mpp) ||
-	    pp->initialized == INIT_REMOVED)
+	if (pp->initialized == INIT_REMOVED)
 		return 0;
 
 	if (pp->tick)
@@ -2372,7 +2368,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		return 0; /* don't check this path yet */
 
 	conf = get_multipath_config();
-	retrigger_tries = conf->retrigger_tries;
 	checkint = conf->checkint;
 	max_checkint = conf->max_checkint;
 	marginal_pathgroups = conf->marginal_pathgroups;
@@ -2383,38 +2378,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		pp->checkint = checkint;
 	};
 
-	if (!pp->mpp && pp->initialized == INIT_MISSING_UDEV) {
-		if (pp->retriggers < retrigger_tries) {
-			static const char change[] = "change";
-			ssize_t ret;
-
-			condlog(2, "%s: triggering change event to reinitialize",
-				pp->dev);
-			pp->initialized = INIT_REQUESTED_UDEV;
-			pp->retriggers++;
-			ret = sysfs_attr_set_value(pp->udev, "uevent", change,
-						   sizeof(change) - 1);
-			if (ret != sizeof(change) - 1)
-				log_sysfs_attr_set_value(1, ret,
-							 "%s: failed to trigger change event",
-							 pp->dev);
-			return 0;
-		} else {
-			condlog(1, "%s: not initialized after %d udev retriggers",
-				pp->dev, retrigger_tries);
-			/*
-			 * Make sure that the "add missing path" code path
-			 * below may reinstate the path later, if it ever
-			 * comes up again.
-			 * The WWID needs not be cleared; if it was set, the
-			 * state hadn't been INIT_MISSING_UDEV in the first
-			 * place.
-			 */
-			pp->initialized = INIT_FAILED;
-			return 0;
-		}
-	}
-
 	/*
 	 * provision a next check soonest,
 	 * in case we exit abnormally from here
@@ -2435,32 +2398,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		pp->checkint = checkint;
 		return 1;
 	}
-	if (!pp->mpp) {
-		if (!strlen(pp->wwid) &&
-		    (pp->initialized == INIT_FAILED ||
-		     pp->initialized == INIT_NEW) &&
-		    (newstate == PATH_UP || newstate == PATH_GHOST)) {
-			condlog(2, "%s: add missing path", pp->dev);
-			conf = get_multipath_config();
-			pthread_cleanup_push(put_multipath_config, conf);
-			ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
-			pthread_cleanup_pop(1);
-			/* INIT_OK implies ret == PATHINFO_OK */
-			if (pp->initialized == INIT_OK) {
-				ev_add_path(pp, vecs, 1);
-				pp->tick = 1;
-			} else {
-				if (ret == PATHINFO_SKIPPED)
-					return -1;
-				/*
-				 * We failed multiple times to initialize this
-				 * path properly. Don't re-check too often.
-				 */
-				pp->checkint = max_checkint;
-			}
-		}
-		return 0;
-	}
 	/*
 	 * Async IO in flight. Keep the previous path state
 	 * and reschedule as soon as possible
@@ -2679,6 +2616,99 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	return 1;
 }
 
+/*
+ * Returns -1 if the path was blacklisted, and 0 otherwise
+ */
+static int
+handle_uninitialized_path(struct vectors * vecs, struct path * pp,
+			  unsigned int ticks)
+{
+	int newstate;
+	int retrigger_tries;
+	unsigned int checkint, max_checkint;
+	struct config *conf;
+	int ret;
+
+	if (pp->initialized != INIT_NEW && pp->initialized != INIT_FAILED &&
+	    pp->initialized != INIT_MISSING_UDEV)
+		return 0;
+
+	if (pp->tick)
+		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
+	if (pp->tick)
+		return 0; /* don't check this path yet */
+
+	conf = get_multipath_config();
+	retrigger_tries = conf->retrigger_tries;
+	checkint = conf->checkint;
+	max_checkint = conf->max_checkint;
+	put_multipath_config(conf);
+
+	if (pp->initialized == INIT_MISSING_UDEV) {
+		if (pp->retriggers < retrigger_tries) {
+			static const char change[] = "change";
+			ssize_t ret;
+
+			condlog(2, "%s: triggering change event to reinitialize",
+				pp->dev);
+			pp->initialized = INIT_REQUESTED_UDEV;
+			pp->retriggers++;
+			ret = sysfs_attr_set_value(pp->udev, "uevent", change,
+						   sizeof(change) - 1);
+			if (ret != sizeof(change) - 1)
+				log_sysfs_attr_set_value(1, ret,
+							 "%s: failed to trigger change event",
+							 pp->dev);
+			return 0;
+		} else {
+			condlog(1, "%s: not initialized after %d udev retriggers",
+				pp->dev, retrigger_tries);
+			/*
+			 * Make sure that the "add missing path" code path
+			 * below may reinstate the path later, if it ever
+			 * comes up again.
+			 * The WWID needs not be cleared; if it was set, the
+			 * state hadn't been INIT_MISSING_UDEV in the first
+			 * place.
+			 */
+			pp->initialized = INIT_FAILED;
+			return 0;
+		}
+	}
+
+	/*
+	 * provision a next check soonest,
+	 * in case we exit abnormally from here
+	 */
+	pp->tick = checkint;
+
+	newstate = check_path_state(pp);
+
+	if (!strlen(pp->wwid) &&
+	    (pp->initialized == INIT_FAILED || pp->initialized == INIT_NEW) &&
+	    (newstate == PATH_UP || newstate == PATH_GHOST)) {
+		condlog(2, "%s: add missing path", pp->dev);
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
+		ret = pathinfo(pp, conf, DI_ALL | DI_BLACKLIST);
+		pthread_cleanup_pop(1);
+		/* INIT_OK implies ret == PATHINFO_OK */
+		if (pp->initialized == INIT_OK) {
+			ev_add_path(pp, vecs, 1);
+			pp->tick = 1;
+		} else {
+			if (ret == PATHINFO_SKIPPED)
+				return -1;
+			/*
+			 * We failed multiple times to initialize this
+			 * path properly. Don't re-check too often.
+			 */
+			pp->checkint = max_checkint;
+		}
+	}
+	return 0;
+}
+
 enum checker_state {
 	CHECKER_STARTING,
 	CHECKER_RUNNING,
@@ -2751,7 +2781,11 @@ checkerloop (void *ap)
 				if (pp->is_checked)
 					continue;
 				pp->is_checked = true;
-				rc = check_path(vecs, pp, ticks);
+				if (pp->mpp)
+					rc = check_path(vecs, pp, ticks);
+				else
+					rc = handle_uninitialized_path(vecs, pp,
+								       ticks);
 				if (rc < 0) {
 					condlog(1, "%s: check_path() failed, removing",
 						pp->dev);
-- 
2.45.0

[PATCH v2 06/20] multipathd: check paths immediately after failing udev initialization

[Benjamin Marzinski]

When handle_uninitialized_path() moves a path from INIT_MISSING_UDEV to
INIT_FAILED, it has already waited for conf->retrigger_delay seconds. It
might as well check the path state now, instead of waiting for the next
iteration.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2331f18e..f9451302 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2672,7 +2672,6 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 			 * place.
 			 */
 			pp->initialized = INIT_FAILED;
-			return 0;
 		}
 	}
 
-- 
2.45.0

[PATCH v2 07/20] multipathd: set pp->tick = max_checkint in handle_uninitialized_path

[Benjamin Marzinski]

When handle_uninitialized_path() checks a path, it will either:
1. trigger a new uevent to initalize the path, in which case it won't
   check the path again until that uevent is processed and updates
   pp->tick
2. blacklist the path, in which case the path gets removed
3. intialize the path correctly, in which case it sets pp->tick = 1
4. fail to initialize the path, in which case it was supposed to set
   pp->tick to max_checkint, but instead it set pp->checkint to
   max_checkint, which never worked correctly.

By setting pp->tick to max_checkint at the start,
handle_uninitialized_path() will continue to work as it previously did,
except in case 4, where in now works correctly. There's no point in
messing with pp->checkint for paths that haven't been initialized yet.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f9451302..7c4d3bf6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2625,7 +2625,6 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 {
 	int newstate;
 	int retrigger_tries;
-	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int ret;
 
@@ -2640,8 +2639,7 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 
 	conf = get_multipath_config();
 	retrigger_tries = conf->retrigger_tries;
-	checkint = conf->checkint;
-	max_checkint = conf->max_checkint;
+	pp->tick = conf->max_checkint;
 	put_multipath_config(conf);
 
 	if (pp->initialized == INIT_MISSING_UDEV) {
@@ -2675,12 +2673,6 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 		}
 	}
 
-	/*
-	 * provision a next check soonest,
-	 * in case we exit abnormally from here
-	 */
-	pp->tick = checkint;
-
 	newstate = check_path_state(pp);
 
 	if (!strlen(pp->wwid) &&
@@ -2695,14 +2687,8 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 		if (pp->initialized == INIT_OK) {
 			ev_add_path(pp, vecs, 1);
 			pp->tick = 1;
-		} else {
-			if (ret == PATHINFO_SKIPPED)
-				return -1;
-			/*
-			 * We failed multiple times to initialize this
-			 * path properly. Don't re-check too often.
-			 */
-			pp->checkint = max_checkint;
+		} else if (ret == PATHINFO_SKIPPED) {
+			return -1;
 		}
 	}
 	return 0;
-- 
2.45.0

[PATCH v2 08/20] multipathd: return 0 from check_path() if that path wasn't checked

[Benjamin Marzinski]

If check_path_state() returns PATH_WILD or PATH_UNCHECKED, then the
path wasn't correctly checked, and check_path() should return 0.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 7c4d3bf6..733952cf 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2386,7 +2386,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 
 	newstate = check_path_state(pp);
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
-		return 1;
+		return 0;
 	} else if ((newstate != PATH_UP && newstate != PATH_GHOST &&
 		    newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
 		/* If path state become failed again cancel path delay state */
-- 
2.45.0

[PATCH v2 09/20] multipathd: reorder path state checks

[Benjamin Marzinski]

Reorder the path state checks in check_path(), so that it first does
all the checks which can result in returning with the path unchecked or
removed from the multipath device.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 733952cf..08b424d6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2385,19 +2385,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	pp->tick = checkint;
 
 	newstate = check_path_state(pp);
-	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
+	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED)
 		return 0;
-	} else if ((newstate != PATH_UP && newstate != PATH_GHOST &&
-		    newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
-		/* If path state become failed again cancel path delay state */
-		pp->state = newstate;
-		/*
-		 * path state bad again should change the check interval time
-		 * to the shortest delay
-		 */
-		pp->checkint = checkint;
-		return 1;
-	}
 	/*
 	 * Async IO in flight. Keep the previous path state
 	 * and reschedule as soon as possible
@@ -2436,6 +2425,17 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		return 0;
 	}
 
+	if ((newstate != PATH_UP && newstate != PATH_GHOST &&
+	     newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
+		/* If path state become failed again cancel path delay state */
+		pp->state = newstate;
+		/*
+		 * path state bad again should change the check interval time
+		 * to the shortest delay
+		 */
+		pp->checkint = checkint;
+		return 1;
+	}
 	if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
 	    (san_path_check_enabled(pp->mpp) ||
 	     marginal_path_check_enabled(pp->mpp))) {
-- 
2.45.0

[PATCH v2 10/20] multipathd: adjust when mpp is synced with the kernel

[Benjamin Marzinski]

Move the code to sync the mpp device state into a helper function and
add a counter to to make sure that the device is synced at least once
every max_checkint secs. This makes sure that multipath devices with no
paths will still get synced with the kernel.  Also, if multiple paths
are checked in the same loop, the multipath device will only be synced
with the kernel once, since every time the mpp is synced in any code
path, mpp->sync_tick is reset.

The code still syncs the mpp before updating the path state for two
main reasons.

1. Sometimes multipathd leaves the mpp with a garbage state. Future
   patches will fix most of these cases, but the code intentially
   does not remove the mpp is resyncing fails while checking paths.
   But this does leave the mpp with a garbage state.

2. The kernel chages the multipath state independently of multipathd. If
   the kernel fails a path, a uevent will arrive shortly. But the kernel
   doesn't provide any notification when it switches the active
   path group or if it ends up picking a different one than multipathd
   selected. Multipathd needs to know the actual current pathgroup to
   know when it should be switching them.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c   |  1 +
 libmultipath/structs.h     |  2 ++
 libmultipath/structs_vec.c |  5 +++
 multipathd/main.c          | 64 +++++++++++++++++++++++++-------------
 4 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index b4de863c..34158e31 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -358,6 +358,7 @@ int setup_map(struct multipath *mpp, char **params, struct vectors *vecs)
 
 	sysfs_set_scsi_tmo(conf, mpp);
 	marginal_pathgroups = conf->marginal_pathgroups;
+	mpp->sync_tick = conf->max_checkint;
 	pthread_cleanup_pop(1);
 
 	if (!mpp->features || !mpp->hwhandler || !mpp->selector) {
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 3b91e39c..002eeae1 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -453,6 +453,8 @@ struct multipath {
 	int ghost_delay;
 	int ghost_delay_tick;
 	int queue_mode;
+	unsigned int sync_tick;
+	bool is_checked;
 	uid_t uid;
 	gid_t gid;
 	mode_t mode;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 731b1bce..60360c76 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -505,10 +505,15 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 	char __attribute__((cleanup(cleanup_charp))) *params = NULL;
 	char __attribute__((cleanup(cleanup_charp))) *status = NULL;
 	unsigned long long size = mpp->size;
+	struct config *conf;
 
 	if (!mpp)
 		return r;
 
+	conf = get_multipath_config();
+	mpp->sync_tick = conf->max_checkint;
+	put_multipath_config(conf);
+
 	r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
 			  (mapid_t) { .str = mpp->alias },
 			  (mapinfo_t) {
diff --git a/multipathd/main.c b/multipathd/main.c
index 08b424d6..d7c87039 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2342,6 +2342,37 @@ check_path_state(struct path *pp)
 	return newstate;
 }
 
+static void
+do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
+{
+	int i, ret;
+	struct path *pp;
+
+	mpp->is_checked = true;
+	ret = update_multipath_strings(mpp, vecs->pathvec);
+	if (ret != DMP_OK) {
+		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
+			"device not found" :
+			"couldn't synchronize with kernel state");
+		vector_foreach_slot (mpp->paths, pp, i)
+			pp->dmstate = PSTATE_UNDEF;
+		return;
+	}
+	set_no_path_retry(mpp);
+}
+
+static void
+sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks)
+{
+	if (mpp->sync_tick)
+		mpp->sync_tick -= (mpp->sync_tick > ticks) ? ticks :
+				  mpp->sync_tick;
+	if (mpp->sync_tick)
+		return;
+
+	do_sync_mpp(vecs, mpp);
+}
+
 /*
  * Returns '1' if the path has been checked and '0' otherwise
  */
@@ -2356,7 +2387,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
-	int ret;
 	bool need_reload;
 
 	if (pp->initialized == INIT_REMOVED)
@@ -2395,26 +2425,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		pp->tick = 1;
 		return 0;
 	}
-	/*
-	 * Synchronize with kernel state
-	 */
-	ret = update_multipath_strings(pp->mpp, vecs->pathvec);
-	if (ret != DMP_OK) {
-		if (ret == DMP_NOT_FOUND) {
-			/* multipath device missing. Likely removed */
-			condlog(1, "%s: multipath device '%s' not found",
-				pp->dev, pp->mpp ? pp->mpp->alias : "");
-			return 0;
-		} else
-			condlog(1, "%s: Couldn't synchronize with kernel state",
-				pp->dev);
-		pp->dmstate = PSTATE_UNDEF;
-	}
-	/* if update_multipath_strings orphaned the path, quit early */
-	if (!pp->mpp)
-		return 0;
-	set_no_path_retry(pp->mpp);
-
 	if (pp->recheck_wwid == RECHECK_WWID_ON &&
 	    (newstate == PATH_UP || newstate == PATH_GHOST) &&
 	    ((pp->state != PATH_UP && pp->state != PATH_GHOST) ||
@@ -2424,7 +2434,12 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		handle_path_wwid_change(pp, vecs);
 		return 0;
 	}
-
+	if (!pp->mpp->is_checked) {
+		do_sync_mpp(vecs, pp->mpp);
+		/* if update_multipath_strings orphaned the path, quit early */
+		if (!pp->mpp)
+			return 0;
+	}
 	if ((newstate != PATH_UP && newstate != PATH_GHOST &&
 	     newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
 		/* If path state become failed again cancel path delay state */
@@ -2752,12 +2767,17 @@ checkerloop (void *ap)
 		while (checker_state != CHECKER_FINISHED) {
 			unsigned int paths_checked = 0, i;
 			struct timespec chk_start_time;
+			struct multipath *mpp;
 
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
+			vector_foreach_slot(vecs->mpvec, mpp, i)
+				mpp->is_checked = false;
 			get_monotonic_time(&chk_start_time);
 			if (checker_state == CHECKER_STARTING) {
+				vector_foreach_slot(vecs->mpvec, mpp, i)
+					sync_mpp(vecs, mpp, ticks);
 				vector_foreach_slot(vecs->pathvec, pp, i)
 					pp->is_checked = false;
 				checker_state = CHECKER_RUNNING;
-- 
2.45.0

[PATCH v2 11/20] multipathd: resync map after setup_map in ev_remove_path

[Benjamin Marzinski]

In ev_remove_path() it was possible to exit after calling setup_map()
without resyncing the mpp state with the kernel. This meant that the
mpp state in multipathd might not match with the kernel state at all.

It's safe to exit before calling setup_map() if either wait_for_udev
or need_do_map is set. In both cases, setup_map() will later be called,
either by a uevent or by the calling function.

Once setup_map() has been called, setup_multipath() and sync_map_state()
are now always called, to make sure the mpp matches the kernel state.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d7c87039..aa3c7eb6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1395,6 +1395,8 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 	 * avoid referring to the map of an orphaned path
 	 */
 	if ((mpp = pp->mpp)) {
+		char devt[BLK_DEV_SIZE];
+
 		/*
 		 * Mark the path as removed. In case of success, we
 		 * will delete it for good. Otherwise, it will be deleted
@@ -1428,12 +1430,6 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 		    flush_map_nopaths(mpp, vecs))
 			goto out;
 
-		if (setup_map(mpp, &params, vecs)) {
-			condlog(0, "%s: failed to setup map for"
-				" removal of path %s", mpp->alias, pp->dev);
-			goto fail;
-		}
-
 		if (mpp->wait_for_udev) {
 			mpp->wait_for_udev = 2;
 			retval = REMOVE_PATH_DELAY;
@@ -1444,6 +1440,12 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 			retval = REMOVE_PATH_DELAY;
 			goto out;
 		}
+
+		if (setup_map(mpp, &params, vecs)) {
+			condlog(0, "%s: failed to setup map for"
+				" removal of path %s", mpp->alias, pp->dev);
+			goto fail;
+		}
 		/*
 		 * reload the map
 		 */
@@ -1453,24 +1455,20 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map)
 				"removal of path %s",
 				mpp->alias, pp->dev);
 			retval = REMOVE_PATH_FAILURE;
-		} else {
-			/*
-			 * update our state from kernel
-			 */
-			char devt[BLK_DEV_SIZE];
-
-			strlcpy(devt, pp->dev_t, sizeof(devt));
-
-			/* setup_multipath will free the path
-			 * regardless of whether it succeeds or
-			 * fails */
-			if (setup_multipath(vecs, mpp))
-				return REMOVE_PATH_MAP_ERROR;
-			sync_map_state(mpp);
+		}
+		/*
+		 * update mpp state from kernel even if domap failed.
+		 * If the path was removed from the mpp, setup_multipath will
+		 * free the path regardless of whether it succeeds or fails
+		 */
+		strlcpy(devt, pp->dev_t, sizeof(devt));
+		if (setup_multipath(vecs, mpp))
+			return REMOVE_PATH_MAP_ERROR;
+		sync_map_state(mpp);
 
+		if (retval == REMOVE_PATH_SUCCESS)
 			condlog(2, "%s: path removed from map %s",
 				devt, mpp->alias);
-		}
 	} else {
 		/* mpp == NULL */
 		if ((i = find_slot(vecs->pathvec, (void *)pp)) != -1)
-- 
2.45.0

Re: [PATCH v2 11/20] multipathd: resync map after setup_map in ev_remove_path

[Martin Wilck]

On Wed, 2024-07-17 at 14:10 -0400, Benjamin Marzinski wrote:
> In ev_remove_path() it was possible to exit after calling setup_map()
> without resyncing the mpp state with the kernel. This meant that the
> mpp state in multipathd might not match with the kernel state at all.
> 
> It's safe to exit before calling setup_map() if either wait_for_udev
> or need_do_map is set. 

Nit: I think you mean "need_do_map is not set" here. At least that's
what you've coded.

Just for clarification.

Martin

[PATCH v2 12/20] multipathd: resync map after setup_map in resize_map

[Benjamin Marzinski]

In resize_map() it was possible to exit after calling setup_map()
without resyncing the mpp state with the kernel. This meant that the mpp
state in multipathd might not match with the kernel state at all.

Once setup_map() has been called, setup_multipath() and sync_map_state()
are now always called, to make sure the mpp matches the kernel state.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index aa3c7eb6..6d1a5382 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1534,6 +1534,7 @@ needs_ro_update(struct multipath *mpp, int ro)
 int resize_map(struct multipath *mpp, unsigned long long size,
 	       struct vectors * vecs)
 {
+	int ret = 0;
 	char *params __attribute__((cleanup(cleanup_charp))) = NULL;
 	unsigned long long orig_size = mpp->size;
 
@@ -1543,7 +1544,8 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 		condlog(0, "%s: failed to setup map for resize : %s",
 			mpp->alias, strerror(errno));
 		mpp->size = orig_size;
-		return 1;
+		ret = 1;
+		goto out;
 	}
 	mpp->action = ACT_RESIZE;
 	mpp->force_udev_reload = 1;
@@ -1551,13 +1553,14 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 		condlog(0, "%s: failed to resize map : %s", mpp->alias,
 			strerror(errno));
 		mpp->size = orig_size;
-		return 1;
+		ret = 1;
 	}
+out:
 	if (setup_multipath(vecs, mpp) != 0)
 		return 2;
 	sync_map_state(mpp);
 
-	return 0;
+	return ret;
 }
 
 static int
-- 
2.45.0

[PATCH v2 13/20] multipathd: always resync map in reload_and_sync_map

[Benjamin Marzinski]

reload_and_sync_map() needs to always resync the map after calling
reload_map(), because the mpp state may no longer match the kernel
state if reload_map() fails.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 6d1a5382..0d2a6780 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2188,13 +2188,15 @@ static int reload_map(struct vectors *vecs, struct multipath *mpp,
 
 int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs)
 {
+	int ret = 0;
+
 	if (reload_map(vecs, mpp, 1))
-		return 1;
+		ret = 1;
 	if (setup_multipath(vecs, mpp) != 0)
 		return 2;
 	sync_map_state(mpp);
 
-	return 0;
+	return ret;
 }
 
 static int check_path_reinstate_state(struct path * pp) {
-- 
2.45.0

[PATCH v2 14/20] multipathd: correctly handle paths removed for a wwid change

[Benjamin Marzinski]

If check_path() exitted because the path's wwid changed and it was
removed, checkerloop() wasn't decrementing the pathvec loop count.
This caused the next path to be skipped by the checker loop.

To solve this, switch check_path() and handle_uninitialized_path() to
symbolic returns and make check_path() return CHECK_PATH_REMOVED when a
path is removed, make handle_uninitialized_path() also remove the path
if it was blacklisted, and make checkerloop() just decrement the loop
count when a path handling function returns CHECK_PATH_REMOVED.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 73 ++++++++++++++++++++++++++---------------------
 multipathd/main.h |  2 +-
 2 files changed, 41 insertions(+), 34 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 0d2a6780..92a0e424 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -974,20 +974,24 @@ rescan_path(struct udev_device *ud)
 	}
 }
 
-void
+/* Returns true if the path was removed */
+bool
 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];
+	bool removed = false;
 
 	if (!pp || !pp->udev)
-		return;
+		return removed;
 
 	strlcpy(dev, pp->dev, sizeof(dev));
 	udd = udev_device_ref(pp->udev);
-	if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) && pp->mpp) {
+	if (ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) {
+		removed = true;
+	} else if (pp->mpp) {
 		pp->dmstate = PSTATE_FAILED;
 		dm_fail_path(pp->mpp->alias, pp->dev_t);
 	}
@@ -997,6 +1001,7 @@ handle_path_wwid_change(struct path *pp, struct vectors *vecs)
 	if (ret != sizeof(add) - 1)
 		log_sysfs_attr_set_value(1, ret,
 					 "%s: failed to trigger add event", dev);
+	return removed;
 }
 
 bool
@@ -2376,9 +2381,12 @@ sync_mpp(struct vectors * vecs, struct multipath *mpp, unsigned int ticks)
 	do_sync_mpp(vecs, mpp);
 }
 
-/*
- * Returns '1' if the path has been checked and '0' otherwise
- */
+enum check_path_return {
+	CHECK_PATH_CHECKED,
+	CHECK_PATH_SKIPPED,
+	CHECK_PATH_REMOVED,
+};
+
 static int
 check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 {
@@ -2393,12 +2401,12 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	bool need_reload;
 
 	if (pp->initialized == INIT_REMOVED)
-		return 0;
+		return CHECK_PATH_SKIPPED;
 
 	if (pp->tick)
 		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
 	if (pp->tick)
-		return 0; /* don't check this path yet */
+		return CHECK_PATH_SKIPPED;
 
 	conf = get_multipath_config();
 	checkint = conf->checkint;
@@ -2419,14 +2427,14 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 
 	newstate = check_path_state(pp);
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED)
-		return 0;
+		return CHECK_PATH_SKIPPED;
 	/*
 	 * Async IO in flight. Keep the previous path state
 	 * and reschedule as soon as possible
 	 */
 	if (newstate == PATH_PENDING) {
 		pp->tick = 1;
-		return 0;
+		return CHECK_PATH_SKIPPED;
 	}
 	if (pp->recheck_wwid == RECHECK_WWID_ON &&
 	    (newstate == PATH_UP || newstate == PATH_GHOST) &&
@@ -2434,14 +2442,14 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	     pp->dmstate == PSTATE_FAILED) &&
 	    check_path_wwid_change(pp)) {
 		condlog(0, "%s: path wwid change detected. Removing", pp->dev);
-		handle_path_wwid_change(pp, vecs);
-		return 0;
+		return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED :
+							  CHECK_PATH_SKIPPED;
 	}
 	if (!pp->mpp->is_checked) {
 		do_sync_mpp(vecs, pp->mpp);
 		/* if update_multipath_strings orphaned the path, quit early */
 		if (!pp->mpp)
-			return 0;
+			return CHECK_PATH_SKIPPED;
 	}
 	if ((newstate != PATH_UP && newstate != PATH_GHOST &&
 	     newstate != PATH_PENDING) && (pp->state == PATH_DELAYED)) {
@@ -2452,7 +2460,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		 * to the shortest delay
 		 */
 		pp->checkint = checkint;
-		return 1;
+		return CHECK_PATH_CHECKED;
 	}
 	if ((newstate == PATH_UP || newstate == PATH_GHOST) &&
 	    (san_path_check_enabled(pp->mpp) ||
@@ -2467,7 +2475,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 					 * in time */
 					pp->tick = 1;
 				pp->state = PATH_DELAYED;
-				return 1;
+				return CHECK_PATH_CHECKED;
 			}
 			if (!pp->marginal) {
 				pp->marginal = 1;
@@ -2525,7 +2533,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 			pp->mpp->failback_tick = 0;
 
 			pp->mpp->stat_path_failures++;
-			return 1;
+			return CHECK_PATH_CHECKED;
 		}
 
 		if (newstate == PATH_UP || newstate == PATH_GHOST) {
@@ -2604,7 +2612,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	pp->state = newstate;
 
 	if (pp->mpp->wait_for_udev)
-		return 1;
+		return CHECK_PATH_CHECKED;
 	/*
 	 * path prio refreshing
 	 */
@@ -2631,12 +2639,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 				switch_pathgroup(pp->mpp);
 		}
 	}
-	return 1;
+	return CHECK_PATH_CHECKED;
 }
 
-/*
- * Returns -1 if the path was blacklisted, and 0 otherwise
- */
 static int
 handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 			  unsigned int ticks)
@@ -2648,12 +2653,12 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 
 	if (pp->initialized != INIT_NEW && pp->initialized != INIT_FAILED &&
 	    pp->initialized != INIT_MISSING_UDEV)
-		return 0;
+		return CHECK_PATH_SKIPPED;
 
 	if (pp->tick)
 		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
 	if (pp->tick)
-		return 0; /* don't check this path yet */
+		return CHECK_PATH_SKIPPED;
 
 	conf = get_multipath_config();
 	retrigger_tries = conf->retrigger_tries;
@@ -2675,7 +2680,7 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 				log_sysfs_attr_set_value(1, ret,
 							 "%s: failed to trigger change event",
 							 pp->dev);
-			return 0;
+			return CHECK_PATH_SKIPPED;
 		} else {
 			condlog(1, "%s: not initialized after %d udev retriggers",
 				pp->dev, retrigger_tries);
@@ -2706,10 +2711,16 @@ handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 			ev_add_path(pp, vecs, 1);
 			pp->tick = 1;
 		} else if (ret == PATHINFO_SKIPPED) {
-			return -1;
+			int i;
+
+			condlog(1, "%s: path blacklisted. removing", pp->dev);
+			if ((i = find_slot(vecs->pathvec, (void *)pp)) != -1)
+				vector_del_slot(vecs->pathvec, i);
+			free_path(pp);
+			return CHECK_PATH_REMOVED;
 		}
 	}
-	return 0;
+	return CHECK_PATH_CHECKED;
 }
 
 enum checker_state {
@@ -2794,14 +2805,10 @@ checkerloop (void *ap)
 				else
 					rc = handle_uninitialized_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);
+				if (rc == CHECK_PATH_REMOVED)
 					i--;
-				} else
-					num_paths += rc;
+				else if (rc == CHECK_PATH_CHECKED)
+					num_paths++;
 				if (++paths_checked % 128 == 0 &&
 				    (lock_has_waiters(&vecs->lock) ||
 				     waiting_clients())) {
diff --git a/multipathd/main.h b/multipathd/main.h
index 4fcd6402..7aa93ca3 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -47,7 +47,7 @@ int setup_multipath(struct vectors * vecs, struct multipath * mpp);
 int update_multipath(struct vectors *vecs, char *mapname);
 int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs);
 
-void handle_path_wwid_change(struct path *pp, struct vectors *vecs);
+bool handle_path_wwid_change(struct path *pp, struct vectors *vecs);
 bool check_path_wwid_change(struct path *pp);
 int finish_path_init(struct path *pp, struct vectors * vecs);
 int resize_map(struct multipath *mpp, unsigned long long size,
-- 
2.45.0

[PATCH v2 15/20] multipathd: handle changed wwid when adding partial path

[Benjamin Marzinski]

If multipathd noticed that the WWID didn't match the device when adding
a partial path, but removing it failed, multipathd wasn't disabling the
path. Instead of calling handle_path_wwid_change(), which doesn't make
much sense when multipathd is expecting a uevent, just manually disable
the path if ev_remove_path() fails.

Since the path hasn't actually been removed, multipathd needs to make
sure that it says failed when the path state is resynced with the
kernel. Set the state to PATH_UNCHECKED in orphan_path(), and
additionally skip all path in INIT_REMOVED in sync_map_state().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs.c     | 1 +
 libmultipath/structs_vec.c | 3 ++-
 multipathd/cli_handlers.c  | 6 +++++-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 1583e001..0a26096a 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -146,6 +146,7 @@ uninitialize_path(struct path *pp)
 		return;
 
 	pp->dmstate = PSTATE_UNDEF;
+	pp->state = PATH_UNCHECKED;
 	pp->uid_attribute = NULL;
 	pp->checker_timeout = 0;
 
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 60360c76..345d3069 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -723,7 +723,8 @@ sync_map_state(struct multipath *mpp)
 
 	vector_foreach_slot (mpp->pg, pgp, i){
 		vector_foreach_slot (pgp->paths, pp, j){
-			if (pp->state == PATH_UNCHECKED ||
+			if (pp->initialized == INIT_REMOVED ||
+			    pp->state == PATH_UNCHECKED ||
 			    pp->state == PATH_WILD ||
 			    pp->state == PATH_DELAYED)
 				continue;
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 772a45d0..3a935aa0 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -540,7 +540,11 @@ add_partial_path(struct path *pp, struct vectors *vecs)
 	if (strlen(wwid) && strncmp(wwid, pp->wwid, WWID_SIZE) != 0) {
 		condlog(0, "%s: path wwid changed from '%s' to '%s'. removing",
 			pp->dev, wwid, pp->wwid);
-		ev_remove_path(pp, vecs, 1);
+		if (!(ev_remove_path(pp, vecs, 1) & REMOVE_PATH_SUCCESS) &&
+		    pp->mpp) {
+			pp->dmstate = PSTATE_FAILED;
+			dm_fail_path(pp->mpp->alias, pp->dev_t);
+		}
 		udev_device_unref(udd);
 		return -1;
 	}
-- 
2.45.0

[PATCH v2 16/20] multipathd: don't read conf->checkint twice in check_path

[Benjamin Marzinski]

check_path() already has saved the value of conf->checkint in the local
variable checkint, so there's no reason to read it again.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 92a0e424..84450906 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2513,9 +2513,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		 * upon state change, reset the checkint
 		 * to the shortest delay
 		 */
-		conf = get_multipath_config();
-		pp->checkint = conf->checkint;
-		put_multipath_config(conf);
+		pp->checkint = checkint;
 
 		if (newstate != PATH_UP && newstate != PATH_GHOST) {
 			/*
-- 
2.45.0

[PATCH v2 17/20] multipathd: make multipath devices manage their path check times

[Benjamin Marzinski]

multipathd's path checking can be very bursty, with all the paths
running their path checkers at the same time, and then all doing nothing
while waiting for the next check time. Alternatively, the paths in a
multipath device might all run their path checkers at a different time,
which can keep the multipath device from having a coherent view of the
state of all of its paths.

This patch makes all the paths of a multipath device converge to running
their checkers at the same time, and spreads out when this time is for
the different multipath devices.

To do this, the checking time is divided into adjustment intervals
(conf->adjust_int), so that the checkers run at some index within this
interval. conf->adjust_int is chosen so that it is a multiple of all
possible pp->checkint values. This means that regardless of
pp->checkint, the path should always be checked on the same indexes,
each adjustement interval.

Each multipath device has a goal index. These are evenly spread out
between 0 and conf->max_checkint. Every conf->adjust_int seconds, each
multipath device should try to check all of its paths on its goal index.
If the multipath device's check times are not aligned with the goal
index, then pp->tick for the next check will be decremented by one, to
align it over time.

In order for the path checkers to run every pp->checkint seconds,
multipathd needs to track how long a path check has been pending for,
and subtract that time from the number of ticks till the checker is run
again. If the checker has been pending for more that pp->checkint,
the path will be rechecked on the next tick after the checker returns.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.c  | 12 ++++++
 libmultipath/config.h  |  1 +
 libmultipath/structs.c |  1 +
 libmultipath/structs.h |  1 +
 multipathd/main.c      | 91 ++++++++++++++++++++++++++++++++++--------
 5 files changed, 89 insertions(+), 17 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 83fa7369..a59533b5 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -982,6 +982,18 @@ int _init_config (const char *file, struct config *conf)
 		conf->checkint = conf->max_checkint;
 	condlog(3, "polling interval: %d, max: %d",
 		conf->checkint, conf->max_checkint);
+	/*
+	 * make sure that that adjust_int is a multiple of all possible values
+	 * of pp->checkint.
+	 */
+	if (conf->max_checkint % conf->checkint == 0) {
+		conf->adjust_int = conf->max_checkint;
+	} else {
+		conf->adjust_int = conf->checkint;
+		while (2 * conf->adjust_int < conf->max_checkint)
+			conf->adjust_int *= 2;
+		conf->adjust_int *= conf->max_checkint;
+	}
 
 	if (conf->blist_devnode == NULL) {
 		conf->blist_devnode = vector_alloc();
diff --git a/libmultipath/config.h b/libmultipath/config.h
index 384193ab..800c0ca9 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -147,6 +147,7 @@ struct config {
 	int minio_rq;
 	unsigned int checkint;
 	unsigned int max_checkint;
+	unsigned int adjust_int;
 	bool use_watchdog;
 	int pgfailback;
 	int rr_weight;
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index 0a26096a..232b4230 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -149,6 +149,7 @@ uninitialize_path(struct path *pp)
 	pp->state = PATH_UNCHECKED;
 	pp->uid_attribute = NULL;
 	pp->checker_timeout = 0;
+	pp->pending_ticks = 0;
 
 	if (checker_selected(&pp->checker))
 		checker_put(&pp->checker);
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 002eeae1..457d7836 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -360,6 +360,7 @@ struct path {
 	unsigned long long size;
 	unsigned int checkint;
 	unsigned int tick;
+	unsigned int pending_ticks;
 	int bus;
 	int offline;
 	int state;
diff --git a/multipathd/main.c b/multipathd/main.c
index 84450906..87ddb101 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2388,7 +2388,7 @@ enum check_path_return {
 };
 
 static int
-check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
+do_check_path (struct vectors * vecs, struct path * pp)
 {
 	int newstate;
 	int new_path_up = 0;
@@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int marginal_pathgroups, marginal_changed = 0;
 	bool need_reload;
 
-	if (pp->initialized == INIT_REMOVED)
-		return CHECK_PATH_SKIPPED;
-
-	if (pp->tick)
-		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
-	if (pp->tick)
-		return CHECK_PATH_SKIPPED;
-
 	conf = get_multipath_config();
 	checkint = conf->checkint;
 	max_checkint = conf->max_checkint;
@@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		pp->checkint = checkint;
 	};
 
-	/*
-	 * provision a next check soonest,
-	 * in case we exit abnormally from here
-	 */
-	pp->tick = checkint;
-
 	newstate = check_path_state(pp);
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED)
 		return CHECK_PATH_SKIPPED;
@@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 				condlog(4, "%s: delay next check %is",
 					pp->dev_t, pp->checkint);
 			}
-			pp->tick = pp->checkint;
 		}
 	}
 	else if (newstate != PATH_UP && newstate != PATH_GHOST) {
@@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	return CHECK_PATH_CHECKED;
 }
 
+static int
+check_path (struct vectors * vecs, struct path * pp, unsigned int ticks,
+	    time_t start_secs)
+{
+	int r;
+	unsigned int adjust_int, max_checkint;
+	struct config *conf;
+	time_t next_idx, goal_idx;
+
+	if (pp->initialized == INIT_REMOVED)
+		return CHECK_PATH_SKIPPED;
+
+	if (pp->tick)
+		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
+	if (pp->tick)
+		return CHECK_PATH_SKIPPED;
+
+	conf = get_multipath_config();
+	max_checkint = conf->max_checkint;
+	adjust_int = conf->adjust_int;
+	put_multipath_config(conf);
+
+	r = do_check_path(vecs, pp);
+
+	/*
+	 * do_check_path() removed or orphaned the path.
+	 */
+	if (r < 0 || !pp->mpp)
+		return r;
+
+	if (pp->tick != 0) {
+		/* the path checker is pending */
+		if (pp->state != PATH_DELAYED)
+			pp->pending_ticks++;
+		else
+			pp->pending_ticks = 0;
+		return r;
+	}
+
+	/* schedule the next check */
+	pp->tick = pp->checkint;
+	if (pp->pending_ticks >= pp->tick)
+		pp->tick = 1;
+	else
+		pp->tick -= pp->pending_ticks;
+	pp->pending_ticks = 0;
+
+	if (pp->tick == 1)
+		return r;
+
+	/*
+	 * every mpp has a goal_idx in the range of
+	 * 0 <= goal_idx < conf->max_checkint
+	 *
+	 * The next check has an index, next_idx, in the range of
+	 * 0 <= next_idx < conf->adjust_int
+	 *
+	 * If the difference between the goal index and the next check index
+	 * is not a multiple of pp->checkint, then the device is not checking
+	 * the paths at its goal index, and pp->tick will be decremented by
+	 * one, to align it over time.
+	 */
+	goal_idx = (find_slot(vecs->mpvec, pp->mpp)) *
+		   max_checkint / VECTOR_SIZE(vecs->mpvec);
+	next_idx = (start_secs + pp->tick) % adjust_int;
+	if ((goal_idx - next_idx) % pp->checkint != 0)
+		pp->tick--;
+
+	return r;
+}
+
 static int
 handle_uninitialized_path(struct vectors * vecs, struct path * pp,
 			  unsigned int ticks)
@@ -2799,7 +2855,8 @@ checkerloop (void *ap)
 					continue;
 				pp->is_checked = true;
 				if (pp->mpp)
-					rc = check_path(vecs, pp, ticks);
+					rc = check_path(vecs, pp, ticks,
+							chk_start_time.tv_sec);
 				else
 					rc = handle_uninitialized_path(vecs, pp,
 								       ticks);
-- 
2.45.0

Re: [PATCH v2 17/20] multipathd: make multipath devices manage their path check times

[Martin Wilck]

On Wed, 2024-07-17 at 14:11 -0400, Benjamin Marzinski wrote:
> multipathd's path checking can be very bursty, with all the paths
> running their path checkers at the same time, and then all doing
> nothing
> while waiting for the next check time. Alternatively, the paths in a
> multipath device might all run their path checkers at a different
> time,
> which can keep the multipath device from having a coherent view of
> the
> state of all of its paths.
> 
> This patch makes all the paths of a multipath device converge to
> running
> their checkers at the same time, and spreads out when this time is
> for
> the different multipath devices.
> 
> To do this, the checking time is divided into adjustment intervals
> (conf->adjust_int), so that the checkers run at some index within
> this
> interval. conf->adjust_int is chosen so that it is a multiple of all
> possible pp->checkint values. This means that regardless of
> pp->checkint, the path should always be checked on the same indexes,
> each adjustement interval.
> 
> Each multipath device has a goal index. These are evenly spread out
> between 0 and conf->max_checkint. Every conf->adjust_int seconds,
> each
> multipath device should try to check all of its paths on its goal
> index.
> If the multipath device's check times are not aligned with the goal
> index, then pp->tick for the next check will be decremented by one,
> to
> align it over time.
> 
> In order for the path checkers to run every pp->checkint seconds,
> multipathd needs to track how long a path check has been pending for,
> and subtract that time from the number of ticks till the checker is
> run
> again. If the checker has been pending for more that pp->checkint,
> the path will be rechecked on the next tick after the checker
> returns.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/config.c  | 12 ++++++
>  libmultipath/config.h  |  1 +
>  libmultipath/structs.c |  1 +
>  libmultipath/structs.h |  1 +
>  multipathd/main.c      | 91 ++++++++++++++++++++++++++++++++++------
> --
>  5 files changed, 89 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 83fa7369..a59533b5 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -982,6 +982,18 @@ int _init_config (const char *file, struct
> config *conf)
>  		conf->checkint = conf->max_checkint;
>  	condlog(3, "polling interval: %d, max: %d",
>  		conf->checkint, conf->max_checkint);
> +	/*
> +	 * make sure that that adjust_int is a multiple of all
> possible values
> +	 * of pp->checkint.
> +	 */
> +	if (conf->max_checkint % conf->checkint == 0) {
> +		conf->adjust_int = conf->max_checkint;
> +	} else {
> +		conf->adjust_int = conf->checkint;
> +		while (2 * conf->adjust_int < conf->max_checkint)
> +			conf->adjust_int *= 2;
> +		conf->adjust_int *= conf->max_checkint;
> +	}
>  
>  	if (conf->blist_devnode == NULL) {
>  		conf->blist_devnode = vector_alloc();
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index 384193ab..800c0ca9 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -147,6 +147,7 @@ struct config {
>  	int minio_rq;
>  	unsigned int checkint;
>  	unsigned int max_checkint;
> +	unsigned int adjust_int;
>  	bool use_watchdog;
>  	int pgfailback;
>  	int rr_weight;
> diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> index 0a26096a..232b4230 100644
> --- a/libmultipath/structs.c
> +++ b/libmultipath/structs.c
> @@ -149,6 +149,7 @@ uninitialize_path(struct path *pp)
>  	pp->state = PATH_UNCHECKED;
>  	pp->uid_attribute = NULL;
>  	pp->checker_timeout = 0;
> +	pp->pending_ticks = 0;
>  
>  	if (checker_selected(&pp->checker))
>  		checker_put(&pp->checker);
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 002eeae1..457d7836 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -360,6 +360,7 @@ struct path {
>  	unsigned long long size;
>  	unsigned int checkint;
>  	unsigned int tick;
> +	unsigned int pending_ticks;
>  	int bus;
>  	int offline;
>  	int state;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 84450906..87ddb101 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2388,7 +2388,7 @@ enum check_path_return {
>  };
>  
>  static int
> -check_path (struct vectors * vecs, struct path * pp, unsigned int
> ticks)
> +do_check_path (struct vectors * vecs, struct path * pp)
>  {
>  	int newstate;
>  	int new_path_up = 0;
> @@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path
> * pp, unsigned int ticks)
>  	int marginal_pathgroups, marginal_changed = 0;
>  	bool need_reload;
>  
> -	if (pp->initialized == INIT_REMOVED)
> -		return CHECK_PATH_SKIPPED;
> -
> -	if (pp->tick)
> -		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
> -	if (pp->tick)
> -		return CHECK_PATH_SKIPPED;
> -
>  	conf = get_multipath_config();
>  	checkint = conf->checkint;
>  	max_checkint = conf->max_checkint;
> @@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path
> * pp, unsigned int ticks)
>  		pp->checkint = checkint;
>  	};
>  
> -	/*
> -	 * provision a next check soonest,
> -	 * in case we exit abnormally from here
> -	 */
> -	pp->tick = checkint;
> -
>  	newstate = check_path_state(pp);
>  	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED)
>  		return CHECK_PATH_SKIPPED;
> @@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path
> * pp, unsigned int ticks)
>  				condlog(4, "%s: delay next check
> %is",
>  					pp->dev_t, pp->checkint);
>  			}
> -			pp->tick = pp->checkint;
>  		}
>  	}
>  	else if (newstate != PATH_UP && newstate != PATH_GHOST) {
> @@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path
> * pp, unsigned int ticks)
>  	return CHECK_PATH_CHECKED;
>  }
>  
> +static int
> +check_path (struct vectors * vecs, struct path * pp, unsigned int
> ticks,
> +	    time_t start_secs)
> +{
> +	int r;
> +	unsigned int adjust_int, max_checkint;
> +	struct config *conf;
> +	time_t next_idx, goal_idx;
> +
> +	if (pp->initialized == INIT_REMOVED)
> +		return CHECK_PATH_SKIPPED;
> +
> +	if (pp->tick)
> +		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
> +	if (pp->tick)
> +		return CHECK_PATH_SKIPPED;
> +
> +	conf = get_multipath_config();
> +	max_checkint = conf->max_checkint;
> +	adjust_int = conf->adjust_int;
> +	put_multipath_config(conf);
> +
> +	r = do_check_path(vecs, pp);
> +
> +	/*
> +	 * do_check_path() removed or orphaned the path.
> +	 */
> +	if (r < 0 || !pp->mpp)
> +		return r;

do_check_path() returns an enum now, so this should be r ==
CHECK_PATH_REMOVED.

Re: [PATCH v2 17/20] multipathd: make multipath devices manage their path check times

[Benjamin Marzinski]

On Thu, Jul 18, 2024 at 10:24:13PM +0200, Martin Wilck wrote:
> On Wed, 2024-07-17 at 14:11 -0400, Benjamin Marzinski wrote:
> > multipathd's path checking can be very bursty, with all the paths
> > running their path checkers at the same time, and then all doing
> > nothing
> > while waiting for the next check time. Alternatively, the paths in a
> > multipath device might all run their path checkers at a different
> > time,
> > which can keep the multipath device from having a coherent view of
> > the
> > state of all of its paths.
> > 
> > This patch makes all the paths of a multipath device converge to
> > running
> > their checkers at the same time, and spreads out when this time is
> > for
> > the different multipath devices.
> > 
> > To do this, the checking time is divided into adjustment intervals
> > (conf->adjust_int), so that the checkers run at some index within
> > this
> > interval. conf->adjust_int is chosen so that it is a multiple of all
> > possible pp->checkint values. This means that regardless of
> > pp->checkint, the path should always be checked on the same indexes,
> > each adjustement interval.
> > 
> > Each multipath device has a goal index. These are evenly spread out
> > between 0 and conf->max_checkint. Every conf->adjust_int seconds,
> > each
> > multipath device should try to check all of its paths on its goal
> > index.
> > If the multipath device's check times are not aligned with the goal
> > index, then pp->tick for the next check will be decremented by one,
> > to
> > align it over time.
> > 
> > In order for the path checkers to run every pp->checkint seconds,
> > multipathd needs to track how long a path check has been pending for,
> > and subtract that time from the number of ticks till the checker is
> > run
> > again. If the checker has been pending for more that pp->checkint,
> > the path will be rechecked on the next tick after the checker
> > returns.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/config.c  | 12 ++++++
> >  libmultipath/config.h  |  1 +
> >  libmultipath/structs.c |  1 +
> >  libmultipath/structs.h |  1 +
> >  multipathd/main.c      | 91 ++++++++++++++++++++++++++++++++++------
> > --
> >  5 files changed, 89 insertions(+), 17 deletions(-)
> > 
> > diff --git a/libmultipath/config.c b/libmultipath/config.c
> > index 83fa7369..a59533b5 100644
> > --- a/libmultipath/config.c
> > +++ b/libmultipath/config.c
> > @@ -982,6 +982,18 @@ int _init_config (const char *file, struct
> > config *conf)
> >  		conf->checkint = conf->max_checkint;
> >  	condlog(3, "polling interval: %d, max: %d",
> >  		conf->checkint, conf->max_checkint);
> > +	/*
> > +	 * make sure that that adjust_int is a multiple of all
> > possible values
> > +	 * of pp->checkint.
> > +	 */
> > +	if (conf->max_checkint % conf->checkint == 0) {
> > +		conf->adjust_int = conf->max_checkint;
> > +	} else {
> > +		conf->adjust_int = conf->checkint;
> > +		while (2 * conf->adjust_int < conf->max_checkint)
> > +			conf->adjust_int *= 2;
> > +		conf->adjust_int *= conf->max_checkint;
> > +	}
> >  
> >  	if (conf->blist_devnode == NULL) {
> >  		conf->blist_devnode = vector_alloc();
> > diff --git a/libmultipath/config.h b/libmultipath/config.h
> > index 384193ab..800c0ca9 100644
> > --- a/libmultipath/config.h
> > +++ b/libmultipath/config.h
> > @@ -147,6 +147,7 @@ struct config {
> >  	int minio_rq;
> >  	unsigned int checkint;
> >  	unsigned int max_checkint;
> > +	unsigned int adjust_int;
> >  	bool use_watchdog;
> >  	int pgfailback;
> >  	int rr_weight;
> > diff --git a/libmultipath/structs.c b/libmultipath/structs.c
> > index 0a26096a..232b4230 100644
> > --- a/libmultipath/structs.c
> > +++ b/libmultipath/structs.c
> > @@ -149,6 +149,7 @@ uninitialize_path(struct path *pp)
> >  	pp->state = PATH_UNCHECKED;
> >  	pp->uid_attribute = NULL;
> >  	pp->checker_timeout = 0;
> > +	pp->pending_ticks = 0;
> >  
> >  	if (checker_selected(&pp->checker))
> >  		checker_put(&pp->checker);
> > diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> > index 002eeae1..457d7836 100644
> > --- a/libmultipath/structs.h
> > +++ b/libmultipath/structs.h
> > @@ -360,6 +360,7 @@ struct path {
> >  	unsigned long long size;
> >  	unsigned int checkint;
> >  	unsigned int tick;
> > +	unsigned int pending_ticks;
> >  	int bus;
> >  	int offline;
> >  	int state;
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 84450906..87ddb101 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2388,7 +2388,7 @@ enum check_path_return {
> >  };
> >  
> >  static int
> > -check_path (struct vectors * vecs, struct path * pp, unsigned int
> > ticks)
> > +do_check_path (struct vectors * vecs, struct path * pp)
> >  {
> >  	int newstate;
> >  	int new_path_up = 0;
> > @@ -2400,14 +2400,6 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> >  	int marginal_pathgroups, marginal_changed = 0;
> >  	bool need_reload;
> >  
> > -	if (pp->initialized == INIT_REMOVED)
> > -		return CHECK_PATH_SKIPPED;
> > -
> > -	if (pp->tick)
> > -		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
> > -	if (pp->tick)
> > -		return CHECK_PATH_SKIPPED;
> > -
> >  	conf = get_multipath_config();
> >  	checkint = conf->checkint;
> >  	max_checkint = conf->max_checkint;
> > @@ -2419,12 +2411,6 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> >  		pp->checkint = checkint;
> >  	};
> >  
> > -	/*
> > -	 * provision a next check soonest,
> > -	 * in case we exit abnormally from here
> > -	 */
> > -	pp->tick = checkint;
> > -
> >  	newstate = check_path_state(pp);
> >  	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED)
> >  		return CHECK_PATH_SKIPPED;
> > @@ -2587,7 +2573,6 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> >  				condlog(4, "%s: delay next check
> > %is",
> >  					pp->dev_t, pp->checkint);
> >  			}
> > -			pp->tick = pp->checkint;
> >  		}
> >  	}
> >  	else if (newstate != PATH_UP && newstate != PATH_GHOST) {
> > @@ -2640,6 +2625,77 @@ check_path (struct vectors * vecs, struct path
> > * pp, unsigned int ticks)
> >  	return CHECK_PATH_CHECKED;
> >  }
> >  
> > +static int
> > +check_path (struct vectors * vecs, struct path * pp, unsigned int
> > ticks,
> > +	    time_t start_secs)
> > +{
> > +	int r;
> > +	unsigned int adjust_int, max_checkint;
> > +	struct config *conf;
> > +	time_t next_idx, goal_idx;
> > +
> > +	if (pp->initialized == INIT_REMOVED)
> > +		return CHECK_PATH_SKIPPED;
> > +
> > +	if (pp->tick)
> > +		pp->tick -= (pp->tick > ticks) ? ticks : pp->tick;
> > +	if (pp->tick)
> > +		return CHECK_PATH_SKIPPED;
> > +
> > +	conf = get_multipath_config();
> > +	max_checkint = conf->max_checkint;
> > +	adjust_int = conf->adjust_int;
> > +	put_multipath_config(conf);
> > +
> > +	r = do_check_path(vecs, pp);
> > +
> > +	/*
> > +	 * do_check_path() removed or orphaned the path.
> > +	 */
> > +	if (r < 0 || !pp->mpp)
> > +		return r;
> 
> do_check_path() returns an enum now, so this should be r ==
> CHECK_PATH_REMOVED.

Oops. Good catch.
-Ben

> 

[PATCH v2 18/20] multipathd: factor out actual path checking loop from checkerloop

[Benjamin Marzinski]

Move the code that actually loops through the paths and checks them into
a separate function, to stop it from being so heavily indented. This
will be more imporant when a future patch makes paths checked by mpp.

No functional changes.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 65 +++++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 87ddb101..161c6962 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2783,6 +2783,38 @@ enum checker_state {
 	CHECKER_FINISHED,
 };
 
+static enum checker_state
+check_paths(struct vectors *vecs, unsigned int ticks, int *num_paths_p)
+{
+	unsigned int paths_checked = 0;
+	struct timespec diff_time, start_time, end_time;
+	struct path *pp;
+	int i, rc;
+
+	get_monotonic_time(&start_time);
+	vector_foreach_slot(vecs->pathvec, pp, i) {
+		if (pp->is_checked)
+			continue;
+		pp->is_checked = true;
+		if (pp->mpp)
+			rc = check_path(vecs, pp, ticks, start_time.tv_sec);
+		else
+			rc = handle_uninitialized_path(vecs, pp, ticks);
+		if (rc == CHECK_PATH_REMOVED)
+			i--;
+		else if (rc == CHECK_PATH_CHECKED)
+			(*num_paths_p)++;
+		if (++paths_checked % 128 == 0 &&
+		    (lock_has_waiters(&vecs->lock) || waiting_clients())) {
+			get_monotonic_time(&end_time);
+			timespecsub(&end_time, &start_time, &diff_time);
+			if (diff_time.tv_sec > 0)
+				return CHECKER_RUNNING;
+		}
+	}
+	return CHECKER_FINISHED;
+}
+
 static void *
 checkerloop (void *ap)
 {
@@ -2814,7 +2846,7 @@ 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;
 		unsigned int ticks = 0;
 		enum checker_state checker_state = CHECKER_STARTING;
 
@@ -2833,16 +2865,14 @@ checkerloop (void *ap)
 			sd_notify(0, "WATCHDOG=1");
 #endif
 		while (checker_state != CHECKER_FINISHED) {
-			unsigned int paths_checked = 0, i;
-			struct timespec chk_start_time;
 			struct multipath *mpp;
+			int i;
 
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
 			vector_foreach_slot(vecs->mpvec, mpp, i)
 				mpp->is_checked = false;
-			get_monotonic_time(&chk_start_time);
 			if (checker_state == CHECKER_STARTING) {
 				vector_foreach_slot(vecs->mpvec, mpp, i)
 					sync_mpp(vecs, mpp, ticks);
@@ -2850,32 +2880,7 @@ checkerloop (void *ap)
 					pp->is_checked = false;
 				checker_state = CHECKER_RUNNING;
 			}
-			vector_foreach_slot(vecs->pathvec, pp, i) {
-				if (pp->is_checked)
-					continue;
-				pp->is_checked = true;
-				if (pp->mpp)
-					rc = check_path(vecs, pp, ticks,
-							chk_start_time.tv_sec);
-				else
-					rc = handle_uninitialized_path(vecs, pp,
-								       ticks);
-				if (rc == CHECK_PATH_REMOVED)
-					i--;
-				else if (rc == CHECK_PATH_CHECKED)
-					num_paths++;
-				if (++paths_checked % 128 == 0 &&
-				    (lock_has_waiters(&vecs->lock) ||
-				     waiting_clients())) {
-					get_monotonic_time(&end_time);
-					timespecsub(&end_time, &chk_start_time,
-						    &diff_time);
-					if (diff_time.tv_sec > 0)
-						goto unlock;
-				}
-			}
-			checker_state = CHECKER_FINISHED;
-unlock:
+			checker_state = check_paths(vecs, ticks, &num_paths);
 			lock_cleanup_pop(vecs->lock);
 			if (checker_state != CHECKER_FINISHED) {
 				/* Yield to waiters */
-- 
2.45.0

[PATCH v2 19/20] multipathd: check paths in order by mpp

[Benjamin Marzinski]

Instead of checking all of the paths in vecs->pathvec in order, first
check all the paths in each multipath device. Then check the
uninitialized paths.

One issue with checking paths this way is that the multipath device can
be resynced or even removed while a path is being checked. The path can
also be removed. If there is any change to the multipath device,
multipathd needs to loop through its paths again, because the current
indexes may not longer be valid.

To do this change mpp->is_checked to an int called mpp->synced_count,
and increment it whenever the multipath device gets resynced. After each
path is checked, make sure that the multipath device still exists, that
mpp->synced_count hasn't changed. If either has happened, restart
checking at the current index in mpvec (which will either be the same
mpp if it was just resynced, or the next mpp if the last one was
deleted). Since the multipath device is resynced when its first path is
checked, this restart will happen to every multipath device at least
once per loop. But the paths themselves aren't rechecked, so it's not
much overhead.

If resyncing a multipath device fails in do_check_mpp(), there may be
path devices that have pp->mpp set, but are no longer in one of the
multipath device's pathgroups, and thus will not get checked. This
almost definitely means the multipath device was deleted. If
do_check_mpp() failed to resync the device, but it wasn't deleted, it
will get called again in max_checkint seconds even if it no longer has
mpp->pg set, and the paths will get checked again after that.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs.h     |  2 +-
 libmultipath/structs_vec.c |  1 +
 multipathd/main.c          | 54 ++++++++++++++++++++++++++++++++------
 3 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 457d7836..91509881 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -455,7 +455,7 @@ struct multipath {
 	int ghost_delay_tick;
 	int queue_mode;
 	unsigned int sync_tick;
-	bool is_checked;
+	int synced_count;
 	uid_t uid;
 	gid_t gid;
 	mode_t mode;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 345d3069..cf9c6fe8 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -513,6 +513,7 @@ update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 	conf = get_multipath_config();
 	mpp->sync_tick = conf->max_checkint;
 	put_multipath_config(conf);
+	mpp->synced_count++;
 
 	r = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_MPATH_ONLY,
 			  (mapid_t) { .str = mpp->alias },
diff --git a/multipathd/main.c b/multipathd/main.c
index 161c6962..5eefa475 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2356,7 +2356,6 @@ do_sync_mpp(struct vectors * vecs, struct multipath *mpp)
 	int i, ret;
 	struct path *pp;
 
-	mpp->is_checked = true;
 	ret = update_multipath_strings(mpp, vecs->pathvec);
 	if (ret != DMP_OK) {
 		condlog(1, "%s: %s", mpp->alias, ret == DMP_NOT_FOUND ?
@@ -2431,7 +2430,7 @@ do_check_path (struct vectors * vecs, struct path * pp)
 		return handle_path_wwid_change(pp, vecs)? CHECK_PATH_REMOVED :
 							  CHECK_PATH_SKIPPED;
 	}
-	if (!pp->mpp->is_checked) {
+	if (pp->mpp->synced_count == 0) {
 		do_sync_mpp(vecs, pp->mpp);
 		/* if update_multipath_strings orphaned the path, quit early */
 		if (!pp->mpp)
@@ -2788,18 +2787,57 @@ check_paths(struct vectors *vecs, unsigned int ticks, int *num_paths_p)
 {
 	unsigned int paths_checked = 0;
 	struct timespec diff_time, start_time, end_time;
+	struct multipath *mpp;
 	struct path *pp;
 	int i, rc;
 
 	get_monotonic_time(&start_time);
+
+	vector_foreach_slot(vecs->mpvec, mpp, i) {
+		struct pathgroup *pgp;
+		struct path *pp;
+		int j, k;
+		bool check_for_waiters = false;
+		/* maps can be rechecked, so this is not always 0 */
+		int synced_count = mpp->synced_count;
+
+		vector_foreach_slot (mpp->pg, pgp, j) {
+			vector_foreach_slot (pgp->paths, pp, k) {
+				if (!pp->mpp || pp->is_checked)
+					continue;
+				pp->is_checked = true;
+				rc = check_path(vecs, pp, ticks,
+						start_time.tv_sec);
+				if (rc == CHECK_PATH_CHECKED)
+					(*num_paths_p)++;
+				if (++paths_checked % 128 == 0)
+					check_for_waiters = true;
+				/*
+				 * mpp has been removed or resynced. Path may
+				 * have been removed.
+				 */
+				if (VECTOR_SLOT(vecs->mpvec, i) != mpp ||
+				    synced_count != mpp->synced_count) {
+					i--;
+					goto next_mpp;
+				}
+			}
+		}
+next_mpp:
+		if (check_for_waiters &&
+		    (lock_has_waiters(&vecs->lock) || waiting_clients())) {
+			get_monotonic_time(&end_time);
+			timespecsub(&end_time, &start_time, &diff_time);
+			if (diff_time.tv_sec > 0)
+				return CHECKER_RUNNING;
+		}
+	}
 	vector_foreach_slot(vecs->pathvec, pp, i) {
-		if (pp->is_checked)
+		if (pp->mpp || pp->is_checked)
 			continue;
 		pp->is_checked = true;
-		if (pp->mpp)
-			rc = check_path(vecs, pp, ticks, start_time.tv_sec);
-		else
-			rc = handle_uninitialized_path(vecs, pp, ticks);
+
+		rc = handle_uninitialized_path(vecs, pp, ticks);
 		if (rc == CHECK_PATH_REMOVED)
 			i--;
 		else if (rc == CHECK_PATH_CHECKED)
@@ -2872,7 +2910,7 @@ checkerloop (void *ap)
 			lock(&vecs->lock);
 			pthread_testcancel();
 			vector_foreach_slot(vecs->mpvec, mpp, i)
-				mpp->is_checked = false;
+				mpp->synced_count = 0;
 			if (checker_state == CHECKER_STARTING) {
 				vector_foreach_slot(vecs->mpvec, mpp, i)
 					sync_mpp(vecs, mpp, ticks);
-- 
2.45.0

[PATCH v2 20/20] multipathd: clean up the correct wwid in check_path_wwid_change

[Benjamin Marzinski]

After check_path_wwid_change() grabbed a new copy of the wwid, it was
stripping trailing whitespace off of pp->wwid, instead of the copy it
just got.

Fixes: 4660cff ("multipathd: add recheck_wwid option to verify the path
wwid")

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 5eefa475..f4910403 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1025,9 +1025,9 @@ check_path_wwid_change(struct path *pp)
 	}
 
 	/*Strip any trailing blanks */
-	for (i = strlen(pp->wwid); i > 0 && pp->wwid[i-1] == ' '; i--);
+	for (i = strlen(wwid); i > 0 && wwid[i-1] == ' '; i--);
 		/* no-op */
-	pp->wwid[i] = '\0';
+	wwid[i] = '\0';
 	condlog(4, "%s: Got wwid %s by sgio", pp->dev, wwid);
 
 	if (strncmp(wwid, pp->wwid, WWID_SIZE)) {
-- 
2.45.0

Re: [PATCH v2 00/20] path checker refactor and misc fixes

[Martin Wilck]

On Wed, 2024-07-17 at 14:10 -0400, Benjamin Marzinski wrote:
> This patchset is based on discussions I had with Martin Wilck about
> my
> last, partially applied patchset. It's based on top of his mwilck-tip
> branch, 4ef98926 "fixup! Makefile: add targets for test-progs.cpio
> and
> test-progs.tar". These patches are mostly orthogonal to the
> libmp_mapinfo() work. I'll rebase them if further changes to that
> patchset stop them form applying cleanly.
> 
> The first two patches are reposts of patches from my earlier
> patchset,
> redone to work with the new libmp_mapinfo() API. They make it
> possible
> to add maps by WWID with "multipathd add map".
> 
> The rest of the patches are work and bugfixes related to refactoring
> checkerloop so that
> 
> 1. multipath devices get resynced with the kernel occasionally, even
> if
> they have not paths
> 
> 2. If multiple paths from a multipath device are checked in the same
> loop, the multipath device will only get resynced once.
> 
> 3. All the paths of a multipath device will converge to being checked
> at
> the same time (at least as much as their differing checkint values
> will
> allow).
> 
> 4. The times for checking the paths of each multipath device will
> spread
> out as much as possible so multipathd doesn't do all of it's checking
> in
> a burst.
> 
> 5. path checking is done my multipath device (for initialized paths,
> the uninitialized paths are handled after all the adopted paths are
> handled).
> 
> Items 1 & 2 are handled by patch 10 and preceding patches.
> 
> Items 3 & 4 and handled by patch 17 and preceding patches.
> 
> Item 5 is handled by patch 18 and 19.
> 
> Changes in v2 (old patch number in quotes):
> 01: change from returning minor number to dm info (Martin)
> 02: adapt to change in patch 01
> 05 (05-07): squash into one commit (Martin)
> 08 (10): fix commit message (Matin)
> 10 (12): rename do_check_mpp amd check_mpp
> 11 (13): move the code that saved the path name to right before it
> was
>          removed (Martin)
> 14 (16): use symbolic return codes (Martin)
> 15 (17): set path state to PATH_UNCHECKED when orphaned, and skip
> paths
>          in INIT_REMOVED in sync_map_state() (Martin)
> 17 (19): clear pp->pending_ticks in check_path() if the path is
> delayed
>          instead of pending. Use a different equation to check if we
>          need to modify the ticks, that can adjust the ticks on every
>          check. adapt to change in patch 14
> 18 (20): adapt to change in patch 14
> 19 (21): adapt to change in patch 10 and 14
> 20 (22): Add fixes trailer
> 
> Benjamin Marzinski (20):
>   libmultipath: rename dm_map_present_by_wwid() and add outputs
>   multipathd: make cli_add_map() handle adding maps by WWID correctly
>   multipathd: remove checker restart optimization
>   multipathd: refactor path state getting code into a helper
>   multipathd: handle uninitialized paths in new function
>   multipathd: check paths immediately after failing udev
> initialization
>   multipathd: set pp->tick = max_checkint in
> handle_uninitialized_path
>   multipathd: return 0 from check_path() if that path wasn't checked
>   multipathd: reorder path state checks
>   multipathd: adjust when mpp is synced with the kernel
>   multipathd: resync map after setup_map in ev_remove_path
>   multipathd: resync map after setup_map in resize_map
>   multipathd: always resync map in reload_and_sync_map
>   multipathd: correctly handle paths removed for a wwid change
>   multipathd: handle changed wwid when adding partial path
>   multipathd: don't read conf->checkint twice in check_path
>   multipathd: make multipath devices manage their path check times
>   multipathd: factor out actual path checking loop from checkerloop
>   multipathd: check paths in order by mpp
>   multipathd: clean up the correct wwid in check_path_wwid_change
> 
>  libmultipath/config.c             |  12 +
>  libmultipath/config.h             |   1 +
>  libmultipath/configure.c          |   1 +
>  libmultipath/devmapper.c          |   5 +-
>  libmultipath/devmapper.h          |   2 +-
>  libmultipath/libmultipath.version |   5 +
>  libmultipath/structs.c            |   2 +
>  libmultipath/structs.h            |   3 +
>  libmultipath/structs_vec.c        |   9 +-
>  libmultipath/valid.c              |   2 +-
>  multipathd/cli_handlers.c         |  66 ++--
>  multipathd/main.c                 | 582 +++++++++++++++++++---------
> --
>  multipathd/main.h                 |   2 +-
>  tests/valid.c                     |  13 +-
>  14 files changed, 432 insertions(+), 273 deletions(-)
> 

For the series, except 17/20: 
Reviewed-by: Martin Wilck <mwilck@suse.com>