[patch 0/3] Add HP hardware handler support to dm-multipath

[patch 0/3] Add HP hardware handler support to dm-multipath

[dwysocha]

The following 3 patches add HP hardware handler support to dm-multipath.
The first patch is very basic and provides a baseline of support but it is not
complete (has no retries, error code handling, etc).  Second and third patches
add retries and some error code handling.

I believe most, if not all, comments have been addressed with these latest
patches.  Alasdair, Mike, please let me know if I missed something.

-- 

[patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[dwysocha]

[-- Attachment #1: dm-hp-sw-v0.0.2.patch --]
[-- Type: text/plain, Size: 6888 bytes --]

This patch adds the most basic dm-multipath hardware support for the 
HP active/passive arrays.


Index: linux-2.6.23-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Makefile
+++ linux-2.6.23-rc1/drivers/md/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
 obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
 obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
 obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
+obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
 obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
 obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -0,0 +1,203 @@
+/*
+ * Copyright (C) 2005 Mike Christie, All rights reserved.
+ * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+ * Authors: Mike Christie
+ *          Dave Wysochanski
+ *
+ * This file is released under the GPL.
+ *
+ * This module implements the specific path activation code for
+ * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
+ * storage arrays.
+ * These storage arrays have controller-based failover, not
+ * LUN-based failover.  However, LUN-based failover is the design
+ * of dm-multipath. Thus, this module is written for LUN-based failover.
+ */
+#include <linux/blkdev.h>
+#include <linux/list.h>
+#include <linux/types.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+
+#include "dm.h"
+#include "dm-hw-handler.h"
+
+#define DM_MSG_PREFIX "multipath hp_sw"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: path activation request
+ * @error: scsi-ml error
+ *
+ *  Check sense data, free request structure, and notify dm that
+ *  pg initialization has completed.
+ *
+ * Context: scsi-ml softirq
+ *
+ * Possible optimizations
+ * 1. Actually check sense data for retryable error (e.g. NOT_READY)
+ */
+static void hp_sw_end_io(struct request *req, int error)
+{
+	struct dm_path *path = req->end_io_data;
+	unsigned err_flags;
+
+	if (!error) {
+		err_flags = 0;
+		DMDEBUG("%s path activation command - success",
+		       	path->dev->name);
+	} else {
+		DMWARN("path activation command on %s - error=0x%x",
+		       path->dev->name, error);
+		err_flags = MP_FAIL_PATH;
+	}
+
+	req->end_io_data = NULL;
+	__blk_put_request(req->q, req);
+	dm_pg_init_complete(path, err_flags);
+}
+
+/**
+ * hp_sw_get_request - Allocate an HP specific path activation request
+ * @path: path on which request will be sent (needed for request queue)
+ *
+ * The START command is used for path activation request.
+ * These arrays are controller-based failover, not LUN based.
+ * One START command issued to a single path will fail over all
+ * LUNs for the same controller.
+ *
+ * Possible optimizations
+ * 1. Make timeout configurable
+ * 2. Preallocate request
+ */
+static struct request *hp_sw_get_request(struct dm_path *path)
+{
+	struct request *req;
+	struct block_device *bdev = path->dev->bdev;
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct hp_sw_context *h = path->hwhcontext;
+
+	req = blk_get_request(q, WRITE, GFP_NOIO);
+	if (!req)
+		goto out;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io_data = path;
+	req->sense = h->sense;
+	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
+
+	memset(&req->cmd, 0, BLK_MAX_CDB);
+	req->cmd[0] = START_STOP;
+	req->cmd[4] = 1;
+	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
+ out:
+	return req;
+}
+
+/**
+ * hp_sw_pg_init - HP path activation implementation.
+ * @hwh: hardware handler specific data
+ * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
+ * @path: path to send initialization command
+ *
+ * Send an HP-specific path activation command on 'path'.
+ * Do not try to optimize in any way, just send the activation command.
+ * More than one path activation command may be sent to the same controller.
+ * This seems to work fine for basic failover support.
+ *
+ * Possible optimizations
+ * 1. Detect an in-progress activation request and avoid submitting another one
+ * 2. Model the controller and only send a single activation request at a time
+ * 3. Determine the state of a path before sending an activation request
+ *
+ * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
+ */
+static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
+			  struct dm_path *path)
+{
+	struct request *req;
+	struct hp_sw_context *h;
+
+	path->hwhcontext = hwh->context;
+	h = hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("%s path activation command allocation fail ",
+		      path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("path activation command sent on %s",
+		path->dev->name);
+
+	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
+	return;
+
+ fail:
+	dm_pg_init_complete(path, MP_FAIL_PATH);
+}
+
+static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
+{
+	struct hp_sw_context *h;
+
+	h = kmalloc(sizeof(*h), GFP_KERNEL);
+	if (!h)
+		return -ENOMEM;
+	hwh->context = h;
+	return 0;
+}
+
+static void hp_sw_destroy(struct hw_handler *hwh)
+{
+	struct hp_sw_context *h = hwh->context;
+
+	kfree(h);
+}
+
+static struct hw_handler_type hp_sw_hwh = {
+	.name = "hp_sw",
+	.module = THIS_MODULE,
+	.create = hp_sw_create,
+	.destroy = hp_sw_destroy,
+	.pg_init = hp_sw_pg_init,
+};
+
+static int __init hp_sw_init(void)
+{
+	int r;
+
+	r = dm_register_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("register failed %d", r);
+	else
+		DMINFO("version 0.0.2 loaded");
+
+	return r;
+}
+
+static void __exit hp_sw_exit(void)
+{
+	int r;
+
+	r = dm_unregister_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("unregister failed %d", r);
+}
+
+module_init(hp_sw_init);
+module_exit(hp_sw_exit);
+
+MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
+MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
+MODULE_LICENSE("GPL");
Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Kconfig
+++ linux-2.6.23-rc1/drivers/md/Kconfig
@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 
+config DM_MULTIPATH_HP
+        tristate "HP MSA multipath support (EXPERIMENTAL)"
+        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
+        ---help---
+          Multipath support for HP MSA (Active/Passive) series hardware.
+
 config DM_DELAY
 	tristate "I/O delaying target (EXPERIMENTAL)"
 	depends on BLK_DEV_DM && EXPERIMENTAL

-- 

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Mike Christie]

dwysocha@redhat.com wrote:

Are you just sending a mail with an attachement? Patch looks ok.

Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 10:18 -0500, Mike Christie wrote:
> dwysocha@redhat.com wrote:
> 
> Are you just sending a mail with an attachement? Patch looks ok.
> 
> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel



[-- Attachment #2: dm-hp-sw-v0.0.2.patch --]
[-- Type: text/x-patch, Size: 7284 bytes --]

Extremely basic hp hardware handler (no retries, no error handling, etc).

This patch adds the most basic dm-multipath hardware support for the 
HP active/passive arrays.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Makefile	2007-07-25 15:42:52.000000000 -0400
+++ linux-2.6.23-rc1/drivers/md/Makefile	2007-07-25 15:43:10.000000000 -0400
@@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
 obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
 obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
 obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
+obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
 obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
 obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c	2007-07-26 11:48:47.000000000 -0400
@@ -0,0 +1,203 @@
+/*
+ * Copyright (C) 2005 Mike Christie, All rights reserved.
+ * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+ * Authors: Mike Christie
+ *          Dave Wysochanski
+ *
+ * This file is released under the GPL.
+ *
+ * This module implements the specific path activation code for
+ * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
+ * storage arrays.
+ * These storage arrays have controller-based failover, not
+ * LUN-based failover.  However, LUN-based failover is the design
+ * of dm-multipath. Thus, this module is written for LUN-based failover.
+ */
+#include <linux/blkdev.h>
+#include <linux/list.h>
+#include <linux/types.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+
+#include "dm.h"
+#include "dm-hw-handler.h"
+
+#define DM_MSG_PREFIX "multipath hp_sw"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: path activation request
+ * @error: scsi-ml error
+ *
+ *  Check sense data, free request structure, and notify dm that
+ *  pg initialization has completed.
+ *
+ * Context: scsi-ml softirq
+ *
+ * Possible optimizations
+ * 1. Actually check sense data for retryable error (e.g. NOT_READY)
+ */
+static void hp_sw_end_io(struct request *req, int error)
+{
+	struct dm_path *path = req->end_io_data;
+	unsigned err_flags;
+
+	if (!error) {
+		err_flags = 0;
+		DMDEBUG("%s path activation command - success",
+		       	path->dev->name);
+	} else {
+		DMWARN("path activation command on %s - error=0x%x",
+		       path->dev->name, error);
+		err_flags = MP_FAIL_PATH;
+	}
+
+	req->end_io_data = NULL;
+	__blk_put_request(req->q, req);
+	dm_pg_init_complete(path, err_flags);
+}
+
+/**
+ * hp_sw_get_request - Allocate an HP specific path activation request
+ * @path: path on which request will be sent (needed for request queue)
+ *
+ * The START command is used for path activation request.
+ * These arrays are controller-based failover, not LUN based.
+ * One START command issued to a single path will fail over all
+ * LUNs for the same controller.
+ *
+ * Possible optimizations
+ * 1. Make timeout configurable
+ * 2. Preallocate request
+ */
+static struct request *hp_sw_get_request(struct dm_path *path)
+{
+	struct request *req;
+	struct block_device *bdev = path->dev->bdev;
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct hp_sw_context *h = path->hwhcontext;
+
+	req = blk_get_request(q, WRITE, GFP_NOIO);
+	if (!req)
+		goto out;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io_data = path;
+	req->sense = h->sense;
+	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
+
+	memset(&req->cmd, 0, BLK_MAX_CDB);
+	req->cmd[0] = START_STOP;
+	req->cmd[4] = 1;
+	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
+ out:
+	return req;
+}
+
+/**
+ * hp_sw_pg_init - HP path activation implementation.
+ * @hwh: hardware handler specific data
+ * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
+ * @path: path to send initialization command
+ *
+ * Send an HP-specific path activation command on 'path'.
+ * Do not try to optimize in any way, just send the activation command.
+ * More than one path activation command may be sent to the same controller.
+ * This seems to work fine for basic failover support.
+ *
+ * Possible optimizations
+ * 1. Detect an in-progress activation request and avoid submitting another one
+ * 2. Model the controller and only send a single activation request at a time
+ * 3. Determine the state of a path before sending an activation request
+ *
+ * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
+ */
+static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
+			  struct dm_path *path)
+{
+	struct request *req;
+	struct hp_sw_context *h;
+
+	path->hwhcontext = hwh->context;
+	h = hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("%s path activation command allocation fail ",
+		      path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("path activation command sent on %s",
+		path->dev->name);
+
+	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
+	return;
+
+ fail:
+	dm_pg_init_complete(path, MP_FAIL_PATH);
+}
+
+static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
+{
+	struct hp_sw_context *h;
+
+	h = kmalloc(sizeof(*h), GFP_KERNEL);
+	if (!h)
+		return -ENOMEM;
+	hwh->context = h;
+	return 0;
+}
+
+static void hp_sw_destroy(struct hw_handler *hwh)
+{
+	struct hp_sw_context *h = hwh->context;
+
+	kfree(h);
+}
+
+static struct hw_handler_type hp_sw_hwh = {
+	.name = "hp_sw",
+	.module = THIS_MODULE,
+	.create = hp_sw_create,
+	.destroy = hp_sw_destroy,
+	.pg_init = hp_sw_pg_init,
+};
+
+static int __init hp_sw_init(void)
+{
+	int r;
+
+	r = dm_register_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("register failed %d", r);
+	else
+		DMINFO("version 0.0.2 loaded");
+
+	return r;
+}
+
+static void __exit hp_sw_exit(void)
+{
+	int r;
+
+	r = dm_unregister_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("unregister failed %d", r);
+}
+
+module_init(hp_sw_init);
+module_exit(hp_sw_exit);
+
+MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
+MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
+MODULE_LICENSE("GPL");
Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Kconfig	2007-07-25 15:42:52.000000000 -0400
+++ linux-2.6.23-rc1/drivers/md/Kconfig	2007-07-25 15:43:10.000000000 -0400
@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 
+config DM_MULTIPATH_HP
+        tristate "HP MSA multipath support (EXPERIMENTAL)"
+        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
+        ---help---
+          Multipath support for HP MSA (Active/Passive) series hardware.
+
 config DM_DELAY
 	tristate "I/O delaying target (EXPERIMENTAL)"
 	depends on BLK_DEV_DM && EXPERIMENTAL

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Chandra Seetharaman]

Hi Dave,

some coding style related comments (below).

On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> plain text document attachment (dm-hp-sw-v0.0.2.patch)
> This patch adds the most basic dm-multipath hardware support for the 
> HP active/passive arrays.
> 
> 
> Index: linux-2.6.23-rc1/drivers/md/Makefile
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/Makefile
> +++ linux-2.6.23-rc1/drivers/md/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
>  obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
>  obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
>  obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
> +obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
>  obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
>  obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
>  obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
> Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> @@ -0,0 +1,203 @@
> +/*
> + * Copyright (C) 2005 Mike Christie, All rights reserved.
> + * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
> + * Authors: Mike Christie
> + *          Dave Wysochanski
> + *
> + * This file is released under the GPL.
> + *
> + * This module implements the specific path activation code for
> + * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
> + * storage arrays.
> + * These storage arrays have controller-based failover, not
> + * LUN-based failover.  However, LUN-based failover is the design
> + * of dm-multipath. Thus, this module is written for LUN-based failover.
> + */
> +#include <linux/blkdev.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
> +
> +#include "dm.h"
> +#include "dm-hw-handler.h"
> +
> +#define DM_MSG_PREFIX "multipath hp_sw"
> +
> +struct hp_sw_context {
> +	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> +};
> +
> +
> +/**
> + * hp_sw_end_io - Completion handler for HP path activation.
> + * @req: path activation request
> + * @error: scsi-ml error
> + *
> + *  Check sense data, free request structure, and notify dm that
> + *  pg initialization has completed.
> + *
> + * Context: scsi-ml softirq
> + *
> + * Possible optimizations
> + * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> + */
> +static void hp_sw_end_io(struct request *req, int error)
> +{
> +	struct dm_path *path = req->end_io_data;
> +	unsigned err_flags;
> +
> +	if (!error) {
> +		err_flags = 0;
> +		DMDEBUG("%s path activation command - success",
> +		       	path->dev->name);

Mixed use of space and tab for indentation (many other places too).

> +	} else {
> +		DMWARN("path activation command on %s - error=0x%x",

Could use the same format like "%s: path activation command - status",
you made changes towards that in the next patch. Could make is
consistent.

> +		       path->dev->name, error);
> +		err_flags = MP_FAIL_PATH;
> +	}
> +
> +	req->end_io_data = NULL;
> +	__blk_put_request(req->q, req);
> +	dm_pg_init_complete(path, err_flags);
> +}
> +
> +/**
> + * hp_sw_get_request - Allocate an HP specific path activation request
> + * @path: path on which request will be sent (needed for request queue)
> + *
> + * The START command is used for path activation request.
> + * These arrays are controller-based failover, not LUN based.
> + * One START command issued to a single path will fail over all
> + * LUNs for the same controller.
> + *
> + * Possible optimizations
> + * 1. Make timeout configurable
> + * 2. Preallocate request
> + */
> +static struct request *hp_sw_get_request(struct dm_path *path)
> +{
> +	struct request *req;
> +	struct block_device *bdev = path->dev->bdev;
> +	struct request_queue *q = bdev_get_queue(bdev);
> +	struct hp_sw_context *h = path->hwhcontext;
> +
> +	req = blk_get_request(q, WRITE, GFP_NOIO);
> +	if (!req)
> +		goto out;
> +
> +	req->timeout = 60*HZ;
> +
> +	req->errors = 0;
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
> +	req->end_io_data = path;
> +	req->sense = h->sense;
> +	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
> +
> +	memset(&req->cmd, 0, BLK_MAX_CDB);
> +	req->cmd[0] = START_STOP;
> +	req->cmd[4] = 1;
> +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> + out:

I think there will be no space before the label (one more below).

> +	return req;
> +}
> +
> +/**
> + * hp_sw_pg_init - HP path activation implementation.
> + * @hwh: hardware handler specific data
> + * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
> + * @path: path to send initialization command
> + *
> + * Send an HP-specific path activation command on 'path'.
> + * Do not try to optimize in any way, just send the activation command.
> + * More than one path activation command may be sent to the same controller.
> + * This seems to work fine for basic failover support.
> + *
> + * Possible optimizations
> + * 1. Detect an in-progress activation request and avoid submitting another one
> + * 2. Model the controller and only send a single activation request at a time
> + * 3. Determine the state of a path before sending an activation request
> + *
> + * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
> + */
> +static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
> +			  struct dm_path *path)
> +{
> +	struct request *req;
> +	struct hp_sw_context *h;
> +
> +	path->hwhcontext = hwh->context;
> +	h = hwh->context;
> +
> +	req = hp_sw_get_request(path);
> +	if (!req) {
> +		DMERR("%s path activation command allocation fail ",
> +		      path->dev->name);
> +		goto fail;
> +	}
> +
> +	DMDEBUG("path activation command sent on %s",
> +		path->dev->name);
> +
> +	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> +	return;
> +
> + fail:
> +	dm_pg_init_complete(path, MP_FAIL_PATH);
> +}
> +
> +static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> +{
> +	struct hp_sw_context *h;
> +
> +	h = kmalloc(sizeof(*h), GFP_KERNEL);
> +	if (!h)
> +		return -ENOMEM;
> +	hwh->context = h;
> +	return 0;
> +}
> +
> +static void hp_sw_destroy(struct hw_handler *hwh)
> +{
> +	struct hp_sw_context *h = hwh->context;
> +
> +	kfree(h);
> +}
> +
> +static struct hw_handler_type hp_sw_hwh = {
> +	.name = "hp_sw",
> +	.module = THIS_MODULE,
> +	.create = hp_sw_create,
> +	.destroy = hp_sw_destroy,
> +	.pg_init = hp_sw_pg_init,
> +};
> +
> +static int __init hp_sw_init(void)
> +{
> +	int r;
> +
> +	r = dm_register_hw_handler(&hp_sw_hwh);
> +	if (r < 0)
> +		DMERR("register failed %d", r);
> +	else
> +		DMINFO("version 0.0.2 loaded");
> +
> +	return r;
> +}
> +
> +static void __exit hp_sw_exit(void)
> +{
> +	int r;
> +
> +	r = dm_unregister_hw_handler(&hp_sw_hwh);
> +	if (r < 0)
> +		DMERR("unregister failed %d", r);
> +}
> +
> +module_init(hp_sw_init);
> +module_exit(hp_sw_exit);
> +
> +MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
> +MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
> +MODULE_LICENSE("GPL");
> Index: linux-2.6.23-rc1/drivers/md/Kconfig
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/Kconfig
> +++ linux-2.6.23-rc1/drivers/md/Kconfig
> @@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
>  	---help---
>  	  Multipath support for LSI/Engenio RDAC.
> 
> +config DM_MULTIPATH_HP
> +        tristate "HP MSA multipath support (EXPERIMENTAL)"
> +        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
> +        ---help---
> +          Multipath support for HP MSA (Active/Passive) series hardware.
> +
>  config DM_DELAY
>  	tristate "I/O delaying target (EXPERIMENTAL)"
>  	depends on BLK_DEV_DM && EXPERIMENTAL
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 12:09 -0700, Chandra Seetharaman wrote:
> Hi Dave,
> 
> some coding style related comments (below).
> 
> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > plain text document attachment (dm-hp-sw-v0.0.2.patch)
> > This patch adds the most basic dm-multipath hardware support for the 
> > HP active/passive arrays.
> > 
> > 
> > Index: linux-2.6.23-rc1/drivers/md/Makefile
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/Makefile
> > +++ linux-2.6.23-rc1/drivers/md/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
> >  obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
> >  obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
> >  obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
> > +obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
> >  obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
> >  obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
> >  obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
> > Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > ===================================================================
> > --- /dev/null
> > +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > @@ -0,0 +1,203 @@
> > +/*
> > + * Copyright (C) 2005 Mike Christie, All rights reserved.
> > + * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
> > + * Authors: Mike Christie
> > + *          Dave Wysochanski
> > + *
> > + * This file is released under the GPL.
> > + *
> > + * This module implements the specific path activation code for
> > + * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
> > + * storage arrays.
> > + * These storage arrays have controller-based failover, not
> > + * LUN-based failover.  However, LUN-based failover is the design
> > + * of dm-multipath. Thus, this module is written for LUN-based failover.
> > + */
> > +#include <linux/blkdev.h>
> > +#include <linux/list.h>
> > +#include <linux/types.h>
> > +#include <scsi/scsi.h>
> > +#include <scsi/scsi_cmnd.h>
> > +
> > +#include "dm.h"
> > +#include "dm-hw-handler.h"
> > +
> > +#define DM_MSG_PREFIX "multipath hp_sw"
> > +
> > +struct hp_sw_context {
> > +	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> > +};
> > +
> > +
> > +/**
> > + * hp_sw_end_io - Completion handler for HP path activation.
> > + * @req: path activation request
> > + * @error: scsi-ml error
> > + *
> > + *  Check sense data, free request structure, and notify dm that
> > + *  pg initialization has completed.
> > + *
> > + * Context: scsi-ml softirq
> > + *
> > + * Possible optimizations
> > + * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> > + */
> > +static void hp_sw_end_io(struct request *req, int error)
> > +{
> > +	struct dm_path *path = req->end_io_data;
> > +	unsigned err_flags;
> > +
> > +	if (!error) {
> > +		err_flags = 0;
> > +		DMDEBUG("%s path activation command - success",
> > +		       	path->dev->name);
> 
> Mixed use of space and tab for indentation (many other places too).
> 
> > +	} else {
> > +		DMWARN("path activation command on %s - error=0x%x",
> 
> Could use the same format like "%s: path activation command - status",
> you made changes towards that in the next patch. Could make is
> consistent.
> 
> > +		       path->dev->name, error);
> > +		err_flags = MP_FAIL_PATH;
> > +	}
> > +
> > +	req->end_io_data = NULL;
> > +	__blk_put_request(req->q, req);
> > +	dm_pg_init_complete(path, err_flags);
> > +}
> > +
> > +/**
> > + * hp_sw_get_request - Allocate an HP specific path activation request
> > + * @path: path on which request will be sent (needed for request queue)
> > + *
> > + * The START command is used for path activation request.
> > + * These arrays are controller-based failover, not LUN based.
> > + * One START command issued to a single path will fail over all
> > + * LUNs for the same controller.
> > + *
> > + * Possible optimizations
> > + * 1. Make timeout configurable
> > + * 2. Preallocate request
> > + */
> > +static struct request *hp_sw_get_request(struct dm_path *path)
> > +{
> > +	struct request *req;
> > +	struct block_device *bdev = path->dev->bdev;
> > +	struct request_queue *q = bdev_get_queue(bdev);
> > +	struct hp_sw_context *h = path->hwhcontext;
> > +
> > +	req = blk_get_request(q, WRITE, GFP_NOIO);
> > +	if (!req)
> > +		goto out;
> > +
> > +	req->timeout = 60*HZ;
> > +
> > +	req->errors = 0;
> > +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> > +	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
> > +	req->end_io_data = path;
> > +	req->sense = h->sense;
> > +	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
> > +
> > +	memset(&req->cmd, 0, BLK_MAX_CDB);
> > +	req->cmd[0] = START_STOP;
> > +	req->cmd[4] = 1;
> > +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> > + out:
> 
> I think there will be no space before the label (one more below).
> 
> > +	return req;
> > +}
> > +
> > +/**
> > + * hp_sw_pg_init - HP path activation implementation.
> > + * @hwh: hardware handler specific data
> > + * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
> > + * @path: path to send initialization command
> > + *
> > + * Send an HP-specific path activation command on 'path'.
> > + * Do not try to optimize in any way, just send the activation command.
> > + * More than one path activation command may be sent to the same controller.
> > + * This seems to work fine for basic failover support.
> > + *
> > + * Possible optimizations
> > + * 1. Detect an in-progress activation request and avoid submitting another one
> > + * 2. Model the controller and only send a single activation request at a time
> > + * 3. Determine the state of a path before sending an activation request
> > + *
> > + * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
> > + */
> > +static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
> > +			  struct dm_path *path)
> > +{
> > +	struct request *req;
> > +	struct hp_sw_context *h;
> > +
> > +	path->hwhcontext = hwh->context;
> > +	h = hwh->context;
> > +
> > +	req = hp_sw_get_request(path);
> > +	if (!req) {
> > +		DMERR("%s path activation command allocation fail ",
> > +		      path->dev->name);
> > +		goto fail;
> > +	}
> > +
> > +	DMDEBUG("path activation command sent on %s",
> > +		path->dev->name);
> > +
> > +	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> > +	return;
> > +
> > + fail:
> > +	dm_pg_init_complete(path, MP_FAIL_PATH);
> > +}
> > +
> > +static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> > +{
> > +	struct hp_sw_context *h;
> > +
> > +	h = kmalloc(sizeof(*h), GFP_KERNEL);
> > +	if (!h)
> > +		return -ENOMEM;
> > +	hwh->context = h;
> > +	return 0;
> > +}
> > +
> > +static void hp_sw_destroy(struct hw_handler *hwh)
> > +{
> > +	struct hp_sw_context *h = hwh->context;
> > +
> > +	kfree(h);
> > +}
> > +
> > +static struct hw_handler_type hp_sw_hwh = {
> > +	.name = "hp_sw",
> > +	.module = THIS_MODULE,
> > +	.create = hp_sw_create,
> > +	.destroy = hp_sw_destroy,
> > +	.pg_init = hp_sw_pg_init,
> > +};
> > +
> > +static int __init hp_sw_init(void)
> > +{
> > +	int r;
> > +
> > +	r = dm_register_hw_handler(&hp_sw_hwh);
> > +	if (r < 0)
> > +		DMERR("register failed %d", r);
> > +	else
> > +		DMINFO("version 0.0.2 loaded");
> > +
> > +	return r;
> > +}
> > +
> > +static void __exit hp_sw_exit(void)
> > +{
> > +	int r;
> > +
> > +	r = dm_unregister_hw_handler(&hp_sw_hwh);
> > +	if (r < 0)
> > +		DMERR("unregister failed %d", r);
> > +}
> > +
> > +module_init(hp_sw_init);
> > +module_exit(hp_sw_exit);
> > +
> > +MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
> > +MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
> > +MODULE_LICENSE("GPL");
> > Index: linux-2.6.23-rc1/drivers/md/Kconfig
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/Kconfig
> > +++ linux-2.6.23-rc1/drivers/md/Kconfig
> > @@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
> >  	---help---
> >  	  Multipath support for LSI/Engenio RDAC.
> > 
> > +config DM_MULTIPATH_HP
> > +        tristate "HP MSA multipath support (EXPERIMENTAL)"
> > +        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
> > +        ---help---
> > +          Multipath support for HP MSA (Active/Passive) series hardware.
> > +
> >  config DM_DELAY
> >  	tristate "I/O delaying target (EXPERIMENTAL)"
> >  	depends on BLK_DEV_DM && EXPERIMENTAL
> > 
> -- 
> 

