All of lore.kernel.org
 help / color / mirror / Atom feed
* nfsd & svcrpc patches (mainly cleanup) for 3.7
@ 2012-08-21 20:57 J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 01/14] svcrpc: standardize svc_setup_socket return convention J. Bruce Fields
                   ` (13 more replies)
  0 siblings, 14 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs

This mainly cleanup (and one minor bugfix) noticed while I was debugging
some problems in the neighborhood.

--b.


^ permalink raw reply	[flat|nested] 22+ messages in thread
* [PATCH 01/14] svcrpc: standardize svc_setup_socket return convention
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 02/14] svcrpc: clean up control flow J. Bruce Fields
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Use the kernel-standard ptr-or-error return convention instead of
passing a pointer to the error.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svcsock.c |   40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 998aa8c..d028b51 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -59,7 +59,7 @@
 
 
 static struct svc_sock *svc_setup_socket(struct svc_serv *, struct socket *,
-					 int *errp, int flags);
+					 int flags);
 static void		svc_udp_data_ready(struct sock *, int);
 static int		svc_udp_recvfrom(struct svc_rqst *);
 static int		svc_udp_sendto(struct svc_rqst *);
@@ -900,8 +900,9 @@ static struct svc_xprt *svc_tcp_accept(struct svc_xprt *xprt)
 	 */
 	newsock->sk->sk_sndtimeo = HZ*30;
 
-	if (!(newsvsk = svc_setup_socket(serv, newsock, &err,
-				 (SVC_SOCK_ANONYMOUS | SVC_SOCK_TEMPORARY))))
+	newsvsk = svc_setup_socket(serv, newsock,
+				 (SVC_SOCK_ANONYMOUS | SVC_SOCK_TEMPORARY));
+	if (IS_ERR(newsvsk))
 		goto failed;
 	svc_xprt_set_remote(&newsvsk->sk_xprt, sin, slen);
 	err = kernel_getsockname(newsock, sin, &slen);
@@ -1383,29 +1384,29 @@ EXPORT_SYMBOL_GPL(svc_sock_update_bufs);
  */
 static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 						struct socket *sock,
-						int *errp, int flags)
+						int flags)
 {
 	struct svc_sock	*svsk;
 	struct sock	*inet;
 	int		pmap_register = !(flags & SVC_SOCK_ANONYMOUS);
+	int		err = 0;
 
 	dprintk("svc: svc_setup_socket %p\n", sock);
-	if (!(svsk = kzalloc(sizeof(*svsk), GFP_KERNEL))) {
-		*errp = -ENOMEM;
-		return NULL;
-	}
+	svsk = kzalloc(sizeof(*svsk), GFP_KERNEL);
+	if (!svsk)
+		return ERR_PTR(-ENOMEM);
 
 	inet = sock->sk;
 
 	/* Register socket with portmapper */
-	if (*errp >= 0 && pmap_register)
-		*errp = svc_register(serv, sock_net(sock->sk), inet->sk_family,
+	if (pmap_register)
+		err = svc_register(serv, sock_net(sock->sk), inet->sk_family,
 				     inet->sk_protocol,
 				     ntohs(inet_sk(inet)->inet_sport));
 
-	if (*errp < 0) {
+	if (err < 0) {
 		kfree(svsk);
-		return NULL;
+		return ERR_PTR(err);
 	}
 
 	inet->sk_user_data = svsk;
@@ -1463,10 +1464,12 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 	else {
 		if (!try_module_get(THIS_MODULE))
 			err = -ENOENT;
-		else
-			svsk = svc_setup_socket(serv, so, &err,
-						SVC_SOCK_DEFAULTS);
-		if (svsk) {
+		else {
+			svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
+			if (IS_ERR(svsk))
+				err = PTR_ERR(svsk);
+		}
+		if (err == 0) {
 			struct sockaddr_storage addr;
 			struct sockaddr *sin = (struct sockaddr *)&addr;
 			int salen;
@@ -1563,11 +1566,12 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 			goto bummer;
 	}
 
-	if ((svsk = svc_setup_socket(serv, sock, &error, flags)) != NULL) {
+	svsk = svc_setup_socket(serv, sock, flags);
+	if (!IS_ERR(svsk)) {
 		svc_xprt_set_local(&svsk->sk_xprt, newsin, newlen);
 		return (struct svc_xprt *)svsk;
 	}
-
+	error = PTR_ERR(svsk);
 bummer:
 	dprintk("svc: svc_create_socket error = %d\n", -error);
 	sock_release(sock);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 02/14] svcrpc: clean up control flow
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 01/14] svcrpc: standardize svc_setup_socket return convention J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 03/14] svcrpc: make svc_create_xprt enqueue on clearing XPT_BUSY J. Bruce Fields
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Mainly, use the kernel standard

	err = -ERROR;
	if (something_bad)
		goto out;
	normal case;

rather than

	if (something_bad)
		err = -ERROR
	else {
		normal case;
	}

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svcsock.c |   69 +++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 35 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index d028b51..bf10b72 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1451,44 +1451,42 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 	int err = 0;
 	struct socket *so = sockfd_lookup(fd, &err);
 	struct svc_sock *svsk = NULL;
+	struct sockaddr_storage addr;
+	struct sockaddr *sin = (struct sockaddr *)&addr;
+	int salen;
 
 	if (!so)
 		return err;
+	err = -EAFNOSUPPORT;
 	if ((so->sk->sk_family != PF_INET) && (so->sk->sk_family != PF_INET6))
-		err =  -EAFNOSUPPORT;
-	else if (so->sk->sk_protocol != IPPROTO_TCP &&
+		goto out;
+	err =  -EPROTONOSUPPORT;
+	if (so->sk->sk_protocol != IPPROTO_TCP &&
 	    so->sk->sk_protocol != IPPROTO_UDP)
-		err =  -EPROTONOSUPPORT;
-	else if (so->state > SS_UNCONNECTED)
-		err = -EISCONN;
-	else {
-		if (!try_module_get(THIS_MODULE))
-			err = -ENOENT;
-		else {
-			svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
-			if (IS_ERR(svsk))
-				err = PTR_ERR(svsk);
-		}
-		if (err == 0) {
-			struct sockaddr_storage addr;
-			struct sockaddr *sin = (struct sockaddr *)&addr;
-			int salen;
-			if (kernel_getsockname(svsk->sk_sock, sin, &salen) == 0)
-				svc_xprt_set_local(&svsk->sk_xprt, sin, salen);
-			clear_bit(XPT_TEMP, &svsk->sk_xprt.xpt_flags);
-			spin_lock_bh(&serv->sv_lock);
-			list_add(&svsk->sk_xprt.xpt_list, &serv->sv_permsocks);
-			spin_unlock_bh(&serv->sv_lock);
-			svc_xprt_received(&svsk->sk_xprt);
-			err = 0;
-		} else
-			module_put(THIS_MODULE);
-	}
-	if (err) {
-		sockfd_put(so);
-		return err;
+		goto out;
+	err = -EISCONN;
+	if (so->state > SS_UNCONNECTED)
+		goto out;
+	err = -ENOENT;
+	if (!try_module_get(THIS_MODULE))
+		goto out;
+	svsk = svc_setup_socket(serv, so, SVC_SOCK_DEFAULTS);
+	if (IS_ERR(svsk)) {
+		module_put(THIS_MODULE);
+		err = PTR_ERR(svsk);
+		goto out;
 	}
+	if (kernel_getsockname(svsk->sk_sock, sin, &salen) == 0)
+		svc_xprt_set_local(&svsk->sk_xprt, sin, salen);
+	clear_bit(XPT_TEMP, &svsk->sk_xprt.xpt_flags);
+	spin_lock_bh(&serv->sv_lock);
+	list_add(&svsk->sk_xprt.xpt_list, &serv->sv_permsocks);
+	spin_unlock_bh(&serv->sv_lock);
+	svc_xprt_received(&svsk->sk_xprt);
 	return svc_one_sock_name(svsk, name_return, len);
+out:
+	sockfd_put(so);
+	return err;
 }
 EXPORT_SYMBOL_GPL(svc_addsock);
 
@@ -1567,11 +1565,12 @@ static struct svc_xprt *svc_create_socket(struct svc_serv *serv,
 	}
 
 	svsk = svc_setup_socket(serv, sock, flags);
-	if (!IS_ERR(svsk)) {
-		svc_xprt_set_local(&svsk->sk_xprt, newsin, newlen);
-		return (struct svc_xprt *)svsk;
+	if (IS_ERR(svsk)) {
+		error = PTR_ERR(svsk);
+		goto bummer;
 	}
-	error = PTR_ERR(svsk);
+	svc_xprt_set_local(&svsk->sk_xprt, newsin, newlen);
+	return (struct svc_xprt *)svsk;
 bummer:
 	dprintk("svc: svc_create_socket error = %d\n", -error);
 	sock_release(sock);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 03/14] svcrpc: make svc_create_xprt enqueue on clearing XPT_BUSY
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 01/14] svcrpc: standardize svc_setup_socket return convention J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 02/14] svcrpc: clean up control flow J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 04/14] svcrpc: share some setup of listening sockets J. Bruce Fields
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

The rule is that whever we clear XPT_BUSY we should call
svc_xprt_enqueue().  Without that we may fail to notice any events (such
as new connections) that arrived while XPT_BUSY was set.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svc_xprt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index e1810b9..4801fda 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -238,7 +238,7 @@ int svc_create_xprt(struct svc_serv *serv, const char *xprt_name,
 		list_add(&newxprt->xpt_list, &serv->sv_permsocks);
 		spin_unlock_bh(&serv->sv_lock);
 		newport = svc_xprt_local_port(newxprt);
-		clear_bit(XPT_BUSY, &newxprt->xpt_flags);
+		svc_xprt_received(newxprt);
 		return newport;
 	}
  err:
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 04/14] svcrpc: share some setup of listening sockets
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (2 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 03/14] svcrpc: make svc_create_xprt enqueue on clearing XPT_BUSY J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 05/14] nfsd: remove redundant "port" argument J. Bruce Fields
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

There's some duplicate code here.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/sunrpc/svc_xprt.h |    1 +
 net/sunrpc/svc_xprt.c           |   16 ++++++++++------
 net/sunrpc/svcsock.c            |    6 +-----
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index b3f64b1..73c7a68 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -124,6 +124,7 @@ struct	svc_xprt *svc_find_xprt(struct svc_serv *serv, const char *xcl_name,
 			struct net *net, const sa_family_t af,
 			const unsigned short port);
 int	svc_xprt_names(struct svc_serv *serv, char *buf, const int buflen);
+void	svc_add_new_perm_xprt(struct svc_serv *serv, struct svc_xprt *xprt);
 
 static inline void svc_xprt_get(struct svc_xprt *xprt)
 {
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 4801fda..ee15663 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -208,6 +208,15 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 	return xcl->xcl_ops->xpo_create(serv, net, sap, len, flags);
 }
 
+void svc_add_new_perm_xprt(struct svc_serv *serv, struct svc_xprt *new)
+{
+	clear_bit(XPT_TEMP, &new->xpt_flags);
+	spin_lock_bh(&serv->sv_lock);
+	list_add(&new->xpt_list, &serv->sv_permsocks);
+	spin_unlock_bh(&serv->sv_lock);
+	svc_xprt_received(new);
+}
+
 int svc_create_xprt(struct svc_serv *serv, const char *xprt_name,
 		    struct net *net, const int family,
 		    const unsigned short port, int flags)
@@ -232,13 +241,8 @@ int svc_create_xprt(struct svc_serv *serv, const char *xprt_name,
 			module_put(xcl->xcl_owner);
 			return PTR_ERR(newxprt);
 		}
-
-		clear_bit(XPT_TEMP, &newxprt->xpt_flags);
-		spin_lock_bh(&serv->sv_lock);
-		list_add(&newxprt->xpt_list, &serv->sv_permsocks);
-		spin_unlock_bh(&serv->sv_lock);
+		svc_add_new_perm_xprt(serv, newxprt);
 		newport = svc_xprt_local_port(newxprt);
-		svc_xprt_received(newxprt);
 		return newport;
 	}
  err:
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index bf10b72..c7a7b14 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1478,11 +1478,7 @@ int svc_addsock(struct svc_serv *serv, const int fd, char *name_return,
 	}
 	if (kernel_getsockname(svsk->sk_sock, sin, &salen) == 0)
 		svc_xprt_set_local(&svsk->sk_xprt, sin, salen);
-	clear_bit(XPT_TEMP, &svsk->sk_xprt.xpt_flags);
-	spin_lock_bh(&serv->sv_lock);
-	list_add(&svsk->sk_xprt.xpt_list, &serv->sv_permsocks);
-	spin_unlock_bh(&serv->sv_lock);
-	svc_xprt_received(&svsk->sk_xprt);
+	svc_add_new_perm_xprt(serv, &svsk->sk_xprt);
 	return svc_one_sock_name(svsk, name_return, len);
 out:
 	sockfd_put(so);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 05/14] nfsd: remove redundant "port" argument
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (3 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 04/14] svcrpc: share some setup of listening sockets J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports J. Bruce Fields
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

"port" in all these functions is always NFS_PORT.

nfsd can already be run on a nonstandard port using the "nfsd/portlist"
interface.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfsctl.c |    2 +-
 fs/nfsd/nfsd.h   |    2 +-
 fs/nfsd/nfssvc.c |   14 +++++++-------
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 009632a..89be13c 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -406,7 +406,7 @@ static ssize_t write_threads(struct file *file, char *buf, size_t size)
 			return rv;
 		if (newthreads < 0)
 			return -EINVAL;
-		rv = nfsd_svc(NFS_PORT, newthreads);
+		rv = nfsd_svc(newthreads);
 		if (rv < 0)
 			return rv;
 	} else
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 36243a3..80d5ce4 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -65,7 +65,7 @@ extern const struct seq_operations nfs_exports_op;
 /*
  * Function prototypes.
  */
-int		nfsd_svc(unsigned short port, int nrservs);
+int		nfsd_svc(int nrservs);
 int		nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp);
 
 int		nfsd_nrthreads(void);
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 240473c..dd2b734 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -183,18 +183,18 @@ int nfsd_nrthreads(void)
 	return rv;
 }
 
-static int nfsd_init_socks(int port)
+static int nfsd_init_socks(void)
 {
 	int error;
 	if (!list_empty(&nfsd_serv->sv_permsocks))
 		return 0;
 
-	error = svc_create_xprt(nfsd_serv, "udp", &init_net, PF_INET, port,
+	error = svc_create_xprt(nfsd_serv, "udp", &init_net, PF_INET, NFS_PORT,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
 
-	error = svc_create_xprt(nfsd_serv, "tcp", &init_net, PF_INET, port,
+	error = svc_create_xprt(nfsd_serv, "tcp", &init_net, PF_INET, NFS_PORT,
 					SVC_SOCK_DEFAULTS);
 	if (error < 0)
 		return error;
@@ -204,7 +204,7 @@ static int nfsd_init_socks(int port)
 
 static bool nfsd_up = false;
 
-static int nfsd_startup(unsigned short port, int nrservs)
+static int nfsd_startup(int nrservs)
 {
 	int ret;
 
@@ -218,7 +218,7 @@ static int nfsd_startup(unsigned short port, int nrservs)
 	ret = nfsd_racache_init(2*nrservs);
 	if (ret)
 		return ret;
-	ret = nfsd_init_socks(port);
+	ret = nfsd_init_socks();
 	if (ret)
 		goto out_racache;
 	ret = lockd_up(&init_net);
@@ -436,7 +436,7 @@ int nfsd_set_nrthreads(int n, int *nthreads)
  * this is the first time nrservs is nonzero.
  */
 int
-nfsd_svc(unsigned short port, int nrservs)
+nfsd_svc(int nrservs)
 {
 	int	error;
 	bool	nfsd_up_before;
@@ -458,7 +458,7 @@ nfsd_svc(unsigned short port, int nrservs)
 
 	nfsd_up_before = nfsd_up;
 
-	error = nfsd_startup(port, nrservs);
+	error = nfsd_startup(nrservs);
 	if (error)
 		goto out_destroy;
 	error = svc_set_num_threads(nfsd_serv, NULL, nrservs);
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (4 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 05/14] nfsd: remove redundant "port" argument J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 21:25   ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 07/14] svcrpc: minor udp code cleanup J. Bruce Fields
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Note a 16-bit value can require up to 5 digits.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfsctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 89be13c..e41a08ff 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -712,7 +712,7 @@ static ssize_t __write_ports_addxprt(char *buf)
 	int port, err;
 	struct net *net = &init_net;
 
-	if (sscanf(buf, "%15s %4u", transport, &port) != 2)
+	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
 		return -EINVAL;
 
 	if (port < 1 || port > USHRT_MAX)
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 07/14] svcrpc: minor udp code cleanup
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (5 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 08/14] svcrpc: ignore unknown address type in udp receive J. Bruce Fields
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Order the code in a more boring way.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svcsock.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index c7a7b14..06ae8a7 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -620,10 +620,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 	if (!svc_udp_get_dest_address(rqstp, cmh)) {
 		net_warn_ratelimited("svc: received unknown control message %d/%d; dropping RPC reply datagram\n",
 				     cmh->cmsg_level, cmh->cmsg_type);
-out_free:
-		trace_kfree_skb(skb, svc_udp_recvfrom);
-		skb_free_datagram_locked(svsk->sk_sk, skb);
-		return 0;
+		goto out_free;
 	}
 	rqstp->rq_daddrlen = svc_addr_len(svc_daddr(rqstp));
 
@@ -662,6 +659,10 @@ out_free:
 		serv->sv_stats->netudpcnt++;
 
 	return len;
+out_free:
+	trace_kfree_skb(skb, svc_udp_recvfrom);
+	skb_free_datagram_locked(svsk->sk_sk, skb);
+	return 0;
 }
 
 static int
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (6 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 07/14] svcrpc: minor udp code cleanup J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 21:02   ` Chuck Lever
  2012-08-21 20:57 ` [PATCH 09/14] svcrpc: make xpo_recvfrom return only >=0 J. Bruce Fields
                   ` (5 subsequent siblings)
  13 siblings, 1 reply; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

