[PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Bart Van Assche]

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.
- Disconnecting from a target without unloading ib_srp becomes possible.
- Switchover can be triggered explicitly by deleting an initiator device.

Changes since v1:
- Switched from iSCSI-style sysfs parameters to FC-style sysfs parameters
  (fast_io_fail_tmo and dev_loss_tmo).
- Restored the original behavior of "add_target": a second login to a target
  is again allowed and also stops error recovery for previously created
  sessions.
- Changed srp_disconnect_target() such that it waits for the last completion
  after a disconnect.
- Moved code for enlarging block layer timeout from srp_slave_alloc() to
  srp_slave_configure(), and moved that function to after srp_reset_host().
  Also addressed Dave's other comments to the code for enlarging the
  timeout value.
- Split the "ib_srp: Rework error handling" patch in three separate patches to
  make reviewing easier.
- Added a patch that slightly improves ib_srp performance.
- Made comment more detailed of patch 09/14 "srp_transport: Fix attribute
  registration"
- Merged multiline log strings into a single line.
- Dropped patch 05/14 "ib_srp: Avoid that late SRP replies cause trouble".
- Implemented several other small changes.

The individual patches are:
0001-ib_srp-Introduce-pr_fmt.patch
0002-ib_srp-Consolidate-repetitive-sysfs-code.patch
0003-ib_srp-Enlarge-block-layer-timeout.patch
0004-ib_srp-Micro-optimize-completion-handlers.patch
0005-ib_srp-Separate-connection-and-host-state.patch
0006-ib_srp-Wait-for-last-completion-when-disconnecting.patch
0007-ib_srp-Introduce-three-helper-functions.patch
0008-ib_srp-Eliminate-state-SRP_TARGET_DEAD.patch
0009-srp_transport-Fix-atttribute-registration.patch
0010-srp_transport-Simplify-attribute-initialization-code.patch
0011-srp_transport-Document-sysfs-attributes.patch
0012-ib_srp-Document-sysfs-attributes.patch
0013-ib_srp-Allow-SRP-disconnect-through-sysfs.patch
0014-ib_srp-Move-target-port-removal-code.patch
0015-ib_srp-Maintain-a-single-connection-per-I_T-nexus.patch
0016-scsi-Add-scsi_host_template.slave_delete-callback.patch
0017-srp_transport-Add-transport-layer-recovery-support.patch
0018-ib_srp-Rework-error-handling.patch

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/18] ib_srp: Introduce pr_fmt()

[Bart Van Assche]

Make the logging code a little more brief by replacing
printk(KERN_WARNING PFX ...) by pr_warn(...) and by replacing
printk(KERN_ERR PFX ...) by pr_err(...).  Join log messages
split over multiple lines into a single line to make it easier
to grep for these messages. Change the severity of the log
statement in srp_qp_event() from "error" to "debug". Change a
double space into a single in the "bad IO class parameter" error
message. Remove one trailing space to avoid a checkpatch warning.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   54 ++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0bfa545..895a9ff 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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_debug("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,8 @@ 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,14 +2042,14 @@ 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"
-				       " %x specified (use %x or %x).\n",
-				       token, SRP_REV10_IB_IO_CLASS, SRP_REV16A_IB_IO_CLASS);
+				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;
 			}
 			target->io_class = token;
@@ -2064,7 +2067,8 @@ 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 +2076,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,15 +2085,16 @@ 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 "
-			       "'%s' in target creation request\n", p);
+			pr_warn("unknown parameter or missing value '%s' in target creation request\n",
+				p);
 			goto out;
 		}
 	}
@@ -2100,9 +2105,8 @@ 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 "
-				       "missing parameter '%s'\n",
-				       srp_opt_tokens[i].pattern);
+				pr_warn("target creation request is missing parameter '%s'\n",
+					srp_opt_tokens[i].pattern);
 
 out:
 	kfree(options);
@@ -2149,7 +2153,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,8 +2313,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",
-		       device->name);
+		pr_warn("Query device failed for %s\n", device->name);
 		goto free_attr;
 	}
 
@@ -2459,7 +2462,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 +2471,15 @@ 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 +2490,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 +2499,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.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/18] ib_srp: Consolidate repetitive sysfs code

[Bart Van Assche]

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.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   49 ++++++++++++----------------------
 1 files changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 895a9ff..bcbf22e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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);
 }
 
@@ -2432,6 +2416,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.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/18] ib_srp: Enlarge block layer timeout

[Bart Van Assche]

Enlarge the block layer timeout such that it is above the
InfiniBand transport layer timeout. This is necessary to avoid
that an SRP response is received after the SCSI layer has
already killed the associated SCSI command. Note: the timeout is
only set for SCSI disk devices but not for any other type of
SCSI device (M/O disk, tape, CD-ROM, ...).

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   46 +++++++++++++++++++++++++++++++++++
 drivers/infiniband/ulp/srp/ib_srp.h |    2 +
 2 files changed, 48 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index bcbf22e..b1327c6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1372,6 +1372,33 @@ err:
 	return -ENOMEM;
 }
 
+static uint32_t srp_compute_rq_tmo(struct ib_qp_attr *qp_attr, int attr_mask)
+{
+	uint64_t T_tr_ns, max_compl_time_ms;
+	uint32_t rq_tmo_jiffies;
+
+	/*
+	 * According to section 11.2.4.2 in the IBTA spec (Modify Queue Pair,
+	 * table 91), both the QP timeout and the retry count have to be set
+	 * for RC QP's during the RTR to RTS transition.
+	 */
+	WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) !=
+		(IB_QP_TIMEOUT | IB_QP_RETRY_CNT));
+
+	/*
+	 * Set target->rq_tmo_jiffies to one second more than the largest time
+	 * it can take before an error completion is generated. See also
+	 * C9-140..C9-142 in the IBTA spec for more information about how to
+	 * convert the QP Local ACK Timeout value to nanoseconds.
+	 */
+	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, NSEC_PER_MSEC);
+	rq_tmo_jiffies = msecs_to_jiffies(max_compl_time_ms + 1000);
+
+	return rq_tmo_jiffies;
+}
+
 static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 			       struct srp_login_rsp *lrsp,
 			       struct srp_target_port *target)
@@ -1431,6 +1458,8 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 	if (ret)
 		goto error_free;
 
+	target->rq_tmo_jiffies = srp_compute_rq_tmo(qp_attr, attr_mask);
+
 	ret = ib_modify_qp(target->qp, qp_attr, attr_mask);
 	if (ret)
 		goto error_free;
@@ -1689,6 +1718,22 @@ static int srp_reset_host(struct scsi_cmnd *scmnd)
 	return ret;
 }
 
+static int srp_slave_configure(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct srp_target_port *target = host_to_target(shost);
+	struct request_queue *q = sdev->request_queue;
+	unsigned long timeout;
+
+	WARN_ON(target->rq_tmo_jiffies == 0);
+	if (sdev->type == TYPE_DISK) {
+		timeout = max_t(unsigned, 30 * HZ, target->rq_tmo_jiffies);
+		blk_queue_rq_timeout(q, timeout);
+	}
+
+	return 0;
+}
+
 static ssize_t show_id_ext(struct device *dev, struct device_attribute *attr,
 			   char *buf)
 {
@@ -1821,6 +1866,7 @@ static struct scsi_host_template srp_template = {
 	.module				= THIS_MODULE,
 	.name				= "InfiniBand SRP initiator",
 	.proc_name			= DRV_NAME,
+	.slave_configure		= srp_slave_configure,
 	.info				= srp_target_info,
 	.queuecommand			= srp_queuecommand,
 	.eh_abort_handler		= srp_abort,
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 020caf0..e3a6304 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -163,6 +163,8 @@ struct srp_target_port {
 	struct ib_sa_query     *path_query;
 	int			path_query_id;
 
+	u32			rq_tmo_jiffies;
+
 	struct ib_cm_id	       *cm_id;
 
 	int			max_ti_iu_len;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/18] ib_srp: Micro-optimize completion handlers

[Bart Van Assche]

Reduce completion queue lock contention by polling for multiple
work completions at once. Help the CPU branch predictor by making
it clear that IB_WC_SUCCESS is the most likely case. Move the
error handling code into the new function srp_handle_qp_err().

Also, convert srp_target_port.qp_in_error from int to bool and
move the initialization of that variable into srp_connect_target().

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   57 +++++++++++++++++++++--------------
 drivers/infiniband/ulp/srp/ib_srp.h |    4 ++-
 2 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index b1327c6..afbddd8 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -515,6 +515,8 @@ static int srp_connect_target(struct srp_target_port *target)
 	int retries = 3;
 	int ret;
 
+	target->qp_in_error = false;
+
 	ret = srp_lookup_path(target);
 	if (ret)
 		return ret;
@@ -648,7 +650,6 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	for (i = 0; i < SRP_SQ_SIZE; ++i)
 		list_add(&target->tx_ring[i]->list, &target->free_tx);
 
-	target->qp_in_error = 0;
 	ret = srp_connect_target(target);
 	if (ret)
 		goto err;
@@ -1215,42 +1216,53 @@ 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 %s status %d\n",
+		     wc_opcode & IB_WC_RECV ? "receive" : "send", wc_status);
+	target->qp_in_error = true;
+}
+
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;
-	struct ib_wc wc;
+	struct ib_wc *const wc = target->recv_wc;
+	int i, n;
 
 	ib_req_notify_cq(cq, IB_CQ_NEXT_COMP);