I think the attached patch addresses all of your comments.



[-- Attachment #2: dm-hp-sw-v0.0.2.patch --]
[-- Type: text/x-patch, Size: 7040 bytes --]

Extremely basic hp hardware handler (no retries, no error handling, etc).

This patch adds the most basic dm-multipath hardware support for the 
HP active/passive arrays.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Makefile
+++ linux-2.6.23-rc1/drivers/md/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
 obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
 obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
 obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
+obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
 obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
 obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2005 Mike Christie, All rights reserved.
+ * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+ * Authors: Mike Christie
+ *          Dave Wysochanski
+ *
+ * This file is released under the GPL.
+ *
+ * This module implements the specific path activation code for
+ * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
+ * storage arrays.
+ * These storage arrays have controller-based failover, not
+ * LUN-based failover.  However, LUN-based failover is the design
+ * of dm-multipath. Thus, this module is written for LUN-based failover.
+ */
+#include <linux/blkdev.h>
+#include <linux/list.h>
+#include <linux/types.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+
+#include "dm.h"
+#include "dm-hw-handler.h"
+
+#define DM_MSG_PREFIX "multipath hp_sw"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: path activation request
+ * @error: scsi-ml error
+ *
+ *  Check sense data, free request structure, and notify dm that
+ *  pg initialization has completed.
+ *
+ * Context: scsi-ml softirq
+ *
+ * Possible optimizations
+ * 1. Actually check sense data for retryable error (e.g. NOT_READY)
+ */
+static void hp_sw_end_io(struct request *req, int error)
+{
+	struct dm_path *path = req->end_io_data;
+	unsigned err_flags;
+
+	if (!error) {
+		err_flags = 0;
+		DMDEBUG("%s path activation command - success",
+			path->dev->name);
+	} else {
+		DMWARN("%s path activation command - error=0x%x",
+			path->dev->name, error);
+		err_flags = MP_FAIL_PATH;
+	}
+
+	req->end_io_data = NULL;
+	__blk_put_request(req->q, req);
+	dm_pg_init_complete(path, err_flags);
+}
+
+/**
+ * hp_sw_get_request - Allocate an HP specific path activation request
+ * @path: path on which request will be sent (needed for request queue)
+ *
+ * The START command is used for path activation request.
+ * These arrays are controller-based failover, not LUN based.
+ * One START command issued to a single path will fail over all
+ * LUNs for the same controller.
+ *
+ * Possible optimizations
+ * 1. Make timeout configurable
+ * 2. Preallocate request
+ */
+static struct request *hp_sw_get_request(struct dm_path *path)
+{
+	struct request *req;
+	struct block_device *bdev = path->dev->bdev;
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct hp_sw_context *h = path->hwhcontext;
+
+	req = blk_get_request(q, WRITE, GFP_NOIO);
+	if (!req)
+		goto out;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io_data = path;
+	req->sense = h->sense;
+	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
+
+	memset(&req->cmd, 0, BLK_MAX_CDB);
+	req->cmd[0] = START_STOP;
+	req->cmd[4] = 1;
+	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
+out:
+	return req;
+}
+
+/**
+ * hp_sw_pg_init - HP path activation implementation.
+ * @hwh: hardware handler specific data
+ * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
+ * @path: path to send initialization command
+ *
+ * Send an HP-specific path activation command on 'path'.
+ * Do not try to optimize in any way, just send the activation command.
+ * More than one path activation command may be sent to the same controller.
+ * This seems to work fine for basic failover support.
+ *
+ * Possible optimizations
+ * 1. Detect an in-progress activation request and avoid submitting another one
+ * 2. Model the controller and only send a single activation request at a time
+ * 3. Determine the state of a path before sending an activation request
+ *
+ * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
+ */
+static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
+			struct dm_path *path)
+{
+	struct request *req;
+	struct hp_sw_context *h;
+
+	path->hwhcontext = hwh->context;
+	h = hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("%s path activation command - allocation fail",
+			path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("%s path activation command - sent", path->dev->name);
+
+	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
+	return;
+
+fail:
+	dm_pg_init_complete(path, MP_FAIL_PATH);
+}
+
+static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
+{
+	struct hp_sw_context *h;
+
+	h = kmalloc(sizeof(*h), GFP_KERNEL);
+	if (!h)
+		return -ENOMEM;
+	hwh->context = h;
+	return 0;
+}
+
+static void hp_sw_destroy(struct hw_handler *hwh)
+{
+	struct hp_sw_context *h = hwh->context;
+
+	kfree(h);
+}
+
+static struct hw_handler_type hp_sw_hwh = {
+	.name = "hp_sw",
+	.module = THIS_MODULE,
+	.create = hp_sw_create,
+	.destroy = hp_sw_destroy,
+	.pg_init = hp_sw_pg_init,
+};
+
+static int __init hp_sw_init(void)
+{
+	int r;
+
+	r = dm_register_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("register failed %d", r);
+	else
+		DMINFO("version 0.0.2 loaded");
+
+	return r;
+}
+
+static void __exit hp_sw_exit(void)
+{
+	int r;
+
+	r = dm_unregister_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("unregister failed %d", r);
+}
+
+module_init(hp_sw_init);
+module_exit(hp_sw_exit);
+
+MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
+MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
+MODULE_LICENSE("GPL");
Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Kconfig
+++ linux-2.6.23-rc1/drivers/md/Kconfig
@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 
+config DM_MULTIPATH_HP
+        tristate "HP MSA multipath support (EXPERIMENTAL)"
+        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
+        ---help---
+          Multipath support for HP MSA (Active/Passive) series hardware.
+
 config DM_DELAY
 	tristate "I/O delaying target (EXPERIMENTAL)"
 	depends on BLK_DEV_DM && EXPERIMENTAL

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Chandra Seetharaman]

