[PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts struct Scsi_Host->cmd_serial_number to an atomic_t so that
scsi_cmd_get_serial() can be safely called without struct Scsi_Host->host_lock
in scsi_dispatch_cmd().  This patch also adds a struct Scsi_Host->use_serial_number
to signal serial_number usage, but this is now disabled by default in
scsi_host_alloc().

One special item in scsi_cmd_get_serial() recommended by Joe Eykholt is 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.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/scsi/hosts.c     |    4 ++++
 drivers/scsi/scsi.c      |   13 ++++++++++---
 include/scsi/scsi_host.h |    8 +++++---
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 8a8f803..aff1c9c 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -380,6 +380,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
+	/*
+	 * Set the default shost->cmd_serial_number to 1.
+	 */
+	atomic_set(&shost->cmd_serial_number, 1);
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index ad0ed21..4724ce7 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -636,9 +636,16 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
  */
 static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
-	cmd->serial_number = host->cmd_serial_number++;
-	if (cmd->serial_number == 0) 
-		cmd->serial_number = host->cmd_serial_number++;
+	/*
+	 * The use of per struct scsi_cmnd->serial_number is disabled by default
+	 */
+	if (!(host->use_serial_number))
+		return;
+	/*
+	 * 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);
 }
 
 /**
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b7bdecb..b08d0f2 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -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;
@@ -636,6 +635,9 @@ struct Scsi_Host {
 	/* Asynchronous scan in progress */
 	unsigned async_scan:1;
 
+	/* Signal usage of per struct scsi_cmnd->serial_number */
+	unsigned use_serial_number:1;
+
 	/*
 	 * Optional work queue to be utilized by the transport
 	 */
-- 
1.7.2.3

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Joe Eykholt]

On 9/17/10 11:21 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts struct Scsi_Host->cmd_serial_number to an atomic_t so that
> scsi_cmd_get_serial() can be safely called without struct Scsi_Host->host_lock
> in scsi_dispatch_cmd().  This patch also adds a struct Scsi_Host->use_serial_number
> to signal serial_number usage, but this is now disabled by default in
> scsi_host_alloc().
> 
> One special item in scsi_cmd_get_serial() recommended by Joe Eykholt is 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.
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/scsi/hosts.c     |    4 ++++
>  drivers/scsi/scsi.c      |   13 ++++++++++---
>  include/scsi/scsi_host.h |    8 +++++---
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 8a8f803..aff1c9c 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -380,6 +380,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>  	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
>  	shost->use_clustering = sht->use_clustering;
>  	shost->ordered_tag = sht->ordered_tag;
> +	/*
> +	 * Set the default shost->cmd_serial_number to 1.
> +	 */

Comment not necessary.

> +	atomic_set(&shost->cmd_serial_number, 1);
>  
>  	if (sht->supported_mode == MODE_UNKNOWN)
>  		/* means we didn't set it ... default to INITIATOR */
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> index ad0ed21..4724ce7 100644
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -636,9 +636,16 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
>   */
>  static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>  {
> -	cmd->serial_number = host->cmd_serial_number++;
> -	if (cmd->serial_number == 0) 
> -		cmd->serial_number = host->cmd_serial_number++;
> +	/*
> +	 * The use of per struct scsi_cmnd->serial_number is disabled by default
> +	 */
> +	if (!(host->use_serial_number))
> +		return;
> +	/*
> +	 * 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);
>  }
>  
>  /**
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index b7bdecb..b08d0f2 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -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;
> @@ -636,6 +635,9 @@ struct Scsi_Host {
>  	/* Asynchronous scan in progress */
>  	unsigned async_scan:1;
>  
> +	/* Signal usage of per struct scsi_cmnd->serial_number */
> +	unsigned use_serial_number:1;
> +
>  	/*
>  	 * Optional work queue to be utilized by the transport
>  	 */

Simple code is so much fun to critique!   So here goes:

This patch would break any drivers that care about serial_numbers
because it never sets use_serial_number in any drivers.  So that should
be done in the same patch so it's bisect-able.

How about instead of adding use_serial_number, let's just have the
drivers that want a serial number call scsi_cmd_get_serial()
and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
used in debug messages in some drivers.  I didn't find other usages
but didn't do an exhaustive search.  If the drivers do it themselves,
maybe they're already under a lock when they get the serial number
and then we wouldn't need the atomic.

Thanks for using my increment by 2 idea.

	Joe

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 12:03 -0700, Joe Eykholt wrote:
> On 9/17/10 11:21 AM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch converts struct Scsi_Host->cmd_serial_number to an atomic_t so that
> > scsi_cmd_get_serial() can be safely called without struct Scsi_Host->host_lock
> > in scsi_dispatch_cmd().  This patch also adds a struct Scsi_Host->use_serial_number
> > to signal serial_number usage, but this is now disabled by default in
> > scsi_host_alloc().
> > 
> > One special item in scsi_cmd_get_serial() recommended by Joe Eykholt is 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.
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > ---
> >  drivers/scsi/hosts.c     |    4 ++++
> >  drivers/scsi/scsi.c      |   13 ++++++++++---
> >  include/scsi/scsi_host.h |    8 +++++---
> >  3 files changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> > index 8a8f803..aff1c9c 100644
> > --- a/drivers/scsi/hosts.c
> > +++ b/drivers/scsi/hosts.c
> > @@ -380,6 +380,10 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
> >  	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
> >  	shost->use_clustering = sht->use_clustering;
> >  	shost->ordered_tag = sht->ordered_tag;
> > +	/*
> > +	 * Set the default shost->cmd_serial_number to 1.
> > +	 */
> 
> Comment not necessary.

<nod>

> 
> > +	atomic_set(&shost->cmd_serial_number, 1);
> >  
> >  	if (sht->supported_mode == MODE_UNKNOWN)
> >  		/* means we didn't set it ... default to INITIATOR */
> > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> > index ad0ed21..4724ce7 100644
> > --- a/drivers/scsi/scsi.c
> > +++ b/drivers/scsi/scsi.c
> > @@ -636,9 +636,16 @@ void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
> >   */
> >  static inline void scsi_cmd_get_serial(struct Scsi_Host *host, struct scsi_cmnd *cmd)
> >  {
> > -	cmd->serial_number = host->cmd_serial_number++;
> > -	if (cmd->serial_number == 0) 
> > -		cmd->serial_number = host->cmd_serial_number++;
> > +	/*
> > +	 * The use of per struct scsi_cmnd->serial_number is disabled by default
> > +	 */
> > +	if (!(host->use_serial_number))
> > +		return;
> > +	/*
> > +	 * 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);
> >  }
> >  
> >  /**
> > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> > index b7bdecb..b08d0f2 100644
> > --- a/include/scsi/scsi_host.h
> > +++ b/include/scsi/scsi_host.h
> > @@ -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;
> > @@ -636,6 +635,9 @@ struct Scsi_Host {
> >  	/* Asynchronous scan in progress */
> >  	unsigned async_scan:1;
> >  
> > +	/* Signal usage of per struct scsi_cmnd->serial_number */
> > +	unsigned use_serial_number:1;
> > +
> >  	/*
> >  	 * Optional work queue to be utilized by the transport
> >  	 */
> 
> Simple code is so much fun to critique!   So here goes:
> 
> This patch would break any drivers that care about serial_numbers
> because it never sets use_serial_number in any drivers.  So that should
> be done in the same patch so it's bisect-able.
> 

<nod>

> How about instead of adding use_serial_number, let's just have the
> drivers that want a serial number call scsi_cmd_get_serial()
> and stop calling it from scsi_dispatch_cmd()?

This makes sense to me...

>    AFAICT, it's only
> used in debug messages in some drivers.  I didn't find other usages
> but didn't do an exhaustive search.

This is the same conclusion that I reached when I had a first glance,
that ->serial_number is only used for informational purposes in about
1/2 dozen or so mostly old and semi-obsecure LLDs.

James and Co, would this be acceptable to move scsi_cmd_get_serial()
directly into LLD SHT->queuecommand() callers for the old drivers that
still use struct scsi_cmnd->serial_number for whatever purposes..?

>   If the drivers do it themselves,
> maybe they're already under a lock when they get the serial number
> and then we wouldn't need the atomic.
> 

Yeah, not sure on this part yet..

> Thanks for using my increment by 2 idea.


Thanks Joe!

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Mike Christie]

On 09/17/2010 02:03 PM, Joe Eykholt wrote:
>
> How about instead of adding use_serial_number, let's just have the
> drivers that want a serial number call scsi_cmd_get_serial()

I think this sounds better.

You could also convert drivers to the host tagging if you needed a 
unique id for each command sent to a host.

> and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> used in debug messages in some drivers.  I didn't find other usages
> but didn't do an exhaustive search.

The comments for serial_number say that it is only supposed to be used 
for debugging printks and most drivers use it for that. However, it 
looks like mpt and dpt_i2o are using it for error handling and/or lookup 
type of operations. I think the mpt* uses are not needed in the abort 
checks.

And eata is using it for ordering and tracking or something. It could 
probably be converted to the host tagging if or what you suggested if it 
needs the uniqueue id.

zfcp looks like it copies it. It does not look like the driver needs it.

scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
is set.

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Mike Christie]

