Make ib_srp better suited for H.A. purposes

This patch series makes the ib_srp driver better suited for use in a H.A. setup because:
- Switchover without triggering read or write errors become possible. Such errors are bad because these can make a filesystem switch to read-only mode.
- A ping mechanism has been added that allows to reduce the switch-over time greatly.
- Disconnecting from a target without unloading ib_srp becomes possible.
- Switchover can be triggered explicitly by deleting an initiator device.

Where possible these mechanisms have been modelled after similar functionality in the open-iscsi kernel code.

According to the measurements I ran these patches do not affect ib_srp performance.

The individual patches are:
0001-ib_srp-Introduce-pr_fmt.patch 0002-ib_srp-Consolidate-repetitive-sysfs-code.patch 0003-ib_srp-Disallow-duplicate-logins.patch 0004-ib_srp-Set-block-layer-timeout.patch 0005-ib_srp-Avoid-that-late-SRP-replies-cause-trouble.patch 0006-ib_srp-Micro-optimize-completion-handlers.patch 0007-ib_srp-Introduce-srp_handle_qp_err.patch 0008-srp_transport-Document-sysfs-attributes.patch 0009-srp_transport-Fix-atttribute-registration-race.patch 0010-srp_transport-Simplify-attribute-initialization-code.patch 0011-ib_srp-Document-sysfs-attributes.patch 0012-ib_srp-Rework-error-handling.patch 0013-ib_srp-Implement-transport-layer-ping.patch 0014-ib_srp-Allow-SRP-disconnect-through-sysfs.patch
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Make the logging code more a little more brief by replacing printk(KERN_WARNING PFX ...) by pr_warn(...) and by replacing printk(KERN_ERR PFX ...) by pr_err(...). Remove one trailing space to avoid a checkpatch warning.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   40 ++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 19 deletions(-)

index 0bfa545..5093cc6 100644
@@ -30,6 +30,8 @@
  * SOFTWARE.
  */
 
+#define pr_fmt(fmt) PFX fmt
+
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>

@@ -165,7 +167,7 @@ static void srp_free_iu(struct srp_host *host, struct srp_iu *iu)
 
 static void srp_qp_event(struct ib_event *event, void *context)
 {
-	printk(KERN_ERR PFX "QP event %d\n", event->event);
+	pr_err("QP event %d\n", event->event);
 }
 
 static int srp_init_qp(struct srp_target_port *target,

@@ -1989,7 +1991,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 				goto out;
 			}
 			if (strlen(p) != 32) {
-				printk(KERN_WARNING PFX "bad dest GID parameter '%s'\n", p);
+				pr_warn("bad dest GID parameter '%s'\n", p);
 				kfree(p);
 				goto out;
 			}

@@ -2004,7 +2006,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_PKEY:
 			if (match_hex(args, &token)) {
-				printk(KERN_WARNING PFX "bad P_Key parameter '%s'\n", p);
+				pr_warn("bad P_Key parameter '%s'\n", p);
 				goto out;
 			}
 			target->path.pkey = cpu_to_be16(token);

@@ -2023,7 +2025,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_MAX_SECT:
 			if (match_int(args, &token)) {
-				printk(KERN_WARNING PFX "bad max sect parameter '%s'\n", p);
+				pr_warn("bad max sect parameter '%s'\n", p);
 				goto out;
 			}
 			target->scsi_host->max_sectors = token;

@@ -2031,7 +2033,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_MAX_CMD_PER_LUN:
 			if (match_int(args, &token)) {
-				printk(KERN_WARNING PFX "bad max cmd_per_lun parameter '%s'\n", p);
+				pr_warn("bad max cmd_per_lun parameter '%s'\n", p);
 				goto out;
 			}
 			target->scsi_host->cmd_per_lun = min(token, SRP_CMD_SQ_SIZE);

@@ -2039,12 +2041,12 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_IO_CLASS:
 			if (match_hex(args, &token)) {
-				printk(KERN_WARNING PFX "bad  IO class parameter '%s' \n", p);
+				pr_warn("bad  IO class parameter '%s'\n", p);
 				goto out;
 			}
 			if (token != SRP_REV10_IB_IO_CLASS &&
 			    token != SRP_REV16A_IB_IO_CLASS) {
-				printk(KERN_WARNING PFX "unknown IO class parameter value"
+				pr_warn("unknown IO class parameter value"
 				       " %x specified (use %x or %x).\n",
 				       token, SRP_REV10_IB_IO_CLASS, SRP_REV16A_IB_IO_CLASS);
 				goto out;

@@ -2064,7 +2066,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_CMD_SG_ENTRIES:
 			if (match_int(args, &token) || token < 1 || token > 255) {
-				printk(KERN_WARNING PFX "bad max cmd_sg_entries parameter '%s'\n", p);
+				pr_warn("bad max cmd_sg_entries parameter '%s'\n", p);
 				goto out;
 			}
 			target->cmd_sg_cnt = token;

@@ -2072,7 +2074,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 
 		case SRP_OPT_ALLOW_EXT_SG:
 			if (match_int(args, &token)) {
-				printk(KERN_WARNING PFX "bad allow_ext_sg parameter '%s'\n", p);
+				pr_warn("bad allow_ext_sg parameter '%s'\n", p);
 				goto out;
 			}
 			target->allow_ext_sg = !!token;

@@ -2081,14 +2083,14 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 		case SRP_OPT_SG_TABLESIZE:
 			if (match_int(args, &token) || token < 1 ||
 					token > SCSI_MAX_SG_CHAIN_SEGMENTS) {
-				printk(KERN_WARNING PFX "bad max sg_tablesize parameter '%s'\n", p);
+				pr_warn("bad max sg_tablesize parameter '%s'\n", p);
 				goto out;
 			}
 			target->sg_tablesize = token;
 			break;
 
 		default:
-			printk(KERN_WARNING PFX "unknown parameter or missing value "
+			pr_warn("unknown parameter or missing value "
 			       "'%s' in target creation request\n", p);
 			goto out;
 		}

@@ -2100,7 +2102,7 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
 		for (i = 0; i < ARRAY_SIZE(srp_opt_tokens); ++i)
 			if ((srp_opt_tokens[i].token & SRP_OPT_ALL) &&
 			    !(srp_opt_tokens[i].token & opt_mask))
-				printk(KERN_WARNING PFX "target creation request is "
+				pr_warn("target creation request is "
 				       "missing parameter '%s'\n",
 				       srp_opt_tokens[i].pattern);
 

@@ -2149,7 +2151,7 @@ static ssize_t srp_create_target(struct device *dev,
 
 	if (!host->srp_dev->fmr_pool && !target->allow_ext_sg &&
 				target->cmd_sg_cnt < target->sg_tablesize) {
-		printk(KERN_WARNING PFX "No FMR pool and no external indirect descriptors, limiting sg_tablesize to cmd_sg_cnt\n");
+		pr_warn("No FMR pool and no external indirect descriptors, limiting sg_tablesize to cmd_sg_cnt\n");
 		target->sg_tablesize = target->cmd_sg_cnt;
 	}
 

@@ -2309,7 +2311,7 @@ static void srp_add_one(struct ib_device *device)
 		return;
 
 	if (ib_query_device(device, dev_attr)) {
-		printk(KERN_WARNING PFX "Query device failed for %s\n",
+		pr_warn("Query device failed for %s\n",
 		       device->name);
 		goto free_attr;
 	}

@@ -2459,7 +2461,7 @@ static int __init srp_init_module(void)
 	BUILD_BUG_ON(FIELD_SIZEOF(struct ib_wc, wr_id) < sizeof(void *));
 
 	if (srp_sg_tablesize) {
-		printk(KERN_WARNING PFX "srp_sg_tablesize is deprecated, please use cmd_sg_entries\n");
+		pr_warn("srp_sg_tablesize is deprecated, please use cmd_sg_entries\n");
 		if (!cmd_sg_entries)
 			cmd_sg_entries = srp_sg_tablesize;
 	}

@@ -2468,14 +2470,14 @@ static int __init srp_init_module(void)
 		cmd_sg_entries = SRP_DEF_SG_TABLESIZE;
 
 	if (cmd_sg_entries > 255) {
-		printk(KERN_WARNING PFX "Clamping cmd_sg_entries to 255\n");
+		pr_warn("Clamping cmd_sg_entries to 255\n");
 		cmd_sg_entries = 255;
 	}
 
 	if (!indirect_sg_entries)
 		indirect_sg_entries = cmd_sg_entries;
 	else if (indirect_sg_entries < cmd_sg_entries) {
-		printk(KERN_WARNING PFX "Bumping up indirect_sg_entries to match cmd_sg_entries (%u)\n", cmd_sg_entries);
+		pr_warn("Bumping up indirect_sg_entries to match cmd_sg_entries (%u)\n", cmd_sg_entries);
 		indirect_sg_entries = cmd_sg_entries;
 	}
 

@@ -2486,7 +2488,7 @@ static int __init srp_init_module(void)
 
 	ret = class_register(&srp_class);
 	if (ret) {
-		printk(KERN_ERR PFX "couldn't register class infiniband_srp\n");
+		pr_err("couldn't register class infiniband_srp\n");
 		srp_release_transport(ib_srp_transport_template);
 		return ret;
 	}

@@ -2495,7 +2497,7 @@ static int __init srp_init_module(void)
 
 	ret = ib_register_client(&srp_client);
 	if (ret) {
-		printk(KERN_ERR PFX "couldn't register IB client\n");
+		pr_err("couldn't register IB client\n");
 		srp_release_transport(ib_srp_transport_template);
 		ib_sa_unregister_client(&srp_sa_client);
 		class_unregister(&srp_class);

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This is a good step; three minor nits:
1) Please go ahead and merge the multiline strings into one, even though checkpatch will complain about the line length. I'm not a huge fan of the long lines either, but Linus has clearly stated his preference for being able to grep the string in the message
2) Please re-align the variables on wrapped lines to just after the (; below the work token should be pushed over by one space.
3) All the commit message needs to really say is 'Using pr_* is preferred over printk.' Probably can just be the short summary, and no commit body needed.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Remove sysfs attributes before removing a target instead of testing the target state in every sysfs attribute callback method. Note: it is safe to invoke a sysfs attribute removal method like device_remove_file() twice on the same attribute.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   49 ++++++++++++----------------------
 1 files changed, 17 insertions(+), 32 deletions(-)

index 5093cc6..868e7f8 100644
@@ -474,6 +474,21 @@ static void srp_free_req_data(struct srp_target_port *target)
 	}
 }
 
+/**
+ * srp_del_scsi_host_attr() - Remove attributes defined in the host template.
+ * @shost: SCSI host whose attributes to remove from sysfs.
+ *
+ * Note: Any attributes defined in the host template and that did not exist
+ * before invocation of this function will be ignored.
+ */
+static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
+{
+	struct device_attribute **attr;
+
+	for (attr = shost->hostt->shost_attrs; attr && *attr; ++attr)
+		device_remove_file(&shost->shost_dev, *attr);
+}
+
 static void srp_remove_work(struct work_struct *work)
 {
 	struct srp_target_port *target =

@@ -486,6 +501,7 @@ static void srp_remove_work(struct work_struct *work)
 	list_del(&target->list);
 	spin_unlock(&target->srp_host->target_lock);
 
+	srp_del_scsi_host_attr(target->scsi_host);
 	srp_remove_host(target->scsi_host);
 	scsi_remove_host(target->scsi_host);
 	ib_destroy_cm_id(target->cm_id);

@@ -1678,10 +1694,6 @@ static ssize_t show_id_ext(struct device *dev, struct device_attribute *attr,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "0x%016llx\n",
 		       (unsigned long long) be64_to_cpu(target->id_ext));
 }

@@ -1691,10 +1703,6 @@ static ssize_t show_ioc_guid(struct device *dev, struct device_attribute *attr,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "0x%016llx\n",
 		       (unsigned long long) be64_to_cpu(target->ioc_guid));
 }

@@ -1704,10 +1712,6 @@ static ssize_t show_service_id(struct device *dev,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "0x%016llx\n",
 		       (unsigned long long) be64_to_cpu(target->service_id));
 }

