[PATCH scsi-misc-2.6 00/07] scsi: timer updates

[PATCH scsi-misc-2.6 00/07] scsi: timer updates

[Tejun Heo]

 Hello, James, Jens and Christoph.

 This patchset removes misuses of scmd->eh_timeout and unexports SCSI
timer interface such that no one can misuse it anymore.  #02 assumes
that the preceding scsi_send_eh_cmnd() patch is applied.  Tested and
worked for me.

 The following bugs are fixed.

 * Race condition between eh and normal completion path for eh_timer
 * scsi_delete_timer() race in scsi_queue_insert()

[ Start of patch descriptions ]

01_scsi_timer_update_aic7xxx.patch
	: make aic7xxx use its own timer instead of scmd->eh_timeout

	aic7xxx used scmd->eh_timeout in its dv routines.  This kind
	of usage requires knowledge of and creates dependency into the
	SCSI midlayer unnecessarily.  This patch makes aic7xxx driver
	use its own timer instead of scmd->eh_timeout.
	Suggested by Christoph Hellwig.

02_scsi_timer_eh_timer_fix.patch
	: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

	scmd->eh_timeout is used to resolve the race between command
	completion and timeout.  However, during error handling,
	scsi_send_eh_cmnd uses scmd->eh_timeout.  This creates a race
	condition between eh and normal completion for a request which
	has timed out and in the process of error handling.  If the
	request completes while scmd->eh_timeout is being used by eh,
	eh timeout is lost and the command will be handled by both eh
	and completion path.  This patch fixes the race by making
	scsi_send_eh_cmnd() use its own timer.

03_scsi_timer_dispatch_race_fix.patch
	: remove a timer race in scsi_queue_insert()

	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.

04_scsi_timer_remove_delete_timer_from_reset_provider.patch
	: remove unnecessary scsi_delete_timer() call in scsi_reset_provider()

	scsi_reset_provider() calls scsi_delete_timer() on exit which
	isn't necessary.  Remove it.

05_scsi_timer_unexport_timer_functions.patch
	: unexport scsi_{add|delete}_timer()

	SCSI cmd timer has specific synchronization/semantic
	requirements and shouldn't be directly used outside SCSI
	midlayer.  With aic7xxx driver updated, there's no user left.
	This patch unexports scsi_{add|delete}_timer() routines and
	also removes @complete argument from scsi_add_timer().  The
	change makes the use of scsi_times_out() confined in
	scsi_error.c, so move it upward such that no prototype is
	needed and make it static.

06_scsi_timer_update_api_doc.patch
	: Delete scsi_{add|delete}_timer() from scsi_mid_low_api.txt

	As scsi_{add|delete}_timer() got unexported, remove them from
	the API doc.

07_scsi_timer_strict_reuse.patch
	: make reuse of SCSI cmd timer strict

	SCSI cmd timer shouldn't be reused while it's active.  Make
	sure that the unused condition is marked with
	eh_timeout->function = NULL and BUG() active reuse path.

[ End of patch descriptions ]

 Thanks a lot.

Re: [PATCH scsi-misc-2.6 01/07] scsi: make aic7xxx use its own timer instead of scmd->eh_timeout

[Tejun Heo]

01_scsi_timer_update_aic7xxx.patch

	aic7xxx used scmd->eh_timeout in its dv routines.  This kind
	of usage requires knowledge of and creates dependency into the
	SCSI midlayer unnecessarily.  This patch makes aic7xxx driver
	use its own timer instead of scmd->eh_timeout.
	Suggested by Christoph Hellwig.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 aic79xx_osm.c |   24 ++++++++++++++++--------
 aic7xxx_osm.c |   24 ++++++++++++++++--------
 2 files changed, 32 insertions(+), 16 deletions(-)

Index: scsi-reqfn-export/drivers/scsi/aic7xxx/aic79xx_osm.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/aic7xxx/aic79xx_osm.c	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/aic7xxx/aic79xx_osm.c	2005-04-11 03:42:11.000000000 +0900
@@ -460,7 +460,7 @@ static void ahd_linux_initialize_scsi_bu
 static void ahd_linux_size_nseg(void);
 static void ahd_linux_thread_run_complete_queue(struct ahd_softc *ahd);
 static void ahd_linux_start_dv(struct ahd_softc *ahd);
-static void ahd_linux_dv_timeout(struct scsi_cmnd *cmd);
+static void ahd_linux_dv_timeout(unsigned long data);
 static int  ahd_linux_dv_thread(void *data);
 static void ahd_linux_kill_dv_thread(struct ahd_softc *ahd);
 static void ahd_linux_dv_target(struct ahd_softc *ahd, u_int target);