Do you want to keep the DMDEBUG in this patch ?

Other than that it looks good...

Thanks

chandra
On Mon, 2007-07-30 at 14:06 -0400, Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:09 -0700, Chandra Seetharaman wrote:
> > Hi Dave,
> > 
> > some coding style related comments (below).
> > 
> > On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > > plain text document attachment (dm-hp-sw-v0.0.2.patch)
> > > This patch adds the most basic dm-multipath hardware support for the 
> > > HP active/passive arrays.
> > > 
> > > 
> > > Index: linux-2.6.23-rc1/drivers/md/Makefile
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/Makefile
> > > +++ linux-2.6.23-rc1/drivers/md/Makefile
> > > @@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
> > >  obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
> > >  obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
> > >  obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
> > > +obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
> > >  obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
> > >  obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
> > >  obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
> > > Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > > ===================================================================
> > > --- /dev/null
> > > +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > > @@ -0,0 +1,203 @@
> > > +/*
> > > + * Copyright (C) 2005 Mike Christie, All rights reserved.
> > > + * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
> > > + * Authors: Mike Christie
> > > + *          Dave Wysochanski
> > > + *
> > > + * This file is released under the GPL.
> > > + *
> > > + * This module implements the specific path activation code for
> > > + * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
> > > + * storage arrays.
> > > + * These storage arrays have controller-based failover, not
> > > + * LUN-based failover.  However, LUN-based failover is the design
> > > + * of dm-multipath. Thus, this module is written for LUN-based failover.
> > > + */
> > > +#include <linux/blkdev.h>
> > > +#include <linux/list.h>
> > > +#include <linux/types.h>
> > > +#include <scsi/scsi.h>
> > > +#include <scsi/scsi_cmnd.h>
> > > +
> > > +#include "dm.h"
> > > +#include "dm-hw-handler.h"
> > > +
> > > +#define DM_MSG_PREFIX "multipath hp_sw"
> > > +
> > > +struct hp_sw_context {
> > > +	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> > > +};
> > > +
> > > +
> > > +/**
> > > + * hp_sw_end_io - Completion handler for HP path activation.
> > > + * @req: path activation request
> > > + * @error: scsi-ml error
> > > + *
> > > + *  Check sense data, free request structure, and notify dm that
> > > + *  pg initialization has completed.
> > > + *
> > > + * Context: scsi-ml softirq
> > > + *
> > > + * Possible optimizations
> > > + * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> > > + */
> > > +static void hp_sw_end_io(struct request *req, int error)
> > > +{
> > > +	struct dm_path *path = req->end_io_data;
> > > +	unsigned err_flags;
> > > +
> > > +	if (!error) {
> > > +		err_flags = 0;
> > > +		DMDEBUG("%s path activation command - success",
> > > +		       	path->dev->name);
> > 
> > Mixed use of space and tab for indentation (many other places too).
> > 
> > > +	} else {
> > > +		DMWARN("path activation command on %s - error=0x%x",
> > 
> > Could use the same format like "%s: path activation command - status",
> > you made changes towards that in the next patch. Could make is
> > consistent.
> > 
> > > +		       path->dev->name, error);
> > > +		err_flags = MP_FAIL_PATH;
> > > +	}
> > > +
> > > +	req->end_io_data = NULL;
> > > +	__blk_put_request(req->q, req);
> > > +	dm_pg_init_complete(path, err_flags);
> > > +}
> > > +
> > > +/**
> > > + * hp_sw_get_request - Allocate an HP specific path activation request
> > > + * @path: path on which request will be sent (needed for request queue)
> > > + *
> > > + * The START command is used for path activation request.
> > > + * These arrays are controller-based failover, not LUN based.
> > > + * One START command issued to a single path will fail over all
> > > + * LUNs for the same controller.
> > > + *
> > > + * Possible optimizations
> > > + * 1. Make timeout configurable
> > > + * 2. Preallocate request
> > > + */
> > > +static struct request *hp_sw_get_request(struct dm_path *path)
> > > +{
> > > +	struct request *req;
> > > +	struct block_device *bdev = path->dev->bdev;
> > > +	struct request_queue *q = bdev_get_queue(bdev);
> > > +	struct hp_sw_context *h = path->hwhcontext;
> > > +
> > > +	req = blk_get_request(q, WRITE, GFP_NOIO);
> > > +	if (!req)
> > > +		goto out;
> > > +
> > > +	req->timeout = 60*HZ;
> > > +
> > > +	req->errors = 0;
> > > +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> > > +	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
> > > +	req->end_io_data = path;
> > > +	req->sense = h->sense;
> > > +	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
> > > +
> > > +	memset(&req->cmd, 0, BLK_MAX_CDB);
> > > +	req->cmd[0] = START_STOP;
> > > +	req->cmd[4] = 1;
> > > +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> > > + out:
> > 
> > I think there will be no space before the label (one more below).
> > 
> > > +	return req;
> > > +}
> > > +
> > > +/**
> > > + * hp_sw_pg_init - HP path activation implementation.
> > > + * @hwh: hardware handler specific data
> > > + * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
> > > + * @path: path to send initialization command
> > > + *
> > > + * Send an HP-specific path activation command on 'path'.
> > > + * Do not try to optimize in any way, just send the activation command.
> > > + * More than one path activation command may be sent to the same controller.
> > > + * This seems to work fine for basic failover support.
> > > + *
> > > + * Possible optimizations
> > > + * 1. Detect an in-progress activation request and avoid submitting another one
> > > + * 2. Model the controller and only send a single activation request at a time
> > > + * 3. Determine the state of a path before sending an activation request
> > > + *
> > > + * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
> > > + */
> > > +static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
> > > +			  struct dm_path *path)
> > > +{
> > > +	struct request *req;
> > > +	struct hp_sw_context *h;
> > > +
> > > +	path->hwhcontext = hwh->context;
> > > +	h = hwh->context;
> > > +
> > > +	req = hp_sw_get_request(path);
> > > +	if (!req) {
> > > +		DMERR("%s path activation command allocation fail ",
> > > +		      path->dev->name);
> > > +		goto fail;
> > > +	}
> > > +
> > > +	DMDEBUG("path activation command sent on %s",
> > > +		path->dev->name);
> > > +
> > > +	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> > > +	return;
> > > +
> > > + fail:
> > > +	dm_pg_init_complete(path, MP_FAIL_PATH);
> > > +}
> > > +
> > > +static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> > > +{
> > > +	struct hp_sw_context *h;
> > > +
> > > +	h = kmalloc(sizeof(*h), GFP_KERNEL);
> > > +	if (!h)
> > > +		return -ENOMEM;
> > > +	hwh->context = h;
> > > +	return 0;
> > > +}
> > > +
> > > +static void hp_sw_destroy(struct hw_handler *hwh)
> > > +{
> > > +	struct hp_sw_context *h = hwh->context;
> > > +
> > > +	kfree(h);
> > > +}
> > > +
> > > +static struct hw_handler_type hp_sw_hwh = {
> > > +	.name = "hp_sw",
> > > +	.module = THIS_MODULE,
> > > +	.create = hp_sw_create,
> > > +	.destroy = hp_sw_destroy,
> > > +	.pg_init = hp_sw_pg_init,
> > > +};
> > > +
> > > +static int __init hp_sw_init(void)
> > > +{
> > > +	int r;
> > > +
> > > +	r = dm_register_hw_handler(&hp_sw_hwh);
> > > +	if (r < 0)
> > > +		DMERR("register failed %d", r);
> > > +	else
> > > +		DMINFO("version 0.0.2 loaded");
> > > +
> > > +	return r;
> > > +}
> > > +
> > > +static void __exit hp_sw_exit(void)
> > > +{
> > > +	int r;
> > > +
> > > +	r = dm_unregister_hw_handler(&hp_sw_hwh);
> > > +	if (r < 0)
> > > +		DMERR("unregister failed %d", r);
> > > +}
> > > +
> > > +module_init(hp_sw_init);
> > > +module_exit(hp_sw_exit);
> > > +
> > > +MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
> > > +MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
> > > +MODULE_LICENSE("GPL");
> > > Index: linux-2.6.23-rc1/drivers/md/Kconfig
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/Kconfig
> > > +++ linux-2.6.23-rc1/drivers/md/Kconfig
> > > @@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
> > >  	---help---
> > >  	  Multipath support for LSI/Engenio RDAC.
> > > 
> > > +config DM_MULTIPATH_HP
> > > +        tristate "HP MSA multipath support (EXPERIMENTAL)"
> > > +        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
> > > +        ---help---
> > > +          Multipath support for HP MSA (Active/Passive) series hardware.
> > > +
> > >  config DM_DELAY
> > >  	tristate "I/O delaying target (EXPERIMENTAL)"
> > >  	depends on BLK_DEV_DM && EXPERIMENTAL
> > > 
> > -- 
> > 
> 
> I think the attached patch addresses all of your comments.
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Dave Wysochanski]

On Mon, 2007-07-30 at 12:25 -0700, Chandra Seetharaman wrote:
> Do you want to keep the DMDEBUG in this patch ?
> 
> Other than that it looks good...
> 
> Thanks
> 

Unless there are objections I'd keep it.

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Chandra Seetharaman]

On Mon, 2007-07-30 at 16:10 -0400, Dave Wysochanski wrote:
> On Mon, 2007-07-30 at 12:25 -0700, Chandra Seetharaman wrote:
> > Do you want to keep the DMDEBUG in this patch ?
> > 
> > Other than that it looks good...
> > 
> > Thanks
> > 
> 
> Unless there are objections I'd keep it.

No, I don't have any objections. Just wanted to make sure that you
intend to have it that way (as you have mentioned in the other patch you
are getting rid of DMDEBUG.

chandra
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Mike Christie]

Chandra Seetharaman wrote:
> Hi Dave,
> 
> some coding style related comments (below).
> 


>> +	if (!error) {
>> +		err_flags = 0;
>> +		DMDEBUG("%s path activation command - success",
>> +		       	path->dev->name);
> 
> Mixed use of space and tab for indentation (many other places too).
> 

Where is that rule for this type of plcae?

I think that is fine in those types of places. In fact I would leave it 
with mixed because that is how the rest of the dm code does it.


>> +	memset(&req->cmd, 0, BLK_MAX_CDB);
>> +	req->cmd[0] = START_STOP;
>> +	req->cmd[4] = 1;
>> +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
>> + out:
> 
> I think there will be no space before the label (one more below).
> 

Either is normally fine, but in this case to fit with the other dm code 
that is best.

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Dave Wysochanski]

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