@@ -1717,10 +1721,6 @@ static ssize_t show_pkey(struct device *dev, struct device_attribute *attr,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "0x%04x\n", be16_to_cpu(target->path.pkey));
 }
 

@@ -1729,10 +1729,6 @@ static ssize_t show_dgid(struct device *dev, struct device_attribute *attr,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "%pI6\n", target->path.dgid.raw);
 }
 

@@ -1741,10 +1737,6 @@ static ssize_t show_orig_dgid(struct device *dev,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "%pI6\n", target->orig_dgid);
 }
 

@@ -1753,10 +1745,6 @@ static ssize_t show_req_lim(struct device *dev,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "%d\n", target->req_lim);
 }
 

@@ -1765,10 +1753,6 @@ static ssize_t show_zero_req_lim(struct device *dev,
 {
 	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -ENODEV;
-
 	return sprintf(buf, "%d\n", target->zero_req_lim);
 }
 

@@ -2431,6 +2415,7 @@ static void srp_remove_one(struct ib_device *device)
 
 		list_for_each_entry_safe(target, tmp_target,
 					 &host->target_list, list) {
+			srp_del_scsi_host_attr(target->scsi_host);
 			srp_remove_host(target->scsi_host);
 			scsi_remove_host(target->scsi_host);
 			srp_disconnect_target(target);

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This is much cleaner, thanks.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Currently the ib_srp driver allows to log in multiple times to the same target via its sysfs interface. This leads to each target LUN being imported multiple times at the initiator side, something that should not be allowed.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

index 868e7f8..2e0169b 100644
@@ -2095,6 +2095,34 @@ out:
 	return ret;
 }
 
+/**
+ * srp_target_exists() - Check whether a given target already exists.
+ *
+ * Returns -EEXIST if the target already exists and 0 if not.
+ */
+static int srp_target_exists(struct srp_host *host,
+			     struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	int ret = 0;
+
+	spin_lock(&host->target_lock);
+	list_for_each_entry(t, &host->target_list, list) {
+		WARN_ON(t == target);
+		if (target->id_ext == t->id_ext &&
+		    target->ioc_guid == t->ioc_guid &&
+		    memcmp(target->orig_dgid, t->orig_dgid, 16) == 0 &&
+		    target->path.pkey == t->path.pkey &&
+		    target->service_id == t->service_id) {
+			ret = -EEXIST;
+			break;
+		}
+	}
+	spin_unlock(&host->target_lock);
+
+	return ret;
+}
+
 static ssize_t srp_create_target(struct device *dev,
 				 struct device_attribute *attr,
 				 const char *buf, size_t count)

@@ -2133,6 +2161,10 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err;
 
+	ret = srp_target_exists(host, target);
+	if (ret)
+		goto err;
+
 	if (!host->srp_dev->fmr_pool && !target->allow_ext_sg &&
 				target->cmd_sg_cnt < target->sg_tablesize) {
 		pr_warn("No FMR pool and no external indirect descriptors, limiting sg_tablesize to cmd_sg_cnt\n");

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm of mixed opinion on this one. It seems like we're working around a broken target -- we request single RDMA channel operation in our login requests, so the standard says that the target should disconnect all other connections for that I_T nexus when it receives a new login request. That should keep us from having multiple instances of LUNs from the same target over a single srp_host (HCA).

In the past, we've seen connections having issues but not going away, and the only way to kill them was to create a new connection to the RAID controller, which would then kill the old one. It's been a few years since I've seen this, so the details are a bit fuzzy, but I recall the current behavior being useful.

However, I'm not adverse to going beyond the standard if it makes sense, and your later patch to allow disconnecting channels without removing the module would fix the use case above. What problem are we solving here other than a potentially confused admin?

orig_dgid as above) or the service id? Those don't change the I_T nexus, so the target should disconnect previous channels appropriately.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I agree that relogin is a useful feature. But I doubt the way it works now is fine because during a certain time window all LUNs appear twice at the initiator side - half of them working, half of them not. Relogin is still possible with this patch set, but it requires an extra step: deleting a target before attempting to log in again.

I'm fine with leaving out the PKey and the service ID. But I'm not sure it's fine to leave the DGID out because leaving out the DGID will prevent setting up redundant connections from an initiator to a target with multiple active IB ports. The reason the orig_dgid is being used in the above comparison instead of dgid is because these two ID's may differ due to automatic path migration.

Does that mean that you'd like to see a pr_err() in addition to the error code returned via the sysfs write() call ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

According to the spec, they should only appear once -- the old connection should be disconnected before the new login request is accepted. Of course, the spec does not always match reality, though it has worked that way on the HW I've tested. But as I said, I'm fine with adding this to the initiator -- I just want good justification.

A SRP target port (for the purposes of an I_T nexus) is defined by the IOC GUID and extension; if you want to have redundant connections to a port, multi-channel logins must be supported. The way most targets avoid this issue is to present a different IOC on each IB port, though you should be also able to export multiple ServiceEntries in the IOControllerProfile response to allow different service extensions to be selected.

As for APM, I thought we had determined the last time it came up that implementing that would require protocol changes to SRP. I haven't gone back to refresh my memory -- and it isn't implemented yet either way, so we could go back and relax this check as part of the implementation.

Yes, similar to what we do for bad parameters in srp_parse_options(). Most scripts use echo to add a target, and only see succeed/fail; the admin will need a way to see what failed. The errno is useful to daemons using write(), though.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm considering to leave out this patch since deleting an SRP target port already provides a way to avoid that the initiator creates duplicate LUNs.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Make sure that the default block layer timeout is above the InfiniBand transport layer timeout. The default block layer timeout is 30 seconds. Typical values for the QP local ack timeout and retry count are 19 and 7 respectively, which means that it can take up to 60.1 seconds before a HCA submits an error completion. The block layer timeout must be larger than the transport layer timeout because otherwise it can happen that an SRP response is received after the SCSI layer has already killed a command.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   30 ++++++++++++++++++++++++++++++
 drivers/infiniband/ulp/srp/ib_srp.h |    1 +
 2 files changed, 31 insertions(+), 0 deletions(-)

index 2e0169b..0e2353d 100644
@@ -1372,6 +1372,17 @@ err:
 	return -ENOMEM;
 }
 
