IB/srp patches for the stable tree

PATCH In review Bart Van Assche 3 participants 9 messages Oct 10, 2013 – Oct 25, 2013 5 messages not in the archive

Subsystems (2)

InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Bart Van Assche

author

2013-10-10 11:48 UTC #0 lore raw

cover IB/srp patches for the stable tree

The new rules for stable kernel tree patches require these patches to
be posted separately from patches introducing new features. Hence a short patch series with the following three patches:

0001-IB-srp-Remove-target-from-list-before-freeing-Scsi_H.patch 0002-IB-srp-Avoid-offlining-operational-SCSI-devices.patch 0003-IB-srp-Report-receive-errors-correctly.patch
--
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

2013-10-10 11:50 UTC #2 lore raw

PATCH 1/3 IB/srp: Remove target from list before freeing Scsi_Host structure

From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>

Remove an SRP target from the SRP target list before invoking the last scsi_host_put() call. This change is necessary because that last put frees the memory that holds the srp_target_port structure.

This patch prevents that the following kernel oops can be triggered:

RIP: 0010:[<ffffffff810b00d0>] __lock_acquire+0x500/0x1570
Call Trace:
 [<ffffffff810b11e4>] lock_acquire+0xa4/0x120
 [<ffffffff81531206>] _spin_lock+0x36/0x70
 [<ffffffffa01b6d8f>] srp_remove_work+0xef/0x180 [ib_srp]
 [<ffffffff8109125c>] worker_thread+0x21c/0x3d0
 [<ffffffff81096e86>] kthread+0x96/0xa0
 [<ffffffff8100c20a>] child_rip+0xa/0x20

Signed-off-by Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>

[bvanassche: Modified path description and CC'ed stable]

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

Cc David Dillow <[email protected]>

Cc Roland Dreier <[email protected]>

Cc Sebastian Riemer <[email protected]>

Cc Jack Wang <[email protected]>

Cc <[email protected]>

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

drivers/infiniband/ulp/srp/ib_srp.c +5 -4

index f93baf8..f1318a8 100644
@@ -534,6 +534,11 @@ static void srp_remove_target(struct srp_target_port *target)
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
+
+	spin_lock(&target->srp_host->target_lock);
+	list_del(&target->list);
+	spin_unlock(&target->srp_host->target_lock);
+
 	scsi_host_put(target->scsi_host);
 }

@@ -545,10 +550,6 @@ static void srp_remove_work(struct work_struct *work)
 	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 
 	srp_remove_target(target);
-
-	spin_lock(&target->srp_host->target_lock);
-	list_del(&target->list);
-	spin_unlock(&target->srp_host->target_lock);
 }
 
 static void srp_rport_delete(struct srp_rport *rport)

Signature · 6 lines

1.8.1.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]

Jack Wang

2013-10-10 12:45 UTC #4 lore raw

Re: [PATCH 1/3] IB/srp: Remove target from list before freeing Scsi_Host structure

Quoted 46 lines · Bart Van Assche

From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
Remove an SRP target from the SRP target list before invoking the last scsi_host_put() call. This change is necessary because that last put frees the memory that holds the srp_target_port structure.
This patch prevents that the following kernel oops can be triggered:
RIP: 0010:[<ffffffff810b00d0>] __lock_acquire+0x500/0x1570
Call Trace:
 [<ffffffff810b11e4>] lock_acquire+0xa4/0x120
 [<ffffffff81531206>] _spin_lock+0x36/0x70
 [<ffffffffa01b6d8f>] srp_remove_work+0xef/0x180 [ib_srp]
 [<ffffffff8109125c>] worker_thread+0x21c/0x3d0
 [<ffffffff81096e86>] kthread+0x96/0xa0
 [<ffffffff8100c20a>] child_rip+0xa/0x20
