1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Edward Goggin]

This is the 2nd take on a set of patches submitted about
3 weeks ago intended to (1) avoid unnecessary dm-multipath
path group switch-over events for EMC CLARiiON and
(2) improve the event logging of the dm-multipath hardware
handler interface.

This patch set differs from previous patch set by:

- There are now two patches -- the first for content
specific to the EMC CLARiiON dm-multipath hardware
handler dm_emc.c. -- the second to dm, dm-multipath,
and dm-multipath hardware handler framework code to
support an improved event logging capability for 
hardware handlers.

- The dm_emc.c hw handler now gets requests from
blk_get_request instead of using pre-allocated requests.
As a result, the rq_init macro of ll_rw_blk.c is no
longer converted to a function and exported.

- The dm_emc.c hw handler now issues its ios asynchronously
via blk_execute_rq_nowait and has a single threaded worker
queue to allocate and queue the io requests.

- The 2nd patch exports dm_put instead of a new function
dm_table_put_md.

- The 2nd patch modifies the dm-multipath hardware handler
interface by adding a 4th parameter, a ptr to the mapped
device, to the create callout instead of adding a new init
callout.  dm_mpath.c and dm_emc.c are changed to pass and
use this additional parameter respectively.

- Coding and patch conventions followed.
	use "diff -up"
	no lines of code > 80 chars
	avoids mixed case variable names
	start else clause on same line as "}"
	EXPORT_SYMBOL_GPL reference immediately after function

This is the first patch (dm_emc.c only) in a set of 2 patches.

Signed-off-by: Ed Goggin <egoggin@emc.com>
--- linux-2.6.18-rc4/drivers/md/dm-emc.c.orig	2006-08-28
22:37:34.000000000 -0500
+++ linux-2.6.18-rc4/drivers/md/dm-emc.c	2006-08-28
22:20:17.000000000 -0500
@@ -7,227 +7,448 @@
  * Multipath support for EMC CLARiiON AX/CX-series hardware.
  */
 
+/* Code borrowed from dm-lsi-rdac by Mike Christie */
+
 #include "dm.h"
 #include "dm-hw-handler.h"
 #include <scsi/scsi.h>
+#include <scsi/scsi_eh.h>
 #include <scsi/scsi_cmnd.h>
 
-#define DM_MSG_PREFIX "multipath emc"
+#define DM_MSG_PREFIX		"multipath emc"
+#define TRESPASS_PAGE		0x22
+#define BUFFER_SIZE		0x80
+#define EMC_HANDLER_TIMEOUT	(60 * HZ)
+#define UNBOUND_LU		-1
 
+/*
+ * Four variations of the CLARiiON trespass MODE_SENSE page.
+ */
+unsigned char long_trespass_and_hr_pg[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,        /* Page code */
+	0x09,                 /* Page length - 2 */
+	0x01,  		      /* Trespass code + Honor reservation bit
*/
+	0xff, 0xff,           /* Trespass target */
+	0, 0, 0, 0, 0, 0      /* Reserved bytes / unknown */
+};
+unsigned char long_trespass_pg[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,        /* Page code */
+	0x09,                 /* Page length - 2 */
+	0x81,  		      /* Trespass code + Honor reservation bit
*/
+	0xff, 0xff,           /* Trespass target */
+	0, 0, 0, 0, 0, 0      /* Reserved bytes / unknown */
+};
+unsigned char short_trespass_and_hr_pg[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,        /* Page code */
+	0x02,                 /* Page length - 2 */
+	0x01,  		      /* Trespass code + Honor reservation bit
*/
+	0xff,                 /* Trespass target */
+};
+unsigned char short_trespass_pg[] = {
+	0, 0, 0, 0,
+	TRESPASS_PAGE,        /* Page code */
+	0x02,                 /* Page length - 2 */
+	0x81,  		      /* Trespass code + Honor reservation bit
*/
+	0xff,                 /* Trespass target */
+};
+
+/*
+ * EMC hardware handler context structure containing CLARiiON LU
specific
+ * information for a particular dm multipath mapped device.
+ */
 struct emc_handler {
 	spinlock_t lock;
-
-	/* Whether we should send the short trespass command (FC-series)
-	 * or the long version (default for AX/CX CLARiiON arrays). */
+	struct hw_handler *hwh;
+	struct path *path;
+	struct work_struct wkq;
+	/* Use short trespass command (FC-series) or the long version
+	 * (default for AX/CX CLARiiON arrays). */
 	unsigned short_trespass;
-	/* Whether or not to honor SCSI reservations when initiating a
-	 * switch-over. Default: Don't. */
+	/* Whether or not (default) to honor SCSI reservations when
+	 * initiating a switch-over. */
 	unsigned hr;
-
+	/* I/O buffer for both MODE_SELECT and INQUIRY commands. */
+	char buffer[BUFFER_SIZE];
+	/* SCSI sense buffer for commands -- assumes serial issuance
+	 * and completion sequence of all commands for same multipath.
*/
 	unsigned char sense[SCSI_SENSE_BUFFERSIZE];
+	/* which SP (A=0,B=1,UNBOUND=-1) is dflt SP for path's mapped
dev */
+	int default_sp;
+	/* which SP (A=0,B=1,UNBOUND=-1) is active for path's mapped dev
*/
+	int current_sp;
+	/*
+	 * flag when set (reset) differentiates get_sp_info after
(before)
+	 * pg_init
+	 */
+	int pg_init_sent;
 };
 
-#define TRESPASS_PAGE 0x22
-#define EMC_FAILOVER_TIMEOUT (60 * HZ)
+static int send_cmd(struct emc_handler *h, struct path *path, int cmd,
+		    rq_end_io_fn *endio);
+static rq_end_io_fn sp_info_endio, pg_init_endio;
+static int parse_sp_info_reply(struct request *req, struct emc_handler
*h,
+			       struct path *path, int *default_sp,
+			       int *current_sp, int *new_current_sp);
+static int parse_pg_init_reply(struct request *req, struct emc_handler
*h,
+			       struct path *path);
 
