[PATCH] 2.5.x add back missing scsi_queue_next_request calls

[PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Patrick Mansfield]

The change to use a pool for scsi_cmnd allocations removed some
scsi_queue_next_request calls, this patch restores the calls, and
exports scsi_put_command and scsi_get_command.

The extra scsi_queue_next_request calls are needed to handle non-block
device IO completion (char devices and scsi scanning).

This patch applies cleanly on top of the previous "starved" patch, but
should apply with offsets to the current 2.5.x tree.

diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi.c put_cmd-25/drivers/scsi/scsi.c
--- starve-25/drivers/scsi/scsi.c	Wed Mar 19 18:08:46 2003
+++ put_cmd-25/drivers/scsi/scsi.c	Wed Mar 19 18:30:47 2003
@@ -307,7 +307,7 @@ struct scsi_cmnd *scsi_get_command(struc
 }				
 
 /*
- * Function:	scsi_put_command()
+ * Function:	__scsi_put_command()
  *
  * Purpose:	Free a scsi command block
  *
@@ -317,7 +317,7 @@ struct scsi_cmnd *scsi_get_command(struc
  *
  * Notes:	The command must not belong to any lists.
  */
-void scsi_put_command(struct scsi_cmnd *cmd)
+void __scsi_put_command(struct scsi_cmnd *cmd)
 {
 	struct Scsi_Host *shost = cmd->device->host;
 	unsigned long flags;
@@ -340,6 +340,23 @@ void scsi_put_command(struct scsi_cmnd *
 }
 
 /*
+ * Function:	scsi_put_command()
+ *
+ * Purpose:	Free a scsi command block and call scsi_q
+ *
+ * Arguments:	cmd	- command block to free
+ *
+ * Returns:	Nothing.
+ */
+void scsi_put_command(struct scsi_cmnd *cmd)
+{
+	struct request_queue *q = cmd->device->request_queue;
+
+	__scsi_put_command(cmd);
+	scsi_queue_next_request(q, NULL);
+}
+
+/*
  * Function:	scsi_setup_command_freelist()
  *
  * Purpose:	Setup the command freelist for a scsi host.
diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi.h put_cmd-25/drivers/scsi/scsi.h
--- starve-25/drivers/scsi/scsi.h	Wed Mar 19 16:36:40 2003
+++ put_cmd-25/drivers/scsi/scsi.h	Wed Mar 19 18:10:54 2003
@@ -416,6 +416,7 @@ extern int scsi_maybe_unblock_host(Scsi_
 extern void scsi_setup_cmd_retry(Scsi_Cmnd *SCpnt);
 extern void scsi_io_completion(Scsi_Cmnd * SCpnt, int good_sectors,
 			       int block_sectors);
+extern void scsi_queue_next_request(request_queue_t *q, Scsi_Cmnd *cmd);
 extern int scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
 extern request_queue_t *scsi_alloc_queue(struct Scsi_Host *shost);
 extern void scsi_free_queue(request_queue_t *q);
@@ -429,6 +430,7 @@ extern int scsi_dispatch_cmd(Scsi_Cmnd *
 extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
 extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int flags);
+extern void __scsi_put_command(struct scsi_cmnd *cmd);
 extern void scsi_put_command(struct scsi_cmnd *cmd);
 extern void scsi_adjust_queue_depth(Scsi_Device *, int, int);
 extern int scsi_track_queue_full(Scsi_Device *, int);
diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi_lib.c put_cmd-25/drivers/scsi/scsi_lib.c
--- starve-25/drivers/scsi/scsi_lib.c	Wed Mar 19 16:36:40 2003
+++ put_cmd-25/drivers/scsi/scsi_lib.c	Wed Mar 19 18:10:54 2003
@@ -351,7 +351,7 @@ void scsi_setup_cmd_retry(struct scsi_cm
  *		permutations grows as 2**N, and if too many more special cases
  *		get added, we start to get screwed.
  */
-static void scsi_queue_next_request(request_queue_t *q, struct scsi_cmnd *cmd)
+void scsi_queue_next_request(request_queue_t *q, struct scsi_cmnd *cmd)
 {
 	struct scsi_device *sdev, *sdev2;
 	struct Scsi_Host *shost;
@@ -487,7 +487,6 @@ static struct scsi_cmnd *scsi_end_reques
 	 * need to worry about launching another command.
 	 */
 	scsi_put_command(cmd);
-	scsi_queue_next_request(q, NULL);
 	return NULL;
 }
 
@@ -906,7 +905,7 @@ static int scsi_init_io(struct scsi_cmnd
 			req->current_nr_sectors);
 
 	/* release the command and kill it */
-	scsi_put_command(cmd);
+	__scsi_put_command(cmd);
 	return BLKPREP_KILL;
 }
 
