Add HP hardware handler support to dm-multipath

[email protected] wrote:

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

Signed-off-by Mike Christie <[email protected]>

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

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

drivers/md/Makefile +1 -0

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

drivers/md/dm-hp-sw.c new file +43 -0

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

drivers/md/dm-hp-sw.c new file +37 -0

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

drivers/md/dm-hp-sw.c new file +101 -0

+	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 <[email protected]>");
+MODULE_LICENSE("GPL");

Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================

drivers/md/Kconfig +6 -0

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

Hi Dave,

some coding style related comments (below).

Quoted 65 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] 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
===================================================================

drivers/md/Makefile +1 -0

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

drivers/md/dm-hp-sw.c new file +43 -0

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

drivers/md/dm-hp-sw.c line 53

Quoted 10 lines

drivers/md/dm-hp-sw.c new file +10 -0

+ */
+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).

drivers/md/dm-hp-sw.c line 55

Quoted 2 lines

drivers/md/dm-hp-sw.c new file +2 -0

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

Quoted 37 lines

drivers/md/dm-hp-sw.c new file +37 -0

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

drivers/md/dm-hp-sw.c line 102

Quoted 10 lines

drivers/md/dm-hp-sw.c new file +10 -0

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

Quoted 109 lines

drivers/md/dm-hp-sw.c new file +101 -0

+	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 <[email protected]>");
+MODULE_LICENSE("GPL");

Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================

drivers/md/Kconfig +0 -0

@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 

Quoted 10 lines

drivers/md/Kconfig +6 -0

+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

Quoted 249 lines · Chandra Seetharaman

Hi Dave,

some coding style related comments (below).

Quoted 65 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] 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
===================================================================

drivers/md/Makefile +1 -0

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

drivers/md/dm-hp-sw.c new file +43 -0

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

drivers/md/dm-hp-sw.c line 53

Quoted 10 lines

drivers/md/dm-hp-sw.c new file +10 -0

+ */
+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).

drivers/md/dm-hp-sw.c line 55

Quoted 2 lines

drivers/md/dm-hp-sw.c new file +2 -0

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

Quoted 37 lines

drivers/md/dm-hp-sw.c new file +37 -0

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

drivers/md/dm-hp-sw.c line 102

Quoted 10 lines

drivers/md/dm-hp-sw.c new file +10 -0

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

Quoted 109 lines

drivers/md/dm-hp-sw.c new file +101 -0

+	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 <[email protected]>");
+MODULE_LICENSE("GPL");

Index: linux-2.6.23-rc1/drivers/md/Kconfig
===================================================================

drivers/md/Kconfig +0 -0

@@ -267,6 +267,12 @@ config DM_MULTIPATH_RDAC
 	---help---
 	  Multipath support for LSI/Engenio RDAC.
 

Quoted 12 lines

Quoted 10 lines

drivers/md/Kconfig +6 -0

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

Do you want to keep the DMDEBUG in this patch ?

Other than that it looks good...

Thanks

Quoted 6 lines · Chandra Seetharaman

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.

Hi Dave,

some coding style related comments (below).

drivers/md/dm-hp-sw.c line 53

Quoted 4 lines

drivers/md/dm-hp-sw.c new file +4 -0

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

drivers/md/dm-hp-sw.c line 102

Quoted 5 lines

drivers/md/dm-hp-sw.c new file +5 -0

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

Quoted 6 lines · Chandra Seetharaman

Hi Dave,

some coding style related comments (below).

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

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

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Quoted 6 lines · Chandra Seetharaman

Hi Dave,

some coding style related comments (below).

Quoted 7 lines

drivers/md/dm-hp-sw.c line 53

Quoted 4 lines

drivers/md/dm-hp-sw.c new file +4 -0

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

Quoted 8 lines

drivers/md/dm-hp-sw.c line 102

Quoted 5 lines

drivers/md/dm-hp-sw.c new file +5 -0

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

Quoted 12 lines · Mike Christie

Quoted 11 lines · Chandra Seetharaman

Hi Dave,

some coding style related comments (below).

drivers/md/dm-hp-sw.c line 53

Quoted 4 lines

drivers/md/dm-hp-sw.c new file +4 -0

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

Quoted 1 line

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

Quoted 13 lines · Chandra Seetharaman

Quoted 12 lines · Mike Christie

Quoted 11 lines · Chandra Seetharaman

Hi Dave,

some coding style related comments (below).

drivers/md/dm-hp-sw.c line 53

Quoted 4 lines

drivers/md/dm-hp-sw.c new file +4 -0

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

