mpt2sas: Remove queuecommand wrapper

-----Original Message-----
From: Matthew Wilcox [mailto:[email protected]]
Sent: Tuesday, July 05, 2011 12:57 AM
To: [email protected]
Cc: DL-MPT Fusion Linux
Subject: [PATCH] mpt2sas: Remove queuecommand wrapper

Now that mpt2sas no longer uses the serial_number, we can remove the wrapper that acquires the host_lock around queuecommand. This is the most contended lock for one benchmark, and on a 8-socket machine this patch leads to a 70% application-level improvement.

Signed-off-by Matthew Wilcox <[email protected]>

drivers/scsi/mpt2sas/mpt2sas_scsih.c +2 -2

index a7dbc68..67708b3 100644
@@ -3680,9 +3680,9 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER
 *ioc, struct scsi_cmnd *scmd,
  * 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;

drivers/scsi/mpt2sas/mpt2sas_scsih.c +0 -1

 	struct MPT2SAS_TARGET *sas_target_priv_data;
 	struct _raid_device *raid_device;

@@ -3690,7 +3690,6 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
 (*done)(struct scsi_cmnd *))
 	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) {

drivers/scsi/mpt2sas/mpt2sas_scsih.c +0 -2

 		scmd->result = DID_NO_CONNECT << 16;

@@ -3814,8 +3813,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

Matthew, I am fine with the patch, but just wanted to double check we do have included changes related to below conversation. Mpt2sas and mptsas driver wants IRQ and preemption disable when it enter in to qcmd callback.

http://140.211.166.79/mailarchive/linux-scsi/2010/12/23/6887919

I have gone through another round of code walkthrough (for mpt2sas and mptfusion) to \
find out dependency w.r.t new host lock less mode. In my opinion, w/ interrupts \
disable is a good ideal. (Earlier, I mentioned that mpt2sas is safe even if \
interrupts are enabled) In real scenario, *preemption* disable is more IMP for \
mpt2sas/mptfusion driver than interrupts disable.

e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
(Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
pending Scsi command to mid-layer, and once control come back to original \
scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
or anything critical.....)

Matthew, I am fine with the patch, but just wanted to double check we do have included changes related to below conversation. Mpt2sas and mptsas driver wants IRQ and preemption disable when it enter in to qcmd callback.

http://140.211.166.79/mailarchive/linux-scsi/2010/12/23/6887919

I have gone through another round of code walkthrough (for mpt2sas and mptfusion) to \
find out dependency w.r.t new host lock less mode. In my opinion, w/ interrupts \
disable is a good ideal. (Earlier, I mentioned that mpt2sas is safe even if \
interrupts are enabled) In real scenario, *preemption* disable is more IMP for \
mpt2sas/mptfusion driver than interrupts disable.

e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
(Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
pending Scsi command to mid-layer, and once control come back to original \
scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
or anything critical.....)

Quoted 8 lines

Quoted 7 lines

e.a if scsih_qcmd_xxx has executed half of the code and (due to preemption is not \
disable for the same CPU), Scheduler can execute any other process on the SAME cpu \
(Though IRQ is disable). Consider Error handling is kicked off on the same CPU and as \
part of EH, it executed HBA reset. As part of HBA reset Driver has return back all \
pending Scsi command to mid-layer, and once control come back to original \
scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data corruption/h/w hung \
or anything critical.....)

Quoted 3 lines

I don't believe this scenario can happen. Error handling will not commence until all commands are returned to the midlayer, which requires that queuecommand is no longer running.

I was able to see this scenario when I intially started doing code review. Since host_lock is removed Error handling can remove code from queuecommand anytime ( I asssume preemption is not disable, only IRQ is disable).. I am almost sure if we are planning to remove host_lock from SML, we at least need irq and preemption disable for mpt2sas and mptfusion to work properly.

-----Original Message-----
From: Matthew Wilcox [mailto:[email protected]]
Sent: Tuesday, July 05, 2011 8:52 PM
To: Desai, Kashyap
Cc: Matthew Wilcox; [email protected]; DL-MPT Fusion Linux; [email protected]
Subject: Re: [PATCH] mpt2sas: Remove queuecommand wrapper

Quoted 1 line · Kashyap

Quoted 1 line

Quoted 1 line

e.a if scsih_qcmd_xxx has executed half of the code and (due to

preemption is not \

Quoted 1 line

Quoted 1 line

Quoted 1 line

disable for the same CPU), Scheduler can execute any other process