-/* Code borrowed from dm-lsi-rdac by Mike Christie */
+struct workqueue_struct *kemchd;
 
-static inline void free_bio(struct bio *bio)
-{
-	__free_page(bio->bi_io_vec[0].bv_page);
-	bio_put(bio);
+/*
+ * Parse EVPD 0xC0 INQUIRY cmd reply.
+ */
+static int parse_sp_info_reply(struct request *req, struct emc_handler
*h,
+			       struct path *path, int *default_sp,
+			       int *current_sp, int *new_current_sp)
+{
+	int err = 0;
+
+	if (req->errors == 0) {
+		/* check for in-progress ucode upgrade (NDU) */
+		if (h->buffer[48] != 0) {
+			DMWARN("Detected in-progress ucode upgrade NDU "
+			       "operation while finding current active "
+			       "SP using path %s.", path->dev->name);
+			err = MP_BYPASS_PG;
+		} else {
+			*default_sp = h->buffer[5];
+
+			if (h->buffer[4] == 2)
+				/* SP for path (in h->buffer[8]) is
current */
+				*current_sp = h->buffer[8];
+			else {
+				if (h->buffer[4] == 1)
+					/* SP for this path is NOT
current */
+					if (h->buffer[8] == 0)
+						*current_sp = 1;
+					else
+						*current_sp = 0;
+				else
+					/* unbound LU or LUNZ */
+					*current_sp = UNBOUND_LU;
+			}
+			*new_current_sp =  h->buffer[8];
+		}
+	} else {
+		struct scsi_sense_hdr sshdr;
+
+		err = MP_FAIL_PATH;
+
+		if (req->sense_len && scsi_normalize_sense(req->sense,
+
req->sense_len,
+							   &sshdr))
+			DMERR("Found valid sense data 0x%2x, 0x%2x,
0x%2x "
+			      "while finding current active SP using
path %s.",
+		       	      sshdr.sense_key, sshdr.asc, sshdr.ascq,
+			      path->dev->name);
+		else DMERR("Error 0x%x finding current active SP using "
+		     	   "path %s.", req->errors, path->dev->name);
+	}
+
+	return (err);
 }
 
-static int emc_endio(struct bio *bio, unsigned int bytes_done, int
error)
+/*
+ * Parse MODE_SELECT cmd reply.
+ */
+static int parse_pg_init_reply(struct request *req, struct emc_handler
*h,
+			       struct path *path)
 {
-	struct path *path = bio->bi_private;
+	struct scsi_sense_hdr sshdr;
+	int err = 0;
 
-	if (bio->bi_size)
-		return 1;
+	if (req->sense_len && scsi_normalize_sense(req->sense,
+						   req->sense_len,
&sshdr)) {
 
-	/* We also need to look at the sense keys here whether or not to
-	 * switch to the next PG etc.
-	 *
-	 * For now simple logic: either it works or it doesn't.
-	 */
-	if (error)
-		dm_pg_init_complete(path, MP_FAIL_PATH);
-	else
-		dm_pg_init_complete(path, 0);
+		DMERR("Found valid sense data 0x%2x, 0x%2x, 0x%2x while
"
+		      "sending CLARiiON trespass command using path
%s.",
+		      sshdr.sense_key, sshdr.asc, sshdr.ascq,
path->dev->name);
+
+		if ((sshdr.sense_key = 0x05) &&
+	    	    (sshdr.asc = 0x04) &&
+	    	    (sshdr.ascq = 0x00)) {
+			/*
+			 * Array based copy in progress -- do not send
+			 * pg_init or copy will be aborted mid-stream.
+			 */
+			DMWARN("Array Based Copy in progress while "
+			       "sending CLARiiON trespass command "
+			       "using path %s.", path->dev->name);
+			err = MP_BYPASS_PG;
+		} else if ((sshdr.sense_key = 0x02) &&
+	    	    	 (sshdr.asc = 0x04) &&
+	    	    	 (sshdr.ascq = 0x03)) {
+			/*
+			 * LUN Not Ready - Manual Intervention Required
+			 * indicates in-progress ucode upgrade (NDU).
+			 */
+			DMWARN("Detected in-progress ucode upgrade NDU "
+			       "operation while sending CLARiiON
trespass "
+			       " command using path %s.",
path->dev->name);
+			err = MP_BYPASS_PG;
+		} else
+			err = MP_FAIL_PATH;
+	} else if (req->errors) {
+		DMERR("Error 0x%x while sending CLARiiON trespass
command "
+		      "using path %s.", req->errors, path->dev->name);
+		err = MP_FAIL_PATH;
+	}
 
-	/* request is freed in block layer */
-	free_bio(bio);
+	/* release ref on block request */
+	__blk_put_request(req->q, req);
 
-	return 0;
+	return (err);
 }
 
-static struct bio *get_failover_bio(struct path *path, unsigned
data_size)
+/*
+ * Completion handler for EVPD page 0xC0 INQUIRY cmd.
+ */
+static void sp_info_endio(struct request *req, int uptodate)
 {
-	struct bio *bio;
-	struct page *page;
+	struct path *path = req->end_io_data;
+	struct emc_handler *h = (struct emc_handler *)path->hwhcontext;
+	int default_sp, current_sp, new_current_sp;
+	unsigned long flags;
+	int err_flags;
 
-	bio = bio_alloc(GFP_ATOMIC, 1);
-	if (!bio) {
-		DMERR("get_failover_bio: bio_alloc() failed.");
-		return NULL;
-	}
+	if ((err_flags = parse_sp_info_reply(req, h, path, &default_sp,
+					     &current_sp,
&new_current_sp))) {
+		dm_pg_init_complete(path, err_flags);
 
-	bio->bi_rw |= (1 << BIO_RW);
-	bio->bi_bdev = path->dev->bdev;
-	bio->bi_sector = 0;
-	bio->bi_private = path;
-	bio->bi_end_io = emc_endio;
-
-	page = alloc_page(GFP_ATOMIC);
-	if (!page) {
-		DMERR("get_failover_bio: alloc_page() failed.");
-		bio_put(bio);
-		return NULL;
+		/* release ref on block request */
+		__blk_put_request(req->q, req);
+
+		return;
 	}
 
-	if (bio_add_page(bio, page, data_size, 0) != data_size) {
-		DMERR("get_failover_bio: alloc_page() failed.");
-		__free_page(page);
-		bio_put(bio);
-		return NULL;
+	/* release ref on block request */
+	__blk_put_request(req->q, req);
+
+	spin_lock_irqsave(&h->lock, flags);
+	if (h->pg_init_sent) {
+		h->default_sp = default_sp;
+		h->current_sp = current_sp;
+		h->pg_init_sent = 0;
+		spin_unlock_irqrestore(&h->lock, flags);
+
+		/* done now */
+		dm_pg_init_complete(path, 0);
+	} else {
+		spin_unlock_irqrestore(&h->lock, flags);
+
+		/*
+	 	 * Do not issue the actual pg_init request if either (1)
+	 	 * we do not know the identity of the current SP or (2)
+		 * the prospective new SP is already current.
+	 	 */
+		if ((current_sp != UNBOUND_LU) &&
+		    (new_current_sp == current_sp)) {
+
+			spin_lock_irqsave(&h->lock, flags);
+			if (h->default_sp == UNBOUND_LU) {
+				h->default_sp = default_sp;
+				h->current_sp = current_sp;
+			}
+			spin_unlock_irqrestore(&h->lock, flags);
+
+			/* yet, its as good as doing it */
+			dm_pg_init_complete(path, 0);
+			DMINFO("Ignoring path group switch-over command
for "
+			       "CLARiiON SP%s since mapped device is "
+			       "already initialized for path %s.",
+		       	       current_sp ? "B" : "A", path->dev->name);
+		} else {
+			/* send path initialization request */
+			DMINFO("Issuing CLARiiON trespass command to "
+	       		       "activate SP%s using path %s.",
+			       new_current_sp ? "B" : "A",
path->dev->name);
+
+			h->path = path;	/* kemchd will use this path */
+			queue_work(kemchd, &h->wkq);
+		}
 	}
 
-	return bio;
+	return;
+}
+
+/*
+ * Completion handler for MODE_SELECT cmd.
+ */
+static void pg_init_endio(struct request *req, int uptodate)
+{
+	struct path *path = req->end_io_data;
+	struct emc_handler *h = (struct emc_handler *)path->hwhcontext;
+	int err_flags;
+
+	if ((err_flags = parse_pg_init_reply(req, h, path)))
+		dm_pg_init_complete(path, err_flags);
+
+	/* release ref on block request */
+	__blk_put_request(req->q, req);
+
+	/* send sp_info request */
+	h->pg_init_sent = 1;
+	queue_work(kemchd, &h->wkq);
+
+	return;
 }
 
-static struct request *get_failover_req(struct emc_handler *h,
-					struct bio *bio, struct path
*path)
+/*
+ * Get block request for REQ_BLOCK_PC command issued to path.
Currently
+ * limited to MODE_SELECT (trespass) and INQUIRY (VPD page 0xC0)
commands.
+ *
+ * Uses data and sense buffers in hardware handler context structure
and
+ * assumes serial servicing of commands, both issuance and completion.
+ */
+static struct request *get_req(struct path *path, int opcode)
 {
+	struct emc_handler *h = (struct emc_handler *)path->hwhcontext;
+	struct request_queue *q = bdev_get_queue(path->dev->bdev);
 	struct request *rq;
-	struct block_device *bdev = bio->bi_bdev;
-	struct request_queue *q = bdev_get_queue(bdev);
+	void *buffer;
+	int len = 0;
 
-	/* FIXME: Figure out why it fails with GFP_ATOMIC. */
-	rq = blk_get_request(q, WRITE, __GFP_WAIT);
+	rq = blk_get_request(q, (opcode == MODE_SELECT) ? WRITE : READ,
+			     __GFP_WAIT);
 	if (!rq) {
-		DMERR("get_failover_req: blk_get_request failed");
+		DMERR("dm-emc: get_req: blk_get_request failed");
 		return NULL;
 	}
 
-	rq->bio = rq->biotail = bio;
-	blk_rq_bio_prep(q, rq, bio);
-
-	rq->rq_disk = bdev->bd_contains->bd_disk;
+	memset(&rq->cmd, 0, BLK_MAX_CDB);
+	rq->cmd[0] = opcode;
+	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
 
-	/* bio backed don't set data */
-	rq->buffer = rq->data = NULL;
-	/* rq data_len used for pc cmd's request_bufflen */
-	rq->data_len = bio->bi_size;
+	switch (opcode) {
+	case MODE_SELECT:
+		rq->flags |= REQ_RW;
+		rq->cmd[1] = 0x10;
+		len = h->short_trespass ?
sizeof(short_trespass_and_hr_pg) :
+				        sizeof(long_trespass_and_hr_pg);
+		buffer = h->short_trespass ?
+			       h->hr ? short_trespass_and_hr_pg
+				     : short_trespass_pg
+			     :
+			       h->hr ? long_trespass_and_hr_pg
+				     : long_trespass_pg;
+		/*
+		 * Can't DMA from kernel BSS -- must copy selected
trespass
+		 * command mode page contents to context buffer which is
+		 * allocated by kmalloc.
+		 */
+		BUG_ON((len > BUFFER_SIZE));
+		memcpy(h->buffer, buffer, len);
+		break;
+	case INQUIRY:
+		rq->cmd[1] = 0x1;
+		rq->cmd[2] = 0xC0;
+		len = BUFFER_SIZE;
+		memset(h->buffer, 0, BUFFER_SIZE);
+		break;
+	default:
+		BUG_ON(1);
+		break;
+	}
+	rq->cmd[4] = len;
+
+	rq->buffer = rq->data = h->buffer;
+	rq->data_len = len;
+	rq->bio = rq->biotail = NULL;
 
 	rq->sense = h->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
 
-	memset(&rq->cmd, 0, BLK_MAX_CDB);
-
-	rq->timeout = EMC_FAILOVER_TIMEOUT;
-	rq->flags |= (REQ_BLOCK_PC | REQ_FAILFAST | REQ_NOMERGE);
+	rq->flags |= (REQ_BLOCK_PC | REQ_FAILFAST);
+	rq->timeout = EMC_HANDLER_TIMEOUT;
+	rq->retries = 0;
 
 	return rq;
 }
 
-static struct request *emc_trespass_get(struct emc_handler *h,
-					struct path *path)
+/*
+ * Send cmd and setup asynchronous i/o completion handler.
+ */
+static int send_cmd(struct emc_handler *h, struct path *path, int cmd,
+		    rq_end_io_fn *endio)
 {
-	struct bio *bio;
+	struct request_queue *q = bdev_get_queue(path->dev->bdev);
 	struct request *rq;
-	unsigned char *page22;
-	unsigned char long_trespass_pg[] = {
-		0, 0, 0, 0,
-		TRESPASS_PAGE,        /* Page code */
-		0x09,                 /* Page length - 2 */
-		h->hr ? 0x01 : 0x81,  /* Trespass code + Honor
reservation bit */
-		0xff, 0xff,           /* Trespass target */
-		0, 0, 0, 0, 0, 0      /* Reserved bytes / unknown */
-		};
-	unsigned char short_trespass_pg[] = {
-		0, 0, 0, 0,
-		TRESPASS_PAGE,        /* Page code */
-		0x02,                 /* Page length - 2 */
-		h->hr ? 0x01 : 0x81,  /* Trespass code + Honor
reservation bit */
-		0xff,                 /* Trespass target */
-		};
-	unsigned data_size = h->short_trespass ?
sizeof(short_trespass_pg) :
-				sizeof(long_trespass_pg);
-
-	/* get bio backing */
-	if (data_size > PAGE_SIZE)
-		/* this should never happen */
-		return NULL;
-
-	bio = get_failover_bio(path, data_size);
-	if (!bio) {
-		DMERR("emc_trespass_get: no bio");
-		return NULL;
-	}
-
-	page22 = (unsigned char *)bio_data(bio);
-	memset(page22, 0, data_size);
 
-	memcpy(page22, h->short_trespass ?
-		short_trespass_pg : long_trespass_pg, data_size);
+	/* get and initialize block request */
+	rq = get_req(path, cmd);
+	if (!rq)
+		return MP_FAIL_PATH;
+
+	/* issue the cmd asynchronously (at head of q) */
+	rq->end_io_data = path;
+	blk_execute_rq_nowait(q, NULL, rq, 1, endio);
+	return 0;
+}
 
-	/* get request for block layer packet command */
-	rq = get_failover_req(h, bio, path);
-	if (!rq) {
-		DMERR("emc_trespass_get: no rq");
-		free_bio(bio);
-		return NULL;
+/*
+ * Work queue service routine used to issue i/o without already holding
+ * a request queue lock.
+ */
+static void service_wkq(void *data)
+{
+	struct emc_handler *h = (struct emc_handler *)data;
+	struct path *path = h->path;
+	int err_flags;
+
+	if (h->pg_init_sent) {
+		if ((err_flags = send_cmd(h, path, INQUIRY,
+					  sp_info_endio))) {
+			h->pg_init_sent = 0;
+			dm_pg_init_complete(path, err_flags);
+		}
+	}
+	else {
+		/* send pg_init request */
+		if ((err_flags = send_cmd(h, path, MODE_SELECT,
+					  pg_init_endio)))
+			dm_pg_init_complete(path, err_flags);
 	}
-
-	/* Prepare the command. */
-	rq->cmd[0] = MODE_SELECT;
-	rq->cmd[1] = 0x10;
-	rq->cmd[4] = data_size;
-	rq->cmd_len = COMMAND_SIZE(rq->cmd[0]);
-
-	return rq;
 }
 
+/* initialize path group command */
 static void emc_pg_init(struct hw_handler *hwh, unsigned bypassed,
 			struct path *path)
 {
-	struct request *rq;
-	struct request_queue *q = bdev_get_queue(path->dev->bdev);
+	struct emc_handler *h = (struct emc_handler *)hwh->context;
+	int err_flags;
 
-	/*
-	 * We can either blindly init the pg (then look at the sense),
-	 * or we can send some commands to get the state here (then
-	 * possibly send the fo cmnd), or we can also have the
-	 * initial state passed into us and then get an update here.
-	 */
-	if (!q) {
-		DMINFO("emc_pg_init: no queue");
-		goto fail_path;
-	}
+	path->hwhcontext = h; /* needed by endio handlers */
 
-	/* FIXME: The request should be pre-allocated. */
-	rq = emc_trespass_get(hwh->context, path);
-	if (!rq) {
-		DMERR("emc_pg_init: no rq");
-		goto fail_path;
-	}
-
-	DMINFO("emc_pg_init: sending switch-over command");
-	elv_add_request(q, rq, ELEVATOR_INSERT_FRONT, 1);
+	if ((err_flags = send_cmd(h, path, INQUIRY, sp_info_endio)))
+		dm_pg_init_complete(path, err_flags);
 	return;
-
-fail_path:
-	dm_pg_init_complete(path, MP_FAIL_PATH);
 }
 
-static struct emc_handler *alloc_emc_handler(void)
+static struct emc_handler *alloc_emc_handler(unsigned int
short_trespass,
+					     unsigned hr)
 {
-	struct emc_handler *h = kmalloc(sizeof(*h), GFP_KERNEL);
+	struct emc_handler *h = kzalloc(sizeof(*h), GFP_KERNEL);
 
 	if (h) {
-		memset(h, 0, sizeof(*h));
 		spin_lock_init(&h->lock);
+
+		INIT_WORK(&h->wkq, service_wkq, h);
+
+		if ((h->short_trespass = short_trespass))
+			DMINFO("Short trespass command to be sent.");
+		else
+			DMINFO("Long trespass command to be sent
(default).");
+		if ((h->hr = hr))
+			DMINFO("Honor reservation bit will be set.");
+		else
+			DMINFO("Honor reservation bit is not set
(default).");
+
+		h->default_sp = UNBOUND_LU;
+		h->current_sp = UNBOUND_LU;
 	}
 
 	return h;
@@ -243,37 +464,30 @@ static int emc_create(struct hw_handler 
 		hr = 0;
 		short_trespass = 0;
 	} else if (argc != 2) {
-		DMWARN("incorrect number of arguments");
+		DMWARN("Incorrect number (0x%x) of arguments.  "
+		       "Should be 2.", argc);
 		return -EINVAL;
 	} else {
 		if ((sscanf(argv[0], "%u", &short_trespass) != 1)
 			|| (short_trespass > 1)) {
-			DMWARN("invalid trespass mode selected");
+			DMWARN("Invalid trespass mode (0x%x) selected.",
+			       short_trespass);
 			return -EINVAL;
 		}
 
 		if ((sscanf(argv[1], "%u", &hr) != 1)
 			|| (hr > 1)) {
-			DMWARN("invalid honor reservation flag
selected");
+			DMWARN("Invalid honor reservation flag (0x%x).",
hr);
 			return -EINVAL;
 		}
 	}
 
-	h = alloc_emc_handler();
+	h = alloc_emc_handler(short_trespass, hr);
 	if (!h)
 		return -ENOMEM;
 
 	hwh->context = h;
-
-	if ((h->short_trespass = short_trespass))
-		DMWARN("short trespass command will be send");
-	else
-		DMWARN("long trespass command will be send");
-
-	if ((h->hr = hr))
-		DMWARN("honor reservation bit will be set");
-	else
-		DMWARN("honor reservation bit will not be set
(default)");
+	h->hwh = hwh;
 
 	return 0;
 }
@@ -324,6 +538,31 @@ static unsigned emc_error(struct hw_hand
 	return dm_scsi_err_handler(hwh, bio);
 }
 
+static int emc_status(struct hw_handler *hwh, status_type_t type,
+		      char *result, unsigned int maxlen)
+{
+	struct emc_handler *h = (struct emc_handler *)hwh->context;
+	unsigned long flags;
+
+	int sz = 0;
+
+	spin_lock_irqsave(&h->lock, flags);
+
+	if (type == STATUSTYPE_INFO)
+		DMEMIT("2 %d %d ", h->default_sp, h->current_sp);
+	else {
+		if (h->short_trespass || h->hr)
+			DMEMIT("3 %s %u %u ", hwh->type->name,
+			       h->short_trespass, h->hr);
+		else
+			DMEMIT("1 %s ", hwh->type->name);
+	}
+
+	spin_unlock_irqrestore(&h->lock, flags);
+
+	return sz;
+}
+
 static struct hw_handler_type emc_hwh = {
 	.name = "emc",
 	.module = THIS_MODULE,
@@ -331,6 +570,7 @@ static struct hw_handler_type emc_hwh = 
 	.destroy = emc_destroy,
 	.pg_init = emc_pg_init,
 	.error = emc_error,
+	.status = emc_status,
 };
 
 static int __init dm_emc_init(void)
@@ -338,19 +578,30 @@ static int __init dm_emc_init(void)
 	int r = dm_register_hw_handler(&emc_hwh);
 
 	if (r < 0)
-		DMERR("register failed %d", r);
+		DMERR("Register failed %d.", r);
+	else {
+		kemchd = create_singlethread_workqueue("kemchd");
+		if (!kemchd) {
+			DMERR("Failed to create workqueue kemchd.");
+			dm_unregister_hw_handler(&emc_hwh);
+			return -ENOMEM;
+		}
+	}
 
-	DMINFO("version 0.0.3 loaded");
+	DMINFO("Version 0.0.4 loaded.");
 
 	return r;
 }
 
 static void __exit dm_emc_exit(void)
 {
-	int r = dm_unregister_hw_handler(&emc_hwh);
+	int r;
+
+	destroy_workqueue(kemchd);
 
+	r = dm_unregister_hw_handler(&emc_hwh);
 	if (r < 0)
-		DMERR("unregister failed %d", r);
+		DMERR("Unregister failed %d.", r);
 }
 
 module_init(dm_emc_init);

Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Mike Christie]

Edward Goggin wrote:
> +
> +	rq->buffer = rq->data = h->buffer;
> +	rq->data_len = len;
> +	rq->bio = rq->biotail = NULL;
>  

I think I only suggested that you use the block layer map functions in
the previous review. Now I will say, use them and fix the core code to
not allocate memory or use a mempool since it will also fix the path
testers at the same time :) The reason is that the scsi layer is trying
to make every thing run with scatterlists. In 2.6.18, every place is
converted (they should be at least), so you should not be adding another
place where we send down a data buffer like this.

Re: Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Mike Christie]

Mike Christie wrote:
> Edward Goggin wrote:
>> +
>> +	rq->buffer = rq->data = h->buffer;
>> +	rq->data_len = len;
>> +	rq->bio = rq->biotail = NULL;
>>  
> 
> I think I only suggested that you use the block layer map functions in
> the previous review. Now I will say, use them and fix the core code to
> not allocate memory or use a mempool since it will also fix the path
> testers at the same time :) The reason is that the scsi layer is trying
> to make every thing run with scatterlists. In 2.6.18, every place is
> converted (they should be at least), so you should not be adding another
> place where we send down a data buffer like this.
> 

And if we are going to continue to do some scsi processing in dm I
already did some helpers you could base yourself off of here

http://www.cs.wisc.edu/~michaelc/block/dm/v4/


In dm_scsi_end_rq, you could handle errors like transient transport
failures for all hw handlers at one tine.

http://www.cs.wisc.edu/~michaelc/block/dm/v4/02-dm-scsi-helpers.patch

And in fact a lot of your patch has already been done with this patch

http://www.cs.wisc.edu/~michaelc/block/dm/v4/03-emc.patch a year ago.

We really need to change how dm-multipath development works IMHO.
Alasdair is just too overworked. It is not fair to him to have so much
work piled on top of him and not fair to the community to have problems
like this sit around for so long.

The basics of the hw handler framework were done when I did my pg group
callouts that improved on Joe's idea to put everything in the path
selectors. It took a year after that to get hw handlers, which made some
of the same architectural mistakes as well as odd coding choices that I
made, in the kernel. And now for another year we have been sitting on
these type of bugs and problems where we need some of the hw handler
functionality in the scsi layer as well as dm layer.

With Alasdair having to maintain every driver that dm handles and those
drivers increasing in complexity the work load is too large for one
person and this makes coordination with other subsystems impossible.
This is a large problem, when for something like dm-multipath it needs
help from the block layer, and low levels like scsi.

We really need a system like in the scsi layer where there are driver
maintainers and then one core maintainer of the subsystem. The core
maintainer has final (well ok community has final say I guess) say but
trusts the low level driver maintainers to some extent. The low level
driver maintainers still have to post patches for review to the
community and maintainer but the core maintainer does not have to do a
line by line review of the dm target module and learn all the lower
level details of the sub-subsytem/lld/dm target module/transport/spec
unless it affects dm core or it is a suspicious patch.

Just my 2 cents.

Re: Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Hannes Reinecke]

Mike Christie wrote:
[ .. ]
> We really need to change how dm-multipath development works IMHO.
> Alasdair is just too overworked. It is not fair to him to have so much
> work piled on top of him and not fair to the community to have problems
> like this sit around for so long.
> 
[ .. ]
> With Alasdair having to maintain every driver that dm handles and those
> drivers increasing in complexity the work load is too large for one
> person and this makes coordination with other subsystems impossible.
> This is a large problem, when for something like dm-multipath it needs
> help from the block layer, and low levels like scsi.
> 
> We really need a system like in the scsi layer where there are driver
> maintainers and then one core maintainer of the subsystem. The core
> maintainer has final (well ok community has final say I guess) say but
> trusts the low level driver maintainers to some extent. The low level
> driver maintainers still have to post patches for review to the
> community and maintainer but the core maintainer does not have to do a
> line by line review of the dm target module and learn all the lower
> level details of the sub-subsytem/lld/dm target module/transport/spec
> unless it affects dm core or it is a suspicious patch.
> 
Totally agree.

Of course, Alasdairs vote's the one that counts ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke			hare@suse.de
SuSE Linux Products GmbH		S390 & zSeries
Maxfeldstraße 5				+49 911 74053 688
90409 Nürnberg				http://www.suse.de

RE: Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Edward Goggin]

On Friday, September 08, 2006 2:00 PM, Mike Christie wrote
> 
> Mike Christie wrote:
> > Edward Goggin wrote:
> >> +
> >> +	rq->buffer = rq->data = h->buffer;
> >> +	rq->data_len = len;
> >> +	rq->bio = rq->biotail = NULL;
> >>  
> > 
> > I think I only suggested that you use the block layer map 
> functions in
> > the previous review. Now I will say, use them and fix the 
> core code to
> > not allocate memory or use a mempool since it will also fix the path
> > testers at the same time :) The reason is that the scsi 
> layer is trying
> > to make every thing run with scatterlists. In 2.6.18, every place is
> > converted (they should be at least), so you should not be 
> adding another
> > place where we send down a data buffer like this.
> > 
> 
> And if we are going to continue to do some scsi processing in dm I
> already did some helpers you could base yourself off of here
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/

