[PATCH 0/5] IPv6-related clean-ups

[PATCH 0/5] IPv6-related clean-ups

[Chuck Lever]

Hi Bruce, Trond -

This series introduces support for IPv6 in the NFSv4 callback server, and
cleans up parts of NLM before we start to introduce IPv6 support there.

Please consider these for 2.6.28.

(Whoops, looks like one more is coming in this series.  Expect a sixth
shortly).

---

Chuck Lever (5):
      lockd: Add address family-agnostic helper for zeroing the port number
      lockd: Specify address family for source address
      lockd: address-family independent printable addresses
      NLM: Clean up before introducing new debugging messages
      NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets


 fs/lockd/host.c             |  118 ++++++++++++++++++++++++++++++++-----------
 fs/nfs/callback.c           |   11 ++--
 fs/nfs/callback.h           |    4 +
 fs/nfs/client.c             |    2 -
 include/linux/lockd/lockd.h |    4 +
 5 files changed, 101 insertions(+), 38 deletions(-)

-- 
Chuck Lever

[PATCH 1/5] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[Chuck Lever]

Allow the NFS callback server to listen for requests via an AF_INET6 or
AF_INET socket.

The NFS callback server determines the listener's address family from the
address the client uses to contact the server.  The server will always
call the client back using the same address family.

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

 fs/nfs/callback.c |   11 ++++++-----
 fs/nfs/callback.h |    4 ++--
 fs/nfs/client.c   |    2 +-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 6a09760..59948ef 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -97,7 +97,7 @@ nfs_callback_svc(void *vrqstp)
 /*
  * Bring up the callback thread if it is not already up.
  */
-int nfs_callback_up(void)
+int nfs_callback_up(const sa_family_t family)
 {
 	struct svc_serv *serv = NULL;
 	int ret = 0;
@@ -106,7 +106,7 @@ int nfs_callback_up(void)
 	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
 		goto out;
 	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
-				AF_INET, NULL);
+				family, NULL);
 	ret = -ENOMEM;
 	if (!serv)
 		goto out_err;
@@ -116,7 +116,8 @@ int nfs_callback_up(void)
 	if (ret <= 0)
 		goto out_err;
 	nfs_callback_tcpport = ret;
-	dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
+	dprintk("NFS: Callback listener port = %u (af %u)\n",
+			nfs_callback_tcpport, family);
 
 	nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
 	if (IS_ERR(nfs_callback_info.rqst)) {
@@ -149,8 +150,8 @@ out:
 	mutex_unlock(&nfs_callback_mutex);
 	return ret;
 out_err:
-	dprintk("Couldn't create callback socket or server thread; err = %d\n",
-		ret);
+	dprintk("NFS: Couldn't create callback socket or server thread; "
+		"err = %d\n", ret);
 	nfs_callback_info.users--;
 	goto out;
 }
diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
index bb25d21..85d8102 100644
--- a/fs/nfs/callback.h
+++ b/fs/nfs/callback.h
@@ -63,10 +63,10 @@ extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getat
 extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
 
 #ifdef CONFIG_NFS_V4
-extern int nfs_callback_up(void);
+extern int nfs_callback_up(const sa_family_t family);
 extern void nfs_callback_down(void);
 #else
-#define nfs_callback_up()	(0)
+#define nfs_callback_up(x)	(0)
 #define nfs_callback_down()	do {} while(0)
 #endif
 
diff --git a/fs/nfs/client.c b/fs/nfs/client.c
index fc6a95d..5f8889f 100644
--- a/fs/nfs/client.c
+++ b/fs/nfs/client.c
@@ -120,7 +120,7 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
 	clp->rpc_ops = cl_init->rpc_ops;
 
 	if (cl_init->rpc_ops->version == 4) {
-		if (nfs_callback_up() < 0)
+		if (nfs_callback_up(cl_init->addr->sa_family) < 0)
 			goto error_2;
 		__set_bit(NFS_CS_CALLBACK, &clp->cl_res_state);
 	}

[PATCH 2/5] NLM: Clean up before introducing new debugging messages

[Chuck Lever]

We're about to introduce some extra debugging messages in nlm_lookup_host().
Bring the coding style up to date first so we can cleanly introduce the new
debugging messages.

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

 fs/lockd/host.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index a17664c..cb26e3d 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -103,16 +103,19 @@ static struct nlm_host *nlm_lookup_host(int server,
 		nlm_get_host(host);
 		goto out;
 	}
-	if (nsm)
-		atomic_inc(&nsm->sm_count);
-
-	host = NULL;
 
-	/* Sadly, the host isn't in our hash table yet. See if
-	 * we have an NSM handle for it. If not, create one.
+	/*
+	 * The host wasn't in our hash table.  If we don't
+	 * have an NSM handle for it yet, create one.
 	 */
