[PATCH 1/3] libata: flush is an IO command

[PATCH 1/3] libata: flush is an IO command

[Tejun Heo]

ATA_QCFLAG_IO is used to mark commands which are used to perform
regluar IO transfers via block layer.  These commands are assumed to
be valid and taken more seriously during error handling.  Cache flush
is used by regular IO path and necessary for data integrity.  Mark it
with ATA_QCFLAG_IO.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-scsi.c |    3 +++
 1 file changed, 3 insertions(+)

Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -1040,6 +1040,9 @@ static unsigned int ata_scsi_flush_xlat(
 	else
 		tf->command = ATA_CMD_FLUSH;
 
+	/* flush is critical for IO integrity, consider it an IO command */
+	qc->flags |= ATA_QCFLAG_IO;
+
 	return 0;
 }
 

[PATCH 2/3] libata: stop being overjealous about non-IO commands

[Tejun Heo]

libata EH always revalidated device and retried failed command after
error except for ATAPI CCs.  This is unnecessary and hinders with
users issuing direct commands.  This patch makes the following
changes.

* Make sata_sil24 not request ATA_EH_REVALIDATE on device errors.
  sil24 is the only driver which does this.  All others let libata EH
  core code decide.

* Don't request revalidation after device error of non-IO command.
  Revalidation doesn't really help anybody.  As ATA_EH_REVALIDATE
  isn't set by default, there's no reason to clear it after sense data
  is read.  Kill ATA_EH_REVALIDATE clearing code while at it.

* Don't retry non-IO command after device error.  Device has rejected
  the command.  There's no point in retrying.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c  |   16 +++++++++++-----
 drivers/ata/sata_sil24.c |    6 +++---
 2 files changed, 14 insertions(+), 8 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -1800,10 +1800,8 @@ static void ata_eh_link_autopsy(struct a
 			qc->err_mask &= ~AC_ERR_OTHER;
 
 		/* SENSE_VALID trumps dev/unknown error and revalidation */
-		if (qc->flags & ATA_QCFLAG_SENSE_VALID) {
+		if (qc->flags & ATA_QCFLAG_SENSE_VALID)
 			qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER);
-			ehc->i.action &= ~ATA_EH_REVALIDATE;
-		}
 
 		/* accumulate error info */
 		ehc->i.dev = qc->dev;