On 09/17/2010 09:45 PM, Mike Christie wrote:
> The comments for serial_number say that it is only supposed to be used
> for debugging printks and most drivers use it for that. However, it
> looks like mpt and dpt_i2o are using it for error handling and/or lookup
> type of operations. I think the mpt* uses are not needed in the abort
> checks.

Oh yeah, forgot to say, for dpt I think you could just replace the 
serial_number with the host wide tagging.

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
> On 09/17/2010 02:03 PM, Joe Eykholt wrote:
> >
> > How about instead of adding use_serial_number, let's just have the
> > drivers that want a serial number call scsi_cmd_get_serial()
> 
> I think this sounds better.
> 

In that case I will go ahead and add explict scsi_cmd_get_serial() calls
to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
an obvious and simple informational purpose.

> You could also convert drivers to the host tagging if you needed a 
> unique id for each command sent to a host.

Hmmm, what does this entail again..?

> 
> > and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> > used in debug messages in some drivers.  I didn't find other usages
> > but didn't do an exhaustive search.
> 
> The comments for serial_number say that it is only supposed to be used 
> for debugging printks and most drivers use it for that.

So I would suppose it would be OK for those drivers to continue to
printk serial_number to show the internal serial_number allocation is
now disabled by default.

> However, it looks like mpt and dpt_i2o are using it for error handling
> and/or lookup type of operations. I think the mpt* uses are not needed
> in the abort checks.
> 

In that case, then adding an explict scsi_cmd_get_serial() call in mpt*
and dpt_i20 ->queuecommand() callers would be a good first step.

> And eata is using it for ordering and tracking or something. It could 
> probably be converted to the host tagging if or what you suggested if it 
> needs the uniqueue id.

Adding an explict scsi_cmd_get_serial() for eata for now, and we can
consider LLD canidates for host tag conversion as a future step.

> 
> zfcp looks like it copies it. It does not look like the driver needs it.
> 

Ok, I will look at removing it's usage in zfcp or if necessary add an
the explict scsi_cmd_get_serial() call.

> scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
> completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
> is set.

Hmmm, good catch here.  Jejb and hch, does this item work for you..?

If so then I will take another peek for any ML uses of a struct
scsi_cmnd->serial_number that need to be addressed, and include Joe's
and Mike's recommendations into a v3 series.

Thanks for your comments here Mike!

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Sat, 2010-09-18 at 12:58 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
> > On 09/17/2010 02:03 PM, Joe Eykholt wrote:
> > >
> > > How about instead of adding use_serial_number, let's just have the
> > > drivers that want a serial number call scsi_cmd_get_serial()
> > 
> > I think this sounds better.
> > 
> 
> In that case I will go ahead and add explict scsi_cmd_get_serial() calls
> to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
> an obvious and simple informational purpose.
> 
> > You could also convert drivers to the host tagging if you needed a 
> > unique id for each command sent to a host.
> 
> Hmmm, what does this entail again..?
> 
> > 
> > > and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> > > used in debug messages in some drivers.  I didn't find other usages
> > > but didn't do an exhaustive search.
> > 
> > The comments for serial_number say that it is only supposed to be used 
> > for debugging printks and most drivers use it for that.
> 
> So I would suppose it would be OK for those drivers to continue to
> printk serial_number to show the internal serial_number allocation is
> now disabled by default.
> 
> > However, it looks like mpt and dpt_i2o are using it for error handling
> > and/or lookup type of operations. I think the mpt* uses are not needed
> > in the abort checks.
> > 
> 
> In that case, then adding an explict scsi_cmd_get_serial() call in mpt*
> and dpt_i20 ->queuecommand() callers would be a good first step.
> 
> > And eata is using it for ordering and tracking or something. It could 
> > probably be converted to the host tagging if or what you suggested if it 
> > needs the uniqueue id.
> 
> Adding an explict scsi_cmd_get_serial() for eata for now, and we can
> consider LLD canidates for host tag conversion as a future step.
> 
> > 
> > zfcp looks like it copies it. It does not look like the driver needs it.
> > 
> 
> Ok, I will look at removing it's usage in zfcp or if necessary add an
> the explict scsi_cmd_get_serial() call.
> 
> > scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
> > completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
> > is set.
> 
> Hmmm, good catch here.  Jejb and hch, does this item work for you..?
> 
> If so then I will take another peek for any ML uses of a struct
> scsi_cmnd->serial_number that need to be addressed, and include Joe's
> and Mike's recommendations into a v3 series.
> 

