Re: [RFC v4 10/19] lpfc: Remove host_lock unlock() + lock() fromlpfc_queuecommand()

[Nicholas A. Bellinger]

On Tue, 2010-09-28 at 00:04 -0500, Mike Christie wrote:
> On 09/27/2010 09:06 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger<nab@xxxxxxxxxxxxxxx>
> >
> > This patch removes the now legacy host_lock unlock() + lock() optimization
> > from lpfc_scsi.c:lpfc_queuecommand().  This also includes setting the
> > SHT->unlocked_qcmd=1 for host_lock less lpfc lpfc_queuecommand() operation.
> >
> > Signed-off-by: Nicholas A. Bellinger<nab@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/scsi/lpfc/lpfc_scsi.c |    4 ++--
> >   1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
> > index 2e51aa6..69fe31e 100644
> > --- a/drivers/scsi/lpfc/lpfc_scsi.c
> > +++ b/drivers/scsi/lpfc/lpfc_scsi.c
> > @@ -3023,11 +3023,9 @@ lpfc_queuecommand(struct scsi_cmnd *cmnd, void (*done) (struct scsi_cmnd *))
> >   		goto out_host_busy_free_buf;
> >   	}
> >   	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);
> >   	}
> > @@ -3723,6 +3721,7 @@ struct scsi_host_template lpfc_template = {
> >   	.slave_destroy		= lpfc_slave_destroy,
> >   	.scan_finished		= lpfc_scan_finished,
> >   	.this_id		= -1,
> > +	.unlocked_qcmd		= 1,
> >   	.sg_tablesize		= LPFC_DEFAULT_SG_SEG_CNT,
> >   	.cmd_per_lun		= LPFC_CMD_PER_LUN,
> >   	.use_clustering		= ENABLE_CLUSTERING,
> > @@ -3746,6 +3745,7 @@ struct scsi_host_template lpfc_vport_template = {
> >   	.slave_destroy		= lpfc_slave_destroy,
> >   	.scan_finished		= lpfc_scan_finished,
> >   	.this_id		= -1,
> > +	.unlocked_qcmd		= 1,
> >   	.sg_tablesize		= LPFC_DEFAULT_SG_SEG_CNT,
> >   	.cmd_per_lun		= LPFC_CMD_PER_LUN,
> >   	.use_clustering		= ENABLE_CLUSTERING,
> 
> The FC class sets the rport state and bits with the host lock held. 
> Drivers were then calling fc_remote_port_chkready from the queuecommand 
> with the host lock held. If we remove the host lock from queuecommand is 
> it possible that the on one proc the fc class calls fc_remote_port_add 
> to re-add a rport, this sets the rport state to online, it unblocks the 
> devices, but then on some other processor we start calling queuecommand 
> and see that the rport is not online (maybe blocked with 
> FC_RPORT_FAST_FAIL_TIMEDOUT set) and so we end up failing the IO?

Hey Mike,

So it sounds like we have two options here:

*) Add a per struct fc_rport lock to protect rport->port_state in
fc_remote_port_chkready() (and other places..?) that assume they will
be held under host_lock.  Unfortuately fc_remote_port_chkready() does
not mention the hard requirement for host_lock held usage, so I assume
other callers will not either.. :-(

*) Drop the lockless ->queuecommand() patches for LLD users of
fc_remote_port_chkready() for now and use the legacy ->queuecommand() ->
unlock -> do_lld_work() -> lock optimization.  Here is what that list
currently looks like in drivers/scsi:

drivers/scsi/bfa/bfad_im.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/bfa/bfad_im.c:	rc = fc_remote_port_chkready(rport);
drivers/scsi/fnic/fnic_main.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/fnic/fnic_scsi.c:	ret = fc_remote_port_chkready(rport);
drivers/scsi/fnic/fnic_scsi.c:	if (fc_remote_port_chkready(rport) == 0)
drivers/scsi/fnic/fnic_scsi.c:	if (fc_remote_port_chkready(rport))
drivers/scsi/ibmvscsi/ibmvfc.c:	if (unlikely((rc = fc_remote_port_chkready(rport))) ||
drivers/scsi/ibmvscsi/ibmvfc.c:	if (unlikely(rc || (rport && (rc = fc_remote_port_chkready(rport)))) ||
drivers/scsi/ibmvscsi/ibmvfc.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/libfc/fc_fcp.c:	rval = fc_remote_port_chkready(rport);
drivers/scsi/libfc/fc_fcp.c:	rval = fc_remote_port_chkready(rport);
drivers/scsi/libfc/fc_fcp.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/lpfc/lpfc_scsi.c:	err = fc_remote_port_chkready(rport);
drivers/scsi/lpfc/lpfc_scsi.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/qla2xxx/qla_os.c:	rval = fc_remote_port_chkready(rport);
drivers/scsi/qla2xxx/qla_os.c:	if (!rport || fc_remote_port_chkready(rport))
drivers/scsi/scsi_transport_fc.c:	 * the port_state or flags, so that fc_remote_port_chkready will

So what you think we should do here..?

Also, does anyone know if any of the same type of host_lock held
assumptions are also made by libsas and/or libata code..?

Best,

--nab



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/