[PATCH 1/2] mpt2sas: Remove acquisition of host_lock

[PATCH 1/2] mpt2sas: Remove acquisition of host_lock

[Matthew Wilcox]

We can eliminate the use of the scsi command serial_number, as the race
that the driver is checking for cannot happen.

Then the driver no longer needs to use the DEF_SCSI_QCMD() macro and no
longer acquires the host_lock.  This improves performance substantially
on high-IOPS workloads.

Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 5ded3db..863218f 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -2031,7 +2031,6 @@ mpt2sas_scsih_issue_tm(struct MPT2SAS_ADAPTER *ioc, u16 handle, uint channel,
 	u16 smid = 0;
 	u32 ioc_state;
 	unsigned long timeleft;
-	struct scsi_cmnd *scmd_lookup;
 	int rc;
 
 	mutex_lock(&ioc->tm_cmds.mutex);
@@ -2132,12 +2131,7 @@ mpt2sas_scsih_issue_tm(struct MPT2SAS_ADAPTER *ioc, u16 handle, uint channel,
 		goto bypass_sanity_checks;
 	switch (type) {
 	case MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK:
-		scmd_lookup = _scsih_scsi_lookup_get(ioc, smid_task);
-		if (scmd_lookup && (scmd_lookup->serial_number ==
-		    scmd->serial_number))
-			rc = FAILED;
-		else
-			rc = SUCCESS;
+		rc = SUCCESS;
 		break;
 
 	case MPI2_SCSITASKMGMT_TASKTYPE_TARGET_RESET:
@@ -3358,16 +3352,15 @@ _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(struct Scsi_Host *shost, struct scsi_cmnd *scmd)
 {
-	struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device->host);
+	struct MPT2SAS_ADAPTER *ioc = shost_priv(shost);
 	struct MPT2SAS_DEVICE *sas_device_priv_data;
 	struct MPT2SAS_TARGET *sas_target_priv_data;
 	Mpi2SCSIIORequest_t *mpi_request;
 	u32 mpi_control;
 	u16 smid;
 
-	scmd->scsi_done = done;
 	sas_device_priv_data = scmd->device->hostdata;
 	if (!sas_device_priv_data || !sas_device_priv_data->sas_target) {
 		scmd->result = DID_NO_CONNECT << 16;
@@ -3484,8 +3477,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *))
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
-static DEF_SCSI_QCMD(_scsih_qcmd)
-
 /**
  * _scsih_normalize_sense - normalize descriptor and fixed format sense data
  * @sense_buffer: sense data returned by target

Re: [PATCH 1/2] mpt2sas: Remove acquisition of host_lock

[Christoph Hellwig]

On Tue, Apr 05, 2011 at 05:43:55PM -0400, Matthew Wilcox wrote:
> 
> We can eliminate the use of the scsi command serial_number, as the race
> that the driver is checking for cannot happen.
> 
> Then the driver no longer needs to use the DEF_SCSI_QCMD() macro and no
> longer acquires the host_lock.  This improves performance substantially
> on high-IOPS workloads.


Looks fine.  Note that this somehow clashes with my patch to simply
remove the serial_number check from mpt2sas.  We could just drop my
smaller patch if this one gets in in a timely fashion.

Re: [PATCH 1/2] mpt2sas: Remove acquisition of host_lock

[Matthew Wilcox]

On Wed, Apr 06, 2011 at 09:45:55AM -0400, Christoph Hellwig wrote:
> On Tue, Apr 05, 2011 at 05:43:55PM -0400, Matthew Wilcox wrote:
> > 
> > We can eliminate the use of the scsi command serial_number, as the race
> > that the driver is checking for cannot happen.
> > 
> > Then the driver no longer needs to use the DEF_SCSI_QCMD() macro and no
> > longer acquires the host_lock.  This improves performance substantially
> > on high-IOPS workloads.
> 
> Looks fine.  Note that this somehow clashes with my patch to simply
> remove the serial_number check from mpt2sas.  We could just drop my
> smaller patch if this one gets in in a timely fashion.

We should probably split this patch apart into the serial_number removal
and then the host_lock removal anyway.

But I don't think your patch is correct:

-               if (scmd_lookup && (scmd_lookup->serial_number ==
-                   scmd->serial_number))
+               if (scmd_lookup)
                        rc = FAILED;
                else
                        rc = SUCCESS;

The second part of the conditional is always false (right?  because that
command can't be in flight).

So that's (scmd_lookup && 0), which is if (0), so we can just state rc
= SUCCESS.  Or is my reasoning faulty somewhere?

RE: [PATCH 1/2] mpt2sas: Remove acquisition of host_lock

[Moore, Eric]

On Wednesday, April 06, 2011 11:31 PM, Matthew Wilcox wrote: 
> On Wed, Apr 06, 2011 at 09:45:55AM -0400, Christoph Hellwig wrote:
> > On Tue, Apr 05, 2011 at 05:43:55PM -0400, Matthew Wilcox wrote:
> > >
> > > We can eliminate the use of the scsi command serial_number, as the race
> > > that the driver is checking for cannot happen.
> > >
> > > Then the driver no longer needs to use the DEF_SCSI_QCMD() macro and no
> > > longer acquires the host_lock.  This improves performance substantially
> > > on high-IOPS workloads.
> >
> > Looks fine.  Note that this somehow clashes with my patch to simply
> > remove the serial_number check from mpt2sas.  We could just drop my
> > smaller patch if this one gets in in a timely fashion.
> 
> We should probably split this patch apart into the serial_number removal
> and then the host_lock removal anyway.
> 
> But I don't think your patch is correct:
> 
> -               if (scmd_lookup && (scmd_lookup->serial_number ==
> -                   scmd->serial_number))
> +               if (scmd_lookup)
>                         rc = FAILED;
>                 else
>                         rc = SUCCESS;
> 
> The second part of the conditional is always false (right?  because that
> command can't be in flight).
> 
> So that's (scmd_lookup && 0), which is if (0), so we can just state rc
> = SUCCESS.  Or is my reasoning faulty somewhere?


The patch that Christoph provided is correct.   We are doing this check following the task management completion. The purpose of this check is to determine whether the outstanding IO was completed.   For whatever reason, if the scmd_lookup is non-zero, it means the IO is still in driver queue. It's not guaranteed that the IO is returned by the task abort.  So we need to return FAILED status if the scmd_lookup is non-zero.  

The serial_number check is there if IO is resent while error recovery is going on, it appears I merged that in from the older generation mptsas driver. It's not needed since unjam host locks down the IO queues.