Different sequence of "exportfs" produce different effects on nfs client mounts

Commit Message

NeilBrown Oct. 20, 2013, 10:49 p.m. UTC

On Fri, 18 Oct 2013 08:32:53 +1100 NeilBrown <[e-mail]> wrote:

> On Thu, 17 Oct 2013 09:14:02 -0400 Chuck Lever <[e-mail]> wrote:
> 
> > Hi-
> > 
> > 281         if (ident[0] == '\0')
> > 282                 return MCL_ANONYMOUS;
> > 283         if (ident[0] == '@') {
> > 284 #ifndef HAVE_INNETGR
> > 285                 xlog(L_WARNING, "netgroup support not compiled in");
> > 286 #endif
> > 287                 return MCL_NETGROUP;
> > 288         }
> > 289         for (sp = ident; *sp; sp++) {
> > 290                 if (*sp == '*' || *sp == '?' || *sp == '[')
> > 291                         return MCL_WILDCARD;
> > 292                 if (*sp == '/')
> > 293                         return MCL_SUBNETWORK;
> > 294                 if (*sp == '\\' && sp[1])
> > 295                         sp++;
> > 296         }
> > 
> > is still in play today.  The "host_pton()" code you posted was added by commit 502edf1d just after this paragraph.  But here is what that commit replaced.
> > 
> > -       /* check for N.N.N.N */
> > -       sp = ident;
> > -       if(!isdigit(*sp) || strtoul(sp, &sp, 10) > 255 || *sp != '.') return MCL_FQDN;
> > -       sp++; if(!isdigit(*sp) || strtoul(sp, &sp, 10) > 255 || *sp != '.') return MCL_F
> > -       sp++; if(!isdigit(*sp) || strtoul(sp, &sp, 10) > 255 || *sp != '.') return MCL_F
> > -       sp++; if(!isdigit(*sp) || strtoul(sp, &sp, 10) > 255 || *sp != '\0') return MCL_
> > -       /* we lie here a bit. but technically N.N.N.N == N.N.N.N/32 :) */
> > -       return MCL_SUBNETWORK;
> > +
> > +       /*
> > +        * Treat unadorned IP addresses as MCL_SUBNETWORK.
> > +        * Everything else is MCL_FQDN.
> > +        */
> > +       ai = host_pton(ident);
> > +       if (ai != NULL) {
> > +               freeaddrinfo(ai);
> > +               return MCL_SUBNETWORK;
> > +       }
> > +
> > +       return MCL_FQDN;
> >  }
> > 
> > The replaced logic also treats IP addresses as MCL_SUBNETWORK.  That's from commit 54669c98 in 2005.  Neil, do you remember why this semantic change was made?
> > 
> 
> See this thread:
> 
>   http://marc.info/?t=110993941000003&r=1&w=2
> 
> It was a simple (though possibly flawed) solution to clearly differentiate
> those addresses where DNS looked might be needed, and those where it was not.
> 
> I may have more to say later but I have to rush off now, so thought I'd just
> post this anyway.
> 

Unfortunately I cannot see how that change ever made any important
difference, and the email exchanges ends without resolving anything.

It could only make a difference to the number of DNS lookups if there was
somewhere a test for whether clientlist[MCL_FQDN] was NULL, but there isn't
and never was.

So it seems very likely that:


is appropriate and may well fix the current issue.

It would be good to test how many DNS looks (hopefully none) are performed
when using a exports file that contains only IP addresses, both before and
after the patch.

NeilBrown

Comments

Wangminlan Oct. 21, 2013, 9:47 a.m. UTC | #1

I applied this patch to nfs-utils-1.2.6, it fixed the current issue.

My /etc/exports looks like this:
/media/fs1/     10.158.0.0/16(rw,root_squash,no_subtree_check)
/media/fs1/     10.158.192.71(rw,no_root_squash,no_subtree_check)
/media/fs1/     10.158.192.72(rw,no_root_squash,no_subtree_check)
/media/fs1/     10.158.192.73(rw,no_root_squash,no_subtree_check)
/media/fs1/     10.158.192.74(rw,no_root_squash,no_subtree_check)

And I also captured dns lookup traffic by command "tcpdump -i eth0 -vvv host dns_server" during the following procedure:
#exportfs -ua
#exportfs -a
on the server, and the nfs mount procedure on the client.
There's none dns lookup related traffic, neither before the patch, nor after it.

Thanks to all of you, for helping me fix this issue, and the time you spent on reviewing the code.

B.R.
Minlan Wang

> -----Original Message-----

> From: NeilBrown [mailto:[e-mail]]

> Sent: Monday, October 21, 2013 6:49 AM

> To: NeilBrown

> Cc: Chuck Lever; Wangminlan; J. Bruce Fields; [e-mail]

> Subject: Re: Different sequence of "exportfs" produce different effects on nfs

> client mounts

> 

> 

> Unfortunately I cannot see how that change ever made any important

> difference, and the email exchanges ends without resolving anything.

> 

> It could only make a difference to the number of DNS lookups if there was

> somewhere a test for whether clientlist[MCL_FQDN] was NULL, but there isn't

> and never was.

> 

> So it seems very likely that:

> 

> diff --git a/support/export/client.c b/support/export/client.c index

> ba2db8f..adbeed8 100644

> --- a/support/export/client.c

> +++ b/support/export/client.c

> @@ -767,15 +767,5 @@ client_gettype(char *ident)

>  			sp++;

>  	}

> 

> -	/*

> -	 * Treat unadorned IP addresses as MCL_SUBNETWORK.

> -	 * Everything else is MCL_FQDN.

> -	 */

> -	ai = host_pton(ident);

> -	if (ai != NULL) {

> -		freeaddrinfo(ai);

> -		return MCL_SUBNETWORK;

> -	}

> -

>  	return MCL_FQDN;

>  }

> 

> is appropriate and may well fix the current issue.

> 

> It would be good to test how many DNS looks (hopefully none) are performed

> when using a exports file that contains only IP addresses, both before and after

> the patch.

> 

> NeilBrown

Patch

diff --git a/support/export/client.c b/support/export/client.c
index ba2db8f..adbeed8 100644
--- a/support/export/client.c
+++ b/support/export/client.c
@@ -767,15 +767,5 @@  client_gettype(char *ident)
 			sp++;
 	}
 
-	/*
-	 * Treat unadorned IP addresses as MCL_SUBNETWORK.
-	 * Everything else is MCL_FQDN.
-	 */
-	ai = host_pton(ident);
-	if (ai != NULL) {
-		freeaddrinfo(ai);
-		return MCL_SUBNETWORK;
-	}
-
 	return MCL_FQDN;
 }