@@ -2628,6 +2628,7 @@ ahd_linux_dv_target(struct ahd_softc *ah
 	struct	 ahd_devinfo devinfo;
 	struct	 ahd_linux_target *targ;
 	struct	 scsi_cmnd *cmd;
+	struct	 timer_list timer;
 	struct	 scsi_device *scsi_dev;
 	struct	 scsi_sense_data *sense;
 	uint8_t *buffer;
@@ -2723,8 +2724,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);
+		init_timer(&timer);
 #ifdef AHD_DEBUG
 		if ((ahd_debug & AHD_SHOW_MESSAGES) != 0)
 			/*
@@ -2734,7 +2734,10 @@ ahd_linux_dv_target(struct ahd_softc *ah
 			 */
 			timeout += HZ;
 #endif
-		scsi_add_timer(cmd, timeout, ahd_linux_dv_timeout);
+		timer.expires = jiffies + timeout;
+		timer.function = ahd_linux_dv_timeout;
+		timer.data = (unsigned long)cmd;
+		add_timer(&timer);
 		/*
 		 * In 2.5.X, it is assumed that all calls from the
 		 * "midlayer" (which we are emulating) will have the
@@ -2754,6 +2757,13 @@ ahd_linux_dv_target(struct ahd_softc *ah
 #endif
 		down_interruptible(&ahd->platform_data->dv_cmd_sem);
 		/*
+		 * Although the timer could have gone off while we're
+		 * on normal completion path before reaching the
+		 * following line, it's safe because we use
+		 * cmd->host_scribble for synchronization.
+		 */
+		del_timer(&timer);
+		/*
 		 * Wait for the SIMQ to be released so that DV is the
 		 * only reason the queue is frozen.
 		 */
@@ -3636,8 +3646,9 @@ ahd_linux_fallback(struct ahd_softc *ahd
 }
 
 static void
