[SUMMARY] libata EH

[SUMMARY] libata EH

[Tejun Heo]

 Hello, Jeff & libata developers.

 I've been spending some time on libata EH and am trying to make a
list of things which can be improved.  If you have something, please
add to the list.

1. Errors are handled in multiple paths.

	* ATA errors are handled directly in intr context

	* timeouts are handled by ->eng_timeout via SCSI EH

	* ATAPI errors are forwarded to ->eng_timeout in somewhat hackish way

2. Synchronization

	* SCSI EH entrance is not synchronized with normal processing.
	  ATAPI error handling/timeout handling can run concurrently
	  with normal command processing.  Albert, I think it's the
	  same problem you're trying to solve by moving ATA_QCFLAG_ACTIVE
	  clearing.

	  http://marc.theaimsgroup.com/?l=linux-ide&m=112417360223374&w=2

	* SCSI EH entrance is not synchronized with polling tasks.
	  We should cancel & wait for polling tasks on entry to EH, but
	  we don't.

3. Error handling too weak

	* We need to check the device responds to commands (say, w/
	  IDENTIFY or CHK_POWER) after an error, and then reset if it
          doens't.  To do this, we need to handle all errors in EH.

4. Better error reporting

	* We currently depend on ATA_STAT and ATA_ERR register values
	  to check for and report errors.  As Jeff said, this is way
	  too crude.  We need better error reporting.  I think having
	  unified code path for error handling will help implementing
	  this.

5. EH is currently holding off other improvements

	* NCQ controllers (or any other non-legacy ATA interface based
          ones) don't fit nicely into current ATA error handling in
          interrupt scheme.  As NCQ errors require issuing read log
	  command, we need to be in EH context to handle these errors.

	* To properly implement hotplug, we need to have solid error
          handling.  IMHO, implementing hotplug with current EH will
	  be quite fragile.

 Whether we choose to stay with ->eh_strategy_handler or move over to
fine-grained SCSI EH, IMHO, libata EH needs some work.  So... how
should we proceed?

 Thanks.

-- 
tejun

Re: [SUMMARY] libata EH

[Jeff Garzik]

(added Alan, Bart and Jens to CC)

Tejun Heo wrote:
>  Hello, Jeff & libata developers.
> 
>  I've been spending some time on libata EH and am trying to make a
> list of things which can be improved.  If you have something, please
> add to the list.
> 
> 1. Errors are handled in multiple paths.
> 
> 	* ATA errors are handled directly in intr context

Simple stuff like "command aborted" (invalid command) can be handled 
immediately, no need to kick in the error handling.

But as long as the right hardware interrupts are acknowledged, I don't 
mind if all error handling is moved to the thread.


> 	* timeouts are handled by ->eng_timeout via SCSI EH
> 
> 	* ATAPI errors are forwarded to ->eng_timeout in somewhat hackish way

->eng_timeout is somewhat misnamed.  A more correct name would be 
"error_hander".  ATAPI errors are intentionally forwarded to SCSI EH via 
the normal check-condition handling paths, which cause the SCSI EH to be 
invoked.


> 2. Synchronization
> 
> 	* SCSI EH entrance is not synchronized with normal processing.
> 	  ATAPI error handling/timeout handling can run concurrently
> 	  with normal command processing.  Albert, I think it's the
> 	  same problem you're trying to solve by moving ATA_QCFLAG_ACTIVE
> 	  clearing.
> 
> 	  http://marc.theaimsgroup.com/?l=linux-ide&m=112417360223374&w=2

The SCSI layer stops all command processing before calling 
->eh_strategy_handler().  Where do you see that it runs concurrently 
with normal command processing?  That should definitely -not- be happening.


> 	* SCSI EH entrance is not synchronized with polling tasks.

Yes, this definitely needs fixing.

Luckily the polling task is very rarely used, by normal users.


> 3. Error handling too weak
> 
> 	* We need to check the device responds to commands (say, w/
> 	  IDENTIFY or CHK_POWER) after an error, and then reset if it
>           doens't.  To do this, we need to handle all errors in EH.

First you need to classify the error, then handle based on that 
classification.

- DMA and PCI bus errors are usually indicated via status bits in 
controller registers, such as ATA_DMA_ERR on standard PCI IDE controllers.

- Device errors will be indicated via the ATA Status and Error registers.


- PCI bus errors should be handled by resetting the host controller (if 
possible), and then retrying the command [NOTE: better suggestions welcome]

