SCSI LLDs, the SCSI error handler and host resource lifetime

SCSI LLDs, the SCSI error handler and host resource lifetime

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

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(-)
---
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 0d4d39b..406bb92 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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,
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 593085a..e7554be 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -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;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 4908480..67f6684 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -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

Re: SCSI LLDs, the SCSI error handler and host resource lifetime

[Hannes Reinecke]

On 11/20/2012 03:24 PM, Bart Van Assche wrote:
> 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.
>
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
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: SCSI LLDs, the SCSI error handler and host resource lifetime

[Bart Van Assche]

On 11/21/12 08:19, Hannes Reinecke wrote:
> On 11/20/2012 03:24 PM, Bart Van Assche wrote:
>> 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.
>>
> 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.

Re: SCSI LLDs, the SCSI error handler and host resource lifetime

[Bart Van Assche]

On 11/21/12 08:19, Hannes Reinecke wrote:
> 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(-)

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index c1b05a8..697f7f2 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -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;
-- 
1.7.10.4

Re: SCSI LLDs, the SCSI error handler and host resource lifetime

[Hannes Reinecke]

On 11/26/2012 06:23 PM, Bart Van Assche wrote:
> On 11/21/12 08:19, Hannes Reinecke wrote:
>> 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 ?
>
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

-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html