I was first trying to submit these changes to the upstream code
(I chose 2.6.18-rc4) since I this is what I thought you had
suggested that I do when we talked about it briefly at the dm bof
session at OLS.

Should I not be taking this tack?

> 
> 
> In dm_scsi_end_rq, you could handle errors like transient transport
> failures for all hw handlers at one tine.
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/02-dm-scsi-helpers.patch

Again, doesn't this imply a change from what we already agreed upon?
That is fine, I just need to know.

> 
> And in fact a lot of your patch has already been done with this patch
> 
> http://www.cs.wisc.edu/~michaelc/block/dm/v4/03-emc.patch a year ago.

I certainly read your patch set at that time it was sent since this is
how I learned about the availability of the blk_execute_rq_nowait
interface.  Yet, I certainly didn't mean to plagiarize any of your
code.  I apologize if I have inadvertently done so.  I definitely
remember having a difficult time getting the io initiated by dm-emc.c
to work at all with my changes.  Turns out I was trying to DMA from
kernel static sections (as is your code).  Yet, I don't remember
looking back at your patch set for inspiration to solve the problem.

The primary intention of my patch is to (1) allow dm-emc.c to
distinguish between dm and non-dm initiated multipath path group
switch events, (2) provide a dm-multipath hw-handler status callback
for dm-emc.c, and (3) provide a somewhat more informative error
logging capability for the dm-multipath hw-handler interface.

