[dm-devel] RE: [RFC PATCH 4/4] convert scsi to blkerr error values
goggin, edward
egoggin at emc.com
Fri Sep 16 20:32:27 UTC 2005
Mike,
I don't think it is reasonably possible to anticipate
all possible parsing requirements for the asc and ascq
portions of SCSI sense information across all device
models. I'm in favor of having a "small" framework in
SCSI where a SCSI sense interpreter module (per
vendor & model possibly) could be registered
dynamically, by dm-emc.c for instance.
The extended error interpreter callout would be
triggered indirectly by a call from
__end_that_request_first to a extended error parser
associated with the io request's queue whenever it
sees a non-zero sense field of the io request.
Perhaps the sense and sense_len fields in the
request structure should be changed to not be
SCSI specific.
Also, in order to allow for more variation and detail
in the interpretation of device specific SCSI asc and
ascq values, the results of the interpretation should
not be required to be block layer generic, but instead
are saved in something like a void *bi_extended_error
field of the bio. __end_that_request_first would push
the results of the extended_error interpretation to the
bi_extended_error field of each bio in the request,
similar to how Jens's code currently works.
This extended error parsing approach should extend easily
(without requiring a kernel revision for new BLKERR values)
to new SCSI devices/models and their device specific asc
and ascq values. This design could also be extended to
support the interpretation of extended error information
for non-SCSI block devices like DASD.
Ed Goggin
> -----Original Message-----
> From: linux-scsi-owner at vger.kernel.org
> [mailto:linux-scsi-owner at vger.kernel.org] On Behalf Of Mike Christie
> Sent: Wednesday, August 24, 2005 5:05 AM
> To: axboe at suse.de; linux-scsi at vger.kernel.org; dm-devel at redhat.com
> Subject: [RFC PATCH 4/4] convert scsi to blkerr error values
>
> This patch convert scsi to use the BLKERR values.
> It uses the scsi_cmnd->result values to try
> and "guess" (LLDs are not all consistent in
> their host byte usage) if it is a transport or
> device or driver error. Maybe it would be better to
> have the transport classes define new host byte error
> values (or add a transport byte), and scsi-ml can then
> call into the transport class to tell it if the error
> is retryable or fatal.
>
> If this approach is ok, I guess the next step
> is more testing and figure out how to best
> support vendor specifics. While converting
> dm-emc's asc/ascq code there are comments
> indicating we might need to send additional
> commands to try and figure out what to do
> (Enginio or Sun people did you need this too?)
>
> The patch has been lightly tested with iscsi_sfnet
> and open-iscsi using dm-multipath to verify that
> when we send retryable errors dm-multipath does
> nice things like not automatically fail a path
> and fail the IO.
>
> Signed-off-by: Mike Christie <michaelc at cs.wisc.edu>
>
> diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -1269,6 +1269,7 @@ int scsi_device_cancel(struct scsi_devic
> scsi_eh_scmd_add(scmd,
> SCSI_EH_CANCEL_CMD);
> } else {
> scmd->result = (DID_ABORT << 16);
> + scmd->blk_result = BLKERR_FATAL_DEV;
> scsi_finish_command(scmd);
> }
> }
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1213,6 +1213,7 @@ int scsi_decide_disposition(struct scsi_
> SCSI_LOG_ERROR_RECOVERY(5, printk("%s: device
> offline - report"
> " as SUCCESS\n",
> __FUNCTION__));
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return SUCCESS;
> }
>
> @@ -1228,6 +1229,7 @@ int scsi_decide_disposition(struct scsi_
> * did_ok.
> */
> scmd->result &= 0xff00ffff;
> + scmd->blk_result = BLK_SUCCESS;
> return SUCCESS;
> case DID_OK:
> /*
> @@ -1236,7 +1238,10 @@ int scsi_decide_disposition(struct scsi_
> break;
> case DID_NO_CONNECT:
> case DID_BAD_TARGET:
> + scmd->blk_result = BLKERR_FATAL_XPT;
> + return SUCCESS;
> case DID_ABORT:
> + scmd->blk_result = BLKERR_RETRY_DEV;
> /*
> * note - this means that we just report the status back
> * to the top level driver, not that we actually think
> @@ -1253,6 +1258,7 @@ int scsi_decide_disposition(struct scsi_
> * and not get stuck in a loop.
> */
> case DID_SOFT_ERROR:
> + scmd->blk_result = BLKERR_RETRY_DEV;
> goto maybe_retry;
> case DID_IMM_RETRY:
> return NEEDS_RETRY;
> @@ -1269,11 +1275,12 @@ int scsi_decide_disposition(struct scsi_
> */
> break;
> /* fallthrough */
> -
> case DID_BUS_BUSY:
> case DID_PARITY:
> + scmd->blk_result = BLKERR_RETRY_XPT;
> goto maybe_retry;
> case DID_TIME_OUT:
> + scmd->blk_result = BLKERR_FATAL_DEV;
> /*
> * when we scan the bus, we get timeout messages for
> * these commands if there is no device available.
> @@ -1286,16 +1293,20 @@ int scsi_decide_disposition(struct scsi_
> return FAILED;
> }
> case DID_RESET:
> + scmd->blk_result = BLKERR_RETRY_DEV;
> return SUCCESS;
> default:
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return FAILED;
> }
>
> /*
> * next, check the message byte.
> */
> - if (msg_byte(scmd->result) != COMMAND_COMPLETE)
> + if (msg_byte(scmd->result) != COMMAND_COMPLETE) {
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return FAILED;
> + }
>
> /*
> * check the status byte to see if this indicates
> anything special.
> @@ -1315,13 +1326,19 @@ int scsi_decide_disposition(struct scsi_
> */
> return ADD_TO_MLQUEUE;
> case GOOD:
> + scmd->blk_result = BLK_SUCCESS;
> + return SUCCESS;
> case COMMAND_TERMINATED:
> case TASK_ABORTED:
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return SUCCESS;
> case CHECK_CONDITION:
> rtn = scsi_check_sense(scmd);
> - if (rtn == NEEDS_RETRY)
> + if (rtn == NEEDS_RETRY) {
> + scmd->blk_result = BLKERR_RETRY_DEV;
> goto maybe_retry;
> + } else
> + scmd->blk_result = BLKERR_FATAL_DEV;
> /* if rtn == FAILED, we have no sense information;
> * returning FAILED will wake the error handler thread
> * to collect the sense and redo the decide
> @@ -1334,6 +1351,7 @@ int scsi_decide_disposition(struct scsi_
> /*
> * who knows? FIXME(eric)
> */
> + scmd->blk_result = BLK_SUCCESS;
> return SUCCESS;
>
> case RESERVATION_CONFLICT:
> @@ -1341,10 +1359,13 @@ int scsi_decide_disposition(struct scsi_
> " %d channel %d id %d lun %d\n",
> scmd->device->host->host_no,
> scmd->device->channel,
> scmd->device->id, scmd->device->lun);
> + scmd->blk_result = BLK_SUCCESS;
> return SUCCESS; /* causes immediate i/o error */
> default:
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return FAILED;
> }
> + scmd->blk_result = BLKERR_FATAL_DEV;
> return FAILED;
>
> maybe_retry:
> @@ -1519,6 +1540,7 @@ static void scsi_eh_flush_done_q(struct
> SCSI_LOG_ERROR_RECOVERY(3, printk("%s:
> flush finish"
> " cmd: %p\n",
>
> current->comm, scmd));
> + scmd->blk_result = BLKERR_RETRY_DEV;
> scsi_finish_command(scmd);
> }
> }
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -513,7 +513,7 @@ void scsi_run_host_queues(struct Scsi_Ho
> * of upper level post-processing and scsi_io_completion).
> *
> * Arguments: cmd - command that is complete.
> - * uptodate - 1 if I/O indicates success, <= 0
> for I/O error.
> + * uptodate - a block layer error value
> * bytes - number of bytes of completed I/O
> * requeue - indicates whether we should requeue
> leftovers.
> *
> @@ -527,7 +527,7 @@ void scsi_run_host_queues(struct Scsi_Ho
> * We are guaranteeing that the request queue will
> be goosed
> * at some point during this call.
> */
> -static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
> *cmd, int uptodate,
> +static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd
> *cmd, int blkerr,
> int bytes, int requeue)
> {
> request_queue_t *q = cmd->device->request_queue;
> @@ -538,15 +538,15 @@ static struct scsi_cmnd *scsi_end_reques
> * If there are blocks left over at the end, set up the command
> * to queue the remainder of them.
> */
> - if (end_that_request_chunk(req, uptodate, bytes)) {
> + if (end_that_request_chunk(req, blkerr, bytes)) {
> int leftover = (req->hard_nr_sectors << 9);
>
> if (blk_pc_request(req))
> leftover = req->data_len;
>
> /* kill remainder if no retrys */
> - if (!uptodate && blk_noretry_request(req))
> - end_that_request_chunk(req, 0, leftover);
> + if (blkerr != BLK_SUCCESS && blk_noretry_request(req))
> + end_that_request_chunk(req, blkerr, leftover);
> else {
> if (requeue)
> /*
> @@ -781,7 +781,8 @@ void scsi_io_completion(struct scsi_cmnd
> * requeueing right here - we will requeue down below
> * when we handle the bad sectors.
> */
> - cmd = scsi_end_request(cmd, 1, good_bytes, result == 0);
> + cmd = scsi_end_request(cmd, BLK_SUCCESS, good_bytes,
> + result == 0);
>
> /*
> * If the command completed without error, then
> either finish off the
> @@ -804,8 +805,8 @@ void scsi_io_completion(struct scsi_cmnd
> * and quietly refuse further access.
> */
> cmd->device->changed = 1;
> - cmd = scsi_end_request(cmd, 0,
> - this_count, 1);
> + cmd = scsi_end_request(cmd,
> BLKERR_FATAL_DEV,
> + this_count, 1);
> return;
> } else {
> /*
> @@ -838,7 +839,8 @@ void scsi_io_completion(struct scsi_cmnd
> scsi_requeue_command(q, cmd);
> result = 0;
> } else {
> - cmd = scsi_end_request(cmd, 0,
> this_count, 1);
> + cmd = scsi_end_request(cmd,
> cmd->blk_result,
> + this_count, 1);
> return;
> }
> break;
> @@ -853,7 +855,8 @@ void scsi_io_completion(struct scsi_cmnd
> }
> printk(KERN_INFO "Device %s not ready.\n",
> req->rq_disk ?
> req->rq_disk->disk_name : "");
> - cmd = scsi_end_request(cmd, 0, this_count, 1);
> + cmd = scsi_end_request(cmd, cmd->blk_result,
> + this_count, 1);
> return;
> case VOLUME_OVERFLOW:
> printk(KERN_INFO "Volume overflow <%d
> %d %d %d> CDB: ",
> @@ -862,7 +865,8 @@ void scsi_io_completion(struct scsi_cmnd
> (int)cmd->device->id,
> (int)cmd->device->lun);
> __scsi_print_command(cmd->data_cmnd);
> scsi_print_sense("", cmd);
> - cmd = scsi_end_request(cmd, 0, block_bytes, 1);
> + cmd = scsi_end_request(cmd, cmd->blk_result,
> + block_bytes, 1);
> return;
> default:
> break;
> @@ -894,7 +898,7 @@ void scsi_io_completion(struct scsi_cmnd
> block_bytes = req->hard_cur_sectors << 9;
> if (!block_bytes)
> block_bytes = req->data_len;
> - cmd = scsi_end_request(cmd, 0, block_bytes, 1);
> + cmd = scsi_end_request(cmd, cmd->blk_result,
> block_bytes, 1);
> }
> }
> EXPORT_SYMBOL(scsi_io_completion);
> @@ -1246,7 +1250,8 @@ static void scsi_kill_requests(request_q
> while ((req = elv_next_request(q)) != NULL) {
> blkdev_dequeue_request(req);
> req->flags |= REQ_QUIET;
> - while (end_that_request_first(req, 0, req->nr_sectors))
> + while (end_that_request_first(req, BLKERR_FATAL_DEV,
> + req->nr_sectors))
> ;
> end_that_request_last(req);
> }
> @@ -1301,7 +1306,8 @@ static void scsi_request_fn(struct reque
> sdev->host->host_no, sdev->id,
> sdev->lun);
> blkdev_dequeue_request(req);
> req->flags |= REQ_QUIET;
> - while (end_that_request_first(req, 0,
> req->nr_sectors))
> + while (end_that_request_first(req,
> BLKERR_FATAL_DEV,
> + req->nr_sectors))
> ;
> end_that_request_last(req);
> continue;
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -754,7 +754,8 @@ static void sd_end_flush(request_queue_t
> /*
> * force journal abort of barriers
> */
> - end_that_request_first(rq, -EOPNOTSUPP,
> rq->hard_nr_sectors);
> + end_that_request_first(rq, BLKERR_NOTSUPP,
> + rq->hard_nr_sectors);
> end_that_request_last(rq);
> }
> }
> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
> --- a/include/scsi/scsi_cmnd.h
> +++ b/include/scsi/scsi_cmnd.h
> @@ -127,6 +127,7 @@ struct scsi_cmnd {
> * to be at an
> address < 16Mb). */
>
> int result; /* Status code from lower level
> driver */
> + int blk_result; /* block layer return value */
>
> unsigned char tag; /* SCSI-II queued command tag */
> unsigned long pid; /* Process ID, starts at 0.
> Unique per host. */
>
>
> -
> To unsubscribe from this list: send the line "unsubscribe
> linux-scsi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
More information about the dm-devel
mailing list