scsi: Drop struct Scsi_Host->host_lock usage in scsi_dispatch_cmd()
RFC v3 1/15 scsi: Drop struct Scsi_Host->host_lock usage in scsi_dispatch_cmd()
From: Nicholas Bellinger <[email protected]>
This patch converts scsi_dispatch_cmd() drop the struct Scsi_Host->host_lock legacy usage so that SHT->queuecommand() can be called without this lock held softirq interrupts disabled via spin_lock_irq()
This patch drops the usage of scsi_cmd_get_serial() in scsi_dispatch_cmd() and assumes the legacy SCSI LLDs that depend upon struct scsi_cmnd->serial_number will call the now EXPORT_SYMBOL()'ed scsi_cmd_get_serial() call.
This patch also updates scsi_error.c:scsi_try_to_abort_cmd() to use:
if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
return SUCCESS;instead of checking for (scmd->serial_number == 0) following a recommendation by Mike Christie.
This patch also converts the remaining struct Scsi_Host->cmd_serial_number to atomic_t following a recommedation by Joe Eykholt to start struct Scsi_Host-> cmd_serial_number at 1, and increment each serial_number by 2 so that the serial is odd, and wraps to 1 instead of 0.
Many thanks to Vasu Dev, Tim Chen, Mike and Joe for their help with particular series!
--- drivers/scsi/scsi.c | 29 ++++++++++++++--------------- drivers/scsi/scsi_error.c | 10 +++++++--- include/scsi/scsi_cmnd.h | 1 + include/scsi/scsi_host.h | 5 ++--- 4 files changed, 24 insertions(+), 21 deletions(-)
drivers/scsi/scsi.c +14 -15
@@ -628,18 +628,22 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)/** * scsi_cmd_get_serial - Assign a serial number to a command -* @host: the scsi host * @cmd: command to assign serial number to * * Description: a serial number identifies a request for error recovery -* and debugging purposes. Protected by the Host_Lock of host. +* and debugging purposes. Called directly by SCSI LLDs that have a +* legacy requirement for struct scsi_cmnd->serial_number. */ -static inline void scsi_cmd_get_serial (struct Scsi_Host *host,struct scsi_cmnd *cmd) +void scsi_cmd_get_serial (struct scsi_cmnd *cmd) { - cmd->serial_number = host->cmd_serial_number++; -if (cmd->serial_number ==0 ) - cmd->serial_number = host->cmd_serial_number++; +struct Scsi_Host *host = cmd->device->host; +/* +* Increment the host->cmd_serial_number by 2 so cmd->serial_number +* is always odd and wraps to 1 instead of 0. +*/ + cmd->serial_number =atomic_add_return (2 , &host->cmd_serial_number); } +EXPORT_SYMBOL (scsi_cmd_get_serial);/** * scsi_dispatch_command - Dispatch a command to the low-level driver.
@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;
@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );
@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
drivers/scsi/scsi_error.c +7 -3
@@ -39,6 +39,8 @@#include "scsi_logging.h" #include "scsi_transport_api.h" +#include <../block/blk.h> /* For REQ_ATOM_COMPLETE */ +#include <trace/events/scsi.h> #define SENSE_TIMEOUT (10*HZ)
@@ -645,11 +647,13 @@ static int __scsi_try_to_abort_cmd(struct scsi_cmnd *scmd)static int scsi_try_to_abort_cmd (struct scsi_cmnd *scmd) {/* -* scsi_done was called just after the command timed out and before -* we had a chance to process it. (db) +* Use the struct request atomic_flags here to check if +* block/blk.h:blk_mark_rq_complete() has already been called +* from the block softirq */ -if (scmd->serial_number ==0 ) +if (test_bit (REQ_ATOM_COMPLETE , &scmd->request->atomic_flags))return SUCCESS ; +return __scsi_try_to_abort_cmd (scmd); }
include/scsi/scsi_cmnd.h +1 -0
@@ -136,6 +136,7 @@ extern struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *, gfp_t);extern void scsi_put_command (struct scsi_cmnd *);extern void __scsi_put_command (struct Scsi_Host *,struct scsi_cmnd *,struct device *); +extern void scsi_cmd_get_serial (struct scsi_cmnd *);extern void scsi_finish_command (struct scsi_cmnd *cmd); extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
include/scsi/scsi_host.h +2 -3
@@ -602,10 +602,9 @@ struct Scsi_Host {short unsigned int max_sectors;unsigned long dma_boundary;/* -* Used to assign serial numbers to the cmds. -* Protected by the host lock. +* Used to assign serial numbers to the cmds in scsi_cmd_get_serial() */ -unsigned long cmd_serial_number; +atomic_t cmd_serial_number; unsigned active_mode:2; unsigned unchecked_isa_dma:1;
Signature · 1 line
1.7.3
Quoted 28 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 8 lines
drivers/scsi/scsi.c +1
-1@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&
Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Signature · 3 lines
Brian King Linux on Power Virtualization IBM Linux Technology Center
Quoted 26 lines · Brian King
Quoted 25 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);
Quoted 16 lines
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 11 lines
drivers/scsi/scsi.c +1
-1if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Hi Brian,
I was under the impression that this would be unnecessary for the vast majority of existing LLD drivers, but if you are aware of specific LLDs that would still need the struct Scsi_Host->host_lock held in their SHT->queuecommand() for whaterver reason please let me know and I would be happy to include this into an RFCv4.
Thanks for your comments!
--nab
Quoted 44 lines · Nicholas A. Bellinger
Quoted 26 lines · Brian King
Quoted 25 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);Quoted 16 lines
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 11 lines
drivers/scsi/scsi.c +1
-1if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Quoted 7 lines
Hi Brian,
I was under the impression that this would be unnecessary for the vast majority of existing LLD drivers, but if you are aware of specific LLDs that would still need the struct
Scsi_Host->host_lockheld in theirSHT->queuecommand()for whaterver reason please let me know and I would be happy to include this into an RFCv4.
I would think that most drivers might have issues without some pretty careful auditing. When Christoph did this for the EH handlers, the first step was to simply move acquiring the host lock into the LLDs. That way we can optimize drivers one at a time after ensuring they can run lockless in their queuecommand handler.
A couple examples of possible issues with drivers I'm familiar with (ibmvfc, ipr):
* Some drivers will do list manipulation for resources needed to send commands. If done lockless, this could result in list corruption with multiple readers/writers.
* Some drivers check the state of the hardware before sending a command. Failing to do this when the hardware is being reset may result in nasty things like PCI bus errors or even sending a command to the wrong device.
These are all the sorts of errors that will be very difficult to hit but have pretty bad consequence when they are hit.
Thanks,
Brian
Signature · 3 lines
Brian King Linux on Power Virtualization IBM Linux Technology Center
Quoted 59 lines · Brian King
Quoted 44 lines · Nicholas A. Bellinger
Quoted 26 lines · Brian King
Quoted 25 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);Quoted 16 lines
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 11 lines
drivers/scsi/scsi.c +1
-1if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Quoted 7 lines
Hi Brian,
I was under the impression that this would be unnecessary for the vast majority of existing LLD drivers, but if you are aware of specific LLDs that would still need the struct
Scsi_Host->host_lockheld in theirSHT->queuecommand()for whaterver reason please let me know and I would be happy to include this into an RFCv4.I would think that most drivers might have issues without some pretty careful auditing. When Christoph did this for the EH handlers, the first step was to simply move acquiring the host lock into the LLDs. That way we can optimize drivers one at a time after ensuring they can run lockless in their queuecommand handler.
Quoted 9 lines
A couple examples of possible issues with drivers I'm familiar with (ibmvfc, ipr):
* Some drivers will do list manipulation for resources needed to send commands. If done lockless, this could result in list corruption with multiple readers/writers.
* Some drivers check the state of the hardware before sending a command. Failing to do this when the hardware is being reset may result in nasty things like PCI bus errors or even sending a command to the wrong device.
Indeed, I can very much see some older LLDs making these types of assumptions.
Quoted 3 lines
These are all the sorts of errors that will be very difficult to hit but have pretty bad consequence when they are hit.
I think pretty bad would be an under-statement when running into either of the above two items in ancient LLD code.
In that case, I will re-spin a v4 series that contains a legacy host_lock held w/ SHT->queuecomand() for all of the "historically high host_lock optimized in ->queuecommand()" LLDs that are in RFCv3, and include the other specific ones (namely mpt/fusion and mpt2sas) that we know are safe to drop host_lock.
Many thanks for your invaluable input on some of the non-obvious items at play here.
Best,
--nab
Quoted 46 lines · Nicholas A. Bellinger
Quoted 26 lines · Brian King
Quoted 25 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);Quoted 16 lines
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 11 lines
drivers/scsi/scsi.c +1
-1if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Hi Brian,
Quoted 7 lines
I was under the impression that this would be unnecessary for the vast majority of existing LLD drivers, but if you are aware of specific LLDs that would still need the struct
Scsi_Host->host_lockheld in theirSHT->queuecommand()for whaterver reason please let me know and I would be happy to include this into an RFCv4.Thanks for your comments!
zfcp relies on having the interrupts disabled when calling queuecommand. Without the spin_lock_irqsave in scsi_dispatch_cmd, the locking in zfcp_fsf_send_fcp_command_task has to be changed from spin_lock(&qdio->req_q_lock) to spin_lock_irqsave. It is a simple change, but other drivers might have similar requirements.
Christof
Quoted 53 lines · Christof Schmitt
Quoted 52 lines · Nicholas A. Bellinger
Quoted 26 lines · Brian King
Quoted 25 lines · Nicholas A. Bellinger
drivers/scsi/scsi.c +3
-8@@ -651,7 +655,6 @@ static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmndint scsi_dispatch_cmd (struct scsi_cmnd *cmd) {struct Scsi_Host *host = cmd->device->host; -unsigned long flags =0 ;unsigned long timeout;int rtn =0 ;@@ -736,15 +739,11 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)scsi_done (cmd); goto out; } - -spin_lock_irqsave (host->host_lock, flags);/* -* AK: unlikely race here: for some reason the timer could -* expire before the serial number is set up below. -* -* TODO: kill serial or move to blk layer +* Note that scsi_cmd_get_serial() used to be called here, but +* now we expect the legacy SCSI LLDs that actually need this +* to call it directly within their SHT->queuecommand() caller. */ -scsi_cmd_get_serial (host, cmd);Quoted 16 lines
drivers/scsi/scsi.c int scsi_dispatch_cmd(struct scsi_cmnd *cmd) · line 758
Quoted 11 lines
drivers/scsi/scsi.c +1
-1if (unlikely (host->shost_state ==SHOST_DEL )) { cmd->result = (DID_NO_CONNECT <<16 );@@ -753,7 +752,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *cmd)trace_scsi_dispatch_cmd_start (cmd); rtn = host->hostt->queuecommand (cmd, scsi_done); } -spin_unlock_irqrestore (host->host_lock, flags); +if (rtn) {trace_scsi_dispatch_cmd_error (cmd, rtn); if (rtn != SCSI_MLQUEUE_DEVICE_BUSY &&Are you planning a future revision that moves the acquiring of the host lock into the LLDD's queuecommand for all the other drivers you don't currently touch in this patch set?
Hi Brian,
I was under the impression that this would be unnecessary for the vast majority of existing LLD drivers, but if you are aware of specific LLDs that would still need the struct
Scsi_Host->host_lockheld in theirSHT->queuecommand()for whaterver reason please let me know and I would be happy to include this into an RFCv4.
Quoted 8 lines
Quoted 1 line
Thanks for your comments!
zfcp relies on having the interrupts disabled when calling queuecommand. Without the
spin_lock_irqsaveinscsi_dispatch_cmd, the locking inzfcp_fsf_send_fcp_command_taskhas to be changed fromspin_lock(&qdio->req_q_lock)tospin_lock_irqsave. It is a simple change, but other drivers might have similar requirements.
Very good to know, thanks for this useful bit of knowledge Christof.
So my next steps for an RFC v4 of this series is to:
*) Create a seperate scsi_legacy_dispatch_cmd() that still uses host_lock for SHT->queuecommand(), and enable this by default for all the LLDs not included in the RFCv3 series to drop their historical '->queuecommand() unlock -> do_work() -> lock' optimization and that we know are 100% sure we can run w/o host_lock being held. Also note that MPT/Fusion and mpt2sas will do not require host_lock as Tim Chen has demonstrated with his testing. (Any LSI folks comments here..?)
*) Following Mike Anderson's comments wrt to the signaling scsi_try_to_abort_cmd() callpath for block softirq completion w/o struct scsi_cmnd->serial_number, and how it currently breaks scsi_unjam_host -> scsi_eh_abort_cmds -> scsi_try_to_abort_cmd().
I don't for see any particular issues with the former at this point, and I will be reposting with these changes soon. For the latter it appears we need a seperate struct request->atomic_flags bit for signaling the call from block softirq context, so that the scsi_try_to_abort_cmd() can still complete the outstanding struct scsi_cmnd descriptor in the EH scsi_unjam_host() path. Any other comments on this particular item from Intel, IBM and other distro kernel folks would be apperciated.
Thanks for your comments!
--nab