Greetings Mike and Co,

I was doing some followup on these items for a v3 series and started
with a patch following mnc's recommendations for dropping the
scsi_error.c codes depending upon struct scsi_cmnd->serial_number:

diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 1de30eb..f35c127 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -644,11 +644,7 @@ 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)
-        */
-       if (scmd->serial_number == 0)
+       if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
                return SUCCESS;
        return __scsi_try_to_abort_cmd(scmd);
 }

and while building I noticed that the simple single enum REQ_ATOM_COMPLETE=0 is
currently located in block/blk.h and along with blk_mark_rq_complete() and
blk_clear_rq_complete() for setting this bit within struct request->atomic_flags.

jens, hch, tejun, and co, do you guys have a preference how this should be handled
so that scsi_try_to_abort_cmd() can use proper atomic struct request bits here and
we can get rid of this (racy..?) method of using struct scsi_cmnd->serial_number
for anything wrt to per struct scsi_cmnd context timeout handling.

Thanks!

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Jens Axboe]

On 2010-09-23 23:46, Nicholas A. Bellinger wrote:
> On Sat, 2010-09-18 at 12:58 -0700, Nicholas A. Bellinger wrote:
>> On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
>>> On 09/17/2010 02:03 PM, Joe Eykholt wrote:
>>>>
>>>> How about instead of adding use_serial_number, let's just have the
>>>> drivers that want a serial number call scsi_cmd_get_serial()
>>>
>>> I think this sounds better.
>>>
>>
>> In that case I will go ahead and add explict scsi_cmd_get_serial() calls
>> to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
>> an obvious and simple informational purpose.
>>
>>> You could also convert drivers to the host tagging if you needed a 
>>> unique id for each command sent to a host.
>>
>> Hmmm, what does this entail again..?
>>
>>>
>>>> and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
>>>> used in debug messages in some drivers.  I didn't find other usages
>>>> but didn't do an exhaustive search.
>>>
>>> The comments for serial_number say that it is only supposed to be used 
>>> for debugging printks and most drivers use it for that.
>>
>> So I would suppose it would be OK for those drivers to continue to
>> printk serial_number to show the internal serial_number allocation is
>> now disabled by default.
>>
>>> However, it looks like mpt and dpt_i2o are using it for error handling
>>> and/or lookup type of operations. I think the mpt* uses are not needed
>>> in the abort checks.
>>>
>>
>> In that case, then adding an explict scsi_cmd_get_serial() call in mpt*
>> and dpt_i20 ->queuecommand() callers would be a good first step.
>>
>>> And eata is using it for ordering and tracking or something. It could 
>>> probably be converted to the host tagging if or what you suggested if it 
>>> needs the uniqueue id.
>>
>> Adding an explict scsi_cmd_get_serial() for eata for now, and we can
>> consider LLD canidates for host tag conversion as a future step.
>>
>>>
>>> zfcp looks like it copies it. It does not look like the driver needs it.
>>>
>>
>> Ok, I will look at removing it's usage in zfcp or if necessary add an
>> the explict scsi_cmd_get_serial() call.
>>
>>> scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
>>> completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
>>> is set.
>>
>> Hmmm, good catch here.  Jejb and hch, does this item work for you..?
>>
>> If so then I will take another peek for any ML uses of a struct
>> scsi_cmnd->serial_number that need to be addressed, and include Joe's
>> and Mike's recommendations into a v3 series.
>>
> 
> Greetings Mike and Co,
> 
> I was doing some followup on these items for a v3 series and started
> with a patch following mnc's recommendations for dropping the
> scsi_error.c codes depending upon struct scsi_cmnd->serial_number:
> 
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 1de30eb..f35c127 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -644,11 +644,7 @@ 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)
> -        */
> -       if (scmd->serial_number == 0)
> +       if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
>                 return SUCCESS;
>         return __scsi_try_to_abort_cmd(scmd);
>  }
> 
> and while building I noticed that the simple single enum
> REQ_ATOM_COMPLETE=0 is

> currently located in block/blk.h and along with blk_mark_rq_complete()
> and blk_clear_rq_complete() for setting this bit within struct
> request->atomic_flags.
> 
> jens, hch, tejun, and co, do you guys have a preference how this
> should be handled so that scsi_try_to_abort_cmd() can use proper
> atomic struct request bits here and we can get rid of this (racy..?)
> method of using struct scsi_cmnd->serial_number for anything wrt to
> per struct scsi_cmnd context timeout handling.

Just add a

static inline int blk_test_rq_complete(struct request *rq)
{
        return test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
}

in block/blk.h

I need this too for some lockless completion patches I am playing with.

-- 
Jens Axboe

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Mike Anderson]