@@ -1014,7 +1013,7 @@ static int scsi_prep_fn(struct request_q
 		 */
 		if (unlikely(!sdt->init_command(cmd))) {
 			scsi_release_buffers(cmd);
-			scsi_put_command(cmd);
+			__scsi_put_command(cmd);
 			return BLKPREP_KILL;
 		}
 	}
diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi_syms.c put_cmd-25/drivers/scsi/scsi_syms.c
--- starve-25/drivers/scsi/scsi_syms.c	Wed Mar 19 11:52:25 2003
+++ put_cmd-25/drivers/scsi/scsi_syms.c	Wed Mar 19 18:10:54 2003
@@ -60,6 +60,8 @@ EXPORT_SYMBOL(scsi_allocate_request);
 EXPORT_SYMBOL(scsi_release_request);
 EXPORT_SYMBOL(scsi_wait_req);
 EXPORT_SYMBOL(scsi_do_req);
+EXPORT_SYMBOL(scsi_get_command);
+EXPORT_SYMBOL(scsi_put_command);
 
 EXPORT_SYMBOL(scsi_report_bus_reset);
 EXPORT_SYMBOL(scsi_block_requests);

-- Patrick Mansfield

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Luben Tuikov]

Patrick Mansfield wrote:
> The change to use a pool for scsi_cmnd allocations removed some
> scsi_queue_next_request calls, this patch restores the calls, and
> exports scsi_put_command and scsi_get_command.

And there was a point to this removal.  I did mention it on linux-scsi.

Look, don't try to make the code look as it did *before* -- there's
always a point to a change -- I think I mentioned exactly this
before on linux-scsi...

So *ALL* this patch does is REMOVE ONE CALL TO
scsi_queue_next_request(q, NULL).

And in order to accomodate this removal, you contaminate the slab
allocation implementation by introducing ONE MORE needless
__scsi_put_command(), then rename from scsi_put_command() to
__scsi_put_command() and contaminate scsi_put_command()... I think not!

> The extra scsi_queue_next_request calls are needed to handle non-block
> device IO completion (char devices and scsi scanning).

Ok, but this ``handling'' should be accommodated for *elsewhere* in
SCSI Core or in the _respective_ ULDD!  This is important for
modularizaion.

scsi_get_command(), __scsi_get_command() and scsi_put_command()
have EXACTLY defined behaviour.  They are memory management for
scsi command structs -- they have *NOTHING* to do with running/
unplugging of block/request queues and/or getting more requests!

Please, respect the implementation (modularization)!

Moreover, *THAT* was the whole point of the slab allocation patch for
scsi structs -- to modularize the allocation/deallocation.

And by introducing this patch here, we get a spaghetti dish, literally.
Please do not contaminate the command struct slab allocation.

And do not forget that LLDD and ULDD are free to call scsi_get/put_command()
at will.

Here is what you do: centralize the calls to scsi_get/put_command()
*in* SCSI Core and *after* the call to scsi_put_command(), you can do
a few tests to see if you can *indeed*(1) enqueue the next request.
But the slab implementaion should not be contaminated.

(1) You may not be able to... hot plugging, queueing limits, etc...

And please do not introduce __scsi_put_command() to do the dirty
work which should be handled elsewhere.

It was alright to call scsi_queue_next_request(q, NULL) from
scsi_end_request() -- at least logically!!!  But what does
scsi_put_command() have to do with scsi_queue_next_request()???