I was also intent on having the possibly multiple ios initiated by
dm-emc.c not involve allocating memory for request or bio structures
from mempools or otherwise.  Doing so is proving especially difficult.

> 
> We really need to change how dm-multipath development works IMHO.
> Alasdair is just too overworked. It is not fair to him to have so much
> work piled on top of him and not fair to the community to 
> have problems
> like this sit around for so long.
> 
> The basics of the hw handler framework were done when I did 
> my pg group
> callouts that improved on Joe's idea to put everything in the path
> selectors. It took a year after that to get hw handlers, 
> which made some
> of the same architectural mistakes as well as odd coding 
> choices that I
> made, in the kernel. And now for another year we have been sitting on
> these type of bugs and problems where we need some of the hw handler
> functionality in the scsi layer as well as dm layer.
> 
> With Alasdair having to maintain every driver that dm handles 
> and those
> drivers increasing in complexity the work load is too large for one
> person and this makes coordination with other subsystems impossible.
> This is a large problem, when for something like dm-multipath it needs
> help from the block layer, and low levels like scsi.
> 
> We really need a system like in the scsi layer where there are driver
> maintainers and then one core maintainer of the subsystem. The core
> maintainer has final (well ok community has final say I guess) say but
> trusts the low level driver maintainers to some extent. The low level
> driver maintainers still have to post patches for review to the
> community and maintainer but the core maintainer does not have to do a
> line by line review of the dm target module and learn all the lower
> level details of the sub-subsytem/lld/dm target module/transport/spec
> unless it affects dm core or it is a suspicious patch.
> 
> Just my 2 cents.