On Mon, 2007-07-30 at 16:08 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > Hi Dave,
> > 
> > some coding style related comments (below).
> > 
> 
> 
> >> +	if (!error) {
> >> +		err_flags = 0;
> >> +		DMDEBUG("%s path activation command - success",
> >> +		       	path->dev->name);
> > 
> > Mixed use of space and tab for indentation (many other places too).
> > 
> 
> Where is that rule for this type of plcae?
> 
> I think that is fine in those types of places. In fact I would leave it 
> with mixed because that is how the rest of the dm code does it.
> 
> 
> >> +	memset(&req->cmd, 0, BLK_MAX_CDB);
> >> +	req->cmd[0] = START_STOP;
> >> +	req->cmd[4] = 1;
> >> +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> >> + out:
> > 
> > I think there will be no space before the label (one more below).
> > 
> 
> Either is normally fine, but in this case to fit with the other dm code 
> that is best.
> 

Ok.  I think the attached is more consistent with the dm code.



[-- Attachment #2: dm-hp-sw-v0.0.2.patch --]
[-- Type: text/x-patch, Size: 7053 bytes --]

Extremely basic hp hardware handler (no retries, no error handling, etc).

This patch adds the most basic dm-multipath hardware support for the 
HP active/passive arrays.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Makefile
+++ linux-2.6.23-rc1/drivers/md/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_DM_CRYPT)		+= dm-crypt.o
 obj-$(CONFIG_DM_DELAY)		+= dm-delay.o
 obj-$(CONFIG_DM_MULTIPATH)	+= dm-multipath.o dm-round-robin.o
 obj-$(CONFIG_DM_MULTIPATH_EMC)	+= dm-emc.o
+obj-$(CONFIG_DM_MULTIPATH_HP)	+= dm-hp-sw.o
 obj-$(CONFIG_DM_MULTIPATH_RDAC)	+= dm-rdac.o
 obj-$(CONFIG_DM_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -0,0 +1,202 @@
+/*
+ * Copyright (C) 2005 Mike Christie, All rights reserved.
+ * Copyright (C) 2007 Red Hat, Inc. All rights reserved.
+ * Authors: Mike Christie
+ *          Dave Wysochanski
+ *
+ * This file is released under the GPL.
+ *
+ * This module implements the specific path activation code for
+ * HP StorageWorks and FSC FibreCat Asymmetric (Active/Passive)
+ * storage arrays.
+ * These storage arrays have controller-based failover, not
+ * LUN-based failover.  However, LUN-based failover is the design
+ * of dm-multipath. Thus, this module is written for LUN-based failover.
+ */
+#include <linux/blkdev.h>
+#include <linux/list.h>
+#include <linux/types.h>
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+
+#include "dm.h"
+#include "dm-hw-handler.h"
+
+#define DM_MSG_PREFIX "multipath hp_sw"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: path activation request
+ * @error: scsi-ml error
+ *
+ *  Check sense data, free request structure, and notify dm that
+ *  pg initialization has completed.
+ *
+ * Context: scsi-ml softirq
+ *
+ * Possible optimizations
+ * 1. Actually check sense data for retryable error (e.g. NOT_READY)
+ */
+static void hp_sw_end_io(struct request *req, int error)
+{
+	struct dm_path *path = req->end_io_data;
+	unsigned err_flags;
+
+	if (!error) {
+		err_flags = 0;
+		DMDEBUG("%s path activation command - success",
+			path->dev->name);
+	} else {
+		DMWARN("%s path activation command - error=0x%x",
+		       path->dev->name, error);
+		err_flags = MP_FAIL_PATH;
+	}
+
+	req->end_io_data = NULL;
+	__blk_put_request(req->q, req);
+	dm_pg_init_complete(path, err_flags);
+}
+
+/**
+ * hp_sw_get_request - Allocate an HP specific path activation request
+ * @path: path on which request will be sent (needed for request queue)
+ *
+ * The START command is used for path activation request.
+ * These arrays are controller-based failover, not LUN based.
+ * One START command issued to a single path will fail over all
+ * LUNs for the same controller.
+ *
+ * Possible optimizations
+ * 1. Make timeout configurable
+ * 2. Preallocate request
+ */
+static struct request *hp_sw_get_request(struct dm_path *path)
+{
+	struct request *req;
+	struct block_device *bdev = path->dev->bdev;
+	struct request_queue *q = bdev_get_queue(bdev);
+	struct hp_sw_context *h = path->hwhcontext;
+
+	req = blk_get_request(q, WRITE, GFP_NOIO);
+	if (!req)
+		goto out;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io_data = path;
+	req->sense = h->sense;
+	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
+
+	memset(&req->cmd, 0, BLK_MAX_CDB);
+	req->cmd[0] = START_STOP;
+	req->cmd[4] = 1;
+	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
+out:
+	return req;
+}
+
+/**
+ * hp_sw_pg_init - HP path activation implementation.
+ * @hwh: hardware handler specific data
+ * @bypassed: unused; is the path group bypassed? (see dm-mpath.c)
+ * @path: path to send initialization command
+ *
+ * Send an HP-specific path activation command on 'path'.
+ * Do not try to optimize in any way, just send the activation command.
+ * More than one path activation command may be sent to the same controller.
+ * This seems to work fine for basic failover support.
+ *
+ * Possible optimizations
+ * 1. Detect an in-progress activation request and avoid submitting another one
+ * 2. Model the controller and only send a single activation request at a time
+ * 3. Determine the state of a path before sending an activation request
+ *
+ * Context: kmpathd (see process_queued_ios() in dm-mpath.c)
+ */
+static void hp_sw_pg_init(struct hw_handler *hwh, unsigned bypassed,
+			  struct dm_path *path)
+{
+	struct request *req;
+	struct hp_sw_context *h;
+
+	path->hwhcontext = hwh->context;
+	h = hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("%s path activation command - allocation fail",
+		      path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("%s path activation command - sent", path->dev->name);
+
+	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
+	return;
+
+fail:
+	dm_pg_init_complete(path, MP_FAIL_PATH);
+}
+
+static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
+{
+	struct hp_sw_context *h;
+
+	h = kmalloc(sizeof(*h), GFP_KERNEL);
+	if (!h)
+		return -ENOMEM;
+	hwh->context = h;
+	return 0;
+}
+
+static void hp_sw_destroy(struct hw_handler *hwh)
+{
+	struct hp_sw_context *h = hwh->context;
+
+	kfree(h);
+}
+
+static struct hw_handler_type hp_sw_hwh = {
+	.name = "hp_sw",
+	.module = THIS_MODULE,
+	.create = hp_sw_create,
+	.destroy = hp_sw_destroy,
+	.pg_init = hp_sw_pg_init,
+};
+
+static int __init hp_sw_init(void)
+{
+	int r;
+
+	r = dm_register_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("register failed %d", r);
+	else
+		DMINFO("version 0.0.2 loaded");
+
+	return r;
+}
+
+static void __exit hp_sw_exit(void)
+{
+	int r;
+
+	r = dm_unregister_hw_handler(&hp_sw_hwh);
+	if (r < 0)
+		DMERR("unregister failed %d", r);
+}
+
+module_init(hp_sw_init);
+module_exit(hp_sw_exit);
+
+MODULE_DESCRIPTION("HP StorageWorks and FSC FibreCat (A/P) support for dm-multipath");
+MODULE_AUTHOR("Mike Christie <michaelc@cs.wisc.edu>");
+MODULE_LICENSE("GPL");
Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/Kconfig
+++ linux-2.6.23-rc1/drivers/md/Kconfig
@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 
+config DM_MULTIPATH_HP
+        tristate "HP MSA multipath support (EXPERIMENTAL)"
+        depends on DM_MULTIPATH && BLK_DEV_DM && EXPERIMENTAL
+        ---help---
+          Multipath support for HP MSA (Active/Passive) series hardware.
+
 config DM_DELAY
 	tristate "I/O delaying target (EXPERIMENTAL)"
 	depends on BLK_DEV_DM && EXPERIMENTAL

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Chandra Seetharaman]

On Mon, 2007-07-30 at 16:08 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > Hi Dave,
> > 
> > some coding style related comments (below).
> > 
> 
> 
> >> +	if (!error) {
> >> +		err_flags = 0;
> >> +		DMDEBUG("%s path activation command - success",
> >> +		       	path->dev->name);
> > 
> > Mixed use of space and tab for indentation (many other places too).
> > 
> 
> Where is that rule for this type of plcae?

Documentation/CodingStyle:
"Outside of comments, documentation and except in Kconfig, spaces are
never used for indentation, and the above example is deliberately
broken."

> 
> I think that is fine in those types of places. In fact I would leave it 
> with mixed because that is how the rest of the dm code does it.
> 
> 
> >> +	memset(&req->cmd, 0, BLK_MAX_CDB);
> >> +	req->cmd[0] = START_STOP;
> >> +	req->cmd[4] = 1;
> >> +	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> >> + out:
> > 
> > I think there will be no space before the label (one more below).
> > 
> 
> Either is normally fine, but in this case to fit with the other dm code 
> that is best.

It is not explicitly stated in CodingStyle. But, the example under
"goto" section does have the label start at column 0 (that is why I said
"I think" :).


-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Mike Christie]

Chandra Seetharaman wrote:
> On Mon, 2007-07-30 at 16:08 -0500, Mike Christie wrote:
>> Chandra Seetharaman wrote:
>>> Hi Dave,
>>>
>>> some coding style related comments (below).
>>>
>>
>>>> +	if (!error) {
>>>> +		err_flags = 0;
>>>> +		DMDEBUG("%s path activation command - success",
>>>> +		       	path->dev->name);
>>> Mixed use of space and tab for indentation (many other places too).
>>>
>> Where is that rule for this type of plcae?
> 
> Documentation/CodingStyle:
> "Outside of comments, documentation and except in Kconfig, spaces are
> never used for indentation, and the above example is deliberately
> broken."

Ah I see. I still think it is ok for this type of case though. Well, the 
above chunk of code should actually be tabbed because it lines up 
perfectly (I think there are 8 spaces in the above snippet), but some of 
the other places are not. checkpatch.pl did not care about the others 
and it looks like reviewers like Andrew Morton did not care about some 
of the wierder spacing used in dm-mpath-rdac which did not fit the 
coding style doc or the existing code :)

If we are getting all picky about that coding style I guess Dave should 
run scripts/checkpatch.pl over his patches, because I think there are 
some other issues.

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Chandra Seetharaman]

On Mon, 2007-07-30 at 22:35 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > On Mon, 2007-07-30 at 16:08 -0500, Mike Christie wrote:
> >> Chandra Seetharaman wrote:
> >>> Hi Dave,
> >>>
> >>> some coding style related comments (below).
> >>>
> >>
> >>>> +	if (!error) {
> >>>> +		err_flags = 0;
> >>>> +		DMDEBUG("%s path activation command - success",
> >>>> +		       	path->dev->name);
> >>> Mixed use of space and tab for indentation (many other places too).
> >>>
> >> Where is that rule for this type of plcae?
> > 
> > Documentation/CodingStyle:
> > "Outside of comments, documentation and except in Kconfig, spaces are
> > never used for indentation, and the above example is deliberately
> > broken."
> 
> Ah I see. I still think it is ok for this type of case though. Well, the 

I see what you mean and agree with you.

> above chunk of code should actually be tabbed because it lines up 
> perfectly (I think there are 8 spaces in the above snippet), but some of 
> the other places are not. checkpatch.pl did not care about the others 
> and it looks like reviewers like Andrew Morton did not care about some 
> of the wierder spacing used in dm-mpath-rdac which did not fit the 
> coding style doc or the existing code :)

I will check it out

> 
> If we are getting all picky about that coding style I guess Dave should 

No, I am not getting all picky :). Was trying to help to avoid some
churn when sent to mainline.

> run scripts/checkpatch.pl over his patches, because I think there are 
> some other issues.
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).

[Mike Christie]

Chandra Seetharaman wrote:
>> If we are getting all picky about that coding style I guess Dave should 
> 
> No, I am not getting all picky :). Was trying to help to avoid some
> churn when sent to mainline.
> 

I agree. I should have added a smiley face when I wrote that sentence. I 
did not mean it like how it came out.

[patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[dwysocha]

[-- Attachment #1: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/plain, Size: 5358 bytes --]

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"



Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries)
+		m->pg_init_required = 1;
+	else
+		fail_path(pgpath);
+	spin_unlock_irqrestore(&m->lock, flags);
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

-- 

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Mike Christie]

dwysocha@redhat.com wrote:


looks ok

Acked-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 10:20 -0500, Mike Christie wrote:
> dwysocha@redhat.com wrote:
> 
> 
> looks ok
> 
> Acked-by: Mike Christie <michaelc@cs.wisc.edu>
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel

There was actually a locking bug in the retry_pg() function (need to
drop m->lock before calling fail_path, since fail_path grabs m->lock).
I found this after realizing my original tests did not exercise the
retry path very well and did some more thorough tests.  Attached patch
fixes it.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5582 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +996,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1032,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1189,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Chandra Seetharaman]

