On Wed, Jun 08, 2011 at 03:29:36PM -0700, Andy Grover wrote:
> This has the benefit of streamlining some paths that had to behave
> differently when we used two allocation methods. The downside is that
> all accesses to the data buffer need to kmap it before use. The performance
> impact of this is negligible -- on most archs kmap is a few instructions,
> but it does add a few lines to routines parsing data, most notably the
> PR, ALUA, and cdb code.
On most 64-bit architectures kmap in fact is a no-op, so I think we
should be more than fine with this, especially given that none of these
commands is a fast-path.
Note that the scsi layer for the initiator side also always uses S/G lists.
> +void *transport_kmap_first_data_page(struct se_cmd *cmd)
> +{
> + struct se_mem *se_mem;
> +
> + BUG_ON(list_empty(&cmd->t_mem_list));
> +
> + se_mem = list_first_entry(&cmd->t_mem_list, struct se_mem, se_list);
> +
> + /*
> + * 1st se_mem should point to a page, and we shouldn't need more than
> + * that for this cmd
> + */
> + BUG_ON(cmd->data_length > PAGE_SIZE);
Do we actually verify these conditions earlier on? Given that these
are user controlled we should reject commands that we can't accept early
on. We'll also need to make sure that data never straddles a page,
which is something initiators could do by careful placement. I don't
think we have to support that as no sane initiator would do it, but
we'll need to check for, and reject it.
Alternatively the command emulation could have local buffers and copy
them to the S/G list, which works fine especially for commands with
small output that can be on the stack.
> @@ -4758,7 +4720,7 @@ int transport_generic_new_cmd(struct se_cmd *cmd)
> * cmd->t_mem_list of struct se_mem->se_page
> */
> if (!(cmd->se_cmd_flags & SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC)) {
> - ret = transport_allocate_resources(cmd);
> + ret = transport_generic_get_mem(cmd);
This doesn't look correct. We only used to call
transport_generic_get_mem when SCF_SCSI_DATA_SG_IO_CDB or
SCF_SCSI_CONTROL_SG_IO_CDB where set here, and now we do it
undconditionally. Given that we always set
SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC for these the code does in fact seem
dead.
> @@ -108,7 +108,6 @@ enum se_cmd_flags_table {
> SCF_EMULATED_TASK_SENSE = 0x00000004,
> SCF_SCSI_DATA_SG_IO_CDB = 0x00000008,
> SCF_SCSI_CONTROL_SG_IO_CDB = 0x00000010,
> - SCF_SCSI_CONTROL_NONSG_IO_CDB = 0x00000020,
> SCF_SCSI_NON_DATA_CDB = 0x00000040,
> SCF_SCSI_CDB_EXCEPTION = 0x00000080,
> SCF_SCSI_RESERVATION_CONFLICT = 0x00000100,
SCF_PASSTHROUGH_CONTIG_TO_SG should also be dead after your patch.
> int (*cdb_none)(struct se_task *);
> /*
> - * For SCF_SCSI_CONTROL_NONSG_IO_CDB
> - */
> - int (*map_task_non_SG)(struct se_task *);
> - /*
> * For SCF_SCSI_DATA_SG_IO_CDB and SCF_SCSI_CONTROL_SG_IO_CDB
> */
> int (*map_task_SG)(struct se_task *);
As a follow-on cdb_none should probably also go away and just be treated
as a NULL-S/G list special case in map_task_SG.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
