[PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Christie]

Currently dm-multipath has what it calls hw_handlers that contain hw
details and currently those handlers can be used to failover storage
works, LSI/Engenio RDAC mode, and EMC Clariion boxes. This code is a
little strange in dm-multipath and they are doing scsi no good (think
about problems initializing scsi disks/paths when they are in a passive
path that requires explicit/manual activation and we retry over and over
and over).

The patch below begins to push the scsi hw handler code down to the scsi
layer. I only began to covert dm-emc.c and it only hooks in at the sense
decoding in scsi_error.c. I wanted to make sure I was going about the
module loading and binding correctly. With a new target bus we could do
some driver model stuff instead, but I was not sure if that was
appropriate for this?

Next up would be to get Jens's cmd type tree and work on the message
passing code.

And this patch currently does not work since I do not have a Clariion
box and I do not know what to match for the {vendor, model} tuple. I
think it was something like DCG or DGC and the model had different RAID
strings depending on how it was setup and could have some other string
if it did not use RAID. If you have a Clariion or you work for EMC and
happen to know this info could you pass those strings to me?

This patch was made over 2.6.18-r2 since I screwed up my git trees and I
cannot seem to download them from where I am at.

diff -Naurp linux-2.6.18-rc2/drivers/scsi/Kconfig linux-2.6.18-rc2.work/drivers/scsi/Kconfig
--- linux-2.6.18-rc2/drivers/scsi/Kconfig	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/drivers/scsi/Kconfig	2006-07-21 06:44:58.000000000 -0400
@@ -244,6 +244,17 @@ config SCSI_SAS_ATTRS
 
 endmenu
 
+menu "SCSI Target Drivers"
+	depends on SCSI
+
+config SCSI_CLARIION
+	tristate "EMC CLARiiON Target Driver"
+	depends on SCSI
+	help
+	  If you have a EMC CLARiiON selec y. Otherwise, say N.
+
+endmenu
+
 menu "SCSI low-level drivers"
 	depends on SCSI!=n
 
diff -Naurp linux-2.6.18-rc2/drivers/scsi/Makefile linux-2.6.18-rc2.work/drivers/scsi/Makefile
--- linux-2.6.18-rc2/drivers/scsi/Makefile	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/drivers/scsi/Makefile	2006-07-21 06:40:42.000000000 -0400
@@ -138,6 +138,7 @@ obj-$(CONFIG_SCSI_SATA_ULI)	+= libata.o 
 obj-$(CONFIG_SCSI_SATA_MV)	+= libata.o sata_mv.o
 obj-$(CONFIG_SCSI_PDC_ADMA)	+= libata.o pdc_adma.o
 obj-$(CONFIG_SCSI_HPTIOP)	+= hptiop.o
+obj-$(CONFIG_SCSI_CLARIION)	+= scsi_emc_clariion.o
 
 obj-$(CONFIG_ARM)		+= arm/
 
diff -Naurp linux-2.6.18-rc2/drivers/scsi/scsi.c linux-2.6.18-rc2.work/drivers/scsi/scsi.c
--- linux-2.6.18-rc2/drivers/scsi/scsi.c	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/drivers/scsi/scsi.c	2006-07-21 06:29:02.000000000 -0400
@@ -135,6 +135,8 @@ static struct scsi_host_cmd_pool scsi_cm
 };
 
 static DEFINE_MUTEX(host_cmd_pool_mutex);
+static DEFINE_MUTEX(target_driver_mutex);
+static LIST_HEAD(target_driver_list);
 
 static struct scsi_cmnd *__scsi_get_command(struct Scsi_Host *shost,
 					    gfp_t gfp_mask)
@@ -1076,6 +1078,54 @@ int scsi_device_cancel(struct scsi_devic
 }
 EXPORT_SYMBOL(scsi_device_cancel);
 
