Re: [PATCH] 6/7 add and use a per-scsi_device queue_lock

[Patrick Mansfield <patmans@us.ibm.com>]

On Tue, Mar 25, 2003 at 04:03:27PM -0500, Luben Tuikov wrote:
> Patrick Mansfield wrote:

> >           */
> >  	scsi_host_busy_dec_and_test(host, device);
> > +	spin_lock_irqsave(SCpnt->device->request_queue->queue_lock, flags);
> > +	SCpnt->device->device_busy--;
> > +	spin_unlock_irqrestore(SCpnt->device->request_queue->queue_lock, flags);
> 
> 
> Such acrobatics warrant an atomic variable. I.e. make device_busy an
> atomic_t and avoid all this juggling.
> 
> atomic_dec(SCpnt->device->device_busy);

If we handled all of the SCSI IO completions the same way (got rid of
scsi_retry_command and more), the device_busy-- might happen in
scsi_queue_next_request or a related function, where we might do more than
just decrement device_busy with the lock held. This is related somewhat to
the current handling of scsi_queue_next_request, in that we should always
call it after IO completion.

Having both an atomic_t for device_busy and a lock protecting other
scsi_device and queue data does not appear to give any overall reduction
in executed code (at least on x86, I don't know about other archs). We get
rid of a spin lock/unlock for the above, but we have to add 3
atomic_read's and one atomic_inc in the main line code path - on x86 SMP
four assembler lock instructions (counting in my current tree with all the
lock patches applied).

We should leave this as-is for now, and future SCSI IO completion clean up
could determine how best to protect device_busy.

-- Patrick Mansfield