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.

Quoted 9 lines · 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.

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.

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 11 lines · Mike Christie

Quoted 9 lines · 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.

Quoted 4 lines

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.

drivers/md/dm-mpath.c static void activate_path(struct work_struct *work) · line 1207

Quoted 9 lines · Hannes Reinecke

/*
+ * Evaluate scsi return code
+ */

drivers/md/dm-mpath.c +6 -0

+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?

Hannes,
My .02 cents on this below..

________________________________________
From: [email protected] [[email protected]] On Behalf Of Hannes Reinecke [[email protected]]
Sent: Thursday, November 19, 2009 6:25 AM
To: James Bottomley
Cc: [email protected]; [email protected]
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 <[email protected]>

---
 drivers/md/dm-mpath.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

drivers/md/dm-mpath.c +35 -0

index dce971d..460e11f 100644
@@ -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.