> This patch applies cleanly on top of the previous "starved" patch, but
> should apply with offsets to the current 2.5.x tree.
> 
> diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi.c put_cmd-25/drivers/scsi/scsi.c
> --- starve-25/drivers/scsi/scsi.c	Wed Mar 19 18:08:46 2003
> +++ put_cmd-25/drivers/scsi/scsi.c	Wed Mar 19 18:30:47 2003
> @@ -307,7 +307,7 @@ struct scsi_cmnd *scsi_get_command(struc
>  }				
>  
>  /*
> - * Function:	scsi_put_command()
> + * Function:	__scsi_put_command()
>   *
>   * Purpose:	Free a scsi command block
>   *
> @@ -317,7 +317,7 @@ struct scsi_cmnd *scsi_get_command(struc
>   *
>   * Notes:	The command must not belong to any lists.
>   */
> -void scsi_put_command(struct scsi_cmnd *cmd)
> +void __scsi_put_command(struct scsi_cmnd *cmd)
>  {
>  	struct Scsi_Host *shost = cmd->device->host;
>  	unsigned long flags;
> @@ -340,6 +340,23 @@ void scsi_put_command(struct scsi_cmnd *
>  }
>  
>  /*
> + * Function:	scsi_put_command()
> + *
> + * Purpose:	Free a scsi command block and call scsi_q
> + *
> + * Arguments:	cmd	- command block to free
> + *
> + * Returns:	Nothing.
> + */
> +void scsi_put_command(struct scsi_cmnd *cmd)
> +{
> +	struct request_queue *q = cmd->device->request_queue;
> +
> +	__scsi_put_command(cmd);
> +	scsi_queue_next_request(q, NULL);
> +}
> +
> +/*
>   * Function:	scsi_setup_command_freelist()
>   *
>   * Purpose:	Setup the command freelist for a scsi host.
> diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi.h put_cmd-25/drivers/scsi/scsi.h
> --- starve-25/drivers/scsi/scsi.h	Wed Mar 19 16:36:40 2003
> +++ put_cmd-25/drivers/scsi/scsi.h	Wed Mar 19 18:10:54 2003
> @@ -416,6 +416,7 @@ extern int scsi_maybe_unblock_host(Scsi_
>  extern void scsi_setup_cmd_retry(Scsi_Cmnd *SCpnt);
>  extern void scsi_io_completion(Scsi_Cmnd * SCpnt, int good_sectors,
>  			       int block_sectors);
> +extern void scsi_queue_next_request(request_queue_t *q, Scsi_Cmnd *cmd);
>  extern int scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
>  extern request_queue_t *scsi_alloc_queue(struct Scsi_Host *shost);
>  extern void scsi_free_queue(request_queue_t *q);
> @@ -429,6 +430,7 @@ extern int scsi_dispatch_cmd(Scsi_Cmnd *
>  extern int scsi_setup_command_freelist(struct Scsi_Host *shost);
>  extern void scsi_destroy_command_freelist(struct Scsi_Host *shost);
>  extern struct scsi_cmnd *scsi_get_command(struct scsi_device *dev, int flags);
> +extern void __scsi_put_command(struct scsi_cmnd *cmd);
>  extern void scsi_put_command(struct scsi_cmnd *cmd);
>  extern void scsi_adjust_queue_depth(Scsi_Device *, int, int);
>  extern int scsi_track_queue_full(Scsi_Device *, int);
> diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi_lib.c put_cmd-25/drivers/scsi/scsi_lib.c
> --- starve-25/drivers/scsi/scsi_lib.c	Wed Mar 19 16:36:40 2003
> +++ put_cmd-25/drivers/scsi/scsi_lib.c	Wed Mar 19 18:10:54 2003
> @@ -351,7 +351,7 @@ void scsi_setup_cmd_retry(struct scsi_cm
>   *		permutations grows as 2**N, and if too many more special cases
>   *		get added, we start to get screwed.
>   */
> -static void scsi_queue_next_request(request_queue_t *q, struct scsi_cmnd *cmd)
> +void scsi_queue_next_request(request_queue_t *q, struct scsi_cmnd *cmd)
>  {
>  	struct scsi_device *sdev, *sdev2;
>  	struct Scsi_Host *shost;
> @@ -487,7 +487,6 @@ static struct scsi_cmnd *scsi_end_reques
>  	 * need to worry about launching another command.
>  	 */
>  	scsi_put_command(cmd);
> -	scsi_queue_next_request(q, NULL);
>  	return NULL;
>  }
>  
> @@ -906,7 +905,7 @@ static int scsi_init_io(struct scsi_cmnd
>  			req->current_nr_sectors);
>  
>  	/* release the command and kill it */
> -	scsi_put_command(cmd);
> +	__scsi_put_command(cmd);
>  	return BLKPREP_KILL;
>  }
>  
> @@ -1014,7 +1013,7 @@ static int scsi_prep_fn(struct request_q
>  		 */
>  		if (unlikely(!sdt->init_command(cmd))) {
>  			scsi_release_buffers(cmd);
> -			scsi_put_command(cmd);
> +			__scsi_put_command(cmd);
>  			return BLKPREP_KILL;
>  		}
>  	}
> diff -purN -X /home/patman/dontdiff starve-25/drivers/scsi/scsi_syms.c put_cmd-25/drivers/scsi/scsi_syms.c
> --- starve-25/drivers/scsi/scsi_syms.c	Wed Mar 19 11:52:25 2003
> +++ put_cmd-25/drivers/scsi/scsi_syms.c	Wed Mar 19 18:10:54 2003
> @@ -60,6 +60,8 @@ EXPORT_SYMBOL(scsi_allocate_request);
>  EXPORT_SYMBOL(scsi_release_request);
>  EXPORT_SYMBOL(scsi_wait_req);
>  EXPORT_SYMBOL(scsi_do_req);
> +EXPORT_SYMBOL(scsi_get_command);
> +EXPORT_SYMBOL(scsi_put_command);
>  
>  EXPORT_SYMBOL(scsi_report_bus_reset);
>  EXPORT_SYMBOL(scsi_block_requests);
> 
> -- Patrick Mansfield
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Luben

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Douglas Gilbert]

