[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

[Scott Guthridge]

The patch we suggested where we simply changed BLK_MAX_CDB and MAX_COMMAND_SIZE from 16 to 32 was meant to be a "stick in the sand".  It's the simplest description of the functionality we're looking for.  Other approaches that accomplish the same thing would be fine.

I don't know if the reason that "sd" went with the mempool for 32-byte commands was binary compatibility with modules, concerns about memory usage on small/embedded systems, or both.  But given that "sd" uses the mempool, it's reasonable to assume that greater than 16 byte pass-through requests might also take this approach.  A potential advantage of using a mempool or other dynamic allocation for the pass-through CDB is that it could be implemented to allow arbitrary CDB sizes, which would be more general than just increasing the fixed limit from 16 to 32.

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

All of the SCSI drives we're seeing now come from the vendor formatted with PI type 2.  Linux automatically detects the format and uses PI, so it's no longer a corner case -- it's now the normal case.

If you accept that dynamically allocated CDB's have become the rule rather than the exception, then it makes sense that the fixed size __cmd buffer in struct request should eventually be phased out.

Two places in the kernel accept SG_IO (pass-through) requests.  One is the "sg" driver; the other (and the one we actually care about more) is scsi_cmd_ioctl in block/scsi_ioctl.c, which handles pass-through requests sent directly to an "sd" device.

What we are looking for is simply to be able to send 32-byte pass-through commands.  These commands do not have the  prot_op and prot_type fields of struct scsi_cmnd set, so from the perspective of the Linux SCSI subsystem and the adapter firmware, they are -not- PI I/O's.  But if a pass-through command sent to the device happened to be a 32-byte read with RDPROTECT=3, for example, then the drive will return the PI interleaved with the data -- it's this raw pass-through functionality that we need.  Conveniently, increasing the CDB size doesn't break binary compatibility with the existing sg_io_hdr_t.

One could develop a more general pass-through command that gave the user access to the prot_op and prot_type fields and that provided a DIX style side-buffer to hold de-interleaved PI data.  That would be a nice extension, but it's not something we currently need.

Scott Guthridge
IBM Almaden Research Center

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

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

[Martin K. Petersen]

>>>>> "Scott" == Scott Guthridge <guthridg@us.ibm.com> writes:

Scott,

Scott> I don't know if the reason that "sd" went with the mempool for
Scott> 32-byte commands was binary compatibility with modules, concerns
Scott> about memory usage on small/embedded systems, or both.  

The latter. Type 2 is an edge case and we didn't want to penalize
everybody on the planet by doubling the default command size.


Scott> All of the SCSI drives we're seeing now come from the vendor
Scott> formatted with PI type 2.  Linux automatically detects the format
Scott> and uses PI, so it's no longer a corner case -- it's now the
Scott> normal case.

Drives don't come as Type 2 by default. You have to ask for them to be
formatted that way. All the drives we ship at Oracle are Type 1.

The main reason I'm objecting to the use of Type 2 is that the
protection scheme offered is a poor match for a general purpose OS. If
you look closely at how we treat Type 2 device you'll see that in
reality we drive them as if they were Type 1. I.e. the expected initial
reference tag is set to the LBA. This renders the additional
functionality offered by Type 2 useless and so effectively you get a
Type 1 drive with the cost of a bigger CDB. Plus there are several HBAs
out there that do not even support 32-byte CDBs.


Scott> What we are looking for is simply to be able to send 32-byte
Scott> pass-through commands.

That works today, though. If you send a passthrough command that's
bigger than BLK_MAX_CDB we'll allocate a separate cmd buffer and pin it
to the request. Much like sd does.


Scott> But if a pass-through command sent to the device happened to be a
Scott> 32-byte read with RDPROTECT=3, for example, then the drive will
Scott> return the PI interleaved with the data -- it's this raw
Scott> pass-through functionality that we need.

Also shouldn't be a problem.

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Martin K. Petersen
> Sent: Wednesday, 19 December, 2012 10:56 AM
> To: Scott Guthridge
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 0/2] Change type-2 dif to use rq embedded 32 byte cdb
> 
> >>>>> "Scott" == Scott Guthridge <guthridg@us.ibm.com> writes:
> 
> Scott,
> 
> Scott> I don't know if the reason that "sd" went with the mempool for
> Scott> 32-byte commands was binary compatibility with modules, concerns
> Scott> about memory usage on small/embedded systems, or both.
> 
> The latter. Type 2 is an edge case and we didn't want to penalize
> everybody on the planet by doubling the default command size.
> 
> 
> Scott> All of the SCSI drives we're seeing now come from the vendor
> Scott> formatted with PI type 2.  Linux automatically detects the format
> Scott> and uses PI, so it's no longer a corner case -- it's now the
> Scott> normal case.
> 
> Drives don't come as Type 2 by default. You have to ask for them to be
> formatted that way. All the drives we ship at Oracle are Type 1.
> 

