Re: [PATCH 06/20] target: Eliminate usage of struct se_mem

[Christoph Hellwig <hch@infradead.org>]

>  
>  	iov_count += TRANSPORT_IOV_DATA_BUFFER;
>  
> @@ -815,7 +815,7 @@ static int iscsit_alloc_buffs(struct iscsi_cmd *cmd)
>  
>  	/* BIDI ops not supported */
>  
> -	/* make se_mem list from the memory */
> +	/* Tell the core about our preallocated memory */
>  	transport_generic_map_mem_to_cmd(&cmd->se_cmd, sgl, nents, NULL, 0);

Probably not worth it for this series, but for a next one, but what
about killing transport_generic_map_mem_to_cmd?  The drivers can do the
assignment just as fine, and we could probably set
SCF_SCSI_CONTROL_NONSG_IO_CDB automatically when the fabric sets the S/G
list before calling in for processing.

Or maybe we should stop calling transport_generic_get_mem from the
core entirely and just export it as a helper to the fabrics as the
default implementation.  This actually sounds like the nicest option
to me.

>  	struct se_task *task;
>  	struct se_device *dev = cmd->se_dev;
> -	unsigned long flags;
>  
>  	task = dev->transport->alloc_task(cmd);
>  	if (!task) {
> @@ -1723,10 +1707,6 @@ transport_generic_get_task(struct se_cmd *cmd,
>  	task->se_dev = dev;
>  	task->task_data_direction = data_direction;
>  
> -	spin_lock_irqsave(&cmd->t_state_lock, flags);
> -	list_add_tail(&task->t_list, &cmd->t_task_list);
> -	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> -

How is moving the task <-> cmd linkage related to the rest of the patch?
It probably should be a separate preparatory patch, but at least needs
some good documentation.

>  static int transport_new_cmd_obj(struct se_cmd *cmd)
>  {
>  	struct se_device *dev = cmd->se_dev;
>  	u32 task_cdbs;
>  	u32 rc;
> +	int set_counts = 1;
>  
> +	/*
> +	 * Setup any BIDI READ tasks and memory from
> +	 * cmd->t_mem_bidi_list so the READ struct se_tasks
> +	 * are queued first for the non pSCSI passthrough case.
> +	 */
> +	if (cmd->t_bidi_data_sg &&
> +	    (dev->transport->transport_type != TRANSPORT_PLUGIN_PHBA_PDEV)) {

no need for braces around the comparism.

> +static u32 transport_allocate_tasks(
> +	struct se_cmd *cmd,
> +	unsigned long long lba,
> +	enum dma_data_direction data_direction,
> +	struct scatterlist *sgl,
> +	unsigned int sgl_nents)
> +{
> +	int ret;
> +
> +	if (cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
> +		return transport_allocate_data_tasks(cmd, lba, data_direction,
> +						     sgl, sgl_nents);
> +	} else {
> +		ret = transport_allocate_control_task(cmd);
> +		if (ret < 0)
> +			return ret;
> +		else
> +			return 1;

Any reason you don't directly return the expected value from
transport_allocate_control_task?

>  	 * Determine is the TCM fabric module has already allocated physical
>  	 * memory, and is directly calling transport_generic_map_mem_to_cmd()
> -	 * to setup beforehand the linked list of physical memory at
> -	 * cmd->t_mem_list of struct se_mem->se_page
> +	 * beforehand.
>  	 */
>  	if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
>  		ret = transport_generic_get_mem(cmd);
> @@ -4868,19 +4400,13 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
>  	if (ret < 0)
>  		return ret;
>  
> -	if (cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
> -		list_for_each_entry(task, &cmd->t_task_list, t_list) {
> -			if (atomic_read(&task->task_sent))
> -				continue;
> -			if (!dev->transport->map_task_SG)
> -				continue;
> -
> -			ret = dev->transport->map_task_SG(task);
> -			if (ret < 0)
> -				return ret;
> -		}
> -	} else {
> -		ret = transport_map_control_cmd_to_task(cmd);
> +	list_for_each_entry(task, &cmd->t_task_list, t_list) {
> +		if (atomic_read(&task->task_sent))
> +			continue;
> +		if (!dev->transport->map_task_SG)
> +			continue;
> + 
> +		ret = dev->transport->map_task_SG(task);
>  		if (ret < 0)
>  			return ret;
>  	}

This still looks incorrect to me as control CDBs might have to
end up in ->cdb_none.  Or did I miss how this gets handled now?