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

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

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

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

Bart,

You wrote "resend" in the subject line, anything new, in these patches OR in other patches merged through the SCSI tree for 3.8 vs. what you had posted earlier on Dec 20th, 2012
(http://marc.info/?t=135592692800001&r=1&w=2)?! as I wrote there, it didn't work for me.

Dave, Roland, as I wrote here
http://marc.info/?l=linux-rdma&m=135603401830703&w=2 I think what we need now is 2nd opinion, and I asked if Dave can give the patches a try, no more but no less... we need to know if it works

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

Hello Or,

I have just reposted my SCSI device removal patch series. There are several changes in v8 of that patch series compared to v7, including:
* Saving and restoring the host scribble field in the SCSI error
handler.
* A robustness improvement for when the SCSI timeout is below the
maximum LLD response processing time.

Version eight of the SCSI device removal patch series can be found here: http://github.com/bvanassche/linux/tree/device-removal-fixes.

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

Bart, I'd like to sharpen the point: could you please clarify if the series posted to linux-rdma stands for itself in the sense that SRP HA scheme X (please state it) now works/better when the patches applied on top of the latest 3.8-rc cut? OR for X to do better/work, one needs this series AND the one you posted to linux-scsi.

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

Hello Or,

A huge number of patches have been taken upstream between 3.8-rc1 and 3.8-rc6. I have retested these three patches with 3.8-rc6 and would appreciate if you would also repeat your tests.

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

not really... this is what I see on Linus tree for the relevant directories, anywhere else I need to look

linux-2.6]# git log --oneline v3.8-rc1..v3.8-rc6 drivers/scsi/ drivers/block/ block/drivers/infiniband/ulp/srp bdb0ae6 Merge branch 'x86-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
83e6818 efi: Make 'efi_enabled' a function to query EFI facilities
2263647 Merge tag 'fixes-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux
8d85fce Drivers: block: remove __dev* attributes.
6f03979 Drivers: scsi: remove __dev* attributes.
f4953fe virtio-blk: Don't free ida when disk is in use

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

Hello Or,

Nobody outside Mellanox has ever been able to reproduce the behavior reported by you. Something in your tests might have been specific to the Mellanox environment. Have you perhaps been running your tests with a firmware version that is not available to the general public ? I would appreciate it if you could check your test environment and repeat your tests.

Thanks for your understanding,

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

I have asked for 2nd opinion so we can get a quorum either way.

NO

We will repeat the tests, indeed.

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

Hello Bart,

I tested your 3.8 v3 patchset. I did the following:
- clone & checkout Roland's ib tree for-next branch
- applied Bart's 3.8 v3 patchset
- applied "save & restore host_scribble during error handling" patch - http://www.mail-archive.com/[email protected]/msg17809.html

I have two paths to target thru port 1 & 2 (scsi_host host9 & host10)

- run I/Os
- disable port 1 @ 19:11:30
- error recovery for host9 kick in @ 19:12:04
- multipath remove the path, I/Os fail-over @ 19:12:51
- error recovery was still going on with host9 (sysfs entry for host9 still intact)
- enable port 1 @19:15:00
- host9 reconnect to target thru error recovery, multipathd module re-instate the path in kernel; and then host9 is REMOVED, usermode "multipath -l" did not show re-instate path thru host9

Feb  6 19:15:04 vsa30 kernel: scsi host9: SRP abort called
Feb  6 19:15:05 vsa30 multipathd: overflow in attribute '/sys/devices/pci0000:00/0000:00:02.0/0000:02:00.0/host9/target9:0:0/9:0:0:2/state'
Feb  6 19:15:14 vsa30 kernel: scsi host9: SRP abort called
Feb  6 19:15:14 vsa30 kernel: scsi host9: SRP reset_device called
Feb  6 19:15:14 vsa30 kernel: scsi host9: ib_srp: SRP reset_host called
Feb  6 19:15:14 vsa30 kernel: scsi host9: ib_srp: reconnect succeeded
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180003: sdd - tur checker reports path is up
Feb  6 19:15:26 vsa30 multipathd: 8:48: reinstated
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180003: remaining active paths: 2
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180002: sdc - tur checker reports path is up
Feb  6 19:15:26 vsa30 multipathd: 8:32: reinstated
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180002: remaining active paths: 2
Feb  6 19:15:26 vsa30 multipathd: sdc: remove path (uevent)
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180002: load table [0 409600 multipath 0 0 1 1 round-robin 0 1 1 8:80 1]
Feb  6 19:15:26 vsa30 multipathd: sdc: path removed from map 3600144f0665c4400000050a522180002
Feb  6 19:15:26 vsa30 kernel: sd 9:0:0:1: [sdc] Synchronizing SCSI cache
Feb  6 19:15:26 vsa30 multipathd: sdd: remove path (uevent)
Feb  6 19:15:26 vsa30 multipathd: 3600144f0665c4400000050a522180003: load table [0 409600 multipath 0 0 1 1 round-robin 0 1 1 8:96 1]
Feb  6 19:15:26 vsa30 multipathd: sdd: path removed from map 3600144f0665c4400000050a522180003
Feb  6 19:15:26 vsa30 kernel: sd 9:0:0:2: [sdd] Synchronizing SCSI cache

- disable port 2 @19:22:50
- error recovery kicked in on host10 @ 19:23:40
- I/Os failed with NO path to target @ 19:24:27
- without enabling port 2, error recovery was still going on host10 still 19:57:52 and stop.
- host10 was still in sysfs /sys/class/scsi_host/host10 & taking reference on ib_srp module
- enable port 2 - nothing happened.

