[PATCH 2/3] target: simplify cmd to task mapping

[PATCH 2/3] target: simplify cmd to task mapping

[Christoph Hellwig]

The cmd to task mapping is almost the same for all control CDBs,
except for calling different backend methods to do the backed-specific
mapping.  Merge the code performing the mapping into a single
new transport_map_control_cmd_to_task helper, and kill the
transport_map_task callback by simplify calling the backend methods
directly for the single task for a control CDB, and hardcoding
->map_task_SG for data CDBs.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: lio-core/drivers/target/target_core_transport.c
===================================================================
--- lio-core.orig/drivers/target/target_core_transport.c	2010-11-16 18:55:26.414004751 +0100
+++ lio-core/drivers/target/target_core_transport.c	2010-11-16 18:55:30.075004681 +0100
@@ -2258,132 +2258,6 @@ static struct se_task *transport_generic
 	return task;
 }
 
-static int transport_do_se_mem_map(struct se_device *, struct se_task *,
-	struct list_head *, void *, struct se_mem *, struct se_mem **,
-	u32 *, u32 *);
-
-/*	transport_process_control_sg_transform():
- *
- *
- */
-static int transport_process_control_sg_transform(
-	struct se_cmd *cmd,
-	struct se_transform_info *ti)
-{
-	unsigned char *cdb;
-	struct se_task *task;
-	struct se_mem *se_mem, *se_mem_lout = NULL;
-	struct se_device *dev = SE_DEV(cmd);
-	int ret;
-	u32 se_mem_cnt = 0, task_offset = 0;
-
-	list_for_each_entry(se_mem, T_TASK(cmd)->t_mem_list, se_list)
-		break;
-
-	if (!se_mem) {
-		printk(KERN_ERR "se_mem is NULL!\n");
-		return -1;
-	}
-
-	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
-					  cmd->data_direction);
-	if (!(task))
-		return -1;
-
-	task->transport_map_task = ti->se_obj_ptr->transport->map_task_SG;
-
-	cdb = TRANSPORT(dev)->get_cdb(task);
-	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
-			scsi_command_size(T_TASK(cmd)->t_task_cdb));
-
-	task->task_size = cmd->data_length;
-	task->task_sg_num = 1;
-
-	atomic_inc(&T_TASK(cmd)->t_fe_count);
-	atomic_inc(&T_TASK(cmd)->t_se_count);
-
-	ret = transport_do_se_mem_map(ti->se_obj_ptr, task,
-			T_TASK(cmd)->t_mem_list, NULL, se_mem, &se_mem_lout,
-			&se_mem_cnt, &task_offset);
-	if (ret < 0)
-		return ret;
-
-	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_SG_IO_CDB task_size: %d\n",
-			task->task_no, task->task_size);
-	return 0;
-}
-
-/*	transport_process_control_nonsg_transform():
- *
- *
- */
-static int transport_process_control_nonsg_transform(
-	struct se_cmd *cmd,
-	struct se_transform_info *ti)
-{
-	struct se_device *dev = SE_DEV(cmd);
-	unsigned char *cdb;
-	struct se_task *task;
-
-	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
-					  cmd->data_direction);
-	if (!(task))
-		return -1;
-
-	task->transport_map_task = ti->se_obj_ptr->transport->map_task_non_SG;
-
-	cdb = TRANSPORT(dev)->get_cdb(task);
-	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
-			scsi_command_size(T_TASK(cmd)->t_task_cdb));
-
-	task->task_size = cmd->data_length;
-	task->task_sg_num = 0;
-
-	atomic_inc(&T_TASK(cmd)->t_fe_count);
-	atomic_inc(&T_TASK(cmd)->t_se_count);
-
-	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_NONSG_IO_CDB task_size:"
-			" %d\n", task->task_no, task->task_size);
-	return 0;
-}
-
-/*	transport_process_non_data_transform():
- *
- *
- */
-static int transport_process_non_data_transform(
-	struct se_cmd *cmd,
-	struct se_transform_info *ti)
-{
-	struct se_device *dev = SE_DEV(cmd);
-	unsigned char *cdb;
-	struct se_task *task;
-
-	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
-					  cmd->data_direction);
-	if (!(task))
-		return -1;
-
-	task->transport_map_task = ti->se_obj_ptr->transport->cdb_none;
-
-	cdb = TRANSPORT(dev)->get_cdb(task);
-	if (cdb)
-		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
-			scsi_command_size(T_TASK(cmd)->t_task_cdb));
-
-	task->task_size = cmd->data_length;
-	task->task_sg_num = 0;
-
-	atomic_inc(&T_TASK(cmd)->t_fe_count);
-	atomic_inc(&T_TASK(cmd)->t_se_count);
-
-	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_NON_DATA_CDB task_size: %d\n",
-			task->task_no, task->task_size);
-	return 0;
-}
-
 static int transport_generic_cmd_sequencer(struct se_cmd *, unsigned char *);
 
 void transport_device_setup_cmd(struct se_cmd *cmd)