Luben Tuikov wrote:
> Patrick Mansfield wrote:
> 
>> The change to use a pool for scsi_cmnd allocations removed some
>> scsi_queue_next_request calls, this patch restores the calls, and
>> exports scsi_put_command and scsi_get_command.
> 
> 
> And there was a point to this removal.  I did mention it on linux-scsi.
> 
> Look, don't try to make the code look as it did *before* -- there's
> always a point to a change -- I think I mentioned exactly this
> before on linux-scsi...
<snip/>

Luben,
 From my own experience, I know that some patches have a
negative impact. If that is done by someone else in a driver
that I maintain then the bug reports (almost always polite)
come to me. In most cases it means modifying the original
patch to stop "collateral damage".

If you don't like what Patrick is proposing, please explain
why. [Rather than re-explain, you could supply a url to the
prior post in the marc.theaimsgroup.com archive.] I don't
think boldface exclamations are required.
Naturally you are at liberty to present another patch which
merges what you are trying to do with Patrick's work.

Doug Gilbert

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Luben Tuikov]

Douglas Gilbert wrote:
> 
> If you don't like what Patrick is proposing, please explain
> why.

I think I _did_ explain why: contamination of scsi_put_command().
I also think that I explain it in quite a dry and formal way
with examples, trying really hard to convey what I meant.

Please feel free to give me pointers on where I ``missed the mark''.

> [Rather than re-explain, you could supply a url to the
> prior post in the marc.theaimsgroup.com archive.]

On slab allocation:
http://MARC.10East.com/?l=linux-scsi&m=104438145823604&w=2
The whole message is about that.

The second ``Point 2'', on ``change'', ``new'' and ``before'':
http://MARC.10East.com/?l=linux-scsi&m=104387923212007&w=2
Excerpt:
	2. This is a pickle and might I mention that Doug's queue depth
	changes were *for a reason*, so there's no point in saying
	``the way it was before''.  Furthermore ``the number of outstanding
	commands'' is ambiguous and any which way it is it, is NOT enough.
	I.e. outstaning in LLDD, oustanding free, etc. -- it just doesn't
	compute.
This was about the ``way it was before'', i.e. we should not improve
SCSI Core by means of its old workings, but by means of new infrastructure...
(Here's I'd be repeating myself, as I've said those things before on
linux-scsi... sorry, no time to search the archives now.)

