From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Benjamin Marzinski <bmarzins@redhat.com>

Date: Thu, 11 Jun 2020 15:41:18 -0500

Subject: [PATCH] multipathd: fix check_path errors with removed map

If a multipath device is removed during, or immediately before the call

to check_path(), multipathd can behave incorrectly. A missing multpath

device will cause update_multipath_strings() to fail, setting

pp->dmstate to PSTATE_UNDEF.  If the path is up, this state will cause

reinstate_path() to be called, which will also fail.  This will trigger

a reload, restoring the recently removed device.

If update_multipath_strings() fails because there is no multipath

device, check_path should just quit, since the remove dmevent and uevent

are likely already queued up. Also, I don't see any reason to reload the

multipath device if reinstate fails. This code was added by

fac68d7a99ef17d496079538a5c6836acd7911ab, which clamined that reinstate

could fail if the path was disabled.  Looking through the current kernel

code, I can't see any reason why a reinstate would fail, where a reload

would help. If the path was missing from the multipath device,

update_multipath_strings() would already catch that, and quit

check_path() early, which make more sense to me than reloading does.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

---

 multipathd/main.c | 44 +++++++++++++++++++-------------------------

 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c

index e3427d3d..1d9ce7f7 100644

--- a/multipathd/main.c

+++ b/multipathd/main.c

@@ -1611,22 +1611,18 @@ fail_path (struct path * pp, int del_active)

 /*

  * caller must have locked the path list before calling that function

  */

-static int

+static void

 reinstate_path (struct path * pp)

 {

-	int ret = 0;

-

 	if (!pp->mpp)

-		return 0;

+		return;

-	if (dm_reinstate_path(pp->mpp->alias, pp->dev_t)) {

+	if (dm_reinstate_path(pp->mpp->alias, pp->dev_t))

 		condlog(0, "%s: reinstate failed", pp->dev_t);

-		ret = 1;

-	} else {

+	else {

 		condlog(2, "%s: reinstated", pp->dev_t);

 		update_queue_mode_add_path(pp->mpp);

 	}

-	return ret;

 }

 static void

@@ -2087,9 +2083,16 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)

 	/*

 	 * Synchronize with kernel state

 	 */

-	if (update_multipath_strings(pp->mpp, vecs->pathvec, 1) != DMP_OK) {

-		condlog(1, "%s: Could not synchronize with kernel state",

-			pp->dev);

+	ret = update_multipath_strings(pp->mpp, vecs->pathvec, 1);

+	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->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 */

@@ -2179,12 +2182,8 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)

 		/*

 		 * reinstate this path

 		 */

-		if (!disable_reinstate && reinstate_path(pp)) {

-			condlog(3, "%s: reload map", pp->dev);

-			ev_add_path(pp, vecs, 1);

-			pp->tick = 1;

-			return 0;

-		}

+		if (!disable_reinstate)

+			reinstate_path(pp);

 		new_path_up = 1;

 		if (oldchkrstate != PATH_UP && oldchkrstate != PATH_GHOST)

@@ -2200,15 +2199,10 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)

 	else if (newstate == PATH_UP || newstate == PATH_GHOST) {

 		if ((pp->dmstate == PSTATE_FAILED ||

 		    pp->dmstate == PSTATE_UNDEF) &&

-		    !disable_reinstate) {

+		    !disable_reinstate)

 			/* Clear IO errors */

-			if (reinstate_path(pp)) {

-				condlog(3, "%s: reload map", pp->dev);

-				ev_add_path(pp, vecs, 1);

-				pp->tick = 1;

-				return 0;

-			}

-		} else {

+			reinstate_path(pp);

+		else {

 			LOG_MSG(4, verbosity, pp);

 			if (pp->checkint != max_checkint) {

 				/*

-- 

2.17.2