-	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (wc.status) {
-			shost_printk(KERN_ERR, target->scsi_host,
-				     PFX "failed receive status %d\n",
-				     wc.status);
-			target->qp_in_error = 1;
-			break;
+	while ((n = ib_poll_cq(cq, ARRAY_SIZE(target->recv_wc), wc)) > 0) {
+		for (i = 0; i < n; ++i) {
+			if (likely(wc[i].status == IB_WC_SUCCESS)) {
+				srp_handle_recv(target, &wc[i]);
+			} else {
+				srp_handle_qp_err(wc[i].status, wc[i].opcode,
+						  target);
+				return;
+			}
 		}
-
-		srp_handle_recv(target, &wc);
 	}
 }
 
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 {
 	struct srp_target_port *target = target_ptr;
-	struct ib_wc wc;
+	struct ib_wc *const wc = target->send_wc;
+	int i, n;
 	struct srp_iu *iu;
 
-	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (wc.status) {
-			shost_printk(KERN_ERR, target->scsi_host,
-				     PFX "failed send status %d\n",
-				     wc.status);
-			target->qp_in_error = 1;
-			break;
+	while ((n = ib_poll_cq(cq, ARRAY_SIZE(target->send_wc), wc)) > 0) {
+		for (i = 0; i < n; ++i) {
+			if (likely(wc[i].status == IB_WC_SUCCESS)) {
+				iu = (struct srp_iu *) (uintptr_t) wc[i].wr_id;
+				list_add(&iu->list, &target->free_tx);
+			} else {
+				srp_handle_qp_err(wc[i].status, wc[i].opcode,
+						  target);
+				return;
+			}
 		}
-
-		iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
-		list_add(&iu->list, &target->free_tx);
 	}
 }
 
@@ -2238,7 +2250,6 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err_free_ib;
 
-	target->qp_in_error = 0;
 	ret = srp_connect_target(target);
 	if (ret) {
 		shost_printk(KERN_ERR, target->scsi_host,
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index e3a6304..91f92aa 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -135,6 +135,8 @@ struct srp_target_port {
 	struct ib_cq	       *send_cq ____cacheline_aligned_in_smp;
 	struct ib_cq	       *recv_cq;
 	struct ib_qp	       *qp;
+	struct ib_wc		recv_wc[16];
+	struct ib_wc		send_wc[16];
 	u32			lkey;
 	u32			rkey;
 	enum srp_target_state	state;
@@ -180,7 +182,7 @@ struct srp_target_port {
 	struct list_head	list;
 	struct completion	done;
 	int			status;
-	int			qp_in_error;
+	bool			qp_in_error;
 
 	struct completion	tsk_mgmt_done;
 	u8			tsk_mgmt_status;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/18] ib_srp: Separate connection and host state

[Bart Van Assche]

Separate connection and host state. Only report QP errors while
connected. Only invoke ib_send_cm_dreq() from inside
srp_disconnect_target() when connected such that invoking
srp_disconnect_target() after having received a DREQ does not
cause an error message to be printed.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   52 +++++++++++++++++++++++++----------
 drivers/infiniband/ulp/srp/ib_srp.h |    2 +-
 2 files changed, 38 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index afbddd8..ad8f168 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -428,17 +428,34 @@ static int srp_send_req(struct srp_target_port *target)
 	return status;
 }
 
+static bool srp_change_conn_state(struct srp_target_port *target,
+				  bool connected)
+{
+	bool changed = false;
+
+	spin_lock_irq(&target->lock);
+	if (target->connected != connected) {
+		target->connected = connected;
+		changed = true;
+	}
+	spin_unlock_irq(&target->lock);
+
+	return changed;
+}
+
 static void srp_disconnect_target(struct srp_target_port *target)
 {
-	/* XXX should send SRP_I_LOGOUT request */
+	if (srp_change_conn_state(target, false)) {
+		/* XXX should send SRP_I_LOGOUT request */
 
-	init_completion(&target->done);
-	if (ib_send_cm_dreq(target->cm_id, NULL, 0)) {
-		shost_printk(KERN_DEBUG, target->scsi_host,
-			     PFX "Sending CM DREQ failed\n");
-		return;
+		init_completion(&target->done);
+		if (ib_send_cm_dreq(target->cm_id, NULL, 0)) {
+			shost_printk(KERN_DEBUG, target->scsi_host,
+				     PFX "Sending CM DREQ failed\n");
+		} else {
+			wait_for_completion(&target->done);
+		}
 	}
-	wait_for_completion(&target->done);
 }
 
 static bool srp_change_state(struct srp_target_port *target,
@@ -515,6 +532,8 @@ static int srp_connect_target(struct srp_target_port *target)
 	int retries = 3;
 	int ret;
 
+	WARN_ON(target->connected);
+
 	target->qp_in_error = false;
 
 	ret = srp_lookup_path(target);
@@ -536,6 +555,7 @@ static int srp_connect_target(struct srp_target_port *target)
 		 */
 		switch (target->status) {
 		case 0:
+			srp_change_conn_state(target, true);
 			return 0;
 
 		case SRP_PORT_REDIRECT:
@@ -614,7 +634,7 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	struct ib_wc wc;
 	int i, ret;
 
-	if (!srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_CONNECTING))
+	if (target->state != SRP_TARGET_LIVE)
 		return -EAGAIN;
 
 	srp_disconnect_target(target);
@@ -654,9 +674,6 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	if (ret)
 		goto err;
 
-	if (!srp_change_state(target, SRP_TARGET_CONNECTING, SRP_TARGET_LIVE))
-		ret = -EAGAIN;
-
 	return ret;
 
 err:
@@ -673,7 +690,7 @@ err:
 	 * the flush_scheduled_work() in srp_remove_one().
 	 */
 	spin_lock_irq(&target->lock);
-	if (target->state == SRP_TARGET_CONNECTING) {
+	if (target->state == SRP_TARGET_LIVE) {
 		target->state = SRP_TARGET_DEAD;
 		INIT_WORK(&target->work, srp_remove_work);
 		queue_work(ib_wq, &target->work);
@@ -1220,8 +1237,11 @@ 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 %s status %d\n",
-		     wc_opcode & IB_WC_RECV ? "receive" : "send", wc_status);
+	if (target->connected)
+		shost_printk(KERN_ERR, target->scsi_host,
+			     PFX "failed %s status %d\n",
+			     wc_opcode & IB_WC_RECV ? "receive" : "send",
+			     wc_status);
 	target->qp_in_error = true;
 }
 
@@ -1276,7 +1296,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 	unsigned long flags;
 	int len;
 
-	if (target->state == SRP_TARGET_CONNECTING)
+	if (!target->connected)
 		goto err;
 
 	if (target->state == SRP_TARGET_DEAD ||
@@ -1593,6 +1613,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 	case IB_CM_DREQ_RECEIVED:
 		shost_printk(KERN_WARNING, target->scsi_host,
 			     PFX "DREQ received - connection closed\n");
+		srp_change_conn_state(target, false);
 		if (ib_send_cm_drep(cm_id, NULL, 0))
 			shost_printk(KERN_ERR, target->scsi_host,
 				     PFX "Sending CM DREP failed\n");
@@ -1917,6 +1938,7 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 	spin_unlock(&host->target_lock);
 
 	target->state = SRP_TARGET_LIVE;
+	target->connected = false;
 
 	scsi_scan_target(&target->scsi_host->shost_gendev,
 			 0, target->scsi_id, SCAN_WILD_CARD, 0);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 91f92aa..5f288fe 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -80,7 +80,6 @@ enum {
 
 enum srp_target_state {
 	SRP_TARGET_LIVE,
-	SRP_TARGET_CONNECTING,
 	SRP_TARGET_DEAD,
 	SRP_TARGET_REMOVED
 };
@@ -166,6 +165,7 @@ struct srp_target_port {
 	int			path_query_id;
 
 	u32			rq_tmo_jiffies;
+	bool			connected;
 
 	struct ib_cm_id	       *cm_id;
 
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/18] ib_srp: Wait for last completion when disconnecting

[Bart Van Assche]

When disconnecting the IB connection via the IB CM, wait until
any invoked completion handlers have finished processing SRP
protocol data and prevent that new work completions are queued.
Change the IB completion handlers such that all error completions
are processed instead of a subset and also such that receiving a
completion with zero wr_id is recognized as an end-of-work marker.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   81 ++++++++++++++++++++++++++++++----
 drivers/infiniband/ulp/srp/ib_srp.h |    3 +
 2 files changed, 74 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index ad8f168..0265a10 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -40,7 +40,7 @@
 #include <linux/parser.h>
 #include <linux/random.h>
 #include <linux/jiffies.h>
-
+#include <linux/delay.h>
 #include <linux/atomic.h>
 
 #include <scsi/scsi.h>
@@ -443,8 +443,58 @@ static bool srp_change_conn_state(struct srp_target_port *target,
 	return changed;
 }
 
+static void srp_wait_last_recv_wqe(struct srp_target_port *target)
+{
+	struct ib_recv_wr wr, *bad_wr;
+	int ret;
+
+	if (target->last_recv_wqe)
+		return;
+
+	memset(&wr, 0, sizeof(wr));
+	ret = ib_post_recv(target->qp, &wr, &bad_wr);
+	if (ret < 0) {
+		shost_printk(KERN_ERR, target->scsi_host,
+			     "ib_post_recv() failed (%d)\n", ret);
+		return;
+	}
+
+	ret = wait_event_timeout(target->qp_wq, target->last_recv_wqe,
+				 target->rq_tmo_jiffies);
+	WARN(ret <= 0, "Timeout while waiting for last recv WQE (ret = %d)\n",
+	     ret);
+}
+
+static void srp_wait_last_send_wqe(struct srp_target_port *target)
+{
+	unsigned long deadline = jiffies + target->rq_tmo_jiffies;
+	struct ib_send_wr wr, *bad_wr;
+	int ret;
+
+	if (target->last_send_wqe)
+		return;
+
+	memset(&wr, 0, sizeof(wr));
+	ret = ib_post_send(target->qp, &wr, &bad_wr);
+	if (ret < 0) {
+		shost_printk(KERN_ERR, target->scsi_host,
+			     "ib_post_send() failed (%d)\n", ret);
+		return;
+	}
+
+	while (!target->last_send_wqe && time_before(jiffies, deadline)) {
+		srp_send_completion(target->send_cq, target);
+		msleep(20);
+	}
+
+	WARN_ON(!target->last_send_wqe);
+}
+
 static void srp_disconnect_target(struct srp_target_port *target)
 {
+	struct ib_qp_attr qp_attr;
+	int ret;
+
 	if (srp_change_conn_state(target, false)) {
 		/* XXX should send SRP_I_LOGOUT request */
 
@@ -456,6 +506,16 @@ static void srp_disconnect_target(struct srp_target_port *target)
 			wait_for_completion(&target->done);
 		}
 	}
+
+	if (target->qp) {
+		qp_attr.qp_state = IB_QPS_ERR;
+		ret = ib_modify_qp(target->qp, &qp_attr, IB_QP_STATE);
+		WARN(ret != 0, "ib_modify_qp() failed: %d\n", ret);
+
+		srp_wait_last_recv_wqe(target);
+
+		srp_wait_last_send_wqe(target);
+	}
 }
 
 static bool srp_change_state(struct srp_target_port *target,
@@ -535,6 +595,8 @@ static int srp_connect_target(struct srp_target_port *target)
 	WARN_ON(target->connected);
 
 	target->qp_in_error = false;
+	target->last_recv_wqe = false;
+	target->last_send_wqe = false;
 
 	ret = srp_lookup_path(target);
 	if (ret)
@@ -631,7 +693,6 @@ static void srp_reset_req(struct srp_target_port *target, struct srp_request *re
 static int srp_reconnect_target(struct srp_target_port *target)
 {
 	struct ib_qp_attr qp_attr;
-	struct ib_wc wc;
 	int i, ret;
 
 	if (target->state != SRP_TARGET_LIVE)
@@ -655,11 +716,6 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	if (ret)
 		goto err;
 
-	while (ib_poll_cq(target->recv_cq, 1, &wc) > 0)
-		; /* nothing */
-	while (ib_poll_cq(target->send_cq, 1, &wc) > 0)
-		; /* nothing */
-
 	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
 		struct srp_request *req = &target->req_ring[i];
 		if (req->scmnd)
@@ -1237,7 +1293,7 @@ static void srp_handle_qp_err(enum ib_wc_status wc_status,
 			      enum ib_wc_opcode wc_opcode,
 			      struct srp_target_port *target)
 {
-	if (target->connected)
+	if (target->connected && !target->qp_in_error)
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "failed %s status %d\n",
 			     wc_opcode & IB_WC_RECV ? "receive" : "send",
@@ -1257,9 +1313,12 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 			if (likely(wc[i].status == IB_WC_SUCCESS)) {
 				srp_handle_recv(target, &wc[i]);
 			} else {
+				if (wc[i].wr_id == 0) {
+					target->last_recv_wqe = true;
+					wake_up(&target->qp_wq);
+				}
 				srp_handle_qp_err(wc[i].status, wc[i].opcode,
 						  target);
-				return;
 			}
 		}
 	}
@@ -1278,9 +1337,10 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 				iu = (struct srp_iu *) (uintptr_t) wc[i].wr_id;
 				list_add(&iu->list, &target->free_tx);
 			} else {
+				if (wc[i].wr_id == 0)
+					target->last_send_wqe = true;
 				srp_handle_qp_err(wc[i].status, wc[i].opcode,
 						  target);
-				return;
 			}
 		}
 	}
@@ -2231,6 +2291,7 @@ static ssize_t srp_create_target(struct device *dev,
 	spin_lock_init(&target->lock);
 	INIT_LIST_HEAD(&target->free_tx);
 	INIT_LIST_HEAD(&target->free_reqs);
+	init_waitqueue_head(&target->qp_wq);
 	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
 		struct srp_request *req = &target->req_ring[i];
 
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 5f288fe..fc411ae 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -183,6 +183,9 @@ struct srp_target_port {
 	struct completion	done;
 	int			status;
 	bool			qp_in_error;
+	bool			last_recv_wqe;
+	bool			last_send_wqe;
+	wait_queue_head_t	qp_wq;
 
 	struct completion	tsk_mgmt_done;
 	u8			tsk_mgmt_status;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/18] ib_srp: Introduce three helper functions

[Bart Van Assche]

Introduce srp_remove_target(), srp_change_state_to_removed() and
srp_scan_target().

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   49 +++++++++++++++++++++++++----------
 1 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0265a10..57f8a6f 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -428,6 +428,20 @@ static int srp_send_req(struct srp_target_port *target)
 	return status;
 }
 
+static bool srp_change_state_to_removed(struct srp_target_port *target)
+{
+	bool changed = false;
+
+	spin_lock_irq(&target->lock);
+	if (target->state != SRP_TARGET_REMOVED) {
+		target->state = SRP_TARGET_REMOVED;
+		changed = true;
+	}
+	spin_unlock_irq(&target->lock);
+
+	return changed;
+}
+
 static bool srp_change_conn_state(struct srp_target_port *target,
 				  bool connected)
 {
@@ -566,6 +580,17 @@ static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
 		device_remove_file(&shost->shost_dev, *attr);
 }
 
+static void srp_remove_target(struct srp_target_port *target)
+{
+	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);
+	srp_free_target_ib(target);
+	srp_free_req_data(target);
+	scsi_host_put(target->scsi_host);
+}
+
 static void srp_remove_work(struct work_struct *work)
 {
 	struct srp_target_port *target =
@@ -578,13 +603,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);
-	srp_free_target_ib(target);
-	srp_free_req_data(target);
-	scsi_host_put(target->scsi_host);
+	srp_remove_target(target);
 }
 
 static int srp_connect_target(struct srp_target_port *target)
@@ -690,6 +709,12 @@ static void srp_reset_req(struct srp_target_port *target, struct srp_request *re
 	srp_remove_req(target, req, 0);
 }
 
+static void srp_scan_target(struct srp_target_port *target)
+{
+	scsi_scan_target(&target->scsi_host->shost_gendev, 0, target->scsi_id,
+			 SCAN_WILD_CARD, 0);
+}
+
 static int srp_reconnect_target(struct srp_target_port *target)
 {
 	struct ib_qp_attr qp_attr;
@@ -2000,8 +2025,7 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 	target->state = SRP_TARGET_LIVE;
 	target->connected = false;
 
-	scsi_scan_target(&target->scsi_host->shost_gendev,
-			 0, target->scsi_id, SCAN_WILD_CARD, 0);
+	srp_scan_target(target);
 
 	return 0;
 }
@@ -2540,11 +2564,8 @@ static void srp_remove_one(struct ib_device *device)
 		 * commands and don't try to reconnect.
 		 */
 		spin_lock(&host->target_lock);
-		list_for_each_entry(target, &host->target_list, list) {
-			spin_lock_irq(&target->lock);
-			target->state = SRP_TARGET_REMOVED;
-			spin_unlock_irq(&target->lock);
-		}
+		list_for_each_entry(target, &host->target_list, list)
+			srp_change_state_to_removed(target);
 		spin_unlock(&host->target_lock);
 
 		/*
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/18] ib_srp: Eliminate state SRP_TARGET_DEAD

[Bart Van Assche]

Only queue removal work after having changed the target state
into SRP_TARGET_REMOVED and not if that state was already equal
to SRP_TARGET_REMOVED. That allows to remove the state
SRP_TARGET_DEAD. Add a call to srp_disconnect_target() in
srp_remove_target() - due to previous changes it is now safe to
invoke that last function even if the IB connection has already
been disconnected. Rename srp_target_port.work into
srp_target_port.remove_work and move function srp_change_state()
to just before srp_change_state_to_removed().

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   97 +++++++++++++---------------------
 drivers/infiniband/ulp/srp/ib_srp.h |    5 +-
 2 files changed, 39 insertions(+), 63 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 57f8a6f..67932aa 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -428,13 +428,15 @@ static int srp_send_req(struct srp_target_port *target)
 	return status;
 }
 
-static bool srp_change_state_to_removed(struct srp_target_port *target)
+static bool srp_change_state(struct srp_target_port *target,
+			     enum srp_target_state old,
+			     enum srp_target_state new)
 {
 	bool changed = false;
 
 	spin_lock_irq(&target->lock);
-	if (target->state != SRP_TARGET_REMOVED) {
-		target->state = SRP_TARGET_REMOVED;
+	if (target->state == old) {
+		target->state = new;
 		changed = true;
 	}
 	spin_unlock_irq(&target->lock);
@@ -442,6 +444,11 @@ static bool srp_change_state_to_removed(struct srp_target_port *target)
 	return changed;
 }
 
+static bool srp_change_state_to_removed(struct srp_target_port *target)
+{
+	return srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_REMOVED);
+}
+
 static bool srp_change_conn_state(struct srp_target_port *target,
 				  bool connected)
 {
@@ -532,21 +539,6 @@ static void srp_disconnect_target(struct srp_target_port *target)
 	}
 }
 
-static bool srp_change_state(struct srp_target_port *target,
-			    enum srp_target_state old,
-			    enum srp_target_state new)
-{
-	bool changed = false;
-
-	spin_lock_irq(&target->lock);
-	if (target->state == old) {
-		target->state = new;
-		changed = true;
-	}
-	spin_unlock_irq(&target->lock);
-	return changed;
-}
-
 static void srp_free_req_data(struct srp_target_port *target)
 {
 	struct ib_device *ibdev = target->srp_host->srp_dev->dev;
@@ -582,9 +574,12 @@ static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
 
 static void srp_remove_target(struct srp_target_port *target)
 {
+	WARN_ON(target->state != SRP_TARGET_REMOVED);
+
 	srp_del_scsi_host_attr(target->scsi_host);
 	srp_remove_host(target->scsi_host);
 	scsi_remove_host(target->scsi_host);
+	srp_disconnect_target(target);
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
@@ -594,10 +589,9 @@ static void srp_remove_target(struct srp_target_port *target)
 static void srp_remove_work(struct work_struct *work)
 {
 	struct srp_target_port *target =
-		container_of(work, struct srp_target_port, work);
+		container_of(work, struct srp_target_port, remove_work);
 
-	if (!srp_change_state(target, SRP_TARGET_DEAD, SRP_TARGET_REMOVED))
-		return;
+	WARN_ON(target->state != SRP_TARGET_REMOVED);
 
 	spin_lock(&target->srp_host->target_lock);
 	list_del(&target->list);
@@ -766,17 +760,9 @@ err:
 	 * However, we have to defer the real removal because we
 	 * are in the context of the SCSI error handler now, which
 	 * will deadlock if we call scsi_remove_host().
-	 *
-	 * Schedule our work inside the lock to avoid a race with
-	 * the flush_scheduled_work() in srp_remove_one().
 	 */
-	spin_lock_irq(&target->lock);
-	if (target->state == SRP_TARGET_LIVE) {
-		target->state = SRP_TARGET_DEAD;
-		INIT_WORK(&target->work, srp_remove_work);
-		queue_work(ib_wq, &target->work);
-	}
-	spin_unlock_irq(&target->lock);
+	if (srp_change_state_to_removed(target))
+		queue_work(ib_wq, &target->remove_work);
 
 	return ret;
 }
@@ -1384,8 +1370,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 	if (!target->connected)
 		goto err;
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED) {
+	if (target->state == SRP_TARGET_REMOVED) {
 		scmnd->result = DID_BAD_TARGET << 16;
 		scmnd->scsi_done(scmnd);
 		return 0;
@@ -1736,8 +1721,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	struct srp_iu *iu;
 	struct srp_tsk_mgmt *tsk_mgmt;
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
+	if (target->state == SRP_TARGET_REMOVED)
 		return -1;
 
 	init_completion(&target->tsk_mgmt_done);
@@ -2312,6 +2296,7 @@ static ssize_t srp_create_target(struct device *dev,
 			     sizeof (struct srp_indirect_buf) +
 			     target->cmd_sg_cnt * sizeof (struct srp_direct_buf);
 
+	INIT_WORK(&target->remove_work, srp_remove_work);
 	spin_lock_init(&target->lock);
 	INIT_LIST_HEAD(&target->free_tx);
 	INIT_LIST_HEAD(&target->free_reqs);
@@ -2546,8 +2531,7 @@ static void srp_remove_one(struct ib_device *device)
 {
 	struct srp_device *srp_dev;
 	struct srp_host *host, *tmp_host;
-	LIST_HEAD(target_list);
-	struct srp_target_port *target, *tmp_target;
+	struct srp_target_port *target;
 
 	srp_dev = ib_get_client_data(device, &srp_client);
 
@@ -2560,32 +2544,25 @@ static void srp_remove_one(struct ib_device *device)
 		wait_for_completion(&host->released);
 
 		/*
-		 * Mark all target ports as removed, so we stop queueing
-		 * commands and don't try to reconnect.
+		 * Remove all target ports. Wait for any removal tasks that
+		 * may already have been started.
 		 */
 		spin_lock(&host->target_lock);
-		list_for_each_entry(target, &host->target_list, list)
-			srp_change_state_to_removed(target);
-		spin_unlock(&host->target_lock);
-
-		/*
-		 * Wait for any reconnection tasks that may have
-		 * started before we marked our target ports as
-		 * removed, and any target port removal tasks.
-		 */
-		flush_workqueue(ib_wq);
-
-		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);
-			ib_destroy_cm_id(target->cm_id);
-			srp_free_target_ib(target);
-			srp_free_req_data(target);
-			scsi_host_put(target->scsi_host);
+		while (!list_empty(&host->target_list)) {
+			target = list_first_entry(&host->target_list,
+						  struct srp_target_port, list);
+			if (srp_change_state_to_removed(target)) {
+				list_del(&target->list);
+				spin_unlock(&host->target_lock);
+				srp_remove_target(target);
+				spin_lock(&host->target_lock);
+			} else {
+				spin_unlock(&host->target_lock);
+				flush_work_sync(&target->remove_work);
+				spin_lock(&host->target_lock);
+			}
 		}
+		spin_unlock(&host->target_lock);
 
 		kfree(host);
 	}
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index fc411ae..3882adf 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -80,8 +80,7 @@ enum {
 
 enum srp_target_state {
 	SRP_TARGET_LIVE,
-	SRP_TARGET_DEAD,
-	SRP_TARGET_REMOVED
+	SRP_TARGET_REMOVED,
 };
 
 enum srp_iu_type {
@@ -177,7 +176,7 @@ 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 work_struct	remove_work;
 
 	struct list_head	list;
 	struct completion	done;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/18] srp_transport: Fix atttribute registration

[Bart Van Assche]

Register transport attributes after the attribute array has been
set up instead of before. The current code can trigger a race
condition because the code reading the attribute array can run
on another thread than the code that initialized that array.
Make sure that any code reading the attribute array will see all
values written into that array.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: David Dillow <dillowda@ornl.gov>
Cc: Roland Dreier <roland@purestorage.com>
Cc: <stable@kernel.org>
---
 drivers/scsi/scsi_transport_srp.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 21a045e..07c4394 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -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.7

[PATCH 10/18] srp_transport: Simplify attribute initialization code

[Bart Van Assche]

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,
verify whether SRP_RPORT_ATTRS is large enough since it is easy
to forget to update that macro when adding new attributes.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: David Dillow <dillowda@ornl.gov>
Cc: Roland Dreier <roland@purestorage.com>
---
 drivers/scsi/scsi_transport_srp.c |   26 ++++----------------------
 1 files changed, 4 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 07c4394..0d85f79 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -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;
+	BUG_ON(count > ARRAY_SIZE(i->rport_attrs));
 
 	transport_container_register(&i->rport_attr_cont);
 
-- 
1.7.7

[PATCH 11/18] srp_transport: Document sysfs attributes

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: David Dillow <dillowda@ornl.gov>
Cc: Roland Dreier <roland@purestorage.com>
---
 Documentation/ABI/stable/sysfs-transport-srp |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-transport-srp

diff --git a/Documentation/ABI/stable/sysfs-transport-srp b/Documentation/ABI/stable/sysfs-transport-srp
new file mode 100644
index 0000000..7b0d4a5
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -0,0 +1,12 @@
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/port_id
+Date:		June 27, 2007
+KernelVersion:	2.6.24
+Contact:	linux-scsi@vger.kernel.org
+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:	linux-scsi@vger.kernel.org
+Description:	Role of the remote port. Either "SRP Initiator" or "SRP Target".
-- 
1.7.7

[PATCH 12/18] ib_srp: Document sysfs attributes

[Bart Van Assche]

Document the sysfs attributes of the SRP initiator (ib_srp) according
to the rules specified in Documentation/ABI/README.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 Documentation/ABI/stable/sysfs-driver-ib_srp |  173 ++++++++++++++++++++++++++
 1 files changed, 173 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-ib_srp

diff --git a/Documentation/ABI/stable/sysfs-driver-ib_srp b/Documentation/ABI/stable/sysfs-driver-ib_srp
new file mode 100644
index 0000000..0e0224f
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-ib_srp
@@ -0,0 +1,173 @@
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/add_target
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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 eight
+		  byte identifier extension in the 16-byte SRP target port
+		  identifier. The target port identifier is sent by ib_srp
+		  to the target in the SRP_LOGIN_REQ request.
+		* ioc_guid, a 16-digit hexadecimal number specifying the eight
+		  byte I/O controller GUID portion of the 16-byte target port
+		  identifier.
+		* 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 to establish communication with
+		  the SRP target. How to find out the value of the service ID
+		  is specified in the documentation of the SRP target.
+		* max_sect, a decimal number specifying the maximum number of
+		  512-byte sectors to be transferred via a single SCSI command.
+		* max_cmd_per_lun, a decimal number specifying the maximum
+		  number of outstanding commands for a single LUN.
+		* io_class, a hexadecimal number specifying the SRP I/O class.
+		  Must be either 0xff00 (rev 10) or 0x0100 (rev 16a). The I/O
+		  class defines the format of the SRP initiator and target
+		  port identifiers.
+		* initiator_ext, a 16-digit hexadecimal number specifying the
+		  identifier extension portion of the SRP initiator port
+		  identifier. This data is sent by the initiator to the target
+		  in the SRP_LOGIN_REQ request.
+		* cmd_sg_entries, a number in the range 1..255 that specifies
+		  the maximum number of direct data buffer descriptors in an
+		  SRP_CMD. Direct data buffer descriptors are communicated via
+		  the SRP_CMD header. With allow_ext_sg=0 the parameter
+		  cmd_sg_entries defines the maximum S/G list length for a
+		  single SRP_CMD. Also, commands whose S/G list length exceeds
+		  this limit after S/G list collapsing will fail.
+		* allow_ext_sg, whether ib_srp is allowed to use indirect data
+		  buffer descriptors when communicating with an SRP target.
+		  Indirect data buffer descriptors are communicated between
+		  initiator and target via an additional RDMA
+		  transfer. Enabling indirect data buffer descriptors
+		  increases the maximum amount of data that can be transferred
+		  via a single SCSI command between initiator and
+		  target. Since not all SRP target implementations support
+		  indirect data buffer descriptors by default support for
+		  indirect data buffer descriptors is disabled.
+		* sg_tablesize, a number in the range 1..2048 specifying the
+		  maximum S/G list length the SCSI layer is allowed to pass to
+		  ib_srp. Specifying a value that exceeds cmd_sg_entries is
+		  only safe with indirect data buffer support enabled
+		  (allow_ext_sg=1).
+
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/ibdev
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	HCA name (<hca>).
+
+What:		/sys/class/infiniband_srp/srp-<hca>-<port_number>/port
+Date:		January 2, 2006
+KernelVersion:	2.6.15
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Whether ib_srp is allowed to use indirect data buffer
+		descriptors when communicating with an SRP target.
+
+What:		/sys/class/scsi_host/host<n>/cmd_sg_entries
+Date:		May 19, 2011
+KernelVersion:	2.6.39
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Maximum number of direct data buffer descriptors to send to
+		the target in a single SRP_CMD.
+
+What:		/sys/class/scsi_host/host<n>/dgid
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Number of consecutive failed SRP reconnection attempts.
+
+What:		/sys/class/scsi_host/host<n>/id_ext
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Eight-byte identifier extension portion of the 16-byte target
+		port identifier.
+
+What:		/sys/class/scsi_host/host<n>/ioc_guid
+Date:		June 17, 2006
+KernelVersion:	2.6.17
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Eight-byte I/O controller GUID portion of the 16-byte target
+		port identifier.
+
+What:		/sys/class/scsi_host/host<n>/local_ib_device
+Date:		November 29, 2006
+KernelVersion:	2.6.19
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	InfiniBand service ID used for establishing communication with
+		the SRP	target.
+
+What:		/sys/class/scsi_host/host<n>/zero_req_lim
+Date:		September 20, 2006
+KernelVersion:	2.6.18
+Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/18] ib_srp: Allow SRP disconnect through sysfs

[Bart Van Assche]

Make it possible to disconnect 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.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Cc: FUJITA Tomonori <fujita.tomonori-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
Cc: Brian King <brking-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
---
 Documentation/ABI/stable/sysfs-transport-srp |    7 +++++++
 drivers/infiniband/ulp/srp/ib_srp.c          |   26 +++++++++++++++++++++++---
 drivers/infiniband/ulp/srp/ib_srp.h          |    1 +
 drivers/scsi/scsi_transport_srp.c            |   20 +++++++++++++++++++-
 include/scsi/scsi_transport_srp.h            |   10 ++++++++++
 5 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp b/Documentation/ABI/stable/sysfs-transport-srp
index 7b0d4a5..9b78ace 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -1,3 +1,10 @@
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/delete
+Date:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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>/port_id
 Date:		June 27, 2007
 KernelVersion:	2.6.24
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 67932aa..e8b699b 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -600,6 +600,21 @@ static void srp_remove_work(struct work_struct *work)
 	srp_remove_target(target);
 }
 
+/*
+ * Note: it is up to the caller to ensure that invoking srp_rport_delete()
+ * does not trigger a race against target port removal in srp_remove_target().
+ * As an example, invoking srp_rport_delete() from the SCSI EH is not safe.
+ */
+static void srp_rport_delete(struct srp_rport *rport)
+{
+	struct srp_target_port *target = rport->lld_data;
+
+	BUG_ON(!target);
+
+	if (srp_change_state_to_removed(target))
+		queue_work(system_long_wq, &target->remove_work);
+}
+
 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;
@@ -1982,6 +1997,10 @@ static struct scsi_host_template srp_template = {
 	.shost_attrs			= srp_host_attrs
 };
 
+static struct srp_function_template ib_srp_transport_functions = {
+	.rport_delete		 = srp_rport_delete,
+};
+
 static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 {
 	struct srp_rport_identifiers ids;
@@ -2002,6 +2021,10 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 		return PTR_ERR(rport);
 	}
 
+	rport->ft = &ib_srp_transport_functions;
+	rport->lld_data = target;
+	target->rport = rport;
+
 	spin_lock(&host->target_lock);
 	list_add_tail(&target->list, &host->target_list);
 	spin_unlock(&host->target_lock);
@@ -2575,9 +2598,6 @@ static void srp_remove_one(struct ib_device *device)
 	kfree(srp_dev);
 }
 
-static struct srp_function_template ib_srp_transport_functions = {
-};
-
 static int __init srp_init_module(void)
 {
 	int ret;
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 3882adf..22e0c5d 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -154,6 +154,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;
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 0d85f79..35f85bc 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -38,7 +38,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 2
+#define SRP_RPORT_ATTRS 3
 
 struct srp_internal {
 	struct scsi_transport_template t;
@@ -116,6 +116,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 void srp_rport_release(struct device *dev)
 {
 	struct srp_rport *rport = dev_to_rport(dev);
@@ -309,6 +325,8 @@ srp_attach_transport(struct srp_function_template *ft)
 	count = 0;
 	i->rport_attrs[count++] = &dev_attr_port_id;
 	i->rport_attrs[count++] = &dev_attr_roles;
+	if (ft->rport_delete)
+		i->rport_attrs[count++] = &dev_attr_delete;
 	i->rport_attrs[count++] = NULL;
 	BUG_ON(count > ARRAY_SIZE(i->rport_attrs));
 
diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
index 9c60ca1..1a109ff 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -14,13 +14,23 @@ struct srp_rport_identifiers {
 };
 
 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 */
 };
 
 struct srp_function_template {
+	/* for initiator drivers */
+	void (*rport_delete)(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);
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/18] ib_srp: Move target port removal code

[Bart Van Assche]

Move the code for removing a target port if reconnecting during
a reset triggered by the SCSI mid-layer fails from inside
srp_reconnect_target() into srp_reset_host().

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index e8b699b..a8a3a2f 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -764,21 +764,7 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	if (ret)
 		goto err;
 
-	return ret;
-
 err:
-	shost_printk(KERN_ERR, target->scsi_host,
-		     PFX "reconnect failed (%d), removing target port.\n", ret);
-
-	/*
-	 * We couldn't reconnect, so kill our target port off.
-	 * However, we have to defer the real removal because we
-	 * are in the context of the SCSI error handler now, which
-	 * will deadlock if we call scsi_remove_host().
-	 */
-	if (srp_change_state_to_removed(target))
-		queue_work(ib_wq, &target->remove_work);
-
 	return ret;
 }
 
@@ -1825,14 +1811,26 @@ static int srp_reset_device(struct scsi_cmnd *scmnd)
 static int srp_reset_host(struct scsi_cmnd *scmnd)
 {
 	struct srp_target_port *target = host_to_target(scmnd->device->host);
-	int ret = FAILED;
+	int res;
 
 	shost_printk(KERN_ERR, target->scsi_host, PFX "SRP reset_host called\n");
 
-	if (!srp_reconnect_target(target))
-		ret = SUCCESS;
+	res = srp_reconnect_target(target);
+	if (res == 0)
+		return SUCCESS;
 
-	return ret;
+	/*
+	 * We couldn't reconnect, so kill our target port off.  However, we
+	 * have to defer the real removal because we are in the context of the
+	 * SCSI error handler now, which will deadlock if we call
+	 * scsi_remove_host().
+	 */
+	shost_printk(KERN_ERR, target->scsi_host,
+		     PFX "reconnect failed (%d), removing target port.\n",
+		     res);
+	if (srp_change_state_to_removed(target))
+		queue_work(system_long_wq, &target->remove_work);
+	return FAILED;
 }
 
 static int srp_slave_configure(struct scsi_device *sdev)
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/18] ib_srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

The sysfs attribute 'add_target' may be used to relogin to a
target. An SRP target that receives a second login request from
an initiator will disconnect the previous connection. So before
trying to reconnect, check whether another connection to the
same SRP target identifier already exists. If so, remove the
target port.  Add a target to the target list before connecting
instead of after such that this algorithm has a chance to work.

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   97 +++++++++++++++++++++++++++--------
 drivers/infiniband/ulp/srp/ib_srp.h |   10 ++++
 2 files changed, 86 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a8a3a2f..eb31a14 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -574,16 +574,20 @@ static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
 
 static void srp_remove_target(struct srp_target_port *target)
 {
+	struct Scsi_Host *shost = target->scsi_host;
+
 	WARN_ON(target->state != SRP_TARGET_REMOVED);
 
-	srp_del_scsi_host_attr(target->scsi_host);
-	srp_remove_host(target->scsi_host);
-	scsi_remove_host(target->scsi_host);
+	srp_del_scsi_host_attr(shost);
+	if (target->scsi_host_added) {
+		srp_remove_host(shost);
+		scsi_remove_host(shost);
+	}
 	srp_disconnect_target(target);
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
-	scsi_host_put(target->scsi_host);
+	scsi_host_put(shost);
 }
 
 static void srp_remove_work(struct work_struct *work)
@@ -615,6 +619,32 @@ static void srp_rport_delete(struct srp_rport *rport)
 		queue_work(system_long_wq, &target->remove_work);
 }
 