Signed-off-by Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
[bvanassche: Modified path description and CC'ed stable]
Signed-off-by Bart Van Assche <[email protected]>
Cc David Dillow <[email protected]>
Cc Roland Dreier <[email protected]>
Cc Sebastian Riemer <[email protected]>
Cc Jack Wang <[email protected]>
Cc <[email protected]>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +5 -0
index f93baf8..f1318a8 100644
@@ -534,6 +534,11 @@ static void srp_remove_target(struct srp_target_port *target)
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
+
+	spin_lock(&target->srp_host->target_lock);
+	list_del(&target->list);
+	spin_unlock(&target->srp_host->target_lock);
+
 	scsi_host_put(target->scsi_host);
 }
 

drivers/infiniband/ulp/srp/ib_srp.c static void srp_remove_work(struct work_struct *work) · line 553

Quoted 8 lines

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

@@ -545,10 +550,6 @@ static void srp_remove_work(struct work_struct *work)
 	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 
 	srp_remove_target(target);
-
-	spin_lock(&target->srp_host->target_lock);
-	list_del(&target->list);
-	spin_unlock(&target->srp_host->target_lock);
 }

Or move the 3 line code on top of srp_remove_target(target);? Patch looks good.

Jack

drivers/infiniband/ulp/srp/ib_srp.c static void srp_remove_work(struct work_struct *work) · line 555

Quoted 3 lines

drivers/infiniband/ulp/srp/ib_srp.c +0 -0
 static void srp_rport_delete(struct srp_rport *rport)
 

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

2013-10-10 12:48 UTC #6 lore raw

Re: [PATCH 1/3] IB/srp: Remove target from list before freeing Scsi_Host structure

Quoted 49 lines · Jack Wang

Quoted 48 lines · Bart Van Assche
From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
Remove an SRP target from the SRP target list before invoking the last scsi_host_put() call. This change is necessary because that last put frees the memory that holds the srp_target_port structure.
This patch prevents that the following kernel oops can be triggered:
RIP: 0010:[<ffffffff810b00d0>] __lock_acquire+0x500/0x1570
Call Trace:
  [<ffffffff810b11e4>] lock_acquire+0xa4/0x120
  [<ffffffff81531206>] _spin_lock+0x36/0x70
  [<ffffffffa01b6d8f>] srp_remove_work+0xef/0x180 [ib_srp]
  [<ffffffff8109125c>] worker_thread+0x21c/0x3d0
  [<ffffffff81096e86>] kthread+0x96/0xa0
  [<ffffffff8100c20a>] child_rip+0xa/0x20
Signed-off-by Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
[bvanassche: Modified path description and CC'ed stable]
Signed-off-by Bart Van Assche <[email protected]>
Cc David Dillow <[email protected]>
Cc Roland Dreier <[email protected]>
Cc Sebastian Riemer <[email protected]>
Cc Jack Wang <[email protected]>
Cc <[email protected]>
---
  drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +5 -0
index f93baf8..f1318a8 100644
@@ -534,6 +534,11 @@ static void srp_remove_target(struct srp_target_port *target)
  	ib_destroy_cm_id(target->cm_id);
  	srp_free_target_ib(target);
  	srp_free_req_data(target);
+
+	spin_lock(&target->srp_host->target_lock);
+	list_del(&target->list);
+	spin_unlock(&target->srp_host->target_lock);
+
  	scsi_host_put(target->scsi_host);
  }
 
@@ -545,10 +550,6 @@ static void srp_remove_work(struct work_struct *work)
  	WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 

Quoted 8 lines

drivers/infiniband/ulp/srp/ib_srp.c static void srp_remove_work(struct work_struct *work) · line 553
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c +0 -4
  	srp_remove_target(target);
-
-	spin_lock(&target->srp_host->target_lock);
-	list_del(&target->list);
-	spin_unlock(&target->srp_host->target_lock);
  }
Or move the 3 line code on top of srp_remove_target(target);?

That would break srp_conn_unique() - removal from the target list must only happen after a DREQ has been sent by the initiator to the target.

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]

Jack Wang

2013-10-10 13:42 UTC #8 lore raw

Re: [PATCH 1/3] IB/srp: Remove target from list before freeing Scsi_Host structure

Quoted 62 lines · Bart Van Assche

