Hi Dave, some coding style related comments (below). On Thu, 2007-07-26 at 00:44 -0400, dwysocha@xxxxxxxxxx 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@xxxxxxxxxxx>"); > +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@xxxxxxxxxx | .......you may get it. ---------------------------------------------------------------------- -- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel