IB/SRP patches for kernel 3.8

This patch series avoids that SCSI error handling triggers an endless loop and also restores reporting of QP errors in the kernel log.

Changes between v3 and v2:
- As proposed by Dave, added a patch that prevents sending of a task management function over a closed connection.

Changes between v2 and v1:
- Track connection state properly.
- Make srp_reset_host() reset requests even if reconnecting fails

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The connection state must be initialized before srp_connect_target() is invoked. Drop the assignment in srp_add_target() since it occurs after srp_connect_target() and since scsi_host_alloc() zero-initializes the Scsi_Host structure anyway.

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

index d5088ce..94f76b9 100644
@@ -1972,7 +1972,6 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 	spin_unlock(&host->target_lock);
 
 	target->state = SRP_TARGET_LIVE;
-	target->connected = false;
 
 	scsi_scan_target(&target->scsi_host->shost_gendev,
 			 0, target->scsi_id, SCAN_WILD_CARD, 0);

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

The commit message is still wrong. The issue is that srp_connect_target() has marked us as active, and this patch removes an assignment that reverses that.

This shouldn't go upstream without the fixed commit message. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Dave,

How about replacing the commit message by the following text:

"Remove an assignment that incorrectly overwrites the connection state update by srp_connect_target()."

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Works for me, ship it!

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks ! I will post a fourth version of this patch with the above commit message to make it easier for Roland.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Remove an assignment that incorrectly overwrites the connection state update by srp_connect_target().

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

index d5088ce..94f76b9 100644
@@ -1972,7 +1972,6 @@ static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
 	spin_unlock(&host->target_lock);
 
 	target->state = SRP_TARGET_LIVE;
-	target->connected = false;
 
 	scsi_scan_target(&target->scsi_host->shost_gendev,
 			 0, target->scsi_id, SCAN_WILD_CARD, 0);

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave, did you gave the patches a try?

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

This one looks to be an obvious fix, but I have not tested the others; I have been relying on Bart, Alex, and yourself for the validation. --
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Taking into account that we've been going back and forth w.r.t that validation -- that is initially things weren't working on our setups, later with picking some patches from Bart's tree, that host removal went OK, and now with the patches posted here, it doesn't go fine on my setup -- all in all, sounds like a 2nd opinion which is strengthen by a test is needed here. You only need to boot with Roland's for-next
+ Bart's patches, connect to a target, take down the IB link and issue dd. No multipath scheme is needed for the very basic validation, can you?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Do not send a task management function if sending will fail anyway because either there is no RDMA/RC connection or the QP is in the error state.

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

index 94f76b9..2633258 100644
@@ -1695,6 +1695,9 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	struct srp_iu *iu;
 	struct srp_tsk_mgmt *tsk_mgmt;
 
+	if (!target->connected || target->qp_in_error)
+		return -1;
+
 	init_completion(&target->tsk_mgmt_done);
 
 	spin_lock_irq(&target->lock);

@@ -1754,8 +1757,6 @@ static int srp_reset_device(struct scsi_cmnd *scmnd)
 
 	shost_printk(KERN_ERR, target->scsi_host, "SRP reset_device called\n");
 
-	if (target->qp_in_error)
-		return FAILED;
 	if (srp_send_tsk_mgmt(target, SRP_TAG_NO_REQ, scmnd->device->lun,
 			      SRP_TSK_LUN_RESET))
 		return FAILED;

1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

If a SCSI command times out it is passed to the SCSI error handler. The SCSI error handler will try to abort the commands that timed out. If aborting failed a device reset will be attempted. If the device reset also fails a host reset will be attempted. If the host reset also fails the whole procedure will be repeated.

srp_abort() and srp_reset_device() fail for a QP in the error state. srp_reset_host() fails after host removal has started. Hence if the SCSI error handler gets invoked after host removal has started and with the QP in the error state an endless loop will be triggered.

Modify the SCSI error handling functions in ib_srp as follows:
- Abort SCSI commands properly even if the QP is in the error state.
- Make srp_reset_host() reset SCSI requests even after host removal has already started or if reconnecting fails.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

index 2633258..8a7eb9f 100644
@@ -700,23 +700,24 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	struct Scsi_Host *shost = target->scsi_host;
 	int i, ret;
 
-	if (target->state != SRP_TARGET_LIVE)
-		return -EAGAIN;
-
 	scsi_target_block(&shost->shost_gendev);
 
 	srp_disconnect_target(target);
 	/*
-	 * Now get a new local CM ID so that we avoid confusing the
-	 * target in case things are really fouled up.
+	 * Now get a new local CM ID so that we avoid confusing the target in
+	 * case things are really fouled up. Doing so also ensures that all CM
+	 * callbacks will have finished before a new QP is allocated.
 	 */
 	ret = srp_new_cm_id(target);
