Re: [PATCH] first cut at fixing unable to requeue with no outstanding commands

[Mike Anderson <andmike@us.ibm.com>]

James Bottomley [James.Bottomley@steeleye.com] wrote:
> patmans@us.ibm.com said:
> > What about applications and devices that only send one IO at a time?
> > They could still hang.
> 
> > We have: tape (st or osst), sg usage, partitioning, scanning, direct
> > use of the block device (i.e. dd if=/dev/sda), and probably others. 
> 
> I don't believe so.
> 
> Unplugging is a global thing.  The request function for a plugged queue will 
> always be run, that's a guarantee, so using this approach, the queue will 
> stall but never hang forever.
> 
> I think for a rejection of a command with none outstanding, a stall is what 
> you want (give the host/device time to recover from whatever the problem is).  
> The length of the stall will be dependent on I/O pressure in the system, which 
> is also roughly what you want: we can afford to give a device quite a while to 
> recover if we're not desparate to get I/O to it. We can also handle BUSY 
> returns this way too...
> 
> James

ok I see the call path now. I was unsure that blk_run_queues would be
called for non-fs IO.

I traced a dd (dd if=/dev/sda of=/dev/null count=2 bs=512) command under
uml with a modified scsi_debug to return 1 on a queuecommand call and
your patch. The trace showed blk_run_queues being called through
wb_kupdate.

#0  scsi_request_fn (...) at scsi_lib.c:738
#1  0xa00beda2 in blk_run_queues () at ll_rw_blk.c:1045
#2  0xa004fb0d in wb_kupdate (...) at page-writeback.c:270
#3  0xa004f6c9 in __pdflush (...) at pdflush.c:130
#4  0xa004f76e in pdflush (...) at pdflush.c:178
#5  0xa001aba4 in run_kernel_thread (...) at process.c:232


Since you are modifying scsi_mlqueue_insert what about a couple of these
cleanups.

	- Is the call to scsi_delete_timer really necessary. All callers
	  have already deleted the timer. The del_timer function takes a
	  lock which would be nice to avoid if we do not need to call
	  it.  If we want to protect the code we could do a quick check
	  on SCset->eh_timeout.function prior to calling.

	- Patrick pointed out a while ago that the "if (host->host_busy
	  == 0)" check and the similar one for the device will never be
	  called because to be in this function the values of these
	  variables need to be at least 1. I believe this direct call to
	  scsi_retry_command should be removed instead of adjusting the
	  check to "== 1" as this seems counter to how you are trying to
	  handle busy.

-andmike
--
Michael Anderson
andmike@us.ibm.com