[PATCH v3 0/3] differentiate between I/O errors

[PATCH v3 0/3] differentiate between I/O errors

[Mike Snitzer]

Refreshed against 2.6.38 merge window commit (52cfd503ad717).
Added Hannes' Acked-by to patch 2 and 3.  Removed RFC tag from all
patches now that we have consensus.

James, it would be outstanding if we could get this squeezed in to the
2.6.38 merge.  But if not, at least getting it staged in your tree for
2.6.39 merge would be great.

The non-scsi changes _could_ go through block and DM trees
respectively but I figured it easiest to keep the patchset together in
one tree (biggest change being SCSI it seemed to make sense to push
the changes through your tree).
sense).

Please advise, thanks.
Mike

Hannes Reinecke (1):
  scsi: Detailed I/O errors

Mike Snitzer (2):
  dm mpath: propagate target errors immediately
  block: improve detail in I/O error messages

 block/blk-core.c          |   20 +++++++++++++++++---
 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, 60 insertions(+), 22 deletions(-)
-- 
1.7.3.4

[PATCH v3 1/3] scsi: Detailed I/O errors

[Mike Snitzer]

From: Hannes Reinecke <hare@suse.de>

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 <hare@suse.de>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
 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 45c7564..a6c4f5d 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;
 	}
@@ -787,6 +787,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;
@@ -1469,6 +1470,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
@@ -1486,6 +1495,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 9045c52..7c5894c 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 648d233..18cb2ca 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -434,6 +434,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                           */
 
 /*
@@ -463,6 +465,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.3.4

Re: [PATCH v3 1/3] scsi: Detailed I/O errors

[Jonathan McDowell]

On Fri, Jan 14, 2011 at 10:58:54AM -0500, Mike Snitzer wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> 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.
...
> @@ -1486,6 +1495,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;
...
> +#define DID_TARGET_FAILURE 0x10 /* Permanent target failure, do not retry on
> +				 * other paths */

I'd have viewed a reservation conflict as being tied to a particular
path, rather than the entire target. I've seen multipath setups where
there are reservation issues on some of the paths but others are fine
and this is expected (eg use of reservations to fence off particular
paths).

J.

-- 
/-\                             |   It's more than good enough so I
|@/  Debian GNU/Linux Developer |           ain't switch'n.
\-                              |

Re: [PATCH v3 1/3] scsi: Detailed I/O errors

[Mike Snitzer]

On Fri, Jan 14 2011 at 11:10am -0500,
Jonathan McDowell <noodles@earth.li> wrote:

> On Fri, Jan 14, 2011 at 10:58:54AM -0500, Mike Snitzer wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > 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.
> ...
> > @@ -1486,6 +1495,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;
> ...
> > +#define DID_TARGET_FAILURE 0x10 /* Permanent target failure, do not retry on
> > +				 * other paths */
> 
> I'd have viewed a reservation conflict as being tied to a particular
> path, rather than the entire target. I've seen multipath setups where
> there are reservation issues on some of the paths but others are fine
> and this is expected (eg use of reservations to fence off particular
> paths).

