lkml.org 
[lkml]   [2010]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC v2 16/21] tcm: Add PSCSI subsystem plugin
    From
    Date
    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




    \
     
     \ /
      Last update: 2010-11-09 11:03    [W:3.131 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site