[Stgt-devel] iSER patches, second release

[Pete Wyckoff]

nezhinsky at gmail.com wrote on Sat, 08 Sep 2007 13:49 +0200:
> Thus, to minimize changes in the existing code, i'd suggest doing
> something in the middle:
> defining a per-transport method, which corresponds more or less to
> what the function do_recv() does today.
> 
> That is, the RX state machine code still asks to read the PDUs by chunks,
> but these chunks are requested by type, not merely by size.
> This way, tcp will read bytes from the stream, while iser will only
> manipulate pointers, setting them to the correct locations within the
> receive buffer. We retain the RX state-machine code almost as is.
> The only changes necessary there are related to the pointers like
> conn->rx_buffer, which  probably  should be moved into the tcp
> realization of .do_read. Perhaps, bhs may be also referenced by pointer.
> But this is it, more or less.

I agree with your analysis.  This does seem like the right
compromise between totally rewriting the state machine and doing the
byte-wise copy in iser we do now.  Conceptually to iscsid, the
difference is that do_read (please rename) can return more than you
asked for.  In iser, we'll hang the recv pdu on the task, in-place, and
adjust ahs, data pointers into it.  For tcp, we'll alloc new space
for ahs and data then call do_read more to copy the bytes in.
Hopefully this can all be abstracted out nicely.

As a side note, I could almost be convinced that having an iser-only
rx state machine would be appropriate.  6 of the 11 states have to
do with digests, which we will never see in iser.  And iser doesn't
need to track rx_iostate at all:  it gets all bhs, ahs, immed data
in one go.  Similar arguments for the tx state machine.  In case you
change your mind about keeping the current state machines, once you
get into the actual implementation.

> Another example of making a coarse-grain separation is allocation & release
> of iscsi tasks. I wanted to write a separate mail about it but this fits the
> current discussion.
> 
> Function conn->tp->ep_free() gets called only in iscsi_free_task(), separately
> for the data and the descriptor. Because the data buffer has no identification
> attached, it must be looked for through the list of the buffers, for iser.
> 
> If we make task_free() a transport-specific pointer, instead, no
> overhead is incurred.
> iscsi will use free(), while iser will return the buffer to the list
> without searching.
> Of course task_alloc() should also be turned into a per-transport method,
> after it is called some basic initializations are done on the common level,
> in iscsid.c. It looks that if done this way, there is no need for
> ep_free/ep_maloc
> anymore.

I like your idea about getting rid of ep_malloc and ep_free.  That
is the wrong level to do the abstraction.  But watch out for
bidi_uaddr.  You don't know the size of the read buffer in a
bidirectional command until the AHS has been processed.  This comes
after task alloc in the current code, hence the second call to
malloc.  In iser this should be a pre-registered buffer, while for
tcp we can get away with normal malloc.  Maybe this issue goes away
if you do task_alloc differently.

> I do want to contribute, both with the PDU queuing and with implementing
> my suggestion above, if we agree on it or on another alternative.

I'm all for it.  It's clear to me that your read_pdu and task_alloc
abstraction points are better than the existing ep_read and
ep_malloc ones.

Let me know if you have questions on any aspects of the current
code.  It's not very heavily commented.  I'll test your patches when
they're ready.

		-- Pete