does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Krzysztof Błaszkowski]

Hello Eric,

I think it is not necessary and current behavior of kernel like 2.6.31 
confuses QA people. 
They need explanation that message like below:

sd 5:0:1:0: [sdb] Sense Key : Recovered Error [current] [descriptor]
Descriptor sense data with sense descriptors (in hex):
        72 01 00 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
        00 00 00 00 00 50
sd 5:0:1:0: [sdb] Add. Sense: ATA pass through information available

stands for that sense data contains ata registers file and the command was 
executed properly.

Here is additional condition i used in scsi_io_completion() which suppresses 
this message. Is it okay ?

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f3c4089..7e8555a 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -774,8 +774,18 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
 	 * is what gets returned to the user
 	 */
 	if (sense_valid && sshdr.sense_key == RECOVERED_ERROR) {
-		if (!(req->cmd_flags & REQ_QUIET))
-			scsi_print_sense("", cmd);
+		if (!(req->cmd_flags & REQ_QUIET) && 
+		    !((req->cmd[0] == ATA_16 || req->cmd[0] == ATA_12) 
+			&& (req->cmd[2] & (1 << 5))) ) {
+				/* skip reporting sense data for satl
+					ATA_12/16 if CK_COND bit is set
+					because:
+					- sense is delivered to app,
+					- by default cmd ends up with
+					    0x50 status byte
+				*/
+				scsi_print_sense("", cmd);
+			}
 		result = 0;
 		/* BLOCK_PC may have set error */
 		error = 0;



This is something offtopic but i don't know yet where to look for answer to 
following issue with native 2.6.31.5.  I noticed that very often there is 
used oom killer on a box with xeon (dual core), 2G ram during e.g. make -j2 
modules so make can be killed but also e.g. ssh, hald (saw in dmesg). Anyway 
make modules  (! without j2 !)somewhat works also this issue doesn't happen 
with kernel like 2.6.27.19 (Suse patches) - no matter make forks any time it 
wants.

Any ideas ?


Regards,
Krzysztof Blaszkowski




--
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: does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Douglas Gilbert]

Krzysztof Błaszkowski wrote:
> Hello Eric,
> 
> I think it is not necessary and current behavior of kernel like 2.6.31 
> confuses QA people. 
> They need explanation that message like below:
> 
> sd 5:0:1:0: [sdb] Sense Key : Recovered Error [current] [descriptor]
> Descriptor sense data with sense descriptors (in hex):
>         72 01 00 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
>         00 00 00 00 00 50
> sd 5:0:1:0: [sdb] Add. Sense: ATA pass through information available
> 
> stands for that sense data contains ata registers file and the command was 
> executed properly.
> 
> Here is additional condition i used in scsi_io_completion() which suppresses 
> this message. Is it okay ?

Close but I would like to see a filter on the additional
sense code ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d].
There is nothing in SAT that precludes the return of a RECOVERED
ERROR sense code for a read on a SATA disk that was recovered
after some extra work (for example).
With that filter in place there is no need to filter on the command.

IOWs just filter on sense code RECOVERED ERROR and additional
sense code ATA PASS-THROUGH INFORMATION AVAILABLE. SAT and SAT-2
compliant implementations should then dump less to the log.

Doug Gilbert
--
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: does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Krzysztof Błaszkowski]

On Tuesday 22 December 2009 20:44, Douglas Gilbert wrote:
> Krzysztof Błaszkowski wrote:
> > Hello Eric,
> >
> > I think it is not necessary and current behavior of kernel like 2.6.31
> > confuses QA people.
> > They need explanation that message like below:
> >
> > sd 5:0:1:0: [sdb] Sense Key : Recovered Error [current] [descriptor]
> > Descriptor sense data with sense descriptors (in hex):
> >         72 01 00 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
> >         00 00 00 00 00 50
> > sd 5:0:1:0: [sdb] Add. Sense: ATA pass through information available
> >
> > stands for that sense data contains ata registers file and the command
> > was executed properly.
> >
> > Here is additional condition i used in scsi_io_completion() which
> > suppresses this message. Is it okay ?
>
> Close but I would like to see a filter on the additional
> sense code ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d].
> There is nothing in SAT that precludes the return of a RECOVERED
> ERROR sense code for a read on a SATA disk that was recovered
> after some extra work (for example).

