DIF/DIX updates for 2.6.32

DIF/DIX updates for 2.6.32

[Martin K. Petersen]

Updates to the DIF/DIX code for 2.6.32:

 - Add support for 32-byte CDBs to SCSI layer

 - Deprecate the DIX CONVERT ops

 - Detach pure DIF support from CONFIG_BLK_DEV_INTEGRITY

 - Add support for DIF Type 2 devices in sd

 - Add DIF Type 2 support to scsi_debug

[PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
bigger command buffer.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_lib.c    |   36 +++++++++++++++++++++++++++++++++++-
 include/scsi/scsi.h        |    1 +
 include/scsi/scsi_device.h |    1 +
 3 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 662024d..fc752b5 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
 
 struct kmem_cache *scsi_sdb_cache;
 
+struct kmem_cache *cdb_cache;
+mempool_t *cdb_pool;
+
 static void scsi_run_queue(struct request_queue *q);
 
 /*
@@ -642,6 +645,10 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
 
 	if (scsi_prot_sg_count(cmd))
 		scsi_free_sgtable(cmd->prot_sdb);
+
+	if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
+		mempool_free(cmd->cmnd, cdb_pool);
+
 }
 
 /*
@@ -1109,7 +1116,18 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
 	if (unlikely(!cmd))
 		return BLKPREP_DEFER;
 
-	memset(cmd->cmnd, 0, BLK_MAX_CDB);
+	if (sdev->use_32_for_rw) {
+		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
+
+		if (unlikely(!req->cmd))
+			return BLKPREP_DEFER;
+
+		cmd->cmnd = req->cmd;
+		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
+		memset(cmd->cmnd, 0, cmd->cmd_len);
+	} else
+		memset(cmd->cmnd, 0, BLK_MAX_CDB);
+
 	return scsi_init_io(cmd, GFP_ATOMIC);
 }
 EXPORT_SYMBOL(scsi_setup_fs_cmnd);
@@ -1745,6 +1763,19 @@ int __init scsi_init_queue(void)
 		}
 	}
 
+	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
+				      0, 0, NULL);
+	if (!cdb_cache) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
+		goto cleanup_sdb;
+	}
+
+	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
+	if (!cdb_pool) {
+		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
+		goto cleanup_sdb;
+	}
+
 	return 0;
 
 cleanup_sdb:
@@ -1771,6 +1802,9 @@ void scsi_exit_queue(void)
 		mempool_destroy(sgp->pool);
 		kmem_cache_destroy(sgp->slab);
 	}
+
+	mempool_destroy(cdb_pool);
+	kmem_cache_destroy(cdb_cache);
 }
 
 /**
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 084478e..a7ae10a 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -138,6 +138,7 @@ struct scsi_cmnd;
  *	SCSI command lengths
  */
 
+#define SCSI_EXT_CDB_SIZE 32
 #define SCSI_MAX_VARLEN_CDB_SIZE 260
 
 /* defined in T10 SCSI Primary Commands-2 (SPC2) */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3f566af..de2966b 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -129,6 +129,7 @@ struct scsi_device {
 				 * this device */
 	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
 				     * because we did a bus reset. */
+	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
 	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
 	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
 	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */
-- 
1.6.0.6

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Boaz Harrosh]

On 08/26/2009 09:17 AM, Martin K. Petersen wrote:
> DIF Type 2 drives require 32-byte commands.  Add a suitable flag to
> struct scsi_device that will cause scsi_setup_fs_cmnd() to allocate a
> bigger command buffer.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c    |   36 +++++++++++++++++++++++++++++++++++-
>  include/scsi/scsi.h        |    1 +
>  include/scsi/scsi_device.h |    1 +
>  3 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 662024d..fc752b5 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -67,6 +67,9 @@ static struct scsi_host_sg_pool scsi_sg_pools[] = {
>  
>  struct kmem_cache *scsi_sdb_cache;
>  
> +struct kmem_cache *cdb_cache;
> +mempool_t *cdb_pool;
> +
>  static void scsi_run_queue(struct request_queue *q);
>  
>  /*
> @@ -642,6 +645,10 @@ static void __scsi_release_buffers(struct scsi_cmnd *cmd, int do_bidi_check)
>  
>  	if (scsi_prot_sg_count(cmd))
>  		scsi_free_sgtable(cmd->prot_sdb);
> +
> +	if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
> +		mempool_free(cmd->cmnd, cdb_pool);
> +

This will not work. The cmd->cmnd might happen to be form an upper
layer through bsg or blk_execute_xxx and might just be SCSI_EXT_CDB_SIZE.

We could say to maybe:
	if (cmd->cmnd != cmd->req->cmd)

which means scsi allocated the buffer and not use the block supplied one
but that might be dangerous and will need a fat comment that states that
scsi drivers must use the scsi_cmnd pointer and not the request's.
Alternatively you could:

	if ((cmd->cmd_len == SCSI_EXT_CDB_SIZE) && blk_fs_request(cmnd->req))

since variable length can only come BLOCK_PC currently. But this will need to change
again later when ULDs might want to issue other commands that are SCSI_EXT_CDB_SIZE
in the future, which again might be dangerous.

or use an extra flag. You could use the first "fat comment" option above, all the drivers
I know do only use the scsi_cmnd->cmnd pointer.

>  }
>  
>  /*
> @@ -1109,7 +1116,18 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
>  	if (unlikely(!cmd))
>  		return BLKPREP_DEFER;
>  
> -	memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +	if (sdev->use_32_for_rw) {
> +		req->cmd = mempool_alloc(cdb_pool, GFP_ATOMIC);
> +

James could we get that live-lock problem we had when not
atomically allocating the scsi_cmnd, (when sense was separated
out)

> +		if (unlikely(!req->cmd))
> +			return BLKPREP_DEFER;
> +
> +		cmd->cmnd = req->cmd;
> +		cmd->cmd_len = req->cmd_len = SCSI_EXT_CDB_SIZE;
> +		memset(cmd->cmnd, 0, cmd->cmd_len);
> +	} else
> +		memset(cmd->cmnd, 0, BLK_MAX_CDB);
> +
>  	return scsi_init_io(cmd, GFP_ATOMIC);
>  }
>  EXPORT_SYMBOL(scsi_setup_fs_cmnd);
> @@ -1745,6 +1763,19 @@ int __init scsi_init_queue(void)
>  		}
>  	}
>  
> +	cdb_cache = kmem_cache_create("scsi_cdb_32", SCSI_EXT_CDB_SIZE,
> +				      0, 0, NULL);
> +	if (!cdb_cache) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb cache\n");
> +		goto cleanup_sdb;
> +	}
> +
> +	cdb_pool = mempool_create_slab_pool(SG_MEMPOOL_SIZE, cdb_cache);
> +	if (!cdb_pool) {
> +		printk(KERN_ERR "SCSI: can't init scsi cdb pool\n");
> +		goto cleanup_sdb;
> +	}
> +
>  	return 0;
>  
>  cleanup_sdb:
> @@ -1771,6 +1802,9 @@ void scsi_exit_queue(void)
>  		mempool_destroy(sgp->pool);
>  		kmem_cache_destroy(sgp->slab);
>  	}
> +
> +	mempool_destroy(cdb_pool);
> +	kmem_cache_destroy(cdb_cache);
>  }
>  
>  /**
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index 084478e..a7ae10a 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -138,6 +138,7 @@ struct scsi_cmnd;
>   *	SCSI command lengths
>   */
>  
> +#define SCSI_EXT_CDB_SIZE 32
>  #define SCSI_MAX_VARLEN_CDB_SIZE 260
>  
>  /* defined in T10 SCSI Primary Commands-2 (SPC2) */
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 3f566af..de2966b 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -129,6 +129,7 @@ struct scsi_device {
>  				 * this device */
>  	unsigned expecting_cc_ua:1; /* Expecting a CHECK_CONDITION/UNIT_ATTN
>  				     * because we did a bus reset. */
> +	unsigned use_32_for_rw:1; /* use 32-byte read / write (DIF Type 2) */
>  	unsigned use_10_for_rw:1; /* first try 10-byte read / write */
>  	unsigned use_10_for_ms:1; /* first try 10-byte mode sense/select */
>  	unsigned skip_ms_page_8:1;	/* do not use MODE SENSE page 0x08 */


Boaz

Re: [PATCH 1/5] SCSI: Add support for 32-byte CDBs

[Martin K. Petersen]

>> + if (cmd->cmd_len == SCSI_EXT_CDB_SIZE)
>> + 	mempool_free(cmd->cmnd, cdb_pool);
>> +

Boaz> This will not work. The cmd->cmnd might happen to be form an upper
Boaz> layer through bsg or blk_execute_xxx and might just be
Boaz> SCSI_EXT_CDB_SIZE.

This patch has been kicking around in my tree for a while and it was a
huge, gnarly mess.  I trimmed it down a lot before sending it out
yesterday and forgot why I originally had an allocation flag in
scsi_cmnd.  I think I'll reinstate that just to play it safe.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 2/5] SCSI: Deprecate SCSI_PROT_*_CONVERT operations

[Martin K. Petersen]

The checksum format is orthogonal to whether the protection information
is being passed on beyond the HBA or not.  It is perfectly valid to use
a non-T10 CRC with WRITE_STRIP and READ_INSERT.

Consequently it no longer makes sense to explicitly refer to the
conversion in the protection operation.  Update sd_dif and lpfc
accordingly.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/lpfc/lpfc_scsi.c |   15 +++------------
 drivers/scsi/sd_dif.c         |   20 ++++----------------
 include/scsi/scsi_cmnd.h      |    4 ----
 3 files changed, 7 insertions(+), 32 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
index da59c4f..ad75e19 100644
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -56,8 +56,6 @@ static char *dif_op_str[] = {
 	"SCSI_PROT_WRITE_INSERT",
 	"SCSI_PROT_READ_PASS",
 	"SCSI_PROT_WRITE_PASS",
-	"SCSI_PROT_READ_CONVERT",
-	"SCSI_PROT_WRITE_CONVERT"
 };
 static void
 lpfc_release_scsi_buf_s4(struct lpfc_hba *phba, struct lpfc_scsi_buf *psb);
@@ -1131,13 +1129,11 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
 			ret_prof = LPFC_PROF_A1;
 			break;
 
-		case SCSI_PROT_READ_CONVERT:
-		case SCSI_PROT_WRITE_CONVERT:
+		case SCSI_PROT_READ_PASS:
+		case SCSI_PROT_WRITE_PASS:
 			ret_prof = LPFC_PROF_AST1;
 			break;
 
-		case SCSI_PROT_READ_PASS:
-		case SCSI_PROT_WRITE_PASS:
 		case SCSI_PROT_NORMAL:
 		default:
 			printk(KERN_ERR "Bad op/guard:%d/%d combination\n",
@@ -1157,8 +1153,6 @@ lpfc_sc_to_sli_prof(struct scsi_cmnd *sc)
 			ret_prof = LPFC_PROF_C1;
 			break;
 
-		case SCSI_PROT_READ_CONVERT:
-		case SCSI_PROT_WRITE_CONVERT:
 		case SCSI_PROT_READ_INSERT:
 		case SCSI_PROT_WRITE_STRIP:
 		case SCSI_PROT_NORMAL:
@@ -1209,8 +1203,7 @@ lpfc_get_cmd_dif_parms(struct scsi_cmnd *sc, uint16_t *apptagmask,
 	static int cnt;
 
 	if (protcnt && (op == SCSI_PROT_WRITE_STRIP ||
-				op == SCSI_PROT_WRITE_PASS ||
-				op == SCSI_PROT_WRITE_CONVERT)) {
+				op == SCSI_PROT_WRITE_PASS)) {
 
 		cnt++;
 		spt = page_address(sg_page(scsi_prot_sglist(sc))) +
@@ -1501,8 +1494,6 @@ lpfc_prot_group_type(struct lpfc_hba *phba, struct scsi_cmnd *sc)
 	case SCSI_PROT_WRITE_STRIP:
 	case SCSI_PROT_READ_PASS:
 	case SCSI_PROT_WRITE_PASS:
-	case SCSI_PROT_WRITE_CONVERT:
-	case SCSI_PROT_READ_CONVERT:
 		ret = LPFC_PG_TYPE_DIF_BUF;
 		break;
 	default:
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 82f14a9..84224dd 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -364,15 +364,9 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
  */
 void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsigned int type)
 {
-	int csum_convert, prot_op;
+	int prot_op;
 
-	prot_op = 0;
-
-	/* Convert checksum? */
-	if (scsi_host_get_guard(scmd->device->host) != SHOST_DIX_GUARD_CRC)
-		csum_convert = 1;
-	else
-		csum_convert = 0;
+	prot_op = SCSI_PROT_NORMAL;
 
 	BUG_ON(dif && (scmd->cmnd[0] == READ_6 || scmd->cmnd[0] == WRITE_6));
 
@@ -382,10 +376,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
 	case READ_12:
 	case READ_16:
 		if (dif && dix)
-			if (csum_convert)
-				prot_op = SCSI_PROT_READ_CONVERT;
-			else
-				prot_op = SCSI_PROT_READ_PASS;
+			prot_op = SCSI_PROT_READ_PASS;
 		else if (dif && !dix)
 			prot_op = SCSI_PROT_READ_STRIP;
 		else if (!dif && dix)
@@ -398,10 +389,7 @@ void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsig
 	case WRITE_12:
 	case WRITE_16:
 		if (dif && dix)
-			if (csum_convert)
-				prot_op = SCSI_PROT_WRITE_CONVERT;
-			else
-				prot_op = SCSI_PROT_WRITE_PASS;
+			prot_op = SCSI_PROT_WRITE_PASS;
 		else if (dif && !dix)
 			prot_op = SCSI_PROT_WRITE_INSERT;
 		else if (!dif && dix)
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 3878d1d..a5e885a 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -229,10 +229,6 @@ enum scsi_prot_operations {
 	/* OS-HBA: Protected, HBA-Target: Protected */
 	SCSI_PROT_READ_PASS,
 	SCSI_PROT_WRITE_PASS,
-
-	/* OS-HBA: Protected, HBA-Target: Protected, checksum conversion */
-	SCSI_PROT_READ_CONVERT,
-	SCSI_PROT_WRITE_CONVERT,
 };
 
 static inline void scsi_set_prot_op(struct scsi_cmnd *scmd, unsigned char op)
-- 
1.6.0.6

[PATCH 3/5] sd: Detach DIF from block integrity infrastructure

[Martin K. Petersen]

So far we have only issued DIF commands if CONFIG_BLK_DEV_INTEGRITY is
enabled.  However, communication between initiator and target should be
independent of protection information DMA.  There are DIF-only host
adapters coming out that will be able to take advantage of this.

Move the relevant DIF bits to sd.c.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c        |   61 +++++++++++++++++++++++++++++++--------------
 drivers/scsi/sd.h        |    4 ---
 drivers/scsi/sd_dif.c    |   53 ----------------------------------------
 include/scsi/scsi_host.h |   15 ++++++++---
 4 files changed, 53 insertions(+), 80 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b7b9fec..36e2333 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -370,6 +370,31 @@ static void scsi_disk_put(struct scsi_disk *sdkp)
 	mutex_unlock(&sd_ref_mutex);
 }
 
+static void sd_prot_op(struct scsi_cmnd *scmd, unsigned int dif)
+{
+	unsigned int prot_op = SCSI_PROT_NORMAL;
+	unsigned int dix = scsi_prot_sg_count(scmd);
+
+	if (scmd->sc_data_direction == DMA_FROM_DEVICE) {
+		if (dif && dix)
+			prot_op = SCSI_PROT_READ_PASS;
+		else if (dif && !dix)
+			prot_op = SCSI_PROT_READ_STRIP;
+		else if (!dif && dix)
+			prot_op = SCSI_PROT_READ_INSERT;
+	} else {
+		if (dif && dix)
+			prot_op = SCSI_PROT_WRITE_PASS;
+		else if (dif && !dix)
+			prot_op = SCSI_PROT_WRITE_INSERT;
+		else if (!dif && dix)
+			prot_op = SCSI_PROT_WRITE_STRIP;
+	}
+
+	scsi_set_prot_op(scmd, prot_op);
+	scsi_set_prot_type(scmd, dif);
+}
+
 /**
  *	sd_init_command - build a scsi (read or write) command from
  *	information in the request structure.
@@ -578,8 +603,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 
 	/* If DIF or DIX is enabled, tell HBA how to handle request */
 	if (host_dif || scsi_prot_sg_count(SCpnt))
-		sd_dif_op(SCpnt, host_dif, scsi_prot_sg_count(SCpnt),
-			  sdkp->protection_type);
+		sd_prot_op(SCpnt, host_dif);
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
@@ -1238,34 +1262,33 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 	u8 type;
 
 	if (scsi_device_protection(sdp) == 0 || (buffer[12] & 1) == 0)
-		type = 0;
-	else
-		type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
+		return;
+
+	type = ((buffer[12] >> 1) & 7) + 1; /* P_TYPE 0 = Type 1 */
+
+	if (type == sdkp->protection_type || !sdkp->first_scan)
+		return;
 
 	sdkp->protection_type = type;
 
 	switch (type) {
-	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
-	case SD_DIF_TYPE2_PROTECTION:
-		sd_printk(KERN_ERR, sdkp, "formatted with DIF Type 2 "	\
-			  "protection which is currently unsupported. "	\
-			  "Disabling disk!\n");
-		goto disable;
-
 	default:
-		sd_printk(KERN_ERR, sdkp, "formatted with unknown "	\
-			  "protection type %d. Disabling disk!\n", type);
-		goto disable;
+		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
+			  "protection type %u. Disabling disk!\n", type);
+		sdkp->capacity = 0;
+		return;
 	}
 
-	return;
-
-disable:
-	sdkp->capacity = 0;
+	if (scsi_host_dif_capable(sdp->host, type))
+		sd_printk(KERN_NOTICE, sdkp,
+			  "Enabling DIF Type %u protection\n", type);
+	else
+		sd_printk(KERN_NOTICE, sdkp,
+			  "Disabling DIF Type %u protection\n", type);
 }
 
 static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 8474b5b..ce1f5f8 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -101,16 +101,12 @@ struct sd_dif_tuple {
 
 #ifdef CONFIG_BLK_DEV_INTEGRITY
 
-extern void sd_dif_op(struct scsi_cmnd *, unsigned int, unsigned int, unsigned int);
 extern void sd_dif_config_host(struct scsi_disk *);
 extern int sd_dif_prepare(struct request *rq, sector_t, unsigned int);
 extern void sd_dif_complete(struct scsi_cmnd *, unsigned int);
 
 #else /* CONFIG_BLK_DEV_INTEGRITY */
 
-static inline void sd_dif_op(struct scsi_cmnd *cmd, unsigned int a, unsigned int b, unsigned int c)
-{
-}
 static inline void sd_dif_config_host(struct scsi_disk *disk)
 {
 }
diff --git a/drivers/scsi/sd_dif.c b/drivers/scsi/sd_dif.c
index 84224dd..88da977 100644
--- a/drivers/scsi/sd_dif.c
+++ b/drivers/scsi/sd_dif.c
@@ -320,15 +320,6 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
 		dif = 0; dix = 1;
 	}
 
-	if (type) {
-		if (dif)
-			sd_printk(KERN_NOTICE, sdkp,
-				  "Enabling DIF Type %d protection\n", type);
-		else
-			sd_printk(KERN_NOTICE, sdkp,
-				  "Disabling DIF Type %d protection\n", type);
-	}
-
 	if (!dix)
 		return;
 
@@ -360,50 +351,6 @@ void sd_dif_config_host(struct scsi_disk *sdkp)
 }
 
 /*
- * DIF DMA operation magic decoder ring.
- */
-void sd_dif_op(struct scsi_cmnd *scmd, unsigned int dif, unsigned int dix, unsigned int type)
-{
-	int prot_op;
-
-	prot_op = SCSI_PROT_NORMAL;
-
-	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:
-		if (dif && dix)
-			prot_op = SCSI_PROT_READ_PASS;
-		else if (dif && !dix)
-			prot_op = SCSI_PROT_READ_STRIP;
-		else if (!dif && dix)
-			prot_op = SCSI_PROT_READ_INSERT;
-
-		break;
-
-	case WRITE_6:
-	case WRITE_10:
-	case WRITE_12:
-	case WRITE_16:
-		if (dif && dix)
-			prot_op = SCSI_PROT_WRITE_PASS;
-		else if (dif && !dix)
-			prot_op = SCSI_PROT_WRITE_INSERT;
-		else if (!dif && dix)
-			prot_op = SCSI_PROT_WRITE_STRIP;
-
-		break;
-	}
-
-	scsi_set_prot_op(scmd, prot_op);
-	if (dif)
-		scsi_set_prot_type(scmd, type);
-}
-
-/*
  * The virtual start sector is the one that was originally submitted
  * by the block layer.	Due to partitioning, MD/DM cloning, etc. the
  * actual physical start sector is likely to be different.  Remap
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index b62a097..6e728b1 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -798,9 +798,15 @@ static inline unsigned int scsi_host_get_prot(struct Scsi_Host *shost)
 static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
 	switch (target_type) {
-	case 1: return shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION;
-	case 2: return shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION;
-	case 3: return shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION;
+	case 1:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE1_PROTECTION)
+			return target_type;
+	case 2:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE2_PROTECTION)
+			return target_type;
+	case 3:
+		if (shost->prot_capabilities & SHOST_DIF_TYPE3_PROTECTION)
+			return target_type;
 	}
 
 	return 0;
@@ -808,13 +814,14 @@ static inline unsigned int scsi_host_dif_capable(struct Scsi_Host *shost, unsign
 
 static inline unsigned int scsi_host_dix_capable(struct Scsi_Host *shost, unsigned int target_type)
 {
+#if defined(CONFIG_BLK_DEV_INTEGRITY)
 	switch (target_type) {
 	case 0: return shost->prot_capabilities & SHOST_DIX_TYPE0_PROTECTION;
 	case 1: return shost->prot_capabilities & SHOST_DIX_TYPE1_PROTECTION;
 	case 2: return shost->prot_capabilities & SHOST_DIX_TYPE2_PROTECTION;
 	case 3: return shost->prot_capabilities & SHOST_DIX_TYPE3_PROTECTION;
 	}
-
+#endif
 	return 0;
 }
 
-- 
1.6.0.6

[PATCH 4/5] sd: Support disks formatted with DIF Type 2

[Martin K. Petersen]

Disks formatted with DIF Type 2 reject READ/WRITE 6/10/12/16 commands
when protection is enabled.  Only the 32-byte variants are supported.

Implement support for issusing 32-byte READ/WRITE and enable support for
Type 2 drives in the protection type detection logic.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/sd.c   |   55 ++++++++++++++++++++++++++++++++++++++-------------
 include/scsi/scsi.h |    3 ++
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 36e2333..a26371f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -413,6 +413,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	sector_t threshold;
 	unsigned int this_count = blk_rq_sectors(rq);
 	int ret, host_dif;
+	unsigned char protect;
 
 	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
 		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
@@ -545,13 +546,41 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 	/* Set RDPROTECT/WRPROTECT if disk is formatted with DIF */
 	host_dif = scsi_host_dif_capable(sdp->host, sdkp->protection_type);
 	if (host_dif)
