RE: [Patch] Fix Suspend I/O block/unblock patch

RE: [Patch] Fix Suspend I/O block/unblock patch

[James.Smart]

> Could you give an example of this problem?  If there's a demonstrable
> issue with device_for_each_child(), we can probably persuade Greg to
> alter it.

Here's the stack trace that was giving us headaches...

Oct 11 10:04:37 daffy kernel: Debug: sleeping function called from invalid context at include/linux/rwsem.h:43
Oct 11 10:04:37 daffy kernel: in_atomic():1, irqs_disabled():0
Oct 11 10:04:37 daffy kernel:  [<c0122f15>] __might_sleep+0x95/0xb0
Oct 11 10:04:37 daffy kernel:  [<f8834600>] fc_timeout_blocked_tgt+0x0/0x40 [scsi_transport_fc]
Oct 11 10:04:37 daffy kernel:  [<c0230516>] device_for_each_child+0x26/0x80
Oct 11 10:04:37 daffy kernel:  [<f88345f0>] fc_device_unblock+0x0/0x10 [scsi_transport_fc]
Oct 11 10:04:37 daffy kernel:  [<f8834600>] fc_timeout_blocked_tgt+0x0/0x40 [scsi_transport_fc]
Oct 11 10:04:38 daffy kernel:  [<c012e2f9>] run_timer_softirq+0xd9/0x170
Oct 11 10:04:38 daffy kernel:  [<c012a262>] __do_softirq+0x62/0xe0
Oct 11 10:04:38 daffy kernel:  [<c012a30d>] do_softirq+0x2d/0x40
Oct 11 10:04:38 daffy kernel:  [<c011ac3a>] smp_apic_timer_interrupt+0xfa/0x110
Oct 11 10:04:38 daffy kernel:  [<c0107a6e>] apic_timer_interrupt+0x1a/0x20
Oct 11 10:04:38 daffy kernel:  [<c0105030>] default_idle+0x0/0x40
Oct 11 10:04:38 daffy kernel:  [<c0105059>] default_idle+0x29/0x40
Oct 11 10:04:38 daffy kernel:  [<c01050e4>] cpu_idle+0x34/0x50
Oct 11 10:04:38 daffy kernel:  target207:0:2: blocked target time out: target resuming


-- James S

RE: [Patch] Fix Suspend I/O block/unblock patch

[James Bottomley]

On Sat, 2004-10-16 at 16:59, James.Smart@Emulex.Com wrote:
> > Could you give an example of this problem?  If there's a demonstrable
> > issue with device_for_each_child(), we can probably persuade Greg to
> > alter it.
> 
> Here's the stack trace that was giving us headaches...
> 
> Oct 11 10:04:37 daffy kernel: Debug: sleeping function called from invalid context at include/linux/rwsem.h:43
> Oct 11 10:04:37 daffy kernel: in_atomic():1, irqs_disabled():0

Yes, call from our own timer.  I was slightly worried about the amount
of work the list might have to do in the timer.  This provides another
excuse to move it to a workqueue.

How does the attached fare in this situation (it compiles but I have no
ability to test it)?

James

===== drivers/scsi/scsi_transport_fc.c 1.6 vs edited =====
--- 1.6/drivers/scsi/scsi_transport_fc.c	2004-10-11 10:03:45 -05:00
+++ edited/drivers/scsi/scsi_transport_fc.c	2004-10-16 18:03:28 -05:00
@@ -29,6 +29,8 @@
 
 static void transport_class_release(struct class_device *class_dev);
 static void host_class_release(struct class_device *class_dev);
+static void fc_timeout_blocked_host(void *data);
+static void fc_timeout_blocked_tgt(void *data);
 
 #define FC_STARGET_NUM_ATTRS 	4	/* increase this if you add attributes */
 #define FC_STARGET_OTHER_ATTRS 	0	/* increase this if you add "always on"
@@ -87,7 +89,8 @@
 	fc_starget_port_name(starget) = -1;
 	fc_starget_port_id(starget) = -1;
 	fc_starget_dev_loss_tmo(starget) = -1;
-	init_timer(&fc_starget_dev_loss_timer(starget));
+	INIT_WORK(&fc_starget_dev_loss_work(starget),
+		  fc_timeout_blocked_tgt, starget);
 	return 0;
 }
 
@@ -99,7 +102,8 @@
 	 * all transport attributes to valid values per host.
 	 */
 	fc_host_link_down_tmo(shost) = -1;
-	init_timer(&fc_host_link_down_timer(shost));
+	INIT_WORK(&fc_host_link_down_work(shost),
+		  fc_timeout_blocked_host, shost);
 	return 0;
 }
 
@@ -353,7 +357,7 @@
  *			 that fail to recover in the alloted time.
  * @data:	scsi target that failed to reappear in the alloted time.
  **/
-static void fc_timeout_blocked_tgt(unsigned long data)
+static void fc_timeout_blocked_tgt(void  *data)
 {
 	struct scsi_target *starget = (struct scsi_target *)data;
 
@@ -388,7 +392,7 @@
 fc_target_block(struct scsi_target *starget)
 {
 	int timeout = fc_starget_dev_loss_tmo(starget);
-	struct timer_list *timer = &fc_starget_dev_loss_timer(starget);
+	struct work_struct *work = &fc_starget_dev_loss_work(starget);
 
 	if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
 		return -EINVAL;
@@ -396,10 +400,7 @@
 	device_for_each_child(&starget->dev, NULL, fc_device_block);
 
 	/* The scsi lld blocks this target for the timeout period only. */
-	timer->data = (unsigned long)starget;
-	timer->expires = jiffies + timeout * HZ;
-	timer->function = fc_timeout_blocked_tgt;
-	add_timer(timer);
+	schedule_delayed_work(work, timeout * HZ);
 
 	return 0;
 }
@@ -424,7 +425,7 @@
 	 * failure as the state machine state change will validate the
 	 * transaction. 
 	 */
-	del_timer_sync(&fc_starget_dev_loss_timer(starget));
+	del_timer_sync(&fc_starget_dev_loss_work(starget).timer);
 
 	device_for_each_child(&starget->dev, NULL, fc_device_unblock);
 }
@@ -436,7 +437,7 @@
  * @data:	scsi host that failed to recover its devices in the alloted
  *		time.
  **/
-static void fc_timeout_blocked_host(unsigned long data)
+static void fc_timeout_blocked_host(void  *data)
 {
 	struct Scsi_Host *shost = (struct Scsi_Host *)data;
 	struct scsi_device *sdev;
@@ -475,7 +476,7 @@
 {
 	struct scsi_device *sdev;
 	int timeout = fc_host_link_down_tmo(shost);
-	struct timer_list *timer = &fc_host_link_down_timer(shost);
+	struct work_struct *work = &fc_host_link_down_work(shost);
 
 	if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
 		return -EINVAL;
@@ -484,11 +485,7 @@
 		scsi_internal_device_block(sdev);
 	}
 
-	/* The scsi lld blocks this host for the timeout period only. */
-	timer->data = (unsigned long)shost;
-	timer->expires = jiffies + timeout * HZ;
-	timer->function = fc_timeout_blocked_host;
-	add_timer(timer);
+	schedule_delayed_work(work, timeout * HZ);
 
 	return 0;
 }
@@ -516,7 +513,7 @@
 	 * failure as the state machine state change will validate the
 	 * transaction.
 	 */
-	del_timer_sync(&fc_host_link_down_timer(shost));
+	del_timer_sync(&fc_host_link_down_work(shost).timer);
 	shost_for_each_device(sdev, shost) {
 		scsi_internal_device_unblock(sdev);
 	}
===== include/scsi/scsi_transport_fc.h 1.5 vs edited =====
--- 1.5/include/scsi/scsi_transport_fc.h	2004-10-11 10:03:45 -05:00
+++ edited/include/scsi/scsi_transport_fc.h	2004-10-16 17:49:20 -05:00
@@ -29,7 +29,7 @@
 	uint64_t node_name;
 	uint64_t port_name;
 	uint32_t dev_loss_tmo;	/* Remote Port loss timeout in seconds. */
-	struct timer_list dev_loss_timer;
+	struct work_struct dev_loss_work;
 };
 
 #define fc_starget_port_id(x) \
@@ -40,18 +40,18 @@
 	(((struct fc_starget_attrs *)&(x)->starget_data)->port_name)
 #define fc_starget_dev_loss_tmo(x) \
 	(((struct fc_starget_attrs *)&(x)->starget_data)->dev_loss_tmo)
-#define fc_starget_dev_loss_timer(x) \
-	(((struct fc_starget_attrs *)&(x)->starget_data)->dev_loss_timer)
+#define fc_starget_dev_loss_work(x) \
+	(((struct fc_starget_attrs *)&(x)->starget_data)->dev_loss_work)
 
 struct fc_host_attrs {
 	uint32_t link_down_tmo;	/* Link Down timeout in seconds. */
-	struct timer_list link_down_timer;
+	struct work_struct link_down_work;
 };
 
 #define fc_host_link_down_tmo(x) \
 	(((struct fc_host_attrs *)(x)->shost_data)->link_down_tmo)
-#define fc_host_link_down_timer(x) \
-	(((struct fc_host_attrs *)(x)->shost_data)->link_down_timer)
+#define fc_host_link_down_work(x) \
+	(((struct fc_host_attrs *)(x)->shost_data)->link_down_work)
 

 /* The functions by which the transport class and the driver communicate */

[Patch] Fix Suspend I/O block/unblock patch

[James.Smart]

In further testing of the fc block/unblock patch, the recommendation to use device_for_each_child() instead of shost_for_each_device() in the target functions was not a good one. The calling sequences on the unblock path, which may eventually block, are not good for device_for_each_child(), which takes and holds a semaphore.

