[PATCH 0/2] showmount bug fixes

[PATCH 0/2] showmount bug fixes

[Chuck Lever]

Hi Steve-

As promised, a couple of bug fixes for the recent improvements to the
showmount command.

---

Chuck Lever (2):
      showmount: Use CLNT_CONTROL for version fallback
      showmount: Eliminate compiler warnings


 utils/showmount/showmount.c |   14 ++++++++++----
 1 files changed, 10 insertions(+), 4 deletions(-)

-- 
Chuck Lever <chuck.lever@oracle.com>

[PATCH 1/2] showmount: Eliminate compiler warnings

[Chuck Lever]

Introduced by "showmount: Try the highest mount version then fall back
to lower ones", seen with "-Wextra".

showmount.c: In function =E2=80=98main=E2=80=99:
showmount.c:210: warning: comparison between signed and unsigned intege=
r expressions
showmount.c:249: warning: comparison between signed and unsigned intege=
r expressions

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/showmount/showmount.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/utils/showmount/showmount.c b/utils/showmount/showmount.c
index 74cf116..652aa85 100644
--- a/utils/showmount/showmount.c
+++ b/utils/showmount/showmount.c
@@ -90,7 +90,7 @@ static const rpcvers_t mount_vers_tbl[] =3D {
 	MOUNTVERS_POSIX,
 	MOUNTVERS,
 };
-static const int max_vers_tblsz =3D=20
+static const unsigned int max_vers_tblsz =3D=20
 	(sizeof(mount_vers_tbl)/sizeof(mount_vers_tbl[0]));
