patch to discovery.c to still get path priority for paths with path state of PATH_DOWN

patch to discovery.c to still get path priority for paths with path state of PATH_DOWN

[Edward Goggin]

[-- Attachment #1: discovery.c.diff --]
[-- Type: application/octet-stream, Size: 697 bytes --]

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index efc2016..15fe89b 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -701,7 +701,14 @@ pathinfo (struct path *pp, vector hwtabl
 	if (mask & DI_CHECKER && get_state(pp))
 		goto blank;
 	
-	if (mask & DI_PRIO && pp->state != PATH_DOWN)
+
+	/*
+	 * Path state of PATH_DOWN does not necessarily prevent
+	 * path priority callout (or getuid callouot) from
+	 * succeeding since the path may be being considered
+	 * failed for reasons other than transport connectivity.
+	 */
+	if (mask & DI_PRIO /*&& pp->state != PATH_DOWN*/)
 		get_prio(pp);
 
 	if (mask & DI_WWID && !strlen(pp->wwid))

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: patch to discovery.c to still get path priority for paths with path state of PATH_DOWN

[Dave Wysochanski]

Ed, I am looking through your patches and in particular this change,
which reverts a previous change I submitted a little while back.

Maybe we can do something better like clarify states.  Looking through
the code now and will have more feedback.



On Fri, 2006-11-10 at 18:01 -0500, Edward Goggin wrote:
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

RE: patch to discovery.c to still get path priority forpaths with path state of PATH_DOWN

[Edward Goggin]

Dave,

Sorry for the confusion.

I realize that at first glance the patch may not make sense, but it
is an attempt to avoid the case of not being able to properly assign
path groups with a group_by_priority path grouping policy if any of
the paths respond successfully to scsi_id(8) and mpath_prio_* but not
their multipath path checker.  Such is the case for LUNZ and inactive
snapshot logical units on CLARiiON.

Ed

> -----Original Message-----
> From: dm-devel-bounces@redhat.com 
> [mailto:dm-devel-bounces@redhat.com] On Behalf Of Dave Wysochanski
> Sent: Monday, November 13, 2006 3:05 PM
> To: device-mapper development
> Cc: christophe.varoqui@free.fr
> Subject: Re: [dm-devel] patch to discovery.c to still get 
> path priority forpaths with path state of PATH_DOWN
> 
> Ed, I am looking through your patches and in particular this change,
> which reverts a previous change I submitted a little while back.
> 
> Maybe we can do something better like clarify states.  Looking through
> the code now and will have more feedback.
> 
> 
> 
> On Fri, 2006-11-10 at 18:01 -0500, Edward Goggin wrote:
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
> 
> 

RE: patch to discovery.c to still get path priority forpaths with path state of PATH_DOWN

[Dave Wysochanski]

[-- Attachment #1: Type: text/plain, Size: 2719 bytes --]

Ed,

Are you saying that there's no way in the path checker to distinguish
between this kind of path and a path that is genuinely down?  By "down",
I mean "fails ioctl" either directly or with an unexpected
CHECK_CONDITION (this seems to be what PATH_DOWN means in the code today
for all the path checkers).  Can you return another state in your path
checker for this type of path/LUN?

IMO, if at all possible, it would be good to leave "PATH_DOWN" with its
current meaning and not call the priority callouts for paths in this
state.  If the priority callouts could obtain priorities without SG_IO
succeeding, it might make sense, but this is not the case today.  If you
once had a good priority because you could get a command through, now
you call it when the path is down it will be replaced with an incorrect
one.

Arguably the states are fuzzy and "types of paths" are mixed in with
"path states" which leads to the fuzzyness/confusion.  Right now I don't
have a good enough feel for it to offer clarifying suggestions though
other than the attached comment patch which tries to clarify the meaning
of each state as it is in the code today.


On Mon, 2006-11-13 at 15:47 -0500, Edward Goggin wrote:
> Dave,
> 
> Sorry for the confusion.
> 
> I realize that at first glance the patch may not make sense, but it
> is an attempt to avoid the case of not being able to properly assign
> path groups with a group_by_priority path grouping policy if any of
> the paths respond successfully to scsi_id(8) and mpath_prio_* but not
> their multipath path checker.  Such is the case for LUNZ and inactive
> snapshot logical units on CLARiiON.
> 
> Ed
> 
> > -----Original Message-----
> > From: dm-devel-bounces@redhat.com 
> > [mailto:dm-devel-bounces@redhat.com] On Behalf Of Dave Wysochanski
> > Sent: Monday, November 13, 2006 3:05 PM
> > To: device-mapper development
> > Cc: christophe.varoqui@free.fr
> > Subject: Re: [dm-devel] patch to discovery.c to still get 
> > path priority forpaths with path state of PATH_DOWN
> > 
> > Ed, I am looking through your patches and in particular this change,
> > which reverts a previous change I submitted a little while back.
> > 
> > Maybe we can do something better like clarify states.  Looking through
> > the code now and will have more feedback.
> > 
> > 
> > 
> > On Fri, 2006-11-10 at 18:01 -0500, Edward Goggin wrote:
> > > --
> > > dm-devel mailing list
> > > dm-devel@redhat.com
> > > https://www.redhat.com/mailman/listinfo/dm-devel
> > 
> > --
> > dm-devel mailing list
> > dm-devel@redhat.com
> > https://www.redhat.com/mailman/listinfo/dm-devel
> > 
> > 
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

[-- Attachment #2: clarify-path-states.patch --]
[-- Type: text/x-patch, Size: 2042 bytes --]

Index: multipath-tools/libcheckers/checkers.h
===================================================================
--- multipath-tools.orig/libcheckers/checkers.h	2006-10-17 18:28:05.000000000 -0400
+++ multipath-tools/libcheckers/checkers.h	2006-11-14 09:18:39.000000000 -0500
@@ -2,7 +2,47 @@
 #define _CHECKERS_H
 
 /*
- * path states
+ *
+ * Userspace (multipath/multipathd) path states
+ *
+ * PATH_WILD:
+ * Use: None of the checkers (returned if we don't have an fd)
+ * Description: Corner case where "fd <= 0" for path fd (see checker_check())
+ * ??? Might need clarification (when does it happen, etc)
+ *
+ * PATH_UNCHECKED:
+ * - Use: Only in directio checker
+ * - Description: set when fcntl(F_GETFL) fails to return flags or O_DIRECT
+ *   not include in flags, or O_DIRECT read fails
+ * - Notes:
+ *   - multipathd: uses it to skip over paths in sync_map_state()
+ *   - multipath: used in update_paths(); if state==PATH_UNCHECKED, call
+ *     pathinfo()
+ * ??? Might need clarification
+ *
+ * PATH_DOWN
+ * - Use: All checkers (directio, emc_clariion, hp_sw, readsector0, tur)
+ * - Description: Either a) SG_IO ioctl failed, or b) check condition on some
+ *   SG_IO ioctls that succeed (tur, readsector0 checkers); path is down and
+ *   you shouldn't try to send commands to it
+ *
+ * PATH_UP
+ * - Use: All checkers (directio, emc_clariion, hp_sw, readsector0, tur)
+ * - Description: Path is up and I/O can be sent to it
+ *
+ * PATH_SHAKY
+ * - Use: Only emc_clariion
+ * - Description: Indicates path not available for "normal" operations
+ * ??? Probably needs a little clarification
+ *
+ * PATH_GHOST
+ * - Use: Only hp_sw
+ * - Description: Indicates a "passive/standby" path on active/passive HP
+ *   arrays.  These paths will return valid answers to certain SCSI commands
+ *   (tur, read_capacity, inquiry, start_stop), but will fail I/O commands.
+ *   The path needs an initialization command to be sent to it in order for
+ *   I/Os to succeed.
+ *
  */
 #define PATH_WILD	-1
 #define PATH_UNCHECKED	0

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

RE: patch to discovery.c to still get path priorityforpaths with path state of PATH_DOWN

[Edward Goggin]

On Tuesday, November 14, 2006 9:27 AM, Dave Wysochanski wrote:
> Are you saying that there's no way in the path checker to distinguish
> between this kind of path and a path that is genuinely down?

No, I'm certainly not saying that at all.  I would rather not
complicate matters by adding yet another path state though.

The particular problem I'm trying to fix is that the path group
membership for a multipath map can be incorrectly initially set
up if scsi_id(8) is successful but the paths are all failed.
When the paths return to a ready state, the path group membership
is not corrected by reloading the map -- this only happens if the
paths are removed and later added back.

What do you think of a fix involving changing need_switch_pathgroup()
in multipathd to check and reload a corrected version of the map
in this case?
  
> By "down",
> I mean "fails ioctl" either directly or with an unexpected
> CHECK_CONDITION (this seems to be what PATH_DOWN means in the 
> code today
> for all the path checkers).  Can you return another state in your path
> checker for this type of path/LUN?
> 
> IMO, if at all possible, it would be good to leave 
> "PATH_DOWN" with its
> current meaning and not call the priority callouts for paths in this
> state.  If the priority callouts could obtain priorities without SG_IO
> succeeding, it might make sense, but this is not the case 
> today.  If you
> once had a good priority because you could get a command through, now
> you call it when the path is down it will be replaced with an 
> incorrect
> one.

Yes, good point.  The converse of this statement is indeed what my
patch is trying to prevent.  That is, initially having an incorrect
priority before the path groups are created then having the good or
correct priority calculated only after the path groups are already
created.

> 
> Arguably the states are fuzzy and "types of paths" are mixed in with
> "path states" which leads to the fuzzyness/confusion.  Right 
> now I don't
> have a good enough feel for it to offer clarifying suggestions though
> other than the attached comment patch which tries to clarify 
> the meaning
> of each state as it is in the code today.

RE: patch to discovery.c to still get path priorityforpaths with path state of PATH_DOWN

[Dave Wysochanski]

On Tue, 2006-11-14 at 15:09 -0500, Edward Goggin wrote:
> On Tuesday, November 14, 2006 9:27 AM, Dave Wysochanski wrote:
> > Are you saying that there's no way in the path checker to distinguish
> > between this kind of path and a path that is genuinely down?
> 
> No, I'm certainly not saying that at all.  I would rather not
> complicate matters by adding yet another path state though.
> 
> The particular problem I'm trying to fix is that the path group
> membership for a multipath map can be incorrectly initially set
> up if scsi_id(8) is successful but the paths are all failed.
> When the paths return to a ready state, the path group membership
> is not corrected by reloading the map -- this only happens if the
> paths are removed and later added back.
> 
> What do you think of a fix involving changing need_switch_pathgroup()
> in multipathd to check and reload a corrected version of the map
> in this case?
>  

Maybe something like that yes.  It doesn't look like the code handles a
switch in priority of a path really at all so putting that in there
somewhere (checkerloop?) seems worth doing.  Such an approach would have
to be careful that triggering off a change in priority wouldn't cause
other problems if the priority callout fails in a transient way (e.g. if
the priority goes to -1, you probably don't want to modify anything).


>  
> > By "down",
> > I mean "fails ioctl" either directly or with an unexpected
> > CHECK_CONDITION (this seems to be what PATH_DOWN means in the 
> > code today
> > for all the path checkers).  Can you return another state in your path
> > checker for this type of path/LUN?
> > 
> > IMO, if at all possible, it would be good to leave 
> > "PATH_DOWN" with its
> > current meaning and not call the priority callouts for paths in this
> > state.  If the priority callouts could obtain priorities without SG_IO
> > succeeding, it might make sense, but this is not the case 
> > today.  If you
> > once had a good priority because you could get a command through, now
> > you call it when the path is down it will be replaced with an 
> > incorrect
> > one.
> 
> Yes, good point.  The converse of this statement is indeed what my
> patch is trying to prevent.  That is, initially having an incorrect
> priority before the path groups are created then having the good or
> correct priority calculated only after the path groups are already
> created.
> 

One other thought I had was the notion of a "priority valid" flag (or a
special priority value) in the path for the case of group_by_prio.  Set
it to invalid in alloc_path(), then just don't add the path to the
multipath struct in coalesce_paths() if the priority value was invalid.
Then over in checkerloop(), add the path to the multipath map when you
get a valid priority.

It is more complicated than that I'm sure (e.g. checkerloop() assumes
pp->mpp is non-null in places, etc) but seemed like a half-decent
approach to at least consider.

This approach doesn't take into consideration the general case of a
change in path priority though.


> > 
> > Arguably the states are fuzzy and "types of paths" are mixed in with
> > "path states" which leads to the fuzzyness/confusion.  Right 
> > now I don't
> > have a good enough feel for it to offer clarifying suggestions though
> > other than the attached comment patch which tries to clarify 
> > the meaning
> > of each state as it is in the code today.

RE: patch to discovery.c to still get pathpriorityforpaths with path state of PATH_DOWN

[Edward Goggin]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote

> One other thought I had was the notion of a "priority valid" 
> flag (or a
> special priority value) in the path for the case of 
> group_by_prio.  Set
> it to invalid in alloc_path(), then just don't add the path to the
> multipath struct in coalesce_paths() if the priority value 
> was invalid.
> Then over in checkerloop(), add the path to the multipath map when you
> get a valid priority.
> 
> It is more complicated than that I'm sure (e.g. checkerloop() assumes
> pp->mpp is non-null in places, etc) but seemed like a half-decent
> approach to at least consider.
> 
> This approach doesn't take into consideration the general case of a
> change in path priority though.

I very much like the first part of your "priority valid" idea
mentioned above.

I've enclosed a patch for the first part of your idea.  The patch
should address the concern you had about recalculating priority
for a path when its path state changes from not PATH_DOWN to
PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
paths if the path's priority has never been successfully
retrieved before.

I've punted for now on the reloading of the map if a path priority
change has caused a path group membership change.  This is kind of
complex, has lots of down sides, and there doesn't seem to be a
need for it as of now.

[-- Attachment #2: structs.h.diff --]
[-- Type: application/octet-stream, Size: 297 bytes --]

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index faa2b8f..c6fcb1f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -97,6 +97,7 @@ struct path {
 	int dmstate;
 	int failcount;
 	int priority;
+	int prio_valid;
 	int pgindex;
 	char * getuid;
 	char * getprio;

[-- Attachment #3: structs.c.diff --]
[-- Type: application/octet-stream, Size: 294 bytes --]

diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index f0564c2..82bddf4 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -31,6 +31,7 @@ alloc_path (void)
 		pp->sg_id.scsi_id = -1;
 		pp->sg_id.lun = -1;
 		pp->fd = -1;
+		pp->prio_valid = 0;
 	}
 	return pp;
 }

[-- Attachment #4: discovery.c.diff --]
[-- Type: application/octet-stream, Size: 1108 bytes --]

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index efc2016..689ae9f 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -639,6 +639,7 @@ get_prio (struct path * pp)
 	}
 	if (!pp->getprio) {
 		pp->priority = 1;
+		pp->prio_valid = 1;
 	} else if (apply_format(pp->getprio, &buff[0], pp)) {
 		condlog(0, "error formatting prio callout command");
 		pp->priority = -1;
@@ -647,8 +648,10 @@ get_prio (struct path * pp)
 		condlog(0, "error calling out %s", buff);
 		pp->priority = -1;
 		return 1;
-	} else
+	} else {
 		pp->priority = atoi(prio);
+		pp->prio_valid = 1;
+	}
 
 	condlog(3, "%s: prio = %u", pp->dev, pp->priority);
 	return 0;
@@ -701,7 +704,12 @@ pathinfo (struct path *pp, vector hwtabl
 	if (mask & DI_CHECKER && get_state(pp))
 		goto blank;
 	
-	if (mask & DI_PRIO && pp->state != PATH_DOWN)
+
+	/*
+	 * Retrieve path priority for even PATH_DOWN paths if it has never
+	 * been successfully obtained before.
+	 */
+	if (mask & DI_PRIO && ((pp->state != PATH_DOWN) || !pp->prio_valid))
 		get_prio(pp);
 
 	if (mask & DI_WWID && !strlen(pp->wwid))

[-- Attachment #5: Type: text/plain, Size: 0 bytes --]

RE: patch to discovery.c to still get pathpriorityforpaths with path state of PATH_DOWN

[Dave Wysochanski]

On Wed, 2006-11-15 at 14:48 -0500, Edward Goggin wrote:
> On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote
> 
> > One other thought I had was the notion of a "priority valid" 
> > flag (or a
> > special priority value) in the path for the case of 
> > group_by_prio.  Set
> > it to invalid in alloc_path(), then just don't add the path to the
> > multipath struct in coalesce_paths() if the priority value 
> > was invalid.
> > Then over in checkerloop(), add the path to the multipath map when you
> > get a valid priority.
> > 
> > It is more complicated than that I'm sure (e.g. checkerloop() assumes
> > pp->mpp is non-null in places, etc) but seemed like a half-decent
> > approach to at least consider.
> > 
> > This approach doesn't take into consideration the general case of a
> > change in path priority though.
> 
> I very much like the first part of your "priority valid" idea
> mentioned above.
> 
> I've enclosed a patch for the first part of your idea.  The patch
> should address the concern you had about recalculating priority
> for a path when its path state changes from not PATH_DOWN to
> PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
> paths if the path's priority has never been successfully
> retrieved before.
> 
Great.  Your patch looks fine.

> I've punted for now on the reloading of the map if a path priority
> change has caused a path group membership change.  This is kind of
> complex, has lots of down sides, and there doesn't seem to be a
> need for it as of now.

Sure that's fine.  That would be extreme and I'm all for simplicity.

RE: patch to discovery.c to still get pathpriorityforpaths with path state of PATH_DOWN

[Christophe Varoqui]

[-- Attachment #1: Type: text/plain, Size: 1621 bytes --]

Le mercredi 15 novembre 2006 à 14:48 -0500, Edward Goggin a écrit :
> On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote
> 
> > One other thought I had was the notion of a "priority valid" 
> > flag (or a
> > special priority value) in the path for the case of 
> > group_by_prio.  Set
> > it to invalid in alloc_path(), then just don't add the path to the
> > multipath struct in coalesce_paths() if the priority value 
> > was invalid.
> > Then over in checkerloop(), add the path to the multipath map when you
> > get a valid priority.
> > 
> > It is more complicated than that I'm sure (e.g. checkerloop() assumes
> > pp->mpp is non-null in places, etc) but seemed like a half-decent
> > approach to at least consider.
> > 
> > This approach doesn't take into consideration the general case of a
> > change in path priority though.
> 
> I very much like the first part of your "priority valid" idea
> mentioned above.
> 
> I've enclosed a patch for the first part of your idea.  The patch
> should address the concern you had about recalculating priority
> for a path when its path state changes from not PATH_DOWN to
> PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
> paths if the path's priority has never been successfully
> retrieved before.
> 
How about the following, clarifying PRIO values and avoiding the
additional "struct path" element ?

Please have a careful look at the multipath/main.c:update_paths change,
because the (!pp->priority) that was there awfully looks like a bug (as
0 is not a defined prio value).

Regards,
cvaroqui



[-- Attachment #2: 0010-prio-defines.diff --]
[-- Type: text/x-patch, Size: 3043 bytes --]

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index b66cf23..9d17022 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -441,7 +441,7 @@ coalesce_paths (struct vectors * vecs, v
 		if ((mpp = add_map_with_path(vecs, pp1, 0)) == NULL)
 			return 1;
 
-		if (pp1->priority < 0)
+		if (pp1->priority == PRIO_UNDEF)
 			mpp->action = ACT_REJECT;
 
 		if (!mpp->paths) {
@@ -468,7 +468,7 @@ coalesce_paths (struct vectors * vecs, v
 					mpp->size);
 				mpp->action = ACT_REJECT;
 			}
-			if (pp2->priority < 0)
+			if (pp2->priority == PRIO_UNDEF)
 				mpp->action = ACT_REJECT;
 		}
 		verify_paths(mpp, vecs, NULL);
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 69c9bc3..f21e5bc 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -638,14 +638,14 @@ get_prio (struct path * pp)
 		pp->getprio_selected = 1;
 	}
 	if (!pp->getprio) {
-		pp->priority = 1;
+		pp->priority = PRIO_DEFAULT;
 	} else if (apply_format(pp->getprio, &buff[0], pp)) {
 		condlog(0, "error formatting prio callout command");
-		pp->priority = -1;
+		pp->priority = PRIO_UNDEF;
 		return 1;
 	} else if (execute_program(buff, prio, 16)) {
 		condlog(0, "error calling out %s", buff);
-		pp->priority = -1;
+		pp->priority = PRIO_UNDEF;
 		return 1;
 	} else
 		pp->priority = atoi(prio);
@@ -701,13 +701,12 @@ pathinfo (struct path *pp, vector hwtabl
 	if (mask & DI_CHECKER && get_state(pp))
 		goto blank;
 	
-	/*
-	 * Path state of PATH_DOWN does not necessarily prevent
-	 * path priority callout (or getuid callouot) from
-	 * succeeding since the path may be being considered
-	 * failed for reasons other than transport connectivity.
-	 */
-	if (mask & DI_PRIO)
+	 /*
+	  * Retrieve path priority for even PATH_DOWN paths if it has never
+	  * been successfully obtained before.
+	  */
+	if (mask & DI_PRIO &&
+	    (pp->state != PATH_DOWN || pp->priority != PRIO_UNDEF))
 		get_prio(pp);
 
 	if (mask & DI_WWID && !strlen(pp->wwid))
diff --git a/libmultipath/structs.c b/libmultipath/structs.c
index f0564c2..db3f824 100644
--- a/libmultipath/structs.c
+++ b/libmultipath/structs.c
@@ -31,6 +31,7 @@ alloc_path (void)
 		pp->sg_id.scsi_id = -1;
 		pp->sg_id.lun = -1;
 		pp->fd = -1;
+		pp->priority = PRIO_UNDEF;
 	}
 	return pp;
 }
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index faa2b8f..7db7faa 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -18,6 +18,9 @@ #define NO_PATH_RETRY_UNDEF	0
 #define NO_PATH_RETRY_FAIL	-1
 #define NO_PATH_RETRY_QUEUE	-2
 
+#define PRIO_UNDEF		-1
+#define PRIO_DEFAULT		1
+
 enum free_path_switch {
 	KEEP_PATHS,
 	FREE_PATHS
diff --git a/multipath/main.c b/multipath/main.c
index a5f122e..accb230 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -134,7 +134,7 @@ update_paths (struct multipath * mpp)
 			if (pp->state == PATH_UNCHECKED)
 				pathinfo(pp, conf->hwtable, DI_CHECKER);
 
-			if (!pp->priority)
+			if (pp->priority == PRIO_UNDEF)
 				pathinfo(pp, conf->hwtable, DI_PRIO);
 		}
 	}

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

RE: patch to discovery.c to still get pathpriorityforpaths with path state of PATH_DOWN

[David Wysochanski]

On Wed, 2006-11-15 at 23:33 +0100, Christophe Varoqui wrote:
> Le mercredi 15 novembre 2006 à 14:48 -0500, Edward Goggin a écrit :
> > On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote
> > 
> > > One other thought I had was the notion of a "priority valid" 
> > > flag (or a
> > > special priority value) in the path for the case of 
> > > group_by_prio.  Set
> > > it to invalid in alloc_path(), then just don't add the path to the
> > > multipath struct in coalesce_paths() if the priority value 
> > > was invalid.
> > > Then over in checkerloop(), add the path to the multipath map when you
> > > get a valid priority.
> > > 
> > > It is more complicated than that I'm sure (e.g. checkerloop() assumes
> > > pp->mpp is non-null in places, etc) but seemed like a half-decent
> > > approach to at least consider.
> > > 
> > > This approach doesn't take into consideration the general case of a
> > > change in path priority though.
> > 
> > I very much like the first part of your "priority valid" idea
> > mentioned above.
> > 
> > I've enclosed a patch for the first part of your idea.  The patch
> > should address the concern you had about recalculating priority
> > for a path when its path state changes from not PATH_DOWN to
> > PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
> > paths if the path's priority has never been successfully
> > retrieved before.
> > 
> How about the following, clarifying PRIO values and avoiding the
> additional "struct path" element ?
> 
> Please have a careful look at the multipath/main.c:update_paths change,
> because the (!pp->priority) that was there awfully looks like a bug (as
> 0 is not a defined prio value).
> 

0 is the value you'll get though after alloc_path and before anyone
calls pathinfo right?

Your patch should be equivalent though and is clearer.


> Regards,
> cvaroqui
> 
> 

RE: patch to discovery.c to still get pathpriorityforpaths with path state of PATH_DOWN

[Christophe Varoqui]

> > Please have a careful look at the multipath/main.c:update_paths change,
> > because the (!pp->priority) that was there awfully looks like a bug (as
> > 0 is not a defined prio value).
> > 
> 
> 0 is the value you'll get though after alloc_path and before anyone
> calls pathinfo right?
> 
You're right. I forgot the memset(0...) through MALLOC().

> Your patch should be equivalent though and is clearer.
> 
Then merged.
Now that this issue is cleared, I'll push the head to korg.

Regards,
cvaroqui

RE: patch to discovery.c to still getpathpriorityforpaths with path state of PATH_DOWN

[Edward Goggin]

OK.  I was trying not to have to introduce another
prio state because that approach just seems prone
to error.

I'll give it a try tomorrow.

> -----Original Message-----
> From: Christophe Varoqui [mailto:christophe.varoqui@free.fr] 
> Sent: Wednesday, November 15, 2006 5:34 PM
> To: goggin, edward
> Cc: dave.wysochanski@redhat.com; dm-devel@redhat.com
> Subject: RE: [dm-devel] patch to discovery.c to still 
> getpathpriorityforpaths with path state of PATH_DOWN
> 
> Le mercredi 15 novembre 2006 à 14:48 -0500, Edward Goggin a écrit :
> > On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote
> > 
> > > One other thought I had was the notion of a "priority valid" 
> > > flag (or a
> > > special priority value) in the path for the case of 
> > > group_by_prio.  Set
> > > it to invalid in alloc_path(), then just don't add the path to the
> > > multipath struct in coalesce_paths() if the priority value 
> > > was invalid.
> > > Then over in checkerloop(), add the path to the multipath 
> map when you
> > > get a valid priority.
> > > 
> > > It is more complicated than that I'm sure (e.g. 
> checkerloop() assumes
> > > pp->mpp is non-null in places, etc) but seemed like a half-decent
> > > approach to at least consider.
> > > 
> > > This approach doesn't take into consideration the general 
> case of a
> > > change in path priority though.
> > 
> > I very much like the first part of your "priority valid" idea
> > mentioned above.
> > 
> > I've enclosed a patch for the first part of your idea.  The patch
> > should address the concern you had about recalculating priority
> > for a path when its path state changes from not PATH_DOWN to
> > PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
> > paths if the path's priority has never been successfully
> > retrieved before.
> > 
> How about the following, clarifying PRIO values and avoiding the
> additional "struct path" element ?
> 
> Please have a careful look at the 
> multipath/main.c:update_paths change,
> because the (!pp->priority) that was there awfully looks like 
> a bug (as
> 0 is not a defined prio value).
> 
> Regards,
> cvaroqui
> 
> 
> 

