[PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[Rob Evers]

These patches replace the original t10 type 2 dif mempool implementation
by increasing the block and scsi cdb maximum sizes from 16 to 32 bytes.
The cdb embedded in the request structure can then be used for type 2
dif commands, or other 32 byte cdbs as required.

Motivation for this is that type-2 dif commands should be treated as
any other read/writes generally, without any performance penalty.

This patch set conflicts with a patch previously posted which addresses
a race in UA induced retries with type 2 dif commands.  These patches also
address that problem:

http://marc.info/?l=linux-scsi&m=135186352200668&w=2

Testing:

Readily reproduced the race condition panic by simultaneously
inducing UAs while load testing 8 scsi_debug devices.  With patch
applied, same testing ran for 15 hours without a panic.

Sanity tested performance using scsi_debug, with and without
type 2 dif enabled.  Order 80% performance increase noticed
with patches when scsi_debug was running in dif type 2 mode.
Running scsi_debug without type 2 dif, performance was approximately
equal.

Used 3.7.0-rc5 for testing and patch generation.

root (2):
  Change the cdb size limits in block and scsi to 32 bytes
  Change dif type 2 commands to use embedded 32 byte cdb

 drivers/scsi/sd.c        | 45 +--------------------------------------------
 drivers/scsi/sd.h        |  5 -----
 include/linux/blkdev.h   |  2 +-
 include/scsi/scsi_cmnd.h | 13 ++-----------
 4 files changed, 4 insertions(+), 61 deletions(-)

-- 
1.7.11.7

[PATCH 1/2] Change the cdb size limits in block and scsi to 32 bytes

[Rob Evers]

Signed-off-by: Rob Evers <revers@redhat.com>
---
 include/linux/blkdev.h   | 2 +-
 include/scsi/scsi_cmnd.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1756001..cafce6e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -85,7 +85,7 @@ enum rq_cmd_type_bits {
 	REQ_TYPE_ATA_PC,
 };
 
-#define BLK_MAX_CDB	16
+#define BLK_MAX_CDB	32
 
 /*
  * try to put the fields that are referenced together in the same cacheline.
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index de5f5d8..9890d29 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -24,7 +24,7 @@ struct scsi_driver;
  * So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml
  * supports without specifying a cmd_len by ULD's
  */
-#define MAX_COMMAND_SIZE 16
+#define MAX_COMMAND_SIZE 32
 #if (MAX_COMMAND_SIZE > BLK_MAX_CDB)
 # error MAX_COMMAND_SIZE can not be bigger than BLK_MAX_CDB
 #endif
-- 
1.7.11.7

[PATCH 2/2] Change dif type 2 commands to use embedded 32 byte cdb

[Rob Evers]

The original implementation of type 2 dif in sd.c used
a mempool to allocate scsi cdbs that were 32 bytes in
length.

With the previous change in this set, 32 byte cdbs are
available in the request structure, removing the need
for the mempool.

Also fixed up a comment regarding MAX_COMMAND_SIZE

Signed-off-by: Rob Evers <revers@redhat.com>
---
 drivers/scsi/sd.c        | 45 +--------------------------------------------
 drivers/scsi/sd.h        |  5 -----
 include/scsi/scsi_cmnd.h | 11 +----------
 3 files changed, 2 insertions(+), 59 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 12f6fdf..f72ae4a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -122,9 +122,6 @@ static DEFINE_IDA(sd_index_ida);
  * object after last put) */
 static DEFINE_MUTEX(sd_ref_mutex);
 
