[PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Tejun Heo]

SCSI command completion path bumps ioerr_cnt whenever scsi_cmd->result
isn't zero; unfortunately, this means that non-error sense reporting
bumps the counter too.  This is pronounced with ATA passthrough
commands because most of them explicitly request the resulting
taskfile to be transported via sense data bumping the count
unconditionally.

Don't bump the counter if scsi_cmd->result simply indicates that sense
data is available.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dave Jones <dsj@fb.com>
---
 drivers/scsi/scsi_lib.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4..e90e3f7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request *rq)
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	atomic_inc(&cmd->device->iodone_cnt);
-	if (cmd->result)
+	if (cmd->result &&
+	    cmd->result != ((DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION))
 		atomic_inc(&cmd->device->ioerr_cnt);
 
 	disposition = scsi_decide_disposition(cmd);

[PATCH REPOST] scsi: don't count non-failure CHECK_CONDITION as error

[Tejun Heo]

SCSI command completion path bumps ioerr_cnt whenever scsi_cmd->result
isn't zero; unfortunately, this means that non-error sense reporting
bumps the counter too.  This is pronounced with ATA passthrough
commands because most of them explicitly request the resulting
taskfile to be transported via sense data bumping the count
unconditionally.

Don't bump the counter if scsi_cmd->result simply indicates that sense
data is available.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dave Jones <dsj@fb.com>
---
Hello,

Reposting because the email address listed under "SCSI SUBSYSTEM",
JBottomley@odin.com, bounced.  James, can you please update the entry?

Thanks.

 drivers/scsi/scsi_lib.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa6b2c4..e90e3f7 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request *rq)
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	atomic_inc(&cmd->device->iodone_cnt);
-	if (cmd->result)
+	if (cmd->result &&
+	    cmd->result != ((DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION))
 		atomic_inc(&cmd->device->ioerr_cnt);
 
 	disposition = scsi_decide_disposition(cmd);

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Hannes Reinecke]

On 01/14/2016 10:46 PM, Tejun Heo wrote:
> SCSI command completion path bumps ioerr_cnt whenever scsi_cmd->result
> isn't zero; unfortunately, this means that non-error sense reporting
> bumps the counter too.  This is pronounced with ATA passthrough
> commands because most of them explicitly request the resulting
> taskfile to be transported via sense data bumping the count
> unconditionally.
>
> Don't bump the counter if scsi_cmd->result simply indicates that sense
> data is available.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Dave Jones <dsj@fb.com>
> ---
>   drivers/scsi/scsi_lib.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index fa6b2c4..e90e3f7 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request *rq)
>   	INIT_LIST_HEAD(&cmd->eh_entry);
>
>   	atomic_inc(&cmd->device->iodone_cnt);
> -	if (cmd->result)
> +	if (cmd->result &&
> +	    cmd->result != ((DRIVER_SENSE << 24) | SAM_STAT_CHECK_CONDITION))
>   		atomic_inc(&cmd->device->ioerr_cnt);
>
>   	disposition = scsi_decide_disposition(cmd);

Reviewed-by: Hannes Reinecke <hare@suse.com>

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
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

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[James Bottomley]

On Thu, 2016-01-14 at 16:46 -0500, Tejun Heo wrote:
> SCSI command completion path bumps ioerr_cnt whenever scsi_cmd
> ->result isn't zero; unfortunately, this means that non-error sense 
> reporting bumps the counter too.  This is pronounced with ATA 
> passthrough commands because most of them explicitly request the 
> resulting taskfile to be transported via sense data bumping the count
> unconditionally.
> 
> Don't bump the counter if scsi_cmd->result simply indicates that 
> sense data is available.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Dave Jones <dsj@fb.com>
> ---
>  drivers/scsi/scsi_lib.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index fa6b2c4..e90e3f7 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request
> *rq)
>  	INIT_LIST_HEAD(&cmd->eh_entry);
>  
>  	atomic_inc(&cmd->device->iodone_cnt);
> -	if (cmd->result)
> +	if (cmd->result &&
> +	    cmd->result != ((DRIVER_SENSE << 24) |
> SAM_STAT_CHECK_CONDITION))
>  		atomic_inc(&cmd->device->ioerr_cnt);

OK, it makes sense to me that we don't include non-error check
conditions.  However, then you shouldn't be checking DRIVER_SENSE.  We
still have a few drivers that rely on the error handler to fetch sense
explicitly ... they could eventually return non-error conditions as
well.

