2.4.19 SCSI error handing, door locking, etc

2.4.19 SCSI error handing, door locking, etc

[Russell King]

Ok, I've uploaded the patches below to:

	ftp://ftp.linux.org.uk/pub/linux/rmk/scsi
	http://ftp.linux.org.uk/pub/linux/rmk/scsi

You'll find all the patches there, plus a tarball of all patches
and the text below (the README is a copy of the text below.)

Things outstanding:

- scsi-cmd-retry - brings in scsi_setup_cmd_retry without a
  prototype; expect some compiler warnings for now.

- scsi-door-lock - sd/st/sr (etc) door-locking callers don't
  themselves check the return value.  Problem noticed by
  Patrick Mansfield.

- scsi-unjam-clean - has only been _compile_ tested.  It is
  the result of a merge of my new error handling code.

Unfortunately, the earliest I'll be able to address anything arising
from this will be Saturday, I've got to context-switch onto other
stuff for the next couple of days.  This means the earliest I
can provide answers to feedback received is Saturday.

Thanks to Doug Ledford, James Bottomley, Patrick Mansfield and
Mike Anderson for comments thus far.


SCSI subsystem patches
----------------------

This is a set of patches created to make the SCSI error handling
more robust.  I have experienced a number of deadlocks and bad
behaviour by the SCSI subsystem in the presence of errors, such
as:

- locking device doors on devices that aren't in use.
- deadlocking trying to lock device doors
- deadlocking during scanning for devices with a faulty device
  (fault caused by test code in the HBA)

The filenames indicate the patch series, and other requirements.
The format of these names is:

	nn-desc-number.diff

nn	This is the series number, and series are expected to be
	applied in order.  Applying out of order may produce
	patch rejects that need to be fixed by hand, or fuzz.

desc	Brief description of what the patch does

number	Sub-series number, specifies the order the patches must
	be applied in.  If a series has 8 patches, and you want
	number 4, you need to apply patches 1 to 4 and you can
	ignore 5 to 8.

01-scsi-cmd-retry-1.diff

	Provide scsi_setup_cmd_retry function to setup a Scsi_Cmnd
	structure for a retry of the command.  Make the four places
	that re-setup the command for a retry use scsi_setup_cmd_retry

01-scsi-cmd-retry-2.diff

	Move the NEEDS_RETRY mechanism out of scsi_send_eh_cmnd
	into the caller of that function.  This corrects a
	serious error handling bug where we retry a command
	without re-initialising the Scsi_Cmnd structure.

01-scsi-cmd-retry-3.diff

	Bound the number of retries in the error handler.  Infinite
	command retries lock the host.  It is far better that we
	try to free up the host.  Problem noticed by Doug Ledford.
	Basic idea for solution from Mike Anderson.

02-scsi-cmd-report.diff

	When reporting scsi errors to the user, tell the user about
	the ultimate command we were trying to perform, not the
	current command which may be an error handing/driver invoked
	request sense command.

03-scsi-restart-ops.diff

	Don't stop kicking device queues because one device is
	blocked. This test is actually redundant; we check for a
	blocked device in scsi_request_fn() and exit virtually
	immediately.  We therefore just call the function and let
	it deal with the issues in one place.

04-scsi-door-lock-1.diff

	Introduce a new per-device flag - "locked".  When this flag
	is set, it means that user space wanted the door locked, and
	we have successfully locked it.  When clear, it means something
	user space wanted the door unlocked, and we have successfully
	unlocked it.

	Introduce "scsi_set_medium_removal()".  This handles all door
	locking and unlocking requests that essentially originate from
	user space. ie, when a device is opened or closed, or an ioctl
	is received.  This function takes care of issuing the
	ALLOW_MEDIUM_REMOVAL command via the ioctl layers.

	This function isn't really anything new; its existing code
	moved into a function.  It also removes code duplication.

	The new bit is what it does after the command has completed.
	If the command was successful, we update the device "locked"
	flag to indicate the new state.

	Rather than indirecting via scsi_ioctl() from the various
	drivers, we now call scsi_set_medium_removal direct

