[PATCH] updated: suspending I/Os to a device

[PATCH] updated: suspending I/Os to a device

[James.Smart]

Here is an updated mid-layer patch for adding a Suspend state to scsi devices. Changes were minor.

As mentioned, the goal is to insulate against temporary device disappearances that may occur due to cable pulls, switch reboots, etc. The design wants to suspend all i/o to a device, similar to the notion of scsi_block_requests - but at a device level, not a host level. As not all devices will support it - it's up to the LLDD to set a recovery template as part of slave_alloc. The template is used later when the LLDD requests to move into/outof the suspend state via the new routines scsi_device_suspend() and scsi_device_continue(). To ensure this is not an infinite condition, when a device is suspended, a timeout is started. The length of the timeout is set to the recovery value initialized by the LLDD, as it's likely driver and transport related. If the timer expires, the device is placed in the cancelled state.

Thanks...

-- James S



Signed-off-by: James Smart <james.smart@emulex.com>


diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi.c changed_kernel_2_6_7/drivers/scsi/scsi.c
--- clean_kernel_2_6_7/drivers/scsi/scsi.c	2004-07-19 13:04:44.898201000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi.c	2004-08-04 11:57:16.665960000 -0400
@@ -512,6 +512,19 @@
 		/* return 0 (because the command has been processed) */
 		goto out;
 	}
+
+	/* Check to see if the scsi lld put this device into suspend. */
+	if (unlikely(cmd->device->sdev_state == SDEV_SUSPEND)) {
+		/* in SDEV_SUSPEND, the command is just put back on the device
+		 * queue.  The suspend state has already blocked the queue so
+		 * future requests should not occur until the device 
+		 * transitions out of the suspend state.
+		 */
+		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		SCSI_LOG_MLQUEUE(3, printk("queuecommand : request rejected\n"));
+		goto out;
+	}
+
 	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
 	if (++serial_number == 0)
@@ -1088,8 +1101,8 @@
 
 /**
  * scsi_device_cancel - cancel outstanding IO to this device
- * @sdev:	pointer to struct scsi_device
- * @data:	pointer to cancel value.
+ * @sdev:	Pointer to struct scsi_device
+ * @recovery:	Boolean instructing function to recover device or not.
  *
  **/
 int scsi_device_cancel(struct scsi_device *sdev, int recovery)
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_error.c changed_kernel_2_6_7/drivers/scsi/scsi_error.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_error.c	2004-07-23 15:46:15.061800000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_error.c	2004-08-10 14:58:29.246640000 -0400
@@ -24,6 +24,7 @@
 #include <linux/blkdev.h>
 #include <linux/smp_lock.h>
 #include <scsi/scsi_ioctl.h>
+#include <scsi/scsi_transport.h>
 
 #include "scsi.h"
 #include "hosts.h"
@@ -155,6 +156,79 @@
 }
 
 /**
+ * scsi_add_sdev_timer - Start a timer for a scsi device that
+ *			 has been suspened by the scsi lld.
+ * @scmd:	scsi device to start timing.
+ * @timeout:	amount of time this device has to recover.
+ * @complete:	timeout function to call if timer isn't cancelled.
+ *
+ * Notes:
+ *	The scsi device is given a limited amount of time to
+ *	call scsi_device_continue and cancel this timer.  If the
+ *	scsi lld does not call in time, the device transitions 
+ *	to the cancel state.
+ *
+ * Returns:
+ *	None.
+ **/
+void scsi_add_sdev_timer(struct scsi_device *sdev, int timeout,
+			 void (*complete)(struct scsi_device *))
+{
+	/*
+	 * If the clock was already running for this command, then
+	 * first delete the timer.  The timer handling code gets rather
+	 * confused if we don't do this.
+	 */
+	if (sdev->device_recovert) {
+		if (sdev->device_recovert->eh_nodev_timeout.function)
+			del_timer(&sdev->device_recovert->eh_nodev_timeout);
+
+		sdev->device_recovert->eh_nodev_timeout.data = (unsigned long) sdev;
+		sdev->device_recovert->eh_nodev_timeout.expires = jiffies + timeout;
+		sdev->device_recovert->eh_nodev_timeout.function = (void (*)(unsigned long)) complete;
+
+		SCSI_LOG_ERROR_RECOVERY(5, printk("%s: sdev: %p, time:"
+			" %d, (%p)\n", __FUNCTION__, sdev, timeout, complete));
+
+		add_timer(&sdev->device_recovert->eh_nodev_timeout);
+	}
+}
+
+/**
+ * scsi_delete_sdev_timer - Cancel timer for a given scsi device that was
+ *			    previously suspended.
+ *
+ * @sdev:	Scsi device on which to cancel the timer.
+ *
+ * Notes:
+ *	The scsi device is guaranteed to have recovered if this call
+ *	executes.  However, the scsi device could have recovered too
+ *	late and this routine needs to detect this scenerio.
+ *
+ * Return value:
+ *	0 if the timer function has already started to run.
+ *	1 if the timer was successfully detached.
+ *
+ **/
+int scsi_delete_sdev_timer(struct scsi_device *sdev)
+{
+	int rtn = -EACCES;
+
+	if (sdev->device_recovert) {
+		rtn = del_timer(&sdev->device_recovert->eh_nodev_timeout);
+
+		SCSI_LOG_ERROR_RECOVERY(5, printk("%s: sdev: %p,"
+			" rtn: %d\n", __FUNCTION__, sdev, rtn));
+
+		sdev->device_recovert->eh_nodev_timeout.data = (unsigned long)NULL;
+		sdev->device_recovert->eh_nodev_timeout.function = NULL;
+	}
+
+	return rtn;
+}
+
+
+/**
  * scsi_times_out - Timeout function for normal scsi commands.
  * @scmd:	Cmd that is timing out.
  *
@@ -174,6 +248,24 @@
 }
 
 /**
+ * scsi_timeout_suspended_sdev - Timeout handler for suspended scsi devices
+ *				 that fail to recover in the alloted time.
+ * @sdev:	scsi device that failed to reappear in the alloted time.
+ **/
+void scsi_timeout_suspended_sdev(struct scsi_device *sdev)
+{
+	int rtn;
+
+	/* Cancel all IO to this device and do not attempt recovery. */
+	rtn = scsi_device_cancel(sdev, 0);
+	if (rtn) {
+		SCSI_LOG_ERROR_RECOVERY(5, printk("%s: rtn: %d\n", __FUNCTION__,
+					  rtn));
+	}
+}
+
+
+/**
  * scsi_block_when_processing_errors - Prevent cmds from being queued.
  * @sdev:	Device on which we are performing recovery.
  *
@@ -1196,6 +1288,7 @@
 	return;
 }
 
+
 /**
  * scsi_sleep_done - timer function for scsi_sleep
  * @sem:	semphore to signal
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_lib.c changed_kernel_2_6_7/drivers/scsi/scsi_lib.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_lib.c	2004-07-19 13:04:44.873200000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_lib.c	2004-08-06 15:29:15.911920000 -0400
@@ -18,8 +18,8 @@
 
 #include <scsi/scsi_driver.h>
 #include <scsi/scsi_host.h>
+#include <scsi/scsi_transport.h>
 #include "scsi.h"
-
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
@@ -59,6 +59,7 @@
 #undef SP
 
 
+
 /*
  * Function:    scsi_insert_special_req()
  *
@@ -1581,6 +1582,12 @@
 		case SDEV_CREATED:
 		case SDEV_OFFLINE:
 		case SDEV_QUIESCE:
+		case SDEV_SUSPEND:
+			printk(KERN_WARNING "scsi_device_set_state: transitioning"
+				"to SDEV_RUNNING from current state %d"
+				"on host %d channel %d id %d lun %d\n", oldstate,
+				sdev->host->host_no, sdev->channel, sdev->id, 
+				sdev->lun);
 			break;
 		default:
 			goto illegal;
@@ -1608,11 +1615,32 @@
 		}
 		break;
 
+	case SDEV_SUSPEND:
+		switch (oldstate) {
+		case SDEV_CREATED:
+		case SDEV_RUNNING:
+			printk(KERN_WARNING "scsi_device_set_state: transitioning"
+				"to SDEV_SUSPEND from current state %d"
+				"on host %d channel %d id %d lun %d\n", oldstate,
+				sdev->host->host_no, sdev->channel, sdev->id, 
+				sdev->lun);
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
 	case SDEV_CANCEL:
 		switch (oldstate) {
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_OFFLINE:
+		case SDEV_SUSPEND:
+			printk(KERN_WARNING "scsi_device_set_state: transitioning to"
+				"SDEV_CANCEL from current state %d on host %d "
+				"channel %d id %d lun %d\n", oldstate, 
+				sdev->host->host_no, sdev->channel, sdev->id, 
+				sdev->lun);
 			break;
 		default:
 			goto illegal;
@@ -1627,7 +1655,6 @@
 			goto illegal;
 		}
 		break;
-
 	}
 	sdev->sdev_state = state;
 	return 0;
@@ -1691,3 +1718,134 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ * Function:	scsi_device_suspend()
+ *
+ * Purpose:	Suspend request made by scsi lld's to temporarily stop all
+ *		scsi commands on the specified device.  Called from interrupt
+ *		or normal process context.
+ *
+ * Arguments:	sdev:	scsi device to suspend
+ *
+ * Returns:	 0	if successful 
+ *		-1	if error
+ *		  
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_SUSPEND state
+ *	(which must be a legal transition).  When the device is in this
+ *	state, all commands are deferred until the scsi lld reenables
+ *	the device with scsi_device_continue or device_suspend_tmo fires.
+ *	This routine assumes no locks are held upon entry.
+ **/
+int
+scsi_device_suspend(struct scsi_device *sdev)
+{
+	request_queue_t *q = sdev->request_queue;
+	unsigned long flags;
+	int  nodev_timeout = 0;
+
+	/* Before setting the new device state, make sure the host managing this
+	 * device has a recovery attribute for the suspend timeout.
+	 */
+	if (sdev->device_recovert) {
+		if (sdev->device_recovert->nodev_timeout < 1) {
+			printk(KERN_WARNING "scsi_device_suspend: nodev_timeout"
+				"value invalid on host %d channel %d id %d" 
+				"lun %d nodev_timeout=%d\n",
+				sdev->host->host_no, sdev->channel, sdev->id, 
+				sdev->lun, sdev->device_recovert->nodev_timeout);
+		return -1;
+		}
+
+		nodev_timeout = sdev->device_recovert->nodev_timeout;
+	} else {
+		printk(KERN_WARNING "scsi_device_suspend: host %d channel %d"
+			"id %d lun %d has no recovery data available.\n",
+			sdev->host->host_no, sdev->channel, sdev->id, 
+			sdev->lun);
+		return -1;
+	}
+
+	if (scsi_device_set_state(sdev, SDEV_SUSPEND) != 0) {
+		printk(KERN_WARNING "scsi_device_suspend: state set to suspend failed"
+			"on host %d channel %d id %d lun %d state=%d\n",
+			sdev->host->host_no, sdev->channel, sdev->id, sdev->lun,
+			sdev->sdev_state);
+		return -1;
+	}
+
+	/* 
+	 * The device has transitioned to SDEV_SUSPEND.  Stop the
+	 * block layer from calling the midlayer with this device's
+	 * request queue.  
+	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_stop_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/*
+	 * Start timing the suspend.  The scsi lld is only allowed to suspend for
+	 * the nodev_timeout value in seconds. 
+	 */
+	scsi_add_sdev_timer(sdev, nodev_timeout, scsi_timeout_suspended_sdev);
+	return 0;
+}
+EXPORT_SYMBOL(scsi_device_suspend);
+
+/**
+ * Function:	scsi_device_continue()
+ *
+ * Purpose:	Called by scsi lld's or the midlayer to restart the device queue
+ *		for the previously suspended scsi device.  Called from
+ *		interrupt or normal process context.
+ *
+ * Arguments:	sdev:	scsi device to restart
+ *
+ * Returns:	 0	if successful 
+ *		-1	if error
+ *		  
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_RUNNING state
+ *	(which must be a legal transition) allowing the midlayer to
+ *	goose the queue for this device.  This routine assumes no locks
+ *	are held upon entry.
+ **/
+int
+scsi_device_continue(struct scsi_device *sdev)
+{
+	request_queue_t *q = sdev->request_queue; 
+	int err;
+	unsigned long flags;
+	
+	/* Stop the scsi device timer first. Take no action on the del_timer
+	 * failure as the state machine state change will validate the
+	 * transaction.
+	 */
+	err = scsi_delete_sdev_timer(sdev);
+	if (!err) {
+		printk(KERN_WARNING "scsi_device_continue: no device timeout on" 
+			"host %d channel %d id %d lun %d state %d\n",
+			sdev->host->host_no, sdev->channel, sdev->id, sdev->lun,
+			sdev->sdev_state);
+	} 
+
+	/* Try to transition the scsi device to SDEV_RUNNING
+	 * and goose the device queue if successful.  
+	 */
+	err = scsi_device_set_state(sdev, SDEV_RUNNING);
+	if (err) {
+		printk(KERN_WARNING "scsi_device_continue: state set to running"
+			"failed on host %d channel %d id %d lun %d state=%d\n",
+			sdev->host->host_no, sdev->channel, sdev->id, sdev->lun,
+			sdev->sdev_state);
+		return err;
+	}
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_start_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL(scsi_device_continue);
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_priv.h changed_kernel_2_6_7/drivers/scsi/scsi_priv.h
--- clean_kernel_2_6_7/drivers/scsi/scsi_priv.h	2004-07-19 13:04:45.468202000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_priv.h	2004-07-30 17:32:15.779440000 -0400
@@ -100,6 +100,7 @@
 
 /* scsi_error.c */
 extern void scsi_times_out(struct scsi_cmnd *cmd);
+extern void scsi_timeout_suspended_sdev(struct scsi_device *sdev);
 extern int scsi_error_handler(void *host);
 extern int scsi_decide_disposition(struct scsi_cmnd *cmd);
 extern void scsi_eh_wakeup(struct Scsi_Host *shost);
@@ -116,6 +117,7 @@
 extern void scsi_free_queue(struct request_queue *q);
 extern int scsi_init_queue(void);
 extern void scsi_exit_queue(void);
+extern int scsi_get_transport_info(void);
 
 /* scsi_proc.c */
 #ifdef CONFIG_SCSI_PROC_FS
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_syms.c changed_kernel_2_6_7/drivers/scsi/scsi_syms.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_syms.c	2004-07-19 13:04:44.842200000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_syms.c	2004-07-27 12:56:37.106600000 -0400
@@ -86,6 +86,8 @@
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);
+EXPORT_SYMBOL(scsi_device_suspend);
+EXPORT_SYMBOL(scsi_device_continue);
 
 EXPORT_SYMBOL(__scsi_mode_sense);
 EXPORT_SYMBOL(scsi_mode_sense);
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_sysfs.c changed_kernel_2_6_7/drivers/scsi/scsi_sysfs.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_sysfs.c	2004-07-19 13:04:43.644200000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_sysfs.c	2004-08-05 09:41:49.706920000 -0400
@@ -29,6 +29,7 @@
 	{ SDEV_DEL, "deleted" },
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
+	{ SDEV_SUSPEND,	"suspend" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c changed_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c	2004-07-19 13:04:47.270200000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c	2004-07-29 12:39:05.466520000 -0400
@@ -28,7 +28,7 @@
 
 static void transport_class_release(struct class_device *class_dev);
 
-#define FC_NUM_ATTRS 	3	/* increase this if you add attributes */
+#define FC_NUM_ATTRS 	4	/* increase this if you add attributes */
 #define FC_OTHER_ATTRS 	0	/* increase this if you add "always on"
 				 * attributes */
 
@@ -61,11 +61,14 @@
 
 static int fc_setup_transport_attrs(struct scsi_device *sdev)
 {
-	/* I'm not sure what values are invalid.  We should pick some invalid
-	 * values for the defaults */
+	/*
+	 * I'm not sure what values are invalid.  We should pick some invalid
+	 * values for the defaults.
+	 */
 	fc_node_name(sdev) = -1;
 	fc_port_name(sdev) = -1;
 	fc_port_id(sdev) = -1;
+	fc_nodevice_tmo(sdev) = -1;
 
 	return 0;
 }
@@ -125,6 +128,7 @@
 fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr(port_id, "0x%06x\n");
+fc_transport_rw_attr(nodevice_tmo, "%d\n");
 
 #define SETUP_ATTRIBUTE_RD(field)				\
 	i->private_attrs[count] = class_device_attr_##field;	\
@@ -165,6 +169,7 @@
 	SETUP_ATTRIBUTE_RD(port_id);
 	SETUP_ATTRIBUTE_RD(port_name);
 	SETUP_ATTRIBUTE_RD(node_name);
+	SETUP_ATTRIBUTE_RW(nodevice_tmo);
 
 	BUG_ON(count > FC_NUM_ATTRS);
 
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_device.h changed_kernel_2_6_7/include/scsi/scsi_device.h
--- clean_kernel_2_6_7/include/scsi/scsi_device.h	2004-07-19 13:05:49.284200000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_device.h	2004-08-04 13:36:20.854920000 -0400
@@ -30,6 +30,9 @@
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
+	SDEV_SUSPEND,		/* Device suspended by scsi lld.  No block 
+				 * commands from user and midlayer should be issued
+				 * to the scsi lld. */
 };
 
 struct scsi_device {
@@ -99,17 +102,16 @@
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
-	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
-	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
+	unsigned use_10_for_rw:1;	/* first try 10-byte read / write */
+	unsigned use_10_for_ms:1;	/* first try 10-byte mode sense/select */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
 	unsigned skip_ms_page_3f:1;	/* do not use MODE SENSE page 0x3f */
-	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
+	unsigned use_192_bytes_for_3f:1;/* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
-	unsigned allow_restart:1; /* issue START_UNIT in error handler */
+	unsigned allow_restart:1;	/* issue START_UNIT in error handler */
 
 	unsigned int device_blocked;	/* Device returned QUEUE_FULL. */
-
-	unsigned int max_device_blocked; /* what device_blocked counts down from  */
+	unsigned int max_device_blocked;   /* what device_blocked counts down from */
 #define SCSI_DEFAULT_DEVICE_BLOCKED	3
 
 	int timeout;
@@ -118,10 +120,11 @@
 	struct class_device	sdev_classdev;
 
 	struct class_device	transport_classdev;
-
+	struct device_recover_template	*device_recovert; /* Device recovery template. */
 	enum scsi_device_state sdev_state;
 	unsigned long		transport_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
+
 #define	to_scsi_device(d)	\
 	container_of(d, struct scsi_device, sdev_gendev)
 #define	class_to_sdev(d)	\
@@ -133,7 +136,8 @@
 		uint, uint, uint);
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_device_cancel(struct scsi_device *, int);
-
+int scsi_device_suspend(struct scsi_device *);
+int scsi_device_continue(struct scsi_device *);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_eh.h changed_kernel_2_6_7/include/scsi/scsi_eh.h
--- clean_kernel_2_6_7/include/scsi/scsi_eh.h	2004-07-19 13:05:49.340200000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_eh.h	2004-07-27 12:54:27.578603000 -0400
@@ -4,6 +4,11 @@
 extern void scsi_add_timer(struct scsi_cmnd *, int,
 			   void (*)(struct scsi_cmnd *));
 extern int scsi_delete_timer(struct scsi_cmnd *);
