NVMe path failover update

Bring the output of NVMe discovery inline with the other transports, and set the sg_id mappings to avoid ugly '#' outputs.

Signed-off-by Hannes Reinecke <[email protected]>

---

Quoted 6 lines · Hannes Reinecke

Bring the output of NVMe discovery inline with the other transports, and set the sg_id mappings to avoid ugly '#' outputs.

Signed-off-by Hannes Reinecke <[email protected]>

---

libmultipath/discovery.c scsi_sysfs_pathinfo (struct path * pp, vector hwtable) · line 1210

Quoted 9 lines

libmultipath/discovery.c +7 -2

-	}
-	if (!nvme)
+	if (sscanf(attr_path, "nvme%dn%d",
+		   &pp->sg_id.host_no,
+		   &pp->sg_id.scsi_id) != 2)
+		return 1;
+	pp->sg_id.channel = 0;
+	pp->sg_id.lun = 0;
+

As seen in PATCH0/4, it's better to use nvme_id fields which shows the true topology of NVMe/NVMf storage something like this: host NQN ->portid->subsytem NQN -> controller ID ->namespace id .
The NQN is too long may be we could select the last few bytes.

However, your current method looks fine. And some suggestions:

(1)How about assigning sg_id.channel to controller id of target (seen in the /sys/class/nvme/nvme0/nvme0n1/device/cntlid)
(2)And assigning sg_id.lun to namespace ID of target(seen in the :/sys/class/nvme/nvme0/nvme0n1/nsid)

The NVMe path states 'resetting' and 'reconnecting' indicate that the controller could not talk to the namespace, which translates into a path down state, not a path pending state.
Path pending should only be used for short lived intermediate states like 'new' or 'deleting'.
I agree with the statements --Path pending should only be used for short

Signed-off-by Hannes Reinecke <[email protected]>

---
 libmultipath/discovery.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

libmultipath/discovery.c +1 -2

index 95c138b..a880f4b 100644
@@ -1449,8 +1449,7 @@ path_offline (struct path * pp)
 		}
 		pp->offline = 0;
 		if (!strncmp(buff, "new", 3) ||
-		    !strncmp(buff, "reconnecting", 12) ||
-		    !strncmp(buff, "resetting", 9))
+		    !strncmp(buff, "deleting", 8))
 			return PATH_PENDING;
 		else if (!strncmp(buff, "live", 4))
 			return PATH_UP;

Quoted 5 lines · Hannes Reinecke

The NVMe path states 'resetting' and 'reconnecting' indicate that the controller could not talk to the namespace, which translates into a path down state, not a path pending state.
Path pending should only be used for short lived intermediate states like 'new' or 'deleting'.

I agree with the statements --Path pending should only be used for short lived intermediate states. But it's better to schedule path state checking so soon as possible in the next second for "resetting" and "reconnecting" states because those two states is transient in a short time.
How about introducing a new state for multipath: PATH_TRANSITING to distinguish PATH_PENDING?

For NVMe we don't have a good way of checking the path state, so add a new checker 'none' which doesn't do any checking, but rely on the internal sysfs state for path checking.

Signed-off-by Hannes Reinecke <[email protected]>

---
 libmultipath/checkers.c    | 22 ++++++++++++++++------
 libmultipath/checkers.h    |  3 ++-
 libmultipath/discovery.c   | 10 ++++++----
 libmultipath/discovery.h   |  2 +-
 multipath/multipath.conf.5 |  3 +++
 multipathd/main.c          |  2 +-
 6 files changed, 29 insertions(+), 13 deletions(-)

libmultipath/checkers.c +16 -6

index c32f727..cd6d6a3 100644
@@ -19,6 +19,7 @@ char *checker_state_names[] = {
 	"timeout",
 	"removed",
 	"delayed",
+	"none",
 };
 
 static LIST_HEAD(checkers);

@@ -102,6 +103,8 @@ struct checker * add_checker (char *multipath_dir, char * name)
 	if (!c)
 		return NULL;
 	snprintf(c->name, CHECKER_NAME_LEN, "%s", name);
+	if (!strncmp(c->name, NONE, 4))
+		goto done;
 	snprintf(libname, LIB_CHECKER_NAMELEN, "%s/libcheck%s.so",
 		 multipath_dir, name);
 	if (stat(libname,&stbuf) < 0) {

@@ -145,7 +148,7 @@ struct checker * add_checker (char *multipath_dir, char * name)
 		condlog(0, "A dynamic linking error occurred: (%s)", errstr);
 	if (!c->repair)
 		goto out;
-
+done:
 	c->fd = -1;
 	c->sync = 1;
 	list_add(&c->node, &checkers);

@@ -195,14 +198,16 @@ int checker_init (struct checker * c, void ** mpctxt_addr)
 	if (!c)
 		return 1;
 	c->mpcontext = mpctxt_addr;
-	return c->init(c);
+	if (c->init)
+		return c->init(c);
+	return 0;
 }
 
 void checker_put (struct checker * dst)
 {
 	struct checker * src;
 
-	if (!dst || !dst->check)
+	if (!dst || !strlen(dst->name))
 		return;
 	src = checker_lookup(dst->name);
 	if (dst->free)

@@ -221,11 +226,11 @@ void checker_repair (struct checker * c)
 		MSG(c, "checker disabled");
 		return;
 	}
-
-	c->repair(c);
+	if (c->repair)
+		c->repair(c);
 }
 