on the SAME cpu \

Quoted 1 line

Quoted 1 line

Quoted 1 line

(Though IRQ is disable). Consider Error handling is kicked off on

the same CPU and as \

Quoted 1 line

Quoted 1 line

Quoted 1 line

part of EH, it executed HBA reset. As part of HBA reset Driver has

return back all \

Quoted 1 line

Quoted 1 line

Quoted 1 line

pending Scsi command to mid-layer, and once control come back to

original \

Quoted 1 line

Quoted 1 line

Quoted 1 line

scsih_qcmd_xxx LLD drive will see bad results (Kernel crash/Data

corruption/h/w hung \

Quoted 4 lines

Quoted 4 lines

Quoted 1 line

or anything critical.....)

I don't believe this scenario can happen. Error handling will not commence until all commands are returned to the midlayer, which

requires

Quoted 3 lines

Quoted 1 line

that queuecommand is no longer running.

I was able to see this scenario when I intially started doing code

review. Since host_lock is removed Error handling can remove code from queuecommand anytime ( I asssume preemption is not disable, only IRQ is disable).. I am almost sure if we are planning to remove host_lock from SML, we at least need irq and preemption disable for mpt2sas and mptfusion to work properly.

I'm pretty sure that can't happen. Look:

void scsi_eh_wakeup(struct Scsi_Host *shost)

{
        if (shost->host_busy == shost->host_failed) {

                trace_scsi_eh_wakeup(shost);
                wake_up_process(shost->ehandler);

So the SCSI error handler doesn't run until host_busy == host_failed. host_busy is incremented in scsi_request_fn() (before scsi_dispatch_cmd is called). host_failed is incremented in scsi_eh_scmd_add(). So after a command has been issued, it prevents the EH from running until it too fails. That means queuecommand cannot be running at the same time as the EH.

That's documented in Documentation/scsi/scsi_eh.txt section [1-3].

Quoted 11 lines

-----Original Message-----
From: Matthew Wilcox [mailto:[email protected]]
Sent: Tuesday, July 05, 2011 8:52 PM
To: Desai, Kashyap
Cc: Matthew Wilcox; [email protected]; DL-MPT Fusion Linux; [email protected]
Subject: Re: [PATCH] mpt2sas: Remove queuecommand wrapper

void scsi_eh_wakeup(struct Scsi_Host *shost)

{
        if (shost->host_busy == shost->host_failed) {

I think you are right here.

I did below experiment.
Since now host_lock is removed from dispatch() callback.
I added sleep() inside scsih_qcmd().. Just to mimic preemption. And called Host reset using
"Sg_reset -h" at the sametime when driver is sleeping inside qcmd(). I see the kernel crash due to invalid scsi command pointer access after driver comes out from sleep().

As you said, due to actual Error handling will not wake up, but if user/customer do testing using sg_tools They can kick off Host reset on demand.

What is your input on my above test case/observation ?

Quoted 11 lines

                trace_scsi_eh_wakeup(shost);
                wake_up_process(shost->ehandler);

So the SCSI error handler doesn't run until host_busy == host_failed. host_busy is incremented in scsi_request_fn() (before scsi_dispatch_cmd is called). host_failed is incremented in scsi_eh_scmd_add(). So after a command has been issued, it prevents the EH from running until it too fails. That means queuecommand cannot be running at the same time as the EH.

That's documented in Documentation/scsi/scsi_eh.txt section [1-3].

Quoted 4 lines · Kashyap

I did below experiment.
Since now host_lock is removed from dispatch() callback.
I added sleep() inside scsih_qcmd().. Just to mimic preemption. And called Host reset using
"Sg_reset -h" at the sametime when driver is sleeping inside qcmd(). I see the kernel crash due to invalid scsi command pointer access after driver comes out from sleep().

OK, but I don't think we've introduced a new race here, just expanded the window.

If you follow the path down from userspace, we get to scsi_nonblockable_ioctl() (doesn't take the host_lock), then scsi_reset_provider (which takes and releases the host_lock to set tmf_in_progress), then scsi_try_host_reset() which calls eh_host_reset_handler() without holding the host_lock.

So it's entirely possible to hit the same race on an SMP machine already as there's nothing to synchronise the eh_host_reset_handler call with queuecommand.

I think someone who uses sg_reset ought to be aware of the possibility that they can trash their system.  Doug, what do you think?