This patch lets one bind the local side of NFS sockets to a particular
IP address. This can be useful for users on multi-homed systems.
This patch must be on top of the previous patch to fix the IPv6 address
comparison or it will not work.
Comments and suggestions welcome...I'll incorporate those and post an
official signed-off patch after that.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Tue, 2010-09-07 at 14:10 -0700, Ben Greear wrote:
> On 09/07/2010 01:54 PM, Trond Myklebust wrote:
> > On Tue, 2010-09-07 at 13:41 -0700, Ben Greear wrote:
> >> On 09/07/2010 10:56 AM, Trond Myklebust wrote:
> >>> On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
> >>>> This patch lets one bind the local side of NFS sockets to a particular
> >>>> IP address. This can be useful for users on multi-homed systems.
> >>>>
> >>>> This patch must be on top of the previous patch to fix the IPv6 address
> >>>> comparison or it will not work.
> >>>>
> >>>> Comments and suggestions welcome...I'll incorporate those and post an
> >>>> official signed-off patch after that.
> >>>>
> >>>> Thanks,
> >>>> Ben
> >>>>
> >>>
> >>> The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
> >>> Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
> >>> really dislike that new boolean argument to nfs_find_client(). If you
> >>> don't want to compare the source address, then have the caller pass a
> >>> NULL pointer).
> >>
> >>
> >> Would this fix the callback issue you speak of? The idea is to
> >> use source and dest to match if it exists, but if we find one
> >> where server address matches and srcaddr isn't specified,
> >> then we will use that.
> >
> > No. As I said, it needs to match the clientaddr argument, not the
> > srcaddr.
> >
> > The problem is that you are now potentially introducing cases where the
> > server may have multiple combinations of clientaddr and srcaddr.
>
> Ok, so what do you think about allowing a flag to bind() to clientaddr
> instead of having the separate srcaddr option?
That might be slightly less intrusive, but I'm still unconvinced it is
something we need to support in the upstream kernels.
Cheers
Trond
On 09/07/2010 10:56 AM, Trond Myklebust wrote:
> On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
>> This patch lets one bind the local side of NFS sockets to a particular
>> IP address. This can be useful for users on multi-homed systems.
>>
>> This patch must be on top of the previous patch to fix the IPv6 address
>> comparison or it will not work.
>>
>> Comments and suggestions welcome...I'll incorporate those and post an
>> official signed-off patch after that.
>>
>> Thanks,
>> Ben
>>
>
> The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
> Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
> really dislike that new boolean argument to nfs_find_client(). If you
> don't want to compare the source address, then have the caller pass a
> NULL pointer).
Would this fix the callback issue you speak of? The idea is to
use source and dest to match if it exists, but if we find one
where server address matches and srcaddr isn't specified,
then we will use that.
@@ -384,10 +390,30 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
if (!nfs_sockaddr_match_ipaddr(addr, clap))
continue;
+ if (srcaddr) {
+ const struct sockaddr *sa;
+ sa = (const struct sockaddr *)&clp->srcaddr;
+ if (!nfs_sockaddr_match_ipaddr(srcaddr, sa)) {
+ /* If clp doesn't bind to srcaddr, then
+ * it is a potential match if we don't find
+ * a better one.
+ */
+ if (sa->sa_family == AF_UNSPEC && !ok_fit)
+ ok_fit = clp;
+ continue;
+ }
+ }
+found_one:
atomic_inc(&clp->cl_count);
spin_unlock(&nfs_client_lock);
return clp;
}
+
+ if (ok_fit) {
+ clp = ok_fit;
+ goto found_one;
+ }
+
spin_unlock(&nfs_client_lock);
return NULL;
}
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 09/07/2010 01:54 PM, Trond Myklebust wrote:
> On Tue, 2010-09-07 at 13:41 -0700, Ben Greear wrote:
>> On 09/07/2010 10:56 AM, Trond Myklebust wrote:
>>> On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
>>>> This patch lets one bind the local side of NFS sockets to a particular
>>>> IP address. This can be useful for users on multi-homed systems.
>>>>
>>>> This patch must be on top of the previous patch to fix the IPv6 address
>>>> comparison or it will not work.
>>>>
>>>> Comments and suggestions welcome...I'll incorporate those and post an
>>>> official signed-off patch after that.
>>>>
>>>> Thanks,
>>>> Ben
>>>>
>>>
>>> The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
>>> Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
>>> really dislike that new boolean argument to nfs_find_client(). If you
>>> don't want to compare the source address, then have the caller pass a
>>> NULL pointer).
>>
>>
>> Would this fix the callback issue you speak of? The idea is to
>> use source and dest to match if it exists, but if we find one
>> where server address matches and srcaddr isn't specified,
>> then we will use that.
>
> No. As I said, it needs to match the clientaddr argument, not the
> srcaddr.
>
> The problem is that you are now potentially introducing cases where the
> server may have multiple combinations of clientaddr and srcaddr.
Ok, so what do you think about allowing a flag to bind() to clientaddr
instead of having the separate srcaddr option?
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Tue, 2010-09-07 at 13:41 -0700, Ben Greear wrote:
> On 09/07/2010 10:56 AM, Trond Myklebust wrote:
> > On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
> >> This patch lets one bind the local side of NFS sockets to a particular
> >> IP address. This can be useful for users on multi-homed systems.
> >>
> >> This patch must be on top of the previous patch to fix the IPv6 address
> >> comparison or it will not work.
> >>
> >> Comments and suggestions welcome...I'll incorporate those and post an
> >> official signed-off patch after that.
> >>
> >> Thanks,
> >> Ben
> >>
> >
> > The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
> > Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
> > really dislike that new boolean argument to nfs_find_client(). If you
> > don't want to compare the source address, then have the caller pass a
> > NULL pointer).
>
>
> Would this fix the callback issue you speak of? The idea is to
> use source and dest to match if it exists, but if we find one
> where server address matches and srcaddr isn't specified,
> then we will use that.
No. As I said, it needs to match the clientaddr argument, not the
srcaddr.
The problem is that you are now potentially introducing cases where the
server may have multiple combinations of clientaddr and srcaddr.
> @@ -384,10 +390,30 @@ struct nfs_client *nfs_find_client(const struct sockaddr *addr, u32 nfsversion)
> if (!nfs_sockaddr_match_ipaddr(addr, clap))
> continue;
>
> + if (srcaddr) {
> + const struct sockaddr *sa;
> + sa = (const struct sockaddr *)&clp->srcaddr;
> + if (!nfs_sockaddr_match_ipaddr(srcaddr, sa)) {
> + /* If clp doesn't bind to srcaddr, then
> + * it is a potential match if we don't find
> + * a better one.
> + */
> + if (sa->sa_family == AF_UNSPEC && !ok_fit)
> + ok_fit = clp;
> + continue;
> + }
> + }
> +found_one:
> atomic_inc(&clp->cl_count);
> spin_unlock(&nfs_client_lock);
> return clp;
> }
> +
> + if (ok_fit) {
> + clp = ok_fit;
> + goto found_one;
> + }
> +
> spin_unlock(&nfs_client_lock);
> return NULL;
> }
>
>
> Thanks,
> Ben
>
On 09/07/2010 10:56 AM, Trond Myklebust wrote:
> On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
>> This patch lets one bind the local side of NFS sockets to a particular
>> IP address. This can be useful for users on multi-homed systems.
>>
>> This patch must be on top of the previous patch to fix the IPv6 address
>> comparison or it will not work.
>>
>> Comments and suggestions welcome...I'll incorporate those and post an
>> official signed-off patch after that.
>>
>> Thanks,
>> Ben
>>
>
> The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
> Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
> really dislike that new boolean argument to nfs_find_client(). If you
> don't want to compare the source address, then have the caller pass a
> NULL pointer).
It would break things to have clientaddr different than srcaddr, but as
long as they are the same (or either is not specified), I think my patch
will be fine. I have tested NFS without specifying srcaddr and it seems
to work fine, for instance.
I think if one were to pass a flag that said 'bind-to-clientaddr' that
would also solve my needs, but I don't think it would make the patch
much less intrusive.
I'm fine with getting rid of the boolean.
> As has been pointed out to you before, all this is very intrusive, and
> you have yet to give a description of why it is useful, and better than
> using private socket namespaces (which is what container virtualised
> systems will be wanting). The latter can even ensure that it all works
> for userspace applications (such as rpc.statd) too.
Namespaces can be difficult to manage and relatively heavy weight,
so they are no good for my use. This srcaddr patch is useful because
it allows multiple mounts on different network adapters or IP addresses,
(virtual or otherwise). My particular use for this is to do load testing
on NFS servers, but I'm sure other folks with have other uses.
I really don't see how it's so intrusive. It does add some arguments to some
methods, but the overall logic change is quite small.
It should be simple enough to fix rpc.statd to bind to IPs properly,
but I haven't looked at that code.
> IOW: I'd be quite happy to take patches to support private namespaces
> properly: afaics, we do need to make nfs_find_client aware of them, and
> ditto for lockd and the NFSv4 callback channel.
> I remain less than convinced that we need to be able to specify
> per-mountpoint source addresses...
I got my hopes up because the cifs guys were interested in supporting srcaddr,
so I thought maybe NFS folks were now of similar mind. If you still see no use in
it, I'll wait a few more years and try again.
In the meantime, I'll fix the boolean issue and work to fix any other
technical issues that come up.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On 09/07/2010 02:12 PM, Trond Myklebust wrote:
> On Tue, 2010-09-07 at 14:10 -0700, Ben Greear wrote:
>> On 09/07/2010 01:54 PM, Trond Myklebust wrote:
>>> No. As I said, it needs to match the clientaddr argument, not the
>>> srcaddr.
>>>
>>> The problem is that you are now potentially introducing cases where the
>>> server may have multiple combinations of clientaddr and srcaddr.
>>
>> Ok, so what do you think about allowing a flag to bind() to clientaddr
>> instead of having the separate srcaddr option?
>
> That might be slightly less intrusive, but I'm still unconvinced it is
> something we need to support in the upstream kernels.
It seems clientaddr is saved as a string instead of a struct sockaddr_storage,
which means doing conversion in lots of places. So, using a bindclient flag
and clientaddr would be even trickier than my srcaddr= approach.
I can still try to get that working if that's the only hurdle to upstream
inclusion, but if it's all for my own use, I'm just going to use the same
api as for cifs, and ensure the caller always uses same thing for srcaddr
and clientaddr.
Thanks,
Ben
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Fri, 2010-09-03 at 11:55 -0700, Ben Greear wrote:
> This patch lets one bind the local side of NFS sockets to a particular
> IP address. This can be useful for users on multi-homed systems.
>
> This patch must be on top of the previous patch to fix the IPv6 address
> comparison or it will not work.
>
> Comments and suggestions welcome...I'll incorporate those and post an
> official signed-off patch after that.
>
> Thanks,
> Ben
>
The code in nfs_callback_authenticate is going to break NFSv4 callbacks.
Callbacks are sent to the -oclientaddr address, not srcaddr (btw, I
really dislike that new boolean argument to nfs_find_client(). If you
don't want to compare the source address, then have the caller pass a
NULL pointer).
As has been pointed out to you before, all this is very intrusive, and
you have yet to give a description of why it is useful, and better than
using private socket namespaces (which is what container virtualised
systems will be wanting). The latter can even ensure that it all works
for userspace applications (such as rpc.statd) too.
IOW: I'd be quite happy to take patches to support private namespaces
properly: afaics, we do need to make nfs_find_client aware of them, and
ditto for lockd and the NFSv4 callback channel.
I remain less than convinced that we need to be able to specify
per-mountpoint source addresses...
Trond