IB SRP initiator patches for kernel 3.11

If the add_one callback fails during driver load no resources are allocated so there isn't a need to release any resources. Trying to clean the resource may lead to the following kernel panic:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0132331>] srp_remove_one+0x31/0x240 [ib_srp]
RIP: 0010:[<ffffffffa0132331>]  [<ffffffffa0132331>] srp_remove_one+0x31/0x240 [ib_srp]
Process rmmod (pid: 4562, threadinfo ffff8800dd738000, task ffff8801167e60c0)
Call Trace:
  [<ffffffffa024500e>] ib_unregister_client+0x4e/0x120 [ib_core]
  [<ffffffffa01361bd>] srp_cleanup_module+0x15/0x71 [ib_srp]
  [<ffffffff810ac6a4>] sys_delete_module+0x194/0x260
  [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b

[bvanassche: Shortened patch description]

Signed-off-by Dotan Barak <[email protected]>

Reviewed-by Eli Cohen <eli-VPRAkNaXOzVS1MOuV/[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c |    2 ++
  1 file changed, 2 insertions(+)

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

index 7ccf328..368d160 100644

Quoted 42 lines · Bart Van Assche

If the add_one callback fails during driver load no resources are allocated so there isn't a need to release any resources. Trying to clean the resource may lead to the following kernel panic:

BUG: unable to handle kernel NULL pointer dereference at (null)
IP: [<ffffffffa0132331>] srp_remove_one+0x31/0x240 [ib_srp]
RIP: 0010:[<ffffffffa0132331>]  [<ffffffffa0132331>]
srp_remove_one+0x31/0x240 [ib_srp]
Process rmmod (pid: 4562, threadinfo ffff8800dd738000, task
ffff8801167e60c0)
Call Trace:
  [<ffffffffa024500e>] ib_unregister_client+0x4e/0x120 [ib_core]
  [<ffffffffa01361bd>] srp_cleanup_module+0x15/0x71 [ib_srp]
  [<ffffffff810ac6a4>] sys_delete_module+0x194/0x260
  [<ffffffff8100b0f2>] system_call_fastpath+0x16/0x1b

[bvanassche: Shortened patch description]

Signed-off-by Dotan Barak <[email protected]>

Reviewed-by Eli Cohen <eli-VPRAkNaXOzVS1MOuV/[email protected]>

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c |    2 ++
  1 file changed, 2 insertions(+)

drivers/infiniband/ulp/srp/ib_srp.c +2 -0

index 7ccf328..368d160 100644
@@ -2507,6 +2507,8 @@ static void srp_remove_one(struct ib_device
 *device)
      struct srp_target_port *target;
 
      srp_dev = ib_get_client_data(device, &srp_client);
+    if (!srp_dev)
+        return;
 
      list_for_each_entry_safe(host, tmp_host, &srp_dev->dev_list,

Quoted 3 lines

list) {
          device_unregister(&host->dev);

Please note that this patch was authored by Dotan Barak, so I should have mentioned:

From: Dotan Barak <[email protected]>

Avoid that srp_claim_command() can claim a command while srp_queuecommand() is still busy queueing the same command. Found this via source reading.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

index 368d160..9c638dd 100644
@@ -1367,7 +1367,6 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 
 	req = list_first_entry(&target->free_reqs, struct srp_request, list);
 	list_del(&req->list);
-	spin_unlock_irqrestore(&target->lock, flags);
 
 	dev = target->srp_host->srp_dev->dev;
 	ib_dma_sync_single_for_cpu(dev, iu->dma, target->max_iu_len,

@@ -1401,6 +1400,7 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 		shost_printk(KERN_ERR, target->scsi_host, PFX "Send failed\n");
 		goto err_unmap;
 	}
+	spin_unlock_irqrestore(&target->lock, flags);
 
 	return 0;
 

Wait a minute, so you've changed this commit to also hold that target lock in the following functions in error case:

srp_unmap_data(),
srp_put_tx_iu()

This is different from:
https://github.com/bvanassche/ib_srp-backport/commit/6ce0e30dbb69973926df84292239f0c20f6a2d6c

srp_unmap_data() calls ib_fmr_pool_unmap() which uses an own spin lock (pool->pool_lock).

srp_put_tx_iu() acquires the target lock as well (target->lock). That's spin lock in spin lock. I would say that this dead locks.

I like the other version more.

The SCSI error handler assumes that the transport layer is operational if an eh_abort_handler() returns SUCCESS. Hence let srp_abort() only return SUCCESS if sending the ABORT TASK task management function succeeded. This patch avoids that the SCSI error handler skips the srp_reset_host() call after a transport layer error.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

index 9c638dd..fb37b47 100644
@@ -1742,18 +1742,23 @@ static int srp_abort(struct scsi_cmnd *scmnd)
 {
 	struct srp_target_port *target = host_to_target(scmnd->device->host);
 	struct srp_request *req = (struct srp_request *) scmnd->host_scribble;
+	int ret;
 

The SRP initiator implements host reset by reconnecting to the SRP target. That means that communication with the target is possible as soon as host reset finished. Hence skip the host settle delay.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c |    1 +
 1 file changed, 1 insertion(+)

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

index fb37b47..be12780 100644

Quoted 26 lines · Bart Van Assche

The SCSI error handler assumes that the transport layer is operational if an eh_abort_handler() returns SUCCESS. Hence let srp_abort() only return SUCCESS if sending the ABORT TASK task management function succeeded. This patch avoids that the SCSI error handler skips the srp_reset_host() call after a transport layer error.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

index 9c638dd..fb37b47 100644
@@ -1742,18 +1742,23 @@ static int srp_abort(struct scsi_cmnd *scmnd)
  {
  	struct srp_target_port *target = host_to_target(scmnd->device->host);
  	struct srp_request *req = (struct srp_request *) scmnd->host_scribble;
+	int ret;
 

drivers/infiniband/ulp/srp/ib_srp.c static int srp_abort(struct scsi_cmnd *scmnd) · line 1754

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +4 -2

  	shost_printk(KERN_ERR, target->scsi_host, "SRP abort called\n");
 
  	if (!req || !srp_claim_req(target, req, scmnd))
  		return FAILED;
-	srp_send_tsk_mgmt(target, req->index, scmnd->device->lun,
-			  SRP_TSK_ABORT_TASK);
+	if (srp_send_tsk_mgmt(target, req->index, scmnd->device->lun,
+			      SRP_TSK_ABORT_TASK) == 0 ||
+	    target->transport_offline)
+		ret = SUCCESS;

Here you try to hide a little trick. Returning success upon (target->transport_offline == true) is perhaps not the best way. I guess you try to fail IO fast here but up to this point "target->transport_offline = true" is only set in srp_reset_host().

Please explain for what you need that in this patch!

Furthermore, returning "FAST_IO_FAIL" sounds better to me in this situation.

Quoted 19 lines · Bart Van Assche

The SRP initiator implements host reset by reconnecting to the SRP target. That means that communication with the target is possible as soon as host reset finished. Hence skip the host settle delay.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c |    1 +
  1 file changed, 1 insertion(+)

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

index fb37b47..be12780 100644

drivers/infiniband/ulp/srp/ib_srp.c static struct scsi_host_template srp_template = { · line 1955

Quoted 7 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

@@ -1949,6 +1949,7 @@ static struct scsi_host_template srp_template = {
  	.eh_abort_handler		= srp_abort,
  	.eh_device_reset_handler	= srp_reset_device,
  	.eh_host_reset_handler		= srp_reset_host,
+	.skip_settle_delay		= true,
  	.sg_tablesize			= SRP_DEF_SG_TABLESIZE,
  	.can_queue			= SRP_CMD_SQ_SIZE,
  	.this_id			= -1,

Signed-off-by Sebastian Riemer <[email protected]>

Acked-by Sebastian Riemer <[email protected]>

Tested-by Sebastian Riemer <[email protected]>

Reviewed-by Sebastian Riemer <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Choose something,

An SRP target is required to maintain a single connection between initiator and target. This means that if the 'add_target' attribute is used to create a second connection to a target that the first connection will be logged out and that the SCSI error handler will kick in. The SCSI error handler will cause the SRP initiator to reconnect, which will cause I/O over the second connection to fail. Avoid such ping-pong behavior by disabling relogins. Note: if reconnecting manually is necessary, that is possible by deleting and recreating an rport via sysfs.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c |   38 +++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

drivers/infiniband/ulp/srp/ib_srp.c +20 -0

index be12780..1a73b24 100644
@@ -556,6 +556,36 @@ static void srp_rport_delete(struct srp_rport *rport)
 	srp_queue_remove_work(target);
 }
 
+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */
+static bool srp_conn_unique(struct srp_host *host,
+			    struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	bool ret = false;
+
+	if (target->state == SRP_TARGET_REMOVED)
+		goto out;
+
+	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 &&
+		    target->initiator_ext == t->initiator_ext) {

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;

@@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)

You've only changed the style of this function. Functionality is still the same. Fine for me.

But why do you put it that high in the source code? Do you (still) need it for something else?

I would put it directly in front of srp_create_target() or even in front of that option parsing stuff for correct bottom-up.

Quoted 5 lines

  static int srp_connect_target(struct srp_target_port *target)
  {
  	int retries = 3;

drivers/infiniband/ulp/srp/ib_srp.c +0 -0

@@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
  	if (ret)

drivers/infiniband/ulp/srp/ib_srp.c static ssize_t srp_create_target(struct device *dev, · line 2301

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +8 -0

  		goto err;
 
+	if (!srp_conn_unique(target->srp_host, target)) {
+		shost_printk(KERN_INFO, target->scsi_host,
+			     PFX "Already connected to target port %.*s\n",
+			     (int)count, buf);
+		ret = -EEXIST;
+		goto err;
+	}
+

Yes, this looks good! Nice idea to print the connection string! Would be even cooler without trailing '\n' from within 'buf' but that's okay.

I was a little bit afraid of overflows here so I did security testing. But srp_parse_options() already rejected my evil connection strings. :-)

I've tried things like this:
id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
xxxxxxxxxxxxxxxxxxxxxxxxx... until 4096 chars

id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
id_ext=0002c903004ed0b2,\
ioc_guid=0002c903004ed0b2,\
dgid=fe800000000000000002c903004ed0b4,\
pkey=ffff,service_id=0002c903004ed0b2,\
... until 4096 chars

This string looked kind of funny. Also in the kernel message it was a little bit longer than usual but the parsing detected that I have too many parameters. So everything is fine in terms of security.

The "%.*s" should only copy the data provided by the user, even if it is not '\0' terminated. Stripping the trailing newline is probably possible with something like the (untested) code below (will only work if there is only one newline in the input string and if it's at the end):

		shost_printk(KERN_INFO, target->scsi_host,
			     PFX "Already connected to target port %.*s\n",
			     (int)count - (memchr(buf, '\n', count) ==
					   buf + count - 1), buf);

drivers/infiniband/ulp/srp/ib_srp.c +8 -0

+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */
+static bool srp_conn_unique(struct srp_host *host,
+			    struct srp_target_port *target)
+{
+	struct srp_target_port *t;
+	bool ret = false;

drivers/infiniband/ulp/srp/ib_srp.c +13 -0

+		    target->ioc_guid == t->ioc_guid &&
+		    target->initiator_ext == t->initiator_ext) {
+			ret = false;
+			break;
+		}
+	}
+	spin_unlock(&host->target_lock);
+
+out:
+	return ret;
+}
+
 static int srp_connect_target(struct srp_target_port *target)
 {
 	int retries = 3;

@@ -2261,6 +2291,14 @@ static ssize_t srp_create_target(struct device *dev,
 	if (ret)
 		goto err;
 
+	if (!srp_conn_unique(target->srp_host, target)) {

Hello Bart,

Quoted 9 lines

+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */

drivers/infiniband/ulp/srp/ib_srp.c +5 -0

+static bool srp_conn_unique(struct srp_host *host,
+                struct srp_target_port *target)
+{
+    struct srp_target_port *t;
+    bool ret = false;

drivers/infiniband/ulp/srp/ib_srp.c static void srp_rport_delete(struct srp_rport *rport) · line 576

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +10 -0

+
+    if (target->state == SRP_TARGET_REMOVED)
+        goto out;
+
+    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 &&

Targets may advertise/expose on different pkeys
You can have multiple connections (or paths/scsi hosts) to same target with different pkeys.
We need extra check to detect the uniqueness:

                                             target->path.pkey ==

t->path.pkey &&

Documentation/ABI/stable/sysfs-transport-srp +9 -0

+
+What:		/sys/class/srp_remote_ports/port-<h>:<n>/fast_io_fail_tmo
+Date:		September 1, 2013
+KernelVersion:	3.11
+Contact:	[email protected], [email protected]
+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.

drivers/scsi/scsi_transport_srp.c +8 -0

+
+/**
+ * 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.
+ */

+ * srp_reconnect_rport - reconnect by invoking srp_function_template.reconnect()
+ *
+ * Blocks SCSI command queueing before invoking reconnect() such that the
+ * scsi_host_template.queuecommand() won't be invoked concurrently with
+ * reconnect(). This is important since a reconnect() implementation may
+ * reallocate resources needed by queuecommand(). Please note that this
+ * function neither waits until outstanding requests have finished nor tries
+ * to abort these. It is the responsibility of the reconnect() function to
+ * finish outstanding commands before reconnecting to the target port.
+ */

drivers/scsi/scsi_transport_srp.c +10 -0

+int srp_reconnect_rport(struct srp_rport *rport)
+{
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	struct srp_internal *i = to_srp_internal(shost->transportt);
+	struct scsi_device *sdev;
+	int res;
+
+	pr_debug("SCSI host %s\n", dev_name(&shost->shost_gendev));
+
+	res = mutex_lock_interruptible(&rport->mutex);

drivers/scsi/scsi_transport_srp.c +19 -0

+	while (scsi_request_fn_active(shost))
+		msleep(20);
+
+	res = i->f->reconnect(rport);
+	scsi_unblock_requests(shost);
+	pr_debug("%s (state %d): transport.reconnect() returned %d\n",
+		 dev_name(&shost->shost_gendev), rport->state, res);
+	if (res == 0) {
+		cancel_delayed_work(&rport->fast_io_fail_work);
+		cancel_delayed_work(&rport->dev_loss_work);
+		rport->failed_reconnects = 0;
+		scsi_target_unblock(&shost->shost_gendev, SDEV_RUNNING);
+		srp_rport_set_state(rport, SRP_RPORT_RUNNING);
+		/*
+		 * It can occur that after fast_io_fail_tmo expired and before
+		 * dev_loss_tmo expired that the SCSI error handler has
+		 * offlined one or more devices. scsi_target_unblock() doesn't
+		 * change the state of these devices into running, so do that
+		 * explicitly.

drivers/scsi/scsi_transport_srp.c +130 -0

+	}
+	mutex_unlock(&rport->mutex);
+
+out:
+	return res;
+}
+EXPORT_SYMBOL(srp_reconnect_rport);
+
+static void srp_reconnect_work(struct work_struct *work)
+{
+	struct srp_rport *rport = container_of(to_delayed_work(work),
+					struct srp_rport, reconnect_work);
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	int delay, res;
+
+	res = srp_reconnect_rport(rport);
+	if (res != 0) {
+		shost_printk(KERN_ERR, shost,
+			     "reconnect attempt %d failed (%d)\n",
+			     ++rport->failed_reconnects, res);
+		delay = rport->reconnect_delay *
+			min(100, max(1, rport->failed_reconnects - 10));
+		if (delay > 0)
+			queue_delayed_work(system_long_wq,
+					   &rport->reconnect_work, delay * HZ);
+	}
+}
+
+static void __rport_fast_io_fail_timedout(struct srp_rport *rport)
+{
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	struct srp_internal *i;
+
+	lockdep_assert_held(&rport->mutex);
+
+	if (srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST))
+		return;
+	scsi_target_unblock(rport->dev.parent, SDEV_TRANSPORT_OFFLINE);
+
+	/* Involve the LLDD if possible to terminate all io on the rport. */
+	i = to_srp_internal(shost->transportt);
+	if (i->f->terminate_rport_io)
+		i->f->terminate_rport_io(rport);
+}
+
+/**
+ * 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 = container_of(to_delayed_work(work),
+					struct srp_rport, fast_io_fail_work);
+	struct Scsi_Host *shost = rport_to_shost(rport);
+
+	pr_debug("fast_io_fail_tmo expired for %s.\n",
+		 dev_name(&shost->shost_gendev));
+
+	mutex_lock(&rport->mutex);
+	__rport_fast_io_fail_timedout(rport);
+	mutex_unlock(&rport->mutex);
+}
+
+/**
+ * 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 = container_of(to_delayed_work(work),
+					struct srp_rport, dev_loss_work);
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	struct srp_internal *i = to_srp_internal(shost->transportt);
+
+	pr_err("dev_loss_tmo expired for %s.\n",
+	       dev_name(&shost->shost_gendev));
+
+	mutex_lock(&rport->mutex);
+	WARN_ON(srp_rport_set_state(rport, SRP_RPORT_LOST) != 0);
+	scsi_target_unblock(rport->dev.parent, SDEV_TRANSPORT_OFFLINE);
+	mutex_unlock(&rport->mutex);
+
+	i->f->rport_delete(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)
+{
+	struct Scsi_Host *shost = rport_to_shost(rport);
+	int delay;
+
+	mutex_lock(&rport->mutex);
+	pr_debug("%s current state: %d\n", dev_name(&shost->shost_gendev),
+		 rport->state);
+	if (srp_rport_set_state(rport, SRP_RPORT_BLOCKED) != 0)
+		goto out_unlock;
+	pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev),
+		 rport->state);
+	scsi_target_block(&shost->shost_gendev);
+
+	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);
+	if (rport->dev_loss_tmo >= 0)
+		queue_delayed_work(system_long_wq, &rport->dev_loss_work,
+				   1UL * rport->dev_loss_tmo * HZ);
+	delay = rport->reconnect_delay;
+	if (delay > 0)
+		queue_delayed_work(system_long_wq, &rport->reconnect_work,
+				   1UL * delay * HZ);
+out_unlock:
+	mutex_unlock(&rport->mutex);
+}
+EXPORT_SYMBOL(srp_start_tl_fail_timers);
+
+/**
+ * srp_timed_out - SRP transport intercept of the SCSI timeout EH
+ *
+ * If a timeout occurs while an rport is in the blocked state, ask the SCSI
+ * EH to continue waiting (BLK_EH_RESET_TIMER). Otherwise let the SCSI core
+ * handle the timeout (BLK_EH_NOT_HANDLED).
+ *

Quoted 26 lines · Vu Pham

Hello Bart,

Quoted 9 lines

+/**
+ * srp_conn_unique() - check whether the connection to a target is unique
+ */

drivers/infiniband/ulp/srp/ib_srp.c +5 -0

+static bool srp_conn_unique(struct srp_host *host,
+                struct srp_target_port *target)
+{
+    struct srp_target_port *t;
+    bool ret = false;

drivers/infiniband/ulp/srp/ib_srp.c static void srp_rport_delete(struct srp_rport *rport) · line 576

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +10 -0

+
+    if (target->state == SRP_TARGET_REMOVED)
+        goto out;
+
+    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 &&

Targets may advertise/expose on different pkeys
You can have multiple connections (or paths/scsi hosts) to same target with different pkeys.
We need extra check to detect the uniqueness:

Quoted 2 lines

                                             target->path.pkey ==

t->path.pkey &&

Hello Vu,

Thanks for the feedback. This is something I have already thinking about myself. Unfortunately I have not found any requirements in the T10 SRP standard with regard to InfiniBand partitions. However, in that document there is a section about single RDMA channel operation. In that section it is explained that an SRP target must log out established sessions upon receipt of a new login request. What I'm not sure about is whether only sessions with the same P_Key must be logged out or all established sessions if a new login request is received. I assume the latter since otherwise that would mean that an SRP target would be required to maintain multiple sessions if it allows connections with more than one P_Key to a target port ? My concern about adding a pkey comparison in the function srp_conn_unique() is that if a target allows an initiator to choose which partition to use when logging in, that this could result in the undesired SRP initiator ping-pong effect this patch tries to avoid.

Bart.

Quoted 38 lines · Bart Van Assche

Quoted 12 lines · Vu Pham

Hello Bart,

Quoted 9 lines

+/** + * srp_conn_unique() - check whether the connection to a target is unique + */ +static bool srp_conn_unique(struct
srp_host *host, + struct srp_target_port
*target) +{ + struct srp_target_port *t; + bool ret =
false; + + if (target->state == SRP_TARGET_REMOVED) +
goto out; + + 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 &&

Quoted 4 lines

Targets may advertise/expose on different pkeys You can have multiple connections (or paths/scsi hosts) to same target with different pkeys. We need extra check to detect the uniqueness:
target->path.pkey == t->path.pkey &&

Hello Vu,

Thanks for the feedback. This is something I have already thinking about myself. Unfortunately I have not found any requirements in the T10 SRP standard with regard to InfiniBand partitions. However, in that document there is a section about single RDMA channel operation. In that section it is explained that an SRP target must log out established sessions upon receipt of a new login request. What I'm not sure about is whether only sessions with the same P_Key must be logged out or all established sessions if a new login request is received. I assume the latter since otherwise that would mean that an SRP target would be required to maintain multiple sessions if it allows connections with more than one P_Key to a target port ? My concern about adding a pkey comparison in the function srp_conn_unique() is that if a target allows an initiator to choose which partition to use when logging in, that this could result in the undesired SRP initiator ping-pong effect this patch tries to avoid.

Quoted 2 lines

Bart.

Hello Bart,

Yes, you pointed out the unclear/undefined area.

If we stick to single RDMA channel per IT Nexus with unique tuple Initiator Port Identier - Target Port Indentifier then newly created connection with same tuple (I_port_id, T_port_id) but with different P_Key or different DGID is not unique.

Sticking to this rule by excluding P_Key and DGID out of rdma channel indentity, your srp_conn_unique() checking is ok; however, some SRP target implementations may include DGID as part of rdma channel identifier. I'm not sure about different p_key part.

-vu

Hello Bart,

Review

Quoted 9 lines

(quoted diff) +5 -0

+What:        /sys/class/srp_remote_ports/port-<h>:<n>/dev_loss_tmo
+Date:        September 1, 2013
+KernelVersion:    3.11
+Contact:    [email protected], [email protected]
+Description:    Number of seconds the SCSI layer will wait after a 

transport

(quoted diff) +2 -0

+        layer error has been observed before removing a target port.
+        Zero means immediate removal.

A negative value will disable the target port removal.

<snip>

Quoted 16 lines

(quoted diff) +5 -0

+
+/**
+ * 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.
+ */

(quoted diff) +6 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&
+            (dev_loss_tmo < 0 ||

Review

Quoted 4 lines

(quoted diff) +4 -0

+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

<snip>

Quoted 28 lines

+ * srp_reconnect_rport - reconnect by invoking srp_function_template.reconnect()
+ *
+ * Blocks SCSI command queueing before invoking reconnect() such that the
+ * scsi_host_template.queuecommand() won't be invoked concurrently with
+ * reconnect(). This is important since a reconnect() implementation may
+ * reallocate resources needed by queuecommand(). Please note that this
+ * function neither waits until outstanding requests have finished nor tries
+ * to abort these. It is the responsibility of the reconnect() function to
+ * finish outstanding commands before reconnecting to the target port.
+ */

(quoted diff) +13 -0

+int srp_reconnect_rport(struct srp_rport *rport)
+{
+    struct Scsi_Host *shost = rport_to_shost(rport);
+    struct srp_internal *i = to_srp_internal(shost->transportt);
+    struct scsi_device *sdev;
+    int res;
+
+    pr_debug("SCSI host %s\n", dev_name(&shost->shost_gendev));
+
+    res = mutex_lock_interruptible(&rport->mutex);
+    if (res) {
+        pr_debug("%s: mutex_lock_interruptible() returned %d\n",
+             dev_name(&shost->shost_gendev), res);

Review

Quoted 7 lines

(quoted diff) +7 -0

+        goto out;
+    }
+
+    spin_lock_irq(shost->host_lock);
+    scsi_block_requests(shost);
+    spin_unlock_irq(shost->host_lock);
+

In scsi_block_requests() definition, no locks are assumed held.

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to do extra block with scsi_block_requests()

Shouldn't we avoid using both scsi_block/unblock_requests() and scsi_target_block/unblock()?
ie. in srp_start_tl_fail_timers() call scsi_block_requests(), in __rport_fast_io_fail_timedout() and rport_dev_loss_timedout() call scsi_unblock_requests()
and using scsi_block/unblock_requests in this function.

Quoted 19 lines

(quoted diff) +19 -0

+    while (scsi_request_fn_active(shost))
+        msleep(20);
+
+    res = i->f->reconnect(rport);
+    scsi_unblock_requests(shost);
+    pr_debug("%s (state %d): transport.reconnect() returned %d\n",
+         dev_name(&shost->shost_gendev), rport->state, res);
+    if (res == 0) {
+        cancel_delayed_work(&rport->fast_io_fail_work);
+        cancel_delayed_work(&rport->dev_loss_work);
+        rport->failed_reconnects = 0;
+        scsi_target_unblock(&shost->shost_gendev, SDEV_RUNNING);
+        srp_rport_set_state(rport, SRP_RPORT_RUNNING);
+        /*
+         * It can occur that after fast_io_fail_tmo expired and before
+         * dev_loss_tmo expired that the SCSI error handler has
+         * offlined one or more devices. scsi_target_unblock() doesn't
+         * change the state of these devices into running, so do that
+         * explicitly.

Review

Quoted 10 lines

(quoted diff) +10 -0

+         */
+        spin_lock_irq(shost->host_lock);
+        __shost_for_each_device(sdev, shost)
+            if (sdev->sdev_state == SDEV_OFFLINE)
+                sdev->sdev_state = SDEV_RUNNING;
+        spin_unlock_irq(shost->host_lock);
+    } else if (rport->state == SRP_RPORT_RUNNING) {
+        srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST);
+        scsi_target_unblock(&shost->shost_gendev,
+                    SDEV_TRANSPORT_OFFLINE);

Would this unblock override the fast_io_fail_tmo functionality?

Quoted 5 lines

(quoted diff) +5 -0

+static enum blk_eh_timer_return srp_timed_out(struct scsi_cmnd *scmd)
+{
+    struct scsi_device *sdev = scmd->device;
+
+    pr_debug("timeout for sdev %s\n", dev_name(&sdev->sdev_gendev));

Review

Quoted 2 lines

(quoted diff) +2 -0

+    return scsi_device_blocked(sdev) ? BLK_EH_RESET_TIMER :
+        BLK_EH_NOT_HANDLED;

if one turn off fast_io_fail_tmo, rport_fast_io_fail_timedout() won't be called, and reconnect is failed, scsi_device_blocked remains true, error recovery cannot happen
I think it should be
return (scsi_device_blocked(sdev) && rport->fast_io_fail_tmo > 0) ? BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;

Quoted 56 lines

Quoted 37 lines · Bart Van Assche

Quoted 13 lines · Vu Pham

Hello Bart,

Quoted 10 lines

+/** + * srp_conn_unique() - check whether the connection to a target is unique + */ +static bool srp_conn_unique(struct
srp_host *host, + struct srp_target_port
*target) +{ + struct srp_target_port *t; + bool ret =
false; + + if (target->state == SRP_TARGET_REMOVED) +
goto out; + + 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 &&

Quoted 4 lines

Targets may advertise/expose on different pkeys You can have
multiple connections (or paths/scsi hosts) to same target with
different pkeys. We need extra check to detect the uniqueness: target->path.pkey == t->path.pkey &&

Hello Vu,

Thanks for the feedback. This is something I have already
thinking about myself. Unfortunately I have not found any
requirements in the T10 SRP standard with regard to InfiniBand
partitions. However, in that document there is a section about
single RDMA channel operation. In that section it is explained
that an SRP target must log out established sessions upon receipt
of a new login request. What I'm not sure about is whether only
sessions with the same P_Key must be logged out or all
established sessions if a new login request is received. I assume
the latter since otherwise that would mean that an SRP target would be required to maintain multiple sessions if it allows connections with more than one P_Key to a target port ? My concern about adding a pkey comparison in the function
srp_conn_unique() is that if a target allows an initiator to
choose which partition to use when logging in, that this could
result in the undesired SRP initiator ping-pong effect this patch
tries to avoid.

Quoted 3 lines

Bart.

Hello Bart,

Yes, you pointed out the unclear/undefined area.

If we stick to single RDMA channel per IT Nexus with unique tuple Initiator Port Identier - Target Port Indentifier then newly created connection with same tuple (I_port_id, T_port_id) but with
different P_Key or different DGID is not unique.

Sticking to this rule by excluding P_Key and DGID out of rdma channel indentity, your srp_conn_unique() checking is ok; however, some SRP target implementations may include DGID as part of rdma channel identifier. I'm not sure about different p_key part.

Quoted 2 lines

-vu

Hi Vu!

For what do you need the same target with multiple pkeys on the same local SRP port?

Which other SRP targets exist?

I only know SCST, Solaris COMSTAR and that broken LIO stuff. Does SCST still not support to set the pkey?

Why should we check the dgid?

Doesn't make any sense to me to connect both target ports to the same local port.

If you do so, the multipath-tools will crash. Note: This function is called per local SRP port. Perhaps, a note should be added to that function that it only has to be called per local SRP port.

Cheers,
Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJRuuSCAAoJEH4DRb7WXajZcFcH+gKsSs64Js/CUqMSyPeFPQ7u 7jKHvLr2wqHqSMIg5rEeZxZJpE9rL+wi8k5TMAMBrV+Povdwr8tWHgdq7mh5N1xO V517YTgdzrwPIFy9e2uktxx4VYpsFGrV8iw3rdAzXRmcYa5U8feXhiD1VZyKjs4p 3//wvGAR0po7Pm0WgU9Q+h0arQos8CmeHkpoaNp/nNINXpXlTX21WVvHjwQrMFhC Kr8zoCOTd0Sn+WoSs+CT/7Y4oTknukwR5vh6wfKgz2W74YkMKpD658QZozlafyK/ rwdajV19YYvi8YRTjUXuY5TN0qshYOGDxJDtNFkRGbx+IxIqFkGyyFCp0LPCfto= =nlf2
-----END PGP SIGNATURE-----

Quoted 32 lines

Hello Bart,

Review

Quoted 9 lines

(quoted diff) +5 -0

+What:        /sys/class/srp_remote_ports/port-<h>:<n>/dev_loss_tmo
+Date:        September 1, 2013
+KernelVersion:    3.11
+Contact:    [email protected], [email protected]
+Description:    Number of seconds the SCSI layer will wait after a 

transport

(quoted diff) +2 -0

+        layer error has been observed before removing a target port.
+        Zero means immediate removal.

A negative value will disable the target port removal.

<snip>

Quoted 15 lines

(quoted diff) +5 -0

+
+/**
+ * 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.
+ */

(quoted diff) +5 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +5 -0

+            (dev_loss_tmo < 0 ||
+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

OK, will update the documentation such that it correctly refers to "off" instead of a negative value and I will also mention that dev_loss_tmo can now be disabled.

Quoted 30 lines

<snip>

Quoted 28 lines

+ * srp_reconnect_rport - reconnect by invoking srp_function_template.reconnect()
+ *
+ * Blocks SCSI command queueing before invoking reconnect() such that the
+ * scsi_host_template.queuecommand() won't be invoked concurrently with
+ * reconnect(). This is important since a reconnect() implementation may
+ * reallocate resources needed by queuecommand(). Please note that this
+ * function neither waits until outstanding requests have finished nor tries
+ * to abort these. It is the responsibility of the reconnect() function to
+ * finish outstanding commands before reconnecting to the target port.
+ */

(quoted diff) +14 -0

+int srp_reconnect_rport(struct srp_rport *rport)
+{
+    struct Scsi_Host *shost = rport_to_shost(rport);
+    struct srp_internal *i = to_srp_internal(shost->transportt);
+    struct scsi_device *sdev;
+    int res;
+
+    pr_debug("SCSI host %s\n", dev_name(&shost->shost_gendev));
+
+    res = mutex_lock_interruptible(&rport->mutex);
+    if (res) {
+        pr_debug("%s: mutex_lock_interruptible() returned %d\n",
+             dev_name(&shost->shost_gendev), res);
+        goto out;

Quoted 9 lines

Review

Quoted 7 lines

(quoted diff) +6 -0

+    }
+
+    spin_lock_irq(shost->host_lock);
+    scsi_block_requests(shost);
+    spin_unlock_irq(shost->host_lock);
+

In scsi_block_requests() definition, no locks are assumed held.

Good catch :-) However, if you look around in drivers/scsi you will see that several SCSI LLD drivers invoke scsi_block_requests() with the host lock held. I'm not sure whether these LLDs or the scsi_block_requests() documentation is incorrect. Anyway, I'll leave the locking statements out since these are not necessary around this call of scsi_block_requests().

Quoted 2 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to do extra block with scsi_block_requests()

Please keep in mind that srp_reconnect_rport() can be called from two different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in this function.

Quoted 5 lines

Shouldn't we avoid using both scsi_block/unblock_requests() and scsi_target_block/unblock()?
ie. in srp_start_tl_fail_timers() call scsi_block_requests(), in __rport_fast_io_fail_timedout() and rport_dev_loss_timedout() call scsi_unblock_requests() and using scsi_block/unblock_requests in this function.

I agree that using only one of these two mechanisms would be more elegant. However, the advantage of using scsi_target_block() as long as fast_io_fail_tmo has not expired is that this functions set the SDEV_BLOCK state which is recognized by multipathd.

The reason scsi_block_requests() is used in the above function is to prevent that a user can reenable requests via sysfs while a new InfiniBand RC connection is being established. A call of srp_queuecommand() concurrently with srp_create_target_ib() can namely result in a kernel crash.

Quoted 30 lines

Quoted 30 lines

(quoted diff) +29 -0

+    while (scsi_request_fn_active(shost))
+        msleep(20);
+
+    res = i->f->reconnect(rport);
+    scsi_unblock_requests(shost);
+    pr_debug("%s (state %d): transport.reconnect() returned %d\n",
+         dev_name(&shost->shost_gendev), rport->state, res);
+    if (res == 0) {
+        cancel_delayed_work(&rport->fast_io_fail_work);
+        cancel_delayed_work(&rport->dev_loss_work);
+        rport->failed_reconnects = 0;
+        scsi_target_unblock(&shost->shost_gendev, SDEV_RUNNING);
+        srp_rport_set_state(rport, SRP_RPORT_RUNNING);
+        /*
+         * It can occur that after fast_io_fail_tmo expired and before
+         * dev_loss_tmo expired that the SCSI error handler has
+         * offlined one or more devices. scsi_target_unblock() doesn't
+         * change the state of these devices into running, so do that
+         * explicitly.
+         */
+        spin_lock_irq(shost->host_lock);
+        __shost_for_each_device(sdev, shost)
+            if (sdev->sdev_state == SDEV_OFFLINE)
+                sdev->sdev_state = SDEV_RUNNING;
+        spin_unlock_irq(shost->host_lock);
+    } else if (rport->state == SRP_RPORT_RUNNING) {
+        srp_rport_set_state(rport, SRP_RPORT_FAIL_FAST);
+        scsi_target_unblock(&shost->shost_gendev,
+                    SDEV_TRANSPORT_OFFLINE);

Quoted 2 lines

Would this unblock override the fast_io_fail_tmo functionality?

Sorry but I don't think so. If something goes wrong with the transport layer then the function srp_start_tl_fail_timers() gets invoked. That function starts with changing the state from SRP_RPORT_RUNNING into SRP_RPORT_BLOCKED. In other words, the code below "if (rport->state == SRP_RPORT_RUNNING)" can only be reached if srp_reconnect_rport() is invoked from the context of the SCSI error handler and not if it is called from inside one of the new SRP transport layer times. In other words, whether or not that code will be reached depends on whether fast_io_fail_tmo is below or above the SCSI command timeout.

Quoted 12 lines

Review

Quoted 8 lines

(quoted diff) +7 -0

+static enum blk_eh_timer_return srp_timed_out(struct scsi_cmnd *scmd)
+{
+    struct scsi_device *sdev = scmd->device;
+
+    pr_debug("timeout for sdev %s\n", dev_name(&sdev->sdev_gendev));
+    return scsi_device_blocked(sdev) ? BLK_EH_RESET_TIMER :
+        BLK_EH_NOT_HANDLED;

if one turn off fast_io_fail_tmo, rport_fast_io_fail_timedout() won't be called, and reconnect is failed, scsi_device_blocked remains true, error recovery cannot happen I think it should be
return (scsi_device_blocked(sdev) && rport->fast_io_fail_tmo > 0) ? BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;

How about the following alternative ?

drivers/scsi/scsi_transport_srp.c +3 -2

index 53b34b3..84574fb 100644
@@ -550,11 +550,12 @@ void srp_start_tl_fail_timers(struct srp_rport *rport)
                goto out_unlock;
        pr_debug("%s new state: %d\n", dev_name(&shost->shost_gendev),
                 rport->state);
-       scsi_target_block(&shost->shost_gendev);
 
-       if (rport->fast_io_fail_tmo >= 0)
+       if (rport->fast_io_fail_tmo >= 0) {
+               scsi_target_block(&shost->shost_gendev);
                queue_delayed_work(system_long_wq, &rport->fast_io_fail_work,
                                   1UL * rport->fast_io_fail_tmo * HZ);
+       }
        if (rport->dev_loss_tmo >= 0)
                queue_delayed_work(system_long_wq, &rport->dev_loss_work,
                                   1UL * rport->dev_loss_tmo * HZ);

Bart.

Quoted 25 lines · Vu Pham

Quoted 23 lines

Quoted 9 lines

+/**
+ * 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.
+ */

Review

Quoted 10 lines

(quoted diff) +10 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&
+            (dev_loss_tmo < 0 ||
+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

Quoted 4 lines

OK, will update the documentation such that it correctly refers to "off" instead of a negative value and I will also mention that dev_loss_tmo can now be disabled.

It's not only the documentation but also the code logic, you cannot turn dev_loss_tmo off if fast_io_fail_tmo already turned off and vice versa with the return statement above.

Quoted 11 lines

Quoted 2 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to do extra block with scsi_block_requests()

Please keep in mind that srp_reconnect_rport() can be called from two different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in this function.

Yes, srp_reconnect_rport() can be called from two contexts; however, it deals with same rport & rport's state.
I'm thinking something like this:

    if (rport->state != SRP_RPORT_BLOCKED) {
     scsi_block_requests(shost);

I think that we can use only the pair scsi_block_requests()/scsi_unblock_requests() unless the advantage of multipathd recognizing the SDEV_BLOCK is noticeable.

Quoted 31 lines · Vu Pham

Quoted 27 lines · Vu Pham

Quoted 17 lines

Quoted 17 lines

+/**
+ * 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.
+ */

(quoted diff) +6 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&
+            (dev_loss_tmo < 0 ||

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

Quoted 4 lines

OK, will update the documentation such that it correctly refers to "off" instead of a negative value and I will also mention that dev_loss_tmo can now be disabled.

Quoted 5 lines

It's not only the documentation but also the code logic, you cannot turn
dev_loss_tmo off if fast_io_fail_tmo already turned off and vice versa
with the return statement above.

Does this mean that you think it would be useful to disable both the fast_io_fail and the dev_loss mechanisms, and hence rely on the user to remove remote ports that have disappeared and on the SCSI command timeout to detect path failures ? I'll start testing this to see whether that combination does not trigger any adverse behavior.

Quoted 15 lines

Quoted 4 lines

Quoted 3 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need
to do extra block with scsi_block_requests()

Quoted 11 lines

Please keep in mind that srp_reconnect_rport() can be called from two
different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI
error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in
this function.

Quoted 7 lines

Yes, srp_reconnect_rport() can be called from two contexts; however, it
deals with same rport & rport's state.
I'm thinking something like this:

    if (rport->state != SRP_RPORT_BLOCKED) {
     scsi_block_requests(shost);

Sorry but I'm afraid that that approach would still allow the user to unblock one or more SCSI devices via sysfs during the i->f->reconnect(rport) call, something we do not want.

Quoted 3 lines

I think that we can use only the pair scsi_block_requests()/scsi_unblock_requests() unless the advantage of multipathd recognizing the SDEV_BLOCK is noticeable.

I think the advantage of multipathd recognizing the SDEV_BLOCK state before the fast_io_fail_tmo timer has expired is important. Multipathd does not queue I/O to paths that are in the SDEV_BLOCK state so setting that state helps I/O to fail over more quickly, especially for large values of fast_io_fail_tmo.

Quoted 75 lines · Bart Van Assche

Quoted 31 lines · Vu Pham

Quoted 27 lines · Vu Pham

Quoted 17 lines

Quoted 17 lines

+/**
+ * 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.
+ */

(quoted diff) +6 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&
+            (dev_loss_tmo < 0 ||

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

Quoted 4 lines

OK, will update the documentation such that it correctly refers to "off" instead of a negative value and I will also mention that dev_loss_tmo can now be disabled.

Quoted 5 lines

It's not only the documentation but also the code logic, you cannot turn
dev_loss_tmo off if fast_io_fail_tmo already turned off and vice versa
with the return statement above.

Does this mean that you think it would be useful to disable both the fast_io_fail and the dev_loss mechanisms, and hence rely on the user to remove remote ports that have disappeared and on the SCSI command timeout to detect path failures ? I'll start testing this to see whether that combination does not trigger any adverse behavior.

Quoted 15 lines

Quoted 4 lines

Quoted 3 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need
to do extra block with scsi_block_requests()

Quoted 11 lines

Please keep in mind that srp_reconnect_rport() can be called from two
different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI
error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in
this function.

Quoted 7 lines

Yes, srp_reconnect_rport() can be called from two contexts; however, it
deals with same rport & rport's state.
I'm thinking something like this:

     if (rport->state != SRP_RPORT_BLOCKED) {
      scsi_block_requests(shost);

Sorry but I'm afraid that that approach would still allow the user to unblock one or more SCSI devices via sysfs during the i->f->reconnect(rport) call, something we do not want.

Quoted 3 lines

I think that we can use only the pair scsi_block_requests()/scsi_unblock_requests() unless the advantage of multipathd recognizing the SDEV_BLOCK is noticeable.

Quoted 6 lines

I think the advantage of multipathd recognizing the SDEV_BLOCK state before the fast_io_fail_tmo timer has expired is important. Multipathd does not queue I/O to paths that are in the SDEV_BLOCK state so setting that state helps I/O to fail over more quickly, especially for large values of fast_io_fail_tmo.

Sadly it doesn't work that way.

SDEV_BLOCK will instruct multipath to not queue _new_ I/Os to the path, but there still will be I/O queued on that path. For these multipath _has_ to wait for I/O completion.
And as it turns out, in most cases the application itself will wait for completion on these I/O before continue sending more I/O. So in effect multipath would queue new I/O to other paths, but won't _receive_ new I/O as the upper layers are still waiting for completion of the queued I/O.

The only way to excite fast failover with multipathing is to set fast_io_fail to a _LOW_ value (eg 5 seconds), as this will terminate the outstanding I/Os.

Large values of fast_io_fail will almost guarantee sluggish I/O failover.

Quoted 1 line · Hannes Reinecke

On 06/15/2013 11:52 AM, Bart Van Assche wrote:

Quoted 18 lines

Quoted 7 lines

I think the advantage of multipathd recognizing the SDEV_BLOCK state before the fast_io_fail_tmo timer has expired is important. Multipathd does not queue I/O to paths that are in the SDEV_BLOCK state so setting that state helps I/O to fail over more quickly, especially for large values of fast_io_fail_tmo.

Sadly it doesn't work that way.

SDEV_BLOCK will instruct multipath to not queue _new_ I/Os to the path, but there still will be I/O queued on that path. For these multipath _has_ to wait for I/O completion.
And as it turns out, in most cases the application itself will wait for completion on these I/O before continue sending more I/O. So in effect multipath would queue new I/O to other paths, but won't _receive_ new I/O as the upper layers are still waiting for completion of the queued I/O.

Quoted 6 lines

The only way to excite fast failover with multipathing is to set fast_io_fail to a _LOW_ value (eg 5 seconds), as this will terminate the outstanding I/Os.

Large values of fast_io_fail will almost guarantee sluggish I/O failover.

Hello Hannes,

I agree that the value of fast_io_fail_tmo should be kept small. Although as you explained changing the SCSI device state into SDEV_BLOCK doesn't help for I/O that has already been queued on a failed path, I think it's still useful for I/O that is queued after the fast_io_fail timer has been started and before that timer has expired.

Quoted 6 lines · Bart Van Assche

I agree that the value of fast_io_fail_tmo should be kept small. Although as you explained changing the SCSI device state into SDEV_BLOCK doesn't help for I/O that has already been queued on a failed path, I think it's still useful for I/O that is queued after the fast_io_fail timer has been started and before that timer has expired.

Why, but of course.

The typical scenario would be:
-> detect link-loss
-> call scsi_block_request()
-> start dev_loss_tmo and fast_io_fail_tmo

-> When fast_io_fail_tmo triggers:

    -> Abort all outstanding requests

-> When dev_loss_tmo triggers:

    -> Abort all outstanding requests
    -> Remove/disable the I_T nexus
    -> call scsi_unblock_request()

However, if and whether multipath detects SDEV_BLOCK doesn't guarantee a fast failover; in fact is was only added rather recently as it's not a big win in most cases.

Quoted 23 lines · Hannes Reinecke

Quoted 6 lines · Bart Van Assche

I agree that the value of fast_io_fail_tmo should be kept small. Although as you explained changing the SCSI device state into SDEV_BLOCK doesn't help for I/O that has already been queued on a failed path, I think it's still useful for I/O that is queued after the fast_io_fail timer has been started and before that timer has expired.

Why, but of course.

The typical scenario would be:
-> detect link-loss
-> call scsi_block_request()
-> start dev_loss_tmo and fast_io_fail_tmo

-> When fast_io_fail_tmo triggers:

    -> Abort all outstanding requests

-> When dev_loss_tmo triggers:

    -> Abort all outstanding requests
    -> Remove/disable the I_T nexus
    -> call scsi_unblock_request()

Quoted 3 lines

However, if and whether multipath detects SDEV_BLOCK doesn't guarantee a fast failover; in fact is was only added rather recently as it's not a big win in most cases.

Even if setting the state SDEV_BLOCK doesn't help much with improving failover time, it still has the advantage over using scsi_block_requests() that it can be overridden by a user via sysfs.

Quoted 3 lines

For what do you need the same target with multiple pkeys on the same local SRP port?

There is no need, it's just a gray area that you can choose to have multiple connections to same target using different pkeys (same as dgid)

Quoted 2 lines

Which other SRP targets exist?

Netapp/LSI/Engenio, DDN, TexasMemorySystem Ramsan (IBM), Nimbus, Violin Memory, StreamScale
The last three may be derived from SCST base target.

Quoted 4 lines

I only know SCST, Solaris COMSTAR and that broken LIO stuff. Does SCST still not support to set the pkey?

Yes, I think so

Quoted 3 lines

Why should we check the dgid?

If you want to have multiple connections/qps to same target, but as I said above, it's a gray area.

Quoted 2 lines

Doesn't make any sense to me to connect both target ports to the same local port.

What if a target always expose single consistent and unique SRP port with tuple <id_ext, ioc_guid>, the ioc_guid part is not derived from any of its local HCA's GUID, then you can connect to this target thru different HCA ports (different dgid) as different paths to same target.

Quoted 3 lines

If you do so, the multipath-tools will crash. Note: This function is called per local SRP port. Perhaps, a note should be added to that function that it only has to be called per local SRP port.

Quoted 23 lines · Hannes Reinecke

Quoted 6 lines · Bart Van Assche

I agree that the value of fast_io_fail_tmo should be kept small. Although as you explained changing the SCSI device state into SDEV_BLOCK doesn't help for I/O that has already been queued on a failed path, I think it's still useful for I/O that is queued after the fast_io_fail timer has been started and before that timer has expired.

Why, but of course.

The typical scenario would be:
-> detect link-loss
-> call scsi_block_request()
-> start dev_loss_tmo and fast_io_fail_tmo

-> When fast_io_fail_tmo triggers:

    -> Abort all outstanding requests

-> When dev_loss_tmo triggers:

    -> Abort all outstanding requests
    -> Remove/disable the I_T nexus
    -> call scsi_unblock_request()

Quoted 3 lines

However, if and whether multipath detects SDEV_BLOCK doesn't guarantee a fast failover; in fact is was only added rather recently as it's not a big win in most cases.

Even if setting the state SDEV_BLOCK doesn't help much with improving failover time, it still has the advantage over using scsi_block_requests() that it can be overridden by a user via sysfs.

Quoted 29 lines · Vu Pham

Quoted 29 lines · Vu Pham

Quoted 15 lines

Quoted 15 lines

+/**
+ * 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.
+ */

(quoted diff) +6 -0

+int srp_tmo_valid(int fast_io_fail_tmo, int dev_loss_tmo)
+{
+    return (fast_io_fail_tmo < 0 && 1 <= dev_loss_tmo &&
+        dev_loss_tmo <= SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+        || (0 <= fast_io_fail_tmo &&
+            (dev_loss_tmo < 0 ||

Quoted 8 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+             (fast_io_fail_tmo < dev_loss_tmo &&
+              dev_loss_tmo < LONG_MAX / HZ))) ? 0 : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(srp_tmo_valid);

fast_io_fail_tmo is off, one cannot turn off dev_loss_tmo with negative value
dev_loss_tmo is off, one cannot turn off fast_io_fail_tmo with negative value

OK, will update the documentation such that it correctly refers to "off" instead of a negative value and I will also mention that dev_loss_tmo can now be disabled.

Quoted 3 lines

It's not only the documentation but also the code logic, you cannot turn dev_loss_tmo off if fast_io_fail_tmo already turned off and vice versa with the return statement above.

Does this mean that you think it would be useful to disable both the fast_io_fail and the dev_loss mechanisms, and hence rely on the user to remove remote ports that have disappeared and on the SCSI command timeout to detect path failures ?

I'll start testing this to see whether that combination does not trigger any adverse behavior.

Quoted 11 lines

Quoted 11 lines

Quoted 2 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to do extra block with scsi_block_requests()

Please keep in mind that srp_reconnect_rport() can be called from two different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in this function.

Quoted 6 lines

Yes, srp_reconnect_rport() can be called from two contexts; however, it deals with same rport & rport's state.
I'm thinking something like this:

    if (rport->state != SRP_RPORT_BLOCKED) {
     scsi_block_requests(shost);

Sorry but I'm afraid that that approach would still allow the user to unblock one or more SCSI devices via sysfs during the i->f->reconnect(rport) call, something we do not want.

Quoted 3 lines

I think that we can use only the pair scsi_block_requests()/scsi_unblock_requests() unless the advantage of multipathd recognizing the SDEV_BLOCK is noticeable.

I think the advantage of multipathd recognizing the SDEV_BLOCK state before the fast_io_fail_tmo timer has expired is important. Multipathd does not queue I/O to paths that are in the SDEV_BLOCK state so setting that state helps I/O to fail over more quickly, especially for large values of fast_io_fail_tmo.

Hope this helps,

Bart.

Quoted 20 lines · Bart Van Assche

Quoted 18 lines · Vu Pham

Quoted 12 lines · Vu Pham

Quoted 2 lines

If rport's state is already SRP_RPORT_BLOCKED, I don't think we need to do extra block with scsi_block_requests()

Please keep in mind that srp_reconnect_rport() can be called from two different contexts: that function can not only be called from inside the SRP transport layer but also from inside the SCSI error handler (see also the srp_reset_device() modifications in a later patch in this series). If this function is invoked from the context of the SCSI error handler the chance is high that the SCSI device will have another state than SDEV_BLOCK. Hence the scsi_block_requests() call in this function.

Yes, srp_reconnect_rport() can be called from two contexts; however, it deals with same rport & rport's state.
I'm thinking something like this:

    if (rport->state != SRP_RPORT_BLOCKED) {
     scsi_block_requests(shost);

Quoted 4 lines

Sorry but I'm afraid that that approach would still allow the user to unblock one or more SCSI devices via sysfs during the i->f->reconnect(rport) call, something we do not want.

I don't think that user can unblock scsi device(s) via sysfs if you use scsi_block_requests(shost) in srp_start_tl_fail_timers().

Review

Quoted 12 lines · Bart Van Assche

(quoted diff) +12 -0

+		/*
+		 * It can occur that after fast_io_fail_tmo expired and before
+		 * dev_loss_tmo expired that the SCSI error handler has
+		 * offlined one or more devices. scsi_target_unblock() doesn't
+		 * change the state of these devices into running, so do that
+		 * explicitly.
+		 */
+		spin_lock_irq(shost->host_lock);
+		__shost_for_each_device(sdev, shost)
+			if (sdev->sdev_state == SDEV_OFFLINE)
+				sdev->sdev_state = SDEV_RUNNING;
+		spin_unlock_irq(shost->host_lock);

Is it possible for this to race with scsi_eh_offline_sdevs? Can it be looping over cmds offlining devices while this is looping over devices onlining them?

It seems this can also happen for all transports/drivers. Maybe a a scsi eh/lib helper function that syncrhonizes with the scsi eh completion would be better.

If the add_one callback fails during driver load no resources are allocated so there isn't a need to release any resources. Trying to clean the resource may lead to the following kernel panic:

Not sure how I missed that ... I will drop this version and replace it with the proper fix.

The SCSI error handler assumes that the transport layer is operational if an eh_abort_handler() returns SUCCESS. Hence let srp_abort() only return SUCCESS if sending the ABORT TASK task management function succeeded. This patch avoids that the SCSI error handler skips the srp_reset_host() call after a transport layer error.

The SRP initiator implements host reset by reconnecting to the SRP target. That means that communication with the target is possible as soon as host reset finished. Hence skip the host settle delay.

An SRP target is required to maintain a single connection between initiator and target. This means that if the 'add_target' attribute is used to create a second connection to a target that the first connection will be logged out and that the SCSI error handler will kick in. The SCSI error handler will cause the SRP initiator to reconnect, which will cause I/O over the second connection to fail. Avoid such ping-pong behavior by disabling relogins. Note: if reconnecting manually is necessary, that is possible by deleting and recreating an rport via sysfs.

From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>

Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. The transport layer timeout in nanoseconds is computed as follows from the retry count:

rc_timeout = rc_retry_count * 4 * 4096 * (1 << qp->timeout)

The default value for tl_retry_count is changed from 7 into 2.

Several InfiniBand HCA's allow to configure the completion vector per queue pair. This allows to spread the workload created by IB completion interrupts over multiple MSI-X vectors and hence over multiple CPU cores. In other words, configuring the completion vector properly not only allows to reduce latency on an initiator connected to multiple SRP targets but also allows to improve throughput.

Quoted 3 lines · Bart Van Assche

If the add_one callback fails during driver load no resources are allocated so there isn't a need to release any resources. Trying to clean the resource may lead to the following kernel panic:

Acked-by David Dillow <[email protected]>

Quoted 2 lines · Bart Van Assche

Not sure how I missed that ... I will drop this version and replace it with the proper fix.

I expect the merge window will be opening soon; I should be able to make time on Sunday to review anything you post by then -- do you think the next version will be out then?

I'll keep pushing through the series in the meantime.

Quoted 9 lines · Bart Van Assche

An SRP target is required to maintain a single connection between initiator and target. This means that if the 'add_target' attribute is used to create a second connection to a target that the first connection will be logged out and that the SCSI error handler will kick in. The SCSI error handler will cause the SRP initiator to reconnect, which will cause I/O over the second connection to fail. Avoid such ping-pong behavior by disabling relogins. Note: if reconnecting manually is necessary, that is possible by deleting and recreating an rport via sysfs.

Will wait for the second version of this as well.

Was an agreement reached on the pkey issue?
I also would rather we mimic the current messages rather than regurgitate the user's login string back at them.

Quoted 6 lines · David Dillow

Quoted 3 lines · Bart Van Assche

If the add_one callback fails during driver load no resources are allocated so there isn't a need to release any resources. Trying to clean the resource may lead to the following kernel panic:

Acked-by David Dillow <[email protected]>

Thanks, I've queued up the 1, 3, and 4/14 patches that Dave acked so far.

Quoted 7 lines · Bart Van Assche

Several InfiniBand HCA's allow to configure the completion vector per queue pair. This allows to spread the workload created by IB completion interrupts over multiple MSI-X vectors and hence over multiple CPU cores. In other words, configuring the completion vector properly not only allows to reduce latency on an initiator connected to multiple SRP targets but also allows to improve throughput.

Shouldn't there be some documentation about how to set this? What happens when you try to set it on an HCA that doesn't support this how does the user know what's wrong?

Quoted 9 lines · Bart Van Assche

From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>

Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. The transport layer timeout in nanoseconds is computed as follows from the retry count:

rc_timeout = rc_retry_count * 4 * 4096 * (1 << qp->timeout)

The default value for tl_retry_count is changed from 7 into 2.

I'm not sure you should change the default, but I've not thought about it much.

It should probably also have a default set via module parameter similar to the indirect_command_descriptors, etc. Unless srp_daemon is patched, this won't be usable without that. I'm not tied to this position, though -- we should be moving in the direction of more control via the login string.

Quoted 3 lines · David Dillow

Shouldn't there be some documentation about how to set this? What happens when you try to set it on an HCA that doesn't support this how does the user know what's wrong?

Hello Dave,

HCA drivers that support only one completion vector ignore the comp_vector parameter. See e.g. c2_create_cq() in drivers/infiniband/hw/amso1100/c2_provider.c for an example. The third argument of that function, called "vector", is ignored.

What will happen if an out-of-range value is chosen for this parameter depends on the HCA driver. This is how the mlx4 driver handles the comp_vector parameter:

	vector = dev->eq_table[vector % ibdev->num_comp_vectors];

So an out-of-range value for comp_vector won't cause a failure. And if ever a HCA driver would be added to the kernel tree that causes create_cq() to fail if the comp_vector parameter is out of range then the ib_srp driver will propagate the error code returned by ib_create_cq() to the process that wrote into the add_target sysfs attribute.

The procedure for choosing a value for this parameter is as follows:
* Look up the interrupt numbers that have been assigned to each HCA completion vector, e.g. via grep mlx4-ib- /proc/interrupts.
* Decide which CPU to bind each interrupt to and update /proc/irq/$irq/smp_affinity.
* Decide which CPU and completion vector should be used for processing SRP completions and specify that completion vector number in the SRP login string.

To me this looks like a standard procedure that applies to any HCA that supports multiple MSI-X vectors ?

Quoted 11 lines · David Dillow

Quoted 9 lines · Bart Van Assche

From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>

Allow the InfiniBand RC retry count to be configured by the user as an option in the target login string. The transport layer timeout in nanoseconds is computed as follows from the retry count:

rc_timeout = rc_retry_count * 4 * 4096 * (1 << qp->timeout)

The default value for tl_retry_count is changed from 7 into 2.

Quoted 8 lines

I'm not sure you should change the default, but I've not thought about it much.

It should probably also have a default set via module parameter similar to the indirect_command_descriptors, etc. Unless srp_daemon is patched, this won't be usable without that. I'm not tied to this position, though -- we should be moving in the direction of more control via the login string.

Hello Dave,

Are you perhaps aware of any IB networks where it is necessary to set the RC retry count above 2 ?

Anyway, I can prepare an srp_daemon patch
that makes it easy to configure this parameter if you want.

Quoted 3 lines · Bart Van Assche

On 06/27/13 23:24, David Dillow wrote:
To me this looks like a standard procedure that applies to any HCA that supports multiple MSI-X vectors ?

To you and I, sure, this is relatively standard. And now that google has it, newcomers will be able to find it. However, this changes the login string and so Documentation/ABI/stable/sysfs-driver-ib_srp needs to be updated, with at least minimal text.

This does need to be documented in Documentation/ABI/stable/sysfs-driver-ib_srp, though.