[PATCH] SCSI core: fix leakage of scsi_cmnd's

[PATCH] SCSI core: fix leakage of scsi_cmnd's

[Alan Stern]

[This message is completely separate from the concurrent discussion of 
earlier patches, as542-as546; this patch fixes a different problem.]

James:

The SCSI core has a problem with leakage of scsi_cmnd structs.  It occurs
when a request is requeued; the request keeps its associated scsi_cmnd so
that the core doesn't need to assign a new one.  However, the routines
that read the device queue sometimes delete entries without bothering to
free the associated scsi_cmnd.  Among other things, this bug manifests as
error-handler processes remaining alive when a hot-pluggable device is
unplugged in the middle of executing a command.

This patch (as559) fixes the problem by calling scsi_put_command and 
scsi_release_buffers at the appropriate times.  It also reorganizes a 
couple of bits of code, adding a new subroutine to find the scsi_cmnd 
associated with a requeued request.

This addresses Bugzilla entry #5195.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Index: 2613mm1/drivers/scsi/scsi_lib.c
===================================================================
--- 2613mm1.orig/drivers/scsi/scsi_lib.c
+++ 2613mm1/drivers/scsi/scsi_lib.c
@@ -1103,11 +1103,30 @@ static void scsi_generic_done(struct scs
 	scsi_io_completion(cmd, cmd->result == 0 ? cmd->bufflen : 0, 0);
 }
 
+static struct scsi_cmnd *scsi_cmd_from_req(struct request *req)
+{
+	struct scsi_cmnd *cmd;
+
+	/*
+	 * If this request was requeued, it may already have an associated
+	 * scsi_cmnd.  Find out if this is so and return the cmd.
+	 */
+	cmd = req->special;
+	if ((req->flags & REQ_SPECIAL) && req->special) {
+		struct scsi_request *sreq = req->special;
+
+		if (sreq->sr_magic == SCSI_REQ_MAGIC)
+			cmd = NULL;
+	}
+	return cmd;
+}
+
 static int scsi_prep_fn(struct request_queue *q, struct request *req)
 {
 	struct scsi_device *sdev = q->queuedata;
-	struct scsi_cmnd *cmd;
+	struct scsi_cmnd *cmd = scsi_cmd_from_req(req);
 	int specials_only = 0;
+	struct scsi_request *sreq = NULL;
 
 	/*
 	 * Just check to see if the device is online.  If it isn't, we
@@ -1117,6 +1136,8 @@ static int scsi_prep_fn(struct request_q
 	if (unlikely(!scsi_device_online(sdev))) {
 		printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to offline device\n",
 		       sdev->host->host_no, sdev->id, sdev->lun);
+		if (cmd)
+			scsi_put_command(cmd);
 		return BLKPREP_KILL;
 	}
 	if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
@@ -1127,6 +1148,8 @@ static int scsi_prep_fn(struct request_q
 			 * at all allowed down */
 			printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to dead device\n",
 			       sdev->host->host_no, sdev->id, sdev->lun);
+			if (cmd)
+				scsi_put_command(cmd);
 			return BLKPREP_KILL;
 		}
 		/* OK, we only allow special commands (i.e. not
@@ -1142,53 +1165,54 @@ static int scsi_prep_fn(struct request_q
 	 * the remainder of a partially fulfilled request that can 
 	 * come up when there is a medium error.  We have to treat
 	 * these two cases differently.  We differentiate by looking
-	 * at request->cmd, as this tells us the real story.
+	 * at req->special, as this tells us the real story.
 	 */