+
+int scsi_register_target_template(struct scsi_target_template *tmpl)
+{
+	mutex_lock(&target_driver_mutex);
+	list_add_tail(&tmpl->list, &target_driver_list);
+	mutex_unlock(&target_driver_mutex);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(scsi_register_target_template);
+
+void scsi_unregister_target_template(struct scsi_target_template *tmpl)
+{
+	mutex_lock(&target_driver_mutex);
+	list_del(&tmpl->list);
+	mutex_unlock(&target_driver_mutex);
+}
+EXPORT_SYMBOL_GPL(scsi_unregister_target_template);
+
+void scsi_bind_target(struct scsi_target *starget, const unsigned char *vendor,
+		      const unsigned char *model)
+{
+	struct scsi_target_template *tmpl;
+	int rc;
+
+	mutex_lock(&target_driver_mutex);
+	if (starget->stargett)
+		goto done;
+
+	list_for_each_entry(tmpl, &target_driver_list, list) {
+		rc = tmpl->setup(starget, vendor, model);
+		switch (rc) {
+		case SCSI_DRIVER_NO_MATCH:
+			continue;
+		case SCSI_DRIVER_SETUP_FAILED:
+			/* we can limp along without the driver attached */
+			printk(KERN_INFO "Could not bind target driver to "
+			       "%s %s\n", vendor, model);
+			break;
+		case SCSI_DRIVER_SETUP_SUCCESS:
+			starget->stargett = tmpl;
+			break;
+		}
+	}
+
+done:
+	mutex_unlock(&target_driver_mutex);
+}
+
 MODULE_DESCRIPTION("SCSI core");
 MODULE_LICENSE("GPL");
 
diff -Naurp linux-2.6.18-rc2/drivers/scsi/scsi_emc_clariion.c linux-2.6.18-rc2.work/drivers/scsi/scsi_emc_clariion.c
--- linux-2.6.18-rc2/drivers/scsi/scsi_emc_clariion.c	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.18-rc2.work/drivers/scsi/scsi_emc_clariion.c	2006-07-21 06:58:03.000000000 -0400
@@ -0,0 +1,199 @@
+/*
+ * Target driver for EMC CLARiiON AX/CX-series hardware.
+ * Based on code from Lars Marowsky-Bree <lmb@suse.de>
+ */
+#include <linux/blkdev.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_eh.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+
+struct clariion_target {
+	/*
+	 * Whether we should send the short trespass command (FC-series)
+	 * or the long version (default for AX/CX CLARiiON arrays).
+	 */
+	unsigned short_trespass;
+	/*
+	 * Whether or not to honor SCSI reservations when initiating a
+	 * switch-over. Default: Don't.
+	 */
+	unsigned hr;
+};
+
+#define TRESPASS_PAGE 0x22
+
+static unsigned char long_trespass[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,		/* Page code */
+	0x09,			/* Page length - 2 */
+	0x81,			/* Trespass code + Honor reservation bit */
+	0xff, 0xff,		/* Trespass target */
+	0, 0, 0, 0, 0, 0	/* Reserved bytes / unknown */
+};
+
+static unsigned char long_trespass_hr[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,		/* Page code */
+	0x09,			/* Page length - 2 */
+	0x01,			/* Trespass code + Honor reservation bit */
+	0xff, 0xff,		/* Trespass target */
+	0, 0, 0, 0, 0, 0	/* Reserved bytes / unknown */
+};
+
+static unsigned char short_trespass[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,		/* Page code */
+	0x02,			/* Page length - 2 */
+	0x81,			/* Trespass code + Honor reservation bit */
+	0xff,			/* Trespass target */
+};
+
+static unsigned char short_trespass_hr[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,		/* Page code */
+	0x02,			/* Page length - 2 */
+	0x01,			/* Trespass code + Honor reservation bit */
+	0xff,			/* Trespass target */
+};
+
+static void clariion_activate_done(void *data, char *sense, int result,
+				   int resid)
+{
+	/*
+	struct request *rq = data;
+
+	 * TODO look at sense and result
+	 * Set some bits on the request for dm-multipath to do something
+	 * smart???
+
+	if (result)
+		scsi_complete_msg(rq, -EIO);
+	else
+		scsi_complete_msg(rq, 0);
+	*/
+}
+
+/* TODO - make configurable */
+#define CLARIION_TMO 30
+#define CLARIION_RETRIES 3
+
+static int clariion_activate(struct scsi_device *sdev, struct request *rq)
+{
+	struct scsi_target *stgt = scsi_target(sdev);
+	struct clariion_target *ctgt = stgt->targetdata;
+	unsigned char *page22;
+	unsigned size;
+	unsigned char cmd[MAX_COMMAND_SIZE];
+
+	if (ctgt->short_trespass) {
+		page22 = ctgt->hr ? short_trespass_hr : short_trespass;
+		size = sizeof(short_trespass);
+	} else {
+		page22 = ctgt->hr ? long_trespass_hr : long_trespass;
+		size = sizeof(long_trespass);
+	}
+
+	memset(cmd, 0, MAX_COMMAND_SIZE);
+	cmd[0] = MODE_SELECT;
+	cmd[1] = 0x10;
+	cmd[2] = size;
+
+	if (scsi_execute_async(sdev, cmd, COMMAND_SIZE(MODE_SELECT),
+				DMA_TO_DEVICE, page22, size, 0, CLARIION_TMO,
+				CLARIION_RETRIES, rq, clariion_activate_done,
+				GFP_ATOMIC))
+		return SCSI_MLQUEUE_DEVICE_BUSY;
+
+	return 0;
+}
+
+static int clariion_check_sense(struct scsi_sense_hdr *sense_hdr)
+{
+	switch (sense_hdr->sense_key) {
+	case NOT_READY:
+		if (sense_hdr->asc == 0x04 && sense_hdr->ascq == 0x03)
+			/* LUN Not Ready - Manual Intervention Required
+			 * indicates this is a passive path.
+			 *
+			 * FIXME: However, if this is seen and EVPD C0
+			 * indicates that this is due to a NDU in
+			 * progress, we should set FAIL_PATH too.
+			 * This indicates we might have to do a SCSI
+			 * inquiry in the end_io path. Ugh. */
+			return FAILED;
+		break;
+	case ILLEGAL_REQUEST:
+		if (sense_hdr->asc == 0x25 && sense_hdr->ascq == 0x01)
+			/* An array based copy is in progress. Do not
+			 * fail the path, do not bypass to another PG,
+			 * do not retry. Fail the IO immediately.
+			 * (Actually this is the same conclusion as in
+			 * the default handler, but lets make sure.) */
+			return FAILED;
+		break;
+	case UNIT_ATTENTION:
+		if (sense_hdr->asc == 0x29 && sense_hdr->ascq == 0x00)
+			/* Unit Attention Code. This is the first IO
+			 * to the new path, so just retry. */
+			return NEEDS_RETRY;
+		break;
+	}
+
+	/* success just means we do not care what scsi-ml does */
+	return SUCCESS;
+}
+
+/* TODO do we need to get some other device info so we can set
+ * what type of command we want to do?????
+ */
+static int clariion_setup(struct scsi_target *stgt,
+			  const unsigned char *vendor,
+			  const unsigned char *model)
+{
+	struct clariion_target *ctgt;
+
+	if (strcmp(vendor, "something") || strcmp(model, "something else"))
+		return SCSI_DRIVER_NO_MATCH;
+
+	ctgt = kzalloc(sizeof(*ctgt), GFP_KERNEL);
+	if (!ctgt)
+		return SCSI_DRIVER_SETUP_FAILED;
+	stgt->targetdata = ctgt;
+	ctgt->hr = 0;
+	ctgt->short_trespass = 0;
+
+	return SCSI_DRIVER_SETUP_SUCCESS;
+}
+
+static void clariion_destroy(struct scsi_target *stgt)
+{
+	struct clariion_target *ctgt = stgt->targetdata;
+	kfree(ctgt);
+}
+
+static struct scsi_target_template clariion_template = {
+	.name		= "EMC CLARiiON target driver",
+	.module		= THIS_MODULE,	
+	.check_sense	= clariion_check_sense,
+	.activate	= clariion_activate,
+	.setup		= clariion_setup,
+	.destroy	= clariion_destroy,
+};
+
+static int __init clariion_init(void)
+{
+	return scsi_register_target_template(&clariion_template);
+}
+
+static void __exit clariion_exit(void)
+{
+	scsi_unregister_target_template(&clariion_template);
+}
+
+module_init(clariion_init);
+module_exit(clariion_exit);
+
+MODULE_DESCRIPTION("EMC CX/AX/FC-family target driver");
+MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu");
+MODULE_LICENSE("GPL");
diff -Naurp linux-2.6.18-rc2/drivers/scsi/scsi_error.c linux-2.6.18-rc2.work/drivers/scsi/scsi_error.c
--- linux-2.6.18-rc2/drivers/scsi/scsi_error.c	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/drivers/scsi/scsi_error.c	2006-07-21 07:01:47.000000000 -0400
@@ -289,6 +289,7 @@ static inline void scsi_eh_prt_fail_stat
  **/
 static int scsi_check_sense(struct scsi_cmnd *scmd)
 {
+	struct scsi_target *starget = scsi_target(scmd->device);
 	struct scsi_sense_hdr sshdr;
 
 	if (! scsi_command_normalize_sense(scmd, &sshdr))
@@ -297,6 +298,12 @@ static int scsi_check_sense(struct scsi_
 	if (scsi_sense_is_deferred(&sshdr))
 		return NEEDS_RETRY;
 
+	if (starget->stargett && starget->stargett->check_sense) {
+		int rc = starget->stargett->check_sense(&sshdr);
+		if (rc)
+			return rc;
+	}
+
 	/*
 	 * Previous logic looked for FILEMARK, EOM or ILI which are
 	 * mainly associated with tapes and returned SUCCESS.
diff -Naurp linux-2.6.18-rc2/drivers/scsi/scsi_scan.c linux-2.6.18-rc2.work/drivers/scsi/scsi_scan.c
--- linux-2.6.18-rc2/drivers/scsi/scsi_scan.c	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/drivers/scsi/scsi_scan.c	2006-07-21 07:06:30.000000000 -0400
@@ -288,6 +288,8 @@ static void scsi_target_dev_release(stru
 	struct device *parent = dev->parent;
 	struct scsi_target *starget = to_scsi_target(dev);
 
+	if (starget->stargett && starget->stargett->destroy)
+		starget->stargett->destroy(starget);
 	kfree(starget);
 	put_device(parent);
 }
@@ -911,6 +913,8 @@ static int scsi_probe_and_add_lun(struct
 	/*
 	 * result contains valid SCSI INQUIRY data.
 	 */
+	scsi_bind_target(starget, sdev->vendor, sdev->model);
+
 	if (((result[0] >> 5) == 3) && !(bflags & BLIST_ATTACH_PQ3)) {
 		/*
 		 * For a Peripheral qualifier 3 (011b), the SCSI
diff -Naurp linux-2.6.18-rc2/include/scsi/scsi_device.h linux-2.6.18-rc2.work/include/scsi/scsi_device.h
--- linux-2.6.18-rc2/include/scsi/scsi_device.h	2006-07-15 17:53:08.000000000 -0400
+++ linux-2.6.18-rc2.work/include/scsi/scsi_device.h	2006-07-21 06:22:29.000000000 -0400
@@ -8,6 +8,8 @@
 #include <asm/atomic.h>
 
 struct request_queue;
+struct request;
+struct scsi_target;
 struct scsi_cmnd;
 struct scsi_lun;
 struct scsi_sense_hdr;
@@ -161,6 +163,27 @@ enum scsi_target_state {
 	STARGET_DEL,
 };
 
+enum {
+	SCSI_DRIVER_NO_MATCH,
+	SCSI_DRIVER_SETUP_FAILED,
+	SCSI_DRIVER_SETUP_SUCCESS,
+};
+
+struct scsi_target_template {
+	struct module *module;
+	const char *name;
+	unsigned targetdata_size;
+
+	int (* check_sense)(struct scsi_sense_hdr *);
+	int (* activate)(struct scsi_device *, struct request *);
+	int (* setup)(struct scsi_target *, const unsigned char *vendor,
+		     const unsigned char *model);
+	void (* destroy)(struct scsi_target *);
+
+	/* scsi ml private data */
+	struct list_head list;
+};
+
 /*
  * scsi_target: representation of a scsi target, for now, this is only
  * used for single_lun devices. If no one has active IO to the target,
@@ -178,8 +201,11 @@ struct scsi_target {
 	unsigned int		create:1; /* signal that it needs to be added */
 	unsigned int		pdt_1f_for_no_lun;	/* PDT = 0x1f */
 						/* means no lun present */
-
 	char			scsi_level;
+
+	struct scsi_target_template *stargett;
+	void			*targetdata; /* for the target driver */
+
 	struct execute_work	ew;
 	enum scsi_target_state	state;
 	void 			*hostdata; /* available to low-level driver */
@@ -284,6 +310,10 @@ extern void int_to_scsilun(unsigned int,
 extern const char *scsi_device_state_name(enum scsi_device_state);
 extern int scsi_is_sdev_device(const struct device *);
 extern int scsi_is_target_device(const struct device *);
+extern void scsi_bind_target(struct scsi_target *, const unsigned char *,
+			     const unsigned char *);
+extern void scsi_unregister_target_template(struct scsi_target_template *);
+extern int scsi_register_target_template(struct scsi_target_template *);
 extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 			int data_direction, void *buffer, unsigned bufflen,
 			unsigned char *sense, int timeout, int retries,

Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Hannes Reinecke]

Am Fr 21.07.2006 13:20 schrieb Mike Christie <michaelc@cs.wisc.edu>:

> Currently dm-multipath has what it calls hw_handlers that contain hw
> details and currently those handlers can be used to failover storage
> works, LSI/Engenio RDAC mode, and EMC Clariion boxes. This code is a
> little strange in dm-multipath and they are doing scsi no good (think
> about problems initializing scsi disks/paths when they are in a
> passive
> path that requires explicit/manual activation and we retry over and
> over
> and over).
> 
Yes, good idea. And it would even solve the problem of getting all this
weird
error messages during booting as now these handlers are independently of
of
the multipathing configuration and we're now able to handle weird path 
configurations natively.

> The patch below begins to push the scsi hw handler code down to the
> scsi
> layer. I only began to covert dm-emc.c and it only hooks in at the
> sense
> decoding in scsi_error.c. I wanted to make sure I was going about the
> module loading and binding correctly. With a new target bus we could
> do
> some driver model stuff instead, but I was not sure if that was
> appropriate for this?
> 
Why don't we use scsi_devinfo for this?
We have to have some sort of device table anyway as these handlers are
far from being generic, so any sense code which triggers action on one
device might be perfectly ok for others.

Easiest way would be to have one BLIST flag for each hardware handler
and merge the respective hardware handler into that target if the
blacklist entry matches.

> Next up would be to get Jens's cmd type tree and work on the message
> passing code.
> 
> And this patch currently does not work since I do not have a Clariion
> box and I do not know what to match for the {vendor, model} tuple. I
> think it was something like DCG or DGC and the model had different
> RAID
> strings depending on how it was setup and could have some other string
> if it did not use RAID. If you have a Clariion or you work for EMC and
> happen to know this info could you pass those strings to me?
> 
IIRC this is

'DGC' 'DISK'
'DGC' 'RAID 10'
'DGC' 'RAID 5'

Hrmph. There is one bit which doesn't quite work out.
While the hardware handler knows how to handle error codes and how to
switch
paths for a specific device, it doesn't know _when_ to switch it.
I don't think it's a clever idea to switch paths whenever you encounter
an
passive path. Seems like you could do a nice ping-pong that way ...

Cheers,

Hannes

Re: Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Christie]

Hannes Reinecke wrote:
>> The patch below begins to push the scsi hw handler code down to the
>> scsi
>> layer. I only began to covert dm-emc.c and it only hooks in at the
>> sense
>> decoding in scsi_error.c. I wanted to make sure I was going about the
>> module loading and binding correctly. With a new target bus we could
>> do
>> some driver model stuff instead, but I was not sure if that was
>> appropriate for this?
>>
> Why don't we use scsi_devinfo for this?

I was adding my fields when I noticed this comment:


 * Do not add to this list, use the command line or proc interface to add
 * to the scsi_dev_info_list. This table will eventually go away.


> We have to have some sort of device table anyway as these handlers are
> far from being generic, so any sense code which triggers action on one
> device might be perfectly ok for others.

When I was looking for the history of that commet, I thought I read that
we are supposed to be moving to some userspace approach that pushes that
info down via some magic interface.

> 
> Easiest way would be to have one BLIST flag for each hardware handler
> and merge the respective hardware handler into that target if the
> blacklist entry matches.
> 
>> Next up would be to get Jens's cmd type tree and work on the message
>> passing code.
>>
>> And this patch currently does not work since I do not have a Clariion
>> box and I do not know what to match for the {vendor, model} tuple. I
>> think it was something like DCG or DGC and the model had different
>> RAID
>> strings depending on how it was setup and could have some other string
>> if it did not use RAID. If you have a Clariion or you work for EMC and
>> happen to know this info could you pass those strings to me?
>>
> IIRC this is
> 
> 'DGC' 'DISK'
> 'DGC' 'RAID 10'
> 'DGC' 'RAID 5'

Thanks.

> 
> Hrmph. There is one bit which doesn't quite work out.
> While the hardware handler knows how to handle error codes and how to
> switch
> paths for a specific device, it doesn't know _when_ to switch it.
> I don't think it's a clever idea to switch paths whenever you encounter
> an
> passive path. Seems like you could do a nice ping-pong that way ...


This is what we talked about at the bof. It is what the message passing
cmd type stuff is for. The reason I could not do it yet is that I could
not get Jens's git tree from my hotel.

dm-multipath still decide when to switch like before, but scsi hw
handlers will execute the low level details now.

Re: [dm-devel] Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Christie]

Mike Christie wrote:
> Hannes Reinecke wrote:
>>> The patch below begins to push the scsi hw handler code down to the
>>> scsi
>>> layer. I only began to covert dm-emc.c and it only hooks in at the
>>> sense
>>> decoding in scsi_error.c. I wanted to make sure I was going about the
>>> module loading and binding correctly. With a new target bus we could
>>> do
>>> some driver model stuff instead, but I was not sure if that was
>>> appropriate for this?
>>>
>> Why don't we use scsi_devinfo for this?
> 
> I was adding my fields when I noticed this comment:
> 
> 
>  * Do not add to this list, use the command line or proc interface to add
>  * to the scsi_dev_info_list. This table will eventually go away.
> 
> 
>> We have to have some sort of device table anyway as these handlers are
>> far from being generic, so any sense code which triggers action on one
>> device might be perfectly ok for others.
> 
> When I was looking for the history of that commet, I thought I read that
> we are supposed to be moving to some userspace approach that pushes that
> info down via some magic interface.
> 

I added this comment at the wrong place. I meant to say I thought we are
supposed to be moving away from the kernel devinfo list to some
userspace one that gets sent down via the module_param or some new magic
interface.

Re: Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Hannes Reinecke]

Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:

