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

Tejun Heo
2005-03-25T03:15:41Z

Hello, James and Jens.

On Thu, Mar 24, 2005 at 06:45:58PM -0600, James Bottomley wrote:
> On Wed, 2005-03-23 at 16:25 +0100, Jens Axboe wrote:
> > 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
> failure:
>
> 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 <htejun@gmail.com>

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
# 2005/03/25 11:57:25+09:00 tj@htj.dyndns.org
# midlayer special command termination fix
#
# drivers/scsi/scsi_lib.c
# 2005/03/25 11:57:17+09:00 tj@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