Conclusion:
1. disable the port/path long enough >35 minutes, we have dangling scsi host.
2. enable the port within 30 minute, scsi host re-establish connection, path re-instate and then scsi_host was removed (no entry in sysfs)

I attached a log here to show what happened above.

thanks,
-vu

Hello Vu,

I found the following in the attached logs:

[ ... ]

Feb  6 19:24:25 vsa30 kernel: scsi host10: ib_srp: SRP reset_host called
[ ... ]
Feb  6 19:25:28 vsa30 kernel: scsi host10: SRP abort called

[ ... ]

It is easy to see in patch 3/3 that srp_reset_host() invokes srp_reconnect_target() unconditionally and that that last function kills all outstanding requests via srp_reset_req(). So to me the above output means that the attached logs were generated by a kernel missing at least patch 3/3. This means that the above conclusions are invalid.

I think it is also worth mentioning here that I asked Mellanox two months ago via private e-mail to provide me access to a setup on which this issue can be reproduced and on which I can recompile the kernel myself. However, such access was never provided.

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

Hi Bart,

Please calm down, we truly think your work is great step in the right direction. We will double check the environment and the logs provided to you.
On the half side of the glass, I think Vu also saw things that work better with these patches (BTW - if the fourth patch that Vu used "save & restore host_scribble during error handling" is also needed, maybe you add it to this series, so they are reviewed/accepted together).

We will set with you online session in the coming 2-3 working days to build the kernel and conduct the test together.

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

Hello Or,

The three patches I posted guarantee timely host removal even without the host_scribble patch. Even if srp_abort() would not have aborted a request because the host_scribble field had been overwritten, srp_reset_host() will be invoked later on and will finish such a request properly.

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

Hello Bart,

srp_reconnect_target() kill all outstanding requests, fail to reconnect (port offline), queued to remove scsi_host --> srp_reset_host() return FAILED.

While scsi host has not been removed, multipath periodically still send TUR commands to check liveness of this path. Current srp_queuecommand() still process these TUR commands; therefore, the next SRP aborts are for those tur checker commands

You can see these tur checker errors from multipath in the log.

Sagi from Mellanox will work with you on webex.

thanks,
-vu

--
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 an SRP target is no longer reachable and srp_reset_host() fails to reconnect then ib_srp will invoke scsi_remove_host(). That function will invoke __scsi_remove_device() for each LUN. And that last function will change the device state from SDEV_TRANSPORT_OFFLINE into SDEV_CANCEL. Certain user space software, e.g. older versions of multipathd, continue queueing I/O to SCSI devices that are in the SDEV_CANCEL state. If these I/O requests are submitted as SG_IO that means that the REQ_PREEMPT flag will be set and hence that these requests will be passed to srp_queuecommand(). These requests will time out. If new requests are queued fast enough from user space these active requests will prevent __scsi_remove_device() to finish. Avoid this by failing I/O requests in the SDEV_CANCEL state if the transport is offline. Introduce a new variable to keep track of the transport state instead of failing requests if (!target->connected || target->qp_in_error) such that the SCSI error handler has a chance to retry commands after a transport layer failure occurred.

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

index 8a7eb9f..b34752d 100644
@@ -734,6 +734,7 @@ static int srp_reconnect_target(struct srp_target_port *target)
 
 	scsi_target_unblock(&shost->shost_gendev, ret == 0 ? SDEV_RUNNING :
 			    SDEV_TRANSPORT_OFFLINE);
+	target->transport_offline = ret != 0;
 
 	if (ret)
 		goto err;

@@ -1353,6 +1354,12 @@ static int srp_queuecommand(struct Scsi_Host *shost, struct scsi_cmnd *scmnd)
 	unsigned long flags;
 	int len;
 
+	if (unlikely(target->transport_offline)) {
+		scmnd->result = DID_NO_CONNECT << 16;
+		scmnd->scsi_done(scmnd);
+		return 0;
+	}
+
 	spin_lock_irqsave(&target->lock, flags);
 	iu = __srp_get_tx_iu(target, SRP_IU_CMD);
 	if (!iu)

index de2d0b3..66fbedd 100644
@@ -140,6 +140,7 @@ struct srp_target_port {
 	unsigned int		cmd_sg_cnt;
 	unsigned int		indirect_size;
 	bool			allow_ext_sg;
+	bool			transport_offline;
 
 	/* Everything above this point is used in the hot path of
 	 * command processing. Try to keep them packed into cachelines.

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

Hey David,

This indeed solve scsi_host removal issues.
Vu is on vacation, I'll perform some more failover tests...

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

Hello Sagi,

Since no further feedback was posted on the list I assume that means that all tests passed ?

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

Hey Bart,
Sorry for the delay I was just about to reply...

From my end, the related patchset seems solve the scsi_host removal issue and prevents the SCSI error handling loop. Generally our tests passed, I still have some issue with long-term failover test but I'm not sure its SRP (perhaps might origin in IB layer). So ack from me...

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

OK

Dave/Roland - sounds like we did managed to get real progress here... lets get these fix in for 3.9 and 3.8-stable too,

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

Personally I prefer changing the assignment into the first alternative since it's more explicit than the second alternative - it doesn't require the person who's reading the source code that the transport_offline variable has been declared as "bool".

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