mpt2sas: Convert to host_lock less w/ interrupts disabled externally
PATCH 6/11 mpt2sas: Convert to host_lock less w/ interrupts disabled externally
From: Nicholas Bellinger <[email protected]>
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
--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/scsi/mpt2sas/mpt2sas_scsih.c +3 -3
@@ -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)/** * _scsih_normalize_sense - normalize descriptor and fixed format sense data
Signature · 1 line
1.7.2.3
Quoted 34 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch converts the mpt2sas driver to run in
host_lockless mode with the newIRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger <[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)drivers/scsi/mpt2sas/mpt2sas_scsih.c +2
-2@@ -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;
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1@@ -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
Thanks
Boaz
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0/** * _scsih_normalize_sense - normalize descriptor and fixed format sense data
<Trimming CC list>
Quoted 47 lines · Boaz Harrosh
Quoted 34 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch converts the mpt2sas driver to run in
host_lockless mode with the newIRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger <[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)drivers/scsi/mpt2sas/mpt2sas_scsih.c +2
-2@@ -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;drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1@@ -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_lckthere 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:
Quoted 5 lines
"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.
This patch currently includes IRQ_DISABLE_SCSI_QCMD() usage which I am not even sure is needed (according to Vasu's original findings), but I decided to err on the conservative side until we can get some feedback from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
--nab
Quoted 53 lines · Nicholas A. Bellinger
<Trimming CC list>
Quoted 50 lines · Boaz Harrosh
Quoted 34 lines · Nicholas A. Bellinger
From: Nicholas Bellinger<[email protected]>
This patch converts the mpt2sas driver to run in
host_lockless mode with the newIRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger<[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)drivers/scsi/mpt2sas/mpt2sas_scsih.c +2
-2@@ -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;drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1@@ -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_lckthere 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 ..."
Quoted 7 lines
Quoted 2 lines
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_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable during his JBODlock_lesssmall block IOP performance test.
Test results are not the same as reviewing and understanding the locking in the driver, and adjusting the code accordingly...
_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()
Jeff
Quoted 57 lines · Jeff Garzik
Quoted 56 lines · Nicholas A. Bellinger
<Trimming CC list>
Quoted 52 lines · Boaz Harrosh
Quoted 34 lines · Nicholas A. Bellinger
From: Nicholas Bellinger<[email protected]>
This patch converts the mpt2sas driver to run in
host_lockless mode with the newIRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger<[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)drivers/scsi/mpt2sas/mpt2sas_scsih.c +2
-2@@ -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;drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1@@ -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_lckthere 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
Quoted 7 lines
Quoted 4 lines
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable during his JBODlock_lesssmall 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..
Quoted 10 lines
_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 SCSIhost_lockto guarantee access to members such asioc->ioc_link_reset_in_progressorsas_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 insidempt2sas_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
Quoted 76 lines
-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Friday, November 19, 2010 4:28 AM
To: Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externally<Trimming CC list>
Quoted 4 lines · Boaz Harrosh
Quoted 3 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch converts the mpt2sas driver to run in
host_locklessmode
Quoted 9 lines
Review
Quoted 9 lines
with the new
IRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger <[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/scsi/mpt2sas/mpt2sas_scsih.cQuoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 4 lines
index 1a96a00..e564fe7 100644
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd*scmd, u16
ioc_status)Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1}/** -* _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)Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-1* 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*))Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3318
Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-0+_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0{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*))Quoted 8 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1return 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
Quoted 1 line
that if you remove the lock,xx_qcmd_lck,unlock then inside the
xx_qcmd_lckQuoted 1 line
there was an unlock,do123,lock and that driver was effectively
running lockless
Quoted 1 line
before. (like in iscsi). But here this is new behaviour. If it is
correct
Quoted 2 lines
I would like to see a statement from you that:
"I have audited this driver, and all shared resources areprotected by
Quoted 2 lines
XYZ so ..."
Quoted 9 lines
Quoted 1 line
Otherwise how can I know this is correct? I have never audited this
driver myself
Quoted 1 line
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable during his JBODlock_lesssmall block IOP performance test.
Nicholas, mpt2sas driver is safe even if in lock less condition.
I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver
I have seen static DEF_SCSI_QCMD(_scsih_qcmd) is available for mpt2sas driver. I also did testing removing that particular macro and making
Mpt2sas driver in actual host lock less mode. Below observation is after removing static DEF_SCSI_QCMD(_scsih_qcmd) from mpt2sas (that is what I refer as actual host lock less mode)
I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for IRQ_DISABLE_SCSI_QCMD() patch too. Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.
I do not see any solid need for this patch. Please correct me if I am missing anything here.
~ Kashyap
Quoted 7 lines
This patch currently includes
IRQ_DISABLE_SCSI_QCMD()usage which I am not even sure is needed (according to Vasu's original findings), but I decided to err on the conservative side until we can get some feedback from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).--nab
Quoted 90 lines · Kashyap
Quoted 76 lines
-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Friday, November 19, 2010 4:28 AM
To: Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externally<Trimming CC list>
Quoted 4 lines · Boaz Harrosh
Quoted 3 lines · Nicholas A. Bellinger
From: Nicholas Bellinger<[email protected]>
This patch converts the mpt2sas driver to run in
host_locklessmode
Quoted 9 lines
Review
Quoted 9 lines
with the new
IRQ_DISABLE_SCSI_QCMD()that disables interrupts while calling ->queuecommand() dispatchSigned-off-by Nicholas A. Bellinger<[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/scsi/mpt2sas/mpt2sas_scsih.cQuoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 4 lines
index 1a96a00..e564fe7 100644
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd*scmd, u16
ioc_status)Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1}/** -* _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)Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-1* 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*))Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3318
Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-0+_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0{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*))Quoted 8 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1return 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
Quoted 1 line
that if you remove the lock,xx_qcmd_lck,unlock then inside the
xx_qcmd_lckQuoted 1 line
there was an unlock,do123,lock and that driver was effectively
running lockless
Quoted 1 line
before. (like in iscsi). But here this is new behaviour. If it is
correct
Quoted 2 lines
I would like to see a statement from you that:
"I have audited this driver, and all shared resources areprotected by
Quoted 2 lines
XYZ so ..."Quoted 9 lines
Quoted 1 line
Otherwise how can I know this is correct? I have never audited this
driver myself
Quoted 1 line
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode was stable during his JBODlock_lesssmall block IOP performance test.Nicholas, mpt2sas driver is safe even if in lock less condition.
I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver
I have seen staticDEF_SCSI_QCMD(_scsih_qcmd)is available for mpt2sas driver. I also did testing removing that particular macro and making
Mpt2sas driver in actual host lock less mode. Below observation is after removing staticDEF_SCSI_QCMD(_scsih_qcmd)from mpt2sas (that is what I refer as actual host lock less mode)
Quoted 4 lines
I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for
IRQ_DISABLE_SCSI_QCMD()patch too. Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.I do not see any solid need for this patch. Please correct me if I am missing anything here.
Which locks are held when setting or testing sas_target_priv_data->tm_busy? ioc->ioc_link_reset_in_progress?sas_target_priv_data->deleted?
Thanks,
Jeff
Quoted 118 lines
-----Original Message-----
From: Jeff Garzik [mailto:[email protected]] On Behalf Of Jeff Garzik
Sent: Friday, November 19, 2010 10:09 PM
To: Desai, Kashyap
Cc: Nicholas A. Bellinger; Boaz Harrosh; linux-scsi; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externallyQuoted 7 lines · Kashyap
Quoted 7 lines
-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Friday, November 19, 2010 4:28 AM
To: Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig;Mike
Quoted 6 lines
Quoted 6 lines
Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externally<Trimming CC list>
Quoted 7 lines
Quoted 7 lines · Boaz Harrosh
Quoted 4 lines · Nicholas A. Bellinger
Quoted 3 lines
From: Nicholas Bellinger<[email protected]>
This patch converts the mpt2sas driver to run in
host_locklessmode
Quoted 1 line
Quoted 1 line
with the new
IRQ_DISABLE_SCSI_QCMD()that disables interruptswhile
Quoted 28 lines
Quoted 28 lines
Quoted 8 lines
Review
Quoted 8 lines
calling ->queuecommand() dispatch
Signed-off-by Nicholas A. Bellinger<[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/scsi/mpt2sas/mpt2sas_scsih.cQuoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 4 lines
index 1a96a00..e564fe7 100644
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd*scmd, u16
ioc_status)Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1}/** -* _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)Quoted 4 lines
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-1* SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full */ static int -_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(structQuoted 36 lines
Quoted 5 lines
scsi_cmnd*))Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3318
Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-0+_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 2 lines
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0{ struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-host);
Quoted 30 lines
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0struct MPT2SAS_DEVICE *sas_device_priv_data;@@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 8 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1return 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
Quoted 1 line
that if you remove the lock,xx_qcmd_lck,unlock then inside the
xx_qcmd_lckQuoted 1 line
there was an unlock,do123,lock and that driver was effectively
running lockless
Quoted 1 line
before. (like in iscsi). But here this is new behaviour. If it is
correct
Quoted 2 lines
I would like to see a statement from you that:
"I have audited this driver, and all shared resources areprotected by
Quoted 3 lines
XYZ so ..."Otherwise how can I know this is correct? I have never audited this
driver myself
Quoted 1 line
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode wasstable
Quoted 4 lines
Quoted 2 lines
during his JBOD
lock_lesssmall block IOP performance test.Nicholas, mpt2sas driver is safe even if in lock less condition. I have tested mpt2sas driver available in 2.6.37-rc2 where I have
seen host lock push down code is available for mpt2sas driver
Quoted 1 line
I have seen static
DEF_SCSI_QCMD(_scsih_qcmd)is available formpt2sas driver. I also did testing removing that particular macro and making
Quoted 1 line
Mpt2sas driver in actual host lock less mode. Below observation is
after removing static
DEF_SCSI_QCMD(_scsih_qcmd)from mpt2sas (that is what I refer as actual host lock less mode)Quoted 2 lines
I have tested stability of the driver and I/O integrity. Things were
fine. I am not seeing any need for even going for
IRQ_DISABLE_SCSI_QCMD()patch too.Quoted 1 line
Mpt2sas driver is able to handle host lock less mode seamlessly,
since we have all required (racy) places under spinlocks.
Quoted 1 line
Quoted 8 lines
Quoted 1 line
I do not see any solid need for this patch. Please correct me if I am
missing anything here.
Which locks are held when setting or testing sas_target_priv_data-
Quoted 1 line
tm_busy?
ioc->ioc_link_reset_in_progress?sas_target_priv_data->deleted?
Jeff, tm_busy, ioc_link_reset_in_process and deleted field of respective data structure are not so critical. They are not exactly real-time, since they all are representing firmware state for the Host and Target. (It means there is always some open window when firmware state and driver data structure will not match) e.atm_busy is getting set to 1 when firmware sends "MPI2_EVENT_SAS_DEV_STAT_RC_INTERNAL_DEVICE_RESET". Even if we secure tm_busy read/write using some spinlock, it will not give any benefit, since there is already some delay when driver sets tm_busy and there is already some window in which few IOs will be sent to firmware. But main reason for driver is to stop sending IOs to firmware when firmware is doing Task management. There is no harm if few commands go to firmware when task mangagement is running by firmware. (This is also unavoidable to not send single IO to firmware immediately after it sends MPI2_EVENT_SAS_DEV_STAT_RC_INTERNAL_DEVICE_RESET)
So my call is all three field of relative structure are not having any dependency with host_lock. They are additional sanity check before sending IOs to firmware and even if we sends few IOs in a small open windows (considering there is some open windows before setting variable at driver), it is fine.
~ Kashyap
Quoted 5 lines
Thanks,
Jeff
Nicholas,
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.....)
So I would suggest to disable preemption also in your macro " IRQ_DISABLE_SCSI_QCMD " as below two level of prevention.
local_irq_save(flags);
preempt_disable();mpt2sas parameters checks like ioc_link_reset_in_process,tm_busy, scs_priv_target_data->deleted are mainly to communicate device/IOC state of the firmware so there is no hard dependency w.r.t host_lock.
Thanks, Kashyap
Quoted 128 lines
-----Original Message-----
From: Desai, Kashyap
Sent: Friday, November 19, 2010 4:33 PM
To: Nicholas A. Bellinger; Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux
Subject: RE: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externallyQuoted 9 lines
-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Friday, November 19, 2010 4:28 AM
To: Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externallyQuoted 9 lines
<Trimming CC list>
Quoted 4 lines · Boaz Harrosh
Quoted 3 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch converts the mpt2sas driver to run in
host_locklessmode
Quoted 1 line
Quoted 1 line
with the new
IRQ_DISABLE_SCSI_QCMD()that disables interruptswhile
Quoted 28 lines
Quoted 8 lines
Review
Quoted 8 lines
calling ->queuecommand() dispatch
Signed-off-by Nicholas A. Bellinger <[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/scsi/mpt2sas/mpt2sas_scsih.cQuoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 4 lines
index 1a96a00..e564fe7 100644
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd*scmd, u16
ioc_status)Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1}/** -* _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)Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-1* SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full */ static int -_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(structQuoted 8 lines
scsi_cmnd*))Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3318
Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-0+_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3320
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0{ struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-host);
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd,
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0struct MPT2SAS_DEVICE *sas_device_priv_data;@@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd,void
Quoted 21 lines
(*done)(struct
scsi_cmnd*))Quoted 8 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1return 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
Quoted 1 line
that if you remove the lock,xx_qcmd_lck,unlock then inside the
xx_qcmd_lckQuoted 1 line
there was an unlock,do123,lock and that driver was effectively
running lockless
Quoted 1 line
before. (like in iscsi). But here this is new behaviour. If it is
correct
Quoted 2 lines
I would like to see a statement from you that:
"I have audited this driver, and all shared resources areprotected by
Quoted 2 lines
XYZ so ..."Quoted 7 lines
Quoted 1 line
Otherwise how can I know this is correct? I have never audited this
driver myself
Quoted 1 line
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode wasstable
Quoted 2 lines
during his JBOD
lock_lesssmall block IOP performance test.Nicholas, mpt2sas driver is safe even if in lock less condition. I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver
I have seen staticDEF_SCSI_QCMD(_scsih_qcmd)is available for mpt2sas driver. I also did testing removing that particular macro and making Mpt2sas driver in actual host lock less mode. Below observation is after removing staticDEF_SCSI_QCMD(_scsih_qcmd)from mpt2sas (that is what I refer as actual host lock less mode)I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for
IRQ_DISABLE_SCSI_QCMD()patch too.
Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.I do not see any solid need for this patch. Please correct me if I am missing anything here.
~ Kashyap
Quoted 1 line
This patch currently includes
IRQ_DISABLE_SCSI_QCMD()usage which Iam
Quoted 1 line
not even sure is needed (according to Vasu's original findings), but
I
Quoted 1 line
decided to err on the conservative side until we can get some
feedback
Quoted 4 lines
from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
--nab
Quoted 2 lines · Kashyap
Nicholas,
Hi Kashyap,
My apologies for the delayed response, as I have been AFK for the past days..
Quoted 4 lines
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.
Thank you for the clarification here.
Quoted 8 lines
e.a if
scsih_qcmd_xxxhas 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 originalscsih_qcmd_xxxLLD drive will see bad results (Kernel crash/Data corruption/h/w hung or anything critical.....)
Hmmm, very good point.
Quoted 5 lines
So I would suggest to disable preemption also in your macro "
IRQ_DISABLE_SCSI_QCMD" as below two level of prevention.local_irq_save(flags); preempt_disable();
Ok, I have been asked to drop the IRQ_DISABLE_SCSI_QCMD() macro and open code this for the handful of LLDs in the host_lock-less series. I will add the functional equivilent and disable preemption following your recommendation in mpt2sas/mptfusion (and the other drivers using IRQ_DISABLE_SCSI_QCMD) in the next round of lock_less-LLDs-for-38-v4.
Quoted 3 lines
mpt2sas parameters checks like
ioc_link_reset_in_process,tm_busy,scs_priv_target_data->deletedare mainly to communicate device/IOC state of the firmware so there is no hard dependency w.r.thost_lock.
Also understood. Thanks for the feedback here and I will respin the mpt2sas/mptfusion for your review next week.
Best Regards,
--nab
Quoted 135 lines
Thanks, Kashyap
Quoted 128 lines
-----Original Message-----
From: Desai, Kashyap
Sent: Friday, November 19, 2010 4:33 PM
To: Nicholas A. Bellinger; Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux
Subject: RE: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externallyQuoted 9 lines
-----Original Message-----
From: Nicholas A. Bellinger [mailto:[email protected]]
Sent: Friday, November 19, 2010 4:28 AM
To: Boaz Harrosh
Cc: linux-scsi; Jeff Garzik; James Bottomley; Christoph Hellwig; Mike Christie; Vasu Dev; Tejun Heo; DL-MPT Fusion Linux; Desai, Kashyap
Subject: Re: [PATCH 06/11] mpt2sas: Convert tohost_lockless w/ interrupts disabled externallyQuoted 9 lines
<Trimming CC list>
Quoted 4 lines · Boaz Harrosh
Quoted 3 lines · Nicholas A. Bellinger
From: Nicholas Bellinger <[email protected]>
This patch converts the mpt2sas driver to run in
host_locklessmode
Quoted 1 line
Quoted 1 line
with the new
IRQ_DISABLE_SCSI_QCMD()that disables interruptswhile
Quoted 28 lines
Quoted 8 lines
Review
Quoted 8 lines
calling ->queuecommand() dispatch
Signed-off-by Nicholas A. Bellinger <[email protected]>--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 6 +++--- 1 files changed, 3 insertions(+), 3 deletions(-)(quoted diff) +0
-0
b/drivers/scsi/mpt2sas/mpt2sas_scsih.cQuoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 4 lines
index 1a96a00..e564fe7 100644
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0@@ -3304,7 +3304,7 @@ _scsih_eedp_error_handling(struct scsi_cmnd*scmd, u16
ioc_status)Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd
Quoted 9 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1}/** -* _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)Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd *scmd, u16 ioc_status)
Quoted 4 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-1* SCSI_MLQUEUE_HOST_BUSY if the entire host queue is full */ static int -_scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(structQuoted 8 lines
scsi_cmnd*))Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3318
Quoted 1 line
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-0+_scsih_qcmd_irq_disable(struct scsi_cmnd *scmd, void(*done)(struct
scsi_cmnd*))Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_eedp_error_handling(struct scsi_cmnd · line 3320
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0{ struct MPT2SAS_ADAPTER *ioc = shost_priv(scmd->device-host);
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd,
Quoted 2 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +0
-0struct MPT2SAS_DEVICE *sas_device_priv_data;@@ -3441,7 +3441,7 @@ _scsih_qcmd_lck(struct scsi_cmnd *scmd,void
Quoted 21 lines
(*done)(struct
scsi_cmnd*))Quoted 8 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c _scsih_qcmd_lck(struct scsi_cmnd *scmd, void (*done)(struct scsi_cmnd *)) · line 3445
Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_scsih.c +1
-1return 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
Quoted 1 line
that if you remove the lock,xx_qcmd_lck,unlock then inside the
xx_qcmd_lckQuoted 1 line
there was an unlock,do123,lock and that driver was effectively
running lockless
Quoted 1 line
before. (like in iscsi). But here this is new behaviour. If it is
correct
Quoted 2 lines
I would like to see a statement from you that:
"I have audited this driver, and all shared resources areprotected by
Quoted 2 lines
XYZ so ..."Quoted 7 lines
Quoted 1 line
Otherwise how can I know this is correct? I have never audited this
driver myself
Quoted 1 line
So for this specific mpt2sas case, Vasu Dev had been testing the
lock_lesscase w/o disabling interrupts for his original SHT->unlocked_qcmd=1 patch, and from his comments this mode wasstable
Quoted 2 lines
during his JBOD
lock_lesssmall block IOP performance test.Nicholas, mpt2sas driver is safe even if in lock less condition. I have tested mpt2sas driver available in 2.6.37-rc2 where I have seen host lock push down code is available for mpt2sas driver
I have seen staticDEF_SCSI_QCMD(_scsih_qcmd)is available for mpt2sas driver. I also did testing removing that particular macro and making Mpt2sas driver in actual host lock less mode. Below observation is after removing staticDEF_SCSI_QCMD(_scsih_qcmd)from mpt2sas (that is what I refer as actual host lock less mode)I have tested stability of the driver and I/O integrity. Things were fine. I am not seeing any need for even going for
IRQ_DISABLE_SCSI_QCMD()patch too.
Mpt2sas driver is able to handle host lock less mode seamlessly, since we have all required (racy) places under spinlocks.I do not see any solid need for this patch. Please correct me if I am missing anything here.
~ Kashyap
Quoted 1 line
This patch currently includes
IRQ_DISABLE_SCSI_QCMD()usage which Iam
Quoted 1 line
not even sure is needed (according to Vasu's original findings), but
I
Quoted 1 line
decided to err on the conservative side until we can get some
feedback
Quoted 4 lines
from the the mpt2sas maintainer (Kashyap @ LSI CC'ed).
--nab