Skip to contentSkip to conversation

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

PATCH Krzysztof Błaszkowski 3 participants 6 messages Dec 22, 2009 – Dec 27, 2009 SCSI

lore
Share & tools
lore
#0 lore raw

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 ?

drivers/scsi/scsi_lib.c +0 -0
index f3c4089..7e8555a 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#1 lore raw
Quoted 6 lines · 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):

Quoted 7 lines
        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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 16 lines · Douglas Gilbert
Quoted 15 lines · 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.

Quoted 8 lines
Quoted 2 lines

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

?

Quoted 1 line

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.

Quoted 4 lines

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".

Quoted 2 lines

Doug Gilbert

Krzysztof Blaszkowski
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 37 lines · Krzysztof Błaszkowski
Quoted 16 lines · Douglas Gilbert
Quoted 15 lines · 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.

Quoted 7 lines
Quoted 2 lines

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

?

Quoted 1 line

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.

Quoted 7 lines
Quoted 3 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 66 lines · Douglas Gilbert
Quoted 40 lines · Krzysztof Błaszkowski
Quoted 16 lines · Douglas Gilbert
Quoted 15 lines · 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.

Quoted 8 lines
Quoted 2 lines

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

?

Quoted 1 line

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.

Quoted 8 lines
Quoted 3 lines

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;
         }
Quoted 6 lines

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

Quoted 4 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
In reply to Douglas Gilbert
Quoted 2 lines

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...