Hannes Reinecke [[email protected]] wrote:
> > Also (although this might be a bit off topic from your patch),
> > can we expand such a distinction to what should be logged?
> > Currently, it's difficult to distinguish important SCSI/block errors
> > and less important ones in kernel log.
> > For example, when I get a link failure on sda, kernel prints something
> > like below, regardless of whether the I/O is recovered by multipathing or not:
> > end_request: I/O error, dev sda, sector XXXXX
> >
> Indeed, when using the above we could be modifying the above
> message, eg by
>
> end_request: transport error, dev sda, sector XXXXX
>
> or
>
> end_request: target error, dev sda, sector XXXXX
>
> which would improve the output noticeable.
>
> > Setting REQ_QUIET in dm-multipath could mask the message
> > but also other important ones in SCSI.
> >
> Hmm. Not sure about that, but I think the above modifications will
> be useful already.
>
> I'll be sending an updated patch.
Hannes, is there an updated version of this patch? It applied fine with
Linus git tree with a minor reject! I would like to test an updated
version if you have one (the update seems to refer to better logging
only, right?).
Thanks, Malahal.
On Thu, Nov 18 2010 at 10:11pm -0500,
Malahal Naineni <[email protected]> wrote:
> Hannes Reinecke [[email protected]] wrote:
> > > Also (although this might be a bit off topic from your patch),
> > > can we expand such a distinction to what should be logged?
> > > Currently, it's difficult to distinguish important SCSI/block errors
> > > and less important ones in kernel log.
> > > For example, when I get a link failure on sda, kernel prints something
> > > like below, regardless of whether the I/O is recovered by multipathing or not:
> > > end_request: I/O error, dev sda, sector XXXXX
> > >
> > Indeed, when using the above we could be modifying the above
> > message, eg by
> >
> > end_request: transport error, dev sda, sector XXXXX
> >
> > or
> >
> > end_request: target error, dev sda, sector XXXXX
> >
> > which would improve the output noticeable.
> >
> > > Setting REQ_QUIET in dm-multipath could mask the message
> > > but also other important ones in SCSI.
> > >
> > Hmm. Not sure about that, but I think the above modifications will
> > be useful already.
> >
> > I'll be sending an updated patch.
>
> Hannes, is there an updated version of this patch? It applied fine with
> Linus git tree with a minor reject! I would like to test an updated
> version if you have one (the update seems to refer to better logging
> only, right?).
Hannes,
Any chance you've had time to fold your proposed logging changes in and
rebase this patch? Could you post that updated patch?
I'd like to help see this patch through to inclussion when 2.6.38 merge
window opens. I can help with further review, testing and development.
Please advise, thanks.
Mike
From: Hannes Reinecke <[email protected]>
Instead of just passing 'EIO' for any I/O error we should be
notifying the upper layers with more details about the cause
of this error.
Update the possible I/O errors to:
- ENOLINK: Link failure between host and target
- EIO: Retryable I/O error
- EREMOTEIO: Non-retryable I/O error
'Retryable' in this context means that an I/O error _might_ be
restricted to the I_T_L nexus (vulgo: path), so retrying on another
nexus / path might succeed.
'Non-retryable' means target failure or reservation conflict.
Signed-off-by: Hannes Reinecke <[email protected]>
Signed-off-by: Mike Snitzer <[email protected]>
---
drivers/scsi/scsi_error.c | 24 +++++++++++++++++-------
drivers/scsi/scsi_lib.c | 24 ++++++++++++++++++++++--
include/scsi/scsi.h | 3 +++
3 files changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 824b8fc..48fdd85 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -223,7 +223,7 @@ static inline void scsi_eh_prt_fail_stats(struct Scsi_Host *shost,
* @scmd: Cmd to have sense checked.
*
* Return value:
- * SUCCESS or FAILED or NEEDS_RETRY
+ * SUCCESS or FAILED or NEEDS_RETRY or TARGET_ERROR
*
* Notes:
* When a deferred error is detected the current command has
@@ -326,17 +326,19 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
*/
return SUCCESS;
- /* these three are not supported */
+ /* these are not supported */
case COPY_ABORTED:
case VOLUME_OVERFLOW:
case MISCOMPARE:
- return SUCCESS;
+ case BLANK_CHECK:
+ case DATA_PROTECT:
+ return TARGET_ERROR;
case MEDIUM_ERROR:
if (sshdr.asc == 0x11 || /* UNRECOVERED READ ERR */
sshdr.asc == 0x13 || /* AMNF DATA FIELD */
sshdr.asc == 0x14) { /* RECORD NOT FOUND */
- return SUCCESS;
+ return TARGET_ERROR;
}
return NEEDS_RETRY;
@@ -344,11 +346,9 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
if (scmd->device->retry_hwerror)
return ADD_TO_MLQUEUE;
else
- return SUCCESS;
+ return TARGET_ERROR;
case ILLEGAL_REQUEST:
- case BLANK_CHECK:
- case DATA_PROTECT:
default:
return SUCCESS;
}
@@ -809,6 +809,7 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
case SUCCESS:
case NEEDS_RETRY:
case FAILED:
+ case TARGET_ERROR:
break;
case ADD_TO_MLQUEUE:
rtn = NEEDS_RETRY;
@@ -1502,6 +1503,14 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
rtn = scsi_check_sense(scmd);
if (rtn == NEEDS_RETRY)
goto maybe_retry;
+ else if (rtn == TARGET_ERROR) {
+ /*
+ * Need to modify host byte to signal a
+ * permanent target failure
+ */
+ scmd->result |= (DID_TARGET_FAILURE << 16);
+ rtn = SUCCESS;
+ }
/* if rtn == FAILED, we have no sense information;
* returning FAILED will wake the error handler thread
* to collect the sense and redo the decide
@@ -1519,6 +1528,7 @@ int scsi_decide_disposition(struct scsi_cmnd *scmd)
case RESERVATION_CONFLICT:
sdev_printk(KERN_INFO, scmd->device,
"reservation conflict\n");
+ scmd->result |= (DID_TARGET_FAILURE << 16);
return SUCCESS; /* causes immediate i/o error */
default:
return FAILED;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eafeeda..4da6459 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -667,6 +667,26 @@ void scsi_release_buffers(struct scsi_cmnd *cmd)
}
EXPORT_SYMBOL(scsi_release_buffers);
+static int __scsi_error_from_host_byte(struct scsi_cmnd *cmd, int result)
+{
+ int error = 0;
+
+ switch(host_byte(result)) {
+ case DID_TRANSPORT_FAILFAST:
+ error = -ENOLINK;
+ break;
+ case DID_TARGET_FAILURE:
+ cmd->result |= (DID_OK << 16);
+ error = -EREMOTEIO;
+ break;
+ default:
+ error = -EIO;
+ break;
+ }
+
+ return error;
+}
+
/*
* Function: scsi_io_completion()
*
@@ -737,7 +757,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
req->sense_len = len;
}
if (!sense_deferred)
- error = -EIO;
+ error = __scsi_error_from_host_byte(cmd, result);
}
req->resid_len = scsi_get_resid(cmd);
@@ -796,7 +816,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
return;
- error = -EIO;
+ error = __scsi_error_from_host_byte(cmd, result);
if (host_byte(result) == DID_RESET) {
/* Third party bus reset or reset for error recovery
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 216af85..73d27d9 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -405,6 +405,8 @@ static inline int scsi_is_wlun(unsigned int lun)
* recover the link. Transport class will
* retry or fail IO */
#define DID_TRANSPORT_FAILFAST 0x0f /* Transport class fastfailed the io */
+#define DID_TARGET_FAILURE 0x10 /* Permanent target failure, do not retry on
+ * other paths */
#define DRIVER_OK 0x00 /* Driver status */
/*
@@ -434,6 +436,7 @@ static inline int scsi_is_wlun(unsigned int lun)
#define TIMEOUT_ERROR 0x2007
#define SCSI_RETURN_NOT_HANDLED 0x2008
#define FAST_IO_FAIL 0x2009
+#define TARGET_ERROR 0x200A
/*
* Midlevel queue return values.
--
1.7.2.3
Refreshed Hannes' initial "scsi: Detailed I/O errors" patch against
v2.6.37-rc5. v2 introduces __scsi_error_from_host_byte to avoid
the duplicate switch statement. Also a few whitespace and comment
changes.
Split DM mpath change out to separate v2 patch; failed discard is now
retryable in the face of a non-target IO error.
Added improved block layer's I/O error message (based on the finer
grained I/O error returns afforded by SCSI).
Comments/suggestions are welcome.
Thanks,
Mike
Hannes Reinecke (1):
scsi: Detailed I/O errors
Mike Snitzer (2):
dm mpath: propagate target I/O errors immediately
block: improve detail in I/O error messages
block/blk-core.c | 12 +++++++++---
drivers/md/dm-mpath.c | 11 +----------
drivers/scsi/scsi_error.c | 24 +++++++++++++++++-------
drivers/scsi/scsi_lib.c | 24 ++++++++++++++++++++++--
include/scsi/scsi.h | 3 +++
5 files changed, 52 insertions(+), 22 deletions(-)
--
1.7.2.3
Classify severity of I/O errors for target and transport errors.
Signed-off-by: Mike Snitzer <[email protected]>
---
block/blk-core.c | 12 +++++++++---
1 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 4ce953f..ab8c776 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2028,9 +2028,15 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
if (error && req->cmd_type == REQ_TYPE_FS &&
!(req->cmd_flags & REQ_QUIET)) {
- printk(KERN_ERR "end_request: I/O error, dev %s, sector %llu\n",
- req->rq_disk ? req->rq_disk->disk_name : "?",
- (unsigned long long)blk_rq_pos(req));
+ char *error_type = "I/O";
+
+ if (error == -ENOLINK)
+ error_type = "recoverable transport";
+ else if (error == -EREMOTEIO)
+ error_type = "critical target";
+ printk(KERN_ERR "end_request: %s error, dev %s, sector %llu\n",
+ error_type, req->rq_disk ? req->rq_disk->disk_name : "?",
+ (unsigned long long)blk_rq_pos(req));
}
blk_account_io_completion(req, nr_bytes);
--
1.7.2.3
DM now has more information about the nature of the underlying storage
failure. Path failure is avoided if a request failed due to a target
error. Instead the target error is immediately passed up the stack.
Discard requests that fail due to non-target errors may now be retried.
Signed-off-by: Mike Snitzer <[email protected]>
---
drivers/md/dm-mpath.c | 11 +----------
1 files changed, 1 insertions(+), 10 deletions(-)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 487ecda..071529a 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1270,16 +1270,7 @@ static int do_end_io(struct multipath *m, struct request *clone,
if (!error && !clone->errors)
return 0; /* I/O complete */
- if (error == -EOPNOTSUPP)
- return error;
-
- if (clone->cmd_flags & REQ_DISCARD)
- /*
- * Pass all discard request failures up.
- * FIXME: only fail_path if the discard failed due to a
- * transport problem. This requires precise understanding
- * of the underlying failure (e.g. the SCSI sense).
- */
+ if (error == -EOPNOTSUPP || error == -EREMOTEIO)
return error;
if (mpio->pgpath)
--
1.7.2.3
Hello.
On 08-12-2010 2:16, Mike Snitzer wrote:
> Classify severity of I/O errors for target and transport errors.
> Signed-off-by: Mike Snitzer<[email protected]>
[...]
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 4ce953f..ab8c776 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2028,9 +2028,15 @@ bool blk_update_request(struct request *req, int error, unsigned int nr_bytes)
>
> if (error&& req->cmd_type == REQ_TYPE_FS&&
> !(req->cmd_flags& REQ_QUIET)) {
> - printk(KERN_ERR "end_request: I/O error, dev %s, sector %llu\n",
> - req->rq_disk ? req->rq_disk->disk_name : "?",
> - (unsigned long long)blk_rq_pos(req));
> + char *error_type = "I/O";
> +
> + if (error == -ENOLINK)
> + error_type = "recoverable transport";
> + else if (error == -EREMOTEIO)
> + error_type = "critical target";
*switch* would be more natural here.
WBR, Sergei
Classify severity of I/O errors for target and transport errors.
Signed-off-by: Mike Snitzer <[email protected]>
---
block/blk-core.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c
+++ linux-2.6/block/blk-core.c
@@ -2028,9 +2028,23 @@ bool blk_update_request(struct request *
if (error && req->cmd_type == REQ_TYPE_FS &&
!(req->cmd_flags & REQ_QUIET)) {
- printk(KERN_ERR "end_request: I/O error, dev %s, sector %llu\n",
- req->rq_disk ? req->rq_disk->disk_name : "?",
- (unsigned long long)blk_rq_pos(req));
+ char *error_type;
+
+ switch (error) {
+ case -ENOLINK:
+ error_type = "recoverable transport";
+ break;
+ case -EREMOTEIO:
+ error_type = "critical target";
+ break;
+ case -EIO:
+ default:
+ error_type = "I/O";
+ break;
+ }
+ printk(KERN_ERR "end_request: %s error, dev %s, sector %llu\n",
+ error_type, req->rq_disk ? req->rq_disk->disk_name : "?",
+ (unsigned long long)blk_rq_pos(req));
}
blk_account_io_completion(req, nr_bytes);
Mike Snitzer [[email protected]] wrote:
> Refreshed Hannes' initial "scsi: Detailed I/O errors" patch against
> v2.6.37-rc5. v2 introduces __scsi_error_from_host_byte to avoid
> the duplicate switch statement. Also a few whitespace and comment
> changes.
>
> Split DM mpath change out to separate v2 patch; failed discard is now
> retryable in the face of a non-target IO error.
>
> Added improved block layer's I/O error message (based on the finer
> grained I/O error returns afforded by SCSI).
>
> Comments/suggestions are welcome.
I did test the Hannes original patch with the latest Linus' git tree! I
used scsi_debug to simulate path failures as well as 'Media' failures
and it did work as expected. I will test your patches soon.
Thanks, Malahal.
On 11/30/2010 11:59 PM, Mike Snitzer wrote:
> On Thu, Nov 18 2010 at 10:11pm -0500,
> Malahal Naineni <[email protected]> wrote:
>
>> Hannes Reinecke [[email protected]] wrote:
>>>> Also (although this might be a bit off topic from your patch),
>>>> can we expand such a distinction to what should be logged?
>>>> Currently, it's difficult to distinguish important SCSI/block errors
>>>> and less important ones in kernel log.
>>>> For example, when I get a link failure on sda, kernel prints something
>>>> like below, regardless of whether the I/O is recovered by multipathing or not:
>>>> end_request: I/O error, dev sda, sector XXXXX
>>>>
>>> Indeed, when using the above we could be modifying the above
>>> message, eg by
>>>
>>> end_request: transport error, dev sda, sector XXXXX
>>>
>>> or
>>>
>>> end_request: target error, dev sda, sector XXXXX
>>>
>>> which would improve the output noticeable.
>>>
>>>> Setting REQ_QUIET in dm-multipath could mask the message
>>>> but also other important ones in SCSI.
>>>>
>>> Hmm. Not sure about that, but I think the above modifications will
>>> be useful already.
>>>
>>> I'll be sending an updated patch.
>>
>> Hannes, is there an updated version of this patch? It applied fine with
>> Linus git tree with a minor reject! I would like to test an updated
>> version if you have one (the update seems to refer to better logging
>> only, right?).
>
> Hannes,
>
> Any chance you've had time to fold your proposed logging changes in and
> rebase this patch? Could you post that updated patch?
>
yes, will be following shortly.
> I'd like to help see this patch through to inclussion when 2.6.38 merge
> window opens. I can help with further review, testing and development.
>
Ok, thanks.
Cheers,
Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Markus Rex, HRB 16746 (AG N?rnberg)
On Fri, Dec 17 2010 at 4:47am -0500,
Hannes Reinecke <[email protected]> wrote:
> On 11/30/2010 11:59 PM, Mike Snitzer wrote:
> > Hannes,
> >
> > Any chance you've had time to fold your proposed logging changes in and
> > rebase this patch? Could you post that updated patch?
> >
> yes, will be following shortly.
>
> > I'd like to help see this patch through to inclussion when 2.6.38 merge
> > window opens. I can help with further review, testing and development.
> >
> Ok, thanks.
I took some steps at furthering your work. Here is the cover letter to
the patches I resently sent to dm-devel:
https://www.redhat.com/archives/dm-devel/2010-December/msg00090.html
And here are the patches:
https://patchwork.kernel.org/patch/384612/
https://patchwork.kernel.org/patch/384602/
https://patchwork.kernel.org/patch/390882/
Please feel free to change these how ever you see fit but your feedback
is really appreciated.
Thanks,
Mike