-static struct kmem_cache *sd_cdb_cache;
-static mempool_t *sd_cdb_pool;
-
 static const char *sd_cache_types[] = {
 	"write through", "none", "write back",
 	"write back, no read (daft)"
@@ -852,14 +849,8 @@ static int sd_prep_fn(struct request_queue *q, struct request *rq)
 		protect = 0;
 
 	if (host_dif == SD_DIF_TYPE2_PROTECTION) {
-		SCpnt->cmnd = mempool_alloc(sd_cdb_pool, GFP_ATOMIC);
-
-		if (unlikely(SCpnt->cmnd == NULL)) {
-			ret = BLKPREP_DEFER;
-			goto out;
-		}
 
-		SCpnt->cmd_len = SD_EXT_CDB_SIZE;
+		SCpnt->cmd_len = MAX_COMMAND_SIZE;
 		memset(SCpnt->cmnd, 0, SCpnt->cmd_len);
 		SCpnt->cmnd[0] = VARIABLE_LENGTH_CMD;
 		SCpnt->cmnd[7] = 0x18;
@@ -1547,21 +1538,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
 	if (rq_data_dir(SCpnt->request) == READ && scsi_prot_sg_count(SCpnt))
 		sd_dif_complete(SCpnt, good_bytes);
 
-	if (scsi_host_dif_capable(sdkp->device->host, sdkp->protection_type)
-	    == SD_DIF_TYPE2_PROTECTION && SCpnt->cmnd != SCpnt->request->cmd) {
-
-		/* We have to print a failed command here as the
-		 * extended CDB gets freed before scsi_io_completion()
-		 * is called.
-		 */
-		if (result)
-			scsi_print_command(SCpnt);
-
-		mempool_free(SCpnt->cmnd, sd_cdb_pool);
-		SCpnt->cmnd = NULL;
-		SCpnt->cmd_len = 0;
-	}
-
 	return good_bytes;
 }
 
@@ -2964,24 +2940,8 @@ static int __init init_sd(void)
 	if (err)
 		goto err_out_class;
 
-	sd_cdb_cache = kmem_cache_create("sd_ext_cdb", SD_EXT_CDB_SIZE,
-					 0, 0, NULL);
-	if (!sd_cdb_cache) {
-		printk(KERN_ERR "sd: can't init extended cdb cache\n");
-		goto err_out_class;
-	}
-
-	sd_cdb_pool = mempool_create_slab_pool(SD_MEMPOOL_SIZE, sd_cdb_cache);
-	if (!sd_cdb_pool) {
-		printk(KERN_ERR "sd: can't init extended cdb pool\n");
-		goto err_out_cache;
-	}
-
 	return 0;
 
-err_out_cache:
-	kmem_cache_destroy(sd_cdb_cache);
-
 err_out_class:
 	class_unregister(&sd_disk_class);
 err_out:
@@ -3001,9 +2961,6 @@ static void __exit exit_sd(void)
 
 	SCSI_LOG_HLQUEUE(3, printk("exit_sd: exiting sd driver\n"));
 
-	mempool_destroy(sd_cdb_pool);
-	kmem_cache_destroy(sd_cdb_cache);
-
 	scsi_unregister_driver(&sd_template.gendrv);
 	class_unregister(&sd_disk_class);
 
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 47c52a6..eec4154 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -34,11 +34,6 @@
 #define SD_LAST_BUGGY_SECTORS	8
 
 enum {
-	SD_EXT_CDB_SIZE = 32,	/* Extended CDB size */
-	SD_MEMPOOL_SIZE = 2,	/* CDB pool size */
-};
-
-enum {
 	SD_LBP_FULL = 0,	/* Full logical block provisioning */
 	SD_LBP_UNMAP,		/* Use UNMAP command */
 	SD_LBP_WS16,		/* Use WRITE SAME(16) with UNMAP bit */
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 9890d29..8f55189 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -13,16 +13,7 @@ struct scsi_device;
 struct scsi_driver;
 
 /*
- * MAX_COMMAND_SIZE is:
- * The longest fixed-length SCSI CDB as per the SCSI standard.
- * fixed-length means: commands that their size can be determined
- * by their opcode and the CDB does not carry a length specifier, (unlike
- * the VARIABLE_LENGTH_CMD(0x7f) command). This is actually not exactly
- * true and the SCSI standard also defines extended commands and
- * vendor specific commands that can be bigger than 16 bytes. The kernel
- * will support these using the same infrastructure used for VARLEN CDB's.
- * So in effect MAX_COMMAND_SIZE means the maximum size command scsi-ml
- * supports without specifying a cmd_len by ULD's
+ * MAX_COMMAND_SIZE is the max size in bytes, a cdb can be.
  */
 #define MAX_COMMAND_SIZE 32
 #if (MAX_COMMAND_SIZE > BLK_MAX_CDB)
-- 
1.7.11.7

Re: [PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[Rob Evers]

cc Lukasz and Maciej from Intel and changing Dan William's email


On 11/21/2012 03:07 PM, Rob Evers wrote:
> These patches replace the original t10 type 2 dif mempool implementation
> by increasing the block and scsi cdb maximum sizes from 16 to 32 bytes.
> The cdb embedded in the request structure can then be used for type 2
> dif commands, or other 32 byte cdbs as required.
>
> Motivation for this is that type-2 dif commands should be treated as
> any other read/writes generally, without any performance penalty.
>
> This patch set conflicts with a patch previously posted which addresses
> a race in UA induced retries with type 2 dif commands.  These patches also
> address that problem:
>
> http://marc.info/?l=linux-scsi&m=135186352200668&w=2
>
> Testing:
>
> Readily reproduced the race condition panic by simultaneously
> inducing UAs while load testing 8 scsi_debug devices.  With patch
> applied, same testing ran for 15 hours without a panic.
>
> Sanity tested performance using scsi_debug, with and without
> type 2 dif enabled.  Order 80% performance increase noticed
> with patches when scsi_debug was running in dif type 2 mode.
> Running scsi_debug without type 2 dif, performance was approximately
> equal.
>
> Used 3.7.0-rc5 for testing and patch generation.
>
> root (2):
>    Change the cdb size limits in block and scsi to 32 bytes
>    Change dif type 2 commands to use embedded 32 byte cdb
>
>   drivers/scsi/sd.c        | 45 +--------------------------------------------
>   drivers/scsi/sd.h        |  5 -----
>   include/linux/blkdev.h   |  2 +-
>   include/scsi/scsi_cmnd.h | 13 ++-----------
>   4 files changed, 4 insertions(+), 61 deletions(-)
>

Re: [PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[Martin K. Petersen]

>>>>> "Rob" == Rob Evers <revers@redhat.com> writes:

Rob> These patches replace the original t10 type 2 dif mempool
Rob> implementation by increasing the block and scsi cdb maximum sizes
Rob> from 16 to 32 bytes.  The cdb embedded in the request structure can
Rob> then be used for type 2 dif commands, or other 32 byte cdbs as
Rob> required.

Rob> Motivation for this is that type-2 dif commands should be treated
Rob> as any other read/writes generally, without any performance
Rob> penalty.

We did the mempool because we did not want to penalize everybody else by
always allocating 32-byte CDBs. Type 2 is a really rare corner case.

So while I agree we should handle the UA scenario correctly, I'm not
sure I agree with the fix. Why are you messing with Type 2 devices in
the first place? These should really only be used inside disk arrays.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[Rob Evers]

On 11/26/2012 06:58 PM, Martin K. Petersen wrote:
>>>>>> "Rob" == Rob Evers<revers@redhat.com>  writes:
> Rob>  These patches replace the original t10 type 2 dif mempool
> Rob>  implementation by increasing the block and scsi cdb maximum sizes
> Rob>  from 16 to 32 bytes.  The cdb embedded in the request structure can
> Rob>  then be used for type 2 dif commands, or other 32 byte cdbs as
> Rob>  required.
>
> Rob>  Motivation for this is that type-2 dif commands should be treated
> Rob>  as any other read/writes generally, without any performance
> Rob>  penalty.
>
> We did the mempool because we did not want to penalize everybody else by
> always allocating 32-byte CDBs. Type 2 is a really rare corner case.

I didn't see a penalty in the non dif case, in the sanity checking I did.
If someone has results that indicate otherwise, please indicate.

> So while I agree we should handle the UA scenario correctly, I'm not
> sure I agree with the fix. Why are you messing with Type 2 devices in
> the first place? These should really only be used inside disk arrays.
>

The previous fix for the mempool race condition was found when
one of our partners reported a problem in a system that had
type 2 dif storage attached to the host.

Use of BLK_MAX_CDB and MAX_COMMAND_SIZE could
use some feedback.

Re: [PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb

[Rob Evers]

On 11/21/2012 03:07 PM, Rob Evers wrote:
> These patches replace the original t10 type 2 dif mempool implementation
> by increasing the block and scsi cdb maximum sizes from 16 to 32 bytes.
> The cdb embedded in the request structure can then be used for type 2
> dif commands, or other 32 byte cdbs as required.
>
> Motivation for this is that type-2 dif commands should be treated as
> any other read/writes generally, without any performance penalty.
>
> This patch set conflicts with a patch previously posted which addresses
> a race in UA induced retries with type 2 dif commands.  These patches also
> address that problem:
>
> http://marc.info/?l=linux-scsi&m=135186352200668&w=2
>
> Testing:
>
> Readily reproduced the race condition panic by simultaneously
> inducing UAs while load testing 8 scsi_debug devices.  With patch
> applied, same testing ran for 15 hours without a panic.
>
> Sanity tested performance using scsi_debug, with and without
> type 2 dif enabled.  Order 80% performance increase noticed
> with patches when scsi_debug was running in dif type 2 mode.
> Running scsi_debug without type 2 dif, performance was approximately
> equal.
>
> Used 3.7.0-rc5 for testing and patch generation.
>
> root (2):
>    Change the cdb size limits in block and scsi to 32 bytes
>    Change dif type 2 commands to use embedded 32 byte cdb
>
>   drivers/scsi/sd.c        | 45 +--------------------------------------------
>   drivers/scsi/sd.h        |  5 -----
>   include/linux/blkdev.h   |  2 +-
>   include/scsi/scsi_cmnd.h | 13 ++-----------
>   4 files changed, 4 insertions(+), 61 deletions(-)
>

James,

Can you give some input on this?

Thanks, Rob