On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> core.  The flag is a generic one, but in this instance we use it to flag
> cases where we must retry a pg_init command.  The patch is useful for cases
> where a hw handler sends a path initialization command to the storage and
> it sees the command complete with an error code indicating the command
> should be retried.  In this case, the hardware handler should call 
> dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> that the dm-mpath core will actually retry the pg_init.  The number of actual
> retries is governed by the multipath feature argument "pg_init_retries".  
> Once the dm-mpath core has retried the command "pg_init_retries" times
> without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> cause the path to be failed via fail_path().  To specify a value of
> pg_init_retries, add a line similar to the following in the 'device' section
> of your /etc/multipath.conf file:
> features                "2 pg_init_retries 7"
> 
> 
> 
> Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
>  #define MP_FAIL_PATH 1
>  #define MP_BYPASS_PG 2
>  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> +#define MP_RETRY 8
> 
>  #endif
> Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> @@ -75,6 +75,8 @@ struct multipath {
>  	unsigned queue_io;		/* Must we queue all I/O? */
>  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
>  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> +	unsigned pg_init_count;         /* Number of times pg_init called */
> 
>  	struct work_struct process_queued_ios;
>  	struct bio_list queued_ios;
> @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
>  	if (hwh->type && hwh->type->pg_init) {
>  		m->pg_init_required = 1;
>  		m->queue_io = 1;
> +		m->pg_init_count = 0;
>  	} else {
>  		m->pg_init_required = 0;
>  		m->queue_io = 0;
> @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
>  		must_queue = 0;
> 
>  	if (m->pg_init_required && !m->pg_init_in_progress) {
> +		m->pg_init_count++;
>  		m->pg_init_required = 0;
>  		m->pg_init_in_progress = 1;
>  		init_required = 1;
> @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
>  	int r;
>  	unsigned argc;
>  	struct dm_target *ti = m->ti;
> +	char *name;
> 
>  	static struct param _params[] = {
> -		{0, 1, "invalid number of feature args"},
> +		{0, 4, "invalid number of feature args"},

Isn't it "3" (instead of "4") ?

> +		{0, 50, "invalid number of pg_init retries"},
>  	};
> 
>  	r = read_param(_params, shift(as), &argc, &ti->error);
> @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
>  	if (!argc)
>  		return 0;
> 
> -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> -		return queue_if_no_path(m, 1, 0);
> -	else {
> -		ti->error = "Unrecognised multipath feature request";
> -		return -EINVAL;
> +	while (argc && !r) {
> +		name = shift(as);
> +		argc--;
> +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> +			r = queue_if_no_path(m, 1, 0);
> +			DMDEBUG("setting queue_if_no_path");

Shouldn't this DEBUG be printed only when r == 0 ?

> +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> +			   (argc >= 1)) {

mixed use of space/tab.
> +			r = read_param(_params + 1, shift(as),
> +					&m->pg_init_retries, &ti->error);
> +			argc--;
> +			DMDEBUG("setting pg_init_retries to %u",
> +				m->pg_init_retries);

Shouldn't this DEBUG be printed only when r == 0 ?
> +		} else {
> +			ti->error = "Unrecognised multipath feature request";
> +			return -EINVAL;
> +		}
>  	}
> +
> +	return r;
>  }
> 
>  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
>  }
> 
>  /*
> + * Retry pg_init on the same path group and path
> + */
> +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&m->lock, flags);
> +	if (m->pg_init_count <= m->pg_init_retries)
> +		m->pg_init_required = 1;
> +	else
> +		fail_path(pgpath);
> +	spin_unlock_irqrestore(&m->lock, flags);
> +}
> +
> +/*
>   * pg_init must call this when it has completed its initialisation
>   */
>  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
>  	if (err_flags & MP_BYPASS_PG)
>  		bypass_pg(m, pg, 1);
> 
> +	if (err_flags & MP_RETRY)
> +		retry_pg(m, pgpath);
> +
>  	spin_lock_irqsave(&m->lock, flags);
> -	if (err_flags) {
> +	if (err_flags & ~MP_RETRY) {
>  		m->current_pgpath = NULL;
>  		m->current_pg = NULL;
>  	} else if (!m->pg_init_required)
> @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
>  	/* Features */
>  	if (type == STATUSTYPE_INFO)
>  		DMEMIT("1 %u ", m->queue_size);
> -	else if (m->queue_if_no_path)
> +	else if (m->queue_if_no_path && !m->pg_init_retries)
>  		DMEMIT("1 queue_if_no_path ");
> +	else if (!m->queue_if_no_path && m->pg_init_retries)
> +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> +	else if (m->queue_if_no_path && m->pg_init_retries)
> +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> +			m->pg_init_retries);
>  	else
>  		DMEMIT("0 ");
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> > This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> > core.  The flag is a generic one, but in this instance we use it to flag
> > cases where we must retry a pg_init command.  The patch is useful for cases
> > where a hw handler sends a path initialization command to the storage and
> > it sees the command complete with an error code indicating the command
> > should be retried.  In this case, the hardware handler should call 
> > dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> > to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> > that the dm-mpath core will actually retry the pg_init.  The number of actual
> > retries is governed by the multipath feature argument "pg_init_retries".  
> > Once the dm-mpath core has retried the command "pg_init_retries" times
> > without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> > cause the path to be failed via fail_path().  To specify a value of
> > pg_init_retries, add a line similar to the following in the 'device' section
> > of your /etc/multipath.conf file:
> > features                "2 pg_init_retries 7"
> > 
> > 
> > 
> > Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> > +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
> >  #define MP_FAIL_PATH 1
> >  #define MP_BYPASS_PG 2
> >  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> > +#define MP_RETRY 8
> > 
> >  #endif
> > Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> > +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > @@ -75,6 +75,8 @@ struct multipath {
> >  	unsigned queue_io;		/* Must we queue all I/O? */
> >  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
> >  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> > +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> > +	unsigned pg_init_count;         /* Number of times pg_init called */
> > 
> >  	struct work_struct process_queued_ios;
> >  	struct bio_list queued_ios;
> > @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
> >  	if (hwh->type && hwh->type->pg_init) {
> >  		m->pg_init_required = 1;
> >  		m->queue_io = 1;
> > +		m->pg_init_count = 0;
> >  	} else {
> >  		m->pg_init_required = 0;
> >  		m->queue_io = 0;
> > @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
> >  		must_queue = 0;
> > 
> >  	if (m->pg_init_required && !m->pg_init_in_progress) {
> > +		m->pg_init_count++;
> >  		m->pg_init_required = 0;
> >  		m->pg_init_in_progress = 1;
> >  		init_required = 1;
> > @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
> >  	int r;
> >  	unsigned argc;
> >  	struct dm_target *ti = m->ti;
> > +	char *name;
> > 
> >  	static struct param _params[] = {
> > -		{0, 1, "invalid number of feature args"},
> > +		{0, 4, "invalid number of feature args"},
> 
> Isn't it "3" (instead of "4") ?
> 
> > +		{0, 50, "invalid number of pg_init retries"},
> >  	};
> > 
> >  	r = read_param(_params, shift(as), &argc, &ti->error);
> > @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
> >  	if (!argc)
> >  		return 0;
> > 
> > -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> > -		return queue_if_no_path(m, 1, 0);
> > -	else {
> > -		ti->error = "Unrecognised multipath feature request";
> > -		return -EINVAL;
> > +	while (argc && !r) {
> > +		name = shift(as);
> > +		argc--;
> > +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> > +			r = queue_if_no_path(m, 1, 0);
> > +			DMDEBUG("setting queue_if_no_path");
> 
> Shouldn't this DEBUG be printed only when r == 0 ?
> 
> > +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> > +			   (argc >= 1)) {
> 
> mixed use of space/tab.
> > +			r = read_param(_params + 1, shift(as),
> > +					&m->pg_init_retries, &ti->error);
> > +			argc--;
> > +			DMDEBUG("setting pg_init_retries to %u",
> > +				m->pg_init_retries);
> 
> Shouldn't this DEBUG be printed only when r == 0 ?
> > +		} else {
> > +			ti->error = "Unrecognised multipath feature request";
> > +			return -EINVAL;
> > +		}
> >  	}
> > +
> > +	return r;
> >  }
> > 
> >  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> > @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
> >  }
> > 
> >  /*
> > + * Retry pg_init on the same path group and path
> > + */
> > +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> > +{
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&m->lock, flags);
> > +	if (m->pg_init_count <= m->pg_init_retries)
> > +		m->pg_init_required = 1;
> > +	else
> > +		fail_path(pgpath);
> > +	spin_unlock_irqrestore(&m->lock, flags);
> > +}
> > +
> > +/*
> >   * pg_init must call this when it has completed its initialisation
> >   */
> >  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> > @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
> >  	if (err_flags & MP_BYPASS_PG)
> >  		bypass_pg(m, pg, 1);
> > 
> > +	if (err_flags & MP_RETRY)
> > +		retry_pg(m, pgpath);
> > +
> >  	spin_lock_irqsave(&m->lock, flags);
> > -	if (err_flags) {
> > +	if (err_flags & ~MP_RETRY) {
> >  		m->current_pgpath = NULL;
> >  		m->current_pg = NULL;
> >  	} else if (!m->pg_init_required)
> > @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
> >  	/* Features */
> >  	if (type == STATUSTYPE_INFO)
> >  		DMEMIT("1 %u ", m->queue_size);
> > -	else if (m->queue_if_no_path)
> > +	else if (m->queue_if_no_path && !m->pg_init_retries)
> >  		DMEMIT("1 queue_if_no_path ");
> > +	else if (!m->queue_if_no_path && m->pg_init_retries)
> > +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> > +	else if (m->queue_if_no_path && m->pg_init_retries)
> > +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> > +			m->pg_init_retries);
> >  	else
> >  		DMEMIT("0 ");
> > 
> > 

