DIF/DIX fixes for 2.6.29

PATCH New Martin K. Petersen 2 participants 7 messages Jan 4, 2009 – Jan 5, 2009 SCSI

lore

Share & tools

lore

Martin K. Petersen

author

2009-01-04 08:22 UTC #0 lore raw

PATCH 1/1 DIF/DIX fixes for 2.6.29

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

Martin K. Petersen

author

2009-01-04 08:22 UTC #1 lore raw

PATCH 1/1 scsi: Fix error handling for DIF/DIX

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 <[email protected]>

---

1 file changed, 8 insertions(+), 1 deletion(-)

drivers/scsi/scsi_lib.c | 9 ++++++++-

drivers/scsi/scsi_lib.c +8 -1

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

Martin K. Petersen

author

2009-01-04 08:22 UTC #2 lore raw

PATCH 1/1 sd: Show app tag on error

In reply to Martin K. Petersen

Add application tag to the output displayed on error.

Signed-off-by Martin K. Petersen <[email protected]>

---

1 file changed, 3 insertions(+), 2 deletions(-)

drivers/scsi/sd_dif.c | 5 +++--

drivers/scsi/sd_dif.c +3 -2

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

Martin K. Petersen

author

2009-01-04 08:22 UTC #3 lore raw

PATCH 1/1 sd: Correctly handle 6-byte commands with DIX

In reply to 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 <[email protected]>

---

1 file changed, 4 insertions(+)

drivers/scsi/sd_dif.c | 4 ++++

drivers/scsi/sd_dif.c +4 -0

@@ -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:

Martin K. Petersen

author

2009-01-04 08:22 UTC #6 lore raw

PATCH 1/1 sd: Fix tagging on platforms with signed char

In reply to Martin K. Petersen

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

Reported-by Tim LaBerge <[email protected]>

Signed-off-by Martin K. Petersen <[email protected]>

---

1 file changed, 4 insertions(+), 4 deletions(-)

drivers/scsi/sd_dif.c | 8 ++++----

drivers/scsi/sd_dif.c +4 -4

@@ -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++) {

Matthew Wilcox

Reviewed-by on v1 1/1

2009-01-04 13:29 UTC #4 lore raw

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

In reply to Martin K. Petersen

drivers/scsi/sd_dif.c void sd_dif_op(struct scsi_cmnd *scmd, u · line 377

Quoted 5 lines · Martin K. Petersen

drivers/scsi/sd_dif.c +1 -0

@@ -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 <[email protected]>

Quoted 20 lines

drivers/scsi/sd_dif.c +3 -1
+
 	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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

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."

Martin K. Petersen

author

2009-01-05 02:35 UTC #5 lore raw

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

Quoted 4 lines

Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Matthew" == Matthew Wilcox <[email protected]> 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.

Signature · 1 line

Martin K. Petersen	Oracle Linux Engineering