+static int srp_slave_alloc(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct srp_target_port *target = host_to_target(shost);
+
+	if (!WARN_ON(target->rq_tmo_jiffies == 0))
+		blk_queue_rq_timeout(sdev->request_queue,
+				     target->rq_tmo_jiffies);
+	return 0;
+}
+
 static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 			       struct srp_login_rsp *lrsp,
 			       struct srp_target_port *target)

@@ -1431,6 +1442,24 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 	if (ret)
 		goto error_free;
 
+	if (!WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) == 0)) {
+		uint64_t T_tr_ns, max_compl_time_ms;
+
+		/*
+		 * Set target->rq_tmo_jiffies to one second more than the
+		 * largest time it can take before an error completion is
+		 * generated.
+		 */
+		T_tr_ns = 1ULL << (12 + qp_attr->timeout);
+		max_compl_time_ms = qp_attr->retry_cnt * 4 * T_tr_ns;
+		do_div(max_compl_time_ms, 1000 * 1000);
+		target->rq_tmo_jiffies =
+			msecs_to_jiffies(max_compl_time_ms + 1000);
+		pr_debug("max. IB completion time = %lld ms; block layer"
+			 " timeout = %d jiffies\n", max_compl_time_ms,
+			 target->rq_tmo_jiffies);
+	}
+
 	ret = ib_modify_qp(target->qp, qp_attr, attr_mask);
 	if (ret)
 		goto error_free;

@@ -1821,6 +1850,7 @@ static struct scsi_host_template srp_template = {
 	.module				= THIS_MODULE,
 	.name				= "InfiniBand SRP initiator",
 	.proc_name			= DRV_NAME,
+	.slave_alloc			= srp_slave_alloc,
 	.info				= srp_target_info,
 	.queuecommand			= srp_queuecommand,
 	.eh_abort_handler		= srp_abort,

index 020caf0..f010bd9 100644
@@ -138,6 +138,7 @@ struct srp_target_port {
 	u32			lkey;
 	u32			rkey;
 	enum srp_target_state	state;
+	u32			rq_tmo_jiffies;
 	unsigned int		max_iu_len;
 	unsigned int		cmd_sg_cnt;
 	unsigned int		indirect_size;

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Good idea, but a few nits:

	if (attr_mask & (IP_QP_TIMEOUT | IB_QP_RETRY_CNT)) {

        /* Everything above this point is used in the hot path of
         * command processing. Try to keep them packed into cachelines.
         */

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

That WARN_ON() statement is there to verify that rq_tmo_jiffies has been initialized to a non-zero value - which is the case unless something bad like memory corruption has happened. I can leave it out if you want.

According to table 91 in the IB spec (section 11.2.4.2, Modify Queue Pair), both the QP timeout and the retry count have to be set for RC QP's upon the RTR to RTS transition.

All these magic numbers come straight from the IB spec. I can add a comment mentioning that if you want.

That debug statement was convenient to verify the result of the computation. But you are right, it's not really necessary. I'll leave it out.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Better to just set it to a nonzero value that is appropriate for the defaults and set it up when the CM handler gives you the reply. Sounds like 61 seconds is the correct value, from your original commit message. This way you know you have a sane value, even if the CM is broken, and the user doesn't get a confusing stack trace that doesn't really tell them what's wrong.

Ok, then safer to only change from the default set in srp_create_target() when
(attr_mask & (IP_QP_TIMEOUT|IB_OP_RETRY_CNT) == (IP_OP_TIMEOUT|IB_OP_RETRY_CNT))

Changing to something like

/* The maximum detection time for a lost ACK is 4 times the retry timeout,
* and we'll retry a set number of times before declaring the connection
* dead (IB spec C9-140, C9-141, C9-142). Set our default IO timeout to be
* one second longer to make sure we wait long enough before declaring a
* request dead.
*/
T_tr_ns = 4096 * (1ULL << qp_attr->timeout);
max_compl_time_ms = ...

may make it pretty obvious. Also, you could use USEC_PER_MSEC and MSEC_PER_SEC from <linux/time.h> to make the other numbers meaningful.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Could you provider a pointer to where this is defined?

isn't timeout(R,X) = R * ~4us * 2^X ==> timeout(7,19) = 7 * ~2seconds = ~14 seconds

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

C9-141 says we could take up to 4 times longer than 4us * 2^X to detect the ACK loss.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Sorry, it's not the block layer but sd (SCSI disk) that sets that timeout value. From drivers/scsi/sd.h: #define SD_TIMEOUT (30 * HZ)

I started looking at the timeout settings after I noticed while performing cable pulling tests with an mlx4-controlled HCA that it could take up to (about) 61 seconds before an error completion was delivered. And it looks like Dave already answered the question where the factor four comes from ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

If a user misconfigures the block layer timeout such that it is below the InfiniBand RC timeout it can happen that an SRP reply arrives after the SCSI error handler has already killed the associated SCSI command. Avoid that late replies cause a kernel crash.

---
 drivers/infiniband/ulp/srp/ib_srp.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

index 0e2353d..a2624bf 100644
@@ -1074,10 +1074,12 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 	} else {
 		req = &target->req_ring[rsp->tag];
 		scmnd = req->scmnd;
-		if (!scmnd)
+		if (unlikely(!scmnd)) {
 			shost_printk(KERN_ERR, target->scsi_host,
 				     "Null scmnd for RSP w/tag %016llx\n",
 				     (unsigned long long) rsp->tag);
+			return;
+		}
 		scmnd->result = rsp->status;
 
 		if (rsp->flags & SRP_RSP_FLAG_SNSVALID) {

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm not sure that the case you describe can happen. The request do not get killed until srp_remove_req() is called on them. In the event of timeouts, this will be through srp_abort() initially, and then srp_reset_device() and srp_reset_host(). If srp_abort() is successful, we will never see a reply for that request unless the target is broken. If it fails, a successful srp_reset_device() gives us the same assurance. And if that fails, we kill the connection in srp_reset_host(), so there is no possibility of a late reply.

However, the code as it currently is leaves us open to malformed requests from the target. We cannot do anything about it if the target is swapping tags around on the requests, but we should detect invalid tags and take appropriate action. Checking for a null req->scmnd and returning isn't the right action, as that leaks the request, and does nothing to protect against bad tags.

I'm happy to code up a patch that sanity-checks the response for in-range and outstanding (req->scmnd != NULL) responses, and kills the connection, or you are welcome to do so as well. I think it would be better suited later in the patch series, building on top of your work to disconnect a target without removing the module.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Help the CPU branch predictor by rearranging two if-statements such that the most likely code is in the if-part instead of the else-part.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

index a2624bf..e6d1aef 100644
@@ -1224,15 +1224,15 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 
 	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
 	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (wc.status) {
+		if (wc.status == IB_WC_SUCCESS) {
+			srp_handle_recv(target, &wc);
+		} else {
 			shost_printk(KERN_ERR, target->scsi_host,
 				     PFX "failed receive status %d\n",
 				     wc.status);
 			target->qp_in_error = 1;
 			break;
 		}
-
-		srp_handle_recv(target, &wc);
 	}
 }
 

@@ -1243,16 +1243,16 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 	struct srp_iu *iu;
 
 	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (wc.status) {
+		if (wc.status == IB_WC_SUCCESS) {
+			iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
+			list_add(&iu->list, &target->free_tx);
+		} else {
 			shost_printk(KERN_ERR, target->scsi_host,
 				     PFX "failed send status %d\n",
 				     wc.status);
 			target->qp_in_error = 1;
 			break;
 		}
-
-		iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
-		list_add(&iu->list, &target->free_tx);
 	}
 }
 

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

perhaps this should use the unlikely() macro instead:

	if (unlikely(wc.status)) {
		...
	}

	...

If it needs to change at all, this is the direction it should take.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Dave,

Prepatch 3.2-rc5 has been released which means that the 3.3 merge window is coming closer. Do you have any other comments about this patch series than those already posted to the list ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I do; I'm sorry to keep you waiting, but other pressures at work have kept me occupied. I'll have feedback for you this week, or will do it over the weekend if necessary.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Factor out common code into a new function.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

index e6d1aef..44c810b 100644
@@ -1217,6 +1217,15 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 			     PFX "Recv failed with error code %d\n", res);
 }
 
