Patchwork libata-eh don't waste time retrying media errors (v2)

login
register
mail settings
Submitter Mark Lord
Date May 2, 2012, 3:03 a.m.
Message ID <4FA0A3F7.7000401@teksavvy.com>
Download mbox | patch
Permalink /patch/156323/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Mark Lord - May 2, 2012, 3:03 a.m.
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.
Tejun Heo - May 2, 2012, 3:54 p.m.
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)))
Mark Lord - May 2, 2012, 7:10 p.m.
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.  :)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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