The attached patch should address your comments.  I removed the DMDEBUG
statements as they did not seem too useful beyond basic tests.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5468 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;       /* Number of times to retry pg_init */
+	unsigned pg_init_count;         /* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+				(argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

CVS pull of device mapper

[Wood, Brian J]

Hello everyone, I've just built a test system with Fedora 7 and am
having a problem with running dmeventd after getting the latest pull
from the CVS tree. From the "make" and "make install" commands
everything is error free, but when I run the dmeventd daemon I get this
error:

/sbin/dmeventd: error while loading shared libraries:
libdevmapper-event.so.1.02: cannot open shared object file: No such file
or directory

I've looked in /lib and there is an instance of
libdevmapper-event.so.1.02 (and a symbolic link named
libdevmapper-event.so), so I'm confused as to why the daemon can't find
them? When I run configure the only parameters I'm passing in are
"--enable-debug" and "--enable-dmeventd"

Thank you, 

Brian Wood
Intel Corporation 
Digital Enterprise Group
Manageability & Platform Software Division
brian.j.wood@intel.com

Re: CVS pull of device mapper

[Mike Snitzer]

On 7/30/07, Wood, Brian J <brian.j.wood@intel.com> wrote:
> Hello everyone, I've just built a test system with Fedora 7 and am
> having a problem with running dmeventd after getting the latest pull
> from the CVS tree. From the "make" and "make install" commands
> everything is error free, but when I run the dmeventd daemon I get this
> error:
>
> /sbin/dmeventd: error while loading shared libraries:
> libdevmapper-event.so.1.02: cannot open shared object file: No such file
> or directory
>
> I've looked in /lib and there is an instance of
> libdevmapper-event.so.1.02 (and a symbolic link named
> libdevmapper-event.so), so I'm confused as to why the daemon can't find
> them? When I run configure the only parameters I'm passing in are
> "--enable-debug" and "--enable-dmeventd"

Try using ldconfig (-v can be useful) and ldd /sbin/dmeventd

RE: CVS pull of device mapper

[Wood, Brian J]

Brian Wood
Intel Corporation 
Digital Enterprise Group
Manageability & Platform Software Division
brian.j.wood@intel.com

>-----Original Message-----
>From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
On
>Behalf Of Mike Snitzer
>Sent: Monday, July 30, 2007 2:50 PM
>To: device-mapper development
>Subject: Re: [dm-devel] CVS pull of device mapper
>
>On 7/30/07, Wood, Brian J <brian.j.wood@intel.com> wrote:
>> Hello everyone, I've just built a test system with Fedora 7 and am
>> having a problem with running dmeventd after getting the latest pull
>> from the CVS tree. From the "make" and "make install" commands
>> everything is error free, but when I run the dmeventd daemon I get
this
>> error:
>>
>> /sbin/dmeventd: error while loading shared libraries:
>> libdevmapper-event.so.1.02: cannot open shared object file: No such
file
>> or directory
>>
>> I've looked in /lib and there is an instance of
>> libdevmapper-event.so.1.02 (and a symbolic link named
>> libdevmapper-event.so), so I'm confused as to why the daemon can't
find
>> them? When I run configure the only parameters I'm passing in are
>> "--enable-debug" and "--enable-dmeventd"
>
>Try using ldconfig (-v can be useful) and ldd /sbin/dmeventd
>

I exported the variable LD_DEBUG=all and found that it's searching the
/lib64 libraries for the .so (which it should since this is a F7 x86_64
load). Here is a dump of the output:

/sbin/dmeventd
      3127:
      3127:     file=libdl.so.2 [0];  needed by /sbin/dmeventd [0]
      3127:     find library=libdl.so.2 [0]; searching
      3127:      search cache=/etc/ld.so.cache
      3127:       trying file=/lib64/libdl.so.2
      3127:
      3127:     file=libdl.so.2 [0];  generating link map
      3127:       dynamic: 0x0000003c7ba02da0  base: 0x0000000000000000
size: 0x0000000000203100
      3127:         entry: 0x0000003c7b800e10  phdr: 0x0000003c7b800040
phnum:                  9
      3127:
      3127:
      3127:     file=libdevmapper.so.1.02 [0];  needed by /sbin/dmeventd
[0]
      3127:     find library=libdevmapper.so.1.02 [0]; searching
      3127:      search cache=/etc/ld.so.cache
      3127:       trying file=/lib64/libdevmapper.so.1.02
      3127:
      3127:     file=libdevmapper.so.1.02 [0];  generating link map
      3127:       dynamic: 0x0000003c83011038  base: 0x0000000000000000
size: 0x00000000002128c8
      3127:         entry: 0x0000003c82e039f0  phdr: 0x0000003c82e00040
phnum:                  5
      3127:
      3127:
      3127:     file=libpthread.so.0 [0];  needed by /sbin/dmeventd [0]
      3127:     find library=libpthread.so.0 [0]; searching
      3127:      search cache=/etc/ld.so.cache
      3127:       trying file=/lib64/libpthread.so.0
      3127:
      3127:     file=libpthread.so.0 [0];  generating link map
      3127:       dynamic: 0x0000003c7d614db0  base: 0x0000000000000000
size: 0x0000000000219370
      3127:         entry: 0x0000003c7d405720  phdr: 0x0000003c7d400040
phnum:                  9
      3127:
      3127:
      3127:     file=libdevmapper-event.so.1.02 [0];  needed by
/sbin/dmeventd [0]
      3127:     find library=libdevmapper-event.so.1.02 [0]; searching
      3127:      search cache=/etc/ld.so.cache
      3127:      search
path=/lib64/tls/x86_64:/lib64/tls:/lib64/x86_64:/lib64:/usr/lib64/tls/x8
6_64:/usr/lib64/tls:/usr/lib64/x86_64:/usr/lib64(system search path)
      3127:       trying
file=/lib64/tls/x86_64/libdevmapper-event.so.1.02
      3127:       trying file=/lib64/tls/libdevmapper-event.so.1.02
      3127:       trying file=/lib64/x86_64/libdevmapper-event.so.1.02
      3127:       trying file=/lib64/libdevmapper-event.so.1.02
      3127:       trying
file=/usr/lib64/tls/x86_64/libdevmapper-event.so.1.02
      3127:       trying file=/usr/lib64/tls/libdevmapper-event.so.1.02
      3127:       trying
file=/usr/lib64/x86_64/libdevmapper-event.so.1.02
      3127:       trying file=/usr/lib64/libdevmapper-event.so.1.02
      3127:
/sbin/dmeventd: error while loading shared libraries:
libdevmapper-event.so.1.02: cannot open shared object file: No such file
or directory


One thing I noticed when building/installing was that my .so being put
into into /lib in the stdout messages?

Two questions:

1. How come my ld.so.cache doesn't have this to be searched if the make
file is putting it there?

2. Why didn't this problem occur on my FC6 x86_64 system? Both were just
out of the box systems, so did something change in F7 that I didn't
realize.

Thanks for the help on this puzzling issue :)

>--
>dm-devel mailing list
>dm-devel@redhat.com
>https://www.redhat.com/mailman/listinfo/dm-devel

RE: CVS pull of device mapper

[Wood, Brian J]

>-----Original Message-----
>From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
On
>Behalf Of Wood, Brian J
>Sent: Monday, July 30, 2007 3:21 PM
>To: device-mapper development
>Subject: RE: [dm-devel] CVS pull of device mapper
>
>
>
>Brian Wood
>Intel Corporation
>Digital Enterprise Group
>Manageability & Platform Software Division
>brian.j.wood@intel.com
>
>>-----Original Message-----
>>From: dm-devel-bounces@redhat.com [mailto:dm-devel-bounces@redhat.com]
>On
>>Behalf Of Mike Snitzer
>>Sent: Monday, July 30, 2007 2:50 PM
>>To: device-mapper development
>>Subject: Re: [dm-devel] CVS pull of device mapper
>>
>>On 7/30/07, Wood, Brian J <brian.j.wood@intel.com> wrote:
>>> Hello everyone, I've just built a test system with Fedora 7 and am
>>> having a problem with running dmeventd after getting the latest pull
>>> from the CVS tree. From the "make" and "make install" commands
>>> everything is error free, but when I run the dmeventd daemon I get
>this
>>> error:
>>>
>>> /sbin/dmeventd: error while loading shared libraries:
>>> libdevmapper-event.so.1.02: cannot open shared object file: No such
>file
>>> or directory
>>>
>>> I've looked in /lib and there is an instance of
>>> libdevmapper-event.so.1.02 (and a symbolic link named
>>> libdevmapper-event.so), so I'm confused as to why the daemon can't
>find
>>> them? When I run configure the only parameters I'm passing in are
>>> "--enable-debug" and "--enable-dmeventd"
>>
>>Try using ldconfig (-v can be useful) and ldd /sbin/dmeventd
>>
>
>I exported the variable LD_DEBUG=all and found that it's searching the
>/lib64 libraries for the .so (which it should since this is a F7 x86_64
>load). Here is a dump of the output:
>
>/sbin/dmeventd
>      3127:
>      3127:     file=libdl.so.2 [0];  needed by /sbin/dmeventd [0]
>      3127:     find library=libdl.so.2 [0]; searching
>      3127:      search cache=/etc/ld.so.cache
>      3127:       trying file=/lib64/libdl.so.2
>      3127:
>      3127:     file=libdl.so.2 [0];  generating link map
>      3127:       dynamic: 0x0000003c7ba02da0  base: 0x0000000000000000
>size: 0x0000000000203100
>      3127:         entry: 0x0000003c7b800e10  phdr: 0x0000003c7b800040
>phnum:                  9
>      3127:
>      3127:
>      3127:     file=libdevmapper.so.1.02 [0];  needed by
/sbin/dmeventd
>[0]
>      3127:     find library=libdevmapper.so.1.02 [0]; searching
>      3127:      search cache=/etc/ld.so.cache
>      3127:       trying file=/lib64/libdevmapper.so.1.02
>      3127:
>      3127:     file=libdevmapper.so.1.02 [0];  generating link map
>      3127:       dynamic: 0x0000003c83011038  base: 0x0000000000000000
>size: 0x00000000002128c8
>      3127:         entry: 0x0000003c82e039f0  phdr: 0x0000003c82e00040
>phnum:                  5
>      3127:
>      3127:
>      3127:     file=libpthread.so.0 [0];  needed by /sbin/dmeventd [0]
>      3127:     find library=libpthread.so.0 [0]; searching
>      3127:      search cache=/etc/ld.so.cache
>      3127:       trying file=/lib64/libpthread.so.0
>      3127:
>      3127:     file=libpthread.so.0 [0];  generating link map
>      3127:       dynamic: 0x0000003c7d614db0  base: 0x0000000000000000
>size: 0x0000000000219370
>      3127:         entry: 0x0000003c7d405720  phdr: 0x0000003c7d400040
>phnum:                  9
>      3127:
>      3127:
>      3127:     file=libdevmapper-event.so.1.02 [0];  needed by
>/sbin/dmeventd [0]
>      3127:     find library=libdevmapper-event.so.1.02 [0]; searching
>      3127:      search cache=/etc/ld.so.cache
>      3127:      search
>path=/lib64/tls/x86_64:/lib64/tls:/lib64/x86_64:/lib64:/usr/lib64/tls/x
8
>6_64:/usr/lib64/tls:/usr/lib64/x86_64:/usr/lib64(system search path)
>      3127:       trying
>file=/lib64/tls/x86_64/libdevmapper-event.so.1.02
>      3127:       trying file=/lib64/tls/libdevmapper-event.so.1.02
>      3127:       trying file=/lib64/x86_64/libdevmapper-event.so.1.02
>      3127:       trying file=/lib64/libdevmapper-event.so.1.02
>      3127:       trying
>file=/usr/lib64/tls/x86_64/libdevmapper-event.so.1.02
>      3127:       trying file=/usr/lib64/tls/libdevmapper-event.so.1.02
>      3127:       trying
>file=/usr/lib64/x86_64/libdevmapper-event.so.1.02
>      3127:       trying file=/usr/lib64/libdevmapper-event.so.1.02
>      3127:
>/sbin/dmeventd: error while loading shared libraries:
>libdevmapper-event.so.1.02: cannot open shared object file: No such
file
>or directory
>
>
>One thing I noticed when building/installing was that my .so is being
put
> into /lib in the stdout messages?
>
>Two questions:
>
>1. How come my ld.so.cache doesn't have this to be searched if the make
>file is putting it there?
>
>2. Why didn't this problem occur on my FC6 x86_64 system? Both were
just
>out of the box systems, so did something change in F7 that I didn't
>realize.
>
>Thanks for the help on this puzzling issue :)
>

In digging in the .configure file I see that the variable "libdir" is
set to /lib if "exec_prefix" is sent in empty. Being completely new to
Linux .configure files (I hope this assumption is on the right track),
but shouldn't there be a check here if we're on an x86_64 system to make
sure the .so is put into the right search directory? Or is there
somewhere that ldconfig should be called to setup the cache? This is
actually pretty interesting and I would love to find out how to diagnose
this if someone is willing to explain it to me :)

Thank you

>>--
>>dm-devel mailing list
>>dm-devel@redhat.com
>>https://www.redhat.com/mailman/listinfo/dm-devel
>
>--
>dm-devel mailing list
>dm-devel@redhat.com
>https://www.redhat.com/mailman/listinfo/dm-devel

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Chandra Seetharaman]

Definitions of pg_init_retries and pg_init_count still uses spaces
(instead of tabs, which is the case with rest of the definitions around.

Other than that it looks good.

thanks

chandra
On Mon, 2007-07-30 at 14:54 -0400, Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:15 -0700, Chandra Seetharaman wrote:
> > On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > > plain text document attachment (dm-mpath-add-retry-pg-init.patch)
> > > This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
> > > core.  The flag is a generic one, but in this instance we use it to flag
> > > cases where we must retry a pg_init command.  The patch is useful for cases
> > > where a hw handler sends a path initialization command to the storage and
> > > it sees the command complete with an error code indicating the command
> > > should be retried.  In this case, the hardware handler should call 
> > > dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
> > > to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
> > > that the dm-mpath core will actually retry the pg_init.  The number of actual
> > > retries is governed by the multipath feature argument "pg_init_retries".  
> > > Once the dm-mpath core has retried the command "pg_init_retries" times
> > > without success, a subsequent dm_pg_init_complete() with MP_RETRY will
> > > cause the path to be failed via fail_path().  To specify a value of
> > > pg_init_retries, add a line similar to the following in the 'device' section
> > > of your /etc/multipath.conf file:
> > > features                "2 pg_init_retries 7"
> > > 
> > > 
> > > 
> > > Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
> > > +++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
> > > @@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
> > >  #define MP_FAIL_PATH 1
> > >  #define MP_BYPASS_PG 2
> > >  #define MP_ERROR_IO  4	/* Don't retry this I/O */
> > > +#define MP_RETRY 8
> > > 
> > >  #endif
> > > Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
> > > +++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
> > > @@ -75,6 +75,8 @@ struct multipath {
> > >  	unsigned queue_io;		/* Must we queue all I/O? */
> > >  	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
> > >  	unsigned saved_queue_if_no_path;/* Saved state during suspension */
> > > +	unsigned pg_init_retries;       /* Number of times to retry pg_init */
> > > +	unsigned pg_init_count;         /* Number of times pg_init called */
> > > 
> > >  	struct work_struct process_queued_ios;
> > >  	struct bio_list queued_ios;
> > > @@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
> > >  	if (hwh->type && hwh->type->pg_init) {
> > >  		m->pg_init_required = 1;
> > >  		m->queue_io = 1;
> > > +		m->pg_init_count = 0;
> > >  	} else {
> > >  		m->pg_init_required = 0;
> > >  		m->queue_io = 0;
> > > @@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
> > >  		must_queue = 0;
> > > 
> > >  	if (m->pg_init_required && !m->pg_init_in_progress) {
> > > +		m->pg_init_count++;
> > >  		m->pg_init_required = 0;
> > >  		m->pg_init_in_progress = 1;
> > >  		init_required = 1;
> > > @@ -689,9 +693,11 @@ static int parse_features(struct arg_set
> > >  	int r;
> > >  	unsigned argc;
> > >  	struct dm_target *ti = m->ti;
> > > +	char *name;
> > > 
> > >  	static struct param _params[] = {
> > > -		{0, 1, "invalid number of feature args"},
> > > +		{0, 4, "invalid number of feature args"},
> > 
> > Isn't it "3" (instead of "4") ?
> > 
> > > +		{0, 50, "invalid number of pg_init retries"},
> > >  	};
> > > 
> > >  	r = read_param(_params, shift(as), &argc, &ti->error);
> > > @@ -701,12 +707,26 @@ static int parse_features(struct arg_set
> > >  	if (!argc)
> > >  		return 0;
> > > 
> > > -	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
> > > -		return queue_if_no_path(m, 1, 0);
> > > -	else {
> > > -		ti->error = "Unrecognised multipath feature request";
> > > -		return -EINVAL;
> > > +	while (argc && !r) {
> > > +		name = shift(as);
> > > +		argc--;
> > > +		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
> > > +			r = queue_if_no_path(m, 1, 0);
> > > +			DMDEBUG("setting queue_if_no_path");
> > 
> > Shouldn't this DEBUG be printed only when r == 0 ?
> > 
> > > +		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
> > > +			   (argc >= 1)) {
> > 
> > mixed use of space/tab.
> > > +			r = read_param(_params + 1, shift(as),
> > > +					&m->pg_init_retries, &ti->error);
> > > +			argc--;
> > > +			DMDEBUG("setting pg_init_retries to %u",
> > > +				m->pg_init_retries);
> > 
> > Shouldn't this DEBUG be printed only when r == 0 ?
> > > +		} else {
> > > +			ti->error = "Unrecognised multipath feature request";
> > > +			return -EINVAL;
> > > +		}
> > >  	}
> > > +
> > > +	return r;
> > >  }
> > > 
> > >  static int multipath_ctr(struct dm_target *ti, unsigned int argc,
> > > @@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
> > >  }
> > > 
> > >  /*
> > > + * Retry pg_init on the same path group and path
> > > + */
> > > +static void retry_pg(struct multipath *m, struct pgpath *pgpath)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&m->lock, flags);
> > > +	if (m->pg_init_count <= m->pg_init_retries)
> > > +		m->pg_init_required = 1;
> > > +	else
> > > +		fail_path(pgpath);
> > > +	spin_unlock_irqrestore(&m->lock, flags);
> > > +}
> > > +
> > > +/*
> > >   * pg_init must call this when it has completed its initialisation
> > >   */
> > >  void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
> > > @@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
> > >  	if (err_flags & MP_BYPASS_PG)
> > >  		bypass_pg(m, pg, 1);
> > > 
> > > +	if (err_flags & MP_RETRY)
> > > +		retry_pg(m, pgpath);
> > > +
> > >  	spin_lock_irqsave(&m->lock, flags);
> > > -	if (err_flags) {
> > > +	if (err_flags & ~MP_RETRY) {
> > >  		m->current_pgpath = NULL;
> > >  		m->current_pg = NULL;
> > >  	} else if (!m->pg_init_required)
> > > @@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
> > >  	/* Features */
> > >  	if (type == STATUSTYPE_INFO)
> > >  		DMEMIT("1 %u ", m->queue_size);
> > > -	else if (m->queue_if_no_path)
> > > +	else if (m->queue_if_no_path && !m->pg_init_retries)
> > >  		DMEMIT("1 queue_if_no_path ");
> > > +	else if (!m->queue_if_no_path && m->pg_init_retries)
> > > +		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
> > > +	else if (m->queue_if_no_path && m->pg_init_retries)
> > > +		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
> > > +			m->pg_init_retries);
> > >  	else
> > >  		DMEMIT("0 ");
> > > 
> > > 
> 
> The attached patch should address your comments.  I removed the DMDEBUG
> statements as they did not seem too useful beyond basic tests.
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Mike Christie]

Chandra Seetharaman wrote:
> Definitions of pg_init_retries and pg_init_count still uses spaces
> (instead of tabs, which is the case with rest of the definitions around.
> 

I agree with Chandra on that one though.

Re: [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

[Dave Wysochanski]

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

On Mon, 2007-07-30 at 16:11 -0500, Mike Christie wrote:
> Chandra Seetharaman wrote:
> > Definitions of pg_init_retries and pg_init_count still uses spaces
> > (instead of tabs, which is the case with rest of the definitions around.
> > 
> 
> I agree with Chandra on that one though.

See attached.



[-- Attachment #2: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/x-patch, Size: 5515 bytes --]

Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.

This patch adds a MP_RETRY flag and "pg_init_retries" feature to dm-multipath
core.  The flag is a generic one, but in this instance we use it to flag
cases where we must retry a pg_init command.  The patch is useful for cases
where a hw handler sends a path initialization command to the storage and
it sees the command complete with an error code indicating the command
should be retried.  In this case, the hardware handler should call 
dm_pg_init_complete() with MP_RETRY set in the err_flags, and this suggests
to the dm-mpath core to retry the pg_init.  However, it is not a guarantee
that the dm-mpath core will actually retry the pg_init.  The number of actual
retries is governed by the multipath feature argument "pg_init_retries".  
Once the dm-mpath core has retried the command "pg_init_retries" times
without success, a subsequent dm_pg_init_complete() with MP_RETRY will
cause the path to be failed via fail_path().  To specify a value of
pg_init_retries, add a line similar to the following in the 'device' section
of your /etc/multipath.conf file:
features                "2 pg_init_retries 7"

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Mike Christie <michaelc@cs.wisc.edu>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hw-handler.h
+++ linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
@@ -58,5 +58,6 @@ unsigned dm_scsi_err_handler(struct hw_h
 #define MP_FAIL_PATH 1
 #define MP_BYPASS_PG 2
 #define MP_ERROR_IO  4	/* Don't retry this I/O */
+#define MP_RETRY 8
 
 #endif
Index: linux-2.6.23-rc1/drivers/md/dm-mpath.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath.c
@@ -75,6 +75,8 @@ struct multipath {
 	unsigned queue_io;		/* Must we queue all I/O? */
 	unsigned queue_if_no_path;	/* Queue I/O if last path fails? */
 	unsigned saved_queue_if_no_path;/* Saved state during suspension */
+	unsigned pg_init_retries;	/* Number of times to retry pg_init */
+	unsigned pg_init_count;		/* Number of times pg_init called */
 
 	struct work_struct process_queued_ios;
 	struct bio_list queued_ios;
@@ -221,6 +223,7 @@ static void __switch_pg(struct multipath
 	if (hwh->type && hwh->type->pg_init) {
 		m->pg_init_required = 1;
 		m->queue_io = 1;
+		m->pg_init_count = 0;
 	} else {
 		m->pg_init_required = 0;
 		m->queue_io = 0;
@@ -424,6 +427,7 @@ static void process_queued_ios(struct wo
 		must_queue = 0;
 
 	if (m->pg_init_required && !m->pg_init_in_progress) {
+		m->pg_init_count++;
 		m->pg_init_required = 0;
 		m->pg_init_in_progress = 1;
 		init_required = 1;
@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 3, "invalid number of feature args"},
+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);
@@ -701,12 +707,23 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))
-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+		} else if (!strnicmp(name, MESG_STR("pg_init_retries")) &&
+			   (argc >= 1)) {
+			r = read_param(_params + 1, shift(as),
+				       &m->pg_init_retries, &ti->error);
+			argc--;
+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,
@@ -976,6 +993,23 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries) {
+		m->pg_init_required = 1;
+		spin_unlock_irqrestore(&m->lock, flags);
+	} else {
+		spin_unlock_irqrestore(&m->lock, flags);
+		fail_path(pgpath);
+	}
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)
@@ -995,8 +1029,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)
@@ -1149,8 +1186,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[dwysocha]

[-- Attachment #1: dm-hp-sw-add-retries-handle-not-ready.patch --]
[-- Type: text/plain, Size: 3820 bytes --]

This patch adds retries to the hp hardware handler, and utilizes the 
MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
pg_init completed with a check condition we just assume we can retry the
pg_init command.  We make this assumption because of incomplete data on
specific check condition code of the HP hardware, and because testing
has shown the HP path initialization command to be idempotent.
The number of times we retry is settable via the "pg_init_retries"
multipath map feature.


Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"
@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.
@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {
@@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);
+			err_flags = MP_RETRY;
+			goto out;
+		}
+		DMWARN("%s path activation fail - error=0x%x",
+			path->dev->name, error);
 		err_flags = MP_FAIL_PATH;
 	}
-
+ out:
 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);
@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 
 	return r;
 }

-- 

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Mike Christie]