James

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[James Bottomley]

On Fri, 2016-01-15 at 07:46 -0800, James Bottomley wrote:
> On Thu, 2016-01-14 at 16:46 -0500, Tejun Heo wrote:
> > SCSI command completion path bumps ioerr_cnt whenever scsi_cmd
> > ->result isn't zero; unfortunately, this means that non-error sense
> > reporting bumps the counter too.  This is pronounced with ATA 
> > passthrough commands because most of them explicitly request the 
> > resulting taskfile to be transported via sense data bumping the
> > count
> > unconditionally.
> > 
> > Don't bump the counter if scsi_cmd->result simply indicates that 
> > sense data is available.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Reported-by: Dave Jones <dsj@fb.com>
> > ---
> >  drivers/scsi/scsi_lib.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index fa6b2c4..e90e3f7 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request
> > *rq)
> >  	INIT_LIST_HEAD(&cmd->eh_entry);
> >  
> >  	atomic_inc(&cmd->device->iodone_cnt);
> > -	if (cmd->result)
> > +	if (cmd->result &&
> > +	    cmd->result != ((DRIVER_SENSE << 24) |
> > SAM_STAT_CHECK_CONDITION))
> >  		atomic_inc(&cmd->device->ioerr_cnt);
> 
> OK, it makes sense to me that we don't include non-error check
> conditions.  However, then you shouldn't be checking DRIVER_SENSE. 
>  We still have a few drivers that rely on the error handler to fetch
> sense explicitly ... they could eventually return non-error 
> conditions as well.

Actually, I take this back: if we add your proposal, we never increment
the ioerr_cnt even for sense returns indicating failure.  That looks to
be even worse than incrementing it too often.

The other problem is that if we do this for you, we should do the same
for SCSI with BUSY and QUEUE_FULL ... they indicate temporary retry
conditions and shouldn't be treated as errors.

I'll stop looking now before I find any more problems with the
statistics code ...  I think it needs a rethink.

James

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Douglas Gilbert]

On 16-01-15 04:55 PM, James Bottomley wrote:
> On Fri, 2016-01-15 at 07:46 -0800, James Bottomley wrote:
>> On Thu, 2016-01-14 at 16:46 -0500, Tejun Heo wrote:
>>> SCSI command completion path bumps ioerr_cnt whenever scsi_cmd
>>> ->result isn't zero; unfortunately, this means that non-error sense
>>> reporting bumps the counter too.  This is pronounced with ATA
>>> passthrough commands because most of them explicitly request the
>>> resulting taskfile to be transported via sense data bumping the
>>> count
>>> unconditionally.
>>>
>>> Don't bump the counter if scsi_cmd->result simply indicates that
>>> sense data is available.
>>>
>>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>> Reported-by: Dave Jones <dsj@fb.com>
>>> ---
>>>   drivers/scsi/scsi_lib.c |    3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>>> index fa6b2c4..e90e3f7 100644
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct request
>>> *rq)
>>>   	INIT_LIST_HEAD(&cmd->eh_entry);
>>>
>>>   	atomic_inc(&cmd->device->iodone_cnt);
>>> -	if (cmd->result)
>>> +	if (cmd->result &&
>>> +	    cmd->result != ((DRIVER_SENSE << 24) |
>>> SAM_STAT_CHECK_CONDITION))
>>>   		atomic_inc(&cmd->device->ioerr_cnt);
>>
>> OK, it makes sense to me that we don't include non-error check
>> conditions.  However, then you shouldn't be checking DRIVER_SENSE.
>>   We still have a few drivers that rely on the error handler to fetch
>> sense explicitly ... they could eventually return non-error
>> conditions as well.
>
> Actually, I take this back: if we add your proposal, we never increment
> the ioerr_cnt even for sense returns indicating failure.  That looks to
> be even worse than incrementing it too often.
>
> The other problem is that if we do this for you, we should do the same
> for SCSI with BUSY and QUEUE_FULL ... they indicate temporary retry
> conditions and shouldn't be treated as errors.
>
> I'll stop looking now before I find any more problems with the
> statistics code ...  I think it needs a rethink.