How would this happen?

In any case, it appears this would be returned all the way up to the
caller of svc_recv(), and it's obvious that none of them are equipped to
handle it, and not clear what they would want to do with it anyway.
Let's just drop this and return -EAGAIN.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svcsock.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 06ae8a7..97ce23f 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -602,7 +602,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 	}
 	len = svc_addr_len(svc_addr(rqstp));
 	if (len == 0)
-		return -EAFNOSUPPORT;
+		return -EAGAIN;
 	rqstp->rq_addrlen = len;
 	if (skb->tstamp.tv64 == 0) {
 		skb->tstamp = ktime_get_real();
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 09/14] svcrpc: make xpo_recvfrom return only >=0
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (7 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 08/14] svcrpc: ignore unknown address type in udp receive J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 10/14] svcrpc: remove handling of unknown errors from svc_recv J. Bruce Fields
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

The only errors returned from xpo_recvfrom have been -EAGAIN and
-EAFNOSUPPORT.  The latter looked incorrect to me, so was removed by a
previous patch.  That leaves only -EAGAIN, which is treated by the
caller (svc_recv) identically to 0.

So, just ditch -EAGAIN and return 0 instead.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svc_xprt.c |    2 +-
 net/sunrpc/svcsock.c  |    8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index ee15663..3e31730 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -743,7 +743,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 	svc_xprt_received(xprt);
 
 	/* No data, incomplete (TCP) read, or accept() */
