* [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() @ 2012-09-24 5:30 Li Zhong 2012-09-24 5:44 ` James Bottomley 0 siblings, 1 reply; 16+ messages in thread From: Li Zhong @ 2012-09-24 5:30 UTC (permalink / raw) To: LKML; +Cc: JBottomley, linux-scsi, Paul E. McKenney Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function. Maybe it is not possible at run time, but from the code itself, we'd better have this check? This patch actually is a delta of my previous patch [https://lkml.org/lkml/2012/4/11/814] and commit 919f797 Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 2936b44..b9dd7f0 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -832,7 +832,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd) if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) { int old_good_bytes = good_bytes; drv = scsi_cmd_to_driver(cmd); - if (drv->done) + if (drv && drv->done) good_bytes = drv->done(cmd); /* * USB may not give sense identifying bad sector and ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 5:30 [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Li Zhong @ 2012-09-24 5:44 ` James Bottomley 2012-09-24 7:03 ` Li Zhong 0 siblings, 1 reply; 16+ messages in thread From: James Bottomley @ 2012-09-24 5:44 UTC (permalink / raw) To: Li Zhong; +Cc: LKML, linux-scsi, Paul E. McKenney On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote: > Just noticed that after commit 919f797, it is possible that > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv > returned from the above function. > > Maybe it is not possible at run time, but from the code itself, we'd better > have this check? There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that. James ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 5:44 ` James Bottomley @ 2012-09-24 7:03 ` Li Zhong 2012-09-24 7:35 ` James Bottomley 0 siblings, 1 reply; 16+ messages in thread From: Li Zhong @ 2012-09-24 7:03 UTC (permalink / raw) To: James Bottomley; +Cc: LKML, linux-scsi, Paul E. McKenney On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote: > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote: > > Just noticed that after commit 919f797, it is possible that > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv > > returned from the above function. > > > > Maybe it is not possible at run time, but from the code itself, we'd better > > have this check? > > There's not much point having a check that never trips, unless it's an > assert, in which case a NULL deref does that. All it does is add > pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC > commands can be submitted without a driver, so the check above would > seem to preclude that. Hi James, Thank you, it sounds reasonable to me. Let's drop it. Thanks, Zhong > James > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 7:03 ` Li Zhong @ 2012-09-24 7:35 ` James Bottomley 2012-09-24 9:25 ` Li Zhong 2012-09-27 2:02 ` Martin K. Petersen 0 siblings, 2 replies; 16+ messages in thread From: James Bottomley @ 2012-09-24 7:35 UTC (permalink / raw) To: Li Zhong; +Cc: LKML, linux-scsi, Paul E. McKenney, Martin K. Petersen On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote: > On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote: > > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote: > > > Just noticed that after commit 919f797, it is possible that > > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv > > > returned from the above function. > > > > > > Maybe it is not possible at run time, but from the code itself, we'd better > > > have this check? > > > > There's not much point having a check that never trips, unless it's an > > assert, in which case a NULL deref does that. All it does is add > > pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC > > commands can be submitted without a driver, so the check above would > > seem to preclude that. > > Hi James, > > Thank you, it sounds reasonable to me. Let's drop it. Well, there is another thing you might do: The path length of scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be worth getting it back to what it was (this looks to be doable with the same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached (I've cc'd Martin to see what he thinks). James ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 7:35 ` James Bottomley @ 2012-09-24 9:25 ` Li Zhong 2012-09-24 9:33 ` James Bottomley 2012-09-27 2:02 ` Martin K. Petersen 1 sibling, 1 reply; 16+ messages in thread From: Li Zhong @ 2012-09-24 9:25 UTC (permalink / raw) To: James Bottomley; +Cc: LKML, linux-scsi, Paul E. McKenney, Martin K. Petersen On Mon, 2012-09-24 at 11:35 +0400, James Bottomley wrote: > On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote: > > On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote: > > > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote: > > > > Just noticed that after commit 919f797, it is possible that > > > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv > > > > returned from the above function. > > > > > > > > Maybe it is not possible at run time, but from the code itself, we'd better > > > > have this check? > > > > > > There's not much point having a check that never trips, unless it's an > > > assert, in which case a NULL deref does that. All it does is add > > > pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC > > > commands can be submitted without a driver, so the check above would > > > seem to preclude that. > > > > Hi James, > > > > Thank you, it sounds reasonable to me. Let's drop it. > > Well, there is another thing you might do: The path length of > scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be > worth getting it back to what it was (this looks to be doable with the > same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it > fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC > commands when a driver is and isn't attached (I've cc'd Martin to see > what he thinks). Hi James, Do you mean something like this: ======= diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index de2337f..c1b05a8 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, int cmnd_size, int timeout, unsigned sense_bytes) { struct scsi_device *sdev = scmd->device; - struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); struct Scsi_Host *shost = sdev->host; DECLARE_COMPLETION_ONSTACK(done); unsigned long timeleft; @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, scsi_eh_restore_cmnd(scmd, &ses); - if (sdrv && sdrv->eh_action) - rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) { + struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); + if (sdrv->eh_action) + rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + } return rtn; } diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index ac06cc5..377df4a 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -134,16 +134,7 @@ struct scsi_cmnd { static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd) { - struct scsi_driver **sdp; - - if (!cmd->request->rq_disk) - return NULL; - - sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data; - if (!sdp) - return NULL; - - return *sdp; + return *(struct scsi_driver **)cmd->request->rq_disk->private_data; } extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 9:25 ` Li Zhong @ 2012-09-24 9:33 ` James Bottomley 0 siblings, 0 replies; 16+ messages in thread From: James Bottomley @ 2012-09-24 9:33 UTC (permalink / raw) To: Li Zhong; +Cc: LKML, linux-scsi, Paul E. McKenney, Martin K. Petersen On Mon, 2012-09-24 at 17:25 +0800, Li Zhong wrote: > On Mon, 2012-09-24 at 11:35 +0400, James Bottomley wrote: > > On Mon, 2012-09-24 at 15:03 +0800, Li Zhong wrote: > > > On Mon, 2012-09-24 at 09:44 +0400, James Bottomley wrote: > > > > On Mon, 2012-09-24 at 13:30 +0800, Li Zhong wrote: > > > > > Just noticed that after commit 919f797, it is possible that > > > > > scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv > > > > > returned from the above function. > > > > > > > > > > Maybe it is not possible at run time, but from the code itself, we'd better > > > > > have this check? > > > > > > > > There's not much point having a check that never trips, unless it's an > > > > assert, in which case a NULL deref does that. All it does is add > > > > pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC > > > > commands can be submitted without a driver, so the check above would > > > > seem to preclude that. > > > > > > Hi James, > > > > > > Thank you, it sounds reasonable to me. Let's drop it. > > > > Well, there is another thing you might do: The path length of > > scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be > > worth getting it back to what it was (this looks to be doable with the > > same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it > > fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC > > commands when a driver is and isn't attached (I've cc'd Martin to see > > what he thinks). > > Hi James, > > Do you mean something like this: Pretty much, yes. James > ======= > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index de2337f..c1b05a8 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > int cmnd_size, int timeout, unsigned sense_bytes) > { > struct scsi_device *sdev = scmd->device; > - struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); > struct Scsi_Host *shost = sdev->host; > DECLARE_COMPLETION_ONSTACK(done); > unsigned long timeleft; > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > > scsi_eh_restore_cmnd(scmd, &ses); > > - if (sdrv && sdrv->eh_action) > - rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > + if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) { > + struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); > + if (sdrv->eh_action) > + rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > + } > > return rtn; > } > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index ac06cc5..377df4a 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -134,16 +134,7 @@ struct scsi_cmnd { > > static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd) > { > - struct scsi_driver **sdp; > - > - if (!cmd->request->rq_disk) > - return NULL; > - > - sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data; > - if (!sdp) > - return NULL; > - > - return *sdp; > + return *(struct scsi_driver **)cmd->request->rq_disk->private_data; > } > > extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-24 7:35 ` James Bottomley 2012-09-24 9:25 ` Li Zhong @ 2012-09-27 2:02 ` Martin K. Petersen 2012-09-27 4:44 ` James Bottomley 1 sibling, 1 reply; 16+ messages in thread From: Martin K. Petersen @ 2012-09-27 2:02 UTC (permalink / raw) To: James Bottomley; +Cc: Li Zhong, LKML, linux-scsi, Paul E. McKenney >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes: James> Plus, I think it fixes a bug where you get different behaviours James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't James> attached (I've cc'd Martin to see what he thinks). I'm fine with having the eh action be triggered for FS requests only, if that's what you're asking? -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-27 2:02 ` Martin K. Petersen @ 2012-09-27 4:44 ` James Bottomley 2012-09-27 9:51 ` [PATCH scsi] Short the path length of scsi_cmd_to_driver() Li Zhong 2012-09-27 17:41 ` [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Martin K. Petersen 0 siblings, 2 replies; 16+ messages in thread From: James Bottomley @ 2012-09-27 4:44 UTC (permalink / raw) To: Martin K. Petersen; +Cc: Li Zhong, LKML, linux-scsi, Paul E. McKenney On Wed, 2012-09-26 at 22:02 -0400, Martin K. Petersen wrote: > >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes: > > James> Plus, I think it fixes a bug where you get different behaviours > James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't > James> attached (I've cc'd Martin to see what he thinks). > > I'm fine with having the eh action be triggered for FS requests only, if > that's what you're asking? Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC commands (which includes flush and other strange types), but if you think it should only be for REQ_TYPE_FS, that's fine too. James ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH scsi] Short the path length of scsi_cmd_to_driver() 2012-09-27 4:44 ` James Bottomley @ 2012-09-27 9:51 ` Li Zhong 2012-09-27 17:43 ` Martin K. Petersen 2012-09-27 17:41 ` [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Martin K. Petersen 1 sibling, 1 reply; 16+ messages in thread From: Li Zhong @ 2012-09-27 9:51 UTC (permalink / raw) To: James Bottomley; +Cc: Martin K. Petersen, LKML, linux-scsi, Paul E. McKenney Hi James, Martin, Here is the updated version, please help to review. Thanks, Zhong ======== As suggested by James: this patch tries to short the path length of scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so we could avoid the related NULL checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you get different behaviors from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached. And Martin pointed out that we could have eh action be triggered for REQ_TYPE_FS type only. Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- drivers/scsi/scsi_error.c | 8 +++++--- include/scsi/scsi_cmnd.h | 12 ++---------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index de2337f..4001559 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, int cmnd_size, int timeout, unsigned sense_bytes) { struct scsi_device *sdev = scmd->device; - struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); struct Scsi_Host *shost = sdev->host; DECLARE_COMPLETION_ONSTACK(done); unsigned long timeleft; @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, scsi_eh_restore_cmnd(scmd, &ses); - if (sdrv && sdrv->eh_action) - rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + if (scmd->request->cmd_type == REQ_TYPE_FS) { + struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); + if (sdrv->eh_action) + rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + } return rtn; } diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index ac06cc5..de5f5d8 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -132,18 +132,10 @@ struct scsi_cmnd { unsigned char tag; /* SCSI-II queued command tag */ }; +/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd) { - struct scsi_driver **sdp; - - if (!cmd->request->rq_disk) - return NULL; - - sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data; - if (!sdp) - return NULL; - - return *sdp; + return *(struct scsi_driver **)cmd->request->rq_disk->private_data; } extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t); -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver() 2012-09-27 9:51 ` [PATCH scsi] Short the path length of scsi_cmd_to_driver() Li Zhong @ 2012-09-27 17:43 ` Martin K. Petersen 2012-09-28 7:13 ` Li Zhong 2012-09-28 7:45 ` James Bottomley 0 siblings, 2 replies; 16+ messages in thread From: Martin K. Petersen @ 2012-09-27 17:43 UTC (permalink / raw) To: Li Zhong Cc: James Bottomley, Martin K. Petersen, LKML, linux-scsi, Paul E. McKenney >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes: > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > > scsi_eh_restore_cmnd(scmd, &ses); > >- if (sdrv && sdrv->eh_action) >- rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); >+ if (scmd->request->cmd_type == REQ_TYPE_FS) { >+ struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); >+ if (sdrv->eh_action) >+ rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); >+ } > > return rtn; > } My only concern is whether our device lifetime rules guarantee that the ULD is always attached when we service an error handling command? -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver() 2012-09-27 17:43 ` Martin K. Petersen @ 2012-09-28 7:13 ` Li Zhong 2012-09-28 7:45 ` James Bottomley 1 sibling, 0 replies; 16+ messages in thread From: Li Zhong @ 2012-09-28 7:13 UTC (permalink / raw) To: Martin K. Petersen; +Cc: James Bottomley, LKML, linux-scsi, Paul E. McKenney On Thu, 2012-09-27 at 13:43 -0400, Martin K. Petersen wrote: > >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes: > > > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > > > > scsi_eh_restore_cmnd(scmd, &ses); > > > >- if (sdrv && sdrv->eh_action) > >- rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > >+ if (scmd->request->cmd_type == REQ_TYPE_FS) { > >+ struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); > >+ if (sdrv->eh_action) > >+ rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > >+ } > > > > return rtn; > > } > > My only concern is whether our device lifetime rules guarantee that the > ULD is always attached when we service an error handling command? Thank you, Martin, for the review. I don't know much about scsi, it might take me some more time to have an answer to the above question. For now, if I understand correctly, maybe we could only do the not-consistent behaviours bug fix? Or could we provide two versions of scsi_cmd_to_driver(), one with NULL checking for scsi_send_eh_cmnd(), one without the checking for scsi_finish_command()? Thanks, Zhong ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver() 2012-09-27 17:43 ` Martin K. Petersen 2012-09-28 7:13 ` Li Zhong @ 2012-09-28 7:45 ` James Bottomley 1 sibling, 0 replies; 16+ messages in thread From: James Bottomley @ 2012-09-28 7:45 UTC (permalink / raw) To: Martin K. Petersen; +Cc: Li Zhong, LKML, linux-scsi, Paul E. McKenney On Thu, 2012-09-27 at 13:43 -0400, Martin K. Petersen wrote: > >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes: > > > @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, > > > > scsi_eh_restore_cmnd(scmd, &ses); > > > >- if (sdrv && sdrv->eh_action) > >- rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > >+ if (scmd->request->cmd_type == REQ_TYPE_FS) { > >+ struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); > >+ if (sdrv->eh_action) > >+ rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); > >+ } > > > > return rtn; > > } > > My only concern is whether our device lifetime rules guarantee that the > ULD is always attached when we service an error handling command? Yes, they are. We can only get REQ_TYPE_FS through a filesystem, which must be mounted on a block device, which is provided by the ULD. You can't unmount with outstanding I/O (which people complain about when it goes into error handling, I'll admit), so the ULD has to stay bound. James ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-27 4:44 ` James Bottomley 2012-09-27 9:51 ` [PATCH scsi] Short the path length of scsi_cmd_to_driver() Li Zhong @ 2012-09-27 17:41 ` Martin K. Petersen 2012-09-28 7:48 ` James Bottomley 1 sibling, 1 reply; 16+ messages in thread From: Martin K. Petersen @ 2012-09-27 17:41 UTC (permalink / raw) To: James Bottomley Cc: Martin K. Petersen, Li Zhong, LKML, linux-scsi, Paul E. McKenney >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes: >> I'm fine with having the eh action be triggered for FS requests only, >> if that's what you're asking? James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC James> commands (which includes flush and other strange types), but if James> you think it should only be for REQ_TYPE_FS, that's fine too. I'm ok either way. -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() 2012-09-27 17:41 ` [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Martin K. Petersen @ 2012-09-28 7:48 ` James Bottomley 2012-09-29 4:23 ` [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver() Li Zhong 0 siblings, 1 reply; 16+ messages in thread From: James Bottomley @ 2012-09-28 7:48 UTC (permalink / raw) To: Martin K. Petersen; +Cc: Li Zhong, LKML, linux-scsi, Paul E. McKenney On Thu, 2012-09-27 at 13:41 -0400, Martin K. Petersen wrote: > >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes: > > >> I'm fine with having the eh action be triggered for FS requests only, > >> if that's what you're asking? > > James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC > James> commands (which includes flush and other strange types), but if > James> you think it should only be for REQ_TYPE_FS, that's fine too. > > I'm ok either way. OK, let's mirror the code in scsi.c then, so != REQ_TYPE_BLOCK_PC. James ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver() 2012-09-28 7:48 ` James Bottomley @ 2012-09-29 4:23 ` Li Zhong 2012-10-02 22:22 ` Martin K. Petersen 0 siblings, 1 reply; 16+ messages in thread From: Li Zhong @ 2012-09-29 4:23 UTC (permalink / raw) To: James Bottomley; +Cc: Martin K. Petersen, LKML, linux-scsi, Paul E. McKenney As suggested by James: this patch tries to short the path length of scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so we could avoid the related NULL checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you get different behaviors from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached. v2: mirror code != REQ_TYPE_BLOCK_PC in scsi.c, rather than == REQ_TYPE_FS Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com> --- drivers/scsi/scsi_error.c | 8 +++++--- include/scsi/scsi_cmnd.h | 12 ++---------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index de2337f..c1b05a8 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, int cmnd_size, int timeout, unsigned sense_bytes) { struct scsi_device *sdev = scmd->device; - struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); struct Scsi_Host *shost = sdev->host; DECLARE_COMPLETION_ONSTACK(done); unsigned long timeleft; @@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, scsi_eh_restore_cmnd(scmd, &ses); - if (sdrv && sdrv->eh_action) - rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) { + struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd); + if (sdrv->eh_action) + rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn); + } return rtn; } diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index ac06cc5..de5f5d8 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -132,18 +132,10 @@ struct scsi_cmnd { unsigned char tag; /* SCSI-II queued command tag */ }; +/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd) { - struct scsi_driver **sdp; - - if (!cmd->request->rq_disk) - return NULL; - - sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data; - if (!sdp) - return NULL; - - return *sdp; + return *(struct scsi_driver **)cmd->request->rq_disk->private_data; } extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t); -- 1.7.7.6 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver() 2012-09-29 4:23 ` [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver() Li Zhong @ 2012-10-02 22:22 ` Martin K. Petersen 0 siblings, 0 replies; 16+ messages in thread From: Martin K. Petersen @ 2012-10-02 22:22 UTC (permalink / raw) To: Li Zhong Cc: James Bottomley, Martin K. Petersen, LKML, linux-scsi, Paul E. McKenney >>>>> "Li" == Li Zhong <zhong@linux.vnet.ibm.com> writes: Li> As suggested by James: this patch tries to short the path length of Li> scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be Li> submitted without a driver, so we could avoid the related NULL Li> checking, as long as we make sure we don't use it for Li> REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you Li> get different behaviors from REQ_TYPE_BLOCK_PC commands when a Li> driver is and isn't attached. Acked-by: Martin K. Petersen <martin.petersen@oracle.com> -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-10-02 22:22 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-09-24 5:30 [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Li Zhong 2012-09-24 5:44 ` James Bottomley 2012-09-24 7:03 ` Li Zhong 2012-09-24 7:35 ` James Bottomley 2012-09-24 9:25 ` Li Zhong 2012-09-24 9:33 ` James Bottomley 2012-09-27 2:02 ` Martin K. Petersen 2012-09-27 4:44 ` James Bottomley 2012-09-27 9:51 ` [PATCH scsi] Short the path length of scsi_cmd_to_driver() Li Zhong 2012-09-27 17:43 ` Martin K. Petersen 2012-09-28 7:13 ` Li Zhong 2012-09-28 7:45 ` James Bottomley 2012-09-27 17:41 ` [PATCH scsi] Add NULL checking of return value from scsi_cmd_to_driver() Martin K. Petersen 2012-09-28 7:48 ` James Bottomley 2012-09-29 4:23 ` [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver() Li Zhong 2012-10-02 22:22 ` Martin K. Petersen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).