Very good point (as I think you're correct).  Technically a reservation
conflict is retryable across _different_ paths but (relative to the
error path as it relates to multipath) it appears Hannes elected to go
with the conservative approach of always failing the IO upward given the
potential for data corruption when queue_if_no_path is used.

Hannes previously touched on this here:
https://www.redhat.com/archives/dm-devel/2009-November/msg00190.html

"This also solves a potential data corruption with multipathing
and persistent reservations. When queue_if_no_path is active
multipath will queue any I/O failure (including those failed
with RESERVATION CONFLICT) until the reservation status changes.
But by then I/O might have been ongoing on the other paths,
thus the delayed submission will severely corrupt your data."

Even in the context of that older SCSI sense-based mpath patchset a
reservation conflict would always fail upward (regardless of path count
and/or queue_if_no_path).

All said, the above doesn't excuse what seems to be a mis-categorization
of reservation conflict as a pure non-retryable TARGET_FAILURE
(EREMOTEIO).

But I'd like to defer to Hannes for the authorative answer ;)

Regards,
Mike

Re: [PATCH v3 1/3] scsi: Detailed I/O errors

[Hannes Reinecke]

On 01/14/2011 06:16 PM, Mike Snitzer wrote:
> On Fri, Jan 14 2011 at 11:10am -0500,
> Jonathan McDowell <noodles@earth.li> wrote:
> 
>> On Fri, Jan 14, 2011 at 10:58:54AM -0500, Mike Snitzer wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> 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.
>> ...
>>> @@ -1486,6 +1495,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;
>> ...
>>> +#define DID_TARGET_FAILURE 0x10 /* Permanent target failure, do not retry on
>>> +				 * other paths */
>>
>> I'd have viewed a reservation conflict as being tied to a particular
>> path, rather than the entire target. I've seen multipath setups where
>> there are reservation issues on some of the paths but others are fine
>> and this is expected (eg use of reservations to fence off particular
>> paths).
> 
> Very good point (as I think you're correct).  Technically a reservation
> conflict is retryable across _different_ paths but (relative to the
> error path as it relates to multipath) it appears Hannes elected to go
> with the conservative approach of always failing the IO upward given the
> potential for data corruption when queue_if_no_path is used.
> 
> Hannes previously touched on this here:
> https://www.redhat.com/archives/dm-devel/2009-November/msg00190.html
> 
> "This also solves a potential data corruption with multipathing
> and persistent reservations. When queue_if_no_path is active
> multipath will queue any I/O failure (including those failed
> with RESERVATION CONFLICT) until the reservation status changes.
> But by then I/O might have been ongoing on the other paths,
> thus the delayed submission will severely corrupt your data."
> 
> Even in the context of that older SCSI sense-based mpath patchset a
> reservation conflict would always fail upward (regardless of path count
> and/or queue_if_no_path).
> 
> All said, the above doesn't excuse what seems to be a mis-categorization
> of reservation conflict as a pure non-retryable TARGET_FAILURE
> (EREMOTEIO).
> 
Ho-hum.

Yes, and no.

Yes, it is correct that persistent reservations are in fact per
ITL nexus, and hence might yield different responses if retried on
another path.

And no, it is not entirely correct to return the standard EIO error
here as then the no_path_retry mechanism might kick in and we're
back to square one.

That said we probably need to invent another error code with
meaning 'Retry on other ITL nexus if present, but ignore no_path_retry'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
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: [PATCH v3 1/3] scsi: Detailed I/O errors

[Mike Snitzer]

On Mon, Jan 17 2011 at 10:52am -0500,
Hannes Reinecke <hare@suse.de> wrote:

> On 01/14/2011 06:16 PM, Mike Snitzer wrote:
> > On Fri, Jan 14 2011 at 11:10am -0500,
> > Jonathan McDowell <noodles@earth.li> wrote:
> >>
> >> I'd have viewed a reservation conflict as being tied to a particular
> >> path, rather than the entire target. I've seen multipath setups where
> >> there are reservation issues on some of the paths but others are fine
> >> and this is expected (eg use of reservations to fence off particular
> >> paths).
> > 
> > Very good point (as I think you're correct).  Technically a reservation
> > conflict is retryable across _different_ paths but (relative to the
> > error path as it relates to multipath) it appears Hannes elected to go
> > with the conservative approach of always failing the IO upward given the
> > potential for data corruption when queue_if_no_path is used.
> > 
> > Hannes previously touched on this here:
> > https://www.redhat.com/archives/dm-devel/2009-November/msg00190.html
> > 
> > "This also solves a potential data corruption with multipathing
> > and persistent reservations. When queue_if_no_path is active
> > multipath will queue any I/O failure (including those failed
> > with RESERVATION CONFLICT) until the reservation status changes.
> > But by then I/O might have been ongoing on the other paths,
> > thus the delayed submission will severely corrupt your data."
> > 
> > Even in the context of that older SCSI sense-based mpath patchset a
> > reservation conflict would always fail upward (regardless of path count
> > and/or queue_if_no_path).
> > 
> > All said, the above doesn't excuse what seems to be a mis-categorization
> > of reservation conflict as a pure non-retryable TARGET_FAILURE
> > (EREMOTEIO).
> > 
> Ho-hum.
> 
> Yes, and no.
> 
> Yes, it is correct that persistent reservations are in fact per
> ITL nexus, and hence might yield different responses if retried on
> another path.
> 
> And no, it is not entirely correct to return the standard EIO error
> here as then the no_path_retry mechanism might kick in and we're
> back to square one.
>
> That said we probably need to invent another error code with
> meaning 'Retry on other ITL nexus if present, but ignore no_path_retry'.

That sounds right.  So something like the following?:
- set ITL_NEXUS_ERROR/DID_ITL_NEXXUS_FAILURE in scsi (comparable to how
  you did TARGET_ERROR/DID_TARGET_FAILURE) 
- then return -EAGAIN from __scsi_error_from_host_byte() to signal to
  upper layer(s) that a retry could be worthwhile -- driver specific

In mpath's case it can respond to -EAGAIN by conversatively retrying
with the 'Retry on other ITL nexus if present, but ignore no_path_retry'
semantic?

(overloading -EAGAIN leaves something to be desired but I welcome other
ideas).

Thanks,
Mike

[PATCH v3 2/3] dm mpath: propagate target errors immediately

[Mike Snitzer]

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 <snitzer@redhat.com>
Acked-by: Hannes Reinecke <hare@suse.de>
---
 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 b82d288..532b845 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -1283,16 +1283,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.3.4

[PATCH v3 3/3] block: improve detail in I/O error messages

[Mike Snitzer]

Classify severity of I/O errors for target and transport errors.

Signed-off-by: Mike Snitzer <snitzer@redhat.com>
Acked-by: Hannes Reinecke <hare@suse.de>
---
 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
@@ -2044,9 +2044,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);