+/**
+ * srp_conn_unique() - Check whether the connection to a target is unique.
+ *
+ * Connections are compared by their SCSI target port identifier: identifier
+ * extension + IO controller GUID. See also paragraph B.5 in the SRP spec.
+ */
+static bool srp_conn_unique(struct srp_host *host,
+			    struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	bool ret = true;
+
+	spin_lock(&host->target_lock);
+	list_for_each_entry(t, &host->target_list, list) {
+		if (t != target &&
+		    target->id_ext == t->id_ext &&
+		    target->ioc_guid == t->ioc_guid) {
+			ret = false;
+			break;
+		}
+	}
+	spin_unlock(&host->target_lock);
+
+	return ret;
+}
+
 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;
@@ -728,9 +758,21 @@ static int srp_reconnect_target(struct srp_target_port *target)
 {
 	struct ib_qp_attr qp_attr;
 	int i, ret;
+	bool unique;
 
-	if (target->state != SRP_TARGET_LIVE)
+	unique = srp_conn_unique(target->srp_host, target);
+	if (unique) {
+	} else if (srp_change_state_to_removed(target)) {
+		shost_printk(KERN_INFO, target->scsi_host,
+			     PFX "deleting SCSI host because obsolete.\n");
+		queue_work(system_long_wq, &target->remove_work);
+		return -ENXIO;
+	}
+	if (target->state == SRP_TARGET_REMOVED) {
+		shost_printk(KERN_DEBUG, target->scsi_host,
+			     PFX "Already removed\n");
 		return -EAGAIN;
+	}
 
 	srp_disconnect_target(target);
 	/*
@@ -2023,15 +2065,6 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 	rport->lld_data = target;
 	target->rport = rport;
 
-	spin_lock(&host->target_lock);
-	list_add_tail(&target->list, &host->target_list);
-	spin_unlock(&host->target_lock);
-
-	target->state = SRP_TARGET_LIVE;
-	target->connected = false;
-
-	srp_scan_target(target);
-
 	return 0;
 }
 
@@ -2317,6 +2350,7 @@ static ssize_t srp_create_target(struct device *dev,
 			     sizeof (struct srp_indirect_buf) +
 			     target->cmd_sg_cnt * sizeof (struct srp_direct_buf);
 
+	mutex_init(&target->mutex);
 	INIT_WORK(&target->remove_work, srp_remove_work);
 	spin_lock_init(&target->lock);
 	INIT_LIST_HEAD(&target->free_tx);
@@ -2363,24 +2397,45 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err_free_ib;
 
+	target->connected = false;
+	target->rq_tmo_jiffies = 1 * HZ;
+	target->state = SRP_TARGET_CONNECTING;
+	target->scsi_host_added = false;
+
+	spin_lock(&host->target_lock);
+	list_add_tail(&target->list, &host->target_list);
+	spin_unlock(&host->target_lock);
+
+	mutex_lock(&target->mutex);
+	if (!srp_change_state(target, SRP_TARGET_CONNECTING, SRP_TARGET_LIVE)) {
+		ret = -ENOENT;
+		goto err_unlock_remove;
+	}
+
 	ret = srp_connect_target(target);
 	if (ret) {
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "Connection failed\n");
-		goto err_cm_id;
+		goto err_unlock_remove;
 	}
 
 	ret = srp_add_target(host, target);
+	target->scsi_host_added = ret == 0;
+	mutex_unlock(&target->mutex);
+
+	srp_scan_target(target);
+
 	if (ret)
-		goto err_disconnect;
+		goto err_remove;
 
 	return count;
 
-err_disconnect:
-	srp_disconnect_target(target);
-
-err_cm_id:
-	ib_destroy_cm_id(target->cm_id);
+err_unlock_remove:
+	mutex_unlock(&target->mutex);
+err_remove:
+	if (srp_change_state_to_removed(target))
+		srp_remove_work(&target->remove_work);
+	return ret;
 
 err_free_ib:
 	srp_free_target_ib(target);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 22e0c5d..a603c6d 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -78,7 +78,15 @@ enum {
 	SRP_MAP_NO_FMR		= 1,
 };
 
+/**
+ * enum srp_target_state - State of the SCSI host associated with an SRP target.
+ * @SRP_TARGET_CONNECTING: IB connection being established and SCSI host being
+ *                      added.
+ * @SRP_TARGET_LIVE: IB RC connection is established and SCSI host is unblocked.
+ * @SRP_TARGET_REMOVED: SCSI host removal is pending.
+ */
 enum srp_target_state {
+	SRP_TARGET_CONNECTING,
 	SRP_TARGET_LIVE,
 	SRP_TARGET_REMOVED,
 };
@@ -166,6 +174,7 @@ struct srp_target_port {
 
 	u32			rq_tmo_jiffies;
 	bool			connected;
+	bool			scsi_host_added;
 
 	struct ib_cm_id	       *cm_id;
 
@@ -186,6 +195,7 @@ struct srp_target_port {
 	bool			last_recv_wqe;
 	bool			last_send_wqe;
 	wait_queue_head_t	qp_wq;
+	struct mutex		mutex;
 
 	struct completion	tsk_mgmt_done;
 	u8			tsk_mgmt_status;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 16/18] scsi: Add scsi_host_template.slave_delete callback

[Bart Van Assche]

Add a callback function in the SCSI host template that is invoked
just before device removal. This allows drivers that keep a
reference to the device itself to remove that reference.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: James Bottomley <JBottomley@parallels.com>
Cc: David Dillow <dillowda@ornl.gov>
---
 drivers/scsi/scsi_sysfs.c |    7 ++++++-
 include/scsi/scsi_host.h  |    9 +++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..ea5658d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -572,7 +572,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
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5f7d5b3..7c92948 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -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
-- 
1.7.7

[PATCH 17/18] srp_transport: Add transport layer recovery support

[Bart Van Assche]

Add the necessary functions in the SRP transport module to allow
an SRP initiator driver to implement transport layer recovery.
This includes:
- A ping mechanism to check whether the transport layer is still
  operational.
- Support for implementing fast_io_fail_tmo, the time that should
  elapse after having detected a transport layer problem and
  before failing I/O.
- Support for implementing dev_loss_tmo, the time that should
  elapse after having detected a transport layer problem and
  before removing a remote port.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Cc: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: Brian King <brking@linux.vnet.ibm.com>
Cc: David Dillow <dillowda@ornl.gov>
---
 drivers/scsi/scsi_transport_srp.c |  430 ++++++++++++++++++++++++++++++++++++-
 include/scsi/scsi_transport_srp.h |   33 +++
 2 files changed, 462 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 35f85bc..a7628c9 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -2,6 +2,7 @@
  * SCSI RDMA (SRP) transport class
  *
  * Copyright (C) 2007 FUJITA Tomonori <tomof@acm.org>
+ * Copyright (C) 2011 Bart Van Assche <bvanassche@acm.org>
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -28,8 +29,10 @@
 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_eh.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_transport_srp.h>
+#include "scsi_priv.h"
 #include "scsi_transport_srp_internal.h"
 
 struct srp_host_attrs {
@@ -38,7 +41,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 3
+#define SRP_RPORT_ATTRS 7
 
 struct srp_internal {
 	struct scsi_transport_template t;
@@ -132,6 +135,415 @@ static ssize_t store_srp_rport_delete(struct device *dev,
 
 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)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+
+	return sprintf(buf, "%d\n", rport->ping_itv);
+}
+
+static ssize_t store_srp_rport_ping_interval(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+	char ch[16];
+	int res, ping_itv;
+
+	sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
+	res = kstrtoint(ch, 0, &ping_itv);
+	if (res)
+		goto out;
+	res = -EINVAL;
+	if (ping_itv > ULONG_MAX / HZ)
+		goto out;
+	rport->ping_itv = ping_itv;
+	if (ping_itv > 0)
+		queue_delayed_work(system_long_wq, &rport->ping_work,
+				   1UL * ping_itv * HZ);
+	else
+		cancel_delayed_work(&rport->ping_work);
+	res = count;
+out:
+	return res;
+}
+
+static DEVICE_ATTR(ping_interval, S_IRUGO | S_IWUSR,
+		   show_srp_rport_ping_interval, store_srp_rport_ping_interval);
+
+static ssize_t show_srp_rport_ping_timeout(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+
+	return sprintf(buf, "%d\n", rport->ping_tmo);
+}
+
+static ssize_t store_srp_rport_ping_timeout(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+	char ch[16];
+	int res;
+
+	sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
+	res = kstrtoint(ch, 0, &rport->ping_tmo);
+	return res == 0 ? count : res;
+}
+
+static DEVICE_ATTR(ping_timeout, S_IRUGO | S_IWUSR, show_srp_rport_ping_timeout,
+		   store_srp_rport_ping_timeout);
+
+/**
+ * srp_tmo_valid() - Check timeout combination validity.
+ *
+ * If no fast I/O fail timeout has been configured then the device loss timeout
+ * must be below SCSI_DEVICE_BLOCK_MAX_TIMEOUT. If a fast I/O fail timeout has
+ * been configured then it must be below the device loss timeout.
+ */
+static int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+	return (fast_io_fail_tmo < 0 &&
+		0 <= dev_loss_tmo &&
+		dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+		|| (0 <= fast_io_fail_tmo &&
+		    fast_io_fail_tmo < dev_loss_tmo &&
+		    dev_loss_tmo < ULONG_MAX / HZ) ? 0 : -EINVAL;
+}
+
+static ssize_t show_srp_rport_fast_io_fail_tmo(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+
+	if (rport->fast_io_fail_tmo >= 0)
+		return sprintf(buf, "%d\n", rport->fast_io_fail_tmo);
+	else
+		return sprintf(buf, "off\n");
+}
+
+static ssize_t store_srp_rport_fast_io_fail_tmo(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+	char ch[16];
+	int res;
+	int fast_io_fail_tmo;
+
+	if (count >= 3 && memcmp(buf, "off", 3) == 0) {
+		fast_io_fail_tmo = -1;
+	} else {
+		sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
+		res = kstrtoint(ch, 0, &fast_io_fail_tmo);
+		if (res)
+			goto out;
+	}
+	res = srp_tmo_valid(fast_io_fail_tmo, rport->dev_loss_tmo);
+	if (res)
+		goto out;
+	rport->fast_io_fail_tmo = fast_io_fail_tmo;
+	if (rport->state == SRP_RPORT_BLOCKED)
+		srp_start_tl_fail_timers(rport);
+	res = count;
+out:
+	return res;
+}
+
+static DEVICE_ATTR(fast_io_fail_tmo, S_IRUGO | S_IWUSR,
+		   show_srp_rport_fast_io_fail_tmo,
+		   store_srp_rport_fast_io_fail_tmo);
+
+static ssize_t show_srp_rport_dev_loss_tmo(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+
+	return sprintf(buf, "%d\n", rport->dev_loss_tmo);
+}
+
+static ssize_t store_srp_rport_dev_loss_tmo(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t count)
+{
+	struct srp_rport *rport = transport_class_to_srp_rport(dev);
+	char ch[16];
+	int res;
+	int dev_loss_tmo;
+
+	sprintf(ch, "%.*s", min_t(int, sizeof(ch) - 1, count), buf);
+	res = kstrtoint(ch, 0, &dev_loss_tmo);
+	if (res)
+		goto out;
+	res = srp_tmo_valid(rport->fast_io_fail_tmo, dev_loss_tmo);
+	if (res)
+		goto out;
+	rport->dev_loss_tmo = dev_loss_tmo;
+	if (rport->state == SRP_RPORT_BLOCKED)
+		srp_start_tl_fail_timers(rport);
+	res = count;
+out:
+	return res;
+}
+
+static DEVICE_ATTR(dev_loss_tmo, S_IRUGO | S_IWUSR,
+		   show_srp_rport_dev_loss_tmo,
+		   store_srp_rport_dev_loss_tmo);
+
+static bool srp_unblock_rport(struct srp_rport *rport)
+{
+	unsigned long flags;
+	enum srp_rport_state prev_state;
+	bool unblock;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	prev_state = rport->state;
+	if (prev_state == SRP_RPORT_BLOCKED) {
+		rport->state = SRP_RPORT_LIVE;
+		unblock = true;
+	}
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	if (unblock) {
+		dev_info(&rport->dev, "unblocked SRP rport\n");
+		scsi_target_unblock(rport->dev.parent);
+	}
+
+	return unblock;
+}
+
+/**
+ * rport_fast_io_fail_timedout() - Fast I/O failure timeout handler.
+ *
+ * Unblocks the SCSI host.
+ */
+static void rport_fast_io_fail_timedout(struct work_struct *work)
+{
+	struct srp_rport *rport;
+
+	rport = container_of(to_delayed_work(work), struct srp_rport,
+			     fast_io_fail_work);
+
+	pr_err("SRP transport: fast_io_fail_tmo (%ds) expired - unblocking %s.\n",
+	       rport->fast_io_fail_tmo, dev_name(&rport->dev));
+
+	BUG_ON(!rport->ft);
+
+	/* Involve the LLDD if possible to terminate all io on the rport. */
+	if (rport->ft->terminate_rport_io)
+		rport->ft->terminate_rport_io(rport);
+
+	srp_unblock_rport(rport);
+}
+
+/**
+ * rport_dev_loss_timedout() - Device loss timeout handler.
+ *
+ * Note: rport->ft->rport_delete must either unblock the SCSI host or schedule
+ * SCSI host removal.
+ */
+static void rport_dev_loss_timedout(struct work_struct *work)
+{
+	struct srp_rport *rport;
+
+	rport = container_of(to_delayed_work(work), struct srp_rport,
+			     dev_loss_work);
+
+	pr_err("SRP transport: dev_loss_tmo (%ds) expired - removing %s.\n",
+	       rport->dev_loss_tmo, dev_name(&rport->dev));
+
+	BUG_ON(!rport->ft);
+	BUG_ON(!rport->ft->rport_delete);
+
+	rport->ft->rport_delete(rport);
+}
+
+/**
+ * srp_resume_io() - Unblock an rport and cancel the failure timers.
+ */
+void srp_resume_io(struct srp_rport *rport)
+{
+	unsigned long flags;
+
+	if (!srp_unblock_rport(rport))
+		return;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	rport->latest_ping_response = jiffies;
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	cancel_delayed_work(&rport->fast_io_fail_work);
+	cancel_delayed_work(&rport->dev_loss_work);
+}
+EXPORT_SYMBOL(srp_resume_io);
+
+/**
+ * srp_block_rport() - Block an rport.
+ */
+void srp_block_rport(struct srp_rport *rport)
+{
+	unsigned long flags;
+	bool block = false;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	if (rport->state == SRP_RPORT_LIVE) {
+		rport->state = SRP_RPORT_BLOCKED;
+		block = true;
+	}
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	if (block) {
+		dev_info(&rport->dev, "blocked SRP rport\n");
+		scsi_target_block(rport->dev.parent);
+	}
+}
+EXPORT_SYMBOL(srp_block_rport);
+
+/**
+ * srp_start_tl_fail_timers() - Start the transport layer failure timers.
+ *
+ * Start the transport layer fast I/O failure and device loss timers. Do not
+ * modify a timer that was already started.
+ */
+void srp_start_tl_fail_timers(struct srp_rport *rport)
+{
+	if (rport->fast_io_fail_tmo >= 0)
+		queue_delayed_work(system_long_wq, &rport->fast_io_fail_work,
+				   1UL * rport->fast_io_fail_tmo * HZ);
+	WARN_ON(rport->dev_loss_tmo < 0);
+	queue_delayed_work(system_long_wq, &rport->dev_loss_work,
+			   1UL * rport->dev_loss_tmo * HZ);
+}
+EXPORT_SYMBOL(srp_start_tl_fail_timers);
+
+/**
+ * srp_rport_disable_ping() - Stop pinging and prevent reenabling pinging.
+ */
+void srp_rport_disable_ping(struct srp_rport *rport)
+{
+	struct device *dev = rport->dev.parent;
+	struct Scsi_Host *shost = dev ? dev_to_shost(dev) : NULL;
+
+	if (dev) {
+		WARN_ON(!shost);
+		if (shost)
+			WARN_ON(shost->host_self_blocked);
+	}
+	device_remove_file(dev, &dev_attr_ping_timeout);
+	device_remove_file(dev, &dev_attr_ping_interval);
+	cancel_delayed_work_sync(&rport->ping_work);
+}
+EXPORT_SYMBOL(srp_rport_disable_ping);
+
+/**
+ * srp_disable_tl_fail_timers() - Disable the transport layer failure timers.
+ */
+void srp_disable_tl_fail_timers(struct srp_rport *rport)
+{
+	struct device *dev = rport->dev.parent;
+
+	device_remove_file(dev, &dev_attr_fast_io_fail_tmo);
+	device_remove_file(dev, &dev_attr_dev_loss_tmo);
+	cancel_delayed_work_sync(&rport->fast_io_fail_work);
+	cancel_delayed_work_sync(&rport->dev_loss_work);
+}
+EXPORT_SYMBOL(srp_disable_tl_fail_timers);
+
+/**
+ * srp_stop_rport() - Stop asynchronous work for an rport.
+ */
+void srp_stop_rport(struct srp_rport *rport)
+{
+	srp_rport_disable_ping(rport);
+	srp_disable_tl_fail_timers(rport);
+}
+EXPORT_SYMBOL(srp_stop_rport);
+
+/**
+ * srp_rport_set_sdev() - Set the SCSI device that will be used for pinging.
+ */
+void srp_rport_set_sdev(struct srp_rport *rport, struct scsi_device *sdev)
+{
+	unsigned long flags;
+
+	if (sdev && !get_device(&sdev->sdev_dev))
+		sdev = NULL;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	swap(rport->sdev, sdev);
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	if (sdev)
+		put_device(&sdev->sdev_dev);
+}
+EXPORT_SYMBOL(srp_rport_set_sdev);
+
+/**
+ * srp_rport_get_sdev() - Get the SCSI device to be used for pinging.
+ */
+static struct scsi_device *rport_get_sdev(struct srp_rport *rport)
+{
+	struct scsi_device *sdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&rport->lock, flags);
+	sdev = rport->sdev;
+	if (sdev && !get_device(&sdev->sdev_dev))
+		sdev = NULL;
+	spin_unlock_irqrestore(&rport->lock, flags);
+
+	return sdev;
+}
+
+/**
+ * rport_ping() - Verify whether the transport layer is still operational.
+ */
+static void rport_ping(struct work_struct *work)
+{
+	struct scsi_sense_hdr sshdr;
+	struct srp_rport *rport;
+	struct scsi_device *sdev;
+	int res, itv, tmo;
+
+	rport = container_of(work, struct srp_rport, ping_work.work);
+	itv = rport->ping_itv;
+	tmo = rport->ping_tmo;
+	sdev = rport_get_sdev(rport);
+	if (itv <= 0)
+		goto out;
+	if (!sdev)
+		goto schedule;
+	if (rport->state == SRP_RPORT_BLOCKED)
+		goto put;
+	memset(&sshdr, 0, sizeof(sshdr));
+	res = scsi_test_unit_ready(sdev, itv, 1, NULL);
+	pr_debug("scsi_test_unit_ready() result = 0x%x / %s%s\n", res,
+		 scsi_sense_valid(&sshdr) ? "sense valid" : "sense not valid",
+		 scsi_sense_valid(&sshdr) &&
+		 sshdr.sense_key == UNIT_ATTENTION ? " (unit attention)" : "");
+	if (scsi_status_is_good(res) || (res & SAM_STAT_CHECK_CONDITION)) {
+		rport->latest_ping_response = jiffies;
+	} else if (tmo > 0 &&
+		   time_after(jiffies, rport->latest_ping_response + tmo)) {
+		shost_printk(KERN_INFO, sdev->host,
+			     "SRP ping timeout elapsed\n");
+		if (rport->ft->rport_ping_timedout)
+			rport->ft->rport_ping_timedout(rport);
+	}
+put:
+	put_device(&sdev->sdev_dev);
+schedule:
+	queue_delayed_work(system_long_wq, &rport->ping_work, itv * HZ);
+out:
+	return;
+}
+
 static void srp_rport_release(struct device *dev)
 {
 	struct srp_rport *rport = dev_to_rport(dev);
@@ -208,6 +620,15 @@ struct srp_rport *srp_rport_add(struct Scsi_Host *shost,
 	memcpy(rport->port_id, ids->port_id, sizeof(rport->port_id));
 	rport->roles = ids->roles;
 
+	rport->fast_io_fail_tmo = -1;
+	rport->dev_loss_tmo = 60;
+	INIT_DELAYED_WORK(&rport->ping_work, rport_ping);
+	INIT_DELAYED_WORK(&rport->fast_io_fail_work,
+			  rport_fast_io_fail_timedout);
+	INIT_DELAYED_WORK(&rport->dev_loss_work, rport_dev_loss_timedout);
+	spin_lock_init(&rport->lock);
+	rport->state = SRP_RPORT_LIVE;
+
 	id = atomic_inc_return(&to_srp_host_attrs(shost)->next_port_id);
 	dev_set_name(&rport->dev, "port-%d:%d", shost->host_no, id);
 
@@ -325,6 +746,13 @@ srp_attach_transport(struct srp_function_template *ft)
 	count = 0;
 	i->rport_attrs[count++] = &dev_attr_port_id;
 	i->rport_attrs[count++] = &dev_attr_roles;
+	if (ft->rport_ping_timedout) {
+		i->rport_attrs[count++] = &dev_attr_ping_interval;
+		i->rport_attrs[count++] = &dev_attr_ping_timeout;
+		i->rport_attrs[count++] = &dev_attr_fast_io_fail_tmo;
+		if (ft->rport_delete)
+			i->rport_attrs[count++] = &dev_attr_dev_loss_tmo;
+	}
 	if (ft->rport_delete)
 		i->rport_attrs[count++] = &dev_attr_delete;
 	i->rport_attrs[count++] = NULL;
diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
index 1a109ff..896b490 100644
--- a/include/scsi/scsi_transport_srp.h
+++ b/include/scsi/scsi_transport_srp.h
@@ -8,6 +8,16 @@
 #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;
@@ -26,10 +36,25 @@ struct srp_rport {
 	/* for initiator drivers */
 
 	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			fast_io_fail_tmo;
+	int			dev_loss_tmo;
+	struct delayed_work	ping_work;
+	struct delayed_work	fast_io_fail_work;
+	struct delayed_work	dev_loss_work;
 };
 
 struct srp_function_template {
 	/* for initiator drivers */
+	void (*rport_ping_timedout) (struct srp_rport *rport);
+	void (*terminate_rport_io)(struct srp_rport *rport);
 	void (*rport_delete)(struct srp_rport *rport);
 	/* for target drivers */
 	int (* tsk_mgmt_response)(struct Scsi_Host *, u64, u64, int);
@@ -43,6 +68,14 @@ 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_resume_io(struct srp_rport *rport);
+extern void srp_block_rport(struct srp_rport *rport);
+extern void srp_start_tl_fail_timers(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_disable_tl_fail_timers(struct srp_rport *rport);
+extern void srp_stop_rport(struct srp_rport *rport);
 
 extern void srp_remove_host(struct Scsi_Host *);
 
-- 
1.7.7

[PATCH 18/18] ib_srp: Rework error handling

[Bart Van Assche]

Add fast_io_fail_tmo and dev_loss_tmo sysfs attributes. Block
the SCSI target as soon as a transport layer error has been
detected (ping timeout, disconnect or IB error completion). Try
to reconnect until dev_loss_tmo elapses.

Disconnect the IB connection earlier in srp_remove_target() to
make sure that error recovery is not triggered during host
removal. Swap the "connected" and "removed" tests in
srp_queuecommand() because of this change.

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

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 Documentation/ABI/stable/sysfs-transport-srp |   35 ++++
 drivers/infiniband/ulp/srp/ib_srp.c          |  248 ++++++++++++++++++++++++--
 drivers/infiniband/ulp/srp/ib_srp.h          |    9 +
 3 files changed, 280 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp b/Documentation/ABI/stable/sysfs-transport-srp
index 9b78ace..85d1eba 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -5,6 +5,41 @@ Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
 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>/dev_loss_tmo
+Date:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Number of seconds the SCSI layer will wait after a transport
+		layer error has been observed before removing a target port.
+		Zero means immediate removal.
+
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/fast_io_fail_tmo
+Date:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+Description:	Number of seconds the SCSI layer will wait after a transport
+		layer error has been observed before failing I/O. Zero means
+		immediate removal. A negative value will disable this
+		behavior.
+
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/ping_interval
+Date:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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:		January 1, 2012
+KernelVersion:	3.3
+Contact:	linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+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
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index eb31a14..36a55e0 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2005 Cisco Systems.  All rights reserved.
+ * Copyright (c) 2010-2011 Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>.
  *
  * This software is available to you under a choice of one of two
  * licenses.  You may choose to be licensed under the terms of the GNU
@@ -446,7 +447,16 @@ static bool srp_change_state(struct srp_target_port *target,
 
 static bool srp_change_state_to_removed(struct srp_target_port *target)
 {
-	return srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_REMOVED);
+	bool changed = false;
+
+	spin_lock_irq(&target->lock);
+	if (target->state != SRP_TARGET_REMOVED) {
+		target->state = SRP_TARGET_REMOVED;
+		changed = true;
+	}
+	spin_unlock_irq(&target->lock);
+
+	return changed;
 }
 
 static bool srp_change_conn_state(struct srp_target_port *target,
@@ -511,7 +521,8 @@ static void srp_wait_last_send_wqe(struct srp_target_port *target)
 	WARN_ON(!target->last_send_wqe);
 }
 
-static void srp_disconnect_target(struct srp_target_port *target)
+static void srp_disconnect_target(struct srp_target_port *target,
+				  bool cancel_block_work)
 {
 	struct ib_qp_attr qp_attr;
 	int ret;
@@ -537,6 +548,9 @@ static void srp_disconnect_target(struct srp_target_port *target)
 
 		srp_wait_last_send_wqe(target);
 	}
+
+	if (cancel_block_work)
+		cancel_work_sync(&target->block_work);
 }
 
 static void srp_free_req_data(struct srp_target_port *target)
@@ -572,18 +586,72 @@ static void srp_del_scsi_host_attr(struct Scsi_Host *shost)
 		device_remove_file(&shost->shost_dev, *attr);
 }
 
+/**
+ * srp_disable_ping() - Stop pinging a target.
+ *
+ * Note: Can be invoked concurrently via the SCSI host sysfs attribute "delete"
+ * and one of the rport callback functions.
+ */
+static void srp_disable_ping(struct scsi_device *sdev)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct srp_target_port *target = host_to_target(shost);
+	struct srp_rport *rport = target->rport;
+
+	if (rport->sdev == sdev) {
+		shost_printk(KERN_DEBUG, target->scsi_host,
+			     PFX "Disabled pinging\n");
+		srp_rport_set_sdev(rport, NULL);
+		srp_rport_disable_ping(rport);
+	}
+}
+
+/*
+ * srp_remove_target() - Remove an SRP target.
+ *
+ * The strategy to remove a target is as follows:
+ * - The caller must have set target->state to SRP_TARGET_REMOVED before
+ *   invoking or queueing this function such that new calls to
+ *   srp_disconnect_target(), srp_reconnect_target() or srp_block_work() do
+ *   not have any effect.
+ * - Remove the sysfs attributes registered by ib_srp such that the registered
+ *   sysfs callback functions won't be invoked anymore.
+ * - Disconnect the IB connection, wait until processing completions finished
+ *   and cancel block_work.
+ * - Unblock the rport such that srp_stop_rport() doesn't deadlock.
+ * - Cancel any asynchronous work started by the SRP transport layer.
+ * - Invoke scsi_remove_host() such that all pending SCSI commands get killed.
+ *   See e.g. sd_probe().
+ * - Cancel any asynchronous work started by ib_srp.
+ * - Tear down the IB resources associated with the target.
+ * - Invoke scsi_host_put() which will also free the target structure.
+ */
 static void srp_remove_target(struct srp_target_port *target)
 {
-	struct Scsi_Host *shost = target->scsi_host;
+	struct Scsi_Host *shost;
+	struct srp_rport *rport;
 
 	WARN_ON(target->state != SRP_TARGET_REMOVED);
 
-	srp_del_scsi_host_attr(shost);
+	/* Wait until any concurrent critical sections have finished. */
+	mutex_lock(&target->mutex);
+	mutex_unlock(&target->mutex);
+
+	shost = target->scsi_host;
+	rport = target->rport;
+	WARN_ON((rport != NULL) != target->scsi_host_added);
+
+	if (target->scsi_host_added)
+		srp_del_scsi_host_attr(shost);
+	srp_disconnect_target(target, true);
 	if (target->scsi_host_added) {
+		srp_resume_io(rport);
+		srp_stop_rport(rport);
 		srp_remove_host(shost);
 		scsi_remove_host(shost);
 	}
-	srp_disconnect_target(target);
+	cancel_work_sync(&target->scan_work);
+	cancel_delayed_work_sync(&target->reconnect_work);
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
@@ -645,6 +713,21 @@ static bool srp_conn_unique(struct srp_host *host,
 	return ret;
 }
 
+static void srp_ping_timedout(struct srp_rport *rport)
+{
+	struct srp_target_port *target = rport->lld_data;
+
+	pr_debug("ping timeout: rport = %p; target = %p / state %d\n",
+		 rport, target, target->state);
+
+	mutex_lock(&target->mutex);
+	if (srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_BLOCKED)) {
+		srp_block_rport(rport);
+		srp_start_tl_fail_timers(target->rport);
+	}
+	mutex_unlock(&target->mutex);
+}
+
 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;
@@ -676,6 +759,7 @@ static int srp_connect_target(struct srp_target_port *target)
 		switch (target->status) {
 		case 0:
 			srp_change_conn_state(target, true);
+			target->failed_reconnects = 0;
 			return 0;
 
 		case SRP_PORT_REDIRECT:
@@ -754,7 +838,16 @@ static void srp_scan_target(struct srp_target_port *target)
 			 SCAN_WILD_CARD, 0);
 }
 
-static int srp_reconnect_target(struct srp_target_port *target)
+static void srp_scan_work(struct work_struct *work)
+{
+	struct srp_target_port *target;
+
+	target = container_of(work, struct srp_target_port, scan_work);
+	srp_scan_target(target);
+}
+
+static int srp_reconnect_target(struct srp_target_port *target,
+				bool cancel_block_work)
 {
 	struct ib_qp_attr qp_attr;
 	int i, ret;
@@ -762,6 +855,11 @@ static int srp_reconnect_target(struct srp_target_port *target)
 
 	unique = srp_conn_unique(target->srp_host, target);
 	if (unique) {
+		mutex_lock(&target->mutex);
+		if (srp_change_state(target, SRP_TARGET_LIVE,
+				     SRP_TARGET_BLOCKED))
+			srp_block_rport(target->rport);
+		mutex_unlock(&target->mutex);
 	} else if (srp_change_state_to_removed(target)) {
 		shost_printk(KERN_INFO, target->scsi_host,
 			     PFX "deleting SCSI host because obsolete.\n");
@@ -774,7 +872,7 @@ static int srp_reconnect_target(struct srp_target_port *target)
 		return -EAGAIN;
 	}
 
-	srp_disconnect_target(target);
+	srp_disconnect_target(target, cancel_block_work);
 	/*
 	 * Now get a new local CM ID so that we avoid confusing the
 	 * target in case things are really fouled up.
@@ -806,10 +904,63 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	if (ret)
 		goto err;
 
+	mutex_lock(&target->mutex);
+	if (srp_change_state(target, SRP_TARGET_BLOCKED, SRP_TARGET_LIVE))
+		srp_resume_io(target->rport);
+	else
+		ret = -EAGAIN;
+	mutex_unlock(&target->mutex);
+
+	/*
+	 * Since this code can be invoked from the context of the SCSI error
+	 * handler, invoke SCSI scanning asynchronously.
+	 */
+	if (ret == 0)
+		queue_work(system_long_wq, &target->scan_work);
+
 err:
 	return ret;
 }
 
+/**
+ * srp_reconnect_repeatedly() - Attempt to reconnect repeatedly.
+ *
+ * Return value: True if and only if the reconnect attempt hasn't succeeded
+ * and a subsequent reconnect attempt is scheduled. If the return value is
+ * true that also means that the target and the rport state have been changed
+ * from LIVE into BLOCKED.
+ */
+static void srp_reconnect_repeatedly(struct srp_target_port *target,
+				     bool cancel_block_work)
+{
+	int res, tmo;
+
+	res = srp_reconnect_target(target, cancel_block_work);
+	if (res == 0)
+		return;
+
+	++target->failed_reconnects;
+
+	shost_printk(KERN_ERR, target->scsi_host,
+		     PFX "reconnect attempt %d failed (%d).\n",
+		     target->failed_reconnects, res);
+
+	tmo = target->reconnect_tmo;
+	if (tmo > 0)
+		queue_delayed_work(system_long_wq, &target->reconnect_work,
+				   tmo * HZ);
+}
+
+static void srp_reconnect_work(struct work_struct *work)
+{
+	struct srp_target_port *target;
+
+	target = container_of(to_delayed_work(work), struct srp_target_port,
+			      reconnect_work);
+
+	srp_reconnect_repeatedly(target, true);
+}
+
 static void srp_map_desc(struct srp_map_state *state, dma_addr_t dma_addr,
 			 unsigned int dma_len, u32 rkey)
 {
@@ -1343,15 +1494,30 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 			     PFX "Recv failed with error code %d\n", res);
 }
 
+/*
+ * Start the transport layer failure timers, block the SCSI host and try to
+ * reconnect.
+ */
+static void srp_block_work(struct work_struct *work)
+{
+	struct srp_target_port *target;
+
+	target = container_of(work, struct srp_target_port, block_work);
+	srp_start_tl_fail_timers(target->rport);
+	srp_reconnect_repeatedly(target, false);
+}
+
 static void srp_handle_qp_err(enum ib_wc_status wc_status,
 			      enum ib_wc_opcode wc_opcode,
 			      struct srp_target_port *target)
 {
-	if (target->connected && !target->qp_in_error)
+	if (target->connected && !target->qp_in_error) {
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "failed %s status %d\n",
 			     wc_opcode & IB_WC_RECV ? "receive" : "send",
 			     wc_status);
+		queue_work(system_long_wq, &target->block_work);
+	}
 	target->qp_in_error = true;
 }
 