-	if (req->flags & REQ_SPECIAL && req->special) {
-		struct scsi_request *sreq = req->special;
+	if ((req->flags & REQ_SPECIAL) && req->special) {
+		sreq = req->special;
+		if (sreq->sr_magic != SCSI_REQ_MAGIC)
+			sreq = NULL;
 
-		if (sreq->sr_magic == SCSI_REQ_MAGIC) {
-			cmd = scsi_get_command(sreq->sr_device, GFP_ATOMIC);
-			if (unlikely(!cmd))
-				goto defer;
-			scsi_init_cmd_from_req(cmd, sreq);
-		} else
-			cmd = req->special;
 	} else if (req->flags & (REQ_CMD | REQ_BLOCK_PC)) {
-
-		if(unlikely(specials_only) && !(req->flags & REQ_SPECIAL)) {
-			if(specials_only == SDEV_QUIESCE ||
+		if (unlikely(specials_only) && !(req->flags & REQ_SPECIAL)) {
+			if (specials_only == SDEV_QUIESCE ||
 					specials_only == SDEV_BLOCK)
 				return BLKPREP_DEFER;
 			
 			printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to device being removed\n",
 			       sdev->host->host_no, sdev->id, sdev->lun);
+			if (cmd)
+				scsi_put_command(cmd);
 			return BLKPREP_KILL;
 		}
-			
-			
-		/*
-		 * Now try and find a command block that we can use.
-		 */
-		if (!req->special) {
-			cmd = scsi_get_command(sdev, GFP_ATOMIC);
-			if (unlikely(!cmd))
-				goto defer;
-		} else
-			cmd = req->special;
-		
-		/* pull a tag out of the request if we have one */
-		cmd->tag = req->tag;
+
 	} else {
 		blk_dump_rq_flags(req, "SCSI bad req");
+		if (cmd)		/* Paranoia */
+			scsi_put_command(cmd);
 		return BLKPREP_KILL;
 	}
+			
+	/*
+	 * Now try and find a command block that we can use.
+	 */
+	if (!cmd) {
+		cmd = scsi_get_command(sdev, GFP_ATOMIC);
+		if (unlikely(!cmd))
+			goto defer;
+
+		if (sreq)
+			scsi_init_cmd_from_req(cmd, sreq);
+		else {
+			/* pull a tag out of the request if we have one */
+			cmd->tag = req->tag;
+		}
 	
-	/* note the overloading of req->special.  When the tag
-	 * is active it always means cmd.  If the tag goes
-	 * back for re-queueing, it may be reset */
-	req->special = cmd;
-	cmd->request = req;
+		/* note the overloading of req->special.  When the tag
+		 * is active it always means cmd.  If the tag goes
+		 * back for re-queueing, it may be reset */
+		req->special = cmd;
+		cmd->request = req;
+	}
 	
 	/*
 	 * FIXME: drop the lock here because the functions below
@@ -1337,16 +1361,21 @@ static inline int scsi_host_queue_ready(
 /*
  * Kill requests for a dead device
  */
-static void scsi_kill_requests(request_queue_t *q)
+static void scsi_kill_request(struct request *req, struct request_queue *q)
 {
-	struct request *req;
+	struct scsi_cmnd *cmd = scsi_cmd_from_req(req);
+
+	blkdev_dequeue_request(req);
+	req->flags |= REQ_QUIET;
+	while (end_that_request_first(req, 0, req->nr_sectors))
+		;
+	end_that_request_last(req);
 
-	while ((req = elv_next_request(q)) != NULL) {
-		blkdev_dequeue_request(req);
-		req->flags |= REQ_QUIET;
-		while (end_that_request_first(req, 0, req->nr_sectors))
-			;
-		end_that_request_last(req);
+	if (cmd) {
+		spin_unlock(q->queue_lock);
+		scsi_release_buffers(cmd);
+		scsi_put_command(cmd);
+		spin_lock(q->queue_lock);
 	}
 }
 
@@ -1370,7 +1399,8 @@ static void scsi_request_fn(struct reque
 
 	if (!sdev) {
 		printk("scsi: killing requests for dead queue\n");
-		scsi_kill_requests(q);
+		while ((req = elv_next_request(q)) != NULL)
+			scsi_kill_request(req, q);
 		return;
 	}
 
@@ -1397,11 +1427,7 @@ static void scsi_request_fn(struct reque
 		if (unlikely(!scsi_device_online(sdev))) {
 			printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to offline device\n",
 			       sdev->host->host_no, sdev->id, sdev->lun);
-			blkdev_dequeue_request(req);
-			req->flags |= REQ_QUIET;
-			while (end_that_request_first(req, 0, req->nr_sectors))
-				;
-			end_that_request_last(req);
+			scsi_kill_request(req, q);
 			continue;
 		}
 

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[James Bottomley]

> The SCSI core has a problem with leakage of scsi_cmnd structs.  It occurs
> when a request is requeued; the request keeps its associated scsi_cmnd so
> that the core doesn't need to assign a new one.  However, the routines
> that read the device queue sometimes delete entries without bothering to
> free the associated scsi_cmnd.  Among other things, this bug manifests as
> error-handler processes remaining alive when a hot-pluggable device is
> unplugged in the middle of executing a command.
> 
> This patch (as559) fixes the problem by calling scsi_put_command and 
> scsi_release_buffers at the appropriate times.  It also reorganizes a 
> couple of bits of code, adding a new subroutine to find the scsi_cmnd 
> associated with a requeued request.

Nate Dailey also had a patch for this, a reply to which is incomplete
still in my drafts folder.

The bottom line is that I don't think any modifications to the prep_fn()
are necessary.  It's guarded by REQ_DONTPREP, so is never called again
if we actually manage to prepare a command fully.  The returns from it
are:

BLKPREP_DEFER:  Requeue the command for re-preparation (no resources
should be allocated)

BLKPREP_KILL: destroy the command (no resources should be allocated)

BLKPREP_OK: Command is prepared, resources are allocated, REQ_DONTPREP
is set to prevent any additional prep_fn() call.

So in the DEFER or KILL case, all you need to worry about is resources
you may have allocated in the current prep_fn() invocation.  It seems to
me that we do release these correctly, unless I'm missing something?

The second problem is a bug (also spotted by Nate).  However, what I
think we should be doing in this case is calling __scsi_done with
DID_NO_CONNECT which should clean up correctly and also send the error
indications back up the stack to the correct sources (that's what we do
in scsi_dispatch_cmd() for this problem).

James

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[Alan Stern]

On Thu, 8 Sep 2005, James Bottomley wrote:

> > The SCSI core has a problem with leakage of scsi_cmnd structs.  It occurs
> > when a request is requeued; the request keeps its associated scsi_cmnd so
> > that the core doesn't need to assign a new one.  However, the routines
> > that read the device queue sometimes delete entries without bothering to
> > free the associated scsi_cmnd.  Among other things, this bug manifests as
> > error-handler processes remaining alive when a hot-pluggable device is
> > unplugged in the middle of executing a command.
> > 
> > This patch (as559) fixes the problem by calling scsi_put_command and 
> > scsi_release_buffers at the appropriate times.  It also reorganizes a 
> > couple of bits of code, adding a new subroutine to find the scsi_cmnd 
> > associated with a requeued request.
> 
> Nate Dailey also had a patch for this, a reply to which is incomplete
> still in my drafts folder.

CC'ed to Nate.  Thanks for pointing out his patch.  Mine does much the 
same thing as his, with a few additions: call scsi_release_buffers when 
deleting a request from scsi_request_fn, and centralize the code to 
find the scsi_cmnd instead of repeating it several times.

> The bottom line is that I don't think any modifications to the prep_fn()
> are necessary.  It's guarded by REQ_DONTPREP, so is never called again
> if we actually manage to prepare a command fully.  The returns from it
> are:
> 
> BLKPREP_DEFER:  Requeue the command for re-preparation (no resources
> should be allocated)
> 
> BLKPREP_KILL: destroy the command (no resources should be allocated)
> 
> BLKPREP_OK: Command is prepared, resources are allocated, REQ_DONTPREP
> is set to prevent any additional prep_fn() call.
> 
> So in the DEFER or KILL case, all you need to worry about is resources
> you may have allocated in the current prep_fn() invocation.  It seems to
> me that we do release these correctly, unless I'm missing something?

You _are_ missing something: scsi_requeue_command turns off the
REQ_DONTPREP flag before calling blk_requeue_request.  So requeued
requests do indeed get re-prepped.  This is needed for allocating
scatter-gather lists and so on; the original ones get deallocated the
first time the command finishes, so new ones are needed when the command
is requeued.

> The second problem is a bug (also spotted by Nate).  However, what I
> think we should be doing in this case is calling __scsi_done with
> DID_NO_CONNECT which should clean up correctly and also send the error
> indications back up the stack to the correct sources (that's what we do
> in scsi_dispatch_cmd() for this problem).

Is there some reason for not doing this same sort of thing from within
scsi_prep_fn when rejecting a request?  Obviously most of the cleanup
won't be needed, but you still want to send the error indications back to
the correct sources.  It seems that prep_fn and request_fn should be
consistent in the way they handle problems.

Alan Stern

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[James Bottomley]

On Thu, 2005-09-08 at 12:44 -0400, Alan Stern wrote:
> You _are_ missing something: scsi_requeue_command turns off the
> REQ_DONTPREP flag before calling blk_requeue_request.  So requeued
> requests do indeed get re-prepped.  This is needed for allocating
> scatter-gather lists and so on; the original ones get deallocated the
> first time the command finishes, so new ones are needed when the command
> is requeued.

Sigh, I might have guessed we'd have a leak somewhere ...

Is not a better way to fix this actually to have the
scsi_requeue_command put the command and reset req->special to NULL?
That way the command gets reprepared as well and the prep fn assumptions
should be fully valid.  This would also correct what looks like a bug in
the command transformation routines (The ones that convert ten byte
commands to six byte ones if we get an illegal request response) ...
they look to be assuming that scsi_requeue_command() will actually
reissue the correct command instead of simply reusing the existing one.

> > The second problem is a bug (also spotted by Nate).  However, what I
> > think we should be doing in this case is calling __scsi_done with
> > DID_NO_CONNECT which should clean up correctly and also send the error
> > indications back up the stack to the correct sources (that's what we do
> > in scsi_dispatch_cmd() for this problem).
> 
> Is there some reason for not doing this same sort of thing from within
> scsi_prep_fn when rejecting a request?  Obviously most of the cleanup
> won't be needed, but you still want to send the error indications back to
> the correct sources.  It seems that prep_fn and request_fn should be
> consistent in the way they handle problems.

No real reason except that in the prep_fn() you often don't have a
command to call __scsi_done() on.  Both methods are fully valid and
(with the all commands go via bios merger) supply the indications at all
the correct stack levels.

James

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[Alan Stern]

On Thu, 8 Sep 2005, James Bottomley wrote:

> Sigh, I might have guessed we'd have a leak somewhere ...
> 
> Is not a better way to fix this actually to have the
> scsi_requeue_command put the command and reset req->special to NULL?
> That way the command gets reprepared as well and the prep fn assumptions
> should be fully valid.  This would also correct what looks like a bug in
> the command transformation routines (The ones that convert ten byte
> commands to six byte ones if we get an illegal request response) ...
> they look to be assuming that scsi_requeue_command() will actually
> reissue the correct command instead of simply reusing the existing one.

This patch (as559b) does what you suggest.  It adds a new routine,
scsi_unprep_request, which gets called every place a request is requeued.  
(That includes scsi_queue_insert as well as scsi_requeue_command.)  It
also changes scsi_kill_requests to make it call __scsi_done with result
equal to DID_NO_CONNECT << 16.  (I'm not sure if it's necessary to call
scsi_init_cmd_errh here; maybe you can check on that.)  Finally, the
patch changes the return value from scsi_end_request, to avoid returning a
stale pointer in the case where the request was requeued.  Fortunately the
return value is used in only place, and the change actually simplified it.

This hasn't been tested very thoroughly, so please look through it 
carefully.

Alan Stern



Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Index: 2613mm1/drivers/scsi/scsi_lib.c
===================================================================
--- 2613mm1.orig/drivers/scsi/scsi_lib.c
+++ 2613mm1/drivers/scsi/scsi_lib.c
@@ -97,6 +97,30 @@ int scsi_insert_special_req(struct scsi_
 }
 
 static void scsi_run_queue(struct request_queue *q);
+static void scsi_release_buffers(struct scsi_cmnd *cmd);
+
+/*
+ * Function:	scsi_unprep_request()
+ *
+ * Purpose:	Remove all preparation done for a request, including its
+ *		associated scsi_cmnd, so that it can be requeued.
+ *
+ * Arguments:	req	- request to unprepare
+ *
+ * Lock status:	Assumed that no locks are held upon entry.
+ *
+ * Returns:	Nothing.
+ */
+static void scsi_unprep_request(struct request *req)
+{
+	struct scsi_cmnd *cmd = req->special;
+
+	req->flags &= ~REQ_DONTPREP;
+	req->special = (req->flags & REQ_SPECIAL) ? cmd->sc_request : NULL;
+
+	scsi_release_buffers(cmd);
+	scsi_put_command(cmd);
+}
 
 /*
  * Function:    scsi_queue_insert()
@@ -116,12 +140,14 @@ static void scsi_run_queue(struct reques
  *              commands.
  * Notes:       This could be called either from an interrupt context or a
  *              normal process context.
+ * Notes:	Upon return, cmd is a stale pointer.
  */
 int scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 {
 	struct Scsi_Host *host = cmd->device->host;
 	struct scsi_device *device = cmd->device;
 	struct request_queue *q = device->request_queue;
+	struct request *req = cmd->request;
 	unsigned long flags;
 
 	SCSI_LOG_MLQUEUE(1,
@@ -162,8 +188,9 @@ int scsi_queue_insert(struct scsi_cmnd *
 	 * function.  The SCSI request function detects the blocked condition
 	 * and plugs the queue appropriately.
          */
+	scsi_unprep_request(req);
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	blk_requeue_request(q, req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
 	scsi_run_queue(q);
@@ -552,15 +579,16 @@ static void scsi_run_queue(struct reques
  *		I/O errors in the middle of the request, in which case
  *		we need to request the blocks that come after the bad
  *		sector.
+ * Notes:	Upon return, cmd is a stale pointer.
  */
 static void scsi_requeue_command(struct request_queue *q, struct scsi_cmnd *cmd)
 {
+	struct request *req = cmd->request;
 	unsigned long flags;
 
-	cmd->request->flags &= ~REQ_DONTPREP;
-
+	scsi_unprep_request(req);
 	spin_lock_irqsave(q->queue_lock, flags);
-	blk_requeue_request(q, cmd->request);
+	blk_requeue_request(q, req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 
 	scsi_run_queue(q);
@@ -595,13 +623,14 @@ void scsi_run_host_queues(struct Scsi_Ho
  *
  * Lock status: Assumed that lock is not held upon entry.
  *
- * Returns:     cmd if requeue done or required, NULL otherwise
+ * Returns:     cmd if requeue required, NULL otherwise.
  *
  * Notes:       This is called for block device requests in order to
  *              mark some number of sectors as complete.
  * 
  *		We are guaranteeing that the request queue will be goosed
  *		at some point during this call.
+ * Notes:	If cmd was requeued, upon return it will be a stale pointer.
  */
 static struct scsi_cmnd *scsi_end_request(struct scsi_cmnd *cmd, int uptodate,
 					  int bytes, int requeue)
@@ -624,14 +653,15 @@ static struct scsi_cmnd *scsi_end_reques
 		if (!uptodate && blk_noretry_request(req))
 			end_that_request_chunk(req, 0, leftover);
 		else {
-			if (requeue)
+			if (requeue) {
 				/*
 				 * Bleah.  Leftovers again.  Stick the
 				 * leftovers in the front of the
 				 * queue, and goose the queue again.
 				 */
 				scsi_requeue_command(q, cmd);
-
+				cmd = NULL;
+			}
 			return cmd;
 		}
 	}
@@ -857,15 +887,13 @@ void scsi_io_completion(struct scsi_cmnd
 		 * requeueing right here - we will requeue down below
 		 * when we handle the bad sectors.
 		 */
-		cmd = scsi_end_request(cmd, 1, good_bytes, result == 0);
 
 		/*
-		 * If the command completed without error, then either finish off the
-		 * rest of the command, or start a new one.
+		 * If the command completed without error, then either
+		 * finish off the rest of the command, or start a new one.
 		 */
-		if (result == 0 || cmd == NULL ) {
+		if (scsi_end_request(cmd, 1, good_bytes, result == 0) == NULL)
 			return;
-		}
 	}
 	/*
 	 * Now, if we were good little boys and girls, Santa left us a request
@@ -880,7 +908,7 @@ void scsi_io_completion(struct scsi_cmnd
 				 * and quietly refuse further access.
 				 */
 				cmd->device->changed = 1;
-				cmd = scsi_end_request(cmd, 0,
+				scsi_end_request(cmd, 0,
 						this_count, 1);
 				return;
 			} else {
@@ -914,7 +942,7 @@ void scsi_io_completion(struct scsi_cmnd
 				scsi_requeue_command(q, cmd);
 				result = 0;
 			} else {
-				cmd = scsi_end_request(cmd, 0, this_count, 1);
+				scsi_end_request(cmd, 0, this_count, 1);
 				return;
 			}
 			break;
@@ -929,7 +957,7 @@ void scsi_io_completion(struct scsi_cmnd
 			}
 			printk(KERN_INFO "Device %s not ready.\n",
 			       req->rq_disk ? req->rq_disk->disk_name : "");
-			cmd = scsi_end_request(cmd, 0, this_count, 1);
+			scsi_end_request(cmd, 0, this_count, 1);
 			return;
 		case VOLUME_OVERFLOW:
 			printk(KERN_INFO "Volume overflow <%d %d %d %d> CDB: ",
@@ -938,7 +966,7 @@ void scsi_io_completion(struct scsi_cmnd
 			       (int)cmd->device->id, (int)cmd->device->lun);
 			__scsi_print_command(cmd->data_cmnd);
 			scsi_print_sense("", cmd);
-			cmd = scsi_end_request(cmd, 0, block_bytes, 1);
+			scsi_end_request(cmd, 0, block_bytes, 1);
 			return;
 		default:
 			break;
@@ -971,7 +999,7 @@ void scsi_io_completion(struct scsi_cmnd
 		block_bytes = req->hard_cur_sectors << 9;
 		if (!block_bytes)
 			block_bytes = req->data_len;
-		cmd = scsi_end_request(cmd, 0, block_bytes, 1);
+		scsi_end_request(cmd, 0, block_bytes, 1);
 	}
 }
 EXPORT_SYMBOL(scsi_io_completion);
@@ -1335,19 +1363,24 @@ static inline int scsi_host_queue_ready(
 }
 
 /*
- * Kill requests for a dead device
+ * Kill a request for a dead device
  */
-static void scsi_kill_requests(request_queue_t *q)
+static void scsi_kill_request(struct request *req, request_queue_t *q)
 {
-	struct request *req;
+	struct scsi_cmnd *cmd = req->special;
 
-	while ((req = elv_next_request(q)) != NULL) {
-		blkdev_dequeue_request(req);
-		req->flags |= REQ_QUIET;
-		while (end_that_request_first(req, 0, req->nr_sectors))
-			;
-		end_that_request_last(req);
-	}
+	spin_unlock(q->queue_lock);
+	if (unlikely(cmd == NULL)) {
+		printk(KERN_CRIT "impossible request in %s.\n",
+				 __FUNCTION__);
+		BUG();
+	}
+
+	scsi_init_cmd_errh(cmd);
+	cmd->result = DID_NO_CONNECT << 16;
+	atomic_inc(&cmd->device->iorequest_cnt);
+	__scsi_done(cmd);
+	spin_lock(q->queue_lock);
 }
 
 /*
@@ -1370,7 +1403,8 @@ static void scsi_request_fn(struct reque
 
 	if (!sdev) {
 		printk("scsi: killing requests for dead queue\n");
-		scsi_kill_requests(q);
+		while ((req = elv_next_request(q)) != NULL)
+			scsi_kill_request(req, q);
 		return;
 	}
 
@@ -1398,10 +1432,7 @@ static void scsi_request_fn(struct reque
 			printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to offline device\n",
 			       sdev->host->host_no, sdev->id, sdev->lun);
 			blkdev_dequeue_request(req);
-			req->flags |= REQ_QUIET;
-			while (end_that_request_first(req, 0, req->nr_sectors))
-				;
-			end_that_request_last(req);
+			scsi_kill_request(req, q);
 			continue;
 		}
 
@@ -1414,6 +1445,14 @@ static void scsi_request_fn(struct reque
 		sdev->device_busy++;
 
 		spin_unlock(q->queue_lock);
+		cmd = req->special;
+		if (unlikely(cmd == NULL)) {
+			printk(KERN_CRIT "impossible request in %s.\n"
+					 "please mail a stack trace to "
+					 "linux-scsi@vger.kernel.org",
+					 __FUNCTION__);
+			BUG();
+		}
 		spin_lock(shost->host_lock);
 
 		if (!scsi_host_queue_ready(q, shost, sdev))
@@ -1432,15 +1471,6 @@ static void scsi_request_fn(struct reque
 		 */
 		spin_unlock_irq(shost->host_lock);
 
-		cmd = req->special;
-		if (unlikely(cmd == NULL)) {
-			printk(KERN_CRIT "impossible request in %s.\n"
-					 "please mail a stack trace to "
-					 "linux-scsi@vger.kernel.org",
-					 __FUNCTION__);
-			BUG();
-		}
-
 		/*
 		 * Finally, initialize any error handling parameters, and set up
 		 * the timers for timeouts.
@@ -1476,6 +1506,7 @@ static void scsi_request_fn(struct reque
 	 * cases (host limits or settings) should run the queue at some
 	 * later time.
 	 */
+	scsi_unprep_request(req);
 	spin_lock_irq(q->queue_lock);
 	blk_requeue_request(q, req);
 	sdev->device_busy--;

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[James Bottomley]

On Thu, 2005-09-08 at 16:49 -0400, Alan Stern wrote:
> This hasn't been tested very thoroughly, so please look through it 
> carefully.

Actually, just one problem and one cosmetic fix:

1) We need to dequeue for the loop and kill case (it seems easiest
simply to dequeue in the scsi_kill_request() routine)
2) There's no real need to drop the queue lock.  __scsi_done() is lock
agnostic, so since there's no requirement, let's just leave it in to
avoid any locking issues.

Thanks,

James

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1370,7 +1370,8 @@ static void scsi_kill_request(struct req
 {
 	struct scsi_cmnd *cmd = req->special;
 
-	spin_unlock(q->queue_lock);
+	blkdev_dequeue_request(req);
+
 	if (unlikely(cmd == NULL)) {
 		printk(KERN_CRIT "impossible request in %s.\n",
 				 __FUNCTION__);
@@ -1381,7 +1382,6 @@ static void scsi_kill_request(struct req
 	cmd->result = DID_NO_CONNECT << 16;
 	atomic_inc(&cmd->device->iorequest_cnt);
 	__scsi_done(cmd);
-	spin_lock(q->queue_lock);
 }
 
 /*
@@ -1432,7 +1432,6 @@ static void scsi_request_fn(struct reque
 		if (unlikely(!scsi_device_online(sdev))) {
 			printk(KERN_ERR "scsi%d (%d:%d): rejecting I/O to offline device\n",
 			       sdev->host->host_no, sdev->id, sdev->lun);
-			blkdev_dequeue_request(req);
 			scsi_kill_request(req, q);
 			continue;
 		}

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[Alan Stern]

On Fri, 9 Sep 2005, James Bottomley wrote:

> On Thu, 2005-09-08 at 16:49 -0400, Alan Stern wrote:
> > This hasn't been tested very thoroughly, so please look through it 
> > carefully.
> 
> Actually, just one problem and one cosmetic fix:
> 
> 1) We need to dequeue for the loop and kill case (it seems easiest
> simply to dequeue in the scsi_kill_request() routine)
> 2) There's no real need to drop the queue lock.  __scsi_done() is lock
> agnostic, so since there's no requirement, let's just leave it in to
> avoid any locking issues.

Have you applied this to any publicly-accessible tree yet?  I looked at 
the git browsers on parisc-linux.org and kernel.org, but the patch doesn't 
seem to be there yet.

For that matter, what about the other patches you agreed to take (as543 - 
as546)?  They aren't in the git repositories either.  It would be good if 
all these things could be ready in time for the next -mm release.

Alan Stern

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[James Bottomley]

On Tue, 2005-09-13 at 13:00 -0400, Alan Stern wrote:
> On Fri, 9 Sep 2005, James Bottomley wrote:
> 
> > On Thu, 2005-09-08 at 16:49 -0400, Alan Stern wrote:
> > > This hasn't been tested very thoroughly, so please look through it 
> > > carefully.
> > 
> > Actually, just one problem and one cosmetic fix:
> > 
> > 1) We need to dequeue for the loop and kill case (it seems easiest
> > simply to dequeue in the scsi_kill_request() routine)
> > 2) There's no real need to drop the queue lock.  __scsi_done() is lock
> > agnostic, so since there's no requirement, let's just leave it in to
> > avoid any locking issues.
> 
> Have you applied this to any publicly-accessible tree yet?  I looked at 
> the git browsers on parisc-linux.org and kernel.org, but the patch doesn't 
> seem to be there yet.

Yes, they should both be in 2.6.14-rc1

> For that matter, what about the other patches you agreed to take (as543 - 
> as546)?  They aren't in the git repositories either.  It would be good if 
> all these things could be ready in time for the next -mm release.

That I think was these [from the latest GIT PATCH email]:

> Alan Stern:
>   o Fix module removal/device add race
>   o fix callers of scsi_remove_device() who already hold the scan
> muted
>   o add missing scan mutex to scsi_scan_target()

James

Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's

[Alan Stern]

On Tue, 13 Sep 2005, James Bottomley wrote:

> > Have you applied this to any publicly-accessible tree yet?  I looked at 
> > the git browsers on parisc-linux.org and kernel.org, but the patch doesn't 
> > seem to be there yet.
> 
> Yes, they should both be in 2.6.14-rc1

And indeed they are.

> > For that matter, what about the other patches you agreed to take (as543 - 
> > as546)?  They aren't in the git repositories either.  It would be good if 
> > all these things could be ready in time for the next -mm release.
> 
> That I think was these [from the latest GIT PATCH email]:
> 
> > Alan Stern:
> >   o Fix module removal/device add race
> >   o fix callers of scsi_remove_device() who already hold the scan
> > muted
> >   o add missing scan mutex to scsi_scan_target()

Yes, I see those in 2.6.14-rc1 as well.  However there's still one patch 
missing (as545):

http://marc.theaimsgroup.com/?l=linux-scsi&m=112238804301664&w=2

That one fixes a real problem; it shows up whenever you remove a USB 
storage device.


An implicit point of my previous message was that I don't have a good way
to know when you have accepted one of my patch submissions.  Is there one
particular git tree I should look at, where an accepted patch will first
show up?  Or could you arrange to borrow Greg KH's script that sends out a
form email message whenever he applies a new patch to one of his main
development trees?

Alan Stern