Jens Axboe <axboe@kernel.dk> wrote:
> On 2010-09-23 23:46, Nicholas A. Bellinger wrote:
> > On Sat, 2010-09-18 at 12:58 -0700, Nicholas A. Bellinger wrote:
> >> On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
> >>> On 09/17/2010 02:03 PM, Joe Eykholt wrote:
> >>>>
> >>>> How about instead of adding use_serial_number, let's just have the
> >>>> drivers that want a serial number call scsi_cmd_get_serial()
> >>>
> >>> I think this sounds better.
> >>>
> >>
> >> In that case I will go ahead and add explict scsi_cmd_get_serial() calls
> >> to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
> >> an obvious and simple informational purpose.
> >>
> >>> You could also convert drivers to the host tagging if you needed a 
> >>> unique id for each command sent to a host.
> >>
> >> Hmmm, what does this entail again..?
> >>
> >>>
> >>>> and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> >>>> used in debug messages in some drivers.  I didn't find other usages
> >>>> but didn't do an exhaustive search.
> >>>
> >>> The comments for serial_number say that it is only supposed to be used 
> >>> for debugging printks and most drivers use it for that.
> >>
> >> So I would suppose it would be OK for those drivers to continue to
> >> printk serial_number to show the internal serial_number allocation is
> >> now disabled by default.
> >>
> >>> However, it looks like mpt and dpt_i2o are using it for error handling
> >>> and/or lookup type of operations. I think the mpt* uses are not needed
> >>> in the abort checks.
> >>>
> >>
> >> In that case, then adding an explict scsi_cmd_get_serial() call in mpt*
> >> and dpt_i20 ->queuecommand() callers would be a good first step.
> >>
> >>> And eata is using it for ordering and tracking or something. It could 
> >>> probably be converted to the host tagging if or what you suggested if it 
> >>> needs the uniqueue id.
> >>
> >> Adding an explict scsi_cmd_get_serial() for eata for now, and we can
> >> consider LLD canidates for host tag conversion as a future step.
> >>
> >>>
> >>> zfcp looks like it copies it. It does not look like the driver needs it.
> >>>
> >>
> >> Ok, I will look at removing it's usage in zfcp or if necessary add an
> >> the explict scsi_cmd_get_serial() call.
> >>
> >>> scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
> >>> completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
> >>> is set.
> >>
> >> Hmmm, good catch here.  Jejb and hch, does this item work for you..?
> >>
> >> If so then I will take another peek for any ML uses of a struct
> >> scsi_cmnd->serial_number that need to be addressed, and include Joe's
> >> and Mike's recommendations into a v3 series.
> >>
> > 
> > Greetings Mike and Co,
> > 
> > I was doing some followup on these items for a v3 series and started
> > with a patch following mnc's recommendations for dropping the
> > scsi_error.c codes depending upon struct scsi_cmnd->serial_number:
> > 
> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 1de30eb..f35c127 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -644,11 +644,7 @@ 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)
> > -        */
> > -       if (scmd->serial_number == 0)
> > +       if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
> >                 return SUCCESS;
> >         return __scsi_try_to_abort_cmd(scmd);
> >  }
> > 
> > and while building I noticed that the simple single enum
> > REQ_ATOM_COMPLETE=0 is
> 
> > currently located in block/blk.h and along with blk_mark_rq_complete()
> > and blk_clear_rq_complete() for setting this bit within struct
> > request->atomic_flags.
> > 
> > jens, hch, tejun, and co, do you guys have a preference how this
> > should be handled so that scsi_try_to_abort_cmd() can use proper
> > atomic struct request bits here and we can get rid of this (racy..?)
> > method of using struct scsi_cmnd->serial_number for anything wrt to
> > per struct scsi_cmnd context timeout handling.
> 
> Just add a
> 
> static inline int blk_test_rq_complete(struct request *rq)
> {
>         return test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
> }
> 
> in block/blk.h
> 
> I need this too for some lockless completion patches I am playing with.

In reviewing the current code paths wasn't the serial_number also used to
avoid calling __scsi_try_to_abort_cmd for START_UNIT case also.

If we skip __scsi_try_to_abort_cmd when REQ_ATOM_COMPLETE is set it would
be correct for the scsi_decide_disposition cases but it would appear this
would stop __scsi_try_to_abort_cmd from being called in the time out
case as REQ_ATOM_COMPLETE is set prior to calling blk_rq_timed_out.

1.) Request timed out path to scsi_eh_scmd_add.

blk_rq_timed_out_timer
	...
	if (blk_mark_rq_complete(rq))
		continue;
	blk_rq_timed_out
		q->rq_timed_out_fn "scsi_times_out"
			scsi_times_out
				scsi_eh_scmd_add

2.) Request completion path to scsi_eh_scmd_add based on
scsi_decide_disposition disposition set to FAILED (actually
scsi_eh_scmd_add called from the default case with the disposition of
FAILED appearing to be the only disposition returned that would hit this
case vs. the other three handled cases). This should not be a common path
outside of handling allow_restart.

blk_complete_request
	...
	if (!blk_mark_rq_complete(req))
		__blk_complete_request
			...
			raise_softirq_irqoff(BLOCK_SOFTIRQ);
blk_done_softirq
	rq->q->softirq_done_fn "scsi_softirq_done"
		scsi_softirq_done
			scsi_eh_scmd_add

3.) Call path to scsi_try_to_abort_cmd.
scsi_error_handler
	scsi_unjam_host
		scsi_eh_abort_cmds
			scsi_try_to_abort_cmd
Thanks,

-andmike
--
Michael Anderson
andmike@linux.vnet.ibm.com

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Fri, 2010-09-24 at 08:32 +0200, Jens Axboe wrote:
> On 2010-09-23 23:46, Nicholas A. Bellinger wrote:
> > On Sat, 2010-09-18 at 12:58 -0700, Nicholas A. Bellinger wrote:

<SNIP>

> > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > index 1de30eb..f35c127 100644
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -644,11 +644,7 @@ 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)
> > -        */
> > -       if (scmd->serial_number == 0)
> > +       if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
> >                 return SUCCESS;
> >         return __scsi_try_to_abort_cmd(scmd);
> >  }
> > 
> > and while building I noticed that the simple single enum
> > REQ_ATOM_COMPLETE=0 is
> 
> > currently located in block/blk.h and along with blk_mark_rq_complete()
> > and blk_clear_rq_complete() for setting this bit within struct
> > request->atomic_flags.
> > 
> > jens, hch, tejun, and co, do you guys have a preference how this
> > should be handled so that scsi_try_to_abort_cmd() can use proper
> > atomic struct request bits here and we can get rid of this (racy..?)
> > method of using struct scsi_cmnd->serial_number for anything wrt to
> > per struct scsi_cmnd context timeout handling.
> 
> Just add a
> 
> static inline int blk_test_rq_complete(struct request *rq)
> {
>         return test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
> }
> 
> in block/blk.h
> 
> I need this too for some lockless completion patches I am playing with.
> 

Hi Jens,

Perfect, thanks for the heads up on this wrapper.  I will go ahead and
include this into a RFCv4 series and update scsi_try_to_abort_cmd()
accordingly.

Best,

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Fri, 2010-09-24 at 11:33 -0700, Mike Anderson wrote:
> Jens Axboe <axboe@kernel.dk> wrote:
> > On 2010-09-23 23:46, Nicholas A. Bellinger wrote:
> > > On Sat, 2010-09-18 at 12:58 -0700, Nicholas A. Bellinger wrote:
> > >> On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:

<SNIP>

