Re: [PATCH 1/3] tcm: Add support for BIDI-COMMAND passthrough

From: Boaz Harrosh
Date: Wed Sep 22 2010 - 06:35:32 EST

On 09/22/2010 08:08 AM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
>
> This patch adds support for BIDI-COMMAND passthrough into TCM Core by adding
> new struct se_task->task_sg_bidi[] for the TCM/pSCSI backstore case to setup the
> extra mapping for the SCSI READ payload from T_TASK(cmd)->t_mem_bidi_list.
>
> This patch updates transport_generic_cmd_sequencer() to check for this case
> with TCM/pSCSI plugin backstores and does the exntra task->task_sg_bidi[] allocation
> in transport_calc_sg_num(). It also updates transport_generic_get_cdb_count()
> and transport_do_se_mem_map() to handle the extra task->task_sg_bidi mapping
> with a second call to transport_map_mem_to_sg().
>
> This nice thing about this patch is that it allows TCM Core to still handle the
> recieved XDWRITEREAD_* transfer length > backstore max_sectors case transparently.
>
> Signed-off-by: Nicholas A. Bellinger <nab@xxxxxxxxxxxxxxx>
> ---
> drivers/target/target_core_transport.c | 105 +++++++++++++++++++++++++++----
> include/target/target_core_base.h | 1 +
> 2 files changed, 92 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index a20a4a9..e514ffc 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -5399,7 +5399,7 @@ static int transport_generic_cmd_sequencer(
> {
> struct se_device *dev = SE_DEV(cmd);
> struct se_subsystem_dev *su_dev = dev->se_sub_dev;
> - int ret, sector_ret = 0;
> + int ret, sector_ret = 0, passthrough;
> u32 sectors = 0, size = 0, pr_reg_type = 0;
> u16 service_action;
> u8 alua_ascq = 0;
> @@ -5587,8 +5587,15 @@ static int transport_generic_cmd_sequencer(
> TCM_MAX_COMMAND_SIZE, cdb[7], service_action);
> return TGCS_INVALID_CDB_FIELD;
> }
> + /*
> + * Determine if this is TCM/PSCSI device and we should disable
> + * internal emulation for this CDB.
> + */
> + passthrough = (TRANSPORT(dev)->transport_type ==
> + TRANSPORT_PLUGIN_PHBA_PDEV);
> +
> switch (service_action) {
> - case 0x0007: /* XDWRITE_READ_32 */
> + case XDWRITEREAD_32:
> sectors = transport_get_sectors_32(cdb, cmd, &sector_ret);
> if (sector_ret)
> return TGCS_UNSUPPORTED_CDB;
> @@ -5602,6 +5609,11 @@ static int transport_generic_cmd_sequencer(
> cmd->transport_split_cdb = &split_cdb_XX_32;
> cmd->transport_get_long_lba = &transport_lba_64_ext;
> /*
> + * Skip the remaining assignments for TCM/PSCSI passthrough
> + */
> + if (passthrough)
> + break;
> + /*
> * Setup BIDI XOR callback to be run during
> * transport_generic_complete_ok()
> */
> @@ -6475,6 +6487,7 @@ void transport_free_dev_tasks(struct se_cmd *cmd)
> if (!task->transport_req)
> continue;
>
> + kfree(task->task_sg_bidi);
> kfree(task->task_sg);
>
> spin_unlock_irqrestore(&T_TASK(cmd)->t_state_lock, flags);
> @@ -7056,6 +7069,7 @@ extern u32 transport_calc_sg_num(
> u32 task_offset)
> {
> struct se_cmd *se_cmd = task->task_se_cmd;
> + struct se_device *se_dev = SE_DEV(se_cmd);
> struct se_mem *se_mem = in_se_mem;
> struct target_core_fabric_ops *tfo = CMD_TFO(se_cmd);
> u32 sg_length, task_size = task->task_size, task_sg_num_padded;
> @@ -7127,15 +7141,31 @@ next:
> " task->task_sg\n");
> return 0;
> }
> -
> sg_init_table(&task->task_sg[0], task_sg_num_padded);
> /*
> + * Setup task->task_sg_bidi for SCSI READ payload for
> + * TCM/pSCSI passthrough if present for BIDI-COMMAND
> + */
> + if ((T_TASK(se_cmd)->t_mem_bidi_list != NULL) &&
> + (TRANSPORT(se_dev)->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV)) {
> + task->task_sg_bidi = kzalloc(task_sg_num_padded *
> + sizeof(struct scatterlist), GFP_KERNEL);

Is there a place where you make sure (task_sg_num_padded * sizeof(struct scatterlist))
will not be bigger then a PAGE_SIZE?

Is there any chance to convert this code to use sg_alloc_table() with
an embedded struct sg_table in task-> ?

>From my experience the use of struct sg_table cleans up the code
tremendously. (Perhaps put it on some todo list)

> + if (!(task->task_sg_bidi)) {
> + printk(KERN_ERR "Unable to allocate memory for"
> + " task->task_sg_bidi\n");
> + return 0;
> + }
> + sg_init_table(&task->task_sg_bidi[0], task_sg_num_padded);
> + }
> + /*
> * For the chaining case, setup the proper end of SGL for the
> * initial submission struct task into struct se_subsystem_api.
> * This will be cleared later by transport_do_task_sg_chain()
> */
> - if (task->task_padded_sg)
> + if (task->task_padded_sg) {
> sg_mark_end(&task->task_sg[task->task_sg_num - 1]);
> + sg_mark_end(&task->task_sg_bidi[task->task_sg_num - 1]);
> + }
>
> DEBUG_SC("Successfully allocated task->task_sg_num(%u),"
> " task_sg_num_padded(%u)\n", task->task_sg_num,
> @@ -7485,17 +7515,34 @@ static int transport_do_se_mem_map(
> return ret;
> }
> /*
> - * Assume default that transport plugin speaks preallocated
> - * scatterlists.
> + * This is the normal path for all normal non BIDI and BIDI-COMMAND
> + * WRITE payloads.. If we need to do BIDI READ passthrough for
> + * TCM/pSCSI the first call to transport_do_se_mem_map ->
> + * transport_calc_sg_num() -> transport_map_mem_to_sg() will do the
> + * allocation for task->task_sg_bidi, and the subsequent call to
> + * transport_do_se_mem_map() from transport_generic_get_cdb_count()
> */
> - if (!(transport_calc_sg_num(task, in_se_mem, task_offset)))
> - return -1;
> - /*
> - * struct se_task->task_sg now contains the struct scatterlist array.
> - */
> - return transport_map_mem_to_sg(task, se_mem_list, task->task_sg,
> + if (!(task->task_sg_bidi)) {
> + /*
> + * Assume default that transport plugin speaks preallocated
> + * scatterlists.
> + */
> + if (!(transport_calc_sg_num(task, in_se_mem, task_offset)))
> + return -1;
> + /*
> + * struct se_task->task_sg now contains the struct scatterlist array.
> + */
> + return transport_map_mem_to_sg(task, se_mem_list, task->task_sg,
> in_se_mem, out_se_mem, se_mem_cnt,
> task_offset_in);
> + }
> + /*
> + * Handle the se_mem_list -> struct task->task_sg_bidi
> + * memory map for the extra BIDI READ payload
> + */
> + return transport_map_mem_to_sg(task, se_mem_list, task->task_sg_bidi,
> + in_se_mem, out_se_mem, se_mem_cnt,
> + task_offset_in);
> }
>
> u32 transport_generic_get_cdb_count(
> @@ -7509,9 +7556,10 @@ u32 transport_generic_get_cdb_count(
> unsigned char *cdb = NULL;
> struct se_task *task;
> struct se_mem *se_mem = NULL, *se_mem_lout = NULL;
> + struct se_mem *se_mem_bidi = NULL, *se_mem_bidi_lout = NULL;
> struct se_device *dev = SE_DEV(cmd);
> int max_sectors_set = 0, ret;
> - u32 task_offset_in = 0, se_mem_cnt = 0, task_cdbs = 0;
> + u32 task_offset_in = 0, se_mem_cnt = 0, se_mem_bidi_cnt = 0, task_cdbs = 0;
> unsigned long long lba;
>
> if (!mem_list) {
> @@ -7526,6 +7574,15 @@ u32 transport_generic_get_cdb_count(
> if (!(list_empty(mem_list)))
> se_mem = list_entry(mem_list->next, struct se_mem, se_list);
> /*
> + * Check for extra se_mem_bidi mapping for BIDI-COMMANDs to
> + * struct se_task->task_sg_bidi for TCM/pSCSI passthrough operation
> + */
> + if ((T_TASK(cmd)->t_mem_bidi_list != NULL) &&
> + !(list_empty(T_TASK(cmd)->t_mem_bidi_list)) &&
> + (TRANSPORT(dev)->transport_type == TRANSPORT_PLUGIN_PHBA_PDEV))
> + se_mem_bidi = list_entry(T_TASK(cmd)->t_mem_bidi_list->next,
> + struct se_mem, se_list);
> + /*
> * Locate the start volume segment in which the received LBA will be
> * executed upon.
> */
> @@ -7573,7 +7630,8 @@ u32 transport_generic_get_cdb_count(
>
> /*
> * Perform the SE OBJ plugin and/or Transport plugin specific
> - * mapping for T_TASK(cmd)->t_mem_list.
> + * mapping for T_TASK(cmd)->t_mem_list. And setup the
> + * task->task_sg and if necessary task->task_sg_bidi
> */
> ret = transport_do_se_mem_map(dev, task, mem_list,
> NULL, se_mem, &se_mem_lout, &se_mem_cnt,
> @@ -7582,6 +7640,25 @@ u32 transport_generic_get_cdb_count(
> goto out;
>
> se_mem = se_mem_lout;
> + /*
> + * Setup the T_TASK(cmd)->t_mem_bidi_list -> task->task_sg_bidi
> + * mapping for SCSI READ for BIDI-COMMAND passthrough with TCM/pSCSI
> + *
> + * Note that the first call to transport_do_se_mem_map() above will
> + * allocate struct se_task->task_sg_bidi in transport_do_se_mem_map()
> + * -> transport_calc_sg_num(), and the second here will do the
> + * mapping for SCSI READ for BIDI-COMMAND passthrough with TCM/pSCSI.
> + */
> + if (task->task_sg_bidi != NULL) {
> + ret = transport_do_se_mem_map(dev, task,
> + T_TASK(cmd)->t_mem_bidi_list, NULL,
> + se_mem_bidi, &se_mem_bidi_lout, &se_mem_bidi_cnt,
> + &task_offset_in);
> + if (ret < 0)
> + goto out;
> +
> + se_mem_bidi = se_mem_bidi_lout;
> + }
> task_cdbs++;
>
> DEBUG_VOL("Incremented task_cdbs(%u) task->task_sg_num(%u)\n",
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index b68dea1..b6f3b75 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -471,6 +471,7 @@ struct se_transport_task {
> struct se_task {
> unsigned char task_sense;
> struct scatterlist *task_sg;
> + struct scatterlist *task_sg_bidi;
> struct scatterlist sgt;
struct sg_table sgt_bidi;

See the sg_table has the scatterlist * inside
plus a couple of element counters for allocation/deallocation.

> void *transport_req;
> u8 task_scsi_status;
> u8 task_flags;

Thanks
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/