[ofa-general][PATCH 2/4] SRP fail-over faster

[ofa-general][PATCH 2/4] SRP fail-over faster

[Vu Pham]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: [ofa-general][PATCH 2/4] SRP fail-over faster,.eml --]
[-- Type: message/rfc822, Size: 1800 bytes --]

[-- Attachment #2.1.1: Type: text/plain, Size: 110 bytes --]

Disconnect connection without waiting.

Signed-off-by: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>



[-- Attachment #2.1.2: srp_2_disconnect_without_wait.patch --]
[-- Type: text/plain, Size: 727 bytes --]

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

Index: ofed_kernel/drivers/infiniband/ulp/srp/ib_srp.c
===================================================================
--- ofed_kernel.orig/drivers/infiniband/ulp/srp/ib_srp.c
+++ ofed_kernel/drivers/infiniband/ulp/srp/ib_srp.c
@@ -421,7 +421,6 @@ static void srp_disconnect_target(struct
 			     PFX "Sending CM DREQ failed\n");
 		return;
 	}
-	wait_for_completion(&target->done);
 }
 
 static void srp_remove_work(struct work_struct *work)
@@ -1301,7 +1300,6 @@ static int srp_cm_handler(struct ib_cm_i
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "connection closed\n");
 
-		comp = 1;
 		target->status = 0;
 		break;
 

Re: [ofa-general][PATCH 2/4] SRP fail-over faster

[Roland Dreier]

 > -	wait_for_completion(&target->done);

How do you avoid leaking connection on module unload etc?  Don't we have
to wait for the disconnect to finish somewhere?

 - R.
--
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: [ofa-general][PATCH 2/4] SRP fail-over faster

[Vu Pham]

Roland Dreier wrote:
>  > -	wait_for_completion(&target->done);
>
> How do you avoid leaking connection on module unload etc?  Don't we have
> to wait for the disconnect to finish somewhere?
>
>  - R.
>   
Are you talking about cm_id?
I think that we wait because we want to reuse cq/qp associate with the 
connection.
On module unload, we destroy cm_id, cq, qp altogether.

-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: [ofa-general][PATCH 2/4] SRP fail-over faster

[Roland Dreier]

 > >  > -	wait_for_completion(&target->done);
 > >
 > > How do you avoid leaking connection on module unload etc?  Don't we have
 > > to wait for the disconnect to finish somewhere?
 > >
 > >  - R.
 > >   
 > Are you talking about cm_id?
 > I think that we wait because we want to reuse cq/qp associate with the
 > connection.
 > On module unload, we destroy cm_id, cq, qp altogether.

OK, I think I understand this piece locally (although if you delete the
wait_for_completion() it would probably make more sense to delete the
init_completion() just before it as well).

However is it really a correct to reuse a cm_id that is in state
timewait to initiate a new connection?  That seems pretty fragile to me.
--
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: [ofa-general][PATCH 2/4] SRP fail-over faster

[Vu Pham]

Roland Dreier wrote:
>  > >  > -	wait_for_completion(&target->done);
>  > >
>  > > How do you avoid leaking connection on module unload etc?  Don't we have
>  > > to wait for the disconnect to finish somewhere?
>  > >
>  > >  - R.
>  > >   
>  > Are you talking about cm_id?
>  > I think that we wait because we want to reuse cq/qp associate with the
>  > connection.
>  > On module unload, we destroy cm_id, cq, qp altogether.
>
> OK, I think I understand this piece locally (although if you delete the
> wait_for_completion() it would probably make more sense to delete the
> init_completion() just before it as well).
>
> However is it really a correct to reuse a cm_id that is in state
> timewait to initiate a new connection?  That seems pretty fragile to me.
>   
Understood. However, we have to wait (1 << max(timeout - 8, 0)) ms; 
therefore, we need to disconnect without wait
--
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: [ofa-general][PATCH 2/4] SRP fail-over faster

[Vu Pham]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

Roland Dreier wrote:
>  > >  > -	wait_for_completion(&target->done);
>  > >
>  > > How do you avoid leaking connection on module unload etc?  Don't we have
>  > > to wait for the disconnect to finish somewhere?
>  > >
>  > >  - R.
>  > >   
>  > Are you talking about cm_id?
>  > I think that we wait because we want to reuse cq/qp associate with the
>  > connection.
>  > On module unload, we destroy cm_id, cq, qp altogether.
>
> OK, I think I understand this piece locally (although if you delete the
> wait_for_completion() it would probably make more sense to delete the
> init_completion() just before it as well).
>
> However is it really a correct to reuse a cm_id that is in state
> timewait to initiate a new connection?  That seems pretty fragile to me.
>   
Here is the updated patch to remove both init_completion() and 
wait_for_completion()


[-- Attachment #2: srp_2_disconnect_without_wait.patch --]
[-- Type: text/plain, Size: 1094 bytes --]

Disconnect the connection without waiting (1 << max(timeout -8),0) ms

Signed-off-by: Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

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

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 54c8fe2..dfa14de 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -415,13 +415,11 @@ static void srp_disconnect_target(struct srp_target_port *target)
 {
 	/* XXX should send SRP_I_LOGOUT request */
 
-	init_completion(&target->done);
 	if (ib_send_cm_dreq(target->cm_id, NULL, 0)) {
 		shost_printk(KERN_DEBUG, target->scsi_host,
 			     PFX "Sending CM DREQ failed\n");
 		return;
 	}
-	wait_for_completion(&target->done);
 }
 
 static void srp_remove_work(struct work_struct *work)
@@ -1301,7 +1299,6 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "connection closed\n");
 
-		comp = 1;
 		target->status = 0;
 		break;