Quoted 6 lines

Quoted 1 line

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.

Quoted 1 line

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.

[email protected] wrote:

looks ok

Acked-by Mike Christie <[email protected]>

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

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

Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================

drivers/md/dm-hw-handler.h +1 -0

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

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

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

drivers/md/dm-mpath.c +1 -1

+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);

@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))

drivers/md/dm-mpath.c +31 -2

+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,

@@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries)
+		m->pg_init_required = 1;
+	else
+		fail_path(pgpath);
+	spin_unlock_irqrestore(&m->lock, flags);
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)

@@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)

@@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

Quoted 61 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================

drivers/md/dm-hw-handler.h +1 -0

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

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

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

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 699

Quoted 8 lines

drivers/md/dm-mpath.c +2 -1

@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},

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

Quoted 9 lines

drivers/md/dm-mpath.c +1 -1

+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);

@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 715

Quoted 10 lines

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

-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");

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

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 717

Quoted 2 lines

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

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

mixed use of space/tab.

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 722

Quoted 5 lines

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

+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);

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

Quoted 53 lines

drivers/md/dm-mpath.c +27 -2

+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,

@@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries)
+		m->pg_init_required = 1;
+	else
+		fail_path(pgpath);
+	spin_unlock_irqrestore(&m->lock, flags);
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)

@@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)

@@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)

drivers/md/dm-mpath.c static int multipath_status(struct dm_ta · line 1199

Quoted 8 lines

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

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

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

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

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

-----Original Message-----
From: [email protected] [mailto:[email protected]]

Behalf Of Mike Snitzer
Sent: Monday, July 30, 2007 2:50 PM
To: device-mapper development
Subject: Re: [dm-devel] CVS pull of device mapper

Quoted 4 lines · Brian J

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

Quoted 4 lines

error:

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

Quoted 5 lines

or directory

I've looked in /lib and there is an instance of libdevmapper-event.so.1.02 (and a symbolic link named libdevmapper-event.so), so I'm confused as to why the daemon can't

Quoted 2 lines

them? When I run configure the only parameters I'm passing in are "--enable-debug" and "--enable-dmeventd"

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

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

-----Original Message-----
From: [email protected] [mailto:[email protected]]

Behalf Of Wood, Brian J
Sent: Monday, July 30, 2007 3:21 PM
To: device-mapper development
Subject: RE: [dm-devel] CVS pull of device mapper

Brian Wood
Intel Corporation
Digital Enterprise Group
Manageability & Platform Software Division
[email protected]

Quoted 2 lines

-----Original Message-----
From: [email protected] [mailto:[email protected]]

On

Quoted 10 lines

Behalf Of Mike Snitzer
Sent: Monday, July 30, 2007 2:50 PM
To: device-mapper development
Subject: Re: [dm-devel] CVS pull of device mapper

Quoted 4 lines · Brian J

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

this

Quoted 4 lines

Quoted 4 lines

error:

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

file

Quoted 5 lines

Quoted 5 lines

or directory

I've looked in /lib and there is an instance of libdevmapper-event.so.1.02 (and a symbolic link named libdevmapper-event.so), so I'm confused as to why the daemon can't

find

Quoted 5 lines

Quoted 2 lines

them? When I run configure the only parameters I'm passing in are "--enable-debug" and "--enable-dmeventd"

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

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

/sbin/dmeventd

     3127:
     3127:     file=libdl.so.2 [0];  needed by /sbin/dmeventd [0]
     3127:     find library=libdl.so.2 [0]; searching
     3127:      search cache=/etc/ld.so.cache
     3127:       trying file=/lib64/libdl.so.2
     3127:
     3127:     file=libdl.so.2 [0];  generating link map
     3127:       dynamic: 0x0000003c7ba02da0  base: 0x0000000000000000
size: 0x0000000000203100
     3127:         entry: 0x0000003c7b800e10  phdr: 0x0000003c7b800040
phnum:                  9
     3127:
     3127:
     3127:     file=libdevmapper.so.1.02 [0];  needed by

[0]

     3127:     find library=libdevmapper.so.1.02 [0]; searching
     3127:      search cache=/etc/ld.so.cache
     3127:       trying file=/lib64/libdevmapper.so.1.02
     3127:
     3127:     file=libdevmapper.so.1.02 [0];  generating link map
     3127:       dynamic: 0x0000003c83011038  base: 0x0000000000000000
size: 0x00000000002128c8
     3127:         entry: 0x0000003c82e039f0  phdr: 0x0000003c82e00040
phnum:                  5
     3127:
     3127:
     3127:     file=libpthread.so.0 [0];  needed by /sbin/dmeventd [0]
     3127:     find library=libpthread.so.0 [0]; searching
     3127:      search cache=/etc/ld.so.cache
     3127:       trying file=/lib64/libpthread.so.0
     3127:
     3127:     file=libpthread.so.0 [0];  generating link map
     3127:       dynamic: 0x0000003c7d614db0  base: 0x0000000000000000
size: 0x0000000000219370
     3127:         entry: 0x0000003c7d405720  phdr: 0x0000003c7d400040
phnum:                  9
     3127:
     3127:
     3127:     file=libdevmapper-event.so.1.02 [0];  needed by
/sbin/dmeventd [0]
     3127:     find library=libdevmapper-event.so.1.02 [0]; searching
     3127:      search cache=/etc/ld.so.cache
     3127:      search

path=/lib64/tls/x86_64:/lib64/tls:/lib64/x86_64:/lib64:/usr/lib64/tls/x

6_64:/usr/lib64/tls:/usr/lib64/x86_64:/usr/lib64(system search path)

     3127:       trying
file=/lib64/tls/x86_64/libdevmapper-event.so.1.02
     3127:       trying file=/lib64/tls/libdevmapper-event.so.1.02
     3127:       trying file=/lib64/x86_64/libdevmapper-event.so.1.02
     3127:       trying file=/lib64/libdevmapper-event.so.1.02
     3127:       trying
file=/usr/lib64/tls/x86_64/libdevmapper-event.so.1.02
     3127:       trying file=/usr/lib64/tls/libdevmapper-event.so.1.02
     3127:       trying
file=/usr/lib64/x86_64/libdevmapper-event.so.1.02
     3127:       trying file=/usr/lib64/libdevmapper-event.so.1.02
     3127:

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

or directory

One thing I noticed when building/installing was that my .so is being

into /lib in the stdout messages?

Two questions:

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

2. Why didn't this problem occur on my FC6 x86_64 system? Both were

out of the box systems, so did something change in F7 that I didn't realize.

Thanks for the help on this puzzling issue :)