And all the SCSI drives we ship at HP have no DIF whatsoever.

Same for all ATA drives from all companies, since DIF isn't even defined in ATA, and all USB drives (although SCSI based, the consumer market doesn't use this feature).

---
Rob Elliott    HP Server Storage

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

[Douglas Gilbert]

On 12-12-19 05:34 AM, Scott Guthridge wrote:
> The patch we suggested where we simply changed BLK_MAX_CDB and MAX_COMMAND_SIZE from 16 to 32 was meant to be a "stick in the sand".  It's the simplest description of the functionality we're looking for.  Other approaches that accomplish the same thing would be fine.
>
> I don't know if the reason that "sd" went with the mempool for 32-byte commands was binary compatibility with modules, concerns about memory usage on small/embedded systems, or both.  But given that "sd" uses the mempool, it's reasonable to assume that greater than 16 byte pass-through requests might also take this approach.  A potential advantage of using a mempool or other dynamic allocation for the pass-through CDB is that it could be implemented to allow arbitrary CDB sizes, which would be more general than just increasing the fixed limit from 16 to 32.
>
>>> 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.
>
> All of the SCSI drives we're seeing now come from the vendor formatted with PI type 2.  Linux automatically detects the format and uses PI, so it's no longer a corner case -- it's now the normal case.
>
> If you accept that dynamically allocated CDB's have become the rule rather than the exception, then it makes sense that the fixed size __cmd buffer in struct request should eventually be phased out.
>
> Two places in the kernel accept SG_IO (pass-through) requests.  One is the "sg" driver; the other (and the one we actually care about more) is scsi_cmd_ioctl in block/scsi_ioctl.c, which handles pass-through requests sent directly to an "sd" device.

Scott,
You overlooked the bsg driver which will allow you to issue
32 byte cdbs (and larger). It is probably about time I
submitted a patch to allow the sg driver to handle cdbs
larger than 16 bytes.

> What we are looking for is simply to be able to send 32-byte pass-through commands.  These commands do not have the  prot_op and prot_type fields of struct scsi_cmnd set, so from the perspective of the Linux SCSI subsystem and the adapter firmware, they are -not- PI I/O's.  But if a pass-through command sent to the device happened to be a 32-byte read with RDPROTECT=3, for example, then the drive will return the PI interleaved with the data -- it's this raw pass-through functionality that we need.  Conveniently, increasing the CDB size doesn't break binary compatibility with the existing sg_io_hdr_t.

If you follow the code for sg_write_same in sg3_utils (version 1.34)
there is a case in which the 32 byte cdb is issued. To work in Linux
it must take the path for bsg through my lower level library. That
will end up populating an instance of struct sg_io_v4 and sending
it to a bsg device node via the SG_IO ioctl.

You might also look at the examples/bsg_queue_tst.c file in the
sg3_utils tarball.

Doug Gilbert