-	if (ret)
-		goto unblock;
-
-	ret = srp_create_target_ib(target);
-	if (ret)
-		goto unblock;
+	/*
+	 * Whether or not creating a new CM ID succeeded, create a new
+	 * QP. This guarantees that all completion callback function
+	 * invocations have finished before request resetting starts.
+	 */
+	if (ret == 0)
+		ret = srp_create_target_ib(target);
+	else
+		srp_create_target_ib(target);
 
 	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
 		struct srp_request *req = &target->req_ring[i];

@@ -728,9 +729,9 @@ static int srp_reconnect_target(struct srp_target_port *target)
 	for (i = 0; i < SRP_SQ_SIZE; ++i)
 		list_add(&target->tx_ring[i]->list, &target->free_tx);
 
-	ret = srp_connect_target(target);
+	if (ret == 0)
+		ret = srp_connect_target(target);
 
-unblock:
 	scsi_target_unblock(&shost->shost_gendev, ret == 0 ? SDEV_RUNNING :
 			    SDEV_TRANSPORT_OFFLINE);
 

@@ -1739,7 +1740,7 @@ static int srp_abort(struct scsi_cmnd *scmnd)
 
 	shost_printk(KERN_ERR, target->scsi_host, "SRP abort called\n");
 
-	if (!req || target->qp_in_error || !srp_claim_req(target, req, scmnd))
+	if (!req || !srp_claim_req(target, req, scmnd))
 		return FAILED;
 	srp_send_tsk_mgmt(target, req->index, scmnd->device->lun,
 			  SRP_TSK_ABORT_TASK);

1.7.10.4


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

It's not the cleanest way to handle this, but it is the simplest and we're short on time.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart,

I tried the patches now, took these three commits from git://github.com/bvanassche/linux.git
branch 3.8-ib-srp-fixes which I assume are the ones you posted here and applied them on Roland's for-next branch

commit 62862c0f93d47853b8d321fe5dcdd5d789e92d08
IB/srp: Avoid endless SCSI error handling loop

commit d73006faf751df4fc2fff7514f6fc6a74c41fd35 IB/srp: Avoid sending a task management function needlessly

commit befde6a2ceca6b894fb183ff96d175d6c380546b
IB/srp: Track connection state properly

Basically, I connected to an SRP target, later took down the initiator IB port, and then attempted to issue some IO over the SRP lun (no multipathing). What happens is that things are seemed to be detected properly, reconnection goes in a loop, but the SCSI host (host6) isn't deleted, I can't unload the module since there's non-zero ref count. Only when I bring back the port, I see

scsi host6: ib_srp: reconnect succeeded
sd 6:0:0:1: [sdc] Synchronizing SCSI cache
... etc on all the luns of that host

and only then the SCSI host is finally removed, the module ref count goes to zero and I canunload the module. Maybe another patch is needed here? I think few days ago you had a patch on your tree named "Save and restore host_scribble during error handling", is it possible we need this here for happy removal of the scsi host?

Or.

