Re: [PATCHSET #upstream] libata: improve FLUSH error handling

[Tejun Heo <htejun@gmail.com>]

Hello, Mark.

Mark Lord wrote:
> Speaking of which.. these are all WRITEs.
> 
> In 18 years of IDE/ATA development,
> I have *never* seen a hard disk drive report a WRITE error.
>
> Which makes sense, if you think about it -- it's rewriting the sector
> with new ECC info, so it *should* succeed.  The only case where it won't,
> is if the sector has been marked as "bad" internally, and the drive is
> too dumb to try anyways after it runs out of remap space.
> 
> In which case we've already lost data, and taking more than a hundred
> and twenty seconds isn't going to make a serious difference.

Yeah, the disk must be knee deep in shit to report WRITE failure.  I
don't really expect the code to be exercised often but was mainly trying
fill the loophole in libata error handling as this type of behavior is
what the spec requires on FLUSH errors.

I didn't add global timeout because retries are done iff the drive is
reporting progress.

1. Drives genuinely deep in shit and getting lots of WRITE errors would
report different sectors on each FLUSH and we NEED to keep retrying.
That's what the spec requires and the FLUSH could be from shutdown and
if so that would be the drive's last chance to write data to the drive.

2. There are other issues causing the command to fail (e.g. timeout, HSM
violation or somesuch).  This is the case EH can take a really long time
if it keeps retrying but the posted code doesn't retry if this is the case.

3. The drive is crazy and reporting errors for no good reason.  Unless
the drive is really anti-social and raise such error condition only
after tens of seconds, this shouldn't take too long.  Also, if LBA
doesn't change for each retry, the tries count is halved.

So, I think the code should be safe.  Do you still think we need a
global timeout?  It is easy to add.  I'm just not sure whether we need
it or not.

> Mmm.. anyone got a spare modern-ish drive to risk destroying?
> Say, one of the few still-functioning DeathStars, or an buggy-NCQ Maxtor ?
> 
> If so, it might be fun to try and produce a no-more-remaps scenario on it.
> One could use "hdparm --make-bad-sector" to corrupt a few hundred/thousand
> sectors in a row (sequentially numbered).
>
> Then loop and attempt to read from them individually with "hdparm
> --read-sector"
> (should fail on all, but it might force the drive to remap them).
> 
> Then finally try and write back to them with "hdparm --write-sector",
> and see if a WRITE ERROR is ever reported.  Maybe time the individual
> WRITEs
> to see if any of them take more than a few milliseconds.
> 
> Perhaps try this whole thing with/without the write cache enabled.
> 
> Mmm...

Heh... :-)

-- 
tejun