[RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

This patchset implements cleanup that is primarily driven by Greg Bank's 
review of the transport switch. A summary follows:

- Fix the svc_create_xprt to properly allocate the temporary sockaddr storage
  necessary for IPv6.
 
- Fix the svc_find_xprt service to lock the sv_permsocks list

- Fix the svc_find_xprt service to handle the address family correctly
  when matching the port number on IPv6 connections.

Thanks Greg.

Other changes include:

- Use the svc_find_xprt service to support adding and removing knfsd transports
  by writing strings to the /proc/fs/nfs/portlist file.

Signed-off-by: Tom Tucker <tom@opengridcomputing.com>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[RFC, PATCH 1/3] svc: Change sockaddr to sockaddr_storage in svc_create_xprt

[Tom Tucker]

A sockaddr is too small to handle an IPv6 address. Change the 
newsin local variable to point to sockaddr_storage.

Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
---

 net/sunrpc/svcsock.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 4bb2d57..cc752b7 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1278,7 +1278,8 @@ svc_create_socket(struct svc_serv *serv,
 	int		error;
 	int		type;
 	char		buf[RPC_MAX_ADDRBUFLEN];
-	struct sockaddr	newsin;
+	struct sockaddr_storage addr;
+	struct sockaddr *newsin = (struct sockaddr *)&addr;
 	int		newlen;
 
 	dprintk("svc: svc_create_socket(%s, %d, %s)\n",
@@ -1305,7 +1306,7 @@ svc_create_socket(struct svc_serv *serv,
 		goto bummer;
 
 	newlen = len;
-	error = kernel_getsockname(sock, &newsin, &newlen);
+	error = kernel_getsockname(sock, newsin, &newlen);
 	if (error < 0)
 		goto bummer;
 
@@ -1315,7 +1316,7 @@ svc_create_socket(struct svc_serv *serv,
 	}
 
 	if ((svsk = svc_setup_socket(serv, sock, &error, flags)) != NULL) {
-		memcpy(&svsk->sk_xprt.xpt_local, &newsin, newlen);
+		memcpy(&svsk->sk_xprt.xpt_local, newsin, newlen);
 		svc_xprt_received(&svsk->sk_xprt);
 		return (struct svc_xprt *)svsk;
 	}

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[RFC,PATCH 2/3] svc: Fix bugs in svc_find_xprt

[Tom Tucker]

The bug fixes are as follows:

- Verify the required arguments to the function.
- Change the address family wildcard to the more type-friendly AF_UNSPEC.
- Properly handle the address family when comparing ports
- The svc_find_xprt service needs a lock on the sv_permsocks list

Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
---

 fs/lockd/svc.c        |    4 ++--
 net/sunrpc/svc_xprt.c |   26 ++++++++++++++++----------
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 470af01..ee4a9bc 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -230,11 +230,11 @@ static int make_socks(struct svc_serv *s
 	int err = 0;
 
 	if (proto == IPPROTO_UDP || nlm_udpport)
-		if (!svc_find_xprt(serv, "udp", 0, 0))
+		if (!svc_find_xprt(serv, "udp", AF_UNSPEC, 0))
 			err = svc_create_xprt(serv, "udp", nlm_udpport,
 					      SVC_SOCK_DEFAULTS);
 	if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport))
-		if (!svc_find_xprt(serv, "tcp", 0, 0))
+		if (!svc_find_xprt(serv, "tcp", AF_UNSPEC, 0))
 			err = svc_create_xprt(serv, "tcp", nlm_tcpport,
 					      SVC_SOCK_DEFAULTS);
 
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index d35195e..5fc8d41 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -960,25 +960,31 @@ static struct svc_deferred_req *svc_defe
  * connections/peer traffic from the specified transport class,
  * address family and port.
  *
- * Specifying 0 for the address family or port is a wildcard and will
- * match any transport with the firt transport with the same class
- * name active for the service.
+ * AF_UNSPEC for the address family or zero for a port
+ * number are wild cards.
  */
 struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
 			       int af, int port)
 {
-	struct svc_xprt *xprt = NULL;
+	struct svc_xprt *xprt;
+	struct svc_xprt *found = NULL;
+
+	/* Sanity check args */
+	if (!serv || !xcl_name)
+		return found;
+
+	spin_lock_bh(&serv->sv_lock);
 	list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
-		struct sockaddr_in *sin;
 		if (strcmp(xprt->xpt_class->xcl_name, xcl_name))
 			continue;
-		sin = (struct sockaddr_in *)&xprt->xpt_local;
-		if (af && sin->sin_family != af)
+		if (af != AF_UNSPEC && af != xprt->xpt_local.ss_family)
 			continue;
-		if (port && (sin->sin_port != port))
+		if (port && port != svc_local_port(xprt))
 			continue;
-		return xprt;
+		found = xprt;
+		break;
 	}
-	return NULL;
+	spin_unlock_bh(&serv->sv_lock);
+	return found;
 }
 EXPORT_SYMBOL_GPL(svc_find_xprt);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

[RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Tom Tucker]

This patch enhances the write_ports function as follows:

- Check if a server transport instance already exists before attempting to 
  create a new one, and 

- Implement the ability to remove a previously created server transport
  instance. 

Signed-off-by: Tom Tucker <tom@opengridcomputing.com>
---

 fs/nfsd/nfsctl.c      |   36 +++++++++++++++++++++++++++++++++---
 net/sunrpc/svc_xprt.c |    1 +
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 923b817..0dfc130 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -540,7 +540,7 @@ static ssize_t write_ports(struct file *
 		}
 		return err < 0 ? err : 0;
 	}
