All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] client-side lockd doesn't start UDP listener
@ 2008-10-03 21:15 Chuck Lever
       [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Chuck Lever @ 2008-10-03 21:15 UTC (permalink / raw)
  To: bfields, neilb; +Cc: linux-nfs

Hi Bruce, Neil-

Here's my initial proposal to address the NFSv2/v3 lock recovery issue 
that results from having no UDP lockd listener.

Comments?  Did I miss anything?

---

Chuck Lever (3):
      NLM: Remove unused argument from svc_addsock() function
      NLM: Remove "proto" argument from lockd_up()
      NLM: Always start both UDP and TCP listeners

 fs/lockd/clntlock.c            |    4 ++--
 fs/lockd/svc.c                 |   40 ++++++++++++++++++++--------------------
 fs/nfsd/nfsctl.c               |    5 ++---
 fs/nfsd/nfssvc.c               |   19 +++++++------------
 include/linux/lockd/bind.h     |    2 +-
 include/linux/sunrpc/svcsock.h |    5 +----
 net/sunrpc/svcsock.c           |    4 +---
 7 files changed, 34 insertions(+), 45 deletions(-)

-- 
Chuck Lever

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] NLM: Always start both UDP and TCP listeners
       [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
@ 2008-10-03 21:15   ` Chuck Lever
  2008-10-03 21:15   ` [PATCH 2/3] NLM: Remove "proto" argument from lockd_up() Chuck Lever
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 14+ messages in thread
From: Chuck Lever @ 2008-10-03 21:15 UTC (permalink / raw)
  To: bfields, neilb; +Cc: linux-nfs

Commit 24e36663, which first appeared in 2.6.19, changed lockd so that
the client side starts a UDP listener only if there is a UDP NFSv2/v3
mount.  Its description notes:

    This... means that lockd will *not* listen on UDP if the only
    mounts are TCP mount (and nfsd hasn't started).

    The latter is the only one that concerns me at all - I don't know
    if this might be a problem with some servers.

Unfortunately it is a problem for Linux itself.  The rpc.statd daemon
on Linux uses UDP for contacting the local lockd, no matter which
protocol is used for NFS mounts.  Without a local lockd UDP listener,
NFSv2/v3 lock recovery from Linux NFS clients always fails.

Revert parts of commit 24e36663 so lockd_up() always starts both
listeners.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Neil Brown <neilb@suse.de>
---

 fs/lockd/svc.c |   37 +++++++++++++++++++------------------
 1 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 1553fec..583d30d 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -203,25 +203,28 @@ lockd(void *vrqstp)
 }
 
 /*
- * Make any sockets that are needed but not present.
- * If nlm_udpport or nlm_tcpport were set as module
- * options, make those sockets unconditionally
+ * Ensure there are active UDP and TCP listeners for lockd.
+ *
+ * Even if we have only TCP NFS mounts and/or TCP NFSDs, some
+ * local services (such as rpc.statd) still require UDP, and
+ * some NFS servers do not yet support NLM over TCP.
+ *
+ * Returns zero if all listeners are available; otherwise a
+ * negative errno value is returned.
  */
-static int make_socks(struct svc_serv *serv, int proto)
+static int make_socks(struct svc_serv *serv)
 {
 	static int warned;
 	struct svc_xprt *xprt;
 	int err = 0;
 
-	if (proto == IPPROTO_UDP || nlm_udpport) {
-		xprt = svc_find_xprt(serv, "udp", 0, 0);
-		if (!xprt)
-			err = svc_create_xprt(serv, "udp", nlm_udpport,
-					      SVC_SOCK_DEFAULTS);
-		else
-			svc_xprt_put(xprt);
-	}
-	if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport)) {
+	xprt = svc_find_xprt(serv, "udp", 0, 0);
+	if (!xprt)
+		err = svc_create_xprt(serv, "udp", nlm_udpport,
+				      SVC_SOCK_DEFAULTS);
+	else
+		svc_xprt_put(xprt);
+	if (err >= 0) {
 		xprt = svc_find_xprt(serv, "tcp", 0, 0);
 		if (!xprt)
 			err = svc_create_xprt(serv, "tcp", nlm_tcpport,
@@ -251,11 +254,8 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 	/*
 	 * Check whether we're already up and running.
 	 */
-	if (nlmsvc_rqst) {
-		if (proto)
-			error = make_socks(nlmsvc_rqst->rq_server, proto);
+	if (nlmsvc_rqst)
 		goto out;
-	}
 
 	/*
 	 * Sanity check: if there's no pid,
@@ -272,7 +272,8 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
 		goto out;
 	}
 
-	if ((error = make_socks(serv, proto)) < 0)
+	error = make_socks(serv);
+	if (error < 0)
 		goto destroy_and_out;
 
 	/*


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] NLM: Remove "proto" argument from lockd_up()
       [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
  2008-10-03 21:15   ` [PATCH 1/3] NLM: Always start both UDP and TCP listeners Chuck Lever
@ 2008-10-03 21:15   ` Chuck Lever
  2008-10-03 21:15   ` [PATCH 3/3] NLM: Remove unused argument from svc_addsock() function Chuck Lever
  2008-10-04 21:36   ` [PATCH 0/3] client-side lockd doesn't start UDP listener J. Bruce Fields
  3 siblings, 0 replies; 14+ messages in thread
From: Chuck Lever @ 2008-10-03 21:15 UTC (permalink / raw)
  To: bfields, neilb; +Cc: linux-nfs

Clean up: Now that lockd_up() starts listeners for both transports, the
"proto" argument is no longer needed.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Neil Brown <neilb@suse.de>
---

 fs/lockd/clntlock.c        |    4 ++--
 fs/lockd/svc.c             |    3 +--
 fs/nfsd/nfsctl.c           |    5 ++---
 fs/nfsd/nfssvc.c           |   19 +++++++------------
 include/linux/lockd/bind.h |    2 +-
 5 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/fs/lockd/clntlock.c b/fs/lockd/clntlock.c
index 2976bf0..8307dd6 100644
--- a/fs/lockd/clntlock.c
+++ b/fs/lockd/clntlock.c
@@ -54,7 +54,7 @@ struct nlm_host *nlmclnt_init(const struct nlmclnt_initdata *nlm_init)
 	u32 nlm_version = (nlm_init->nfs_version == 2) ? 1 : 4;
 	int status;
 
-	status = lockd_up(nlm_init->protocol);
+	status = lockd_up();
 	if (status < 0)
 		return ERR_PTR(status);
 
@@ -215,7 +215,7 @@ reclaimer(void *ptr)
 	/* This one ensures that our parent doesn't terminate while the
 	 * reclaim is in progress */
 	lock_kernel();
-	lockd_up(0); /* note: this cannot fail as lockd is already running */
+	lockd_up();	/* note: this cannot fail as lockd is already running */
 
 	dprintk("lockd: reclaiming locks for host %s\n", host->h_name);
 
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 583d30d..ea9cfa4 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -244,8 +244,7 @@ static int make_socks(struct svc_serv *serv)
 /*
  * Bring up the lockd process if it's not already up.
  */
-int
-lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
+int lockd_up(void)
 {
 	struct svc_serv *serv;
 	int		error = 0;
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index c53e65f..862dff5 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -614,10 +614,9 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size)
 			return -EINVAL;
 		err = nfsd_create_serv();
 		if (!err) {
-			int proto = 0;
-			err = svc_addsock(nfsd_serv, fd, buf, &proto);
+			err = svc_addsock(nfsd_serv, fd, buf, NULL);
 			if (err >= 0) {
-				err = lockd_up(proto);
+				err = lockd_up();
 				if (err < 0)
 					svc_sock_names(buf+strlen(buf)+1, nfsd_serv, buf);
 			}
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 4eed938..052325a 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -244,25 +244,20 @@ static int nfsd_init_socks(int port)
 	if (!list_empty(&nfsd_serv->sv_permsocks))
 		return 0;
 
-	error = lockd_up(IPPROTO_UDP);
-	if (error >= 0) {
-		error = svc_create_xprt(nfsd_serv, "udp", port,
+	error = svc_create_xprt(nfsd_serv, "udp", port,
 					SVC_SOCK_DEFAULTS);
-		if (error < 0)
-			lockd_down();
-	}
 	if (error < 0)
 		return error;
 
-	error = lockd_up(IPPROTO_TCP);
-	if (error >= 0) {
-		error = svc_create_xprt(nfsd_serv, "tcp", port,
+	error = svc_create_xprt(nfsd_serv, "tcp", port,
 					SVC_SOCK_DEFAULTS);
-		if (error < 0)
-			lockd_down();
-	}
 	if (error < 0)
 		return error;
+
+	error = lockd_up();
+	if (error < 0)
+		return error;
+
 	return 0;
 }
 
diff --git a/include/linux/lockd/bind.h b/include/linux/lockd/bind.h
index 3d25bcd..7fa42fd 100644
--- a/include/linux/lockd/bind.h
+++ b/include/linux/lockd/bind.h
@@ -53,7 +53,7 @@ extern void	nlmclnt_done(struct nlm_host *host);
 
 extern int	nlmclnt_proc(struct nlm_host *host, int cmd,
 					struct file_lock *fl);
-extern int	lockd_up(int proto);
+extern int	lockd_up(void);
 extern void	lockd_down(void);
 
 unsigned long get_nfs_grace_period(void);


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] NLM: Remove unused argument from svc_addsock() function
       [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
  2008-10-03 21:15   ` [PATCH 1/3] NLM: Always start both UDP and TCP listeners Chuck Lever
  2008-10-03 21:15   ` [PATCH 2/3] NLM: Remove "proto" argument from lockd_up() Chuck Lever
@ 2008-10-03 21:15   ` Chuck Lever
  2008-10-04 21:36   ` [PATCH 0/3] client-side lockd doesn't start UDP listener J. Bruce Fields
  3 siblings, 0 replies; 14+ messages in thread
From: Chuck Lever @ 2008-10-03 21:15 UTC (permalink / raw)
  To: bfields, neilb; +Cc: linux-nfs

Clean up: The svc_addsock() function no longer uses its "proto"
argument, so remove it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Neil Brown <neilb@suse.de>
---

 fs/nfsd/nfsctl.c               |    2 +-
 include/linux/sunrpc/svcsock.h |    5 +----
 net/sunrpc/svcsock.c           |    4 +---
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 862dff5..97543df 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -614,7 +614,7 @@ static ssize_t __write_ports(struct file *file, char *buf, size_t size)
 			return -EINVAL;
 		err = nfsd_create_serv();
 		if (!err) {
-			err = svc_addsock(nfsd_serv, fd, buf, NULL);
+			err = svc_addsock(nfsd_serv, fd, buf);
 			if (err >= 0) {
 				err = lockd_up();
 				if (err < 0)
diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
index 8cff696..483e103 100644
--- a/include/linux/sunrpc/svcsock.h
+++ b/include/linux/sunrpc/svcsock.h
@@ -39,10 +39,7 @@ int		svc_send(struct svc_rqst *);
 void		svc_drop(struct svc_rqst *);
 void		svc_sock_update_bufs(struct svc_serv *serv);
 int		svc_sock_names(char *buf, struct svc_serv *serv, char *toclose);
-int		svc_addsock(struct svc_serv *serv,
-			    int fd,
-			    char *name_return,
-			    int *proto);
+int		svc_addsock(struct svc_serv *serv, int fd, char *name_return);
 void		svc_init_xprt_sock(void);
 void		svc_cleanup_xprt_sock(void);
 
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index f91377c..95293f5 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1167,8 +1167,7 @@ static struct svc_sock *svc_setup_socket(struct svc_serv *serv,
 
 int svc_addsock(struct svc_serv *serv,
 		int fd,
-		char *name_return,
-		int *proto)
+		char *name_return)
 {
 	int err = 0;
 	struct socket *so = sockfd_lookup(fd, &err);
@@ -1203,7 +1202,6 @@ int svc_addsock(struct svc_serv *serv,
 		sockfd_put(so);
 		return err;
 	}
-	if (proto) *proto = so->sk->sk_protocol;
 	return one_sock_name(name_return, svsk);
 }
 EXPORT_SYMBOL_GPL(svc_addsock);


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
       [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
                     ` (2 preceding siblings ...)
  2008-10-03 21:15   ` [PATCH 3/3] NLM: Remove unused argument from svc_addsock() function Chuck Lever
@ 2008-10-04 21:36   ` J. Bruce Fields
  2008-10-12 23:15     ` Neil Brown
  3 siblings, 1 reply; 14+ messages in thread
From: J. Bruce Fields @ 2008-10-04 21:36 UTC (permalink / raw)
  To: Chuck Lever; +Cc: neilb, linux-nfs

On Fri, Oct 03, 2008 at 05:15:14PM -0400, Chuck Lever wrote:
> Hi Bruce, Neil-
> 
> Here's my initial proposal to address the NFSv2/v3 lock recovery issue 
> that results from having no UDP lockd listener.
> 
> Comments?  Did I miss anything?

Looks fine; I can't see any problem.  So I've applied to for-2.6.28.
(An ack from Neil would be reassuring, though, if he gets a chance.)

--b.

> 
> ---
> 
> Chuck Lever (3):
>       NLM: Remove unused argument from svc_addsock() function
>       NLM: Remove "proto" argument from lockd_up()
>       NLM: Always start both UDP and TCP listeners
> 
>  fs/lockd/clntlock.c            |    4 ++--
>  fs/lockd/svc.c                 |   40 ++++++++++++++++++++--------------------
>  fs/nfsd/nfsctl.c               |    5 ++---
>  fs/nfsd/nfssvc.c               |   19 +++++++------------
>  include/linux/lockd/bind.h     |    2 +-
>  include/linux/sunrpc/svcsock.h |    5 +----
>  net/sunrpc/svcsock.c           |    4 +---
>  7 files changed, 34 insertions(+), 45 deletions(-)
> 
> -- 
> Chuck Lever

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-04 21:36   ` [PATCH 0/3] client-side lockd doesn't start UDP listener J. Bruce Fields
@ 2008-10-12 23:15     ` Neil Brown
       [not found]       ` <18674.34066.316972.695627-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
  0 siblings, 1 reply; 14+ messages in thread
From: Neil Brown @ 2008-10-12 23:15 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Chuck Lever, linux-nfs

On Saturday October 4, bfields@fieldses.org wrote:
> On Fri, Oct 03, 2008 at 05:15:14PM -0400, Chuck Lever wrote:
> > Hi Bruce, Neil-
> > 
> > Here's my initial proposal to address the NFSv2/v3 lock recovery issue 
> > that results from having no UDP lockd listener.
> > 
> > Comments?  Did I miss anything?
> 
> Looks fine; I can't see any problem.  So I've applied to for-2.6.28.
> (An ack from Neil would be reassuring, though, if he gets a chance.)

Sorry for my tardiness.  September was a very hectic month for me.

One consequence of this change is that lockd always listens on TCP
even if NFSD and NFS are only using UDP.
Do we care?  I suspect not.

An alternative fix to the problem would be to always listen on UDP and
only listen on TCP if it was requested.  This would probably be a
smaller code change, and might in some sense be more flexible.

Will we ever want to talk NLM over any other protocol? RDMA?
Is UDP6 a different protocol in this context?  I suspect not.

I guess I have a small leaning towards just always listening on UDP
and leaving everything else the same, but it is small, not strong.  So
unless either of you lean the same way I'm happy to give my

 Acked-By: NeilBrown <neilb@suse.de>

to the current patches.

Thanks,
NeilBrown

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
       [not found]       ` <18674.34066.316972.695627-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
@ 2008-10-13 15:42         ` Chuck Lever
  2008-10-13 17:46           ` J. Bruce Fields
  0 siblings, 1 reply; 14+ messages in thread
From: Chuck Lever @ 2008-10-13 15:42 UTC (permalink / raw)
  To: Neil Brown; +Cc: J. Bruce Fields, linux-nfs

Hi Neil-

On Oct 12, 2008, at Oct 12, 2008, 7:15 PM, Neil Brown wrote:
> On Saturday October 4, bfields@fieldses.org wrote:
>> On Fri, Oct 03, 2008 at 05:15:14PM -0400, Chuck Lever wrote:
>>> Hi Bruce, Neil-
>>>
>>> Here's my initial proposal to address the NFSv2/v3 lock recovery  
>>> issue
>>> that results from having no UDP lockd listener.
>>>
>>> Comments?  Did I miss anything?
>>
>> Looks fine; I can't see any problem.  So I've applied to for-2.6.28.
>> (An ack from Neil would be reassuring, though, if he gets a chance.)
>
> Sorry for my tardiness.  September was a very hectic month for me.
>
> One consequence of this change is that lockd always listens on TCP
> even if NFSD and NFS are only using UDP.
> Do we care?  I suspect not.

The server side usually has to start both anyway, so I thought making  
both sides work the same way was slightly nicer than keeping the  
"proto" argument to lockd_up(), and the run-time cost on the client is  
fairly minimal.  Plus, the overall trend is away from NFS over UDP,  
and towards NFS over TCP.  UDP is legacy, and TCP is the common case,  
going forward, so it's likely the TCP listener will nearly always be  
running anyway.

However, do we care about the -T and -U options on rpc.nfsd affecting  
how server-side lockd works?  Maybe that is a valid reason to keep the  
"proto" argument to lockd_up().

> An alternative fix to the problem would be to always listen on UDP and
> only listen on TCP if it was requested.  This would probably be a
> smaller code change, and might in some sense be more flexible.
>
> Will we ever want to talk NLM over any other protocol? RDMA?
> Is UDP6 a different protocol in this context?  I suspect not.
>
> I guess I have a small leaning towards just always listening on UDP
> and leaving everything else the same, but it is small, not strong.  So
> unless either of you lean the same way I'm happy to give my
>
> Acked-By: NeilBrown <neilb@suse.de>
>
> to the current patches.

Thanks for the review.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-13 15:42         ` Chuck Lever
@ 2008-10-13 17:46           ` J. Bruce Fields
  2008-10-13 20:45             ` Chuck Lever
  0 siblings, 1 reply; 14+ messages in thread
From: J. Bruce Fields @ 2008-10-13 17:46 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Neil Brown, linux-nfs

On Mon, Oct 13, 2008 at 11:42:47AM -0400, Chuck Lever wrote:
> Hi Neil-
>
> On Oct 12, 2008, at Oct 12, 2008, 7:15 PM, Neil Brown wrote:
>> On Saturday October 4, bfields@fieldses.org wrote:
>>> On Fri, Oct 03, 2008 at 05:15:14PM -0400, Chuck Lever wrote:
>>>> Hi Bruce, Neil-
>>>>
>>>> Here's my initial proposal to address the NFSv2/v3 lock recovery  
>>>> issue
>>>> that results from having no UDP lockd listener.
>>>>
>>>> Comments?  Did I miss anything?
>>>
>>> Looks fine; I can't see any problem.  So I've applied to for-2.6.28.
>>> (An ack from Neil would be reassuring, though, if he gets a chance.)
>>
>> Sorry for my tardiness.  September was a very hectic month for me.
>>
>> One consequence of this change is that lockd always listens on TCP
>> even if NFSD and NFS are only using UDP.
>> Do we care?  I suspect not.
>
> The server side usually has to start both anyway, so I thought making  
> both sides work the same way was slightly nicer than keeping the "proto" 
> argument to lockd_up(), and the run-time cost on the client is fairly 
> minimal.  Plus, the overall trend is away from NFS over UDP, and towards 
> NFS over TCP.  UDP is legacy, and TCP is the common case, going forward, 
> so it's likely the TCP listener will nearly always be running anyway.
>
> However, do we care about the -T and -U options on rpc.nfsd affecting  
> how server-side lockd works?  Maybe that is a valid reason to keep the  
> "proto" argument to lockd_up().

We might at least want to fix the man page.

I suppose worst case scenarios would be:

	- a bug is found that affects lockd/tcp and not lockd/udp, and
	  people that used -T are affected when they needn't have been,
	  or
	- someone uses -T and only bothers to firewall the udp port?

Is there any evidence that anyone uses -U or -T?

--b.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-13 17:46           ` J. Bruce Fields
@ 2008-10-13 20:45             ` Chuck Lever
  2008-10-13 20:57               ` J. Bruce Fields
  2008-10-14  9:54               ` Neil Brown
  0 siblings, 2 replies; 14+ messages in thread
From: Chuck Lever @ 2008-10-13 20:45 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Neil Brown, linux-nfs

On Oct 13, 2008, at Oct 13, 2008, 1:46 PM, J. Bruce Fields wrote:
> On Mon, Oct 13, 2008 at 11:42:47AM -0400, Chuck Lever wrote:
>> Hi Neil-
>>
>> On Oct 12, 2008, at Oct 12, 2008, 7:15 PM, Neil Brown wrote:
>>> On Saturday October 4, bfields@fieldses.org wrote:
>>>> On Fri, Oct 03, 2008 at 05:15:14PM -0400, Chuck Lever wrote:
>>>>> Hi Bruce, Neil-
>>>>>
>>>>> Here's my initial proposal to address the NFSv2/v3 lock recovery
>>>>> issue
>>>>> that results from having no UDP lockd listener.
>>>>>
>>>>> Comments?  Did I miss anything?
>>>>
>>>> Looks fine; I can't see any problem.  So I've applied to  
>>>> for-2.6.28.
>>>> (An ack from Neil would be reassuring, though, if he gets a  
>>>> chance.)
>>>
>>> Sorry for my tardiness.  September was a very hectic month for me.
>>>
>>> One consequence of this change is that lockd always listens on TCP
>>> even if NFSD and NFS are only using UDP.
>>> Do we care?  I suspect not.
>>
>> The server side usually has to start both anyway, so I thought making
>> both sides work the same way was slightly nicer than keeping the  
>> "proto"
>> argument to lockd_up(), and the run-time cost on the client is fairly
>> minimal.  Plus, the overall trend is away from NFS over UDP, and  
>> towards
>> NFS over TCP.  UDP is legacy, and TCP is the common case, going  
>> forward,
>> so it's likely the TCP listener will nearly always be running anyway.
>>
>> However, do we care about the -T and -U options on rpc.nfsd affecting
>> how server-side lockd works?  Maybe that is a valid reason to keep  
>> the
>> "proto" argument to lockd_up().
>
> We might at least want to fix the man page.
>
> I suppose worst case scenarios would be:
>
> 	- a bug is found that affects lockd/tcp and not lockd/udp, and
> 	  people that used -T are affected when they needn't have been,
> 	  or
> 	- someone uses -T and only bothers to firewall the udp port?
>
> Is there any evidence that anyone uses -U or -T?

NFSD start-up code makes two separate and unconditional calls to  
lockd_up(): one for UDP and one for TCP.  So whether or not NFSD  
actually honors the "-T" and "-U" flags, lockd certainly does not  
honor them in current (unpatched) code.

In other words, the patches you've already reviewed shouldn't change  
the server's behavior around -T/-U.  Only the client side should be  
affected.

It looks like, prior to commit 24e36663, the client side started both  
lockd listeners no matter what type of mount was requested.

Additional review is welcome, but let's just move forward with what  
you've already got.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-13 20:45             ` Chuck Lever
@ 2008-10-13 20:57               ` J. Bruce Fields
  2008-10-14  9:54               ` Neil Brown
  1 sibling, 0 replies; 14+ messages in thread
From: J. Bruce Fields @ 2008-10-13 20:57 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Neil Brown, linux-nfs

On Mon, Oct 13, 2008 at 04:45:47PM -0400, Chuck Lever wrote:
> NFSD start-up code makes two separate and unconditional calls to  
> lockd_up(): one for UDP and one for TCP.  So whether or not NFSD  
> actually honors the "-T" and "-U" flags, lockd certainly does not honor 
> them in current (unpatched) code.
>
> In other words, the patches you've already reviewed shouldn't change the 
> server's behavior around -T/-U.  Only the client side should be  
> affected.
>
> It looks like, prior to commit 24e36663, the client side started both  
> lockd listeners no matter what type of mount was requested.
>
> Additional review is welcome, but let's just move forward with what  
> you've already got.

OK, sounds fine to me.

The rpc.nfsd man page could still use revision, though.

--b.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-13 20:45             ` Chuck Lever
  2008-10-13 20:57               ` J. Bruce Fields
@ 2008-10-14  9:54               ` Neil Brown
       [not found]                 ` <18676.27741.921124.742371-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
  1 sibling, 1 reply; 14+ messages in thread
From: Neil Brown @ 2008-10-14  9:54 UTC (permalink / raw)
  To: Chuck Lever; +Cc: J. Bruce Fields, linux-nfs

On Monday October 13, chuck.lever@oracle.com wrote:
> 
> NFSD start-up code makes two separate and unconditional calls to  
> lockd_up(): one for UDP and one for TCP.  So whether or not NFSD  
> actually honors the "-T" and "-U" flags, lockd certainly does not  
> honor them in current (unpatched) code.

Not exactly correct.  The calls are not unconditional.
I assume we are looking at nfsd_init_socks in nfssvc.c
Note the condition at the top:
	if (!list_empty(&nfsd_serv->sv_permsocks))
		return 0;

If a UDP port has previously been created by a write to "portlist",
then when the first thread is stared, no new sockets will be created,
and nfsd (and lockd) will just listen on UDP.

So your patches do make a real functional change which is more than
just "always listen on UDP".

I agree that the man page should reflect reality.

NeilBrown

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
       [not found]                 ` <18676.27741.921124.742371-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
@ 2008-10-14 15:52                   ` Chuck Lever
  2008-10-14 17:43                     ` J. Bruce Fields
  0 siblings, 1 reply; 14+ messages in thread
From: Chuck Lever @ 2008-10-14 15:52 UTC (permalink / raw)
  To: Neil Brown; +Cc: J. Bruce Fields, linux-nfs

On Oct 14, 2008, at Oct 14, 2008, 5:54 AM, Neil Brown wrote:
> On Monday October 13, chuck.lever@oracle.com wrote:
>>
>> NFSD start-up code makes two separate and unconditional calls to
>> lockd_up(): one for UDP and one for TCP.  So whether or not NFSD
>> actually honors the "-T" and "-U" flags, lockd certainly does not
>> honor them in current (unpatched) code.
>
> Not exactly correct.  The calls are not unconditional.
> I assume we are looking at nfsd_init_socks in nfssvc.c
> Note the condition at the top:
> 	if (!list_empty(&nfsd_serv->sv_permsocks))
> 		return 0;
>
> If a UDP port has previously been created by a write to "portlist",
> then when the first thread is stared, no new sockets will be created,
> and nfsd (and lockd) will just listen on UDP.
>
> So your patches do make a real functional change which is more than
> just "always listen on UDP".

OK, I'm already collecting a set of additional fixes and cleanups in  
this area for 2.6.29.  We can add this to the list.

Why are there two separate and undocumented kernel interfaces for  
starting NFSD?

> I agree that the man page should reflect reality.

Several people have said that, but no-one has suggested what specific  
parts of the man page are problematic.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-14 15:52                   ` Chuck Lever
@ 2008-10-14 17:43                     ` J. Bruce Fields
  2008-10-14 18:14                       ` Chuck Lever
  0 siblings, 1 reply; 14+ messages in thread
From: J. Bruce Fields @ 2008-10-14 17:43 UTC (permalink / raw)
  To: Chuck Lever; +Cc: Neil Brown, linux-nfs

On Tue, Oct 14, 2008 at 11:52:52AM -0400, Chuck Lever wrote:
> On Oct 14, 2008, at Oct 14, 2008, 5:54 AM, Neil Brown wrote:
>> I agree that the man page should reflect reality.
>
> Several people have said that, but no-one has suggested what specific  
> parts of the man page are problematic.

Look at, e.g., the text for "-T":
	Disable rpc.nfsd from accepting TCP connections from clients.

It should probably say lockd is unaffected.  (If that's what we want.)

Also the wording is a little imprecise, since rpc.nfsd itself doesn't
accept network connections.

--b.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] client-side lockd doesn't start UDP listener
  2008-10-14 17:43                     ` J. Bruce Fields
@ 2008-10-14 18:14                       ` Chuck Lever
  0 siblings, 0 replies; 14+ messages in thread
From: Chuck Lever @ 2008-10-14 18:14 UTC (permalink / raw)
  To: J. Bruce Fields; +Cc: Neil Brown, linux-nfs

On Oct 14, 2008, at Oct 14, 2008, 1:43 PM, J. Bruce Fields wrote:
> On Tue, Oct 14, 2008 at 11:52:52AM -0400, Chuck Lever wrote:
>> On Oct 14, 2008, at Oct 14, 2008, 5:54 AM, Neil Brown wrote:
>>> I agree that the man page should reflect reality.
>>
>> Several people have said that, but no-one has suggested what specific
>> parts of the man page are problematic.
>
> Look at, e.g., the text for "-T":
> 	Disable rpc.nfsd from accepting TCP connections from clients.
>
> It should probably say lockd is unaffected.  (If that's what we want.)

That's the problem.  I'm not sure how this should behave.  I don't  
know what the implications are of always having both lockd listeners  
on the server.

It seems to me the previous behavior must have been broken -- if -U is  
specified, rpc.nfsd prevents the creation of both lockd and nfsd  
listeners on UDP, thus we have the same lock recovery problem we have  
on the client.

So we must always have a lockd UDP listener on the server, at least.

I would like to understand why we have -T and -U, and whether users of  
these options might want lockd to reflect the setting of this option.   
Is it acceptable (does it make sense) to have both UDP and TCP  
listeners for lockd, while having just one listener for NFSD?

And I don't understand what is being accomplished with the check that  
prevents nfsd_init_socks() from starting additional listeners.  Neil  
did this recently enough that I'm hoping he remembers what the  
rationale was and then we can add a comment or two in fs/nfsd/nfsctl.c  
to help inform future generations.

> Also the wording is a little imprecise, since rpc.nfsd itself doesn't
> accept network connections.

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2008-10-14 18:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-10-03 21:15 [PATCH 0/3] client-side lockd doesn't start UDP listener Chuck Lever
     [not found] ` <20081003211305.9870.6835.stgit-07a7zB5ZJzbwdl/1UfZZQIVfYA8g3rJ/@public.gmane.org>
2008-10-03 21:15   ` [PATCH 1/3] NLM: Always start both UDP and TCP listeners Chuck Lever
2008-10-03 21:15   ` [PATCH 2/3] NLM: Remove "proto" argument from lockd_up() Chuck Lever
2008-10-03 21:15   ` [PATCH 3/3] NLM: Remove unused argument from svc_addsock() function Chuck Lever
2008-10-04 21:36   ` [PATCH 0/3] client-side lockd doesn't start UDP listener J. Bruce Fields
2008-10-12 23:15     ` Neil Brown
     [not found]       ` <18674.34066.316972.695627-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2008-10-13 15:42         ` Chuck Lever
2008-10-13 17:46           ` J. Bruce Fields
2008-10-13 20:45             ` Chuck Lever
2008-10-13 20:57               ` J. Bruce Fields
2008-10-14  9:54               ` Neil Brown
     [not found]                 ` <18676.27741.921124.742371-wvvUuzkyo1EYVZTmpyfIwg@public.gmane.org>
2008-10-14 15:52                   ` Chuck Lever
2008-10-14 17:43                     ` J. Bruce Fields
2008-10-14 18:14                       ` Chuck Lever

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.