- DMA errors should be handled by hueristics:  If more than $N (3?) DMA 
errors happen in 15 minutes,
* decrease SATA PHY speed.  if speed cannot be decreased,
* decrease UDMA xfer speed.  if at UDMA0, switch to PIO4
* decrease PIO xfer speed.  if at PIO3, complain, but continue

Commands should be retried after DMA errors.

- Device errors should handled as per ATA specs.  Usually these error do 
not cause a retry, but they may require additional inquiry such as READ 
LOG EXT if its a media error.


> 4. Better error reporting
> 
> 	* We currently depend on ATA_STAT and ATA_ERR register values
> 	  to check for and report errors.  As Jeff said, this is way
> 	  too crude.  We need better error reporting.  I think having
> 	  unified code path for error handling will help implementing
> 	  this.

This will fall naturally out of better error handling (stuff I described 
above).


> 5. EH is currently holding off other improvements
> 
> 	* NCQ controllers (or any other non-legacy ATA interface based
>           ones) don't fit nicely into current ATA error handling in
>           interrupt scheme.  As NCQ errors require issuing read log
> 	  command, we need to be in EH context to handle these errors.
> 
> 	* To properly implement hotplug, we need to have solid error
>           handling.  IMHO, implementing hotplug with current EH will
> 	  be quite fragile.

Correct.  Both NCQ and hotplug really want decent error handling -- 
particularly hotplug.  Hotplug will likely travel the error handling 
paths, though at that point we prefer the English word "exception" 
rather than "error"  :)


>  Whether we choose to stay with ->eh_strategy_handler or move over to
> fine-grained SCSI EH, IMHO, libata EH needs some work.  So... how
> should we proceed?

Well, moving over to the fine-grained hooks should make the remaining 
problems Mark sees go away, as well as guaranteeing that we get command 
completion right.

Staying with ->eh_strategy_handler() means auditing the entire SCSI 
layer to check and see what gets set on error, which libata must then 
manually reset.  Obviously there are still some bugs in this area, as 
Mark has demonstrated.  The big reason why I don't like 
->eh_strategy_handler() is that it continues to be an unknown quantity, 
in terms of, we don't still have a complete list of things-to-do during 
error handling.

If you are interested in tackling the work, you're more than welcome to 
choose either path.  Either way, the work won't go to waste.

	Jeff

Re: [SUMMARY] libata EH

[Tejun Heo]

  Hello, guys.

Jeff Garzik wrote:
> 
> (added Alan, Bart and Jens to CC)
> 
> Tejun Heo wrote:
> 
>>  Hello, Jeff & libata developers.
>>
>>  I've been spending some time on libata EH and am trying to make a
>> list of things which can be improved.  If you have something, please
>> add to the list.
>>
>> 1. Errors are handled in multiple paths.
>>
>>     * ATA errors are handled directly in intr context
> 
> 
> Simple stuff like "command aborted" (invalid command) can be handled 
> immediately, no need to kick in the error handling.
> 
> But as long as the right hardware interrupts are acknowledged, I don't 
> mind if all error handling is moved to the thread.
> 

  My preference is toward unifying into single path as long as 
performance penalty is acceptable for the sake of simplicity.

> 
>>     * timeouts are handled by ->eng_timeout via SCSI EH
>>
>>     * ATAPI errors are forwarded to ->eng_timeout in somewhat hackish way
> 
> 
> ->eng_timeout is somewhat misnamed.  A more correct name would be 
> "error_hander".  ATAPI errors are intentionally forwarded to SCSI EH via 
> the normal check-condition handling paths, which cause the SCSI EH to be 
> invoked.
> 
> 
>> 2. Synchronization
>>
>>     * SCSI EH entrance is not synchronized with normal processing.
>>       ATAPI error handling/timeout handling can run concurrently
>>       with normal command processing.  Albert, I think it's the
>>       same problem you're trying to solve by moving ATA_QCFLAG_ACTIVE
>>       clearing.
>>
>>       http://marc.theaimsgroup.com/?l=linux-ide&m=112417360223374&w=2
> 
> 
> The SCSI layer stops all command processing before calling 
> ->eh_strategy_handler().  Where do you see that it runs concurrently 
> with normal command processing?  That should definitely -not- be happening.
> 

  There are currently two problems.

  * As we don't grab host_set lock on entry to ata_scsi_error(), we can
    run concurrently with latter part of ata_qc_complete().  This race is
    addressed by the following patches I've just posted.

    http://marc.theaimsgroup.com/?l=linux-ide&m=112454734102242&w=2

  * After entering EH, normal command completion or spurious interrupt
    can occur.  We currently don't peg those interrupts, so interrupt
    handling can interfere with EH.

