Re: [RFC] support for bidirectional transfers and variable length CDBs

[Benny Halevy <bhalevy@panasas.com>]

Christoph Hellwig wrote:
> On Fri, Oct 27, 2006 at 02:14:26PM +0200, Benny Halevy wrote:
>   
>> Hi,
>>
>> Attached are patches for linux-2.6.18, open-iscsi (2.0-711), and
>> ibm-osd-initiator (v3.1.1), sketching a proposal for supporting 
>> bi-directional
>> scsi commands and variable length CDBs.  The motivation behind this is
>> having an OSD stack in the kernel for the Object-based pNFS layout driver
>> in the linux kernel that we're working on with CITI.
>> (part of NFSv4.1, see also 
>> http://www.ietf.org/internet-drafts/draft-ietf-nfsv4-pnfs-obj-02.txt)
>>     
>
> Offtopic note:  unless you opensource all your filesystem code and merge
> into the kernel there's not chance we'll put in nfs hooks for you.  This
> doesn't affect the scsi bits though as we plan to have bidi commands
> for other reasons though (which doesn't mean it's legal to use for
> binary modules either)
>   
That's what the pnfs-over-objects layout driver is all about...
>   
>> The core changes we propose in the scsi layer to support bidi are:
>>  - adding struct scsi_cmnd_sgl for holding a mapping of the caller 
>> buffers as
>>    <use_sg, sglist_len, request_buffer, request_bufflen, request>
>>     
>
> I would rather call this scsi_data_buffer, but that's just a smallish thing.
> Having this as a separate stuct is a lot nicer than my previous proposal,
> though.
>   
cool, works for me.
>   
>>   - using it in struct scsi_cmnd to map the read buffer of 
>> bidirectional commands.
>>     for backward compatibility and to keep this patch minimalistic we 
>> kept the mapping
>>     of the uni-directional buffers as well as the write buffers of 
>> bi-directional transfers intact;
>>     in the future I think it will be cleaner to hold these in a 
>> scsi_cmnd_sgl of their own.
>>     
>
> This on the other hand is not very nice.  We should put an in and and an
> out scsi_data_buffer in struct scsi_cmnd.  As we need to revist this part
> of the scsi midlayer <-> LLDD interface anyway we plan to handle this
> change as part of the bigger overall transition.  Here are the helpers
> I had envisioned at storage summit in the version where they still
> translate to the currently existing code, but can be nicely adopted
> to bidi-capable versions with two sg lists:
>
> size_t scsi_dma_map(struct device *dev, struct scsi_cmnd *cmd)
> {
> 	ssize_t nseg = 0;
>
> 	if (cmd->use_sg) {
> 		struct scatterlist *sg =
> 			(struct scatterlist *) cmd->request_buffer;
>
> 		nseg = dma_map_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
> 		if (unlikely(!nseg))
> 			return -ENOMEM;
> 	}
>
> 	return nseg;
> }
>
> void scsi_dma_unmap(struct device *dev, struct scsi_cmnd *cmd)
> {
> 	if (cmd->use_sg) {
> 		struct scatterlist *sg =
> 			(struct scatterlist *) cmd->request_buffer;
> 		dma_unmap_sg(dev, sg, cmd->use_sg, cmd->sc_data_direction);
> 	}
> }
>
>
> and something similar for pio / virtual drivers.  Note that the
> above doesn't handle non-sg commands - we still have one user of them
> in the error handler, but I'll finally submit the patch to remove it
> soon.  Also note that actually bidi-capable driver still will have
> to opencode the map and know about the two data buffers.  I expect
> driver actually handling bidi commands to be the absolute minority,
> though.
>   
I'm not sure I understand exactly when these helpers are to be used.
Can you point me at more info please?
Does this solve the dependency of scsi on struct request for buffer mapping?
(i.e. will this be used in the scsi_init_io() path instead of calling 
blk_rq_map_kern()
or the calls to the block layer in scsi_merge_bio()?)

>   
>>  - allocate another struct request when executing a bidirectional 
>> command and use it to map the bidi read
>>    buffer (or sglist).  keep a pointer to it in the scsi_io_context.
>>     
>
> That's also not the way to go ahead - struct request should support
> bidi commands the same way scsi should.  I'd rather finish the scsi
> bits first, and until that is done your approach might be fine for
> (out of tree) debugging.
>   
work for me
>   
>> Core changes proposed to support variable length CDBs:
>>     
>
> This should be an entirely separate patch.  (As should your updates to
> the list of scsi device types and the typo fix in a comment in your
> patch)  I haven't looked at this code in detail but it looks rather
> messy already ;-)  I'd prefer if we could postponed implementing this
> properly until we've landed bidi support.
>   
ok
>   
>>   - API crafted in the spirit of scsi_execute_async and is a super set of
>>     scsi_execute and scsi_execute_async (i.e. the varlen cdb and 
>> bidi_read_buff
>>     are optional)
>>     
>
> We might aswell change scsi_execute_async to just support bidi and varlen
> commands in the end.  There's very few user and having yet another API
> to submit scsi commands doesn't sound very helpfull.
>   
cool.  and what about scsi_execute?  are you ok with changing it too or 
would you
rather add a new synchronous API call for bidi. The reason I'm asking is 
that
it doesn't currently allocate a scsi_io_context and therefore it's more 
efficient
than the async api.
>   
>>  - use two scsi_cmnd_sgl's in struct scsi_cmnd to map uni/bidi_write 
>> buffers and bidi_read buffers
>>    respectively (or alternatively use one to map write buffers and the 
>> other to map read buffers, this
>>    doesn't matter much id everybody uses scsi_get_{in,out,uni}_buff() 
>> rather than dereferencing
>>    a scsi_cmnd pointer to get to the fields.
>>
>>  - support bidi in struct request.  This will save allocating yet 
>> another request for mapping the
>>    bidi_read buffers
>>     
>
> As per comments above I think we should do this from the beginning.
>
> As for the further todo I'd suggest the following:
>
>  - I submit the patch to get rid of the remaining non-sg I/O ASAP
>  - you submit your patch to add the osd type and the addition device
>    type descriptions
>  - you submit your patch that fixes the scsi_execute comment typo
>  - you submit your patch to add the VARLEN_CDB opcode
>  - you start converting as many as possible drivers to the
>    above wrapper API (I already had a big FC card vendor signed up
>    for this, but no updates so far..)
>   
fine with me, if ok with them too, please send them my way so they can send
us whatever they've done already.
>  
>  And then once we have almost all direct references to the request_buffer
>  & friends gone you submit a new bidi patch based on the comments in this
>  mail.
>   
sounds like a plan!
> Thanks for working on this.
>   
you're welcome, my pleasure...
(especially after the much needed cleanup in 2.6.18 :)