Skip to contentSkip to conversation

mpt2sas: Remove acquisition of host_lock

PATCH In review Matthew Wilcox 3 participants 4 messages Apr 5, 2011 – Apr 10, 2011 SCSI

lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

PATCH 1/2 mpt2sas: Remove acquisition of host_lock

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 <[email protected]>
drivers/scsi/mpt2sas/mpt2sas_scsih.c +3 -12
index 5ded3db..863218f 100644
@@ -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
#1 lore raw
Quoted 7 lines · 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.

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.

author
#2 lore raw
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Matthew Wilcox

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

Quoted 7 lines
Quoted 3 lines

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?

#3 lore raw
Quoted 26 lines · Matthew Wilcox
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Matthew Wilcox

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

Quoted 7 lines
Quoted 3 lines

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:

(quoted diff) +1 -2
-               if (scmd_lookup && (scmd_lookup->serial_number ==
-                   scmd->serial_number))
+               if (scmd_lookup)
                        rc = FAILED;
                else
                        rc = SUCCESS;
Quoted 5 lines

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.