[PATCH] multipath: Evaluate request result and sense code

[PATCH] multipath: Evaluate request result and sense code

[Hannes Reinecke]

As we now see the result for every command we
can use it to make some more elaborate choices
if we should retry the request on another path.

This solves a potential data corruption when
a request is being terminated with RESERVATION
CONFLICT and queue_if_no_path is active; the
request will be queued until the reservation
status changes and then transmitted.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index dce971d..460e11f 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -19,6 +19,7 @@
 #include <linux/time.h>
 #include <linux/workqueue.h>
 #include <scsi/scsi_dh.h>
+#include <scsi/scsi_eh.h>
 #include <asm/atomic.h>
 
 #define DM_MSG_PREFIX "multipath"
@@ -101,6 +102,7 @@ struct multipath {
 struct dm_mpath_io {
 	struct pgpath *pgpath;
 	size_t nr_bytes;
+	char sense[SCSI_SENSE_BUFFERSIZE];
 };
 
 typedef int (*action_fn) (struct pgpath *pgpath);
@@ -913,6 +915,9 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
 
 	map_context->ptr = mpio;
 	clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
+	/* Always attach a sense buffer */
+	if (!clone->sense)
+		clone->sense = mpio->sense;
 	r = map_io(m, clone, mpio, 0);
 	if (r < 0 || r == DM_MAPIO_REQUEUE)
 		mempool_free(mpio, m->mpio_pool);
@@ -1192,6 +1197,42 @@ static void activate_path(struct work_struct *work)
 }
 
 /*
+ * Evaluate scsi return code
+ */
+static int eval_scsi_error(int result, char *sense, int sense_len)
+{
+	struct scsi_sense_hdr sshdr;
+	int r = DM_ENDIO_REQUEUE;
+
+	if (host_byte(result) != DID_OK)
+		return r;
+
+	if (msg_byte(result) != COMMAND_COMPLETE)
+		return r;
+
+	if (status_byte(result) == RESERVATION_CONFLICT)
+		/* Do not retry here, possible data corruption */
+		return -EIO;
+
+	if (status_byte(result) == CHECK_CONDITION &&
+	    !scsi_normalize_sense(sense, sense_len, &sshdr)) {
+
+		switch (sshdr.sense_key) {
+		case MEDIUM_ERROR:
+		case DATA_PROTECT:
+		case BLANK_CHECK:
+		case COPY_ABORTED:
+		case VOLUME_OVERFLOW:
+		case MISCOMPARE:
+			r = -EIO;
+			break;
+		}
+	}
+
+	return r;
+}
+
+/*
  * end_io handling
  */
 static int do_end_io(struct multipath *m, struct request *clone,
@@ -1217,6 +1258,10 @@ static int do_end_io(struct multipath *m, struct request *clone,
 	if (error == -EOPNOTSUPP)
 		return error;
 
+	r = eval_scsi_error(clone->errors, clone->sense, clone->sense_len);
+	if (r != DM_ENDIO_REQUEUE)
+		return r;
+
 	if (mpio->pgpath)
 		fail_path(mpio->pgpath);
 
@@ -1243,6 +1288,10 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
 		if (ps->type->end_io)
 			ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
 	}
+	if (clone->sense == mpio->sense) {
+		clone->sense = NULL;
+		clone->sense_len = 0;
+	}
 	mempool_free(mpio, m->mpio_pool);
 
 	return r;
-- 
1.5.3.2

Re: [PATCH] multipath: Evaluate request result and sense code

[Mike Christie]

Hannes Reinecke wrote:
> As we now see the result for every command we
> can use it to make some more elaborate choices
> if we should retry the request on another path.
> 
> This solves a potential data corruption when
> a request is being terminated with RESERVATION
> CONFLICT and queue_if_no_path is active; the
> request will be queued until the reservation
> status changes and then transmitted.


I had the same bz. To handle it I just converted the error to some other 
-EXYZ value. But like I said in my patch that I sent to the list I did 
not like it much.

I thought of going the route you did in this patch, but thought it was 
too scsi specific. Does dasd want advanced error handling? If not, then 
I am fine since for hw handlers we assume they are always scsi_dh modules.

Re: [dm-devel] [PATCH] multipath: Evaluate request result and sense code

[Hannes Reinecke]

Mike Christie wrote:
> Hannes Reinecke wrote:
>> As we now see the result for every command we
>> can use it to make some more elaborate choices
>> if we should retry the request on another path.
>>
>> This solves a potential data corruption when
>> a request is being terminated with RESERVATION
>> CONFLICT and queue_if_no_path is active; the
>> request will be queued until the reservation
>> status changes and then transmitted.
> 
> 
> I had the same bz. To handle it I just converted the error to some other
> -EXYZ value. But like I said in my patch that I sent to the list I did
> not like it much.
> 
Yeah, that was my thought, too.
The scsi result codes simply don't fit on the -EXXX values.

> I thought of going the route you did in this patch, but thought it was
> too scsi specific. Does dasd want advanced error handling? If not, then
> I am fine since for hw handlers we assume they are always scsi_dh modules.
Well, DASD has it's own sort of problems; basically DASD will _always_
try to do it's own internal recovery by waiting for the storage array
to respond. So with DASD we basically only ever have three states:
- GOOD
- Failure as being evaluated by the _storage array_
- Don't know (if failfast is set)
And any advanced checking won't give us much here, as either the link
is on-line and we have details about the error or the link is severed
and we don't know anything.

But there's a good point, we should be doing some sort of
cross-check to determine if it really is a SCSI error.
Just to prevent any accidental spill-overs in the future.

I'll be sending a patch.

Cheers,

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


-- 
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] multipath: Evaluate request result and sense code