-int checker_check (struct checker * c)
+int checker_check (struct checker * c, int path_state)
 {
 	int r;
 

@@ -237,6 +242,9 @@ int checker_check (struct checker * c)
 		MSG(c, "checker disabled");
 		return PATH_UNCHECKED;
 	}
+	if (!strncmp(c->name, NONE, 4))
+		return path_state;
+
 	if (c->fd < 0) {
 		MSG(c, "no usable fd");
 		return PATH_WILD;

@@ -250,6 +258,8 @@ int checker_selected (struct checker * c)
 {
 	if (!c)
 		return 0;
+	if (!strncmp(c->name, NONE, 4))
+		return 1;
 	return (c->check) ? 1 : 0;
 }
 

libmultipath/checkers.h +2 -1

index 1d225de..713399f 100644
@@ -85,6 +85,7 @@ enum path_check_state {
 #define EMC_CLARIION "emc_clariion"
 #define READSECTOR0  "readsector0"
 #define CCISS_TUR    "cciss_tur"
+#define NONE         "none"
 #define RBD          "rbd"
 
 #define ASYNC_TIMEOUT_SEC	30

@@ -135,7 +136,7 @@ void checker_set_fd (struct checker *, int);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
 void checker_repair (struct checker *);
-int checker_check (struct checker *);
+int checker_check (struct checker *, int);
 int checker_selected (struct checker *);
 char * checker_name (struct checker *);
 char * checker_message (struct checker *);

libmultipath/discovery.c +6 -4

index a880f4b..225fd4d 100644
@@ -1550,7 +1550,7 @@ cciss_ioctl_pathinfo (struct path * pp, int mask)
 }
 
 int
-get_state (struct path * pp, struct config *conf, int daemon)
+get_state (struct path * pp, struct config *conf, int daemon, int oldstate)
 {
 	struct checker * c = &pp->checker;
 	int state;

@@ -1588,8 +1588,9 @@ get_state (struct path * pp, struct config *conf, int daemon)
 	if (!conf->checker_timeout &&
 	    sysfs_get_timeout(pp, &(c->timeout)) <= 0)
 		c->timeout = DEF_TIMEOUT;
-	state = checker_check(c);
-	condlog(3, "%s: state = %s", pp->dev, checker_state_name(state));
+	state = checker_check(c, oldstate);
+	condlog(3, "%s: %s state = %s", pp->dev,
+		checker_name(c), checker_state_name(state));
 	if (state != PATH_UP && state != PATH_GHOST &&
 	    strlen(checker_message(c)))
 		condlog(3, "%s: checker msg is \"%s\"",

@@ -1960,7 +1961,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 
 	if (mask & DI_CHECKER) {
 		if (path_state == PATH_UP) {
-			pp->chkrstate = pp->state = get_state(pp, conf, 0);
+			pp->chkrstate = pp->state = get_state(pp, conf, 0,
+							      path_state);
 			if (pp->state == PATH_UNCHECKED ||
 			    pp->state == PATH_WILD)
 				goto blank;

libmultipath/discovery.h +1 -1

index 51c23d6..6a54b78 100644
@@ -34,7 +34,7 @@ int path_discovery (vector pathvec, int flag);
 
 int do_tur (char *);
 int path_offline (struct path *);
-int get_state (struct path * pp, struct config * conf, int daemon);
+int get_state (struct path * pp, struct config * conf, int daemon, int state);
 int get_vpd_sgio (int fd, int pg, char * str, int maxlen);
 int pathinfo (struct path * pp, struct config * conf, int mask);
 int alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,

multipath/multipath.conf.5 +3 -0

index d9ac279..e0cada3 100644
@@ -451,6 +451,9 @@ Please use \fItur\fR instead.
 (Hardware-dependent)
 Check the path state for HP/COMPAQ Smart Array(CCISS) controllers.
 .TP
+.I none
+Do not check the device, fallback to use the values retrieved from sysfs
+.TP
 .I rbd
 Check if the path is in the Ceph blacklist and remap the path if it is.
 .TP

multipathd/main.c +1 -1

index 4be2c57..657ff83 100644
@@ -1623,7 +1623,7 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
 
 	if (newstate == PATH_UP) {
 		conf = get_multipath_config();
-		newstate = get_state(pp, conf, 1);
+		newstate = get_state(pp, conf, 1, newstate);
 		put_multipath_config(conf);
 	} else
 		checker_clear_message(&pp->checker);

We shouldn't be using the directio checker for NVMe, as this might cause spurious path failures under high load.
As the 'state' attribute provides reliable information about the link status it's sufficient to just check this and don't call any path checkers.

Signed-off-by Hannes Reinecke <[email protected]>

---
 libmultipath/hwtable.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

libmultipath/hwtable.c +1 -1

index 9e14ec1..9ac09ff 100644
@@ -1113,7 +1113,7 @@ static struct hwentry default_hw[] = {
 		.vendor        = "NVME",
 		.product       = ".*",
 		.uid_attribute = "ID_WWN",
-		.checker_name  = DIRECTIO,
+		.checker_name  = NONE,
 		.retain_hwhandler = RETAIN_HWHANDLER_OFF,
 	},
 	/*

Hi all,

the current NVMe support is fairly rudimentary, and we've had reports about spurious path failures under high load.
Which is a bit disappointing, seeing that high load is exactly what NVMe is about ...
So here's a patchset to clean up the situation somewhat.
In particular:
- I've fixed up the discovery to fall in line with the other transports, like filling out the sg_id fields (so that we're not greeted with weird hashes in the topology output)