-		SCpnt->cmnd[1] = 1 << 5;
+		protect = 1 << 5;
 	else
-		SCpnt->cmnd[1] = 0;
-
-	if (block > 0xffffffff) {
+		protect = 0;
+
+	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
+		BUG_ON(rq->cmd_len != SCSI_EXT_CDB_SIZE);
+		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
+		SCpnt->cmnd[7] = 0x18;
+		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
+		SCpnt->cmnd[10] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
+
+		/* LBA */
+		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
+		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
+		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
+		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
+		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
+		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
+		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
+		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
+
+		/* Expected Indirect LBA */
+		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
+		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
+		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
+		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
+
+		/* Transfer length */
+		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
+		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
+		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
+		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
+	} else if (block > 0xffffffff) {
 		SCpnt->cmnd[0] += READ_16 - READ_6;
-		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
+		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
 		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
 		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
 		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
@@ -572,7 +601,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 			this_count = 0xffff;
 
 		SCpnt->cmnd[0] += READ_10 - READ_6;
-		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
+		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
 		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
 		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
 		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
@@ -1271,22 +1300,20 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
 
 	sdkp->protection_type = type;
 
-	switch (type) {
-	case SD_DIF_TYPE1_PROTECTION:
-	case SD_DIF_TYPE3_PROTECTION:
-		break;
-
-	default:
+	if (type > SD_DIF_TYPE3_PROTECTION) {
 		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
 			  "protection type %u. Disabling disk!\n", type);
 		sdkp->capacity = 0;
 		return;
 	}
 
-	if (scsi_host_dif_capable(sdp->host, type))
+	if (scsi_host_dif_capable(sdp->host, type)) {
 		sd_printk(KERN_NOTICE, sdkp,
 			  "Enabling DIF Type %u protection\n", type);
-	else
+
+		if (type == SD_DIF_TYPE2_PROTECTION)
+			sdp->use_32_for_rw = 1;
+	} else
 		sd_printk(KERN_NOTICE, sdkp,
 			  "Disabling DIF Type %u protection\n", type);
 }
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index a7ae10a..847de86 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -129,6 +129,9 @@ struct scsi_cmnd;
 #define MI_REPORT_TARGET_PGS  0x0a
 /* values for maintenance out */
 #define MO_SET_TARGET_PGS     0x0a