[Mike Christie]

Hannes Reinecke wrote:
> Mike Christie wrote:
>> Hannes Reinecke wrote:
>>> As we now see the result for every command we
>>> can use it to make some more elaborate choices
>>> if we should retry the request on another path.
>>>
>>> This solves a potential data corruption when
>>> a request is being terminated with RESERVATION
>>> CONFLICT and queue_if_no_path is active; the
>>> request will be queued until the reservation
>>> status changes and then transmitted.
>>
>> I had the same bz. To handle it I just converted the error to some other
>> -EXYZ value. But like I said in my patch that I sent to the list I did
>> not like it much.
>>
> Yeah, that was my thought, too.
> The scsi result codes simply don't fit on the -EXXX values.
> 

What about replaceing -EXXX errors with some new BLKERR values?

Send some comments on the patch in the other mail.

Re: [PATCH] multipath: Evaluate request result and sense code

[Mike Christie]

Hannes Reinecke wrote:
>  /*
> + * Evaluate scsi return code
> + */
> +static int eval_scsi_error(int result, char *sense, int sense_len)
> +{
> +	struct scsi_sense_hdr sshdr;
> +	int r = DM_ENDIO_REQUEUE;
> +
> +	if (host_byte(result) != DID_OK)


For values like DID_NO_CONNECT or DID_TRANSPORT FAILFAST, I think it 
makes sense to fail the path. Not in this patch, but a new one, would we 
want to modify dm-mpath so that we do not fail the path for errors like 
DID_ABORT or DID_SOFT_ERROR, DID_RESET or DID_ERROR?

Re: [dm-devel] [PATCH] multipath: Evaluate request result and sense code

[Hannes Reinecke]

Mike Christie wrote:
> Hannes Reinecke wrote:
>>  /*
>> + * Evaluate scsi return code
>> + */
>> +static int eval_scsi_error(int result, char *sense, int sense_len)
>> +{
>> +    struct scsi_sense_hdr sshdr;
>> +    int r = DM_ENDIO_REQUEUE;
>> +
>> +    if (host_byte(result) != DID_OK)
> 
> 
> For values like DID_NO_CONNECT or DID_TRANSPORT FAILFAST, I think it
> makes sense to fail the path. Not in this patch, but a new one, would we
> want to modify dm-mpath so that we do not fail the path for errors like
> DID_ABORT or DID_SOFT_ERROR, DID_RESET or DID_ERROR?
Yeah, well, this patch was made to model the existing behaviour closely
so as the patch submission was not blocked by irrelevant side discussions ...

But yes, of course we should be a bit more selective on how to respond
to the various error codes. But this requires some more discussion with
the various vendors as things like 'DID_ERROR' have different meanings
for different HBAs ...

But this should be done once we agreed on the principle, ie on _how_
to pass the error codes up the the multipathing layer.

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] multipath: Evaluate request result and sense code

[Moger, Babu]

Hannes,
  My .02 cents on this below..

________________________________________
From: linux-scsi-owner@vger.kernel.org [linux-scsi-owner@vger.kernel.org] On Behalf Of Hannes Reinecke [hare@suse.de]
Sent: Thursday, November 19, 2009 6:25 AM
To: James Bottomley
Cc: dm-devel@redhat.com; linux-scsi@vger.kernel.org
Subject: [PATCH] multipath: Evaluate request result and sense code

As we now see the result for every command we
can use it to make some more elaborate choices
if we should retry the request on another path.

This solves a potential data corruption when
a request is being terminated with RESERVATION
CONFLICT and queue_if_no_path is active; the
request will be queued until the reservation
status changes and then transmitted.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/md/dm-mpath.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index dce971d..460e11f 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -19,6 +19,7 @@
 #include <linux/time.h>
 #include <linux/workqueue.h>
 #include <scsi/scsi_dh.h>
+#include <scsi/scsi_eh.h>
 #include <asm/atomic.h>

 #define DM_MSG_PREFIX "multipath"
@@ -101,6 +102,7 @@ struct multipath {
 struct dm_mpath_io {
        struct pgpath *pgpath;
        size_t nr_bytes;
+       char sense[SCSI_SENSE_BUFFERSIZE];
 };

 typedef int (*action_fn) (struct pgpath *pgpath);
@@ -913,6 +915,9 @@ static int multipath_map(struct dm_target *ti, struct request *clone,

        map_context->ptr = mpio;
        clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
+       /* Always attach a sense buffer */
+       if (!clone->sense)
+               clone->sense = mpio->sense;
        r = map_io(m, clone, mpio, 0);
        if (r < 0 || r == DM_MAPIO_REQUEUE)
                mempool_free(mpio, m->mpio_pool);
@@ -1192,6 +1197,42 @@ static void activate_path(struct work_struct *work)
 }

 /*
+ * Evaluate scsi return code
+ */
+static int eval_scsi_error(int result, char *sense, int sense_len)
+{
+       struct scsi_sense_hdr sshdr;
+       int r = DM_ENDIO_REQUEUE;
+
+       if (host_byte(result) != DID_OK)
+               return r;
+
+       if (msg_byte(result) != COMMAND_COMPLETE)
+               return r;
+
+       if (status_byte(result) == RESERVATION_CONFLICT)
+               /* Do not retry here, possible data corruption */
+               return -EIO;
+
+       if (status_byte(result) == CHECK_CONDITION &&
+           !scsi_normalize_sense(sense, sense_len, &sshdr)) {
+
+               switch (sshdr.sense_key) {
+               case MEDIUM_ERROR:
+               case DATA_PROTECT:
+               case BLANK_CHECK:
+               case COPY_ABORTED:
+               case VOLUME_OVERFLOW:
+               case MISCOMPARE:
+                       r = -EIO;
+                       break;
+               }

The above sense key list does not cover all the cases(at least for my case). For example, I have these requirements for my storage.
1. For certain check conditions I should be reporting I/O error immediately without retrying on other paths. You have covered some cases here but we a have bigger list.
2. For few other check condtions I should be calling activate_path instead of failing the path.
So, I am thinking it may be better to handle this with scsi device handlers(may be providing a new scsi_dh interface and fallback to default handling if the device handler does not handle the sense).  I am not sure if this is feasible but something to think about.

+       }
+
+       return r;
+}
+
+/*
  * end_io handling
  */
 static int do_end_io(struct multipath *m, struct request *clone,
@@ -1217,6 +1258,10 @@ static int do_end_io(struct multipath *m, struct request *clone,
        if (error == -EOPNOTSUPP)
                return error;

+       r = eval_scsi_error(clone->errors, clone->sense, clone->sense_len);
+       if (r != DM_ENDIO_REQUEUE)
+               return r;
+
        if (mpio->pgpath)
                fail_path(mpio->pgpath);

@@ -1243,6 +1288,10 @@ static int multipath_end_io(struct dm_target *ti, struct request *clone,
                if (ps->type->end_io)
                        ps->type->end_io(ps, &pgpath->path, mpio->nr_bytes);
        }
+       if (clone->sense == mpio->sense) {
+               clone->sense = NULL;
+               clone->sense_len = 0;
+       }
        mempool_free(mpio, m->mpio_pool);

        return r;
--
1.5.3.2

--
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] multipath: Evaluate request result and sense code

[Hannes Reinecke]

Moger, Babu wrote:
> Hannes,
>   My .02 cents on this below..
> 
> ________________________________________
> From: linux-scsi-owner@vger.kernel.org [linux-scsi-owner@vger.kernel.org] On Behalf Of Hannes Reinecke [hare@suse.de]
> Sent: Thursday, November 19, 2009 6:25 AM
> To: James Bottomley
> Cc: dm-devel@redhat.com; linux-scsi@vger.kernel.org
> Subject: [PATCH] multipath: Evaluate request result and sense code
> 
> As we now see the result for every command we
> can use it to make some more elaborate choices
> if we should retry the request on another path.
> 
> This solves a potential data corruption when
> a request is being terminated with RESERVATION
> CONFLICT and queue_if_no_path is active; the
> request will be queued until the reservation
> status changes and then transmitted.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/md/dm-mpath.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 49 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index dce971d..460e11f 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -19,6 +19,7 @@
>  #include <linux/time.h>
>  #include <linux/workqueue.h>
>  #include <scsi/scsi_dh.h>
> +#include <scsi/scsi_eh.h>
>  #include <asm/atomic.h>
> 
>  #define DM_MSG_PREFIX "multipath"
> @@ -101,6 +102,7 @@ struct multipath {
>  struct dm_mpath_io {
>         struct pgpath *pgpath;
>         size_t nr_bytes;
> +       char sense[SCSI_SENSE_BUFFERSIZE];
>  };
> 
>  typedef int (*action_fn) (struct pgpath *pgpath);
> @@ -913,6 +915,9 @@ static int multipath_map(struct dm_target *ti, struct request *clone,
> 
>         map_context->ptr = mpio;
>         clone->cmd_flags |= REQ_FAILFAST_TRANSPORT;
> +       /* Always attach a sense buffer */
> +       if (!clone->sense)
> +               clone->sense = mpio->sense;
>         r = map_io(m, clone, mpio, 0);
>         if (r < 0 || r == DM_MAPIO_REQUEUE)
>                 mempool_free(mpio, m->mpio_pool);
> @@ -1192,6 +1197,42 @@ static void activate_path(struct work_struct *work)
>  }
> 
>  /*
> + * Evaluate scsi return code
> + */
> +static int eval_scsi_error(int result, char *sense, int sense_len)
> +{
> +       struct scsi_sense_hdr sshdr;
> +       int r = DM_ENDIO_REQUEUE;
> +
> +       if (host_byte(result) != DID_OK)
> +               return r;
> +
> +       if (msg_byte(result) != COMMAND_COMPLETE)
> +               return r;
> +
> +       if (status_byte(result) == RESERVATION_CONFLICT)
> +               /* Do not retry here, possible data corruption */
> +               return -EIO;
> +
> +       if (status_byte(result) == CHECK_CONDITION &&
> +           !scsi_normalize_sense(sense, sense_len, &sshdr)) {
> +
> +               switch (sshdr.sense_key) {
> +               case MEDIUM_ERROR:
> +               case DATA_PROTECT:
> +               case BLANK_CHECK:
> +               case COPY_ABORTED:
> +               case VOLUME_OVERFLOW:
> +               case MISCOMPARE:
> +                       r = -EIO;
> +                       break;
> +               }
> 
> The above sense key list does not cover all the cases(at least for my case).
Of course. This is by no means meant to be the final list on sense keys.
This is basically copied from scsi_decide_dispostion to give us a starting
point with some sane defaults.

> For example, I have these requirements for my storage.
> 1. For certain check conditions I should be reporting I/O error immediately
> without retrying on other paths. You have covered some cases here but we a
> have bigger list.
> 2. For few other check condtions I should be calling activate_path instead
> of failing the path.
> So, I am thinking it may be better to handle this with scsi device handlers
> (may be providing a new scsi_dh interface and fallback to default handling
> if the device handler does not handle the sense).  I am not sure if this is
> feasible but something to think about.
> 
Yes, this is the plan. Each device_handler has it's own sense_handler to
handle the sense codes it cares about. This handler is called prior
to the generic one (ie the one I posted), enabling it to modify
the return code so that the generic handler can do the right decision.

Of course this patch isn't final, it was meant as a starting point
to agree on the infrastructure. How the individual errors should
be handled should be discussed in the next stage once the infrastructure
is in place.

Alternatively you could sent a patch on top of my patchset to modify
the behaviour. I'll be happy to include it in the next revision of the
patchset :-)

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