> > > Greetings Mike and Co,
> > > 
> > > I was doing some followup on these items for a v3 series and started
> > > with a patch following mnc's recommendations for dropping the
> > > scsi_error.c codes depending upon struct scsi_cmnd->serial_number:
> > > 
> > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> > > index 1de30eb..f35c127 100644
> > > --- a/drivers/scsi/scsi_error.c
> > > +++ b/drivers/scsi/scsi_error.c
> > > @@ -644,11 +644,7 @@ 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)
> > > -        */
> > > -       if (scmd->serial_number == 0)
> > > +       if (test_bit(REQ_ATOM_COMPLETE, &scmd->request->atomic_flags))
> > >                 return SUCCESS;
> > >         return __scsi_try_to_abort_cmd(scmd);
> > >  }
> > > 
> > > and while building I noticed that the simple single enum
> > > REQ_ATOM_COMPLETE=0 is
> > 
> > > currently located in block/blk.h and along with blk_mark_rq_complete()
> > > and blk_clear_rq_complete() for setting this bit within struct
> > > request->atomic_flags.
> > > 
> > > jens, hch, tejun, and co, do you guys have a preference how this
> > > should be handled so that scsi_try_to_abort_cmd() can use proper
> > > atomic struct request bits here and we can get rid of this (racy..?)
> > > method of using struct scsi_cmnd->serial_number for anything wrt to
> > > per struct scsi_cmnd context timeout handling.
> > 
> > Just add a
> > 
> > static inline int blk_test_rq_complete(struct request *rq)
> > {
> >         return test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags);
> > }
> > 
> > in block/blk.h
> > 
> > I need this too for some lockless completion patches I am playing with.
> 

Greetings Mike,

> In reviewing the current code paths wasn't the serial_number also used to
> avoid calling __scsi_try_to_abort_cmd for START_UNIT case also.

Not sure on this item, but checking on this now..

> 
> If we skip __scsi_try_to_abort_cmd when REQ_ATOM_COMPLETE is set it would
> be correct for the scsi_decide_disposition cases but it would appear this
> would stop __scsi_try_to_abort_cmd from being called in the time out
> case as REQ_ATOM_COMPLETE is set prior to calling blk_rq_timed_out.

Hmmmmmmm..

> 
> 1.) Request timed out path to scsi_eh_scmd_add.
> 
> blk_rq_timed_out_timer
> 	...
> 	if (blk_mark_rq_complete(rq))
> 		continue;
> 	blk_rq_timed_out
> 		q->rq_timed_out_fn "scsi_times_out"
> 			scsi_times_out
> 				scsi_eh_scmd_add
> 
> 2.) Request completion path to scsi_eh_scmd_add based on
> scsi_decide_disposition disposition set to FAILED (actually
> scsi_eh_scmd_add called from the default case with the disposition of
> FAILED appearing to be the only disposition returned that would hit this
> case vs. the other three handled cases). This should not be a common path
> outside of handling allow_restart.
> 
> blk_complete_request
> 	...
> 	if (!blk_mark_rq_complete(req))
> 		__blk_complete_request
> 			...
> 			raise_softirq_irqoff(BLOCK_SOFTIRQ);
> blk_done_softirq
> 	rq->q->softirq_done_fn "scsi_softirq_done"
> 		scsi_softirq_done
> 			scsi_eh_scmd_add
> 
> 3.) Call path to scsi_try_to_abort_cmd.
> scsi_error_handler
> 	scsi_unjam_host
> 		scsi_eh_abort_cmds
> 			scsi_try_to_abort_cmd
> Thanks,
> 

Ok, what I think is being said here is that the usage of
blk_test_rq_complete() in scsi_try_to_abort_cmd() completly breaks 
__scsi_try_to_abort_cmd from being called from within the struct request
timeout handler, right..?

Not being exquistiely fimilar with the scsi_error.c generic LLD logic w/
struct request timeouts code, I assume this means we need to revisit
scsi_unjam_host() once again to take into account the new
blk_test_rq_complete() calls from the normal fast path block softirq in
order to be able to handle the individual struct request timeout cases
for outstanding struct scsi_cmnd when they individual timer contexts
fire.

Perhaps we should be setting another bit in struct request->atomic_flags
to signal the REQ_BLKSOFTIRQ_COMPLETE to let scsi_unjam_host() know what
is going on..?

Thanks for your comments Mike!

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Mike Christie]

On 09/24/2010 01:33 PM, Mike Anderson wrote:
> In reviewing the current code paths wasn't the serial_number also used to
> avoid calling __scsi_try_to_abort_cmd for START_UNIT case also.

You mean from scsi_eh_try_stu....->__scsi_try_to_abort_cmd? How does it 
work for that case or what is the code path? Is it when it is called 
from the sas code through scsi_eh_ready_devs?


>
> If we skip __scsi_try_to_abort_cmd when REQ_ATOM_COMPLETE is set it would
> be correct for the scsi_decide_disposition cases but it would appear this
> would stop __scsi_try_to_abort_cmd from being called in the time out
> case as REQ_ATOM_COMPLETE is set prior to calling blk_rq_timed_out.
>
> 1.) Request timed out path to scsi_eh_scmd_add.
>
> blk_rq_timed_out_timer
> 	...
> 	if (blk_mark_rq_complete(rq))
> 		continue;
> 	blk_rq_timed_out
> 		q->rq_timed_out_fn "scsi_times_out"
> 			scsi_times_out
> 				scsi_eh_scmd_add
>

You are right.

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Christof Schmitt]

On Sat, Sep 18, 2010 at 12:57:58PM -0700, Nicholas A. Bellinger wrote:
> On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
> > On 09/17/2010 02:03 PM, Joe Eykholt wrote:
> > >
> > > How about instead of adding use_serial_number, let's just have the
> > > drivers that want a serial number call scsi_cmd_get_serial()
> > 
> > I think this sounds better.
> > 
> 
> In that case I will go ahead and add explict scsi_cmd_get_serial() calls
> to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
> an obvious and simple informational purpose.
> 
> > You could also convert drivers to the host tagging if you needed a 
> > unique id for each command sent to a host.
> 
> Hmmm, what does this entail again..?
> 
> > 
> > > and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> > > used in debug messages in some drivers.  I didn't find other usages
> > > but didn't do an exhaustive search.
> > 
> > The comments for serial_number say that it is only supposed to be used 
> > for debugging printks and most drivers use it for that.
> 
> So I would suppose it would be OK for those drivers to continue to
> printk serial_number to show the internal serial_number allocation is
> now disabled by default.
> 
> > However, it looks like mpt and dpt_i2o are using it for error handling
> > and/or lookup type of operations. I think the mpt* uses are not needed
> > in the abort checks.
> > 
> 
> In that case, then adding an explict scsi_cmd_get_serial() call in mpt*
> and dpt_i20 ->queuecommand() callers would be a good first step.
> 
> > And eata is using it for ordering and tracking or something. It could 
> > probably be converted to the host tagging if or what you suggested if it 
> > needs the uniqueue id.
> 
> Adding an explict scsi_cmd_get_serial() for eata for now, and we can
> consider LLD canidates for host tag conversion as a future step.
> 
> > 
> > zfcp looks like it copies it. It does not look like the driver needs it.
> > 
> 
> Ok, I will look at removing it's usage in zfcp or if necessary add an
> the explict scsi_cmd_get_serial() call.

