On 06/25/2012 04:14 PM, James Bottomley wrote: > On Mon, 2012-06-25 at 18:15 +0000, Bart Van Assche wrote: >> Since scsi_prep_fn() may be invoked concurrently with >> __scsi_remove_device(), keep the queuedata pointer in >> __scsi_remove_device(). This patch fixes a kernel oops that >> can be triggered by USB device removal. See also >> http://www.spinics.net/lists/linux-scsi/msg56254.html. > > This patch causes a subtle change of semantics: you're substituting our > signal for dead queue as !sdev with a check of blk_queue_dead(). > >> Reported-by: Jun'ichi Nomura <j-nomura@xxxxxxxxxxxxx> >> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx> >> Reviewed-by: Mike Christie <michaelc@xxxxxxxxxxx> >> Cc: James Bottomley <JBottomley@xxxxxxxxxxxxx> >> Cc: Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx> >> Cc: <stable@xxxxxxxxxx> >> --- >> drivers/scsi/hosts.c | 8 +++++++- >> drivers/scsi/scsi_lib.c | 35 ++++++++--------------------------- >> drivers/scsi/scsi_priv.h | 1 - >> drivers/scsi/scsi_sysfs.c | 5 +---- >> 4 files changed, 16 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c >> index a3a056a..6b9d89a 100644 >> --- a/drivers/scsi/hosts.c >> +++ b/drivers/scsi/hosts.c >> @@ -299,9 +299,15 @@ static void scsi_host_dev_release(struct device *dev) >> destroy_workqueue(shost->work_q); >> q = shost->uspace_req_q; >> if (q) { >> + /* >> + * Note: freeing queuedata before invoking blk_cleanup_queue() >> + * is safe here because no request function is associated with >> + * uspace_req_q. See also the __scsi_alloc_queue() call in >> + * drivers/scsi/scsi_tgt_lib.c. >> + */ > > This comment doesn't really make a whole lot of sense. What I think > it's saying is that it's OK for the commands executed by the drain in > blk_cleanup_queue to have a NULL queuedata and by the time we reach > this, there can be no concurrent racing calls to queuecommand? Is this > true, and why? Only the scsi_tgt_lib.c code uses the uses the uspace_req_q. That code does not use it in a traditional way. It passes __scsi_alloc_queue NULL for the request_fn function argument, so this request queue does not have a function that pulls requests off a queue like is done for the initiator path. That target code mostly uses the queue struct so that it can use the block/scsi functions that need a request queue to build scatterlists, cmds, bios, etc. When that code was made originally we were going to use the queue in a more tradition way or break out the queue limits into another struct and pass them around. I think we have the latter now, but the target code has not been converted. But so that is why we cannot hit a race like you are thinking about in the initiator path. It is also a weird use of the queue so it is also why it does not make sense :) -- 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