> I don't
> think boldface exclamations are required.

I was just trying to make myself clear.

> Naturally you are at liberty to present another patch which
> merges what you are trying to do with Patrick's work.

No, I don't want to.  This has to do with managing ppl and the reasons are
more or less as follows: Patrick is already doing the work -- let him
continue to do it, he's doing a great job;  I don't believe in taking someone's
patch (idea) and rewriting it and resubmitting it as my own.  If they step
out of line, I'd let them know, they can correct it, resubmit the patch, and
thusly continue to do the great work they've been doing.

This way they learn, get more enthusiastic and feel really good about themselves
and next time they'll do an even better job.

If you guys were closer, I'd all buy you a beer! :-)

-- 
Luben

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Patrick Mansfield]

On Thu, Mar 20, 2003 at 04:24:10PM -0500, Luben Tuikov wrote:
> Patrick Mansfield wrote:
> > The change to use a pool for scsi_cmnd allocations removed some
> > scsi_queue_next_request calls, this patch restores the calls, and
> > exports scsi_put_command and scsi_get_command.
> 
> And there was a point to this removal.  I did mention it on linux-scsi.
> 
> Look, don't try to make the code look as it did *before* -- there's
> always a point to a change -- I think I mentioned exactly this
> before on linux-scsi...
> 
> So *ALL* this patch does is REMOVE ONE CALL TO
> scsi_queue_next_request(q, NULL).

It effectively adds one call, not removes one call.

Anwyay, there is a bug, and it needs to be fixed.

I see 3 ways to fix this, 2 easy 1 hard:

1) Go with this patch or similiar - if there are strong opinions against
using scsi_put_command I can change the name.

2) Call scsi_queue_next_request as needed after each scsi_put_command.
This is easy for the upper level drivers if we want to "pollute"
scsi_release_request(). The usage of scsi_put_command in lower-level
drivers then requires an export of scsi_queue_next_request.

3) Deeper changes such that command completion code is handled the same
way. This might not be possible without changing upper level drivers, and
We might end up with similiar interfaces to what we have now.

Are you OK with approach 2?

James - what is your take?

-- Patrick Mansfield

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Luben Tuikov]

Patrick Mansfield wrote:
> On Thu, Mar 20, 2003 at 04:24:10PM -0500, Luben Tuikov wrote:
> 
>>So *ALL* this patch does is REMOVE ONE CALL TO
>>scsi_queue_next_request(q, NULL).
> 
> 
> It effectively adds one call, not removes one call.

Patrick,

Line 81 of your patch removes a function call to
scsi_queue_next_reqeust(q, NULL), as follows:

-	scsi_queue_next_request(q, NULL);

This is in the scsi_end_request() function.

In order to accomodate for this removal,
you contaminate the slab allocation implementation by
introducing ONE MORE needless __scsi_put_command()
function, this is line 9 -- this basically becomes
the original scsi_put_command().

Then you alter the implementation of scsi_put_command()
to include the call to scsi_queue_next_request(q, NULL)
in order to make up for the removed call on line 81.

So effectively you remove one function call to
scsi_queue_next_request(q, NULL) from scsi_end_request(),
and in order to accomodate this removal, you rename
scsi_put_command() to __scsi_put_command()
and then pollute/contaminate the original implementation
of scsi_put_command() to include the removed call
scsi_queue_next_request().

> Anwyay, there is a bug, and it needs to be fixed.

I just want to leave the SCSI command struct allocation
implementation ALONE -- it does its job very nicely and
should be left alone, it should be considered a black
box.

I.e. scsi_get_command(), __scsi_get_command() and scsi_put_command()
should be a black box as far as SCSI Core, ULDD, LLDD are concerned,
just because the memory management may change in the distant
future, without changing the rest of SCSI Core, ULDD and LLDD.

Furthermore, I see MORE parts of SCSI Core become black boxes
and thus SCSI Core's functionality will be defined in terms
of their _interaction_.

> 2) Call scsi_queue_next_request as needed after each scsi_put_command.

Right, this is the one!  Centralize SCSI Core's call to scsi_put_command()
somewhere -- e.g. right before the request is passed back to the
block layer/caller.  At this point we no longer need the scsi command.