Dec 20 14:06:13 vsa33 kernel: scsi6 : SRP.T10:0002C9030010B014
Dec 20 14:06:13 vsa33 kernel: scsi 6:0:0:0: Direct-Access     SUN      COMSTAR          1.0  PQ: 0 ANSI: 5
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:0: Attached scsi generic sg1 type 0
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:0: [sdb] 409600 512-byte logical blocks: (209 MB/200 MiB)
Dec 20 14:06:13 vsa33 kernel: scsi 6:0:0:1: Direct-Access     SUN      COMSTAR          1.0  PQ: 0 ANSI: 5
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:1: Attached scsi generic sg2 type 0
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:1: [sdc] 409600 512-byte logical blocks: (209 MB/200 MiB)
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:1: [sdc] Write Protect is off
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:1: [sdc] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Dec 20 14:06:13 vsa33 kernel: scsi 6:0:0:2: Direct-Access     SUN      COMSTAR          1.0  PQ: 0 ANSI: 5
Dec 20 14:06:13 vsa33 kernel: sdc: unknown partition table
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:2: Attached scsi generic sg3 type 0
Dec 20 14:06:13 vsa33 kernel: scsi 6:0:0:3: Direct-Access     SUN      COMSTAR          1.0  PQ: 0 ANSI: 5
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:3: Attached scsi generic sg4 type 0
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:2: [sdd] 409600 512-byte logical blocks: (209 MB/200 MiB)
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:3: [sde] 104857600 512-byte logical blocks: (53.6 GB/50.0 GiB)
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:0: [sdb] Write Protect is off
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:3: [sde] Write Protect is off
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:3: [sde] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:1: [sdc] Attached SCSI disk
Dec 20 14:06:13 vsa33 kernel: sdb: unknown partition table
Dec 20 14:06:13 vsa33 kernel: sde: unknown partition table
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:2: [sdd] Write Protect is off
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:0: [sdb] Attached SCSI disk
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:3: [sde] Attached SCSI disk
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:2: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
Dec 20 14:06:13 vsa33 kernel: sdd: unknown partition table
Dec 20 14:06:13 vsa33 kernel: sd 6:0:0:2: [sdd] Attached SCSI disk
Dec 20 14:08:02 vsa33 kernel: scsi host6: ib_srp: failed send status 5
Dec 20 14:08:29 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:08:39 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:08:39 vsa33 kernel: scsi host6: SRP reset_device called
Dec 20 14:08:39 vsa33 kernel: scsi host6: ib_srp: SRP reset_host called
Dec 20 14:08:40 vsa33 kernel: scsi host6: ib_srp: Got failed path rec status -110
Dec 20 14:08:40 vsa33 kernel: scsi host6: ib_srp: Path record query failed
Dec 20 14:08:40 vsa33 kernel: scsi host6: ib_srp: reconnect failed (-110), removing target port.
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:1: Device offlined - not ready after error recovery
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:1: [sdc] Unhandled error code
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:1: [sdc]
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:1: [sdc] CDB:
Dec 20 14:08:40 vsa33 kernel: end_request: I/O error, dev sdc, sector 0
Dec 20 14:08:40 vsa33 kernel: Buffer I/O error on device sdc, logical block 0
Dec 20 14:08:40 vsa33 kernel: Buffer I/O error on device sdc, logical block 1
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:1: rejecting I/O to offline device
Dec 20 14:08:40 vsa33 kernel: Buffer I/O error on device sdc, logical block 2
Dec 20 14:08:40 vsa33 kernel: Buffer I/O error on device sdc, logical block 3
Dec 20 14:08:40 vsa33 kernel: sd 6:0:0:0: [sdb] Synchronizing SCSI cache
Dec 20 14:09:41 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:09:51 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:09:51 vsa33 kernel: scsi host6: SRP reset_device called
Dec 20 14:09:51 vsa33 kernel: scsi host6: ib_srp: SRP reset_host called
Dec 20 14:09:52 vsa33 kernel: scsi host6: ib_srp: Got failed path rec status -110
Dec 20 14:09:52 vsa33 kernel: scsi host6: ib_srp: Path record query failed
Dec 20 14:09:52 vsa33 kernel: scsi host6: ib_srp: reconnect failed (-110), removing target port.
Dec 20 14:09:52 vsa33 kernel: sd 6:0:0:0: Device offlined - not ready after error recovery
Dec 20 14:10:53 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:11:03 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:11:03 vsa33 kernel: scsi host6: SRP reset_device called
Dec 20 14:11:03 vsa33 kernel: scsi host6: ib_srp: SRP reset_host called
Dec 20 14:11:04 vsa33 kernel: scsi host6: ib_srp: Got failed path rec status -110
Dec 20 14:11:04 vsa33 kernel: scsi host6: ib_srp: Path record query failed
Dec 20 14:11:04 vsa33 kernel: scsi host6: ib_srp: reconnect failed (-110), removing target port.
Dec 20 14:11:04 vsa33 kernel: sd 6:0:0:0: Device offlined - not ready after error recovery
Dec 20 14:12:05 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:12:15 vsa33 kernel: scsi host6: SRP abort called
Dec 20 14:12:15 vsa33 kernel: scsi host6: SRP reset_device called
Dec 20 14:12:15 vsa33 kernel: scsi host6: ib_srp: SRP reset_host called
Dec 20 14:12:15 vsa33 kernel: scsi host6: ib_srp: reconnect succeeded
Dec 20 14:12:25 vsa33 kernel: sd 6:0:0:1: [sdc] Synchronizing SCSI cache
Dec 20 14:12:25 vsa33 kernel: sd 6:0:0:2: [sdd] Synchronizing SCSI cache
Dec 20 14:12:25 vsa33 kernel: sd 6:0:0:3: [sde] Synchronizing SCSI cache

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

No. Host removal works fine even without that patch. That's because srp_abort() has been modified such that it finishes a request whether or not sending a task management function to the target succeeded. As you can see in scsi_eh_abort_cmnds() if the eh_abort_handler callback function returns SUCCESS then scsi_eh_test_devices() will be passed an empty work_q list. Hence the SCSI error handler function scsi_eh_prep_cmnd() does not get invoked and hence does not have a chance of overwriting the host_scribble field.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

not on my setup, how do you suggest to proceed here?

Or.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html