@@ -5942,8 +5816,6 @@ u32 transport_generic_get_cdb_count(
 		task->task_size = (task->task_sectors *
 				   DEV_ATTRIB(dev)->block_size);
 
-		task->transport_map_task = dev->transport->map_task_SG;
-
 		cdb = TRANSPORT(dev)->get_cdb(task);
 		if ((cdb)) {
 			memcpy(cdb, T_TASK(cmd)->t_task_cdb,
@@ -6011,6 +5883,61 @@ out:
 	return 0;
 }
 
+static int
+transport_map_control_cmd_to_task(struct se_cmd *cmd,
+		struct se_transform_info *ti)
+{
+	struct se_device *dev = SE_DEV(cmd);
+	unsigned char *cdb;
+	struct se_task *task;
+	int ret;
+
+	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
+				  cmd->data_direction);
+	if (!task)
+		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
+
+	cdb = TRANSPORT(dev)->get_cdb(task);
+	if (cdb)
+		memcpy(cdb, cmd->t_task->t_task_cdb,
+			scsi_command_size(cmd->t_task->t_task_cdb));
+
+	task->task_size = cmd->data_length;
+	task->task_sg_num =
+		(cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) ? 1 : 0;
+
+	atomic_inc(&cmd->t_task->t_fe_count);
+	atomic_inc(&cmd->t_task->t_se_count);
+
+	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
+		struct se_mem *se_mem, *se_mem_lout = NULL;
+		u32 se_mem_cnt = 0, task_offset = 0;
+
+		BUG_ON(list_empty(cmd->t_task->t_mem_list));
+
+		ret = transport_do_se_mem_map(dev, task,
+				cmd->t_task->t_mem_list, NULL, se_mem,
+				&se_mem_lout, &se_mem_cnt, &task_offset);
+		if (ret < 0)
+			return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
+
+		if (dev->transport->map_task_SG)
+			return dev->transport->map_task_SG(task);
+		return 0;
+	} else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
+		if (dev->transport->map_task_non_SG(task))
+			return dev->transport->map_task_non_SG(task);
+		return 0;
+	} else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
+		if (dev->transport->cdb_none(task))
+			return dev->transport->cdb_none(task);
+		return 0;
+	} else {
+		BUG();
+		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
+	}
+}
+
 /*	 transport_generic_new_cmd(): Called from transport_processing_thread()
  *
  *	 Allocate storage transport resources from a set of values predefined
@@ -6041,16 +5968,17 @@ int transport_generic_new_cmd(struct se_
 		if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
 			ret = transport_allocate_resources(cmd);
 			if (ret < 0)
-				goto failure;
+				return ret;
 		}
 
 		ret = transport_get_sectors(cmd);
 		if (ret < 0)
-			goto failure;
+			return ret;
 
 		ret = transport_new_cmd_obj(cmd, &ti, 0);
 		if (ret < 0)
-			goto failure;
+			return ret;
+
 		/*
 		 * Determine if the calling TCM fabric module is talking to
 		 * Linux/NET via kernel sockets and needs to allocate a
@@ -6059,41 +5987,26 @@ int transport_generic_new_cmd(struct se_
 		se_tpg = SE_LUN(cmd)->lun_sep->sep_tpg;
 		if (TPG_TFO(se_tpg)->alloc_cmd_iovecs != NULL) {
 			ret = TPG_TFO(se_tpg)->alloc_cmd_iovecs(cmd);
-			if (ret < 0) {
-				ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
-				goto failure;
-			}
+			if (ret < 0)
+				return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
 		}
 	}
 
-	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB)
-		ret = transport_process_control_sg_transform(cmd, &ti);
-	else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB)
-		ret = transport_process_control_nonsg_transform(cmd, &ti);
-	else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB)
-		ret = transport_process_non_data_transform(cmd, &ti);
-	else
-		BUG_ON(!(cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB));
-
-	if (ret < 0) {
-		ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
-		goto failure;
-	}
+	if (cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
+		list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
+			if (atomic_read(&task->task_sent))
+				continue;
+			if (!ti.se_obj_ptr->transport->map_task_SG)
+				continue;
 
-	/*
-	 * Set the correct (usually DMAable) buffer pointers from the master
-	 * buffer list in struct se_cmd to the transport task's native
-	 * buffers format.
-	 */
-	list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
-		if (atomic_read(&task->task_sent))
-			continue;
-
-		if (task->transport_map_task) {
-			ret = task->transport_map_task(task);
+			ret = ti.se_obj_ptr->transport->map_task_SG(task);
 			if (ret < 0)
-				goto failure;
+				return ret;
 		}
+	} else {
+		ret = transport_map_control_cmd_to_task(cmd, &ti);
+		if (ret < 0)
+			return ret;
 	}
 
 	/*
@@ -6113,9 +6026,6 @@ int transport_generic_new_cmd(struct se_
 	 */
 	transport_execute_tasks(cmd);
 	return 0;