-	if (len == 0 || len == -EAGAIN)
+	if (len <= 0)
 		goto out;
 
 	clear_bit(XPT_OLD, &xprt->xpt_flags);
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 97ce23f..33008f2 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -598,11 +598,11 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 			dprintk("svc: recvfrom returned error %d\n", -err);
 			set_bit(XPT_DATA, &svsk->sk_xprt.xpt_flags);
 		}
-		return -EAGAIN;
+		return 0;
 	}
 	len = svc_addr_len(svc_addr(rqstp));
 	if (len == 0)
-		return -EAGAIN;
+		return 0;
 	rqstp->rq_addrlen = len;
 	if (skb->tstamp.tv64 == 0) {
 		skb->tstamp = ktime_get_real();
@@ -1176,13 +1176,13 @@ error:
 	if (len != -EAGAIN)
 		goto err_other;
 	dprintk("RPC: TCP recvfrom got EAGAIN\n");
-	return -EAGAIN;
+	return 0;
 err_other:
 	printk(KERN_NOTICE "%s: recvfrom returned errno %d\n",
 	       svsk->sk_xprt.xpt_server->sv_name, -len);
 	set_bit(XPT_CLOSE, &svsk->sk_xprt.xpt_flags);
 err_noclose:
-	return -EAGAIN;	/* record not complete */
+	return 0;	/* record not complete */
 }
 
 /*
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 10/14] svcrpc: remove handling of unknown errors from svc_recv
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (8 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 09/14] svcrpc: make xpo_recvfrom return only >=0 J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 11/14] svcrpc: make svc_xprt_received static J. Bruce Fields
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

svc_recv() returns only -EINTR or -EAGAIN.  If we really want to worry
about the case where it has a bug that causes it to return something
else, we could stick a WARN() in svc_recv.  But it's silly to require
every caller to have all this boilerplate to handle that case.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/lockd/svc.c    |   17 ++---------------
 fs/nfs/callback.c |   16 ++--------------
 fs/nfsd/nfssvc.c  |   12 +-----------
 3 files changed, 5 insertions(+), 40 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 31a63f8..e515569 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -126,7 +126,7 @@ static void restart_grace(void)
 static int
 lockd(void *vrqstp)
 {
-	int		err = 0, preverr = 0;
+	int		err = 0;
 	struct svc_rqst *rqstp = vrqstp;
 
 	/* try_to_freeze() is called from svc_recv() */
@@ -165,21 +165,8 @@ lockd(void *vrqstp)
 		 * recvfrom routine.
 		 */
 		err = svc_recv(rqstp, timeout);
