On 05/30/12 17:27, Mike Christie wrote:
> It should be waiting now if the scsi_cmnd has a request backing
> shouldn't it? We will allocate a request struct with blk_get_request or
> one of the other blk helpers for each scsi_cmnd, and that will increment
> the q->rq.count. If we then go down the error path because a cmd timed
> out or because scsi_decide_disposition returned FAILED, then we will
> still have that request backing the scsi cmnd and the count should still
> be incremented for it. When we call scsi_send_eh_cmnd for eh operations
> the request is then still there and not freed yet. The request will get
> freed later when scsi_eh_flush_done_q is called. In there we will either
> retry or call scsi_finish_command which will go through the normal
> completion process and eventually call __blk_put_request and freed_request.
OK, that means that the counter manipulation code can be left out.
Skipping the queuecommand() call once device removal started is still
useful though since when not doing that scsi_remove_host() sometimes
takes much longer than expected. A call stack I obtained via echo w
>/proc/sysrq-trigger while scsi_remove_host() took longer than expected
is as follows:
[<ffffffff81404799>] schedule+0x29/0x70
[<ffffffff81063c55>] async_synchronize_cookie_domain+0x75/0x120
[<ffffffff8105c940>] ? wake_up_bit+0x40/0x40
[<ffffffff812c88dc>] ? __pm_runtime_resume+0x6c/0xa0
[<ffffffff81063d15>] async_synchronize_cookie+0x15/0x20
[<ffffffff81063d3c>] async_synchronize_full+0x1c/0x40
[<ffffffffa015aaf6>] sd_remove+0x36/0xc0 [sd_mod]
[<ffffffff812bce1c>] __device_release_driver+0x7c/0xe0
[<ffffffff812bd00f>] device_release_driver+0x2f/0x50
[<ffffffff812bc6cb>] bus_remove_device+0xfb/0x170
[<ffffffff812b97cd>] device_del+0x12d/0x1c0
[<ffffffffa003e714>] __scsi_remove_device+0xd4/0xe0 [scsi_mod]
[<ffffffffa003d10f>] scsi_forget_host+0x6f/0x80 [scsi_mod]
[<ffffffffa003266a>] scsi_remove_host+0x7a/0x130 [scsi_mod]
[<ffffffffa0564096>] srp_remove_target+0xa6/0x100 [ib_srp]
[<ffffffffa05642d4>] srp_remove_work+0x64/0x90 [ib_srp]
[<ffffffff81054f98>] process_one_work+0x1a8/0x530
[<ffffffff81054f29>] ? process_one_work+0x139/0x530
[<ffffffffa0564270>] ? srp_remove_one+0x180/0x180 [ib_srp]
[<ffffffff81056cea>] worker_thread+0x16a/0x350
[<ffffffff81056b80>] ? manage_workers+0x250/0x250
[<ffffffff8105c12e>] kthread+0xae/0xc0
[<ffffffff8140f514>] kernel_thread_helper+0x4/0x10
With the patch below these delays do not occur:
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 386f0c5..0d6ab69 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -791,14 +791,15 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
scsi_log_send(scmd);
scmd->scsi_done = scsi_eh_done;
- shost->hostt->queuecommand(shost, scmd);
-
- timeleft = wait_for_completion_timeout(&done, timeout);
-
+ if (sdev->sdev_state != SDEV_DEL &&
+ shost->hostt->queuecommand(shost, scmd) == 0) {
+ timeleft = wait_for_completion_timeout(&done, timeout);
+ scsi_log_completion(scmd, SUCCESS);
+ } else {
+ timeleft = 0;
+ }
shost->eh_action = NULL;
- scsi_log_completion(scmd, SUCCESS);
-
SCSI_LOG_ERROR_RECOVERY(3,
printk("%s: scmd: %p, timeleft: %ld\n",
__func__, scmd, timeleft));
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 42c35ff..f32757c 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -955,24 +955,30 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
void __scsi_remove_device(struct scsi_device *sdev)
{
struct device *dev = &sdev->sdev_gendev;
+ struct request_queue *q = sdev->request_queue;
if (sdev->is_visible) {
if (scsi_device_set_state(sdev, SDEV_CANCEL) != 0)
return;
- bsg_unregister_queue(sdev->request_queue);
+ bsg_unregister_queue(q);
device_unregister(&sdev->sdev_dev);
transport_remove_device(dev);
device_del(dev);
} else
put_device(&sdev->sdev_dev);
+
+ /*
+ * Stop accepting new requests and wait until all queuecommand()
+ * invocations have finished before tearing down the device.
+ */
scsi_device_set_state(sdev, SDEV_DEL);
+ blk_cleanup_queue(q);
+
if (sdev->host->hostt->slave_destroy)
sdev->host->hostt->slave_destroy(sdev);
transport_destroy_device(dev);
- /* Freeing the queue signals to block that we're done */
- blk_cleanup_queue(sdev->request_queue);
put_device(dev);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
