Patchwork [10/12] multipathd: clean up set_no_path_retry

login
register
mail settings
Submitter Benjamin Marzinski
Date Dec. 7, 2017, 6:49 p.m.
Message ID <1512672546-12785-11-git-send-email-bmarzins@redhat.com>
Download mbox | patch
Permalink /patch/10100865/
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show

Comments

Benjamin Marzinski - Dec. 7, 2017, 6:49 p.m.
set_no_path_retry() was always updating the queue_if_no_path setting,
even if it was unnecessary.  This caused problems when no_path_retry was
set to a number, where any time __setup_multipath() was called with
reset, queuing automatically restarted, even if there were no usable
paths.

Instead, set_no_path_retry() should just fix the queueing if it is set
to an incorrect value.  This is simple to detect if no_path_retry is set
to "queue" or "fail".  When it is set to a number, multipathd needs to
see if there are usable paths. If so, queueing should be enabled.  If
not, queueing may be either enabled or disabled, but if it is enabled
and the device isn't already in recovery mode, it should be.

Also, calling dm_map_present() is pointless in __setup_multipath(),
since the existence of the map was just checked in dm_get_info().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Martin Wilck - Dec. 7, 2017, 10:14 p.m.
On Thu, 2017-12-07 at 12:49 -0600, Benjamin Marzinski wrote:
> set_no_path_retry() was always updating the queue_if_no_path setting,
> even if it was unnecessary.  This caused problems when no_path_retry
> was
> set to a number, where any time __setup_multipath() was called with
> reset, queuing automatically restarted, even if there were no usable
> paths.
> 
> Instead, set_no_path_retry() should just fix the queueing if it is
> set
> to an incorrect value.  This is simple to detect if no_path_retry is
> set
> to "queue" or "fail".  When it is set to a number, multipathd needs
> to
> see if there are usable paths. If so, queueing should be enabled.  If
> not, queueing may be either enabled or disabled, but if it is enabled
> and the device isn't already in recovery mode, it should be.
> 
> Also, calling dm_map_present() is pointless in __setup_multipath(),
> since the existence of the map was just checked in dm_get_info().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>

Patch

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 2f1f277..fbab61f 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -325,21 +325,27 @@  void enter_recovery_mode(struct multipath *mpp)
 
 static void set_no_path_retry(struct multipath *mpp)
 {
-	mpp->retry_tick = 0;
+	char is_queueing = 0;
+
 	mpp->nr_active = pathcount(mpp, PATH_UP) + pathcount(mpp, PATH_GHOST);
+	if (mpp->features && strstr(mpp->features, "queue_if_no_path"))
+		is_queueing = 1;
 
 	switch (mpp->no_path_retry) {
 	case NO_PATH_RETRY_UNDEF:
 		break;
 	case NO_PATH_RETRY_FAIL:
-		dm_queue_if_no_path(mpp->alias, 0);
+		if (is_queueing)
+			dm_queue_if_no_path(mpp->alias, 0);
 		break;
 	case NO_PATH_RETRY_QUEUE:
-		dm_queue_if_no_path(mpp->alias, 1);
+		if (!is_queueing)
+			dm_queue_if_no_path(mpp->alias, 1);
 		break;
 	default:
-		dm_queue_if_no_path(mpp->alias, 1);
-		if (mpp->nr_active == 0)
+		if (mpp->nr_active > 0)
+			dm_queue_if_no_path(mpp->alias, 1);
+		else if (is_queueing && mpp->retry_tick == 0)
 			enter_recovery_mode(mpp);
 		break;
 	}
@@ -354,12 +360,6 @@  int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		goto out;
 	}
 
-	if (!dm_map_present(mpp->alias)) {
-		/* Table has been removed */
-		condlog(3, "%s: table does not exist", mpp->alias);
-		goto out;
-	}
-
 	if (update_multipath_strings(mpp, vecs->pathvec, 1)) {
 		condlog(0, "%s: failed to setup multipath", mpp->alias);
 		goto out;