Re: [PATCH] scsi, mptsas : drop scsi_host lock when calling mptsas_qcmd

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

On Thu, 2010-09-16 at 14:44 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 14:34 -0700, Nicholas A. Bellinger wrote:
> > On Thu, 2010-09-16 at 23:25 +0200, Andi Kleen wrote:
> > > > I asked James about getting Vasu's unlocked_qcmds=1 patch merged, but he
> > > > convinced me that doing conditional locking while is very simple, is not
> > > > the proper way for getting this resolved in mainline code.  I think in
> > > > the end this will require a longer sit down to do a wholesale conversion
> > > > of all existing SCSI LLD drivers, and identifing the broken ones that
> > > > still need a struct Scsi_Host->host_lock'ed SHT->queuecommand() for
> > > > whatever strange & legacy reasons.
> > > 
> > > The standard way to do that would be to first move the lock down
> > > into the drivers (similar to how it has been done with the BKL).
> > > This would be a fairly mechanic mindless patch. Lots of typing,
> > > but not really a lot of real code review needed.
> > > 
> > > Then next step the drivers who know they don't want it can remove it.
> > 
> > Well, the main bit is finding those drives that actually call some form
> > of struct Scsi_Host->host_lock unlock() -> do_work() -> lock() from
> > within their own ->queuecomamnd() caller, and avoid running into the big
> > ugly deadlock that would happen here..
> > 
> > I know that Open-iSCSi and my own iSCSI Initiator do this
> > "optimization", but I am not sure which other LLDs also do this as
> > well..
> > 
> > Best,
> > 
> 
> Ok, spotted another one of these in drivers/scsi/lpfc/lpfc_scsi.c:lpfc_queuecommand():
> 
> 	<SNIP>
> 
>         if (phba->cfg_poll & ENABLE_FCP_RING_POLLING) {
>                 spin_unlock(shost->host_lock);
>                 lpfc_sli_handle_fast_ring_event(phba,
>                         &phba->sli.ring[LPFC_FCP_RING], HA_R0RE_REQ);
> 
>                 spin_lock(shost->host_lock);
>                 if (phba->cfg_poll & DISABLE_FCP_RING_INT)
>                         lpfc_poll_rearm_timer(phba);
>         }
> 
>         return 0;
> 
> Best,

And ditto ->queuecommand() with qla2xxx and qla4xxx LLDs as well:

	<SNIP>

        spin_unlock_irq(ha->host->host_lock);

        srb = qla4xxx_get_new_srb(ha, ddb_entry, cmd, done);
        if (!srb)
                goto qc_host_busy_lock;

        rval = qla4xxx_send_command_to_isp(ha, srb);
        if (rval != QLA_SUCCESS)
                goto qc_host_busy_free_sp;

        spin_lock_irq(ha->host->host_lock);
        return 0;


So yeah, it looks like the high performance LLDs have adopted this trick
over the years..  :-)

Best,

--nab