As far as SCSI Core is concerned it should have only one call to
scsi_put_command() since char request also go through SCSI Core.

Also the key words here are ``as needed''.  I.e. some callers,
say LLDD, may NOT need to run the queue after scsi_put_command()
since they maybe doing their own freeing of commands for their
own purposes... (but should be allowed to piggyback mem mangmnt to
SCSI Core for command structs).

I.e. scsi_release_request() or in scsi_end_request().

Note that ULDD call scsi_release_request() and this should be ok.

LLDD upon removing a device may call a series of scsi_put_command()
which they called (scsi_get_command()*) when loaded, in which case we
DO NOT want to hit the queue, just so that those commands go
through SCSI Core to come back with errors.

* (for their [LLDD] own functioning)

> This is easy for the upper level drivers if we want to "pollute"
> scsi_release_request().

scsi_release_request() is at higher level than scsi_put_command().

Actually, if I were you, I'd try to have only one call, centralized,
to scsi_put_command(), and the same for scsi_get_command() in
all of SCSI Core.

This way, you'd have a well defined state machine for SCSI Core.

> The usage of scsi_put_command in lower-level
> drivers then requires an export of scsi_queue_next_request.

NO! It does NOT!

It does *only* if your patch is applied as you posted it yesterday.

> Are you OK with approach 2?

Yes.  As far as SCSI struct command allocation (slab) is left ALONE,
and no other functions of the sort are introduced.

-- 
Luben

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Patrick Mansfield]

On Fri, Mar 21, 2003 at 02:55:17PM -0500, Luben Tuikov wrote:
> Patrick Mansfield wrote:
> > On Thu, Mar 20, 2003 at 04:24:10PM -0500, Luben Tuikov wrote:
> > 
> >>So *ALL* this patch does is REMOVE ONE CALL TO
> >>scsi_queue_next_request(q, NULL).
> > 
> > 
> > It effectively adds one call, not removes one call.
> 
> Patrick,
> 
> Line 81 of your patch removes a function call to
> scsi_queue_next_reqeust(q, NULL), as follows:
> 
> -	scsi_queue_next_request(q, NULL);
> 
> This is in the scsi_end_request() function.
> 
> In order to accomodate for this removal,
> you contaminate the slab allocation implementation by
> introducing ONE MORE needless __scsi_put_command()
> function, this is line 9 -- this basically becomes
> the original scsi_put_command().
> 
> Then you alter the implementation of scsi_put_command()
> to include the call to scsi_queue_next_request(q, NULL)
> in order to make up for the removed call on line 81.
> 
> So effectively you remove one function call to
> scsi_queue_next_request(q, NULL) from scsi_end_request(),
> and in order to accomodate this removal, you rename
> scsi_put_command() to __scsi_put_command()
> and then pollute/contaminate the original implementation
> of scsi_put_command() to include the removed call
> scsi_queue_next_request().

No - I'm trying to make sure scsi_queue_next_request() is called for all
IO, not just block IO. Please look at all the places scsi_put_command is
called that I did *not* change.

> > 2) Call scsi_queue_next_request as needed after each scsi_put_command.
> 
> Right, this is the one!  Centralize SCSI Core's call to scsi_put_command()
> somewhere -- e.g. right before the request is passed back to the
> block layer/caller.  At this point we no longer need the scsi command.
> 
> As far as SCSI Core is concerned it should have only one call to
> scsi_put_command() since char request also go through SCSI Core.
> 
> Also the key words here are ``as needed''.  I.e. some callers,
> say LLDD, may NOT need to run the queue after scsi_put_command()
> since they maybe doing their own freeing of commands for their
> own purposes... (but should be allowed to piggyback mem mangmnt to
> SCSI Core for command structs).
> 
> I.e. scsi_release_request() or in scsi_end_request().
> 
> Note that ULDD call scsi_release_request() and this should be ok.
> 
> LLDD upon removing a device may call a series of scsi_put_command()
> which they called (scsi_get_command()*) when loaded, in which case we
> DO NOT want to hit the queue, just so that those commands go
> through SCSI Core to come back with errors.
> 
> * (for their [LLDD] own functioning)
> 
> > This is easy for the upper level drivers if we want to "pollute"
> > scsi_release_request().
> 
> scsi_release_request() is at higher level than scsi_put_command().
> 
> Actually, if I were you, I'd try to have only one call, centralized,
> to scsi_put_command(), and the same for scsi_get_command() in
> all of SCSI Core.

