[PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[Rob Evers]

---
 drivers/scsi/scsi_lib.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fc3f168..9905e2f 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -920,12 +920,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			action = ACTION_FAIL;
 			break;
 		default:
-			description = "Unhandled sense code";
+			SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
+							   "Unhandled sense code\n"));
 			action = ACTION_FAIL;
 			break;
 		}
 	} else {
-		description = "Unhandled error code";
+		SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
+						   "Unhandled error code\n"));
 		action = ACTION_FAIL;
 	}
 
-- 
1.7.4.2

Re: [PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[James Bottomley]

On Fri, 2011-10-07 at 13:59 -0400, Rob Evers wrote:
> ---
>  drivers/scsi/scsi_lib.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index fc3f168..9905e2f 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -920,12 +920,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			action = ACTION_FAIL;
>  			break;
>  		default:
> -			description = "Unhandled sense code";
> +			SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
> +							   "Unhandled sense code\n"));
>  			action = ACTION_FAIL;
>  			break;
>  		}
>  	} else {
> -		description = "Unhandled error code";
> +		SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
> +						   "Unhandled error code\n"));

I just don't understand the necessity of this.  We're going to fail the
command and we'd like someone to understand why.

Before this patch we print

sdX: Unhandled sense code
sdX: <sense information>
sdX: <command information>

Which hopefully causes someone to investigate what the actual problem
is.

If I apply this patch we're going to print

sdX: <sense information>
sdX: <command information>

Which is even less helpful.

If you're seeing some error or sense code that's causing a common retry,
then perhaps we should add it to the codes we check for instead of
trying to hide it?

James

Re: [PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[Mike Christie]

On 10/07/2011 01:23 PM, James Bottomley wrote:
> If you're seeing some error or sense code that's causing a common retry,
> then perhaps we should add it to the codes we check for instead of
> trying to hide it?

We get questions about pretty much all of the error codes that do not
have a extra string and can get failed here.

The problem is with the use of the word unhandled. Users think for
retryable errors we did not retry, or they think it means even if they
have multipath/raid that the error is going to the application or FS so
they get worried and make extra support requests.

Was the patch that just changed the strings to ""Extended sense
description not available" and "Extended error description not available":
http://www.spinics.net/lists/linux-scsi/msg54874.html
ok?

I think in the end Rob is just trying to get out of having to figure out
strings for all the error codes :)

Re: [PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[Mike Christie]

On 10/07/2011 02:28 PM, Mike Christie wrote:
> On 10/07/2011 01:23 PM, James Bottomley wrote:
>> If you're seeing some error or sense code that's causing a common retry,
>> then perhaps we should add it to the codes we check for instead of
>> trying to hide it?
> 
> We get questions about pretty much all of the error codes that do not
> have a extra string and can get failed here.
> 
> The problem is with the use of the word unhandled. Users think for
> retryable errors we did not retry, or they think it means even if they
> have multipath/raid that the error is going to the application or FS so
> they get worried and make extra support requests.
> 
> Was the patch that just changed the strings to ""Extended sense
> description not available" and "Extended error description not available":
> http://www.spinics.net/lists/linux-scsi/msg54874.html
> ok?
> 
> I think in the end Rob is just trying to get out of having to figure out
> strings for all the error codes :)

I put a smiley face there, but I really think that at least for the host
byte error codes and scsi-ml error codes we should have a string.

Re: [PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[Rob Evers]

  On 10/07/2011 02:23 PM, James Bottomley wrote:
> On Fri, 2011-10-07 at 13:59 -0400, Rob Evers wrote:
>> ---
>>   drivers/scsi/scsi_lib.c |    6 ++++--
>>   1 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index fc3f168..9905e2f 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -920,12 +920,14 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>   			action = ACTION_FAIL;
>>   			break;
>>   		default:
>> -			description = "Unhandled sense code";
>> +			SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
>> +							   "Unhandled sense code\n"));
>>   			action = ACTION_FAIL;
>>   			break;
>>   		}
>>   	} else {
>> -		description = "Unhandled error code";
>> +		SCSI_LOG_HLCOMPLETE(1, scmd_printk(KERN_INFO, cmd,
>> +						   "Unhandled error code\n"));
> I just don't understand the necessity of this.  We're going to fail the
> command and we'd like someone to understand why.

As someone who looks at this code and try's to understand what
happens, I agree but this is not always the case with our general
consumers of this code.

> Before this patch we print
>
> sdX: Unhandled sense code
> sdX:<sense information>
> sdX:<command information>
>
> Which hopefully causes someone to investigate what the actual problem
> is.
>
> If I apply this patch we're going to print
>
> sdX:<sense information>
> sdX:<command information>
>
> Which is even less helpful.
>
> If you're seeing some error or sense code that's causing a common retry,
> then perhaps we should add it to the codes we check for instead of
> trying to hide it?

I only currently have one example of multiple unhandled error and sense 
conditions.

Perhaps I could collect the others.

> James
>
>

According to the report I got, there is no functional problem, just
noise in /var/log/messages when fabric faults are occurring.

This is occurring for both un-handled error and un-handled sense codes.

I've been asked to reduce the noise as "Unhandled error code" and
"Unhandled sense code" seem to be causing some concern.

Can you suggest something to reduce the user concerns but doesn't
loose the information as to what code path is executing before other
informational prints occur?

This is what I was trying to get at in the previous post where Mike
Christie suggested the message be 'softened' but then Hannes suggested
the change I have here.  Perhaps softening is the best course until all the
cases are collected and appropriate decisions are made regarding
the types of messages for each of them?

Rob

Re: [PATCH] log unhandled scsi error and sense messages via SCSI_LOG

[Rob Evers]

  On 10/07/2011 03:35 PM, Mike Christie wrote:
> On 10/07/2011 02:28 PM, Mike Christie wrote:
>> On 10/07/2011 01:23 PM, James Bottomley wrote:
>>> If you're seeing some error or sense code that's causing a common retry,
>>> then perhaps we should add it to the codes we check for instead of
>>> trying to hide it?
>> We get questions about pretty much all of the error codes that do not
>> have a extra string and can get failed here.
>>
>> The problem is with the use of the word unhandled. Users think for
>> retryable errors we did not retry, or they think it means even if they
>> have multipath/raid that the error is going to the application or FS so
>> they get worried and make extra support requests.
>>
>> Was the patch that just changed the strings to ""Extended sense
>> description not available" and "Extended error description not available":
>> http://www.spinics.net/lists/linux-scsi/msg54874.html
>> ok?
>>
>> I think in the end Rob is just trying to get out of having to figure out
>> strings for all the error codes :)

What's the worst that could happen?

Thanks, I'll give it a try.