Hi Wendy-
Looking at your recent lockd-failover-by-IP changes... I'd like to
make sure I understand this logic before I merge it into my NLM IPv6
patch set.
In fs/lockd/svcsubs.c:
> static int
> nlmsvc_match_ip(void *datap, struct nlm_host *host)
> {
> __be32 *server_addr = datap;
>
> return host->h_saddr.sin_addr.s_addr == *server_addr;
h_saddr is the local host's source address, not the server address,
and is used only on multi-interface systems. Is that what you wanted
to compare, or did you mean ->h_addr?
Does it make sense to use nlm_cmp_addr() here as is done in other
places in lockd?
> }
>
> int
> nlmsvc_unlock_all_by_ip(__be32 server_addr)
Should this be "struct in_addr server_addr" ? It would be even nicer
if this were a "struct sockaddr *".
> {
> int ret;
> ret = nlm_traverse_files(&server_addr, nlmsvc_match_ip, NULL);
> return ret ? -EIO : 0;
> }
> EXPORT_SYMBOL_GPL(nlmsvc_unlock_all_by_ip);
The only call site for nlmsvc_unlock_all_by_ip() is in fs/nfsd/nfsctl.c:
> /* get ipv4 address */
> if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> return -EINVAL;
> server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>
> return nlmsvc_unlock_all_by_ip(server_ip);
Why can't you use in4_pton() to convert your IP address?
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On May 2, 2008, at 6:00 PM, Wendy Cheng wrote:
> Chuck Lever wrote:
>> Hi Wendy-
>>
>> Looking at your recent lockd-failover-by-IP changes... I'd like to
>> make sure I understand this logic before I merge it into my NLM
>> IPv6 patch set.
>>
>> In fs/lockd/svcsubs.c:
>> > static int
>> > nlmsvc_match_ip(void *datap, struct nlm_host *host)
>> > {
>> > __be32 *server_addr = datap;
>> >
>> > return host->h_saddr.sin_addr.s_addr == *server_addr;
>>
>> h_saddr is the local host's source address, not the server address,
>> and is used only on multi-interface systems. Is that what you
>> wanted to compare, or did you mean ->h_addr?
>
> Yes, it is what we want (trying not to change the mainline logic if
> all possible). Bruce did the merge and he did this *right*. Check
> out how nlm_lookup_host() fills in the h_saddr (and how
> nlmsvc_lookup_host passes in "ssin") before our changes. But I'll do
> the testing over the weekend nevertheless - will report back if I
> see problems.
>
> We'll let you worry about how to make ipv6 working in this
> case :) .. (just joking .. will help if needed) ..
>
>> Does it make sense to use nlm_cmp_addr() here as is done in other
>> places in lockd?
>
> It can - but would suggest leave it as today until your ipv6 patches
> are ready (are they ready now ?).
Yes. I plan to test basic functionality next week at Connectathon.
>> > }
>> >
>> > int
>> > nlmsvc_unlock_all_by_ip(__be32 server_addr)
>>
>> Should this be "struct in_addr server_addr" ? It would be even
>> nicer if this were a "struct sockaddr *".
>
> Yes, it would be nice. But it would be even "nicer" if we let people
> run this stuff before another round of revising.
Given that the IPv6 support needs to be finished soon (IPv6 support
will be a purchasing requirement for US government later this year), I
don't think we can wait. The changes are not terribly invasive, and
can be validated with IPv4 testing.
>> > /* get ipv4 address */
>> > if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
>> > return -EINVAL;
>> > server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>> >
>> > return nlmsvc_unlock_all_by_ip(server_ip);
>>
>> Why can't you use in4_pton() to convert your IP address?
> I think in4_pton was my original code (?) We changed it because of a
> legitimate comment from a code review (but I forgot the details from
> the top of my head at this moment). Will follow up after I check the
> archives tomorrow.
Yes, I'd like to see the reviewer's comment.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, May 02, 2008 at 04:58:58PM -0400, Chuck Lever wrote:
> Hi Wendy-
>
> Looking at your recent lockd-failover-by-IP changes... I'd like to make
> sure I understand this logic before I merge it into my NLM IPv6 patch
> set.
>
> In fs/lockd/svcsubs.c:
> > static int
> > nlmsvc_match_ip(void *datap, struct nlm_host *host)
> > {
> > __be32 *server_addr = datap;
> >
> > return host->h_saddr.sin_addr.s_addr == *server_addr;
>
> h_saddr is the local host's source address, not the server address, and
> is used only on multi-interface systems. Is that what you wanted to
> compare, or did you mean ->h_addr?
This is server-side code--h_saddr, last I checked, isn't even filled in
on the client side. So the current host *is* the server.
--b.
>
> Does it make sense to use nlm_cmp_addr() here as is done in other places
> in lockd?
>
> > }
> >
> > int
> > nlmsvc_unlock_all_by_ip(__be32 server_addr)
>
> Should this be "struct in_addr server_addr" ? It would be even nicer if
> this were a "struct sockaddr *".
>
> > {
> > int ret;
> > ret = nlm_traverse_files(&server_addr, nlmsvc_match_ip, NULL);
> > return ret ? -EIO : 0;
> > }
> > EXPORT_SYMBOL_GPL(nlmsvc_unlock_all_by_ip);
>
> The only call site for nlmsvc_unlock_all_by_ip() is in fs/nfsd/nfsctl.c:
>
> > /* get ipv4 address */
> > if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> > return -EINVAL;
> > server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> >
> > return nlmsvc_unlock_all_by_ip(server_ip);
>
> Why can't you use in4_pton() to convert your IP address?
On May 2, 2008, at 5:26 PM, J. Bruce Fields wrote:
> On Fri, May 02, 2008 at 04:58:58PM -0400, Chuck Lever wrote:
>> Hi Wendy-
>>
>> Looking at your recent lockd-failover-by-IP changes... I'd like to
>> make
>> sure I understand this logic before I merge it into my NLM IPv6 patch
>> set.
>>
>> In fs/lockd/svcsubs.c:
>>> static int
>>> nlmsvc_match_ip(void *datap, struct nlm_host *host)
>>> {
>>> __be32 *server_addr = datap;
>>>
>>> return host->h_saddr.sin_addr.s_addr == *server_addr;
>>
>> h_saddr is the local host's source address, not the server address,
>> and
>> is used only on multi-interface systems. Is that what you wanted to
>> compare, or did you mean ->h_addr?
>
> This is server-side code--h_saddr, last I checked, isn't even filled
> in
> on the client side. So the current host *is* the server.
So this API is requesting that the local host should drop locks?
Okay, that makes sense. It's not well documented in the code, though.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, May 02, 2008 at 05:35:47PM -0400, Chuck Lever wrote:
> On May 2, 2008, at 5:26 PM, J. Bruce Fields wrote:
>> On Fri, May 02, 2008 at 04:58:58PM -0400, Chuck Lever wrote:
>>> Hi Wendy-
>>>
>>> Looking at your recent lockd-failover-by-IP changes... I'd like to
>>> make
>>> sure I understand this logic before I merge it into my NLM IPv6 patch
>>> set.
>>>
>>> In fs/lockd/svcsubs.c:
>>>> static int
>>>> nlmsvc_match_ip(void *datap, struct nlm_host *host)
>>>> {
>>>> __be32 *server_addr = datap;
>>>>
>>>> return host->h_saddr.sin_addr.s_addr == *server_addr;
>>>
>>> h_saddr is the local host's source address, not the server address,
>>> and
>>> is used only on multi-interface systems. Is that what you wanted to
>>> compare, or did you mean ->h_addr?
>>
>> This is server-side code--h_saddr, last I checked, isn't even filled
>> in
>> on the client side. So the current host *is* the server.
>
> So this API is requesting that the local host should drop locks?
It's requesting that the server-side lockd drop any locks that it's
holding on behalf of any clients that accessed the server through the
given ip address.
> Okay,
> that makes sense. It's not well documented in the code, though.
Could be; suggestions welcomed.
--b.
On May 2, 2008, at 5:40 PM, J. Bruce Fields wrote:
> On Fri, May 02, 2008 at 05:35:47PM -0400, Chuck Lever wrote:
>> On May 2, 2008, at 5:26 PM, J. Bruce Fields wrote:
>>> On Fri, May 02, 2008 at 04:58:58PM -0400, Chuck Lever wrote:
>>>> Hi Wendy-
>>>>
>>>> Looking at your recent lockd-failover-by-IP changes... I'd like to
>>>> make
>>>> sure I understand this logic before I merge it into my NLM IPv6
>>>> patch
>>>> set.
>>>>
>>>> In fs/lockd/svcsubs.c:
>>>>> static int
>>>>> nlmsvc_match_ip(void *datap, struct nlm_host *host)
>>>>> {
>>>>> __be32 *server_addr = datap;
>>>>>
>>>>> return host->h_saddr.sin_addr.s_addr == *server_addr;
>>>>
>>>> h_saddr is the local host's source address, not the server address,
>>>> and
>>>> is used only on multi-interface systems. Is that what you wanted
>>>> to
>>>> compare, or did you mean ->h_addr?
>>>
>>> This is server-side code--h_saddr, last I checked, isn't even filled
>>> in
>>> on the client side. So the current host *is* the server.
>>
>> So this API is requesting that the local host should drop locks?
>
> It's requesting that the server-side lockd drop any locks that it's
> holding on behalf of any clients that accessed the server through the
> given ip address.
>
>> Okay,
>> that makes sense. It's not well documented in the code, though.
>
> Could be; suggestions welcomed.
I'll add a patch to my IPv6 series, and we can look at it when we go
over 2.6.27 merge candidates.
--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com
On Fri, May 02, 2008 at 05:56:09PM -0400, Chuck Lever wrote:
> On May 2, 2008, at 5:40 PM, J. Bruce Fields wrote:
>> On Fri, May 02, 2008 at 05:35:47PM -0400, Chuck Lever wrote:
>>> On May 2, 2008, at 5:26 PM, J. Bruce Fields wrote:
>>>> On Fri, May 02, 2008 at 04:58:58PM -0400, Chuck Lever wrote:
>>>>> Hi Wendy-
>>>>>
>>>>> Looking at your recent lockd-failover-by-IP changes... I'd like to
>>>>> make
>>>>> sure I understand this logic before I merge it into my NLM IPv6
>>>>> patch
>>>>> set.
>>>>>
>>>>> In fs/lockd/svcsubs.c:
>>>>>> static int
>>>>>> nlmsvc_match_ip(void *datap, struct nlm_host *host)
>>>>>> {
>>>>>> __be32 *server_addr = datap;
>>>>>>
>>>>>> return host->h_saddr.sin_addr.s_addr == *server_addr;
>>>>>
>>>>> h_saddr is the local host's source address, not the server address,
>>>>> and
>>>>> is used only on multi-interface systems. Is that what you wanted
>>>>> to
>>>>> compare, or did you mean ->h_addr?
>>>>
>>>> This is server-side code--h_saddr, last I checked, isn't even filled
>>>> in
>>>> on the client side. So the current host *is* the server.
>>>
>>> So this API is requesting that the local host should drop locks?
>>
>> It's requesting that the server-side lockd drop any locks that it's
>> holding on behalf of any clients that accessed the server through the
>> given ip address.
>>
>>> Okay,
>>> that makes sense. It's not well documented in the code, though.
>>
>> Could be; suggestions welcomed.
>
> I'll add a patch to my IPv6 series, and we can look at it when we go
> over 2.6.27 merge candidates.
OK, thanks.--b.
Chuck Lever wrote:
> Hi Wendy-
>
> Looking at your recent lockd-failover-by-IP changes... I'd like to
> make sure I understand this logic before I merge it into my NLM IPv6
> patch set.
>
> In fs/lockd/svcsubs.c:
> > static int
> > nlmsvc_match_ip(void *datap, struct nlm_host *host)
> > {
> > __be32 *server_addr = datap;
> >
> > return host->h_saddr.sin_addr.s_addr == *server_addr;
>
> h_saddr is the local host's source address, not the server address,
> and is used only on multi-interface systems. Is that what you wanted
> to compare, or did you mean ->h_addr?
Yes, it is what we want (trying not to change the mainline logic if all
possible). Bruce did the merge and he did this *right*. Check out how
nlm_lookup_host() fills in the h_saddr (and how nlmsvc_lookup_host
passes in "ssin") before our changes. But I'll do the testing over the
weekend nevertheless - will report back if I see problems.
We'll let you worry about how to make ipv6 working in this case :) ..
(just joking .. will help if needed) ..
>
> Does it make sense to use nlm_cmp_addr() here as is done in other
> places in lockd?
It can - but would suggest leave it as today until your ipv6 patches are
ready (are they ready now ?).
>
> > }
> >
> > int
> > nlmsvc_unlock_all_by_ip(__be32 server_addr)
>
> Should this be "struct in_addr server_addr" ? It would be even nicer
> if this were a "struct sockaddr *".
Yes, it would be nice. But it would be even "nicer" if we let people run
this stuff before another round of revising.
>
> > {
> > int ret;
> > ret = nlm_traverse_files(&server_addr, nlmsvc_match_ip, NULL);
> > return ret ? -EIO : 0;
> > }
> > EXPORT_SYMBOL_GPL(nlmsvc_unlock_all_by_ip);
>
> The only call site for nlmsvc_unlock_all_by_ip() is in fs/nfsd/nfsctl.c:
I know .. It is a result from last round of code review. I don't have a
strong opinion about it though.
>
> > /* get ipv4 address */
> > if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> > return -EINVAL;
> > server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> >
> > return nlmsvc_unlock_all_by_ip(server_ip);
>
> Why can't you use in4_pton() to convert your IP address?
>
I think in4_pton was my original code (?) We changed it because of a
legitimate comment from a code review (but I forgot the details from the
top of my head at this moment). Will follow up after I check the
archives tomorrow.
BTW, thank you for doing this ipv6 thing - not a trivial task.
-- Wendy