[PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Matthew Wilcox]

In preparation for moving some drivers out from under the host_lock,
make cmd_serial_number an atomic.

Signed-off-by: Matthew Wilcox <matthew.r.wilcox@intel.com>

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 2aeb2e9..13a3bf0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -631,14 +631,15 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
  * @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.
+ * A serial number identifies a request for error recovery and debugging
+ * purposes.
  */
 void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
-	cmd->serial_number = host->cmd_serial_number++;
+ again:
+	cmd->serial_number = atomic_inc_return(&host->cmd_serial_number);
 	if (cmd->serial_number == 0) 
-		cmd->serial_number = host->cmd_serial_number++;
+		goto again;
 }
 EXPORT_SYMBOL(scsi_cmd_get_serial);
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e7e3858..69dfabb 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -621,11 +621,9 @@ struct Scsi_Host {
 	short unsigned int sg_prot_tablesize;
 	short unsigned int max_sectors;
 	unsigned long dma_boundary;
-	/* 
-	 * Used to assign serial numbers to the cmds.
-	 * Protected by the host lock.
-	 */
-	unsigned long cmd_serial_number;
+
+	/* Used to assign serial numbers to the cmds.  */
+	atomic_t cmd_serial_number;
 	
 	unsigned active_mode:2;
 	unsigned unchecked_isa_dma:1;

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Christoph Hellwig]

On Fri, Apr 01, 2011 at 04:20:51PM -0400, Matthew Wilcox wrote:
> 
> In preparation for moving some drivers out from under the host_lock,
> make cmd_serial_number an atomic.

The right fix is to stop abusing cmd_serial_nubmer in those few
drivers, and use a driver-private and more scalable lookup data
structure than a linear list search.

And yes, I promised to at least move cmd_serial_number into these
drivers long time ago - time to get back to that.

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[James Bottomley]

On Sat, 2011-04-02 at 09:36 -0400, Christoph Hellwig wrote:
> On Fri, Apr 01, 2011 at 04:20:51PM -0400, Matthew Wilcox wrote:
> > 
> > In preparation for moving some drivers out from under the host_lock,
> > make cmd_serial_number an atomic.
> 
> The right fix is to stop abusing cmd_serial_nubmer in those few
> drivers, and use a driver-private and more scalable lookup data
> structure than a linear list search.
> 
> And yes, I promised to at least move cmd_serial_number into these
> drivers long time ago - time to get back to that.

Gosh, I'm glad you said that ... I was afraid it would sound like I was
a Nag.

James

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Jeff Garzik]

On 04/02/2011 09:36 AM, Christoph Hellwig wrote:
> On Fri, Apr 01, 2011 at 04:20:51PM -0400, Matthew Wilcox wrote:
>>
>> In preparation for moving some drivers out from under the host_lock,
>> make cmd_serial_number an atomic.
>
> The right fix is to stop abusing cmd_serial_nubmer in those few
> drivers, and use a driver-private and more scalable lookup data
> structure than a linear list search.

Agreed.

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Matthew Wilcox]

On Sat, Apr 02, 2011 at 09:36:43AM -0400, Christoph Hellwig wrote:
> On Fri, Apr 01, 2011 at 04:20:51PM -0400, Matthew Wilcox wrote:
> > 
> > In preparation for moving some drivers out from under the host_lock,
> > make cmd_serial_number an atomic.
> 
> The right fix is to stop abusing cmd_serial_nubmer in those few
> drivers, and use a driver-private and more scalable lookup data
> structure than a linear list search.
> 
> And yes, I promised to at least move cmd_serial_number into these
> drivers long time ago - time to get back to that.

Fair enough, but you're making the perfect the enemy of the good.
How about putting this patch in for now (since it harms nothing), and
then it'll all go away when you delete cmd_serial_number?

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Christoph Hellwig]

On Sat, Apr 02, 2011 at 03:02:40PM -0600, Matthew Wilcox wrote:
> Fair enough, but you're making the perfect the enemy of the good.
> How about putting this patch in for now (since it harms nothing), and
> then it'll all go away when you delete cmd_serial_number?

If you change it anyway for mpt2sas just keep the atomic counter in
it's local structures.  Or even better verify with LSI if the
serial_number check can't simply be removed entirely, which I think it
could.

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[James Bottomley]