Quoted 4 lines

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

Quoted 159 lines · Chandra Seetharaman

Quoted 61 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hw-handler.h
===================================================================

drivers/md/dm-hw-handler.h +1 -0

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

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

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

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 699

Quoted 8 lines

drivers/md/dm-mpath.c +2 -1

@@ -689,9 +693,11 @@ static int parse_features(struct arg_set
 	int r;
 	unsigned argc;
 	struct dm_target *ti = m->ti;
+	char *name;
 
 	static struct param _params[] = {
-		{0, 1, "invalid number of feature args"},
+		{0, 4, "invalid number of feature args"},

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

Quoted 9 lines

drivers/md/dm-mpath.c +1 -1

+		{0, 50, "invalid number of pg_init retries"},
 	};
 
 	r = read_param(_params, shift(as), &argc, &ti->error);

@@ -701,12 +707,26 @@ static int parse_features(struct arg_set
 	if (!argc)
 		return 0;
 
-	if (!strnicmp(shift(as), MESG_STR("queue_if_no_path")))

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 715

Quoted 10 lines

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

-		return queue_if_no_path(m, 1, 0);
-	else {
-		ti->error = "Unrecognised multipath feature request";
-		return -EINVAL;
+	while (argc && !r) {
+		name = shift(as);
+		argc--;
+		if (!strnicmp(name, MESG_STR("queue_if_no_path"))) {
+			r = queue_if_no_path(m, 1, 0);
+			DMDEBUG("setting queue_if_no_path");

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

drivers/md/dm-mpath.c static int parse_features(struct arg_set · line 716

Quoted 2 lines

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

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

mixed use of space/tab.

drivers/md/dm-mpath.c static int parse_features(struct arg_set

Quoted 5 lines

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

+			r = read_param(_params + 1, shift(as),
+					&m->pg_init_retries, &ti->error);
+			argc--;
+			DMDEBUG("setting pg_init_retries to %u",
+				m->pg_init_retries);

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

Quoted 53 lines

drivers/md/dm-mpath.c +27 -2

+		} else {
+			ti->error = "Unrecognised multipath feature request";
+			return -EINVAL;
+		}
 	}
+
+	return r;
 }
 
 static int multipath_ctr(struct dm_target *ti, unsigned int argc,

@@ -976,6 +996,21 @@ static int bypass_pg_num(struct multipat
 }
 
 /*
+ * Retry pg_init on the same path group and path
+ */
+static void retry_pg(struct multipath *m, struct pgpath *pgpath)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&m->lock, flags);
+	if (m->pg_init_count <= m->pg_init_retries)
+		m->pg_init_required = 1;
+	else
+		fail_path(pgpath);
+	spin_unlock_irqrestore(&m->lock, flags);
+}
+
+/*
  * pg_init must call this when it has completed its initialisation
  */
 void dm_pg_init_complete(struct dm_path *path, unsigned err_flags)

@@ -995,8 +1030,11 @@ void dm_pg_init_complete(struct dm_path 
 	if (err_flags & MP_BYPASS_PG)
 		bypass_pg(m, pg, 1);
 
+	if (err_flags & MP_RETRY)
+		retry_pg(m, pgpath);
+
 	spin_lock_irqsave(&m->lock, flags);
-	if (err_flags) {
+	if (err_flags & ~MP_RETRY) {
 		m->current_pgpath = NULL;
 		m->current_pg = NULL;
 	} else if (!m->pg_init_required)

@@ -1149,8 +1187,13 @@ static int multipath_status(struct dm_ta
 	/* Features */
 	if (type == STATUSTYPE_INFO)
 		DMEMIT("1 %u ", m->queue_size);
-	else if (m->queue_if_no_path)
+	else if (m->queue_if_no_path && !m->pg_init_retries)
 		DMEMIT("1 queue_if_no_path ");
+	else if (!m->queue_if_no_path && m->pg_init_retries)

Quoted 8 lines

drivers/md/dm-mpath.c static int multipath_status(struct dm_ta · line 1198

Quoted 8 lines

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

+		DMEMIT("2 pg_init_retries %u ", m->pg_init_retries);
+	else if (m->queue_if_no_path && m->pg_init_retries)
+		DMEMIT("3 queue_if_no_path pg_init_retries %u ",
+			m->pg_init_retries);
 	else
 		DMEMIT("0 ");
 

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

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

Quoted 3 lines · Chandra Seetharaman

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

I agree with Chandra on that one though.

[email protected] wrote:

  struct hp_sw_context {
  	unsigned char sense[SCSI_SENSE_BUFFERSIZE];

(quoted diff) +1 -0

+	unsigned argc;
  };

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

Also do not forget your signed off line.

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

Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================

drivers/md/dm-hp-sw.c +35 -2

@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"

@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.

@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {

drivers/md/dm-hp-sw.c +5 -6

 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);

@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)

@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 
 	return r;
 }

Quoted 77 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================

drivers/md/dm-hp-sw.c +35 -2

@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"

@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.

@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 90

Quoted 8 lines

drivers/md/dm-hp-sw.c +3 -2

@@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);

mixed use of space and tabs

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 98

Quoted 9 lines

drivers/md/dm-hp-sw.c +6 -1

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

space in front of label

Quoted 32 lines

drivers/md/dm-hp-sw.c +5 -6

 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);

@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)

@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 

drivers/md/dm-hp-sw.c static int __init hp_sw_init(void) · line 225

Quoted 3 lines

drivers/md/dm-hp-sw.c +0 -0

 	return r;
 }

Quoted 132 lines · Chandra Seetharaman

Quoted 77 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================

drivers/md/dm-hp-sw.c +35 -2

@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"

@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.

@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 90

Quoted 8 lines

drivers/md/dm-hp-sw.c +3 -2

@@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);