@@ -1816,7 +1814,8 @@ static void ata_eh_link_autopsy(struct a
 	if (ap->pflags & ATA_PFLAG_FROZEN ||
 	    all_err_mask & (AC_ERR_HSM | AC_ERR_TIMEOUT))
 		ehc->i.action |= ATA_EH_SOFTRESET;
-	else if (all_err_mask)
+	else if ((is_io && all_err_mask) ||
+		 (!is_io && (all_err_mask & ~AC_ERR_DEV)))
 		ehc->i.action |= ATA_EH_REVALIDATE;
 
 	/* if we have offending qcs and the associated failed device */
@@ -2674,8 +2673,15 @@ void ata_eh_finish(struct ata_port *ap)
 			/* FIXME: Once EH migration is complete,
 			 * generate sense data in this function,
 			 * considering both err_mask and tf.
+			 *
+			 * There's no point in retrying invalid
+			 * (detected by libata) and non-IO device
+			 * errors (rejected by device).  Finish them
+			 * immediately.
 			 */
-			if (qc->err_mask & AC_ERR_INVALID)
+			if ((qc->err_mask & AC_ERR_INVALID) ||
+			    (!(qc->flags & ATA_QCFLAG_IO) &&
+			     qc->err_mask == AC_ERR_DEV))
 				ata_eh_qc_complete(qc);
 			else
 				ata_eh_qc_retry(qc);
Index: work/drivers/ata/sata_sil24.c
===================================================================
--- work.orig/drivers/ata/sata_sil24.c
+++ work/drivers/ata/sata_sil24.c
@@ -265,11 +265,11 @@ static struct sil24_cerr_info {
 	unsigned int err_mask, action;
 	const char *desc;
 } sil24_cerr_db[] = {
-	[0]			= { AC_ERR_DEV, ATA_EH_REVALIDATE,
+	[0]			= { AC_ERR_DEV, 0,
 				    "device error" },
-	[PORT_CERR_DEV]		= { AC_ERR_DEV, ATA_EH_REVALIDATE,
+	[PORT_CERR_DEV]		= { AC_ERR_DEV, 0,
 				    "device error via D2H FIS" },
-	[PORT_CERR_SDB]		= { AC_ERR_DEV, ATA_EH_REVALIDATE,
+	[PORT_CERR_SDB]		= { AC_ERR_DEV, 0,
 				    "device error via SDB FIS" },
 	[PORT_CERR_DATA]	= { AC_ERR_ATA_BUS, ATA_EH_SOFTRESET,
 				    "error in data FIS" },

[PATCH 3/3] libata: implement and use ATA_QCFLAG_QUIET

[Tejun Heo]

Implement ATA_QCFLAG_QUIET which indicates that there's no need to
report if the command fails with AC_ERR_DEV and set it for passthrough
commands.

Combined with previous changes, this now makes device errors for all
direct commands reported directly to the issuer without going through
EH actions and reporting.

Note that EH is still invoked after non-IO device errors to determine
the nature of the error and resume command execution (some controller
requires special care after error to continue).  It just performs
default maintenance after error, examines what's going on, realizes
that it's none of its business and reports the command failure without
logging any error messages.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
 drivers/ata/libata-eh.c   |    4 +++-
 drivers/ata/libata-scsi.c |    4 ++--
 include/linux/libata.h    |    1 +
 3 files changed, 6 insertions(+), 3 deletions(-)

Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -1878,7 +1878,9 @@ static void ata_eh_link_report(struct at
 	for (tag = 0; tag < ATA_MAX_QUEUE; tag++) {
 		struct ata_queued_cmd *qc = __ata_qc_from_tag(ap, tag);
 
-		if (!(qc->flags & ATA_QCFLAG_FAILED) || qc->dev->link != link)
+		if (!(qc->flags & ATA_QCFLAG_FAILED) || qc->dev->link != link ||
+		    ((qc->flags & ATA_QCFLAG_QUIET) &&
+		     qc->err_mask == AC_ERR_DEV))
 			continue;
 		if (qc->flags & ATA_QCFLAG_SENSE_VALID && !qc->err_mask)
 			continue;
Index: work/drivers/ata/libata-scsi.c
===================================================================
--- work.orig/drivers/ata/libata-scsi.c
+++ work/drivers/ata/libata-scsi.c
@@ -2699,8 +2699,8 @@ static unsigned int ata_scsi_pass_thru(s
 	 */
 	qc->nbytes = scsi_bufflen(scmd);
 
-	/* request result TF */
-	qc->flags |= ATA_QCFLAG_RESULT_TF;
+	/* request result TF and be quiet about device error */
+	qc->flags |= ATA_QCFLAG_RESULT_TF | ATA_QCFLAG_QUIET;
 
 	return 0;
 
Index: work/include/linux/libata.h
===================================================================
--- work.orig/include/linux/libata.h
+++ work/include/linux/libata.h
@@ -218,6 +218,7 @@ enum {
 	ATA_QCFLAG_IO		= (1 << 3), /* standard IO command */
 	ATA_QCFLAG_RESULT_TF	= (1 << 4), /* result TF requested */
 	ATA_QCFLAG_CLEAR_EXCL	= (1 << 5), /* clear excl_link on completion */
+	ATA_QCFLAG_QUIET	= (1 << 6), /* don't report device error */
 
 	ATA_QCFLAG_FAILED	= (1 << 16), /* cmd failed and is owned by EH */
 	ATA_QCFLAG_SENSE_VALID	= (1 << 17), /* sense data valid */

Re: [PATCH 1/3] libata: flush is an IO command

[Jeff Garzik]

Tejun Heo wrote:
> ATA_QCFLAG_IO is used to mark commands which are used to perform
> regluar IO transfers via block layer.  These commands are assumed to
> be valid and taken more seriously during error handling.  Cache flush
> is used by regular IO path and necessary for data integrity.  Mark it
> with ATA_QCFLAG_IO.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
>  drivers/ata/libata-scsi.c |    3 +++
>  1 file changed, 3 insertions(+)

patch series looks fine... ok for 2.6.24-rc?

Re: [PATCH 1/3] libata: flush is an IO command

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> ATA_QCFLAG_IO is used to mark commands which are used to perform
>> regluar IO transfers via block layer.  These commands are assumed to
>> be valid and taken more seriously during error handling.  Cache flush
>> is used by regular IO path and necessary for data integrity.  Mark it
>> with ATA_QCFLAG_IO.
>>
>> Signed-off-by: Tejun Heo <htejun@gmail.com>
>> ---
>>  drivers/ata/libata-scsi.c |    3 +++
>>  1 file changed, 3 insertions(+)
> 
> patch series looks fine... ok for 2.6.24-rc?

Yeap, shouldn't be dangerous.  Thanks.

-- 
tejun