All of lore.kernel.org
 help / color / mirror / Atom feed
* [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-05-24  5:17 [patch 0/3] Add HP hardware handler support to dm-mp dwysocha
@ 2007-05-24  5:17 ` dwysocha
  0 siblings, 0 replies; 21+ messages in thread
From: dwysocha @ 2007-05-24  5:17 UTC (permalink / raw)
  To: dm-devel; +Cc: Mike Christie, Dave Wysochanski, Alasdair G Kergon

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

Signed-off-by: Dave Wysochanski <dave.wysochanski@redhat.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>

--
Index: linux-2.6.22-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.22-rc1.orig/drivers/md/Makefile
+++ linux-2.6.22-rc1/drivers/md/Makefile
@@ -34,6 +34,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_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
 obj-$(CONFIG_DM_ZERO)		+= dm-zero.o
Index: linux-2.6.22-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.22-rc1/drivers/md/dm-hp-sw.c
@@ -0,0 +1,204 @@
+/*
+ * 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/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"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: failover 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("hp_sw: path activation command on %s - success",
+		       	path->dev->name);
+	} else {
+		DMWARN("hp_sw: 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=NULL;
+	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_ATOMIC);
+	if (req == NULL)
+		goto exit;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io = hp_sw_end_io;
+	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]);
+exit:
+	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 = (struct hp_sw_context *) hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("hp_sw: path activation command allocation fail on %s ",
+		      path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("hp_sw: path activation command sent on %s",
+		path->dev->name);
+
+	elv_add_request(req->q, req, ELEVATOR_INSERT_FRONT, 1);
+	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);
+	hwh->context = NULL;
+}
+
+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("hp_sw: register failed %d", r);
+
+	DMINFO("hp_sw 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("hp_sw: 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.22-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.22-rc1.orig/drivers/md/Kconfig
+++ linux-2.6.22-rc1/drivers/md/Kconfig
@@ -271,6 +271,12 @@ config DM_DELAY
 
 	If unsure, say N.
 
+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.
+
 endmenu
 
 endif

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-05-24  5:24 [patch 0/3] Add HP hardware handler support to dm-mp dwysocha
@ 2007-05-24  5:24 ` dwysocha
  0 siblings, 0 replies; 21+ messages in thread
From: dwysocha @ 2007-05-24  5:24 UTC (permalink / raw)
  To: dm-devel

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

Index: linux-2.6.22-rc1/drivers/md/Makefile
===================================================================
--- linux-2.6.22-rc1.orig/drivers/md/Makefile
+++ linux-2.6.22-rc1/drivers/md/Makefile
@@ -34,6 +34,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_SNAPSHOT)	+= dm-snapshot.o
 obj-$(CONFIG_DM_MIRROR)		+= dm-mirror.o
 obj-$(CONFIG_DM_ZERO)		+= dm-zero.o
