Re: [PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling

2013-07-01 Thread Bart Van Assche 

On 06/30/13 23:05, David Dillow wrote:

On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+   return (fast_io_fail_tmo  0 || dev_loss_tmo  0 ||
+   fast_io_fail_tmo  dev_loss_tmo) 
+   fast_io_fail_tmo  LONG_MAX / HZ 
+   dev_loss_tmo  LONG_MAX / HZ ? 0 : -EINVAL;
+}


They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
LONG_MAX / HZ, I think.


The fast_io_fail_tmo should indeed be capped by that value. However, I'm 
not sure about dev_loss_tmo. I think there are several use cases (e.g. 
initiator-side mirroring) where it's useful to set dev_loss_tmo to a 
larger value than ten minutes.



+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], *p;
+   int res;
+   int fast_io_fail_tmo;
+
+   sprintf(ch, %.*s, min_t(int, sizeof(ch) - 1, count), buf);
+   p = strchr(ch, '\n');
+   if (p)
+   *p = '\0';


Again, no need for the sprintf if you don't modify the buffer? Instead
of using strchr() to make the strcmp() work with newlines, just do

if (!strcmp(buf, off) || !strcmp(buf, off\n)) {
fast_io_fail_tmo = 1;
} else {
res = kstrtoint(buf, 0, fast_io_fail_tmo);
...

instead?

Same comment applies for store_srp_rport_dev_loss_tmo().


OK, will remove the temporary char arrays.

Bart.

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

Re: [PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling

2013-07-01 Thread David Dillow 
On Mon, 2013-07-01 at 09:01 +0200, Bart Van Assche wrote:
 On 06/30/13 23:05, David Dillow wrote:
  On Fri, 2013-06-28 at 14:53 +0200, Bart Van Assche wrote:
  +int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
  +{
  +  return (fast_io_fail_tmo  0 || dev_loss_tmo  0 ||
  +  fast_io_fail_tmo  dev_loss_tmo) 
  +  fast_io_fail_tmo  LONG_MAX / HZ 
  +  dev_loss_tmo  LONG_MAX / HZ ? 0 : -EINVAL;
  +}
 
  They should also be capped by SCSI_DEVICE_BLOCK_MAX_TIMEOUT instead of
  LONG_MAX / HZ, I think.
 
 The fast_io_fail_tmo should indeed be capped by that value. However, I'm 
 not sure about dev_loss_tmo. I think there are several use cases (e.g. 
 initiator-side mirroring) where it's useful to set dev_loss_tmo to a 
 larger value than ten minutes.

Ah, yes, very good point.

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

[PATCH v2 08/15] scsi_transport_srp: Add transport layer error handling

2013-06-28 Thread Bart Van Assche 
Add the necessary functions in the SRP transport module to allow
an SRP initiator driver to implement transport layer error handling
similar to the functionality already provided by the FC transport
layer. This includes:
- 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 bvanass...@acm.org
Cc: Roland Dreier rol...@purestorage.com
Cc: James Bottomley jbottom...@parallels.com
Cc: David Dillow dillo...@ornl.gov
Cc: Vu Pham v...@mellanox.com
Cc: Sebastian Riemer sebastian.rie...@profitbricks.com
---
 Documentation/ABI/stable/sysfs-transport-srp |   37 ++
 drivers/scsi/scsi_transport_srp.c|  477 +-
 include/scsi/scsi_transport_srp.h|   62 +++-
 3 files changed, 573 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/stable/sysfs-transport-srp 
b/Documentation/ABI/stable/sysfs-transport-srp
index b36fb0d..6a4d651 100644
--- a/Documentation/ABI/stable/sysfs-transport-srp
+++ b/Documentation/ABI/stable/sysfs-transport-srp
@@ -5,6 +5,24 @@ Contact:   linux-scsi@vger.kernel.org, 
linux-r...@vger.kernel.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:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.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. Setting this attribute to off
+   will disable this behavior.
+
+What:  /sys/class/srp_remote_ports/port-h:n/fast_io_fail_tmo
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.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. Setting this attribute to off will
+   disable this behavior.
+
 What:  /sys/class/srp_remote_ports/port-h:n/port_id
 Date:  June 27, 2007
 KernelVersion: 2.6.24
@@ -12,8 +30,27 @@ 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/reconnect_delay
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   Number of seconds the SCSI layer will wait after a reconnect
+   attempt failed before retrying.
+
 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.
+
+What:  /sys/class/srp_remote_ports/port-h:n/state
+Date:  October 1, 2013
+KernelVersion: 3.11
+Contact:   linux-scsi@vger.kernel.org, linux-r...@vger.kernel.org
+Description:   State of the transport layer to the remote port. running if
+   the transport layer is operational; blocked if a transport
+   layer error has been encountered but the fail_io_fast_tmo
+   timer has not yet fired; fail-fast after the
+   fail_io_fast_tmo timer has fired and before the dev_loss_tmo
+   timer has fired; lost after the dev_loss_tmo timer has
+   fired and before the port is finally removed.
diff --git a/drivers/scsi/scsi_transport_srp.c 
b/drivers/scsi/scsi_transport_srp.c
index f7ba94a..44b27dd 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -24,11 +24,13 @@
 #include linux/err.h
 #include linux/slab.h
 #include linux/string.h
+#include linux/delay.h
 
 #include scsi/scsi.h
 #include scsi/scsi_device.h
 #include scsi/scsi_host.h
 #include scsi/scsi_transport.h
+#include scsi/scsi_cmnd.h
 #include scsi/scsi_transport_srp.h
 #include scsi_transport_srp_internal.h
 
@@ -38,7 +40,7 @@ struct srp_host_attrs {
 #define to_srp_host_attrs(host)((struct srp_host_attrs 
*)(host)-shost_data)
 
 #define SRP_HOST_ATTRS 0
-#define SRP_RPORT_ATTRS 3
+#define SRP_RPORT_ATTRS 8
 
 struct srp_internal {
struct scsi_transport_template t;
@@ -54,6 +56,25 @@ struct srp_internal {
 
 #definedev_to_rport(d) container_of(d, struct srp_rport, dev)
 #define transport_class_to_srp_rport(dev) dev_to_rport((dev)-parent)
+static inline struct