libata: improve FLUSH error handling

ata_tf_to_lba[48]() currently return LBA in tf + 1 for ata_read_native_max_address(). Make them return LBA and make it global so that it can be used to read LBA off TF for other purposes. ata_read_native_max_address() now adds 1 itself.

Signed-off-by Tejun Heo <[email protected]>

---
 drivers/ata/libata-core.c |   12 ++++++------
 drivers/ata/libata.h      |    2 ++
 2 files changed, 8 insertions(+), 6 deletions(-)

When errors occur during flush, instead of writing out the rest and reporting the errors, ATA halts the processing and report the error, so FLUSH must be retried on failure. Also, the spec says that FLUSH may take more than 30 secs but doesn't mention anything about the actual limit.

This patch improves FLUSH error handling such that...

1. It's always retried with longer timeout (60s for now) after failure.

2. If the device is making progress by retrying, humor it for longer (20 tries for now).

3. If the device is fails the command with the same failed sector, retry fewer times (log2 of the original number of tries).

4. If retried FLUSH fails for something other than device error, don't keep retrying. We're likely wasting time.

This patch is inspired by Alan's patch to improve ata_flush_cache().

Signed-off-by Tejun Heo <[email protected]>

Cc Alan Cox <[email protected]>

---
 drivers/ata/libata-eh.c |  131 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/libata.h  |    5 ++-
 2 files changed, 134 insertions(+), 2 deletions(-)

As the code is being smart against retrying needlessly, it won't be too dangerous to increase the 20 tries (taken from Alan's patch) but I think it's as good as any other random number. If anyone knows any meaningful number, please chime in. The same goes for 60 secs timeout too.

Absolute theoretical worst case for a drive with a buffer 4X the largest
current size: 328 seconds. Not taking into account having bad-sector
retries for each of those I/O blocks, but *nobody* is going to wait
that long anyway. They'll have long since pulled the power cord or reached for the BIG RED BUTTON.

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.

Mark Lord wrote:
..

Quoted 4 lines

Absolute theoretical worst case for a drive with a buffer 4X the largest
current size: 328 seconds. Not taking into account having bad-sector
retries for each of those I/O blocks, but *nobody* is going to wait
that long anyway. They'll have long since pulled the power cord or reached for the BIG RED BUTTON.

..

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.

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...

Cheers

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.

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...

Hello, Mark.

Quoted 5 lines · Mark Lord

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.

Quoted 7 lines

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.

Quoted 41 lines · Tejun Heo

Hello, Mark.

Quoted 5 lines · Mark Lord

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.

Quoted 7 lines

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.

..

With EH becoming more and more capable and complex,
a global deadline for FLUSH looks like a reasonable thing.
People who have no backups can leave it at the default "near-infinity" setting
that is there now, and folks with RAID1 (or better) can set it to a much shorter number -- so that their system-recovery reboot doesn't take 3 hours to get past the FLUSH_CACHE on the failing drive. :)

Cheers

I think that is a really important knob to have. Not just for RAID systems, but we use the FLUSH_CACHE on systems without barriers mainly when we power down & do the unmounts, etc.

If you hit a bad block during power down of a laptop, I can image that have a worst case of (30?) seconds is infinitely better than multiple minutes ;-)

Quoted 7 lines · Ric Wheeler

I think that is a really important knob to have. Not just for RAID systems, but we use the FLUSH_CACHE on systems without barriers mainly when we power down & do the unmounts, etc.

If you hit a bad block during power down of a laptop, I can image that have a worst case of (30?) seconds is infinitely better than multiple minutes ;-)

Fully finishing FLUSH CACHE requires command repetition. Not fully finishing FLUSH CACHE on shutdown means sure data loss. Given that FLUSH CACHE failure is very rare and it's repeatedly retried if and only if the device actively indicates failure, I'm not too sure. Also note that if FLUSH CACHE fails, you cannot even trust the FS journal. Things can get silently corrupt.

I do agree with the above, we should try to get the FLUSH done according to spec, I meant to argue that we should bound the time spent. If my laptop spends more than 30? 60? 120? seconds trying to flush a write cache, I will probably be looking for a way to force it to power down ;-)

It is also worth noting that most users of ext3 run without barriers enabled (and the drive write cache enabled) which means that we test this corruption path on any non-UPS power failure.

Quoted 1 line

I do agree with the above, we should try to get the FLUSH done according to spec, I meant to argue that we should bound the time spent. If my laptop spends more than 30? 60? 120? seconds trying to flush a write cache, I will probably be looking for a way to force it to power down ;-)

But if your PhD thesis is being written back you'd be different 8). I am not sure we can exceed 30 seconds, currently although we set 60 second I/O timeouts we are timing out at 30 seconds in some traces I get sent so something is messing up our timeout handling back to the default. I've tried tracing it and so far failed to figure it out.

Quoted 2 lines

It is also worth noting that most users of ext3 run without barriers enabled (and the drive write cache enabled) which means that we test this corruption path on any non-UPS power failure.

It is most unfortunate that distributions continue to ship that default.

Alan

Quoted 6 lines · Tejun Heo

As the code is being smart against retrying needlessly, it won't be too dangerous to increase the 20 tries (taken from Alan's patch) but I think it's as good as any other random number. If anyone knows any meaningful number, please chime in. The same goes for 60 secs timeout too.

..

I really think that we should enforce a strict upper limit on the time that can be spent inside the flush-cache near-infinite loop being introduced.

Some things rely on I/O completing or failing in a time deterministic manner.

