Hi -
This patch fixes a problem with the current linus bk tree setting
scsi queue depths to 1. Please apply.
Without the patch:
[patman@elm3a50 patman]$ cat /proc/scsi/sg/device_hdr /proc/scsi/sg/devices
host chan id lun type opens qdepth busy online
0 0 0 0 0 2 1 0 1
0 0 1 0 0 1 1 0 1
0 0 15 0 3 0 1 0 1
With the patch:
[patman@elm3a50 patman]$ cat /proc/scsi/sg/device_hdr /proc/scsi/sg/devices
host chan id lun type opens qdepth busy online
0 0 0 0 0 2 253 0 1
0 0 1 0 0 1 253 0 1
0 0 15 0 3 0 2 0 1
--- 1.51/drivers/scsi/scsi.c Tue Oct 29 01:03:27 2002
+++ edited/drivers/scsi/scsi.c Wed Oct 30 08:36:23 2002
@@ -1511,7 +1511,6 @@
kfree((char *) SCpnt);
}
SDpnt->current_queue_depth = 0;
- SDpnt->new_queue_depth = 0;
spin_unlock_irqrestore(&device_request_lock, flags);
}
-- Patrick Mansfield
[email protected] said:
> This patch fixes a problem with the current linus bk tree setting scsi
> queue depths to 1. Please apply.
This patch causes the depth specification to be retained when we release
commandblocks. Since releasing command blocks is supposed only to be done
when we give up the device (and therefore, is supposed to clear everything),
your fix looks like it's merely masking a problem, not fixing it.
Is the real problem that this controller is getting a release command blocks
and then a reallocate of them after slave_attach is called? If so, that's
probably what needs to be fixed.
James
On Wed, Oct 30, 2002 at 11:17:52AM -0600, James Bottomley wrote:
> [email protected] said:
> > This patch fixes a problem with the current linus bk tree setting scsi
> > queue depths to 1. Please apply.
>
> This patch causes the depth specification to be retained when we release
> commandblocks. Since releasing command blocks is supposed only to be done
> when we give up the device (and therefore, is supposed to clear everything),
> your fix looks like it's merely masking a problem, not fixing it.
>
> Is the real problem that this controller is getting a release command blocks
> and then a reallocate of them after slave_attach is called? If so, that's
> probably what needs to be fixed.
>
> James
Yes, the problem is that in scsi_register_host() if there are no upper
level drivers - the standard case if building no modules - we call
scsi_release_commandblocks even though we are NOT getting rid of
the scsi_device. So, with current code, new_queue_depth and
current_queue_depth are zero.
When we register upper level drivers in scsi_register_device(), we
call scsi_build_commandblocks (again), and get a queue depth of 1,
since we've cleared new_queue_depth.
(In many cases, for one device we call build command blocks twice, call
release command blocks, and then build command blocks again. Yuck)
Removing the scsi_release_commandblocks() in scsi_register_host()
would also fix the problem, and in most cases, would not waste any
space. In the worst case AFAICT it would waste one scsi_cmnd (about 300
or so bytes?).
I see no good reason to zero new_queue_depth in scsi_release_commandblocks,
as new_queue_depth is the desired queue depth, and should remain so until
scsi_adjust_queue_depth is called. Setting new_queue_depth to zero means
we have to call slave_attach again to set it right, and depending on what
else an adapter slave_attach does could be very wrong.
-- Patrick Mansfield
> Yes, the problem is that in scsi_register_host() if there are no upper
> level drivers - the standard case if building no modules - we call
> scsi_release_commandblocks even though we are NOT getting rid of the
> scsi_device. So, with current code, new_queue_depth and
> current_queue_depth are zero.
But slave_attach isn't called here (even though it should be for attached
devices). I assume it's getting added by the scan between register_host &
register_device.
> When we register upper level drivers in scsi_register_device(), we
> call scsi_build_commandblocks (again), and get a queue depth of 1,
> since we've cleared new_queue_depth.
OK, we have a slight mess up here. Perhaps the rule for slave_attach should
be that we only call it if we actually have an upper level device attached (if
we haven't, there's little point asking the HBA to allocate space for queueing
for a device we're not currently using). Then, we should do slave_attach when
something actually decides to attach to the device.
if we follow this approach, slave_attach wouldn't be called until
register_device in your problem scenario, and then everything would work as
expected.
> Removing the scsi_release_commandblocks() in scsi_register_host()
> would also fix the problem, and in most cases, would not waste any
> space. In the worst case AFAICT it would waste one scsi_cmnd (about
> 300 or so bytes?).
Well, if there's no device attached, there's no need for a queue. This would
waste 1 SCSI command per unattached device (and SCSI commands are DMA'able
memory which is precious on some systems). Right now, that's OK for small
systems. When we move to a lazy attachment model because we have an array
with 65535 LUNs and we're only interested in one of them, it won't be.
> I see no good reason to zero new_queue_depth in scsi_release_commandblo
> cks, as new_queue_depth is the desired queue depth, and should remain
> so until scsi_adjust_queue_depth is called. Setting new_queue_depth to
> zero means we have to call slave_attach again to set it right, and
> depending on what else an adapter slave_attach does could be very
> wrong.
Well, to my way of thinking, build and release commandblocks are like
constructor and destructor for the device queue. On general design
principles, I don't like the idea of queue specific information persisting
past its destruction.
James