Quoted 52 lines · Jack Wang
Quoted 51 lines · Bart Van Assche
From: Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
Remove an SRP target from the SRP target list before invoking the last scsi_host_put() call. This change is necessary because that last put frees the memory that holds the srp_target_port structure.
This patch prevents that the following kernel oops can be triggered:
RIP: 0010:[<ffffffff810b00d0>] __lock_acquire+0x500/0x1570
Call Trace:
  [<ffffffff810b11e4>] lock_acquire+0xa4/0x120
  [<ffffffff81531206>] _spin_lock+0x36/0x70
  [<ffffffffa01b6d8f>] srp_remove_work+0xef/0x180 [ib_srp]
  [<ffffffff8109125c>] worker_thread+0x21c/0x3d0
  [<ffffffff81096e86>] kthread+0x96/0xa0
  [<ffffffff8100c20a>] child_rip+0xa/0x20
Signed-off-by Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
[bvanassche: Modified path description and CC'ed stable]
Signed-off-by Bart Van Assche <[email protected]>
Cc David Dillow <[email protected]>
Cc Roland Dreier <[email protected]>
Cc Sebastian Riemer <[email protected]>
Cc Jack Wang <[email protected]>
Cc <[email protected]>
---
  drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +5 -0
index f93baf8..f1318a8 100644
@@ -534,6 +534,11 @@ static void srp_remove_target(struct
 srp_target_port *target)
      ib_destroy_cm_id(target->cm_id);
      srp_free_target_ib(target);
      srp_free_req_data(target);
+
+    spin_lock(&target->srp_host->target_lock);
+    list_del(&target->list);
+    spin_unlock(&target->srp_host->target_lock);
+
      scsi_host_put(target->scsi_host);
  }
@@ -545,10 +550,6 @@ static void srp_remove_work(struct work_struct
 *work)
      WARN_ON_ONCE(target->state != SRP_TARGET_REMOVED);
 
Quoted 8 lines
drivers/infiniband/ulp/srp/ib_srp.c static void srp_remove_work(struct work_struct *work) · line 553
Quoted 6 lines
drivers/infiniband/ulp/srp/ib_srp.c +0 -4
      srp_remove_target(target);
-
-    spin_lock(&target->srp_host->target_lock);
-    list_del(&target->list);
-    spin_unlock(&target->srp_host->target_lock);
  }
Or move the 3 line code on top of srp_remove_target(target);?

Quoted 5 lines

That would break srp_conn_unique() - removal from the target list must only happen after a DREQ has been sent by the initiator to the target.
Bart.

You're right, this solution is better. It is possible to break srp_conn_unique() in my proposal.

Thanks,
Jack

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

2013-10-10 11:52 UTC #9 lore raw

PATCH 2/3 IB/srp: Avoid offlining operational SCSI devices

In reply to a message not in the archive

If SCSI commands are submitted with a SCSI request timeout that is lower than the the IB RC timeout it can happen that the SCSI error handler has already started device recovery before transport layer error handling starts. So it can happen that the SCSI error handler tries to abort a SCSI command after it has been reset by srp_rport_reconnect(). Tell the SCSI error handler that such commands have finished and that it is not necessary to continue its recovery strategy for commands that have been reset by srp_rport_reconnect().

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

Cc David Dillow <[email protected]>

Cc Roland Dreier <[email protected]>

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

Cc Sebastian Riemer <[email protected]>

Cc Jack Wang <[email protected]>

Cc <[email protected]>

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

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

index f1318a8..93a35a1 100644
@@ -1752,7 +1752,7 @@ static int srp_abort(struct scsi_cmnd *scmnd)
 	shost_printk(KERN_ERR, target->scsi_host, "SRP abort called\n");
 
 	if (!req || !srp_claim_req(target, req, scmnd))
-		return FAILED;
+		return SUCCESS;
 	if (srp_send_tsk_mgmt(target, req->index, scmnd->device->lun,
 			      SRP_TSK_ABORT_TASK) == 0)
 		ret = SUCCESS;

Signature · 6 lines

1.8.1.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]

Jack Wang

2013-10-10 12:31 UTC #11 lore raw

