On Tue, 2010-11-09 at 11:16 +0200, Boaz Harrosh wrote:
> On 11/09/2010 07:13 AM, Nicholas A. Bellinger wrote:
> > <More follow up on Boaz comments from last week>
> >
>
> <snip>
>
> >>> +static inline void pscsi_blk_init_request(
> >>> + struct se_task *task,
> >>> + struct pscsi_plugin_task *pt,
> >>> + struct request *req,
> >>> + int bidi_read)
> >>> +{
> >>> + /*
> >>> + * Defined as "scsi command" in include/linux/blkdev.h.
> >>> + */
> >>> + req->cmd_type = REQ_TYPE_BLOCK_PC;
> >>> + /*
> >>> + * For the extra BIDI-COMMAND READ struct request we do not
> >>> + * need to setup the remaining structure members
> >>> + */
> >>> + if (bidi_read)
> >>> + return;
> >>> + /*
> >>> + * Setup the done function pointer for struct request,
> >>> + * also set the end_io_data pointer.to struct se_task.
> >>> + */
> >>> + req->end_io = pscsi_req_done;
> >>> + req->end_io_data = (void *)task;
> >>> + /*
> >>> + * Load the referenced struct se_task's SCSI CDB into
> >>> + * include/linux/blkdev.h:struct request->cmd
> >>> + */
> >>> + req->cmd_len = scsi_command_size(pt->pscsi_cdb);
> >>> + req->cmd = &pt->pscsi_cdb[0];
> >>
> >> Here! req->cmd = TASK_CMD(task)->t_task_cdb;
> >>
> >> I don't see in this patch the actual memcpy of TASK_CMD(task)->t_task_cdb into
> >> pt->pscsi_cdb, which I suspect is done in Generic code through the use of
> >> ->get_cdb(struct se_task *);?
> >>
> >> If so then it means all the plugins have their own copy of the CDB? Now I finally
> >> understand the get_max_cdb_len().
> >>
> >> All that could be totally clean. All plugins use the TASK_CMD(task)->t_task_cdb
> >> directly. Then get_max_cdb_len(), get_cdb(), the memcpy() and all these extra buffers
> >> just magically go away.
> >
> > As described in the last response, the T_TASK(cmd)->t_task_cdb is used
> > as a base for all CDBs, and is the primary storage for all *non*
> > SCF_SCSI_DATA_SG_IO_CDB type ops.
> >
> > For all bulk SCF_SCSI_DATA_SG_IO_CDB, we will memcpy
> > T_TASK(cmd)->t_task_cdb into the backend specific location (for
> > TCM/pSCSI this is pt->pscsi_cdb[]) and then update the LBA+length when
> > splitting the single received struct se_cmd across multiple struct
> > se_task w/ backend descriptors due to backend struct
> > se_dev_limits->limits.max_sectors limitations for underlying backend HW.
> >
>
> OK Thanks I was missing this part. Sorry I only looked at the patches and
> not the complete code.
>
> So for pSCSI could you use the cmnd buffer at struct request? Maybe
> postpone a little bit the patching of the cdb to after the request allocation.
<nod> Again, this would only be for the non SCF_SCSI_DATA_SG_IO_CDB
cases for TCM/pSCSI, so I am not sure if the complexity is worth the
benefit for the non SCF_SCSI_DATA_SG_IO_CDB descriptors..
> Than anything bigger then 16 bytes (max cmnd at struct request) you allocate
> and free on request completion.
Hmm yeah, that adds a bit of extra complexity for TCM/pSCSI considering
the default TCM_MAX_COMMAND_SIZE=32
> But all that could be cleaned up later. You are currently busy with bigger
> stuff, just as a future note.
>
<nod> point taken.
> The rest look good. I have some holes in my knowledge of how you did bidi.
> I promise to one of these days actually clone the tree and look at the real
> code.
> My goal is to have Lio-iscsi => lio-pSCSI => iscsi-initiator as a passthrough
> of OSD commands. That will prove things are done right.
>
Excellent, please let me know if you have any questions getting setup.
> Also a Lio-iscsi => lio-tgt_if => tgtd-user-mode should also be very useful
> and OSD-able
>
Indeed, I am still planning to spend a day or two before the end of the
year to get the basic pieces running for target_core_stgt.c and the main
I/O path for a userspace passthrough backend module function.
As usual, I am happy to test and accept patches before then.. ;)
> Thanks, Nic. Hope you feeling better now
> Boaz
Most certainly. Thanks again for your detailed review!
--nab
--
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
