On Mon, 2012-06-25 at 17:05 -0500, Mike Christie wrote: > 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 :) OK, could we encapsulate all that in the comment so I don't have to ask again in a year's time ... ? Thanks, James -- 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