Good ideas.  This approach should scale more effectively.

> 

Re: Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Mike Christie]

Edward Goggin wrote:
> On Friday, September 08, 2006 2:00 PM, Mike Christie wrote
>> Mike Christie wrote:
>>> Edward Goggin wrote:
>>>> +
>>>> +	rq->buffer = rq->data = h->buffer;
>>>> +	rq->data_len = len;
>>>> +	rq->bio = rq->biotail = NULL;
>>>>  
>>> I think I only suggested that you use the block layer map 
>> functions in
>>> the previous review. Now I will say, use them and fix the 
>> core code to
>>> not allocate memory or use a mempool since it will also fix the path
>>> testers at the same time :) The reason is that the scsi 
>> layer is trying
>>> to make every thing run with scatterlists. In 2.6.18, every place is
>>> converted (they should be at least), so you should not be 
>> adding another
>>> place where we send down a data buffer like this.
>>>
>> And if we are going to continue to do some scsi processing in dm I
>> already did some helpers you could base yourself off of here
>>
>> http://www.cs.wisc.edu/~michaelc/block/dm/v4/
> 
> I was first trying to submit these changes to the upstream code
> (I chose 2.6.18-rc4) since I this is what I thought you had
> suggested that I do when we talked about it briefly at the dm bof
> session at OLS.
> 
> Should I not be taking this tack?
> 

