Re: [PATCH scsi-misc-2.6 05/13] scsi: remove a timer race from scsi_queue_insert() and cleanup timer

2005-03-31 Thread Tejun Heo
 Hello, Chritoph.

On Thu, Mar 31, 2005 at 11:13:53AM +0100, Christoph Hellwig wrote:
> > /* Queue the command and wait for it to complete */
> > /* Abuse eh_timeout in the scsi_cmnd struct for our purposes */
> > init_timer(&cmd->eh_timeout);
> > +   cmd->eh_timeout.function = NULL;
> 
> I'd rather not see aic7xxx poke even deeper into this internal code.
> Can you please switch it to use a timer of it's own first?

 Yes, I'll.

 Thanks.

-- 
tejun

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH scsi-misc-2.6 05/13] scsi: remove a timer race from scsi_queue_insert() and cleanup timer

2005-03-31 Thread Christoph Hellwig
>   /* Queue the command and wait for it to complete */
>   /* Abuse eh_timeout in the scsi_cmnd struct for our purposes */
>   init_timer(&cmd->eh_timeout);
> + cmd->eh_timeout.function = NULL;

I'd rather not see aic7xxx poke even deeper into this internal code.
Can you please switch it to use a timer of it's own first?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


Re: [PATCH scsi-misc-2.6 05/13] scsi: remove a timer race from scsi_queue_insert() and cleanup timer

2005-03-31 Thread Tejun Heo
05_scsi_timer_cleanup.patch

scsi_queue_insert() has four callers.  Three callers call with
timer disabled and one (the second invocation in
scsi_dispatch_cmd()) calls with timer activated.
scsi_queue_insert() used to always call scsi_delete_timer()
and ignore the return value.  This results in race with timer
expiration.  Remove scsi_delete_timer() call from
scsi_queue_insert() and make the caller delete timer and check
the return value.

While at it, as, once a scsi timer is added, it should expire
or be deleted before reused, make scsi_add_timer() strict
about timer reuses.  Now timer expiration function should
clear ->function and all timer deletion should go through
scsi_delete_timer().  Also, remove bogus ->eh_action tests
from scsi_eh_{done|times_out} functions.  The condition is
always true and the test is somewhat misleading.

Signed-off-by: Tejun Heo <[EMAIL PROTECTED]>

 aic7xxx/aic79xx_osm.c |1 +
 aic7xxx/aic7xxx_osm.c |1 +
 scsi.c|   10 ++
 scsi_error.c  |   25 -
 scsi_lib.c|6 --
 5 files changed, 16 insertions(+), 27 deletions(-)

Index: scsi-export/drivers/scsi/aic7xxx/aic79xx_osm.c
===
--- scsi-export.orig/drivers/scsi/aic7xxx/aic79xx_osm.c 2005-03-31 
17:42:04.0 +0900
+++ scsi-export/drivers/scsi/aic7xxx/aic79xx_osm.c  2005-03-31 
18:06:21.0 +0900
@@ -2725,6 +2725,7 @@ ahd_linux_dv_target(struct ahd_softc *ah
/* Queue the command and wait for it to complete */
/* Abuse eh_timeout in the scsi_cmnd struct for our purposes */
init_timer(&cmd->eh_timeout);
+   cmd->eh_timeout.function = NULL;
 #ifdef AHD_DEBUG
if ((ahd_debug & AHD_SHOW_MESSAGES) != 0)
/*
Index: scsi-export/drivers/scsi/aic7xxx/aic7xxx_osm.c
===
--- scsi-export.orig/drivers/scsi/aic7xxx/aic7xxx_osm.c 2005-03-31 
17:42:04.0 +0900
+++ scsi-export/drivers/scsi/aic7xxx/aic7xxx_osm.c  2005-03-31 
18:06:21.0 +0900
@@ -2409,6 +2409,7 @@ ahc_linux_dv_target(struct ahc_softc *ah
/* Queue the command and wait for it to complete */
/* Abuse eh_timeout in the scsi_cmnd struct for our purposes */
init_timer(&cmd->eh_timeout);
+   cmd->eh_timeout.function = NULL;
 #ifdef AHC_DEBUG
if ((ahc_debug & AHC_SHOW_MESSAGES) != 0)
/*
Index: scsi-export/drivers/scsi/scsi.c
===
--- scsi-export.orig/drivers/scsi/scsi.c2005-03-31 18:06:20.0 
+0900
+++ scsi-export/drivers/scsi/scsi.c 2005-03-31 18:06:21.0 +0900
@@ -638,10 +638,12 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
}
spin_unlock_irqrestore(host->host_lock, flags);
if (rtn) {
-   atomic_inc(&cmd->device->iodone_cnt);
-   scsi_queue_insert(cmd,
-   (rtn == SCSI_MLQUEUE_DEVICE_BUSY) ?
-rtn : SCSI_MLQUEUE_HOST_BUSY);
+   if (scsi_delete_timer(cmd)) {
+   atomic_inc(&cmd->device->iodone_cnt);
+   scsi_queue_insert(cmd,
+ (rtn == SCSI_MLQUEUE_DEVICE_BUSY) ?
+ rtn : SCSI_MLQUEUE_HOST_BUSY);
+   }
SCSI_LOG_MLQUEUE(3,
printk("queuecommand : request rejected\n"));
}
Index: scsi-export/drivers/scsi/scsi_error.c
===
--- scsi-export.orig/drivers/scsi/scsi_error.c  2005-03-31 18:06:20.0 
+0900
+++ scsi-export/drivers/scsi/scsi_error.c   2005-03-31 18:06:21.0 
+0900
@@ -107,14 +107,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
 void scsi_add_timer(struct scsi_cmnd *scmd, int timeout,
void (*complete)(struct scsi_cmnd *))
 {
-
-   /*
-* 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 (scmd->eh_timeout.function)
-   del_timer(&scmd->eh_timeout);
+   BUG_ON(scmd->eh_timeout.function);
 
scmd->eh_timeout.data = (unsigned long)scmd;
scmd->eh_timeout.expires = jiffies + timeout;
@@ -168,6 +161,8 @@ EXPORT_SYMBOL(scsi_delete_timer);
  **/
 void scsi_times_out(struct scsi_cmnd *scmd)
 {
+   scmd->eh_timeout.function = NULL;
+
scsi_log_completion(scmd, TIMEOUT_ERROR);
 
if (scmd->device->host->hostt->eh_timed_out)
@@ -439,12 +434,12 @@ static int scsi_eh_completed_normally(