+static void srp_handle_qp_err(enum ib_wc_status wc_status,
+			      enum ib_wc_opcode wc_opcode,
+			      struct srp_target_port *target)
+{
+	shost_printk(KERN_ERR, target->scsi_host,
+		     PFX "failed receive status %d\n", wc_status);
+	target->qp_in_error = 1;
+}
+
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;

@@ -1227,10 +1236,7 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 		if (wc.status == IB_WC_SUCCESS) {
 			srp_handle_recv(target, &wc);
 		} else {
-			shost_printk(KERN_ERR, target->scsi_host,
-				     PFX "failed receive status %d\n",
-				     wc.status);
-			target->qp_in_error = 1;
+			srp_handle_qp_err(wc.status, wc.opcode, target);
 			break;
 		}
 	}

@@ -1247,10 +1253,7 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 			iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
 			list_add(&iu->list, &target->free_tx);
 		} else {
-			shost_printk(KERN_ERR, target->scsi_host,
-				     PFX "failed send status %d\n",
-				     wc.status);
-			target->qp_in_error = 1;
+			srp_handle_qp_err(wc.status, wc.opcode, target);
 			break;
 		}
 	}

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Eliminate the private_rport_attrs[] array and the SETUP_*() macros used to set up that array since the information in that array duplicates the information in the static device attributes. Also, trigger a kernel warning if SRP_RPORT_ATTRS is not large enough since it is easy to forget to update that macro when adding new attributes.

---
 drivers/scsi/scsi_transport_srp.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)

index 07c4394..1cbf097 100644
@@ -47,7 +47,6 @@ struct srp_internal {
 	struct device_attribute *host_attrs[SRP_HOST_ATTRS + 1];
 
 	struct device_attribute *rport_attrs[SRP_RPORT_ATTRS + 1];
-	struct device_attribute private_rport_attrs[SRP_RPORT_ATTRS];
 	struct transport_container rport_attr_cont;
 };
 