That sounds like the patch I posted, just change the name of
scsi_put_command to scsi_put_command_and_wtf.

> 
> This way, you'd have a well defined state machine for SCSI Core.
> 
> > The usage of scsi_put_command in lower-level
> > drivers then requires an export of scsi_queue_next_request.
> 
> NO! It does NOT!
> 
> It does *only* if your patch is applied as you posted it yesterday.

I don't see any way around this, I'll post the alternate approach, it is
worse than I orginally thought since we have to save
scmd->device->request_queue prior to each scsi_queue_next_request.

I have no problems with you posting an alternate patch.

-- Patrick Mansfield

Re: [PATCH] 2.5.x add back missing scsi_queue_next_request calls

[Luben Tuikov]

Patrick Mansfield wrote:
>>So effectively you remove one function call to
>>scsi_queue_next_request(q, NULL) from scsi_end_request(),
>>and in order to accomodate this removal, you rename
>>scsi_put_command() to __scsi_put_command()
>>and then pollute/contaminate the original implementation
>>of scsi_put_command() to include the removed call
>>scsi_queue_next_request().
> 
> 
> No - I'm trying to make sure scsi_queue_next_request() is called for all
> IO, not just block IO.

No problem here.  This okay.

> Please look at all the places scsi_put_command is
> called that I did *not* change.

The problem was messing with the implementation of scsi_put_command().
This was my beef.  Now I'm glad you posted a new patch.

>>Actually, if I were you, I'd try to have only one call, centralized,
>>to scsi_put_command(), and the same for scsi_get_command() in
>>all of SCSI Core.
> 
> 
> That sounds like the patch I posted, just change the name of
> scsi_put_command to scsi_put_command_and_wtf.

The problem is the spaghetti code which will result.

What I meant in ``centralizing'' is the mirroring of the prep
function -- when scsi_get_command() is called, i.e.
from the scsi_prep_fn() <-- only place in SCSI Core.*

* Ignoring of course scsi_reset_provider()...

What's scsi_prep_fn()'s counterpart?  This is where scsi_put_command()
should be called and where a decision should be made as to whether
scsi_queue_next_request() should be called (host reset, hot plugs,
queue depths...)

> I don't see any way around this, I'll post the alternate approach, it is
> worse than I orginally thought since we have to save
> scmd->device->request_queue prior to each scsi_queue_next_request.
> 
> I have no problems with you posting an alternate patch.

Here are a few thoughts on what I'd submit for a patch:
making clean, _homogeneous_ entries in SCSI Core for a
scsi request, and scsi command.  Two of each -- one
blocking and one non-blocking.  Four, total.

Homogeneous in this context means opposite of what scsi_do_req()
does: NOT to export scsi command (via scsi_done() fn for completion)
IF the ULDD has submitted a scsi_request !!!

scsi command is _part_ of scsi request, and SCSI Core instantiates it
as it needs it, so it should NOT be shown back to the ULDD for completion
purposes -- i.e. need another completion function for scsi_requests.

this way you get (block structure):
-------------------------------------------------
	submit a request R via scsi_do_req()

		instantiate a scsi command C,
		hook it up to R, (R->C)

			do work, when done,
			call scsi_done(C)

scsi_done(C):
		do needed work,
		DESTROY scsi command C
		if (R) do minimal work
		and call scsi_done_req(R)

scsi_done_req(R):
	notifies ULDD of completion status

-------------------------------------------------

This is the logical infrastructure and is mirrored
horizontally about ``do work''.

The functional structure would break this apart as those
will become separate functions.

Then you can centralize all code for instantiating
scsi commands et al. AND the code in ``do work'' can
be reused for entities who just submit scsi commands.
(This would involve a double scsi_done() one for the
command and if request, do minimal work and then call
the scsi_done_req() fn.)

This would also allow a more flexible control as the
logical path is well defined.

I *would* submit such a patch only if I had the time.

-- 
Luben