SCSI status and sense data is non-trivial to decode. It looks
like someone thought that one-liner would bypass a lot of hard
work. Most of the time sense data indicates an error but not
always. Even worse, it can contain vendor specific codes. If
this statistic is on the fast path then IMO it should be retired
(and any others like it). For backward compatibility set it to
0 once at initialization and document the change. Or you could
have a discouraging kernel config option such as
CONFIG_EXPENSIVE_SCSI_STATISTICS ("nonsensical" is another term
that comes to mind).

Doug Gilbert

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[James Bottomley]

On Fri, 2016-01-15 at 17:50 +0100, Douglas Gilbert wrote:
> On 16-01-15 04:55 PM, James Bottomley wrote:
> > On Fri, 2016-01-15 at 07:46 -0800, James Bottomley wrote:
> > > On Thu, 2016-01-14 at 16:46 -0500, Tejun Heo wrote:
> > > > SCSI command completion path bumps ioerr_cnt whenever scsi_cmd
> > > > ->result isn't zero; unfortunately, this means that non-error
> > > > sense
> > > > reporting bumps the counter too.  This is pronounced with ATA
> > > > passthrough commands because most of them explicitly request
> > > > the
> > > > resulting taskfile to be transported via sense data bumping the
> > > > count
> > > > unconditionally.
> > > > 
> > > > Don't bump the counter if scsi_cmd->result simply indicates
> > > > that
> > > > sense data is available.
> > > > 
> > > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > > Reported-by: Dave Jones <dsj@fb.com>
> > > > ---
> > > >   drivers/scsi/scsi_lib.c |    3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > > index fa6b2c4..e90e3f7 100644
> > > > --- a/drivers/scsi/scsi_lib.c
> > > > +++ b/drivers/scsi/scsi_lib.c
> > > > @@ -1622,7 +1622,8 @@ static void scsi_softirq_done(struct
> > > > request
> > > > *rq)
> > > >   	INIT_LIST_HEAD(&cmd->eh_entry);
> > > > 
> > > >   	atomic_inc(&cmd->device->iodone_cnt);
> > > > -	if (cmd->result)
> > > > +	if (cmd->result &&
> > > > +	    cmd->result != ((DRIVER_SENSE << 24) |
> > > > SAM_STAT_CHECK_CONDITION))
> > > >   		atomic_inc(&cmd->device->ioerr_cnt);
> > > 
> > > OK, it makes sense to me that we don't include non-error check
> > > conditions.  However, then you shouldn't be checking
> > > DRIVER_SENSE.
> > >   We still have a few drivers that rely on the error handler to
> > > fetch
> > > sense explicitly ... they could eventually return non-error
> > > conditions as well.
> > 
> > Actually, I take this back: if we add your proposal, we never
> > increment
> > the ioerr_cnt even for sense returns indicating failure.  That
> > looks to
> > be even worse than incrementing it too often.
> > 
> > The other problem is that if we do this for you, we should do the
> > same
> > for SCSI with BUSY and QUEUE_FULL ... they indicate temporary retry
> > conditions and shouldn't be treated as errors.
> > 
> > I'll stop looking now before I find any more problems with the
> > statistics code ...  I think it needs a rethink.
> 
> SCSI status and sense data is non-trivial to decode. It looks
> like someone thought that one-liner would bypass a lot of hard
> work. Most of the time sense data indicates an error but not
> always. Even worse, it can contain vendor specific codes. If
> this statistic is on the fast path then IMO it should be retired
> (and any others like it). For backward compatibility set it to
> 0 once at initialization and document the change. Or you could
> have a discouraging kernel config option such as
> CONFIG_EXPENSIVE_SCSI_STATISTICS ("nonsensical" is another term
> that comes to mind).

Well, I can see sense in having an error count of everything that comes
back that's not good status because it's easy and has a well defined
meaning (calling it the "error count" is more debatable, agreed).  It
appears that Dave and Tejun want the count to mean something else. 
 Lets treat this as a feature exercise: Dave and Tejun, what do you
want, then we can see if we could add an additional counter giving you
that.

James

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Tejun Heo]

Hello, James.

On Fri, Jan 15, 2016 at 10:35:34AM -0800, James Bottomley wrote:
> Well, I can see sense in having an error count of everything that comes
> back that's not good status because it's easy and has a well defined
> meaning (calling it the "error count" is more debatable, agreed).  It
> appears that Dave and Tejun want the count to mean something else. 
>  Lets treat this as a feature exercise: Dave and Tejun, what do you
> want, then we can see if we could add an additional counter giving you
> that.