@@ -1410,15 +1576,15 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 	unsigned long flags;
 	int len;
 
-	if (!target->connected)
-		goto err;
-
 	if (target->state == SRP_TARGET_REMOVED) {
 		scmnd->result = DID_BAD_TARGET << 16;
 		scmnd->scsi_done(scmnd);
 		return 0;
 	}
 
+	if (!target->connected)
+		goto err;
+
 	spin_lock_irqsave(&target->lock, flags);
 	iu = __srp_get_tx_iu(target, SRP_IU_CMD);
 	if (!iu)
@@ -1730,6 +1896,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 		if (ib_send_cm_drep(cm_id, NULL, 0))
 			shost_printk(KERN_ERR, target->scsi_host,
 				     PFX "Sending CM DREP failed\n");
+		queue_work(system_long_wq, &target->block_work);
 		break;
 
 	case IB_CM_TIMEWAIT_EXIT:
@@ -1857,7 +2024,7 @@ static int srp_reset_host(struct scsi_cmnd *scmnd)
 
 	shost_printk(KERN_ERR, target->scsi_host, PFX "SRP reset_host called\n");
 
-	res = srp_reconnect_target(target);
+	res = srp_reconnect_target(target, true);
 	if (res == 0)
 		return SUCCESS;
 
@@ -1879,6 +2046,7 @@ static int srp_slave_configure(struct scsi_device *sdev)
 {
 	struct Scsi_Host *shost = sdev->host;
 	struct srp_target_port *target = host_to_target(shost);
+	struct srp_rport *rport = target->rport;
 	struct request_queue *q = sdev->request_queue;
 	unsigned long timeout;
 
@@ -1888,6 +2056,12 @@ static int srp_slave_configure(struct scsi_device *sdev)
 		blk_queue_rq_timeout(q, timeout);
 	}
 