-		if (err == -EAGAIN || err == -EINTR) {
-			preverr = err;
+		if (err == -EAGAIN || err == -EINTR)
 			continue;
-		}
-		if (err < 0) {
-			if (err != preverr) {
-				printk(KERN_WARNING "%s: unexpected error "
-					"from svc_recv (%d)\n", __func__, err);
-				preverr = err;
-			}
-			schedule_timeout_interruptible(HZ);
-			continue;
-		}
-		preverr = err;
-
 		dprintk("lockd: request from %s\n",
 				svc_print_addr(rqstp, buf, sizeof(buf)));
 
diff --git a/fs/nfs/callback.c b/fs/nfs/callback.c
index 4c8459e..d9e2a18 100644
--- a/fs/nfs/callback.c
+++ b/fs/nfs/callback.c
@@ -45,7 +45,7 @@ unsigned short nfs_callback_tcpport6;
 static int
 nfs4_callback_svc(void *vrqstp)
 {
-	int err, preverr = 0;
+	int err;
 	struct svc_rqst *rqstp = vrqstp;
 
 	set_freezable();
@@ -55,20 +55,8 @@ nfs4_callback_svc(void *vrqstp)
 		 * Listen for a request on the socket
 		 */
 		err = svc_recv(rqstp, MAX_SCHEDULE_TIMEOUT);
-		if (err == -EAGAIN || err == -EINTR) {
-			preverr = err;
+		if (err == -EAGAIN || err == -EINTR)
 			continue;
-		}
-		if (err < 0) {
-			if (err != preverr) {
-				printk(KERN_WARNING "NFS: %s: unexpected error "
-					"from svc_recv (%d)\n", __func__, err);
-				preverr = err;
-			}
-			schedule_timeout_uninterruptible(HZ);
-			continue;
-		}
-		preverr = err;
 		svc_process(rqstp);
 	}
 	return 0;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index dd2b734..2013aa00 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -487,7 +487,7 @@ static int
 nfsd(void *vrqstp)
 {
 	struct svc_rqst *rqstp = (struct svc_rqst *) vrqstp;
-	int err, preverr = 0;
+	int err;
 
 	/* Lock module and set up kernel thread */
 	mutex_lock(&nfsd_mutex);
@@ -534,16 +534,6 @@ nfsd(void *vrqstp)
 			;
 		if (err == -EINTR)
 			break;
-		else if (err < 0) {
-			if (err != preverr) {
-				printk(KERN_WARNING "%s: unexpected error "
-					"from svc_recv (%d)\n", __func__, -err);
-				preverr = err;
-			}
-			schedule_timeout_uninterruptible(HZ);
-			continue;
-		}
-
 		validate_process_creds();
 		svc_process(rqstp);
 		validate_process_creds();
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 11/14] svcrpc: make svc_xprt_received static
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (9 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 10/14] svcrpc: remove handling of unknown errors from svc_recv J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 12/14] svcrpc: break up svc_recv J. Bruce Fields
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Note this isn't used outside svc_xprt.c.

May as well move it so we don't need a declaration while we're here.

Also remove an outdated comment.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 include/linux/sunrpc/svc_xprt.h          |    1 -
 net/sunrpc/svc_xprt.c                    |   41 +++++++++++++++---------------
 net/sunrpc/xprtrdma/svc_rdma_transport.c |    4 ---
 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 73c7a68..e630d63 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -114,7 +114,6 @@ void	svc_xprt_init(struct net *, struct svc_xprt_class *, struct svc_xprt *,
 int	svc_create_xprt(struct svc_serv *, const char *, struct net *,
 			const int, const unsigned short, int);
 void	svc_xprt_enqueue(struct svc_xprt *xprt);
-void	svc_xprt_received(struct svc_xprt *);
 void	svc_xprt_put(struct svc_xprt *xprt);
 void	svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt);
 void	svc_close_xprt(struct svc_xprt *xprt);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 3e31730..295e6ed 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -208,6 +208,26 @@ static struct svc_xprt *__svc_xpo_create(struct svc_xprt_class *xcl,
 	return xcl->xcl_ops->xpo_create(serv, net, sap, len, flags);
 }
 
+/*
+ * svc_xprt_received conditionally queues the transport for processing
+ * by another thread. The caller must hold the XPT_BUSY bit and must
+ * not thereafter touch transport data.
+ *
+ * Note: XPT_DATA only gets cleared when a read-attempt finds no (or
+ * insufficient) data.
+ */
+static void svc_xprt_received(struct svc_xprt *xprt)
+{
+	BUG_ON(!test_bit(XPT_BUSY, &xprt->xpt_flags));
+	/* As soon as we clear busy, the xprt could be closed and
+	 * 'put', so we need a reference to call svc_xprt_enqueue with:
+	 */
+	svc_xprt_get(xprt);
+	clear_bit(XPT_BUSY, &xprt->xpt_flags);
+	svc_xprt_enqueue(xprt);
+	svc_xprt_put(xprt);
+}
+
 void svc_add_new_perm_xprt(struct svc_serv *serv, struct svc_xprt *new)
 {
 	clear_bit(XPT_TEMP, &new->xpt_flags);
@@ -398,27 +418,6 @@ static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
 	return xprt;
 }
 
-/*
- * svc_xprt_received conditionally queues the transport for processing
- * by another thread. The caller must hold the XPT_BUSY bit and must
- * not thereafter touch transport data.
- *
- * Note: XPT_DATA only gets cleared when a read-attempt finds no (or
- * insufficient) data.
- */
-void svc_xprt_received(struct svc_xprt *xprt)
-{
-	BUG_ON(!test_bit(XPT_BUSY, &xprt->xpt_flags));
-	/* As soon as we clear busy, the xprt could be closed and
-	 * 'put', so we need a reference to call svc_xprt_enqueue with:
-	 */
-	svc_xprt_get(xprt);
-	clear_bit(XPT_BUSY, &xprt->xpt_flags);
-	svc_xprt_enqueue(xprt);
-	svc_xprt_put(xprt);
-}
-EXPORT_SYMBOL_GPL(svc_xprt_received);
-
 /**
  * svc_reserve - change the space reserved for the reply to a request.
  * @rqstp:  The request in question
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 73b428b..62e4f9b 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -578,10 +578,6 @@ static void handle_connect_req(struct rdma_cm_id *new_cma_id, size_t client_ird)
 	list_add_tail(&newxprt->sc_accept_q, &listen_xprt->sc_accept_q);
 	spin_unlock_bh(&listen_xprt->sc_lock);
 
-	/*
-	 * Can't use svc_xprt_received here because we are not on a
-	 * rqstp thread
-	*/
 	set_bit(XPT_CONN, &listen_xprt->sc_xprt.xpt_flags);
 	svc_xprt_enqueue(&listen_xprt->sc_xprt);
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 12/14] svcrpc: break up svc_recv
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (10 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 11/14] svcrpc: make svc_xprt_received static J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 13/14] svcrpc: split up svc_handle_xprt J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 14/14] nfsd: document kernel interfaces for nfsd configuration J. Bruce Fields
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Matter of taste, I suppose, but svc_recv breaks up naturally into:

	allocate pages and setup arg
	dequeue (wait for, if necessary) next socket
	do something with that socket

And I find it easier to read when it doesn't go on for pages and pages.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svc_xprt.c |  103 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 36 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 295e6ed..6ebc9a9 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -568,33 +568,12 @@ static void svc_check_conn_limits(struct svc_serv *serv)
 	}
 }
 