> Mike Christie wrote:
> > Hannes Reinecke wrote:
> >>> The patch below begins to push the scsi hw handler code down to
> >>> the
> >>> scsi
> >>> layer. I only began to covert dm-emc.c and it only hooks in at the
> >>> sense
> >>> decoding in scsi_error.c. I wanted to make sure I was going about
> >>> the
> >>> module loading and binding correctly. With a new target bus we
> >>> could
> >>> do
> >>> some driver model stuff instead, but I was not sure if that was
> >>> appropriate for this?
> >>>
> >> Why don't we use scsi_devinfo for this?
> > 
> > I was adding my fields when I noticed this comment:
> > 
> > 
> > * Do not add to this list, use the command line or proc interface to
> >  add
> >  * to the scsi_dev_info_list. This table will eventually go away.
> > 
> > 
> >> We have to have some sort of device table anyway as these handlers
> >> are
> >> far from being generic, so any sense code which triggers action on
> >> one
> >> device might be perfectly ok for others.
> > 
> > When I was looking for the history of that commet, I thought I read
> > that
> > we are supposed to be moving to some userspace approach that pushes
> > that
> > info down via some magic interface.
> > 
> 
> I added this comment at the wrong place. I meant to say I thought we
> are
> supposed to be moving away from the kernel devinfo list to some
> userspace one that gets sent down via the module_param or some new
> magic
> interface.

