On 10/02/2010 11:43 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
>
> Greetings all,
>
> This patch allows TCM Core to process CDBs containing a received scsi_command_size()
> larger than the hardcoded default of TCM_MAX_COMMAND_SIZE (32-bytes). This patch
> first converts the hardcoded per struct se_cmd -> struct se_transport_task->t_task_cdb[]
> to ->__t_task_cdb[TCM_MAX_COMMAND_SIZE], and turns ->t_task_cdb into a pointer used
> to reference ->__t_task_cdb or an extended CDB buffer setup in transport_generic_allocate_tasks().
>
> It next adds a new struct se_subsystem_api->get_max_cdb_len() API caller that is
> used by TCM subsystem plugins to report their individual CDB size limits. Next
> transport_generic_cmd_sequencer() has been changed to use ->get_max_cdb_len() for
> VARIABLE_LENGTH_CMD to determine if the backstore can support the received
>> TCM_MAX_COMMAND_SIZE sized CDB, or to fail the I/O with exception status.
^^ ==> ->get_max_cdb_len()
>
I'm commenting on the change log here. Comments on actual code is below.
First: there are bigger then ->get_max_cdb_len() which are not VARIABLE_LENGTH_CMD, and
there are VARIABLE_LENGTH_CMD which are smaller then 16 bytes. So the VARIABLE_LENGTH_CMD
has nothing to do with it. It's all a matter of scsi_command_size(cmnd);
Second: I don't like that ->get_max_cdb_len(), it means nothing. The fact that the size
check passed, does not mean the actual SCSI-COMMAND is supported. So plugins should be
ready to receive unsupported commands and properly return the correct error. If an
FILEIO does not support OSD_ACT_CREATE it's because it's an OSD command and not because
it is big. Maintaining the proper ->get_max_cdb_len() as code evolves is just a maintenance
headache that buys you nothing!
Note that in pSCSI passthrough plugin the request will fail soon enough and the proper
error handling should be in place, already.
> The extended CDB buffer is setup in transport_generic_allocate_tasks() when necessary,
> and a new SCF_ECDB_ALLOCATION flag is then set. The release of the extended CDB buffer
> happens in transport_free_se_cmd() in the normal struct se_cmd release path.
You have a magic size number which is sizeof(->__t_task_cdb) bigger then "that" alloc.
Then also bigger then "that" free. No need for a flag.
But I'd even do more. if (->t_task_cdb != ->__t_task_cdb) free
>
> This patch also updates TCM/pSCSI to handle extended CDBs up to PSCSI_MAX_CDB_SIZE=240
> (maximum current OSD CDB size) and sets up the extra struct pscsi_plugin_task->pscsi_cdb
No! the max CDB size is SCSI_MAX_VARLEN_CDB_SIZE (260) defined in scsi/scsi.h. Including
the varlen_cdb 8 bytes header. This is do to the fact that the header has a single byte
to denote payload size and must be 4 bytes aligned so 252 + 8 bytes header.
See "struct scsi_varlen_cdb_hdr" in scsi.h
> allocation in struct se_task context in order to complete the > TCM_MAX_COMMAND_SIZE=32
> request. The IBLOCK, FILEIO, RAMDISK, and STGT subsystem plugins have been updated to
> enforce the TCM_MAX_COMMAND_SIZE=32 limit via struct se_subsystem_api->get_max_cdb_len().
>
> So far this code been tested with 'sgv4_xdwriteread -e' into a TCM_Loop -> TCM/pSCSI
> -> scsi_debug backstore with a TCM_MAX_COMMAND_SIZE=16 to force the extended CDB
> allocations. This was done due to the lack of an easy method to test > 32-byte CDBs
> w/o an TCM/OSD fabric module.
>
> Many thanks to Boaz Harrosh for his help in this area!
>
> Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_file.c | 6 +++++
> drivers/target/target_core_iblock.c | 6 +++++
> drivers/target/target_core_pscsi.c | 39 ++++++++++++++++++++++++++++++++
> drivers/target/target_core_pscsi.h | 6 ++++-
> drivers/target/target_core_rd.c | 6 +++++
> drivers/target/target_core_stgt.c | 6 +++++
> drivers/target/target_core_transport.c | 34 +++++++++++++++++++++++----
> include/target/target_core_base.h | 6 +++-
> include/target/target_core_transport.h | 5 ++++
> 9 files changed, 106 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index d0afb50..bf9bfc2 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -1125,6 +1125,11 @@ static unsigned char *fd_get_cdb(struct se_task *task)
> return req->fd_scsi_cdb;
> }
>
> +static u32 fd_get_max_cdb_len(struct se_device *dev)
> +{
> + return TCM_MAX_COMMAND_SIZE;
> +}
> +
> /* fd_get_blocksize(): (Part of se_subsystem_api_t template)
> *
> *
> @@ -1224,6 +1229,7 @@ static struct se_subsystem_api fileio_template = {
> .check_lba = fd_check_lba,
> .check_for_SG = fd_check_for_SG,
> .get_cdb = fd_get_cdb,
> + .get_max_cdb_len = fd_get_max_cdb_len,
> .get_blocksize = fd_get_blocksize,
> .get_device_rev = fd_get_device_rev,
> .get_device_type = fd_get_device_type,
> diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
> index 2dae7fd..3337357 100644
> --- a/drivers/target/target_core_iblock.c
> +++ b/drivers/target/target_core_iblock.c
> @@ -1059,6 +1059,11 @@ static unsigned char *iblock_get_cdb(struct se_task *task)
> return req->ib_scsi_cdb;
> }
>
> +static u32 iblock_get_max_cdb_len(struct se_device *dev)
> +{
> + return TCM_MAX_COMMAND_SIZE;
> +}
> +
> static u32 iblock_get_blocksize(struct se_device *dev)
> {
> struct iblock_dev *ibd = dev->dev_ptr;
> @@ -1167,6 +1172,7 @@ static struct se_subsystem_api iblock_template = {
> .check_lba = iblock_check_lba,
> .check_for_SG = iblock_check_for_SG,
> .get_cdb = iblock_get_cdb,
> + .get_max_cdb_len = iblock_get_max_cdb_len,
> .get_blocksize = iblock_get_blocksize,
> .get_device_rev = iblock_get_device_rev,
> .get_device_type = iblock_get_device_type,
> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
> index 8dfedb1..b550725 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -714,13 +714,40 @@ static void *pscsi_allocate_request(
> struct se_task *task,
> struct se_device *dev)
> {
> + struct se_cmd *cmd = task->task_se_cmd;
> struct pscsi_plugin_task *pt;
> + unsigned char *cdb = T_TASK(cmd)->t_task_cdb;
>
> pt = kzalloc(sizeof(struct pscsi_plugin_task), GFP_KERNEL);
> if (!(pt)) {
> printk(KERN_ERR "Unable to allocate struct pscsi_plugin_task\n");
> return NULL;
> }
> + /*
> + * If TCM Core is signaling a > TCM_MAX_COMMAND_SIZE allocation,
> + * allocate the extended CDB buffer for per struct se_task context
> + * pt->pscsi_cdb now.
> + */
> + if (cmd->se_cmd_flags & SCF_ECDB_ALLOCATION) {
Just check the size here, Why do you need the flag.
> + /*
> + * PSCSI_MAX_CDB_SIZE is currently set to 240 bytes which
> + * allows the largest OSD CDB sizes again.
> + */
> + if (scsi_command_size(cdb) > PSCSI_MAX_CDB_SIZE) {
> + printk(KERN_ERR "pSCSI: Received CDB of size: %u larger"
> + " than PSCSI_MAX_CDB_SIZE: %u\n",
> + scsi_command_size(cdb), PSCSI_MAX_CDB_SIZE);
> + return NULL;
> + }
> +
> + pt->pscsi_cdb = kzalloc(scsi_command_size(cdb), GFP_KERNEL);
> + if (!(pt->pscsi_cdb)) {
> + printk(KERN_ERR "pSCSI: Unable to allocate extended"
> + " pt->pscsi_cdb\n");
> + return NULL;
> + }
> + } else
> + pt->pscsi_cdb = &pt->__pscsi_cdb[0];
What? another copy and another pointer. Why? Why can't you just pass to
block layer the command from T_TASK(cmd)->t_task_cdb. Don't you keep the
T_TASK(cmd) associated with the pscsi_plugin_task up until completion.
(The complete hunk looks pointless)
>
> return pt;
> }
> @@ -818,6 +845,12 @@ static void pscsi_free_task(struct se_task *task)
> {
> struct pscsi_plugin_task *pt = task->transport_req;
> /*
> + * Release the extended CDB allocation from pscsi_allocate_request()
> + * if one exists.
> + */
> + if (task->task_se_cmd->se_cmd_flags & SCF_ECDB_ALLOCATION)
> + kfree(pt->pscsi_cdb);
> + /*
Here! you have the struct se_task up to the end. pt->pscsi_cdb is totally
redundant.
> * We do not release the bio(s) here associated with this task, as
> * this is handled by bio_put() and pscsi_bi_endio().
> */
> @@ -1366,6 +1399,11 @@ static unsigned char *pscsi_get_cdb(struct se_task *task)
> return pt->pscsi_cdb;
> }
>
> +static u32 pscsi_get_max_cdb_len(struct se_device *dev)
> +{
> + return PSCSI_MAX_CDB_SIZE;
> +}
> +
I don't think you need that, but if you do why not return the scsi_host->max_cmd_len ?
> /* pscsi_get_sense_buffer():
> *
> *
> @@ -1533,6 +1571,7 @@ static struct se_subsystem_api pscsi_template = {
> .check_lba = pscsi_check_lba,
> .check_for_SG = pscsi_check_for_SG,
> .get_cdb = pscsi_get_cdb,
> + .get_max_cdb_len = pscsi_get_max_cdb_len,
> .get_sense_buffer = pscsi_get_sense_buffer,
> .get_blocksize = pscsi_get_blocksize,
> .get_device_rev = pscsi_get_device_rev,
> diff --git a/drivers/target/target_core_pscsi.h b/drivers/target/target_core_pscsi.h
> index d3c8972..e7f07e3 100644
> --- a/drivers/target/target_core_pscsi.h
> +++ b/drivers/target/target_core_pscsi.h
> @@ -9,6 +9,9 @@
> #define INQUIRY_DATA_SIZE 0x24
> #endif
>
> +/* Maximum extended CDB size for handling OSD passthrough */
> +#define PSCSI_MAX_CDB_SIZE 240
> +
Wrong and redundant use SCSI_MAX_VARLEN_CDB_SIZE
> /* used in pscsi_add_device_to_list() */
> #define PSCSI_DEFAULT_QUEUEDEPTH 1
>
> @@ -28,7 +31,8 @@ extern int linux_blockdevice_check(int, int);
> #include <linux/kobject.h>
>
> struct pscsi_plugin_task {
> - unsigned char pscsi_cdb[TCM_MAX_COMMAND_SIZE];
> + unsigned char *pscsi_cdb;
> + unsigned char __pscsi_cdb[TCM_MAX_COMMAND_SIZE];
As said above just drop it.
> unsigned char pscsi_sense[SCSI_SENSE_BUFFERSIZE];
> int pscsi_direction;
> int pscsi_result;
> diff --git a/drivers/target/target_core_rd.c b/drivers/target/target_core_rd.c
> index 1debdd3..0a54c57 100644
> --- a/drivers/target/target_core_rd.c
> +++ b/drivers/target/target_core_rd.c
> @@ -1392,6 +1392,11 @@ static unsigned char *rd_get_cdb(struct se_task *task)
> return req->rd_scsi_cdb;
> }
>
> +static u32 rd_get_max_cdb_len(struct se_device *dev)
> +{
> + return TCM_MAX_COMMAND_SIZE;
> +}
> +
> /* rd_get_blocksize(): (Part of se_subsystem_api_t template)
> *
> *
> @@ -1475,6 +1480,7 @@ static struct se_subsystem_api rd_dr_template = {
> .check_lba = rd_DIRECT_check_lba,
> .check_for_SG = rd_check_for_SG,
> .get_cdb = rd_get_cdb,
> + .get_max_cdb_len = rd_get_max_cdb_len,
> .get_blocksize = rd_get_blocksize,
> .get_device_rev = rd_get_device_rev,
> .get_device_type = rd_get_device_type,
> diff --git a/drivers/target/target_core_stgt.c b/drivers/target/target_core_stgt.c
> index fdcd7eb..ecd4601 100644
> --- a/drivers/target/target_core_stgt.c
> +++ b/drivers/target/target_core_stgt.c
> @@ -751,6 +751,11 @@ static unsigned char *stgt_get_cdb(struct se_task *task)
> return pt->stgt_cdb;
> }
>
> +static u32 stgt_get_max_cdb_len(struct se_device *dev)
> +{
> + return TCM_MAX_COMMAND_SIZE;
> +}
> +
Surely not maximum for stgt. Or I missed the meaning of this completely.
> /* stgt_get_sense_buffer():
> *
> *
> @@ -931,6 +936,7 @@ static struct se_subsystem_api stgt_template = {
> .check_lba = stgt_check_lba,
> .check_for_SG = stgt_check_for_SG,
> .get_cdb = stgt_get_cdb,
> + .get_max_cdb_len = stgt_get_max_cdb_len,
> .get_sense_buffer = stgt_get_sense_buffer,
> .get_blocksize = stgt_get_blocksize,
> .get_device_rev = stgt_get_device_rev,
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 830c5bb..866b0fa 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2860,6 +2860,11 @@ void transport_free_se_cmd(
> if (se_cmd->se_tmr_req)
> core_tmr_release_req(se_cmd->se_tmr_req);
> /*
> + * Check and free any extended CDB buffer that was allocated
> + */
> + if (se_cmd->se_cmd_flags & SCF_ECDB_ALLOCATION)
> + kfree(T_TASK(se_cmd)->t_task_cdb);
What about if (T_TASK(se_cmd)->t_task_cdb != T_TASK(se_cmd)->__t_task_cdb)
> + /*
> * Release any optional TCM fabric dependent iovecs allocated by
> * transport_allocate_iovecs_for_cmd()
> */
> @@ -2903,6 +2908,23 @@ int transport_generic_allocate_tasks(
> if (non_data_cdb < 0)
> return -1;
> /*
> + * If the received CDB is larger than TCM_MAX_COMMAND_SIZE,
> + * allocate the additional extended CDB buffer now.. Otherwise
> + * setup the pointer from __t_task_cdb to t_task_cdb.
> + */
> + if (scsi_command_size(cdb) > TCM_MAX_COMMAND_SIZE) {
^^ sizeof(T_TASK(cmd)->__t_task_cdb)
> + T_TASK(cmd)->t_task_cdb = kzalloc(scsi_command_size(cdb),
> + GFP_KERNEL);
> + if (!(T_TASK(cmd)->t_task_cdb)) {
> + printk(KERN_ERR "Unable to allocate T_TASK(cmd)->t_task_cdb"
> + " %u > TCM_MAX_COMMAND_SIZE ops\n",
> + scsi_command_size(cdb));
> + return -1;
> + }
> + cmd->se_cmd_flags |= SCF_ECDB_ALLOCATION;
> + } else
> + T_TASK(cmd)->t_task_cdb = &T_TASK(cmd)->__t_task_cdb[0];
T_TASK(cmd)->t_task_cdb = T_TASK(cmd)->__t_task_cdb;
> + /*
> * Copy the original CDB into T_TASK(cmd).
> */
> memcpy(T_TASK(cmd)->t_task_cdb, cdb, scsi_command_size(cdb));
> @@ -5746,13 +5768,15 @@ static int transport_generic_cmd_sequencer(
> service_action = get_unaligned_be16(&cdb[8]);
> /*
> * Check the additional CDB length (+ 8 bytes for header) does
> - * not exceed our TCM_MAX_COMMAND_SIZE.
> + * not exceed our backsores ->get_max_cdb_len()
> */
> - if (scsi_varlen_cdb_length(&cdb[0]) > TCM_MAX_COMMAND_SIZE) {
> + if (scsi_varlen_cdb_length(&cdb[0]) >
> + TRANSPORT(dev)->get_max_cdb_len(dev)) {
Again VARIABLE_LENGTH_CMD can be small as well as none-VARIABLE_LENGTH_CMD
can be big. The check should be more generic and against scsi_command_size(cdb).
BUT!! Please just drop that ->get_max_cdb_len(dev) it's useless.
> printk(KERN_INFO "Only %u-byte extended CDBs currently"
> - " supported for VARIABLE_LENGTH_CMD, received:"
> - " %d for service action: 0x%04x\n",
> - TCM_MAX_COMMAND_SIZE,
> + " supported for VARIABLE_LENGTH_CMD backstore %s,"
> + " received: %d for service action: 0x%04x\n",
> + TRANSPORT(dev)->get_max_cdb_len(dev),
> + TRANSPORT(dev)->name,
> scsi_varlen_cdb_length(&cdb[0]), service_action);
> return TGCS_INVALID_CDB_FIELD;
> }
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 3905a0e..fc5300c 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -137,7 +137,8 @@ enum se_cmd_flags_table {
> SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00800000,
> SCF_EMULATE_SYNC_CACHE = 0x01000000,
> SCF_EMULATE_CDB_ASYNC = 0x02000000,
> - SCF_EMULATE_SYNC_UNMAP = 0x04000000
> + SCF_EMULATE_SYNC_UNMAP = 0x04000000,
> + SCF_ECDB_ALLOCATION = 0x08000000
> };
>
> /* struct se_device->type for known subsystem plugins */
> @@ -424,7 +425,8 @@ struct se_queue_obj {
> * drivers/target/target_core_transport.c:__transport_alloc_se_cmd()
> */
> struct se_transport_task {
> - unsigned char t_task_cdb[MAX_COMMAND_SIZE];
> + unsigned char *t_task_cdb;
> + unsigned char __t_task_cdb[MAX_COMMAND_SIZE];
> unsigned long long t_task_lba;
> int t_tasks_failed;
> int t_tasks_fua;
> diff --git a/include/target/target_core_transport.h b/include/target/target_core_transport.h
> index ab7336b..d45063e 100644
> --- a/include/target/target_core_transport.h
> +++ b/include/target/target_core_transport.h
> @@ -519,6 +519,11 @@ struct se_subsystem_api {
> */
> unsigned char *(*get_cdb)(struct se_task *);
> /*
> + * get_max_cdb_len(): Used by subsystems backstoers to signal the
> + * maximum receivable SCSI CDB size.
> + */
> + u32 (*get_max_cdb_len)(struct se_device *);
> + /*
> * get_blocksize():
> */
> u32 (*get_blocksize)(struct se_device *);
Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