04-scsi-door-lock-2.diff

	The "new" error handling code.  Just before we attempt to
	restart operations on a host in scsi_restart_operations(), we
	loop through all devices on the host.  At this point, we don't
	have much idea which devices received a reset.

	Any device that is online (something the old code never checked
	for) and was locked, we try to re-lock the door.  Note how we
	handle this.  We create a request structure, and fill in all
	the relevant values, and insert this request at the _head_ of
	the queue.

	The requests done function merely frees the command.  If the
	command fails, we can't really do much to recover from it.
	We could retry, but the generic SCSI command handling will
	have done that for us already.

	The "old" code.  Yes, suggestion: read the comment.  Since we
	have the "new" error handling code in place, we must not to
	lock the door while processing requests.  If we do, we'll
	certainly deadlock.

	However, we do take note of the currently requested lock state,
	and only ask for the door to be locked if it was previously
	locked.

05-scsi-unjam-clean-1.diff

	Carve up the huge scsi_unjam_host() function into a number of
	functions that perform one and only one action.  Although
	we keep scsi_unjam_host(), it now iterates through an array
	of functions, which have the "knowledge" about unjamming a
	host.

	There are no functional changes in this patch.

05-scsi-unjam-clean-2.diff

	New function - scsi_eh_find_failed_command().  This finds the
	first failed command on a device.  Unfortunately, we need a
	Scsi_Cmnd structure to call the various HBA reset functions
	with, rather than a Scsi_Device/Scsi_Host+channel/Scsi_Host for
	bus_device_reset/bus_reset/host_reset respectively.

	However, in this patch there is only one user, the BDR code.

	There are no functional changes in this patch.

05-scsi-unjam-clean-3.diff

	There is a fair amount of duplication across all the reset
	handlers; after performing any type of reset, they send a test
	unit ready command, and if that succeeds, they retry the original
	command.

	This patch factors that functionality out into a new function -
	scsi_eh_test_and_retry().

	There are no functional changes in this patch.

05-scsi-unjam-clean-4.diff

	In a similar way, we factor out the code which takes devices off
	line; this ends up in scsi_eh_set_device_offline().

	Once we have taken a device off line, there is no point looking
	at any other commands on this device, since we've declared it
	dead.  We therefore finish all commands with the appropriate
	status.

	Note: this does mean that we guarantee that we won't call any
	reset handlers more than once for each device that has failed.