dwysocha@redhat.com wrote:


  struct hp_sw_context {
  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
  };


argc does not seem to be used anywhere that I could see.

Also do not forget your signed off line.

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 10:16 -0500, Mike Christie wrote:
> dwysocha@redhat.com wrote:
> 
> 
>   struct hp_sw_context {
>   	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> +	unsigned argc;
>   };
> 
> 
> argc does not seem to be used anywhere that I could see.
> 
> Also do not forget your signed off line.
> 

Indeed, argc was a remnant of prior code that had retries in the
hardware handler.

 



[-- Attachment #2: dm-hp-sw-add-retries-handle-not-ready.patch --]
[-- Type: text/x-patch, Size: 3956 bytes --]

Add retries to hp hardware handler if path initialization command completes
with a check condition.  

This patch adds retries to the hp hardware handler, and utilizes the 
MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
pg_init completed with a check condition we just assume we can retry the
pg_init command.  We make this assumption because of incomplete data on
specific check condition code of the HP hardware, and because testing
has shown the HP path initialization command to be idempotent.
The number of times we retry is settable via the "pg_init_retries"
multipath map feature.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"
@@ -28,6 +29,39 @@ struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.
@@ -39,8 +73,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {
@@ -52,11 +84,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);
+			err_flags = MP_RETRY;
+			goto out;
+		}
+		DMWARN("%s path activation fail - error=0x%x",
+			path->dev->name, error);
 		err_flags = MP_FAIL_PATH;
 	}
-
+ out:
 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);
@@ -134,17 +172,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
@@ -181,7 +218,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 
 	return r;
 }

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Chandra Seetharaman]

On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> plain text document attachment (dm-hp-sw-add-retries-handle-not-
> ready.patch)
> This patch adds retries to the hp hardware handler, and utilizes the 
> MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
> pg_init completed with a check condition we just assume we can retry the
> pg_init command.  We make this assumption because of incomplete data on
> specific check condition code of the HP hardware, and because testing
> has shown the HP path initialization command to be idempotent.
> The number of times we retry is settable via the "pg_init_retries"
> multipath map feature.
> 
> 
> Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> ===================================================================
> --- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
> +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> @@ -18,6 +18,7 @@
>  #include <linux/types.h>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_dbg.h>
> 
>  #include "dm.h"
>  #include "dm-hw-handler.h"
> @@ -26,8 +27,42 @@
> 
>  struct hp_sw_context {
>  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> +	unsigned argc;
>  };
> 
> +/**
> + * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
> + * @req: path activation request
> + *
> + * Examine error codes of request and determine whether the error is retryable.
> + * Some error codes are already retried by scsi-ml (see
> + * scsi_decide_disposition), but some HP specific codes are not.
> + * The intent of this routine is to supply the logic for the HP specific
> + * check conditions.
> + *
> + * Returns:
> + *  1 - command completed with retryable error
> + *  0 - command completed with non-retryable error
> + *
> + * Possible optimizations
> + * 1. More hardware-specific error codes
> + */
> +static int hp_sw_error_is_retryable(struct request *req)
> +{
> +	/*
> +	 * NOT_READY is known to be retryable
> +	 * For now we just dump out the sense data and call it retryable
> +	 */
> +	if (status_byte(req->errors) == CHECK_CONDITION)
> +		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
> +
> +	/*
> +	 * At this point we don't have complete information about all the error
> +	 * codes from this hardware, so we are just conservative and retry
> +	 * when in doubt.
> +	 */
> +	return 1;
> +}
> 
>  /**
>   * hp_sw_end_io - Completion handler for HP path activation.
> @@ -39,8 +74,6 @@ struct hp_sw_context {
>   *
>   * Context: scsi-ml softirq
>   *
> - * Possible optimizations
> - * 1. Actually check sense data for retryable error (e.g. NOT_READY)
>   */
>  static void hp_sw_end_io(struct request *req, int error)
>  {
> @@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
>  		DMDEBUG("%s path activation command - success",
>  		       	path->dev->name);
>  	} else {
> -		DMWARN("path activation command on %s - error=0x%x",
> -		       path->dev->name, error);
> +		if (hp_sw_error_is_retryable(req)) {
> +			DMDEBUG("%s path activation command retry",
> +			       path->dev->name);

mixed use of space and tabs
> +			err_flags = MP_RETRY;
> +			goto out;
> +		}
> +		DMWARN("%s path activation fail - error=0x%x",
> +			path->dev->name, error);
>  		err_flags = MP_FAIL_PATH;
>  	}
> -
> + out:

space in front of label

>  	req->end_io_data = NULL;
>  	__blk_put_request(req->q, req);
>  	dm_pg_init_complete(path, err_flags);
> @@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
>  	if (!req) {
>  		DMERR("%s path activation command allocation fail ",
>  		      path->dev->name);
> -		goto fail;
> +		goto retry;
>  	}
> 
> -	DMDEBUG("path activation command sent on %s",
> -		path->dev->name);
> +	DMDEBUG("%s path activation command sent", path->dev->name);
> 
>  	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
>  	return;
> 
> - fail:
> -	dm_pg_init_complete(path, MP_FAIL_PATH);
> + retry:
> +	dm_pg_init_complete(path, MP_RETRY);
>  }
> 
>  static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> @@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
>  	if (r < 0)
>  		DMERR("register failed %d", r);
>  	else
> -		DMINFO("version 0.0.2 loaded");
> +		DMINFO("version 0.0.3 loaded");
> 
>  	return r;
>  }
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Dave Wysochanski]

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

On Thu, 2007-07-26 at 12:19 -0700, Chandra Seetharaman wrote:
> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > plain text document attachment (dm-hp-sw-add-retries-handle-not-
> > ready.patch)
> > This patch adds retries to the hp hardware handler, and utilizes the 
> > MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
> > pg_init completed with a check condition we just assume we can retry the
> > pg_init command.  We make this assumption because of incomplete data on
> > specific check condition code of the HP hardware, and because testing
> > has shown the HP path initialization command to be idempotent.
> > The number of times we retry is settable via the "pg_init_retries"
> > multipath map feature.
> > 
> > 
> > Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > ===================================================================
> > --- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
> > +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/types.h>
> >  #include <scsi/scsi.h>
> >  #include <scsi/scsi_cmnd.h>
> > +#include <scsi/scsi_dbg.h>
> > 
> >  #include "dm.h"
> >  #include "dm-hw-handler.h"
> > @@ -26,8 +27,42 @@
> > 
> >  struct hp_sw_context {
> >  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> > +	unsigned argc;
> >  };
> > 
> > +/**
> > + * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
> > + * @req: path activation request
> > + *
> > + * Examine error codes of request and determine whether the error is retryable.
> > + * Some error codes are already retried by scsi-ml (see
> > + * scsi_decide_disposition), but some HP specific codes are not.
> > + * The intent of this routine is to supply the logic for the HP specific
> > + * check conditions.
> > + *
> > + * Returns:
> > + *  1 - command completed with retryable error
> > + *  0 - command completed with non-retryable error
> > + *
> > + * Possible optimizations
> > + * 1. More hardware-specific error codes
> > + */
> > +static int hp_sw_error_is_retryable(struct request *req)
> > +{
> > +	/*
> > +	 * NOT_READY is known to be retryable
> > +	 * For now we just dump out the sense data and call it retryable
> > +	 */
> > +	if (status_byte(req->errors) == CHECK_CONDITION)
> > +		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
> > +
> > +	/*
> > +	 * At this point we don't have complete information about all the error
> > +	 * codes from this hardware, so we are just conservative and retry
> > +	 * when in doubt.
> > +	 */
> > +	return 1;
> > +}
> > 
> >  /**
> >   * hp_sw_end_io - Completion handler for HP path activation.
> > @@ -39,8 +74,6 @@ struct hp_sw_context {
> >   *
> >   * Context: scsi-ml softirq
> >   *
> > - * Possible optimizations
> > - * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> >   */
> >  static void hp_sw_end_io(struct request *req, int error)
> >  {
> > @@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
> >  		DMDEBUG("%s path activation command - success",
> >  		       	path->dev->name);
> >  	} else {
> > -		DMWARN("path activation command on %s - error=0x%x",
> > -		       path->dev->name, error);
> > +		if (hp_sw_error_is_retryable(req)) {
> > +			DMDEBUG("%s path activation command retry",
> > +			       path->dev->name);
> 
> mixed use of space and tabs
> > +			err_flags = MP_RETRY;
> > +			goto out;
> > +		}
> > +		DMWARN("%s path activation fail - error=0x%x",
> > +			path->dev->name, error);
> >  		err_flags = MP_FAIL_PATH;
> >  	}
> > -
> > + out:
> 
> space in front of label
> 
> >  	req->end_io_data = NULL;
> >  	__blk_put_request(req->q, req);
> >  	dm_pg_init_complete(path, err_flags);
> > @@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
> >  	if (!req) {
> >  		DMERR("%s path activation command allocation fail ",
> >  		      path->dev->name);
> > -		goto fail;
> > +		goto retry;
> >  	}
> > 
> > -	DMDEBUG("path activation command sent on %s",
> > -		path->dev->name);
> > +	DMDEBUG("%s path activation command sent", path->dev->name);
> > 
> >  	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> >  	return;
> > 
> > - fail:
> > -	dm_pg_init_complete(path, MP_FAIL_PATH);
> > + retry:
> > +	dm_pg_init_complete(path, MP_RETRY);
> >  }
> > 
> >  static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> > @@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
> >  	if (r < 0)
> >  		DMERR("register failed %d", r);
> >  	else
> > -		DMINFO("version 0.0.2 loaded");
> > +		DMINFO("version 0.0.3 loaded");
> > 
> >  	return r;
> >  }
> > 

Attached should fix the whitespace issues mentioned.



[-- Attachment #2: dm-hp-sw-add-retries-handle-not-ready.patch --]
[-- Type: text/x-patch, Size: 3890 bytes --]

Add retries to hp hardware handler if path initialization command completes
with a check condition.  

This patch adds retries to the hp hardware handler, and utilizes the 
MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
pg_init completed with a check condition we just assume we can retry the
pg_init command.  We make this assumption because of incomplete data on
specific check condition code of the HP hardware, and because testing
has shown the HP path initialization command to be idempotent.
The number of times we retry is settable via the "pg_init_retries"
multipath map feature.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"
@@ -28,6 +29,39 @@ struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.
@@ -39,8 +73,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {
@@ -52,11 +84,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 			path->dev->name);
 	} else {
-		DMWARN("%s path activation command - error=0x%x",
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command - retry",
+				path->dev->name);
+			err_flags = MP_RETRY;
+			goto out;
+		}
+		DMWARN("%s path activation fail - error=0x%x",
 			path->dev->name, error);
 		err_flags = MP_FAIL_PATH;
 	}
-
+out:
 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);
@@ -134,7 +172,7 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command - allocation fail",
 			path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
 	DMDEBUG("%s path activation command - sent", path->dev->name);
@@ -142,8 +180,8 @@ static void hp_sw_pg_init(struct hw_hand
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
-fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
@@ -180,7 +218,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 
 	return r;
 }

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Chandra Seetharaman]

Looks good.

Acked by: Chandra Seetharaman <sekharan@us.ibm.com>

