[Stgt-devel] [PATCH 03/20] iser transport buf

[FUJITA Tomonori]

On Wed, 14 Nov 2007 16:33:32 +0900
FUJITA Tomonori <tomof at acm.org> wrote:

> On Tue, 13 Nov 2007 19:36:05 -0500
> Pete Wyckoff <pw at osc.edu> wrote:
> 
> > tomof at acm.org wrote on Mon, 12 Nov 2007 23:14 +0900:
> > > On Tue, 16 Oct 2007 11:18:57 -0400
> > > Pete Wyckoff <pw at osc.edu> wrote:
> > > 
> > > > For RDMA, it is often nice to use data from a pool of pre-registered
> > > > buffers.  To do this, the transport allocates memory for a response and
> > > > passes it down to the devices to fill.  Some operations, though,
> > > > allocate their own buffers and return that new memory instead.  These
> > > > are usually small and the allocation is just done for convenience to
> > > > avoid length bounds checking.  Copy the data into the provided transport
> > > > buffer instead.
> > > 
> > > I killed memory allocation in scsi device code (spc, mmc, sbc, smc)
> > > and kill all the hacky memory management code. Now LLDs allocate and
> > > free buffer for data transfer.
> > 
> > But now you need two mallocs per command:  one for the task and one
> > for the (aligned) data.  Bidi needs three.  This will show up in
> > iser performance, but might be a worthwhile trade-off.  Maybe we
> > could add slabs for common sizes (ick).
> 
> I thought that iSER code already does something like that since you
> use pre-registered buffer again and again.
> 
> glib also does something like that. So I don't think that this change
> will effect performance much.
> 
> 
> > I have a lot of rebasing to do after all your bidi buffer work.  It
> > looks like good changes so far, but I have a couple of concerns from
> > a brief peruse of the changes in git.
> > 
> > Zeroing out the data chunk for non-READ/WRITE commands.  There are
> > lots of OSD commands hidden inside the 0x7f extended format, and
> > other non-OSD non-SBC commands too.  Can we just move the zeroing to
> > where it is needed?  Like in each of the inquiry, etc.  This zeroing
> > was a major performance problem for iser.
> 
> Yeah, I added that hack because device type code is broken (assumes
> that buffer is large enough and zero'ed out). We need to fix device
> type code.
> 
> 
> > Accessor and setter functions for scmd.  I don't see what value they
> > add.  Except maybe for the "(void*)(unsigned long)" ones.
> 
> I want to insulate LLDs from scsi_cmd internals like Linux scsi-ml
> does. It enables us to change the structure freely in the future
> without caring about device type code and llds.
> 
> 
> > Also, building functions using cpp to glue names together breaks
> > things like ctags, cscope, and diff -p.  Curious to hear why you
> > like them.
> 
> Yeah, there are the pros and cons of using cpp magic. I prefer using
> cpp magic than defining lots of similar functions. And I think that
> linux scsi developers are already familiar with that (see the
> transport classes).
> 
> 
> > Thanks for working on this.  Bidi handling in bs_* will be much
> > cleaner now.
> 
> No problem. I wanted this cleanups before the code becomes bigger by
> adding iSER, FCoE, etc.
> 
> 
> >  I'll be away from email until 25 nov, but will look at
> > your changes in detail and rebase iser after.
> 
> Thanks. Until 25, I try to merge some of your iSER patches that
> haven't been merged yet.

I think that I've merged all the patches that I can merge. I wait for
your third submission.

Thanks,