nfs-utils: support/nfs/rpcmisc.c

nfs-utils: support/nfs/rpcmisc.c

[Chuck Lever]

Hi all-

I've been stuck on updating the rpc_init() function in support/nfs/ 
rpcmisc.c to use TI-RPC functions for a while now.  This week I found  
yet another issue, and would like to ask your opinion about how to  
architect a solution.

rpc_init() is used only by rpc.statd and rpc.mountd, to set up their  
RPC listeners.  It has logic in it to detect the case where fd 0 is a  
socket being passed in from inetd (I think that's what this does).

It also has some statics to save the UDP and TCP listener xprt's from  
a previous call, so it doesn't create these again.  Specifically for  
rpc.statd, rpc_init() is called only once to set up the UDP and TCP  
listeners, so I don't think those statics would ever be referenced  
after being set during the first call.

Does rpc_init() really need to save the xprts for rpc.statd?   
rpc_init() peeks at xp_port in the returned SVC_XPRT to decide whether  
to use the saved xprt, but TI-RPC's service creation API doesn't fill  
this field in.  So this logic won't even work for xprts created with  
TI-RPC calls.  I seem to recall seeing some code in libtirpc that  
already checks a list of xprts to prevent the creation of duplicates.

I also notice that rpc_init() uses xlog() to report errors, whereas  
rpc.statd uses note(), and does not initialize the xlog() reporting  
framework.  So rpc.statd problems in this area are sporadically  
reported, often without any identifying program name.

And, for rpc.statd, do the listener sockets need to be non-blocking?   
There are four paths in rpc_init() that create listener sockets:  one  
is by passing RPC_ANYSOCK to the RPC library; one is using the pre- 
existing fd 0 if it's a socket.  The other two are local  
implementations that create a fresh socket, but one sets O_NONBLOCK,  
and the other doesn't.  As far as I can tell, O_NONBLOCK support was  
added just for rpc.mountd, but three of these four paths probably do  
not create non-blocking listener sockets.  What does rpc.statd need?

I'm considering creating a version of rpc_init() under utils/statd  
that strips out all of the unnecessary features, uses note() instead  
of xlot(), and then adds support for AF_INET6.

Does this make sense, or am I missing something?

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfs-utils: support/nfs/rpcmisc.c

[Steve Dickson]

Chuck Lever wrote:
> Hi all-
> 
> I've been stuck on updating the rpc_init() function in
> support/nfs/rpcmisc.c to use TI-RPC functions for a while now.  This
> week I found yet another issue, and would like to ask your opinion about
> how to architect a solution.
> 
> rpc_init() is used only by rpc.statd and rpc.mountd, to set up their RPC
> listeners.  It has logic in it to detect the case where fd 0 is a socket
> being passed in from inetd (I think that's what this does).
I would have to agree... why else would a getsockname(0, ....) be done?

> 
> It also has some statics to save the UDP and TCP listener xprt's from a
> previous call, so it doesn't create these again.  Specifically for
> rpc.statd, rpc_init() is called only once to set up the UDP and TCP
> listeners, so I don't think those statics would ever be referenced after
> being set during the first call.
> 
> Does rpc_init() really need to save the xprts for rpc.statd?  rpc_init()
> peeks at xp_port in the returned SVC_XPRT to decide whether to use the
> saved xprt, but TI-RPC's service creation API doesn't fill this field
> in.  So this logic won't even work for xprts created with TI-RPC calls. 
> I seem to recall seeing some code in libtirpc that already checks a list
> of xprts to prevent the creation of duplicates.
I would say no... reusing connections is always a tricky proposition at best
and generally fairly error prone... but if we were going to reuse connections
I would like to see the logic broken out into to different routines, similar
to rpc_init() and rpc_reinit()... 
  
> 
> I also notice that rpc_init() uses xlog() to report errors, whereas
> rpc.statd uses note(), and does not initialize the xlog() reporting
> framework.  So rpc.statd problems in this area are sporadically
> reported, often without any identifying program name.
This is one area that nfs-utils is a bit out of control... every 
daemon seems to have it own way of logging... 

> 
> And, for rpc.statd, do the listener sockets need to be non-blocking? 
> There are four paths in rpc_init() that create listener sockets:  one is
> by passing RPC_ANYSOCK to the RPC library; one is using the pre-existing
> fd 0 if it's a socket.  The other two are local implementations that
> create a fresh socket, but one sets O_NONBLOCK, and the other doesn't. 
> As far as I can tell, O_NONBLOCK support was added just for rpc.mountd,
> but three of these four paths probably do not create non-blocking
> listener sockets.  What does rpc.statd need?
I would say no... The O_NONBLOCK business seems to be needed for mountd to
run with multiple processes reading from the same socket... Since statd 
does not do this, there is no needed for  O_NONBLOCK to be set...

> 
> I'm considering creating a version of rpc_init() under utils/statd that
> strips out all of the unnecessary features, uses note() instead of
> xlot(), and then adds support for AF_INET6.
I would say... clean it up... esp when it comes to using the same logging
routines... 


steved.

Re: nfs-utils: support/nfs/rpcmisc.c

[Chuck Lever]

On Jan 28, 2009, at 7:21 AM, Steve Dickson wrote:
> Chuck Lever wrote:
>> Hi all-
>>
>> I've been stuck on updating the rpc_init() function in
>> support/nfs/rpcmisc.c to use TI-RPC functions for a while now.  This
>> week I found yet another issue, and would like to ask your opinion  
>> about
>> how to architect a solution.
>>
>> rpc_init() is used only by rpc.statd and rpc.mountd, to set up  
>> their RPC
>> listeners.  It has logic in it to detect the case where fd 0 is a  
>> socket
>> being passed in from inetd (I think that's what this does).
> I would have to agree... why else would a getsockname(0, ....) be  
> done?
>
>> It also has some statics to save the UDP and TCP listener xprt's  
>> from a
>> previous call, so it doesn't create these again.  Specifically for
>> rpc.statd, rpc_init() is called only once to set up the UDP and TCP
>> listeners, so I don't think those statics would ever be referenced  
>> after
>> being set during the first call.
>>
>> Does rpc_init() really need to save the xprts for rpc.statd?   
>> rpc_init()
>> peeks at xp_port in the returned SVC_XPRT to decide whether to use  
>> the
>> saved xprt, but TI-RPC's service creation API doesn't fill this field
>> in.  So this logic won't even work for xprts created with TI-RPC  
>> calls.
>> I seem to recall seeing some code in libtirpc that already checks a  
>> list
>> of xprts to prevent the creation of duplicates.
> I would say no... reusing connections is always a tricky proposition  
> at best
> and generally fairly error prone... but if we were going to reuse  
> connections
> I would like to see the logic broken out into to different routines,  
> similar
> to rpc_init() and rpc_reinit()...

Well, AFAICT it's not re-using the listeners, but allowing other  
versions of the RPC protocol to be handled by the same listener socket.

It looks to me like TI-RPC handles this automatically in the library  
already, but legacy RPC may have required some special logic to deal  
with registering multiple versions of the mountd protocol on the same  
port.

This saves a port number or two, but it means that all these mountd  
processes share the same socket, which may limit scalability somewhat.

>> I also notice that rpc_init() uses xlog() to report errors, whereas
>> rpc.statd uses note(), and does not initialize the xlog() reporting
>> framework.  So rpc.statd problems in this area are sporadically
>> reported, often without any identifying program name.
> This is one area that nfs-utils is a bit out of control... every
> daemon seems to have it own way of logging...
>
>> And, for rpc.statd, do the listener sockets need to be non-blocking?
>> There are four paths in rpc_init() that create listener sockets:   
>> one is
>> by passing RPC_ANYSOCK to the RPC library; one is using the pre- 
>> existing
>> fd 0 if it's a socket.  The other two are local implementations that
>> create a fresh socket, but one sets O_NONBLOCK, and the other  
>> doesn't.
>> As far as I can tell, O_NONBLOCK support was added just for  
>> rpc.mountd,
>> but three of these four paths probably do not create non-blocking
>> listener sockets.  What does rpc.statd need?
> I would say no... The O_NONBLOCK business seems to be needed for  
> mountd to
> run with multiple processes reading from the same socket... Since  
> statd
> does not do this, there is no needed for  O_NONBLOCK to be set...
>
>> I'm considering creating a version of rpc_init() under utils/statd  
>> that
>> strips out all of the unnecessary features, uses note() instead of
>> xlot(), and then adds support for AF_INET6.
> I would say... clean it up... esp when it comes to using the same  
> logging
> routines...

I'm working on this right now.  It seems the requirements for mountd  
and statd are different enough that it makes sense for each to have  
its own.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfs-utils: support/nfs/rpcmisc.c

[Jeff Layton]

On Tue, 27 Jan 2009 15:20:02 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> Hi all-
> 
> I've been stuck on updating the rpc_init() function in support/nfs/ 
> rpcmisc.c to use TI-RPC functions for a while now.  This week I found  
> yet another issue, and would like to ask your opinion about how to  
> architect a solution.
> 
> rpc_init() is used only by rpc.statd and rpc.mountd, to set up their  
> RPC listeners.  It has logic in it to detect the case where fd 0 is a  
> socket being passed in from inetd (I think that's what this does).
> 

Weird. That would be my guess too...

> It also has some statics to save the UDP and TCP listener xprt's from  
> a previous call, so it doesn't create these again.  Specifically for  
> rpc.statd, rpc_init() is called only once to set up the UDP and TCP  
> listeners, so I don't think those statics would ever be referenced  
> after being set during the first call.
> 
> Does rpc_init() really need to save the xprts for rpc.statd?   
> rpc_init() peeks at xp_port in the returned SVC_XPRT to decide whether  
> to use the saved xprt, but TI-RPC's service creation API doesn't fill  
> this field in.  So this logic won't even work for xprts created with  
> TI-RPC calls.  I seem to recall seeing some code in libtirpc that  
> already checks a list of xprts to prevent the creation of duplicates.
> 

It doesn't look like this is necessary for statd. rpc_init looks like
it's only called once from main() before you enter the service loop,
so I don't think statd will ever have the opportunity to create a
duplicate xprt.

> I also notice that rpc_init() uses xlog() to report errors, whereas  
> rpc.statd uses note(), and does not initialize the xlog() reporting  
> framework.  So rpc.statd problems in this area are sporadically  
> reported, often without any identifying program name.
> 
> And, for rpc.statd, do the listener sockets need to be non-blocking?   
> There are four paths in rpc_init() that create listener sockets:  one  
> is by passing RPC_ANYSOCK to the RPC library; one is using the pre- 
> existing fd 0 if it's a socket.  The other two are local  
> implementations that create a fresh socket, but one sets O_NONBLOCK,  
> and the other doesn't.  As far as I can tell, O_NONBLOCK support was  
> added just for rpc.mountd, but three of these four paths probably do  
> not create non-blocking listener sockets.  What does rpc.statd need?
> 

Not sure on this. My gut feeling says that blocking sockets are OK
for statd given that it's single threaded (right?), but I've not walked
through the code carefully enough to be sure.

> I'm considering creating a version of rpc_init() under utils/statd  
> that strips out all of the unnecessary features, uses note() instead  
> of xlot(), and then adds support for AF_INET6.
> 

Is there some reason to not just convert statd to use xlog()? It
seems like that would be a better long term solution.

For a proof-of-concept sort of thing, that might be reasonable though.

-- 
Jeff Layton <jlayton@redhat.com>

Re: nfs-utils: support/nfs/rpcmisc.c

[Chuck Lever]

On Jan 28, 2009, at 11:37 AM, Jeff Layton wrote:
> On Tue, 27 Jan 2009 15:20:02 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
>> I'm considering creating a version of rpc_init() under utils/statd
>> that strips out all of the unnecessary features, uses note() instead
>> of xlot(), and then adds support for AF_INET6.
>>
>
> Is there some reason to not just convert statd to use xlog()? It
> seems like that would be a better long term solution.

I'm not sure what the advantage of note() vs. xlog() is.

Be it also noted that sm-notify uses yet another logging mechanism,  
and I've already coded up patches to move it over to using note().   
This is because I'm adding some IPv6-enabled RPC handlers that are  
shared with statd...

And, the RPC libraries also write directly to stderr.  All of those  
error messages are lost in the daemon case, but when using -Fd they  
are useful, and appear on the controlling terminal.

> For a proof-of-concept sort of thing, that might be reasonable though.


It could be an additional clean-up once I'm done with the IPv6  
support.  But I need to look at both implementations to see if one is  
more appropriate for all of nfs-utils.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com

Re: nfs-utils: support/nfs/rpcmisc.c

[Jeff Layton]

On Wed, 28 Jan 2009 13:05:39 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> On Jan 28, 2009, at 11:37 AM, Jeff Layton wrote:
> > On Tue, 27 Jan 2009 15:20:02 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> >> I'm considering creating a version of rpc_init() under utils/statd
> >> that strips out all of the unnecessary features, uses note() instead
> >> of xlot(), and then adds support for AF_INET6.
> >>
> >
> > Is there some reason to not just convert statd to use xlog()? It
> > seems like that would be a better long term solution.
> 
> I'm not sure what the advantage of note() vs. xlog() is.
> 
> Be it also noted that sm-notify uses yet another logging mechanism,  
> and I've already coded up patches to move it over to using note().   
> This is because I'm adding some IPv6-enabled RPC handlers that are  
> shared with statd...
> 
> And, the RPC libraries also write directly to stderr.  All of those  
> error messages are lost in the daemon case, but when using -Fd they  
> are useful, and appear on the controlling terminal.
> 
> > For a proof-of-concept sort of thing, that might be reasonable though.
> 
> 
> It could be an additional clean-up once I'm done with the IPv6  
> support.  But I need to look at both implementations to see if one is  
> more appropriate for all of nfs-utils.
> 

FWIW, I don't have any particular attachment to either logging scheme.
It just seems like we could benefit from some standardization in this
area.

Not a high-priority item, but something to keep in mind if you poking
around in the code anyway.

-- 
Jeff Layton <jlayton@redhat.com>

Re: nfs-utils: support/nfs/rpcmisc.c

[Chuck Lever]

On Jan 28, 2009, at 1:17 PM, Jeff Layton wrote:
> On Wed, 28 Jan 2009 13:05:39 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
>
>> On Jan 28, 2009, at 11:37 AM, Jeff Layton wrote:
>>> On Tue, 27 Jan 2009 15:20:02 -0500
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>>> I'm considering creating a version of rpc_init() under utils/statd
>>>> that strips out all of the unnecessary features, uses note()  
>>>> instead
>>>> of xlot(), and then adds support for AF_INET6.
>>>>
>>>
>>> Is there some reason to not just convert statd to use xlog()? It
>>> seems like that would be a better long term solution.
>>
>> I'm not sure what the advantage of note() vs. xlog() is.
>>
>> Be it also noted that sm-notify uses yet another logging mechanism,
>> and I've already coded up patches to move it over to using note().
>> This is because I'm adding some IPv6-enabled RPC handlers that are
>> shared with statd...
>>
>> And, the RPC libraries also write directly to stderr.  All of those
>> error messages are lost in the daemon case, but when using -Fd they
>> are useful, and appear on the controlling terminal.
>>
>>> For a proof-of-concept sort of thing, that might be reasonable  
>>> though.
>>
>>
>> It could be an additional clean-up once I'm done with the IPv6
>> support.  But I need to look at both implementations to see if one is
>> more appropriate for all of nfs-utils.
>>
>
> FWIW, I don't have any particular attachment to either logging scheme.
> It just seems like we could benefit from some standardization in this
> area.
>
> Not a high-priority item, but something to keep in mind if you poking
> around in the code anyway.

The real benefit would come when we have to do i18n.

/me runs.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com