IB/SRP patches for kernel 3.8

PATCH New Bart Van Assche 3 participants 11 messages Dec 14, 2012 – Dec 19, 2012 8 messages not in the archive

Subsystems (2)

InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Bart Van Assche

author

2012-12-14 15:32 UTC #0 lore raw

PATCH v2 1/1 IB/SRP patches for kernel 3.8

Hello Dave,

It would be appreciated if you could have a look at the following two patches:
* Track connection state properly. Apparently an assignment statement had not been dropped while it should have been dropped.
* Avoid endless SCSI error handling loop after cable pull.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Bart Van Assche

author

2012-12-14 15:34 UTC #2 lore raw

PATCH v2 1/2 IB/srp: Track connection state properly

The connection state must be initialized before srp_connect_target() is invoked. Drop the assignment in srp_add_target() since scsi_host_alloc() zero-initializes the Scsi_Host structure anyway. This patch makes ib_srp again report the first QP error.

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

Cc David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

Cc Roland Dreier <[email protected]>

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

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

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

Signature · 6 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Dillow

Acked-by

2012-12-14 15:48 UTC #4 lore raw

Re: [PATCH v2 1/2] IB/srp: Track connection state properly

Quoted 4 lines · Bart Van Assche

The connection state must be initialized before srp_connect_target() is invoked. Drop the assignment in srp_add_target() since scsi_host_alloc() zero-initializes the Scsi_Host structure anyway. This patch makes ib_srp again report the first QP error.

This change log doesn't capture the actual reason for the change -- if it were really about the assignment being redundant with the zero-initialization, the patch would be a no-op.

"Don't trash the connection state after srp_connect_target() has properly initialized it."

or something like that.

Acked-by David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

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

author

2012-12-14 15:38 UTC #5 lore raw

PATCH v2 2/2 IB/srp: Avoid endless SCSI error handling loop

In reply to a message not in the archive

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

Since srp_abort() and srp_reset_device() fail for a QP in the error state and since srp_reset_host() fails after host removal has started an endless loop will be triggered.

Hence 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 if host removal has already started or if reconnecting fails.

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

Cc David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

Cc Roland Dreier <[email protected]>

Reported-by Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

Cc Alex Turin <alextu-VPRAkNaXOzVWk0Htik3J/[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +15 ~~-14~~

index 94f76b9..42b6286 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);

@@ -1736,7 +1737,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);

Signature · 6 lines

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Dillow

2012-12-14 15:55 UTC #7 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 11 lines · Bart Van Assche

If a SCSI command times out it is passed to the SCSI error handler. The SCSI error handler will try to abort the command that timed out. If aborting failed a device reset will be attempted. If the device reset fails too a host reset will be attempted. If the host reset also fails the whole procedure will be repeated.
Since srp_abort() and srp_reset_device() fail for a QP in the error state and since srp_reset_host() fails after host removal has started an endless loop will be triggered.

Quoted 6 lines

Hence 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 if host removal has already started or if reconnecting fails.

This is much more than your original patch that Alex claimed fixed his issues; are you not merging two separate issues?

Also, there's no reason to invoke srp_send_tsk_mgmt() if we're not connected or the QP is in error -- for those cases, it makes sense to just abort the command directly. Similarly, we should probably be checking the status of srp_send_tsk_mgmt() and failing -- or checking qp_in_error/connected again and directly aborting if we have problems.

No?

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

A message that is not in the archive (sent off-list or not yet received): 1355500552.18309.11.camel-zHLflQxYYDO4Hhoo1DtQwJ9G+ZOsUmrO@public.gmane.org

Bart Van Assche

author

2012-12-14 16:12 UTC #9 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 19 lines · David Dillow

Quoted 11 lines · Bart Van Assche
If a SCSI command times out it is passed to the SCSI error handler. The SCSI error handler will try to abort the command that timed out. If aborting failed a device reset will be attempted. If the device reset fails too a host reset will be attempted. If the host reset also fails the whole procedure will be repeated.
Since srp_abort() and srp_reset_device() fail for a QP in the error state and since srp_reset_host() fails after host removal has started an endless loop will be triggered.
Quoted 6 lines
Hence 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 if host removal has already started or if reconnecting fails.

Quoted 8 lines

This is much more than your original patch that Alex claimed fixed his issues; are you not merging two separate issues?
Also, there's no reason to invoke srp_send_tsk_mgmt() if we're not connected or the QP is in error -- for those cases, it makes sense to just abort the command directly. Similarly, we should probably be checking the status of srp_send_tsk_mgmt() and failing -- or checking qp_in_error/connected again and directly aborting if we have problems.

Hello Dave,

Thanks for the quick reply. You might have missed Vu's message though. Vu Pham reported that v1 of this patch did not fix the endless error handling loop (see e.g. http://www.mail-archive.com/[email protected]/msg13713.html).

As far as I know invoking srp_send_tsk_mgmt() if the QP is in error is harmless and won't even cause a delay.

The proposal to add a connection state test in srp_send_tsk_mgmt() makes sense to me. That would help to reduce the time spent in the SCSI error handler after an orderly target shutdown (when it sent a DREQ).

There is a reason the result status of srp_send_tsk_mgmt() is not checked in srp_abort(): if sending the task management command fails the next step of the SCSI error handler will be to perform a host reset. And a host reset will finish a request anyway, whether or not srp_abort() did.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Dillow

2012-12-14 16:19 UTC #11 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 10 lines · Bart Van Assche