of course but is it good idea to log sense data if:
- it is created on demand by application. (sat-2 ch 12.2.2 and .3)
- it doesn't contain some failure status

?

> With that filter in place there is no need to filter on the command.

right but OTOH if filter used asc/ascq then it would filter out all commands 
even these with some failures. 

i am not sure if this is really what you want to have, ie block all dumps of 
ATA PASS-THROUGH sense data.

>
> IOWs just filter on sense code RECOVERED ERROR and additional
> sense code ATA PASS-THROUGH INFORMATION AVAILABLE. SAT and SAT-2
> compliant implementations should then dump less to the log.

i would say that asc/ascq filter will stop logging all SAT/SAT-2 check 
conditions instead of "less".

that improvement i sent has better "granularity".

>
> Doug Gilbert

Krzysztof Blaszkowski
--
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: does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Douglas Gilbert]

Krzysztof Błaszkowski wrote:
> On Tuesday 22 December 2009 20:44, Douglas Gilbert wrote:
>> Krzysztof Błaszkowski wrote:
>>> Hello Eric,
>>>
>>> I think it is not necessary and current behavior of kernel like 2.6.31
>>> confuses QA people.
>>> They need explanation that message like below:
>>>
>>> sd 5:0:1:0: [sdb] Sense Key : Recovered Error [current] [descriptor]
>>> Descriptor sense data with sense descriptors (in hex):
>>>         72 01 00 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
>>>         00 00 00 00 00 50
>>> sd 5:0:1:0: [sdb] Add. Sense: ATA pass through information available
>>>
>>> stands for that sense data contains ata registers file and the command
>>> was executed properly.
>>>
>>> Here is additional condition i used in scsi_io_completion() which
>>> suppresses this message. Is it okay ?
>> Close but I would like to see a filter on the additional
>> sense code ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d].
>> There is nothing in SAT that precludes the return of a RECOVERED
>> ERROR sense code for a read on a SATA disk that was recovered
>> after some extra work (for example).
> 
> of course but is it good idea to log sense data if:
> - it is created on demand by application. (sat-2 ch 12.2.2 and .3)
> - it doesn't contain some failure status
> 
> ?
> 
>> With that filter in place there is no need to filter on the command.
> 
> right but OTOH if filter used asc/ascq then it would filter out all commands 
> even these with some failures. 
> 
> i am not sure if this is really what you want to have, ie block all dumps of 
> ATA PASS-THROUGH sense data.
> 
>> IOWs just filter on sense code RECOVERED ERROR and additional
>> sense code ATA PASS-THROUGH INFORMATION AVAILABLE. SAT and SAT-2
>> compliant implementations should then dump less to the log.
> 
> i would say that asc/ascq filter will stop logging all SAT/SAT-2 check 
> conditions instead of "less".
> 
> that improvement i sent has better "granularity".

I disagree.

This is the code sequence I had in mind:

         if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
                 /* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
                  * print since app wants ATA registers */
                 if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
                         ;
                 else if (!(req->cmd_flags & REQ_QUIET))
                         scsi_print_sense("", cmd);
                 result = 0;
                 /* BLOCK_PC may have set error */
                 error = 0;
         }

This will suppress logging the sense data associated with the
ATA PASS-THROUGH INFORMATION AVAILABLE additional sense code.
That is only generated in the situation when a SCSI ATA PASS-THROUGH
command sets the CK_COND bit _and_ there was no other error from
the ATA disk or the transport (e.g. SAS can issue an ABORTED COMMAND
sense key). This is shown in sat2r09.pdf section 12.2.5 table 113.

Doug Gilbert

P.S. Haven't seen Eric Youngdale's email address for a while
--
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: does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Krzysztof Błaszkowski]