In zfcp, the serial_number is only accessed for writing it to debug
traces. With the change that the driver has to request the serial
number through use_serial_number, simply remove the serial number from
zfcp.

Thanks,

Christof

> 
> > scsi_error.c uses it in scsi_try_to_abort_cmd to check if a command has 
> > completed, but I think that can be done by checking if REQ_ATOM_COMPLETE 
> > is set.
> 
> Hmmm, good catch here.  Jejb and hch, does this item work for you..?
> 
> If so then I will take another peek for any ML uses of a struct
> scsi_cmnd->serial_number that need to be addressed, and include Joe's
> and Mike's recommendations into a v3 series.
> 
> Thanks for your comments here Mike!
> 
> --nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Nicholas A. Bellinger]

On Mon, 2010-09-27 at 16:05 +0200, Christof Schmitt wrote:
> On Sat, Sep 18, 2010 at 12:57:58PM -0700, Nicholas A. Bellinger wrote:
> > On Fri, 2010-09-17 at 21:45 -0500, Mike Christie wrote:
> > > On 09/17/2010 02:03 PM, Joe Eykholt wrote:
> > > >
> > > > How about instead of adding use_serial_number, let's just have the
> > > > drivers that want a serial number call scsi_cmd_get_serial()
> > > 
> > > I think this sounds better.
> > > 
> > 
> > In that case I will go ahead and add explict scsi_cmd_get_serial() calls
> > to the LLDs that use struct scsi_cmnd->serial_number in anything beyond
> > an obvious and simple informational purpose.
> > 
> > > You could also convert drivers to the host tagging if you needed a 
> > > unique id for each command sent to a host.
> > 
> > Hmmm, what does this entail again..?
> > 
> > > 
> > > > and stop calling it from scsi_dispatch_cmd()?   AFAICT, it's only
> > > > used in debug messages in some drivers.  I didn't find other usages
> > > > but didn't do an exhaustive search.
> > > 
> > > The comments for serial_number say that it is only supposed to be used 
> > > for debugging printks and most drivers use it for that.
> > 
> > So I would suppose it would be OK for those drivers to continue to
> > printk serial_number to show the internal serial_number allocation is
> > now disabled by default.
> > 

<SNIP>

> > > 
> > > zfcp looks like it copies it. It does not look like the driver needs it.
> > > 
> > 
> > Ok, I will look at removing it's usage in zfcp or if necessary add an
> > the explict scsi_cmd_get_serial() call.
> 
> In zfcp, the serial_number is only accessed for writing it to debug
> traces. With the change that the driver has to request the serial
> number through use_serial_number, simply remove the serial number from
> zfcp.

Hi Christof,

So since the use of struct scsi_cmnd->serial_number is purely
informational for some LLDs (including zfcp) , I have been putting the
conversion of these LLDs as a low priority ATM.   I would happily accept
a patch to remove these.  8-)

Best,

--nab

Re: [PATCH v2 01/11] scsi: Convert struct Scsi_Host->cmd_serial_number to atomic_t

[Matthew Wilcox]

On Fri, Sep 17, 2010 at 09:45:10PM -0500, Mike Christie wrote:
> You could also convert drivers to the host tagging if you needed a
> unique id for each command sent to a host.

I just wanted to make people aware of a problem with the host tagging
-- scsi_cmnd->tag is always 0!

Watch this:

scsi_prep_fn:
		ret = scsi_setup_blk_pc_cmnd(sdev, req);

scsi_setup_blk_pc_cmnd:
	cmd = scsi_get_cmd_from_req(sdev, req);

scsi_get_cmd_from_req:
	cmd->tag = req->tag;

scsi_request_fn:
		if (!(blk_queue_tagged(q) && !blk_queue_start_tag(q, req)))
			blk_start_request(req);

blk_queue_start_tag
	rq->tag = tag;

(and yes, they're called in this order)

I encountered this while writing the UAS driver, and decided to just
use the rq->tag directly instead.  I would favour deleting scmd->tag in
order to save others the confusion.

In fact, looking at the users of scmd->tag, it appears there is a common
misconception that it contains the tag type (HEAD / SIMPLE / ORDERED).  I
have no idea where this comes from, but it means the following patch to
delete scmd's ->tag is almost certainly wrong ... but it preserves the
existing behaviour!


diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
index 5d2f148..37ff654 100644
--- a/drivers/scsi/NCR5380.c
+++ b/drivers/scsi/NCR5380.c
@@ -1531,7 +1531,6 @@ part2:
 	tmp[0] = IDENTIFY(((instance->irq == SCSI_IRQ_NONE) ? 0 : 1), cmd->device->lun);
 
 	len = 1;
-	cmd->tag = 0;
 
 	/* Send message(s) */
 	data = tmp;
@@ -2240,7 +2239,7 @@ static void NCR5380_information_transfer(struct Scsi_Host *instance) {
 					}
 					initialize_SCp(cmd->next_link);
 					/* The next command is still part of this process */
-					cmd->next_link->tag = cmd->tag;
+					cmd->next_link->tag = 0;
 					cmd->result = cmd->SCp.Status | (cmd->SCp.Message << 8);
 					dprintk(NDEBUG_LINKED, ("scsi%d : target %d lun %d linked request done, calling scsi_done().\n", instance->host_no, cmd->device->id, cmd->device->lun));
 					collect_stats(hostdata, cmd);
@@ -2604,7 +2603,7 @@ static void NCR5380_reselect(struct Scsi_Host *instance) {
 		do_abort(instance);
 	} else {
 		hostdata->connected = tmp;
-		dprintk(NDEBUG_RESELECTION, ("scsi%d : nexus established, target = %d, lun = %d, tag = %d\n", instance->host_no, tmp->target, tmp->lun, tmp->tag));
+		dprintk(NDEBUG_RESELECTION, ("scsi%d : nexus established, target = %d, lun = %d, tag = %d\n", instance->host_no, tmp->target, tmp->lun, 0));
 	}
 }
 