+
+extern void scsi_add_sdev_timer(struct scsi_device *, int,
+				void (*complete)(struct scsi_device *));
+extern int scsi_delete_sdev_timer(struct scsi_device *sdev);
+
 extern void scsi_report_bus_reset(struct Scsi_Host *, int);
 extern void scsi_report_device_reset(struct Scsi_Host *, int, int);
 extern int scsi_block_when_processing_errors(struct scsi_device *);
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_transport_fc.h changed_kernel_2_6_7/include/scsi/scsi_transport_fc.h
--- clean_kernel_2_6_7/include/scsi/scsi_transport_fc.h	2004-07-19 13:05:49.377200000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_transport_fc.h	2004-07-29 12:39:58.053520000 -0400
@@ -26,6 +26,7 @@
 
 struct fc_transport_attrs {
 	int port_id;
+	int nodevice_tmo;
 	uint64_t node_name;
 	uint64_t port_name;
 };
@@ -34,12 +35,16 @@
 #define fc_port_id(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_id)
 #define fc_node_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->node_name)
 #define fc_port_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_name)
+#define fc_nodevice_tmo(x) (((struct fc_transport_attrs *)&(x)->transport_data)->nodevice_tmo)
 
 /* The functions by which the transport class and the driver communicate */
 struct fc_function_template {
 	void 	(*get_port_id)(struct scsi_device *);
 	void	(*get_node_name)(struct scsi_device *);
 	void	(*get_port_name)(struct scsi_device *);
+	void    (*get_nodevice_tmo)(struct scsi_device *);
+	void	(*set_nodevice_tmo)(struct scsi_device *, uint32_t);
+
 	/* The driver sets these to tell the transport class it
 	 * wants the attributes displayed in sysfs.  If the show_ flag
 	 * is not set, the attribute will be private to the transport
@@ -47,6 +52,7 @@
 	unsigned long	show_port_id:1;
 	unsigned long	show_node_name:1;
 	unsigned long	show_port_name:1;
+	unsigned long   show_nodevice_tmo:1;
 	/* Private Attributes */
 };
 
@@ -54,3 +60,4 @@
 void fc_release_transport(struct scsi_transport_template *);
 
 #endif /* SCSI_TRANSPORT_FC_H */
+
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_transport.h changed_kernel_2_6_7/include/scsi/scsi_transport.h
--- clean_kernel_2_6_7/include/scsi/scsi_transport.h	2004-07-19 13:05:49.363200000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_transport.h	2004-08-05 10:20:04.110923000 -0400
@@ -38,4 +38,22 @@
 	int	size;
 };
 
+/* device_recover_template:  
+ *	Scsi llds that want to take advantage of the suspend/continue device
+ *	feature allocate an instance of this data type during slave alloc.
+ *	The scsi lld sets the nodev_timeout value during slave_configure and
+ *	frees this resource in slave_destroy.
+ *
+ *	This feature is optional to scsi llds.  The midlayer detects the
+ *	presence or absence of this feature by a scsi lld by checking the
+ *	device_recovert pointer in the scsi_device instance. 
+ */
+struct device_recover_template {
+	/* Device recovery template. */
+	unsigned int nodev_timeout;		/* Device suspend timeout value
+						 * in seconds. */
+	struct timer_list eh_nodev_timeout;	/* Used to timeout the suspend 
+						 * state. */
+};
+
 #endif /* SCSI_TRANSPORT_H */