+/* values for variable length command */
+#define READ_32		      0x09
+#define WRITE_32	      0x0b
 
 /* Values for T10/04-262r7 */
 #define	ATA_16		      0x85	/* 16-byte pass-thru */
-- 
1.6.0.6

Re: [PATCH 4/5] sd: Support disks formatted with DIF Type 2

[Boaz Harrosh]

On 08/26/2009 09:18 AM, Martin K. Petersen wrote:
> Disks formatted with DIF Type 2 reject READ/WRITE 6/10/12/16 commands
> when protection is enabled.  Only the 32-byte variants are supported.
> 
> Implement support for issusing 32-byte READ/WRITE and enable support for
> Type 2 drives in the protection type detection logic.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/sd.c   |   55 ++++++++++++++++++++++++++++++++++++++-------------
>  include/scsi/scsi.h |    3 ++
>  2 files changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 36e2333..a26371f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -413,6 +413,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	sector_t threshold;
>  	unsigned int this_count = blk_rq_sectors(rq);
>  	int ret, host_dif;
> +	unsigned char protect;
>  
>  	if (rq->cmd_type == REQ_TYPE_BLOCK_PC) {
>  		ret = scsi_setup_blk_pc_cmnd(sdp, rq);
> @@ -545,13 +546,41 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  	/* Set RDPROTECT/WRPROTECT if disk is formatted with DIF */
>  	host_dif = scsi_host_dif_capable(sdp->host, sdkp->protection_type);
>  	if (host_dif)
> -		SCpnt->cmnd[1] = 1 << 5;
> +		protect = 1 << 5;
>  	else
> -		SCpnt->cmnd[1] = 0;
> -
> -	if (block > 0xffffffff) {
> +		protect = 0;
> +
> +	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
> +		BUG_ON(rq->cmd_len != SCSI_EXT_CDB_SIZE);

This is not nice you check on the rq->cmd_len but proceed to use
the SCpnt->cmnd. Better ask for SCpnt->cmd_len, just for consistency

> +		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
> +		SCpnt->cmnd[7] = 0x18;
> +		SCpnt->cmnd[9] = (rq_data_dir(rq) == READ) ? READ_32 : WRITE_32;
> +		SCpnt->cmnd[10] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
> +
> +		/* LBA */
> +		SCpnt->cmnd[12] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
> +		SCpnt->cmnd[13] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
> +		SCpnt->cmnd[14] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> +		SCpnt->cmnd[15] = sizeof(block) > 4 ? (unsigned char) (block >> 32) & 0xff : 0;
> +		SCpnt->cmnd[16] = (unsigned char) (block >> 24) & 0xff;
> +		SCpnt->cmnd[17] = (unsigned char) (block >> 16) & 0xff;
> +		SCpnt->cmnd[18] = (unsigned char) (block >> 8) & 0xff;
> +		SCpnt->cmnd[19] = (unsigned char) block & 0xff;
> +
> +		/* Expected Indirect LBA */
> +		SCpnt->cmnd[20] = (unsigned char) (block >> 24) & 0xff;
> +		SCpnt->cmnd[21] = (unsigned char) (block >> 16) & 0xff;
> +		SCpnt->cmnd[22] = (unsigned char) (block >> 8) & 0xff;
> +		SCpnt->cmnd[23] = (unsigned char) block & 0xff;
> +
> +		/* Transfer length */
> +		SCpnt->cmnd[28] = (unsigned char) (this_count >> 24) & 0xff;
> +		SCpnt->cmnd[29] = (unsigned char) (this_count >> 16) & 0xff;
> +		SCpnt->cmnd[30] = (unsigned char) (this_count >> 8) & 0xff;
> +		SCpnt->cmnd[31] = (unsigned char) this_count & 0xff;
> +	} else if (block > 0xffffffff) {
>  		SCpnt->cmnd[0] += READ_16 - READ_6;
> -		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> +		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
>  		SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
>  		SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
>  		SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> @@ -572,7 +601,7 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
>  			this_count = 0xffff;
>  
>  		SCpnt->cmnd[0] += READ_10 - READ_6;
> -		SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> +		SCpnt->cmnd[1] = protect | (blk_fua_rq(rq) ? 0x8 : 0);
>  		SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
>  		SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
>  		SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> @@ -1271,22 +1300,20 @@ void sd_read_protection_type(struct scsi_disk *sdkp, unsigned char *buffer)
>  
>  	sdkp->protection_type = type;
>  
> -	switch (type) {
> -	case SD_DIF_TYPE1_PROTECTION:
> -	case SD_DIF_TYPE3_PROTECTION:
> -		break;
> -
> -	default:
> +	if (type > SD_DIF_TYPE3_PROTECTION) {
>  		sd_printk(KERN_ERR, sdkp, "formatted with unsupported "	\
>  			  "protection type %u. Disabling disk!\n", type);
>  		sdkp->capacity = 0;
>  		return;
>  	}
>  
> -	if (scsi_host_dif_capable(sdp->host, type))
> +	if (scsi_host_dif_capable(sdp->host, type)) {
>  		sd_printk(KERN_NOTICE, sdkp,
>  			  "Enabling DIF Type %u protection\n", type);
> -	else
> +
> +		if (type == SD_DIF_TYPE2_PROTECTION)
> +			sdp->use_32_for_rw = 1;
> +	} else
>  		sd_printk(KERN_NOTICE, sdkp,
>  			  "Disabling DIF Type %u protection\n", type);
>  }
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> index a7ae10a..847de86 100644
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -129,6 +129,9 @@ struct scsi_cmnd;
>  #define MI_REPORT_TARGET_PGS  0x0a
>  /* values for maintenance out */
>  #define MO_SET_TARGET_PGS     0x0a
> +/* values for variable length command */
> +#define READ_32		      0x09
> +#define WRITE_32	      0x0b
>  
>  /* Values for T10/04-262r7 */
>  #define	ATA_16		      0x85	/* 16-byte pass-thru */

Boaz

Re: [PATCH 4/5] sd: Support disks formatted with DIF Type 2

[Martin K. Petersen]

>> + BUG_ON(rq->cmd_len != SCSI_EXT_CDB_SIZE);

Boaz> This is not nice you check on the rq->cmd_len but proceed to use
Boaz> the SCpnt->cmnd. Better ask for SCpnt->cmd_len, just for
Boaz> consistency

Well, I didn't actually mean to leave that BUG_ON in the hot path.  In
any case we won't get this far if the cmd_len is not 32.

-- 
Martin K. Petersen	Oracle Linux Engineering

[PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.

Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
enabled.

Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
CDB.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
 1 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index fb9af20..f551ec3 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -50,6 +50,7 @@
 #include <scsi/scsi_host.h>
 #include <scsi/scsicam.h>
 #include <scsi/scsi_eh.h>
+#include <scsi/scsi_dbg.h>
 
 #include "sd.h"
 #include "scsi_logging.h"
@@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
 #define PARAMETER_LIST_LENGTH_ERR 0x1a
 #define INVALID_OPCODE 0x20
 #define ADDR_OUT_OF_RANGE 0x21
+#define INVALID_COMMAND_OPCODE 0x20
 #define INVALID_FIELD_IN_CDB 0x24
 #define INVALID_FIELD_IN_PARAM_LIST 0x26
 #define POWERON_RESET 0x29
@@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
 #define SDEBUG_SENSE_LEN 32
 
 #define SCSI_DEBUG_CANQUEUE  255
-#define SCSI_DEBUG_MAX_CMD_LEN 16
+#define SCSI_DEBUG_MAX_CMD_LEN 32
 
 struct sdebug_dev_info {
 	struct list_head dev_list;
@@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
 }
 
 static void get_data_transfer_info(unsigned char *cmd,
-				   unsigned long long *lba, unsigned int *num)
+				   unsigned long long *lba, unsigned int *num,
+				   u32 *ei_lba)
 {
+	*ei_lba = 0;
+
 	switch (*cmd) {
+	case VARIABLE_LENGTH_CMD:
+		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
+			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
+			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
+			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
+
+		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
+			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
+
+		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
+			(u32)cmd[28] << 24;
+		break;
+
 	case WRITE_16:
 	case READ_16:
 		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
@@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
 }
 
 static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			    unsigned int sectors)
+			    unsigned int sectors, u32 ei_lba)
 {
 	unsigned int i, resid;
 	struct scatterlist *psgl;
@@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 			return 0x01;
 		}
 
-		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
 			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
 			       __func__, (unsigned long)sector);
 			dif_errors++;
 			return 0x03;
 		}
