Re: ata_eh_link_autopsy: Bug?

[Mark Lord <kernel@teksavvy.com>]

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?