IB/srp patches for kernel v4.2

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.13

---
  drivers/scsi/scsi_transport_srp.c | 52 ++++++++++++++++++++++-----------------
  1 file changed, 29 insertions(+), 23 deletions(-)

drivers/scsi/scsi_transport_srp.c +28 -21

index ae45bd9..6ce1c48 100644
@@ -396,6 +396,34 @@ static void srp_reconnect_work(struct work_struct *work)
  	}
  }
 
+/**
+ * scsi_request_fn_active() - number of kernel threads inside scsi_request_fn()
+ * @shost: SCSI host for which to count the number of scsi_request_fn() callers.
+ */
+static int scsi_request_fn_active(struct Scsi_Host *shost)
+{
+	struct scsi_device *sdev;
+	struct request_queue *q;
+	int request_fn_active = 0;
+
+	shost_for_each_device(sdev, shost) {
+		q = sdev->request_queue;
+
+		spin_lock_irq(q->queue_lock);
+		request_fn_active += q->request_fn_active;
+		spin_unlock_irq(q->queue_lock);
+	}
+
+	return request_fn_active;
+}
+
+/* Wait until ongoing shost->hostt->queuecommand() calls have finished. */
+static void srp_wait_for_queuecommand(struct Scsi_Host *shost)
+{
+	while (scsi_request_fn_active(shost))
+		msleep(20);
+}
+
  static void __rport_fail_io_fast(struct srp_rport *rport)
  {
  	struct Scsi_Host *shost = rport_to_shost(rport);

@@ -504,27 +532,6 @@ void srp_start_tl_fail_timers(struct srp_rport *rport)
  EXPORT_SYMBOL(srp_start_tl_fail_timers);
 
  /**
- * scsi_request_fn_active() - number of kernel threads inside scsi_request_fn()
- * @shost: SCSI host for which to count the number of scsi_request_fn() callers.
- */
-static int scsi_request_fn_active(struct Scsi_Host *shost)
-{
-	struct scsi_device *sdev;
-	struct request_queue *q;
-	int request_fn_active = 0;
-
-	shost_for_each_device(sdev, shost) {
-		q = sdev->request_queue;
-
-		spin_lock_irq(q->queue_lock);
-		request_fn_active += q->request_fn_active;
-		spin_unlock_irq(q->queue_lock);
-	}
-
-	return request_fn_active;
-}
-
-/**
   * srp_reconnect_rport() - reconnect to an SRP target port
   * @rport: SRP target port.
   *

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

Quoted 5 lines · Bart Van Assche

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

Can we take a step back here?

This isn;t something driver should be doing, so we really should take care of this in the SCSI midlayer.

Especially given that we don't maintain request_fn_active for blk-mq.

Quoted 12 lines · Christoph Hellwig

Quoted 5 lines · Bart Van Assche

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

Can we take a step back here?

This isn;t something driver should be doing, so we really should take care of this in the SCSI midlayer.

Quoted 1 line

Especially given that we don't maintain request_fn_active for blk-mq.

Hello Christoph,

How about the following:
* Modify scsi_target_block() and scsi_target_unblock(..., SDEV_TRANSPORT_OFFLINE) such that these functions wait until active queuecommand() calls have finished.

Quoted 17 lines · Bart Van Assche

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Bart Van Assche

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

Quoted 6 lines

Can we take a step back here?

This isn;t something driver should be doing, so we really should take care of this in the SCSI midlayer.

Especially given that we don't maintain request_fn_active for blk-mq.

Hello Christoph,

Quoted 4 lines

How about the following:
* Modify scsi_target_block() and scsi_target_unblock(..., SDEV_TRANSPORT_OFFLINE) such that these functions wait until active queuecommand() calls have finished.

Won't this make scsi_target_unblock a sleeping function? It might not fit all the contexts scsi_target_unblock is called.

Quoted 18 lines · Sagi Grimberg

Quoted 17 lines · Bart Van Assche

Quoted 7 lines · Christoph Hellwig

Quoted 5 lines · Bart Van Assche

Introduce the helper function srp_wait_for_queuecommand(). Move the definition of scsi_request_fn_active(). This patch does not change any functionality. A second call to scsi_wait_for_queuecommand() will be introduced in the next patch.

Quoted 6 lines

Can we take a step back here?

This isn;t something driver should be doing, so we really should take care of this in the SCSI midlayer.

Especially given that we don't maintain request_fn_active for blk-mq.

Hello Christoph,

Quoted 7 lines

Quoted 4 lines

How about the following:
* Modify scsi_target_block() and scsi_target_unblock(..., SDEV_TRANSPORT_OFFLINE) such that these functions wait until active queuecommand() calls have finished.

Won't this make scsi_target_unblock a sleeping function? It might not fit all the contexts scsi_target_unblock is called.

The only callers in upstream code of scsi_target_block() and scsi_target_unblock() I am aware of are the FC, iSCSI and SRP transport layers and the drivers that use these transport layers. As far as I can see both functions are always called from thread context and without holding any spinlocks. A possible alternative to what I proposed in my previous e-mail could be to provide a new function that waits for active queuecommand() calls and that has to be called explicitly.

The only callers in upstream code of scsi_target_block() and scsi_target_unblock() I am aware of are the FC, iSCSI and SRP transport layers and the drivers that use these transport layers. As far as I can see both functions are always called from thread context and without holding any spinlocks. A possible alternative to what I proposed in my previous e-mail could be to provide a new function that waits for active queuecommand() calls and that has to be called explicitly.

Quoted 7 lines · Bart Van Assche

The only callers in upstream code of scsi_target_block() and scsi_target_unblock() I am aware of are the FC, iSCSI and SRP transport layers and the drivers that use these transport layers. As far as I can see both functions are always called from thread context and without holding any spinlocks. A possible alternative to what I proposed in my previous e-mail could be to provide a new function that waits for active queuecommand() calls and that has to be called explicitly.

A separate helper sounds fine for now, although I suspect we'll eventually migrate the call to it into scsi_target_block().

Hi Bart,

I've looked at this and didn't really like the unconditional hctx lock in the blk-mq path which might have nasty effects when just using a single hctx.

So I'm taking another step back and try to understand what you're doign here.

Let me try to recreate the issue:

- we get a ->host_reset call for the SRP initiator, which then calls srp_reconnect_rport, at which point we still have outstanding commands on the wire, and we still allow concurrent I/O submission
- srp_reconnect_rport then blocks new I/O, and tries to drain the peding requeuest from ->queuecommand. It then calls into srp_rport_reconnect, which after some work also clears out all commands on the wire and the reconnects

Maybe it's time to move to what Hannes suggested in events.linuxfoundation.org/sites/events/files/slides/SCSI-EH.pdf slides 56+ at least for SRP as a start, that is:

- once escalating to a LUN reset fail all commands for the LUN and block the the LUN for I/O and send a TMF abort
- once scalatating to the host reset fail all I/O for the host and block the host (all LUNs) for I/O, and only then call the host reset action (reconnect in the SRP case) (or rather replace the current RP host reset with the I_T Nexus reset suggested by Hannes)

The advantage is that we can do the full drain much more easily than just waiting for command leaving ->queuecommnd. The other advantage is that we can implement this with fairly small changes in the scsi_error.c code trggered off a host or transport template flag, without touching code in the block layer while at the same time significantly simplifying the transport layer and drivers.

Hello Christoph,

There are multiple events that can cause the SRP initiator driver to initiate a reconnect:
1. The SCSI core invoking eh_host_reset_handler().
2. An error reported by the IB HCA or by the IB core, e.g. an RDMA transmit timeout or a transport layer disconnect reported by the IB/CM.

Quoted 6 lines · Bart Van Assche

There are multiple events that can cause the SRP initiator driver to initiate a reconnect:
1. The SCSI core invoking eh_host_reset_handler().
2. An error reported by the IB HCA or by the IB core, e.g. an RDMA transmit timeout or a transport layer disconnect reported by the IB/CM.

Right, I missed the srp_reconnect_work case. But even with that I think what I wrote above still stands. srp_reconnect_work in that case would just directly trigger the abort all commands and reconnect operation.

The main point I was trying to make is that instead of having a sequence of:

1) block new queuecommand instances
2) flush out pending queuecommand instances
3) do part of the disconnect
4) fail all in-flight commands
5) reconnect

we should aim for:

1) block new queuecommand instances
2) fail all in-flight commands
3) disconnect and reconnect

to avoid the need to keep track of pending queuecommand instances, and instead re-use the existing infrastructure to fail all in-flight commands, which we have the infrastructure for, and which we need to do anyway.

Your proposal absolutely makes sense to me but unfortunately I do not have the time available now to implement it. Would it be acceptable if I rework scsi_wait_for_queuecommand() such that per-CPU counters are introduced in blk-mq instead of one counter per hctx ?

Quoted 8 lines · Bart Van Assche

Hello Christoph,

There are multiple events that can cause the SRP initiator driver to initiate a reconnect:
1. The SCSI core invoking eh_host_reset_handler().
2. An error reported by the IB HCA or by the IB core, e.g. an RDMA transmit timeout or a transport layer disconnect reported by the IB/CM.

Right, I missed the srp_reconnect_work case. But even with that I think what I wrote above still stands. srp_reconnect_work in that case would just directly trigger the abort all commands and reconnect operation.

The main point I was trying to make is that instead of having a sequence of:

1) block new queuecommand instances
2) flush out pending queuecommand instances
3) do part of the disconnect
4) fail all in-flight commands
5) reconnect

we should aim for:

1) block new queuecommand instances
2) fail all in-flight commands
3) disconnect and reconnect

to avoid the need to keep track of pending queuecommand instances, and instead re-use the existing infrastructure to fail all in-flight commands, which we have the infrastructure for, and which we need to do anyway.

What I'm wondering about is whether it will be possible with the above approach to trigger path failover before (2 * SCSI timeout) has expired ? Starting SCSI error handling immediately after the block layer has reported the first SCSI timeout is only safe if all ongoing SCSI commands are canceled in some way. Is this what the function blk_abort_request() is intended for ? As far as I can see invoking that function or any function with a similar purpose is only safe after the queuecommand() callback function has finished. However, blk_mq_run_hw_queue() invokes mq_ops->queue_rq() without holding any lock. So it's not clear to me how to safely cancel ongoing blk-mq requests without waiting until these have timed out. I hope that this means that overlooked something ?

Quoted 11 lines · Bart Van Assche

What I'm wondering about is whether it will be possible with the above approach to trigger path failover before (2 * SCSI timeout) has expired ? Starting SCSI error handling immediately after the block layer has reported the first SCSI timeout is only safe if all ongoing SCSI commands are canceled in some way. Is this what the function blk_abort_request() is intended for ? As far as I can see invoking that function or any function with a similar purpose is only safe after the queuecommand() callback function has finished. However, blk_mq_run_hw_queue() invokes mq_ops->queue_rq() without holding any lock. So it's not clear to me how to safely cancel ongoing blk-mq requests without waiting until these have timed out. I hope that this means that overlooked something ?

For the blk-mq case invoking it earlier should be fine - the REQ_ATOM_STARTED and REQ_ATOM_COMPLETE bit ops are specifily designed so that calling the timeout handler on any request is fine. I'm not sure about the !blk-mq case, though.

Thanks for the feedback. However, I'm still wondering what will happen if blk_abort_request() causes e.g. blk_rq_unmap_user() or blk_update_request() to be called while mq_ops->queue_rq() or q->request_fn() is still in progress ?

Avoid that srp_terminate_io() can get invoked while srp_queuecommand() is in progress. This patch avoids that an I/O timeout can trigger the following kernel warning:

WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:1447 srp_terminate_io+0xef/0x100 [ib_srp]()
Call Trace:
  [<ffffffff814c65a2>] dump_stack+0x4e/0x68
  [<ffffffff81051f71>] warn_slowpath_common+0x81/0xa0
  [<ffffffff8105204a>] warn_slowpath_null+0x1a/0x20
  [<ffffffffa075f51f>] srp_terminate_io+0xef/0x100 [ib_srp]
  [<ffffffffa07495da>] __rport_fail_io_fast+0xba/0xc0 [scsi_transport_srp]
  [<ffffffffa0749a90>] rport_fast_io_fail_timedout+0xe0/0xf0 [scsi_transport_srp]
  [<ffffffff8106e09b>] process_one_work+0x1db/0x780
  [<ffffffff8106e75b>] worker_thread+0x11b/0x450
  [<ffffffff81073c64>] kthread+0xe4/0x100
  [<ffffffff814cf26c>] ret_from_fork+0x7c/0xb0

See also patch "scsi_transport_srp: Add transport layer error handling" (commit ID 29c17324803c).

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.13

---
  drivers/scsi/scsi_transport_srp.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +0 -0

index 6ce1c48..4a44337 100644

Quoted 40 lines · Bart Van Assche

Avoid that srp_terminate_io() can get invoked while srp_queuecommand() is in progress. This patch avoids that an I/O timeout can trigger the following kernel warning:

WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:1447
srp_terminate_io+0xef/0x100 [ib_srp]()
Call Trace:
  [<ffffffff814c65a2>] dump_stack+0x4e/0x68
  [<ffffffff81051f71>] warn_slowpath_common+0x81/0xa0
  [<ffffffff8105204a>] warn_slowpath_null+0x1a/0x20
  [<ffffffffa075f51f>] srp_terminate_io+0xef/0x100 [ib_srp]
  [<ffffffffa07495da>] __rport_fail_io_fast+0xba/0xc0
[scsi_transport_srp]
  [<ffffffffa0749a90>] rport_fast_io_fail_timedout+0xe0/0xf0
[scsi_transport_srp]
  [<ffffffff8106e09b>] process_one_work+0x1db/0x780
  [<ffffffff8106e75b>] worker_thread+0x11b/0x450
  [<ffffffff81073c64>] kthread+0xe4/0x100
  [<ffffffff814cf26c>] ret_from_fork+0x7c/0xb0

See also patch "scsi_transport_srp: Add transport layer error handling" (commit ID 29c17324803c).

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.13

---
  drivers/scsi/scsi_transport_srp.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +0 -0

index 6ce1c48..4a44337 100644
@@ -437,8 +437,10 @@ static void __rport_fail_io_fast(struct srp_rport
 *rport)
 

drivers/scsi/scsi_transport_srp.c static void __rport_fail_io_fast(struct srp_rport *rport) · line 443

Quoted 8 lines

drivers/scsi/scsi_transport_srp.c +0 -0

      /* Involve the LLD if possible to terminate all I/O on the

rport. */

      i = to_srp_internal(shost->transportt);

drivers/scsi/scsi_transport_srp.c +3 -1

-    if (i->f->terminate_rport_io)
+    if (i->f->terminate_rport_io) {
+        srp_wait_for_queuecommand(shost);
          i->f->terminate_rport_io(rport);
+    }

Why not just terminate the inflight IO before unblocking the target?

Fix a scsi_get_host() / scsi_host_put() imbalance in the error path of srp_create_target(). See also patch "IB/srp: Avoid that I/O hangs due to a cable pull during LUN scanning" (commit ID 34aa654ecb8e).

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.19

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

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

index 918814c..5ce6cfd 100644
@@ -3172,11 +3172,11 @@ static ssize_t srp_create_target(struct device *dev,
 
  	ret = srp_parse_options(buf, target);
  	if (ret)
-		goto err;
+		goto out;
 
  	ret = scsi_init_shared_tag_map(target_host, target_host->can_queue);
  	if (ret)
-		goto err;
+		goto out;
 
  	target->req_ring_size = target->queue_size - SRP_TSK_MGMT_SQ_SIZE;
 

@@ -3187,7 +3187,7 @@ static ssize_t srp_create_target(struct device *dev,
  			     be64_to_cpu(target->ioc_guid),
  			     be64_to_cpu(target->initiator_ext));
  		ret = -EEXIST;
-		goto err;
+		goto out;
  	}
 
  	if (!srp_dev->has_fmr && !srp_dev->has_fr && !target->allow_ext_sg &&

@@ -3208,7 +3208,7 @@ static ssize_t srp_create_target(struct device *dev,
  	spin_lock_init(&target->lock);
  	ret = ib_query_gid(ibdev, host->port, 0, &target->sgid);
  	if (ret)
-		goto err;
+		goto out;
 
  	ret = -ENOMEM;
  	target->ch_count = max_t(unsigned, num_online_nodes(),

@@ -3219,7 +3219,7 @@ static ssize_t srp_create_target(struct device *dev,
  	target->ch = kcalloc(target->ch_count, sizeof(*target->ch),
  			     GFP_KERNEL);
  	if (!target->ch)
-		goto err;
+		goto out;
 
  	node_idx = 0;
  	for_each_online_node(node) {

In large networks the subnet timeout may have been set to a value above 20. In small networks it can be safe to reduce the subnet timeout below 20. The CM timeout should be proportional to the subnet timeout. Hence make the CM timeout dependent on the subnet timeout. Since the default subnet timeout used by OpenSM is 18 this patch does not change the CM timeout if the default OpenSM subnet timeout is used.

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

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index 27d3a64..a18a2ae 100644
@@ -704,6 +704,19 @@ static int srp_lookup_path(struct srp_rdma_ch *ch)
  	return ch->status;
  }
 
+static u8 srp_get_subnet_timeout(struct srp_host *host)
+{
+	struct ib_port_attr attr;
+	int ret;
+	u8 subnet_timeout = 18;
+
+	ret = ib_query_port(host->srp_dev->dev, host->port, &attr);
+	if (ret == 0)
+		subnet_timeout = attr.subnet_timeout;
+
+	return subnet_timeout;
+}
+
  static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
  {
  	struct srp_target_port *target = ch->target;

Quoted 46 lines · Bart Van Assche

In large networks the subnet timeout may have been set to a value above 20. In small networks it can be safe to reduce the subnet timeout below 20. The CM timeout should be proportional to the subnet timeout. Hence make the CM timeout dependent on the subnet timeout. Since the default subnet timeout used by OpenSM is 18 this patch does not change the CM timeout if the default OpenSM subnet timeout is used.

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index 27d3a64..a18a2ae 100644
@@ -704,6 +704,19 @@ static int srp_lookup_path(struct srp_rdma_ch *ch)
   	return ch->status;
   }
 
+static u8 srp_get_subnet_timeout(struct srp_host *host)
+{
+	struct ib_port_attr attr;
+	int ret;
+	u8 subnet_timeout = 18;
+
+	ret = ib_query_port(host->srp_dev->dev, host->port, &attr);
+	if (ret == 0)
+		subnet_timeout = attr.subnet_timeout;
+
+	return subnet_timeout;
+}
+
   static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
   {
   	struct srp_target_port *target = ch->target;

@@ -711,8 +724,11 @@ static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
   		struct ib_cm_req_param param;
   		struct srp_login_req   priv;
   	} *req = NULL;
+	u8 subnet_timeout;
   	int status;
 

drivers/infiniband/ulp/srp/ib_srp.c static int srp_send_req(struct srp_rdma_ch *ch, bool multich) · line 730

Quoted 1 line

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

+	subnet_timeout = srp_get_subnet_timeout(target->srp_host);

Is this really something that srp initiator should look at? I'd say that this needs to be a ib_cm code in case the input timeout values are not set (e.g. zero).

Reception of a DREQ message only causes the state of a single channel to change. Modify the SRP initiator such that channel and target connection state are tracked separately. This patch avoids that following false positive warning can be reported by srp_destroy_qp():

WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:617 srp_destroy_qp+0xa6/0x120 [ib_srp]()
Call Trace:
[<ffffffff8106e10f>] warn_slowpath_common+0x7f/0xc0
[<ffffffff8106e16a>] warn_slowpath_null+0x1a/0x20
[<ffffffffa0440226>] srp_destroy_qp+0xa6/0x120 [ib_srp]
[<ffffffffa0440322>] srp_free_ch_ib+0x82/0x1e0 [ib_srp]
[<ffffffffa044408b>] srp_create_target+0x7ab/0x998 [ib_srp]
[<ffffffff81346f60>] dev_attr_store+0x20/0x30
[<ffffffff811dd90f>] sysfs_write_file+0xef/0x170
[<ffffffff8116d248>] vfs_write+0xc8/0x190
[<ffffffff8116d411>] sys_write+0x51/0x90

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.19

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

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

index 5ce6cfd..0eb07d3 100644
@@ -465,14 +465,13 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
   */
  static void srp_destroy_qp(struct srp_rdma_ch *ch)
  {
-	struct srp_target_port *target = ch->target;
  	static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
  	static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
  	struct ib_recv_wr *bad_wr;
  	int ret;
 
  	/* Destroying a QP and reusing ch->done is only safe if not connected */
-	WARN_ON_ONCE(target->connected);
+	WARN_ON_ONCE(ch->connected);
 
  	ret = ib_modify_qp(ch->qp, &attr, IB_QP_STATE);
  	WARN_ONCE(ret, "ib_cm_init_qp_attr() returned %d\n", ret);

@@ -836,6 +835,7 @@ static void srp_disconnect_target(struct srp_target_port *target)
 
  		for (i = 0; i < target->ch_count; i++) {
  			ch = &target->ch[i];
+			ch->connected = false;
  			if (ch->cm_id && ib_send_cm_dreq(ch->cm_id, NULL, 0)) {
  				shost_printk(KERN_DEBUG, target->scsi_host,
  					     PFX "Sending CM DREQ failed\n");

@@ -1017,6 +1017,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
  		switch (ch->status) {
  		case 0:
  			srp_change_conn_state(target, true);
+			ch->connected = true;
  			return 0;
 
  		case SRP_PORT_REDIRECT:

drivers/infiniband/ulp/srp/ib_srp.c static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event) · line 2371

Quoted 2 lines · Bart Van Assche

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

-        srp_change_conn_state(target, false);
+        ch->connected = false;

Shouldn't this be protected by the channel lock (like the target)?

drivers/infiniband/ulp/srp/ib_srp.h struct srp_rdma_ch { · line 173

Quoted 5 lines

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

@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
      struct completion    tsk_mgmt_done;
      u8            tsk_mgmt_status;
+    bool            connected;

I generally agree with this flag, but I'm a bit confused about the semantics of two connected flags? Also, we check the target connected flag on TMFs, although we are executing it on a channel (should we check both?)

I'd say keep only the channel connected flag, the target logic needs to be mandated by the state.

Quoted 5 lines · Sagi Grimberg

drivers/infiniband/ulp/srp/ib_srp.c static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event) · line 2371

Quoted 2 lines · Bart Van Assche

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

-        srp_change_conn_state(target, false);
+        ch->connected = false;

Shouldn't this be protected by the channel lock (like the target)?

On all CPU architectures I'm familiar with changes of boolean variables are atomic so I think modifying that variable without locking is fine.

Quoted 11 lines

drivers/infiniband/ulp/srp/ib_srp.h struct srp_rdma_ch { · line 173

Quoted 5 lines

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

@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
      struct completion    tsk_mgmt_done;
      u8            tsk_mgmt_status;
+    bool            connected;

I generally agree with this flag, but I'm a bit confused about the semantics of two connected flags? Also, we check the target connected flag on TMFs, although we are executing it on a channel (should we check both?)

Quoted 2 lines

I'd say keep only the channel connected flag, the target logic needs to be mandated by the state.

I think we need both flags. The ch->connected flag is used only in srp_destroy_qp() to verify whether a channel has been disconnected before it is destroyed. The target->connected flag provides an easy way to test the connected state in e.g. srp_disconnect_target().

And if it
is attempted to send a task management function over a channel that has just been disconnected that should be fine since any channel disconnect causes tl_err_work to be queued. That last action will sooner or later result in a reconnect.

Quoted 16 lines · Bart Van Assche

Quoted 14 lines · Sagi Grimberg

drivers/infiniband/ulp/srp/ib_srp.h struct srp_rdma_ch { · line 173

Quoted 5 lines · Bart Van Assche

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

@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
      struct completion    tsk_mgmt_done;
      u8            tsk_mgmt_status;
+    bool            connected;

I generally agree with this flag, but I'm a bit confused about the semantics of two connected flags? Also, we check the target connected flag on TMFs, although we are executing it on a channel (should we check both?)

I'd say keep only the channel connected flag, the target logic needs to be mandated by the state.

Quoted 4 lines

I think we need both flags. The ch->connected flag is used only in srp_destroy_qp() to verify whether a channel has been disconnected before it is destroyed. The target->connected flag provides an easy way to test the connected state in e.g. srp_disconnect_target().

We can just as easily check rport state. rport state is modified before target-connected. I still think one is redundant.

Quoted 5 lines

And if it
is attempted to send a task management function over a channel that has just been disconnected that should be fine since any channel disconnect causes tl_err_work to be queued. That last action will sooner or later result in a reconnect.

Will it be fine to queue commands when the channel is in reconnecting stage?

Quoted 22 lines · Sagi Grimberg

Quoted 20 lines · Bart Van Assche

Quoted 12 lines · Sagi Grimberg

Review struct srp_rdma_ch { · line 173

Quoted 5 lines · Bart Van Assche

(quoted diff) +1 -0

@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
      struct completion    tsk_mgmt_done;
      u8            tsk_mgmt_status;
+    bool            connected;

I generally agree with this flag, but I'm a bit confused about the semantics of two connected flags? Also, we check the target connected flag on TMFs, although we are executing it on a channel (should we check both?)

Quoted 2 lines

I'd say keep only the channel connected flag, the target logic needs to be mandated by the state.

I think we need both flags. The ch->connected flag is used only in srp_destroy_qp() to verify whether a channel has been disconnected before it is destroyed. The target->connected flag provides an easy way to test the connected state in e.g. srp_disconnect_target().

Quoted 2 lines

We can just as easily check rport state. rport state is modified before target-connected. I still think one is redundant.

Sorry but I disagree. If e.g. both the fast_io_fail and dev_loss_tmo timers have been disabled then the rport state is SRP_RPORT_RUNNING all the time. At least in that case it is not possible to derive the target->connected state from the rport state.

Quoted 23 lines · Bart Van Assche

Quoted 22 lines · Sagi Grimberg

Quoted 20 lines · Bart Van Assche

Quoted 12 lines · Sagi Grimberg

Review struct srp_rdma_ch { · line 173

Quoted 5 lines · Bart Van Assche

(quoted diff) +1 -0

@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
       struct completion    tsk_mgmt_done;
       u8            tsk_mgmt_status;
+    bool            connected;

I generally agree with this flag, but I'm a bit confused about the semantics of two connected flags? Also, we check the target connected flag on TMFs, although we are executing it on a channel (should we check both?)

Quoted 2 lines

I'd say keep only the channel connected flag, the target logic needs to be mandated by the state.

I think we need both flags. The ch->connected flag is used only in srp_destroy_qp() to verify whether a channel has been disconnected before it is destroyed. The target->connected flag provides an easy way to test the connected state in e.g. srp_disconnect_target().

Quoted 7 lines

Quoted 2 lines

We can just as easily check rport state. rport state is modified before target-connected. I still think one is redundant.

Sorry but I disagree. If e.g. both the fast_io_fail and dev_loss_tmo timers have been disabled then the rport state is SRP_RPORT_RUNNING all the time. At least in that case it is not possible to derive the target->connected state from the rport state.

Can't you rely on ch->connected for that?

We can keep both, but I just think that the meaning of target->connected is confusing now.

Reception of a DREQ message only causes the state of a single channel to change. Modify the SRP initiator such that channel and target connection state are tracked separately. This patch avoids that following false positive warning can be reported by srp_destroy_qp():

WARNING: at drivers/infiniband/ulp/srp/ib_srp.c:617 srp_destroy_qp+0xa6/0x120 [ib_srp]()
Call Trace:
[<ffffffff8106e10f>] warn_slowpath_common+0x7f/0xc0
[<ffffffff8106e16a>] warn_slowpath_null+0x1a/0x20
[<ffffffffa0440226>] srp_destroy_qp+0xa6/0x120 [ib_srp]
[<ffffffffa0440322>] srp_free_ch_ib+0x82/0x1e0 [ib_srp]
[<ffffffffa044408b>] srp_create_target+0x7ab/0x998 [ib_srp]
[<ffffffff81346f60>] dev_attr_store+0x20/0x30
[<ffffffff811dd90f>] sysfs_write_file+0xef/0x170
[<ffffffff8116d248>] vfs_write+0xc8/0x190
[<ffffffff8116d411>] sys_write+0x51/0x90

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

Cc <[email protected]> #v3.19

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

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

index 5ce6cfd..0eb07d3 100644
@@ -465,14 +465,13 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
  */
 static void srp_destroy_qp(struct srp_rdma_ch *ch)
 {
-	struct srp_target_port *target = ch->target;
 	static struct ib_qp_attr attr = { .qp_state = IB_QPS_ERR };
 	static struct ib_recv_wr wr = { .wr_id = SRP_LAST_WR_ID };
 	struct ib_recv_wr *bad_wr;
 	int ret;
 
 	/* Destroying a QP and reusing ch->done is only safe if not connected */
-	WARN_ON_ONCE(target->connected);
+	WARN_ON_ONCE(ch->connected);
 
 	ret = ib_modify_qp(ch->qp, &attr, IB_QP_STATE);
 	WARN_ONCE(ret, "ib_cm_init_qp_attr() returned %d\n", ret);

@@ -836,6 +835,7 @@ static void srp_disconnect_target(struct srp_target_port *target)
 
 		for (i = 0; i < target->ch_count; i++) {
 			ch = &target->ch[i];
+			ch->connected = false;
 			if (ch->cm_id && ib_send_cm_dreq(ch->cm_id, NULL, 0)) {
 				shost_printk(KERN_DEBUG, target->scsi_host,
 					     PFX "Sending CM DREQ failed\n");

@@ -1017,6 +1017,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
 		switch (ch->status) {
 		case 0:
 			srp_change_conn_state(target, true);
+			ch->connected = true;
 			return 0;
 
 		case SRP_PORT_REDIRECT:

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

 		if (ib_send_cm_drep(cm_id, NULL, 0))
 			shost_printk(KERN_ERR, target->scsi_host,
 				     PFX "Sending CM DREP failed\n");

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

index a611556..95a4471 100644
@@ -170,6 +170,7 @@ struct srp_rdma_ch {
 
 	struct completion	tsk_mgmt_done;
 	u8			tsk_mgmt_status;
+	bool			connected;
 };
 
 /**

drivers/infiniband/ulp/srp/ib_srp.c static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event) · line 2371

Quoted 6 lines · Bart Van Assche

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

@@ -2367,7 +2368,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
  	case IB_CM_DREQ_RECEIVED:
  		shost_printk(KERN_WARNING, target->scsi_host,
  			     PFX "DREQ received - connection closed\n");
-		srp_change_conn_state(target, false);
+		ch->connected = false;

So, in this patch, you modify disconnect to set srp_change_conn_state() to false for the target, then loop through all the channels sending cm_dreq's, and on the receiving side, you modify the cm_dreq handler to set each channel to false. However, once you get to 0 channels open, shouldn't you then set the target state to false too just to keep things consistent?

Quoted 14 lines · Doug Ledford

drivers/infiniband/ulp/srp/ib_srp.c static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event) · line 2371

Quoted 6 lines · Bart Van Assche

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

@@ -2367,7 +2368,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
  	case IB_CM_DREQ_RECEIVED:
  		shost_printk(KERN_WARNING, target->scsi_host,
  			     PFX "DREQ received - connection closed\n");
-		srp_change_conn_state(target, false);
+		ch->connected = false;

So, in this patch, you modify disconnect to set srp_change_conn_state() to false for the target, then loop through all the channels sending cm_dreq's, and on the receiving side, you modify the cm_dreq handler to set each channel to false. However, once you get to 0 channels open, shouldn't you then set the target state to false too just to keep things consistent?

Hello Doug,

What is not visible in this patch but only in the ib_srp.c source code is that the first received DREQ initiates a reconnect (the queue_work() call below):

	case IB_CM_DREQ_RECEIVED:
		shost_printk(KERN_WARNING, target->scsi_host,
			     PFX "DREQ received - connection closed\n");
		ch->connected = false;
		if (ib_send_cm_drep(cm_id, NULL, 0))
			shost_printk(KERN_ERR, target->scsi_host,
				     PFX "Sending CM DREP failed\n");
		queue_work(system_long_wq, &target->tl_err_work);
		break;

That should be sufficient to restore communication after a DREQ has been received.

However, while looking through the driver to research this, I noticed something else that seems more important if you ask me. With this patch we now implement individual channel connection tracking. However, in srp_queuecommand() you pick the channel based on the tag, and the blk layer has no idea of these disconnects, so the blk layer is free to assign a tag/channel to a channel that's disconnected, and then as best

I can tell, you will simply try to post a work request to a channel that's already disconnected, which I would expect to fail if we have already disconnected this particular qp and not brought up a new one yet. So it seems to me there is a race condition between new incoming SCSI commands and this disconnect/reconnect window, and that maybe we should be sending these commands back to the mid layer for requeueing when the channel the blk_mq tag points to is disconnected. Or am I missing something in there?

Quoted 6 lines · Doug Ledford

However, while looking through the driver to research this, I noticed something else that seems more important if you ask me. With this patch we now implement individual channel connection tracking. However, in srp_queuecommand() you pick the channel based on the tag, and the blk layer has no idea of these disconnects, so the blk layer is free to assign a tag/channel to a channel that's disconnected, and then as best

Quoted 8 lines

I can tell, you will simply try to post a work request to a channel that's already disconnected, which I would expect to fail if we have already disconnected this particular qp and not brought up a new one yet. So it seems to me there is a race condition between new incoming SCSI commands and this disconnect/reconnect window, and that maybe we should be sending these commands back to the mid layer for requeueing when the channel the blk_mq tag points to is disconnected. Or am I missing something in there?

Hello Doug,

Around the time a cable disconnect or other link layer failure is detected by the SRP initiator or any other SCSI LLD it is unavoidable that one or more SCSI requests fail. It is up to a higher layer (e.g. dm-multipath + multipathd) to decide what to do with such requests, e.g. queue these requests and resend these over another path.

The SRP
initiator driver has been tested thoroughly with the multipath queue_if_no_path policy, with a fio job with I/O verification enabled running on top of a dm device while concurrently repeatedly simulating link layer failures (via ibportstate).

Quoted 16 lines · Bart Van Assche

Quoted 6 lines · Doug Ledford

However, while looking through the driver to research this, I noticed something else that seems more important if you ask me. With this patch we now implement individual channel connection tracking. However, in srp_queuecommand() you pick the channel based on the tag, and the blk layer has no idea of these disconnects, so the blk layer is free to assign a tag/channel to a channel that's disconnected, and then as best

Quoted 8 lines

I can tell, you will simply try to post a work request to a channel that's already disconnected, which I would expect to fail if we have already disconnected this particular qp and not brought up a new one yet. So it seems to me there is a race condition between new incoming SCSI commands and this disconnect/reconnect window, and that maybe we should be sending these commands back to the mid layer for requeueing when the channel the blk_mq tag points to is disconnected. Or am I missing something in there?

Quoted 7 lines

Hello Doug,

Around the time a cable disconnect or other link layer failure is detected by the SRP initiator or any other SCSI LLD it is unavoidable that one or more SCSI requests fail. It is up to a higher layer (e.g. dm-multipath + multipathd) to decide what to do with such requests, e.g. queue these requests and resend these over another path.

Sure, but that wasn't my point. My point was that if you know the channel is disconnected, then why don't you go immediately to the correct action in queuecommand (where correct action could be requeue waiting on reconnect or return with error, whatever is appropriate)? Instead you attempt to post a command to a known disconnected queue pair.

Quoted 4 lines

The SRP initiator driver has been tested thoroughly with the multipath queue_if_no_path policy, with a fio job with I/O verification enabled running on top of a dm device while concurrently repeatedly simulating link layer failures (via ibportstate).

Part of my questions here are because I don't know how the blk_mq handles certain conditions. However, your testing above only handles one case: all channels get dropped. As unlikely it may be, what if resource constraints caused just one channel to be dropped out of the bunch and the others stayed alive? Then the blk_mq would see requests on just one queue come back errored, while the others finished successfully. Does it drop that one queue out of rotation, or does it fail over the entire connection?

Quoted 37 lines · Doug Ledford

Quoted 16 lines · Bart Van Assche

Quoted 6 lines · Doug Ledford

However, while looking through the driver to research this, I noticed something else that seems more important if you ask me. With this patch we now implement individual channel connection tracking. However, in srp_queuecommand() you pick the channel based on the tag, and the blk layer has no idea of these disconnects, so the blk layer is free to assign a tag/channel to a channel that's disconnected, and then as best

Quoted 8 lines

I can tell, you will simply try to post a work request to a channel that's already disconnected, which I would expect to fail if we have already disconnected this particular qp and not brought up a new one yet. So it seems to me there is a race condition between new incoming SCSI commands and this disconnect/reconnect window, and that maybe we should be sending these commands back to the mid layer for requeueing when the channel the blk_mq tag points to is disconnected. Or am I missing something in there?

Quoted 7 lines

Hello Doug,

Around the time a cable disconnect or other link layer failure is detected by the SRP initiator or any other SCSI LLD it is unavoidable that one or more SCSI requests fail. It is up to a higher layer (e.g. dm-multipath + multipathd) to decide what to do with such requests, e.g. queue these requests and resend these over another path.

Sure, but that wasn't my point. My point was that if you know the channel is disconnected, then why don't you go immediately to the correct action in queuecommand (where correct action could be requeue waiting on reconnect or return with error, whatever is appropriate)? Instead you attempt to post a command to a known disconnected queue pair.

Quoted 4 lines

The SRP initiator driver has been tested thoroughly with the multipath queue_if_no_path policy, with a fio job with I/O verification enabled running on top of a dm device while concurrently repeatedly simulating link layer failures (via ibportstate).

Quoted 8 lines

Part of my questions here are because I don't know how the blk_mq handles certain conditions. However, your testing above only handles one case: all channels get dropped. As unlikely it may be, what if resource constraints caused just one channel to be dropped out of the bunch and the others stayed alive? Then the blk_mq would see requests on just one queue come back errored, while the others finished successfully. Does it drop that one queue out of rotation, or does it fail over the entire connection?

Hello Doug,

Sorry but I don't think that a SCSI LLD is the appropriate layer to choose between requeuing or failing a request.

If multiple paths are
available between an initiator system and a SAN and if one path fails

only the multipath layer knows whether there are other working paths available. If a working path is still available then the request should be resent as soon as possible over another path. The multipath layer can only take such a decision after a SCSI LLD has failed a request.

If only one channel fails all other channels are disconnected and the transport layer error handling mechanism is started.

Be that as it may, that doesn't change what I said about posting a command to a known disconnected QP. You could just fail immediately. Something like:

if (!ch->connected) {
	scmnd->result = DID_NO_CONNECT;
	goto err;
}

right after getting the channel in queuecommand would work. That would save a couple spinlocks, several DMA mappings, a call into the low level driver, and a few other things. (And I only left requeue on the table because I wasn't sure how the blk_mq dealt with just a single channel being down versus all of them being down)

But my point in all of this is that if you have a single qp between yourself and the target, then any error including a qp resource error == path error since you only have one path. When you have a multi queue device, that's no longer true. A transient resource problem on one qp

does not mean a path event (at least not necessarily, although your statement below converts a QP event into a path event by virtue disconnecting and reconnecting all of the QPs). My curiosity is now moot given what you wrote about tearing everything down and reconnecting (unless the error handling is modified to be more subtle in its workings), but the original question in my mind was what happens at the blk_mq level if you did have a single queue drop but not all of them and you weren't using multipath.

Quoted 2 lines

If only one channel fails all other channels are disconnected and the transport layer error handling mechanism is started.

I missed that. I assume it's done in srp_start_tl_fail_timers()?

Hello Doug,

Quoted 9 lines · Doug Ledford

Be that as it may, that doesn't change what I said about posting a command to a known disconnected QP. You could just fail immediately. Something like:

if (!ch->connected) {
	scmnd->result = DID_NO_CONNECT;
	goto err;
}

Quoted 5 lines

right after getting the channel in queuecommand would work. That would save a couple spinlocks, several DMA mappings, a call into the low level driver, and a few other things. (And I only left requeue on the table because I wasn't sure how the blk_mq dealt with just a single channel being down versus all of them being down)

What you wrote above looks correct to me. However, it is intentional that such a check is not present in srp_queuecommand(). The intention was to optimize the hot path of that driver as much as possible. Hence the choice to post a work request on the QP even after it has been disconnected and to let the HCA generate an error completion.

Quoted 4 lines

But my point in all of this is that if you have a single qp between yourself and the target, then any error including a qp resource error == path error since you only have one path. When you have a multi queue device, that's no longer true. A transient resource problem on one qp

Quoted 8 lines

does not mean a path event (at least not necessarily, although your statement below converts a QP event into a path event by virtue disconnecting and reconnecting all of the QPs). My curiosity is now moot given what you wrote about tearing everything down and reconnecting (unless the error handling is modified to be more subtle in its workings), but the original question in my mind was what happens at the blk_mq level if you did have a single queue drop but not all of them and you weren't using multipath.

If we want to support this without adding similar code to handle this in every SCSI LLD I think we need to change first how blk-mq and dm-multipath interact. Today dm-multipath is a layer on top of blk-mq. Supporting the above scenario properly is possible e.g. by integrating multipath support in the blk-mq layer. I think Hannes and Christoph have already started to work on this.

Unlike FC, there is no risk that SCSI devices will be offlined by the error handler if it is started while a reconnect attempt is ongoing. Hence report timeout errors as soon as possible if a higher software layer (e.g. multipathd) needs to be informed about a timeout. It is assumed that if both fast_io_fail_tmo < 0 and rport->dev_loss_tmo < 0 that this means that there is no need to report timeouts quickly.

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

---
  drivers/scsi/scsi_transport_srp.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +5 -0

index 4a44337..6667c2b 100644
@@ -61,6 +61,11 @@ static inline struct Scsi_Host *rport_to_shost(struct srp_rport *r)
  	return dev_to_shost(r->dev.parent);
  }
 
+static inline struct srp_rport *shost_to_rport(struct Scsi_Host *shost)
+{
+	return transport_class_to_srp_rport(&shost->shost_gendev);
+}
+
  /**
   * srp_tmo_valid() - check timeout combination validity
   * @reconnect_delay: Reconnect delay in seconds.

@@ -626,9 +631,11 @@ static enum blk_eh_timer_return srp_timed_out(struct scsi_cmnd *scmd)
  	struct scsi_device *sdev = scmd->device;
  	struct Scsi_Host *shost = sdev->host;

Quoted 39 lines · Bart Van Assche

Unlike FC, there is no risk that SCSI devices will be offlined by the error handler if it is started while a reconnect attempt is ongoing. Hence report timeout errors as soon as possible if a higher software layer (e.g. multipathd) needs to be informed about a timeout. It is assumed that if both fast_io_fail_tmo < 0 and rport->dev_loss_tmo < 0 that this means that there is no need to report timeouts quickly.

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

---
  drivers/scsi/scsi_transport_srp.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +5 -0

index 4a44337..6667c2b 100644
@@ -61,6 +61,11 @@ static inline struct Scsi_Host
 *rport_to_shost(struct srp_rport *r)
      return dev_to_shost(r->dev.parent);
  }
 
+static inline struct srp_rport *shost_to_rport(struct Scsi_Host *shost)
+{
+    return transport_class_to_srp_rport(&shost->shost_gendev);
+}
+
  /**
   * srp_tmo_valid() - check timeout combination validity

* @reconnect_delay: Reconnect delay in seconds.

drivers/scsi/scsi_transport_srp.c +0 -0

@@ -626,9 +631,11 @@ static enum blk_eh_timer_return
 srp_timed_out(struct scsi_cmnd *scmd)
      struct scsi_device *sdev = scmd->device;

drivers/scsi/scsi_transport_srp.c static enum blk_eh_timer_return · line 641

Quoted 10 lines

drivers/scsi/scsi_transport_srp.c +3 -1

      struct Scsi_Host *shost = sdev->host;
      struct srp_internal *i = to_srp_internal(shost->transportt);
+    struct srp_rport *rport = shost_to_rport(shost);
 
      pr_debug("timeout for sdev %s\n", dev_name(&sdev->sdev_gendev));
-    return i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
+    return rport->fast_io_fail_tmo < 0 && rport->dev_loss_tmo < 0 &&
+        i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
          BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;
  }

I'm not sure I understand the purpose of this patch? If the user requested fast_io_fail_tmo of X, I would assume it wants srp to retry for the command for X secs. Why should we fail it any earlier than that?

There is a situation where I've seen srp starting the fast_io_fail_tmo later than expected because the QP retry exceeded error completion arrives late for certain workloads. Was this the motivation for this patch?

Quoted 55 lines · Sagi Grimberg

Quoted 39 lines · Bart Van Assche

Unlike FC, there is no risk that SCSI devices will be offlined by the error handler if it is started while a reconnect attempt is ongoing. Hence report timeout errors as soon as possible if a higher software layer (e.g. multipathd) needs to be informed about a timeout. It is assumed that if both fast_io_fail_tmo < 0 and rport->dev_loss_tmo < 0 that this means that there is no need to report timeouts quickly.

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

---
  drivers/scsi/scsi_transport_srp.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +5 -0

index 4a44337..6667c2b 100644
@@ -61,6 +61,11 @@ static inline struct Scsi_Host
 *rport_to_shost(struct srp_rport *r)
      return dev_to_shost(r->dev.parent);
  }
 
+static inline struct srp_rport *shost_to_rport(struct Scsi_Host *shost)
+{
+    return transport_class_to_srp_rport(&shost->shost_gendev);
+}
+
  /**
   * srp_tmo_valid() - check timeout combination validity

* @reconnect_delay: Reconnect delay in seconds.

drivers/scsi/scsi_transport_srp.c +0 -0

@@ -626,9 +631,11 @@ static enum blk_eh_timer_return
 srp_timed_out(struct scsi_cmnd *scmd)
      struct scsi_device *sdev = scmd->device;

drivers/scsi/scsi_transport_srp.c static enum blk_eh_timer_return · line 641

Quoted 10 lines

drivers/scsi/scsi_transport_srp.c +3 -1

      struct Scsi_Host *shost = sdev->host;
      struct srp_internal *i = to_srp_internal(shost->transportt);
+    struct srp_rport *rport = shost_to_rport(shost);
 
      pr_debug("timeout for sdev %s\n", dev_name(&sdev->sdev_gendev));
-    return i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
+    return rport->fast_io_fail_tmo < 0 && rport->dev_loss_tmo < 0 &&
+        i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
          BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;
  }

I'm not sure I understand the purpose of this patch? If the user requested fast_io_fail_tmo of X, I would assume it wants srp to retry for the command for X secs. Why should we fail it any earlier than that?

Quoted 4 lines

There is a situation where I've seen srp starting the fast_io_fail_tmo later than expected because the QP retry exceeded error completion arrives late for certain workloads. Was this the motivation for this patch?

The purpose of the srp_timed_out() function is not about failing commands early but about postponing timeout errors. The above patch causes a SCSI timeout to be handled as soon as the command timer expires if rport->fast_io_fail_tmo >= 0 or rport->dev_loss_tmo >= 0.

Quoted 62 lines · Bart Van Assche

Quoted 56 lines · Sagi Grimberg

Quoted 40 lines · Bart Van Assche

Unlike FC, there is no risk that SCSI devices will be offlined by the error handler if it is started while a reconnect attempt is ongoing. Hence report timeout errors as soon as possible if a higher software layer (e.g. multipathd) needs to be informed about a timeout. It is assumed that if both fast_io_fail_tmo < 0 and rport->dev_loss_tmo < 0 that this means that there is no need to report timeouts quickly.

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

Cc James Bottomley <[email protected]>

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

---
  drivers/scsi/scsi_transport_srp.c | 9 ++++++++-
  1 file changed, 8 insertions(+), 1 deletion(-)

drivers/scsi/scsi_transport_srp.c +5 -0

index 4a44337..6667c2b 100644
@@ -61,6 +61,11 @@ static inline struct Scsi_Host
 *rport_to_shost(struct srp_rport *r)
      return dev_to_shost(r->dev.parent);
  }
 
+static inline struct srp_rport *shost_to_rport(struct Scsi_Host
 *shost)
+{
+    return transport_class_to_srp_rport(&shost->shost_gendev);
+}
+
  /**

* srp_tmo_valid() - check timeout combination validity
* @reconnect_delay: Reconnect delay in seconds.

drivers/scsi/scsi_transport_srp.c +0 -0

@@ -626,9 +631,11 @@ static enum blk_eh_timer_return
 srp_timed_out(struct scsi_cmnd *scmd)
      struct scsi_device *sdev = scmd->device;

drivers/scsi/scsi_transport_srp.c static enum blk_eh_timer_return · line 641

Quoted 10 lines

drivers/scsi/scsi_transport_srp.c +3 -1

      struct Scsi_Host *shost = sdev->host;
      struct srp_internal *i = to_srp_internal(shost->transportt);
+    struct srp_rport *rport = shost_to_rport(shost);
 
      pr_debug("timeout for sdev %s\n", dev_name(&sdev->sdev_gendev));
-    return i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
+    return rport->fast_io_fail_tmo < 0 && rport->dev_loss_tmo < 0 &&
+        i->f->reset_timer_if_blocked && scsi_device_blocked(sdev) ?
          BLK_EH_RESET_TIMER : BLK_EH_NOT_HANDLED;
  }

I'm not sure I understand the purpose of this patch? If the user requested fast_io_fail_tmo of X, I would assume it wants srp to retry for the command for X secs. Why should we fail it any earlier than that?

Quoted 4 lines

There is a situation where I've seen srp starting the fast_io_fail_tmo later than expected because the QP retry exceeded error completion arrives late for certain workloads. Was this the motivation for this patch?

Quoted 4 lines

The purpose of the srp_timed_out() function is not about failing commands early but about postponing timeout errors. The above patch causes a SCSI timeout to be handled as soon as the command timer expires if rport->fast_io_fail_tmo >= 0 or rport->dev_loss_tmo >= 0.

This change is effective only when
time_to_wc_err + fast_io_fail_tmo < cmd_timeout.

If the user chose this configuration, I imagine the wanted behavior is that the timeout errors won't be propagated as soon as the cmd timeout expires no?

Today, srp return BLK_EH_RESET_TIMER until fast_io_fail_tmo kicks in, and at that point, it unblocks the scsi target terminates all the commands with TRANSPORT_FAIL_FAST. This will change the behavior to trigger scsi-eh (abort, reset_device) and reset_host will trigger another reconnect which is redundant (we have reconnect work inflight).

Can you explain the motivation?

A long time ago the data type int64_t was declared as long long on x86 systems and as long on PPC systems. Today that data type is declared as long long on all Linux architectures. This means that the casts from uint64_t into unsigned long long are superfluous. Remove these superfluous casts.

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index 5d7d790..8b6f166 100644
@@ -780,7 +780,7 @@ static int srp_send_req(struct srp_rdma_ch *ch, bool multich)
  		shost_printk(KERN_DEBUG, target->scsi_host,
  			     PFX "Topspin/Cisco initiator port ID workaround "
  			     "activated for target GUID %016llx\n",
-			     (unsigned long long) be64_to_cpu(target->ioc_guid));
+			     be64_to_cpu(target->ioc_guid));
  		memset(req->priv.initiator_port_id, 0, 8);
  		memcpy(req->priv.initiator_port_id + 8,
  		       &target->srp_host->srp_dev->dev->node_guid, 8);

@@ -2561,8 +2561,7 @@ static ssize_t show_id_ext(struct device *dev, struct device_attribute *attr,
  {
  	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	return sprintf(buf, "0x%016llx\n",
-		       (unsigned long long) be64_to_cpu(target->id_ext));
+	return sprintf(buf, "0x%016llx\n", be64_to_cpu(target->id_ext));
  }
 
  static ssize_t show_ioc_guid(struct device *dev, struct device_attribute *attr,

@@ -2570,8 +2569,7 @@ static ssize_t show_ioc_guid(struct device *dev, struct device_attribute *attr,
  {
  	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	return sprintf(buf, "0x%016llx\n",
-		       (unsigned long long) be64_to_cpu(target->ioc_guid));
+	return sprintf(buf, "0x%016llx\n", be64_to_cpu(target->ioc_guid));
  }
 
  static ssize_t show_service_id(struct device *dev,

@@ -2579,8 +2577,7 @@ static ssize_t show_service_id(struct device *dev,
  {
  	struct srp_target_port *target = host_to_target(class_to_shost(dev));
 
-	return sprintf(buf, "0x%016llx\n",
-		       (unsigned long long) be64_to_cpu(target->service_id));
+	return sprintf(buf, "0x%016llx\n", be64_to_cpu(target->service_id));
  }
 
  static ssize_t show_pkey(struct device *dev, struct device_attribute *attr,

Move the module version and release date into separate fields. This makes the modinfo output easier to read.

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

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index 8b6f166..3727b90 100644
@@ -59,9 +59,10 @@
  #define DRV_RELDATE	"July 1, 2013"
 

All srp_free_req_data() callers guarantee that ch->target != NULL hence remove the ch->target test from srp_free_req_data().

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

Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index 3727b90..a73eb1e5 100644

Quoted 2 lines · Bart Van Assche

All srp_free_req_data() callers guarantee that ch->target != NULL hence remove the ch->target test from srp_free_req_data().

ch->target is not referenced anywhere in this routine. So why does this guarantee matter?

Remove the !ch->target tests from the reconnect code. These tests are not needed: upon entry of srp_rport_reconnect() it is guaranteed that all ch->target pointers are non-NULL. None of the functions srp_new_cm_id(), srp_finish_req(), srp_create_ch_ib() nor srp_connect_ch() modifies this pointer. srp_free_ch_ib() is never called concurrently with srp_rport_reconnect().

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

Cc Sagi Grimberg <[email protected]>

Cc Sebastian Parschauer <[email protected]>

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

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

index a73eb1e5..400ef7a 100644
@@ -1216,14 +1216,10 @@ static int srp_rport_reconnect(struct srp_rport *rport)
  	 */
  	for (i = 0; i < target->ch_count; i++) {
  		ch = &target->ch[i];
-		if (!ch->target)
-			break;
  		ret += srp_new_cm_id(ch);
  	}
  	for (i = 0; i < target->ch_count; i++) {
  		ch = &target->ch[i];
-		if (!ch->target)
-			break;
  		for (j = 0; j < target->req_ring_size; ++j) {
  			struct srp_request *req = &ch->req_ring[j];