RE: patch to discovery.c to still getpathpriorityforpaths with path state of PATH_DOWN

[Christophe Varoqui]

Le mercredi 15 novembre 2006 à 17:48 -0500, Edward Goggin a écrit :
> OK.  I was trying not to have to introduce another
> prio state because that approach just seems prone
> to error.
> 
Actually, the patch remove one (0) :)

> I'll give it a try tomorrow.
> 
Thanks.

RE: patch to discovery.c to still getpathpriorityforpaths with path state of PATH_DOWN

[Dave Wysochanski]

On Wed, 2006-11-15 at 17:48 -0500, Edward Goggin wrote:
> OK.  I was trying not to have to introduce another
> prio state because that approach just seems prone
> to error.
> 
> I'll give it a try tomorrow.
> 

Ed has a point.

An effect of using the special value approach over the flag is a
potential conflict with the callouts using the same values.  The flag
approach isolates his initialization case better.


> > -----Original Message-----
> > From: Christophe Varoqui [mailto:christophe.varoqui@free.fr] 
> > Sent: Wednesday, November 15, 2006 5:34 PM
> > To: goggin, edward
> > Cc: dave.wysochanski@redhat.com; dm-devel@redhat.com
> > Subject: RE: [dm-devel] patch to discovery.c to still 
> > getpathpriorityforpaths with path state of PATH_DOWN
> > 
> > Le mercredi 15 novembre 2006 à 14:48 -0500, Edward Goggin a écrit :
> > > On Wednesday, November 15, 2006 1:49 AM, Dave Wysochanski wrote
> > > 
> > > > One other thought I had was the notion of a "priority valid" 
> > > > flag (or a
> > > > special priority value) in the path for the case of 
> > > > group_by_prio.  Set
> > > > it to invalid in alloc_path(), then just don't add the path to the
> > > > multipath struct in coalesce_paths() if the priority value 
> > > > was invalid.
> > > > Then over in checkerloop(), add the path to the multipath 
> > map when you
> > > > get a valid priority.
> > > > 
> > > > It is more complicated than that I'm sure (e.g. 
> > checkerloop() assumes
> > > > pp->mpp is non-null in places, etc) but seemed like a half-decent
> > > > approach to at least consider.
> > > > 
> > > > This approach doesn't take into consideration the general 
> > case of a
> > > > change in path priority though.
> > > 
> > > I very much like the first part of your "priority valid" idea
> > > mentioned above.
> > > 
> > > I've enclosed a patch for the first part of your idea.  The patch
> > > should address the concern you had about recalculating priority
> > > for a path when its path state changes from not PATH_DOWN to
> > > PATH_DOWN.  It now only retrieves the priority for PATH_DOWN
> > > paths if the path's priority has never been successfully
> > > retrieved before.
> > > 
> > How about the following, clarifying PRIO values and avoiding the
> > additional "struct path" element ?
> > 
> > Please have a careful look at the 
> > multipath/main.c:update_paths change,
> > because the (!pp->priority) that was there awfully looks like 
> > a bug (as
> > 0 is not a defined prio value).
> > 
> > Regards,
> > cvaroqui
> > 
> > 
> > 

