[EMAIL PROTECTED] wrote on Tue, 06 Nov 2007 10:50 -0600:
> The attached patch implements BMI multiple address endpoints that we
> talked about some time ago. To refresh everyone's memory, this
> allows a set of addresses to be specified:
>
> tcp://host1:3334/pvfs2-fs,tcp://host2:3335/pvfs2-fs,tcp:://host3:3336/
> pvfs2-fs
>
> In the config file for a given storage endpoint. The BMI code
> manages the endpoint, setting the currently used address to the first
> one in the list. On message failure, the endpoint is transitioned to
> point to the next address in the list. This continues in a round-
> robin fashion.
This is good stuff. I'd like to help review it. Can you do some
trivial things first to make that easier?
1.
> + struct bmi_endpoint_ref_s *newref;
[..]
> + /* haven't seen any of these addresses before, add a new endpoint */
> + newref = malloc(sizeof(struct bmi_addr_ref_s));
Please change all malloc(sizeof()) and memset(,,sizeof()) to use the
variable name, not its type. The bug you did above (and other
places) is way too common. We just have to stop doing that. Like
this instead:
newref = malloc(sizeof(*newref));
2.
There's a bunch of stuff that seems out of place. Can you check
that in or push it to the side so we can concentrate on the core?
179 kB is a big patch. :)
Renaming all the method_ops is a big uninteresting part.
Adding bmi_ in front of everything too, but it's too hard to rip
that out mid-patch now. Random whitespace fixes too. Good changes,
just hard to read.
3.
Some trivial bugs.
> + ref->current = ref->current + 1 % ref->count;
Check your precedence table.
> + * (C) 2001 Clemson University and The University of Chicago
New files go back in time.
Can you put some comments above each of the three new structs in
bmi-addr.c? I keep getting confused on which is the old-style
addr and which is the comma-separated list. And what the various
"link" and "refs" fields point to.
> I've done some basic testing, but there's still more to do. The
> client IO state machine is a bear, and testing all the cases where
> things could failover (requests, flows, acks, etc.) is going to take
> some more work. I wanted to get the patch out there to allow others
> to provide feedback.
Yeah, totally. But it can be made to work.
What did we decide do with mixed method usage? The old semantic
was "ib://foo:2345/pvfs2-fs,tcp://foo:2347/pvfs2-fs" means try to
use IB, but if you don't have an IB nic, switch to TCP. I agree we
decided that was less interesting. Do we just add docs that say
that this comma is now for multi-pathing? If people try this
example with the new code, it will flip from IB to TCP at every
timeout. The old behavior was to stick with the first one where
you had the hardware. In other words, probably some docs somewhere
should be added to this patch.
On the server side, the ,-separated addresses mean "listen on all
these interfaces". What do servers do now when they see your
tcp://host1,tcp://host2 example string above? Looks like they would
fail to listen on anything (host1 can't bind to host2 address?).
This has to be in the fs.conf as the Alias string for each server so
that clients can find the IO servers, not just in the pvfs2tab to
find the config server.
-- Pete
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
