[dm-devel] 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 at vger.kernel.org [linux-scsi-owner at vger.kernel.org] On Behalf Of Hannes Reinecke [hare at suse.de]
> Sent: Thursday, November 19, 2009 6:25 AM
> To: James Bottomley
> Cc: dm-devel at redhat.com; linux-scsi at 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 at 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 at suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)