ata_eh_link_autopsy: Bug?

In libata-eh.c, the function ata_eh_link_autopsy() tries to decide if an operation is worth retrying.

I notice that for MEDIA ERRORs, it always sits there for minutes doing futile retries, when it's pretty obvious that the drive is reporting an uncorrectable error.

Is this code correct?

                /* determine whether the command is worth retrying */
                if (qc->flags & ATA_QCFLAG_IO ||
                    (!(qc->err_mask & AC_ERR_INVALID) &&
                     qc->err_mask != AC_ERR_DEV))
                        qc->flags |= ATA_QCFLAG_RETRY;

I think this part may be incorrect: qc->err_mask != AC_ERR_DEV Shouldn't that test be this instead: !(qc->err_mask & AC_ERR_DEV) ??

The mask is just about never exactly equal to AC_ERR_DEV.
In the case of a media error, it is (AC_ERR_DEV | AC_ERR_MEDIA) at this point.

Tejun?

MMmm.. even that isn't good enough, because the first ATA_QCFLAG_IO test bypasses the rest of that logic and triggers unconditional retries. Ugh.

Quoted 2 lines · Mark Lord

MMmm.. even that isn't good enough, because the first ATA_QCFLAG_IO test bypasses the rest of that logic and triggers unconditional retries. Ugh.

Hmmm... the unconditional retry on ATA_QCFLAG_IO is intenttional so that known good requests from FS are guaranteed to be retried no matter how whacky the underlying device is. I'm not sure whether that was a good decision tho. Maybe we should trust the hardware a bit more. So, I'm not necessarily against changing it.

Quoted 9 lines · Tejun Heo

Quoted 2 lines · Mark Lord

MMmm.. even that isn't good enough, because the first ATA_QCFLAG_IO test bypasses the rest of that logic and triggers unconditional retries. Ugh.

Hmmm... the unconditional retry on ATA_QCFLAG_IO is intenttional so that known good requests from FS are guaranteed to be retried no matter how whacky the underlying device is. I'm not sure whether that was a good decision tho. Maybe we should trust the hardware a bit more. So, I'm not necessarily against changing it.

With multi-terabyte drives being commonplace now, bad sectors seem to be a more frequent occurrence than I can remember from the past.

And when libata stumbles across a bad sector, it literally hangs the machine for _minutes_ doing retries. I have never seen a retry make any difference whatsoever on a bad sector read. New, old, or ancient hardware.

ATA and SATA drives have had built-in retries for media errors for as long as they've been commonplace in computers (early 1990s).

When libata stumbles across a bad sector, it can waste minutes sitting there doing retry after retry before finally giving up and letting the higher layers deal with it.

This patch removes retries for media errors only.

Signed-off-by Mark Lord <[email protected]>

---
version 2; the original patch changed more than intended.

Quoted 16 lines · Mark Lord

ATA and SATA drives have had built-in retries for media errors for as long as they've been commonplace in computers (early 1990s).

When libata stumbles across a bad sector, it can waste minutes sitting there doing retry after retry before finally giving up and letting the higher layers deal with it.

This patch removes retries for media errors only.

Signed-off-by Mark Lord <[email protected]>

---
version 2; the original patch changed more than intended.

drivers/ata/libata-eh.c line 2126

Quoted 6 lines

drivers/ata/libata-eh.c renamed from drivers/ata/libata-eh.c.orig +2 -1

@@ -2122,7 +2122,8 @@
 		if (qc->flags & ATA_QCFLAG_IO ||
 		    (!(qc->err_mask & AC_ERR_INVALID) &&
 		     qc->err_mask != AC_ERR_DEV))
-			qc->flags |= ATA_QCFLAG_RETRY;
+			if (!(qc->err_mask & AC_ERR_MEDIA))
+				qc->flags |= ATA_QCFLAG_RETRY;

It can be combined like the following,

	if (!(qc->err_mask & AC_ERR_MEDIA) &&
	    (qc->flags & ATA_QCFLAG_IO ||
	     (!(qc->err_mask & AC_ERR_INVALID) &&
	      qc->err_mask != AC_ERR_DEV)))

which doesn't look any prettier.

ATA and SATA drives have had built-in retries for media errors for as long as they've been commonplace in computers (early 1990s).

When libata stumbles across a bad sector, it can waste minutes sitting there doing retry after retry before finally giving up and letting the higher layers deal with it.

This patch removes retries for media errors only.

Signed-off-by Mark Lord <[email protected]>

---
version 3: try to improve readability.

drivers/ata/libata-eh.c +0 -0

@@ -2046,6 +2046,26 @@
 }
 

Quoted 18 lines · Mark Lord

ATA and SATA drives have had built-in retries for media errors for as long as they've been commonplace in computers (early 1990s).

When libata stumbles across a bad sector, it can waste minutes sitting there doing retry after retry before finally giving up and letting the higher layers deal with it.

This patch removes retries for media errors only.

Signed-off-by Mark Lord <[email protected]>

---
version 3: try to improve readability.

drivers/ata/libata-eh.c +0 -0

@@ -2046,6 +2046,26 @@
 }
 

drivers/ata/libata-eh.c line 2057

Quoted 10 lines

drivers/ata/libata-eh.c +9 -0

 /**
+ *	ata_eh_worth_retry - analyze error and decide whether to retry
+ *	@qc: qc to possibly retry
+ *
+ *	Look at the cause of the error and decide if a retry
+ * 	might be useful or not.  We don't want to retry media errors
+ *	because the drive itself has probably already taken 10-30 seconds
+ *	doing its own internal retries before reporting the failure.
+ */
+static inline int ata_eh_worth_retry(struct ata_queued_cmd *qc)

Return bool? && maybe split the patch into two - the first separating out the logic into a function, the latter changing emedia handling?

Quoted 4 lines

Quoted 1 line

+static inline int ata_eh_worth_retry(struct ata_queued_cmd *qc)

Return bool? && maybe split the patch into two - the first separating out the logic into a function, the latter changing emedia handling?

I think the two-liner from v2 is better.

Quoted 6 lines · Mark Lord

Quoted 4 lines

Quoted 1 line

+static inline int ata_eh_worth_retry(struct ata_queued_cmd *qc)

Return bool?&& maybe split the patch into two - the first separating out the logic into a function, the latter changing emedia handling?

I think the two-liner from v2 is better.

Heh, I don't know. It probably doesn't matter all that much either way. Let's let Jeff decide. ;)

Quoted 6 lines · Tejun Heo

Quoted 5 lines · Mark Lord

Quoted 4 lines

Quoted 1 line

+static inline int ata_eh_worth_retry(struct ata_queued_cmd *qc)

Return bool?&& maybe split the patch into two - the first separating out the logic into a function, the latter changing emedia handling?

Quoted 4 lines

Quoted 1 line

I think the two-liner from v2 is better.

Heh, I don't know. It probably doesn't matter all that much either way. Let's let Jeff decide. ;)

Queued v3. If anyone is motivated to make additional changes, base them on top of v3...