Really, the entire flush + retries etc.. should never, ever, be permitted to take more than XX seconds total. Not 60 seconds per retry, but XX seconds total for the original command + however many retries we can fit in there.

As for the value of XX, well.. make it a sysfs attribute, with a default
of something "sensible". The time bounds is really dependent upon how
quickly the drive can empty its onboard cache, or how large a cache it has.

Figure the biggest drives will have no more than, say 64MB of cache for many years (biggest SATA drive now uses 16MB). Assuming near-worst case I/O size of 4KB, that's 16384 I/O operations, if none were adjacent on disk.

What's the average access time these days? Say.. 20ms worst case for any
drive with a cache that huge? That's unrealistically slow for data that's
already in the drive cache, but .. 16384 * .020 seconds = 328 seconds.

Absolute theoretical worst case for a drive with a buffer 4X the largest
current size: 328 seconds. Not taking into account having bad-sector
retries for each of those I/O blocks, but *nobody* is going to wait
that long anyway. They'll have long since pulled the power cord or reached for the BIG RED BUTTON.

On a 16MB cache drive, that number would be 328 / 4 = 82 seconds.

That's what I'd put for the limit.
But we could be slighly nonsensical and agree upon 120 seconds. :)

Cheers

I think that the 30 seconds was meant to be that worst case time for the drive to respond to a command. We try to push vendors to respond in much less time than that (it's important to get things like the fast fail path for RAID working correctly), say something like 10-15 seconds.

Quoted 1 line · Ric Wheeler

I think that the 30 seconds was meant to be that worst case time for the drive to respond to a command. We try to push vendors to respond in much less time than that (it's important to get things like the fast fail path for RAID working correctly), say something like 10-15 seconds.

Multiple vendors say the FLUSH CACHE worst case can exceed 30 seconds though...

Jeff

Really, the entire flush + retries etc.. should never, ever, be permitted to take more than XX seconds total. Not 60 seconds per retry, but XX seconds total for the original command + however many retries we can fit in there.

Quoted 4 lines · Mark Lord

Really, the entire flush + retries etc.. should never, ever, be permitted to take more than XX seconds total. Not 60 seconds per retry, but XX seconds total for the original command + however many retries we can fit in there.

I fully agree. Also please make that user limit settable use module_param too, not only sysfs. I recently had the pleasure to deal with a failing disk which started to spew errors already on device probing and it somewhat annoyed me that that slowed down boot up time that much. Perhaps the default during device probing should be also lower?

Regarding the boot time exception, which
version was it and do you have logs?

Quoted 2 lines · Tejun Heo

Regarding the boot time exception, which
version was it and do you have logs?

I tried first 2.6.25-rc4 and then 2.6.24 (similar behaviour).

My impression was also that the exception handling was very heavy handed -- is it really needed to always reset the link on each error?

I'll send you full logs (huge) in separate mail.

Hello, all.

I was going through mailbox and saw Alan's patch[A] which didn't get the love it deserved. It turned out that ata_flush_cache() function the patch modifies had been dead for some time. I ended up re-doing it in the EH framework and it turned out okay.

After the patchset, on FLUSH failure, the following is done.

1. It's always retried with longer timeout (60s for now) after failure.

2. If the device is making progress by retrying, humor it for longer

     (20 tries for now).

3. If the device is fails the command with the same failed sector, retry fewer times (log2 of the original number of tries).

4. If retried FLUSH fails for something other than device error, don't keep retrying. We're likely wasting time.

As the code is being smart against retrying needlessly, it won't be too dangerous to increase the 20 tries (taken from Alan's patch) but I think it's as good as any other random number. If anyone knows any meaningful number, please chime in. The same goes for 60 secs timeout too.

I made a debug patch to trigger timeouts and device errors on FLUSH. I'll post the patch as a reply. It adds the following four module params which can be written runtime via /sys/module/libata/parameters.

flush_dbg_do_timeout:
If non-zero value is written, the specfied number of FLUSHes will be timed out.

flush_dbg_do_deverr:
If non-zero value is written, the specfied number of FLUSHes will be terminated with device error.

flush_dbg_fail_sector:
The failed sector for the next deverr.

flush_dbg_fail_increment:
Number of sectors to add to fail_sector after each deverr.

I tested different scenarios and it all seems to work fine but it would be really great if someone can test this on a (hmmm....) live dying drive.

This patchet is for #upstream but generated on top of

#upstream-fixes (4cde32fc4b32e96a99063af3183acdfd54c563f0)
+ [1] libata: ATA_EHI_LPM should be ATA_EH_LPM

as there is a humongous patchset pending review #upstream. Once this gets acked, I'll move it over to #upstream. It shouldn't interfere too much anyway.

Thanks.

Are we sure that it is ever the right thing to do to reissue a flush command?

Are we sure that it is ever the right thing to do to reissue a flush command?

I am worried that this might be much closer to the media error class of device errors than something that will benefit from a retry of any type.

Also, I am unclear as to how we measure the progress of the device if the flush command has failed?

Quoted 2 lines

4. If retried FLUSH fails for something other than device error, don't keep retrying. We're likely wasting time.

Are we sure that it is ever the right thing to do to reissue a flush command?

I am worried that this might be much closer to the media error class of device errors than something that will benefit from a retry of any type.

Also, I am unclear as to how we measure the progress of the device if the flush command has failed?

Quoted 4 lines

as there is a humongous patchset pending review #upstream. Once this gets acked, I'll move it over to #upstream. It shouldn't interfere too much anyway.

We have access to a fair number of flaky drives, I can see if we can test some of this...

Yes by reporting the first failing sector. Probably it's better to test failed > last_failed than failed == last_failed.