SCSI LLDs, the SCSI error handler and host resource lifetime

PATCH Bart Van Assche 2 participants 5 messages Nov 20, 2012 – Nov 27, 2012

Subsystems (3)

SCSI InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Bart Van Assche

author maintainer

2012-11-20 14:24 UTC #0 lore raw

Hello,

If I interpret the SCSI error handler source code correctly then scsi_unjam_host() may proceed concurrently with scsi_remove_host(). This means that the LLD eh_abort_handler callback may get invoked after scsi_remove_host() finished. At least the SRP initiator (ib_srp) cleans up resources necessary for aborting commands as soon as scsi_remove_host() returns. That looks like a race condition to me. As far as I can see it is only safe to clean up such resources after the EH thread has been stopped. Any opinions about adding an additional callback for this purpose in struct scsi_host_template ?

Note: it doesn't look like a good idea to me to let scsi_remove_host() wait until error recovery has finished since scsi_remove_host() may get invoked from the context of a workqueue. If any work gets queued on the same workqueue related to SCSI error handling letting scsi_remove_host() wait for the error handler to finish might result in a deadlock.

The patch below is a request for comments patch that does not only add a callback to struct scsi_host_template but also fixes a (hard to trigger) race condition in ib_srp: avoid that ib_destroy_cm_id() frees the IB RC connection while srp_send_tsk_mgmt() is using it.

Thanks,

Bart.

---
 drivers/infiniband/ulp/srp/ib_srp.c |   13 ++++++++-----
 drivers/scsi/hosts.c                |    4 ++++
 include/scsi/scsi_host.h            |   10 ++++++++++
 3 files changed, 22 insertions(+), 5 deletions(-)

---

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

index 0d4d39b..406bb92 100644
@@ -504,10 +504,16 @@ static void srp_remove_work(struct work_struct *work)
 	srp_del_scsi_host_attr(target->scsi_host);
 	srp_remove_host(target->scsi_host);
 	scsi_remove_host(target->scsi_host);
+	scsi_host_put(target->scsi_host);
+}
+
+static void srp_host_release(struct Scsi_Host *shost)
+{
+	struct srp_target_port *target = host_to_target(shost);
+
 	ib_destroy_cm_id(target->cm_id);
 	srp_free_target_ib(target);
 	srp_free_req_data(target);
-	scsi_host_put(target->scsi_host);
 }
 
 static int srp_connect_target(struct srp_target_port *target)

@@ -1629,10 +1635,6 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	struct srp_iu *iu;
 	struct srp_tsk_mgmt *tsk_mgmt;
 
-	if (target->state == SRP_TARGET_DEAD ||
-	    target->state == SRP_TARGET_REMOVED)
-		return -1;
-
 	init_completion(&target->tsk_mgmt_done);
 
 	spin_lock_irq(&target->lock);

