Re: [PATCH] fix 2.5 scsi queue depth setting

[Doug Ledford <dledford@redhat.com>]

On Wed, Nov 06, 2002 at 10:24:44AM -0800, Patrick Mansfield wrote:
> On Wed, Nov 06, 2002 at 12:47:28PM -0500, J.E.J. Bottomley wrote:
> > This is what I think needs to be done to the patch, does this look OK?
> > 
> > James
> 
> The only problem is that scsi_build_commandblocks() sets sdev->new_queue_depth 
> to 1, so if the slave_attach() does not set queue depth, we will leave
> it at 1, and the post-slave_attach call to scsi_adjust_queue_depth
> can never be hit.
> 
> It looks like the code could be removed from scsi_build_commandblocks,
> since (in this code path) we will always call scsi_adjust_queue_depth
> before sending IO, and other code paths always set new_queue_depth
> prior to calling scsi_build_commandblocks.
> 
> It would be nice if scsi_request_fn would abort IO if current_queue_depth
> == 0 or new_queue_depth == 0.
> 
> And yes, I missed scsi_syms.c.

Actually, I've been planning a slight API change to the slave_attach() 
lldd stuff.  Basically, if a host implements slave_attach(), then it 
*must* call adjust_queue_depth() on all devices passed to it, whether they 
are going to use tagged queueing or not.  They should either call 
adjust_queue_depth() with a tag parameter of non-0 and the actual depth 
they want, or with a tag parameter of 0 and 1 if they don't support 
multiple untagged commands at a time (many drivers call this linked 
commands even though they don't use the scsi protocol version of linking, 
they just queue the commands up internally and send them one after the 
other) or whatever number they support at one time on untagged devices.  
The rational for this involves me not liking wishy-washy if statements 
like what's in the scsi_slave_attach() right now.  Instead of:

if(host->hostt->slave_attach)
  host->hostt->slave_attach(device);
if ((sdev->new_queue_depth == 0) &&
    (sdev->host->cmd_per_lun != 0))
  scsi_adjust_queue_depth(sdev, 0, sdev->host->cmd_per_lun);
scsi_build_commandblocks(sdev);

(which can be tricked BTW, since some slave_attach() routines don't touch 
untagged devices and we set the new_queue_depth to 1 to enable scanning, 
we won't even set untagged devices to cmd_per_lun like our intent is), I 
would rather we did this:

if(host->hostt->slave_attach)
  blah;
else
  scsi_adjust_queue_depth(sdev, 0, cmd_per_lun);

with the requirement that slave attach always do the correct 
scsi_adjust_queue_depth.  Anyway, that's my preference.

Now, the second reason I want this is in preparation for another change I 
have planned.  This one I would like some discussion on anyway, so I'm 
bringing it up now:

I want to make the queuecommand() interface optional, with the ability to 
replace it with a combo of prepare_command() and send_command() instead.  
A driver can only define queuecommand() or the prepare,send_command() 
pair.  The obvious benefit is that a prepare then send routine is needed 
to make sequential untagged commands fast without having to implement a 
driver internal queue, and this is necessary for streaming devices that 
can't tolerate long delays between commands without falling out of their 
current stream (tapes, early CD-Rs and current very cheap CD-Rs, etc).  
Obviously, this new interface would require that driver authors actually 
think about things like sendable commands vs. just prepped commands, etc.  
Making them always call adjust queue depth is one way of forcing them to 
think about these issues, hence why I like requiring it in all 
slave_attach calls.  Thoughts?

-- 
  Doug Ledford <dledford@redhat.com>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606