Re: [RFC PATCH] scsi, fcoe, libfc: drop scsi host_lock use from fc_queuecommand

[Vasu Dev <vasu.dev-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Wed, 2010-09-01 at 13:10 -0700, Nicholas A. Bellinger wrote:
> 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()..
> 

fcoe/libfc moved to scsi_done w/o holding scsi host_lock a while ago
around dec, 09 and it was done after discussion with Mathew and Chris
Leech from fcoe side at that time, they may have more to comment on
this.

 
> 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.
> 

I fully agree on this approach. I had same intent with this patch to not
impact existing LLD doing queuecommand under host lock, having such LLD
to re-acquire this lock would pose same perf issue as fcoe is having now
since lock still needed inside scsi_dispatch_cmd for shost_state
checking as indicated above by Nab and Mike beside needed for
scsi_cmd_get_serial there.

I'll restore lock for shost_state and for this unlikely SHOST_DEL case
have this lock dropped later, however still have fc_queuecommand w/o
host lock held, so change would be as:-


diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ad0ed21..ce504e5 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -749,11 +749,16 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)
        if (unlikely(host->shost_state == SHOST_DEL)) {
                cmd->result = (DID_NO_CONNECT << 16);
                scsi_done(cmd);
+               spin_unlock_irqrestore(host->host_lock, flags);
        } else {
                trace_scsi_dispatch_cmd_start(cmd);
+               if (!host->unlocked_qcmds)
+                       spin_unlock_irqrestore(host->host_lock, flags);
                rtn = host->hostt->queuecommand(cmd, scsi_done);
+               if (host->unlocked_qcmds)
+                       spin_unlock_irqrestore(host->host_lock, flags);
        }
-       spin_unlock_irqrestore(host->host_lock, flags);
+
        if (rtn) {
                trace_scsi_dispatch_cmd_error(cmd, rtn);
                if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&


Maybe two additional checks here is not so neat but not too bad either
as just two additional checks here, I excluded this unlocked_qcmd checks
from scsi_error queuecommand calling to not clutter its code there with
these additional checks  w/o any good case for that code path.

> >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. 

Good idea.

>  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..?
> 

I'm also curious see more comments these good points. Thanks Nab for all
comments and opening up this patch for wider discussion, will help this
patch done sooner.

	Vasu

> Best,
> 
> --nab
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html