+	if (!rport->sdev) {
+		shost_printk(KERN_DEBUG, target->scsi_host,
+			     PFX "Enabled pinging\n");
+		srp_rport_set_sdev(rport, sdev);
+	}
+
 	return 0;
 }
 
@@ -1990,6 +2164,45 @@ static ssize_t show_allow_ext_sg(struct device *dev,
 	return sprintf(buf, "%s\n", target->allow_ext_sg ? "true" : "false");
 }
 
+static ssize_t show_reconnect_tmo(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct srp_target_port *target = host_to_target(class_to_shost(dev));
+
+	return sprintf(buf, "%d\n", target->reconnect_tmo);
+}
+
+static ssize_t store_reconnect_tmo(struct device *dev,
+				   struct device_attribute *attr,
+				   const char *buf, const size_t count)
+{
+	struct srp_target_port *target = host_to_target(class_to_shost(dev));
+	char ch[16];
+	int res, tmo;
+
+	sprintf(ch, "%.*s", (int)min(sizeof(ch) - 1, count), buf);
+	res = kstrtoint(ch, 0, &tmo);
+	if (res)
+		goto out;
+	target->reconnect_tmo = tmo;
+	if (tmo > 0 && target->state == SRP_TARGET_BLOCKED)
+		queue_delayed_work(system_long_wq, &target->reconnect_work,
+				   tmo * HZ);
+	else if (tmo <= 0)
+		cancel_delayed_work(&target->reconnect_work);
+	res = count;
+out:
+	return res;
+}
+
+static ssize_t show_failed_reconnects(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct srp_target_port *target = host_to_target(class_to_shost(dev));
+
+	return sprintf(buf, "%d\n", target->failed_reconnects);
+}
+
 static DEVICE_ATTR(id_ext,	    S_IRUGO, show_id_ext,	   NULL);
 static DEVICE_ATTR(ioc_guid,	    S_IRUGO, show_ioc_guid,	   NULL);
 static DEVICE_ATTR(service_id,	    S_IRUGO, show_service_id,	   NULL);
@@ -2002,6 +2215,9 @@ static DEVICE_ATTR(local_ib_port,   S_IRUGO, show_local_ib_port,   NULL);
 static DEVICE_ATTR(local_ib_device, S_IRUGO, show_local_ib_device, NULL);
 static DEVICE_ATTR(cmd_sg_entries,  S_IRUGO, show_cmd_sg_entries,  NULL);
 static DEVICE_ATTR(allow_ext_sg,    S_IRUGO, show_allow_ext_sg,    NULL);