I have no idea what Alasdair wants exactly and I have no idea when we
are going to do anything.

I thought you were going to all the trouble of redoing your patches and
fixing them up for review comments because you and alasdair switched and
were staying with the hw handlers completely in dm-multpath route, or I
thought the reason you were going to all this trouble to update your
code and go through the review process was because for RHEL 5, we were
staying with the hw handlers completely in dm-multipath route.


>>
>> In dm_scsi_end_rq, you could handle errors like transient transport
>> failures for all hw handlers at one tine.
>>
>> http://www.cs.wisc.edu/~michaelc/block/dm/v4/02-dm-scsi-helpers.patch
> 
> Again, doesn't this imply a change from what we already agreed upon?
> That is fine, I just need to know.
> 
>> And in fact a lot of your patch has already been done with this patch
>>
>> http://www.cs.wisc.edu/~michaelc/block/dm/v4/03-emc.patch a year ago.
> 
> I certainly read your patch set at that time it was sent since this is
> how I learned about the availability of the blk_execute_rq_nowait
> interface.  Yet, I certainly didn't mean to plagiarize any of your
> code.  I apologize if I have inadvertently done so.  I definitely

I did not mean to accuse you of plagiarizing my work. I was asking you
to plagiarize my work :) I mean I was asking you to take advantage of
the GPL :)