mixed use of space and tabs

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 97

Quoted 9 lines

drivers/md/dm-hp-sw.c +6 -1

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

space in front of label

Quoted 32 lines

drivers/md/dm-hp-sw.c +5 -6

 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);

@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)

@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 

Quoted 3 lines

drivers/md/dm-hp-sw.c static int __init hp_sw_init(void) · line 224

Quoted 3 lines

drivers/md/dm-hp-sw.c +0 -0

 	return r;
 }

Attached should fix the whitespace issues mentioned.

Quoted 128 lines · Chandra Seetharaman

Quoted 75 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================

drivers/md/dm-hp-sw.c +35 -2

@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"

@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.

@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 90

Quoted 8 lines

drivers/md/dm-hp-sw.c +3 -2

@@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);

mixed use of space and tabs

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 97

Quoted 9 lines

drivers/md/dm-hp-sw.c +6 -1

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

space in front of label

Quoted 32 lines

drivers/md/dm-hp-sw.c +5 -6

 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);

@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)

@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 

Quoted 3 lines

drivers/md/dm-hp-sw.c static int __init hp_sw_init(void) · line 224

Quoted 3 lines

drivers/md/dm-hp-sw.c +0 -0

 	return r;
 }

Attached should fix the whitespace issues mentioned.

Quoted 131 lines · Dave Wysochanski