diff -uNr clean_kernel_2_6_7/Makefile changed_kernel_2_6_7/Makefile
--- clean_kernel_2_6_7/Makefile	2004-07-19 13:03:26.385200000 -0400
+++ changed_kernel_2_6_7/Makefile	2004-07-19 15:17:14.137200000 -0400
@@ -1,8 +1,8 @@
 VERSION = 2
 PATCHLEVEL = 6
 SUBLEVEL = 7
-EXTRAVERSION =
-NAME=Zonked Quokka
+EXTRAVERSION = -pae1.358.4.3smp
+NAME=pae
 
 # *DOCUMENTATION*
 # To see a list of typical targets execute "make help"

Re: [PATCH] updated: suspending I/Os to a device

[Christoph Hellwig]

On Tue, Aug 10, 2004 at 04:35:05PM -0400, James.Smart@Emulex.Com wrote:
> + **/
> +void scsi_add_sdev_timer(struct scsi_device *sdev, int timeout,
> +			 void (*complete)(struct scsi_device *))

the name sounds a little too generic.  scsi_device_add_recovery_timer
is more descriptive but also rather longish.

> +	/*
> +	 * If the clock was already running for this command, then
> +	 * first delete the timer.  The timer handling code gets rather
> +	 * confused if we don't do this.
> +	 */
> +	if (sdev->device_recovert) {
> +		if (sdev->device_recovert->eh_nodev_timeout.function)
> +			del_timer(&sdev->device_recovert->eh_nodev_timeout);

This needs to be a del_timer_sync.  Also the .function check is wrong, just
call del_timer_sync unconditionally.

> +
> +		sdev->device_recovert->eh_nodev_timeout.data = (unsigned long) sdev;
> +		sdev->device_recovert->eh_nodev_timeout.expires = jiffies + timeout;
> +		sdev->device_recovert->eh_nodev_timeout.function = (void (*)(unsigned long)) complete;

These lines are longer than 80 characters.  Also device_recovert is a strange
naming choice.  What about just recover or recovery?

> +int scsi_delete_sdev_timer(struct scsi_device *sdev)

Smae naming issue.

> +			printk(KERN_WARNING "scsi_device_suspend: nodev_timeout"
> +				"value invalid on host %d channel %d id %d" 
> +				"lun %d nodev_timeout=%d\n",
> +				sdev->host->host_no, sdev->channel, sdev->id, 
> +				sdev->lun, sdev->device_recovert->nodev_timeout);
> +		return -1;

wrong indentation for the return.

> +struct device_recover_template {
> +	/* Device recovery template. */
> +	unsigned int nodev_timeout;		/* Device suspend timeout value
> +						 * in seconds. */
> +	struct timer_list eh_nodev_timeout;	/* Used to timeout the suspend 
> +						 * state. */
> +};

I'm not so sue the separate allocation is worth it or whether we should just
sick it into the fc transport data.

RE: [PATCH] updated: suspending I/Os to a device

[James.Smart]

All the comments on names, etc are fine - I'll send an updated patch.

> > +struct device_recover_template {
> > +	/* Device recovery template. */
> > +	unsigned int nodev_timeout;		/* Device 
> suspend timeout value
> > +						 * in seconds. */
> > +	struct timer_list eh_nodev_timeout;	/* Used to 
> timeout the suspend 
> > +						 * state. */
> > +};
> 
> I'm not so sue the separate allocation is worth it or whether 
> we should just
> sick it into the fc transport data.

Well - it's not necessarily fc-specific, as I would assume USB, iSCSI, etc also would like to do the same thing. But, as far as just putting it in the device structure - I had the same thoughts. One thing that influenced this was JamesB's comments on not wanting all devices to have the structure, just those being on LLDD's that need to deal with this.  Note - "recovery" as a name was just randomly picked based on this 1 element. Are there perhaps other things that may fit this same definition (cross transport, but optional) ?  If so - is it better to rename this template ?

Also - does it really make sense to have the midlayer own the timer and calling cancel when it expires ? Or is it better to have the LLDD own it, calling continue when it expires, and letting the subsequent i/o failures take the device offline ?  The midlayer owning the timer seems more protectionist, but is it better ?

-- James S

Re: [PATCH] updated: suspending I/Os to a device

[Christoph Hellwig]

On Tue, Aug 10, 2004 at 05:00:40PM -0400, James.Smart@Emulex.Com wrote:
> Well - it's not necessarily fc-specific, as I would assume USB, iSCSI, etc also would like to do the same thing. But, as far as just putting it in the device structure - I had the same thoughts. One thing that influenced this was JamesB's comments on not wanting all devices to have the structure, just those being on LLDD's that need to deal with this.  Note - "recovery" as a name was just randomly picked based on this 1 element. Are there perhaps other things that may fit this same definition (cross transport, but optional) ?  If so - is it better to rename this template ?

I don't like the complexitly this adds and would prefer it in scsi_device
over this.  Seems like James and me disagree on this.

> Also - does it really make sense to have the midlayer own the timer and calling cancel when it expires ? Or is it better to have the LLDD own it, calling continue when it expires, and letting the subsequent i/o failures take the device offline ?  The midlayer owning the timer seems more protectionist, but is it better ?

Sounds like we could start by having this in the driver and when we see that
lots of drivers use it we can still move it to the midlayer.

Re: [PATCH] updated: suspending I/Os to a device

[James Bottomley]

On Tue, 2004-08-10 at 16:35, James.Smart@Emulex.Com wrote:
> Here is an updated mid-layer patch for adding a Suspend state to scsi devices. Changes were minor.
> 
> As mentioned, the goal is to insulate against temporary device disappearances that may occur due to cable pulls, switch reboots, etc. The design wants to suspend all i/o to a device, similar to the notion of scsi_block_requests - but at a device level, not a host level. As not all devices will support it - it's up to the LLDD to set a recovery template as part of slave_alloc. The template is used later when the LLDD requests to move into/outof the suspend state via the new routines scsi_device_suspend() and scsi_device_continue(). To ensure this is not an infinite condition, when a device is suspended, a timeout is started. The length of the timeout is set to the recovery value initialized by the LLDD, as it's likely driver and transport related. If the timer expires, the device is placed in the cancelled state.

OK, I looked it over.

The first thing that struck me is that you didn't use consistent comment
styles (mixture of old scsi and new docbook), and where you used the old
was actually in the middle of a section that had been converted to the
new docbook ones...

There's also this:

 extern int scsi_device_cancel(struct scsi_device *, int);
-
+int scsi_device_suspend(struct scsi_device *);
+int scsi_device_continue(struct scsi_device *);
 extern int scsi_device_get(struct scsi_device *);

If the rest of the .h file uses extern in front of its function
prototypes, then the added functions should too.

Anyway I found it rather complex for what it was trying to do.  How does
the attached look instead.  It makes the exported functions scsi private
(i.e. inaccessible to anything not including scsi_priv.h) and hopefully
makes FC LLDs indirect via the correct fc_ functions.

It also does away with all the extra timeout template stuff.

I've compiled this, but not tested it on anything.

James

===== drivers/scsi/scsi.c 1.145 vs edited =====
--- 1.145/drivers/scsi/scsi.c	2004-06-19 09:38:34 -05:00
+++ edited/drivers/scsi/scsi.c	2004-08-16 22:17:44 -05:00
@@ -518,6 +518,21 @@
 		/* return 0 (because the command has been processed) */
 		goto out;
 	}
+
+	/* Check to see if the scsi lld put this device into suspend. */
+	if (unlikely(cmd->device->sdev_state == SDEV_SUSPEND)) {
+		/* in SDEV_SUSPEND, the command is just put back on the device
+		 * queue.  The suspend state has already blocked the queue so
+		 * future requests should not occur until the device 
+		 * transitions out of the suspend state.
+		 */
+		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		SCSI_LOG_MLQUEUE(3, printk("queuecommand : request to suspended device requeued\n"));
+		/* NOTE: rtn is still zero here because we don't need the
+		 * queue to be plugged on return (it's already stopped) */
+		goto out;
+	}
+
 	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
 	if (++serial_number == 0)
@@ -1100,8 +1115,8 @@
 
 /**
  * scsi_device_cancel - cancel outstanding IO to this device
- * @sdev:	pointer to struct scsi_device
- * @data:	pointer to cancel value.
+ * @sdev:	Pointer to struct scsi_device
+ * @recovery:	Boolean instructing function to recover device or not.
  *
  **/
 int scsi_device_cancel(struct scsi_device *sdev, int recovery)
===== drivers/scsi/scsi_lib.c 1.129 vs edited =====
--- 1.129/drivers/scsi/scsi_lib.c	2004-06-19 09:40:25 -05:00
+++ edited/drivers/scsi/scsi_lib.c	2004-08-16 23:30:54 -05:00
@@ -1584,6 +1584,7 @@
 		case SDEV_CREATED:
 		case SDEV_OFFLINE:
 		case SDEV_QUIESCE:
+		case SDEV_SUSPEND:
 			break;
 		default:
 			goto illegal;
@@ -1611,11 +1612,22 @@
 		}
 		break;
 
+	case SDEV_SUSPEND:
+		switch (oldstate) {
+		case SDEV_CREATED:
+		case SDEV_RUNNING:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
 	case SDEV_CANCEL:
 		switch (oldstate) {
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_OFFLINE:
+		case SDEV_SUSPEND:
 			break;
 		default:
 			goto illegal;
@@ -1630,7 +1642,6 @@
 			goto illegal;
 		}
 		break;
-
 	}
 	sdev->sdev_state = state;
 	return 0;
@@ -1694,3 +1705,121 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ * scsi_timeout_suspended_sdev - Timeout handler for suspended scsi devices
+ *				 that fail to recover in the alloted time.
+ * @data:	scsi device that failed to reappear in the alloted time.
+ **/
+static void scsi_timeout_suspended_sdev(unsigned long data)
+{
+	struct scsi_device *sdev = (struct scsi_device *)data;
+
+	dev_printk(KERN_ERR, &sdev->sdev_gendev, "suspend timed out: device resuming\n");
+
+	/* set the device going again ... if we haven't been resumed
+	 * this will probably result in I/O errors if the host still
+	 * isn't ready */
+	scsi_internal_device_continue(sdev, NULL);
+}
+
+
+/**
+ * scsi_internal_device_suspend - internal function to put a device
+ *				  temporarily into the SDEV_SUSPEND state
+ * @sdev:	device to suspend
+ * @timer:	pointer to a timer to use for the resume timeout
+ * @timeout:	timeout in HZ
+ *
+ * Suspend request made by scsi lld's to temporarily stop all
+ * scsi commands on the specified device.  Called from interrupt
+ * or normal process context.
+ *
+ * Returns zero if successful or error if not
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_SUSPEND state
+ *	(which must be a legal transition).  When the device is in this
+ *	state, all commands are deferred until the scsi lld reenables
+ *	the device with scsi_device_continue or device_suspend_tmo fires.
+ *	This routine assumes no locks are held upon entry.
+ **/
+int
+scsi_internal_device_suspend(struct scsi_device *sdev,
+			     struct timer_list *timer, int timeout)
+{
+	request_queue_t *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (timeout < 0 || timeout > SCSI_DEVICE_SUSPEND_MAX_TIMEOUT)
+		return -EINVAL;
+
+	if (scsi_device_set_state(sdev, SDEV_SUSPEND) != 0)
+		return -EINVAL;
+
+	/* 
+	 * The device has transitioned to SDEV_SUSPEND.  Stop the
+	 * block layer from calling the midlayer with this device's
+	 * request queue.  
+	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_stop_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/*
+	 * Start timing the suspend.  The scsi lld is only allowed to
+	 * suspend for the timeout.
+	 */
+	init_timer(timer);
+	timer->data = (unsigned long)sdev;
+	timer->expires = jiffies + timeout;
+	timer->function = scsi_timeout_suspended_sdev;
+	add_timer(timer);
+	return 0;
+}
+EXPORT_SYMBOL(scsi_internal_device_suspend);
+
+/**
+ * scsi_device_continue - resume a device after a suspend
+ * @sdev:	device to resume
+ * @timer:	pointer to suspend timer (may be NULL)
+ *
+ * Called by scsi lld's or the midlayer to restart the device queue
+ * for the previously suspended scsi device.  Called from interrupt or
+ * normal process context.
+ *
+ * Returns zero if successful or error if not.
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_RUNNING state
+ *	(which must be a legal transition) allowing the midlayer to
+ *	goose the queue for this device.  This routine assumes no locks
+ *	are held upon entry.
+ **/
+int
+scsi_internal_device_continue(struct scsi_device *sdev,
+			      struct timer_list *timer)
+{
+	request_queue_t *q = sdev->request_queue; 
+	int err;
+	unsigned long flags;
+	
+	/* Stop the scsi device timer first. Take no action on the del_timer
+	 * failure as the state machine state change will validate the
+	 * transaction.
+	 */
+	if (timer)
+		del_timer_sync(timer);
+
+	/* Try to transition the scsi device to SDEV_RUNNING
+	 * and goose the device queue if successful.  
+	 */
+	if ((err = scsi_device_set_state(sdev, SDEV_RUNNING)))
+		return err;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_start_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_internal_device_continue);
===== drivers/scsi/scsi_priv.h 1.33 vs edited =====
--- 1.33/drivers/scsi/scsi_priv.h	2004-06-16 10:45:44 -05:00
+++ edited/drivers/scsi/scsi_priv.h	2004-08-16 23:03:09 -05:00
@@ -161,4 +161,13 @@
 extern struct class sdev_class;
 extern struct bus_type scsi_bus_type;
 