+
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {
+			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
+			       __func__, (unsigned long)sector);
+			dif_errors++;
+			return 0x03;
+		}
+
+		ei_lba++;
 	}
 
 	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
@@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
 }
 
 static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		     unsigned int num, struct sdebug_dev_info *devip)
+		     unsigned int num, struct sdebug_dev_info *devip,
+		     u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_read(SCpnt, lba, num);
+		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
@@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
 }
 
 static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
-			     unsigned int sectors)
+			     unsigned int sectors, u32 ei_lba)
 {
 	int i, j, ret;
 	struct sd_dif_tuple *sdt;
@@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 
 	sector = do_div(tmp_sec, sdebug_store_sectors);
 
-	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
-		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
-		return 0;
-	}
-
 	BUG_ON(scsi_sg_count(SCpnt) == 0);
 	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
 
@@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
-			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
+			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
 			    be32_to_cpu(sdt->ref_tag)
 			    != (start_sec & 0xffffffff)) {
 				printk(KERN_ERR
@@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				goto out;
 			}
 
+			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
+				printk(KERN_ERR
+				       "%s: REF check failed on sector %lu\n",
+				       __func__, (unsigned long)sector);
+				ret = 0x03;
+				dump_sector(daddr, scsi_debug_sector_size);
+				goto out;
+			}
+
 			/* Would be great to copy this in bigger
 			 * chunks.  However, for the sake of
 			 * correctness we need to verify each sector
@@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
 				sector = 0;	/* Force wrap */
 
 			start_sec++;
+			ei_lba++;
 			daddr += scsi_debug_sector_size;
 			ppage_offset += sizeof(struct sd_dif_tuple);
 		}
@@ -1853,7 +1888,8 @@ out:
 }
 
 static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
