ata_eh_link_autopsy: Bug?

ata_eh_link_autopsy: Bug?

[Mark Lord]

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?

Re: ata_eh_link_autopsy: Bug?

[Mark Lord]

On 12-05-01 04:12 PM, Mark Lord wrote:
> 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)  ??

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.

So something like this perhaps:

                  /* 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;
+                         if (!(qc->err_mask & AC_ERR_MEDIA))
+                            qc->flags |= ATA_QCFLAG_RETRY;


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

Re: ata_eh_link_autopsy: Bug?

[Tejun Heo]

On Tue, May 01, 2012 at 04:27:00PM -0400, Mark Lord wrote:
> 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.

Thanks.

-- 
tejun

Re: ata_eh_link_autopsy: Bug?

[Mark Lord]

On 12-05-01 05:58 PM, Tejun Heo wrote:
> On Tue, May 01, 2012 at 04:27:00PM -0400, Mark Lord wrote:
>> 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.

Cheers

Re: ata_eh_link_autopsy: Bug?

[Mark Lord]

On 12-05-01 07:48 PM, Mark Lord wrote:
> On 12-05-01 05:58 PM, Tejun Heo wrote:
>> On Tue, May 01, 2012 at 04:27:00PM -0400, Mark Lord wrote:
>>> 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.


And as a reminder to anyone else listening in,
it's easier than you might think to test failure paths like this.

Here, I keep a few 300GB drives around just for that purpose,
and use "hdparm --make-bad-sector" on them to inject media errors
at specific places on the disk or filesystem.

Cheers

[PATCH] libata-eh don't waste time retrying media errors

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

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 <mlord@pobox.com>

--- linux-3.3.4/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-01 19:53:55.586811975 -0400
@@ -2121,8 +2121,9 @@
 		/* 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;
+		     !(qc->err_mask & AC_ERR_DEV)))
+			if (!(qc->err_mask & AC_ERR_MEDIA))
+				qc->flags |= ATA_QCFLAG_RETRY;

 		/* accumulate error info */
 		ehc->i.dev = qc->dev;

[-- Attachment #2: 02_libata_sata_dont_retry_media_errors.patch --]
[-- Type: text/x-patch, Size: 662 bytes --]

Prevent libata from retrying bad sectors.
Otherwise a single bad sector gets stuck here for minutes at a time.

--- linux-3.3.4/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-01 19:53:55.586811975 -0400
@@ -2121,8 +2121,9 @@
 		/* 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;
+		     !(qc->err_mask & AC_ERR_DEV)))
+			if (!(qc->err_mask & AC_ERR_MEDIA))
+				qc->flags |= ATA_QCFLAG_RETRY;
 
 		/* accumulate error info */
 		ehc->i.dev = qc->dev;

[PATCH] libata-eh don't waste time retrying media errors (v2)

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 912 bytes --]

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 <mlord@pobox.com>
---
version 2; the original patch changed more than intended.

--- linux/drivers/ata/libata-eh.c.orig	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-01 22:40:00.182425015 -0400
@@ -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;

 		/* accumulate error info */
 		ehc->i.dev = qc->dev;

[-- Attachment #2: 02_libata_sata_dont_retry_media_errors.patch --]
[-- Type: text/x-patch, Size: 453 bytes --]

--- linux/drivers/ata/libata-eh.c.orig	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-01 22:40:00.182425015 -0400
@@ -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;
 
 		/* accumulate error info */
 		ehc->i.dev = qc->dev;

Re: [PATCH] libata-eh don't waste time retrying media errors (v2)

[Tejun Heo]

On Tue, May 01, 2012 at 11:03:19PM -0400, Mark Lord wrote:
> 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 <mlord@pobox.com>
> ---
> version 2; the original patch changed more than intended.
> 
> --- linux/drivers/ata/libata-eh.c.orig	2012-04-27 13:17:35.000000000 -0400
> +++ linux/drivers/ata/libata-eh.c	2012-05-01 22:40:00.182425015 -0400
> @@ -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.  Hmm... maybe using local vars would
make it better?

	bool emedia = qc->err_mask & AC_ERR_MEDIA;
	bool einval = qc->err_mask & AC_ERR_INVALID;
	bool edev = qc->err_mask == AC_ERR_DEV;
	bool is_io = qc->flags & ATA_QCFLAG_IO;

	if (!emedia && (is_io || (!einval && !edev)))

-- 
tejun

Re: [PATCH] libata-eh don't waste time retrying media errors (v2)

[Mark Lord]

On 12-05-02 11:54 AM, Tejun Heo wrote:
> On Tue, May 01, 2012 at 11:03:19PM -0400, Mark Lord wrote:
>> 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 <mlord@pobox.com>
>> ---
>> version 2; the original patch changed more than intended.
>>
>> --- linux/drivers/ata/libata-eh.c.orig	2012-04-27 13:17:35.000000000 -0400
>> +++ linux/drivers/ata/libata-eh.c	2012-05-01 22:40:00.182425015 -0400
>> @@ -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.

Yeah, I considered that.  But really a bit main reason it looks fugly
is the historical insistence on wrapping lines longer than 80 chars.

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

Anything else I try there ends up just as ugly.  :)