+/* internal scsi timeout functions: for use by mid-layer and transport
+ * classes */
+
+#define SCSI_DEVICE_SUSPEND_MAX_TIMEOUT	(HZ*10)
+extern int scsi_internal_device_suspend(struct scsi_device *sdev,
+					struct timer_list *timer, int timeout);
+extern int scsi_internal_device_continue(struct scsi_device *sdev,
+					 struct timer_list *timer);
+
 #endif /* _SCSI_PRIV_H */
===== drivers/scsi/scsi_sysfs.c 1.52 vs edited =====
--- 1.52/drivers/scsi/scsi_sysfs.c	2004-07-28 22:59:10 -05:00
+++ edited/drivers/scsi/scsi_sysfs.c	2004-08-16 22:05:04 -05:00
@@ -30,6 +30,7 @@
 	{ SDEV_DEL, "deleted" },
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
+	{ SDEV_SUSPEND,	"suspend" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
===== drivers/scsi/scsi_transport_fc.c 1.3 vs edited =====
--- 1.3/drivers/scsi/scsi_transport_fc.c	2004-03-29 03:19:12 -06:00
+++ edited/drivers/scsi/scsi_transport_fc.c	2004-08-16 23:32:50 -05:00
@@ -24,11 +24,13 @@
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_transport_fc.h>
 
+#include "scsi_priv.h"
+
 #define FC_PRINTK(x, l, f, a...)	printk(l "scsi(%d:%d:%d:%d): " f, (x)->host->host_no, (x)->channel, (x)->id, (x)->lun , ##a)
 
 static void transport_class_release(struct class_device *class_dev);
 
-#define FC_NUM_ATTRS 	3	/* increase this if you add attributes */
+#define FC_NUM_ATTRS 	4	/* increase this if you add attributes */
 #define FC_OTHER_ATTRS 	0	/* increase this if you add "always on"
 				 * attributes */
 
@@ -61,11 +63,14 @@
 
 static int fc_setup_transport_attrs(struct scsi_device *sdev)
 {
-	/* I'm not sure what values are invalid.  We should pick some invalid
-	 * values for the defaults */
+	/*
+	 * I'm not sure what values are invalid.  We should pick some invalid
+	 * values for the defaults.
+	 */
 	fc_node_name(sdev) = -1;
 	fc_port_name(sdev) = -1;
 	fc_port_id(sdev) = -1;
+	fc_nodevice_tmo(sdev) = -1;
 
 	return 0;
 }
@@ -125,6 +130,7 @@
 fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr(port_id, "0x%06x\n");
+fc_transport_rw_attr(nodevice_tmo, "%d\n");
 
 #define SETUP_ATTRIBUTE_RD(field)				\
 	i->private_attrs[count] = class_device_attr_##field;	\
@@ -165,6 +171,7 @@
 	SETUP_ATTRIBUTE_RD(port_id);
 	SETUP_ATTRIBUTE_RD(port_name);
 	SETUP_ATTRIBUTE_RD(node_name);
+	SETUP_ATTRIBUTE_RW(nodevice_tmo);
 
 	BUG_ON(count > FC_NUM_ATTRS);
 
@@ -183,6 +190,19 @@
 	kfree(i);
 }
 EXPORT_SYMBOL(fc_release_transport);
+
+int fc_device_suspend(struct scsi_device *sdev)
+{
+	return scsi_internal_device_suspend(sdev, &fc_suspend_timer(sdev),
+					    fc_nodevice_tmo(sdev));
+}
+EXPORT_SYMBOL(fc_device_suspend);
+
+int fc_device_continue(struct scsi_device *sdev)
+{
+	return scsi_internal_device_continue(sdev, &fc_suspend_timer(sdev));
+}
+EXPORT_SYMBOL(fc_device_continue);
 

 MODULE_AUTHOR("Martin Hicks");
===== include/scsi/scsi_device.h 1.19 vs edited =====
--- 1.19/include/scsi/scsi_device.h	2004-07-07 11:24:13 -05:00
+++ edited/include/scsi/scsi_device.h	2004-08-16 23:01:28 -05:00
@@ -30,6 +30,9 @@
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
+	SDEV_SUSPEND,		/* Device suspended by scsi lld.  No block 
+				 * commands from user and midlayer should be issued
+				 * to the scsi lld. */
 };
 
 struct scsi_device {
===== include/scsi/scsi_transport_fc.h 1.2 vs edited =====
--- 1.2/include/scsi/scsi_transport_fc.h	2004-03-22 04:33:42 -06:00
+++ edited/include/scsi/scsi_transport_fc.h	2004-08-16 23:32:33 -05:00
@@ -26,20 +26,28 @@
 
 struct fc_transport_attrs {
 	int port_id;
+	int nodevice_tmo;
 	uint64_t node_name;
 	uint64_t port_name;
+	struct timer_list suspend_timer;
 };
 
 /* accessor functions */
 #define fc_port_id(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_id)
 #define fc_node_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->node_name)
 #define fc_port_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_name)
+#define fc_nodevice_tmo(x) (((struct fc_transport_attrs *)&(x)->transport_data)->nodevice_tmo)
+#define fc_suspend_timer(x) (((struct fc_transport_attrs *)&(x)->transport_data)->suspend_timer)
+
 
 /* The functions by which the transport class and the driver communicate */
 struct fc_function_template {
 	void 	(*get_port_id)(struct scsi_device *);
 	void	(*get_node_name)(struct scsi_device *);
 	void	(*get_port_name)(struct scsi_device *);
+	void    (*get_nodevice_tmo)(struct scsi_device *);
+	void	(*set_nodevice_tmo)(struct scsi_device *, uint32_t);
+
 	/* The driver sets these to tell the transport class it
 	 * wants the attributes displayed in sysfs.  If the show_ flag
 	 * is not set, the attribute will be private to the transport
@@ -47,6 +55,7 @@
 	unsigned long	show_port_id:1;
 	unsigned long	show_node_name:1;
 	unsigned long	show_port_name:1;
+	unsigned long   show_nodevice_tmo:1;
 	/* Private Attributes */
 };
 

RE: [PATCH] updated: suspending I/Os to a device

[James.Smart]

James,

Thanks for looking through this.

After considering Christoph's comments, looking at the patch, followed
by your review, we came to the conclusion that the LLDD should manage the timer:

  - Saves a lot of timers. Rather than a timer per lun, the LLDD, which
    will already have timer(s) running can best decide if there's 1 for
    the link, 1 per target, or 1 per device. This is a potential 
    1000+ vs 1 difference.

  - Cleaner, smaller implementation - that more closely mirrors
    scsi_block_requests().

  - Removes the duplicate sysfs attributes for the timer. There is likely
    always going to be 1 or more in the LLDD or transport. This removes
    the per-scsi device attribute that would have to be there too.

  - Your patch assumed the LLDD had already moved the device to a failed
    state by the time the timer had failed. May not have been the case.

The one con:
  Depends on well-behaved LLDD's to not leave the device blocked indefinitely.
  But this is no different from scsi_block_requests().


Additionally, this patch has the following differences:
  - Renamed the functions to xxx_block/unblock. The terms suspend/resume
    seem to be used in too many other places/ways. Also, it parallels
    what we're modeling - block_requests() functionality but on a device
    level.

  - Adds a parameter to unblock indicating whether the device has failed
    or not. If failed, transition is immediately to CANCEL. This should
    allow faster failure, without all the calls to the LLDD, and should
    lessen the number of error messages to the console. Our testing
    beat on the console pretty well, and our config was still rather small.

  - Removed the indirection through scsi_priv.h. This modification is not
    FC centric - so the hiding behind the transport functions seems odd.
    Leaving it would facilitate the same indirection through the other
    transports (USB, iSCSI, etc). I did leave the fc transport parameter though.    
    and all the transport additions will have to be
      

-- James S



Signed-off-by: James Smart <james.smart@emulex.com>


diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi.c changed_kernel_2_6_7/drivers/scsi/scsi.c
--- clean_kernel_2_6_7/drivers/scsi/scsi.c	2004-07-19 13:04:44.000000000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi.c	2004-08-18 15:26:09.000000000 -0400
@@ -512,6 +512,18 @@
 		/* return 0 (because the command has been processed) */
 		goto out;
 	}
+
+	/* Check to see if the scsi lld put this device into suspend. */
+	if (unlikely(cmd->device->sdev_state == SDEV_BLOCKED)) {
+		/* in SDEV_BLOCKED, the command is just put back on the device
+		 * queue.  The block state has already blocked the queue so
+		 * future requests should not occur until the device 
+		 * transitions out of the suspend state.
+		 */
+		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		goto out;
+	}
+
 	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
 	if (++serial_number == 0)
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_lib.c changed_kernel_2_6_7/drivers/scsi/scsi_lib.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_lib.c	2004-07-19 13:04:44.000000000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_lib.c	2004-08-20 03:12:20.000000000 -0400
@@ -1581,6 +1581,7 @@
 		case SDEV_CREATED:
 		case SDEV_OFFLINE:
 		case SDEV_QUIESCE:
+		case SDEV_BLOCKED:;
 			break;
 		default:
 			goto illegal;
@@ -1608,11 +1609,22 @@
 		}
 		break;
 
