On Thu, 2010-11-18 at 18:13 -0500, Jeff Garzik wrote:
> On 11/18/2010 05:57 PM, Nicholas A. Bellinger wrote:
> > <Trimming CC list>
> >
> > On Thu, 2010-11-18 at 12:15 +0200, Boaz Harrosh wrote:
> >> On 11/18/2010 12:19 AM, Nicholas A. Bellinger wrote:
> >>> From: Nicholas Bellinger<nab@xxxxxxxxxxxxxxx>
> >>>
> >>> This patch converts the mpt2sas driver to run in host_lock less mode
> >>> with the new IRQ_DISABLE_SCSI_QCMD() that disables interrupts while
> >>> calling ->queuecommand() dispatch
> >>>
> >>> Signed-off-by: Nicholas A. Bellinger<nab@xxxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++---
> >>> 1 files changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> index 1a96a00..e564fe7 100644
> >>> --- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> +++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
> >>> @@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >>> }
> >>>
> >>> /**
> >>> - * _scsih_qcmd - main scsi request entry point
> >>> + * _scsih_qcmd_irq_disable - main scsi request entry point
> >>> * @scmd: pointer to scsi command object
> >>> * @done: function pointer to be invoked on completion
> >>> *
> >>> @@ -3315,7 +3315,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
> >>> * SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full
> >>> */
> >>> static int
> >>> -_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >>> +_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >>> {
> >>> struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
> >>> struct MPT2SAS_DEVICE *sas_device_priv_data;
> >>> @@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
> >>> return SCSI_MLQUEUE_HOST_BUSY;
> >>> }
> >>>
> >>> -static DEF_SCSI_QCMD(_scsih_qcmd)
> >>> +static IRQ_DISABLE_SCSI_QCMD(_scsih_qcmd)
> >>>
> >>
> >> How can this (and other in the patchset) can be correct? I mean I expect
> >> that if you remove the lock,xx_qcmd_lck,unlock then inside the xx_qcmd_lck
> >> there was an unlock,do123,lock and that driver was effectively running lockless
> >> before. (like in iscsi). But here this is new behaviour. If it is correct
> >> I would like to see a statement from you that:
> >> "I have audited this driver, and all shared resources are protected by
> >> XYZ so ..."
> >>
> >> Otherwise how can I know this is correct? I have never audited this driver myself
> >>
> >
> > So for this specific mpt2sas case, Vasu Dev had been testing the
> > lock_less case w/o disabling interrupts for his original
> > SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable
> > during his JBOD lock_less small block IOP performance test.
>
> Test results are not the same as reviewing and understanding the locking
> in the driver, and adjusting the code accordingly...
Fair enough..
>
> _scsih_qcmd_lck() seems to rely on per-device and per-host data not
> changing, which might seem to imply that the driver is secretly using
> the SCSI host_lock to guarantee access to members such as
> ioc->ioc_link_reset_in_progress or sas_target_priv_data->tm_busy.
>
> These accesses appear racy in both lock-free and local_irq_save()
> configurations from my naive reading, though I do see additional locking
> inside mpt2sas_base_get_smid_scsiio()
>
Mmmm, so in that case I will go ahead and revert this mpt2sas patch in
lock_less-LLDs-for-38-v2 until this can be addressed by the LSI folks,
or by other folks who have an specific interest in mpt2sas being able to
run in lock_less mode for .38 code (Tim and Vasu..?)
Also, mpt2sas and message/fusion transport handlers are similar enough
that there are some likely issues there as well. Thus far this other
patch to drivers/message/fusion/mptscsih.c has been stable in VMW guest
in my development testing, but this will need to be audited for
lock_less as well.
--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
