[PATCH v2] IB/SRP patches for kernel 3.8

[PATCH v2] IB/SRP patches for kernel 3.8

[Bart Van Assche]

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

Signed-off-by: Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index d5088ce..94f76b9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Dillow]

On Fri, 2012-12-14 at 16:34 +0100, Bart Van Assche wrote:
> 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/g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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 <bvanassche-HInyCGIudOg@public.gmane.org>
Cc: David Dillow <dave-i1Mk8JYDVaaSihdK6806/g@public.gmane.org>
Cc: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
Reported-by: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Alex Turin <alextu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 94f76b9..42b6286 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Dillow]

On Fri, 2012-12-14 at 16:38 +0100, Bart Van Assche wrote:
> 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.

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 12/14/12 16:55, David Dillow wrote:
> On Fri, 2012-12-14 at 16:38 +0100, Bart Van Assche wrote:
>> 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.
>
> 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/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Dillow]

On Fri, 2012-12-14 at 17:12 +0100, Bart Van Assche wrote:
> On 12/14/12 16:55, David Dillow wrote:
> > 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/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 12/14/12 17:19, David Dillow wrote:
> On Fri, 2012-12-14 at 17:12 +0100, Bart Van Assche wrote:
>> On 12/14/12 16:55, David Dillow wrote:
>>> 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.
>>
>> 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/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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?

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Vu Pham]

Bart Van Assche wrote:
> On 12/14/12 17:19, David Dillow wrote:
>> On Fri, 2012-12-14 at 17:12 +0100, Bart Van Assche wrote:
>>> On 12/14/12 16:55, David Dillow wrote:
>>>> 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.
>>>
>>> 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/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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?
>
> 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/linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[David Dillow]

On Fri, 2012-12-14 at 17:12 +0100, Bart Van Assche wrote:
> 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/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Bart Van Assche]

On 12/19/12 05:09, David Dillow wrote:
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html