-	if (buf[0] == '-') {
+	if (buf[0] == '-' && isdigit(buf[1])) {
 		char *toclose = kstrdup(buf+1, GFP_KERNEL);
 		int len = 0;
 		if (!toclose)
@@ -557,16 +557,46 @@ static ssize_t write_ports(struct file *
 	/*
 	 * Add a transport listener by writing it's transport name
 	 */
-	if (isalnum(buf[0])) {
+	if (isalpha(buf[0])) {
 		int err;
 		char transport[16];
 		int port;
 		if (sscanf(buf, "%15s %4d", transport, &port) == 2) {
 			err = nfsd_create_serv();
-			if (!err)
+			if (!err) {
+				if (svc_find_xprt(nfsd_serv, transport,
+						  AF_UNSPEC, port))
+					return -EADDRINUSE;
+
 				err = svc_create_xprt(nfsd_serv,
 						      transport, port,
 						      SVC_SOCK_ANONYMOUS);
+			}
+			return err < 0 ? err : 0;
+		}
+	}
+	/*
+	 * Remove a transport by writing it's transport name and port number
+	 */
+	if (buf[0] == '-' && isalpha(buf[1])) {
+		struct svc_xprt *xprt;
+		int err = -EINVAL;
+		char transport[16];
+		int port;
+		if (sscanf(&buf[1], "%15s %4d", transport, &port) == 2) {
+			if (port == 0)
+				return -EINVAL;
+			lock_kernel();
+			if (nfsd_serv) {
+				xprt = svc_find_xprt(nfsd_serv, transport,
+						     AF_UNSPEC, port);
+				if (xprt) {
+					svc_close_xprt(xprt);
+					err = 0;
+				} else
+					err = -ENOENT;
+			}
+			unlock_kernel();
 			return err < 0 ? err : 0;
 		}
 	}
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 5fc8d41..cbf80b0 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -809,6 +809,7 @@ void svc_close_xprt(struct svc_xprt *xpr
 	clear_bit(XPT_BUSY, &xprt->xpt_flags);
 	svc_xprt_put(xprt);
 }
+EXPORT_SYMBOL_GPL(svc_close_xprt);
 
 void svc_close_all(struct list_head *xprt_list)
 {

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Neil Brown]

On Wednesday October 10, tom@opengridcomputing.com wrote:
>  		if (sscanf(buf, "%15s %4d", transport, &port) == 2) {

We should give a bit of thought to this parsing and future expansion.
Suppose we did want to pump more information into the kernel as has
been suggested (e.g. local IP address), we would want this code to
handle the new format correctly, probably by failing.

So maybe we really want

>  		if (sscanf(buf, "%15s %4d%c", transport, &port, &nl) == 2
	            && nl == '\n') {

or something like that???

BTW, I'm fairly happy with the state of this patch set and suggest
that we aim for 2.6.24-rc1.  I still want to read through the final
code with all patches applied, but I should be able to find time for
that in the next month or so.  I don't expect any significant issues.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 01:00:58PM +1000, Neil Brown wrote:
> On Wednesday October 10, tom@opengridcomputing.com wrote:
> >  		if (sscanf(buf, "%15s %4d", transport, &port) == 2) {
> 
> We should give a bit of thought to this parsing and future expansion.
> Suppose we did want to pump more information into the kernel as has
> been suggested (e.g. local IP address), we would want this code to
> handle the new format correctly, probably by failing.
> 
> So maybe we really want
> 
> >  		if (sscanf(buf, "%15s %4d%c", transport, &port, &nl) == 2
> 	            && nl == '\n') {
> 
> or something like that???
> 
> BTW, I'm fairly happy with the state of this patch set and suggest
> that we aim for 2.6.24-rc1.  I still want to read through the final
> code with all patches applied, but I should be able to find time for
> that in the next month or so.  I don't expect any significant issues.

It seems OK to me, and it would be nice to get this batch in and have
just the rdma stuff left to merge in 2.6.25.

But we're very short for time, and it appears that there's still some
work to be done.  Certainly any user interfaces need to be sorted out,
as we'll be stuck with them....

Other work to be done: these incremental fixes are helpful for reviewers
like Greg who are keeping close watch on the process, but we need to
submit a patch series that shows our changes with perfect hindsight, so
we need to combine these with the previous patches.  I did this with the
most recent two of Tom's 7-patch series; result here:

	git://linux-nfs.org/~bfields/linux.git server-xprt-switch

I didn't apply patch #6 since Greg still had comments, and I haven't
applied the most recent fixes.

If this isn't all ready by Friday (OK, maybe Monday) then I think we're
out of time.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Neil Brown]

On Wednesday October 10, bfields@fieldses.org wrote:
> 
> If this isn't all ready by Friday (OK, maybe Monday) then I think we're
> out of time.

Sorry, when I said "2.6.24-rc1" previously, I was failing to do
correct addition.  I meant 2.6.25-rc1.

I really don't think it is appropriate to submit these for 2.6.24.
And I cannot see any real need to do that.

Sorry for the confusion.
NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Greg Banks]

On Thu, Oct 11, 2007 at 03:09:44PM +1000, Neil Brown wrote:
> On Wednesday October 10, bfields@fieldses.org wrote:
> > 
> > If this isn't all ready by Friday (OK, maybe Monday) then I think we're
> > out of time.
> 
> Sorry, when I said "2.6.24-rc1" previously, I was failing to do
> correct addition.  I meant 2.6.25-rc1.
> 
> I really don't think it is appropriate to submit these for 2.6.24.
> And I cannot see any real need to do that.
> 
> Sorry for the confusion.

So we should be aiming for both the server xprt switch *and*
the rdma xprt for .25?

Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Neil Brown]

On Thursday October 11, gnb@sgi.com wrote:
> On Thu, Oct 11, 2007 at 03:09:44PM +1000, Neil Brown wrote:
> > On Wednesday October 10, bfields@fieldses.org wrote:
> > > 
> > > If this isn't all ready by Friday (OK, maybe Monday) then I think we're
> > > out of time.
> > 
> > Sorry, when I said "2.6.24-rc1" previously, I was failing to do
> > correct addition.  I meant 2.6.25-rc1.
> > 
> > I really don't think it is appropriate to submit these for 2.6.24.
> > And I cannot see any real need to do that.
> > 
> > Sorry for the confusion.
> 
> So we should be aiming for both the server xprt switch *and*
> the rdma xprt for .25?

Probably.  It is a while since I've seen the rdma xprt code so I don't
know what state it is in, but then I'm not sure that matters.
It would be new code that is not used if not enabled.  So adding it
would not be expected to affect stability.  It should be treated like
a new driver which - it has been suggested - can go in at any time.

NeilBrown

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 03:09:44PM +1000, Neil Brown wrote:
> On Wednesday October 10, bfields@fieldses.org wrote:
> > 
> > If this isn't all ready by Friday (OK, maybe Monday) then I think we're
> > out of time.
> 
> Sorry, when I said "2.6.24-rc1" previously, I was failing to do
> correct addition.  I meant 2.6.25-rc1.
>
> I really don't think it is appropriate to submit these for 2.6.24.
> And I cannot see any real need to do that.

OK.  That takes some pressure off!

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Tom Tucker]

Bruce:

What's the most efficient way for you to accept fixes/changes? I've been
going incremental because it's easy on me :-). Is this the best option for
you?

Regarding patch #6, I think the svc_find_xprt API _may_ be ok, but the
implementation was bogus (sorry). I _think_ the implementation is fixed in
the latest incremental. Greg can confirm.

WRT the suggestions below, I'll make those mods and test them tomorrow.

Thanks,
Tom

On 10/10/07 10:46 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Thu, Oct 11, 2007 at 01:00:58PM +1000, Neil Brown wrote:
>> On Wednesday October 10, tom@opengridcomputing.com wrote:
>>> if (sscanf(buf, "%15s %4d", transport, &port) == 2) {
>> 
>> We should give a bit of thought to this parsing and future expansion.
>> Suppose we did want to pump more information into the kernel as has
>> been suggested (e.g. local IP address), we would want this code to
>> handle the new format correctly, probably by failing.
>> 
>> So maybe we really want
>> 
>>> if (sscanf(buf, "%15s %4d%c", transport, &port, &nl) == 2
>>            && nl == '\n') {
>> 
>> or something like that???
>> 
>> BTW, I'm fairly happy with the state of this patch set and suggest
>> that we aim for 2.6.24-rc1.  I still want to read through the final
>> code with all patches applied, but I should be able to find time for
>> that in the next month or so.  I don't expect any significant issues.
> 
> It seems OK to me, and it would be nice to get this batch in and have
> just the rdma stuff left to merge in 2.6.25.
> 
> But we're very short for time, and it appears that there's still some
> work to be done.  Certainly any user interfaces need to be sorted out,
> as we'll be stuck with them....
> 
> Other work to be done: these incremental fixes are helpful for reviewers
> like Greg who are keeping close watch on the process, but we need to
> submit a patch series that shows our changes with perfect hindsight, so
> we need to combine these with the previous patches.  I did this with the
> most recent two of Tom's 7-patch series; result here:
> 
> git://linux-nfs.org/~bfields/linux.git server-xprt-switch
> 
> I didn't apply patch #6 since Greg still had comments, and I haven't
> applied the most recent fixes.
> 
> If this isn't all ready by Friday (OK, maybe Monday) then I think we're
> out of time.
> 
> --b.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 12:11:26AM -0500, Tom Tucker wrote:
> What's the most efficient way for you to accept fixes/changes? I've been
> going incremental because it's easy on me :-). Is this the best option for
> you?

Well, the problem is that there's two different histories we want, and
no tool really makes it easy (or perhaps can make it really easy) to
track them both:

	- There's the final cleaned-up series that we want submitted.
	- There's the incremental changes that are made to fix problems
	  in the original series.

A lot of people deal with this by just resubmitting the whole thing
(usually as a big mailbomb each time) with a changelog in the 0/n
message that explains what changed since the previous submission.

But in practice I think that means we lose some follow-up review because
it's harder for reviewers in the previous round to plow through the
whole series from the start each time.  And it looks like Greg is still
spotting some problems that might not have been otherwise.  So that's
working.

So for now I guess I'll try what seems to be Andrew's approach--take
incremental patches, then merge them in before the end.  If that turns
out to be too hard, I'll complain.

> Regarding patch #6, I think the svc_find_xprt API _may_ be ok, but the
> implementation was bogus (sorry). I _think_ the implementation is fixed in
> the latest incremental. Greg can confirm.

OK!

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[Tom Tucker]

On Thu, 2007-10-11 at 11:25 -0400, J. Bruce Fields wrote:
> On Thu, Oct 11, 2007 at 12:11:26AM -0500, Tom Tucker wrote:
> > What's the most efficient way for you to accept fixes/changes? I've been
> > going incremental because it's easy on me :-). Is this the best option for
> > you?
> 
> Well, the problem is that there's two different histories we want, and
> no tool really makes it easy (or perhaps can make it really easy) to
> track them both:
> 
> 	- There's the final cleaned-up series that we want submitted.
> 	- There's the incremental changes that are made to fix problems
> 	  in the original series.
> 
> A lot of people deal with this by just resubmitting the whole thing
> (usually as a big mailbomb each time) with a changelog in the 0/n
> message that explains what changed since the previous submission.
> 
> But in practice I think that means we lose some follow-up review because
> it's harder for reviewers in the previous round to plow through the
> whole series from the start each time.  And it looks like Greg is still
> spotting some problems that might not have been otherwise.  So that's
> working.
> 
> So for now I guess I'll try what seems to be Andrew's approach--take
> incremental patches, then merge them in before the end.  If that turns
> out to be too hard, I'll complain.

np. I've got a stacked git tree that has the whole set, so I can flatten
it or keep it at your discretion. 

> 
> > Regarding patch #6, I think the svc_find_xprt API _may_ be ok, but the
> > implementation was bogus (sorry). I _think_ the implementation is fixed in
> > the latest incremental. Greg can confirm.
> 
> OK!
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 3/3] knfsd: Modify write_ports to use svc_find_xprt service

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 10:27:08AM -0500, Tom Tucker wrote:
> On Thu, 2007-10-11 at 11:25 -0400, J. Bruce Fields wrote:
> > Well, the problem is that there's two different histories we want, and
> > no tool really makes it easy (or perhaps can make it really easy) to
> > track them both:
> > 
> > 	- There's the final cleaned-up series that we want submitted.
> > 	- There's the incremental changes that are made to fix problems
> > 	  in the original series.
> > 
> > A lot of people deal with this by just resubmitting the whole thing
> > (usually as a big mailbomb each time) with a changelog in the 0/n
> > message that explains what changed since the previous submission.
> > 
> > But in practice I think that means we lose some follow-up review because
> > it's harder for reviewers in the previous round to plow through the
> > whole series from the start each time.  And it looks like Greg is still
> > spotting some problems that might not have been otherwise.  So that's
> > working.
> > 
> > So for now I guess I'll try what seems to be Andrew's approach--take
> > incremental patches, then merge them in before the end.  If that turns
> > out to be too hard, I'll complain.
> 
> np. I've got a stacked git tree that has the whole set, so I can flatten
> it or keep it at your discretion. 

Well, if you want to be a total hero and do *both* the incremental
patches and also keep a series with the merged in as appropriate, that'd
be great.  So given a series of patches:

	1/3	clean up frobnicator
	2/3	introduce new frobnicator-next-generation api
	3/3	move users to frobnicator-next-generation

if you had an incremental fix, you could send it out like:

	0/1	"Oops, here's a fix for my last series.  I also have a
		fixed version of the series here: git://..."
	1/1	rename frobnicator-next-generation to frobnicator-new

and then git://... would have a series that looked like:

	1/3	clean up frobnicator
	2/3	introduce new frobnicator-new api
	3/3	move users to frobnicator-new

I'd both apply the incremental patch and fetch the modified series, diff
them to make sure they both got the same result, then take the modified
series and throw away the old one.

Anyway, for now I think I've caught up to your latest here:

	git://linux-nfs.org/~bfields	server-xprt-switch

mostly left separated out at this point, except the two I fooled with
yesterday.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Greg Banks]

G'day,

Compendium reply.

On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> This patchset implements cleanup that is primarily driven by Greg Bank's 
> review of the transport switch. A summary follows:
> 
> - Fix the svc_create_xprt to properly allocate the temporary sockaddr storage
>   necessary for IPv6.
>  
> - Fix the svc_find_xprt service to lock the sv_permsocks list
> 
> - Fix the svc_find_xprt service to handle the address family correctly
>   when matching the port number on IPv6 connections.
> 
> Thanks Greg.
> 
> Other changes include:
> 
> - Use the svc_find_xprt service to support adding and removing knfsd transports
>   by writing strings to the /proc/fs/nfs/portlist file.
> 

> Subject: [RFC,PATCH 1/3] svc: Change sockaddr to sockaddr_storage in
> svc_create_xprt

ok

> Subject: [RFC,PATCH 2/3] svc: Fix bugs in svc_find_xprt


ok

> Subject: [RFC,PATCH 3/3] knfsd: Modify write_ports to use
> svc_find_xprt service

ok by me, but Neil makes a good point about designing this interface
carefully.


Greg.
-- 
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere.  Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 03:25:10PM +1000, Greg Banks wrote:
> Compendium reply.
> 
> On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > Subject: [RFC,PATCH 3/3] knfsd: Modify write_ports to use
> > svc_find_xprt service
> 
> ok by me, but Neil makes a good point about designing this interface
> carefully.

The important point being that the kernel is required to stay backwards
compatible with older user utilities--we can't tell people "oh, if
you're upgrading from 2.6.25 to 2.6.26 and you want nfs/rdma to keep
working then you'll also have to upgrade to nfs-utils-1.3.0."

(With maybe some leeway if rdma is still in such an experimental state
that there aren't yet any real users....)

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> This patchset implements cleanup that is primarily driven by Greg Bank's 
> review of the transport switch. A summary follows:

With your latest patches (and tronds, and mine), I'm getting a failure
at the start of basic connectathon tests which I haven't tracked down
yet.  One obvious oddity in the network trace, though, is that the
server is retransmitting its reply to an open.  Is there any chance this
could be a problem with the deferral code?

I'll keep investigating.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

On Thu, 2007-10-11 at 16:30 -0400, J. Bruce Fields wrote:
> On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > This patchset implements cleanup that is primarily driven by Greg Bank's 
> > review of the transport switch. A summary follows:
> 
> With your latest patches (and tronds, and mine), I'm getting a failure
> at the start of basic connectathon tests which I haven't tracked down
> yet.  One obvious oddity in the network trace, though, is that the
> server is retransmitting its reply to an open.  Is there any chance this
> could be a problem with the deferral code?
> 
> I'll keep investigating.
> 

Hmm. It looks like something's amiss with v4 still. V3 seems to work
fine for me. Maybe my callback fix didn't take... I'll look here as
well.

FYI, for my tests, I used an old client to mount, build the cthon04
tests in the mounted dir and then run them from the mounted dir.


> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

FYI:

I found by inspection an issue where multiple deferral of the same
request breaks...

Tom

On Thu, 2007-10-11 at 16:30 -0400, J. Bruce Fields wrote:
> On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > This patchset implements cleanup that is primarily driven by Greg Bank's 
> > review of the transport switch. A summary follows:
> 
> With your latest patches (and tronds, and mine), I'm getting a failure
> at the start of basic connectathon tests which I haven't tracked down
> yet.  One obvious oddity in the network trace, though, is that the
> server is retransmitting its reply to an open.  Is there any chance this
> could be a problem with the deferral code?
> 
> I'll keep investigating.
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

I think I found the problem. The patchset is not caching the auth data
properly. This causes the server to fail the setting of the client id
with a 10018 error.

I'll send a patch off tonight....sorry :-\

On Thu, 2007-10-11 at 16:30 -0400, J. Bruce Fields wrote:
> On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > This patchset implements cleanup that is primarily driven by Greg Bank's 
> > review of the transport switch. A summary follows:
> 
> With your latest patches (and tronds, and mine), I'm getting a failure
> at the start of basic connectathon tests which I haven't tracked down
> yet.  One obvious oddity in the network trace, though, is that the
> server is retransmitting its reply to an open.  Is there any chance this
> could be a problem with the deferral code?
> 
> I'll keep investigating.
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Thu, Oct 11, 2007 at 05:04:42PM -0500, Tom Tucker wrote:
> I think I found the problem. The patchset is not caching the auth data
> properly. This causes the server to fail the setting of the client id
> with a 10018 error.
> 
> I'll send a patch off tonight....sorry :-\

OK, thanks!

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

On Thu, 2007-10-11 at 18:08 -0400, J. Bruce Fields wrote:
> On Thu, Oct 11, 2007 at 05:04:42PM -0500, Tom Tucker wrote:
> > I think I found the problem. The patchset is not caching the auth data
> > properly. This causes the server to fail the setting of the client id
> > with a 10018 error.
> > 
> > I'll send a patch off tonight....sorry :-\

A quick update. The problem is not what I thought it was, but I did
discover some interesting things:

- If you wait 60 seconds after the nfs4 mount, everything works fine.
You can run connectathon and the world is happy. Before the 60 seconds
you get the hang. Curiously, there are connection management timers in
the client that have this value.

- It looks like the module load logic that is down in the
crypto_alloc_hash path is not working properly. Maybe the module name
changed or something, or maybe it always worked this way and I'm
confused, but you have to manually load md5 for nfs4 to work

- It looks like the callback path is still broken. It's not the port
number, it's the IP address. Since I moved the IP address, it's likely
that I'm the culprit. 

I'll keep you updated,
Tom

> 
> OK, thanks!
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Fri, Oct 12, 2007 at 10:17:14AM -0500, Tom Tucker wrote:
> On Thu, 2007-10-11 at 18:08 -0400, J. Bruce Fields wrote:
> > On Thu, Oct 11, 2007 at 05:04:42PM -0500, Tom Tucker wrote:
> > > I think I found the problem. The patchset is not caching the auth data
> > > properly. This causes the server to fail the setting of the client id
> > > with a 10018 error.
> > > 
> > > I'll send a patch off tonight....sorry :-\
> 
> A quick update. The problem is not what I thought it was, but I did
> discover some interesting things:
> 
> - If you wait 60 seconds after the nfs4 mount, everything works fine.
> You can run connectathon and the world is happy. Before the 60 seconds
> you get the hang. Curiously, there are connection management timers in
> the client that have this value.

Oh, well that may be just the grace period--we don't generally allow any
opens for the first lease time (45 seconds by default, I think?)

(We should be smarter and skip the grace period in cases where we know
no former clients held state on the server.  I'm hoping that'll be fixed
in the next month.)

> - It looks like the module load logic that is down in the
> crypto_alloc_hash path is not working properly. Maybe the module name
> changed or something, or maybe it always worked this way and I'm
> confused, but you have to manually load md5 for nfs4 to work
> 
> - It looks like the callback path is still broken. It's not the port
> number, it's the IP address. Since I moved the IP address, it's likely
> that I'm the culprit. 

OK, thanks.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

We'll I've come full circle. The bad ip address that was causing the
callback to fail was coming from the client. If I do the mount as
follows:

# mount -t nfs4 -o clientaddr=<correct_addresss> nfssrv:/ /pnfs

Everything works fine and Connectathon runs as expected. I don't know
what the deal is with the mount command, but I'm not using the latest
and greatest for sure.

Can you do me a huge favor and see if you get the same behavior on your
end?  

I did find a few things to clean up, but it works for me with or without
the cleanup. I'll post the clean ups shortly.

Tom

On Fri, 2007-10-12 at 11:41 -0400, J. Bruce Fields wrote:
> On Fri, Oct 12, 2007 at 10:17:14AM -0500, Tom Tucker wrote:
> > On Thu, 2007-10-11 at 18:08 -0400, J. Bruce Fields wrote:
> > > On Thu, Oct 11, 2007 at 05:04:42PM -0500, Tom Tucker wrote:
> > > > I think I found the problem. The patchset is not caching the auth data
> > > > properly. This causes the server to fail the setting of the client id
> > > > with a 10018 error.
> > > > 
> > > > I'll send a patch off tonight....sorry :-\
> > 
> > A quick update. The problem is not what I thought it was, but I did
> > discover some interesting things:
> > 
> > - If you wait 60 seconds after the nfs4 mount, everything works fine.
> > You can run connectathon and the world is happy. Before the 60 seconds
> > you get the hang. Curiously, there are connection management timers in
> > the client that have this value.
> 
> Oh, well that may be just the grace period--we don't generally allow any
> opens for the first lease time (45 seconds by default, I think?)
> 
> (We should be smarter and skip the grace period in cases where we know
> no former clients held state on the server.  I'm hoping that'll be fixed
> in the next month.)
> 
> > - It looks like the module load logic that is down in the
> > crypto_alloc_hash path is not working properly. Maybe the module name
> > changed or something, or maybe it always worked this way and I'm
> > confused, but you have to manually load md5 for nfs4 to work
> > 
> > - It looks like the callback path is still broken. It's not the port
> > number, it's the IP address. Since I moved the IP address, it's likely
> > that I'm the culprit. 
> 
> OK, thanks.
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

On Thu, 2007-10-11 at 16:30 -0400, J. Bruce Fields wrote:
> On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > This patchset implements cleanup that is primarily driven by Greg Bank's 
> > review of the transport switch. A summary follows:
> 
> With your latest patches (and tronds, and mine), I'm getting a failure
> at the start of basic connectathon tests which I haven't tracked down
> yet.  One obvious oddity in the network trace, though, is that the
> server is retransmitting its reply to an open.  Is there any chance this
> could be a problem with the deferral code?
> 

Bruce:

Do you have this tree anywhere I could clone it and try and figure out
what's going on?

Thanks,
Tom
> I'll keep investigating.
> 
> --b.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Fri, Oct 12, 2007 at 04:40:26PM -0500, Tom Tucker wrote:
> On Thu, 2007-10-11 at 16:30 -0400, J. Bruce Fields wrote:
> > On Wed, Oct 10, 2007 at 09:23:56PM -0500, Tom Tucker wrote:
> > > This patchset implements cleanup that is primarily driven by Greg Bank's 
> > > review of the transport switch. A summary follows:
> > 
> > With your latest patches (and tronds, and mine), I'm getting a failure
> > at the start of basic connectathon tests which I haven't tracked down
> > yet.  One obvious oddity in the network trace, though, is that the
> > server is retransmitting its reply to an open.  Is there any chance this
> > could be a problem with the deferral code?
> > 
> 
> Bruce:
> 
> Do you have this tree anywhere I could clone it and try and figure out
> what's going on?

Sorry, yes, it's currently in "master" in my git tree:

	git://linux-nfs.org/~bfields/linux.git master

I believe it's got everything you've sent me so far except the three
patches from today.  I haven't looked at it again since yesterday.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Fri, Oct 12, 2007 at 06:03:00PM -0400, bfields wrote:
> Sorry, yes, it's currently in "master" in my git tree:
> 
> 	git://linux-nfs.org/~bfields/linux.git master
> 
> I believe it's got everything you've sent me so far except the three
> patches from today.  I haven't looked at it again since yesterday.

Bah, don't spend any time on this.  I took another look, and it seems
the problem is in the exported filesystem itself, not in the server.  I
think I just screwed up when I set up this test machine; I'll retest and
update my tree assuming it all looks good.  Sorry for the false alarm.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[J. Bruce Fields]

On Fri, Oct 12, 2007 at 06:30:00PM -0400, bfields wrote:
> On Fri, Oct 12, 2007 at 06:03:00PM -0400, bfields wrote:
> > Sorry, yes, it's currently in "master" in my git tree:
> > 
> > 	git://linux-nfs.org/~bfields/linux.git master
> > 
> > I believe it's got everything you've sent me so far except the three
> > patches from today.  I haven't looked at it again since yesterday.
> 
> Bah, don't spend any time on this.  I took another look, and it seems
> the problem is in the exported filesystem itself, not in the server.

Yeah, there were some filesystem errors in the logs, and after
reformatting that partition I can't reproduce the problem.  I think that
one machine may just have an unreliable drive.

--b.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [RFC, PATCH 0/3] svc: gnb review cleanup for transport switch

[Tom Tucker]

On 10/12/07 10:48 PM, "J. Bruce Fields" <bfields@fieldses.org> wrote:

> On Fri, Oct 12, 2007 at 06:30:00PM -0400, bfields wrote:
>> On Fri, Oct 12, 2007 at 06:03:00PM -0400, bfields wrote:
>>> Sorry, yes, it's currently in "master" in my git tree:
>>> 
>>> git://linux-nfs.org/~bfields/linux.git master
>>> 
>>> I believe it's got everything you've sent me so far except the three
>>> patches from today.  I haven't looked at it again since yesterday.
>> 
>> Bah, don't spend any time on this.  I took another look, and it seems
>> the problem is in the exported filesystem itself, not in the server.
> 
> Yeah, there were some filesystem errors in the logs, and after
> reformatting that partition I can't reproduce the problem.  I think that
> one machine may just have an unreliable drive.
>

No worries, I found three other bugs in the process :-)

Tom 
> --b.



-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs