From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Bottomley Subject: [PATCH] update the fc_transport_class to use a workqueue instead of a timeout Date: 26 Oct 2004 21:43:51 -0400 Sender: linux-scsi-owner@vger.kernel.org Message-ID: <1098841437.1762.683.camel@mulgrave> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: Received: from stat16.steeleye.com ([209.192.50.48]:29114 "EHLO hancock.sc.steeleye.com") by vger.kernel.org with ESMTP id S261522AbUJ0Bn6 (ORCPT ); Tue, 26 Oct 2004 21:43:58 -0400 Received: from midgard.sc.steeleye.com (midgard.sc.steeleye.com [172.17.6.40]) by hancock.sc.steeleye.com (8.11.6/8.11.6) with ESMTP id i9R1hvm27480 for ; Tue, 26 Oct 2004 21:43:57 -0400 List-Id: linux-scsi@vger.kernel.org To: SCSI Mailing List The amount of work that has to be done in the timeout routines is really a bit much if there's a large number of LUNS. Plus the device_for_each_child needs user context to get the bus semaphore, so the solution is to migrate them from a timer to delayed work. There's still a race here in that the timer may still be ticking when the device is destroyed ... To fix this, I think we may need to introduce a destroy callback to the transport class. 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-26 17:12:02 -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,8 @@ * failure as the state machine state change will validate the * transaction. */ - del_timer_sync(&fc_starget_dev_loss_timer(starget)); + if (cancel_delayed_work(&fc_starget_dev_loss_work(starget))) + flush_scheduled_work(); device_for_each_child(&starget->dev, NULL, fc_device_unblock); } @@ -436,7 +438,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 +477,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 +486,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 +514,9 @@ * failure as the state machine state change will validate the * transaction. */ - del_timer_sync(&fc_host_link_down_timer(shost)); + if (cancel_delayed_work(&fc_host_link_down_work(shost))) + flush_scheduled_work(); + 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-26 17:12:10 -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 */