[RFC] Fix device not ready printk

[RFC] Fix device not ready printk

[Matthew Wilcox]

scsi_print_sense_hdr prints KERN_INFO, making the output from
scsi_io_completion ugly:

sd 0:0:0:0: [sdb] Device not ready: <6>: Sense Key : 0x2 [current] 
: ASC=0x4 ASCQ=0x3

By duplicating some of the code from it, we can get the nicer output:

sd 0:0:0:0: [sdb] Device not ready: Sense Key : Not Ready [current] 
sd 0:0:0:0: [sdb] Device not ready: Add. Sense: Logical unit not ready, manual intervention required

(the observant will notice that I turned on SCSI_CONSTANTS between the
two kernels)

I'm in two minds about printing the 'Device not ready' twice.  On the
one hand, it's redundant information.  On the other hand, it helps the
harried sysadmin with multiple simultaneous disc failures tie the two
messages together.

So I'm not signing off on this because I think it warrants further discussion.

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1f5a07b..d0b8608 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -940,7 +940,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
 					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
+				scsi_show_sense_hdr(&sshdr);
+				scmd_printk(KERN_INFO, cmd,
+					    "Device not ready: ");
+				scsi_show_extd_sense(sshdr.asc, sshdr.ascq);
 			}
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;

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

Re: [RFC] Fix device not ready printk

[Matthew Wilcox]

On Fri, Jun 29, 2007 at 07:21:26AM -0600, Matthew Wilcox wrote:
> I'm in two minds about printing the 'Device not ready' twice.  On the
> one hand, it's redundant information.  On the other hand, it helps the
> harried sysadmin with multiple simultaneous disc failures tie the two
> messages together.
> 
> So I'm not signing off on this because I think it warrants further discussion.

After three weeks with no discussion, I guess it may as well be applied.

----

scsi: Better error messages when device not ready

Because scsi_print_sense_hdr prefixes with KERN_INFO, the output from
scsi_io_completion looks like:

sd 0:0:0:0: [sdb] Device not ready: <6>: Sense Key : 0x2 [current] 
: ASC=0x4 ASCQ=0x3

By using scsi_show_sense_hdr, we can get the much more appealing output:

sd 0:0:0:0: [sdb] Device not ready: Sense Key : 0x2 [current] 
sd 0:0:0:0: [sdb] Device not ready: ASC=0x4 ASCQ=0x3

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 1f5a07b..d0b8608 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -940,7 +940,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			if (!(req->cmd_flags & REQ_QUIET)) {
 				scmd_printk(KERN_INFO, cmd,
 					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
+				scsi_show_sense_hdr(&sshdr);
+				scmd_printk(KERN_INFO, cmd,
+					    "Device not ready: ");
+				scsi_show_extd_sense(sshdr.asc, sshdr.ascq);
 			}
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;


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

Re: [RFC] Fix device not ready printk

[Martin K. Petersen]

>>>>> "Matthew" == Matthew Wilcox <matthew@wil.cx> writes:

Matthew> After three weeks with no discussion, I guess it may as well
Matthew> be applied.

Matthew> scsi: Better error messages when device not ready

Looks good to me.

Acked-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [RFC] Fix device not ready printk

[James Bottomley]

On Thu, 2007-07-19 at 12:51 -0600, Matthew Wilcox wrote:
> On Fri, Jun 29, 2007 at 07:21:26AM -0600, Matthew Wilcox wrote:
> > I'm in two minds about printing the 'Device not ready' twice.  On the
> > one hand, it's redundant information.  On the other hand, it helps the
> > harried sysadmin with multiple simultaneous disc failures tie the two
> > messages together.
> > 
> > So I'm not signing off on this because I think it warrants further discussion.
> 
> After three weeks with no discussion, I guess it may as well be applied.

Sorry ... busy with other things.

The main thing I don't like is that we've spent a lot of time moving
sense prints into a library so that if someone gullible^Wsensible enough
to want to do a reporting infrastructure comes along, we have all the
entry points nicely laid out, this is a retrograde step on that because
it reintroduces the open coding.

How about something like the attached.  It does exactly what you want
but retains the tap into the constants file for logging?

James

Index: BUILD-2.6/drivers/scsi/constants.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/constants.c	2007-09-14 11:21:50.000000000 -0500
+++ BUILD-2.6/drivers/scsi/constants.c	2007-09-23 09:04:59.000000000 -0500
@@ -1235,6 +1235,20 @@ scsi_print_sense_hdr(const char *name, s
 }
 EXPORT_SYMBOL(scsi_print_sense_hdr);
 
+/*
+ * Print normalized SCSI sense header with device information and a prefix.
+ */
+void
+scsi_cmd_print_sense_hdr(struct scsi_cmnd *scmd, const char *name,
+			  struct scsi_sense_hdr *sshdr)
+{
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_sense_hdr(sshdr);
+	scmd_printk(KERN_INFO, scmd, "%s: ", name);
+	scsi_show_extd_sense(sshdr->asc, sshdr->ascq);
+}
+EXPORT_SYMBOL(scsi_cmd_print_sense_hdr);
+
 static void
 scsi_decode_sense_buffer(const unsigned char *sense_buffer, int sense_len,
 		       struct scsi_sense_hdr *sshdr)
Index: BUILD-2.6/drivers/scsi/scsi_lib.c
===================================================================
--- BUILD-2.6.orig/drivers/scsi/scsi_lib.c	2007-09-22 11:46:15.000000000 -0500
+++ BUILD-2.6/drivers/scsi/scsi_lib.c	2007-09-23 09:05:47.000000000 -0500
@@ -944,11 +944,11 @@ void scsi_io_completion(struct scsi_cmnd
 					break;
 				}
 			}