Well, currently, for libata devices, all passthrough commands bump it
up because the result taskfile is reported via sense data, which is
pretty misleading for something named ioerr_cnt.  Maybe we can ignore
CHECK_CONDITION for ATA passthrough commands but special casing them
can be confusing in other ways.

Thanks.

-- 
tejun

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[James Bottomley]

On Fri, 2016-01-15 at 13:42 -0500, Tejun Heo wrote:
> Hello, James.
> 
> On Fri, Jan 15, 2016 at 10:35:34AM -0800, James Bottomley wrote:
> > Well, I can see sense in having an error count of everything that
> > comes
> > back that's not good status because it's easy and has a well
> > defined
> > meaning (calling it the "error count" is more debatable, agreed). 
> >  It
> > appears that Dave and Tejun want the count to mean something else. 
> >  Lets treat this as a feature exercise: Dave and Tejun, what do you
> > want, then we can see if we could add an additional counter giving
> > you
> > that.
> 
> Well, currently, for libata devices, all passthrough commands bump it
> up because the result taskfile is reported via sense data, which is
> pretty misleading for something named ioerr_cnt.  Maybe we can ignore
> CHECK_CONDITION for ATA passthrough commands but special casing them
> can be confusing in other ways.

That doesn't really help me pin down what you want.  I already said
ioerr_cnt is misleading, but we're stuck with the name, becuase it's an
ABI.

Under the definition I gave, the behaviour you currently see is
correct: all commands with non good status count as errors.  That
includes a ton of stuff I wouldn't classify as real errors, like queue
full status, deferred sense codes and even auto correct, which is why I
believe the term "error count" is misleading, but, as I said, we're
stuck with it.

If we want to change what is being counted, we have to change the
definition, so what is the definition you want to see for counting
errors? and what's the reason driving this change?

James

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Tejun Heo]

Hello, James.

On Fri, Jan 15, 2016 at 11:09:16AM -0800, James Bottomley wrote:
> If we want to change what is being counted, we have to change the
> definition, so what is the definition you want to see for counting
> errors? and what's the reason driving this change?

There isn't anything else to it than what I already wrote.  It goes up
for things which aren't failures making the counter essentially
useless.  It's confusing from userland as it's unintuitive that these
commands are wrapped in SCSI ATA passthrough commands which use sense
data for result reporting and doing equivalent operations on native
SCSI and ATA devices lead to different outcomes.  The counter as-is is
just useless for libata devices.  If it can be rectified easily,
great.  If not, it isn't anything critical.

Thanks.

-- 
tejun

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[James Bottomley]

On Fri, 2016-01-15 at 14:27 -0500, Tejun Heo wrote:
> Hello, James.
> 
> On Fri, Jan 15, 2016 at 11:09:16AM -0800, James Bottomley wrote:
> > If we want to change what is being counted, we have to change the
> > definition, so what is the definition you want to see for counting
> > errors? and what's the reason driving this change?
> 
> There isn't anything else to it than what I already wrote.  It goes
> up
> for things which aren't failures making the counter essentially
> useless.  It's confusing from userland as it's unintuitive that these
> commands are wrapped in SCSI ATA passthrough commands which use sense
> data for result reporting and doing equivalent operations on native
> SCSI and ATA devices lead to different outcomes.  The counter as-is
> is
> just useless for libata devices.  If it can be rectified easily,
> great.  If not, it isn't anything critical.

That's the point we were making: it can't be rectified easily.  Simply
counting non-good returns is easy.  Counting something that's closer to
errors is hard because first we have to define what an "error" is (I
could see queue full being not an error, but what about not ready or
initializing command required?) and then implement and infrastructure
to classify and count the returns, which means an awful lot of sense
data parsing.

James

Re: [PATCH] scsi: don't count non-failure CHECK_CONDITION as error

[Tejun Heo]

On Fri, Jan 15, 2016 at 11:36:08AM -0800, James Bottomley wrote:
> That's the point we were making: it can't be rectified easily.  Simply
> counting non-good returns is easy.  Counting something that's closer to
> errors is hard because first we have to define what an "error" is (I
> could see queue full being not an error, but what about not ready or
> initializing command required?) and then implement and infrastructure
> to classify and count the returns, which means an awful lot of sense
> data parsing.

Yeap, no objection.  Leaving as-is fine.

-- 
tejun