-	if (!nsm && !(nsm = nsm_find(sin, hostname, hostname_len)))
-		goto out;
+	if (nsm)
+		atomic_inc(&nsm->sm_count);
+	else {
+		host = NULL;
+		nsm = nsm_find(sin, hostname, hostname_len);
+		if (!nsm)
+			goto out;
+	}
 
 	host = kzalloc(sizeof(*host), GFP_KERNEL);
 	if (!host) {

[PATCH 3/5] lockd: address-family independent printable addresses

[Chuck Lever]

Knowing which source address is used for communicating with remote NLM
services can be helpful for debugging configuration problems on hosts
with multiple addresses.

Keep the dprintk debugging here, but adapt it so it displays AF_INET6
addresses properly.  There are also a couple of dprintk clean-ups as
well.

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

 fs/lockd/host.c             |   73 +++++++++++++++++++++++++++++++++----------
 include/linux/lockd/lockd.h |    4 ++
 2 files changed, 60 insertions(+), 17 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index cb26e3d..2d42eed 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <linux/slab.h>
 #include <linux/in.h>
+#include <linux/in6.h>
 #include <linux/sunrpc/clnt.h>
 #include <linux/sunrpc/svc.h>
 #include <linux/lockd/lockd.h>
@@ -38,6 +39,28 @@ static struct nsm_handle *	nsm_find(const struct sockaddr_in *sin,
 					 const char *hostname,
 					 unsigned int hostname_len);
 
+static void nlm_display_address(const struct sockaddr *sap,
+				char *buf, const size_t len)
+{
+	const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
+	const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
+
+	switch (sap->sa_family) {
+	case AF_UNSPEC:
+		snprintf(buf, len, "unspecified");
+		break;
+	case AF_INET:
+		snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
+		break;
+	case AF_INET6:
+		snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
+		break;
+	default:
+		snprintf(buf, len, "unsupported address family");
+		break;
+	}
+}
+
 /*
  * Common host lookup routine for server & client
  */
@@ -54,14 +77,10 @@ static struct nlm_host *nlm_lookup_host(int server,
 	struct nsm_handle *nsm = NULL;
 	int		hash;
 
-	dprintk("lockd: nlm_lookup_host("NIPQUAD_FMT"->"NIPQUAD_FMT
-			", p=%d, v=%u, my role=%s, name=%.*s)\n",
-			NIPQUAD(ssin->sin_addr.s_addr),
-			NIPQUAD(sin->sin_addr.s_addr), proto, version,
-			server? "server" : "client",
-			hostname_len,
-			hostname? hostname : "<none>");
-
+	dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
+			" my role is %s, hostname=%.*s)\n",
+			proto, version, server ? "server" : "client",
+			hostname_len, hostname ? hostname : "<none>");
 
 	hash = NLM_ADDRHASH(sin->sin_addr.s_addr);
 
@@ -101,6 +120,8 @@ static struct nlm_host *nlm_lookup_host(int server,
 		hlist_add_head(&host->h_hash, chain);
 
 		nlm_get_host(host);
+		dprintk("lockd: nlm_lookup_host found host %s (%s)\n",
+				host->h_name, host->h_addrbuf);
 		goto out;
 	}
 
@@ -113,13 +134,17 @@ static struct nlm_host *nlm_lookup_host(int server,
 	else {
 		host = NULL;
 		nsm = nsm_find(sin, hostname, hostname_len);
-		if (!nsm)
+		if (!nsm) {
+			dprintk("lockd: nlm_lookup_host failed; "
+				"no nsm handle\n");
 			goto out;
+		}
 	}
 
 	host = kzalloc(sizeof(*host), GFP_KERNEL);
 	if (!host) {
 		nsm_release(nsm);
+		dprintk("lockd: nlm_lookup_host failed; no memory\n");
 		goto out;
 	}
 	host->h_name	   = nsm->sm_name;
@@ -146,6 +171,15 @@ static struct nlm_host *nlm_lookup_host(int server,
 	INIT_LIST_HEAD(&host->h_reclaim);
 
 	nrhosts++;
+
+	nlm_display_address((struct sockaddr *)&host->h_addr,
+				host->h_addrbuf, sizeof(host->h_addrbuf));
+	nlm_display_address((struct sockaddr *)&host->h_saddr,
+				host->h_saddrbuf, sizeof(host->h_saddrbuf));
+
+	dprintk("lockd: nlm_lookup_host created host %s\n",
+			host->h_name);
+
 out:
 	mutex_unlock(&nlm_host_mutex);
 	return host;
@@ -210,9 +244,8 @@ nlm_bind_host(struct nlm_host *host)
 {
 	struct rpc_clnt	*clnt;
 
-	dprintk("lockd: nlm_bind_host("NIPQUAD_FMT"->"NIPQUAD_FMT")\n",
-			NIPQUAD(host->h_saddr.sin_addr),
-			NIPQUAD(host->h_addr.sin_addr));
+	dprintk("lockd: nlm_bind_host %s (%s), my addr=%s\n",
+			host->h_name, host->h_addrbuf, host->h_saddrbuf);
 
 	/* Lock host handle */
 	mutex_lock(&host->h_mutex);
@@ -224,7 +257,7 @@ nlm_bind_host(struct nlm_host *host)
 		if (time_after_eq(jiffies, host->h_nextrebind)) {
 			rpc_force_rebind(clnt);
 			host->h_nextrebind = jiffies + NLM_HOST_REBIND;
-			dprintk("lockd: next rebind in %ld jiffies\n",
+			dprintk("lockd: next rebind in %lu jiffies\n",
 					host->h_nextrebind - jiffies);
 		}
 	} else {
@@ -327,12 +360,16 @@ void nlm_host_rebooted(const struct sockaddr_in *sin,
 	struct nsm_handle *nsm;
 	struct nlm_host	*host;
 
-	dprintk("lockd: nlm_host_rebooted(%s, %u.%u.%u.%u)\n",
-			hostname, NIPQUAD(sin->sin_addr));
-
 	/* Find the NSM handle for this peer */
-	if (!(nsm = __nsm_find(sin, hostname, hostname_len, 0)))
+	nsm = __nsm_find(sin, hostname, hostname_len, 0);
+	if (nsm == NULL) {
+		dprintk("lockd: never saw rebooted peer '%.*s' before\n",
+				hostname_len, hostname);
 		return;
+	}
+
+	dprintk("lockd: nlm_host_rebooted(%.*s, %s)\n",
+			hostname_len, hostname, nsm->sm_addrbuf);
 
 	/* When reclaiming locks on this peer, make sure that
 	 * we set up a new notification */
@@ -516,6 +553,8 @@ retry:
 	nsm->sm_name = (char *) (nsm + 1);
 	memcpy(nsm->sm_name, hostname, hostname_len);
 	nsm->sm_name[hostname_len] = '\0';
+	nlm_display_address((struct sockaddr *)&nsm->sm_addr,
+				nsm->sm_addrbuf, sizeof(nsm->sm_addrbuf));
 	atomic_set(&nsm->sm_count, 1);
 	goto retry;
 
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index dbb87ab..0691efb 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -61,6 +61,9 @@ struct nlm_host {
 	struct list_head	h_granted;	/* Locks in GRANTED state */
 	struct list_head	h_reclaim;	/* Locks in RECLAIM state */
 	struct nsm_handle *	h_nsmhandle;	/* NSM status handle */
+
+	char			h_addrbuf[48],	/* address eyecatchers */
+				h_saddrbuf[48];
 };
 
 struct nsm_handle {
@@ -70,6 +73,7 @@ struct nsm_handle {
 	struct sockaddr_in	sm_addr;
 	unsigned int		sm_monitored : 1,
 				sm_sticky : 1;	/* don't unmonitor */
+	char			sm_addrbuf[48];	/* address eyecatcher */
 };
 
 /*

[PATCH 4/5] lockd: Specify address family for source address

[Chuck Lever]

Make sure an address family is specified for source addresses passed to
nlm_lookup_host().  nlm_lookup_host() will need this when it becomes
capable of dealing with AF_INET6 addresses.

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

 fs/lockd/host.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 2d42eed..55ef751 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -215,10 +215,12 @@ struct nlm_host *nlmclnt_lookup_host(const struct sockaddr_in *sin,
 				     const char *hostname,
 				     unsigned int hostname_len)
 {
-	struct sockaddr_in ssin = {0};
+	const struct sockaddr_in source = {
+		.sin_family	= AF_UNSPEC,
+	};
 
 	return nlm_lookup_host(0, sin, proto, version,
-			       hostname, hostname_len, &ssin);
+			       hostname, hostname_len, &source);
 }
 
 /*
@@ -228,12 +230,14 @@ struct nlm_host *
 nlmsvc_lookup_host(struct svc_rqst *rqstp,
 			const char *hostname, unsigned int hostname_len)
 {
-	struct sockaddr_in ssin = {0};
+	const struct sockaddr_in source = {
+		.sin_family	= AF_INET,
+		.sin_addr	= rqstp->rq_daddr.addr,
+	};
 
-	ssin.sin_addr = rqstp->rq_daddr.addr;
 	return nlm_lookup_host(1, svc_addr_in(rqstp),
 			       rqstp->rq_prot, rqstp->rq_vers,
-			       hostname, hostname_len, &ssin);
+			       hostname, hostname_len, &source);
 }
 
 /*

[PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[Chuck Lever]

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

 fs/lockd/host.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/fs/lockd/host.c b/fs/lockd/host.c
index 55ef751..3f0c1a8 100644
--- a/fs/lockd/host.c
+++ b/fs/lockd/host.c
@@ -39,6 +39,18 @@ static struct nsm_handle *	nsm_find(const struct sockaddr_in *sin,
 					 const char *hostname,
 					 unsigned int hostname_len);
 
+static void nlm_clear_port(struct sockaddr *sap)
+{
+	switch (sap->sa_family) {
+	case AF_INET:
+		((struct sockaddr_in *)sap)->sin_port = 0;
+		break;
+	case AF_INET6:
+		((struct sockaddr_in6 *)sap)->sin6_port = 0;
+		break;
+	}
+}
+
 static void nlm_display_address(const struct sockaddr *sap,
 				char *buf, const size_t len)
 {
@@ -149,7 +161,7 @@ static struct nlm_host *nlm_lookup_host(int server,
 	}
 	host->h_name	   = nsm->sm_name;
 	host->h_addr       = *sin;
-	host->h_addr.sin_port = 0;	/* ouch! */
+	nlm_clear_port((struct sockaddr *)&host->h_addr);
 	host->h_saddr	   = *ssin;
 	host->h_version    = version;
 	host->h_proto      = proto;

Re: [PATCH 1/5] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[J. Bruce Fields]

On Fri, Aug 22, 2008 at 12:42:02PM -0400, Chuck Lever wrote:
> Allow the NFS callback server to listen for requests via an AF_INET6
> or AF_INET socket.
> 
> The NFS callback server determines the listener's address family from
> the address the client uses to contact the server.  The server will
> always call the client back using the same address family.

Won't the server determine that from the callback address which the
client provides in the setclientid?

--b.

> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/nfs/callback.c |   11 ++++++-----
>  fs/nfs/callback.h |    4 ++--
>  fs/nfs/client.c   |    2 +-
>  3 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
> index 6a09760..59948ef 100644
> --- a/fs/nfs/callback.c
> +++ b/fs/nfs/callback.c
> @@ -97,7 +97,7 @@ nfs_callback_svc(void *vrqstp)
>  /*
>   * Bring up the callback thread if it is not already up.
>   */
> -int nfs_callback_up(void)
> +int nfs_callback_up(const sa_family_t family)
>  {
>  	struct svc_serv *serv = NULL;
>  	int ret = 0;
> @@ -106,7 +106,7 @@ int nfs_callback_up(void)
>  	if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
>  		goto out;
>  	serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
> -				AF_INET, NULL);
> +				family, NULL);
>  	ret = -ENOMEM;
>  	if (!serv)
>  		goto out_err;
> @@ -116,7 +116,8 @@ int nfs_callback_up(void)
>  	if (ret <= 0)
>  		goto out_err;
>  	nfs_callback_tcpport = ret;
> -	dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
> +	dprintk("NFS: Callback listener port = %u (af %u)\n",
> +			nfs_callback_tcpport, family);
>  
>  	nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
>  	if (IS_ERR(nfs_callback_info.rqst)) {
> @@ -149,8 +150,8 @@ out:
>  	mutex_unlock(&nfs_callback_mutex);
>  	return ret;
>  out_err:
> -	dprintk("Couldn't create callback socket or server thread; err = %d\n",
> -		ret);
> +	dprintk("NFS: Couldn't create callback socket or server thread; "
> +		"err = %d\n", ret);
>  	nfs_callback_info.users--;
>  	goto out;
>  }
> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
> index bb25d21..85d8102 100644
> --- a/fs/nfs/callback.h
> +++ b/fs/nfs/callback.h
> @@ -63,10 +63,10 @@ extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getat
>  extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
>  
>  #ifdef CONFIG_NFS_V4
> -extern int nfs_callback_up(void);
> +extern int nfs_callback_up(const sa_family_t family);
>  extern void nfs_callback_down(void);
>  #else
> -#define nfs_callback_up()	(0)
> +#define nfs_callback_up(x)	(0)
>  #define nfs_callback_down()	do {} while(0)
>  #endif
>  
> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> index fc6a95d..5f8889f 100644
> --- a/fs/nfs/client.c
> +++ b/fs/nfs/client.c
> @@ -120,7 +120,7 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
>  	clp->rpc_ops = cl_init->rpc_ops;
>  
>  	if (cl_init->rpc_ops->version == 4) {
> -		if (nfs_callback_up() < 0)
> +		if (nfs_callback_up(cl_init->addr->sa_family) < 0)
>  			goto error_2;
>  		__set_bit(NFS_CS_CALLBACK, &clp->cl_res_state);
>  	}
> 

Re: [PATCH 1/5] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[Chuck Lever]

On Fri, Aug 22, 2008 at 7:18 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Fri, Aug 22, 2008 at 12:42:02PM -0400, Chuck Lever wrote:
>> Allow the NFS callback server to listen for requests via an AF_INET6
>> or AF_INET socket.
>>
>> The NFS callback server determines the listener's address family from
>> the address the client uses to contact the server.  The server will
>> always call the client back using the same address family.
>
> Won't the server determine that from the callback address which the
> client provides in the setclientid?

A client has to have IPv6 networking to mount an IPv6 server.
Otherwise both will use IPv4.  Do you think we should worry about the
case where a client provides a callback address in a different address
family from the server's address?

But I suppose it would be more precise to call nfs_callback_up() using
the family of the passed-in clientaddr= mount option instead of the
passed-in addr= option.  That will require discovering the address
family of the clientaddr string.

We could convert the clientaddr string into a sockaddr temporarily (we
don't already do that... it's converted into a universal address
string, but not into a sockaddr).

>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  fs/nfs/callback.c |   11 ++++++-----
>>  fs/nfs/callback.h |    4 ++--
>>  fs/nfs/client.c   |    2 +-
>>  3 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>> index 6a09760..59948ef 100644
>> --- a/fs/nfs/callback.c
>> +++ b/fs/nfs/callback.c
>> @@ -97,7 +97,7 @@ nfs_callback_svc(void *vrqstp)
>>  /*
>>   * Bring up the callback thread if it is not already up.
>>   */
>> -int nfs_callback_up(void)
>> +int nfs_callback_up(const sa_family_t family)
>>  {
>>       struct svc_serv *serv = NULL;
>>       int ret = 0;
>> @@ -106,7 +106,7 @@ int nfs_callback_up(void)
>>       if (nfs_callback_info.users++ || nfs_callback_info.task != NULL)
>>               goto out;
>>       serv = svc_create(&nfs4_callback_program, NFS4_CALLBACK_BUFSIZE,
>> -                             AF_INET, NULL);
>> +                             family, NULL);
>>       ret = -ENOMEM;
>>       if (!serv)
>>               goto out_err;
>> @@ -116,7 +116,8 @@ int nfs_callback_up(void)
>>       if (ret <= 0)
>>               goto out_err;
>>       nfs_callback_tcpport = ret;
>> -     dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
>> +     dprintk("NFS: Callback listener port = %u (af %u)\n",
>> +                     nfs_callback_tcpport, family);
>>
>>       nfs_callback_info.rqst = svc_prepare_thread(serv, &serv->sv_pools[0]);
>>       if (IS_ERR(nfs_callback_info.rqst)) {
>> @@ -149,8 +150,8 @@ out:
>>       mutex_unlock(&nfs_callback_mutex);
>>       return ret;
>>  out_err:
>> -     dprintk("Couldn't create callback socket or server thread; err = %d\n",
>> -             ret);
>> +     dprintk("NFS: Couldn't create callback socket or server thread; "
>> +             "err = %d\n", ret);
>>       nfs_callback_info.users--;
>>       goto out;
>>  }
>> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
>> index bb25d21..85d8102 100644
>> --- a/fs/nfs/callback.h
>> +++ b/fs/nfs/callback.h
>> @@ -63,10 +63,10 @@ extern __be32 nfs4_callback_getattr(struct cb_getattrargs *args, struct cb_getat
>>  extern __be32 nfs4_callback_recall(struct cb_recallargs *args, void *dummy);
>>
>>  #ifdef CONFIG_NFS_V4
>> -extern int nfs_callback_up(void);
>> +extern int nfs_callback_up(const sa_family_t family);
>>  extern void nfs_callback_down(void);
>>  #else
>> -#define nfs_callback_up()    (0)
>> +#define nfs_callback_up(x)   (0)
>>  #define nfs_callback_down()  do {} while(0)
>>  #endif
>>
>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>> index fc6a95d..5f8889f 100644
>> --- a/fs/nfs/client.c
>> +++ b/fs/nfs/client.c
>> @@ -120,7 +120,7 @@ static struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_
>>       clp->rpc_ops = cl_init->rpc_ops;
>>
>>       if (cl_init->rpc_ops->version == 4) {
>> -             if (nfs_callback_up() < 0)
>> +             if (nfs_callback_up(cl_init->addr->sa_family) < 0)
>>                       goto error_2;
>>               __set_bit(NFS_CS_CALLBACK, &clp->cl_res_state);
>>       }
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
"If you simplify your English, you are freed from the worst follies of
orthodoxy."
 -- George Orwell

Re: [PATCH 1/5] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[Chuck Lever]

On Aug 22, 2008, at Aug 22, 2008, 10:34 PM, Chuck Lever wrote:
> On Fri, Aug 22, 2008 at 7:18 PM, J. Bruce Fields  
> <bfields@fieldses.org> wrote:
>> On Fri, Aug 22, 2008 at 12:42:02PM -0400, Chuck Lever wrote:
>>> Allow the NFS callback server to listen for requests via an AF_INET6
>>> or AF_INET socket.
>>>
>>> The NFS callback server determines the listener's address family  
>>> from
>>> the address the client uses to contact the server.  The server will
>>> always call the client back using the same address family.
>>
>> Won't the server determine that from the callback address which the
>> client provides in the setclientid?
>
> A client has to have IPv6 networking to mount an IPv6 server.
> Otherwise both will use IPv4.  Do you think we should worry about the
> case where a client provides a callback address in a different address
> family from the server's address?
>
> But I suppose it would be more precise to call nfs_callback_up() using
> the family of the passed-in clientaddr= mount option instead of the
> passed-in addr= option.  That will require discovering the address
> family of the clientaddr string.
>
> We could convert the clientaddr string into a sockaddr temporarily (we
> don't already do that... it's converted into a universal address
> string, but not into a sockaddr).

I thought about this more over the weekend.

We've got a similar problem here that we have with starting just a TCP  
or UDP listener for lockd based on what transport protocols were  
requested on the mount command line.

The NFSv4 callback server (and lockd, and probably NFSD) should start  
either a single AF_INET or a single AF_INET6 listener, not both.   
Right now, the callback server's address family is selected based on  
mount options for the first mount request.  But we only want one of  
these listeners for all mount points on a system, and it should be one  
that covers all the needed address families, no matter which address  
family was used during the first mount request.

So these services need to start a listener not based on mount options,  
but on what kind of networking is available on the host.  If IPV6 is  
available in the kernel, I think, an AF_INET6 listener should be  
started; otherwise, start an AF_INET listener.

I think the only externally visible issue with doing this is how  
addresses are presented in kernel log and error messages.  If the  
functions which generate presentation format address strings are smart  
enough to recognize a mapped IPv4 address and present it in dotted- 
quad format, that should be enough.

Thoughts/opinions?

>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> fs/nfs/callback.c |   11 ++++++-----
>>> fs/nfs/callback.h |    4 ++--
>>> fs/nfs/client.c   |    2 +-
>>> 3 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
>>> index 6a09760..59948ef 100644
>>> --- a/fs/nfs/callback.c
>>> +++ b/fs/nfs/callback.c
>>> @@ -97,7 +97,7 @@ nfs_callback_svc(void *vrqstp)
>>> /*
>>>  * Bring up the callback thread if it is not already up.
>>>  */
>>> -int nfs_callback_up(void)
>>> +int nfs_callback_up(const sa_family_t family)
>>> {
>>>      struct svc_serv *serv = NULL;
>>>      int ret = 0;
>>> @@ -106,7 +106,7 @@ int nfs_callback_up(void)
>>>      if (nfs_callback_info.users++ || nfs_callback_info.task !=  
>>> NULL)
>>>              goto out;
>>>      serv = svc_create(&nfs4_callback_program,  
>>> NFS4_CALLBACK_BUFSIZE,
>>> -                             AF_INET, NULL);
>>> +                             family, NULL);
>>>      ret = -ENOMEM;
>>>      if (!serv)
>>>              goto out_err;
>>> @@ -116,7 +116,8 @@ int nfs_callback_up(void)
>>>      if (ret <= 0)
>>>              goto out_err;
>>>      nfs_callback_tcpport = ret;
>>> -     dprintk("Callback port = 0x%x\n", nfs_callback_tcpport);
>>> +     dprintk("NFS: Callback listener port = %u (af %u)\n",
>>> +                     nfs_callback_tcpport, family);
>>>
>>>      nfs_callback_info.rqst = svc_prepare_thread(serv, &serv- 
>>> >sv_pools[0]);
>>>      if (IS_ERR(nfs_callback_info.rqst)) {
>>> @@ -149,8 +150,8 @@ out:
>>>      mutex_unlock(&nfs_callback_mutex);
>>>      return ret;
>>> out_err:
>>> -     dprintk("Couldn't create callback socket or server thread;  
>>> err = %d\n",
>>> -             ret);
>>> +     dprintk("NFS: Couldn't create callback socket or server  
>>> thread; "
>>> +             "err = %d\n", ret);
>>>      nfs_callback_info.users--;
>>>      goto out;
>>> }
>>> diff --git a/fs/nfs/callback.h b/fs/nfs/callback.h
>>> index bb25d21..85d8102 100644
>>> --- a/fs/nfs/callback.h
>>> +++ b/fs/nfs/callback.h
>>> @@ -63,10 +63,10 @@ extern __be32 nfs4_callback_getattr(struct  
>>> cb_getattrargs *args, struct cb_getat
>>> extern __be32 nfs4_callback_recall(struct cb_recallargs *args,  
>>> void *dummy);
>>>
>>> #ifdef CONFIG_NFS_V4
>>> -extern int nfs_callback_up(void);
>>> +extern int nfs_callback_up(const sa_family_t family);
>>> extern void nfs_callback_down(void);
>>> #else
>>> -#define nfs_callback_up()    (0)
>>> +#define nfs_callback_up(x)   (0)
>>> #define nfs_callback_down()  do {} while(0)
>>> #endif
>>>
>>> diff --git a/fs/nfs/client.c b/fs/nfs/client.c
>>> index fc6a95d..5f8889f 100644
>>> --- a/fs/nfs/client.c
>>> +++ b/fs/nfs/client.c
>>> @@ -120,7 +120,7 @@ static struct nfs_client  
>>> *nfs_alloc_client(const struct nfs_client_initdata *cl_
>>>      clp->rpc_ops = cl_init->rpc_ops;
>>>
>>>      if (cl_init->rpc_ops->version == 4) {
>>> -             if (nfs_callback_up() < 0)
>>> +             if (nfs_callback_up(cl_init->addr->sa_family) < 0)
>>>                      goto error_2;
>>>              __set_bit(NFS_CS_CALLBACK, &clp->cl_res_state);
>>>      }
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux- 
>> nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
>
>
> -- 
> "If you simplify your English, you are freed from the worst follies of
> orthodoxy."
> -- George Orwell
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs"  
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 1/5] NFS: Enable NFSv4 callback server to listen on AF_INET6 sockets

[J. Bruce Fields]

On Mon, Aug 25, 2008 at 12:16:00PM -0400, Chuck Lever wrote:
> On Aug 22, 2008, at Aug 22, 2008, 10:34 PM, Chuck Lever wrote:
>> On Fri, Aug 22, 2008 at 7:18 PM, J. Bruce Fields  
>> <bfields@fieldses.org> wrote:
>>> On Fri, Aug 22, 2008 at 12:42:02PM -0400, Chuck Lever wrote:
>>>> Allow the NFS callback server to listen for requests via an AF_INET6
>>>> or AF_INET socket.
>>>>
>>>> The NFS callback server determines the listener's address family  
>>>> from
>>>> the address the client uses to contact the server.  The server will
>>>> always call the client back using the same address family.
>>>
>>> Won't the server determine that from the callback address which the
>>> client provides in the setclientid?
>>
>> A client has to have IPv6 networking to mount an IPv6 server.
>> Otherwise both will use IPv4.  Do you think we should worry about the
>> case where a client provides a callback address in a different address
>> family from the server's address?

Oh!  I don't know.  If there's a choice then I'd think the server would
use whatever address family it thinks the address you sent it in the
cb_location field of the setclientid, but if you tell me it's impossible
for that situation to ever arise then I'll believe you....

>> But I suppose it would be more precise to call nfs_callback_up() using
>> the family of the passed-in clientaddr= mount option instead of the
>> passed-in addr= option.  That will require discovering the address
>> family of the clientaddr string.
>>
>> We could convert the clientaddr string into a sockaddr temporarily (we
>> don't already do that... it's converted into a universal address
>> string, but not into a sockaddr).
>
> I thought about this more over the weekend.
>
> We've got a similar problem here that we have with starting just a TCP  
> or UDP listener for lockd based on what transport protocols were  
> requested on the mount command line.
>
> The NFSv4 callback server (and lockd, and probably NFSD) should start  
> either a single AF_INET or a single AF_INET6 listener, not both.  Right 
> now, the callback server's address family is selected based on mount 
> options for the first mount request.  But we only want one of these 
> listeners for all mount points on a system, and it should be one that 
> covers all the needed address families, no matter which address family 
> was used during the first mount request.
>
> So these services need to start a listener not based on mount options,  
> but on what kind of networking is available on the host.  If IPV6 is  
> available in the kernel, I think, an AF_INET6 listener should be  
> started; otherwise, start an AF_INET listener.
>
> I think the only externally visible issue with doing this is how  
> addresses are presented in kernel log and error messages.  If the  
> functions which generate presentation format address strings are smart  
> enough to recognize a mapped IPv4 address and present it in dotted-quad 
> format, that should be enough.
>
> Thoughts/opinions?

I don't know.  I still haven't had a chance to learn about ipv6, so I
feel a bit at a loss.

--b.

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[J. Bruce Fields]

It would be nice if not necessary just to move nfs_set_port() somewhere
common and use that (maybe with a more generic name).

That aside, all 5 patches are OK by me.

--b.

On Fri, Aug 22, 2008 at 12:42:33PM -0400, Chuck Lever wrote:
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  fs/lockd/host.c |   14 +++++++++++++-
>  1 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
> index 55ef751..3f0c1a8 100644
> --- a/fs/lockd/host.c
> +++ b/fs/lockd/host.c
> @@ -39,6 +39,18 @@ static struct nsm_handle *	nsm_find(const struct sockaddr_in *sin,
>  					 const char *hostname,
>  					 unsigned int hostname_len);
>  
> +static void nlm_clear_port(struct sockaddr *sap)
> +{
> +	switch (sap->sa_family) {
> +	case AF_INET:
> +		((struct sockaddr_in *)sap)->sin_port = 0;
> +		break;
> +	case AF_INET6:
> +		((struct sockaddr_in6 *)sap)->sin6_port = 0;
> +		break;
> +	}
> +}
> +
>  static void nlm_display_address(const struct sockaddr *sap,
>  				char *buf, const size_t len)
>  {
> @@ -149,7 +161,7 @@ static struct nlm_host *nlm_lookup_host(int server,
>  	}
>  	host->h_name	   = nsm->sm_name;
>  	host->h_addr       = *sin;
> -	host->h_addr.sin_port = 0;	/* ouch! */
> +	nlm_clear_port((struct sockaddr *)&host->h_addr);
>  	host->h_saddr	   = *ssin;
>  	host->h_version    = version;
>  	host->h_proto      = proto;
> 

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[J. Bruce Fields]

On Tue, Aug 26, 2008 at 03:03:19PM -0400, bfields wrote:
> It would be nice if not necessary just to move nfs_set_port() somewhere
> common and use that (maybe with a more generic name).
> 
> That aside, all 5 patches are OK by me.

Provisionally applied all but the first (purely client-side) patch to

	git://linux-nfs.org/~bfields/linux.git for-2.6.28

Do you have any good regression tests for nsm?

--b.

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[Chuck Lever]

On Aug 26, 2008, at Aug 26, 2008, 3:03 PM, J. Bruce Fields wrote:
> It would be nice if not necessary just to move nfs_set_port()  
> somewhere
> common and use that (maybe with a more generic name).

I agree.  However...

As we round out to a full IPv6 implementation for NFS, it will become  
clear what additional helper functions can be shared.  This might  
include not only a port setting helper, but also one that can  
construct presentation format addresses, and so on.

At that point, we can consolidate as needed.

Until then, I would like to wait until everything is reviewed and  
integrated so we are sure to include changes and updates related to  
the review process, if that's OK with you.

> On Fri, Aug 22, 2008 at 12:42:33PM -0400, Chuck Lever wrote:
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> fs/lockd/host.c |   14 +++++++++++++-
>> 1 files changed, 13 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index 55ef751..3f0c1a8 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -39,6 +39,18 @@ static struct nsm_handle *	nsm_find(const struct  
>> sockaddr_in *sin,
>> 					 const char *hostname,
>> 					 unsigned int hostname_len);
>>
>> +static void nlm_clear_port(struct sockaddr *sap)
>> +{
>> +	switch (sap->sa_family) {
>> +	case AF_INET:
>> +		((struct sockaddr_in *)sap)->sin_port = 0;
>> +		break;
>> +	case AF_INET6:
>> +		((struct sockaddr_in6 *)sap)->sin6_port = 0;
>> +		break;
>> +	}
>> +}
>> +
>> static void nlm_display_address(const struct sockaddr *sap,
>> 				char *buf, const size_t len)
>> {
>> @@ -149,7 +161,7 @@ static struct nlm_host *nlm_lookup_host(int  
>> server,
>> 	}
>> 	host->h_name	   = nsm->sm_name;
>> 	host->h_addr       = *sin;
>> -	host->h_addr.sin_port = 0;	/* ouch! */
>> +	nlm_clear_port((struct sockaddr *)&host->h_addr);
>> 	host->h_saddr	   = *ssin;
>> 	host->h_version    = version;
>> 	host->h_proto      = proto;
>>

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

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[Chuck Lever]

On Aug 26, 2008, at Aug 26, 2008, 3:17 PM, J. Bruce Fields wrote:
> On Tue, Aug 26, 2008 at 03:03:19PM -0400, bfields wrote:
>> It would be nice if not necessary just to move nfs_set_port()  
>> somewhere
>> common and use that (maybe with a more generic name).
>>
>> That aside, all 5 patches are OK by me.
>
> Provisionally applied all but the first (purely client-side) patch to
>
> 	git://linux-nfs.org/~bfields/linux.git for-2.6.28
>
> Do you have any good regression tests for nsm?

Netapp is providing some testing resources.  Their lock recovery tests  
provide some test coverage, but I don't know details.

One of the issues on the list for the Austin bake-a-thon is to review  
our test coverage, and see if we can flesh this out more.

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

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[J. Bruce Fields]

On Tue, Aug 26, 2008 at 03:56:12PM -0400, Chuck Lever wrote:
> On Aug 26, 2008, at Aug 26, 2008, 3:17 PM, J. Bruce Fields wrote:
>> On Tue, Aug 26, 2008 at 03:03:19PM -0400, bfields wrote:
>>> It would be nice if not necessary just to move nfs_set_port()  
>>> somewhere
>>> common and use that (maybe with a more generic name).
>>>
>>> That aside, all 5 patches are OK by me.
>>
>> Provisionally applied all but the first (purely client-side) patch to
>>
>> 	git://linux-nfs.org/~bfields/linux.git for-2.6.28
>>
>> Do you have any good regression tests for nsm?
>
> Netapp is providing some testing resources.  Their lock recovery tests  
> provide some test coverage, but I don't know details.

OK.  If they've got it in a form that's easy to distribute and use I'd
like to be able to run something like that here to test some of my work
in progress.

> One of the issues on the list for the Austin bake-a-thon is to review  
> our test coverage, and see if we can flesh this out more.

I won't be able to make it to Austin but I'll be interested in whatever
you find out.

--b.

Re: [PATCH 5/5] lockd: Add address family-agnostic helper for zeroing the port number

[Chuck Lever]

On Aug 27, 2008, at Aug 27, 2008, 9:55 AM, J. Bruce Fields wrote:
> On Tue, Aug 26, 2008 at 03:56:12PM -0400, Chuck Lever wrote:
>> On Aug 26, 2008, at Aug 26, 2008, 3:17 PM, J. Bruce Fields wrote:
>>> On Tue, Aug 26, 2008 at 03:03:19PM -0400, bfields wrote:
>>>> It would be nice if not necessary just to move nfs_set_port()
>>>> somewhere
>>>> common and use that (maybe with a more generic name).
>>>>
>>>> That aside, all 5 patches are OK by me.
>>>
>>> Provisionally applied all but the first (purely client-side) patch  
>>> to
>>>
>>> 	git://linux-nfs.org/~bfields/linux.git for-2.6.28
>>>
>>> Do you have any good regression tests for nsm?
>>
>> Netapp is providing some testing resources.  Their lock recovery  
>> tests
>> provide some test coverage, but I don't know details.
>
> OK.  If they've got it in a form that's easy to distribute and use I'd
> like to be able to run something like that here to test some of my  
> work
> in progress.

I believe it is still under development, and I don't know if they had  
plans to package and support it outside of their Q/A team.  I agree  
that it would be useful for the community at large.

>> One of the issues on the list for the Austin bake-a-thon is to review
>> our test coverage, and see if we can flesh this out more.
>
> I won't be able to make it to Austin but I'll be interested in  
> whatever
> you find out.

No problem.

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