05-scsi-unjam-clean-5.diff

	Factor out code that loops through all commands on a device
	trying to restart failed or timed out commands.

	This allows us to fix an outstanding fixme in the bus device
	reset code:

		FIXME(eric) - make sure we handle the case
		where multiple commands to the same device
		have failed. They all must get properly
		restarted.

	Note: We also only attempt no more than one bus reset per device
	(we don't yet have the concept of a bus), and one host reset per
	host.

05-scsi-unjam-clean-6.diff

	This is probably the most yuckiest and difficult patch to read
	of the whole set.  This gives us a per-channel basis for
	performing bus resets.
        
	This also kills off the unnecessary loops within loops within
	loops within loops.  James Bottomley assures me that for any
	Scsi_Cmnd, it will be found in SCpnt->device->device_queue.

	The final point also applies to the host reset code.

05-scsi-unjam-clean-7.diff

	At last, we get to the patch that really makes the difference for
	me.  There are two distinct changes.

	Firstly, rather than taking a device off line because it reports
	that it is not ready shortly after a reset, we give it a total
	of 3 retries 5 seconds apart to report ready.  My tape drive
	regularly fails the original code here while its re-initialising
	after the reset.

	It may be better that we filter out the "medium not ready" sense
	codes and use the command's timeout/retry count for that.  However,
	this code seems to be adequate.

	Secondly, if we fail to restart all commands on a failed device,
	just before taking the device off line, we perform the same
	action we did to free the bus.	This means a host or bus reset
	as appropriate.

	Why is this necessary?	Scenario: a command is sent to a device,
	whose firmware crashes and holds the SCSI bus.	We perform all
	the necessary error handling, and find that a bus reset seems
	to get things moving again, and we re-issue the same command to
	the device.  The device fails in the same way.

	At this point, the existing code will attempt to abort the
	command.  Even if the HBA driver says that it is unable to abort
	the command, the SCSI mid-layer re-uses that command for its own
	purposes.  However, the HBA driver still contains a reference to
	that command.

	Since resets are supposed to clear the HBAs knowledge of commands
	and free the SCSI bus, we use the appropriate reset that fixed
	the problem the last time around immediately prior to taking
	an unrecoverable device off line.

05-scsi-unjam-clean-8.diff

	Final patch of the lot.  This one is rather boring, and is just
	cleanup and styling.  Make comments and most code in this area
	wrap before column 80.  Make sure we have scsi logging where
	appropriate, remove/rename unnecessary variables, and remove
	unnecessary braces.
	

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4.19 SCSI error handing, door locking, etc

[Mike Anderson]

Russell,

	Sorry it took so long to port forward and test these changes.  I
	will be posting the set of patches to the list right after this
	mail.

	Here is an overview the forward port in relation to your
	patches.

01-scsi-cmd-retry-1.diff
	- Brought forward with minor arg/ variable renaming.
01-scsi-cmd-retry-2.diff
	- Brought forward though my original implementation of how retries
	  are controlled is different than your code.
01-scsi-cmd-retry-3.diff
	- I need to think on this. I currently have 1 retry for TUR and
	  non for REQ sense. If a device is not responding the way we want the
	  probability of sending multiple commands in a short time period
	  especially TURs is most likely not going to result in what we
	  want. Though your delay mentioned below is something I
	  currently do not have. 
02-scsi-cmd-report.diff
	- I brought this forward, but it bothers me that the data in a
	  cmd is incorrect as the command should be restored after it has
	  been touched by error handling.
03-scsi-restart-ops.diff
	- Brought forward. In 2.5 we should probably consider removing all
	  these checks and just call the request_fn so that we do not have
	  the checks spread all over which leads to them becoming out of
	  sync or implemented incorrectly.  
04-scsi-door-lock-1.diff
	- Brought forward. I did need to increase the bus reset settle
	  time from 5 seconds to 10 to get the door lock to re-lock the
	  door.
04-scsi-door-lock-2.diff
	- Brought forward. In 2.5 we do not have the old error handler so
	  I removed the door lock completely from the the scsi_request_fn
	  function.
05-scsi-unjam-clean-1.diff
05-scsi-unjam-clean-2.diff
05-scsi-unjam-clean-3.diff
05-scsi-unjam-clean-4.diff
05-scsi-unjam-clean-5.diff
05-scsi-unjam-clean-6.diff
05-scsi-unjam-clean-8.diff
	- The previous patch I created for 2.5 scsi_error cleanup I
	believe covers similar changes as yours for 2.4. Patch is now
	part of 2.5.40.

05-scsi-unjam-clean-7.diff
	- I have not covered all these yet. One change that
	  made a difference is how the tur retry is done and I also
	  increased the bus_reset settle delay from 5 to 10. I will try to
	  ensure that the error policy you want is covered in 2.5.

2.) Added scsi_set_medium_removal to scsi_syms.c and scsi_ioctl.h



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

Re: 2.4.19 SCSI error handing, door locking, etc

[Russell King]

On Fri, Oct 04, 2002 at 12:41:36PM -0700, Mike Anderson wrote:
> 01-scsi-cmd-retry-3.diff
> 	- I need to think on this. I currently have 1 retry for TUR and
> 	  non for REQ sense. If a device is not responding the way we want the
> 	  probability of sending multiple commands in a short time period
> 	  especially TURs is most likely not going to result in what we
> 	  want. Though your delay mentioned below is something I
> 	  currently do not have. 

With the current behaviour, I've seen this situation:

- bus reset
- tape drive responds to bus reset by positioning at the start of tape
- while tape drive is repositioning, it returns "drive not ready"
- we mark the tape drive off line despite there being absolutely
  nothing wrong with it.

> 02-scsi-cmd-report.diff
> 	- I brought this forward, but it bothers me that the data in a
> 	  cmd is incorrect as the command should be restored after it has
> 	  been touched by error handling.

It goes wrong when HBA drivers use the Scsi_Cmnd to perform the
request sense, and don't restore the command structure.  I've seen
no documentation that says "HBA drivers must restore the Scsi_Cmnd
structure to pristine state before handing it back to the scsi core."

Ok, I'll update my 2.4 version with your comments to keep things
consistent.  The bits people are happy with I'm again submit to Alan.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4.19 SCSI error handing, door locking, etc

[Patrick Mansfield]

Hi -

I noticed that the tape drivers (st.c and osst.c) are not using the
scsi_ioctl() for door locking, so they won't ever set the new
Scsi_Device::locked.

There is some auto-relock code, but AFAICT (at least st.c, searching for
MTLOCK) it will only relock after a reset (if was_reset is still set),
and we are calling st_ioctl() for one of these MT commands:

	MTREW MTOFFL MTRETEN MTERASE MTSEEK MTEOM