-
-failure:
-	return ret;
 }
 
 /*	transport_generic_process_write():
Index: lio-core/include/target/target_core_base.h
===================================================================
--- lio-core.orig/include/target/target_core_base.h	2010-11-16 18:55:26.415004611 +0100
+++ lio-core/include/target/target_core_base.h	2010-11-16 18:55:30.076003773 +0100
@@ -499,7 +499,6 @@ struct se_task {
 	atomic_t	task_stop;
 	atomic_t	task_state_active;
 	struct timer_list	task_timer;
-	int (*transport_map_task)(struct se_task *);
 	struct se_device *se_obj_ptr;
 	struct list_head t_list;
 	struct list_head t_execute_list;

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Nicholas A. Bellinger]

On Wed, 2010-11-17 at 16:38 -0500, Christoph Hellwig wrote:
> The cmd to task mapping is almost the same for all control CDBs,
> except for calling different backend methods to do the backed-specific
> mapping.  Merge the code performing the mapping into a single
> new transport_map_control_cmd_to_task helper, and kill the
> transport_map_task callback by simplify calling the backend methods
> directly for the single task for a control CDB, and hardcoding
> ->map_task_SG for data CDBs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This one looks very reasonable as the extra set of bitwise AND
instructions is limited to control path code.

As for the first one, I don't really have a strong preference either
way, as long as the difference for extra 4 AND instructions for the bulk
'fast past' case is really a wash on modern x86_64 silicon..

--nab

> 
> Index: lio-core/drivers/target/target_core_transport.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_transport.c	2010-11-16 18:55:26.414004751 +0100
> +++ lio-core/drivers/target/target_core_transport.c	2010-11-16 18:55:30.075004681 +0100
> @@ -2258,132 +2258,6 @@ static struct se_task *transport_generic
>  	return task;
>  }
>  
> -static int transport_do_se_mem_map(struct se_device *, struct se_task *,
> -	struct list_head *, void *, struct se_mem *, struct se_mem **,
> -	u32 *, u32 *);
> -
> -/*	transport_process_control_sg_transform():
> - *
> - *
> - */
> -static int transport_process_control_sg_transform(
> -	struct se_cmd *cmd,
> -	struct se_transform_info *ti)
> -{
> -	unsigned char *cdb;
> -	struct se_task *task;
> -	struct se_mem *se_mem, *se_mem_lout = NULL;
> -	struct se_device *dev = SE_DEV(cmd);
> -	int ret;
> -	u32 se_mem_cnt = 0, task_offset = 0;
> -
> -	list_for_each_entry(se_mem, T_TASK(cmd)->t_mem_list, se_list)
> -		break;
> -
> -	if (!se_mem) {
> -		printk(KERN_ERR "se_mem is NULL!\n");
> -		return -1;
> -	}
> -
> -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> -					  cmd->data_direction);
> -	if (!(task))
> -		return -1;
> -
> -	task->transport_map_task = ti->se_obj_ptr->transport->map_task_SG;
> -
> -	cdb = TRANSPORT(dev)->get_cdb(task);
> -	if (cdb)
> -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> -
> -	task->task_size = cmd->data_length;
> -	task->task_sg_num = 1;
> -
> -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> -	atomic_inc(&T_TASK(cmd)->t_se_count);
> -
> -	ret = transport_do_se_mem_map(ti->se_obj_ptr, task,
> -			T_TASK(cmd)->t_mem_list, NULL, se_mem, &se_mem_lout,
> -			&se_mem_cnt, &task_offset);
> -	if (ret < 0)
> -		return ret;
> -
> -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_SG_IO_CDB task_size: %d\n",
> -			task->task_no, task->task_size);
> -	return 0;
> -}
> -
> -/*	transport_process_control_nonsg_transform():
> - *
> - *
> - */
> -static int transport_process_control_nonsg_transform(
> -	struct se_cmd *cmd,
> -	struct se_transform_info *ti)
> -{
> -	struct se_device *dev = SE_DEV(cmd);
> -	unsigned char *cdb;
> -	struct se_task *task;
> -
> -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> -					  cmd->data_direction);
> -	if (!(task))
> -		return -1;
> -
> -	task->transport_map_task = ti->se_obj_ptr->transport->map_task_non_SG;
> -
> -	cdb = TRANSPORT(dev)->get_cdb(task);
> -	if (cdb)
> -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> -
> -	task->task_size = cmd->data_length;
> -	task->task_sg_num = 0;
> -
> -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> -	atomic_inc(&T_TASK(cmd)->t_se_count);
> -
> -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_NONSG_IO_CDB task_size:"
> -			" %d\n", task->task_no, task->task_size);
> -	return 0;
> -}
> -
> -/*	transport_process_non_data_transform():
> - *
> - *
> - */
> -static int transport_process_non_data_transform(
> -	struct se_cmd *cmd,
> -	struct se_transform_info *ti)
> -{
> -	struct se_device *dev = SE_DEV(cmd);
> -	unsigned char *cdb;
> -	struct se_task *task;
> -
> -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> -					  cmd->data_direction);
> -	if (!(task))
> -		return -1;
> -
> -	task->transport_map_task = ti->se_obj_ptr->transport->cdb_none;
> -
> -	cdb = TRANSPORT(dev)->get_cdb(task);
> -	if (cdb)
> -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> -
> -	task->task_size = cmd->data_length;
> -	task->task_sg_num = 0;
> -
> -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> -	atomic_inc(&T_TASK(cmd)->t_se_count);
> -
> -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_NON_DATA_CDB task_size: %d\n",
> -			task->task_no, task->task_size);
> -	return 0;
> -}
> -
>  static int transport_generic_cmd_sequencer(struct se_cmd *, unsigned char *);
>  
>  void transport_device_setup_cmd(struct se_cmd *cmd)
> @@ -5942,8 +5816,6 @@ u32 transport_generic_get_cdb_count(
>  		task->task_size = (task->task_sectors *
>  				   DEV_ATTRIB(dev)->block_size);
>  
> -		task->transport_map_task = dev->transport->map_task_SG;
> -
>  		cdb = TRANSPORT(dev)->get_cdb(task);
>  		if ((cdb)) {
>  			memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> @@ -6011,6 +5883,61 @@ out:
>  	return 0;
>  }
>  
> +static int
> +transport_map_control_cmd_to_task(struct se_cmd *cmd,
> +		struct se_transform_info *ti)
> +{
> +	struct se_device *dev = SE_DEV(cmd);
> +	unsigned char *cdb;
> +	struct se_task *task;
> +	int ret;
> +
> +	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> +				  cmd->data_direction);
> +	if (!task)
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +	cdb = TRANSPORT(dev)->get_cdb(task);
> +	if (cdb)
> +		memcpy(cdb, cmd->t_task->t_task_cdb,
> +			scsi_command_size(cmd->t_task->t_task_cdb));
> +
> +	task->task_size = cmd->data_length;
> +	task->task_sg_num =
> +		(cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) ? 1 : 0;
> +
> +	atomic_inc(&cmd->t_task->t_fe_count);
> +	atomic_inc(&cmd->t_task->t_se_count);
> +
> +	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
> +		struct se_mem *se_mem, *se_mem_lout = NULL;
> +		u32 se_mem_cnt = 0, task_offset = 0;
> +
> +		BUG_ON(list_empty(cmd->t_task->t_mem_list));
> +
> +		ret = transport_do_se_mem_map(dev, task,
> +				cmd->t_task->t_mem_list, NULL, se_mem,
> +				&se_mem_lout, &se_mem_cnt, &task_offset);
> +		if (ret < 0)
> +			return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +		if (dev->transport->map_task_SG)
> +			return dev->transport->map_task_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
> +		if (dev->transport->map_task_non_SG(task))
> +			return dev->transport->map_task_non_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
> +		if (dev->transport->cdb_none(task))
> +			return dev->transport->cdb_none(task);
> +		return 0;
> +	} else {
> +		BUG();
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +	}
> +}
> +
>  /*	 transport_generic_new_cmd(): Called from transport_processing_thread()
>   *
>   *	 Allocate storage transport resources from a set of values predefined
> @@ -6041,16 +5968,17 @@ int transport_generic_new_cmd(struct se_
>  		if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
>  			ret = transport_allocate_resources(cmd);
>  			if (ret < 0)
> -				goto failure;
> +				return ret;
>  		}
>  
>  		ret = transport_get_sectors(cmd);
>  		if (ret < 0)
> -			goto failure;
> +			return ret;
>  
>  		ret = transport_new_cmd_obj(cmd, &ti, 0);
>  		if (ret < 0)
> -			goto failure;
> +			return ret;
> +
>  		/*
>  		 * Determine if the calling TCM fabric module is talking to
>  		 * Linux/NET via kernel sockets and needs to allocate a
> @@ -6059,41 +5987,26 @@ int transport_generic_new_cmd(struct se_
>  		se_tpg = SE_LUN(cmd)->lun_sep->sep_tpg;
>  		if (TPG_TFO(se_tpg)->alloc_cmd_iovecs != NULL) {
>  			ret = TPG_TFO(se_tpg)->alloc_cmd_iovecs(cmd);
> -			if (ret < 0) {
> -				ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> -				goto failure;
> -			}
> +			if (ret < 0)
> +				return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
>  		}
>  	}
>  
> -	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB)
> -		ret = transport_process_control_sg_transform(cmd, &ti);
> -	else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB)
> -		ret = transport_process_control_nonsg_transform(cmd, &ti);
> -	else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB)
> -		ret = transport_process_non_data_transform(cmd, &ti);
> -	else
> -		BUG_ON(!(cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB));
> -
> -	if (ret < 0) {
> -		ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> -		goto failure;
> -	}
> +	if (cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
> +		list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
> +			if (atomic_read(&task->task_sent))
> +				continue;
> +			if (!ti.se_obj_ptr->transport->map_task_SG)
> +				continue;
>  
> -	/*
> -	 * Set the correct (usually DMAable) buffer pointers from the master
> -	 * buffer list in struct se_cmd to the transport task's native
> -	 * buffers format.
> -	 */
> -	list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
> -		if (atomic_read(&task->task_sent))
> -			continue;
> -
> -		if (task->transport_map_task) {
> -			ret = task->transport_map_task(task);
> +			ret = ti.se_obj_ptr->transport->map_task_SG(task);
>  			if (ret < 0)
> -				goto failure;
> +				return ret;
>  		}
> +	} else {
> +		ret = transport_map_control_cmd_to_task(cmd, &ti);
> +		if (ret < 0)
> +			return ret;
>  	}
>  
>  	/*
> @@ -6113,9 +6026,6 @@ int transport_generic_new_cmd(struct se_
>  	 */
>  	transport_execute_tasks(cmd);
>  	return 0;
> -
> -failure:
> -	return ret;
>  }
>  
>  /*	transport_generic_process_write():
> Index: lio-core/include/target/target_core_base.h
> ===================================================================
> --- lio-core.orig/include/target/target_core_base.h	2010-11-16 18:55:26.415004611 +0100
> +++ lio-core/include/target/target_core_base.h	2010-11-16 18:55:30.076003773 +0100
> @@ -499,7 +499,6 @@ struct se_task {
>  	atomic_t	task_stop;
>  	atomic_t	task_state_active;
>  	struct timer_list	task_timer;
> -	int (*transport_map_task)(struct se_task *);
>  	struct se_device *se_obj_ptr;
>  	struct list_head t_list;
>  	struct list_head t_execute_list;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Nicholas A. Bellinger]

On Wed, 2010-11-17 at 14:40 -0800, Nicholas A. Bellinger wrote:
> On Wed, 2010-11-17 at 16:38 -0500, Christoph Hellwig wrote:
> > The cmd to task mapping is almost the same for all control CDBs,
> > except for calling different backend methods to do the backed-specific
> > mapping.  Merge the code performing the mapping into a single
> > new transport_map_control_cmd_to_task helper, and kill the
> > transport_map_task callback by simplify calling the backend methods
> > directly for the single task for a control CDB, and hardcoding
> > ->map_task_SG for data CDBs.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> This one looks very reasonable as the extra set of bitwise AND
> instructions is limited to control path code.
> 

Committed as 630eeda6.  Thanks hch!

> --nab
> 
> > 
> > Index: lio-core/drivers/target/target_core_transport.c
> > ===================================================================
> > --- lio-core.orig/drivers/target/target_core_transport.c	2010-11-16 18:55:26.414004751 +0100
> > +++ lio-core/drivers/target/target_core_transport.c	2010-11-16 18:55:30.075004681 +0100
> > @@ -2258,132 +2258,6 @@ static struct se_task *transport_generic
> >  	return task;
> >  }
> >  
> > -static int transport_do_se_mem_map(struct se_device *, struct se_task *,
> > -	struct list_head *, void *, struct se_mem *, struct se_mem **,
> > -	u32 *, u32 *);
> > -
> > -/*	transport_process_control_sg_transform():
> > - *
> > - *
> > - */
> > -static int transport_process_control_sg_transform(
> > -	struct se_cmd *cmd,
> > -	struct se_transform_info *ti)
> > -{
> > -	unsigned char *cdb;
> > -	struct se_task *task;
> > -	struct se_mem *se_mem, *se_mem_lout = NULL;
> > -	struct se_device *dev = SE_DEV(cmd);
> > -	int ret;
> > -	u32 se_mem_cnt = 0, task_offset = 0;
> > -
> > -	list_for_each_entry(se_mem, T_TASK(cmd)->t_mem_list, se_list)
> > -		break;
> > -
> > -	if (!se_mem) {
> > -		printk(KERN_ERR "se_mem is NULL!\n");
> > -		return -1;
> > -	}
> > -
> > -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> > -					  cmd->data_direction);
> > -	if (!(task))
> > -		return -1;
> > -
> > -	task->transport_map_task = ti->se_obj_ptr->transport->map_task_SG;
> > -
> > -	cdb = TRANSPORT(dev)->get_cdb(task);
> > -	if (cdb)
> > -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> > -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> > -
> > -	task->task_size = cmd->data_length;
> > -	task->task_sg_num = 1;
> > -
> > -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> > -	atomic_inc(&T_TASK(cmd)->t_se_count);
> > -
> > -	ret = transport_do_se_mem_map(ti->se_obj_ptr, task,
> > -			T_TASK(cmd)->t_mem_list, NULL, se_mem, &se_mem_lout,
> > -			&se_mem_cnt, &task_offset);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_SG_IO_CDB task_size: %d\n",
> > -			task->task_no, task->task_size);
> > -	return 0;
> > -}
> > -
> > -/*	transport_process_control_nonsg_transform():
> > - *
> > - *
> > - */
> > -static int transport_process_control_nonsg_transform(
> > -	struct se_cmd *cmd,
> > -	struct se_transform_info *ti)
> > -{
> > -	struct se_device *dev = SE_DEV(cmd);
> > -	unsigned char *cdb;
> > -	struct se_task *task;
> > -
> > -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> > -					  cmd->data_direction);
> > -	if (!(task))
> > -		return -1;
> > -
> > -	task->transport_map_task = ti->se_obj_ptr->transport->map_task_non_SG;
> > -
> > -	cdb = TRANSPORT(dev)->get_cdb(task);
> > -	if (cdb)
> > -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> > -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> > -
> > -	task->task_size = cmd->data_length;
> > -	task->task_sg_num = 0;
> > -
> > -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> > -	atomic_inc(&T_TASK(cmd)->t_se_count);
> > -
> > -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_CONTROL_NONSG_IO_CDB task_size:"
> > -			" %d\n", task->task_no, task->task_size);
> > -	return 0;
> > -}
> > -
> > -/*	transport_process_non_data_transform():
> > - *
> > - *
> > - */
> > -static int transport_process_non_data_transform(
> > -	struct se_cmd *cmd,
> > -	struct se_transform_info *ti)
> > -{
> > -	struct se_device *dev = SE_DEV(cmd);
> > -	unsigned char *cdb;
> > -	struct se_task *task;
> > -
> > -	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> > -					  cmd->data_direction);
> > -	if (!(task))
> > -		return -1;
> > -
> > -	task->transport_map_task = ti->se_obj_ptr->transport->cdb_none;
> > -
> > -	cdb = TRANSPORT(dev)->get_cdb(task);
> > -	if (cdb)
> > -		memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> > -			scsi_command_size(T_TASK(cmd)->t_task_cdb));
> > -
> > -	task->task_size = cmd->data_length;
> > -	task->task_sg_num = 0;
> > -
> > -	atomic_inc(&T_TASK(cmd)->t_fe_count);
> > -	atomic_inc(&T_TASK(cmd)->t_se_count);
> > -
> > -	DEBUG_CDB_H("task_no[%u]: SCF_SCSI_NON_DATA_CDB task_size: %d\n",
> > -			task->task_no, task->task_size);
> > -	return 0;
> > -}
> > -
> >  static int transport_generic_cmd_sequencer(struct se_cmd *, unsigned char *);
> >  
> >  void transport_device_setup_cmd(struct se_cmd *cmd)
> > @@ -5942,8 +5816,6 @@ u32 transport_generic_get_cdb_count(
> >  		task->task_size = (task->task_sectors *
> >  				   DEV_ATTRIB(dev)->block_size);
> >  
> > -		task->transport_map_task = dev->transport->map_task_SG;
> > -
> >  		cdb = TRANSPORT(dev)->get_cdb(task);
> >  		if ((cdb)) {
> >  			memcpy(cdb, T_TASK(cmd)->t_task_cdb,
> > @@ -6011,6 +5883,61 @@ out:
> >  	return 0;
> >  }
> >  
> > +static int
> > +transport_map_control_cmd_to_task(struct se_cmd *cmd,
> > +		struct se_transform_info *ti)
> > +{
> > +	struct se_device *dev = SE_DEV(cmd);
> > +	unsigned char *cdb;
> > +	struct se_task *task;
> > +	int ret;
> > +
> > +	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> > +				  cmd->data_direction);
> > +	if (!task)
> > +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> > +
> > +	cdb = TRANSPORT(dev)->get_cdb(task);
> > +	if (cdb)
> > +		memcpy(cdb, cmd->t_task->t_task_cdb,
> > +			scsi_command_size(cmd->t_task->t_task_cdb));
> > +
> > +	task->task_size = cmd->data_length;
> > +	task->task_sg_num =
> > +		(cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) ? 1 : 0;
> > +
> > +	atomic_inc(&cmd->t_task->t_fe_count);
> > +	atomic_inc(&cmd->t_task->t_se_count);
> > +
> > +	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
> > +		struct se_mem *se_mem, *se_mem_lout = NULL;
> > +		u32 se_mem_cnt = 0, task_offset = 0;
> > +
> > +		BUG_ON(list_empty(cmd->t_task->t_mem_list));
> > +
> > +		ret = transport_do_se_mem_map(dev, task,
> > +				cmd->t_task->t_mem_list, NULL, se_mem,
> > +				&se_mem_lout, &se_mem_cnt, &task_offset);
> > +		if (ret < 0)
> > +			return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> > +
> > +		if (dev->transport->map_task_SG)
> > +			return dev->transport->map_task_SG(task);
> > +		return 0;
> > +	} else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
> > +		if (dev->transport->map_task_non_SG(task))
> > +			return dev->transport->map_task_non_SG(task);
> > +		return 0;
> > +	} else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
> > +		if (dev->transport->cdb_none(task))
> > +			return dev->transport->cdb_none(task);
> > +		return 0;
> > +	} else {
> > +		BUG();
> > +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> > +	}
> > +}
> > +
> >  /*	 transport_generic_new_cmd(): Called from transport_processing_thread()
> >   *
> >   *	 Allocate storage transport resources from a set of values predefined
> > @@ -6041,16 +5968,17 @@ int transport_generic_new_cmd(struct se_
> >  		if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
> >  			ret = transport_allocate_resources(cmd);
> >  			if (ret < 0)
> > -				goto failure;
> > +				return ret;
> >  		}
> >  
> >  		ret = transport_get_sectors(cmd);
> >  		if (ret < 0)
> > -			goto failure;
> > +			return ret;
> >  
> >  		ret = transport_new_cmd_obj(cmd, &ti, 0);
> >  		if (ret < 0)
> > -			goto failure;
> > +			return ret;
> > +
> >  		/*
> >  		 * Determine if the calling TCM fabric module is talking to
> >  		 * Linux/NET via kernel sockets and needs to allocate a
> > @@ -6059,41 +5987,26 @@ int transport_generic_new_cmd(struct se_
> >  		se_tpg = SE_LUN(cmd)->lun_sep->sep_tpg;
> >  		if (TPG_TFO(se_tpg)->alloc_cmd_iovecs != NULL) {
> >  			ret = TPG_TFO(se_tpg)->alloc_cmd_iovecs(cmd);
> > -			if (ret < 0) {
> > -				ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> > -				goto failure;
> > -			}
> > +			if (ret < 0)
> > +				return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> >  		}
> >  	}
> >  
> > -	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB)
> > -		ret = transport_process_control_sg_transform(cmd, &ti);
> > -	else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB)
> > -		ret = transport_process_control_nonsg_transform(cmd, &ti);
> > -	else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB)
> > -		ret = transport_process_non_data_transform(cmd, &ti);
> > -	else
> > -		BUG_ON(!(cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB));
> > -
> > -	if (ret < 0) {
> > -		ret = PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> > -		goto failure;
> > -	}
> > +	if (cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
> > +		list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
> > +			if (atomic_read(&task->task_sent))
> > +				continue;
> > +			if (!ti.se_obj_ptr->transport->map_task_SG)
> > +				continue;
> >  
> > -	/*
> > -	 * Set the correct (usually DMAable) buffer pointers from the master
> > -	 * buffer list in struct se_cmd to the transport task's native
> > -	 * buffers format.
> > -	 */
> > -	list_for_each_entry(task, &T_TASK(cmd)->t_task_list, t_list) {
> > -		if (atomic_read(&task->task_sent))
> > -			continue;
> > -
> > -		if (task->transport_map_task) {
> > -			ret = task->transport_map_task(task);
> > +			ret = ti.se_obj_ptr->transport->map_task_SG(task);
> >  			if (ret < 0)
> > -				goto failure;
> > +				return ret;
> >  		}
> > +	} else {
> > +		ret = transport_map_control_cmd_to_task(cmd, &ti);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> >  
> >  	/*
> > @@ -6113,9 +6026,6 @@ int transport_generic_new_cmd(struct se_
> >  	 */
> >  	transport_execute_tasks(cmd);
> >  	return 0;
> > -
> > -failure:
> > -	return ret;
> >  }
> >  
> >  /*	transport_generic_process_write():
> > Index: lio-core/include/target/target_core_base.h
> > ===================================================================
> > --- lio-core.orig/include/target/target_core_base.h	2010-11-16 18:55:26.415004611 +0100
> > +++ lio-core/include/target/target_core_base.h	2010-11-16 18:55:30.076003773 +0100
> > @@ -499,7 +499,6 @@ struct se_task {
> >  	atomic_t	task_stop;
> >  	atomic_t	task_state_active;
> >  	struct timer_list	task_timer;
> > -	int (*transport_map_task)(struct se_task *);
> >  	struct se_device *se_obj_ptr;
> >  	struct list_head t_list;
> >  	struct list_head t_execute_list;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Christoph Hellwig]

On Wed, Nov 17, 2010 at 02:40:07PM -0800, Nicholas A. Bellinger wrote:
> On Wed, 2010-11-17 at 16:38 -0500, Christoph Hellwig wrote:
> > The cmd to task mapping is almost the same for all control CDBs,
> > except for calling different backend methods to do the backed-specific
> > mapping.  Merge the code performing the mapping into a single
> > new transport_map_control_cmd_to_task helper, and kill the
> > transport_map_task callback by simplify calling the backend methods
> > directly for the single task for a control CDB, and hardcoding
> > ->map_task_SG for data CDBs.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> This one looks very reasonable as the extra set of bitwise AND
> instructions is limited to control path code.
> 
> As for the first one, I don't really have a strong preference either
> way, as long as the difference for extra 4 AND instructions for the bulk
> 'fast past' case is really a wash on modern x86_64 silicon..

This one modifies the code touched by the first.  There's one if else
for data vs control path CDBs and then we switch the different types
of control CDBs.

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Nicholas A. Bellinger]

On Wed, 2010-11-17 at 16:38 -0500, Christoph Hellwig wrote:
> The cmd to task mapping is almost the same for all control CDBs,
> except for calling different backend methods to do the backed-specific
> mapping.  Merge the code performing the mapping into a single
> new transport_map_control_cmd_to_task helper, and kill the
> transport_map_task callback by simplify calling the backend methods
> directly for the single task for a control CDB, and hardcoding
> ->map_task_SG for data CDBs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: lio-core/drivers/target/target_core_transport.c
> ===================================================================
> --- lio-core.orig/drivers/target/target_core_transport.c	2010-11-16 18:55:26.414004751 +0100
> +++ lio-core/drivers/target/target_core_transport.c	2010-11-16 18:55:30.075004681 +0100

>  
> +static int
> +transport_map_control_cmd_to_task(struct se_cmd *cmd,
> +		struct se_transform_info *ti)
> +{
> +	struct se_device *dev = SE_DEV(cmd);
> +	unsigned char *cdb;
> +	struct se_task *task;
> +	int ret;
> +
> +	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> +				  cmd->data_direction);
> +	if (!task)
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +	cdb = TRANSPORT(dev)->get_cdb(task);
> +	if (cdb)
> +		memcpy(cdb, cmd->t_task->t_task_cdb,
> +			scsi_command_size(cmd->t_task->t_task_cdb));
> +
> +	task->task_size = cmd->data_length;
> +	task->task_sg_num =
> +		(cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) ? 1 : 0;
> +
> +	atomic_inc(&cmd->t_task->t_fe_count);
> +	atomic_inc(&cmd->t_task->t_se_count);
> +
> +	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
> +		struct se_mem *se_mem, *se_mem_lout = NULL;
> +		u32 se_mem_cnt = 0, task_offset = 0;
> +
> +		BUG_ON(list_empty(cmd->t_task->t_mem_list));
> +
> +		ret = transport_do_se_mem_map(dev, task,
> +				cmd->t_task->t_mem_list, NULL, se_mem,
> +				&se_mem_lout, &se_mem_cnt, &task_offset);
> +		if (ret < 0)
> +			return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +		if (dev->transport->map_task_SG)
> +			return dev->transport->map_task_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
> +		if (dev->transport->map_task_non_SG(task))

While testing this code I ran into this bogus conditional check here..

> +			return dev->transport->map_task_non_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
> +		if (dev->transport->cdb_none(task))

and again here..

> +			return dev->transport->cdb_none(task);
> +		return 0;
> +	} else {
> +		BUG();
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +	}
> +}
> +

Committed as the following patch and pushed to lio-core-2.6.git.

>From a342e38f5a3fa8a4c796cd1d96d8ee5074cdf945 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed, 17 Nov 2010 16:28:21 -0800
Subject: [PATCH] target: Fix up function pointer conditionals in transport_map_control_cmd_to_task()

This patch fixes up a cut and past bug that was added with commit f71d81ec15 where
*task where being incorrectly passed into dev->transport->map_task_non_SG
and dev->transport->cdb_none during the preceeding function pointer conditional.

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1d9209a..b50b75e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -5762,7 +5762,7 @@ transport_map_control_cmd_to_task(struct se_cmd *cmd,
        atomic_inc(&cmd->t_task->t_se_count);

        if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
-               struct se_mem *se_mem, *se_mem_lout = NULL;
+               struct se_mem *se_mem = NULL, *se_mem_lout = NULL;
                u32 se_mem_cnt = 0, task_offset = 0;

                BUG_ON(list_empty(cmd->t_task->t_mem_list));
@@ -5777,11 +5777,11 @@ transport_map_control_cmd_to_task(struct se_cmd *cmd,
                        return dev->transport->map_task_SG(task);
                return 0;
        } else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
-               if (dev->transport->map_task_non_SG(task))
+               if (dev->transport->map_task_non_SG)
                        return dev->transport->map_task_non_SG(task);
                return 0;
        } else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
-               if (dev->transport->cdb_none(task))
+               if (dev->transport->cdb_none)
                        return dev->transport->cdb_none(task);
                return 0;
        } else {
-- 
1.5.6.5

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Boaz Harrosh]

On 11/17/2010 11:38 PM, Christoph Hellwig wrote:
> The cmd to task mapping is almost the same for all control CDBs,
> except for calling different backend methods to do the backed-specific
> mapping.  Merge the code performing the mapping into a single
> new transport_map_control_cmd_to_task helper, and kill the
> transport_map_task callback by simplify calling the backend methods
> directly for the single task for a control CDB, and hardcoding
> ->map_task_SG for data CDBs.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 

<>

> +static int
> +transport_map_control_cmd_to_task(struct se_cmd *cmd,
> +		struct se_transform_info *ti)
> +{
> +	struct se_device *dev = SE_DEV(cmd);
> +	unsigned char *cdb;
> +	struct se_task *task;
> +	int ret;
> +
> +	task = transport_generic_get_task(ti, cmd, ti->se_obj_ptr,
> +				  cmd->data_direction);
> +	if (!task)
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +	cdb = TRANSPORT(dev)->get_cdb(task);
> +	if (cdb)
> +		memcpy(cdb, cmd->t_task->t_task_cdb,
> +			scsi_command_size(cmd->t_task->t_task_cdb));
> +
> +	task->task_size = cmd->data_length;
> +	task->task_sg_num =
> +		(cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) ? 1 : 0;
> +
> +	atomic_inc(&cmd->t_task->t_fe_count);
> +	atomic_inc(&cmd->t_task->t_se_count);
> +
> +	if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_SG_IO_CDB) {
> +		struct se_mem *se_mem, *se_mem_lout = NULL;
> +		u32 se_mem_cnt = 0, task_offset = 0;
> +
> +		BUG_ON(list_empty(cmd->t_task->t_mem_list));
> +
> +		ret = transport_do_se_mem_map(dev, task,
> +				cmd->t_task->t_mem_list, NULL, se_mem,
> +				&se_mem_lout, &se_mem_cnt, &task_offset);
> +		if (ret < 0)
> +			return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +
> +		if (dev->transport->map_task_SG)
> +			return dev->transport->map_task_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_CONTROL_NONSG_IO_CDB) {
> +		if (dev->transport->map_task_non_SG(task))
> +			return dev->transport->map_task_non_SG(task);
> +		return 0;
> +	} else if (cmd->se_cmd_flags & SCF_SCSI_NON_DATA_CDB) {
> +		if (dev->transport->cdb_none(task))
> +			return dev->transport->cdb_none(task);
> +		return 0;

If all these are mutual exclusive (if..elseif..else) can we not make
them an enum and switch() for costless-ness and clarity?

Boaz
> +	} else {
> +		BUG();
> +		return PYX_TRANSPORT_OUT_OF_MEMORY_RESOURCES;
> +	}
> +}
> +

<>

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Christoph Hellwig]

On Thu, Nov 18, 2010 at 02:07:17PM +0200, Boaz Harrosh wrote:
> If all these are mutual exclusive (if..elseif..else) can we not make
> them an enum and switch() for costless-ness and clarity?

We could.  But IMHO we should simply get rid of all the special cases
instead.  First remove the non-SG case, then fold the zero transfer
length one in as a special zero-length SG and then check how many
differences we have left between the data and control plane.

Re: [PATCH 2/3] target: simplify cmd to task mapping

[Boaz Harrosh]

On 11/22/2010 11:07 PM, Christoph Hellwig wrote:
> On Thu, Nov 18, 2010 at 02:07:17PM +0200, Boaz Harrosh wrote:
>> If all these are mutual exclusive (if..elseif..else) can we not make
>> them an enum and switch() for costless-ness and clarity?
> 
> We could.  But IMHO we should simply get rid of all the special cases
> instead.  First remove the non-SG case, then fold the zero transfer
> length one in as a special zero-length SG and then check how many
> differences we have left between the data and control plane.
> 

ACK ^3

Boaz