I read the code. Here's some comments.
Lots of iser.h is original code from OSC, please put back copyright
headers on that file too. Thanks for keeping us on all the other
files.
You should add a patch 4/3 to delete the old RDMA code, to avoid
confusion with having two in the tree. And also excise all the
hooks that old RDMA code put into what used to be generic iscsi
code. Transport abstraction changes too. Since you have copied
everything, there should be no need to test for ->rdma.
I'm still unhappy how you duplicated all the text handling. Seems
like there should have been a way to abstract the buffer handling
more cleanly.
Reading the big iser_ib.c,
->refcount is a good idea, nice improvement
iser_task_add_out_rdma_buf - also a good idea
sched_rdma_rd, iosubmit, etc. I like how you broke up all the
parts involved in handling requests; we only had maybe half as
many phases. But this schedule() interface worries me. It
looks like it only gets called when the fds are quiet. That
seems like a bad way to manage load. As soon as you accept a
request, you want to push it all the way through. We ran into
this problem with iscsi_rdma.
Too bad you broke AHS and bidir. That worked in iscsi_rdma.
I worry you'll never get around to fixing it.
Performance graphs would be useful to allay my concerns about your
use of schedule(), and make sure the fine-grained approach is valid.
Interesting how you changed MAX_POLL_WC to 32. That wants to be
tunable more dynamically.
Testing? Here's some of the ways we beat on the iscsi_rdma code
way back when:
- long client runs, see if memory leaks and/or performance drops
- lots of short-lived clients, ditto
- lots of clients that die at random areas of communication
(using netfilter to block particular client IPs makes this easy)
Hope you are able to spend some time with the code. Some of your
modifications can have a good impact. And the fine-grained sub-task
management is a good idea too, if you can tune the scheduler. I've
already whined about doing a full cut-n-paste instead of just fixing
the existing code, but that's up to you and Tomo.
I'm not going to be working on tgt, not because of your changes,
just because my interest lies elsewhere these days. Will happily
look at graphs or patches if that can help, though.
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