On Sun, 2011-04-03 at 07:00 -0400, Christoph Hellwig wrote:
> On Sat, Apr 02, 2011 at 03:02:40PM -0600, Matthew Wilcox wrote:
> > Fair enough, but you're making the perfect the enemy of the good.
> > How about putting this patch in for now (since it harms nothing), and
> > then it'll all go away when you delete cmd_serial_number?
> 
> If you change it anyway for mpt2sas just keep the atomic counter in
> it's local structures.  Or even better verify with LSI if the
> serial_number check can't simply be removed entirely, which I think it
> could.

Also I'm not really sure atomic counter conversion is the correct thing
to do.  Someone from intel said it would perform worse than the current
spinlock based serial number the last time we discussed the conversion
(converting it to atomic was my first thought at the time).

James

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Matthew Wilcox]

On Sun, Apr 03, 2011 at 07:00:57AM -0400, Christoph Hellwig wrote:
> On Sat, Apr 02, 2011 at 03:02:40PM -0600, Matthew Wilcox wrote:
> > Fair enough, but you're making the perfect the enemy of the good.
> > How about putting this patch in for now (since it harms nothing), and
> > then it'll all go away when you delete cmd_serial_number?
> 
> If you change it anyway for mpt2sas just keep the atomic counter in
> it's local structures.  Or even better verify with LSI if the
> serial_number check can't simply be removed entirely, which I think it
> could.

Hm, yeah, it looks like it's only used for checking whether the command
we're aborting is the same command we're using to do the aborts.  In
which case, can't we simply do this?  Eric?

diff --git a/drivers/scsi/mpt2sas/mpt2sas_scsih.c b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
index 6ceb775..bce9043 100644
--- a/drivers/scsi/mpt2sas/mpt2sas_scsih.c
+++ b/drivers/scsi/mpt2sas/mpt2sas_scsih.c
@@ -2133,8 +2133,7 @@ mpt2sas_scsih_issue_tm(struct MPT2SAS_ADAPTER *ioc, u16 handle, uint channel,
 	switch (type) {
 	case MPI2_SCSITASKMGMT_TASKTYPE_ABORT_TASK:
 		scmd_lookup = _scsih_scsi_lookup_get(ioc, smid_task);
-		if (scmd_lookup && (scmd_lookup->serial_number ==
-		    scmd->serial_number))
+		if (scmd_lookup == scmd))
 			rc = FAILED;
 		else
 			rc = SUCCESS;

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[Christoph Hellwig]

On Sun, Apr 03, 2011 at 07:15:43AM -0600, Matthew Wilcox wrote:
> Hm, yeah, it looks like it's only used for checking whether the command
> we're aborting is the same command we're using to do the aborts.  In
> which case, can't we simply do this?  Eric?

A similar question also applies for the old fusion driver.  There it
even compares the serial number with one taken from the same scsi_cmnd
in the same function.  If we could reuse a scsi_cmnd while in the
->eh_abort handler for fusion it would have to seriously mess up it's
locking and lifetime rules.

Re: [PATCH 1/3] SCSI: Make cmd_serial_number an atomic

[James Bottomley]

On Sun, 2011-04-03 at 09:33 -0400, Christoph Hellwig wrote:
> On Sun, Apr 03, 2011 at 07:15:43AM -0600, Matthew Wilcox wrote:
> > Hm, yeah, it looks like it's only used for checking whether the command
> > we're aborting is the same command we're using to do the aborts.  In
> > which case, can't we simply do this?  Eric?
> 
> A similar question also applies for the old fusion driver.  There it
> even compares the serial number with one taken from the same scsi_cmnd
> in the same function.  If we could reuse a scsi_cmnd while in the
> ->eh_abort handler for fusion it would have to seriously mess up it's
> locking and lifetime rules.

Well, I think there's method in the madness for the old driver.  What it
seems to worry about is that the command gets reissued after the abort
because the abort routine sleeps.  If the reissue occurred, the same
command could be on the active list with a different serial number.

The theory is currently bogus because we halt all activity on the host
while error handler porcessing is active and the reissue won't occur
until after the abort routine returns.  Although if we did running error
handling, problems like this would arise ...

The new code seems to be an incorrect extract from the old code, so both
can currently go.

James