+	case SDEV_BLOCKED:
+		switch (oldstate) {
+		case SDEV_CREATED:
+		case SDEV_RUNNING:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
 	case SDEV_CANCEL:
 		switch (oldstate) {
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_OFFLINE:
+		case SDEV_BLOCKED:
 			break;
 		default:
 			goto illegal;
@@ -1691,3 +1703,91 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+
+/**
+ * scsi_device_block - internal function to put a device
+ *		temporarily into the SDEV_BLOCK state
+ * @sdev:	device to block
+ *
+ * Block request made by scsi lld's to temporarily stop all
+ * scsi commands on the specified device.  Called from interrupt
+ * or normal process context.
+ *
+ * Returns zero if successful or error if not
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_BLOCK state
+ *	(which must be a legal transition).  When the device is in this
+ *	state, all commands are deferred until the scsi lld reenables
+ *	the device with scsi_device_continue or device_suspend_tmo fires.
+ *	This routine assumes no locks are held upon entry.
+ **/
+int
+scsi_device_block(struct scsi_device *sdev)
+{
+	request_queue_t *q = sdev->request_queue;
+	int err;
+	unsigned long flags;
+
+	err = scsi_device_set_state(sdev, SDEV_BLOCKED);
+	if (err)
+		return -EINVAL;
+
+	/* 
+	 * The device has transitioned to SDEV_BLOCK.  Stop the
+	 * block layer from calling the midlayer with this device's
+	 * request queue.  
+	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_stop_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_device_block);
+
+/**
+ * scsi_device_unblock - resume a device after a block request.
+ * @sdev:	device to resume
+ * @device_failure:  Zero if device functioning or 1 if not.
+ *
+ * Called by scsi lld's or the midlayer to restart the device queue
+ * for the previously blocked scsi device.  Called from interrupt or
+ * normal process context.
+ *
+ * Returns zero if successful or error if not.
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_RUNNING state
+ *	(which must be a legal transition) allowing the midlayer to
+ *	goose the queue for this device.  This routine assumes no locks
+ *	are held upon entry.
+ **/
+int
+scsi_device_unblock(struct scsi_device *sdev, uint device_failure)
+{
+	request_queue_t *q = sdev->request_queue; 
+	int err;
+	unsigned long flags;
+
+	if (!device_failure) {
+		err = scsi_device_set_state (sdev, SDEV_RUNNING);
+	} else {
+		err = scsi_device_cancel(sdev, 0);
+	}
+	if (err)
+		return -EINVAL;
+
+	/* 
+	 * The device has transitioned either to SDEV_CANCEL or
+	 * SDEV_RUNNING.  Goose the device queue to drain all
+	 * outstanding commands.
+	 */
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_start_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+	
+	return 0;
+}
+EXPORT_SYMBOL(scsi_device_unblock);
+
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_syms.c changed_kernel_2_6_7/drivers/scsi/scsi_syms.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_syms.c	2004-07-19 13:04:44.000000000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_syms.c	2004-08-18 15:15:58.000000000 -0400
@@ -86,7 +86,8 @@
 EXPORT_SYMBOL(scsi_add_device);
 EXPORT_SYMBOL(scsi_remove_device);
 EXPORT_SYMBOL(scsi_device_cancel);
-
+EXPORT_SYMBOL(scsi_device_block);
+EXPORT_SYMBOL(scsi_device_unblock);
 EXPORT_SYMBOL(__scsi_mode_sense);
 EXPORT_SYMBOL(scsi_mode_sense);
 
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_sysfs.c changed_kernel_2_6_7/drivers/scsi/scsi_sysfs.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_sysfs.c	2004-07-19 13:04:43.000000000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_sysfs.c	2004-08-16 12:57:40.000000000 -0400
@@ -29,6 +29,7 @@
 	{ SDEV_DEL, "deleted" },
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
+	{ SDEV_BLOCKED,	"blocked" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
diff -uNr clean_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c changed_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c
--- clean_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c	2004-07-19 13:04:47.000000000 -0400
+++ changed_kernel_2_6_7/drivers/scsi/scsi_transport_fc.c	2004-08-18 15:16:55.000000000 -0400
@@ -28,7 +28,7 @@
 
 static void transport_class_release(struct class_device *class_dev);
 
-#define FC_NUM_ATTRS 	3	/* increase this if you add attributes */
+#define FC_NUM_ATTRS 	4	/* increase this if you add attributes */
 #define FC_OTHER_ATTRS 	0	/* increase this if you add "always on"
 				 * attributes */
 
@@ -66,6 +66,7 @@
 	fc_node_name(sdev) = -1;
 	fc_port_name(sdev) = -1;
 	fc_port_id(sdev) = -1;
+	fc_nodevice_tmo(sdev) = -1;
 
 	return 0;
 }
@@ -125,6 +126,7 @@
 fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr(port_id, "0x%06x\n");
+fc_transport_rw_attr(nodevice_tmo, "%d\n");
 
 #define SETUP_ATTRIBUTE_RD(field)				\
 	i->private_attrs[count] = class_device_attr_##field;	\
@@ -165,6 +167,7 @@
 	SETUP_ATTRIBUTE_RD(port_id);
 	SETUP_ATTRIBUTE_RD(port_name);
 	SETUP_ATTRIBUTE_RD(node_name);
+	SETUP_ATTRIBUTE_RW(nodevice_tmo);
 
 	BUG_ON(count > FC_NUM_ATTRS);
 
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_device.h changed_kernel_2_6_7/include/scsi/scsi_device.h
--- clean_kernel_2_6_7/include/scsi/scsi_device.h	2004-07-19 13:05:49.000000000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_device.h	2004-08-18 15:06:06.000000000 -0400
@@ -30,6 +30,9 @@
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
+	SDEV_BLOCKED,		/* Device blocked by scsi lld.  No scsi 
+				 * commands from user and midlayer should be issued
+				 * to the scsi lld. */
 };
 
 struct scsi_device {
@@ -133,7 +136,8 @@
 		uint, uint, uint);
 extern void scsi_remove_device(struct scsi_device *);
 extern int scsi_device_cancel(struct scsi_device *, int);
-
+extern int scsi_device_block(struct scsi_device *);
+extern int scsi_device_unblock(struct scsi_device *, uint);
 extern int scsi_device_get(struct scsi_device *);
 extern void scsi_device_put(struct scsi_device *);
 extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
diff -uNr clean_kernel_2_6_7/include/scsi/scsi_transport_fc.h changed_kernel_2_6_7/include/scsi/scsi_transport_fc.h
--- clean_kernel_2_6_7/include/scsi/scsi_transport_fc.h	2004-07-19 13:05:49.000000000 -0400
+++ changed_kernel_2_6_7/include/scsi/scsi_transport_fc.h	2004-08-18 14:01:09.000000000 -0400
@@ -26,6 +26,7 @@
 
 struct fc_transport_attrs {
 	int port_id;
+	int nodevice_tmo;
 	uint64_t node_name;
 	uint64_t port_name;
 };
@@ -34,12 +35,16 @@
 #define fc_port_id(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_id)
 #define fc_node_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->node_name)
 #define fc_port_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_name)
+#define fc_nodevice_tmo(x) (((struct fc_transport_attrs *)&(x)->transport_data)->nodevice_tmo)
 
 /* The functions by which the transport class and the driver communicate */
 struct fc_function_template {
 	void 	(*get_port_id)(struct scsi_device *);
 	void	(*get_node_name)(struct scsi_device *);
 	void	(*get_port_name)(struct scsi_device *);
+	void    (*get_nodevice_tmo)(struct scsi_device *);
+	void	(*set_nodevice_tmo)(struct scsi_device *, uint32_t);
+
 	/* The driver sets these to tell the transport class it
 	 * wants the attributes displayed in sysfs.  If the show_ flag
 	 * is not set, the attribute will be private to the transport
@@ -47,6 +52,7 @@
 	unsigned long	show_port_id:1;
 	unsigned long	show_node_name:1;
 	unsigned long	show_port_name:1;
+	unsigned long   show_nodevice_tmo:1;
 	/* Private Attributes */
 };
 
@@ -54,3 +60,4 @@
 void fc_release_transport(struct scsi_transport_template *);
 
 #endif /* SCSI_TRANSPORT_FC_H */
+

RE: [PATCH] updated: suspending I/Os to a device

[James Bottomley]

On Thu, 2004-08-19 at 13:24, James.Smart@Emulex.Com wrote:
> The one con:
>   Depends on well-behaved LLDD's to not leave the device blocked indefinitely.
>   But this is no different from scsi_block_requests().

But that's the big one.

A significant number of driver bugs have come to us from incorrect timer
handling (not understanding the difference between del_timer and
del_timer_sync being the biggest).

The whole reason for moving the code up into the mid-layer is to avoid
the LLD handling the timer.

After the rewrite to simplify the code, the added overhead of having the
mid-layer (and transport classes) do the timer is scarcely sevently
lines longer than the one you currently attach.

James

RE: [PATCH] updated: suspending I/Os to a device

[James.Smart]

All True...  and I'm not trying to belittle it.

The main things that sent me to the LLDD side were:

1) The number of timers that had to be running. A single cable pull can be handled by 1 timer in the LLDD, which will always be there. If there's a timer per device - and the config (still small) has 4-8 arrays, each array with 256 LUNs - this is a 1k/2k timers vs 1 scenario. A single device disappearance (a target) is a 256 (or more) vs 1 scenario. Not only the amount of resources for the timers, but also the complexity of when all these timers fire and the same time and start spitting i/o - which is not necessarily coordinated with the LLDD state change. Makes for a lot of fun error handling. I'd rather deal with an LLDD that misses an unblock (easy to spot) vs the race conditions that could occur due to the last point. Granted, this should all work, but its in a location that is typically much harder to test and reproduce.

2) Your update mandated that the LLDD provide the timer context, which also mandates something that is roughly a per-device structure for LLDD. To date, although very common, the interfaces had yet to dictate it. We have moved away from device structures (targets are all we really needed to manage).

The other items on cleanliness were gravy - and typically, the cleaner, the better.

I also took Christoph's comments to indicate this was a reasonable thing to do.

-- James



> -----Original Message-----
> From: James Bottomley [mailto:James.Bottomley@SteelEye.com]
> Sent: Thursday, August 19, 2004 1:43 PM
> To: Smart, James
> Cc: SCSI Mailing List
> Subject: RE: [PATCH] updated: suspending I/Os to a device
> 
> 
> On Thu, 2004-08-19 at 13:24, James.Smart@Emulex.Com wrote:
> > The one con:
> >   Depends on well-behaved LLDD's to not leave the device 
> blocked indefinitely.
> >   But this is no different from scsi_block_requests().
> 
> But that's the big one.
> 
> A significant number of driver bugs have come to us from 
> incorrect timer
> handling (not understanding the difference between del_timer and
> del_timer_sync being the biggest).
> 
> The whole reason for moving the code up into the mid-layer is to avoid
> the LLD handling the timer.
> 
> After the rewrite to simplify the code, the added overhead of 
> having the
> mid-layer (and transport classes) do the timer is scarcely sevently
> lines longer than the one you currently attach.
> 
> James
> 
> 
> 

Re: [PATCH] updated: suspending I/Os to a device

[Mike Anderson]

James.Smart@Emulex.Com [James.Smart@Emulex.Com] wrote:
> All True...  and I'm not trying to belittle it.
> 
> The main things that sent me to the LLDD side were:
> 
> 1) The number of timers that had to be running. A single cable pull can be handled by 1 timer in the LLDD, which will always be there. If there's a timer per device - and the config (still small) has 4-8 arrays, each array with 256 LUNs - this is a 1k/2k timers vs 1 scenario. A single device disappearance (a target) is a 256 (or more) vs 1 scenario. Not only the amount of resources for the timers, but also the complexity of when all these timers fire and the same time and start spitting i/o - which is not necessarily coordinated with the LLDD state change. Makes for a lot of fun error handling. I'd rather deal with an LLDD that misses an unblock (easy to spot) vs the race conditions that could occur due to the last point. Granted, this should all work, but its in a location that is typically much harder to test and reproduce.
> 