-/*
- * Receive the next request on any transport.  This code is carefully
- * organised not to touch any cachelines in the shared svc_serv
- * structure, only cachelines in the local svc_pool.
- */
-int svc_recv(struct svc_rqst *rqstp, long timeout)
+int svc_alloc_arg(struct svc_rqst *rqstp)
 {
-	struct svc_xprt		*xprt = NULL;
-	struct svc_serv		*serv = rqstp->rq_server;
-	struct svc_pool		*pool = rqstp->rq_pool;
-	int			len, i;
-	int			pages;
-	struct xdr_buf		*arg;
-	DECLARE_WAITQUEUE(wait, current);
-	long			time_left;
-
-	dprintk("svc: server %p waiting for data (to = %ld)\n",
-		rqstp, timeout);
-
-	if (rqstp->rq_xprt)
-		printk(KERN_ERR
-			"svc_recv: service %p, transport not NULL!\n",
-			 rqstp);
-	if (waitqueue_active(&rqstp->rq_wait))
-		printk(KERN_ERR
-			"svc_recv: service %p, wait queue active!\n",
-			 rqstp);
+	struct svc_serv *serv = rqstp->rq_server;
+	struct xdr_buf *arg;
+	int pages;
+	int i;
 
 	/* now allocate needed pages.  If we get a failure, sleep briefly */
 	pages = (serv->sv_max_mesg + PAGE_SIZE) / PAGE_SIZE;
@@ -624,11 +603,15 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 	arg->page_len = (pages-2)*PAGE_SIZE;
 	arg->len = (pages-1)*PAGE_SIZE;
 	arg->tail[0].iov_len = 0;
+	return 0;
+}
 
-	try_to_freeze();
-	cond_resched();
-	if (signalled() || kthread_should_stop())
-		return -EINTR;
+struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
+{
+	struct svc_xprt *xprt;
+	struct svc_pool		*pool = rqstp->rq_pool;
+	DECLARE_WAITQUEUE(wait, current);
+	long			time_left;
 
 	/* Normally we will wait up to 5 seconds for any required
 	 * cache information to be provided.
@@ -666,7 +649,7 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		if (kthread_should_stop()) {
 			set_current_state(TASK_RUNNING);
 			spin_unlock_bh(&pool->sp_lock);
-			return -EINTR;
+			return ERR_PTR(-EINTR);
 		}
 
 		add_wait_queue(&rqstp->rq_wait, &wait);
@@ -687,19 +670,25 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			spin_unlock_bh(&pool->sp_lock);
 			dprintk("svc: server %p, no data yet\n", rqstp);
 			if (signalled() || kthread_should_stop())
-				return -EINTR;
+				return ERR_PTR(-EINTR);
 			else
-				return -EAGAIN;
+				return ERR_PTR(-EAGAIN);
 		}
 	}
 	spin_unlock_bh(&pool->sp_lock);
+	return xprt;
+}
+
+static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
+{
+	struct svc_serv *serv = rqstp->rq_server;
+	int len = 0;
 
-	len = 0;
 	if (test_bit(XPT_CLOSE, &xprt->xpt_flags)) {
 		dprintk("svc_recv: found XPT_CLOSE\n");
 		svc_delete_xprt(xprt);
 		/* Leave XPT_BUSY set on the dead xprt: */
-		goto out;
+		return 0;
 	}
 	if (test_bit(XPT_LISTENER, &xprt->xpt_flags)) {
 		struct svc_xprt *newxpt;
@@ -727,8 +716,9 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 			svc_xprt_received(newxpt);
 		}
 	} else if (xprt->xpt_ops->xpo_has_wspace(xprt)) {
+		/* XPT_DATA|XPT_DEFERRED case: */
 		dprintk("svc: server %p, pool %u, transport %p, inuse=%d\n",
-			rqstp, pool->sp_id, xprt,
+			rqstp, rqstp->rq_pool->sp_id, xprt,
 			atomic_read(&xprt->xpt_ref.refcount));
 		rqstp->rq_deferred = svc_deferred_dequeue(xprt);
 		if (rqstp->rq_deferred)
@@ -739,7 +729,48 @@ int svc_recv(struct svc_rqst *rqstp, long timeout)
 		rqstp->rq_reserved = serv->sv_max_mesg;
 		atomic_add(rqstp->rq_reserved, &xprt->xpt_reserved);
 	}
+	/* clear XPT_BUSY: */
 	svc_xprt_received(xprt);
+	return len;
+}
+
+/*
+ * Receive the next request on any transport.  This code is carefully
+ * organised not to touch any cachelines in the shared svc_serv
+ * structure, only cachelines in the local svc_pool.
+ */
+int svc_recv(struct svc_rqst *rqstp, long timeout)
+{
+	struct svc_xprt		*xprt = NULL;
+	struct svc_serv		*serv = rqstp->rq_server;
+	int			len, err;
+
+	dprintk("svc: server %p waiting for data (to = %ld)\n",
+		rqstp, timeout);
+
+	if (rqstp->rq_xprt)
+		printk(KERN_ERR
+			"svc_recv: service %p, transport not NULL!\n",
+			 rqstp);
+	if (waitqueue_active(&rqstp->rq_wait))
+		printk(KERN_ERR
+			"svc_recv: service %p, wait queue active!\n",
+			 rqstp);
+
+	err = svc_alloc_arg(rqstp);
+	if (err)
+		return err;
+
+	try_to_freeze();
+	cond_resched();
+	if (signalled() || kthread_should_stop())
+		return -EINTR;
+
+	xprt = svc_get_next_xprt(rqstp, timeout);
+	if (IS_ERR(xprt))
+		return PTR_ERR(xprt);
+
+	len = svc_handle_xprt(rqstp, xprt);
 
 	/* No data, incomplete (TCP) read, or accept() */
 	if (len <= 0)
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 13/14] svcrpc: split up svc_handle_xprt
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (11 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 12/14] svcrpc: break up svc_recv J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  2012-08-21 20:57 ` [PATCH 14/14] nfsd: document kernel interfaces for nfsd configuration J. Bruce Fields
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

Move initialization of newly accepted socket into a helper.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 net/sunrpc/svc_xprt.c |   47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 6ebc9a9..194d865 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -679,6 +679,23 @@ struct svc_xprt *svc_get_next_xprt(struct svc_rqst *rqstp, long timeout)
 	return xprt;
 }
 
+void svc_add_new_temp_xprt(struct svc_serv *serv, struct svc_xprt *newxpt)
+{
+	spin_lock_bh(&serv->sv_lock);
+	set_bit(XPT_TEMP, &newxpt->xpt_flags);
+	list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
+	serv->sv_tmpcnt++;
+	if (serv->sv_temptimer.function == NULL) {
+		/* setup timer to age temp transports */
+		setup_timer(&serv->sv_temptimer, svc_age_temp_xprts,
+			    (unsigned long)serv);
+		mod_timer(&serv->sv_temptimer,
+			  jiffies + svc_conn_age_period * HZ);
+	}
+	spin_unlock_bh(&serv->sv_lock);
+	svc_xprt_received(newxpt);
+}
+
 static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
 {
 	struct svc_serv *serv = rqstp->rq_server;
@@ -692,29 +709,15 @@ static int svc_handle_xprt(struct svc_rqst *rqstp, struct svc_xprt *xprt)
 	}
 	if (test_bit(XPT_LISTENER, &xprt->xpt_flags)) {
 		struct svc_xprt *newxpt;
+		/*
+		 * We know this module_get will succeed because the
+		 * listener holds a reference too
+		 */
+		__module_get(xprt->xpt_class->xcl_owner);
+		svc_check_conn_limits(xprt->xpt_server);
 		newxpt = xprt->xpt_ops->xpo_accept(xprt);
-		if (newxpt) {
-			/*
-			 * We know this module_get will succeed because the
-			 * listener holds a reference too
-			 */
-			__module_get(newxpt->xpt_class->xcl_owner);
-			svc_check_conn_limits(xprt->xpt_server);
-			spin_lock_bh(&serv->sv_lock);
-			set_bit(XPT_TEMP, &newxpt->xpt_flags);
-			list_add(&newxpt->xpt_list, &serv->sv_tempsocks);
-			serv->sv_tmpcnt++;
-			if (serv->sv_temptimer.function == NULL) {
-				/* setup timer to age temp transports */
-				setup_timer(&serv->sv_temptimer,
-					    svc_age_temp_xprts,
-					    (unsigned long)serv);
-				mod_timer(&serv->sv_temptimer,
-					  jiffies + svc_conn_age_period * HZ);
-			}
-			spin_unlock_bh(&serv->sv_lock);
-			svc_xprt_received(newxpt);
-		}
+		if (newxpt)
+			svc_add_new_temp_xprt(serv, newxpt);
 	} else if (xprt->xpt_ops->xpo_has_wspace(xprt)) {
 		/* XPT_DATA|XPT_DEFERRED case: */
 		dprintk("svc: server %p, pool %u, transport %p, inuse=%d\n",
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* [PATCH 14/14] nfsd: document kernel interfaces for nfsd configuration
  2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
                   ` (12 preceding siblings ...)
  2012-08-21 20:57 ` [PATCH 13/14] svcrpc: split up svc_handle_xprt J. Bruce Fields
@ 2012-08-21 20:57 ` J. Bruce Fields
  13 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 20:57 UTC (permalink / raw)
  To: linux-nfs; +Cc: J. Bruce Fields