> 
>>     * SCSI EH entrance is not synchronized with polling tasks.
> 
> 
> Yes, this definitely needs fixing.
> 
> Luckily the polling task is very rarely used, by normal users.
> 
> 
>> 3. Error handling too weak
>>
>>     * We need to check the device responds to commands (say, w/
>>       IDENTIFY or CHK_POWER) after an error, and then reset if it
>>           doens't.  To do this, we need to handle all errors in EH.
> 
> 
> First you need to classify the error, then handle based on that 
> classification.
> 
> - DMA and PCI bus errors are usually indicated via status bits in 
> controller registers, such as ATA_DMA_ERR on standard PCI IDE controllers.
> 
> - Device errors will be indicated via the ATA Status and Error registers.
> 
> 
> - PCI bus errors should be handled by resetting the host controller (if 
> possible), and then retrying the command [NOTE: better suggestions welcome]
> 
> - DMA errors should be handled by hueristics:  If more than $N (3?) DMA 
> errors happen in 15 minutes,
> * decrease SATA PHY speed.  if speed cannot be decreased,
> * decrease UDMA xfer speed.  if at UDMA0, switch to PIO4
> * decrease PIO xfer speed.  if at PIO3, complain, but continue
> 
> Commands should be retried after DMA errors.
> 
> - Device errors should handled as per ATA specs.  Usually these error do 
> not cause a retry, but they may require additional inquiry such as READ 
> LOG EXT if its a media error.
> 
> 
>> 4. Better error reporting
>>
>>     * We currently depend on ATA_STAT and ATA_ERR register values
>>       to check for and report errors.  As Jeff said, this is way
>>       too crude.  We need better error reporting.  I think having
>>       unified code path for error handling will help implementing
>>       this.
> 
> 
> This will fall naturally out of better error handling (stuff I described 
> above).
> 
> 
>> 5. EH is currently holding off other improvements
>>
>>     * NCQ controllers (or any other non-legacy ATA interface based
>>           ones) don't fit nicely into current ATA error handling in
>>           interrupt scheme.  As NCQ errors require issuing read log
>>       command, we need to be in EH context to handle these errors.
>>
>>     * To properly implement hotplug, we need to have solid error
>>           handling.  IMHO, implementing hotplug with current EH will
>>       be quite fragile.
> 
> 
> Correct.  Both NCQ and hotplug really want decent error handling -- 
> particularly hotplug.  Hotplug will likely travel the error handling 
> paths, though at that point we prefer the English word "exception" 
> rather than "error"  :)
> 
> 
>>  Whether we choose to stay with ->eh_strategy_handler or move over to
>> fine-grained SCSI EH, IMHO, libata EH needs some work.  So... how
>> should we proceed?
> 
> 
> Well, moving over to the fine-grained hooks should make the remaining 
> problems Mark sees go away, as well as guaranteeing that we get command 
> completion right.
> 
> Staying with ->eh_strategy_handler() means auditing the entire SCSI 
> layer to check and see what gets set on error, which libata must then 
> manually reset.  Obviously there are still some bugs in this area, as 
> Mark has demonstrated.  The big reason why I don't like 
> ->eh_strategy_handler() is that it continues to be an unknown quantity, 
> in terms of, we don't still have a complete list of things-to-do during 
> error handling.

  As I wrote previously, I'm not very sure if Mark and I are looking at 
the same problem, but I think that once we know how the system is locked 
up, the debugging shouldn't be that difficult.

  As there are concerns regarding semantics of ->eh_strategy_handler and 
it's a less-used and less-charted territory, I'm gonna try to write a 
document describing the following.

  * How SCSI EH works and commands flow through it with the default
    fine-grained hooks.
  * From above, extract what ->eh_strategy_handler() should do.
  * What libata error conditions are there and how qc's should be
    handle.
  * How to integrate libata EH into SCSI EH without losing commands.

  I don't how good the doc will turn out (don't expect too much), but I 
hope it could serve as a basis for discussion if nothing else.

> If you are interested in tackling the work, you're more than welcome to 
> choose either path.  Either way, the work won't go to waste.
> 
>     Jeff

  After writing above mentioned doc, I'll try to improve/revise and 
break down my previously posted EH patchset and explain how they conform 
to above yet-to-be-written document such that it can be better 
understood and easier to review/debug.

  Thanks a lot.

-- 
tejun

Re: [SUMMARY] libata EH

[Alan Cox]

On Sul, 2005-08-21 at 02:31 +0900, Tejun Heo wrote:
>   My preference is toward unifying into single path as long as 
> performance penalty is acceptable for the sake of simplicity.