An option to consider is that what you want is target level suspend in
the mid layer. While the mid-layer does not have a target representation
equal to the lun representation (scsi_device) you get some target linkage
between the scsi_devs using same_target_siblings.

Just pass in the scsi_device as you where doing (maybe update the
function names to match the target suspend). Inside the function use a
list_for_each_entry over same_target_siblings to set the scsi_device
state to suspend. Then use a single timeout timer for each target.

If we want to have full symmetric interfaces to the LLDD for suspending
a target "siblings" or a lun "scsi_device" with protection for
calling already suspend objects we may need a little more locking around
the state changes or some other exclusion method.

It would be nice some day to have real target representation, but this
pseudo support we have now may address your needs.

I am little sleepy so maybe this idea is not as good as it seems
now as it would after a little sleep. YMMV

-andmike
--
Michael Anderson
andmike@us.ibm.com

RE: [PATCH] updated: suspending I/Os to a device

[James.Smart]

Reasonable - as the list_for_each_entry over same_target_siblings is exactly what the LLDD is doing. We were following previous guidance. I'll let James make the call.

-- james s

> -----Original Message-----
> From: Mike Anderson [mailto:andmike@us.ibm.com]
> Sent: Friday, August 20, 2004 4:55 AM
> To: Smart, James
> Cc: James.Bottomley@SteelEye.com; linux-scsi@vger.kernel.org
> Subject: Re: [PATCH] updated: suspending I/Os to a device
> 
> 
> James.Smart@Emulex.Com [James.Smart@Emulex.Com] wrote:
> > All True...  and I'm not trying to belittle it.
> > 
> > The main things that sent me to the LLDD side were:
> > 
> > 1) The number of timers that had to be running. A single 
> cable pull can be handled by 1 timer in the LLDD, which will 
> always be there. If there's a timer per device - and the 
> config (still small) has 4-8 arrays, each array with 256 LUNs 
> - this is a 1k/2k timers vs 1 scenario. A single device 
> disappearance (a target) is a 256 (or more) vs 1 scenario. 
> Not only the amount of resources for the timers, but also the 
> complexity of when all these timers fire and the same time 
> and start spitting i/o - which is not necessarily coordinated 
> with the LLDD state change. Makes for a lot of fun error 
> handling. I'd rather deal with an LLDD that misses an unblock 
> (easy to spot) vs the race conditions that could occur due to 
> the last point. Granted, this should all work, but its in a 
> location that is typically much harder to test and reproduce.
> > 
> 
> An option to consider is that what you want is target level suspend in
> the mid layer. While the mid-layer does not have a target 
> representation
> equal to the lun representation (scsi_device) you get some 
> target linkage
> between the scsi_devs using same_target_siblings.
> 
> Just pass in the scsi_device as you where doing (maybe update the
> function names to match the target suspend). Inside the function use a
> list_for_each_entry over same_target_siblings to set the scsi_device
> state to suspend. Then use a single timeout timer for each target.
> 
> If we want to have full symmetric interfaces to the LLDD for 
> suspending
> a target "siblings" or a lun "scsi_device" with protection for
> calling already suspend objects we may need a little more 
> locking around
> the state changes or some other exclusion method.
> 
> It would be nice some day to have real target representation, but this
> pseudo support we have now may address your needs.
> 
> I am little sleepy so maybe this idea is not as good as it seems
> now as it would after a little sleep. YMMV
> 
> -andmike
> --
> Michael Anderson
> andmike@us.ibm.com
> 
> 

Re: [PATCH] updated: suspending I/Os to a device

[Luben Tuikov]

>  > 1) The number of timers that had to be running. A single cable pull 
> can be handled by 1 timer in the LLDD, which will always be there. If 
> there's a timer per device - and the config (still small) has 4-8 
> arrays, each array with 256 LUNs - this is a 1k/2k timers vs 1 scenario. 
> A single device disappearance (a target) is a 256 (or more) vs 1 
> scenario. Not only the amount of resources for the timers, but also the 
> complexity of when all these timers fire and the same time and start 
> spitting i/o - which is not necessarily coordinated with the LLDD state 
> change. Makes for a lot of fun error handling. I'd rather deal with an 
> LLDD that misses an unblock (easy to spot) vs the race conditions that 
> could occur due to the last point. Granted, this should all work, but 
> its in a location that is typically much harder to test and reproduce.

This is true.  I can imagine this being added in a flexible manner
so that LLDD who can drive it, can use it, and the rest can rely on
SCSI Core.

Similar arguments have been discussed before (flexible command timeout
patch).

> It would be nice some day to have real target representation, but this
> pseudo support we have now may address your needs.

I'd been thinking about scsi_target since my days at Splentec.
(but really _just_ thinking)

Basically there'd be properties and methods that only belong to
SCSI Targets, and not to LUs.  So at some point we may want to
represent/expose those properties and methods to those who may
want to use them and/or are interested in them.

It would be a step closer to SAM, although, one doesn't technically
need it to have a working SCSI layer (as we do now).

		Luben

[PATCH] updated: suspending I/Os to a device

[James.Smart]

Here's the updated patch based on further discussion on the list:

This patch:

- Adds a new sdev state - SDEV_BLOCKED.
- Adds new internal sdev routines to block, unblock, and to unblock on timeout
- Adds a new fc transport attribute for the sdev - dev_loss_tmo
- Adds fc transport routines to block/unblock at the sdev, target, and host levels. Timer will be started at whatever level invoked. Timeout value gleaned from the attribute on the sdev, or 1st sdev on the target/host.

-- James






Signed-off-by: James Smart <james.smart@emulex.com>


diff -uNr clean_kernel_2.6.7/drivers/scsi/scsi.c patched_kernel_2.6.7/drivers/scsi/scsi.c
--- clean_kernel_2.6.7/drivers/scsi/scsi.c	2004-07-19 13:04:44.000000000 -0400
+++ patched_kernel_2.6.7/drivers/scsi/scsi.c	2004-09-02 10:23:22.000000000 -0400
@@ -512,6 +512,21 @@
 		/* return 0 (because the command has been processed) */
 		goto out;
 	}
+
+	/* Check to see if the scsi lld put this device into blocked state. */
+	if (unlikely(cmd->device->sdev_state == SDEV_BLOCK)) {
+		/* in SDEV_BLOCK, the command is just put back on the device
+		 * queue.  The suspend state has already blocked the queue so
+		 * future requests should not occur until the device 
+		 * transitions out of the suspend state.
+		 */
+		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
+		SCSI_LOG_MLQUEUE(3, printk("queuecommand : request to blocked device requeued\n"));
+		/* NOTE: rtn is still zero here because we don't need the
+		 * queue to be plugged on return (it's already stopped) */
+		goto out;
+	}
+
 	/* Assign a unique nonzero serial_number. */
 	/* XXX(hch): this is racy */
 	if (++serial_number == 0)
@@ -1088,8 +1103,8 @@
 
 /**
  * scsi_device_cancel - cancel outstanding IO to this device
- * @sdev:	pointer to struct scsi_device
- * @data:	pointer to cancel value.
+ * @sdev:	Pointer to struct scsi_device
+ * @recovery:	Boolean instructing function to recover device or not.
  *
  **/
 int scsi_device_cancel(struct scsi_device *sdev, int recovery)
diff -uNr clean_kernel_2.6.7/drivers/scsi/scsi_lib.c patched_kernel_2.6.7/drivers/scsi/scsi_lib.c
--- clean_kernel_2.6.7/drivers/scsi/scsi_lib.c	2004-07-19 13:04:44.000000000 -0400
+++ patched_kernel_2.6.7/drivers/scsi/scsi_lib.c	2004-09-01 13:43:39.000000000 -0400
@@ -1581,6 +1581,7 @@
 		case SDEV_CREATED:
 		case SDEV_OFFLINE:
 		case SDEV_QUIESCE:
+		case SDEV_BLOCK:
 			break;
 		default:
 			goto illegal;
@@ -1608,11 +1609,22 @@
 		}
 		break;
 