From: "J. Bruce Fields" <bfields@redhat.com>

These are only needed by nfs-utils.  But I needed to remind myself how
they worked recently and thought this might be helpful.  It's short and
incomplete for now as I was only interested in startup, shutdown, and
configuration of listening sockets.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 .../filesystems/nfs/nfsd-admin-interfaces.txt      |   41 ++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/filesystems/nfs/nfsd-admin-interfaces.txt

diff --git a/Documentation/filesystems/nfs/nfsd-admin-interfaces.txt b/Documentation/filesystems/nfs/nfsd-admin-interfaces.txt
new file mode 100644
index 0000000..56a96fb
--- /dev/null
+++ b/Documentation/filesystems/nfs/nfsd-admin-interfaces.txt
@@ -0,0 +1,41 @@
+Administrative interfaces for nfsd
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Note that normally these interfaces are used only by the utilities in
+nfs-utils.
+
+nfsd is controlled mainly by pseudofiles under the "nfsd" filesystem,
+which is normally mounted at /proc/fs/nfsd/.
+
+The server is always started by the first write of a nonzero value to
+nfsd/threads.
+
+Before doing that, NFSD can be told which sockets to listen on by
+writing to nfsd/portlist; that write may be:
+
+	- an ascii-encoded file descriptor, which should refer to a
+	  bound (and listening, for tcp) socket, or
+	- "transportname port", where transportname is currently either
+	  "udp", "tcp", or "rdma".
+
+If nfsd is started without doing any of these, then it will create one
+udp and one tcp listener at port 2049 (see nfsd_init_socks).
+
+On startup, nfsd and lockd grace periods start.
+
+nfsd is shut down by a write of 0 to nfsd/threads.  All locks and state
+are thrown away at that point.
+
+Between startup and shutdown, the number of threads may be adjusted up
+or down by additional writes to nfsd/threads or by writes to
+nfsd/pool_threads.
+
+For more detail about files under nfsd/ and what they control, see
+fs/nfsd/nfsctl.c; most of them have detailed comments.
+
+Implementation notes
+^^^^^^^^^^^^^^^^^^^^
+
+Note that the rpc server requires the caller to serialize addition and
+removal of listening sockets, and startup and shutdown of the server.
+For nfsd this is done using nfsd_mutex.
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 20:57 ` [PATCH 08/14] svcrpc: ignore unknown address type in udp receive J. Bruce Fields
@ 2012-08-21 21:02   ` Chuck Lever
  2012-08-21 21:24     ` J. Bruce Fields
  0 siblings, 1 reply; 22+ messages in thread
From: Chuck Lever @ 2012-08-21 21:02 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: linux-nfs


On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:

> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> How would this happen?

If an unsupported address family is used in the rqstp.

> In any case, it appears this would be returned all the way up to the
> caller of svc_recv(), and it's obvious that none of them are equipped to
> handle it, and not clear what they would want to do with it anyway.
> Let's just drop this and return -EAGAIN.

EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.

> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> net/sunrpc/svcsock.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 06ae8a7..97ce23f 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -602,7 +602,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> 	}
> 	len = svc_addr_len(svc_addr(rqstp));
> 	if (len == 0)
> -		return -EAFNOSUPPORT;
> +		return -EAGAIN;
> 	rqstp->rq_addrlen = len;
> 	if (skb->tstamp.tv64 == 0) {
> 		skb->tstamp = ktime_get_real();
> -- 
> 1.7.9.5
> 
> --
> 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





^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 21:02   ` Chuck Lever
@ 2012-08-21 21:24     ` J. Bruce Fields
  2012-08-21 21:29       ` Chuck Lever
  0 siblings, 1 reply; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 21:24 UTC (permalink / raw)
  To: Chuck Lever; +Cc: J. Bruce Fields, linux-nfs

On Tue, Aug 21, 2012 at 05:02:14PM -0400, Chuck Lever wrote:
> 
> On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:
> 
> > From: "J. Bruce Fields" <bfields@redhat.com>
> > 
> > How would this happen?
> 
> If an unsupported address family is used in the rqstp.

Right, but that's impossible, isn't it?

> > In any case, it appears this would be returned all the way up to the
> > caller of svc_recv(), and it's obvious that none of them are equipped to
> > handle it, and not clear what they would want to do with it anyway.
> > Let's just drop this and return -EAGAIN.
> 
> EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.

Yeah.  Actually on a quick check this is the only caller that even
checks for this case.  So probably the check should go in svc_addr_len.
Maybe we should be nice and make it a warning.

--b.

commit 4814e806a44f3dee8f7cae00a7d0240487d62583
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Aug 21 17:22:11 2012 -0400

    svcrpc: don't bother checking bad svc_addr_len result
    
    None of the callers should see an unsupported address family (only one
    of them even bothers to check for that case), so just check for the
    buggy case in svc_addr_len and don't bother elsewhere.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index b3f64b1..9324008 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -166,7 +166,7 @@ static inline size_t svc_addr_len(const struct sockaddr *sa)
 	case AF_INET6:
 		return sizeof(struct sockaddr_in6);
 	}
-
+	WARN_ONCE(true, "unknown address family %d\n", sa->sa_family);
 	return 0;
 }
 
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 998aa8c..13b005c 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -601,8 +601,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 		return -EAGAIN;
 	}
 	len = svc_addr_len(svc_addr(rqstp));