RE: patch to discovery.c to stillgetpathpriorityforpaths with path state of PATH_DOWN

[Edward Goggin]

On Wednesday, November 15, 2006 6:10 PM, Dave Wysochanski wrote:

> On Wed, 2006-11-15 at 17:48 -0500, Edward Goggin wrote:
> > OK.  I was trying not to have to introduce another
> > prio state because that approach just seems prone
> > to error.
> > 
> > I'll give it a try tomorrow.
> > 
> 
> Ed has a point.
> 
> An effect of using the special value approach over the flag is a
> potential conflict with the callouts using the same values.  The flag
> approach isolates his initialization case better.

That's OK I think.  I like this approach better since it seems cleaner
as long as the callouts stay away from PRIO_UNDEF literal (-1).

I tested with inactive snapshots and just have one minor (but important)
adjustment to what is in source control at the moment.

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index f21e5bc..6f2059a 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -706,7 +706,7 @@ pathinfo (struct path *pp, vector hwtabl
 	  * been successfully obtained before.
 	  */
 	if (mask & DI_PRIO &&
-	    (pp->state != PATH_DOWN || pp->priority != PRIO_UNDEF))
+	    (pp->state != PATH_DOWN || pp->priority == PRIO_UNDEF))
 		get_prio(pp);
 
 	if (mask & DI_WWID && !strlen(pp->wwid))

RE: patch to discovery.c to stillgetpathpriorityforpaths with path state of PATH_DOWN

[Christophe Varoqui]

> 
> I tested with inactive snapshots and just have one minor (but important)
> adjustment to what is in source control at the moment.
> 
Shame on me.
Applied.