Index: linux-2.6.22-rc1/drivers/md/dm-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.22-rc1/drivers/md/dm-hp-sw.c
@@ -0,0 +1,204 @@
+/*
+ * 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/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"
+
+struct hp_sw_context {
+	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+};
+
+
+/**
+ * hp_sw_end_io - Completion handler for HP path activation.
+ * @req: failover 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("hp_sw: path activation command on %s - success",
+		       	path->dev->name);
+	} else {
+		DMWARN("hp_sw: 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=NULL;
+	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_ATOMIC);
+	if (req == NULL)
+		goto exit;
+
+	req->timeout = 60*HZ;
+
+	req->errors = 0;
+	req->cmd_type = REQ_TYPE_BLOCK_PC;
+	req->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
+	req->end_io = hp_sw_end_io;
+	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]);
+exit:
+	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 = (struct hp_sw_context *) hwh->context;
+
+	req = hp_sw_get_request(path);
+	if (!req) {
+		DMERR("hp_sw: path activation command allocation fail on %s ",
+		      path->dev->name);
+		goto fail;
+	}
+
+	DMDEBUG("hp_sw: path activation command sent on %s",
+		path->dev->name);
+
+	elv_add_request(req->q, req, ELEVATOR_INSERT_FRONT, 1);
+	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);
+	hwh->context = NULL;
+}
+
+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("hp_sw: register failed %d", r);
+
+	DMINFO("hp_sw 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("hp_sw: 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.22-rc1/drivers/md/Kconfig
===================================================================
--- linux-2.6.22-rc1.orig/drivers/md/Kconfig
+++ linux-2.6.22-rc1/drivers/md/Kconfig
@@ -271,6 +271,12 @@ config DM_DELAY
 
 	If unsure, say N.
 
+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.
+
 endmenu
 
 endif

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26  4:44 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
@ 2007-07-26  4:44 ` dwysocha
  2007-07-26 15:18   ` Mike Christie
  2007-07-26 19:09   ` Chandra Seetharaman
  0 siblings, 2 replies; 21+ messages in thread
From: dwysocha @ 2007-07-26  4:44 UTC (permalink / raw)
  To: dm-devel

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

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26  4:44 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
@ 2007-07-26 15:18   ` Mike Christie
  2007-07-26 16:16     ` Dave Wysochanski
  2007-07-26 19:09   ` Chandra Seetharaman
  1 sibling, 1 reply; 21+ messages in thread
From: Mike Christie @ 2007-07-26 15:18 UTC (permalink / raw)
  To: device-mapper development

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>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26 15:18   ` Mike Christie
@ 2007-07-26 16:16     ` Dave Wysochanski
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Wysochanski @ 2007-07-26 16:16 UTC (permalink / raw)
  To: device-mapper development

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26  4:44 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
  2007-07-26 15:18   ` Mike Christie
@ 2007-07-26 19:09   ` Chandra Seetharaman
  2007-07-30 18:06     ` Dave Wysochanski
  2007-07-30 21:08     ` Mike Christie
  1 sibling, 2 replies; 21+ messages in thread
From: Chandra Seetharaman @ 2007-07-26 19:09 UTC (permalink / raw)
  To: device-mapper development

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26 19:09   ` Chandra Seetharaman
@ 2007-07-30 18:06     ` Dave Wysochanski
  2007-07-30 19:25       ` Chandra Seetharaman
  2007-07-30 21:08     ` Mike Christie
  1 sibling, 1 reply; 21+ messages in thread
From: Dave Wysochanski @ 2007-07-30 18:06 UTC (permalink / raw)
  To: sekharan, device-mapper development

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 18:06     ` Dave Wysochanski
@ 2007-07-30 19:25       ` Chandra Seetharaman
  2007-07-30 20:10         ` Dave Wysochanski
  0 siblings, 1 reply; 21+ messages in thread
From: Chandra Seetharaman @ 2007-07-30 19:25 UTC (permalink / raw)
  To: Dave Wysochanski; +Cc: device-mapper development

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 19:25       ` Chandra Seetharaman
@ 2007-07-30 20:10         ` Dave Wysochanski
  2007-07-30 22:05           ` Chandra Seetharaman
  0 siblings, 1 reply; 21+ messages in thread
From: Dave Wysochanski @ 2007-07-30 20:10 UTC (permalink / raw)
  To: sekharan; +Cc: device-mapper development

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-26 19:09   ` Chandra Seetharaman
  2007-07-30 18:06     ` Dave Wysochanski
@ 2007-07-30 21:08     ` Mike Christie
  2007-07-30 22:08       ` Dave Wysochanski
  2007-07-30 23:27       ` Chandra Seetharaman
  1 sibling, 2 replies; 21+ messages in thread
From: Mike Christie @ 2007-07-30 21:08 UTC (permalink / raw)
  To: sekharan, device-mapper development

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 20:10         ` Dave Wysochanski
@ 2007-07-30 22:05           ` Chandra Seetharaman
  0 siblings, 0 replies; 21+ messages in thread
From: Chandra Seetharaman @ 2007-07-30 22:05 UTC (permalink / raw)
  To: Dave Wysochanski; +Cc: device-mapper development

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 21:08     ` Mike Christie
@ 2007-07-30 22:08       ` Dave Wysochanski
  2007-07-30 23:27       ` Chandra Seetharaman
  1 sibling, 0 replies; 21+ messages in thread
From: Dave Wysochanski @ 2007-07-30 22:08 UTC (permalink / raw)
  To: device-mapper development

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 21:08     ` Mike Christie
  2007-07-30 22:08       ` Dave Wysochanski
@ 2007-07-30 23:27       ` Chandra Seetharaman
  2007-07-31  3:35         ` Mike Christie
  1 sibling, 1 reply; 21+ messages in thread
From: Chandra Seetharaman @ 2007-07-30 23:27 UTC (permalink / raw)
  To: Mike Christie; +Cc: device-mapper development

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-30 23:27       ` Chandra Seetharaman
@ 2007-07-31  3:35         ` Mike Christie
  2007-07-31 17:35           ` Chandra Seetharaman
  0 siblings, 1 reply; 21+ messages in thread
From: Mike Christie @ 2007-07-31  3:35 UTC (permalink / raw)
  To: sekharan; +Cc: device-mapper development

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-31  3:35         ` Mike Christie
@ 2007-07-31 17:35           ` Chandra Seetharaman
  2007-07-31 17:37             ` Mike Christie
  0 siblings, 1 reply; 21+ messages in thread
From: Chandra Seetharaman @ 2007-07-31 17:35 UTC (permalink / raw)
  To: Mike Christie; +Cc: device-mapper development

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-07-31 17:35           ` Chandra Seetharaman
@ 2007-07-31 17:37             ` Mike Christie
  0 siblings, 0 replies; 21+ messages in thread
From: Mike Christie @ 2007-07-31 17:37 UTC (permalink / raw)
  To: sekharan; +Cc: device-mapper development

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 0/3] Add HP hardware handler support to dm-multipath
@ 2007-08-02 16:15 dwysocha
  2007-08-02 16:15 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: dwysocha @ 2007-08-02 16:15 UTC (permalink / raw)
  To: dm-devel

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.


-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc).
  2007-08-02 16:15 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
@ 2007-08-02 16:15 ` dwysocha
  2007-08-02 16:15 ` [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init dwysocha
  2007-08-02 16:15 ` [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition dwysocha
  2 siblings, 0 replies; 21+ messages in thread
From: dwysocha @ 2007-08-02 16:15 UTC (permalink / raw)
  To: dm-devel; +Cc: Mike Christie

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

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>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
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
@@ -8,6 +8,7 @@ dm-multipath-objs := dm-hw-handler.o dm-
 dm-snapshot-objs := dm-snap.o dm-exception-store.o
 dm-mirror-objs	:= dm-log.o dm-raid1.o
 dm-rdac-objs	:= dm-mpath-rdac.o
+dm-hp-sw-objs	:= dm-mpath-hp-sw.o
 md-mod-objs     := md.o bitmap.o
 raid456-objs	:= raid5.o raid6algos.o raid6recov.o raid6tables.o \
 		   raid6int1.o raid6int2.o raid6int4.o \
@@ -35,6 +36,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/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
Index: linux-2.6.23-rc1/drivers/md/dm-mpath-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.23-rc1/drivers/md/dm-mpath-hp-sw.c
@@ -0,0 +1,205 @@
+/*
+ * 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"
+#define HP_DM_HWH_NAME "hp_sw"
+#define HP_DM_HWH_VER "0.0.3"
+
+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_DM_HWH_NAME,
+	.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 " HP_DM_HWH_VER " 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("DM Multipath HP StorageWorks / FSC FibreCat (A/P) support");
+MODULE_AUTHOR("Mike Christie, Dave Wysochanski");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HP_DM_HWH_VER);

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init.
  2007-08-02 16:15 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
  2007-08-02 16:15 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
@ 2007-08-02 16:15 ` dwysocha
  2007-08-02 16:15 ` [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition dwysocha
  2 siblings, 0 replies; 21+ messages in thread
From: dwysocha @ 2007-08-02 16:15 UTC (permalink / raw)
  To: dm-devel; +Cc: Mike Christie

[-- Attachment #1: dm-mpath-add-retry-pg-init.patch --]
[-- Type: text/plain, Size: 5444 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"

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 ");
 

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition.
  2007-08-02 16:15 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
  2007-08-02 16:15 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
  2007-08-02 16:15 ` [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init dwysocha
@ 2007-08-02 16:15 ` dwysocha
  2 siblings, 0 replies; 21+ messages in thread
From: dwysocha @ 2007-08-02 16:15 UTC (permalink / raw)
  To: dm-devel

[-- Attachment #1: dm-hp-sw-add-retries-handle-not-ready.patch --]
[-- Type: text/plain, Size: 3799 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.

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-mpath-hp-sw.c
===================================================================
--- linux-2.6.23-rc1.orig/drivers/md/dm-mpath-hp-sw.c
+++ linux-2.6.23-rc1/drivers/md/dm-mpath-hp-sw.c
@@ -18,18 +18,52 @@
 #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"
 
 #define DM_MSG_PREFIX "multipath hp_sw"
 #define HP_DM_HWH_NAME "hp_sw"
-#define HP_DM_HWH_VER "0.0.3"
+#define HP_DM_HWH_VER "1.0.0"
 
 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.
@@ -41,8 +75,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)
 {
@@ -54,11 +86,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);
@@ -136,7 +174,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);
@@ -144,8 +182,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)

-- 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc)
@ 2007-08-02 16:25 Dave Wysochanski
  0 siblings, 0 replies; 21+ messages in thread
From: Dave Wysochanski @ 2007-08-02 16:25 UTC (permalink / raw)
  To: dm-devel

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



[-- Attachment #2: dm-hp-sw-v0.0.2.patch --]
[-- Type: text/x-patch, Size: 7581 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>
Acked-by: Chandra Seetharaman <sekharan@us.ibm.com>

---
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
@@ -8,6 +8,7 @@ dm-multipath-objs := dm-hw-handler.o dm-
 dm-snapshot-objs := dm-snap.o dm-exception-store.o
 dm-mirror-objs	:= dm-log.o dm-raid1.o
 dm-rdac-objs	:= dm-mpath-rdac.o
+dm-hp-sw-objs	:= dm-mpath-hp-sw.o
 md-mod-objs     := md.o bitmap.o
 raid456-objs	:= raid5.o raid6algos.o raid6recov.o raid6tables.o \
 		   raid6int1.o raid6int2.o raid6int4.o \
@@ -35,6 +36,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/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
Index: linux-2.6.23-rc1/drivers/md/dm-mpath-hp-sw.c
===================================================================
--- /dev/null
+++ linux-2.6.23-rc1/drivers/md/dm-mpath-hp-sw.c
@@ -0,0 +1,205 @@
+/*
+ * 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"
+#define HP_DM_HWH_NAME "hp_sw"
+#define HP_DM_HWH_VER "0.0.3"
+
+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_DM_HWH_NAME,
+	.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 " HP_DM_HWH_VER " 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("DM Multipath HP StorageWorks / FSC FibreCat (A/P) support");
+MODULE_AUTHOR("Mike Christie, Dave Wysochanski");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(HP_DM_HWH_VER);

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



^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2007-08-02 16:25 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-08-02 16:15 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
2007-08-02 16:15 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
2007-08-02 16:15 ` [patch 2/3] Add MP_RETRY flag for hw handlers to tell dm-mpath to retry pg_init dwysocha
2007-08-02 16:15 ` [patch 3/3] Add retries to hp hardware handler if path initialization command completes with a check condition dwysocha
  -- strict thread matches above, loose matches on Subject: below --
2007-08-02 16:25 [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) Dave Wysochanski
2007-07-26  4:44 [patch 0/3] Add HP hardware handler support to dm-multipath dwysocha
2007-07-26  4:44 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
2007-07-26 15:18   ` Mike Christie
2007-07-26 16:16     ` Dave Wysochanski
2007-07-26 19:09   ` Chandra Seetharaman
2007-07-30 18:06     ` Dave Wysochanski
2007-07-30 19:25       ` Chandra Seetharaman
2007-07-30 20:10         ` Dave Wysochanski
2007-07-30 22:05           ` Chandra Seetharaman
2007-07-30 21:08     ` Mike Christie
2007-07-30 22:08       ` Dave Wysochanski
2007-07-30 23:27       ` Chandra Seetharaman
2007-07-31  3:35         ` Mike Christie
2007-07-31 17:35           ` Chandra Seetharaman
2007-07-31 17:37             ` Mike Christie
2007-05-24  5:24 [patch 0/3] Add HP hardware handler support to dm-mp dwysocha
2007-05-24  5:24 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha
2007-05-24  5:17 [patch 0/3] Add HP hardware handler support to dm-mp dwysocha
2007-05-24  5:17 ` [patch 1/3] Extremely basic hp hardware handler (no retries, no error handling, etc) dwysocha

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.