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

[FUJITA Tomonori]

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.