@@ -72,24 +71,6 @@ static DECLARE_TRANSPORT_CLASS(srp_host_class, "srp_host", srp_host_setup,
 static DECLARE_TRANSPORT_CLASS(srp_rport_class, "srp_remote_ports",
 			       NULL, NULL, NULL);
 
-#define SETUP_TEMPLATE(attrb, field, perm, test, ro_test, ro_perm)	\
-	i->private_##attrb[count] = dev_attr_##field;		\
-	i->private_##attrb[count].attr.mode = perm;			\
-	if (ro_test) {							\
-		i->private_##attrb[count].attr.mode = ro_perm;		\
-		i->private_##attrb[count].store = NULL;			\
-	}								\
-	i->attrb[count] = &i->private_##attrb[count];			\
-	if (test)							\
-		count++
-
-#define SETUP_RPORT_ATTRIBUTE_RD(field)					\
-	SETUP_TEMPLATE(rport_attrs, field, S_IRUGO, 1, 0, 0)
-
-#define SETUP_RPORT_ATTRIBUTE_RW(field)					\
-	SETUP_TEMPLATE(rport_attrs, field, S_IRUGO | S_IWUSR,		\
-		       1, 1, S_IRUGO)
-
 #define SRP_PID(p) \
 	(p)->port_id[0], (p)->port_id[1], (p)->port_id[2], (p)->port_id[3], \
 	(p)->port_id[4], (p)->port_id[5], (p)->port_id[6], (p)->port_id[7], \

@@ -326,9 +307,10 @@ srp_attach_transport(struct srp_function_template *ft)
 	i->rport_attr_cont.ac.match = srp_rport_match;
 
 	count = 0;
-	SETUP_RPORT_ATTRIBUTE_RD(port_id);
-	SETUP_RPORT_ATTRIBUTE_RD(roles);
-	i->rport_attrs[count] = NULL;
+	i->rport_attrs[count++] = &dev_attr_port_id;
+	i->rport_attrs[count++] = &dev_attr_roles;
+	i->rport_attrs[count++] = NULL;
+	WARN_ON(count > ARRAY_SIZE(i->rport_attrs));
 
 	transport_container_register(&i->rport_attr_cont);
 

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Not sure I care one way or the other, but it looks to have been written the way it was to share a common code structure with the FC, SAS, and other SCSI transport code. They use a BUG_ON instead of WARN_ON, which seems more appropriate, since we just overran a buffer...

The SCSI guys need to review this one, as it needs their ack.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

My opinion about the SETUP_*() macros is that - at least in the SRP transport code - they make the code harder to read without having any real value and hence should be removed.

Sorry, but I disagree. The guideline for Linux kernel code is to use BUG_ON() only if a crash can't be avoided. With SLUB red zoning enabled it is still possible to unload kernel modules after a (mild) buffer overrun. And if you have a look at mm/slub.c, you will see that slab_err() is invoked in case padding has been overwritten. That last function does something similar to WARN_ON().

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

You aren't guaranteed to have SLUB as your allocator, much less having the red-zoning turned on, so you have no guarantee it is safe to continue.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Document the ib_srp sysfs attributes according to the rules specified in Documentation/ABI/README.

---
 Documentation/ABI/stable/sysfs-driver-ib_srp |  141 ++++++++++++++++++++++++++
 1 files changed, 141 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-ib_srp

new file mode 100644
index 0000000..6b74c09
@@ -0,0 +1,141 @@
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/add_target
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	[email protected]
+Description:	Interface for making ib_srp connect to a new target.
+		One can request ib_srp to connect to a new target by writing
+		a comma-separated list of login parameters to this sysfs
+		attribute. The supported parameters are:
+		* id_ext, a 16-digit hexadecimal number specifying the last
+		  eight bytes of the 16-byte target port identifier sent by
+		  ib_srp to the target in the SRP_LOGIN_REQ request.
+		* ioc_guid, a 16-digit hexadecimal number specifying the first
+		  eight bytes of the 16-byte target port identifier sent by
+		  ib_srp to the target in the SRP_LOGIN_REQ request.
+		* dgid, a 16-digit hexadecimal number specifying the
+		  destination GID.
+		* pkey, a four-digit hexadecimal number specifying the
+		  InfiniBand partition key.
+		* service_id, a 16-digit hexadecimal number specifying the
+		  InfiniBand service ID used for communication with the SRP
+		  target.
+		* max_sect, a decimal number specifying the maximum number of
+		  sectors to be transferred via a single SRP_CMD request.
+		* max_cmd_per_lun, a decimal number specifying the maximum
+		  number of outstanding commands per LUN.
+		* io_class, a hexadecimal number specifying the SRP I/O class.
+		  Must be either 0xff00 (rev 10) or 0x0100 (rev 16a).
+		* initiator_ext, a 16-digit hexadecimal number specifying the
+		  last eight bytes of the 16-byte initiator port identifier
+		  sent by ib_srp to the target in the SRP_LOGIN_REQ request.
+		* cmd_sg_entries, the maximum number of memory descriptors
+		  that fit in a single SRP command when using the direct data
+		  buffer descriptor format.
+		* allow_ext_sg, whether ib_srp is allowed to send SRP_CMD
+		  requests to the target with an indirect data buffer
+		  descriptor, that is a data buffer descriptor that has to be
+		  obtained by the target via an additional RDMA transfer.
+		* sg_tablesize, a number specifying the maximum length of
+		  scatter/gather lists passed by the SCSI layer to ib_srp.
+
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/ibdev
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	[email protected]
+Description:	HCA name (<hca>).
+
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/port
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	[email protected]
+Description:	HCA port number (<port_number>).
+
+What:		/sys/class/scsi_host/host<n>/allow_ext_sg
+Date:		May 19, 2011
+KernelVersion:	2.6.39
+Contact:	[email protected]
+Description:	Whether ib_srp is allowed to send SRP_CMD requests to the
+		target with an indirect data buffer descriptor, that is a
+		data buffer descriptor that has to be obtained by the target
+		via an additional RDMA transfer. If set to zero, ib_srp will
+		refuse to process data transfers whose scatter/gather list
+		length exceeds cmd_sg_entries.
+
+What:		/sys/class/scsi_host/host<n>/cmd_sg_entries
+Date:		May 19, 2011
+KernelVersion:	2.6.39
+Contact:	[email protected]
+Description:	See also allow_ext_sg.
+
+What:		/sys/class/scsi_host/host<n>/dgid
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	InfiniBand destination GID used for communication with the SRP
+		target. Differs from orig_dgid if port redirection has happened.
+
+What:		/sys/class/scsi_host/host<n>/id_ext
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	Last eight bytes of the 16-byte target port identifier sent by
+		ib_srp to the target in the SRP_LOGIN_REQ request.
+
+What:		/sys/class/scsi_host/host<n>/ioc_guid
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	First eight bytes of the 16-byte target port identifier sent
+		by ib_srp to the target in the SRP_LOGIN_REQ request.
+
+What:		/sys/class/scsi_host/host<n>/local_ib_device
+Date:		November 29, 2006
+KernelVersion:	2.6.19
+Contact:	[email protected]
+Description:	Name of the InfiniBand HCA used for communicating with the
+		SRP target.
+
+What:		/sys/class/scsi_host/host<n>/local_ib_port
+Date:		November 29, 2006
+KernelVersion:	2.6.19
+Contact:	[email protected]
+Description:	Number of the HCA port used for communicating with the
+		SRP target.
+
+What:		/sys/class/scsi_host/host<n>/orig_dgid
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	InfiniBand destination GID specified in the parameters
+		written to the add_target sysfs attribute.
+
+What:		/sys/class/scsi_host/host<n>/pkey
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	A 16-bit number representing the InfiniBand partition key used
+		for communication with the SRP target.
+
+What:		/sys/class/scsi_host/host<n>/req_lim
+Date:		October 20, 2010
+KernelVersion:	2.6.36
+Contact:	[email protected]
+Description:	Number of requests ib_srp can send to the target before it has
+		to wait for more credits. For more information see also the
+		SRP credit algorithm in the SRP specification.
+
+What:		/sys/class/scsi_host/host<n>/service_id
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	[email protected]
+Description:	InfiniBand service ID used for communication with the SRP
+		target.
+
+What:		/sys/class/scsi_host/host<n>/zero_req_lim
+Date:		September 20, 2006
+KernelVersion:	2.6.18
+Contact:	[email protected]
+Description:	Number of times the initiator had to wait with sending a
+		request to the target because it ran out of credits. For more
+		information see also the SRP credit algorithm in the SRP
+		specification.

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

%s/ib_srp/SRP initiator/g

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

For the current ib_srp implementation cmd_sg_entries is the maximum number of direct data buffer descriptors allocated.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The direct memory descriptor format is only used when there is a single memory region for the command; all other cases are described by an indirect table; cmd_sg_entries limits the number of memory descriptors from that table that will be cached in the command packet.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Add a time-based transport layer test such that fail-over in a multipath setup can happen quickly.

Add the necessary functions in the SRP transport module to allow an SRP initiator driver to implement this.

Add a slave_delete callback in the SCSI host template such that SCSI hosts that hold a reference to a SCSI device can be deleted via the sysfs SCSI host "delete" attribute.

---
 Documentation/ABI/stable/sysfs-transport-srp |   18 +++
 drivers/infiniband/ulp/srp/ib_srp.c          |   48 ++++++++
 drivers/scsi/scsi_sysfs.c                    |    7 +-
 drivers/scsi/scsi_transport_srp.c            |  168 +++++++++++++++++++++++++-
 include/scsi/scsi_host.h                     |    9 ++
 include/scsi/scsi_transport_srp.h            |    9 ++
 6 files changed, 257 insertions(+), 2 deletions(-)

index 7b0d4a5..d8a9048 100644
@@ -1,3 +1,21 @@
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/ping_interval
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected], [email protected]
+Description:	Time in seconds between two sucessive ping attempts. Setting
+		this parameter to zero or a negative value disables the ping
+		mechanism.
+
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/ping_timeout
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected], [email protected]
+Description:	If more time has elapsed than the specified number of seconds
+		since the latest successful ping attempt, the SRP initiator
+		driver that enabled this feature is informed about a transport
+		layer timeout by invoking its rport_ping_timedout callback
+		function.
+
 What:		/sys/class/srp_remote_ports/port-<h>:<n>/port_id
 Date:		June 27, 2007
 KernelVersion:	2.6.24

index e0bd3f7..328caa3 100644
@@ -534,7 +534,12 @@ static DEVICE_ATTR(rescan, S_IWUSR, NULL, store_rescan_field);
 
 static void sdev_store_delete_callback(struct device *dev)
 {
-	scsi_remove_device(to_scsi_device(dev));
+	struct scsi_device *sdev = to_scsi_device(dev);
+	struct scsi_host_template *sht = sdev->host->hostt;
+
+	if (sht->slave_delete)
+		sht->slave_delete(sdev);
+	scsi_remove_device(sdev);
 }
 
 static ssize_t