[PATCH] libata-eh don't waste time retrying media errors (v3)

[Mark Lord]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

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 <mlord@pobox.com>
---
version 3: try to improve readability.

--- old/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-02 15:20:19.946827031 -0400
@@ -2046,6 +2046,26 @@
 }

 /**
+ *	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)
+{
+	if (qc->flags & AC_ERR_MEDIA)
+		return 0;	/* don't retry media errors */
+	if (qc->flags & ATA_QCFLAG_IO)
+		return 1;	/* otherwise retry anything from fs stack */
+	if (qc->err_mask & AC_ERR_INVALID)
+		return 0;	/* don't retry these */
+	return qc->err_mask != AC_ERR_DEV;  /* retry if not dev error */
+}
+
+/**
  *	ata_eh_link_autopsy - analyze error and determine recovery action
  *	@link: host link to perform autopsy on
  *
@@ -2119,9 +2139,7 @@
 			qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER);

 		/* 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))
+		if (ata_eh_worth_retry(qc))
 			qc->flags |= ATA_QCFLAG_RETRY;

 		/* accumulate error info */

[-- Attachment #2: 02_libata_sata_dont_retry_media_errors.patch --]
[-- Type: text/x-patch, Size: 1385 bytes --]

--- old/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
+++ linux/drivers/ata/libata-eh.c	2012-05-02 15:20:19.946827031 -0400
@@ -2046,6 +2046,26 @@
 }
 
 /**
+ *	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)
+{
+	if (qc->flags & AC_ERR_MEDIA)
+		return 0;	/* don't retry media errors */
+	if (qc->flags & ATA_QCFLAG_IO)
+		return 1;	/* otherwise retry anything from fs stack */
+	if (qc->err_mask & AC_ERR_INVALID)
+		return 0;	/* don't retry these */
+	return qc->err_mask != AC_ERR_DEV;  /* retry if not dev error */
+}
+
+/**
  *	ata_eh_link_autopsy - analyze error and determine recovery action
  *	@link: host link to perform autopsy on
  *
@@ -2119,9 +2139,7 @@
 			qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER);
 
 		/* 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))
+		if (ata_eh_worth_retry(qc))
 			qc->flags |= ATA_QCFLAG_RETRY;
 
 		/* accumulate error info */

Re: [PATCH] libata-eh don't waste time retrying media errors (v3)

[Tejun Heo]

On Wed, May 02, 2012 at 03:22:52PM -0400, Mark Lord wrote:
> 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 <mlord@pobox.com>
> ---
> version 3: try to improve readability.
> 
> --- old/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
> +++ linux/drivers/ata/libata-eh.c	2012-05-02 15:20:19.946827031 -0400
> @@ -2046,6 +2046,26 @@
>  }
> 
>  /**
> + *	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?

> +{
> +	if (qc->flags & AC_ERR_MEDIA)
> +		return 0;	/* don't retry media errors */
> +	if (qc->flags & ATA_QCFLAG_IO)
> +		return 1;	/* otherwise retry anything from fs stack */
> +	if (qc->err_mask & AC_ERR_INVALID)
> +		return 0;	/* don't retry these */
> +	return qc->err_mask != AC_ERR_DEV;  /* retry if not dev error */
> +}

Other than that,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH] libata-eh don't waste time retrying media errors (v3)

[Mark Lord]

On 12-05-02 03:33 PM, Tejun Heo wrote:
> On Wed, May 02, 2012 at 03:22:52PM -0400, Mark Lord wrote:
>> 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 <mlord@pobox.com>
>> ---
>> version 3: try to improve readability.
>>
>> --- old/drivers/ata/libata-eh.c	2012-04-27 13:17:35.000000000 -0400
>> +++ linux/drivers/ata/libata-eh.c	2012-05-02 15:20:19.946827031 -0400
>> @@ -2046,6 +2046,26 @@
>>  }
>>
>>  /**
>> + *	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?

I think the two-liner from v2 is better.

Cheers

Re: [PATCH] libata-eh don't waste time retrying media errors (v3)

[Tejun Heo]

On Wed, May 02, 2012 at 03:43:05PM -0400, Mark Lord wrote:
> >> +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. ;)

Thanks.

-- 
tejun

Re: [PATCH] libata-eh don't waste time retrying media errors (v3)

[Jeff Garzik]

On 05/02/2012 03:46 PM, Tejun Heo wrote:
> On Wed, May 02, 2012 at 03:43:05PM -0400, Mark Lord wrote:
>>>> +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. ;)

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

Re: [PATCH] libata-eh don't waste time retrying media errors (v3)

[Mark Lord]

On 12-05-03 10:25 PM, Jeff Garzik wrote:
> On 05/02/2012 03:46 PM, Tejun Heo wrote:
>> On Wed, May 02, 2012 at 03:43:05PM -0400, Mark Lord wrote:
>>>>> +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. ;)
> 
> Queued v3.  If anyone is motivated to make additional changes, base them on top of v3...


Peachy.  Thanks, Jeff!