Re: [SUMMARY] libata EH

[Tejun Heo <htejun@gmail.com>]

  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