+	case SDEV_BLOCK:
+		switch (oldstate) {
+		case SDEV_CREATED:
+		case SDEV_RUNNING:
+			break;
+		default:
+			goto illegal;
+		}
+		break;
+
 	case SDEV_CANCEL:
 		switch (oldstate) {
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_OFFLINE:
+		case SDEV_BLOCK:
 			break;
 		default:
 			goto illegal;
@@ -1691,3 +1703,121 @@
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+/**
+ * scsi_timeout_blocked_sdev - Timeout handler for blocked scsi devices
+ *				 that fail to recover in the alloted time.
+ * @data:	scsi device that failed to reappear in the alloted time.
+ **/
+static void scsi_timeout_blocked_sdev(unsigned long data)
+{
+	struct scsi_device *sdev = (struct scsi_device *)data;
+
+	dev_printk(KERN_ERR, &sdev->sdev_gendev, 
+		"blocked device time out: device resuming\n");
+
+	/* set the device going again ... if we haven't been resumed
+	 * this will probably result in I/O errors if the host still
+	 * isn't ready */
+	scsi_internal_device_unblock(sdev, NULL);
+}
+
+/**
+ * scsi_internal_device_block - internal function to put a device
+ *				temporarily into the SDEV_BLOCK state
+ * @sdev:	device to block
+ * @timer:	pointer to a timer to use for the resume timeout (may be NULL).
+ * @timeout:	timeout in HZ
+ *
+ * Block request made by scsi lld's to temporarily stop all
+ * scsi commands on the specified device.  Called from interrupt
+ * or normal process context.
+ *
+ * Returns zero if successful or error if not
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_BLOCK state
+ *	(which must be a legal transition).  When the device is in this
+ *	state, all commands are deferred until the scsi lld reenables
+ *	the device with scsi_device_unblock or device_block_tmo fires.
+ *	This routine assumes no locks are held upon entry.
+ **/
+int
+scsi_internal_device_block(struct scsi_device *sdev,
+			     struct timer_list *timer, int timeout)
+{
+	request_queue_t *q = sdev->request_queue;
+	unsigned long flags;
+
+	if (timer) {
+		if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+			return -EINVAL;
+	}
+
+	if (scsi_device_set_state(sdev, SDEV_BLOCK) != 0)
+		return -EINVAL;
+
+	/* The device has transitioned to SDEV_BLOCK.  Stop the
+	 * block layer from calling the midlayer with this device's
+	 * request queue. */
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_stop_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	/* The scsi lld is only allowed to block the device for the specified
+	 * timeout. */
+	if (timer) {
+		init_timer(timer);
+		timer->data = (unsigned long)sdev;
+		timer->expires = jiffies + timeout;
+		timer->function = scsi_timeout_blocked_sdev;
+		add_timer(timer);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_internal_device_block);
+
+/**
+ * scsi_internal_device_unblock - resume a device after a block request
+ * @sdev:	device to resume
+ * @timer:	pointer to block timer (may be NULL)
+ *
+ * Called by scsi lld's or the midlayer to restart the device queue
+ * for the previously suspended scsi device.  Called from interrupt or
+ * normal process context.
+ *
+ * Returns zero if successful or error if not.
+ *
+ * Notes:       
+ *	This routine transitions the device to the SDEV_RUNNING state
+ *	(which must be a legal transition) allowing the midlayer to
+ *	goose the queue for this device.  This routine assumes no locks
+ *	are held upon entry.
+ **/
+int
+scsi_internal_device_unblock(struct scsi_device *sdev,
+			     struct timer_list *timer)
+{
+	request_queue_t *q = sdev->request_queue; 
+	int err;
+	unsigned long flags;
+	
+	/* Stop the scsi device timer first. Take no action on the del_timer
+	 * failure as the state machine state change will validate the
+	 * transaction. */
+	if (timer)
+		del_timer_sync(timer);
+
+	/* Try to transition the scsi device to SDEV_RUNNING
+	 * and goose the device queue if successful.  */
+	err = scsi_device_set_state(sdev, SDEV_RUNNING);
+	if (err)
+		return err;
+
+	spin_lock_irqsave(q->queue_lock, flags);
+	blk_start_queue(q);
+	spin_unlock_irqrestore(q->queue_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(scsi_internal_device_unblock);
diff -uNr clean_kernel_2.6.7/drivers/scsi/scsi_priv.h patched_kernel_2.6.7/drivers/scsi/scsi_priv.h
--- clean_kernel_2.6.7/drivers/scsi/scsi_priv.h	2004-07-19 13:04:45.000000000 -0400
+++ patched_kernel_2.6.7/drivers/scsi/scsi_priv.h	2004-08-31 11:10:37.000000000 -0400
@@ -160,4 +160,13 @@
 extern struct class sdev_class;
 extern struct bus_type scsi_bus_type;
 
+/* internal scsi timeout functions: for use by mid-layer and transport
+ * classes */
+
+#define SCSI_DEVICE_BLOCK_MAX_TIMEOUT	(HZ*60)
+extern int scsi_internal_device_block(struct scsi_device *sdev,
+					struct timer_list *timer, int timeout);
+extern int scsi_internal_device_unblock(struct scsi_device *sdev,
+					struct timer_list *timer);
+
 #endif /* _SCSI_PRIV_H */
diff -uNr clean_kernel_2.6.7/drivers/scsi/scsi_sysfs.c patched_kernel_2.6.7/drivers/scsi/scsi_sysfs.c
--- clean_kernel_2.6.7/drivers/scsi/scsi_sysfs.c	2004-07-19 13:04:43.000000000 -0400
+++ patched_kernel_2.6.7/drivers/scsi/scsi_sysfs.c	2004-09-02 10:31:31.000000000 -0400
@@ -29,6 +29,7 @@
 	{ SDEV_DEL, "deleted" },
 	{ SDEV_QUIESCE, "quiesce" },
 	{ SDEV_OFFLINE,	"offline" },
+	{ SDEV_BLOCK,	"blocked" },
 };
 
 const char *scsi_device_state_name(enum scsi_device_state state)
diff -uNr clean_kernel_2.6.7/drivers/scsi/scsi_transport_fc.c patched_kernel_2.6.7/drivers/scsi/scsi_transport_fc.c
--- clean_kernel_2.6.7/drivers/scsi/scsi_transport_fc.c	2004-07-19 13:04:47.000000000 -0400
+++ patched_kernel_2.6.7/drivers/scsi/scsi_transport_fc.c	2004-09-02 11:07:12.000000000 -0400
@@ -23,12 +23,13 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsi_transport.h>
 #include <scsi/scsi_transport_fc.h>
+#include "scsi_priv.h"
 
 #define FC_PRINTK(x, l, f, a...)	printk(l "scsi(%d:%d:%d:%d): " f, (x)->host->host_no, (x)->channel, (x)->id, (x)->lun , ##a)
 
 static void transport_class_release(struct class_device *class_dev);
 
-#define FC_NUM_ATTRS 	3	/* increase this if you add attributes */
+#define FC_NUM_ATTRS 	4	/* increase this if you add attributes */
 #define FC_OTHER_ATTRS 	0	/* increase this if you add "always on"
 				 * attributes */
 
@@ -66,6 +67,7 @@
 	fc_node_name(sdev) = -1;
 	fc_port_name(sdev) = -1;
 	fc_port_id(sdev) = -1;
+	fc_dev_loss_tmo(sdev) = -1;
 
 	return 0;
 }
@@ -125,6 +127,7 @@
 fc_transport_rd_attr_cast(node_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr_cast(port_name, "0x%llx\n", unsigned long long);
 fc_transport_rd_attr(port_id, "0x%06x\n");
+fc_transport_rw_attr(dev_loss_tmo, "%d\n");
 
 #define SETUP_ATTRIBUTE_RD(field)				\
 	i->private_attrs[count] = class_device_attr_##field;	\
@@ -165,6 +168,7 @@
 	SETUP_ATTRIBUTE_RD(port_id);
 	SETUP_ATTRIBUTE_RD(port_name);
 	SETUP_ATTRIBUTE_RD(node_name);
+	SETUP_ATTRIBUTE_RW(dev_loss_tmo);
 
 	BUG_ON(count > FC_NUM_ATTRS);
 
@@ -185,6 +189,292 @@
 EXPORT_SYMBOL(fc_release_transport);
 
 
+int fc_device_block(struct scsi_device *sdev, struct timer_list *timer)
+{
+	return scsi_internal_device_block(sdev, timer, (fc_dev_loss_tmo(sdev) * HZ));
+}
+EXPORT_SYMBOL(fc_device_block);
+
+int fc_device_unblock(struct scsi_device *sdev, struct timer_list *timer)
+{
+	return scsi_internal_device_unblock(sdev, timer);
+}
+EXPORT_SYMBOL(fc_device_unblock);
+
+/**
+ * fc_timeout_blocked_tgt - Timeout handler for blocked scsi targets
+ *			 that fail to recover in the alloted time.
+ * @data:	scsi device managed by the target that failed to reappear
+ *		in the alloted time.
+ **/
+static void fc_timeout_blocked_tgt(unsigned long data)
+{
+	struct scsi_device *tgt_sdev = (struct scsi_device *)data;
+	struct Scsi_Host *shost = tgt_sdev->host;
+	struct scsi_device *sdev;
+
+	dev_printk(KERN_ERR, &tgt_sdev->sdev_gendev, 
+		"blocked target time out: target resuming\n");
+
+	__shost_for_each_device(sdev, shost) {
+		if (sdev->id == tgt_sdev->id) {
+			/* set the device going again ... if the scsi lld didn't
+			 * unblock this device, then IO errors will probably
+			 * result if the host still isn't ready */
+			scsi_internal_device_unblock(sdev, NULL);
+		}
+	}
+}
+
+/**
+ * fc_timeout_blocked_host - Timeout handler for blocked scsi hosts
+ *			 that fail to recover in the alloted time.
+ * @data:	scsi host originating the block that failed to recover its
+ *		devices in the alloted time.
+ **/
+static void fc_timeout_blocked_host(unsigned long data)
+{
+	struct Scsi_Host *shost = (struct scsi_host *)data;
+	struct scsi_device *sdev;
+
+	dev_printk(KERN_ERR, &shost->shost_gendev, 
+		"blocked host time out: host resuming\n");
+
+	__shost_for_each_device(sdev, shost) {
+		/* set the device going again ... if the scsi lld didn't
+		 * unblock this device, then IO errors will probably
+		 * result if the host still isn't ready */
+		scsi_internal_device_unblock(sdev, NULL);
+	}
+}
+
+/**
+ * fc_target_block - Fibre Channel Transport function to put a target
+ *		temporarily into the SDEV_BLOCK state.
+ * @sdev:	device managed by target providing target id.
+ * @timer:	pointer to a timer to use for the target timeout.
+ *		the timer is initialized and managed by the midlayer.
+ *		This timer may not be NULL.
+ *
+ * Target block request made by scsi lld's to temporarily stop all
+ * scsi commands to all devices managed by this target id.  Called 
+ * from interrupt or normal process context.
+ *
+ * Returns zero if successful or error if not
+ *
+ * Notes:       
+ *	This routine requires the caller to pass a timer pointer
+ *	for the timeout mechanism.  The caller is responsible for
+ *	all memory management of the timer pointer.  The timeout
+ *	value is extracted from the fc transport attributes for
+ *	the first scsi device found with the correct target id.
+ *	Although this routine depends on the scsi_internal_device_block
+ *	routine to handle device state transitions, the timer
+ *	control is handled in this module.  This routine assumes no
+ *	locks are held on entry.
+ **/
+int fc_target_block(struct scsi_device *sdev, struct timer_list *timer)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct scsi_device *tmp_sdev = NULL;
+	int tgt_cnt = 0, err = 0, timeout = fc_dev_loss_tmo(sdev);
+
+	if (!timer)
+		return -EINVAL;
+
+	if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
+			return -EINVAL;
+
+	__shost_for_each_device(tmp_sdev, shost) {
+		if (tmp_sdev->id == sdev->id) {
+			err = scsi_internal_device_block(tmp_sdev, NULL, 0);
+			if (err)
+				return err;
+			else
+				tgt_cnt++;
+		}
+	}
+
+	if (!tgt_cnt) {
+		/* This condition should never be true, however, if so, just
+		 * fail the block request. */
+		dev_printk(KERN_ERR, &sdev->sdev_gendev, 
+			"Host %d has no devices matching target id %d\n",
+			shost->host_no, sdev->id);
+		return -EINVAL;
+	}
+
+
+	/* The scsi lld is only allowed to block this target for the timeout. */
+	init_timer(timer);
+	timer->data = (unsigned long)sdev;
+	timer->expires = jiffies + (timeout * HZ);
+	timer->function = fc_timeout_blocked_tgt;
+	add_timer(timer);
+
+	return 0;
+}
+EXPORT_SYMBOL(fc_target_block);
+
+/**
+ * fc_target_unblock - unblock a target following a fc_target_block request.
+ * @sdev:	device managed by target providing target id.
+ * @timer:	timer pointer previously passed to fc_target_block.  This
+ *		timer may not be NULL.
+ *
+ * Called by scsi lld's to restart all devices associated with a particular
+ * target id following a fc_target_block request.  The scsi lld makes this
+ * call to unblock each device queue and restart IO.  Called from interrupt
+ * or normal process context.
+ *
+ * Returns zero if successful or error if not.
+ *
+ * Notes:       
+ *	This routine requires the caller to pass the timer pointer used 
+ *	in the previous fc_target_block call back to this routine.  This 
+ *	routine assumes no locks are held on entry.
+ **/
+int fc_target_unblock(struct scsi_device *sdev, struct timer_list *timer)
+{
+	struct Scsi_Host *shost = sdev->host;
+	struct scsi_device *tmp_sdev;
+	int err = 0;
+
+	/* Stop the target timer first. Take no action on the del_timer
+	 * failure as the state machine state change will validate the
+	 * transaction. */
+	if (!timer)
+		return -EINVAL;
+
+	del_timer_sync(timer);
+
+	/* Block each device matching the caller's target id. */
+	__shost_for_each_device(tmp_sdev, shost) {
+		if (tmp_sdev->id == sdev->id) {
+			err = scsi_internal_device_unblock(tmp_sdev, NULL);
+			if (err)
+				return err;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(fc_target_unblock);
+
+/**
+ * fc_host_block - Fibre Channel Transport function to put all devices
+ *		managed by the calling host temporarily into the SDEV_BLOCK
+ *		state.
+ * @shost:	scsi host pointer that contains all scsi device siblings.
+ * @timer:	pointer to a timer to use for the host timeout.
+ *		the timer is initialized and managed by the midlayer.
+ *
+ * Host block request made by scsi lld's to temporarily stop all
+ * scsi commands to all devices managed by this host.  Called 
+ * from interrupt or normal process context.
+ *
+ * Returns zero if successful or error if not
+ *
+ * Notes:       
+ *	This routine requires the caller to pass a timer pointer
+ *	for the timeout mechanism.  The caller is responsible for
+ *	all memory management of the timer pointer.  The timeout
+ *	value is extracted from the fc transport attributes for
+ *	the first scsi device found on the specified scsi host.
+ *	Although this routine depends on the scsi_internal_device_block
+ *	routine to handle device state transitions, the timer
+ *	control is handled in this module.  This routine assumes no
+ *	locks are held on entry.
+ **/
+int fc_host_block(struct Scsi_Host *shost, struct timer_list *timer)
+{
+	struct scsi_device *sdev, *tmp_sdev = NULL;
+	int err = 0, timeout = -1;
+
+	if (!timer)
+		return -EINVAL;
+
+	/* Find the first device and validate the timeout value.  All devices
+	 * managed by this host are timed using the transport 
+	 * dev_loss_tmo timeout. */
+	__shost_for_each_device(sdev, shost) {
+		tmp_sdev = sdev;
+		timeout = fc_dev_loss_tmo(sdev);
+		if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) {
+			return -EINVAL;
+		} else {
+			break;
+		}
+	}
+	
+	if (!tmp_sdev) {
+		/* This condition should never be true, however, if so, just
+		 * fail the block request. */
+		dev_printk(KERN_ERR, &sdev->sdev_gendev, 
+			"Host %d has no attached devices!  Block failed.\n",
+			shost->host_no);
+		return -EINVAL;
+	}
+
+	__shost_for_each_device(sdev, shost) {
+		err = scsi_internal_device_block(sdev, NULL, 0);
+		if (err)
+			return err;
+	}
+
+	/* The scsi lld is only allowed to block this host for the timeout. */
+	init_timer(timer);
+	timer->data = (unsigned long)shost;
+	timer->expires = jiffies + timeout * HZ;
+	timer->function = fc_timeout_blocked_host;
+	add_timer(timer);
+
+	return 0;
+}
+EXPORT_SYMBOL(fc_host_block);
+
+/**
+ * fc_host_unblock - unblock all devices managed by this host following a 
+ *		fc_host_block request.
+ * @shost:	scsi host containing all scsi device siblings to unblock.
+ * @timer:	timer pointer previously passed to fc_host_block.  This
+ *		timer may not be NULL.
+ *
+ * Called by scsi lld's to restart all scsi devices managed by the specified
+ * scsi host follwoing an fc_host_block request.  The scsi lld makes this
+ * call to unblock each device queue and restart IO.  Called from interrupt
+ * or normal process context.
+ *
+ * Returns zero if successful or error if not.
+ *
+ * Notes:       
+ *	This routine requires the caller to pass the timer pointer used 
+ *	in the previous fc_host_block call back to this routine.  This 
+ *	routine assumes no locks are held on entry.
+ **/
+int fc_host_unblock(struct Scsi_Host *shost, struct timer_list *timer)
+{
+	struct scsi_device *sdev;
+	int err = 0;
+	
+	/* Stop the host timer first. Take no action on the del_timer
+	 * failure as the state machine state change will validate the
+	 * transaction. */
+	if (!timer)
+		return -EINVAL;
+
+	del_timer_sync(timer);
+
+	__shost_for_each_device(sdev, shost) {
+		err = scsi_internal_device_unblock(sdev, NULL);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(fc_host_unblock);
+
 MODULE_AUTHOR("Martin Hicks");
 MODULE_DESCRIPTION("FC Transport Attributes");
 MODULE_LICENSE("GPL");
diff -uNr clean_kernel_2.6.7/include/scsi/scsi_device.h patched_kernel_2.6.7/include/scsi/scsi_device.h
--- clean_kernel_2.6.7/include/scsi/scsi_device.h	2004-07-19 13:05:49.000000000 -0400
+++ patched_kernel_2.6.7/include/scsi/scsi_device.h	2004-09-01 11:37:12.000000000 -0400
@@ -30,6 +30,9 @@
 				 * originate in the mid-layer) */
 	SDEV_OFFLINE,		/* Device offlined (by error handling or
 				 * user request */
+	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
+				 * commands from user or midlayer should be issued
+				 * to the scsi lld. */
 };
 
 struct scsi_device {
diff -uNr clean_kernel_2.6.7/include/scsi/scsi_transport_fc.h patched_kernel_2.6.7/include/scsi/scsi_transport_fc.h
--- clean_kernel_2.6.7/include/scsi/scsi_transport_fc.h	2004-07-19 13:05:49.000000000 -0400
+++ patched_kernel_2.6.7/include/scsi/scsi_transport_fc.h	2004-09-01 11:23:58.000000000 -0400
@@ -26,6 +26,7 @@
 
 struct fc_transport_attrs {
 	int port_id;
+	int dev_loss_tmo;	/* Device loss timeout value in seconds. */
 	uint64_t node_name;
 	uint64_t port_name;
 };
@@ -34,12 +35,16 @@
 #define fc_port_id(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_id)
 #define fc_node_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->node_name)
 #define fc_port_name(x)	(((struct fc_transport_attrs *)&(x)->transport_data)->port_name)
+#define fc_dev_loss_tmo(x) (((struct fc_transport_attrs *)&(x)->transport_data)->dev_loss_tmo)
 
 /* The functions by which the transport class and the driver communicate */
 struct fc_function_template {
 	void 	(*get_port_id)(struct scsi_device *);
 	void	(*get_node_name)(struct scsi_device *);
 	void	(*get_port_name)(struct scsi_device *);
+	void    (*get_dev_loss_tmo)(struct scsi_device *);
+	void	(*set_dev_loss_tmo)(struct scsi_device *, uint32_t);
+
 	/* The driver sets these to tell the transport class it
 	 * wants the attributes displayed in sysfs.  If the show_ flag
 	 * is not set, the attribute will be private to the transport
@@ -47,10 +52,16 @@
 	unsigned long	show_port_id:1;
 	unsigned long	show_node_name:1;
 	unsigned long	show_port_name:1;
+	unsigned long   show_dev_loss_tmo:1;
 	/* Private Attributes */
 };
 
 struct scsi_transport_template *fc_attach_transport(struct fc_function_template *);
 void fc_release_transport(struct scsi_transport_template *);
+int fc_target_block(struct scsi_device *sdev, struct timer_list *timer);
+int fc_target_unblock(struct scsi_device *sdev, struct timer_list *timer);
+int fc_host_block(struct Scsi_Host *shost, struct timer_list *timer);
+int fc_host_unblock(struct Scsi_Host *shost, struct timer_list *timer);
 
 #endif /* SCSI_TRANSPORT_FC_H */
+

Re: [PATCH] updated: suspending I/Os to a device

[Christoph Hellwig]

> +	/* Check to see if the scsi lld put this device into blocked state. */
> +	if (unlikely(cmd->device->sdev_state == SDEV_BLOCK)) {
> +		/* in SDEV_BLOCK, the command is just put back on the device

Style nitpick: In Linux block coments start like this:

/*
 * foo, yodda, ..

 (same issue happens a few times in the patch)

> +		 * queue.  The suspend state has already blocked the queue so
> +		 * future requests should not occur until the device 
> +		 * transitions out of the suspend state.
> +		 */
> +		scsi_queue_insert(cmd, SCSI_MLQUEUE_DEVICE_BUSY);
> +		SCSI_LOG_MLQUEUE(3, printk("queuecommand : request to blocked device requeued\n"));

Please break lines after 80 characters or slightly before.

> +int
> +scsi_internal_device_block(struct scsi_device *sdev,
> +			     struct timer_list *timer, int timeout)
> +{
> +	request_queue_t *q = sdev->request_queue;
> +	unsigned long flags;
> +
> +	if (timer) {
> +		if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> +			return -EINVAL;
> +	}

...

> +
> +	/* The scsi lld is only allowed to block the device for the specified
> +	 * timeout. */
> +	if (timer) {
> +		init_timer(timer);
> +		timer->data = (unsigned long)sdev;

You should probably split this into two routines.   A core one that doesn't
take a timer as argument at all, and a wrapper that always does.

> +EXPORT_SYMBOL(scsi_internal_device_block);

the internal APIs should probably be exported _GPL to mark them as clearly
internal.

> +int
> +scsi_internal_device_unblock(struct scsi_device *sdev,
> +			     struct timer_list *timer)
> +{
> +	request_queue_t *q = sdev->request_queue; 
> +	int err;
> +	unsigned long flags;
> +	
> +	/* Stop the scsi device timer first. Take no action on the del_timer
> +	 * failure as the state machine state change will validate the
> +	 * transaction. */
> +	if (timer)
> +		del_timer_sync(timer);

Same core + timer wrapper issue I'd say.

In fact there's only a singel caller caring about the timer, it could be
moved to that one.

> +static void fc_timeout_blocked_tgt(unsigned long data)
> +{
> +	struct scsi_device *tgt_sdev = (struct scsi_device *)data;
> +	struct Scsi_Host *shost = tgt_sdev->host;
> +	struct scsi_device *sdev;
> +
> +	dev_printk(KERN_ERR, &tgt_sdev->sdev_gendev, 
> +		"blocked target time out: target resuming\n");
> +
> +	__shost_for_each_device(sdev, shost) {

You're missing some locking here.

> +	__shost_for_each_device(sdev, shost) {

Dito.

> +	if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
> +			return -EINVAL;

double indentation.

> +	__shost_for_each_device(tmp_sdev, shost) {

missing locking again.

> +		if (tmp_sdev->id == sdev->id) {
> +			err = scsi_internal_device_block(tmp_sdev, NULL, 0);
> +			if (err)
> +				return err;
> +			else
> +				tgt_cnt++;

superflous else as you're returning early

> +int fc_target_unblock(struct scsi_device *sdev, struct timer_list *timer)
> +{
> +	struct Scsi_Host *shost = sdev->host;
> +	struct scsi_device *tmp_sdev;
> +	int err = 0;
> +
> +	/* Stop the target timer first. Take no action on the del_timer
> +	 * failure as the state machine state change will validate the
> +	 * transaction. */
> +	if (!timer)
> +		return -EINVAL;

Not needed.  IF the caller is broken no need to work around.

> +	/* Block each device matching the caller's target id. */
> +	__shost_for_each_device(tmp_sdev, shost) {

missing locking again.

> +int fc_host_block(struct Scsi_Host *shost, struct timer_list *timer)
> +{
> +	struct scsi_device *sdev, *tmp_sdev = NULL;
> +	int err = 0, timeout = -1;
> +
> +	if (!timer)
> +		return -EINVAL;

same API issue as above.  don't work around broken callers.

> +	/* Find the first device and validate the timeout value.  All devices
> +	 * managed by this host are timed using the transport 
> +	 * dev_loss_tmo timeout. */
> +	__shost_for_each_device(sdev, shost) {

missing locking again.

> +		tmp_sdev = sdev;
> +		timeout = fc_dev_loss_tmo(sdev);
> +		if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT) {
> +			return -EINVAL;
> +		} else {
> +			break;
> +		}

superflous else, superflous braces

> +	__shost_for_each_device(sdev, shost) {

missing locking.

> +int fc_host_unblock(struct Scsi_Host *shost, struct timer_list *timer)
> +{
> +	struct scsi_device *sdev;
> +	int err = 0;
> +	
> +	/* Stop the host timer first. Take no action on the del_timer
> +	 * failure as the state machine state change will validate the
> +	 * transaction. */
> +	if (!timer)
> +		return -EINVAL;

broken caller workaround issue again.

> +	__shost_for_each_device(sdev, shost) {

missing locking.


Your probably want to invent some scheme to not duplicate the same
code for host/target/device each time.