-		      unsigned int num, struct sdebug_dev_info *devip)
+		      unsigned int num, struct sdebug_dev_info *devip,
+		      u32 ei_lba)
 {
 	unsigned long iflags;
 	int ret;
@@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
 
 	/* DIX + T10 DIF */
 	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
-		int prot_ret = prot_verify_write(SCpnt, lba, num);
+		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
 
 		if (prot_ret) {
 			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
@@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
 
 	case SD_DIF_TYPE0_PROTECTION:
 	case SD_DIF_TYPE1_PROTECTION:
+	case SD_DIF_TYPE2_PROTECTION:
 	case SD_DIF_TYPE3_PROTECTION:
 		break;
 
 	default:
-		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
+		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
 		return -EINVAL;
 	}
 
@@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	int len, k;
 	unsigned int num;
 	unsigned long long lba;
+	u32 ei_lba;
 	int errsts = 0;
 	int target = SCpnt->device->id;
 	struct sdebug_dev_info *devip = NULL;
@@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case READ_16:
 	case READ_12:
 	case READ_10:
+		/* READ{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case READ_6:
+read:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 	case WRITE_16:
 	case WRITE_12:
 	case WRITE_10:
+		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
+		    cmd[1] & 0xe0) {
+			mk_sense_buffer(devip, ILLEGAL_REQUEST,
+					INVALID_COMMAND_OPCODE, 0);
+			errsts = check_condition_result;
+			break;
+		}
+
+		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
+		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
+		    (cmd[1] & 0xe0) == 0)
+			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
+
+		/* fall through */
 	case WRITE_6:
+write:
 		errsts = check_readiness(SCpnt, 0, devip);
 		if (errsts)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_write(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (inj_recovered && (0 == errsts)) {
 			mk_sense_buffer(devip, RECOVERED_ERROR,
 					THRESHOLD_EXCEEDED, 0);
@@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
 			break;
 		if (scsi_debug_fake_rw)
 			break;
-		get_data_transfer_info(cmd, &lba, &num);
-		errsts = resp_read(SCpnt, lba, num, devip);
+		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
+		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
-		errsts = resp_write(SCpnt, lba, num, devip);
+		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
 		if (errsts)
 			break;
 		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
 		break;
+	case VARIABLE_LENGTH_CMD:
+		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
+
+			if ((cmd[10] & 0xe0) == 0)
+				printk(KERN_ERR
+				       "Unprotected RD/WR to DIF device\n");
+
+			if (cmd[9] == READ_32)
+				goto read;
+
+			if (cmd[9] == WRITE_32)
+				goto write;
+		}
+
+		mk_sense_buffer(devip, ILLEGAL_REQUEST,
+				INVALID_FIELD_IN_CDB, 0);
+		errsts = check_condition_result;
+		break;
+
 	default:
 		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
 			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "
-- 
1.6.0.6

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/26/2009 09:18 AM, Martin K. Petersen wrote:
> Add support for 32-byte READ/WRITE as well as DIF Type 2 protection.
> 
> Reject protected 10/12/16 byte READ/WRITE commands when Type 2 is
> enabled.
> 
> Verify Type 2 reference tag according to Expected Initial LBA in 32-byte
> CDB.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_debug.c |  135 +++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 112 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index fb9af20..f551ec3 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -50,6 +50,7 @@
>  #include <scsi/scsi_host.h>
>  #include <scsi/scsicam.h>
>  #include <scsi/scsi_eh.h>
> +#include <scsi/scsi_dbg.h>
>  
>  #include "sd.h"
>  #include "scsi_logging.h"
> @@ -64,6 +65,7 @@ static const char * scsi_debug_version_date = "20070104";
>  #define PARAMETER_LIST_LENGTH_ERR 0x1a
>  #define INVALID_OPCODE 0x20
>  #define ADDR_OUT_OF_RANGE 0x21
> +#define INVALID_COMMAND_OPCODE 0x20
>  #define INVALID_FIELD_IN_CDB 0x24
>  #define INVALID_FIELD_IN_PARAM_LIST 0x26
>  #define POWERON_RESET 0x29
> @@ -180,7 +182,7 @@ static int sdebug_sectors_per;		/* sectors per cylinder */
>  #define SDEBUG_SENSE_LEN 32
>  
>  #define SCSI_DEBUG_CANQUEUE  255
> -#define SCSI_DEBUG_MAX_CMD_LEN 16
> +#define SCSI_DEBUG_MAX_CMD_LEN 32
>  
>  struct sdebug_dev_info {
>  	struct list_head dev_list;
> @@ -296,9 +298,25 @@ static void mk_sense_buffer(struct sdebug_dev_info *devip, int key,
>  }
>  
>  static void get_data_transfer_info(unsigned char *cmd,
> -				   unsigned long long *lba, unsigned int *num)
> +				   unsigned long long *lba, unsigned int *num,
> +				   u32 *ei_lba)
>  {
> +	*ei_lba = 0;
> +
>  	switch (*cmd) {
> +	case VARIABLE_LENGTH_CMD:
> +		*lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
> +			(u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
> +			(u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
> +			(u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> +

get_unaligned_be64()

> +		*ei_lba = (u32)cmd[23] | (u32)cmd[22] << 8 |
> +			(u32)cmd[21] << 16 | (u32)cmd[20] << 24;
> +

be32_to_cpup()

> +		*num = (u32)cmd[31] | (u32)cmd[30] << 8 | (u32)cmd[29] << 16 |
> +			(u32)cmd[28] << 24;

be32_to_cpup()

On some arches, above code will produce 96 assembly instructions as opossed to
a single MOV. I understand that the old code is full of this crap, but "new code"?

I know I could never blasphem like that. As if you are not proud of being a programmer

> +		break;
> +
>  	case WRITE_16:
>  	case READ_16:
>  		*lba = (u64)cmd[9] | (u64)cmd[8] << 8 |
> @@ -1589,7 +1607,7 @@ static int do_device_access(struct scsi_cmnd *scmd,
>  }
>  
>  static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
> -			    unsigned int sectors)
> +			    unsigned int sectors, u32 ei_lba)
>  {
>  	unsigned int i, resid;
>  	struct scatterlist *psgl;
> @@ -1636,13 +1654,23 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  			return 0x01;
>  		}
>  
> -		if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
> +		if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
>  		    be32_to_cpu(sdt[i].ref_tag) != (sector & 0xffffffff)) {
>  			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
>  			       __func__, (unsigned long)sector);
>  			dif_errors++;
>  			return 0x03;
>  		}
> +
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    be32_to_cpu(sdt[i].ref_tag) != ei_lba) {

Here you do it right

> +			printk(KERN_ERR "%s: REF check failed on sector %lu\n",
> +			       __func__, (unsigned long)sector);
> +			dif_errors++;
> +			return 0x03;
> +		}
> +
> +		ei_lba++;
>  	}
>  
>  	resid = sectors * 8; /* Bytes of protection data to copy into sgl */
> @@ -1670,7 +1698,8 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  }
>  
>  static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
> -		     unsigned int num, struct sdebug_dev_info *devip)
> +		     unsigned int num, struct sdebug_dev_info *devip,
> +		     u32 ei_lba)
>  {
>  	unsigned long iflags;
>  	int ret;
> @@ -1699,7 +1728,7 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,
>  
>  	/* DIX + T10 DIF */
>  	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
> -		int prot_ret = prot_verify_read(SCpnt, lba, num);
> +		int prot_ret = prot_verify_read(SCpnt, lba, num, ei_lba);
>  
>  		if (prot_ret) {
>  			mk_sense_buffer(devip, ABORTED_COMMAND, 0x10, prot_ret);
> @@ -1735,7 +1764,7 @@ void dump_sector(unsigned char *buf, int len)
>  }
>  
>  static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
> -			     unsigned int sectors)
> +			     unsigned int sectors, u32 ei_lba)
>  {
>  	int i, j, ret;
>  	struct sd_dif_tuple *sdt;
> @@ -1749,11 +1778,6 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  
>  	sector = do_div(tmp_sec, sdebug_store_sectors);
>  
> -	if (((SCpnt->cmnd[1] >> 5) & 7) != 1) {
> -		printk(KERN_WARNING "scsi_debug: WRPROTECT != 1\n");
> -		return 0;
> -	}
> -
>  	BUG_ON(scsi_sg_count(SCpnt) == 0);
>  	BUG_ON(scsi_prot_sg_count(SCpnt) == 0);
>  
> @@ -1808,7 +1832,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				goto out;
>  			}
>  
> -			if (scsi_debug_dif != SD_DIF_TYPE3_PROTECTION &&
> +			if (scsi_debug_dif == SD_DIF_TYPE1_PROTECTION &&
>  			    be32_to_cpu(sdt->ref_tag)
>  			    != (start_sec & 0xffffffff)) {
>  				printk(KERN_ERR
> @@ -1819,6 +1843,16 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				goto out;
>  			}
>  
> +			if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +			    be32_to_cpu(sdt->ref_tag) != ei_lba) {
> +				printk(KERN_ERR
> +				       "%s: REF check failed on sector %lu\n",
> +				       __func__, (unsigned long)sector);
> +				ret = 0x03;
> +				dump_sector(daddr, scsi_debug_sector_size);
> +				goto out;
> +			}
> +
>  			/* Would be great to copy this in bigger
>  			 * chunks.  However, for the sake of
>  			 * correctness we need to verify each sector
> @@ -1832,6 +1866,7 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec,
>  				sector = 0;	/* Force wrap */
>  
>  			start_sec++;
> +			ei_lba++;
>  			daddr += scsi_debug_sector_size;
>  			ppage_offset += sizeof(struct sd_dif_tuple);
>  		}
> @@ -1853,7 +1888,8 @@ out:
>  }
>  
>  static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
> -		      unsigned int num, struct sdebug_dev_info *devip)
> +		      unsigned int num, struct sdebug_dev_info *devip,
> +		      u32 ei_lba)
>  {
>  	unsigned long iflags;
>  	int ret;
> @@ -1864,7 +1900,7 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,
>  
>  	/* DIX + T10 DIF */
>  	if (scsi_debug_dix && scsi_prot_sg_count(SCpnt)) {
> -		int prot_ret = prot_verify_write(SCpnt, lba, num);
> +		int prot_ret = prot_verify_write(SCpnt, lba, num, ei_lba);
>  
>  		if (prot_ret) {
>  			mk_sense_buffer(devip, ILLEGAL_REQUEST, 0x10, prot_ret);
> @@ -2872,11 +2908,12 @@ static int __init scsi_debug_init(void)
>  
>  	case SD_DIF_TYPE0_PROTECTION:
>  	case SD_DIF_TYPE1_PROTECTION:
> +	case SD_DIF_TYPE2_PROTECTION:
>  	case SD_DIF_TYPE3_PROTECTION:
>  		break;
>  
>  	default:
> -		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1 or 3\n");
> +		printk(KERN_ERR "scsi_debug_init: dif must be 0, 1, 2 or 3\n");
>  		return -EINVAL;
>  	}
>  
> @@ -3121,6 +3158,7 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	int len, k;
>  	unsigned int num;
>  	unsigned long long lba;
> +	u32 ei_lba;
>  	int errsts = 0;
>  	int target = SCpnt->device->id;
>  	struct sdebug_dev_info *devip = NULL;
> @@ -3254,14 +3292,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	case READ_16:
>  	case READ_12:
>  	case READ_10:
> +		/* READ{10,12,16} and DIF Type 2 are natural enemies */
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    cmd[1] & 0xe0) {
> +			mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +					INVALID_COMMAND_OPCODE, 0);
> +			errsts = check_condition_result;
> +			break;
> +		}
> +
> +		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
> +		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
> +		    (cmd[1] & 0xe0) == 0)
> +			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
> +
> +		/* fall through */
>  	case READ_6:
> +read:
>  		errsts = check_readiness(SCpnt, 0, devip);
>  		if (errsts)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_read(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
>  		if (inj_recovered && (0 == errsts)) {
>  			mk_sense_buffer(devip, RECOVERED_ERROR,
>  					THRESHOLD_EXCEEDED, 0);
> @@ -3288,14 +3342,30 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  	case WRITE_16:
>  	case WRITE_12:
>  	case WRITE_10:
> +		/* WRITE{10,12,16} and DIF Type 2 are natural enemies */
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION &&
> +		    cmd[1] & 0xe0) {
> +			mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +					INVALID_COMMAND_OPCODE, 0);
> +			errsts = check_condition_result;
> +			break;
> +		}
> +
> +		if ((scsi_debug_dif == SD_DIF_TYPE1_PROTECTION ||
> +		     scsi_debug_dif == SD_DIF_TYPE3_PROTECTION) &&
> +		    (cmd[1] & 0xe0) == 0)
> +			printk(KERN_ERR "Unprotected RD/WR to DIF device\n");
> +
> +		/* fall through */
>  	case WRITE_6:
> +write:
>  		errsts = check_readiness(SCpnt, 0, devip);
>  		if (errsts)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_write(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
>  		if (inj_recovered && (0 == errsts)) {
>  			mk_sense_buffer(devip, RECOVERED_ERROR,
>  					THRESHOLD_EXCEEDED, 0);
> @@ -3341,15 +3411,34 @@ int scsi_debug_queuecommand(struct scsi_cmnd *SCpnt, done_funct_t done)
>  			break;
>  		if (scsi_debug_fake_rw)
>  			break;
> -		get_data_transfer_info(cmd, &lba, &num);
> -		errsts = resp_read(SCpnt, lba, num, devip);
> +		get_data_transfer_info(cmd, &lba, &num, &ei_lba);
> +		errsts = resp_read(SCpnt, lba, num, devip, ei_lba);
>  		if (errsts)
>  			break;
> -		errsts = resp_write(SCpnt, lba, num, devip);
> +		errsts = resp_write(SCpnt, lba, num, devip, ei_lba);
>  		if (errsts)
>  			break;
>  		errsts = resp_xdwriteread(SCpnt, lba, num, devip);
>  		break;
> +	case VARIABLE_LENGTH_CMD:
> +		if (scsi_debug_dif == SD_DIF_TYPE2_PROTECTION) {
> +
> +			if ((cmd[10] & 0xe0) == 0)
> +				printk(KERN_ERR
> +				       "Unprotected RD/WR to DIF device\n");
> +
> +			if (cmd[9] == READ_32)
> +				goto read;
> +
> +			if (cmd[9] == WRITE_32)
> +				goto write;
> +		}
> +
> +		mk_sense_buffer(devip, ILLEGAL_REQUEST,
> +				INVALID_FIELD_IN_CDB, 0);
> +		errsts = check_condition_result;
> +		break;
> +
>  	default:
>  		if (SCSI_DEBUG_OPT_NOISE & scsi_debug_opts)
>  			printk(KERN_INFO "scsi_debug: Opcode: 0x%x not "

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Martin K. Petersen]

>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:

>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;

Boaz> get_unaligned_be64()

As you noticed further down in that patch I do generally use the
get_unaligned_* macros in "my own" code.

However, when I update somebody else's code I try to match the existing
style.  And in this case rest of get_data_transfer_info() is using
explicit shifts and to me it looks absolutely horrendous to mix the two.

I generally avoid mixing cleanups and new functionality.  I don't have a
problem with switching over to the macros, but in that case I think the
whole function should be updated.  And that should be an orthogonal
patch.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> 
> Boaz> get_unaligned_be64()
> 
> As you noticed further down in that patch I do generally use the
> get_unaligned_* macros in "my own" code.
> 
> However, when I update somebody else's code I try to match the existing
> style.  And in this case rest of get_data_transfer_info() is using
> explicit shifts and to me it looks absolutely horrendous to mix the two.
> 
> I generally avoid mixing cleanups and new functionality.  I don't have a
> problem with switching over to the macros, but in that case I think the
> whole function should be updated.  And that should be an orthogonal
> patch.
> 

I don't know. For me it is like checkpatch. I do not submit code over 80
chars even if surrounding code does. "The new code rule".

I generally agree with what you say but I think there is a balance.
Personally, I think this is over the balance point, but it's your call.

Thanks
Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
> >>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> > 
> >>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
> >>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
> >>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
> >>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
> > 
> > Boaz> get_unaligned_be64()
> > 
> > As you noticed further down in that patch I do generally use the
> > get_unaligned_* macros in "my own" code.
> > 
> > However, when I update somebody else's code I try to match the existing
> > style.  And in this case rest of get_data_transfer_info() is using
> > explicit shifts and to me it looks absolutely horrendous to mix the two.
> > 
> > I generally avoid mixing cleanups and new functionality.  I don't have a
> > problem with switching over to the macros, but in that case I think the
> > whole function should be updated.  And that should be an orthogonal
> > patch.
> > 
> 
> I don't know. For me it is like checkpatch. I do not submit code over 80
> chars even if surrounding code does. "The new code rule".

I'd take that as a guideline rather than a rule ... Especially when
lindent will generate 80 column long function templates over tens of
lines squashing about five letters per line ...

> I generally agree with what you say but I think there is a balance.
> Personally, I think this is over the balance point, but it's your call.

The general rule is not to confuse coding styles, so the correct way to
add stuff is to use the existing conventions in the file.  You can
optionally convert the entire style if necessary.  However, for these
get_cpu_be macros, there's no real benefit other than saving typing, so
a global conversion effort simply isn't worth it.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 04:41 PM, James Bottomley wrote:
> 
> The general rule is not to confuse coding styles, so the correct way to
> add stuff is to use the existing conventions in the file.  You can
> optionally convert the entire style if necessary.  However, for these
> get_cpu_be macros, there's no real benefit other than saving typing, so
> a global conversion effort simply isn't worth it.
> 

This is not right. The get_cpu_be macros are ten fold faster and smaller
on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
operation.

Not to mention the heart attack it gives me every time. Is that index go down
or up? the shifts go bigger or smaller? even just for that I would wrap them,
triple check, and never use anything else. But the stronger fact of the matter
is that I don't think there is a single used ARCH that does shifts anymore.

> James
> 
> 

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
> On 08/27/2009 04:41 PM, James Bottomley wrote:
> > 
> > The general rule is not to confuse coding styles, so the correct way to
> > add stuff is to use the existing conventions in the file.  You can
> > optionally convert the entire style if necessary.  However, for these
> > get_cpu_be macros, there's no real benefit other than saving typing, so
> > a global conversion effort simply isn't worth it.
> > 
> 
> This is not right. The get_cpu_be macros are ten fold faster and smaller
> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
> operation.

Assembly comparisons didn't bear this out the last time I looked; what
changed?

> Not to mention the heart attack it gives me every time. Is that index go down
> or up? the shifts go bigger or smaller? even just for that I would wrap them,
> triple check, and never use anything else. But the stronger fact of the matter
> is that I don't think there is a single used ARCH that does shifts anymore.

OK, but for those of us who read the standards, they explicitly specify
byte offset fields for everything, so cmnd[n] does map exactly to that,
so I find the fully folded out form easier to read and compare with the
relevant standard text.

That's why I'm not mandating anything other than keep the styles
consistent per file.  You're free to use whatever you like in your
files.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 05:30 PM, James Bottomley wrote:
> On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 04:41 PM, James Bottomley wrote:
>>>
>>> The general rule is not to confuse coding styles, so the correct way to
>>> add stuff is to use the existing conventions in the file.  You can
>>> optionally convert the entire style if necessary.  However, for these
>>> get_cpu_be macros, there's no real benefit other than saving typing, so
>>> a global conversion effort simply isn't worth it.
>>>
>>
>> This is not right. The get_cpu_be macros are ten fold faster and smaller
>> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
>> operation.
> 
> Assembly comparisons didn't bear this out the last time I looked; what
> changed?
> 

I'm not sure what test you made. But for instance on x86_64 which has unaligned
cpu support, the get_unaligned_be64 is a simple SWAB instructions as opposed to
8 "or" + 8 "shift". Not to mention BE systems which do nothing (memcpy)

>> Not to mention the heart attack it gives me every time. Is that index go down
>> or up? the shifts go bigger or smaller? even just for that I would wrap them,
>> triple check, and never use anything else. But the stronger fact of the matter
>> is that I don't think there is a single used ARCH that does shifts anymore.
> 
> OK, but for those of us who read the standards, they explicitly specify
> byte offset fields for everything, so cmnd[n] does map exactly to that,
> so I find the fully folded out form easier to read and compare with the
> relevant standard text.
> 
> That's why I'm not mandating anything other than keep the styles
> consistent per file.  You're free to use whatever you like in your
> files.
> 
> James
> 
> 

Boaz

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[James Bottomley]

On Thu, 2009-08-27 at 17:47 +0300, Boaz Harrosh wrote:
> On 08/27/2009 05:30 PM, James Bottomley wrote:
> > On Thu, 2009-08-27 at 17:20 +0300, Boaz Harrosh wrote:
> >> On 08/27/2009 04:41 PM, James Bottomley wrote:
> >>>
> >>> The general rule is not to confuse coding styles, so the correct way to
> >>> add stuff is to use the existing conventions in the file.  You can
> >>> optionally convert the entire style if necessary.  However, for these
> >>> get_cpu_be macros, there's no real benefit other than saving typing, so
> >>> a global conversion effort simply isn't worth it.
> >>>
> >>
> >> This is not right. The get_cpu_be macros are ten fold faster and smaller
> >> on all the platforms we ever use. I'm talking about 16-96 to 1 for a 64 bit
> >> operation.
> > 
> > Assembly comparisons didn't bear this out the last time I looked; what
> > changed?
> > 
> 
> I'm not sure what test you made. But for instance on x86_64 which has unaligned
> cpu support, the get_unaligned_be64 is a simple SWAB instructions as opposed to
> 8 "or" + 8 "shift". Not to mention BE systems which do nothing (memcpy)

Well, what I saw on x86 is that gcc optimises the shifts to exactly the
same code as get_unaligned_be.  What does the assembly comparison look
like on x86-64?  However, regardless, this is hardly critical path
affecting stuff.

James

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Douglas Gilbert]

James Bottomley wrote:
> On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
>>> Boaz> get_unaligned_be64()
>>>
>>> As you noticed further down in that patch I do generally use the
>>> get_unaligned_* macros in "my own" code.
>>>
>>> However, when I update somebody else's code I try to match the existing
>>> style.  And in this case rest of get_data_transfer_info() is using
>>> explicit shifts and to me it looks absolutely horrendous to mix the two.
>>>
>>> I generally avoid mixing cleanups and new functionality.  I don't have a
>>> problem with switching over to the macros, but in that case I think the
>>> whole function should be updated.  And that should be an orthogonal
>>> patch.
>>>
>> I don't know. For me it is like checkpatch. I do not submit code over 80
>> chars even if surrounding code does. "The new code rule".
> 
> I'd take that as a guideline rather than a rule ... Especially when
> lindent will generate 80 column long function templates over tens of
> lines squashing about five letters per line ...
> 
>> I generally agree with what you say but I think there is a balance.
>> Personally, I think this is over the balance point, but it's your call.
> 
> The general rule is not to confuse coding styles, so the correct way to
> add stuff is to use the existing conventions in the file.  You can
> optionally convert the entire style if necessary.  However, for these
> get_cpu_be macros, there's no real benefit other than saving typing, so
> a global conversion effort simply isn't worth it.

There is another aspect that I look at: portability to
the user space of Linux and other operating systems.

Are there C99 or POSIX equivalents of get_unaligned_be64()?

Recently sg_read_block_limits was contributed to sg3_utils.
Its author found a novel solution to this "problem" with
network stack calls: ntohl() and friends. So I ran with it
until I found it broke my builds in the MinGW environment
for Windows. Rather than put in conditional compiles and
wonder if my MinGW executables would get bloated, I opted
to go back to what has worked for the last 25 years.

Doug Gilbert

Re: [PATCH 5/5] scsi_debug: Implement support for DIF Type 2

[Boaz Harrosh]

On 08/27/2009 06:17 PM, Douglas Gilbert wrote:
> James Bottomley wrote:
>> On Thu, 2009-08-27 at 12:35 +0300, Boaz Harrosh wrote:
>>> On 08/27/2009 09:58 AM, Martin K. Petersen wrote:
>>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>>> + *lba = (u64)cmd[19] | (u64)cmd[18] << 8 |
>>>>>> + (u64)cmd[17] << 16 | (u64)cmd[16] << 24 |
>>>>>> + (u64)cmd[15] << 32 | (u64)cmd[14] << 40 |
>>>>>> + (u64)cmd[13] << 48 | (u64)cmd[12] << 56;
>>>> Boaz> get_unaligned_be64()
>>>>
>>>> As you noticed further down in that patch I do generally use the
>>>> get_unaligned_* macros in "my own" code.
>>>>
>>>> However, when I update somebody else's code I try to match the existing
>>>> style.  And in this case rest of get_data_transfer_info() is using
>>>> explicit shifts and to me it looks absolutely horrendous to mix the two.
>>>>
>>>> I generally avoid mixing cleanups and new functionality.  I don't have a
>>>> problem with switching over to the macros, but in that case I think the
>>>> whole function should be updated.  And that should be an orthogonal
>>>> patch.
>>>>
>>> I don't know. For me it is like checkpatch. I do not submit code over 80
>>> chars even if surrounding code does. "The new code rule".
>>
>> I'd take that as a guideline rather than a rule ... Especially when
>> lindent will generate 80 column long function templates over tens of
>> lines squashing about five letters per line ...
>>
>>> I generally agree with what you say but I think there is a balance.
>>> Personally, I think this is over the balance point, but it's your call.
>>
>> The general rule is not to confuse coding styles, so the correct way to
>> add stuff is to use the existing conventions in the file.  You can
>> optionally convert the entire style if necessary.  However, for these
>> get_cpu_be macros, there's no real benefit other than saving typing, so
>> a global conversion effort simply isn't worth it.
> 
> There is another aspect that I look at: portability to
> the user space of Linux and other operating systems.
> 
> Are there C99 or POSIX equivalents of get_unaligned_be64()?
> 
> Recently sg_read_block_limits was contributed to sg3_utils.
> Its author found a novel solution to this "problem" with
> network stack calls: ntohl() and friends. So I ran with it
> until I found it broke my builds in the MinGW environment
> for Windows. Rather than put in conditional compiles and
> wonder if my MinGW executables would get bloated, I opted
> to go back to what has worked for the last 25 years.
> 
> Doug Gilbert

Hi

As I said. I don't have a strong heart the way you guys have ;-)
even if I had to write it from scratch for every body of code I do
I would.

That said, the aligned accessors are exported from
linux to user space, the none-aligned I just kindly borrow into my project
as a couple of GPLed headers and that is that. Open source for you.

Boaz

Re: DIF/DIX updates for 2.6.32

[Boaz Harrosh]

On 08/26/2009 09:17 AM, Martin K. Petersen wrote:
> Updates to the DIF/DIX code for 2.6.32:
> 
>  - Add support for 32-byte CDBs to SCSI layer
> 
>  - Deprecate the DIX CONVERT ops
> 
>  - Detach pure DIF support from CONFIG_BLK_DEV_INTEGRITY
> 
>  - Add support for DIF Type 2 devices in sd
> 
>  - Add DIF Type 2 support to scsi_debug
> 

Hi Martin.

I have not followed your work very closely so please forgive me if
I'm asking dumb questions. I've meant to be asking you about something
for a while.

With an sd disk supporting DIF/DIX, do you have this problems?
http://thread.gmane.org/gmane.linux.kernel.device-mapper.devel/5392
[Desynchronizing dm-raid1]

I know that we also have the above problem with iscsi and data-digest
such that when we come to sign the data it might change on us before
the target receives it.

(Personally I'm working on a Mirror system over osd's and will have to
 test for this problem as well. I anticipate a need to copy the data
 before submission like the dm-raid1 does)

Best regards
Boaz

Re: DIF/DIX updates for 2.6.32

[Martin K. Petersen]

>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:

Boaz> I know that we also have the above problem with iscsi and
Boaz> data-digest such that when we come to sign the data it might
Boaz> change on us before the target receives it.

Yep, I have the same problem.  I talked to Andrew Morton a couple of
months ago and he said that modifying pages in flight is "a feature" as
far as ext[234] is concerned.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: DIF/DIX updates for 2.6.32

[Boaz Harrosh]

On 08/27/2009 09:34 AM, Martin K. Petersen wrote:
>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> 
> Boaz> I know that we also have the above problem with iscsi and
> Boaz> data-digest such that when we come to sign the data it might
> Boaz> change on us before the target receives it.
> 
> Yep, I have the same problem.  I talked to Andrew Morton a couple of
> months ago and he said that modifying pages in flight is "a feature" as
> far as ext[234] is concerned.
> 

As you might know, I have a filesystem copied from the ext2 code base.
I'm experimenting with altering the behavior so that pages written to
while been IOed will page fault, then sleep, until IO is done.
Clearly this is a good "feature" until such systems like mirror or signed-
data that are forced to reallocate-copy all IO do to the 2% optimization
that thing gives you.

At the final outcome I hope for a VFS support on a flip of a flag or
something. So under laying device can turn that "feature" off when it
means grate performance gains in it's operations.

If any one has thought about that problem, and as some preliminary strategies,
please I'm all hears. I've just started on this subject and currently I do not
have a clue.

Thanks
Boaz

Re: DIF/DIX updates for 2.6.32

[James Bottomley]

On Thu, 2009-08-27 at 12:49 +0300, Boaz Harrosh wrote:
> On 08/27/2009 09:34 AM, Martin K. Petersen wrote:
> >>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> > 
> > Boaz> I know that we also have the above problem with iscsi and
> > Boaz> data-digest such that when we come to sign the data it might
> > Boaz> change on us before the target receives it.
> > 
> > Yep, I have the same problem.  I talked to Andrew Morton a couple of
> > months ago and he said that modifying pages in flight is "a feature" as
> > far as ext[234] is concerned.
> > 
> 
> As you might know, I have a filesystem copied from the ext2 code base.
> I'm experimenting with altering the behavior so that pages written to
> while been IOed will page fault, then sleep, until IO is done.
> Clearly this is a good "feature" until such systems like mirror or signed-
> data that are forced to reallocate-copy all IO do to the 2% optimization
> that thing gives you.

What about reads to the page?  If you allow them, you get the situation
where something signals a write intent, tries to write and gets put into
wait, then the readers get the old data still.

> At the final outcome I hope for a VFS support on a flip of a flag or
> something. So under laying device can turn that "feature" off when it
> means grate performance gains in it's operations.
> 
> If any one has thought about that problem, and as some preliminary strategies,
> please I'm all hears. I've just started on this subject and currently I do not
> have a clue.

The correct way to handle this is simply to dump the page being written.
It's dirty and was updated after the last write attempt, so it gets
re-written out.  It costs nothing and it's incredibly fast.

What you likely want is a way of telling that the page got re-written so
you don't need to print out scary warning messages about parity
problems.

James

Re: DIF/DIX updates for 2.6.32

[Boaz Harrosh]

On 08/27/2009 04:46 PM, James Bottomley wrote:
> On Thu, 2009-08-27 at 12:49 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 09:34 AM, Martin K. Petersen wrote:
>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>
>>> Boaz> I know that we also have the above problem with iscsi and
>>> Boaz> data-digest such that when we come to sign the data it might
>>> Boaz> change on us before the target receives it.
>>>
>>> Yep, I have the same problem.  I talked to Andrew Morton a couple of
>>> months ago and he said that modifying pages in flight is "a feature" as
>>> far as ext[234] is concerned.
>>>
>>
>> As you might know, I have a filesystem copied from the ext2 code base.
>> I'm experimenting with altering the behavior so that pages written to
>> while been IOed will page fault, then sleep, until IO is done.
>> Clearly this is a good "feature" until such systems like mirror or signed-
>> data that are forced to reallocate-copy all IO do to the 2% optimization
>> that thing gives you.
> 
> What about reads to the page?  If you allow them, you get the situation
> where something signals a write intent, tries to write and gets put into
> wait, then the readers get the old data still.
> 

Is there any guaranty between a parallel write and read about what's first?
But I think in my case the reads will also page-fault so I'm not sure yet.
Thanks for asking that's a good question that should be taken into
consideration.

>> At the final outcome I hope for a VFS support on a flip of a flag or
>> something. So under laying device can turn that "feature" off when it
>> means grate performance gains in it's operations.
>>
>> If any one has thought about that problem, and as some preliminary strategies,
>> please I'm all hears. I've just started on this subject and currently I do not
>> have a clue.
> 
> The correct way to handle this is simply to dump the page being written.
> It's dirty and was updated after the last write attempt, so it gets
> re-written out.  It costs nothing and it's incredibly fast.
> 

This is not an option on a mirror system, and the performance gain/lose
is dependent on the round trip speed. If for every digest error I have an
error recovery cycle, delays, and stalls. Then no it is not better. Not
to mention some iscsi-targets that reset and the all session must be
re-established.

> What you likely want is a way of telling that the page got re-written so
> you don't need to print out scary warning messages about parity
> problems.
> 

Maybe that is a start. I guess I could signal a fast abort for these. What
would be the cost for this knowledge. I guess O(sglist-size) right? loop
on all pages and check? Anything better we can do?

> James
> 
> 

Thanks
Boaz

Re: DIF/DIX updates for 2.6.32

[James Bottomley]

On Thu, 2009-08-27 at 17:40 +0300, Boaz Harrosh wrote:
> On 08/27/2009 04:46 PM, James Bottomley wrote:
> > On Thu, 2009-08-27 at 12:49 +0300, Boaz Harrosh wrote:
> >> On 08/27/2009 09:34 AM, Martin K. Petersen wrote:
> >>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
> >>>
> >>> Boaz> I know that we also have the above problem with iscsi and
> >>> Boaz> data-digest such that when we come to sign the data it might
> >>> Boaz> change on us before the target receives it.
> >>>
> >>> Yep, I have the same problem.  I talked to Andrew Morton a couple of
> >>> months ago and he said that modifying pages in flight is "a feature" as
> >>> far as ext[234] is concerned.
> >>>
> >>
> >> As you might know, I have a filesystem copied from the ext2 code base.
> >> I'm experimenting with altering the behavior so that pages written to
> >> while been IOed will page fault, then sleep, until IO is done.
> >> Clearly this is a good "feature" until such systems like mirror or signed-
> >> data that are forced to reallocate-copy all IO do to the 2% optimization
> >> that thing gives you.
> > 
> > What about reads to the page?  If you allow them, you get the situation
> > where something signals a write intent, tries to write and gets put into
> > wait, then the readers get the old data still.
> > 
> 
> Is there any guaranty between a parallel write and read about what's first?
> But I think in my case the reads will also page-fault so I'm not sure yet.
> Thanks for asking that's a good question that should be taken into
> consideration.
> 
> >> At the final outcome I hope for a VFS support on a flip of a flag or
> >> something. So under laying device can turn that "feature" off when it
> >> means grate performance gains in it's operations.
> >>
> >> If any one has thought about that problem, and as some preliminary strategies,
> >> please I'm all hears. I've just started on this subject and currently I do not
> >> have a clue.
> > 
> > The correct way to handle this is simply to dump the page being written.
> > It's dirty and was updated after the last write attempt, so it gets
> > re-written out.  It costs nothing and it's incredibly fast.
> > 
> 
> This is not an option on a mirror system, and the performance gain/lose
> is dependent on the round trip speed. If for every digest error I have an
> error recovery cycle, delays, and stalls. Then no it is not better. Not
> to mention some iscsi-targets that reset and the all session must be
> re-established.

Your suggestion of putting processes to sleep while I/O is pending will
degrade performance for everyone; that's not really an acceptable
tradeoff for improving one corner case.

> > What you likely want is a way of telling that the page got re-written so
> > you don't need to print out scary warning messages about parity
> > problems.
> > 
> 
> Maybe that is a start. I guess I could signal a fast abort for these. What
> would be the cost for this knowledge. I guess O(sglist-size) right? loop
> on all pages and check? Anything better we can do?

I think it's a page flag indicating write begun on current page.  it
gets set when I/O is begun and reset if another write comes in in the
meantime.  Thus you can check before issue if this flag is set ... if it
is, your digest is likely set.  If not, you need to discard the page
from the I/O (or redo the digest).

James

Re: DIF/DIX updates for 2.6.32

[Boaz Harrosh]

On 08/27/2009 05:51 PM, James Bottomley wrote:
> On Thu, 2009-08-27 at 17:40 +0300, Boaz Harrosh wrote:
>> On 08/27/2009 04:46 PM, James Bottomley wrote:
>>> On Thu, 2009-08-27 at 12:49 +0300, Boaz Harrosh wrote:
>>>> On 08/27/2009 09:34 AM, Martin K. Petersen wrote:
>>>>>>>>>> "Boaz" == Boaz Harrosh <bharrosh@panasas.com> writes:
>>>>>
>>>>> Boaz> I know that we also have the above problem with iscsi and
>>>>> Boaz> data-digest such that when we come to sign the data it might
>>>>> Boaz> change on us before the target receives it.
>>>>>
>>>>> Yep, I have the same problem.  I talked to Andrew Morton a couple of
>>>>> months ago and he said that modifying pages in flight is "a feature" as
>>>>> far as ext[234] is concerned.
>>>>>
>>>>
>>>> As you might know, I have a filesystem copied from the ext2 code base.
>>>> I'm experimenting with altering the behavior so that pages written to
>>>> while been IOed will page fault, then sleep, until IO is done.
>>>> Clearly this is a good "feature" until such systems like mirror or signed-
>>>> data that are forced to reallocate-copy all IO do to the 2% optimization
>>>> that thing gives you.
>>>
>>> What about reads to the page?  If you allow them, you get the situation
>>> where something signals a write intent, tries to write and gets put into
>>> wait, then the readers get the old data still.
>>>
>>
>> Is there any guaranty between a parallel write and read about what's first?
>> But I think in my case the reads will also page-fault so I'm not sure yet.
>> Thanks for asking that's a good question that should be taken into
>> consideration.
>>
>>>> At the final outcome I hope for a VFS support on a flip of a flag or
>>>> something. So under laying device can turn that "feature" off when it
>>>> means grate performance gains in it's operations.
>>>>
>>>> If any one has thought about that problem, and as some preliminary strategies,
>>>> please I'm all hears. I've just started on this subject and currently I do not
>>>> have a clue.
>>>
>>> The correct way to handle this is simply to dump the page being written.
>>> It's dirty and was updated after the last write attempt, so it gets
>>> re-written out.  It costs nothing and it's incredibly fast.
>>>
>>
>> This is not an option on a mirror system, and the performance gain/lose
>> is dependent on the round trip speed. If for every digest error I have an
>> error recovery cycle, delays, and stalls. Then no it is not better. Not
>> to mention some iscsi-targets that reset and the all session must be
>> re-established.
> 
> Your suggestion of putting processes to sleep while I/O is pending will
> degrade performance for everyone; that's not really an acceptable
> tradeoff for improving one corner case.
> 

I'm not suggesting that. I'm suggesting sleep on per-page basis. Only the page
been written is blocked. And again do that only if a device sets a flag.
A dm-raid1 will prefer these stalls, to the realloc+copy of the complete IO stream.

I guess we can also sort out two cases here. 
[1] Write-behind vr write-to-page-cache. and 
[2] memmap vr any-write-out.

Looks like [1] is the more common. Maybe we can just remove pages from cache before
writing them so new writes to same index need to allocate new cache pages.

Also for case [2] we can unmap the written-from pages and if re-written too,
map new physical pages for them.

But that looks like a project that will take years. I'll see what comes up.

>>> What you likely want is a way of telling that the page got re-written so
>>> you don't need to print out scary warning messages about parity
>>> problems.
>>>
>>
>> Maybe that is a start. I guess I could signal a fast abort for these. What
>> would be the cost for this knowledge. I guess O(sglist-size) right? loop
>> on all pages and check? Anything better we can do?
> 
> I think it's a page flag indicating write begun on current page.  it
> gets set when I/O is begun and reset if another write comes in in the
> meantime.  Thus you can check before issue if this flag is set ... if it
> is, your digest is likely set.  If not, you need to discard the page
> from the I/O (or redo the digest).
> 

Yes I thought so. The race here is bad so it will only eliminate some of
the bad transitions, not all.

> James
> 
> 

Thanks
Boaz

Re: DIF/DIX updates for 2.6.32

[James Bottomley]

On Thu, 2009-08-27 at 18:18 +0300, Boaz Harrosh wrote:
> >>> What you likely want is a way of telling that the page got re-written so
> >>> you don't need to print out scary warning messages about parity
> >>> problems.
> >>>
> >>
> >> Maybe that is a start. I guess I could signal a fast abort for these. What
> >> would be the cost for this knowledge. I guess O(sglist-size) right? loop
> >> on all pages and check? Anything better we can do?
> > 
> > I think it's a page flag indicating write begun on current page.  it
> > gets set when I/O is begun and reset if another write comes in in the
> > meantime.  Thus you can check before issue if this flag is set ... if it
> > is, your digest is likely set.  If not, you need to discard the page
> > from the I/O (or redo the digest).
> > 
> 
> Yes I thought so. The race here is bad so it will only eliminate some of
> the bad transitions, not all.

But surely mitigation is good enough?  The array will block digest
failures on its own ... we can recheck the flag when this happens to see
if we lost the race so not print the messages.  Winning this before
issue race most of the time still gives good performance.

James

Re: DIF/DIX updates for 2.6.32

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@suse.de> writes:

>> This is not an option on a mirror system, and the performance
>> gain/lose is dependent on the round trip speed. If for every digest
>> error I have an error recovery cycle, delays, and stalls. Then no it
>> is not better. Not to mention some iscsi-targets that reset and the
>> all session must be re-established.

James> Your suggestion of putting processes to sleep while I/O is
James> pending will degrade performance for everyone; that's not really
James> an acceptable tradeoff for improving one corner case.

I disagree with the notion that this is a corner case.

Originally we locked down pages completely during I/O.  Then the page
lock was split, introducing the page writeback bit.  The writeback bit
is set when the I/O is actually issued and cleared upon completion.  So
the page contents only need to be stable during that window.

XFS and btrfs both make use of the writeback bit, waiting for it to be
cleared before reissuing I/O to the same page.  ext[23] (and maybe 4)
don't.  Some of this is poor conversion to the new page cache API, some
of it, I believe, is intentional.

I agree with Boaz' assertion that changing pages in flight is a bad
practice.  It's been kind-of-ok in the single-disk case.  But once we
get into crypto, RAID, iSCSI and DIX/DIF territory things start falling
apart.

We already buffer things in the page cache once.  Having to do
multi-buffering or copy-pages-on-write and reissue I/O because
filesystems engage in dubious practices is crappy.

In my opinion ext[234] should simply be fixed.  If there's a significant
performance hit on those filesystems we could make the wait conditional
on a block_device flag.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: DIF/DIX updates for 2.6.32

[Chris Mason]

On Thu, Aug 27, 2009 at 04:02:41PM -0400, Martin K. Petersen wrote:
> >>>>> "James" == James Bottomley <James.Bottomley@suse.de> writes:
> 
> >> This is not an option on a mirror system, and the performance
> >> gain/lose is dependent on the round trip speed. If for every digest
> >> error I have an error recovery cycle, delays, and stalls. Then no it
> >> is not better. Not to mention some iscsi-targets that reset and the
> >> all session must be re-established.
> 
> James> Your suggestion of putting processes to sleep while I/O is
> James> pending will degrade performance for everyone; that's not really
> James> an acceptable tradeoff for improving one corner case.
> 
[ changing pages in flight ]

> 
> In my opinion ext[234] should simply be fixed.  If there's a significant
> performance hit on those filesystems we could make the wait conditional
> on a block_device flag.

I agree here.  It is pretty trivial in the filesystem to wait for pages
in flight, and it shouldn't be the job of the lower layers to double
buffer 100% of the time.

-chris

DIF/DIX updates for 2.6.32

[Martin K. Petersen]

Updates to DIF/DIX for 2.6.32.  Only changes since last post are:

 - Add a flag to control deallocation of 32-byte CDBs

 - Removed stray BUG_ON() in sd.c

DIF/DIX updates for 2.6.32

[Martin K. Petersen]

Updates since last post:

 - The CDB allocation has been moved to sd.c and the Type 2 support collapsed
   into a single patch.

The first three patches constitute fixes.  The last two contain new features.

--  
Martin K. Petersen	Oracle Linux Engineering