-	if (len == 0)
-		return -EAFNOSUPPORT;
 	rqstp->rq_addrlen = len;
 	if (skb->tstamp.tv64 == 0) {
 		skb->tstamp = ktime_get_real();

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports
  2012-08-21 20:57 ` [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports J. Bruce Fields
@ 2012-08-21 21:25   ` J. Bruce Fields
  0 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 21:25 UTC (permalink / raw)
  To: linux-nfs

Second time we've seen a mistake like that recently, so maybe we really
should add the string-length-of-a-number macro.

--b.

On Tue, Aug 21, 2012 at 04:57:24PM -0400, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> Note a 16-bit value can require up to 5 digits.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfsctl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 89be13c..e41a08ff 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -712,7 +712,7 @@ static ssize_t __write_ports_addxprt(char *buf)
>  	int port, err;
>  	struct net *net = &init_net;
>  
> -	if (sscanf(buf, "%15s %4u", transport, &port) != 2)
> +	if (sscanf(buf, "%15s %5u", transport, &port) != 2)
>  		return -EINVAL;
>  
>  	if (port < 1 || port > USHRT_MAX)
> -- 
> 1.7.9.5
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 21:24     ` J. Bruce Fields
@ 2012-08-21 21:29       ` Chuck Lever
  2012-08-21 21:33         ` J. Bruce Fields
  0 siblings, 1 reply; 22+ messages in thread
From: Chuck Lever @ 2012-08-21 21:29 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: J. Bruce Fields, linux-nfs


On Aug 21, 2012, at 5:24 PM, J. Bruce Fields wrote:

> On Tue, Aug 21, 2012 at 05:02:14PM -0400, Chuck Lever wrote:
>> 
>> On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:
>> 
>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>> 
>>> How would this happen?
>> 
>> If an unsupported address family is used in the rqstp.
> 
> Right, but that's impossible, isn't it?

The point is to catch this case when someone adds support for new address families.  It won't happen in current code.

>>> In any case, it appears this would be returned all the way up to the
>>> caller of svc_recv(), and it's obvious that none of them are equipped to
>>> handle it, and not clear what they would want to do with it anyway.
>>> Let's just drop this and return -EAGAIN.
>> 
>> EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.
> 
> Yeah.  Actually on a quick check this is the only caller that even
> checks for this case.  So probably the check should go in svc_addr_len.
> Maybe we should be nice and make it a warning.

I normally find BUG pretty harsh, but in this case it's true a software error, and as you say, should be "impossible": thus it should be a BUG.  The code should stop before the zero length is used.

I agree, though, we should raise the red flag near the code that should be fixed.

> 
> --b.
> 
> commit 4814e806a44f3dee8f7cae00a7d0240487d62583
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Tue Aug 21 17:22:11 2012 -0400
> 
>    svcrpc: don't bother checking bad svc_addr_len result
> 
>    None of the callers should see an unsupported address family (only one
>    of them even bothers to check for that case), so just check for the
>    buggy case in svc_addr_len and don't bother elsewhere.
> 
>    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index b3f64b1..9324008 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -166,7 +166,7 @@ static inline size_t svc_addr_len(const struct sockaddr *sa)
> 	case AF_INET6:
> 		return sizeof(struct sockaddr_in6);
> 	}
> -
> +	WARN_ONCE(true, "unknown address family %d\n", sa->sa_family);
> 	return 0;
> }
> 
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 998aa8c..13b005c 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -601,8 +601,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> 		return -EAGAIN;
> 	}
> 	len = svc_addr_len(svc_addr(rqstp));
> -	if (len == 0)
> -		return -EAFNOSUPPORT;
> 	rqstp->rq_addrlen = len;
> 	if (skb->tstamp.tv64 == 0) {
> 		skb->tstamp = ktime_get_real();
> --
> 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





^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 21:29       ` Chuck Lever
@ 2012-08-21 21:33         ` J. Bruce Fields
  2012-08-21 21:38           ` Chuck Lever
  0 siblings, 1 reply; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 21:33 UTC (permalink / raw)
  To: Chuck Lever; +Cc: J. Bruce Fields, linux-nfs

On Tue, Aug 21, 2012 at 05:29:16PM -0400, Chuck Lever wrote:
> 
> On Aug 21, 2012, at 5:24 PM, J. Bruce Fields wrote:
> 
> > On Tue, Aug 21, 2012 at 05:02:14PM -0400, Chuck Lever wrote:
> >> 
> >> On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:
> >> 
> >>> From: "J. Bruce Fields" <bfields@redhat.com>
> >>> 
> >>> How would this happen?
> >> 
> >> If an unsupported address family is used in the rqstp.
> > 
> > Right, but that's impossible, isn't it?
> 
> The point is to catch this case when someone adds support for new address families.  It won't happen in current code.
> 
> >>> In any case, it appears this would be returned all the way up to the
> >>> caller of svc_recv(), and it's obvious that none of them are equipped to
> >>> handle it, and not clear what they would want to do with it anyway.
> >>> Let's just drop this and return -EAGAIN.
> >> 
> >> EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.
> > 
> > Yeah.  Actually on a quick check this is the only caller that even
> > checks for this case.  So probably the check should go in svc_addr_len.
> > Maybe we should be nice and make it a warning.
> 
> I normally find BUG pretty harsh, but in this case it's true a software error, and as you say, should be "impossible": thus it should be a BUG.  The code should stop before the zero length is used.

Eh, OK, I guess I can live with that.

commit 751f877b10f8ce0d12b40d2a102f3b42b26dc08d
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Aug 21 17:22:11 2012 -0400

    svcrpc: don't bother checking bad svc_addr_len result
    
    None of the callers should see an unsupported address family (only one
    of them even bothers to check for that case), so just check for the
    buggy case in svc_addr_len and don't bother elsewhere.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index b3f64b1..4f3a6ab 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -166,8 +166,7 @@ static inline size_t svc_addr_len(const struct sockaddr *sa)
 	case AF_INET6:
 		return sizeof(struct sockaddr_in6);
 	}
-
-	return 0;
+	BUG();
 }
 
 static inline unsigned short svc_xprt_local_port(const struct svc_xprt *xprt)
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 998aa8c..13b005c 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -601,8 +601,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 		return -EAGAIN;
 	}
 	len = svc_addr_len(svc_addr(rqstp));