+static DEVICE_ATTR(reconnect_tmo,   S_IRUGO | S_IWUSR, show_reconnect_tmo,
+		   store_reconnect_tmo);
+static DEVICE_ATTR(failed_reconnects, S_IRUGO, show_failed_reconnects, NULL);
 
 static struct device_attribute *srp_host_attrs[] = {
 	&dev_attr_id_ext,
@@ -2016,6 +2232,8 @@ static struct device_attribute *srp_host_attrs[] = {
 	&dev_attr_local_ib_device,
 	&dev_attr_cmd_sg_entries,
 	&dev_attr_allow_ext_sg,
+	&dev_attr_reconnect_tmo,
+	&dev_attr_failed_reconnects,
 	NULL
 };
 
@@ -2024,6 +2242,7 @@ static struct scsi_host_template srp_template = {
 	.name				= "InfiniBand SRP initiator",
 	.proc_name			= DRV_NAME,
 	.slave_configure		= srp_slave_configure,
+	.slave_delete			= srp_disable_ping,
 	.info				= srp_target_info,
 	.queuecommand			= srp_queuecommand,
 	.eh_abort_handler		= srp_abort,
@@ -2038,6 +2257,7 @@ static struct scsi_host_template srp_template = {
 };
 
 static struct srp_function_template ib_srp_transport_functions = {
+	.rport_ping_timedout	 = srp_ping_timedout,
 	.rport_delete		 = srp_rport_delete,
 };
 
@@ -2351,7 +2571,11 @@ static ssize_t srp_create_target(struct device *dev,
 			     target->cmd_sg_cnt * sizeof (struct srp_direct_buf);
 
 	mutex_init(&target->mutex);
+	INIT_WORK(&target->block_work, srp_block_work);
 	INIT_WORK(&target->remove_work, srp_remove_work);
+	INIT_WORK(&target->scan_work, srp_scan_work);
+	INIT_DELAYED_WORK(&target->reconnect_work, srp_reconnect_work);
+	target->reconnect_tmo = 10;
 	spin_lock_init(&target->lock);
 	INIT_LIST_HEAD(&target->free_tx);
 	INIT_LIST_HEAD(&target->free_reqs);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index a603c6d..e76000e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -83,11 +83,14 @@ enum {
  * @SRP_TARGET_CONNECTING: IB connection being established and SCSI host being
  *                      added.
  * @SRP_TARGET_LIVE: IB RC connection is established and SCSI host is unblocked.
+ * @SRP_TARGET_BLOCKED: An IB RC error occurred. Recovery timer may be running.
+ *                      SCSI host is blocked.
  * @SRP_TARGET_REMOVED: SCSI host removal is pending.
  */
 enum srp_target_state {
 	SRP_TARGET_CONNECTING,
 	SRP_TARGET_LIVE,
+	SRP_TARGET_BLOCKED,
 	SRP_TARGET_REMOVED,
 };
 
@@ -186,7 +189,13 @@ struct srp_target_port {
 	struct srp_iu	       *rx_ring[SRP_RQ_SIZE];
 	struct srp_request	req_ring[SRP_CMD_SQ_SIZE];
 
+	struct work_struct	block_work;
 	struct work_struct	remove_work;
+	struct work_struct	scan_work;
+
+	int			reconnect_tmo;
+	int			failed_reconnects;
+	struct delayed_work	reconnect_work;
 
 	struct list_head	list;
 	struct completion	done;
-- 
1.7.7


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[David Dillow]

On Sat, 2012-01-14 at 12:36 +0000, Bart Van Assche wrote:
> This patch series makes the ib_srp driver better suited for use in a H.A.

What kernel version is this based on?

Patch 17 (srp_transport: Add transport layer recovery support) doesn't
apply with 'git am'. I haven't investigated why yet, so it may be quite
simple.

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

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Bart Van Assche]

On Sat, Jan 14, 2012 at 10:10 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> On Sat, 2012-01-14 at 12:36 +0000, Bart Van Assche wrote:
> > This patch series makes the ib_srp driver better suited for use in a H.A.
>
> What kernel version is this based on?

3.2.0+ (commit 2145199c4f0db7c517dd788abec301dc84b91bd0 in Linus'
tree, which has timestamp 2012-01-14 04:48:42)

> Patch 17 (srp_transport: Add transport layer recovery support) doesn't
> apply with 'git am'. I haven't investigated why yet, so it may be quite
> simple.

Hmm, that's strange. As far as I can see what arrived on the
linux-scsi mailing list is identical to the patches I prepared, and
I've just checked that these apply cleanly on top of Linus' tree. By
the way, this patch series is available on github too:
http://github.com/bvanassche/linux/commits/srp-ha/

$ wget -qO- 'http://marc.info/?l=linux-scsi&m=132654542724428&q=raw' |
diff -u 0009-srp_transport-Fix-atttribute-registration.patch -
--- 0009-srp_transport-Fix-atttribute-registration.patch
2012-01-14 12:31:45.937467613 +0000
+++ -   2012-01-15 09:22:33.334832958 +0000
@@ -1,8 +1,3 @@
-From 8404320e4f4803bd24f538811017249a4741e3d2 Mon Sep 17 00:00:00 2001
-From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
-Date: Fri, 21 Oct 2011 19:31:07 +0200
-Subject: [PATCH 09/18] srp_transport: Fix atttribute registration
-
 Register transport attributes after the attribute array has been
 set up instead of before. The current code can trigger a race
 condition because the code reading the attribute array can run
@@ -43,3 +38,8 @@
 --
 1.7.7

+
+--
+To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
+the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+More majordomo info at  http://vger.kernel.org/majordomo-info.html
\ No newline at end of file

$ wget -qO- 'http://marc.info/?l=linux-scsi&m=132654548224434&q=raw' |
diff -u 0010-srp_transport-Simplify-attribute-initialization-code.patch
-
--- 0010-srp_transport-Simplify-attribute-initialization-code.patch
 2012-01-14 12:31:45.937467613 +0000
+++ -   2012-01-15 09:24:06.834890984 +0000
@@ -1,8 +1,3 @@
-From 4be682ea0060478f62da9c14c3c7086fcbd5373d Mon Sep 17 00:00:00 2001
-From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
-Date: Sat, 10 Sep 2011 16:31:57 +0200
-Subject: [PATCH 10/18] srp_transport: Simplify attribute initialization code
-
 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,
@@ -72,3 +67,8 @@
 --
 1.7.7

+
+--
+To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
+the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+More majordomo info at  http://vger.kernel.org/majordomo-info.html
\ No newline at end of file

$ wget -qO- 'http://marc.info/?l=linux-scsi&m=132654649324672&q=raw' |
diff -u 0017-srp_transport-Add-transport-layer-recovery-support.patch
-
--- 0017-srp_transport-Add-transport-layer-recovery-support.patch
 2012-01-14 12:31:45.947467719 +0000
+++ -   2012-01-15 09:20:19.774797158 +0000
@@ -1,8 +1,3 @@
-From a736f1b201dbda07f397d5a345738abc549a239e Mon Sep 17 00:00:00 2001
-From: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
-Date: Sat, 14 Jan 2012 08:44:52 +0000
-Subject: [PATCH 17/18] srp_transport: Add transport layer recovery support
-
 Add the necessary functions in the SRP transport module to allow
 an SRP initiator driver to implement transport layer recovery.
 This includes:
@@ -567,3 +562,8 @@
 --
 1.7.7

+
+--
+To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
+the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
+More majordomo info at  http://vger.kernel.org/majordomo-info.html
\ No newline at end of file
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Bart Van Assche]

On Sat, Jan 14, 2012 at 1:36 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> This patch series makes the ib_srp driver better suited for use in a H.A.

Hi Dave,

Do you have any review comments about this patch series ?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Dave Dillow]

On Mon, Feb 06, 2012 at 11:16:25AM -0500, Bart Van Assche wrote:
> On Sat, Jan 14, 2012 at 1:36 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> > This patch series makes the ib_srp driver better suited for use in a H.A.
> 
> Do you have any review comments about this patch series ?

I haven't had a chance to review it yet. I had hoped to over the
weekend, but the $DAYJOB intrudes even there. :(

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Joseph Glanville]

Hi David,

These patches have been running in my testing environment for quite
some time now. (since prior to the series first being posted to the
list)
I have reviewed the changes and tested the following functionality to
be working correctly as added by the patch series:

1) Disconnection from target using sysfs delete

2) Fast failover using dm-mulitpath or mdadm multipath by either
physically removing connection or disabling port.
ib_srp did not like this prior to patchset - failover could take as
long as 120-180 seconds.
Adjusting fast_io_fail and dev_loss parameters in sysfs proved very
effective in tuning this behaviour.

3) Use of the /sys/class/{srp_hosts,srp_remote_ports} shortcuts have
been very useful for automation and monitoring of the initiator.

In my testing this patchset has greatly increased the durablity of the
combination of ib_srp and SCST's ib_srpt under failure conditions.
I have stress tested this environment and executed disconnect commands
under I/O and other extranous conditions and have not experienced any
kernel panics etc.

I highly recommend these patches for inclusion and would be willing to
assist in testing wherever I can.

Kind regards,

Joseph.


On 7 February 2012 12:36, Dave Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> On Mon, Feb 06, 2012 at 11:16:25AM -0500, Bart Van Assche wrote:
>> On Sat, Jan 14, 2012 at 1:36 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
>> > This patch series makes the ib_srp driver better suited for use in a H.A.
>>
>> Do you have any review comments about this patch series ?
>
> I haven't had a chance to review it yet. I had hoped to over the
> weekend, but the $DAYJOB intrudes even there. :(
>
> --
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Founder | Director | VP Research
Orion Virtualisation Solutions | www.orionvm.com.au | Phone: 1300 56
99 52 | Mobile: 0428 754 846
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[Bart Van Assche]

On Tue, Feb 7, 2012 at 1:36 AM, Dave Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> On Mon, Feb 06, 2012 at 11:16:25AM -0500, Bart Van Assche wrote:
> > On Sat, Jan 14, 2012 at 1:36 PM, Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org> wrote:
> > > This patch series makes the ib_srp driver better suited for use in a H.A.
> >
> > Do you have any review comments about this patch series ?
>
> I haven't had a chance to review it yet. I had hoped to over the
> weekend, but the $DAYJOB intrudes even there. :(

Hi Dave,

You don't have to feel obliged to review all patches of the srp-ha
series at once. It would help if you could start with reviewing the
first few patches - as your time permits - and queue the patches you
agree with for 3.4.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/18] ib_srp: Introduce pr_fmt()

[David Dillow]

On Sat, 2012-01-14 at 12:39 +0000, Bart Van Assche wrote:
> Make the logging code a little more brief by replacing
> printk(KERN_WARNING PFX ...) by pr_warn(...) and by replacing
> printk(KERN_ERR PFX ...) by pr_err(...).  Join log messages
> split over multiple lines into a single line to make it easier
> to grep for these messages. Change the severity of the log
> statement in srp_qp_event() from "error" to "debug". Change a
> double space into a single in the "bad IO class parameter" error
> message. Remove one trailing space to avoid a checkpatch warning.

All you really need for the commit message is "Using pr_* is preferred
over printk". The rest is superfluous and readily apparent from the
patch itself.

Otherwise, this patch is fine.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/18] ib_srp: Consolidate repetitive sysfs code

[David Dillow]

On Sat, 2012-01-14 at 12:40 +0000, Bart Van Assche wrote:
> 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.

This patch is fine.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] ib_srp: Enlarge block layer timeout

[David Dillow]

On Sat, 2012-01-14 at 12:41 +0000, Bart Van Assche wrote:
> Enlarge the block layer timeout such that it is above the
> InfiniBand transport layer timeout. This is necessary to avoid
> that an SRP response is received after the SCSI layer has
> already killed the associated SCSI command. Note: the timeout is
> only set for SCSI disk devices but not for any other type of
> SCSI device (M/O disk, tape, CD-ROM, ...).

Why disk only? Different default timeouts? If so, there's a better
solution.

> +static uint32_t srp_compute_rq_tmo(struct ib_qp_attr *qp_attr, int attr_mask)
> +{
> +	uint64_t T_tr_ns, max_compl_time_ms;
> +	uint32_t rq_tmo_jiffies;
> +
> +	/*
> +	 * According to section 11.2.4.2 in the IBTA spec (Modify Queue Pair,
> +	 * table 91), both the QP timeout and the retry count have to be set
> +	 * for RC QP's during the RTR to RTS transition.
> +	 */
> +	WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) !=
> +		(IB_QP_TIMEOUT | IB_QP_RETRY_CNT));

Are you guaranteed to get those attributes set if they aren't changed
from the default? You'd get a splat from this non-error case. The user
at least can go looking for the cause of the splat and find a pointer to
why, but I contend it is more user-friendly to just cope:

	/* Use the defaults from ITBA spec if CM doesn't supply them */
	timeout = (attr_mask * IP_QP_TIMEOUT) ? qp_attr->timeout : 19;
	retries = (attr_mask & IP_QP_RETRY_CNT) ? qp_attr->retry_cnt : 7;

> +	 * Set target->rq_tmo_jiffies to one second more than the largest time
> +	 * it can take before an error completion is generated. See also
> +	 * C9-140..C9-142 in the IBTA spec for more information about how to
> +	 * convert the QP Local ACK Timeout value to nanoseconds.
> +	 */
> +	T_tr_ns = 1ULL << (12 + qp_attr->timeout);

I still think T_tr_ns = 4096 * (1ULL << timeout) is more readable, and
the compiler will do the right thing -- not that it matters in this slow
path.

> +static int srp_slave_configure(struct scsi_device *sdev)
> +{
> +	struct Scsi_Host *shost = sdev->host;
> +	struct srp_target_port *target = host_to_target(shost);
> +	struct request_queue *q = sdev->request_queue;
> +	unsigned long timeout;
> +
> +	WARN_ON(target->rq_tmo_jiffies == 0);
> +	if (sdev->type == TYPE_DISK) {
> +		timeout = max_t(unsigned, 30 * HZ, target->rq_tmo_jiffies);
> +		blk_queue_rq_timeout(q, timeout);
> +	}

I think you can get rid of any dependency on the default values by just
doing

	if (q->rq_timeout < target->rq_tmo_jiffies)
		blk_queue_rq_timeout(q, timeout);

This -- or the changes to srp_compute_rq_tmo() -- would also avoid the
need for the WARN_ON().
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/18] ib_srp: Micro-optimize completion handlers

[David Dillow]

On Sat, 2012-01-14 at 12:42 +0000, Bart Van Assche wrote:
> Reduce completion queue lock contention by polling for multiple
> work completions at once. Help the CPU branch predictor by making
> it clear that IB_WC_SUCCESS is the most likely case. Move the
> error handling code into the new function srp_handle_qp_err().
> 
> Also, convert srp_target_port.qp_in_error from int to bool and
> move the initialization of that variable into srp_connect_target().

Don't merge the two different changes here. Introducing
srp_handle_qp_err() is fine, but the change to polling for multiple WC
is completely unrelated to that and to the stated intent of this series.
It also hurt performance when I measured it during the IOP scaling work.

Let's get this series out of the way, then we can talk about the WC
changes.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] ib_srp: Separate connection and host state

[David Dillow]

On Sat, 2012-01-14 at 12:43 +0000, Bart Van Assche wrote:
> Separate connection and host state. Only report QP errors while
> connected. Only invoke ib_send_cm_dreq() from inside
> srp_disconnect_target() when connected such that invoking
> srp_disconnect_target() after having received a DREQ does not
> cause an error message to be printed.

I'm not sure that splitting connection state from the target state is
really buying you anything other than more storage and complexity.
I looked to later patches for a reason this makes sense, but I'm coming
up short, so maybe I'm just missing it.

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/18] ib_srp: Wait for last completion when disconnecting

[David Dillow]

On Sat, 2012-01-14 at 12:44 +0000, Bart Van Assche wrote:
> When disconnecting the IB connection via the IB CM, wait until
> any invoked completion handlers have finished processing SRP
> protocol data and prevent that new work completions are queued.
> Change the IB completion handlers such that all error completions
> are processed instead of a subset and also such that receiving a
> completion with zero wr_id is recognized as an end-of-work marker.

This seems really ugly, and a bit of overkill. There must be a better
way.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/18] ib_srp: Introduce three helper functions

[David Dillow]

On Sat, 2012-01-14 at 12:45 +0000, Bart Van Assche wrote:
> Introduce srp_remove_target(), srp_change_state_to_removed() and
> srp_scan_target().

> +static bool srp_change_state_to_removed(struct srp_target_port *target)
> +{
> +	bool changed = false;
> +
> +	spin_lock_irq(&target->lock);
> +	if (target->state != SRP_TARGET_REMOVED) {
> +		target->state = SRP_TARGET_REMOVED;
> +		changed = true;
> +	}
> +	spin_unlock_irq(&target->lock);
> +
> +	return changed;
> +}

I'm not sure why you introduce this here, just to move
srp_change_state() up and change this back to
	srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_REMOVED);
in the next patch.

There also doesn't seem to be much of a reason to introduce
srp_scan_target() now rather than in patch 18 where you add another use
of it.

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/18] ib_srp: Eliminate state SRP_TARGET_DEAD

[David Dillow]

On Sat, 2012-01-14 at 12:46 +0000, Bart Van Assche wrote:
> Only queue removal work after having changed the target state
> into SRP_TARGET_REMOVED and not if that state was already equal
> to SRP_TARGET_REMOVED. That allows to remove the state
> SRP_TARGET_DEAD. Add a call to srp_disconnect_target() in
> srp_remove_target() - due to previous changes it is now safe to
> invoke that last function even if the IB connection has already
> been disconnected. Rename srp_target_port.work into
> srp_target_port.remove_work and move function srp_change_state()
> to just before srp_change_state_to_removed().