index f1f2644..16536a6 100644
@@ -233,6 +233,15 @@ struct scsi_host_template {
 	 */
 	int (* slave_configure)(struct scsi_device *);
 
+	/**
+	 * Callback invoked just before scsi_remove_device() will be invoked
+	 * e.g. if device removal has been requested via the sdev "delete"
+	 * sysfs attribute.
+	 *
+	 * Status: OPTIONAL
+	 */
+	void (* slave_delete)(struct scsi_device *);
+
 	/*
 	 * Immediately prior to deallocating the device and after all activity
 	 * has ceased the mid layer calls this point so that the low level

index fcda8e3..565cb79 100644
@@ -38,14 +38,20 @@ struct srp_rport {
 	void			*lld_data;	/* LLD private data */
 
 	spinlock_t		lock;
+	struct scsi_device	*sdev;
 	enum srp_rport_state	state;
 
+	int			ping_itv;
+	int			ping_tmo;
+	unsigned long		latest_ping_response;
 	int			recovery_tmo;
+	struct delayed_work	ping_work;
 	struct delayed_work	recovery_work;
 };
 
 struct srp_function_template {
 	/* for initiator drivers */
+	void (*rport_ping_timedout) (struct srp_rport *rport);
 	void (*rport_recovery_timedout) (struct srp_rport *rport);
 	/* for target drivers */
 	int (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);

@@ -62,6 +68,9 @@ extern void srp_rport_del(struct srp_rport *);
 extern void srp_unblock_rport(struct srp_rport *rport);
 extern void srp_block_rport(struct srp_rport *rport);
 extern void srp_start_recovery_timer(struct srp_rport *rport);
+extern void srp_rport_set_sdev(struct srp_rport *rport,
+			       struct scsi_device *sdev);
+extern void srp_rport_disable_ping(struct srp_rport *rport);
 extern void srp_rport_disable_recovery(struct srp_rport *rport);
 extern void srp_stop_rport(struct srp_rport *rport);
 

1.7.3.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Why should this be done in the kernel? multipathd already verifies all paths to a SCSI device are up and that the device is reachable.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

I'm afraid it's impossible to make a transport layer check work reliably from user space. As an example, srp_reset_host() blocks the SCSI host before reconnecting. Before starting to attempt to reconnect, that action does block the SCSI host and hence also all transport layer checks issued from user space. I doubt it's possible to fix the resulting race between a transport layer reconnect issued from srp_reset_host() and a transport layer reconnect triggered from user space.

Bart.

Part of the problem is introduced by allowing for permanent connections rather than using the familiar dev_loss_tmo and fast_io_fail_tmo parameters from other SCSI transports. For instance, in the FC transport, rports are allowed to disappear for up to dev_loss_tmo seconds before being removed from the SCSI device tree. Until they have been gone for fast_fail_io_tmo seconds, they are blocked (as is error handling to prevent offlining devices). Once they have been gone longer that fast_fail_io_tmo, they become unblocked and new IO will be failed.

Now, the FC transport is probably a bit more complex than we want right now, but following it (and the SAS transport's) lead should keep us in sync with where the rest of the SCSI stack is headed.

As for reliability from user space, multipathd checks that the SCSI initiator is not blocked before checking the liveness of the path; blocked paths are assumed to be down. It still seems to default to a 30 second timeout for the test TUR/directIO/etc check, and that doesn't currently look to be configurable (fixable). These timeouts are independent of the SCSI layer, and will mark a path down for new traffic without waiting for the lower level timeout.

The transport layer reconnect from user space (I'm assuming you were thinking ioctl or using the sg device) would coordinated by the SCSI-midlayer, using calls to srp_reset_host(), so I think we avoid any race condition. Or did you mean a manual reconnect attempt from manipulating srp_host/X/reconnect_tmo via sysfs -- in which case it is in our code, so we can certainly avoid race conditions.

I still think this is already solved in user space, but the new reconnect model you've implemented doesn't match up with the expected semantics. It'd be better to match the rest of the SCSI stack for this.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Sorry, but I'm afraid there is no "rest of the SCSI stack [semantics]". I've been following the semantics implemented in open-iscsi. If the semantics implemented for FC differ of those proposed in this patch set, then that means that the semantics implemented for FC differ of those for open-iscsi.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

FC, SAS, SPI, (current) SRP, and FCoE currently use the same semantic to check if the transport is still alive -- user space initiates activity, usually via multipathd. You are changing SRP to match the iSCSI stack, and because SRP does not have a NOP command, you have to fake one up using Test Unit Ready to the first LUN to come through slave_alloc(). You kill the entire connection if that LUN has issues and causes a timeout, though the rest of the LUNs may be fine and able to communicate.

This is all handled properly by existing user space code; there's no need for it to be in kernel, especially if you have to fake it while going against the grain.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I'm not convinced an equivalent of fast_fail_io_tmo is necessary for the SRP transport. If a target disappears briefly from the IB fabric what will happen with the posted patch series is that the initiator is blocked during one ping interval and also that a reconnect is triggered. Also, some SCSI commands may be reissued after reconnecting. But that shouldn't have any adverse consequences, isn't it ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

We don't want to leave a target blocked indefinitely -- commands caught in the blocked queue won't be reissued until the queue is unblocked -- but we may want to keep the sdX mappings around for a long time. fast_io_fail_tmo gives us the ability to do that -- the expiration of fast_io_fail_tmo unblocks the queue and allows commands to be failed due to the transport error. See commit f2818663.

Also, these settings can be used to help tune multipath failover for devices with relatively long LUN transfer times (say with RDAC) vs short ones (ALUA). You still don't want the mappings to go away, but you want to move data to the other paths in a reasonably quick fashion.
http://lkml.org/lkml/2008/1/7/244 talks about this a bit.

As for reissued commands, most shouldn't be an issue -- once they get out of the blocked queue. I would expect there to be problems with certain vendor specific commands, and tape drives will have issues, but those are common to any multipath solution.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Our opinions may differ here. My opinion is that for some use cases it is crucial to be able to block a target indefinitely, e.g. when there is only a single path between initiator and target and when the user prefers that I/O blocks instead of encountering I/O errors. See e.g. the "iSCSI settings for iSCSI root" section in the iSCSI README
(http://www.open-iscsi.org/docs/README).

Regarding commit f2818663: as far as I can see what that commit does is to make it impossible for a user to set fast_io_fail_tmo == -1 together with dev_loss_tmo > SCSI_DEVICE_BLOCK_MAX_TIMEOUT. That change is specific to the FC code - only the FC transport code limits the blocked state time to the SCSI_DEVICE_BLOCK_MAX_TIMEOUT limit.

In a multipath setup the ping_interval, ping_timeout, recovery_tmo and max_reconnects parameters (all added via this patch series) should be sufficient to allow control over how quickly multipath failover happens.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

I can see admins desiring that option, but it's also handled by putting your root on a dm-multipath volume. Either way, I'm open to letting things block indefinitely, but I do think we should look into why the SCSI stack has a concept of the maximum blocking time.

I was pointing to the commit message as evidence of more prominent SCSI developer's line of thought on handling missing devices. As you note, the actual diff is uninteresting.

We should match the rest of the SCSI stack unless there is good reason to go our own way. The iSCSI transport can do what it does because it has a native ping, but I'm not convinced they should have introduced new semantics and/or names when taking advantage of that. As far as I can tell without deeply studying it, replacement_timeout is equivalent to fast_io_fail_tmo.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

iSCSI replacement_timeout is the same as fast_io_fail_tmo for FC. iSCSI replacement_timeout actually came first so you should say FC should have copied our name :)

So FC has fast_io_fail_tmo which controls when to fail IO when a port is blocked. This can be set to so that it does not ever fire. However, FC also has the dev_loss_tmo which controls when to remove devices (and also fail IO) when a port is blocked. This one cannot be turned off, so it will eventually fire and you will get IO errors (this is due to the devices being removed and the scsi layer then failing all IO).

iSCSI had replacement_timeout first, and it only fails IO, because at the time things did not handle hotplug removal well, and I did not know there was a need for the removal. There is patch to add dev_loss_tmo to the iSCSI layer. There have been some bugs though, so I have not pushed it. But it will also work like FC where you cannot turn it off.

SAS has something like the dev_loss_tmo. It does not have something like the fast_io_fail/replacement_timeout.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Indeed, some git-foo seems to indicate both stacks had that functionality mainlined in 2.6.18, with iSCSI getting it first. FC should have used your name.

This says to me that SRP should use the dev_loss_tmo semantics, though the naming of fast_io_fail vs replacement_timeout is a bit more of a question than I thought. I tend to think of SRP more in terms of FC than iSCSI, so I still prefer the former, but perhaps not as strongly now.

I still believe consistency is good -- what do the SCSI devs think here? Should we work on getting a consistent name for this property -- and if so, which one -- or does the name not matter?

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Do we need dev_loss_tmo functionality ? Since multipathd switches over if the active path is in the blocked state, the posted patch set already provides a way to make multipathd switch over if communication is lost.

Bart.

I think so. multipathd isn't the only use case, though I think we need to reorganize the SRP device model to fully take advantage of dev_loss_tmo -- but that can come later. I'd like to see dev_loss_tmo take the place of max_reconnects to give admins an easy way to set a maximum time.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Agreed. But as far as I can see multipathd only supports the FC name (fast_io_fail_tmo) and not the iSCSI name (recovery_tmo). That looks like an argument to me to switch to the FC name for ib_srp.

Bart.

Document the srp_transport sysfs attributes according to the rules specified in Documentation/ABI/README.

---
 Documentation/ABI/stable/sysfs-transport-srp |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-transport-srp

new file mode 100644
index 0000000..7b0d4a5
@@ -0,0 +1,12 @@
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/port_id
+Date:		June 27, 2007
+KernelVersion:	2.6.24
+Contact:	[email protected]
+Description:	16-byte local SRP port identifier in hexadecimal format. An
+		example: 4c:49:4e:55:58:20:56:49:4f:00:00:00:00:00:00:00.
+
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/roles
+Date:		June 27, 2007
+KernelVersion:	2.6.24
+Contact:	[email protected]
+Description:	Role of the remote port. Either "SRP Initiator" or "SRP Target".

1.7.3.4

Register transport attributes after the attribute array has been set up instead of before.

---
 drivers/scsi/scsi_transport_srp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

index 21a045e..07c4394 100644
@@ -324,13 +324,14 @@ srp_attach_transport(struct srp_function_template *ft)
 	i->rport_attr_cont.ac.attrs = &i->rport_attrs[0];
 	i->rport_attr_cont.ac.class = &srp_rport_class.class;
 	i->rport_attr_cont.ac.match = srp_rport_match;
-	transport_container_register(&i->rport_attr_cont);
 
 	count = 0;
 	SETUP_RPORT_ATTRIBUTE_RD(port_id);
 	SETUP_RPORT_ATTRIBUTE_RD(roles);
 	i->rport_attrs[count] = NULL;
 
+	transport_container_register(&i->rport_attr_cont);
+
 	i->f = ft;
 
 	return &i->t;

1.7.3.4

Why?

I don't disagree with the change, but there should be at least a little discussion of why you're doing it. Your last message about this to James Bottomley had quite a bit of content, perhaps too much, but then his comment that 'it's best practice to initialize a structure before registering it' seems appropriate.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Add the necessary functions in the SRP transport module to allow an SRP initiator driver to implement transport layer recovery.

Convert srp_target_port.qp_in_error into a target port state.

Factor out the code for removing an SRP target into the new function srp_remove_target().

In the ib_srp IB completion handlers, do not stop processing completions after the first error completion or a CM DREQ has been received. Block the SCSI target as soon as the first IB error completion has been received. Eliminate the SCSI target state test from srp_queuecommand().

Rework ib_srp transport layer error handling. Instead of letting SCSI commands time out if a transport layer error occurs, block the SCSI host and try to reconnect until the reconnect timeout elapses or until the maximum number of reconnect attempts has been exceeded, whichever happens first.

Rescan LUNs after having unblocked a SCSI target controlled by ib_srp.

Add the sysfs attributes reconnect_tmo, failed_reconnects and max_reconnects.

---
 Documentation/ABI/stable/sysfs-driver-ib_srp |   25 ++
 drivers/infiniband/ulp/srp/ib_srp.c          |  398 ++++++++++++++++++++------
 drivers/infiniband/ulp/srp/ib_srp.h          |   27 ++-
 drivers/scsi/scsi_transport_srp.c            |  162 +++++++++++-
 include/scsi/scsi_transport_srp.h            |   31 ++
 5 files changed, 548 insertions(+), 95 deletions(-)

index 6b74c09..b1f449c 100644
@@ -74,6 +74,12 @@ Contact:	[email protected]
 Description:	InfiniBand destination GID used for communication with the SRP
 		target. Differs from orig_dgid if port redirection has happened.
 
+What:		/sys/class/scsi_host/host<n>/failed_reconnects
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected]
+Description:	Number of consecutive failed SRP reconnect attempts.
+
 What:		/sys/class/scsi_host/host<n>/id_ext
 Date:		June 17, 2006
 KernelVersion:	2.6.17

@@ -102,6 +108,14 @@ Contact:	[email protected]
 Description:	Number of the HCA port used for communicating with the
 		SRP target.
 
+What:		/sys/class/scsi_host/host<n>/max_reconnects
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected]
+Description:	Maximum number of times ib_srp should attempt to reconnect
+		to the SRP target after an I/O error occurred. The value -1
+		means that ib_srp should keep trying to reconnect forever.
+
 What:		/sys/class/scsi_host/host<n>/orig_dgid
 Date:		June 17, 2006
 KernelVersion:	2.6.17

@@ -116,6 +130,17 @@ Contact:	[email protected]
 Description:	A 16-bit number representing the InfiniBand partition key used
 		for communication with the SRP target.
 
+What:		/sys/class/scsi_host/host<n>/reconnect_tmo
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected]
+Description:	Time in seconds to wait after a failed connection attempt
+		before trying to reconnect. Setting this parameter to zero or
+		to a negative value prevents ib_srp to attempt to reconnect.
+		Changing this parameter from a value <= 0 into a value > 0 at
+		a time there is no connection with the target will cause
+		ib_srp to try to reconnect with the target.
+
 What:		/sys/class/scsi_host/host<n>/req_lim
 Date:		October 20, 2010
 KernelVersion:	2.6.36