I meant to say in the previous mail, I did some of the work already and
the reasons why I did some of it were for possibly a good reason, so it
makes sense for you to look at my code, ask questions about why some
things were done so you do not hit some of the problems I already did
and so you can improve upon my solution, and if possible build on top of
my code so you do not have to duplicate the work I already did.

Re: Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Mike Christie]

Mike Christie wrote:
> Edward Goggin wrote:
>> On Friday, September 08, 2006 2:00 PM, Mike Christie wrote
>>> Mike Christie wrote:
>>>> Edward Goggin wrote:
>>>>> +
>>>>> +	rq->buffer = rq->data = h->buffer;
>>>>> +	rq->data_len = len;
>>>>> +	rq->bio = rq->biotail = NULL;
>>>>>  
>>>> I think I only suggested that you use the block layer map 
>>> functions in
>>>> the previous review. Now I will say, use them and fix the 
>>> core code to
>>>> not allocate memory or use a mempool since it will also fix the path
>>>> testers at the same time :) The reason is that the scsi 
>>> layer is trying
>>>> to make every thing run with scatterlists. In 2.6.18, every place is
>>>> converted (they should be at least), so you should not be 
>>> adding another
>>>> place where we send down a data buffer like this.
>>>>
>>> And if we are going to continue to do some scsi processing in dm I
>>> already did some helpers you could base yourself off of here
>>>
>>> http://www.cs.wisc.edu/~michaelc/block/dm/v4/
>> I was first trying to submit these changes to the upstream code
>> (I chose 2.6.18-rc4) since I this is what I thought you had
>> suggested that I do when we talked about it briefly at the dm bof
>> session at OLS.
>>
>> Should I not be taking this tack?
>>
> 
> I have no idea what Alasdair wants exactly and I have no idea when we
> are going to do anything.
> 
> I thought you were going to all the trouble of redoing your patches and
> fixing them up for review comments because you and alasdair switched and
> were staying with the hw handlers completely in dm-multpath route, or I
> thought the reason you were going to all this trouble to update your
> code and go through the review process was because for RHEL 5, we were
> staying with the hw handlers completely in dm-multipath route.
> 

Oh yeah, the comment about making the bios allocated from a per queue
pool like the requests and changing the allocation flags that are used
in those paths should be handled no matter where we put things and what
distro we are talking about. That should be a separate patch that goes
through the block layer maintainer though, since it fixes issues in the
block sg and scsi layer we well as dm.

RE: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Edward Goggin]

On Friday, September 08, 2006 1:21 PM, Mike Christie wrote
> 
> Edward Goggin wrote:
> > +
> > +	rq->buffer = rq->data = h->buffer;
> > +	rq->data_len = len;
> > +	rq->bio = rq->biotail = NULL;
> >  
> 
> I think I only suggested that you use the block layer map functions in
> the previous review. Now I will say, use them and fix the core code to
> not allocate memory or use a mempool since it will also fix the path
> testers at the same time :) The reason is that the scsi layer 
> is trying
> to make every thing run with scatterlists. In 2.6.18, every place is
> converted (they should be at least), so you should not be 
> adding another
> place where we send down a data buffer like this.
> 