Again, commit messages should be about why you changed, not a mechanical
listing of what you change.

"Rename srp_target_port.work into srp_target_port.remove_work" doesn't
tell me anything. I had to read later patches to see that what you mean
was really "Rename srp_target_port.work to reflect its usage and remove
a future conflict when push the SCSI scan into it's own work task."

This would have saved me some head scratching, and is why I try to
review as much as I can of a series in one go -- all too often you make
changes that leave me wondering until I see a later patch.

I'm also not keen on how the code looks after these changes, but I do
like getting rid of the DEAD state... I'm not sure there is a good way
to handle the removal of the module that doesn't look a bit ugly.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/18] ib_srp: Document sysfs attributes

[David Dillow]

On Sat, 2012-01-14 at 12:51 +0000, Bart Van Assche wrote:
> +		* cmd_sg_entries, a number in the range 1..255 that specifies
> +		  the maximum number of direct data buffer descriptors in an

s/direct/indirect/

> +		  SRP_CMD. Direct data buffer descriptors are communicated via
> +		  the SRP_CMD header. With allow_ext_sg=0 the parameter

So are indirect buffer descriptors, which is actually what this
parameter controls.

> +		  cmd_sg_entries defines the maximum S/G list length for a
> +		  single SRP_CMD. Also, commands whose S/G list length exceeds

s/. Also,/ and/

> +		  this limit after S/G list collapsing will fail.
> +		* allow_ext_sg, whether ib_srp is allowed to use indirect data
> +		  buffer descriptors when communicating with an SRP target.

allow_ext_sg, if true, the initiator is allowed to use more indirect
data buffer descriptors than will fit in the SRP_CMD, allowing for
larger requests to be handled. If false (the default), the length of the
S/G list after merging is limited to cmd_sg_entries. Target support for
retrieving the indirect descriptor table via RDMA is required to set
this to true.

> +		* sg_tablesize, a number in the range 1..2048 specifying the
> +		  maximum S/G list length the SCSI layer is allowed to pass to
> +		  ib_srp. Specifying a value that exceeds cmd_sg_entries is
> +		  only safe with indirect data buffer support enabled
> +		  (allow_ext_sg=1).

s/only safe/only completely safe/

If this value exceeds cmd_sg_entries and a failure occurs while
collapsing the list into fewer S/G entries, the command will fail. This
failure case only appears to be possible on IBM HCAs.

> +What:		/sys/class/scsi_host/host<n>/allow_ext_sg
> +Date:		May 19, 2011
> +KernelVersion:	2.6.39
> +Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:	Whether ib_srp is allowed to use indirect data buffer
> +		descriptors when communicating with an SRP target.

Whether the initiator is allowed to use more S/G entries than will fit
in the command request when communicating with an SRP target.

> +What:		/sys/class/scsi_host/host<n>/cmd_sg_entries
> +Date:		May 19, 2011
> +KernelVersion:	2.6.39
> +Contact:	linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> +Description:	Maximum number of direct data buffer descriptors to send to
> +		the target in a single SRP_CMD.

s/direct/indirect/


> +What:		/sys/class/scsi_host/host<n>/failed_reconnects
>+What:		/sys/class/scsi_host/host<n>/reconnect_tmo

These attributes does not exist at this point in the patch series.
Perhaps this patch should be the last one in the series?


-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/18] ib_srp: Allow SRP disconnect through sysfs

[David Dillow]

On Sat, 2012-01-14 at 12:52 +0000, Bart Van Assche wrote:
> Make it possible to disconnect 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.

>  
> +	rport->ft = &ib_srp_transport_functions;

The initiator should not be mucking around with internal functions of
the transport. Even if this is going to live in srp_rport rather than
srp_internal like other transports, this assignment should be done in
srp_rport_add().


> diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
> index 3882adf..22e0c5d 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.h
> +++ b/drivers/infiniband/ulp/srp/ib_srp.h
> @@ -154,6 +154,7 @@ struct srp_target_port {
>  	u16			io_class;
>  	struct srp_host	       *srp_host;
>  	struct Scsi_Host       *scsi_host;
> +	struct srp_rport       *rport;

Why do we need to save this in our target? You don't use it anywhere in
this patch.


> --- a/drivers/scsi/scsi_transport_srp.c
> +++ b/drivers/scsi/scsi_transport_srp.c

> @@ -116,6 +116,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);

This should translate to the srp_internal and then use the function
template there instead of adding them to srp_rport.

	struct Scsi_Host *shost = rport_to_shost(rport);
	struct srp_internal *i = to_srp_internal(shost->transportt);

	if (!i->f->rport_delete)
		return -ENOSYS;

	i->f->rport_delete(rport);
	return count;


> diff --git a/include/scsi/scsi_transport_srp.h b/include/scsi/scsi_transport_srp.h
> index 9c60ca1..1a109ff 100644
> --- a/include/scsi/scsi_transport_srp.h
> +++ b/include/scsi/scsi_transport_srp.h
> @@ -14,13 +14,23 @@ struct srp_rport_identifiers {
>  };
>  
>  struct srp_rport {
> +	/* for initiator and target drivers */
> +
> +	struct srp_function_template *ft;

This isn't needed; while perhaps more efficient, it breaks from the
practice used by other SCSI transports.

>  	struct device dev;
>  
>  	u8 port_id[16];
>  	u8 roles;
> +
> +	/* for initiator drivers */
> +
> +	void			*lld_data;	/* LLD private data */

Other transports add a private data size to the transport's function
templates, and allocate space at the end of the rport. See
fc_function_template:dd_fcrport_size and fc_rport:dd_data.

Using this model will let use store our pointer there for now, and may
let us merge the rport allocation of the initiator with the transport's
allocation in the future.

Or it may just be complexity for no real gain; I can certainly see that
argument against it.

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/18] ib_srp: Maintain a single connection per I_T nexus

[David Dillow]

On Sat, 2012-01-14 at 12:54 +0000, Bart Van Assche wrote:
> The sysfs attribute 'add_target' may be used to relogin to a
> target. An SRP target that receives a second login request from
> an initiator will disconnect the previous connection. So before
> trying to reconnect, check whether another connection to the
> same SRP target identifier already exists. If so, remove the
> target port.  Add a target to the target list before connecting
> instead of after such that this algorithm has a chance to work.

I was OK before with preventing the addition of a duplicate connection,
but the initiator shouldn't be enforcing the removal of previous
connections -- that should happen via the target issuing a DREQ.

If you're going to disallow duplicate requests, then just disallow
adding the new one, don't tear down the old one -- let the admin
manually do that if desired.


> +/**
> + * srp_conn_unique() - Check whether the connection to a target is unique.
> + *
> + * Connections are compared by their SCSI target port identifier: identifier
> + * extension + IO controller GUID. See also paragraph B.5 in the SRP spec.
> + */
> +static bool srp_conn_unique(struct srp_host *host,
> +			    struct srp_target_port *target)
> +{
> +	struct srp_target_port *t;
> +	bool ret = true;
> +
> +	spin_lock(&host->target_lock);
> +	list_for_each_entry(t, &host->target_list, list) {
> +		if (t != target &&
> +		    target->id_ext == t->id_ext &&
> +		    target->ioc_guid == t->ioc_guid) {

You also need to consider target->initiator_ext here -- that will change
the I_T nexus.

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/18] ib_srp: Rework error handling

[David Dillow]

On Sat, 2012-01-14 at 12:57 +0000, Bart Van Assche wrote:
> Add fast_io_fail_tmo and dev_loss_tmo sysfs attributes. Block
> the SCSI target as soon as a transport layer error has been
> detected (ping timeout, disconnect or IB error completion). Try
> to reconnect until dev_loss_tmo elapses.
> 
> Disconnect the IB connection earlier in srp_remove_target() to
> make sure that error recovery is not triggered during host
> removal. Swap the "connected" and "removed" tests in
> srp_queuecommand() because of this change.
> 
> Rescan LUNs after having unblocked a SCSI target controlled by
> ib_srp.

The inclusion of the ping gets 16..18 a NAK from me. As I've previously
said, this is better handled by user space and you add additional
failure cases when you try to retrofit a transport ping into SRP. Just
don't do it.

I'll try to parse through the rest of this soon to give you feedback on
the conversion to dev_loss_tmo etc.

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] ib_srp: Enlarge block layer timeout

[Bart Van Assche]

On Sun, Feb 26, 2012 at 6:32 AM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> On Sat, 2012-01-14 at 12:41 +0000, Bart Van Assche wrote:
> > Enlarge the block layer timeout such that it is above the
> > InfiniBand transport layer timeout. This is necessary to avoid
> > that an SRP response is received after the SCSI layer has
> > already killed the associated SCSI command. Note: the timeout is
> > only set for SCSI disk devices but not for any other type of
> > SCSI device (M/O disk, tape, CD-ROM, ...).
>
> Why disk only? Different default timeouts? If so, there's a better
> solution.

If you have a look at the sd, sr and st source code, you will see that
the timeout set in slave_configure() will be honored only for regular
and MO disks but not for any other type of SCSI device. The default
timeout for MO disks is already more than large enough. By the way, if
you have a look at drivers/scsi/ibmvscsi/ibmvscsi.c, you will see that
in that driver a similar approach is taken for setting the timeout.

> > +static uint32_t srp_compute_rq_tmo(struct ib_qp_attr *qp_attr, int attr_mask)
> > +{
> > +     uint64_t T_tr_ns, max_compl_time_ms;
> > +     uint32_t rq_tmo_jiffies;
> > +
> > +     /*
> > +      * According to section 11.2.4.2 in the IBTA spec (Modify Queue Pair,
> > +      * table 91), both the QP timeout and the retry count have to be set
> > +      * for RC QP's during the RTR to RTS transition.
> > +      */
> > +     WARN_ON((attr_mask & (IB_QP_TIMEOUT | IB_QP_RETRY_CNT)) !=
> > +             (IB_QP_TIMEOUT | IB_QP_RETRY_CNT));
>
> Are you guaranteed to get those attributes set if they aren't changed
> from the default?

Yes. ib_cm_init_qp_attr() has to set these for the RTR to RTS
transition, otherwise it would violate the IBTA spec.

>> +static int srp_slave_configure(struct scsi_device *sdev)
>> +{
>> +     struct Scsi_Host *shost = sdev->host;
>> +     struct srp_target_port *target = host_to_target(shost);
>> +     struct request_queue *q = sdev->request_queue;
>> +     unsigned long timeout;
>> +
>> +     WARN_ON(target->rq_tmo_jiffies == 0);
>> +     if (sdev->type == TYPE_DISK) {
>> +             timeout = max_t(unsigned, 30 * HZ, target->rq_tmo_jiffies);
>> +             blk_queue_rq_timeout(q, timeout);
>> +     }
>
> I think you can get rid of any dependency on the default values by just
> doing
>
>        if (q->rq_timeout < target->rq_tmo_jiffies)
>                blk_queue_rq_timeout(q, timeout);

That might cause the timeout to become smaller than 30s. I'm not sure
using smaller timeouts than the current default would be wise.

> This -- or the changes to srp_compute_rq_tmo() -- would also avoid the
> need for the WARN_ON().

The only reason that WARN_ON() is present is to verify that no timeout
has been set yet by sd, just in case the timeout computation code in
sd would ever be moved. I can leave out that WARN_ON() statement if
you want.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/18] ib_srp: Enlarge block layer timeout

[David Dillow]

On Sun, 2012-02-26 at 19:25 +0000, Bart Van Assche wrote:
> On Sun, Feb 26, 2012 at 6:32 AM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> > On Sat, 2012-01-14 at 12:41 +0000, Bart Van Assche wrote:
> > > Enlarge the block layer timeout such that it is above the
> > > InfiniBand transport layer timeout. This is necessary to avoid
> > > that an SRP response is received after the SCSI layer has
> > > already killed the associated SCSI command. Note: the timeout is
> > > only set for SCSI disk devices but not for any other type of
> > > SCSI device (M/O disk, tape, CD-ROM, ...).
> >
> > Why disk only? Different default timeouts? If so, there's a better
> > solution.
> 
> If you have a look at the sd, sr and st source code, you will see that
> the timeout set in slave_configure() will be honored only for regular
> and MO disks but not for any other type of SCSI device.

Ok, I had thought this code ran after the {sd,st,sr}_probe() calls, but
looking closer I see this isn't the case. Setting it only for disks
makes sense, thanks for the explanation. Bummer there isn't a good place
to hook in to fix the timeout once the s* drivers have set their
default.

> >> +static int srp_slave_configure(struct scsi_device *sdev)

> >> +     WARN_ON(target->rq_tmo_jiffies == 0);
> >> +     if (sdev->type == TYPE_DISK) {
> >> +             timeout = max_t(unsigned, 30 * HZ, target->rq_tmo_jiffies);
> >> +             blk_queue_rq_timeout(q, timeout);
> >> +     }

> The only reason that WARN_ON() is present is to verify that no timeout
> has been set yet by sd, just in case the timeout computation code in
> sd would ever be moved. I can leave out that WARN_ON() statement if
> you want.

No, keeping it is fine, just put a small comment about why it's there.
And if you are intending to guard against the SD timeout calculation
moving, you should be checking q->rq_timeout, not
target->rq_tmo_jiffies.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/18] ib_srp: Consolidate repetitive sysfs code

[Roland Dreier]

I went ahead and merged patches 1-2 which you acked, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] ib_srp: Separate connection and host state

[Bart Van Assche]

On 02/26/12 06:32, David Dillow wrote:
> On Sat, 2012-01-14 at 12:43 +0000, Bart Van Assche wrote:
>> Separate connection and host state. Only report QP errors while
>> connected. Only invoke ib_send_cm_dreq() from inside
>> srp_disconnect_target() when connected such that invoking
>> srp_disconnect_target() after having received a DREQ does not
>> cause an error message to be printed.
> I'm not sure that splitting connection state from the target state is
> really buying you anything other than more storage and complexity.
> I looked to later patches for a reason this makes sense, but I'm coming
> up short, so maybe I'm just missing it.

As explained in the description of this patch, this patch makes sense
even without the later changes.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/18] ib_srp: Introduce three helper functions

[Bart Van Assche]

On 02/26/12 06:32, David Dillow wrote:
> On Sat, 2012-01-14 at 12:45 +0000, Bart Van Assche wrote:
>> Introduce srp_remove_target(), srp_change_state_to_removed() and
>> srp_scan_target().
>> +static bool srp_change_state_to_removed(struct srp_target_port *target)
>> +{
>> +	bool changed = false;
>> +
>> +	spin_lock_irq(&target->lock);
>> +	if (target->state != SRP_TARGET_REMOVED) {
>> +		target->state = SRP_TARGET_REMOVED;
>> +		changed = true;
>> +	}
>> +	spin_unlock_irq(&target->lock);
>> +
>> +	return changed;
>> +}
> I'm not sure why you introduce this here, just to move
> srp_change_state() up and change this back to
> 	srp_change_state(target, SRP_TARGET_LIVE, SRP_TARGET_REMOVED);
> in the next patch.
>
> There also doesn't seem to be much of a reason to introduce
> srp_scan_target() now rather than in patch 18 where you add another use
> of it.

I'll move these changes to a later point in the patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/18] ib_srp: Wait for last completion when disconnecting

[Bart Van Assche]

On 02/26/12 06:32, David Dillow wrote:
> On Sat, 2012-01-14 at 12:44 +0000, Bart Van Assche wrote:
>> When disconnecting the IB connection via the IB CM, wait until
>> any invoked completion handlers have finished processing SRP
>> protocol data and prevent that new work completions are queued.
>> Change the IB completion handlers such that all error completions
>> are processed instead of a subset and also such that receiving a
>> completion with zero wr_id is recognized as an end-of-work marker.
> This seems really ugly, and a bit of overkill. There must be a better
> way.

The only alternative I know of for waiting until completion processing
stopped is to destroy and recreate the queue pair. The reason I had not
chosen for that approach is that some srp_disconnect_target() calls are
from outside a CM callback function and hence destroying the QP inside
srp_disconnect_target() would race with QP manipulations from inside CM
callbacks.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/18] ib_srp: Maintain a single connection per I_T nexus

[Bart Van Assche]

On 02/26/12 06:34, David Dillow wrote:
> On Sat, 2012-01-14 at 12:54 +0000, Bart Van Assche wrote:
>> The sysfs attribute 'add_target' may be used to relogin to a
>> target. An SRP target that receives a second login request from
>> an initiator will disconnect the previous connection. So before
>> trying to reconnect, check whether another connection to the
>> same SRP target identifier already exists. If so, remove the
>> target port.  Add a target to the target list before connecting
>> instead of after such that this algorithm has a chance to work.
> I was OK before with preventing the addition of a duplicate connection,
> but the initiator shouldn't be enforcing the removal of previous
> connections -- that should happen via the target issuing a DREQ.
>
> If you're going to disallow duplicate requests, then just disallow
> adding the new one, don't tear down the old one -- let the admin
> manually do that if desired.