Re: [PATCH 2/3] IB/srp: Avoid offlining operational SCSI devices

Quoted 8 lines · Bart Van Assche

If SCSI commands are submitted with a SCSI request timeout that is lower than the the IB RC timeout it can happen that the SCSI error handler has already started device recovery before transport layer error handling starts. So it can happen that the SCSI error handler tries to abort a SCSI command after it has been reset by srp_rport_reconnect(). Tell the SCSI error handler that such commands have finished and that it is not necessary to continue its recovery strategy for commands that have been reset by srp_rport_reconnect().

Another possible is srp has already finish the req (req is NULL)when scsi error handle call into srp_abort.

I tested it on my side, works for me.
You can add my Tested-by if needed.

Thanks Bart.

Jack

Quoted 18 lines

Signed-off-by Bart Van Assche <[email protected]>
Cc David Dillow <[email protected]>
Cc Roland Dreier <[email protected]>
Cc Vu Pham <vuhuong-VPRAkNaXOzVWk0Htik3J/[email protected]>
Cc Sebastian Riemer <[email protected]>
Cc Jack Wang <[email protected]>
Cc <[email protected]>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/ulp/srp/ib_srp.c +0 -0
index f1318a8..93a35a1 100644

drivers/infiniband/ulp/srp/ib_srp.c static int srp_abort(struct scsi_cmnd *scmnd) · line 1758

Quoted 8 lines

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

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

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

2013-10-10 11:53 UTC #12 lore raw

PATCH 3/3 IB/srp: Report receive errors correctly

In reply to a message not in the archive

The IB spec does not guarantee that the opcode is available in error completions. Hence do not rely on it. See also commit 948d1e88 ("IB/srp: Introduce srp_handle_qp_err()").

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

Cc David Dillow <[email protected]>

Cc Roland Dreier <[email protected]>

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

Cc Sebastian Riemer <[email protected]>

Cc <[email protected]> # v3.8

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

drivers/infiniband/ulp/srp/ib_srp.c +4 -5

index 93a35a1..17b58f4 100644
@@ -1303,14 +1303,13 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 			     PFX "Recv failed with error code %d\n", res);
 }
 
-static void srp_handle_qp_err(enum ib_wc_status wc_status,
-			      enum ib_wc_opcode wc_opcode,
+static void srp_handle_qp_err(enum ib_wc_status wc_status, bool send_err,
 			      struct srp_target_port *target)
 {
 	if (target->connected && !target->qp_in_error) {
 		shost_printk(KERN_ERR, target->scsi_host,
 			     PFX "failed %s status %d\n",
-			     wc_opcode & IB_WC_RECV ? "receive" : "send",
+			     send_err ? "send" : "receive",
 			     wc_status);
 	}
 	target->qp_in_error = true;

@@ -1326,7 +1325,7 @@ static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)
 		if (likely(wc.status == IB_WC_SUCCESS)) {
 			srp_handle_recv(target, &wc);
 		} else {
-			srp_handle_qp_err(wc.status, wc.opcode, target);
+			srp_handle_qp_err(wc.status, false, target);
 		}
 	}
 }

@@ -1342,7 +1341,7 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 			iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
 			list_add(&iu->list, &target->free_tx);
 		} else {
-			srp_handle_qp_err(wc.status, wc.opcode, target);
+			srp_handle_qp_err(wc.status, true, target);
 		}
 	}
 }

Signature · 6 lines

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

David Dillow

Acked-by

2013-10-25 22:16 UTC #13 lore raw

In reply to a message not in the archive

Quoted 7 lines · Bart Van Assche

The new rules for stable kernel tree patches require these patches to be posted separately from patches introducing new features. Hence a short patch series with the following three patches:
0001-IB-srp-Remove-target-from-list-before-freeing-Scsi_H.patch 0002-IB-srp-Avoid-offlining-operational-SCSI-devices.patch 0003-IB-srp-Report-receive-errors-correctly.patch

Acked-by David Dillow <[email protected]>

for all three. Thanks Bart!

Signature · 9 lines

Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office

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