[PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Maurizio Lombardi]

During IO with fabric faults, one generally sees several "Unhandled error
code" messages in the syslog as shown below:

sd 4:0:6:2: [sdbw] Unhandled error code
sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
end_request: I/O error, dev sdbw, sector 0

This comes from scsi_io_completion (in scsi_lib.c) while handling error
codes other than DID_RESET or not deferred sense keys i.e. this is
actually handled by the SCSI mid layer. But what gets displayed here is
"Unhandled error code" which is quite misleading as it indicates
something that is not addressed by the mid layer.

This patch removes "Unhandled error code" and replaces "Unhandled sense code"
with "Failing command with sense code:".


Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
---
 drivers/scsi/scsi_lib.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 9db097a..b3c25cd 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			action = ACTION_FAIL;
 			break;
 		default:
-			description = "Unhandled sense code";
+			description = "Failing command with sense code:";
 			action = ACTION_FAIL;
 			break;
 		}
-	} else {
-		description = "Unhandled error code";
+	} else
 		action = ACTION_FAIL;
-	}
 
 	if (action != ACTION_FAIL &&
 	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
-- 
Maurizio Lombardi

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Christoph Hellwig]

On Fri, Jun 06, 2014 at 10:10:35AM +0200, Maurizio Lombardi wrote:
> During IO with fabric faults, one generally sees several "Unhandled error
> code" messages in the syslog as shown below:
> 
> sd 4:0:6:2: [sdbw] Unhandled error code
> sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
> end_request: I/O error, dev sdbw, sector 0
> 
> This comes from scsi_io_completion (in scsi_lib.c) while handling error
> codes other than DID_RESET or not deferred sense keys i.e. this is
> actually handled by the SCSI mid layer. But what gets displayed here is
> "Unhandled error code" which is quite misleading as it indicates
> something that is not addressed by the mid layer.
> 
> This patch removes "Unhandled error code" and replaces "Unhandled sense code"
> with "Failing command with sense code:".
> 
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Christoph Hellwig]

Can I get another review for this one?

On Fri, Jun 06, 2014 at 10:10:35AM +0200, Maurizio Lombardi wrote:
> During IO with fabric faults, one generally sees several "Unhandled error
> code" messages in the syslog as shown below:
> 
> sd 4:0:6:2: [sdbw] Unhandled error code
> sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
> end_request: I/O error, dev sdbw, sector 0
> 
> This comes from scsi_io_completion (in scsi_lib.c) while handling error
> codes other than DID_RESET or not deferred sense keys i.e. this is
> actually handled by the SCSI mid layer. But what gets displayed here is
> "Unhandled error code" which is quite misleading as it indicates
> something that is not addressed by the mid layer.
> 
> This patch removes "Unhandled error code" and replaces "Unhandled sense code"
> with "Failing command with sense code:".
> 
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/scsi/scsi_lib.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9db097a..b3c25cd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			action = ACTION_FAIL;
>  			break;
>  		default:
> -			description = "Unhandled sense code";
> +			description = "Failing command with sense code:";
>  			action = ACTION_FAIL;
>  			break;
>  		}
> -	} else {
> -		description = "Unhandled error code";
> +	} else
>  		action = ACTION_FAIL;
> -	}
>  
>  	if (action != ACTION_FAIL &&
>  	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
> -- 
> Maurizio Lombardi
> 
> --
> 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
---end quoted text---

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Mike Christie]

On 06/06/2014 03:10 AM, Maurizio Lombardi wrote:
> During IO with fabric faults, one generally sees several "Unhandled error
> code" messages in the syslog as shown below:
> 
> sd 4:0:6:2: [sdbw] Unhandled error code
> sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
> sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
> end_request: I/O error, dev sdbw, sector 0
> 
> This comes from scsi_io_completion (in scsi_lib.c) while handling error
> codes other than DID_RESET or not deferred sense keys i.e. this is
> actually handled by the SCSI mid layer. But what gets displayed here is
> "Unhandled error code" which is quite misleading as it indicates
> something that is not addressed by the mid layer.
> 
> This patch removes "Unhandled error code" and replaces "Unhandled sense code"
> with "Failing command with sense code:".
> 
> 
> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
> ---
>  drivers/scsi/scsi_lib.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 9db097a..b3c25cd 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			action = ACTION_FAIL;
>  			break;
>  		default:
> -			description = "Unhandled sense code";
> +			description = "Failing command with sense code:";
>  			action = ACTION_FAIL;
>  			break;
>  		}
> -	} else {
> -		description = "Unhandled error code";
> +	} else
>  		action = ACTION_FAIL;
> -	}
>  
>  	if (action != ACTION_FAIL &&
>  	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
> 

Looks ok to me.

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

RE: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Wednesday, 25 June, 2014 6:07 AM
> To: Maurizio Lombardi
> Cc: James.Bottomley@HansenPartnership.com; linux-scsi@vger.kernel.org;
> hch@infradead.org
> Subject: Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code"
> messages.
> 
> Can I get another review for this one?
> 
> On Fri, Jun 06, 2014 at 10:10:35AM +0200, Maurizio Lombardi wrote:
...
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
...
> > @@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
> unsigned int good_bytes)
> >  			action = ACTION_FAIL;
> >  			break;
> >  		default:
> > -			description = "Unhandled sense code";
> > +			description = "Failing command with sense code:";

The default case handles sense keys other than:
	#define NOT_READY           0x02
	#define ILLEGAL_REQUEST     0x05
	#define UNIT_ATTENTION      0x06
	#define ABORTED_COMMAND     0x0b
	#define VOLUME_OVERFLOW     0x0d

