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

[Vasu Dev <vasu.dev@linux.intel.com>]

On Thu, 2010-09-16 at 17:13 -0700, Nicholas A. Bellinger wrote:
> On Thu, 2010-09-16 at 19:25 -0400, Christoph Hellwig wrote:
> > On Thu, Sep 16, 2010 at 05:24:47PM -0400, James Bottomley wrote:
> > > > 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.
> > > 
> > > Yes, that's basically what Christoph did when he moved the lock out of
> > > the eh path.
> > 
> > And it is what we should do here as well.  I'm just wondering if we rely
> > on the fact that we hold the lock over the check for the device beeing
> > deleted and the queuecommand call.
> > 
> 
> Ugh, I completely forgot the about the (host->shost_state == SHOST_DEL)
> check in scsi_dispatch_cmd() in patch #1, and just fixed this in
> lio-core-2.6.git/drop-host_lock here:
> 
> http://git.kernel.org/?p=linux/kernel/git/nab/lio-core-2.6.git;a=commitdiff;h=e222907d9baac56425f87c602acdd757dc5fde08
> 
> Here is what the updated scsi_dispatch_cmd() now looks like:
> 
> 	<SNIP>
> 
>         spin_lock_irqsave(host->host_lock, flags);
>         /*
>          * AK: unlikely race here: for some reason the timer could
>          * expire before the serial number is set up below.
>          *
>          * TODO: kill serial or move to blk layer
>          */
>         scsi_cmd_get_serial(host, cmd);
> 
>         if (unlikely(host->shost_state == SHOST_DEL)) {
>                 spin_unlock_irqrestore(host->host_lock, flags);
>                 cmd->result = (DID_NO_CONNECT << 16);
>                 scsi_done(cmd);
>         } else {
>                 spin_unlock_irqrestore(host->host_lock, flags);

This unconditional drop will require LLD still using host lock for their
queuecommand to re-acquire the lock and that would hurt their perf as
Joe mentioned in his response. Also this change requires all such LLD to
grab and drop lock again and that change should go along with this
change.

BTW above change is very similar to patch under discussion, just off by
tiny additional check :-)

   Vasu
>                 trace_scsi_dispatch_cmd_start(cmd);
>                 rtn = host->hostt->queuecommand(cmd, scsi_done);
>         }
> 
> 	<SNIP>
> 
> Thanks!
> 
> --nab
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html