-	if (len == 0)
-		return -EAFNOSUPPORT;
 	rqstp->rq_addrlen = len;
 	if (skb->tstamp.tv64 == 0) {
 		skb->tstamp = ktime_get_real();

^ permalink raw reply related	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 21:33         ` J. Bruce Fields
@ 2012-08-21 21:38           ` Chuck Lever
  2012-08-21 21:42             ` J. Bruce Fields
  0 siblings, 1 reply; 22+ messages in thread
From: Chuck Lever @ 2012-08-21 21:38 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: J. Bruce Fields, linux-nfs


On Aug 21, 2012, at 5:33 PM, J. Bruce Fields wrote:

> On Tue, Aug 21, 2012 at 05:29:16PM -0400, Chuck Lever wrote:
>> 
>> On Aug 21, 2012, at 5:24 PM, J. Bruce Fields wrote:
>> 
>>> On Tue, Aug 21, 2012 at 05:02:14PM -0400, Chuck Lever wrote:
>>>> 
>>>> On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:
>>>> 
>>>>> From: "J. Bruce Fields" <bfields@redhat.com>
>>>>> 
>>>>> How would this happen?
>>>> 
>>>> If an unsupported address family is used in the rqstp.
>>> 
>>> Right, but that's impossible, isn't it?
>> 
>> The point is to catch this case when someone adds support for new address families.  It won't happen in current code.
>> 
>>>>> In any case, it appears this would be returned all the way up to the
>>>>> caller of svc_recv(), and it's obvious that none of them are equipped to
>>>>> handle it, and not clear what they would want to do with it anyway.
>>>>> Let's just drop this and return -EAGAIN.
>>>> 
>>>> EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.
>>> 
>>> Yeah.  Actually on a quick check this is the only caller that even
>>> checks for this case.  So probably the check should go in svc_addr_len.
>>> Maybe we should be nice and make it a warning.
>> 
>> I normally find BUG pretty harsh, but in this case it's true a software error, and as you say, should be "impossible": thus it should be a BUG.  The code should stop before the zero length is used.
> 
> Eh, OK, I guess I can live with that.
> 
> commit 751f877b10f8ce0d12b40d2a102f3b42b26dc08d
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Tue Aug 21 17:22:11 2012 -0400
> 
>    svcrpc: don't bother checking bad svc_addr_len result
> 
>    None of the callers should see an unsupported address family (only one
>    of them even bothers to check for that case), so just check for the
>    buggy case in svc_addr_len and don't bother elsewhere.
> 
>    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

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

> 
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index b3f64b1..4f3a6ab 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -166,8 +166,7 @@ static inline size_t svc_addr_len(const struct sockaddr *sa)
> 	case AF_INET6:
> 		return sizeof(struct sockaddr_in6);
> 	}
> -
> -	return 0;
> +	BUG();
> }
> 
> static inline unsigned short svc_xprt_local_port(const struct svc_xprt *xprt)
> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> index 998aa8c..13b005c 100644
> --- a/net/sunrpc/svcsock.c
> +++ b/net/sunrpc/svcsock.c
> @@ -601,8 +601,6 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
> 		return -EAGAIN;
> 	}
> 	len = svc_addr_len(svc_addr(rqstp));
> -	if (len == 0)
> -		return -EAFNOSUPPORT;
> 	rqstp->rq_addrlen = len;
> 	if (skb->tstamp.tv64 == 0) {
> 		skb->tstamp = ktime_get_real();
> --
> 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





^ permalink raw reply	[flat|nested] 22+ messages in thread
* Re: [PATCH 08/14] svcrpc: ignore unknown address type in udp receive
  2012-08-21 21:38           ` Chuck Lever
@ 2012-08-21 21:42             ` J. Bruce Fields
  0 siblings, 0 replies; 22+ messages in thread
From: J. Bruce Fields @ 2012-08-21 21:42 UTC (permalink / raw)
  To: Chuck Lever; +Cc: J. Bruce Fields, linux-nfs

On Tue, Aug 21, 2012 at 05:38:26PM -0400, Chuck Lever wrote:
> 
> On Aug 21, 2012, at 5:33 PM, J. Bruce Fields wrote:
> 
> > On Tue, Aug 21, 2012 at 05:29:16PM -0400, Chuck Lever wrote:
> >> 
> >> On Aug 21, 2012, at 5:24 PM, J. Bruce Fields wrote:
> >> 
> >>> On Tue, Aug 21, 2012 at 05:02:14PM -0400, Chuck Lever wrote:
> >>>> 
> >>>> On Aug 21, 2012, at 4:57 PM, J. Bruce Fields wrote:
> >>>> 
> >>>>> From: "J. Bruce Fields" <bfields@redhat.com>
> >>>>> 
> >>>>> How would this happen?
> >>>> 
> >>>> If an unsupported address family is used in the rqstp.
> >>> 
> >>> Right, but that's impossible, isn't it?
> >> 
> >> The point is to catch this case when someone adds support for new address families.  It won't happen in current code.
> >> 
> >>>>> In any case, it appears this would be returned all the way up to the
> >>>>> caller of svc_recv(), and it's obvious that none of them are equipped to
> >>>>> handle it, and not clear what they would want to do with it anyway.
> >>>>> Let's just drop this and return -EAGAIN.
> >>>> 
> >>>> EAGAIN is incorrect; the correct error code is EAFNOSUPPORT.  If callers are not prepared for this error return, perhaps BUG_ON() would be more appropriate here.
> >>> 
> >>> Yeah.  Actually on a quick check this is the only caller that even
> >>> checks for this case.  So probably the check should go in svc_addr_len.
> >>> Maybe we should be nice and make it a warning.
> >> 
> >> I normally find BUG pretty harsh, but in this case it's true a software error, and as you say, should be "impossible": thus it should be a BUG.  The code should stop before the zero length is used.
> > 
> > Eh, OK, I guess I can live with that.
> > 
> > commit 751f877b10f8ce0d12b40d2a102f3b42b26dc08d
> > Author: J. Bruce Fields <bfields@redhat.com>
> > Date:   Tue Aug 21 17:22:11 2012 -0400
> > 
> >    svcrpc: don't bother checking bad svc_addr_len result
> > 
> >    None of the callers should see an unsupported address family (only one
> >    of them even bothers to check for that case), so just check for the
> >    buggy case in svc_addr_len and don't bother elsewhere.
> > 
> >    Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>

Ok, thanks.--b.

^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2012-08-21 21:42 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-21 20:57 nfsd & svcrpc patches (mainly cleanup) for 3.7 J. Bruce Fields
2012-08-21 20:57 ` [PATCH 01/14] svcrpc: standardize svc_setup_socket return convention J. Bruce Fields
2012-08-21 20:57 ` [PATCH 02/14] svcrpc: clean up control flow J. Bruce Fields
2012-08-21 20:57 ` [PATCH 03/14] svcrpc: make svc_create_xprt enqueue on clearing XPT_BUSY J. Bruce Fields
2012-08-21 20:57 ` [PATCH 04/14] svcrpc: share some setup of listening sockets J. Bruce Fields
2012-08-21 20:57 ` [PATCH 05/14] nfsd: remove redundant "port" argument J. Bruce Fields
2012-08-21 20:57 ` [PATCH 06/14] nfsd: allow configuring nfsd to listen on 5-digit ports J. Bruce Fields
2012-08-21 21:25   ` J. Bruce Fields
2012-08-21 20:57 ` [PATCH 07/14] svcrpc: minor udp code cleanup J. Bruce Fields
2012-08-21 20:57 ` [PATCH 08/14] svcrpc: ignore unknown address type in udp receive J. Bruce Fields
2012-08-21 21:02   ` Chuck Lever
2012-08-21 21:24     ` J. Bruce Fields
2012-08-21 21:29       ` Chuck Lever
2012-08-21 21:33         ` J. Bruce Fields
2012-08-21 21:38           ` Chuck Lever
2012-08-21 21:42             ` J. Bruce Fields
2012-08-21 20:57 ` [PATCH 09/14] svcrpc: make xpo_recvfrom return only >=0 J. Bruce Fields
2012-08-21 20:57 ` [PATCH 10/14] svcrpc: remove handling of unknown errors from svc_recv J. Bruce Fields
2012-08-21 20:57 ` [PATCH 11/14] svcrpc: make svc_xprt_received static J. Bruce Fields
2012-08-21 20:57 ` [PATCH 12/14] svcrpc: break up svc_recv J. Bruce Fields
2012-08-21 20:57 ` [PATCH 13/14] svcrpc: split up svc_handle_xprt J. Bruce Fields
2012-08-21 20:57 ` [PATCH 14/14] nfsd: document kernel interfaces for nfsd configuration J. Bruce Fields

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.