The user can't even relock the door unless one of the above commands 
was sent (again, if was_reset is still set).

The auto-relock in rw_checks() is never hit if was_reset is set.

Kai - should st.c (and osst.c?) be using scsi_ioctl(SCSI_IOCTL_DOORLOCK/UNLOCK)?

-- Patrick Mansfield

Re: 2.4.19 SCSI error handing, door locking, etc

[Kai Makisara]

On Fri, 4 Oct 2002, Russell King wrote:

> On Fri, Oct 04, 2002 at 12:41:36PM -0700, Mike Anderson wrote:
> > 01-scsi-cmd-retry-3.diff
> > 	- I need to think on this. I currently have 1 retry for TUR and
> > 	  non for REQ sense. If a device is not responding the way we want the
> > 	  probability of sending multiple commands in a short time period
> > 	  especially TURs is most likely not going to result in what we
> > 	  want. Though your delay mentioned below is something I
> > 	  currently do not have.
>
> With the current behaviour, I've seen this situation:
>
> - bus reset
> - tape drive responds to bus reset by positioning at the start of tape
> - while tape drive is repositioning, it returns "drive not ready"
> - we mark the tape drive off line despite there being absolutely
>   nothing wrong with it.

Because of this possible rewind at reset, st needs to know that a reset
has happened. For instance, writing after this may be more or less
catastrophic (depending on the value of the data that was on the tape).

Currently st uses the SDpnt->was_reset flag to reject any request until
the user has performed a command or activity that moves the tape to a
known position (e.g., rewind or insert new tape). After this it is safe to
continue. But, there is no need to offline the tape device after reset.

Looking at the current st code and the midlevel code, there seems to be a
problem in st (and it is good that the device has been offlined ;-).
The midlevel tries to lock the drive door and resets the was_reset flag
during processing of the next request and st loses the knowledge of reset.
I have to change this so that st maintains its own flag. There seem to be
also some other details to fix but these are not fatal.

In general, there is one with resets that I have not seen discussed (or
I have missed the discussion). The drivers can set the mode parameters
they know about. However, in some cases there may be some parameters set
by a user space program. Should the reset cause a hotplug event or
soemthing similar?

	Kai

Re: 2.4.19 SCSI error handing, door locking, etc

[Kai Makisara]

On Fri, 4 Oct 2002, Patrick Mansfield wrote:

> Hi -
>
> I noticed that the tape drivers (st.c and osst.c) are not using the
> scsi_ioctl() for door locking, so they won't ever set the new
> Scsi_Device::locked.
>
> There is some auto-relock code, but AFAICT (at least st.c, searching for
> MTLOCK) it will only relock after a reset (if was_reset is still set),
> and we are calling st_ioctl() for one of these MT commands:
>
> 	MTREW MTOFFL MTRETEN MTERASE MTSEEK MTEOM
>
> The user can't even relock the door unless one of the above commands
> was sent (again, if was_reset is still set).
>
> The auto-relock in rw_checks() is never hit if was_reset is set.
>
> Kai - should st.c (and osst.c?) be using scsi_ioctl(SCSI_IOCTL_DOORLOCK/UNLOCK)?
>
st.c should definitely do it. I will do the changes very soon (together
with some other changes related to resets done by the midlayer). (The
ioctl did not exist when I put the current code into st.c and I have not
seen any pressing reason the change that until now.) The same changes
should probably be made to osst.c. I will tell the maintainers.

	Kai

Re: 2.4.19 SCSI error handing, door locking, etc

[Willem Riede]

On 2002.10.05 03:54 Kai Makisara wrote:
> On Fri, 4 Oct 2002, Patrick Mansfield wrote:
> 
> > Kai - should st.c (and osst.c?) be using
> scsi_ioctl(SCSI_IOCTL_DOORLOCK/UNLOCK)?
> >
> st.c should definitely do it. I will do the changes very soon (together
> with some other changes related to resets done by the midlayer). (The
> ioctl did not exist when I put the current code into st.c and I have not
> seen any pressing reason the change that until now.) The same changes
> should probably be made to osst.c. I will tell the maintainers.
> 
Kai,

If you would be so kind as to tell me how you're changing st to do this,
I'll make the equivalent change for osst.

Thanks, Willem Riede.