Or so they claim. I seem to remember some discussion about it; the net
result was the scsi_devinfo will stay with us for the time being.

Otherwise you'll end up having to configure your kernel / module during
startup. With parameters which are static anyway. Can't say I like it.
And the tricky bit is that these information has to be present prior
to any initialisation, so you basically have to feed it during
modprobe time. Not really clever.

But we can always ask James B. :-)

Cheers,

Hannes

Re: [dm-devel] Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Christie]

Hannes Reinecke wrote:
> Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:
> 
>> Mike Christie wrote:
>>> Hannes Reinecke wrote:
>>>>> The patch below begins to push the scsi hw handler code down to
>>>>> the
>>>>> scsi
>>>>> layer. I only began to covert dm-emc.c and it only hooks in at the
>>>>> sense
>>>>> decoding in scsi_error.c. I wanted to make sure I was going about
>>>>> the
>>>>> module loading and binding correctly. With a new target bus we
>>>>> could
>>>>> do
>>>>> some driver model stuff instead, but I was not sure if that was
>>>>> appropriate for this?
>>>>>
>>>> Why don't we use scsi_devinfo for this?
>>> I was adding my fields when I noticed this comment:
>>>
>>>
>>> * Do not add to this list, use the command line or proc interface to
>>>  add
>>>  * to the scsi_dev_info_list. This table will eventually go away.
>>>
>>>
>>>> We have to have some sort of device table anyway as these handlers
>>>> are
>>>> far from being generic, so any sense code which triggers action on
>>>> one
>>>> device might be perfectly ok for others.
>>> When I was looking for the history of that commet, I thought I read
>>> that
>>> we are supposed to be moving to some userspace approach that pushes
>>> that
>>> info down via some magic interface.
>>>
>> I added this comment at the wrong place. I meant to say I thought we
>> are
>> supposed to be moving away from the kernel devinfo list to some
>> userspace one that gets sent down via the module_param or some new
>> magic
>> interface.
> 
> Or so they claim. I seem to remember some discussion about it; the net
> result was the scsi_devinfo will stay with us for the time being.
> 
> Otherwise you'll end up having to configure your kernel / module during
> startup. With parameters which are static anyway. Can't say I like it.
> And the tricky bit is that these information has to be present prior
> to any initialisation, so you basically have to feed it during
> modprobe time. Not really clever.
> 

He He fun :)

Sticking what we need in devinfo is a lot easier. And I think it makes
sense since the devices info we want to bind with is in there already.
If nobody says anything, I will send the next version of the path with
devinfo integration.

Re: [dm-devel] Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Anderson]

Mike Christie <michaelc@cs.wisc.edu> wrote:
> Hannes Reinecke wrote:
> > Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:
> > 
> >> Mike Christie wrote:
> >>> Hannes Reinecke wrote:
> >>>>> The patch below begins to push the scsi hw handler code down to
> >>>>> the
> >>>>> scsi
> >>>>> layer. I only began to covert dm-emc.c and it only hooks in at the
> >>>>> sense
> >>>>> decoding in scsi_error.c. I wanted to make sure I was going about
> >>>>> the
> >>>>> module loading and binding correctly. With a new target bus we
> >>>>> could
> >>>>> do
> >>>>> some driver model stuff instead, but I was not sure if that was
> >>>>> appropriate for this?
> >>>>>
> >>>> Why don't we use scsi_devinfo for this?
> >>> I was adding my fields when I noticed this comment:
> >>>
> >>>
> >>> * Do not add to this list, use the command line or proc interface to
> >>>  add
> >>>  * to the scsi_dev_info_list. This table will eventually go away.
> >>>
> >>>
> >>>> We have to have some sort of device table anyway as these handlers
> >>>> are
> >>>> far from being generic, so any sense code which triggers action on
> >>>> one
> >>>> device might be perfectly ok for others.
> >>> When I was looking for the history of that commet, I thought I read
> >>> that
> >>> we are supposed to be moving to some userspace approach that pushes
> >>> that
> >>> info down via some magic interface.
> >>>
> >> I added this comment at the wrong place. I meant to say I thought we
> >> are
> >> supposed to be moving away from the kernel devinfo list to some
> >> userspace one that gets sent down via the module_param or some new
> >> magic
> >> interface.
> > 
> > Or so they claim. I seem to remember some discussion about it; the net
> > result was the scsi_devinfo will stay with us for the time being.
> > 
> > Otherwise you'll end up having to configure your kernel / module during
> > startup. With parameters which are static anyway. Can't say I like it.
> > And the tricky bit is that these information has to be present prior
> > to any initialisation, so you basically have to feed it during
> > modprobe time. Not really clever.
> > 
> 
> He He fun :)
> 
> Sticking what we need in devinfo is a lot easier. And I think it makes
> sense since the devices info we want to bind with is in there already.
> If nobody says anything, I will send the next version of the path with
> devinfo integration.

I think Patrick added the comment and the interface so he can add the
history. One can use the module or proc interface to pass update devinfo
information in (the sysfs migration never was done). Well it has the
drawback stated above it can address the issue that certain devices can be
supported without a kernel recompile.

Post storage summit I started creating a hardware handler in SCSI, but for
some reason that I do not recall I started working on SDEV state model
change integrated into devinfo. The thought being that devices would come
up in a standby state and then all the varied commands to determine path
state could be executed from user space.

Well it did solve the issue I was trying to address (passive paths
generating errors on startup), it would need user space assistance with all
the plus / minus issues that brings.