On Wednesday 23 December 2009 06:45, Douglas Gilbert wrote:
> Krzysztof Błaszkowski wrote:
> > On Tuesday 22 December 2009 20:44, Douglas Gilbert wrote:
> >> Krzysztof Błaszkowski wrote:
> >>> Hello Eric,
> >>>
> >>> I think it is not necessary and current behavior of kernel like 2.6.31
> >>> confuses QA people.
> >>> They need explanation that message like below:
> >>>
> >>> sd 5:0:1:0: [sdb] Sense Key : Recovered Error [current] [descriptor]
> >>> Descriptor sense data with sense descriptors (in hex):
> >>>         72 01 00 1d 00 00 00 0e 09 0c 00 00 00 00 00 00
> >>>         00 00 00 00 00 50
> >>> sd 5:0:1:0: [sdb] Add. Sense: ATA pass through information available
> >>>
> >>> stands for that sense data contains ata registers file and the command
> >>> was executed properly.
> >>>
> >>> Here is additional condition i used in scsi_io_completion() which
> >>> suppresses this message. Is it okay ?
> >>
> >> Close but I would like to see a filter on the additional
> >> sense code ATA PASS-THROUGH INFORMATION AVAILABLE [0x0, 0x1d].
> >> There is nothing in SAT that precludes the return of a RECOVERED
> >> ERROR sense code for a read on a SATA disk that was recovered
> >> after some extra work (for example).
> >
> > of course but is it good idea to log sense data if:
> > - it is created on demand by application. (sat-2 ch 12.2.2 and .3)
> > - it doesn't contain some failure status
> >
> > ?
> >
> >> With that filter in place there is no need to filter on the command.
> >
> > right but OTOH if filter used asc/ascq then it would filter out all
> > commands even these with some failures.
> >
> > i am not sure if this is really what you want to have, ie block all dumps
> > of ATA PASS-THROUGH sense data.
> >
> >> IOWs just filter on sense code RECOVERED ERROR and additional
> >> sense code ATA PASS-THROUGH INFORMATION AVAILABLE. SAT and SAT-2
> >> compliant implementations should then dump less to the log.
> >
> > i would say that asc/ascq filter will stop logging all SAT/SAT-2 check
> > conditions instead of "less".
> >
> > that improvement i sent has better "granularity".
>
> I disagree.
>
> This is the code sequence I had in mind:
>
>          if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) {
>                  /* if ATA PASS-THROUGH INFORMATION AVAILABLE skip
>                   * print since app wants ATA registers */
>                  if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d))
>                          ;
>                  else if (!(req->cmd_flags & REQ_QUIET))
>                          scsi_print_sense("", cmd);
>                  result = 0;
>                  /* BLOCK_PC may have set error */
>                  error = 0;
>          }
>
> This will suppress logging the sense data associated with the
> ATA PASS-THROUGH INFORMATION AVAILABLE additional sense code.
> That is only generated in the situation when a SCSI ATA PASS-THROUGH
> command sets the CK_COND bit _and_ there was no other error from
> the ATA disk or the transport (e.g. SAS can issue an ABORTED COMMAND
> sense key). This is shown in sat2r09.pdf section 12.2.5 table 113.

I think you are right that checking CK_COND bit here is not necessary because 
if command was failed due to drive error then sense key wouldn't be 
RECOVERED_ERROR according to section 11 (sat2).

Krzysztof Blaszkowski
>
> Doug Gilbert
>
> P.S. Haven't seen Eric Youngdale's email address for a while


P.S. I took his address from scsi_lib header (2.6.31.5) and i don't think i 
should bother if this address is valid or invalid or if he wishes to reply or 
he doesn't. This is why i posted my conclusions to linux-scsi too.
--
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: does scsi_io_completion need to dump sense data for ata pass through (ck_cond = 1) ?

[Eric Youngdale]

> P.S. Haven't seen Eric Youngdale's email address for a while

Still around, but busy with Real Work(tm).  I don't have much to add 
here as I haven't looked at that source code in years now...