Chuck Lever wrote:
On Jan 22, 2009, at Jan 22, 2009, 12:35 AM, Ben Greear wrote:
Chuck Lever wrote:
A handful of generic comments.
1. This needs to be broken into smaller patches before submission;
preferably before you submit another version for review. Take a look
at the linux-nfs@xxxxxxxxxxxxxxx archives to see how we handle large
changes like this.
2. You should support local addresses only in the text-based path
(utils/mount/stropts.c) and not in the legacy paths
(utils/mount/nfs[4]mount.c). I don't think we're ever going to allow
a version 7 of the mount data structure.
I could remove the version 7 data field, but what about the other code
that creates
sockets for 'pinging' nfs daemons and such? Is that code deprecated
now?
It's still used for text-based NFSv2/v3 mounts, and unmounts. The mount
option rewriting code in stropts.c probably needs to be sensitive of the
local address too. That's the piece that uses all the getport functions.
If nothing else, it looks like "probe_bothports" needs a client-addr
to pass on
to methods it calls, and so forth. That means that other code that
calls those
methods needs to be updated, and thus my huge repetitive patch.
Expanding the synopses of all the getport calls in support/nfs/*.c is
part of what can be split into separate patches. I would start with
support/nfs/rpc_socket.c (the lowest level) and move upwards.
I assume each patch would need to compile, so as I change the low
level, then I'm going to have to change the rest of the levels until
it will compile. Unless each intermediate patch adds dummy addrs
(ie, ADDR_ANY sockaddr structs), and removes the lower level dummy,
I don't see how to break this into several patches. And adding/removing
dummy code just to break up the patches seems a lot of wasted effort to me.
Note that only some of these functions are actually used. Some are
included just to provide a complete getport API.
Just an idea: you might consider allowing the passed in local sockaddr
to point to NULL. In that case, just default to INADDR_ANY. That might
make the upper levels easier to code.
I thought this might make it harder to deal with IPv6 v/s IPv4. If
we're always passing in an struct sockaddr then it can have the appropriate
family already set. This also allows the code to create INADDR_ANY addresses in just
a few places instead of all the low-level methods, and keeps us from having
to do lots of error checking against a null local-addr.
3. There are some umount-related changes coming up for IPv6 that
will touch the umount paths here; that may require some changes in
your modifications.
That should be fine. I tried to make my current patch friendly for
IPv6 and want to support local
IPv6 binding as well.
The upcoming IPv6 support adds a new unmount function that replaces the
functions that currently support only sockaddr_in. That new function is
probably what you would need to modify instead of updating the mnt_*
functions, which will only be used by the legacy part of the mount.nfs
command after my patches.
Any idea when this will hit the git repository?
Can you give an example of a user-space command that would use the
new mount option as you suggest? Do you do /etc/fstab any different
for instance?
Let's call it "localaddr=". Something like:
mount -o tcp,vers=3,localaddr=192.168.0.59 server:/export
Why not clientaddr? That is already used for NFS version 4, and it works
just fine for version 3 as well. I can't think of any reason to have
a new variable.
I think this address would also need to be passed to the kernel's lockd
at mount time by expanding the argument list to nlmclnt_init() and
nlmclnt_lookup_host() in the kernel NFS client. I think lockd already
has some ability to support a local address, but that may be server-side
only.
I'll look at this as well. I have a kernel patch that *appears* to work,
but it may still be missing binding in a few of the more obscure paths.
Still thinking about what needs to be done for NSM/statd.
You will also need to take some care with how NFSv4 generates the
SETCLIENTID request, which sends the client's IP callback address and
port to the server. That will have to take the specified local address
instead. Take a look at the logic in stropts.c around the clientaddr=
option.
If neither localaddr= or clientaddr= is specified, call
nfs_callback_address(). If clientaddr= is specified, use the value of
clientaddr=. If localaddr= is specified, but clientaddr= is not, use
the localaddr= value.
Thanks for the suggestions!
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
