[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 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 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

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

[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

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: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

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 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

[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