=20
 /*

[PATCH 2/2] showmount: Use CLNT_CONTROL for version fallback

[Chuck Lever]

When retrying MNT requests with lower RPC protocol versions, the
showmount command leaves several TCP sockets in TIME_WAIT:  at least
one socket for each rpcbind GETPORT call, and one for each attempted
MNT request.  Note that TIME_WAIT sockets may not be a problem on
an individual client, but this does tie up potentially valuable
network resources on the server for up to 120 seconds.

To reduce the resources required when retrying, downshift the RPC
protocol version of the existing RPC client using CLNT_CONTROL(),
rather than creating a new RPC client for each RPC protocol version.

Without this patch, "showmount -e" targeting a TCP-capable server with
a single export takes 64 packets and leaves 6 sockets in TIME_WAIT if
the server supports only MNTv1.  With it, showmount takes 29 packets
and leaves two TIME_WAIT sockets.

Newer versions of libtirpc will reduce that by one more socket (on
IPv4), and a handful of packets.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---

 utils/showmount/showmount.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/utils/showmount/showmount.c b/utils/showmount/showmount.c
index 652aa85..343f94b 100644
--- a/utils/showmount/showmount.c
+++ b/utils/showmount/showmount.c
@@ -193,12 +193,12 @@ int main(int argc, char **argv)
 		break;
 	}
 
-again:
 	mclient = nfs_get_mount_client(hostname, mount_vers_tbl[vers]);
 	mclient->cl_auth = authunix_create_default();
 	total_timeout.tv_sec = TOTAL_TIMEOUT;
 	total_timeout.tv_usec = 0;
 
+again:
 	if (eflag) {
 		memset(&exportlist, '\0', sizeof(exportlist));
 
@@ -207,8 +207,11 @@ again:
 			(xdrproc_t) xdr_exports, (caddr_t) &exportlist,
 			total_timeout);
 		if (clnt_stat == RPC_PROGVERSMISMATCH) {
-			if (++vers <  max_vers_tblsz)
+			if (++vers < max_vers_tblsz) {
+				(void)CLNT_CONTROL(mclient, CLSET_VERS,
+						(void *)&mount_vers_tbl[vers]);
 				goto again;
+			}
 		}
 		if (clnt_stat != RPC_SUCCESS) {
 			clnt_perror(mclient, "rpc mount export");
@@ -246,8 +249,11 @@ again:
 		(xdrproc_t) xdr_mountlist, (caddr_t) &dumplist,
 		total_timeout);
 	if (clnt_stat == RPC_PROGVERSMISMATCH) {
-		if (++vers <  max_vers_tblsz)
+		if (++vers < max_vers_tblsz) {
+			(void)CLNT_CONTROL(mclient, CLSET_VERS,
+					(void *)&mount_vers_tbl[vers]);
 			goto again;
+		}
 	}
 	if (clnt_stat != RPC_SUCCESS) {
 		clnt_perror(mclient, "rpc mount dump");

Re: [PATCH 0/2] showmount bug fixes

[Steve Dickson]

On 01/07/2010 11:26 AM, Chuck Lever wrote:
> Hi Steve-
> 
> As promised, a couple of bug fixes for the recent improvements to the
> showmount command.
> 
> ---
> 
> Chuck Lever (2):
>       showmount: Use CLNT_CONTROL for version fallback
Why is calling CLNT_CONTROL() verses calling clnt_create again
so much better?? In between failures the server status could 
change so why not just ping it again?? 

steved.

Re: [PATCH 0/2] showmount bug fixes

[Chuck Lever]

On Jan 7, 2010, at 12:52 PM, Steve Dickson wrote:

>
>
> On 01/07/2010 11:26 AM, Chuck Lever wrote:
>> Hi Steve-
>>
>> As promised, a couple of bug fixes for the recent improvements to the
>> showmount command.
>>
>> ---
>>
>> Chuck Lever (2):
>>      showmount: Use CLNT_CONTROL for version fallback
> Why is calling CLNT_CONTROL() verses calling clnt_create again
> so much better?? In between failures the server status could
> change so why not just ping it again??
>

The clnt_create(3) library call is quite heavyweight.  It does a DNS  
lookup via its own transport socket, a GETPORT via its own transport  
socket, then creates and connects a socket to do the application's RPC  
work.

Each time showmount calls clnt_create(3), the library will set up a  
socket for the DNS lookup (usually but not always UDP), set up a  
socket to do a GETPORT, and then set up a third socket for the  
MNTPROC_EXPORT request.  That's three sockets, each time through your  
retry loop.

When contacting a MNTv1-only server over TCP, the showmount command  
can leave up to six TCP sockets in TIME_WAIT (not counting sockets  
used for DNS) for 120 seconds after the command exits, if it doesn't  
use CLNT_CONTROL.

In the TCP case, CLNT_CONTROL leaves the application's transport  
socket connected.  This means showmount doesn't perform any additional  
DNS or GETPORT requests (it does just one of each), and then one  
transport socket is used for all the MNTPROC_EXPORT request retries.   
Thus, with CLNT_CONTROL, showmount uses one TCP socket for a single  
GETPORT, and one socket to do the MNTPROC_EXPORT requests.

So in every case, showmount will use just two transport sockets (not  
counting the DNS query).  It's behavior is no different than your  
original patch if the server supports MNTv3, but much more efficient  
for the MNTv1-only case.

Leaving multiple sockets in TIME_WAIT is a regression from the  
original showmount behavior, hence this bug fix.  If a customer  
reported this problem, you wouldn't hesitate to apply this fix.  I'm  
not sure why you find this controversial.

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

Re: [PATCH 0/2] showmount bug fixes

[Steve Dickson]

On 01/07/2010 01:38 PM, Chuck Lever wrote:
> 
> On Jan 7, 2010, at 12:52 PM, Steve Dickson wrote:
> 
>>
>>
>> On 01/07/2010 11:26 AM, Chuck Lever wrote:
>>> Hi Steve-
>>>
>>> As promised, a couple of bug fixes for the recent improvements to the
>>> showmount command.
>>>
>>> ---
>>>
>>> Chuck Lever (2):
>>>      showmount: Use CLNT_CONTROL for version fallback
>> Why is calling CLNT_CONTROL() verses calling clnt_create again
>> so much better?? In between failures the server status could
>> change so why not just ping it again??
>>
> 
> The clnt_create(3) library call is quite heavyweight.  It does a DNS
> lookup via its own transport socket, a GETPORT via its own transport
> socket, then creates and connects a socket to do the application's RPC
> work.
> 
> Each time showmount calls clnt_create(3), the library will set up a
> socket for the DNS lookup (usually but not always UDP), set up a socket
> to do a GETPORT, and then set up a third socket for the MNTPROC_EXPORT
> request.  That's three sockets, each time through your retry loop.
> 
> When contacting a MNTv1-only server over TCP, the showmount command can
> leave up to six TCP sockets in TIME_WAIT (not counting sockets used for
> DNS) for 120 seconds after the command exits, if it doesn't use
> CLNT_CONTROL.
Now really... who do you know that runs an v2 only server... 
I would call that badly misconfigured server... and I just
don't think it happens...because if it did, the bug would have
become much more prevalent a long time ago...  

Remember the goal of this patch is to deal with v3 only servers...
plus I kinda like the idea having applications re-querying the
server since configuration do indeed change... 

steved.
 

> 
> In the TCP case, CLNT_CONTROL leaves the application's transport socket
> connected.  This means showmount doesn't perform any additional DNS or
> GETPORT requests (it does just one of each), and then one transport
> socket is used for all the MNTPROC_EXPORT request retries.  Thus, with
> CLNT_CONTROL, showmount uses one TCP socket for a single GETPORT, and
> one socket to do the MNTPROC_EXPORT requests.
> 
> So in every case, showmount will use just two transport sockets (not
> counting the DNS query).  It's behavior is no different than your
> original patch if the server supports MNTv3, but much more efficient for
> the MNTv1-only case.
> 
> Leaving multiple sockets in TIME_WAIT is a regression from the original
> showmount behavior, hence this bug fix.  If a customer reported this
> problem, you wouldn't hesitate to apply this fix.  I'm not sure why you
> find this controversial.
> 
> -- 
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
> 
> 
> 

Re: [PATCH 0/2] showmount bug fixes

[Chuck Lever]

On Jan 7, 2010, at 3:25 PM, Steve Dickson wrote:
> On 01/07/2010 01:38 PM, Chuck Lever wrote:
>> On Jan 7, 2010, at 12:52 PM, Steve Dickson wrote:
>>> On 01/07/2010 11:26 AM, Chuck Lever wrote:
>>>> Hi Steve-
>>>>
>>>> As promised, a couple of bug fixes for the recent improvements to  
>>>> the
>>>> showmount command.
>>>>
>>>> ---
>>>>
>>>> Chuck Lever (2):
>>>>     showmount: Use CLNT_CONTROL for version fallback
>>> Why is calling CLNT_CONTROL() verses calling clnt_create again
>>> so much better?? In between failures the server status could
>>> change so why not just ping it again??
>>>
>>
>> The clnt_create(3) library call is quite heavyweight.  It does a DNS
>> lookup via its own transport socket, a GETPORT via its own transport
>> socket, then creates and connects a socket to do the application's  
>> RPC
>> work.
>>
>> Each time showmount calls clnt_create(3), the library will set up a
>> socket for the DNS lookup (usually but not always UDP), set up a  
>> socket
>> to do a GETPORT, and then set up a third socket for the  
>> MNTPROC_EXPORT
>> request.  That's three sockets, each time through your retry loop.
>>
>> When contacting a MNTv1-only server over TCP, the showmount command  
>> can
>> leave up to six TCP sockets in TIME_WAIT (not counting sockets used  
>> for
>> DNS) for 120 seconds after the command exits, if it doesn't use
>> CLNT_CONTROL.
> Now really... who do you know that runs an v2 only server...
> I would call that badly misconfigured server... and I just
> don't think it happens...because if it did, the bug would have
> become much more prevalent a long time ago...

If you think v2-only servers are so rare, why even bother downshifting  
the protocol version at all?  The original showmount command does not  
have any problem connecting to a v2-only server, since it used only  
MNTv1 to do the EXPORT request.

Most legacy servers are NFSv2 only.  I bought a new-in-box ReadyNAS a  
year or so ago that ran Linux 2.4, and it's NFS server was, in fact,  
v2-only.

/me gives up.

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

Re: [PATCH 0/2] showmount bug fixes

[Steve Dickson]

On 01/07/2010 05:43 PM, Chuck Lever wrote:
> On Jan 7, 2010, at 3:25 PM, Steve Dickson wrote:
> If you think v2-only servers are so rare, why even bother downshifting
> the protocol version at all?  
I did think of this... since a point of this exercise is to begin
the end of v2 support... but since we have a habit of being backwards 
compatible (as well as being portable), I figure downshifting was the 
right thing to do...  

 
> The original showmount command does not
> have any problem connecting to a v2-only server, since it used only
> MNTv1 to do the EXPORT request.
True... The problem is showmount does not handle v3 only servers
which is another point of this discussion... 

> 
> Most legacy servers are NFSv2 only.  I bought a new-in-box ReadyNAS a
> year or so ago that ran Linux 2.4, and it's NFS server was, in fact,
> v2-only.
Really.. that is surprising... how well did it perform and could it 
negotiate up to v3?? 

steved.

Re: [PATCH 0/2] showmount bug fixes

[Steve Dickson]

On 01/07/2010 11:26 AM, Chuck Lever wrote:
> Hi Steve-
> 
> As promised, a couple of bug fixes for the recent improvements to the
> showmount command.
> 
> ---
> 
> Chuck Lever (2):
>       showmount: Use CLNT_CONTROL for version fallback
>       showmount: Eliminate compiler warnings
> 
Committed...

steved.