Mike, sorry for the long delay.

I certainly can (and have tried already before submitting the patch
as is) change the code to call blk_rq_map_kern instead of using the
code sequence you have identified above.  As you cite, I was trying
to avoid memory/mempool allocation (for a bio) while servicing the io.

As far as fixing the code to not allocate memory or use a mempool,
let me know if the description below is what you have in mind?

A slightly modified version of __bio_map_kern
(and modified versions of its callers like blk_rq_map_kern)
which would accept a ptr to bio as a parameter instead of
allocating one.  Any kernel resident code like a dm-multipath
hardware handler could obtain a bio beforehand from a distinct
bio_set (bio mempool).  In the dm-multipath hardware handler
case, a single bio_set could be associated with each target
path (struct path).

Solving this for the path testing code path is more complicated
since the code path is more generic and involves ios initiated
from user space.  We could move the path testing to within a
device mapper ioctl interface which makes it easier to obtain
the bio from the path's bio_set and call a  slightly modified
version of bkl_rq_map_user, bio_map_user, and __bio_map_user_iov.

Re: 1st of 2 patches for dm_emc.c and dm-multipath hardware handler interface

[Mike Christie]

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

Edward Goggin wrote:
> On Friday, September 08, 2006 1:21 PM, Mike Christie wrote
>> Edward Goggin wrote:
>>> +
>>> +	rq->buffer = rq->data = h->buffer;
>>> +	rq->data_len = len;
>>> +	rq->bio = rq->biotail = NULL;
>>>  
>> I think I only suggested that you use the block layer map functions in
>> the previous review. Now I will say, use them and fix the core code to
>> not allocate memory or use a mempool since it will also fix the path
>> testers at the same time :) The reason is that the scsi layer 
>> is trying
>> to make every thing run with scatterlists. In 2.6.18, every place is
>> converted (they should be at least), so you should not be 
>> adding another
>> place where we send down a data buffer like this.
>>
> 
> Mike, sorry for the long delay.

No problem.

> 
> I certainly can (and have tried already before submitting the patch
> as is) change the code to call blk_rq_map_kern instead of using the
> code sequence you have identified above.  As you cite, I was trying
> to avoid memory/mempool allocation (for a bio) while servicing the io.
> 
> As far as fixing the code to not allocate memory or use a mempool,
> let me know if the description below is what you have in mind?
> 
> A slightly modified version of __bio_map_kern
> (and modified versions of its callers like blk_rq_map_kern)
> which would accept a ptr to bio as a parameter instead of
> allocating one.  Any kernel resident code like a dm-multipath
> hardware handler could obtain a bio beforehand from a distinct
> bio_set (bio mempool).  In the dm-multipath hardware handler
> case, a single bio_set could be associated with each target
> path (struct path).

So do we need bioset per path or even per module here?

Is the problem we are trying to solve:
1. we would allocate a bio from the same bioset as the layer above dm so
if that bioset is depleted we cannot perform REQ_BLOCK_PC commands (or
whatever they are called in 2.6.19.
2. we need some reserve bio so we can always allocate a bio.

If so could we just make a bio set for all blk pc commands and then for
GFP_NOIO do a wait similar to what ll_rw_blk.c does for requests. See
the attached patch for details. But this approach would solve the
problem for the path testers and hw handlers and sg.c and st.c at the
same time. The problem I see with it is that it is not fair and it is
possible that someone could wait forever, but I am not sure if that is
very realistic and I am not sure if that is a bad enough problem to
justify adding biosets for every driver.

But for all this you should ask/cc Jens Axboe. He is the block layer
maintainer.

[-- Attachment #2: out.patch --]
[-- Type: text/x-patch, Size: 1827 bytes --]

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
diff --git a/fs/bio.c b/fs/bio.c
index 6a0b9ad..e120945 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -75,6 +75,7 @@ struct bio_set {
  * IO code that does not need private memory pools.
  */
 static struct bio_set *fs_bio_set;
+static struct bio_set *blk_pc_bio_set; /* TODO rename for what is is now called */
 
 static inline struct bio_vec *bvec_alloc_bs(gfp_t gfp_mask, int nr, unsigned long *idx, struct bio_set *bs)
 {
@@ -119,6 +120,14 @@ void bio_free(struct bio *bio, struct bi
 /*
  * default destructor for a bio allocated with bio_alloc_bioset()
  */
+static void bio_pc_destructor(struct bio *bio)
+{
+	bio_free(bio, blk_pc_bio_set);
+}
+
+/*
+ * default destructor for a bio allocated with bio_alloc_bioset()
+ */
 static void bio_fs_destructor(struct bio *bio)
 {
 	bio_free(bio, fs_bio_set);
@@ -832,9 +841,13 @@ static struct bio *__bio_map_kern(reques
 	int offset, i;
 	struct bio *bio;
 
-	bio = bio_alloc(gfp_mask, nr_pages);
-	if (!bio)
+	/* todo: move so map_user can use */ 
+	bio = bio_alloc_bioset(gfp_mask, nr_pages, blk_pc_bio_set);
+	if (!bio) {
+		/* if GFP_WAIT then retry and wait like in get_request_wait */
 		return ERR_PTR(-ENOMEM);
+	}
+	bio->bi_destructor = bio_pc_destructor;
 
 	offset = offset_in_page(kaddr);
 	for (i = 0; i < nr_pages; i++) {
@@ -1243,7 +1256,11 @@ static int __init init_bio(void)
 
 	fs_bio_set = bioset_create(BIO_POOL_SIZE, bvec_pool_entries, scale);
 	if (!fs_bio_set)
-		panic("bio: can't allocate bios\n");
+		panic("bio: can't allocate fs bios\n");
+
+	blk_pc_bio_set = bioset_create(BIO_POOL_SIZE, bvec_pool_entries, scale)
+	if (!blk_pc_bio_set)
+		panic("bio: can't allocate blk pc bios\n");
 
 	bio_split_pool = mempool_create_kmalloc_pool(BIO_SPLIT_ENTRIES,
 						     sizeof(struct bio_pair));

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