Quoted 8 lines · David Dillow
This is much more than your original patch that Alex claimed fixed his issues; are you not merging two separate issues?
Also, there's no reason to invoke srp_send_tsk_mgmt() if we're not connected or the QP is in error -- for those cases, it makes sense to just abort the command directly. Similarly, we should probably be checking the status of srp_send_tsk_mgmt() and failing -- or checking qp_in_error/connected again and directly aborting if we have problems.

Quoted 6 lines

Hello Dave,
Thanks for the quick reply. You might have missed Vu's message though. Vu Pham reported that v1 of this patch did not fix the endless error handling loop (see e.g.
http://www.mail-archive.com/[email protected]/msg13713.html).

I saw that, but I also saw your message asking if he was sure he was running with your patch, and I never saw a public reply to clarify.

I saw a message from him yesterday that running your fixes branch did work, but with no posting of updated patches I assumed that was v1 still -- was he testing v2?

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

A message that is not in the archive (sent off-list or not yet received): 1355501996.18309.16.camel-zHLflQxYYDO4Hhoo1DtQwJ9G+ZOsUmrO@public.gmane.org

Bart Van Assche

author

2012-12-14 16:30 UTC #13 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 16 lines · David Dillow

Quoted 10 lines · Bart Van Assche
Quoted 8 lines · David Dillow
This is much more than your original patch that Alex claimed fixed his issues; are you not merging two separate issues?
Also, there's no reason to invoke srp_send_tsk_mgmt() if we're not connected or the QP is in error -- for those cases, it makes sense to just abort the command directly. Similarly, we should probably be checking the status of srp_send_tsk_mgmt() and failing -- or checking qp_in_error/connected again and directly aborting if we have problems.
Quoted 4 lines
Thanks for the quick reply. You might have missed Vu's message though. Vu Pham reported that v1 of this patch did not fix the endless error handling loop (see e.g.
http://www.mail-archive.com/[email protected]/msg13713.html).

Quoted 6 lines

I saw that, but I also saw your message asking if he was sure he was running with your patch, and I never saw a public reply to clarify.
I saw a message from him yesterday that running your fixes branch did work, but with no posting of updated patches I assumed that was v1 still -- was he testing v2?

Hello Dave,

There has been some off-list communication too in which Vu explained me that v1 was not sufficient but that v2 did help.

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

A message that is not in the archive (sent off-list or not yet received): [email protected]

Vu Pham

2012-12-14 18:14 UTC #15 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 24 lines · Bart Van Assche

Quoted 16 lines · David Dillow
Quoted 10 lines · Bart Van Assche
Quoted 8 lines · David Dillow
This is much more than your original patch that Alex claimed fixed his issues; are you not merging two separate issues?
Also, there's no reason to invoke srp_send_tsk_mgmt() if we're not connected or the QP is in error -- for those cases, it makes sense to just abort the command directly. Similarly, we should probably be checking the status of srp_send_tsk_mgmt() and failing -- or checking qp_in_error/connected again and directly aborting if we have problems.
Quoted 4 lines
Thanks for the quick reply. You might have missed Vu's message though. Vu Pham reported that v1 of this patch did not fix the endless error handling loop (see e.g.
http://www.mail-archive.com/[email protected]/msg13713.html).
Quoted 6 lines
I saw that, but I also saw your message asking if he was sure he was running with your patch, and I never saw a public reply to clarify.
I saw a message from him yesterday that running your fixes branch did work, but with no posting of updated patches I assumed that was v1 still -- was he testing v2?

Quoted 6 lines

Hello Dave,
There has been some off-list communication too in which Vu explained me that v1 was not sufficient but that v2 did help.
Bart.

Hello Dave,
To confirm what Bart said:

V1 did not solve the endless error handling loop
V2 together with this patch "Save and restore host_scribble during error handling -
http://www.mail-archive.com/[email protected]/msg17809.html" solves the scsi_remove_host hang and endless abort issues.

Hi Bart,

With V2, I saw that it took 90-240 seconds for I/Os fail-over (depending on the number of outstanding I/Os and the number of paths per physical port) I'm using default multipath.conf with "dev_loss_tmo 60" "fast_io_fail_tmo 10"

Is there way to control/configure the fail-over time?

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

David Dillow

2012-12-19 04:09 UTC #16 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

In reply to a message not in the archive

Quoted 11 lines · Bart Van Assche

Thanks for the quick reply. You might have missed Vu's message though. Vu Pham reported that v1 of this patch did not fix the endless error handling loop (see e.g.
http://www.mail-archive.com/[email protected]/msg13713.html).
As far as I know invoking srp_send_tsk_mgmt() if the QP is in error is harmless and won't even cause a delay.
The proposal to add a connection state test in srp_send_tsk_mgmt() makes sense to me. That would help to reduce the time spent in the SCSI error handler after an orderly target shutdown (when it sent a DREQ).

Did you update the patch? I think I'm on-board with the idea.

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

A message that is not in the archive (sent off-list or not yet received): 1355890164.23969.0.camel-1q1vX8mYZiGLUyTwlgNVppKKF0rrzTr+@public.gmane.org

Bart Van Assche

author

2012-12-19 14:15 UTC #18 lore raw

Re: [PATCH v2 2/2] IB/srp: Avoid endless SCSI error handling loop

Quoted 1 line · David Dillow

Did you update the patch? I think I'm on-board with the idea.

Sorry for the delay. I will post the updated patch series.

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