[PATCH 0 of 4] DIF/DIX fixes for 2.6.29

[PATCH 0 of 4] DIF/DIX fixes for 2.6.29

[Martin K. Petersen]

Data integrity bug fixes for 2.6.29.

2 files changed, 19 insertions(+), 7 deletions(-)
drivers/scsi/scsi_lib.c |    9 ++++++++-
drivers/scsi/sd_dif.c   |   17 +++++++++++------

[PATCH 1 of 4] scsi: Fix error handling for DIF/DIX

[Martin K. Petersen]

Alan's recent cleanup of the I/O completion code broke DIX error
handling.  Also, we are now using EILSEQ to indicate integrity errors to
the upper layers (as opposed to regular EIO failures).  This allows
filesystems to inspect buffers and decide whether to retry the I/O.
Update scsi_io_completion() accordingly.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---
1 file changed, 8 insertions(+), 1 deletion(-)
drivers/scsi/scsi_lib.c |    9 ++++++++-



diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -963,6 +963,8 @@ void scsi_io_completion(struct scsi_cmnd
 		return;
 	this_count = blk_rq_bytes(req);
 
+	error = -EIO;
+
 	if (host_byte(result) == DID_RESET) {
 		/* Third party bus reset or reset for error recovery
 		 * reasons.  Just retry the command and see what
@@ -1004,13 +1006,18 @@ void scsi_io_completion(struct scsi_cmnd
 				/* This will issue a new 6-byte command. */
 				cmd->device->use_10_for_rw = 0;
 				action = ACTION_REPREP;
+			} else if (sshdr.asc == 0x10) /* DIX */ {
+				description = "Host Data Integrity Failure";
+				action = ACTION_FAIL;
+				error = -EILSEQ;
 			} else
 				action = ACTION_FAIL;
 			break;
 		case ABORTED_COMMAND:
 			if (sshdr.asc == 0x10) { /* DIF */
+				description = "Target Data Integrity Failure";
 				action = ACTION_FAIL;
-				description = "Data Integrity Failure";
+				error = -EILSEQ;
 			} else
 				action = ACTION_RETRY;
 			break;

[PATCH 2 of 4] sd: Show app tag on error

[Martin K. Petersen]

Add application tag to the output displayed on error.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---
1 file changed, 3 insertions(+), 2 deletions(-)
drivers/scsi/sd_dif.c |    5 +++--



diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -475,8 +475,9 @@ int sd_dif_prepare(struct request *rq, s
 
 error:
 	kunmap_atomic(sdt, KM_USER0);
-	sd_printk(KERN_ERR, sdkp, "%s: virt %u, phys %u, ref %u\n",
-		  __func__, virt, phys, be32_to_cpu(sdt->ref_tag));
+	sd_printk(KERN_ERR, sdkp, "%s: virt %u, phys %u, ref %u, app %4x\n",
+		  __func__, virt, phys, be32_to_cpu(sdt->ref_tag),
+		  be16_to_cpu(sdt->app_tag));
 
 	return -EIO;
 }

[PATCH 3 of 4] sd: Fix tagging on platforms with signed char

[Martin K. Petersen]

Switch tag arrays to u8 to prevent problems on platforms with signed
char.

Reported-by: Tim LaBerge <tim.laberge@Quantum.Com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---
1 file changed, 4 insertions(+), 4 deletions(-)
drivers/scsi/sd_dif.c |    8 ++++----



diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -142,7 +142,7 @@ static void sd_dif_type1_set_tag(void *p
 static void sd_dif_type1_set_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
+	u8 *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
@@ -154,7 +154,7 @@ static void sd_dif_type1_get_tag(void *p
 static void sd_dif_type1_get_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
+	u8 *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {
@@ -256,7 +256,7 @@ static void sd_dif_type3_set_tag(void *p
 static void sd_dif_type3_set_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
+	u8 *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 6, sdt++) {
@@ -269,7 +269,7 @@ static void sd_dif_type3_get_tag(void *p
 static void sd_dif_type3_get_tag(void *prot, void *tag_buf, unsigned int sectors)
 {
 	struct sd_dif_tuple *sdt = prot;
-	char *tag = tag_buf;
+	u8 *tag = tag_buf;
 	unsigned int i, j;
 
 	for (i = 0, j = 0 ; i < sectors ; i++, j += 2, sdt++) {

[PATCH 4 of 4] sd: Correctly handle 6-byte commands with DIX

[Martin K. Petersen]

DIF does not work with 6-byte commands so we previously ignored those
commands when preparing a request.  However, DIX does not need
RDPROTECT/WRPROTECT to be set and 6-byte commands are consequently
perfectly valid in host-only mode.

This patch fixes a problem where we would set the wrong DIX operation
when issuing commands to a legacy disk.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---
1 file changed, 4 insertions(+)
drivers/scsi/sd_dif.c |    4 ++++



diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -374,7 +374,10 @@ void sd_dif_op(struct scsi_cmnd *scmd, u
 	else
 		csum_convert = 0;
 
+	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));
+
 	switch (scmd->cmnd[0]) {
+	case READ_6:
 	case READ_10:
 	case READ_12:
 	case READ_16:
@@ -390,6 +393,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, u
 
 		break;
 
+	case WRITE_6:
 	case WRITE_10:
 	case WRITE_12:
 	case WRITE_16:

Re: [PATCH 4 of 4] sd: Correctly handle 6-byte commands with DIX

[Matthew Wilcox]

On Sun, Jan 04, 2009 at 03:04:34AM -0500, Martin K. Petersen wrote:
> @@ -374,7 +374,10 @@ void sd_dif_op(struct scsi_cmnd *scmd, u
>  	else
>  		csum_convert = 0;
>  
> +	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));

This BUG_ON gave me a 'Huh?' moment.  I immediately wondered if it was
user-triggerable.  The answer is "no, this is kosher".  sd_dif_op() is
only called from sd.c where it has chosen which READ_*/WRITE_* opcode to
set up, and it would indeed be an internal bug for this combination of
conditions to exist.

Reviewed-by: Matthew Wilcox <willy@linux.intel.com>

> +
>  	switch (scmd->cmnd[0]) {
> +	case READ_6:
>  	case READ_10:
>  	case READ_12:
>  	case READ_16:
> @@ -390,6 +393,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, u
>  
>  		break;
>  
> +	case WRITE_6:
>  	case WRITE_10:
>  	case WRITE_12:
>  	case WRITE_16:
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 4 of 4] sd: Correctly handle 6-byte commands with DIX

[Martin K. Petersen]

>>>>> "Matthew" == Matthew Wilcox <matthew@wil.cx> writes:

>> + BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] ==
>>  	WRITE_6));

Matthew> This BUG_ON gave me a 'Huh?' moment.  I immediately wondered if
Matthew> it was user-triggerable.  The answer is "no, this is kosher".
Matthew> sd_dif_op() is only called from sd.c where it has chosen which
Matthew> READ_*/WRITE_* opcode to set up, and it would indeed be an
Matthew> internal bug for this combination of conditions to exist.

Yep, I recently had to track down a bug where we had accidentally (for
completely different reasons) switched to 6-byte commands.  The code in
the completion path that triggered this condition is now gone, thanks to
Alan.  But I left the BUG_ON in place because it was no fun track this
down.

-- 
Martin K. Petersen	Oracle Linux Engineering