-			if (!(req->cmd_flags & REQ_QUIET)) {
-				scmd_printk(KERN_INFO, cmd,
-					    "Device not ready: ");
-				scsi_print_sense_hdr("", &sshdr);
-			}
+			if (!(req->cmd_flags & REQ_QUIET))
+				scsi_cmd_print_sense_hdr(cmd,
+							 "Device not ready",
+							 &sshdr);
+
 			scsi_end_request(cmd, 0, this_count, 1);
 			return;
 		case VOLUME_OVERFLOW:
Index: BUILD-2.6/include/scsi/scsi_dbg.h
===================================================================
--- BUILD-2.6.orig/include/scsi/scsi_dbg.h	2007-09-14 09:06:13.000000000 -0500
+++ BUILD-2.6/include/scsi/scsi_dbg.h	2007-09-23 09:06:50.000000000 -0500
@@ -9,6 +9,8 @@ extern void __scsi_print_command(unsigne
 extern void scsi_show_extd_sense(unsigned char, unsigned char);
 extern void scsi_show_sense_hdr(struct scsi_sense_hdr *);
 extern void scsi_print_sense_hdr(const char *, struct scsi_sense_hdr *);
+extern void scsi_cmd_print_sense_hdr(struct scsi_cmnd *, const char *,
+				     struct scsi_sense_hdr *);
 extern void scsi_print_sense(char *, struct scsi_cmnd *);
 extern void __scsi_print_sense(const char *name,
 			       const unsigned char *sense_buffer,

Re: [RFC] Fix device not ready printk

[Matthew Wilcox]

On Sun, Sep 23, 2007 at 09:08:46AM -0500, James Bottomley wrote:
> The main thing I don't like is that we've spent a lot of time moving
> sense prints into a library so that if someone gullible^Wsensible enough
> to want to do a reporting infrastructure comes along, we have all the
> entry points nicely laid out, this is a retrograde step on that because
> it reintroduces the open coding.

Your patch is certainly an improvement from that point of view; it
consolidates two calls (which would have to be tied together by said
infrastructure) into one.

> How about something like the attached.  It does exactly what you want
> but retains the tap into the constants file for logging?

> +/*
> + * Print normalized SCSI sense header with device information and a prefix.
> + */
> +void
> +scsi_cmd_print_sense_hdr(struct scsi_cmnd *scmd, const char *name,
> +			  struct scsi_sense_hdr *sshdr)
> +{
> +	scmd_printk(KERN_INFO, scmd, "%s: ", name);
> +	scsi_show_sense_hdr(sshdr);
> +	scmd_printk(KERN_INFO, scmd, "%s: ", name);
> +	scsi_show_extd_sense(sshdr->asc, sshdr->ascq);
> +}

I think 'name' is wrong.  It's not a name, it's a description.  How
about s/name/desc/g ?

With that change, Acked-by: Matthew Wilcox <willy@linux.intel.com>

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."