[PATCH scsi-misc-2.6 08/08] scsi: fix hot unplug sequence

[Tejun Heo]

Hello, James and Jens.

Let me guess, it is hanging in wait_for_completion()?

Yes, I have the trace now. Why is curious. This is the trace of the
Mar 24 18:40:34 localhost kernel: usb 4-2: USB disconnect, address 3
Mar 24 18:40:34 localhost kernel: sd 0:0:0:0: CMD c25c98b0 done, completing
Mar 24 18:40:34 localhost kernel: 0:0:0:0: cmd c25c98b0 returning
Mar 24 18:40:34 localhost kernel: 0:0:0:0: cmd c25c98b0 going out <6>Read Capacity (10) 25 00 00 00 00 00 00 00 00 00
Mar 24 18:40:34 localhost kernel: scsi0 (0:0): rejecting I/O to dead device (req c25c98b0)
Mar 24 18:40:34 localhost kernel: usb 4-2: new full speed USB device using uhci_hcd and address 4
Mar 24 18:40:34 localhost kernel: scsi1 : SCSI emulation for USB Mass Storage devices
Mar 24 18:40:34 localhost kernel: 1:0:0:0: cmd c1a1b4b0 going out <6>Inquiry 12 00 00 00 24 00
The problem occurs when the mid-layer rejects the I/O to the dead
device. Here it returns BLKPREP_KILL to the prep function, but after
that we never get a completion back.

I think I found the cause. Special requests submitted using
scsi_do_req() never initializes ->end_io(). Normally, SCSI midlayer
terminates special requests inside the SCSI midlayer without passing
through the blkdev layer. However, if a device is going away or taken
offline, blkdev layer gets to terminate special requests and, as
->end_io() is never set-up, nothing happens and the completion gets
lost.

The following patch implements scsi_do_req_endio() and sets up
->end_io() and ->end_io_data before sending out special commands.
It's a quick fix & hacky. I think the proper fix might be one of

* Don't return BLKPREP_KILL in the prep_fn and always terminate
special commands inside request_fn without using end_that_*
functions.

* I don't really know why the scsi_request/scsi_cmnd distincion
is made (resource usage?), but, if it's a legacy thing, replace
scsi_request with scsi_cmnd; then we can BLKPREP_KILL without using
dummy scsi_cmnd.

Signed-off-by: Tejun Heo <***@gmail.com>

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2005/03/25 11:57:25+09:00 ***@htj.dyndns.org
# midlayer special command termination fix
#
# drivers/scsi/scsi_lib.c
# 2005/03/25 11:57:17+09:00 ***@htj.dyndns.org +30 -0
# midlayer special command termination fix
#
diff -Nru a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c 2005-03-25 11:59:28 +09:00
+++ b/drivers/scsi/scsi_lib.c 2005-03-25 11:59:28 +09:00
@@ -274,6 +274,33 @@
}

/*
+ * Special requests usually gets terminated inside scsi midlayer
+ * proper; however, they can be terminated by the blkdev layer when
+ * scsi_prep_fn() returns BLKPREP_KILL or scsi_request_fn() detects
+ * offline condition. The following callback is invoked when the
+ * blkdev layer terminates a special request. Emulate DID_NO_CONNECT.
+ */
+static void scsi_do_req_endio(struct request *rq)
+{
+ struct scsi_request *sreq = rq->end_io_data;
+ struct request_queue *q = sreq->sr_device->request_queue;
+ struct scsi_cmnd cmd;
+
+ memset(&cmd, 0, sizeof(cmd));
+ cmd.device = sreq->sr_device;
+ scsi_init_cmd_from_req(&cmd, sreq);
+ /* Our command is dummy, nullify back link. */
+ sreq->sr_command = NULL;
+
+ sreq->sr_result = cmd.result = DID_NO_CONNECT << 16;
+
+ /* The sreq->done() callback expects queue_lock to be unlocked. */
+ spin_unlock(q->queue_lock);
+ cmd.done(&cmd);
+ spin_lock(q->queue_lock);
+}
+
+/*
* Function: scsi_do_req
*
* Purpose: Queue a SCSI request
@@ -326,6 +353,9 @@

if (sreq->sr_cmd_len == 0)
sreq->sr_cmd_len = COMMAND_SIZE(sreq->sr_cmnd[0]);
+
+ sreq->sr_request->end_io = scsi_do_req_endio;
+ sreq->sr_request->end_io_data = sreq;

/*
* head injection *required* here otherwise quiesce won't work
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------

[James Bottomley]

I fully agree that doing done() correctly _is_ a problem, especially when
the SCSI subsystem evolves and the high-level driver writers do not follow
the development closely enough.
One solution to these problems would be to let the drivers still use
scsi_do_req() and their own done() function, but create two
- one to be called at the beginning of done(); it would do what needs to
be done here but lets the driver to do some special things of its own if
necessary
- one to be called to wait for the request to finish
(- one to do scsi_ro_req() and the things necessary before these)

Yes. The drivers that use it just need visiting with a big hammer.
However, our character ULDs (st and sg) use it because they try to
simulate fire and forget in the async write path (That's the only time
you actually don't wait on completion for scsi_do_req).

This comes about because the mid-layer is block oriented, so you can't
use any of the read/write machinery. We could fix this by having a
generic character tap to a block queue for use in cases like SCSI where
the underlying driver uses block queues even if the actual device isn't
a block device.

Essentially, the character tap would simply submit a stream of reads and
writes through the block queue. Then we could modify st and sg to use
an identical framework to the other ULDs ... you get a setup API and a
returning command API which are called for every I/O and the block layer
gets to handle the async/not-async pieces.

James

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/



-------------------------------------------------------------------------------
Achtung: diese Newsgruppe ist eine unidirektional gegatete Mailingliste.
Antworten nur per Mail an die im Reply-To-Header angegebene Adresse.
Fragen zum Gateway -> ***@inka.de.
-------------------------------------------------------------------------------