This patch reverts to using shost_for_each_device(), which takes the proper locks and performs ref counting. Although the block portion doesn't encounter the problem, it too was converted for the sake of consistency.  The patch also makes one other minor change - allowing an offline state change to occur if blocked.

-- James


This patch is specific to the scsi-target-2.6 bitkeeper tree.

diff -uNr scsi-target-2.6/drivers/scsi/scsi_lib.c scsi-target-2.6.patch/drivers/scsi/scsi_lib.c
--- scsi-target-2.6/drivers/scsi/scsi_lib.c	2004-10-14 08:26:13.776167096 -0400
+++ scsi-target-2.6.patch/drivers/scsi/scsi_lib.c	2004-10-14 08:26:13.778166792 -0400
@@ -1664,6 +1664,7 @@
 		case SDEV_CREATED:
 		case SDEV_RUNNING:
 		case SDEV_QUIESCE:
+		case SDEV_BLOCK:
 			break;
 		default:
 			goto illegal;
diff -uNr scsi-target-2.6/drivers/scsi/scsi_transport_fc.c scsi-target-2.6.patch/drivers/scsi/scsi_transport_fc.c
--- scsi-target-2.6/drivers/scsi/scsi_transport_fc.c	2004-10-14 08:26:13.783166032 -0400
+++ scsi-target-2.6.patch/drivers/scsi/scsi_transport_fc.c	2004-10-14 08:26:13.785165728 -0400
@@ -325,29 +325,6 @@
 EXPORT_SYMBOL(fc_release_transport);
 
 
-
-/**
- * fc_device_block - called by target functions to block a scsi device
- * @dev:	scsi device
- * @data:	unused
- **/
-static int fc_device_block(struct device *dev, void *data)
-{
-	scsi_internal_device_block(to_scsi_device(dev));
-	return 0;
-}
-
-/**
- * fc_device_unblock - called by target functions to unblock a scsi device
- * @dev:	scsi device
- * @data:	unused
- **/
-static int fc_device_unblock(struct device *dev, void *data)
-{
-	scsi_internal_device_unblock(to_scsi_device(dev));
-	return 0;
-}
-
 /**
  * fc_timeout_blocked_tgt - Timeout handler for blocked scsi targets
  *			 that fail to recover in the alloted time.
@@ -356,6 +333,8 @@
 static void fc_timeout_blocked_tgt(unsigned long data)
 {
 	struct scsi_target *starget = (struct scsi_target *)data;
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	struct scsi_device *sdev;
 
 	dev_printk(KERN_ERR, &starget->dev, 
 		"blocked target time out: target resuming\n");
@@ -365,7 +344,10 @@
 	 * unblock this device, then IO errors will probably
 	 * result if the host still isn't ready.
 	 */
-	device_for_each_child(&starget->dev, NULL, fc_device_unblock);
+	shost_for_each_device(sdev, shost) {
+		if (sdev->id == starget->id)
+			scsi_internal_device_unblock(sdev);
+	}
 }
 
 /**
@@ -387,13 +369,18 @@
 int
 fc_target_block(struct scsi_target *starget)
 {
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	struct scsi_device *sdev;
 	int timeout = fc_starget_dev_loss_tmo(starget);
 	struct timer_list *timer = &fc_starget_dev_loss_timer(starget);
 
 	if (timeout < 0 || timeout > SCSI_DEVICE_BLOCK_MAX_TIMEOUT)
 		return -EINVAL;
 
-	device_for_each_child(&starget->dev, NULL, fc_device_block);
+	shost_for_each_device(sdev, shost) {
+		if (sdev->id == starget->id)
+			scsi_internal_device_block(sdev);
+	}
 
 	/* The scsi lld blocks this target for the timeout period only. */
 	timer->data = (unsigned long)starget;
@@ -419,6 +406,9 @@
 void
 fc_target_unblock(struct scsi_target *starget)
 {
+	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
+	struct scsi_device *sdev;
+
 	/* 
 	 * Stop the target timer first. Take no action on the del_timer
 	 * failure as the state machine state change will validate the
@@ -426,7 +416,10 @@
 	 */
 	del_timer_sync(&fc_starget_dev_loss_timer(starget));
 
-	device_for_each_child(&starget->dev, NULL, fc_device_unblock);
+	shost_for_each_device(sdev, shost) {
+		if (sdev->id == starget->id)
+			scsi_internal_device_unblock(sdev);
+	}
 }
 EXPORT_SYMBOL(fc_target_unblock);
 

Re: [Patch] Fix Suspend I/O block/unblock patch

[James Bottomley]

On Thu, 2004-10-14 at 09:31, James.Smart@Emulex.Com wrote:
> In further testing of the fc block/unblock patch, the recommendation to use device_for_each_child() instead of shost_for_each_device() in the target functions was not a good one. The calling sequences on the unblock path, which may eventually block, are not good for device_for_each_child(), which takes and holds a semaphore.

Could you give an example of this problem?  If there's a demonstrable
issue with device_for_each_child(), we can probably persuade Greg to
alter it.

James