Re: [PATCH] drivers/scsi/st.c: add command and sense history
The following comments are based on looking at the patch, not testing it.
The tape devices do not provide much information about problems via the normal
API: mostly it just tells that something went wrong. The driver prints the
sense data to the kernel log but this information may not be accessible and/or
it is difficult to connect it to the specific problem at hand. Exporting the
sense data in some form is a better (although not perfect) method to give more
information in case of problems.
On Fri, 22 Jul 2005, Andy Stevens wrote:
> Hello SCSI tapers,
>
> Attached is a patch to add some functionality the st driver so that you can
> monitor the most recent SCSI command and sense buffer. This info is made
> available via sysfs attributes. I want to add these attributes in order to do
> better error processing in some custom tape software which I am writing. This
> patch has been tested on Linux kernel 2.6.12.
>
> The new functionality is as follows:
>
> 1) the most recent "relevant" SCSI command and sense are stored in hex format
> in:
>
> /sys/class/scsi_tape/stN/last_cmnd
> /sys/class/scsi_tape/stN/last_sense
>
Using two files is problematic: the user can't be sure that last_sense is
really related to last_cmnd. If they are in the same file, a single read could
enforce this. (The implementation does not contain the necessary locking. I
don't think it is necessary at this phase but the single file approach would
make atomicity possible if desired.)
> When I say "relevant" SCSI command, I mean: occasionally the most recent SCSI
> command is not the one of interest, e.g. when encountering an ILI, the driver
> will automatically backspace to the beginning of the block. However, the
> last_cmnd and last_sense will show the results of the read error, not the
> backspace.
>
This is the reason why this should be in the ULD. If only the most recent
data would be exported, it could be done at midlevel to benefit all SCSI
devices.
> 2) The raw sense data is also decoded into English language for primary sense
> key, extended sense, and ILI/FM/EOM flags in these sysfs files:
>
> /sys/class/scsi_tape/stN/primary_sense
> /sys/class/scsi_tape/stN/extended_sense
> /sys/class/scsi_tape/stN/sense_flags
>
I don't think these are necessary. Anyway, the sysfs data should be either
single values or an array of values. The contents of the files must be
explained elsewhere (linux/Documentation/scsi/st.txt).
However, I would like to discuss exporting selected fields of the sense data
instead
of the raw data as an alternative. This would make interpretation of the data
easier when devices supporting the descriptor based sense data start
appearing. The values exported could be: sense key, ASC, ASCQ, flags (FM, EOM,
ILI), information field. What do the users really need?
> Details of implementation:
>
> 1) added data fields "last_cmnd" and "last_sense" to struct st_cmdstatus for
> storage of most recent command and sense data. Removed unused "last_cmnd" and
> "last_sense" fields from struct scsi_tape.
>
> 2) added routine st_record_last_command() to save command and sense data
>
> 3) added calls to st_record_last_command() in appropriate locations in the
> code.
>
> 4) added arg to st_int_ioctl() to indicate whether or not to record the SCSI
> command (necessary so that we can record the "relevant" command)
>
> 5) added routines for sysfs attribute files
>
> This is my first time submitting a patch to the list, please go easy on me!!
>
Could you in future make patches from the linux source tree root using
'diff -up' (i.e., the path to st.c would be linux/drivers/scsi/st.c; the file
linux/Documentation/SubmittingPatches contains a lot of useful
information). Appending the diff instead of putting it into an attachment
would make commenting easier for some of us.
> --- st.c.orig 2005-06-17 15:48:29.0 -0400
> +++ st.c 2005-07-22 15:20:06.0 -0400
> @@ -17,7 +17,7 @@
> Last modified: 18-JAN-1998 Richard Gooch <[EMAIL PROTECTED]> Devfs support
> */
>
> -static char *verstr = "20050312";
> +static char *verstr = "20050706";
>
> #include
>
> @@ -215,7 +215,7 @@
> static int find_partition(struct scsi_tape *);
> static int switch_partition(struct scsi_tape *);
>
> -static int st_int_ioctl(struct scsi_tape *, unsigned int, unsigned long);
> +static int st_int_ioctl(struct scsi_tape *, unsigned int, unsigned long,
> int);
>
>
> #include "osst_detect.h"
> @@ -264,6 +264,29 @@
> return tape->disk->disk_name;
> }
>
> +static void st_clear_last_command(struct scsi_tape *STp)
> +{
> + /* clear last command and last sense */
> + memset(STp->buffer->cmdstat.last_cmnd,0,MAX_COMMAND_SIZE);
> + memset(STp->buffer->cmdstat.last_sense,0,SCSI_SENSE_BUFFERSIZE);
> +}
> +
> +static void st_record_last_command(struct scsi_tape *STp, struct
> scsi_request *SRpnt)
> +{
> + int i;
> +
> + st_clear_last_command(STp);
> +
Not necessary.
> + /* make
[PATCH] drivers/scsi/st.c: add command and sense history
Hello SCSI tapers,
Attached is a patch to add some functionality the st driver so that you
can monitor the most recent SCSI command and sense buffer. This info is
made available via sysfs attributes. I want to add these attributes in
order to do better error processing in some custom tape software which I
am writing. This patch has been tested on Linux kernel 2.6.12.
The new functionality is as follows:
1) the most recent "relevant" SCSI command and sense are stored in hex
format in:
/sys/class/scsi_tape/stN/last_cmnd
/sys/class/scsi_tape/stN/last_sense
When I say "relevant" SCSI command, I mean: occasionally the most recent
SCSI command is not the one of interest, e.g. when encountering an ILI,
the driver will automatically backspace to the beginning of the block.
However, the last_cmnd and last_sense will show the results of the read
error, not the backspace.
2) The raw sense data is also decoded into English language for primary
sense key, extended sense, and ILI/FM/EOM flags in these sysfs files:
/sys/class/scsi_tape/stN/primary_sense
/sys/class/scsi_tape/stN/extended_sense
/sys/class/scsi_tape/stN/sense_flags
Details of implementation:
1) added data fields "last_cmnd" and "last_sense" to struct st_cmdstatus
for storage of most recent command and sense data. Removed unused
"last_cmnd" and "last_sense" fields from struct scsi_tape.
2) added routine st_record_last_command() to save command and sense data
3) added calls to st_record_last_command() in appropriate locations in
the code.
4) added arg to st_int_ioctl() to indicate whether or not to record the
SCSI command (necessary so that we can record the "relevant" command)
5) added routines for sysfs attribute files
This is my first time submitting a patch to the list, please go easy on me!!
Regards,
--Andy Stevens
Electrical Science, Inc.
NY, USA
--- st.c.orig 2005-06-17 15:48:29.0 -0400
+++ st.c2005-07-22 15:20:06.0 -0400
@@ -17,7 +17,7 @@
Last modified: 18-JAN-1998 Richard Gooch <[EMAIL PROTECTED]> Devfs support
*/
-static char *verstr = "20050312";
+static char *verstr = "20050706";
#include
@@ -215,7 +215,7 @@
static int find_partition(struct scsi_tape *);
static int switch_partition(struct scsi_tape *);
-static int st_int_ioctl(struct scsi_tape *, unsigned int, unsigned long);
+static int st_int_ioctl(struct scsi_tape *, unsigned int, unsigned long, int);
#include "osst_detect.h"
@@ -264,6 +264,29 @@
return tape->disk->disk_name;
}
+static void st_clear_last_command(struct scsi_tape *STp)
+{
+ /* clear last command and last sense */
+ memset(STp->buffer->cmdstat.last_cmnd,0,MAX_COMMAND_SIZE);
+ memset(STp->buffer->cmdstat.last_sense,0,SCSI_SENSE_BUFFERSIZE);
+}
+
+static void st_record_last_command(struct scsi_tape *STp, struct scsi_request
*SRpnt)
+{
+ int i;
+
+ st_clear_last_command(STp);
+
+ /* make copy of last SCSI command */
+ for(i=0; ibuffer->cmdstat.last_cmnd[i] = SRpnt->sr_cmnd[i];
+ }
+
+ /* make copy of last SCSI sense buffer */
+ for(i=0; ibuffer->cmdstat.last_sense[i] = SRpnt->sr_sense_buffer[i];
+ }
+}
static void st_analyze_sense(struct scsi_request *SRpnt, struct st_cmdstatus
*s)
{
@@ -539,6 +562,7 @@
if (!SRpnt)
return (STp->buffer)->syscall_result;
+ st_record_last_command(STp,SRpnt);
scsi_release_request(SRpnt);
SRpnt = NULL;
@@ -587,6 +611,8 @@
if (!SRpnt)
return (STp->buffer)->syscall_result;
+ st_record_last_command(STp,SRpnt);
+
STps = &(STp->ps[STp->partition]);
if ((STp->buffer)->syscall_result != 0) {
struct st_cmdstatus *cmdstatp = &STp->buffer->cmdstat;
@@ -666,7 +692,7 @@
}
}
if (!result && backspace > 0)
- result = st_int_ioctl(STp, MTBSR, backspace);
+ result = st_int_ioctl(STp, MTBSR, backspace, 1);
} else if (STps->eof == ST_FM_HIT) {
if (STps->drv_file >= 0)
STps->drv_file++;
@@ -700,7 +726,7 @@
} else
arg |= STp->block_size;
if (set_it &&
- st_int_ioctl(STp, SET_DENS_AND_BLK, arg)) {
+ st_int_ioctl(STp, SET_DENS_AND_BLK, arg, 1)) {
printk(KERN_WARNING
"%s: Can't set default block size to %d bytes and
density %x.\n",
name, STm->default_blksize, STm->default_density);
@@ -786,6 +812,8 @@
break;
}
+ st_record_last_command(STp,SRpnt);
+
if (cmdstatp->have_sense) {
scode = cmdstatp->sense_hdr.sense_key;
@@ -919,6 +947,8 @@
goto err_out;
}
+ st_reco