-ahd_linux_dv_timeout(struct scsi_cmnd *cmd)
+ahd_linux_dv_timeout(unsigned long data)
 {
+	struct	scsi_cmnd *cmd = (void *)data;
 	struct	ahd_softc *ahd;
 	struct	scb *scb;
 	u_long	flags;
@@ -3698,9 +3709,6 @@ ahd_linux_dv_complete(struct scsi_cmnd *
 
 	ahd = *((struct ahd_softc **)cmd->device->host->hostdata);
 
-	/* Delete the DV timer before it goes off! */
-	scsi_delete_timer(cmd);
-
 #ifdef AHD_DEBUG
 	if (ahd_debug & AHD_SHOW_DV)
 		printf("%s:%c:%d: Command completed, status= 0x%x\n",
Index: scsi-reqfn-export/drivers/scsi/aic7xxx/aic7xxx_osm.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/aic7xxx/aic7xxx_osm.c	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/aic7xxx/aic7xxx_osm.c	2005-04-11 03:42:11.000000000 +0900
@@ -477,7 +477,7 @@ static void ahc_linux_initialize_scsi_bu
 static void ahc_linux_size_nseg(void);
 static void ahc_linux_thread_run_complete_queue(struct ahc_softc *ahc);
 static void ahc_linux_start_dv(struct ahc_softc *ahc);
-static void ahc_linux_dv_timeout(struct scsi_cmnd *cmd);
+static void ahc_linux_dv_timeout(unsigned long data);
 static int  ahc_linux_dv_thread(void *data);
 static void ahc_linux_kill_dv_thread(struct ahc_softc *ahc);
 static void ahc_linux_dv_target(struct ahc_softc *ahc, u_int target);
@@ -2310,6 +2310,7 @@ ahc_linux_dv_target(struct ahc_softc *ah
 	struct	 ahc_devinfo devinfo;
 	struct	 ahc_linux_target *targ;
 	struct	 scsi_cmnd *cmd;
+	struct	 timer_list timer;
 	struct	 scsi_device *scsi_dev;
 	struct	 scsi_sense_data *sense;
 	uint8_t *buffer;
@@ -2407,8 +2408,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);
+		init_timer(&timer);
 #ifdef AHC_DEBUG
 		if ((ahc_debug & AHC_SHOW_MESSAGES) != 0)
 			/*
@@ -2418,7 +2418,10 @@ ahc_linux_dv_target(struct ahc_softc *ah
 			 */
 			timeout += HZ;
 #endif
-		scsi_add_timer(cmd, timeout, ahc_linux_dv_timeout);
+		timer.expires = jiffies + timeout;
+		timer.function = ahc_linux_dv_timeout;
+		timer.data = (unsigned long)cmd;
+		add_timer(&timer);
 		/*
 		 * In 2.5.X, it is assumed that all calls from the
 		 * "midlayer" (which we are emulating) will have the
@@ -2438,6 +2441,13 @@ ahc_linux_dv_target(struct ahc_softc *ah
 #endif
 		down_interruptible(&ahc->platform_data->dv_cmd_sem);
 		/*
+		 * Although the timer could have gone off while we're
+		 * on normal completion path before reaching the
+		 * following line, it's safe because we use
+		 * cmd->host_scribble for synchronization.
+		 */
+		del_timer(&timer);
+		/*
 		 * Wait for the SIMQ to be released so that DV is the
 		 * only reason the queue is frozen.
 		 */
@@ -3313,8 +3323,9 @@ ahc_linux_fallback(struct ahc_softc *ahc
 }
 
 static void
-ahc_linux_dv_timeout(struct scsi_cmnd *cmd)
+ahc_linux_dv_timeout(unsigned long data)
 {
+	struct	scsi_cmnd *cmd = (void *)data;
 	struct	ahc_softc *ahc;
 	struct	scb *scb;
 	u_long	flags;
@@ -3375,9 +3386,6 @@ ahc_linux_dv_complete(struct scsi_cmnd *
 
 	ahc = *((struct ahc_softc **)cmd->device->host->hostdata);
 
-	/* Delete the DV timer before it goes off! */
-	scsi_delete_timer(cmd);
-
 #ifdef AHC_DEBUG
 	if (ahc_debug & AHC_SHOW_DV)
 		printf("%s:%d:%d: Command completed, status= 0x%x\n",

Re: [PATCH scsi-misc-2.6 02/07] scsi: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

[Tejun Heo]

02_scsi_timer_eh_timer_fix.patch

	scmd->eh_timeout is used to resolve the race between command
	completion and timeout.  However, during error handling,
	scsi_send_eh_cmnd uses scmd->eh_timeout.  This creates a race
	condition between eh and normal completion for a request which
	has timed out and in the process of error handling.  If the
	request completes while scmd->eh_timeout is being used by eh,
	eh timeout is lost and the command will be handled by both eh
	and completion path.  This patch fixes the race by making
	scsi_send_eh_cmnd() use its own timer.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 scsi_error.c |   64 ++++++++++++++++++-----------------------------------------
 scsi_priv.h  |    1 
 2 files changed, 20 insertions(+), 45 deletions(-)

Index: scsi-reqfn-export/drivers/scsi/scsi_error.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_error.c	2005-04-11 03:42:11.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_error.c	2005-04-11 03:42:11.000000000 +0900
@@ -420,46 +420,12 @@ static int scsi_eh_completed_normally(st
 }
 
 /**
- * scsi_eh_times_out - timeout function for error handling.
- * @scmd:	Cmd that is timing out.
- *
- * Notes:
- *    During error handling, the kernel thread will be sleeping waiting
- *    for some action to complete on the device.  our only job is to
- *    record that it timed out, and to wake up the thread.
- **/
-static void scsi_eh_times_out(struct scsi_cmnd *scmd)
-{
-	scsi_eh_eflags_set(scmd, SCSI_EH_REC_TIMEOUT);
-	SCSI_LOG_ERROR_RECOVERY(3, printk("%s: scmd:%p\n", __FUNCTION__,
-					  scmd));
-
-	if (scmd->device->host->eh_action)
-		up(scmd->device->host->eh_action);
-}
-
-/**
  * scsi_eh_done - Completion function for error handling.
  * @scmd:	Cmd that is done.
  **/
 static void scsi_eh_done(struct scsi_cmnd *scmd)
 {
-	/*
-	 * if the timeout handler is already running, then just set the
-	 * flag which says we finished late, and return.  we have no
-	 * way of stopping the timeout handler from running, so we must
-	 * always defer to it.
-	 */
-	if (del_timer(&scmd->eh_timeout)) {
-		scmd->request->rq_status = RQ_SCSI_DONE;
-		scmd->owner = SCSI_OWNER_ERROR_HANDLER;
-
-		SCSI_LOG_ERROR_RECOVERY(3, printk("%s scmd: %p result: %x\n",
-					   __FUNCTION__, scmd, scmd->result));
-
-		if (scmd->device->host->eh_action)
-			up(scmd->device->host->eh_action);
-	}
+	up(scmd->device->host->eh_action);
 }
 
 /**
@@ -478,6 +444,7 @@ static int scsi_send_eh_cmnd(struct scsi
 {
 	struct scsi_device *sdev = scmd->device;
 	struct Scsi_Host *shost = sdev->host;
+	struct timer_list timer;
 	DECLARE_MUTEX_LOCKED(sem);
 	unsigned long flags;
 	int rtn = SUCCESS;
@@ -492,7 +459,11 @@ static int scsi_send_eh_cmnd(struct scsi
 		scmd->cmnd[1] = (scmd->cmnd[1] & 0x1f) |
 			(sdev->lun << 5 & 0xe0);
 
-	scsi_add_timer(scmd, timeout, scsi_eh_times_out);
+	init_timer(&timer);
+	timer.expires = jiffies + timeout;
+	timer.function = (void (*)(unsigned long))scsi_eh_done;
+	timer.data = (unsigned long)scmd;
+	add_timer(&timer);
 
 	/*
 	 * set up the semaphore so we wait for the command to complete.
@@ -508,14 +479,19 @@ static int scsi_send_eh_cmnd(struct scsi
 	down(&sem);
 	scsi_log_completion(scmd, SUCCESS);
 
-	shost->eh_action = NULL;
-
-	/*
-	 * see if timeout.  if so, tell the host to forget about it.
-	 * in other words, we don't want a callback any more.
-	 */
-	if (scsi_eh_eflags_chk(scmd, SCSI_EH_REC_TIMEOUT)) {
-		scsi_eh_eflags_clr(scmd,  SCSI_EH_REC_TIMEOUT);
+	if (del_timer(&timer)) {
+		SCSI_LOG_ERROR_RECOVERY(3,
+			printk("scsi_eh_done scmd: %p result: %x\n",
+			       scmd, scmd->result));
+		scmd->request->rq_status = RQ_SCSI_DONE;
+		scmd->owner = SCSI_OWNER_ERROR_HANDLER;
+	} else {
+		/*
+		 * Timed out.  Tell the host to forget about it.  In
+		 * other words, we don't want a callback any more.
+		 */
+		SCSI_LOG_ERROR_RECOVERY(3,
+			printk("scsi_eh_times_out scmd: %p\n", scmd));
 		scmd->owner = SCSI_OWNER_LOWLEVEL;
 
 		/*
Index: scsi-reqfn-export/drivers/scsi/scsi_priv.h
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_priv.h	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_priv.h	2005-04-11 03:42:11.000000000 +0900
@@ -42,7 +42,6 @@ struct Scsi_Host;
 	(scp->eh_eflags = 0)
 
 #define SCSI_EH_CANCEL_CMD	0x0001	/* Cancel this cmd */
-#define SCSI_EH_REC_TIMEOUT	0x0002	/* EH retry timed out */
 
 #define SCSI_SENSE_VALID(scmd) \
 	(((scmd)->sense_buffer[0] & 0x70) == 0x70)

Re: [PATCH scsi-misc-2.6 02/07] scsi: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

[James Bottomley]

On Mon, 2005-04-11 at 03:45 +0900, Tejun Heo wrote:
> 	scmd->eh_timeout is used to resolve the race between command
> 	completion and timeout.  However, during error handling,
> 	scsi_send_eh_cmnd uses scmd->eh_timeout.  This creates a race
> 	condition between eh and normal completion for a request which
> 	has timed out and in the process of error handling.  If the
> 	request completes while scmd->eh_timeout is being used by eh,
> 	eh timeout is lost and the command will be handled by both eh
> 	and completion path.  This patch fixes the race by making
> 	scsi_send_eh_cmnd() use its own timer.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>

The logic is wrong in there.

The problem is you cannot rely on the timer being pending as a signal
that the command completed normally.  The kernel doesn't define the
elapsed time between the eh_action semaphore going up and the process
waiting for it being scheduled.  If the timer fires within that
undefined interval, you'll think the command timed out when it, in fact,
completed normally.

James

Re: [PATCH scsi-misc-2.6 02/07] scsi: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

[Tejun Heo]

 Hello, James.

On Mon, Apr 18, 2005 at 10:33:21AM -0500, James Bottomley wrote:
> On Mon, 2005-04-11 at 03:45 +0900, Tejun Heo wrote:
> > 	scmd->eh_timeout is used to resolve the race between command
> > 	completion and timeout.  However, during error handling,
> > 	scsi_send_eh_cmnd uses scmd->eh_timeout.  This creates a race
> > 	condition between eh and normal completion for a request which
> > 	has timed out and in the process of error handling.  If the
> > 	request completes while scmd->eh_timeout is being used by eh,
> > 	eh timeout is lost and the command will be handled by both eh
> > 	and completion path.  This patch fixes the race by making
> > 	scsi_send_eh_cmnd() use its own timer.
> > 
> > Signed-off-by: Tejun Heo <htejun@gmail.com>
> 
> The logic is wrong in there.
> 
> The problem is you cannot rely on the timer being pending as a signal
> that the command completed normally.  The kernel doesn't define the
> elapsed time between the eh_action semaphore going up and the process
> waiting for it being scheduled.  If the timer fires within that
> undefined interval, you'll think the command timed out when it, in fact,
> completed normally.

 The original code also uses timer pending status as a signal that
command completed normally in scsi_eh_done() function, and the same
race also exists in the original code, no matter what we do, unless we
make timer expiration and removal of the command atomic, there will be
a window in which command completes normally but considered to have
timed out as long as we use timer pending status as tie breaker.

 The patch moves the test out of scsi_eh_done() into
scsi_send_eh_cmnd() and this does widen the window by delaying removal
of timer until after the original thread gets scheduled, but usually
not by much and that's how timers are done in many cases (through out
the kernel, timer removals are done with intervening scheduling and no
one considers those incorrect).  So...

 * If you're worried about the race itself, it was there before,
   it shouldn't cause any problem, and we really can't help it.
 * If you're worried about the widening of the window, practically,
   it wouldn't cause problem, and it's how timers are done in many
   other places.

 But if you still don't like it, I can rework it (maybe I'll need to
add a field to Scsi_Host or scsi_cmnd).  So, please let me know.

 Thanks a lot.

-- 
tejun

Re: [PATCH scsi-misc-2.6 02/07] scsi: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

[James Bottomley]

On Tue, 2005-04-19 at 07:31 +0900, Tejun Heo wrote:
>  The original code also uses timer pending status as a signal that
> command completed normally in scsi_eh_done() function, and the same
> race also exists in the original code, no matter what we do, unless we
> make timer expiration and removal of the command atomic, there will be
> a window in which command completes normally but considered to have
> timed out as long as we use timer pending status as tie breaker.

True enough; it's a race between the driver calling scsi_done() and the
timer expiring.  However, that's an acceptable race, since the timer
values are usually in the order of a few seconds and the command usually
completes in milliseconds.  the done function is called in interrupt
context after command completion, so it's as close as possible to the
actual command completion

>  The patch moves the test out of scsi_eh_done() into
> scsi_send_eh_cmnd() and this does widen the window by delaying removal
> of timer until after the original thread gets scheduled, but usually
> not by much and that's how timers are done in many cases (through out
> the kernel, timer removals are done with intervening scheduling and no
> one considers those incorrect).  So...

The time between a thread being marked ready to run and actually running
has been measured in seconds on a heavily loaded system.  That makes the
race window potentially as wide as the timer, which is unacceptable.

James

Re: [PATCH scsi-misc-2.6 02/07] scsi: make scsi_send_eh_cmnd use its own timer instead of scmd->eh_timeout

[Tejun Heo]

James Bottomley wrote:
> On Tue, 2005-04-19 at 07:31 +0900, Tejun Heo wrote:
> 
>> The original code also uses timer pending status as a signal that
>>command completed normally in scsi_eh_done() function, and the same
>>race also exists in the original code, no matter what we do, unless we
>>make timer expiration and removal of the command atomic, there will be
>>a window in which command completes normally but considered to have
>>timed out as long as we use timer pending status as tie breaker.
> 
> 
> True enough; it's a race between the driver calling scsi_done() and the
> timer expiring.  However, that's an acceptable race, since the timer
> values are usually in the order of a few seconds and the command usually
> completes in milliseconds.  the done function is called in interrupt
> context after command completion, so it's as close as possible to the
> actual command completion
> 
> 
>> The patch moves the test out of scsi_eh_done() into
>>scsi_send_eh_cmnd() and this does widen the window by delaying removal
>>of timer until after the original thread gets scheduled, but usually
>>not by much and that's how timers are done in many cases (through out
>>the kernel, timer removals are done with intervening scheduling and no
>>one considers those incorrect).  So...
> 
> 
> The time between a thread being marked ready to run and actually running
> has been measured in seconds on a heavily loaded system.  That makes the
> race window potentially as wide as the timer, which is unacceptable.

 Hmm, okay, agreed.  I'll rework it.  I think I can do it by just adding
a private struct scsi_eh_timer_arg to pass along the timer and the cmd
pointer.  I'll repost the patchset soon.

 Thanks a lot.

-- 
tejun

Re: [PATCH scsi-misc-2.6 03/07] scsi: remove a timer race in scsi_queue_insert()

[Tejun Heo]

03_scsi_timer_dispatch_race_fix.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.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 scsi.c     |   10 ++++++----
 scsi_lib.c |    8 +-------
 2 files changed, 7 insertions(+), 11 deletions(-)

Index: scsi-reqfn-export/drivers/scsi/scsi.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi.c	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi.c	2005-04-11 03:42:12.000000000 +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-reqfn-export/drivers/scsi/scsi_lib.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_lib.c	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_lib.c	2005-04-11 03:42:12.000000000 +0900
@@ -124,13 +124,7 @@ int scsi_queue_insert(struct scsi_cmnd *
 		 printk("Inserting command %p into mlqueue\n", cmd));
 
 	/*
-	 * We are inserting the command into the ml queue.  First, we
-	 * cancel the timer, so it doesn't time out.
-	 */
-	scsi_delete_timer(cmd);
-
-	/*
-	 * Next, set the appropriate busy bit for the device/host.
+	 * Set the appropriate busy bit for the device/host.
 	 *
 	 * If the host/device isn't busy, assume that something actually
 	 * completed, and that we should be able to queue a command now.

Re: [PATCH scsi-misc-2.6 04/07] scsi: remove unnecessary scsi_delete_timer() call in scsi_reset_provider()

[Tejun Heo]

04_scsi_timer_remove_delete_timer_from_reset_provider.patch

	scsi_reset_provider() calls scsi_delete_timer() on exit which
	isn't necessary.  Remove it.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 scsi_error.c |    1 -
 1 files changed, 1 deletion(-)

Index: scsi-reqfn-export/drivers/scsi/scsi_error.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_error.c	2005-04-11 03:42:11.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_error.c	2005-04-11 03:42:12.000000000 +0900
@@ -1843,7 +1843,6 @@ scsi_reset_provider(struct scsi_device *
 		rtn = FAILED;
 	}
 
-	scsi_delete_timer(scmd);
 	scsi_next_command(scmd);
 	return rtn;
 }

Re: [PATCH scsi-misc-2.6 05/07] scsi: unexport scsi_{add|delete}_timer()

[Tejun Heo]

05_scsi_timer_unexport_timer_functions.patch

	SCSI cmd timer has specific synchronization/semantic
	requirements and shouldn't be directly used outside SCSI
	midlayer.  With aic7xxx driver updated, there's no user left.
	This patch unexports scsi_{add|delete}_timer() routines and
	also removes @complete argument from scsi_add_timer().  The
	change makes the use of scsi_times_out() confined in
	scsi_error.c, so move it upward such that no prototype is
	needed and make it static.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 drivers/scsi/scsi.c       |    2 -
 drivers/scsi/scsi_error.c |   80 +++++++++++++++++++++-------------------------
 drivers/scsi/scsi_priv.h  |    3 +
 include/scsi/scsi_eh.h    |    3 -
 4 files changed, 41 insertions(+), 47 deletions(-)

Index: scsi-reqfn-export/drivers/scsi/scsi.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi.c	2005-04-11 03:42:12.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi.c	2005-04-11 03:42:12.000000000 +0900
@@ -600,7 +600,7 @@ int scsi_dispatch_cmd(struct scsi_cmnd *
 	 * AK: unlikely race here: for some reason the timer could
 	 * expire before the serial number is set up below.
 	 */
-	scsi_add_timer(cmd, cmd->timeout_per_command, scsi_times_out);
+	scsi_add_timer(cmd, cmd->timeout_per_command);
 
 	scsi_log_send(cmd);
 
Index: scsi-reqfn-export/drivers/scsi/scsi_error.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_error.c	2005-04-11 03:42:12.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_error.c	2005-04-11 03:42:12.000000000 +0900
@@ -88,6 +88,42 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
 }
 
 /**
+ * scsi_times_out - Timeout function for normal scsi commands.
+ * @scmd:	Cmd that is timing out.
+ *
+ * Notes:
+ *     We do not need to lock this.  There is the potential for a race
+ *     only in that the normal completion handling might run, but if the
+ *     normal completion function determines that the timer has already
+ *     fired, then it mustn't do anything.
+ **/
+static void scsi_times_out(struct scsi_cmnd *scmd)
+{
+	scsi_log_completion(scmd, TIMEOUT_ERROR);
+
+	if (scmd->device->host->hostt->eh_timed_out)
+		switch (scmd->device->host->hostt->eh_timed_out(scmd)) {
+		case EH_HANDLED:
+			__scsi_done(scmd);
+			return;
+		case EH_RESET_TIMER:
+			/* This allows a single retry even of a command
+			 * with allowed == 0 */
+			if (scmd->retries++ > scmd->allowed)
+				break;
+			scsi_add_timer(scmd, scmd->timeout_per_command);
+			return;
+		case EH_NOT_HANDLED:
+			break;
+		}
+
+	if (unlikely(!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
+		panic("Error handler thread not present at %p %p %s %d",
+		      scmd, scmd->device->host, __FILE__, __LINE__);
+	}
+}
+
+/**
  * scsi_add_timer - Start timeout timer for a single scsi command.
  * @scmd:	scsi command that is about to start running.
  * @timeout:	amount of time to allow this command to run.
@@ -98,8 +134,7 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
  *    has its own timer, and as it is added to the queue, we set up the
  *    timer.  When the command completes, we cancel the timer.
  **/
-void scsi_add_timer(struct scsi_cmnd *scmd, int timeout,
-		    void (*complete)(struct scsi_cmnd *))
+void scsi_add_timer(struct scsi_cmnd *scmd, int timeout)
 {
 
 	/*
@@ -112,7 +147,7 @@ void scsi_add_timer(struct scsi_cmnd *sc
 
 	scmd->eh_timeout.data = (unsigned long)scmd;
 	scmd->eh_timeout.expires = jiffies + timeout;
-	scmd->eh_timeout.function = (void (*)(unsigned long)) complete;
+	scmd->eh_timeout.function = (void (*)(unsigned long))scsi_times_out;
 
 	SCSI_LOG_ERROR_RECOVERY(5, printk("%s: scmd: %p, time:"
 					  " %d, (%p)\n", __FUNCTION__,
@@ -120,7 +155,6 @@ void scsi_add_timer(struct scsi_cmnd *sc
 
 	add_timer(&scmd->eh_timeout);
 }
-EXPORT_SYMBOL(scsi_add_timer);
 
 /**
  * scsi_delete_timer - Delete/cancel timer for a given function.
@@ -148,44 +182,6 @@ int scsi_delete_timer(struct scsi_cmnd *
 
 	return rtn;
 }
-EXPORT_SYMBOL(scsi_delete_timer);
-
-/**
- * scsi_times_out - Timeout function for normal scsi commands.
- * @scmd:	Cmd that is timing out.
- *
- * Notes:
- *     We do not need to lock this.  There is the potential for a race
- *     only in that the normal completion handling might run, but if the
- *     normal completion function determines that the timer has already
- *     fired, then it mustn't do anything.
- **/
-void scsi_times_out(struct scsi_cmnd *scmd)
-{
-	scsi_log_completion(scmd, TIMEOUT_ERROR);
-
-	if (scmd->device->host->hostt->eh_timed_out)
-		switch (scmd->device->host->hostt->eh_timed_out(scmd)) {
-		case EH_HANDLED:
-			__scsi_done(scmd);
-			return;
-		case EH_RESET_TIMER:
-			/* This allows a single retry even of a command
-			 * with allowed == 0 */
-			if (scmd->retries++ > scmd->allowed)
-				break;
-			scsi_add_timer(scmd, scmd->timeout_per_command,
-				       scsi_times_out);
-			return;
-		case EH_NOT_HANDLED:
-			break;
-		}
-
-	if (unlikely(!scsi_eh_scmd_add(scmd, SCSI_EH_CANCEL_CMD))) {
-		panic("Error handler thread not present at %p %p %s %d",
-		      scmd, scmd->device->host, __FILE__, __LINE__);
-	}
-}
 
 /**
  * scsi_block_when_processing_errors - Prevent cmds from being queued.
Index: scsi-reqfn-export/drivers/scsi/scsi_priv.h
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_priv.h	2005-04-11 03:42:11.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_priv.h	2005-04-11 03:42:12.000000000 +0900
@@ -84,7 +84,8 @@ extern int __init scsi_init_devinfo(void
 extern void scsi_exit_devinfo(void);
 
 /* scsi_error.c */
-extern void scsi_times_out(struct scsi_cmnd *cmd);
+extern void scsi_add_timer(struct scsi_cmnd *scmd, int timeout);
+extern int scsi_delete_timer(struct scsi_cmnd *scmd);
 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);
Index: scsi-reqfn-export/include/scsi/scsi_eh.h
===================================================================
--- scsi-reqfn-export.orig/include/scsi/scsi_eh.h	2005-04-11 03:42:10.000000000 +0900
+++ scsi-reqfn-export/include/scsi/scsi_eh.h	2005-04-11 03:42:12.000000000 +0900
@@ -27,9 +27,6 @@ struct scsi_sense_hdr {		/* See SPC-3 se
 };
 
 
-extern void scsi_add_timer(struct scsi_cmnd *, int,
-		void (*)(struct scsi_cmnd *));
-extern int scsi_delete_timer(struct scsi_cmnd *);
 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 *);

Re: [PATCH scsi-misc-2.6 06/07] scsi: Delete scsi_{add|delete}_timer() from scsi_mid_low_api.txt

[Tejun Heo]

06_scsi_timer_update_api_doc.patch

	As scsi_{add|delete}_timer() got unexported, remove them from
	the API doc.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 scsi_mid_low_api.txt |   41 -----------------------------------------
 1 files changed, 41 deletions(-)

Index: scsi-reqfn-export/Documentation/scsi/scsi_mid_low_api.txt
===================================================================
--- scsi-reqfn-export.orig/Documentation/scsi/scsi_mid_low_api.txt	2005-04-11 03:42:09.000000000 +0900
+++ scsi-reqfn-export/Documentation/scsi/scsi_mid_low_api.txt	2005-04-11 03:42:13.000000000 +0900
@@ -373,13 +373,11 @@ Summary:
    scsi_activate_tcq - turn on tag command queueing
    scsi_add_device - creates new scsi device (lu) instance
    scsi_add_host - perform sysfs registration and SCSI bus scan.
-   scsi_add_timer - (re-)start timer on a SCSI command.
    scsi_adjust_queue_depth - change the queue depth on a SCSI device
    scsi_assign_lock - replace default host_lock with given lock
    scsi_bios_ptable - return copy of block device's partition table
    scsi_block_requests - prevent further commands being queued to given host
    scsi_deactivate_tcq - turn off tag command queueing
-   scsi_delete_timer - cancel timer on a SCSI command.
    scsi_host_alloc - return a new scsi_host instance whose refcount==1
    scsi_host_get - increments Scsi_Host instance's refcount
    scsi_host_put - decrements Scsi_Host instance's refcount (free if 0)
@@ -461,27 +459,6 @@ int scsi_add_host(struct Scsi_Host *shos
 
 
 /**
- * scsi_add_timer - (re-)start timer on a SCSI command.
- * @scmd:    pointer to scsi command instance
- * @timeout: duration of timeout in "jiffies"
- * @complete: pointer to function to call if timeout expires
- *
- *      Returns nothing
- *
- *      Might block: no
- *
- *      Notes: Each scsi command has its own timer, and as it is added
- *      to the queue, we set up the timer. When the command completes, 
- *      we cancel the timer. An LLD can use this function to change
- *      the existing timeout value.
- *
- *      Defined in: drivers/scsi/scsi_error.c
- **/
-void scsi_add_timer(struct scsi_cmnd *scmd, int timeout, 
-                    void (*complete)(struct scsi_cmnd *))
-
-
-/**
  * scsi_adjust_queue_depth - allow LLD to change queue depth on a SCSI device
  * @sdev:       pointer to SCSI device to change queue depth on
  * @tagged:     0 - no tagged queuing
@@ -569,24 +546,6 @@ void scsi_deactivate_tcq(struct scsi_dev
 
 
 /**
- * scsi_delete_timer - cancel timer on a SCSI command.
- * @scmd:    pointer to scsi command instance
- *
- *      Returns 1 if able to cancel timer else 0 (i.e. too late or already
- *      cancelled).
- *
- *      Might block: no [may in the future if it invokes del_timer_sync()]
- *
- *      Notes: All commands issued by upper levels already have a timeout
- *      associated with them. An LLD can use this function to cancel the
- *      timer.
- *
- *      Defined in: drivers/scsi/scsi_error.c
- **/
-int scsi_delete_timer(struct scsi_cmnd *scmd)
-
-
-/**
  * scsi_host_alloc - create a scsi host adapter instance and perform basic
  *                   initialization.
  * @sht:        pointer to scsi host template

Re: [PATCH scsi-misc-2.6 07/07] scsi: make reuse of SCSI cmd timer strict

[Tejun Heo]

07_scsi_timer_strict_reuse.patch

	SCSI cmd timer shouldn't be reused while it's active.  Make
	sure that the unused condition is marked with
	eh_timeout->function = NULL and BUG() active reuse path.

Signed-off-by: Tejun Heo <htejun@gmail.com>

 scsi_error.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

Index: scsi-reqfn-export/drivers/scsi/scsi_error.c
===================================================================
--- scsi-reqfn-export.orig/drivers/scsi/scsi_error.c	2005-04-11 03:42:12.000000000 +0900
+++ scsi-reqfn-export/drivers/scsi/scsi_error.c	2005-04-11 03:42:13.000000000 +0900
@@ -99,6 +99,8 @@ int scsi_eh_scmd_add(struct scsi_cmnd *s
  **/
 static 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)
@@ -136,14 +138,7 @@ static void scsi_times_out(struct scsi_c
  **/
 void scsi_add_timer(struct scsi_cmnd *scmd, int timeout)
 {
-
-	/*
-	 * 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;
@@ -177,7 +172,6 @@ int scsi_delete_timer(struct scsi_cmnd *
 					 " rtn: %d\n", __FUNCTION__,
 					 scmd, rtn));
 
-	scmd->eh_timeout.data = (unsigned long)NULL;
 	scmd->eh_timeout.function = NULL;
 
 	return rtn;