Quoted 130 lines · Chandra Seetharaman

Quoted 77 lines · On Thu, 2007-07-26 at 00:44 -0400, [email protected] wrote:

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

Index: linux-2.6.23-rc1/drivers/md/dm-hp-sw.c
===================================================================

drivers/md/dm-hp-sw.c +35 -2

@@ -18,6 +18,7 @@
 #include <linux/types.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
 
 #include "dm.h"
 #include "dm-hw-handler.h"

@@ -26,8 +27,42 @@
 
 struct hp_sw_context {
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	unsigned argc;
 };
 
+/**
+ * hp_sw_error_is_retryable - Is an HP-specific check condition retryable?
+ * @req: path activation request
+ *
+ * Examine error codes of request and determine whether the error is retryable.
+ * Some error codes are already retried by scsi-ml (see
+ * scsi_decide_disposition), but some HP specific codes are not.
+ * The intent of this routine is to supply the logic for the HP specific
+ * check conditions.
+ *
+ * Returns:
+ *  1 - command completed with retryable error
+ *  0 - command completed with non-retryable error
+ *
+ * Possible optimizations
+ * 1. More hardware-specific error codes
+ */
+static int hp_sw_error_is_retryable(struct request *req)
+{
+	/*
+	 * NOT_READY is known to be retryable
+	 * For now we just dump out the sense data and call it retryable
+	 */
+	if (status_byte(req->errors) == CHECK_CONDITION)
+		__scsi_print_sense("hp_sw", req->sense, req->sense_len);
+
+	/*
+	 * At this point we don't have complete information about all the error
+	 * codes from this hardware, so we are just conservative and retry
+	 * when in doubt.
+	 */
+	return 1;
+}
 
 /**
  * hp_sw_end_io - Completion handler for HP path activation.

@@ -39,8 +74,6 @@ struct hp_sw_context {
  *
  * Context: scsi-ml softirq
  *
- * Possible optimizations
- * 1. Actually check sense data for retryable error (e.g. NOT_READY)
  */
 static void hp_sw_end_io(struct request *req, int error)
 {

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 90

Quoted 8 lines

drivers/md/dm-hp-sw.c +3 -2

@@ -52,11 +85,17 @@ static void hp_sw_end_io(struct request 
 		DMDEBUG("%s path activation command - success",
 		       	path->dev->name);
 	} else {
-		DMWARN("path activation command on %s - error=0x%x",
-		       path->dev->name, error);
+		if (hp_sw_error_is_retryable(req)) {
+			DMDEBUG("%s path activation command retry",
+			       path->dev->name);

mixed use of space and tabs

drivers/md/dm-hp-sw.c static void hp_sw_end_io(struct request · line 98

Quoted 9 lines

drivers/md/dm-hp-sw.c +6 -1

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

space in front of label

Quoted 32 lines

drivers/md/dm-hp-sw.c +5 -6

 	req->end_io_data = NULL;
 	__blk_put_request(req->q, req);
 	dm_pg_init_complete(path, err_flags);

@@ -134,17 +173,16 @@ static void hp_sw_pg_init(struct hw_hand
 	if (!req) {
 		DMERR("%s path activation command allocation fail ",
 		      path->dev->name);
-		goto fail;
+		goto retry;
 	}
 
-	DMDEBUG("path activation command sent on %s",
-		path->dev->name);
+	DMDEBUG("%s path activation command sent", path->dev->name);
 
 	blk_execute_rq_nowait(req->q, NULL, req, 1, hp_sw_end_io);
 	return;
 
- fail:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
+ retry:
+	dm_pg_init_complete(path, MP_RETRY);
 }
 
 static int hp_sw_create(struct hw_handler *hwh, unsigned argc, char **argv)

@@ -181,7 +219,7 @@ static int __init hp_sw_init(void)
 	if (r < 0)
 		DMERR("register failed %d", r);
 	else
-		DMINFO("version 0.0.2 loaded");
+		DMINFO("version 0.0.3 loaded");
 

Quoted 6 lines

Quoted 3 lines

drivers/md/dm-hp-sw.c static int __init hp_sw_init(void) · line 225

Quoted 3 lines

drivers/md/dm-hp-sw.c +0 -0

 	return r;
 }

Attached should fix the whitespace issues mentioned.

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