index f010bd9..5e30cff 100644
@@ -78,11 +78,20 @@ enum {
 	SRP_MAP_NO_FMR		= 1,
 };
 
+/**
+ * @SRP_TARGET_LIVE: IB RC connection is established and SCSI host is unblocked.
+ * @SRP_TARGET_CONNECTING: IB RC connection being established.
+ * @SRP_TARGET_BLOCKED: An IB RC error occurred. Recovery timer may be running.
+ *                      SCSI host is blocked.
+ * @SRP_TARGET_DISCONNECTED: IB RC connection has been disconnected.
+ * @SRP_TARGET_REMOVED: SCSI host removal is pending.
+ */
 enum srp_target_state {
 	SRP_TARGET_LIVE,
 	SRP_TARGET_CONNECTING,
-	SRP_TARGET_DEAD,
-	SRP_TARGET_REMOVED
+	SRP_TARGET_BLOCKED,
+	SRP_TARGET_DISCONNECTED,
+	SRP_TARGET_REMOVED,
 };
 
 enum srp_iu_type {

@@ -155,6 +164,7 @@ struct srp_target_port {
 	u16			io_class;
 	struct srp_host	       *srp_host;
 	struct Scsi_Host       *scsi_host;
+	struct srp_rport       *rport;
 	char			target_name[32];
 	unsigned int		scsi_id;
 	unsigned int		sg_tablesize;

@@ -174,15 +184,22 @@ struct srp_target_port {
 	struct srp_iu	       *rx_ring[SRP_RQ_SIZE];
 	struct srp_request	req_ring[SRP_CMD_SQ_SIZE];
 
-	struct work_struct	work;
-
 	struct list_head	list;
 	struct completion	done;
 	int			status;
-	int			qp_in_error;
+	struct mutex		mutex;
 
 	struct completion	tsk_mgmt_done;
 	u8			tsk_mgmt_status;
+
+	struct work_struct	block_work;
+	struct work_struct	remove_work;
+	struct work_struct	scan_work;
+
+	int			reconnect_tmo;
+	int			max_reconnects;
+	int			failed_reconnects;
+	struct delayed_work	reconnect_work;
 };
 
 struct srp_iu {

index 9c60ca1..fcda8e3 100644
@@ -8,19 +8,45 @@
 #define SRP_RPORT_ROLE_INITIATOR 0
 #define SRP_RPORT_ROLE_TARGET 1
 
+/**
+ * enum srp_rport_state - Rport state.
+ * SRP_RPORT_LIVE: SCSI host logged in.
+ * SRP_RPORT_BLOCKED: SCSI host blocked because of a transport layer issue.
+ */
+enum srp_rport_state {
+	SRP_RPORT_LIVE,
+	SRP_RPORT_BLOCKED,
+};
+
 struct srp_rport_identifiers {
 	u8 port_id[16];
 	u8 roles;
 };
 
 struct srp_rport {
+	/* for initiator and target drivers */
+
+	struct srp_function_template *ft;
+
 	struct device dev;
 
 	u8 port_id[16];
 	u8 roles;
+
+	/* for initiator drivers */
+
+	void			*lld_data;	/* LLD private data */
+
+	spinlock_t		lock;
+	enum srp_rport_state	state;
+
+	int			recovery_tmo;
+	struct delayed_work	recovery_work;
 };
 
 struct srp_function_template {
+	/* for initiator drivers */
+	void (*rport_recovery_timedout) (struct srp_rport *rport);
 	/* for target drivers */
 	int (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
 	int (* it_nexus_response)(struct Scsi_Host *, u64, int);

@@ -33,6 +59,11 @@ extern void srp_release_transport(struct scsi_transport_template *);
 extern struct srp_rport *srp_rport_add(struct Scsi_Host *,
 				       struct srp_rport_identifiers *);
 extern void srp_rport_del(struct srp_rport *);
+extern void srp_unblock_rport(struct srp_rport *rport);
+extern void srp_block_rport(struct srp_rport *rport);
+extern void srp_start_recovery_timer(struct srp_rport *rport);
+extern void srp_rport_disable_recovery(struct srp_rport *rport);
+extern void srp_stop_rport(struct srp_rport *rport);
 
 extern void srp_remove_host(struct Scsi_Host *);
 

1.7.3.4

I've run out of time today to look further, but I did want to point out that I think you're doing way too much in this patch. I like that you're moving some pieces into the transport, as it would be good to handle in-fabric/out-of-fabric there as well, as done for the FC transport. Getting there needs to be done through a series of patches rather one big patches, especially since you're touching code that multiple drivers use and need to get the core changes reviewed by so many people. I would need some quite some persuasion to be comfortable making large changes to srp_transport through the IB tree without ack's by Fujita and Brian.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

This part should probably be folded into patch 7.

This could be a separate patch, making that easy to review.

Why do this? It seems like you're just going to create a flurry of error messages when one would suffice, and we've got to tear down and reconnect anyway.

I think you still want to check the SRP target state in srp_queuecommand() -- you schedule the blocking of the queue, but it could be a while before it happens. No sense sending down more commands when you know they are going to fail -- and if you're worried about the performance of the conditional in the hot path, we can make it unlikely(). Cleaning up the target states lets us just check for SRP_TARGET_LIVE, though you would want to change the creation sequence so that isn't the birth state of the target.

This is good, but should probably be part of the initial disconnect. We want to run the receive queue dry, processing responses to avoid unnecessarily resetting a command that was successful.

When we're blocked for a disconnected target, we may want to call the state something other than SRP_TARGET_BLOCKED. I'd like to eventually handle the case where the target leaves the fabric temporarily -- we don't necessarily disconnect in that case, but we need to block commands until it comes back or we give up on it.

You've tried to add this in the past; I don't mean to deny you credit, and I certainly value your work, but I'm not sure where the threshold is for adding a copyright line.

Roland, care to comment here?

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Blocking the SCSI host doesn't prevent ib_srp from continuing to process IB completion notifications.

The case where the target leaves the fabric temporarily should already be covered by this patch series.

That's on purpose.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

In your series, it doesn't -- and I was wrong about the message spam, you check for the proper state and the work being queued.

I haven't parsed it all out from your changes just yet, but I think part of the reason you may have had problems with req->scmd being null in srp_handle_recv() is due to a new race between the tear down of the connection and continuing to process completion notifications.

Only once a command is sent to it and timesout or we get a QP error. The idea here would be to start the dev_loss_tmo timer once it was detected that it left the fabric. This was handled in OFED via sysfs attributes, but it seems like we could register for the events ourselves.

This isn't something I expect you to implement, but I'd like to leave room for it in the future.

I'm assuming you do this to ensure that everyone has seen the appropriate state and exited the critical section, then? Best to add a comment to that effect. Actually, is there any reason to unlock it again since we're removing the target? I don't have the code in front of me ATM, so I'm assuming that you don't call any other routines that need the lock after this. sparse may complain, though.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

The resources used by the QP completion handler are the srp_target port data structure and the QP data structure. And as you can see in srp_remove_target(), the scsi_host_put() call is invoked after srp_disconnect_target(). That last function waits for the DREP notification from the IB CM. That should guarantee that no IB completions arrive during or after the scsi_host_put() call. Or did I miss something ?

Note: I haven't observed the req->scmd == NULL condition yet.

OK, will do.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

What keeps the srp_recv_completion() --> srp_handle_recv() --> srp_process_rsp() --> etc. call chain from racing with srp_reconnect_target()?

It looks like the srp_reset_req() in srp_reconnect_tartget() could race with srp_process_rsp() if receive completions continue to be processed after a send failure, but perhaps it is I who is missing something.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

In the tests I ran so far no issues came up most likely because the decision to reconnect is taken a long time after the most recent response has been received. I'll see what has to change to make this safe even if there wouldn't be any such delay.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The excerpts below might provide some guidance (source: "The Copyright
Handbook: What Every Writer Needs to Know"):
[ ... ]
There are strict technical requirements as to what a copyright notice must contain. Follow these rules exactly or your notice may be found to be invalid and not accomplish its intended purpose. A valid copyright notice contains three elements:
* the copyright symbol
* the year in which the work was published
* the name of the copyright owner.
It is not required that these elements appear in any particular order in the notice, but most notices are written in the order set forth above. We'll discuss each element in turn.
[ ... ]
If there are multiple authors, they should all be listed in the copyright notice. The authors' names can appear in any order. [ ... ]

If I interpret the above correctly, at least two names are missing in the copyright header of the source file ib_srp.c.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Make it possible to disconnect via sysfs the IB RC connection used by the SRP protocol to communicate with a target.

Let the SRP transport layer create a sysfs "delete" attribute for initiator drivers that support this functionality.

---
 Documentation/ABI/stable/sysfs-transport-srp |    7 ++++
 drivers/infiniband/ulp/srp/ib_srp.c          |   44 +++++++++++++++----------
 drivers/scsi/scsi_transport_srp.c            |   20 +++++++++++-
 include/scsi/scsi_transport_srp.h            |    1 +
 4 files changed, 53 insertions(+), 19 deletions(-)

index d8a9048..b1c8acd 100644
@@ -1,3 +1,10 @@
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/delete
+Date:		November 1, 2011
+KernelVersion:	3.3
+Contact:	[email protected], [email protected]
+Description:	Instructs an SRP initiator to disconnect from a target and to
+		remove all LUNs imported from that target.
+
 What:		/sys/class/srp_remote_ports/port-<h>:<n>/ping_interval
 Date:		November 1, 2011
 KernelVersion:	3.3

index 135a870..e0359b4 100644
@@ -40,7 +40,7 @@ struct srp_host_attrs {
 #define to_srp_host_attrs(host)	((struct srp_host_attrs *)(host)->shost_data)
 
 #define SRP_HOST_ATTRS 0
-#define SRP_RPORT_ATTRS 5
+#define SRP_RPORT_ATTRS 6
 
 struct srp_internal {
 	struct scsi_transport_template t;

@@ -118,6 +118,22 @@ show_srp_rport_roles(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR(roles, S_IRUGO, show_srp_rport_roles, NULL);
 
+static ssize_t store_srp_rport_delete(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+
+	if (rport->ft->rport_delete) {
+		rport->ft->rport_delete(rport);
+		return count;
+	} else {
+		return -ENOSYS;
+	}
+}
+
+static DEVICE_ATTR(delete, S_IWUSR, NULL, store_srp_rport_delete);
+
 static ssize_t show_srp_rport_ping_interval(struct device *dev,
 					   struct device_attribute *attr,
 					   char *buf)

@@ -635,6 +651,8 @@ srp_attach_transport(struct srp_function_template *ft)
 	}
 	if (ft->rport_recovery_timedout)
 		i->rport_attrs[count++] = &dev_attr_recovery_tmo;
+	if (ft->rport_delete)
+		i->rport_attrs[count++] = &dev_attr_delete;
 	i->rport_attrs[count++] = NULL;
 	WARN_ON(count > ARRAY_SIZE(i->rport_attrs));
 

index 565cb79..8782d6a 100644
@@ -51,6 +51,7 @@ struct srp_rport {
 
 struct srp_function_template {
 	/* for initiator drivers */
+	void (*rport_delete)(struct srp_rport *rport);
 	void (*rport_ping_timedout) (struct srp_rport *rport);
 	void (*rport_recovery_timedout) (struct srp_rport *rport);
 	/* for target drivers */

1.7.3.4

This will be a nice feature to have, but I wonder about the proper behavior. I had thought about having a 'drain' state, where we wouldn't start new commands and would let existing ones complete or timeout before killing the connection. That may be in addition to a 'kill it now' variant.

Though perhaps my gentler delete is best done by having a user script walk all of the devices under the host and set them to offline, wait for scsi_host/hostX/host_busy to drop to zero, then hit the SRP delete attr.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

Sorry, but I disagree. One of the goals of this patch set is to make fast fail-over possible. Waiting for an error completion or time out contradicts with that goal.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

In general, I'm glad to see this being done. I won't have the time this week for a detailed review, but will take a look as soon as I can. I have a number of ideas I'd like to see implemented (and have partially done), so it will be interesting to see if we're using similar approaches.

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office