On Wed, 2010-09-01 at 00:57 -0700, Zou, Yi wrote:
> >
> > On 08/31/2010 06:56 PM, Nicholas A. Bellinger wrote:
> > >> + if (host->unlocked_qcmds)
> > >> + spin_unlock_irqrestore(host->host_lock, flags);
> > >> +
> > >> if (unlikely(host->shost_state == SHOST_DEL)) {
> > >> cmd->result = (DID_NO_CONNECT<< 16);
> > >> scsi_done(cmd);
> > >
> > > I don't think it's safe to call scsi_done() for the SHOST_DEL case here
> > > with host->unlocked_qcmds=1 w/o holding host_lock, nor would it be safe
> > > for the SCSI LLD itself using host->unlocked_qcmds=1 to call the
> > > (*scsi_done)() being passed into sht->queuecommand() without
> > > host->host_lock being held by either the SCSI ML or the SCSI LLD.
> >
> > The host state should be checked under the host lock, but I do not think
> > it needs to be held with calling scsi_done. scsi_done just queues up the
> > request to be completed in the block softirq, and the block layer
> > protects against something like the command getting completed by
> > multiple code paths/threads.
>
> It looks safe to me to call scsi_done() w/o host_lock held,
Hmmmm, this indeed this appears to be safe now.. For some reason I had
it in my head (and in TCM_Loop virtual SCSI LLD code as well) that
host_lock needed to be held while calling struct scsi_cmnd->scsi_done().
I assume this is some old age relic from the BLK days in the SCSI
completion path, and the subsequent conversion. I still see a couple of
ancient drivers in drivers/scsi/ that are still doing this, but I
believe I stand corrected in that (all..?) of the modern in-use
drivers/scsi code is indeed *not* holding host_lock while calling struct
scsi_cmnd->scsi_done()..
In that case, I will prepare a patch for TCM_Loop v4 and post it to
linux-scsi. Thanks for the info..!
> in which case,
> there is probably no need for the flag unlocked_qcmds, but just move the
> spin_unlock_ireqrestore() up to just after scsi_cmd_get_serial(), and let
> queuecommand() decide when/where if it wants to grab&drop the host lock, where
> in the case for fc_queuecomamnd(), we won't grab it at all. Just a thought...
>
Yes, but many existing SCSI LLD's SHT->queuecommand() depends upon
unlocking the host_lock being held, but I don't know how many actually
need to do this to begin with...?
I think initially this patch would need to be able to run the
'optimized' path first with a SCSI LLD like an FCoE or iSCSI software
initiator that knows that SHT->queuecommand() is not held, but still
allow existing LLDs that expect to unlock and lock struct
Scsi_Host->host_lock themselves internally do not immediately all break
and deadlock terribly.
>From that point we could discuss for a v2 patch about converting
everything single LLD queuecommand() caller to not directly touch
host_lock, unless they have some bizarre reason for doing so. Again,
this is assume that calling SHT->queuecommand() is safe to begin with,
and there are not cases of interaction by the LLDs in
SHT->queuecommand() that when accessing struct Scsi_Host require the
host_lock to be held.
James and Co, any comments here..?
Best,
--nab
--
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