@@ -2789,7 +2788,7 @@ static int NCR5380_abort(Scsi_Cmnd * cmd) {
 		if (cmd == tmp) {
 			dprintk(NDEBUG_ABORT, ("scsi%d : aborting disconnected command.\n", instance->host_no));
 
-			if (NCR5380_select(instance, cmd, (int) cmd->tag))
+			if (NCR5380_select(instance, cmd, 0))
 				return FAILED;
 			dprintk(NDEBUG_ABORT, ("scsi%d : nexus reestablished.\n", instance->host_no));
 
diff --git a/drivers/scsi/bfa/bfa_cb_ioim_macros.h b/drivers/scsi/bfa/bfa_cb_ioim_macros.h
index 3906ed9..57f2f7d 100644
--- a/drivers/scsi/bfa/bfa_cb_ioim_macros.h
+++ b/drivers/scsi/bfa/bfa_cb_ioim_macros.h
@@ -143,19 +143,8 @@ bfa_cb_ioim_get_taskattr(struct bfad_ioim_s *dio)
 	struct scsi_cmnd *cmnd = (struct scsi_cmnd *)dio;
 	u8         task_attr = UNTAGGED;
 
-	if (cmnd->device->tagged_supported) {
-		switch (cmnd->tag) {
-		case HEAD_OF_QUEUE_TAG:
-			task_attr = HEAD_OF_Q;
-			break;
-		case ORDERED_QUEUE_TAG:
-			task_attr = ORDERED_Q;
-			break;
-		default:
-			task_attr = SIMPLE_Q;
-			break;
-		}
-	}
+	if (cmnd->device->tagged_supported)
+		task_attr = SIMPLE_Q;
 
 	return task_attr;
 }
diff --git a/drivers/scsi/libsrp.c b/drivers/scsi/libsrp.c
index ff6a28c..f506970 100644
--- a/drivers/scsi/libsrp.c
+++ b/drivers/scsi/libsrp.c
@@ -431,7 +431,7 @@ int srp_cmd_queue(struct Scsi_Host *shost, struct srp_cmd *cmd, void *info,
 	memcpy(sc->cmnd, cmd->cdb, MAX_COMMAND_SIZE);
 	sc->sdb.length = len;
 	sc->sdb.table.sgl = (void *) (unsigned long) addr;
-	sc->tag = tag;
+	sc->request->tag = tag;
 	err = scsi_tgt_queue_command(sc, itn_id, (struct scsi_lun *)&cmd->lun,
 				     cmd->tag);
 	if (err)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index ee02d38..42e40de 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1031,8 +1031,6 @@ static struct scsi_cmnd *scsi_get_cmd_from_req(struct scsi_device *sdev,
 		cmd = req->special;
 	}
 
-	/* pull a tag out of the request if we have one */
-	cmd->tag = req->tag;
 	cmd->request = req;
 
 	cmd->cmnd = req->cmd;
diff --git a/drivers/scsi/scsi_tgt_if.c b/drivers/scsi/scsi_tgt_if.c
index a87e21c..c00e363 100644
--- a/drivers/scsi/scsi_tgt_if.c
+++ b/drivers/scsi/scsi_tgt_if.c
@@ -117,7 +117,7 @@ int scsi_tgt_uspace_send_cmd(struct scsi_cmnd *cmd, u64 itn_id,
 	ev.p.cmd_req.data_len = scsi_bufflen(cmd);
 	memcpy(ev.p.cmd_req.scb, cmd->cmnd, sizeof(ev.p.cmd_req.scb));
 	memcpy(ev.p.cmd_req.lun, lun, sizeof(ev.p.cmd_req.lun));
-	ev.p.cmd_req.attribute = cmd->tag;
+	ev.p.cmd_req.attribute = 0;
 	ev.p.cmd_req.tag = tag;
 
 	dprintk("%p %d %u %x %llx\n", cmd, shost->host_no,
diff --git a/drivers/scsi/vmw_pvscsi.c b/drivers/scsi/vmw_pvscsi.c
index 2689445..39cae91 100644
--- a/drivers/scsi/vmw_pvscsi.c
+++ b/drivers/scsi/vmw_pvscsi.c
@@ -665,10 +665,6 @@ static int pvscsi_queue_ring(struct pvscsi_adapter *adapter,
 	memcpy(e->cdb, cmd->cmnd, e->cdbLen);
 
 	e->tag = SIMPLE_QUEUE_TAG;
-	if (sdev->tagged_supported &&
-	    (cmd->tag == HEAD_OF_QUEUE_TAG ||
-	     cmd->tag == ORDERED_QUEUE_TAG))
-		e->tag = cmd->tag;
 
 	if (cmd->sc_data_direction == DMA_FROM_DEVICE)
 		e->flags = PVSCSI_FLAG_CMD_DIR_TOHOST;
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a5e885a..829e341 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -127,8 +127,6 @@ struct scsi_cmnd {
 					 * to be at an address < 16Mb). */
 
 	int result;		/* Status code from lower level driver */
-
-	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);

[PATCH] zfcp: Remove scsi_cmnd->serial_number from debug traces

[Christof Schmitt]

From: Christof Schmitt <christof.schmitt@de.ibm.com>

With the change that drivers have to explicitly request the serial
number for SCSI commands, this field should not be part of the zfcp
traces. It is not worth the effort to request the serial number only
for tracing purposes, so simply remove this field from the debug
traces.

Reviewed-by: Swen Schillig <swen@vnet.ibm.com>
Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
---
 drivers/s390/scsi/zfcp_dbf.c |    4 ----
 drivers/s390/scsi/zfcp_dbf.h |    2 --
 2 files changed, 6 deletions(-)

--- a/drivers/s390/scsi/zfcp_dbf.c
+++ b/drivers/s390/scsi/zfcp_dbf.c
@@ -154,7 +154,6 @@ void _zfcp_dbf_hba_fsf_response(const ch
 		scsi_cmnd = (struct scsi_cmnd *)fsf_req->data;
 		if (scsi_cmnd) {
 			response->u.fcp.cmnd = (unsigned long)scsi_cmnd;
-			response->u.fcp.serial = scsi_cmnd->serial_number;
 			response->u.fcp.data_dir =
 				qtcb->bottom.io.data_direction;
 		}
@@ -330,7 +329,6 @@ static void zfcp_dbf_hba_view_response(c
 			break;
 		zfcp_dbf_out(p, "data_direction", "0x%04x", r->u.fcp.data_dir);
 		zfcp_dbf_out(p, "scsi_cmnd", "0x%0Lx", r->u.fcp.cmnd);
-		zfcp_dbf_out(p, "scsi_serial", "0x%016Lx", r->u.fcp.serial);
 		*p += sprintf(*p, "\n");
 		break;
 
@@ -881,7 +879,6 @@ void _zfcp_dbf_scsi(const char *tag, con
 				}
 				rec->scsi_result = scsi_cmnd->result;
 				rec->scsi_cmnd = (unsigned long)scsi_cmnd;
-				rec->scsi_serial = scsi_cmnd->serial_number;
 				memcpy(rec->scsi_opcode, scsi_cmnd->cmnd,
 					min((int)scsi_cmnd->cmd_len,
 						ZFCP_DBF_SCSI_OPCODE));
@@ -950,7 +947,6 @@ static int zfcp_dbf_scsi_view_format(deb
 	zfcp_dbf_out(&p, "scsi_lun", "0x%08x", r->scsi_lun);
 	zfcp_dbf_out(&p, "scsi_result", "0x%08x", r->scsi_result);
 	zfcp_dbf_out(&p, "scsi_cmnd", "0x%0Lx", r->scsi_cmnd);
-	zfcp_dbf_out(&p, "scsi_serial", "0x%016Lx", r->scsi_serial);
 	zfcp_dbf_outd(&p, "scsi_opcode", r->scsi_opcode, ZFCP_DBF_SCSI_OPCODE,
 		      0, ZFCP_DBF_SCSI_OPCODE);
 	zfcp_dbf_out(&p, "scsi_retries", "0x%02x", r->scsi_retries);
--- a/drivers/s390/scsi/zfcp_dbf.h
+++ b/drivers/s390/scsi/zfcp_dbf.h
@@ -110,7 +110,6 @@ struct zfcp_dbf_hba_record_response {
 	union {
 		struct {
 			u64 cmnd;
-			u64 serial;
 			u32 data_dir;
 		} fcp;
 		struct {
@@ -206,7 +205,6 @@ struct zfcp_dbf_scsi_record {
 	u32 scsi_lun;
 	u32 scsi_result;
 	u64 scsi_cmnd;
-	u64 scsi_serial;
 #define ZFCP_DBF_SCSI_OPCODE	16
 	u8 scsi_opcode[ZFCP_DBF_SCSI_OPCODE];
 	u8 scsi_retries;

Re: [PATCH] zfcp: Remove scsi_cmnd->serial_number from debug traces

[Nicholas A. Bellinger]

On Tue, 2010-09-28 at 10:11 +0200, Christof Schmitt wrote:
> From: Christof Schmitt <christof.schmitt@de.ibm.com>
> 
> With the change that drivers have to explicitly request the serial
> number for SCSI commands, this field should not be part of the zfcp
> traces. It is not worth the effort to request the serial number only
> for tracing purposes, so simply remove this field from the debug
> traces.
> 
> Reviewed-by: Swen Schillig <swen@vnet.ibm.com>
> Signed-off-by: Christof Schmitt <christof.schmitt@de.ibm.com>
> ---

Hi Christof,

This patch has been commited into the drop-host_lock-v5 branch and will
be included in the next RFC posting.

Thanks!

--nab

>  drivers/s390/scsi/zfcp_dbf.c |    4 ----
>  drivers/s390/scsi/zfcp_dbf.h |    2 --
>  2 files changed, 6 deletions(-)
> 
> --- a/drivers/s390/scsi/zfcp_dbf.c
> +++ b/drivers/s390/scsi/zfcp_dbf.c
> @@ -154,7 +154,6 @@ void _zfcp_dbf_hba_fsf_response(const ch
>  		scsi_cmnd = (struct scsi_cmnd *)fsf_req->data;
>  		if (scsi_cmnd) {
>  			response->u.fcp.cmnd = (unsigned long)scsi_cmnd;
> -			response->u.fcp.serial = scsi_cmnd->serial_number;
>  			response->u.fcp.data_dir =
>  				qtcb->bottom.io.data_direction;
>  		}
> @@ -330,7 +329,6 @@ static void zfcp_dbf_hba_view_response(c
>  			break;
>  		zfcp_dbf_out(p, "data_direction", "0x%04x", r->u.fcp.data_dir);
>  		zfcp_dbf_out(p, "scsi_cmnd", "0x%0Lx", r->u.fcp.cmnd);
> -		zfcp_dbf_out(p, "scsi_serial", "0x%016Lx", r->u.fcp.serial);
>  		*p += sprintf(*p, "\n");
>  		break;
>  
> @@ -881,7 +879,6 @@ void _zfcp_dbf_scsi(const char *tag, con
>  				}
>  				rec->scsi_result = scsi_cmnd->result;
>  				rec->scsi_cmnd = (unsigned long)scsi_cmnd;
> -				rec->scsi_serial = scsi_cmnd->serial_number;
>  				memcpy(rec->scsi_opcode, scsi_cmnd->cmnd,
>  					min((int)scsi_cmnd->cmd_len,
>  						ZFCP_DBF_SCSI_OPCODE));
> @@ -950,7 +947,6 @@ static int zfcp_dbf_scsi_view_format(deb
>  	zfcp_dbf_out(&p, "scsi_lun", "0x%08x", r->scsi_lun);
>  	zfcp_dbf_out(&p, "scsi_result", "0x%08x", r->scsi_result);
>  	zfcp_dbf_out(&p, "scsi_cmnd", "0x%0Lx", r->scsi_cmnd);
> -	zfcp_dbf_out(&p, "scsi_serial", "0x%016Lx", r->scsi_serial);
>  	zfcp_dbf_outd(&p, "scsi_opcode", r->scsi_opcode, ZFCP_DBF_SCSI_OPCODE,
>  		      0, ZFCP_DBF_SCSI_OPCODE);
>  	zfcp_dbf_out(&p, "scsi_retries", "0x%02x", r->scsi_retries);
> --- a/drivers/s390/scsi/zfcp_dbf.h
> +++ b/drivers/s390/scsi/zfcp_dbf.h
> @@ -110,7 +110,6 @@ struct zfcp_dbf_hba_record_response {
>  	union {
>  		struct {
>  			u64 cmnd;
> -			u64 serial;
>  			u32 data_dir;
>  		} fcp;
>  		struct {
> @@ -206,7 +205,6 @@ struct zfcp_dbf_scsi_record {
>  	u32 scsi_lun;
>  	u32 scsi_result;
>  	u64 scsi_cmnd;
> -	u64 scsi_serial;
>  #define ZFCP_DBF_SCSI_OPCODE	16
>  	u8 scsi_opcode[ZFCP_DBF_SCSI_OPCODE];
>  	u8 scsi_retries;