@@ -1859,6 +1861,7 @@ static struct scsi_host_template srp_template = {
 	.eh_abort_handler		= srp_abort,
 	.eh_device_reset_handler	= srp_reset_device,
 	.eh_host_reset_handler		= srp_reset_host,
+	.host_release			= srp_host_release,
 	.sg_tablesize			= SRP_DEF_SG_TABLESIZE,
 	.can_queue			= SRP_CMD_SQ_SIZE,
 	.this_id			= -1,

drivers/scsi/hosts.c +4 -0

index 593085a..e7554be 100644
@@ -296,6 +296,10 @@ static void scsi_host_dev_release(struct device *dev)
 
 	if (shost->ehandler)
 		kthread_stop(shost->ehandler);
+
+	if (shost->hostt->host_release)
+		shost->hostt->host_release(shost);
+
 	if (shost->work_q)
 		destroy_workqueue(shost->work_q);
 	q = shost->uspace_req_q;

include/scsi/scsi_host.h +10 -0

index 4908480..67f6684 100644
@@ -367,6 +367,16 @@ struct scsi_host_template {
 #define SCSI_ADAPTER_RESET	1
 #define SCSI_FIRMWARE_RESET	2
 
+	/*
+	 * Callback function invoked after the host device reference count
+	 * dropped to zero, after the SCSI EH thread has stopped and before
+	 * SCSI host resources are freed. Useful to free resources that are
+	 * needed by the eh_* callback functions. See also scsi_host_get() and
+	 * scsi_host_put().
+	 *
+	 * Status: OPTIONAL
+	 */
+	void (*host_release)(struct Scsi_Host *shost);
 
 	/*
 	 * Name of proc directory

Hannes Reinecke

2012-11-21 07:19 UTC #1 lore raw

Quoted 18 lines · Bart Van Assche

Hello,
If I interpret the SCSI error handler source code correctly then scsi_unjam_host() may proceed concurrently with scsi_remove_host(). This means that the LLD eh_abort_handler callback may get invoked after scsi_remove_host() finished. At least the SRP initiator (ib_srp) cleans up resources necessary for aborting commands as soon as scsi_remove_host() returns. That looks like a race condition to me. As far as I can see it is only safe to clean up such resources after the EH thread has been stopped. Any opinions about adding an additional callback for this purpose in struct scsi_host_template ?
Note: it doesn't look like a good idea to me to let scsi_remove_host() wait until error recovery has finished since scsi_remove_host() may get invoked from the context of a workqueue. If any work gets queued on the same workqueue related to SCSI error handling letting scsi_remove_host() wait for the error handler to finish might result in a deadlock.

Quoted 5 lines

The patch below is a request for comments patch that does not only add a callback to struct scsi_host_template but also fixes a (hard to trigger) race condition in ib_srp: avoid that ib_destroy_cm_id() frees the IB RC connection while srp_send_tsk_mgmt() is using it.

Hmm.
This would still mean that the eh thread will run until finished.
Which can take _A LOT_ of time (we're speaking hours here).
I would rather have an additional return code in the various scsi_try_XXX functions to terminate the loop quickly.

Cheers,

Hannes

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Bart Van Assche

author maintainer

2012-11-21 12:26 UTC #2 lore raw

Quoted 22 lines · Hannes Reinecke

Quoted 16 lines · Bart Van Assche
If I interpret the SCSI error handler source code correctly then scsi_unjam_host() may proceed concurrently with scsi_remove_host(). This means that the LLD eh_abort_handler callback may get invoked after scsi_remove_host() finished. At least the SRP initiator (ib_srp) cleans up resources necessary for aborting commands as soon as scsi_remove_host() returns. That looks like a race condition to me. As far as I can see it is only safe to clean up such resources after the EH thread has been stopped. Any opinions about adding an additional callback for this purpose in struct scsi_host_template ?
Note: it doesn't look like a good idea to me to let scsi_remove_host() wait until error recovery has finished since scsi_remove_host() may get invoked from the context of a workqueue. If any work gets queued on the same workqueue related to SCSI error handling letting scsi_remove_host() wait for the error handler to finish might result in a deadlock.
Quoted 5 lines
The patch below is a request for comments patch that does not only add a callback to struct scsi_host_template but also fixes a (hard to trigger) race condition in ib_srp: avoid that ib_destroy_cm_id() frees the IB RC connection while srp_send_tsk_mgmt() is using it.

Quoted 5 lines

Hmm.
This would still mean that the eh thread will run until finished. Which can take _A LOT_ of time (we're speaking hours here). I would rather have an additional return code in the various scsi_try_XXX functions to terminate the loop quickly.

How about combining both approaches ? I think the additional callback is needed anyway to prevent the race condition explained above. Making the SCSI EH stop quicker after scsi_remove_host() has been invoked looks like a good idea to me but I'm not sure that change alone is sufficient.

Bart.

Bart Van Assche

author maintainer

2012-11-26 17:23 UTC #3 lore raw

In reply to Hannes Reinecke

Quoted 5 lines · Hannes Reinecke

Hmm.
This would still mean that the eh thread will run until finished. Which can take _A LOT_ of time (we're speaking hours here). I would rather have an additional return code in the various scsi_try_XXX functions to terminate the loop quickly.

Hello Hannes,

Since I'm not sure that I understood you properly: is something like the patch below what you had in mind ?

Thanks,

Bart.

[PATCH] Skip invoking error handler once device removal started

---
 drivers/scsi/scsi_error.c |   12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/scsi/scsi_error.c +8 -4

index c1b05a8..697f7f2 100644
@@ -582,7 +582,8 @@ static int scsi_try_bus_reset(struct scsi_cmnd *scmd)
 	if (!hostt->eh_bus_reset_handler)
 		return FAILED;
 
-	rtn = hostt->eh_bus_reset_handler(scmd);
+	rtn = scsi_host_scan_allowed(host) ?
+		hostt->eh_bus_reset_handler(scmd) : FAST_IO_FAIL;
 
 	if (rtn == SUCCESS) {
 		if (!hostt->skip_settle_delay)

@@ -621,7 +622,8 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 	if (!hostt->eh_target_reset_handler)
 		return FAILED;
 
-	rtn = hostt->eh_target_reset_handler(scmd);
+	rtn =  scsi_host_scan_allowed(host) ?
+		hostt->eh_target_reset_handler(scmd) : FAST_IO_FAIL;
 	if (rtn == SUCCESS) {
 		spin_lock_irqsave(host->host_lock, flags);
 		__starget_for_each_device(scsi_target(scmd->device), NULL,

@@ -645,12 +647,14 @@ static int scsi_try_target_reset(struct scsi_cmnd *scmd)
 static int scsi_try_bus_device_reset(struct scsi_cmnd *scmd)
 {
 	int rtn;
-	struct scsi_host_template *hostt = scmd->device->host->hostt;
+	struct Scsi_Host *host = scmd->device->host;
+	struct scsi_host_template *hostt = host->hostt;
 
 	if (!hostt->eh_device_reset_handler)
 		return FAILED;
 
-	rtn = hostt->eh_device_reset_handler(scmd);
+	rtn = scsi_host_scan_allowed(host) ?
+		hostt->eh_device_reset_handler(scmd) : FAST_IO_FAIL;
 	if (rtn == SUCCESS)
 		__scsi_report_device_reset(scmd->device, NULL);
 	return rtn;

Signature · 1 line

1.7.10.4

Hannes Reinecke

2012-11-27 15:37 UTC #4 lore raw

Quoted 7 lines · Bart Van Assche

Quoted 5 lines · Hannes Reinecke
Hmm.
This would still mean that the eh thread will run until finished. Which can take _A LOT_ of time (we're speaking hours here). I would rather have an additional return code in the various scsi_try_XXX functions to terminate the loop quickly.

Quoted 5 lines

Hello Hannes,
Since I'm not sure that I understood you properly: is something like the patch below what you had in mind ?

Yeah, something like this.
Although the usage of 'scsi_host_scan_allowed()' is non-obvious; after all, we're not _scanning_ this host, we're just checking if it's working normally.

But thanks for doing this.
Cheers,

Hannes

Signature · 8 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html