recent failover-by-IP changes

recent failover-by-IP changes

[Chuck Lever]

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

Re: recent failover-by-IP changes

[J. Bruce Fields]

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?

Re: recent failover-by-IP changes

[Chuck Lever]

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

Re: recent failover-by-IP changes

[J. Bruce Fields]

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.

Re: recent failover-by-IP changes

[Chuck Lever]

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

Re: recent failover-by-IP changes

[J. Bruce Fields]

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.

Re: recent failover-by-IP changes

[Wendy Cheng]

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

Re: recent failover-by-IP changes

[Chuck Lever]

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