which means these #defines (and any other values)
#define NO_SENSE            0x00
#define RECOVERED_ERROR     0x01
#define MEDIUM_ERROR        0x03
#define HARDWARE_ERROR      0x04
#define DATA_PROTECT        0x07
#define BLANK_CHECK         0x08
#define COPY_ABORTED        0x0a
#define MISCOMPARE          0x0e

The other description strings that result in ACTION_FAIL are based
on the sense key and sometimes the additional sense code:
description = "Media Changed";
description = "Host Data Integrity Failure";
description = "Discard failure";
description = "Write same failure";
description = "Invalid command failure";
description = "Target Data Integrity Failure";
description = "Device not ready";

Also, there is this one, which is not based on the sense key:
description = "Command timed out";

Since the ACTION_FAIL case always prints the sense key
and additional sense code:
	if (!(req->cmd_flags & REQ_QUIET)) {
		if (description)
			scmd_printk(KERN_INFO, cmd, " % \n",
				description);
		scsi_print_result(cmd);
		if (driver_byte(result) & DRIVER_SENSE)
			scsi_print_sense("", cmd);
		scsi_print_command(cmd);
	}

perhaps the description string should be removed altogether?

In this example:
	sd 1:0:0:0: [sdb] Unhandled sense code
	sd 1:0:0:0: [sdb]
	Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE
	sd 1:0:0:0: [sdb]
	Sense Key : Medium Error [current]
	sd 1:0:0:0: [sdb]
	Add. Sense: Unrecovered read error
	sd 1:0:0:0: [sdb] CDB:
	Read(10): 28 00 00 18 f8 a8 00 00 08 00
	end_request: critical medium error, dev sdb, sector 1636520

"Sense Key : Medium Error" is much more informative than
"Unhandled sense code" or "Failing command with sense code".
All the other descriptions represent failing commands,
so using different wording is a bit confusing.

For the "Unhandled error code" (for which you are proposing
removing the string) and the timeout case, the scsi_print_result 
call already prints hostbyte and driverbyte, which explain what 
happened in more detail:

	sd 1:0:0:0: [sda] Unhandled error code                                          
	sd 1:0:0:0: [sda]                                                               
	Result: hostbyte=DID_TIME_OUT driverbyte=DRIVER_OK                              
	sd 1:0:0:0: [sda] CDB:                                                          
	Test Unit Ready:                                                                
	end_request: I/O error, dev sda, sector 481013632                               

---
Rob Elliott    HP Server Storage

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Christoph Hellwig]

On Wed, Jun 25, 2014 at 08:38:47PM +0000, Elliott, Robert (Server Storage) wrote:
> Since the ACTION_FAIL case always prints the sense key
> and additional sense code:

> perhaps the description string should be removed altogether?

> For the "Unhandled error code" (for which you are proposing
> removing the string) and the timeout case, the scsi_print_result 
> call already prints hostbyte and driverbyte, which explain what 
> happened in more detail:

I'm tempted to agree and just remove the description.  Do you want to
send a patch for this?

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Maurizio Lombardi]

On 07/03/2014 10:41 AM, Christoph Hellwig wrote:
> On Wed, Jun 25, 2014 at 08:38:47PM +0000, Elliott, Robert (Server Storage) wrote:
>> Since the ACTION_FAIL case always prints the sense key
>> and additional sense code:
> 
>> perhaps the description string should be removed altogether?
> 
> 
> I'm tempted to agree and just remove the description.  Do you want to
> send a patch for this?

So I'll get rid of the "description" string completely...
I'm going to send a new patch later today.

Regards,
Maurizio Lombardi

Re: [PATCH V2] scsi_lib: removes ambiguous "Unhandled error code" messages.

[Hannes Reinecke]

On 06/25/2014 01:06 PM, Christoph Hellwig wrote:
> Can I get another review for this one?
>
> On Fri, Jun 06, 2014 at 10:10:35AM +0200, Maurizio Lombardi wrote:
>> During IO with fabric faults, one generally sees several "Unhandled error
>> code" messages in the syslog as shown below:
>>
>> sd 4:0:6:2: [sdbw] Unhandled error code
>> sd 4:0:6:2: [sdbw] Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK
>> sd 4:0:6:2: [sdbw] CDB: Read(10): 28 00 00 00 00 00 00 00 08 00
>> end_request: I/O error, dev sdbw, sector 0
>>
>> This comes from scsi_io_completion (in scsi_lib.c) while handling error
>> codes other than DID_RESET or not deferred sense keys i.e. this is
>> actually handled by the SCSI mid layer. But what gets displayed here is
>> "Unhandled error code" which is quite misleading as it indicates
>> something that is not addressed by the mid layer.
>>
>> This patch removes "Unhandled error code" and replaces "Unhandled sense code"
>> with "Failing command with sense code:".
>>
>>
>> Signed-off-by: Maurizio Lombardi <mlombard@redhat.com>
>> ---
>>   drivers/scsi/scsi_lib.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 9db097a..b3c25cd 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -955,14 +955,12 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>>   			action = ACTION_FAIL;
>>   			break;
>>   		default:
>> -			description = "Unhandled sense code";
>> +			description = "Failing command with sense code:";
>>   			action = ACTION_FAIL;
>>   			break;
>>   		}
>> -	} else {
>> -		description = "Unhandled error code";
>> +	} else
>>   		action = ACTION_FAIL;
>> -	}
>>
>>   	if (action != ACTION_FAIL &&
>>   	    time_before(cmd->jiffies_at_alloc + wait_for, jiffies)) {
>> --
>> Maurizio Lombardi
>>
Good to see it finally went in.
We're carrying the same patch in our tree for ages as the original 
attempt to push it upstream failed.

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

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (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