-andmike
--
Michael Anderson
andmike@us.ibm.com

 [EXPERIMENTAL] Add standby support to devinfo.


 drivers/scsi/scsi_devinfo.c |    1 +
 drivers/scsi/scsi_priv.h    |    2 +-
 drivers/scsi/scsi_scan.c    |    6 +++++-
 drivers/scsi/scsi_sysfs.c   |    4 ++--
 drivers/scsi/sd.c           |    4 ++--
 include/scsi/scsi_device.h  |    5 +++++
 include/scsi/scsi_devinfo.h |    1 +
 7 files changed, 17 insertions(+), 6 deletions(-)

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_devinfo.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_devinfo.c	2006-06-12 14:25:50.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_devinfo.c	2006-06-14 21:59:14.000000000 -0700
@@ -171,6 +171,7 @@ static struct {
 	{"IBM", "AuSaV1S2", NULL, BLIST_FORCELUN},
 	{"IBM", "ProFibre 4000R", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
 	{"IBM", "2105", NULL, BLIST_RETRY_HWERROR},
+	{"IBM", "1742-900", NULL, BLIST_DFLT_STDBY},
 	{"iomega", "jaz 1GB", "J.86", BLIST_NOTQ | BLIST_NOLUN},
 	{"IOMEGA", "Io20S         *F", NULL, BLIST_KEY},
 	{"INSITE", "Floptical   F*8I", NULL, BLIST_KEY},
Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_priv.h
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_priv.h	2006-06-12 14:25:50.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_priv.h	2006-06-12 15:35:47.000000000 -0700
@@ -101,7 +101,7 @@ extern void scsi_exit_sysctl(void);
 #endif /* CONFIG_SYSCTL */
 
 /* scsi_sysfs.c */
-extern int scsi_sysfs_add_sdev(struct scsi_device *);
+extern int scsi_sysfs_add_sdev(struct scsi_device *, enum scsi_device_state);
 extern int scsi_sysfs_add_host(struct Scsi_Host *);
 extern int scsi_sysfs_register(void);
 extern void scsi_sysfs_unregister(void);
Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_scan.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_scan.c	2006-06-12 14:25:50.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_scan.c	2006-06-12 15:35:15.000000000 -0700
@@ -631,6 +631,8 @@ static int scsi_probe_lun(struct scsi_de
  **/
 static int scsi_add_lun(struct scsi_device *sdev, char *inq_result, int *bflags)
 {
+	enum scsi_device_state state = SDEV_RUNNING;
+
 	/*
 	 * XXX do not save the inquiry, since it can change underneath us,
 	 * save just vendor/model/rev.
@@ -802,7 +804,9 @@ static int scsi_add_lun(struct scsi_devi
 	 * register it and tell the rest of the kernel
 	 * about it.
 	 */
-	if (scsi_sysfs_add_sdev(sdev) != 0)
+	if (*bflags & BLIST_DFLT_STDBY)
+		state = SDEV_STANDBY;
+	if (scsi_sysfs_add_sdev(sdev, state) != 0)
 		return SCSI_SCAN_NO_RESPONSE;
 
 	return SCSI_SCAN_LUN_PRESENT;
Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_sysfs.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_sysfs.c	2006-06-12 14:29:31.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_sysfs.c	2006-06-12 15:38:16.000000000 -0700
@@ -672,11 +672,11 @@ static int attr_add(struct device *dev, 
  * Return value:
  * 	0 on Success / non-zero on Failure
  **/
-int scsi_sysfs_add_sdev(struct scsi_device *sdev)
+int scsi_sysfs_add_sdev(struct scsi_device *sdev, enum scsi_device_state state)
 {
 	int error, i;
 
-	if ((error = scsi_device_set_state(sdev, SDEV_RUNNING)) != 0)
+	if ((error = scsi_device_set_state(sdev, state)) != 0)
 		return error;
 
 	error = device_add(&sdev->sdev_gendev);
Index: aic94xx-sas-2.6-patched/drivers/scsi/sd.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/sd.c	2006-06-12 14:25:50.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/sd.c	2006-06-13 09:28:05.000000000 -0700
@@ -769,7 +769,7 @@ static int sd_sync_cache(struct scsi_dev
 	int retries, res;
 	struct scsi_sense_hdr sshdr;
 
-	if (!scsi_device_online(sdp))
+	if (scsi_device_access_restricted(sdp))
 		return -ENODEV;
 
 
@@ -1537,7 +1537,7 @@ static int sd_revalidate_disk(struct gen
 	 * If the device is offline, don't try and read capacity or any
 	 * of the other niceties.
 	 */
-	if (!scsi_device_online(sdp))
+	if (scsi_device_access_restricted(sdp))
 		goto out;
 
 	buffer = kmalloc(SD_BUF_SIZE, GFP_KERNEL | __GFP_DMA);
Index: aic94xx-sas-2.6-patched/include/scsi/scsi_device.h
===================================================================
--- aic94xx-sas-2.6-patched.orig/include/scsi/scsi_device.h	2006-06-12 14:29:31.000000000 -0700
+++ aic94xx-sas-2.6-patched/include/scsi/scsi_device.h	2006-06-12 16:00:08.000000000 -0700
@@ -322,6 +322,11 @@ static inline int scsi_device_online(str
 	return sdev->sdev_state != SDEV_OFFLINE;
 }
 
+static inline int scsi_device_access_restricted(struct scsi_device *sdev)
+{
+	return sdev->sdev_state != SDEV_RUNNING;
+}
+
 /* accessor functions for the SCSI parameters */
 static inline int scsi_device_sync(struct scsi_device *sdev)
 {
Index: aic94xx-sas-2.6-patched/include/scsi/scsi_devinfo.h
===================================================================
--- aic94xx-sas-2.6-patched.orig/include/scsi/scsi_devinfo.h	2006-06-07 13:17:30.000000000 -0700
+++ aic94xx-sas-2.6-patched/include/scsi/scsi_devinfo.h	2006-06-12 15:13:11.000000000 -0700
@@ -30,4 +30,5 @@
 #define BLIST_RETRY_HWERROR	0x400000 /* retry HARDWARE_ERROR */
 #define BLIST_MAX_512		0x800000 /* maximum 512 sector cdb length */
 #define BLIST_ATTACH_PQ3	0x1000000 /* Scan: Attach to PQ3 devices */
+#define BLIST_DFLT_STDBY	0x2000000 /* Default to standby */
 #endif

[EXPERIMENTAL] Add a new scsi device state of standby

 drivers/scsi/scsi_lib.c    |   14 ++++++++++++++
 drivers/scsi/scsi_sysfs.c  |    1 +
 include/scsi/scsi_device.h |    1 +
 3 files changed, 16 insertions(+)

Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_lib.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_lib.c	2006-06-12 14:29:22.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_lib.c	2006-06-12 14:29:31.000000000 -0700
@@ -2009,6 +2009,7 @@ scsi_device_set_state(struct scsi_device
 		case SDEV_OFFLINE:
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_STANDBY:
 			break;
 		default:
 			goto illegal;
@@ -2031,6 +2032,7 @@ scsi_device_set_state(struct scsi_device
 		case SDEV_RUNNING:
 		case SDEV_QUIESCE:
 		case SDEV_BLOCK:
+		case SDEV_STANDBY:
 			break;
 		default:
 			goto illegal;
@@ -2053,6 +2055,7 @@ scsi_device_set_state(struct scsi_device
 		case SDEV_RUNNING:
 		case SDEV_OFFLINE:
 		case SDEV_BLOCK:
+		case SDEV_STANDBY:
 			break;
 		default:
 			goto illegal;
@@ -2068,6 +2071,17 @@ scsi_device_set_state(struct scsi_device
 		}
 		break;
 
+	case SDEV_STANDBY:
+		switch (oldstate) {
+		case SDEV_CREATED:
+		case SDEV_RUNNING:
+		case SDEV_OFFLINE:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
 	}
 	sdev->sdev_state = state;
 	return 0;
Index: aic94xx-sas-2.6-patched/drivers/scsi/scsi_sysfs.c
===================================================================
--- aic94xx-sas-2.6-patched.orig/drivers/scsi/scsi_sysfs.c	2006-06-12 14:25:50.000000000 -0700
+++ aic94xx-sas-2.6-patched/drivers/scsi/scsi_sysfs.c	2006-06-12 14:29:31.000000000 -0700
@@ -32,6 +32,7 @@ static const struct {
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
 	{ SDEV_BLOCK,	"blocked" },
+	{ SDEV_STANDBY,	"standby" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
Index: aic94xx-sas-2.6-patched/include/scsi/scsi_device.h
===================================================================
--- aic94xx-sas-2.6-patched.orig/include/scsi/scsi_device.h	2006-06-12 14:27:16.000000000 -0700
+++ aic94xx-sas-2.6-patched/include/scsi/scsi_device.h	2006-06-12 14:29:31.000000000 -0700
@@ -43,6 +43,7 @@ enum scsi_device_state {
 	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
 				 * commands from user or midlayer should be issued
 				 * to the scsi lld. */
+	SDEV_STANDBY,		/* No media access commands to the device */
 };
 
 struct scsi_device {

Re: Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Christie]

Mike Anderson wrote:
> Mike Christie <michaelc@cs.wisc.edu> wrote:
>> Hannes Reinecke wrote:
>>> Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:
>>>
>>>> Mike Christie wrote:
>>>>> Hannes Reinecke wrote:
>>>>>>> The patch below begins to push the scsi hw handler code down to
>>>>>>> the
>>>>>>> scsi
>>>>>>> layer. I only began to covert dm-emc.c and it only hooks in at the
>>>>>>> sense
>>>>>>> decoding in scsi_error.c. I wanted to make sure I was going about
>>>>>>> the
>>>>>>> module loading and binding correctly. With a new target bus we
>>>>>>> could
>>>>>>> do
>>>>>>> some driver model stuff instead, but I was not sure if that was
>>>>>>> appropriate for this?
>>>>>>>
>>>>>> Why don't we use scsi_devinfo for this?
>>>>> I was adding my fields when I noticed this comment:
>>>>>
>>>>>
>>>>> * Do not add to this list, use the command line or proc interface to
>>>>>  add
>>>>>  * to the scsi_dev_info_list. This table will eventually go away.
>>>>>
>>>>>
>>>>>> We have to have some sort of device table anyway as these handlers
>>>>>> are
>>>>>> far from being generic, so any sense code which triggers action on
>>>>>> one
>>>>>> device might be perfectly ok for others.
>>>>> When I was looking for the history of that commet, I thought I read
>>>>> that
>>>>> we are supposed to be moving to some userspace approach that pushes
>>>>> that
>>>>> info down via some magic interface.
>>>>>
>>>> I added this comment at the wrong place. I meant to say I thought we
>>>> are
>>>> supposed to be moving away from the kernel devinfo list to some
>>>> userspace one that gets sent down via the module_param or some new
>>>> magic
>>>> interface.
>>> Or so they claim. I seem to remember some discussion about it; the net
>>> result was the scsi_devinfo will stay with us for the time being.
>>>
>>> Otherwise you'll end up having to configure your kernel / module during
>>> startup. With parameters which are static anyway. Can't say I like it.
>>> And the tricky bit is that these information has to be present prior
>>> to any initialisation, so you basically have to feed it during
>>> modprobe time. Not really clever.
>>>
>> He He fun :)
>>
>> Sticking what we need in devinfo is a lot easier. And I think it makes
>> sense since the devices info we want to bind with is in there already.
>> If nobody says anything, I will send the next version of the path with
>> devinfo integration.
> 
> I think Patrick added the comment and the interface so he can add the
> history. One can use the module or proc interface to pass update devinfo
> information in (the sysfs migration never was done). Well it has the
> drawback stated above it can address the issue that certain devices can be
> supported without a kernel recompile.
> 
> Post storage summit I started creating a hardware handler in SCSI, but for
> some reason that I do not recall I started working on SDEV state model
> change integrated into devinfo. The thought being that devices would come
> up in a standby state and then all the varied commands to determine path
> state could be executed from user space.
> 
> Well it did solve the issue I was trying to address (passive paths
> generating errors on startup), it would need user space assistance with all
> the plus / minus issues that brings.
> 

Yeah, I am fine with all that and have no complaints. How can I complain
when I always pop up with the do it in userspace comment :)

My main focus for this was not really the startup. That was a side
benefit to having the sense and that info in the kernel for failover
requests and IO that is executed when the path is passive or active.

So today, we could use your userspace start up for bring up but we still
need something to do failover and process those results and possibly
process the results of other IO (similar to the emc first IO to the path
case in the patch). For the latter, I think the EMC check_sense tests I
am doing in my patch are already handled by other checks in scsi-ml so
the sense stuff in scsi_emc_clariion.c is not exactly needed. So maybe
we do not need hw handlers for that. As I have said before, I keep
running into this problem where vendors say yes we need it but we do not
have many good examples.

But if scsi-ml's sense decoding is ok by default and we do not need to
override it, this just leaves explicit/manual failover. If we throw that
to userspace along with your strart up code then I guess we do not need
kernel hw handlers at all.

Did you guys end up doing a hw handler that runs in userspace for xdr or
something? Has it worked out? Is there some common code that can be
reused today by any chance? Or is this one of those cases where we are
going too far with putting stuff in userspace do you think?

Re: Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Hannes Reinecke]

Am Fr 21.07.2006 19:00 schrieb Mike Christie <michaelc@cs.wisc.edu>:

> Mike Anderson wrote:
> > Mike Christie <michaelc@cs.wisc.edu> wrote:
> >> He He fun :)
> >>
> >> Sticking what we need in devinfo is a lot easier. And I think it
> >> makes
> >> sense since the devices info we want to bind with is in there
> >> already.
> >> If nobody says anything, I will send the next version of the path
> >> with
> >> devinfo integration.
> > 
> > I think Patrick added the comment and the interface so he can add
> > the
> > history. One can use the module or proc interface to pass update
> > devinfo
> > information in (the sysfs migration never was done). Well it has the
> > drawback stated above it can address the issue that certain devices
> > can be
> > supported without a kernel recompile.
> > 
> > Post storage summit I started creating a hardware handler in SCSI,
> > but for
> > some reason that I do not recall I started working on SDEV state
> > model
> > change integrated into devinfo. The thought being that devices would
> > come
> > up in a standby state and then all the varied commands to determine
> > path
> > state could be executed from user space.
> > 
> > Well it did solve the issue I was trying to address (passive paths
> > generating errors on startup), it would need user space assistance
> > with all
> > the plus / minus issues that brings.
> > 
> 
> Yeah, I am fine with all that and have no complaints. How can I
> complain
> when I always pop up with the do it in userspace comment :)
> 
Actually I talked to hch; he was fine with modifying / updating
scsi_devinfo.
He doesn't see any way of getting rid of scsi_devinfo.c in the near
future,
too. So go for it.

> My main focus for this was not really the startup. That was a side
> benefit to having the sense and that info in the kernel for failover
> requests and IO that is executed when the path is passive or active.
> 
> So today, we could use your userspace start up for bring up but we
> still
> need something to do failover and process those results and possibly
> process the results of other IO (similar to the emc first IO to the
> path
> case in the patch). For the latter, I think the EMC check_sense tests
> I
> am doing in my patch are already handled by other checks in scsi-ml so
> the sense stuff in scsi_emc_clariion.c is not exactly needed. So maybe
> we do not need hw handlers for that. As I have said before, I keep
> running into this problem where vendors say yes we need it but we do
> not
> have many good examples.
> 
> But if scsi-ml's sense decoding is ok by default and we do not need to
> override it, this just leaves explicit/manual failover. If we throw
> that
> to userspace along with your strart up code then I guess we do not
> need
> kernel hw handlers at all.
> 
> Did you guys end up doing a hw handler that runs in userspace for xdr
> or
> something? Has it worked out? Is there some common code that can be
> reused today by any chance? Or is this one of those cases where we are
> going too far with putting stuff in userspace do you think?
> 
Argl. No, I wouldn't put that one in userspace. The thing I really liked
about this approach is that the hardware handler are indeed decoupled
from
the multipath tools, so they would even work without multipathing
active.
Eg during boot. Which would rid us of the boot errors.

Cheers,

Hannes

Re: [dm-devel] Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Mike Anderson]

Mike Christie <michaelc@cs.wisc.edu> wrote:
> Mike Anderson wrote:
> > Mike Christie <michaelc@cs.wisc.edu> wrote:
> >> Hannes Reinecke wrote:
> >>> Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:
> >>>
> >>>> Mike Christie wrote:
> >>>>> Hannes Reinecke wrote:
> >>>>>>> The patch below begins to push the scsi hw handler code down to
> >>>>>>> the
> >>>>>>> scsi
> >>>>>>> layer. I only began to covert dm-emc.c and it only hooks in at the
> >>>>>>> sense
> >>>>>>> decoding in scsi_error.c. I wanted to make sure I was going about
> >>>>>>> the
> >>>>>>> module loading and binding correctly. With a new target bus we
> >>>>>>> could
> >>>>>>> do
> >>>>>>> some driver model stuff instead, but I was not sure if that was
> >>>>>>> appropriate for this?
> >>>>>>>
> >>>>>> Why don't we use scsi_devinfo for this?
> >>>>> I was adding my fields when I noticed this comment:
> >>>>>
> >>>>>
> >>>>> * Do not add to this list, use the command line or proc interface to
> >>>>>  add
> >>>>>  * to the scsi_dev_info_list. This table will eventually go away.
> >>>>>
> >>>>>
> >>>>>> We have to have some sort of device table anyway as these handlers
> >>>>>> are
> >>>>>> far from being generic, so any sense code which triggers action on
> >>>>>> one
> >>>>>> device might be perfectly ok for others.
> >>>>> When I was looking for the history of that commet, I thought I read
> >>>>> that
> >>>>> we are supposed to be moving to some userspace approach that pushes
> >>>>> that
> >>>>> info down via some magic interface.
> >>>>>
> >>>> I added this comment at the wrong place. I meant to say I thought we
> >>>> are
> >>>> supposed to be moving away from the kernel devinfo list to some
> >>>> userspace one that gets sent down via the module_param or some new
> >>>> magic
> >>>> interface.
> >>> Or so they claim. I seem to remember some discussion about it; the net
> >>> result was the scsi_devinfo will stay with us for the time being.
> >>>
> >>> Otherwise you'll end up having to configure your kernel / module during
> >>> startup. With parameters which are static anyway. Can't say I like it.
> >>> And the tricky bit is that these information has to be present prior
> >>> to any initialisation, so you basically have to feed it during
> >>> modprobe time. Not really clever.
> >>>
> >> He He fun :)
> >>
> >> Sticking what we need in devinfo is a lot easier. And I think it makes
> >> sense since the devices info we want to bind with is in there already.
> >> If nobody says anything, I will send the next version of the path with
> >> devinfo integration.
> > 
> > I think Patrick added the comment and the interface so he can add the
> > history. One can use the module or proc interface to pass update devinfo
> > information in (the sysfs migration never was done). Well it has the
> > drawback stated above it can address the issue that certain devices can be
> > supported without a kernel recompile.
> > 
> > Post storage summit I started creating a hardware handler in SCSI, but for
> > some reason that I do not recall I started working on SDEV state model
> > change integrated into devinfo. The thought being that devices would come
> > up in a standby state and then all the varied commands to determine path
> > state could be executed from user space.
> > 
> > Well it did solve the issue I was trying to address (passive paths
> > generating errors on startup), it would need user space assistance with all
> > the plus / minus issues that brings.
> > 
> 
> Yeah, I am fine with all that and have no complaints. How can I complain
> when I always pop up with the do it in userspace comment :)
> 
> My main focus for this was not really the startup. That was a side
> benefit to having the sense and that info in the kernel for failover
> requests and IO that is executed when the path is passive or active.
> 
> So today, we could use your userspace start up for bring up but we still
> need something to do failover and process those results and possibly
> process the results of other IO (similar to the emc first IO to the path
> case in the patch). For the latter, I think the EMC check_sense tests I
> am doing in my patch are already handled by other checks in scsi-ml so
> the sense stuff in scsi_emc_clariion.c is not exactly needed. So maybe
> we do not need hw handlers for that. As I have said before, I keep
> running into this problem where vendors say yes we need it but we do not
> have many good examples.


Well I would not say that the scsi-ml sense / decide_disposition is doing
exactly what we want. One thing I ran into with the PPRC units was that an
error was generated when the PPRC connection between the units was broken.
The error generated was a hardware error which we retry unless the
fastfail flag was set (which it was). This error was then bubbled up to dm
but there was not much it could do with it as it was blind to the context
of the error.

> 
> But if scsi-ml's sense decoding is ok by default and we do not need to
> override it, this just leaves explicit/manual failover. If we throw that
> to userspace along with your strart up code then I guess we do not need
> kernel hw handlers at all.
> 
> Did you guys end up doing a hw handler that runs in userspace for xdr or
> something? Has it worked out? Is there some common code that can be
> reused today by any chance? Or is this one of those cases where we are
> going too far with putting stuff in userspace do you think?

I do not think this is model to repeat for general support as the
environment was unique due the control for the PPRC failover was out of
band and cluster like decisions where being made to decide if to failover
at all. 

My previous attempt was adding a ops table to the scsi_dev_info_list, but
I dropped this for some reason. At the time I was more focused on having a
capability where paths to devices inside SCSI core could be in different
states to reduce the boot up issue with active / passive paths.

-andmike
--
Michael Anderson
andmike@us.ibm.com

Re: [dm-devel] Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Patrick Mansfield]

[-- Attachment #1: Type: text/plain, Size: 4248 bytes --]

On Fri, Jul 21, 2006 at 08:16:56AM -0700, Mike Anderson wrote:
> Mike Christie <michaelc@cs.wisc.edu> wrote:
> > Hannes Reinecke wrote:
> > > Am Fr 21.07.2006 13:55 schrieb Mike Christie <michaelc@cs.wisc.edu>:
> > > 
> > >> Mike Christie wrote:
> > >>> Hannes Reinecke wrote:

> > >>> I was adding my fields when I noticed this comment:
> > >>>
> > >>>
> > >>> * Do not add to this list, use the command line or proc interface to
> > >>>  add
> > >>>  * to the scsi_dev_info_list. This table will eventually go away.
> > >>>
> > >>>
> > >>>> We have to have some sort of device table anyway as these handlers
> > >>>> are
> > >>>> far from being generic, so any sense code which triggers action on
> > >>>> one
> > >>>> device might be perfectly ok for others.
> > >>> When I was looking for the history of that commet, I thought I read
> > >>> that
> > >>> we are supposed to be moving to some userspace approach that pushes
> > >>> that
> > >>> info down via some magic interface.
> > >>>
> > >> I added this comment at the wrong place. I meant to say I thought we
> > >> are
> > >> supposed to be moving away from the kernel devinfo list to some
> > >> userspace one that gets sent down via the module_param or some new
> > >> magic
> > >> interface.
> > > 
> > > Or so they claim. I seem to remember some discussion about it; the net
> > > result was the scsi_devinfo will stay with us for the time being.
> > > 
> > > Otherwise you'll end up having to configure your kernel / module during
> > > startup. With parameters which are static anyway. Can't say I like it.
> > > And the tricky bit is that these information has to be present prior
> > > to any initialisation, so you basically have to feed it during
> > > modprobe time. Not really clever.

I would think distros would like the user space method so they would not
have to release a new kernel to add items to the devinfo / blacklist.

> > He He fun :)
> > 
> > Sticking what we need in devinfo is a lot easier. And I think it makes
> > sense since the devices info we want to bind with is in there already.
> > If nobody says anything, I will send the next version of the path with
> > devinfo integration.
> 
> I think Patrick added the comment and the interface so he can add the
> history. One can use the module or proc interface to pass update devinfo
> information in (the sysfs migration never was done). Well it has the
> drawback stated above it can address the issue that certain devices can be
> supported without a kernel recompile.

I just thought that we should not be putting black lists in the kernel,
and AFAIR thought everyone wanted this type of data in user space (even
back then). The comment in the code has never been enforced. I never did
think of a decent (space efficient) method to access a link list or array
in sysfs.

I didn't look close at the other patches in this thread.

I am attaching some simple user space code I wrote some time ago, feel
free to use it. There is a script version and C version. I don't know
devinfo-current is really current :) 

There should also be a corresponding kernel patch (if distros wanted to
use this) to allow disabling the in-kernel devinfo table.

The script (bload.sh) needs indirection (pointer like reference, this:
${!i}), I don't think nash supports indirection, otherwise the script is
very simple.

Like Hannes said, you need some /etc/modprobe.conf juju to load scsi_mod,
run the attached script or program, and only then allow other scsi modules
to load. Plus similar code in the initrd/ramfs. I actually tried out the
modprobe.conf change at some time, I don't recall the exact modprobe
lines, AFAIR it needed to use the --ignore-install option.

> Post storage summit I started creating a hardware handler in SCSI, but for
> some reason that I do not recall I started working on SDEV state model
> change integrated into devinfo. The thought being that devices would come
> up in a standby state and then all the varied commands to determine path
> state could be executed from user space.
> 
> Well it did solve the issue I was trying to address (passive paths
> generating errors on startup), it would need user space assistance with all
> the plus / minus issues that brings.

-- Patrick Mansfield

[-- Attachment #2: scsi_blacklist.tar.gz --]
[-- Type: application/x-gzip, Size: 7390 bytes --]

Re: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Philip R. Auld]

Rumor has it that on Fri, Jul 21, 2006 at 01:41:28PM +0200 Hannes Reinecke said:
> IIRC this is
> 
> 'DGC' 'DISK'
> 'DGC' 'RAID 10'
> 'DGC' 'RAID 5'
> 
> Hrmph. There is one bit which doesn't quite work out.
> While the hardware handler knows how to handle error codes and how to
> switch
> paths for a specific device, it doesn't know _when_ to switch it.
> I don't think it's a clever idea to switch paths whenever you encounter
> an
> passive path. Seems like you could do a nice ping-pong that way ...


Yes. I agree that having the handlers in SCSI is a better place for
them. However, the architectural decision to have multipath knowledge
at a higher level makes that harder. Some sort of interface into 
the scsi layer handlers from the dm/block layer would be more needed. 
That way dm or some piece that has the knowledge of the multipath 
situation can trigger the failover. 

There will still be a need to bubble up information from the decoded 
error codes so higher layers can make the correct decision. 
Such an interface would need to be in both directions.



Cheers,
Phil


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

-- 
Philip R. Auld, Ph.D.  	        	       Egenera, Inc.    
Software Architect                            165 Forest St.
(508) 858-2628                            Marlboro, MA 01752

RE: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Edward Goggin]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Friday, July 21, 2006 7:41 AM
> To: Mike Christie
> Cc: dm-devel@redhat.com; linux-scsi@vger.kernel.org; agk@redhat.com
> Subject: Re: [PATCH RFC] move scsi parts of dm hw handlers to 
> scsi layer
> 
> Am Fr 21.07.2006 13:20 schrieb Mike Christie <michaelc@cs.wisc.edu>:
> 
> > Currently dm-multipath has what it calls hw_handlers that contain hw
> > details and currently those handlers can be used to failover storage
> > works, LSI/Engenio RDAC mode, and EMC Clariion boxes. This code is a
> > little strange in dm-multipath and they are doing scsi no 
> good (think
> > about problems initializing scsi disks/paths when they are in a
> > passive
> > path that requires explicit/manual activation and we retry over and
> > over
> > and over).
> > 
> Yes, good idea. And it would even solve the problem of 
> getting all this
> weird
> error messages during booting as now these handlers are 
> independently of
> of
> the multipathing configuration and we're now able to handle 
> weird path 
> configurations natively.
> 
> > The patch below begins to push the scsi hw handler code down to the
> > scsi
> > layer. I only began to covert dm-emc.c and it only hooks in at the
> > sense
> > decoding in scsi_error.c. I wanted to make sure I was going 
> about the
> > module loading and binding correctly. With a new target bus we could
> > do
> > some driver model stuff instead, but I was not sure if that was
> > appropriate for this?
> > 
> Why don't we use scsi_devinfo for this?
> We have to have some sort of device table anyway as these handlers are
> far from being generic, so any sense code which triggers action on one
> device might be perfectly ok for others.
> 
> Easiest way would be to have one BLIST flag for each hardware handler
> and merge the respective hardware handler into that target if the
> blacklist entry matches.
> 
> > Next up would be to get Jens's cmd type tree and work on the message
> > passing code.
> > 
> > And this patch currently does not work since I do not have 
> a Clariion
> > box and I do not know what to match for the {vendor, model} tuple. I
> > think it was something like DCG or DGC and the model had different
> > RAID
> > strings depending on how it was setup and could have some 
> other string
> > if it did not use RAID. If you have a Clariion or you work 
> for EMC and
> > happen to know this info could you pass those strings to me?
> > 
> IIRC this is
> 
> 'DGC' 'DISK'
> 'DGC' 'RAID 10'
> 'DGC' 'RAID 5'

And 'DGC' 'LUNZ'

What about supporting a wildcard model string?

> 
> Hrmph. There is one bit which doesn't quite work out.
> While the hardware handler knows how to handle error codes and how to
> switch
> paths for a specific device, it doesn't know _when_ to switch it.
> I don't think it's a clever idea to switch paths whenever you 
> encounter
> an
> passive path. Seems like you could do a nice ping-pong that way ...
> 
> 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
> 

RE: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[Hannes Reinecke]

Am Fr 21.07.2006 21:10 schrieb Edward Goggin <egoggin@emc.com>:

> > IIRC this is
> > 
> > 'DGC' 'DISK'
> > 'DGC' 'RAID 10'
> > 'DGC' 'RAID 5'
> 
> And 'DGC' 'LUNZ'
> 
Yeah. But I left that one out as there is no device attached to it
so we're unlikely to require a hw handler, or?

> What about supporting a wildcard model string?
> 
Well, we're having that.
So 'DGC' 'DISK' and
'DGC' 'RAID*' should be okay.

Cheers,

Hannes

RE: [PATCH RFC] move scsi parts of dm hw handlers to scsi layer

[egoggin]

 

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org 
> [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Hannes Reinecke
> Sent: Friday, July 21, 2006 3:31 PM
> To: goggin, edward
> Cc: michaelc@cs.wisc.edu; dm-devel@redhat.com; 
> linux-scsi@vger.kernel.org; agk@redhat.com
> Subject: RE: [PATCH RFC] move scsi parts of dm hw handlers to 
> scsi layer
> 
> Am Fr 21.07.2006 21:10 schrieb Edward Goggin <egoggin@emc.com>:
> 
> > > IIRC this is
> > > 
> > > 'DGC' 'DISK'
> > > 'DGC' 'RAID 10'
> > > 'DGC' 'RAID 5'
> > 
> > And 'DGC' 'LUNZ'
> > 
> Yeah. But I left that one out as there is no device attached to it
> so we're unlikely to require a hw handler, or?

Isn't the scsi hw handler independent of DM though?  Also, even in
the DM case, it is useful to be able to parse sense codes associated
with failed IO to a LUNZ CLARiiON logical unit.

> 
> > What about supporting a wildcard model string?
> > 
> Well, we're having that.
> So 'DGC' 'DISK' and
> 'DGC' 'RAID*' should be okay.
> 
> 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
>