On 2018-02-26 02:25 AM, Hannes Reinecke wrote:
On 02/25/2018 07:29 PM, Douglas Gilbert wrote:The SCSI PRE-FETCH (10 or 16) command is present both on hard disks and some SSDs. It is useful when the address of the next block(s) to be read is known but it is not following the LBA of the current READ (so read-ahead won't help). It returns two "good" SCSI Status values. If the requested blocks have fitted (or will most likely fit (when the IMMED bit is set)) into the disk's cache, it returns CONDITION MET. If it didn't (or will not) fit then it returns GOOD status. The primary goal of this patch is to stop the SCSI subsystem treating the CONDITION MET SCSI status as an error. The current state makes the PRE-FETCH command effectively unusable via pass-throughs. The hunt to find where the erroneous decision was made led to the scsi_io_completion() function in scsi_lib.c . This is a complicated function made more complicated by the intertwining of good and error (or special case) processing paths. Future work: if the sd driver was to use the PRE-FETCH command, decide whether it and/or the block layer needs to know about the two different "good" statuses. If so a mechanism would be needed to do that. ChangeLog to v2: - further restructure the code, place some early non-zero result processing in a new helper function: scsi_io_completion_nz_result() - this reduces the number of error checks on the zero result path (the fast path) at the expense of some extra work for the non-zero result processing - rename some variables to make the code a little clearer ChangeLog to v1: - expand scsi_status_is_good() to check for CONDITION MET - add another corner case in scsi_io_completion() adjacent to the one for the RECOVERED ERROR sense key case. That is another "non-error" - structure code so extra checks are only on the error path (i.e. when cmd->result is non-zero) This patch is against mkp's 4.17/scsi-queue branch. It also applies to lk 4.15.x where it was re-tested on a SAS SSD. Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx> --- drivers/scsi/scsi_lib.c | 140 +++++++++++++++++++++++++++++------------------- include/scsi/scsi.h | 2 + 2 files changed, 87 insertions(+), 55 deletions(-) diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index aea5a1ae318b..e1e974f08d52 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -738,6 +738,73 @@ static blk_status_t __scsi_error_from_host_byte(struct scsi_cmnd *cmd, } }+/* Helper for scsi_io_completion() when cmd->result is non-zero. */+static int scsi_io_completion_nz_result(struct scsi_cmnd *cmd, + blk_status_t *blk_statp) +{ + bool sense_valid; + bool about_current; + int result = cmd->result; + struct request *req = cmd->request; + struct scsi_sense_hdr sshdr; + + sense_valid = scsi_command_normalize_sense(cmd, &sshdr); + about_current = sense_valid ? !scsi_sense_is_deferred(&sshdr) : true; + + if (blk_rq_is_passthrough(req)) { + if (sense_valid) { + /* + * SG_IO wants current and deferred errors + */ + scsi_req(req)->sense_len = + min(8 + cmd->sense_buffer[7], + SCSI_SENSE_BUFFERSIZE); + } + if (about_current) + *blk_statp = __scsi_error_from_host_byte(cmd, result); + } else if (blk_rq_bytes(req) == 0 && about_current) { + /* + * Flush commands do not transfers any data, and thus cannot use + * good_bytes != blk_rq_bytes(req) as the signal for an error. + * This sets the error explicitly for the problem case. + */ + *blk_statp = __scsi_error_from_host_byte(cmd, result); + } + /* + * Recovered errors need reporting, but they're always treated as + * success, so fiddle the result code here. For passthrough requests + * we already took a copy of the original into sreq->result which + * is what gets returned to the user + */ + if (sense_valid && (sshdr.sense_key == RECOVERED_ERROR)) { + /* + * if ATA PASS-THROUGH INFORMATION AVAILABLE skip + * print since caller wants ATA registers. Only occurs + * on SCSI ATA PASS_THROUGH commands when CK_COND=1 + */ + if ((sshdr.asc == 0x0) && (sshdr.ascq == 0x1d)) + ; + else if (!(req->rq_flags & RQF_QUIET)) + scsi_print_sense(cmd); + result = 0; + *blk_statp = BLK_STS_OK; + /* for passthrough, blk_stat may be set */ + } + /* + * Another corner case: the SCSI status byte is non-zero but 'good'. + * Example: PRE-FETCH command returns SAM_STAT_CONDITION_MET when + * it is able to fit nominated LBs in its cache (and SAM_STAT_GOOD + * if it can't fit). Treat SAM_STAT_CONDITION_MET and the related + * intermediate statuses (both obsolete in SAM-4) as good. + */ + if (status_byte(result) && scsi_status_is_good(result)) { + result = 0; + /* for passthrough error may be set */ + *blk_statp = BLK_STS_OK; + } + return result; +} + /* * Function: scsi_io_completion() *Hmm. Can't we return blk_stat from this function, and adjusting the 'result' value after it with an if-clause like if (blk_stat == BLK_STS_OK) result = 0; That would cleanup up the function and avoid having (essentially) two return values.
Good idea. I still like my v3 which continues the re-factoring of scsi_io_completion(). So I plan to re-issue the patch under the name: scsi_io_completion() cleanup and fix CONDITION MET handling
The only problem here is that __scsi_error_from_hostbyte() will return BLK_STS_IOERR if result == 0; doubt that is intended. And I guess it'll affect this issue, too. Mind sending a separate patch for that?
Looks like you have already done that. Doug Gilbert