I don't think it is a big issue for ATA. The drive tends to take 10-15
seconds before it goes and whines at us, and it is hopefully not a fast
path.

> >>     * SCSI EH entrance is not synchronized with polling tasks.
> > 
> > 
> > Yes, this definitely needs fixing.
> > 
> > Luckily the polling task is very rarely used, by normal users.

The old IDE has this bug on reset paths too - and people do eventually
hit it.

> > - DMA errors should be handled by hueristics:  If more than $N (3?) DMA 
> > errors happen in 15 minutes,
> > * decrease SATA PHY speed.  if speed cannot be decreased,
> > * decrease UDMA xfer speed.  if at UDMA0, switch to PIO4
> > * decrease PIO xfer speed.  if at PIO3, complain, but continue

Remember this means issuing a command to the drive before re-issuing the
failed command. The old IDE also has serious problems with this as it
ends up trying to issue a polling command from an IRQ racing its own
timeout code. One good reason for the EH approach scsi takes of
quiescing first and running in task context.

Re: [SUMMARY] libata EH

[Jeff Garzik]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Simple stuff like "command aborted" (invalid command) can be handled 
>> immediately, no need to kick in the error handling.

>> But as long as the right hardware interrupts are acknowledged, I don't 
>> mind if all error handling is moved to the thread.

>  My preference is toward unifying into single path as long as 
> performance penalty is acceptable for the sake of simplicity.

The hot path is completing reads and writes successfully.
Secondary hot path is completing <other commands> successfully.

For everything else, clear, simple, maintainable code is preferred over 
fast code.


>>> 2. Synchronization
>>>
>>>     * SCSI EH entrance is not synchronized with normal processing.
>>>       ATAPI error handling/timeout handling can run concurrently
>>>       with normal command processing.  Albert, I think it's the
>>>       same problem you're trying to solve by moving ATA_QCFLAG_ACTIVE
>>>       clearing.
>>>
>>>       http://marc.theaimsgroup.com/?l=linux-ide&m=112417360223374&w=2
>>
>>
>>
>> The SCSI layer stops all command processing before calling 
>> ->eh_strategy_handler().  Where do you see that it runs concurrently 
>> with normal command processing?  That should definitely -not- be 
>> happening.
>>
> 
>  There are currently two problems.
> 
>  * As we don't grab host_set lock on entry to ata_scsi_error(), we can
>    run concurrently with latter part of ata_qc_complete().  This race is
>    addressed by the following patches I've just posted.
> 
>    http://marc.theaimsgroup.com/?l=linux-ide&m=112454734102242&w=2


hmmmmm.  I can see a bit of that:

When ->eh_strategy_handler() is called, the SCSI layer has stopped 
sending commands to all ports on the specified SCSI host.

However, it looks like we can race against
(a) interrupt handler completing a command on another port
(b) interrupt handler belatedly completing a command on our port
(c) if polling, another kernel thread

(a) shouldn't matter right now, but will in the future when we take a 
host-reset action that can 'blip' all ports.
(b) is a -very- rare worry in ATA, since commands that don't complete 
after 30 seconds probably will never complete.  But given how CHECK 
CONDITION is implemented in libata's ATAPI code, falling immediately 
over to the EH, this might be a real concern for ATAPI.
(c) was mentioned in previous emails.  A rare worry.

Did I miss anything?


>  * After entering EH, normal command completion or spurious interrupt
>    can occur.  We currently don't peg those interrupts, so interrupt
>    handling can interfere with EH.

As long as it is not the local port, it shouldn't interfere with EH 
(currently).


>  As there are concerns regarding semantics of ->eh_strategy_handler and 
> it's a less-used and less-charted territory, I'm gonna try to write a 
> document describing the following.
> 
>  * How SCSI EH works and commands flow through it with the default
>    fine-grained hooks.
>  * From above, extract what ->eh_strategy_handler() should do.
>  * What libata error conditions are there and how qc's should be
>    handle.
>  * How to integrate libata EH into SCSI EH without losing commands.
> 
>  I don't how good the doc will turn out (don't expect too much), but I 
> hope it could serve as a basis for discussion if nothing else.

It would certainly be nice to get all of this written down.


>  After writing above mentioned doc, I'll try to improve/revise and break 
> down my previously posted EH patchset and explain how they conform to 
> above yet-to-be-written document such that it can be better understood 
> and easier to review/debug.

Cool.  Thank you.

I'll get those patches reviewed sometime this weekend.

	Jeff