But why to wait with disconnecting stale connections until the target
sends a DREQ if we know the target is going to send it anyway ? Such an
approach would make it necessary to introduce an extra state ("stale
connection but not yet cleaned up by target"). Does that approach have
any advantage over the approach followed in the patch I posted ?

Please note that this behavior was introduced because one of your
comments on the v1 series was that you considered allowing an
administrator to issue a duplicate login request as useful functionality
(http://www.spinics.net/lists/linux-rdma/msg10451.html).

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/18] ib_srp: Rework error handling

[Bart Van Assche]

On 02/26/12 06:39, David Dillow wrote:
> On Sat, 2012-01-14 at 12:57 +0000, Bart Van Assche wrote:
>> Add fast_io_fail_tmo and dev_loss_tmo sysfs attributes. Block
>> the SCSI target as soon as a transport layer error has been
>> detected (ping timeout, disconnect or IB error completion). Try
>> to reconnect until dev_loss_tmo elapses.
>>
>> Disconnect the IB connection earlier in srp_remove_target() to
>> make sure that error recovery is not triggered during host
>> removal. Swap the "connected" and "removed" tests in
>> srp_queuecommand() because of this change.
>>
>> Rescan LUNs after having unblocked a SCSI target controlled by
>> ib_srp.
> As I've previously said, this is better handled by user space and
> you add additional failure cases when you try to retrofit a transport
> ping into SRP.

Let's look at this from another point of view. If it's possible for the
SRP protocol to implement most of the transport layer checking and error
handling in user space then that's possible for iSCSI and FC too. Are
you claiming that the device mapper layer and the multipath software
should be redesigned such that certain parts of the transport checking
and recovery mechanisms are moved from kernel space to user space,
including a backwards-incompatible change of the kernel ABI (sysfs) the
multipath software uses today ?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/18] ib_srp: Rework error handling

[David Dillow]

On Sun, 2012-03-04 at 17:05 +0000, Bart Van Assche wrote:
> On 02/26/12 06:39, David Dillow wrote:
> > As I've previously said, this is better handled by user space and
> > you add additional failure cases when you try to retrofit a transport
> > ping into SRP.
> 
> Let's look at this from another point of view. If it's possible for the
> SRP protocol to implement most of the transport layer checking and error
> handling in user space then that's possible for iSCSI and FC too. Are
> you claiming that the device mapper layer and the multipath software
> should be redesigned such that certain parts of the transport checking
> and recovery mechanisms are moved from kernel space to user space,
> including a backwards-incompatible change of the kernel ABI (sysfs) the
> multipath software uses today ?

You mean, let's do it the way it already works?

multipath implements path-checking on a *per-LUN* basis from user space.
Today. No redesign needed. It already works on all SCSI transports.

iSCSI has a protocol-defined mechanism to tell us when all LUNs on a
given target are unavailable without waiting for the connection to be
broken on a command timeout. I don't think FC has that, but it does have
notification that a target is out of the fabric -- and all LUNs are
unavailable. Both of these situations are communicated via an existing
sysfs ABI, and the user space multipath code knows how to fail over and
avoid checking those paths that are known to be bad.

SRP does not have a protocol-defined way to check the link to the
target. IB can tell us when a target leaves the fabric, like FC, but
that's not what you're trying to implement -- you are attempting to fake
the ping capability of iSCSI.

Implementing "most" of the transport layer checking gives you a partial
kernel implementation of the pinging already performed by user space,
and adds bugs to it in that a single LUN experiencing issues may fail
the whole path when the rest of the target is perfectly fine.

Adding bugs while re-implementing user space prevents me from signing
off on it.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/18] ib_srp: Separate connection and host state

[David Dillow]

On Sat, 2012-03-03 at 14:37 +0000, Bart Van Assche wrote:
> On 02/26/12 06:32, David Dillow wrote:
> > On Sat, 2012-01-14 at 12:43 +0000, Bart Van Assche wrote:
> >> Separate connection and host state. Only report QP errors while
> >> connected. Only invoke ib_send_cm_dreq() from inside
> >> srp_disconnect_target() when connected such that invoking
> >> srp_disconnect_target() after having received a DREQ does not
> >> cause an error message to be printed.
> > I'm not sure that splitting connection state from the target state is
> > really buying you anything other than more storage and complexity.
> > I looked to later patches for a reason this makes sense, but I'm coming
> > up short, so maybe I'm just missing it.
> 
> As explained in the description of this patch, this patch makes sense
> even without the later changes.

No, the patch description says simply "Separate connection and host
state."  That tells the reader only what you did and nothing about why
it is a good idea.

If this was all to avoid printing a message after getting a DREQ, then
the commit message should reflect that as the main thrust of the patch,
and not "Separate connection and host state" -- that's a mechanical step
on your path to the goal.
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/18] ib_srp: Maintain a single connection per I_T nexus

[David Dillow]

On Sat, 2012-03-03 at 15:30 +0000, Bart Van Assche wrote:
> On 02/26/12 06:34, David Dillow wrote:
> > I was OK before with preventing the addition of a duplicate connection,
> > but the initiator shouldn't be enforcing the removal of previous
> > connections -- that should happen via the target issuing a DREQ.
> >
> > If you're going to disallow duplicate requests, then just disallow
> > adding the new one, don't tear down the old one -- let the admin
> > manually do that if desired.
> 
> But why to wait with disconnecting stale connections until the target
> sends a DREQ if we know the target is going to send it anyway ? Such an
> approach would make it necessary to introduce an extra state ("stale
> connection but not yet cleaned up by target"). Does that approach have
> any advantage over the approach followed in the patch I posted ?

Only code cleanliness. I'm not happy with the need to keep track of
whether or not this host has been added to the SCSI midlayer. I don't
think you need an extra state -- just leave it live; it will die on the
next command if the DREQ doesn't happen before the login is accepted
(which the spec says should be the case). Of course that's a bit ugly
from a behavioral viewpoint.

> Please note that this behavior was introduced because one of your
> comments on the v1 series was that you considered allowing an
> administrator to issue a duplicate login request as useful functionality
> (http://www.spinics.net/lists/linux-rdma/msg10451.html).

Yes, I recall. I also said that I was OK with disallowing a duplicate
login, as the re-login case is enabled by the admin's ability to
disconnect a single target.

I figured you would go that route, as it was the easier path. Let no one
say you back down from a challenge!

-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 00/18, v2] Make ib_srp better suited for H.A. purposes

[David Dillow]

On Sun, 2012-01-15 at 09:28 +0000, Bart Van Assche wrote:
> On Sat, Jan 14, 2012 at 10:10 PM, David Dillow <dillowda-1Heg1YXhbW8@public.gmane.org> wrote:
> > Patch 17 (srp_transport: Add transport layer recovery support) doesn't
> > apply with 'git am'. I haven't investigated why yet, so it may be quite
> > simple.
> 
> Hmm, that's strange. As far as I can see what arrived on the
> linux-scsi mailing list is identical to the patches I prepared, and
> I've just checked that these apply cleanly on top of Linus' tree. By
> the way, this patch series is available on github too:
> http://github.com/bvanassche/linux/commits/srp-ha/

I suspect it was the local Exchange server substituting spaces for the
tabs; git am was upset with the white space damage. If you can put a URL
in the cover letter, that will be quite helpful for future reviews.

I'm a bit worried about the lack of comments from others about your
changes to scsi_transport_srp.*. We really should see some ACKs from the
SCSI guys, and while I would think they should go via James Bottomley
rather than Roland, I don't think that going through the InfiniBand tree
will be a real issue if James is OK with them.

However, given the lack of any comments from that side, I wonder if it
makes any sense to contain the changes to the initiator for now, and
then migrate the common functionality to the transport layer via the
SCSI tree as we can get the right people's attention?

I'm not particularly happy with that approach, as I'd rather we do it
correctly the first time, but I know people would like to see the
improvements in the SRP initiator as well.

What do others think?
-- 
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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/18] ib_srp: Rework error handling

[Bart Van Assche]

On 03/04/12 20:03, David Dillow wrote:
> multipath implements path-checking on a *per-LUN* basis from user space.
> Today. No redesign needed. It already works on all SCSI transports.
>
> iSCSI has a protocol-defined mechanism to tell us when all LUNs on a
> given target are unavailable without waiting for the connection to be
> broken on a command timeout. I don't think FC has that, but it does have
> notification that a target is out of the fabric -- and all LUNs are
> unavailable. Both of these situations are communicated via an existing
> sysfs ABI, and the user space multipath code knows how to fail over and
> avoid checking those paths that are known to be bad.
>
> SRP does not have a protocol-defined way to check the link to the
> target. IB can tell us when a target leaves the fabric, like FC, but
> that's not what you're trying to implement -- you are attempting to fake
> the ping capability of iSCSI.

Let's focus on what we agree about. I'll repost the patch series but
without the last two patches (those that add dev_loss_tmo,
fast_io_fail_tmo, ping_interval, ping_timeout, failed_reconnects and
reconnect_tmo).

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[Mike Christie]

On 01/14/2012 05:56 AM, Bart Van Assche wrote:
> Add the necessary functions in the SRP transport module to allow
> an SRP initiator driver to implement transport layer recovery.
> This includes:
> - A ping mechanism to check whether the transport layer is still
>   operational.
> - Support for implementing fast_io_fail_tmo, the time that should
>   elapse after having detected a transport layer problem and
>   before failing I/O.
> - Support for implementing dev_loss_tmo, the time that should
>   elapse after having detected a transport layer problem and
>   before removing a remote port.
> 

I was updating my iscsi dev loss patch when I saw this is still not merged.

Not sure about the ping code, but I think the dev loss tmo and fast io
fail related stuff should go to scsi_transport_template. We can then add
some common code to start the timers/delayed_work_queues, stop them,
set/get the value from sysfs, etc that all the transport classes can
then call.

For the srp code to use this patch, it was not clear to me what happens
if dev_loss_tmo fires, but then the port comes back online. It looks
like the target and its devices get deleted like with FC when the
dev_loss_tmo fires. Is there something to automatically re-add the
path/port/target when the issue is resolved? Was it on the patches that
did not make the list (I only saw 6 patches out of 18 on linux-scsi) or
do we have some userspace daemon that figures out when
paths/ports/targets come back?

Also I think the patch that has ib_srp call srp_start_tl_fail_timers and
srp_block_rport did not make the list.

For the ping code, does it use TUR because there is not a transport way
to test the path/port/transport/interconnect?

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[David Dillow]

On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
> On 01/14/2012 05:56 AM, Bart Van Assche wrote:
> > Add the necessary functions in the SRP transport module to allow
> > an SRP initiator driver to implement transport layer recovery.
>
> I was updating my iscsi dev loss patch when I saw this is still not merged.

Yes, I got part way into doing a rework of Bart's code before I got
sidetracked on another project. Cognizant of the looming merge window,
I'm desperately trying to get back to it. 

> Not sure about the ping code, but I think the dev loss tmo and fast io
> fail related stuff should go to scsi_transport_template. We can then add
> some common code to start the timers/delayed_work_queues, stop them,
> set/get the value from sysfs, etc that all the transport classes can
> then call.

Good idea, I'll look into doing this as part of it.

> For the srp code to use this patch, it was not clear to me what happens
> if dev_loss_tmo fires, but then the port comes back online. It looks
> like the target and its devices get deleted like with FC when the
> dev_loss_tmo fires. Is there something to automatically re-add the
> path/port/target when the issue is resolved? Was it on the patches that
> did not make the list (I only saw 6 patches out of 18 on linux-scsi) or
> do we have some userspace daemon that figures out when
> paths/ports/targets come back?

There is srp_daemon, which performs this task.

> For the ping code, does it use TUR because there is not a transport way
> to test the path/port/transport/interconnect?

Yes, SRP does not have a way to test connectivity on a transport level
ala iSCSI NOPs. I'm not at all inclined to add the ping functionality to
the SRP initiator, as I see it as duplicative of the existing multipathd
functionality in userspace, and it can kill the entire nexus even if
only one LUN is having issues.

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

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[Mike Christie]

On 07/16/2012 04:07 PM, Mike Christie wrote:
> For the ping code, does it use TUR because there is not a transport way
> to test the path/port/transport/interconnect?

Maybe not transport as in SCSI transport method, but what about
something similar to using a ICMP ping for testing the network with iscsi.

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[Mike Christie]

On 07/16/2012 04:28 PM, David Dillow wrote:
> On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
>> On 01/14/2012 05:56 AM, Bart Van Assche wrote:
>>> Add the necessary functions in the SRP transport module to allow
>>> an SRP initiator driver to implement transport layer recovery.
>>
>> I was updating my iscsi dev loss patch when I saw this is still not merged.
> 
> Yes, I got part way into doing a rework of Bart's code before I got
> sidetracked on another project. Cognizant of the looming merge window,
> I'm desperately trying to get back to it. 

No problem. The iscsi dev loss patch is even older :)

> 
>> For the ping code, does it use TUR because there is not a transport way
>> to test the path/port/transport/interconnect?
> 
> Yes, SRP does not have a way to test connectivity on a transport level
> ala iSCSI NOPs. I'm not at all inclined to add the ping functionality to
> the SRP initiator, as I see it as duplicative of the existing multipathd
> functionality in userspace, and it can kill the entire nexus even if
> only one LUN is having issues.
> 

I am not in favor of a tur in the transport class too.

What about when multipath is not used or are you saying we should use it
for srp even when there is only one path (for iscsi we have told people
to do something like this if they want mpaths queue_if_no_path behavior).

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[David Dillow]

On Mon, 2012-07-16 at 18:38 -0400, Mike Christie wrote:
> On 07/16/2012 04:28 PM, David Dillow wrote:
> > On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
> >> For the ping code, does it use TUR because there is not a transport way
> >> to test the path/port/transport/interconnect?
> > 
> > Yes, SRP does not have a way to test connectivity on a transport level
> > ala iSCSI NOPs. I'm not at all inclined to add the ping functionality to
> > the SRP initiator, as I see it as duplicative of the existing multipathd
> > functionality in userspace, and it can kill the entire nexus even if
> > only one LUN is having issues.
> > 
> 
> I am not in favor of a tur in the transport class too.
> 
> What about when multipath is not used or are you saying we should use it
> for srp even when there is only one path (for iscsi we have told people
> to do something like this if they want mpaths queue_if_no_path behavior).

If they want to quickly detect and fail idle paths, then perhaps.
Otherwise, the loss of the connection will only be noticed on the next
command.

We can also get notifications from the fabric when a device leaves, so
we can use that to start the dev_loss_tmo and prune the stale
connections.

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

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[bart]

> On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
>> On 01/14/2012 05:56 AM, Bart Van Assche wrote:
>> > Add the necessary functions in the SRP transport module to allow
>> > an SRP initiator driver to implement transport layer recovery.
>>
>> I was updating my iscsi dev loss patch when I saw this is still not
>> merged.
>
> Yes, I got part way into doing a rework of Bart's code before I got
> sidetracked on another project. Cognizant of the looming merge window,
> I'm desperately trying to get back to it.

I'll post the latest version of the patch series next Monday (I'm
traveling now). If you have any suggestions for improvements about the
patches themselves or the implemented algorithms, these are welcome.

Bart.

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[Bart Van Assche]

On 07/16/12 22:28, David Dillow wrote:
> On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
>> On 01/14/2012 05:56 AM, Bart Van Assche wrote:
>>> Add the necessary functions in the SRP transport module to allow
>>> an SRP initiator driver to implement transport layer recovery.
>>
>> I was updating my iscsi dev loss patch when I saw this is still not merged.
> 
> Yes, I got part way into doing a rework of Bart's code before I got
> sidetracked on another project. Cognizant of the looming merge window,
> I'm desperately trying to get back to it. 
> 
>> Not sure about the ping code, but I think the dev loss tmo and fast io
>> fail related stuff should go to scsi_transport_template. We can then add
>> some common code to start the timers/delayed_work_queues, stop them,
>> set/get the value from sysfs, etc that all the transport classes can
>> then call.
> 
> Good idea, I'll look into doing this as part of it.

(replying to an e-mail of one month ago)

Hello Dave,

Have you already been able to make some progress with this work ? If the
patches for moving the shared parts of dev_loss_tmo / fast_io_fail_tmo
handling from scsi_transport_fc to scsi_transport_template would be
available in a reasonable timeframe I could adapt my ib_srp patch series
to take advantage of that work.

Bart.

Re: [PATCH 17/18] srp_transport: Add transport layer recovery support

[David Dillow]

On Sat, 2012-08-18 at 10:50 +0000, Bart Van Assche wrote:
> On 07/16/12 22:28, David Dillow wrote:
> > On Mon, 2012-07-16 at 18:07 -0400, Mike Christie wrote:
> >> Not sure about the ping code, but I think the dev loss tmo and fast io
> >> fail related stuff should go to scsi_transport_template. We can then add
> >> some common code to start the timers/delayed_work_queues, stop them,
> >> set/get the value from sysfs, etc that all the transport classes can
> >> then call.
> > 
> > Good idea, I'll look into doing this as part of it.
> 
> (replying to an e-mail of one month ago)
> 
> Hello Dave,
> 
> Have you already been able to make some progress with this work ? If the
> patches for moving the shared parts of dev_loss_tmo / fast_io_fail_tmo
> handling from scsi_transport_fc to scsi_transport_template would be
> available in a reasonable timeframe I could adapt my ib_srp patch series
> to take advantage of that work.

I've not made it very far; the same high priority project at work has
trumped this for too long. The only good news is that there is light at
the end of the tunnel, as we have a big demo at the end of the month.
After that, I should be able to spend more cycles on this again.

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