On Mon, 2007-07-30 at 15:10 -0400, Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:19 -0700, Chandra Seetharaman wrote:
> > On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> > > plain text document attachment (dm-hp-sw-add-retries-handle-not-
> > > ready.patch)
> > > This patch adds retries to the hp hardware handler, and utilizes the 
> > > MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
> > > pg_init completed with a check condition we just assume we can retry the
> > > pg_init command.  We make this assumption because of incomplete data on
> > > specific check condition code of the HP hardware, and because testing
> > > has shown the HP path initialization command to be idempotent.
> > > The number of times we retry is settable via the "pg_init_retries"
> > > multipath map feature.
> > > 
> > > 
> > > Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > > ===================================================================
> > > --- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
> > > +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> > > @@ -18,6 +18,7 @@
> > >  #include <linux/types.h>
> > >  #include <scsi/scsi.h>
> > >  #include <scsi/scsi_cmnd.h>
> > > +#include <scsi/scsi_dbg.h>
> > > 
> > >  #include "dm.h"
> > >  #include "dm-hw-handler.h"
> > > @@ -26,8 +27,42 @@
> > > 
> > >  struct hp_sw_context {
> > >  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> > > +	unsigned argc;
> > >  };
> > > 
> > > +/**
> > > + * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
> > > + * @req: path activation request
> > > + *
> > > + * Examine error codes of request and determine whether the error is retryable.
> > > + * Some error codes are already retried by scsi-ml (see
> > > + * scsi_decide_disposition), but some HP specific codes are not.
> > > + * The intent of this routine is to supply the logic for the HP specific
> > > + * check conditions.
> > > + *
> > > + * Returns:
> > > + *  1 - command completed with retryable error
> > > + *  0 - command completed with non-retryable error
> > > + *
> > > + * Possible optimizations
> > > + * 1. More hardware-specific error codes
> > > + */
> > > +static int hp_sw_error_is_retryable(struct request *req)
> > > +{
> > > +	/*
> > > +	 * NOT_READY is known to be retryable
> > > +	 * For now we just dump out the sense data and call it retryable
> > > +	 */
> > > +	if (status_byte(req->errors) == CHECK_CONDITION)
> > > +		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
> > > +
> > > +	/*
> > > +	 * At this point we don't have complete information about all the error
> > > +	 * codes from this hardware, so we are just conservative and retry
> > > +	 * when in doubt.
> > > +	 */
> > > +	return 1;
> > > +}
> > > 
> > >  /**
> > >   * hp_sw_end_io - Completion handler for HP path activation.
> > > @@ -39,8 +74,6 @@ struct hp_sw_context {
> > >   *
> > >   * Context: scsi-ml softirq
> > >   *
> > > - * Possible optimizations
> > > - * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> > >   */
> > >  static void hp_sw_end_io(struct request *req, int error)
> > >  {
> > > @@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
> > >  		DMDEBUG("%s path activation command - success",
> > >  		       	path->dev->name);
> > >  	} else {
> > > -		DMWARN("path activation command on %s - error=0x%x",
> > > -		       path->dev->name, error);
> > > +		if (hp_sw_error_is_retryable(req)) {
> > > +			DMDEBUG("%s path activation command retry",
> > > +			       path->dev->name);
> > 
> > mixed use of space and tabs
> > > +			err_flags = MP_RETRY;
> > > +			goto out;
> > > +		}
> > > +		DMWARN("%s path activation fail - error=0x%x",
> > > +			path->dev->name, error);
> > >  		err_flags = MP_FAIL_PATH;
> > >  	}
> > > -
> > > + out:
> > 
> > space in front of label
> > 
> > >  	req->end_io_data = NULL;
> > >  	__blk_put_request(req->q, req);
> > >  	dm_pg_init_complete(path, err_flags);
> > > @@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
> > >  	if (!req) {
> > >  		DMERR("%s path activation command allocation fail ",
> > >  		      path->dev->name);
> > > -		goto fail;
> > > +		goto retry;
> > >  	}
> > > 
> > > -	DMDEBUG("path activation command sent on %s",
> > > -		path->dev->name);
> > > +	DMDEBUG("%s path activation command sent", path->dev->name);
> > > 
> > >  	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> > >  	return;
> > > 
> > > - fail:
> > > -	dm_pg_init_complete(path, MP_FAIL_PATH);
> > > + retry:
> > > +	dm_pg_init_complete(path, MP_RETRY);
> > >  }
> > > 
> > >  static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> > > @@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
> > >  	if (r < 0)
> > >  		DMERR("register failed %d", r);
> > >  	else
> > > -		DMINFO("version 0.0.2 loaded");
> > > +		DMINFO("version 0.0.3 loaded");
> > > 
> > >  	return r;
> > >  }
> > > 
> 
> Attached should fix the whitespace issues mentioned.
> 
> 
-- 

----------------------------------------------------------------------
    Chandra Seetharaman               | Be careful what you choose....
              - sekharan@us.ibm.com   |      .......you may get it.
----------------------------------------------------------------------

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Mike Christie]

Dave Wysochanski wrote:
> On Thu, 2007-07-26 at 12:19 -0700, Chandra Seetharaman wrote:
>> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
>>> plain text document attachment (dm-hp-sw-add-retries-handle-not-
>>> ready.patch)
>>> This patch adds retries to the hp hardware handler, and utilizes the 
>>> MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
>>> pg_init completed with a check condition we just assume we can retry the
>>> pg_init command.  We make this assumption because of incomplete data on
>>> specific check condition code of the HP hardware, and because testing
>>> has shown the HP path initialization command to be idempotent.
>>> The number of times we retry is settable via the "pg_init_retries"
>>> multipath map feature.
>>>
>>>
>>> Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
>>> ===================================================================
>>> --- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
>>> +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
>>> @@ -18,6 +18,7 @@
>>>  #include <linux/types.h>
>>>  #include <scsi/scsi.h>
>>>  #include <scsi/scsi_cmnd.h>
>>> +#include <scsi/scsi_dbg.h>
>>>
>>>  #include "dm.h"
>>>  #include "dm-hw-handler.h"
>>> @@ -26,8 +27,42 @@
>>>
>>>  struct hp_sw_context {
>>>  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
>>> +	unsigned argc;
>>>  };
>>>
>>> +/**
>>> + * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
>>> + * @req: path activation request
>>> + *
>>> + * Examine error codes of request and determine whether the error is retryable.
>>> + * Some error codes are already retried by scsi-ml (see
>>> + * scsi_decide_disposition), but some HP specific codes are not.
>>> + * The intent of this routine is to supply the logic for the HP specific
>>> + * check conditions.
>>> + *
>>> + * Returns:
>>> + *  1 - command completed with retryable error
>>> + *  0 - command completed with non-retryable error
>>> + *
>>> + * Possible optimizations
>>> + * 1. More hardware-specific error codes
>>> + */
>>> +static int hp_sw_error_is_retryable(struct request *req)
>>> +{
>>> +	/*
>>> +	 * NOT_READY is known to be retryable
>>> +	 * For now we just dump out the sense data and call it retryable
>>> +	 */
>>> +	if (status_byte(req->errors) == CHECK_CONDITION)
>>> +		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
>>> +
>>> +	/*
>>> +	 * At this point we don't have complete information about all the error
>>> +	 * codes from this hardware, so we are just conservative and retry
>>> +	 * when in doubt.
>>> +	 */
>>> +	return 1;
>>> +}
>>>
>>>  /**
>>>   * hp_sw_end_io - Completion handler for HP path activation.
>>> @@ -39,8 +74,6 @@ struct hp_sw_context {
>>>   *
>>>   * Context: scsi-ml softirq
>>>   *
>>> - * Possible optimizations
>>> - * 1. Actually check sense data for retryable error (e.g. NOT_READY)
>>>   */
>>>  static void hp_sw_end_io(struct request *req, int error)
>>>  {
>>> @@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
>>>  		DMDEBUG("%s path activation command - success",
>>>  		       	path->dev->name);
>>>  	} else {
>>> -		DMWARN("path activation command on %s - error=0x%x",
>>> -		       path->dev->name, error);
>>> +		if (hp_sw_error_is_retryable(req)) {
>>> +			DMDEBUG("%s path activation command retry",
>>> +			       path->dev->name);
>> mixed use of space and tabs
>>> +			err_flags = MP_RETRY;
>>> +			goto out;
>>> +		}
>>> +		DMWARN("%s path activation fail - error=0x%x",
>>> +			path->dev->name, error);
>>>  		err_flags = MP_FAIL_PATH;
>>>  	}
>>> -
>>> + out:
>> space in front of label
>>
>>>  	req->end_io_data = NULL;
>>>  	__blk_put_request(req->q, req);
>>>  	dm_pg_init_complete(path, err_flags);
>>> @@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
>>>  	if (!req) {
>>>  		DMERR("%s path activation command allocation fail ",
>>>  		      path->dev->name);
>>> -		goto fail;
>>> +		goto retry;
>>>  	}
>>>
>>> -	DMDEBUG("path activation command sent on %s",
>>> -		path->dev->name);
>>> +	DMDEBUG("%s path activation command sent", path->dev->name);
>>>
>>>  	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
>>>  	return;
>>>
>>> - fail:
>>> -	dm_pg_init_complete(path, MP_FAIL_PATH);
>>> + retry:
>>> +	dm_pg_init_complete(path, MP_RETRY);
>>>  }
>>>
>>>  static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
>>> @@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
>>>  	if (r < 0)
>>>  		DMERR("register failed %d", r);
>>>  	else
>>> -		DMINFO("version 0.0.2 loaded");
>>> +		DMINFO("version 0.0.3 loaded");
>>>
>>>  	return r;
>>>  }
>>>
> 
> Attached should fix the whitespace issues mentioned.
> 

That seems more odd with the other code. Please do not just use tabs for 
spacing on those places. Keep it consistent with the rest of the dm code.

Re: [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.

[Dave Wysochanski]

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

On Mon, 2007-07-30 at 16:08 -0500, Mike Christie wrote:
> Dave Wysochanski wrote:
> > On Thu, 2007-07-26 at 12:19 -0700, Chandra Seetharaman wrote:
> >> On Thu, 2007-07-26 at 00:44 -0400, dwysocha@redhat.com wrote:
> >>> plain text document attachment (dm-hp-sw-add-retries-handle-not-
> >>> ready.patch)
> >>> This patch adds retries to the hp hardware handler, and utilizes the 
> >>> MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
> >>> pg_init completed with a check condition we just assume we can retry the
> >>> pg_init command.  We make this assumption because of incomplete data on
> >>> specific check condition code of the HP hardware, and because testing
> >>> has shown the HP path initialization command to be idempotent.
> >>> The number of times we retry is settable via the "pg_init_retries"
> >>> multipath map feature.
> >>>
> >>>
> >>> Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> >>> ===================================================================
> >>> --- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
> >>> +++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
> >>> @@ -18,6 +18,7 @@
> >>>  #include <linux/types.h>
> >>>  #include <scsi/scsi.h>
> >>>  #include <scsi/scsi_cmnd.h>
> >>> +#include <scsi/scsi_dbg.h>
> >>>
> >>>  #include "dm.h"
> >>>  #include "dm-hw-handler.h"
> >>> @@ -26,8 +27,42 @@
> >>>
> >>>  struct hp_sw_context {
> >>>  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
> >>> +	unsigned argc;
> >>>  };
> >>>
> >>> +/**
> >>> + * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
> >>> + * @req: path activation request
> >>> + *
> >>> + * Examine error codes of request and determine whether the error is retryable.
> >>> + * Some error codes are already retried by scsi-ml (see
> >>> + * scsi_decide_disposition), but some HP specific codes are not.
> >>> + * The intent of this routine is to supply the logic for the HP specific
> >>> + * check conditions.
> >>> + *
> >>> + * Returns:
> >>> + *  1 - command completed with retryable error
> >>> + *  0 - command completed with non-retryable error
> >>> + *
> >>> + * Possible optimizations
> >>> + * 1. More hardware-specific error codes
> >>> + */
> >>> +static int hp_sw_error_is_retryable(struct request *req)
> >>> +{
> >>> +	/*
> >>> +	 * NOT_READY is known to be retryable
> >>> +	 * For now we just dump out the sense data and call it retryable
> >>> +	 */
> >>> +	if (status_byte(req->errors) == CHECK_CONDITION)
> >>> +		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
> >>> +
> >>> +	/*
> >>> +	 * At this point we don't have complete information about all the error
> >>> +	 * codes from this hardware, so we are just conservative and retry
> >>> +	 * when in doubt.
> >>> +	 */
> >>> +	return 1;
> >>> +}
> >>>
> >>>  /**
> >>>   * hp_sw_end_io - Completion handler for HP path activation.
> >>> @@ -39,8 +74,6 @@ struct hp_sw_context {
> >>>   *
> >>>   * Context: scsi-ml softirq
> >>>   *
> >>> - * Possible optimizations
> >>> - * 1. Actually check sense data for retryable error (e.g. NOT_READY)
> >>>   */
> >>>  static void hp_sw_end_io(struct request *req, int error)
> >>>  {
> >>> @@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
> >>>  		DMDEBUG("%s path activation command - success",
> >>>  		       	path->dev->name);
> >>>  	} else {
> >>> -		DMWARN("path activation command on %s - error=0x%x",
> >>> -		       path->dev->name, error);
> >>> +		if (hp_sw_error_is_retryable(req)) {
> >>> +			DMDEBUG("%s path activation command retry",
> >>> +			       path->dev->name);
> >> mixed use of space and tabs
> >>> +			err_flags = MP_RETRY;
> >>> +			goto out;
> >>> +		}
> >>> +		DMWARN("%s path activation fail - error=0x%x",
> >>> +			path->dev->name, error);
> >>>  		err_flags = MP_FAIL_PATH;
> >>>  	}
> >>> -
> >>> + out:
> >> space in front of label
> >>
> >>>  	req->end_io_data = NULL;
> >>>  	__blk_put_request(req->q, req);
> >>>  	dm_pg_init_complete(path, err_flags);
> >>> @@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
> >>>  	if (!req) {
> >>>  		DMERR("%s path activation command allocation fail ",
> >>>  		      path->dev->name);
> >>> -		goto fail;
> >>> +		goto retry;
> >>>  	}
> >>>
> >>> -	DMDEBUG("path activation command sent on %s",
> >>> -		path->dev->name);
> >>> +	DMDEBUG("%s path activation command sent", path->dev->name);
> >>>
> >>>  	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
> >>>  	return;
> >>>
> >>> - fail:
> >>> -	dm_pg_init_complete(path, MP_FAIL_PATH);
> >>> + retry:
> >>> +	dm_pg_init_complete(path, MP_RETRY);
> >>>  }
> >>>
> >>>  static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
> >>> @@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
> >>>  	if (r < 0)
> >>>  		DMERR("register failed %d", r);
> >>>  	else
> >>> -		DMINFO("version 0.0.2 loaded");
> >>> +		DMINFO("version 0.0.3 loaded");
> >>>
> >>>  	return r;
> >>>  }
> >>>
> > 
> > Attached should fix the whitespace issues mentioned.
> > 
> 
> That seems more odd with the other code. Please do not just use tabs for 
> spacing on those places. Keep it consistent with the rest of the dm code.
> 

Ok - see attached.



[-- Attachment #2: dm-hp-sw-add-retries-handle-not-ready.patch --]
[-- Type: text/x-patch, Size: 3953 bytes --]

Add retries to hp hardware handler if path initialization command completes
with a check condition.  

This patch adds retries to the hp hardware handler, and utilizes the 
MP_RETRY flag of dm-multipath.  For now in the hp handler, if we get a 
pg_init completed with a check condition we just assume we can retry the
pg_init command.  We make this assumption because of incomplete data on
specific check condition code of the HP hardware, and because testing
has shown the HP path initialization command to be idempotent.
The number of times we retry is settable via the "pg_init_retries"
multipath map feature.

Signed-off-by: Dave Wysochanski <dwysocha@redhat.com>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-hp-sw.c
+++ linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"
@@ -28,6 +29,39 @@ struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.
@@ -39,8 +73,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {
@@ -52,11 +84,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 			path->dev->name);
 	} else {
-		DMWARN("%s path activation command - error=0x%x",
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command - retry",
+				path->dev->name);
+			err_flags = MP_RETRY;
+			goto out;
+		}
+		DMWARN("%s path activation fail - error=0x%x",
 		       path->dev->name, error);
 		err_flags = MP_FAIL_PATH;
 	}
-
+out:
 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);
@@ -134,7 +172,7 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command - allocation fail",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
 	DMDEBUG("%s path activation command - sent", path->dev->name);
@@ -142,8 +180,8 @@ static void hp_sw_pg_init(struct hw_hand
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
-fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)
@@ -180,7 +218,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 
 	return r;
 }

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[patch 0/3] Add HP hardware handler support to dm-multipath

[dwysocha]

The following 3 patches add HP hardware handler support to dm-multipath.
The first patch is very basic and provides a baseline of support but it is not
complete (has no retries, error code handling, etc).  Second and third patches
add retries and some error code handling.

I believe all comments to date have been addressed, including whitespace
issues, and passing of scripts/checkpatch.pl.


--