[stgt] [PATCH RFC] new iser code

[Pete Wyckoff]

alexandern at voltaire.com wrote on Tue, 06 Jul 2010 18:38 +0300:
> This patch reflects a preliminary version of iser-related changes.
> 
> It contains a re-write of the iser code (file iscsi_rdma.c
> is replaced by iser_ib.c to simplify diff). New header iser.h is
> created. iser defines a separate transport lld (named "iser").

Diff is very hard to read.  Big chunks of the old code don't show
up as identical due to how you renamed everything.  There's no need
for renaming; do it first or last in a series of patches if you
really think it is necessary.

> Some code, mostly iscsi text related, is replicated and put into
> a new file iser_text.c, mainly because the functions there are
> not general enough, and rely on specifics of iscsi/tcp structs.
> 
> Large portions of code are moved from iscsid.c and shuffled around.

Why did you duplicate so many shared iscsi functions?
iser_scsi_cmd_done etc. should not know about the particulars of the
transport.  You got rid of the whole transport abstraction?

> This code seems to fix an occasional data corruption that happens
> with the current version.

This would be interesting.  How about isolating the changes, and
describing them one at a time?

> There are many unhandled error cases and rare conditions, left
> until there is a solid common iscsi framework to rely upon. Most of
> them are marked with ToDo comments.
> 
> The code is fairly RDMA-transport agnostic (ib/iwarp), but it was
> never verified over iwarp (this fact is reflected in the file name
> iser_ib.c).
> 
> The code implies RDMA-only mode of work. This means the first burst
> incl. immediate data should be disabled, so that the entire data transfer
> is performed using RDMA. It introduces some preparations for handling
> other (general) scenarios, but as tgt has no framework for multi-buffer
> commands, these extra code segments are either commented or conditioned
> upon events that should never take place. All such places have a ToDo
> comment over them.

Immediate data is a nice optimization for short writes.  I'd like to
support it if possible.

> This specific patch compiles when applied to the current git head,
> but was not verified to work with it (as it'd been branched off from
> a rather old commit).

I'm excited that you're starting to work on this, and contributing
it back.  But it's hard to evaluate what you're doing in a big patch
like this.

		-- Pete
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html