[PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Chuck Lever]

Making debugging output a little cleaner in call_verify by displaying the
name of the RPC procedure instead of it's number.

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

 net/sunrpc/clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index adbc85c..5aa32fa 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
 		error = -EPROTONOSUPPORT;
 		goto out_err;
 	case RPC_PROC_UNAVAIL:
-		dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
+		dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
 				"version %u on server %s\n",
 				task->tk_pid, __func__,
-				task->tk_msg.rpc_proc,
+				task->tk_msg.rpc_proc->p_name,
 				task->tk_client->cl_prog,
 				task->tk_client->cl_vers,
 				task->tk_client->cl_server);

[PATCH 0/8] Initial set of 2.6.27 patches, take 2

[Chuck Lever]

Hi Trond-

Resending the first batch of 2.6.27 patches, with fixes.  The trailing
whitespace in fs/Kconfig has been removed, and the rpc_show_task() changes
place all output data on a single line for each task.  Patch descriptions have
been expanded to provide sample output.

---

Chuck Lever (8):
      SUNRPC: Display symbolic function addresses in rpc_show_tasks
      SUNRPC: Rename "call_" functions that are no longer FSM states
      SUNRPC: Display some debugging information as text rather than numbers
      SUNRPC: Refactor rpc_show_tasks
      SUNRPC: Don't display the rpc_show_tasks header if there are no tasks
      SUNRPC: Use RPC procedure name in call_verify
      SUNRPC: Use RPC procedure name in call_start
      NFS: Update help text for CONFIG_NFS_FS


 fs/Kconfig                   |  115 ++++++++++++++++-----------------
 include/linux/sunrpc/sched.h |    1 
 net/sunrpc/clnt.c            |  147 +++++++++++++++++++++++++++---------------
 net/sunrpc/sched.c           |    2 -
 4 files changed, 153 insertions(+), 112 deletions(-)

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

[PATCH 1/8] NFS: Update help text for CONFIG_NFS_FS

[Chuck Lever]

Clean up: refresh the help text for Kconfig items related to the NFS
client.  Remove obsolete URLs, and make the language consistent among
the options.

Also move the ROOT_NFS config option next to the options related to the
NFS client.

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

 fs/Kconfig |  115 ++++++++++++++++++++++++++++++------------------------------
 1 files changed, 57 insertions(+), 58 deletions(-)


diff --git a/fs/Kconfig b/fs/Kconfig
index cf12c40..07a61f5 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -1544,10 +1544,6 @@ config UFS_FS
           The recently released UFS2 variant (used in FreeBSD 5.x) is
           READ-ONLY supported.
 
-	  If you only intend to mount files from some other Unix over the
-	  network using NFS, you don't need the UFS file system support (but
-	  you need NFS file system support obviously).
-
 	  Note that this option is generally not needed for floppies, since a
 	  good portable way to transport files and directories between unixes
 	  (and even other operating systems) is given by the tar program ("man
@@ -1587,6 +1583,7 @@ menuconfig NETWORK_FILESYSTEMS
 	  Say Y here to get to see options for network filesystems and
 	  filesystem-related networking code, such as NFS daemon and
 	  RPCSEC security modules.
+
 	  This option alone does not add any kernel code.
 
 	  If you say N, all options in this submenu will be skipped and
@@ -1595,76 +1592,92 @@ menuconfig NETWORK_FILESYSTEMS
 if NETWORK_FILESYSTEMS
 
 config NFS_FS
-	tristate "NFS file system support"
+	tristate "NFS client support"
 	depends on INET
 	select LOCKD
 	select SUNRPC
 	select NFS_ACL_SUPPORT if NFS_V3_ACL
 	help
-	  If you are connected to some other (usually local) Unix computer
-	  (using SLIP, PLIP, PPP or Ethernet) and want to mount files residing
-	  on that computer (the NFS server) using the Network File Sharing
-	  protocol, say Y. "Mounting files" means that the client can access
-	  the files with usual UNIX commands as if they were sitting on the
-	  client's hard disk. For this to work, the server must run the
-	  programs nfsd and mountd (but does not need to have NFS file system
-	  support enabled in its kernel). NFS is explained in the Network
-	  Administrator's Guide, available from
-	  <http://www.tldp.org/docs.html#guide>, on its man page: "man
-	  nfs", and in the NFS-HOWTO.
-
-	  A superior but less widely used alternative to NFS is provided by
-	  the Coda file system; see "Coda file system support" below.
+	  Choose Y here if you want to access files residing on other
+	  computers using Sun's Network File System protocol.  To compile
+	  this file system support as a module, choose M here: the module
+	  will be called nfs.
 
-	  If you say Y here, you should have said Y to TCP/IP networking also.
-	  This option would enlarge your kernel by about 27 KB.
+	  To mount file systems exported by NFS servers, you also need to
+	  install the user space mount.nfs command which can be found in
+	  the Linux nfs-utils package, available from http://linux-nfs.org/.
+	  Information about using the mount command is available in the
+	  mount(8) man page.  More detail about the Linux NFS client
+	  implementation is available via the nfs(5) man page.
 
-	  To compile this file system support as a module, choose M here: the
-	  module will be called nfs.
+	  Below you can choose which versions of the NFS protocol are
+	  available in the kernel to mount NFS servers.  Support for NFS
+	  version 2 (RFC 1094) is always available when NFS_FS is selected.
 
-	  If you are configuring a diskless machine which will mount its root
-	  file system over NFS at boot time, say Y here and to "Kernel
-	  level IP autoconfiguration" above and to "Root file system on NFS"
-	  below. You cannot compile this driver as a module in this case.
-	  There are two packages designed for booting diskless machines over
-	  the net: netboot, available from
-	  <http://ftp1.sourceforge.net/netboot/>, and Etherboot,
-	  available from <http://ftp1.sourceforge.net/etherboot/>.
+	  To configure a system which mounts its root file system via NFS
+	  at boot time, say Y here, select "Kernel level IP
+	  autoconfiguration" in the NETWORK menu, and select "Root file
+	  system on NFS" below.  You cannot compile this file system as a
+	  module in this case.
 
-	  If you don't know what all this is about, say N.
+	  If unsure, say N.
 
 config NFS_V3
-	bool "Provide NFSv3 client support"
+	bool "NFS client support for NFS version 3"
 	depends on NFS_FS
 	help
-	  Say Y here if you want your NFS client to be able to speak version
-	  3 of the NFS protocol.
+	  This option enables support for version 3 of the NFS protocol
+	  (RFC 1813) in the kernel's NFS client.
 
 	  If unsure, say Y.
 
 config NFS_V3_ACL
-	bool "Provide client support for the NFSv3 ACL protocol extension"
+	bool "NFS client support for the NFSv3 ACL protocol extension"
 	depends on NFS_V3
 	help
-	  Implement the NFSv3 ACL protocol extension for manipulating POSIX
-	  Access Control Lists.  The server should also be compiled with
-	  the NFSv3 ACL protocol extension; see the CONFIG_NFSD_V3_ACL option.
+	  Some NFS servers support an auxiliary NFSv3 ACL protocol that
+	  Sun added to Solaris but never became an official part of the
+	  NFS version 3 protocol.  This protocol extension allows
+	  applications on NFS clients to manipulate POSIX Access Control
+	  Lists on files residing on NFS servers.  NFS servers enforce
+	  ACLs on local files whether this protocol is available or not.
+
+	  Choose Y here if your NFS server supports the Solaris NFSv3 ACL
+	  protocol extension and you want your NFS client to allow
+	  applications to access and modify ACLs on files on the server.
+
+	  Most NFS servers don't support the Solaris NFSv3 ACL protocol
+	  extension.  You can choose N here or specify the "noacl" mount
+	  option to prevent your NFS client from trying to use the NFSv3
+	  ACL protocol.
 
 	  If unsure, say N.
 
 config NFS_V4
-	bool "Provide NFSv4 client support (EXPERIMENTAL)"
+	bool "NFS client support for NFS version 4 (EXPERIMENTAL)"
 	depends on NFS_FS && EXPERIMENTAL
 	select RPCSEC_GSS_KRB5
 	help
-	  Say Y here if you want your NFS client to be able to speak the newer
-	  version 4 of the NFS protocol.
+	  This option enables support for version 4 of the NFS protocol
+	  (RFC 3530) in the kernel's NFS client.
 
-	  Note: Requires auxiliary userspace daemons which may be found on
-		http://www.citi.umich.edu/projects/nfsv4/
+	  To mount NFS servers using NFSv4, you also need to install user
+	  space programs which can be found in the Linux nfs-utils package,
+	  available from http://linux-nfs.org/.
 
 	  If unsure, say N.
 
+config ROOT_NFS
+	bool "Root file system on NFS"
+	depends on NFS_FS=y && IP_PNP
+	help
+	  If you want your system to mount its root file system via NFS,
+	  choose Y here.  This is common practice for managing systems
+	  without local permanent storage.  For details, read
+	  <file:Documentation/filesystems/nfsroot.txt>.
+
+	  Most people say N here.
+
 config NFSD
 	tristate "NFS server support"
 	depends on INET
@@ -1746,20 +1759,6 @@ config NFSD_V4
 
 	  If unsure, say N.
 
-config ROOT_NFS
-	bool "Root file system on NFS"
-	depends on NFS_FS=y && IP_PNP
-	help
-	  If you want your Linux box to mount its whole root file system (the
-	  one containing the directory /) from some other computer over the
-	  net via NFS (presumably because your box doesn't have a hard disk),
-	  say Y. Read <file:Documentation/filesystems/nfsroot.txt> for
-	  details. It is likely that in this case, you also want to say Y to
-	  "Kernel level IP autoconfiguration" so that your box can discover
-	  its network address at boot time.
-
-	  Most people say N here.
-
 config LOCKD
 	tristate
 

[PATCH 2/8] SUNRPC: Use RPC procedure name in call_start

[Chuck Lever]

Making debugging output a little cleaner in call_start by displaying the
name of the RPC procedure instead of it's number.

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

 net/sunrpc/clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 8945307..adbc85c 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -701,9 +701,9 @@ call_start(struct rpc_task *task)
 {
 	struct rpc_clnt	*clnt = task->tk_client;
 
-	dprintk("RPC: %5u call_start %s%d proc %d (%s)\n", task->tk_pid,
+	dprintk("RPC: %5u call_start %s%u proc %s (%s)\n", task->tk_pid,
 			clnt->cl_protname, clnt->cl_vers,
-			task->tk_msg.rpc_proc->p_proc,
+			task->tk_msg.rpc_proc->p_name,
 			(RPC_IS_ASYNC(task) ? "async" : "sync"));
 
 	/* Increment call count */

[PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Chuck Lever]

Making debugging output a little cleaner in call_verify by displaying the
name of the RPC procedure instead of it's number.

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

 net/sunrpc/clnt.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index adbc85c..5aa32fa 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
 		error = -EPROTONOSUPPORT;
 		goto out_err;
 	case RPC_PROC_UNAVAIL:
-		dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
+		dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
 				"version %u on server %s\n",
 				task->tk_pid, __func__,
-				task->tk_msg.rpc_proc,
+				task->tk_msg.rpc_proc->p_name,
 				task->tk_client->cl_prog,
 				task->tk_client->cl_vers,
 				task->tk_client->cl_server);

[PATCH 4/8] SUNRPC: Don't display the rpc_show_tasks header if there are no tasks

[Chuck Lever]

Clean up: don't display the rpc_show_tasks column header unless there is at
least one task to display.  As far as I can tell, it is safe to let the
list_for_each_entry macro decide that each list is empty.

scripts/checkpatch.pl also wants a KERN_FOO at the start of any newly added
printk() calls, so this and subsequent patches will also add KERN_INFO.

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

 net/sunrpc/clnt.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 5aa32fa..1a12236 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1517,24 +1517,30 @@ struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int
 EXPORT_SYMBOL_GPL(rpc_call_null);
 
 #ifdef RPC_DEBUG
+static void rpc_show_header(void)
+{
+	printk(KERN_INFO "-pid- proc flgs status -client- -prog- --rqstp- "
+		"-timeout -rpcwait -action- ---ops--\n");
+}
+
 void rpc_show_tasks(void)
 {
 	struct rpc_clnt *clnt;
 	struct rpc_task *t;
+	int header = 0;
 
 	spin_lock(&rpc_client_lock);
-	if (list_empty(&all_clients))
-		goto out;
-	printk("-pid- proc flgs status -client- -prog- --rqstp- -timeout "
-		"-rpcwait -action- ---ops--\n");
 	list_for_each_entry(clnt, &all_clients, cl_clients) {
-		if (list_empty(&clnt->cl_tasks))
-			continue;
 		spin_lock(&clnt->cl_lock);
 		list_for_each_entry(t, &clnt->cl_tasks, tk_task) {
 			const char *rpc_waitq = "none";
 			int proc;
 
+			if (!header) {
+				rpc_show_header();
+				header++;
+			}
+
 			if (t->tk_msg.rpc_proc)
 				proc = t->tk_msg.rpc_proc->p_proc;
 			else
@@ -1554,7 +1560,6 @@ void rpc_show_tasks(void)
 		}
 		spin_unlock(&clnt->cl_lock);
 	}
-out:
 	spin_unlock(&rpc_client_lock);
 }
 #endif

[PATCH 5/8] SUNRPC: Refactor rpc_show_tasks

[Chuck Lever]

Clean up: move the logic that displays each task to its own function.
This removes indentation and makes future changes easier.

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

 net/sunrpc/clnt.c |   46 ++++++++++++++++++++++++----------------------
 1 files changed, 24 insertions(+), 22 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 1a12236..15c0f5b 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1523,40 +1523,42 @@ static void rpc_show_header(void)
 		"-timeout -rpcwait -action- ---ops--\n");
 }
 
+static void rpc_show_task(const struct rpc_clnt *clnt,
+			  const struct rpc_task *task)
+{
+	const char *rpc_waitq = "none";
+	int proc = -1;
+
+	if (task->tk_msg.rpc_proc)
+		proc = task->tk_msg.rpc_proc->p_proc;
+
+	if (RPC_IS_QUEUED(task))
+		rpc_waitq = rpc_qname(task->tk_waitqueue);
+
+	printk(KERN_INFO "%5u %04d %04x %6d %8p %6d %8p %8ld %8s %8p %8p\n",
+		task->tk_pid, proc,
+		task->tk_flags, task->tk_status,
+		clnt, clnt->cl_prog,
+		task->tk_rqstp, task->tk_timeout,
+		rpc_waitq,
+		task->tk_action, task->tk_ops);
+}
+
 void rpc_show_tasks(void)
 {
 	struct rpc_clnt *clnt;
-	struct rpc_task *t;
+	struct rpc_task *task;
 	int header = 0;
 
 	spin_lock(&rpc_client_lock);
 	list_for_each_entry(clnt, &all_clients, cl_clients) {
 		spin_lock(&clnt->cl_lock);
-		list_for_each_entry(t, &clnt->cl_tasks, tk_task) {
-			const char *rpc_waitq = "none";
-			int proc;
-
+		list_for_each_entry(task, &clnt->cl_tasks, tk_task) {
 			if (!header) {
 				rpc_show_header();
 				header++;
 			}
-
-			if (t->tk_msg.rpc_proc)
-				proc = t->tk_msg.rpc_proc->p_proc;
-			else
-				proc = -1;
-
-			if (RPC_IS_QUEUED(t))
-				rpc_waitq = rpc_qname(t->tk_waitqueue);
-
-			printk("%5u %04d %04x %6d %8p %6d %8p %8ld %8s %8p %8p\n",
-				t->tk_pid, proc,
-				t->tk_flags, t->tk_status,
-				t->tk_client,
-				(t->tk_client ? t->tk_client->cl_prog : 0),
-				t->tk_rqstp, t->tk_timeout,
-				rpc_waitq,
-				t->tk_action, t->tk_ops);
+			rpc_show_task(clnt, task);
 		}
 		spin_unlock(&clnt->cl_lock);
 	}

[PATCH 6/8] SUNRPC: Display some debugging information as text rather than numbers

[Chuck Lever]

In rpc_show_tasks(), display the program name, version number, and
procedure name as human-readable variable-length text fields rather than
columnar numbers.

Sample output:

 -pid- flgs status -client- --rqstp- -timeout -action- ---ops--
 14470 0001      0 ee83eaf0 f79a2060    60000 f8cefa00 f8eaa764 nfs3 WRITE wq:xprt_pending
  2970 0001      0 ee83eaf0 f79a2060    60000 f8cefa00 f8eaa77c nfs3 COMMIT wq:xprt_pending
 12468 0080      0 ee83eaf0 f79a2060        0 f8cefa00 f8d03634 nfs3 SETATTR
 40513 0001      0 ee83eaf0 f79a2060        0 f8cefa00 f8eaa6c8 nfs3 READ

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

 net/sunrpc/clnt.c |   25 +++++++++++--------------
 1 files changed, 11 insertions(+), 14 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 15c0f5b..6f94a70 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1519,29 +1519,26 @@ EXPORT_SYMBOL_GPL(rpc_call_null);
 #ifdef RPC_DEBUG
 static void rpc_show_header(void)
 {
-	printk(KERN_INFO "-pid- proc flgs status -client- -prog- --rqstp- "
-		"-timeout -rpcwait -action- ---ops--\n");
+	printk(KERN_INFO "-pid- flgs status -client- --rqstp- "
+		"-timeout -action- ---ops--\n");
 }
 
 static void rpc_show_task(const struct rpc_clnt *clnt,
 			  const struct rpc_task *task)
 {
-	const char *rpc_waitq = "none";
-	int proc = -1;
+	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %8p %s%u",
+		task->tk_pid, task->tk_flags, task->tk_status,
+		clnt, task->tk_rqstp, task->tk_timeout,
+		task->tk_action, task->tk_ops, clnt->cl_protname,
+		clnt->cl_vers);
 
 	if (task->tk_msg.rpc_proc)
-		proc = task->tk_msg.rpc_proc->p_proc;
+		printk(KERN_CONT " %s", task->tk_msg.rpc_proc->p_name);
 
 	if (RPC_IS_QUEUED(task))
-		rpc_waitq = rpc_qname(task->tk_waitqueue);
-
-	printk(KERN_INFO "%5u %04d %04x %6d %8p %6d %8p %8ld %8s %8p %8p\n",
-		task->tk_pid, proc,
-		task->tk_flags, task->tk_status,
-		clnt, clnt->cl_prog,
-		task->tk_rqstp, task->tk_timeout,
-		rpc_waitq,
-		task->tk_action, task->tk_ops);
+		printk(KERN_CONT " wq:%s", rpc_qname(task->tk_waitqueue));
+
+	printk(KERN_CONT "\n");
 }
 
 void rpc_show_tasks(void)

[PATCH 7/8] SUNRPC: Rename "call_" functions that are no longer FSM states

[Chuck Lever]

The RPC client uses a finite state machine to move RPC tasks through each
step of an RPC request.  Each state is contained in a function in
net/sunrpc/clnt.c, and named call_foo.

Some of the functions named call_foo have changed over the past few years and
are no longer states in the FSM.  These include: call_encode, call_header,
and call_verify.  As a clean up, rename the functions that have changed.

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

 net/sunrpc/clnt.c |   35 ++++++++++++++---------------------
 1 files changed, 14 insertions(+), 21 deletions(-)


diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index 6f94a70..c33d727 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -58,7 +58,6 @@ static void	call_start(struct rpc_task *task);
 static void	call_reserve(struct rpc_task *task);
 static void	call_reserveresult(struct rpc_task *task);
 static void	call_allocate(struct rpc_task *task);
-static void	call_encode(struct rpc_task *task);
 static void	call_decode(struct rpc_task *task);
 static void	call_bind(struct rpc_task *task);
 static void	call_bind_status(struct rpc_task *task);
@@ -70,9 +69,9 @@ static void	call_refreshresult(struct rpc_task *task);
 static void	call_timeout(struct rpc_task *task);
 static void	call_connect(struct rpc_task *task);
 static void	call_connect_status(struct rpc_task *task);
-static __be32 *	call_header(struct rpc_task *task);
-static __be32 *	call_verify(struct rpc_task *task);
 
+static __be32	*rpc_encode_header(struct rpc_task *task);
+static __be32	*rpc_verify_header(struct rpc_task *task);
 static int	rpc_ping(struct rpc_clnt *clnt, int flags);
 
 static void rpc_register_client(struct rpc_clnt *clnt)
@@ -861,7 +860,7 @@ rpc_xdr_buf_init(struct xdr_buf *buf, void *start, size_t len)
  * 3.	Encode arguments of an RPC call
  */
 static void
-call_encode(struct rpc_task *task)
+rpc_xdr_encode(struct rpc_task *task)
 {
 	struct rpc_rqst	*req = task->tk_rqstp;
 	kxdrproc_t	encode;
@@ -876,13 +875,14 @@ call_encode(struct rpc_task *task)
 			 (char *)req->rq_buffer + req->rq_callsize,
 			 req->rq_rcvsize);
 
-	/* Encode header and provided arguments */
-	encode = task->tk_msg.rpc_proc->p_encode;
-	if (!(p = call_header(task))) {
-		printk(KERN_INFO "RPC: call_header failed, exit EIO\n");
+	p = rpc_encode_header(task);
+	if (p == NULL) {
+		printk(KERN_INFO "RPC: couldn't encode RPC header, exit EIO\n");
 		rpc_exit(task, -EIO);
 		return;
 	}
+
+	encode = task->tk_msg.rpc_proc->p_encode;
 	if (encode == NULL)
 		return;
 
@@ -1046,7 +1046,7 @@ call_transmit(struct rpc_task *task)
 	/* Encode here so that rpcsec_gss can use correct sequence number. */
 	if (rpc_task_need_encode(task)) {
 		BUG_ON(task->tk_rqstp->rq_bytes_sent != 0);
-		call_encode(task);
+		rpc_xdr_encode(task);
 		/* Did the encode result in an error condition? */
 		if (task->tk_status != 0)
 			return;
@@ -1224,8 +1224,7 @@ call_decode(struct rpc_task *task)
 		goto out_retry;
 	}
 
-	/* Verify the RPC header */
-	p = call_verify(task);
+	p = rpc_verify_header(task);
 	if (IS_ERR(p)) {
 		if (p == ERR_PTR(-EAGAIN))
 			goto out_retry;
@@ -1243,7 +1242,7 @@ call_decode(struct rpc_task *task)
 	return;
 out_retry:
 	task->tk_status = 0;
-	/* Note: call_verify() may have freed the RPC slot */
+	/* Note: rpc_verify_header() may have freed the RPC slot */
 	if (task->tk_rqstp == req) {
 		req->rq_received = req->rq_rcv_buf.len = 0;
 		if (task->tk_client->cl_discrtry)
@@ -1290,11 +1289,8 @@ call_refreshresult(struct rpc_task *task)
 	return;
 }
 
-/*
- * Call header serialization
- */
 static __be32 *
-call_header(struct rpc_task *task)
+rpc_encode_header(struct rpc_task *task)
 {
 	struct rpc_clnt *clnt = task->tk_client;
 	struct rpc_rqst	*req = task->tk_rqstp;
@@ -1314,11 +1310,8 @@ call_header(struct rpc_task *task)
 	return p;
 }
 
-/*
- * Reply header verification
- */
 static __be32 *
-call_verify(struct rpc_task *task)
+rpc_verify_header(struct rpc_task *task)
 {
 	struct kvec *iov = &task->tk_rqstp->rq_rcv_buf.head[0];
 	int len = task->tk_rqstp->rq_rcv_buf.len >> 2;
@@ -1392,7 +1385,7 @@ call_verify(struct rpc_task *task)
 			task->tk_action = call_bind;
 			goto out_retry;
 		case RPC_AUTH_TOOWEAK:
-			printk(KERN_NOTICE "call_verify: server %s requires stronger "
+			printk(KERN_NOTICE "RPC: server %s requires stronger "
 			       "authentication.\n", task->tk_client->cl_server);
 			break;
 		default:

[PATCH 8/8] SUNRPC: Display symbolic function addresses in rpc_show_tasks

[Chuck Lever]

For each running RPC task, rpc_show_tasks displays the hex address of the
call_foo function that the task is running.  To make debugging slightly
nicer, let's display the call_foo function name instead.

Sample output:

 -pid- flgs status -client- --rqstp- -timeout ---ops--
 27915 0001      0 ee84a460 eedf00d0    60000 f8fa677c nfs3 COMMIT act:status wq:xprt_pending
 27918 0080      0 ee84a460 eedf0000    60000 f8d527a8 nfs3 SETATTR act:status wq:xprt_pending

 -pid- flgs status -client- --rqstp- -timeout ---ops--
 60859 0001      0 ee84a460 eedf00d0        0 f8fa66c8 nfs3 READ act:status
 60860 0080      0 ee84a460 eedf0000        0 f8d527a8 nfs3 GETATTR act:status


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

 include/linux/sunrpc/sched.h |    1 +
 net/sunrpc/clnt.c            |   52 +++++++++++++++++++++++++++++++++++++++---
 net/sunrpc/sched.c           |    2 +-
 3 files changed, 50 insertions(+), 5 deletions(-)


diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
index d1a5c8c..108342e 100644
--- a/include/linux/sunrpc/sched.h
+++ b/include/linux/sunrpc/sched.h
@@ -212,6 +212,7 @@ struct rpc_wait_queue {
 struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
 struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
 void		rpc_put_task(struct rpc_task *);
+void		rpc_prepare_task(struct rpc_task *);
 void		rpc_exit_task(struct rpc_task *);
 void		rpc_release_calldata(const struct rpc_call_ops *, void *);
 void		rpc_killall_tasks(struct rpc_clnt *);
diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index c33d727..ec9c072 100644
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1510,24 +1510,68 @@ struct rpc_task *rpc_call_null(struct rpc_clnt *clnt, struct rpc_cred *cred, int
 EXPORT_SYMBOL_GPL(rpc_call_null);
 
 #ifdef RPC_DEBUG
+/*
+ * To make it easier to tell what action each running RPC task
+ * is executing, use a table to map the content of tk_action to
+ * a human-readable name.  This uses a little extra memory, but
+ * causes no additional run-time overhead per RPC request.
+ */
+typedef void   (*rpc_task_action)(struct rpc_task *);
+
+static struct {
+	rpc_task_action		action;
+	const char		*name;
+} rpc_action_table[] = {
+	{ rpc_prepare_task,		"prepare"		},
+	{ call_start,			"start"			},
+	{ call_reserve,			"reserve"		},
+	{ call_reserveresult,		"reserveresult"		},
+	{ call_allocate,		"allocate"		},
+	{ call_bind,			"bind"			},
+	{ call_bind_status,		"bindstatus"		},
+	{ call_connect,			"connect"		},
+	{ call_connect_status,		"connectstatus"		},
+	{ call_transmit,		"transmit"		},
+	{ call_transmit_status,		"transmitstatus"	},
+	{ call_status,			"status"		},
+	{ call_timeout,			"timeout"		},
+	{ call_decode,			"decode"		},
+	{ call_refresh,			"refresh"		},
+	{ call_refreshresult,		"refreshresult"		},
+	{ rpc_exit_task,		"exit"			},
+	{ NULL,				"null"			},
+};
+
+static const char *rpc_show_action(rpc_task_action action)
+{
+	unsigned int i;
+
+	for (i = 0; i <= ARRAY_SIZE(rpc_action_table); i++)
+		if (rpc_action_table[i].action == action)
+			return rpc_action_table[i].name;
+
+	return "unknown";
+}
+
 static void rpc_show_header(void)
 {
 	printk(KERN_INFO "-pid- flgs status -client- --rqstp- "
-		"-timeout -action- ---ops--\n");
+		"-timeout ---ops--\n");
 }
 
 static void rpc_show_task(const struct rpc_clnt *clnt,
 			  const struct rpc_task *task)
 {
-	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %8p %s%u",
+	printk(KERN_INFO "%5u %04x %6d %8p %8p %8ld %8p %s%u",
 		task->tk_pid, task->tk_flags, task->tk_status,
 		clnt, task->tk_rqstp, task->tk_timeout,
-		task->tk_action, task->tk_ops, clnt->cl_protname,
-		clnt->cl_vers);
+		task->tk_ops, clnt->cl_protname, clnt->cl_vers);
 
 	if (task->tk_msg.rpc_proc)
 		printk(KERN_CONT " %s", task->tk_msg.rpc_proc->p_name);
 
+	printk(KERN_CONT " act:%s", rpc_show_action(task->tk_action));
+
 	if (RPC_IS_QUEUED(task))
 		printk(KERN_CONT " wq:%s", rpc_qname(task->tk_waitqueue));
 
diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
index 6eab9bf..9aa9948 100644
--- a/net/sunrpc/sched.c
+++ b/net/sunrpc/sched.c
@@ -574,7 +574,7 @@ EXPORT_SYMBOL_GPL(rpc_delay);
 /*
  * Helper to call task->tk_ops->rpc_call_prepare
  */
-static void rpc_prepare_task(struct rpc_task *task)
+void rpc_prepare_task(struct rpc_task *task)
 {
 	lock_kernel();
 	task->tk_ops->rpc_call_prepare(task, task->tk_calldata);

Re: [PATCH 8/8] SUNRPC: Display symbolic function addresses in rpc_show_tasks

[Trond Myklebust]

On Tue, 2008-05-20 at 16:30 -0400, Chuck Lever wrote:
> For each running RPC task, rpc_show_tasks displays the hex address of=
 the
> call_foo function that the task is running.  To make debugging slight=
ly
> nicer, let's display the call_foo function name instead.
>=20
> Sample output:
>=20
>  -pid- flgs status -client- --rqstp- -timeout ---ops--
>  27915 0001      0 ee84a460 eedf00d0    60000 f8fa677c nfs3 COMMIT ac=
t:status wq:xprt_pending
>  27918 0080      0 ee84a460 eedf0000    60000 f8d527a8 nfs3 SETATTR a=
ct:status wq:xprt_pending
>=20
>  -pid- flgs status -client- --rqstp- -timeout ---ops--
>  60859 0001      0 ee84a460 eedf00d0        0 f8fa66c8 nfs3 READ act:=
status
>  60860 0080      0 ee84a460 eedf0000        0 f8d527a8 nfs3 GETATTR a=
ct:status
>=20
>=20
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>=20
>  include/linux/sunrpc/sched.h |    1 +
>  net/sunrpc/clnt.c            |   52 ++++++++++++++++++++++++++++++++=
+++++++---
>  net/sunrpc/sched.c           |    2 +-
>  3 files changed, 50 insertions(+), 5 deletions(-)
>=20
>=20
> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sche=
d.h
> index d1a5c8c..108342e 100644
> --- a/include/linux/sunrpc/sched.h
> +++ b/include/linux/sunrpc/sched.h
> @@ -212,6 +212,7 @@ struct rpc_wait_queue {
>  struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
>  struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
>  void		rpc_put_task(struct rpc_task *);
> +void		rpc_prepare_task(struct rpc_task *);
>  void		rpc_exit_task(struct rpc_task *);
>  void		rpc_release_calldata(const struct rpc_call_ops *, void *);
>  void		rpc_killall_tasks(struct rpc_clnt *);
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index c33d727..ec9c072 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1510,24 +1510,68 @@ struct rpc_task *rpc_call_null(struct rpc_cln=
t *clnt, struct rpc_cred *cred, int
>  EXPORT_SYMBOL_GPL(rpc_call_null);
> =20
>  #ifdef RPC_DEBUG
> +/*
> + * To make it easier to tell what action each running RPC task
> + * is executing, use a table to map the content of tk_action to
> + * a human-readable name.  This uses a little extra memory, but
> + * causes no additional run-time overhead per RPC request.
> + */
> +typedef void   (*rpc_task_action)(struct rpc_task *);
> +
> +static struct {
> +	rpc_task_action		action;
> +	const char		*name;
> +} rpc_action_table[] =3D {
> +	{ rpc_prepare_task,		"prepare"		},
> +	{ call_start,			"start"			},
> +	{ call_reserve,			"reserve"		},
> +	{ call_reserveresult,		"reserveresult"		},
> +	{ call_allocate,		"allocate"		},
> +	{ call_bind,			"bind"			},
> +	{ call_bind_status,		"bindstatus"		},
> +	{ call_connect,			"connect"		},
> +	{ call_connect_status,		"connectstatus"		},
> +	{ call_transmit,		"transmit"		},
> +	{ call_transmit_status,		"transmitstatus"	},
> +	{ call_status,			"status"		},
> +	{ call_timeout,			"timeout"		},
> +	{ call_decode,			"decode"		},
> +	{ call_refresh,			"refresh"		},
> +	{ call_refreshresult,		"refreshresult"		},
> +	{ rpc_exit_task,		"exit"			},
> +	{ NULL,				"null"			},
> +};
>
> +
> +static const char *rpc_show_action(rpc_task_action action)
> +{
> +	unsigned int i;
> +
> +	for (i =3D 0; i <=3D ARRAY_SIZE(rpc_action_table); i++)
> +		if (rpc_action_table[i].action =3D=3D action)
> +			return rpc_action_table[i].name;
> +
> +	return "unknown";
> +}

=EF=BB=BF
NACK. Maintaining tables like the above in the long run is a nightmare.
What's more, by eliminating the printout of the actual pointer you are
potentially replacing useful information that could otherwise easily be
looked up using /proc/kallsyms with "unknown".

The above translation could easily be done using a simple script in
userspace instead.


--=20
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Trond Myklebust]

On Tue, 2008-05-20 at 16:29 -0400, Chuck Lever wrote:
> Making debugging output a little cleaner in call_verify by displaying the
> name of the RPC procedure instead of it's number.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
> 
>  net/sunrpc/clnt.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> index adbc85c..5aa32fa 100644
> --- a/net/sunrpc/clnt.c
> +++ b/net/sunrpc/clnt.c
> @@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
>  		error = -EPROTONOSUPPORT;
>  		goto out_err;
>  	case RPC_PROC_UNAVAIL:
> -		dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
> +		dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
>  				"version %u on server %s\n",
>  				task->tk_pid, __func__,
> -				task->tk_msg.rpc_proc,
> +				task->tk_msg.rpc_proc->p_name,
>  				task->tk_client->cl_prog,
>  				task->tk_client->cl_vers,
>  				task->tk_client->cl_server);

This will cause a crash if you ever call it while somebody is calling
rpc_call_null. There may be other cases too.


-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 8/8] SUNRPC: Display symbolic function addresses in rpc_show_tasks

[Chuck Lever]

On May 20, 2008, at 5:11 PM, Trond Myklebust wrote:
> On Tue, 2008-05-20 at 16:30 -0400, Chuck Lever wrote:
>> For each running RPC task, rpc_show_tasks displays the hex address  
>> of the
>> call_foo function that the task is running.  To make debugging  
>> slightly
>> nicer, let's display the call_foo function name instead.
>>
>> Sample output:
>>
>> -pid- flgs status -client- --rqstp- -timeout ---ops--
>> 27915 0001      0 ee84a460 eedf00d0    60000 f8fa677c nfs3 COMMIT  
>> act:status wq:xprt_pending
>> 27918 0080      0 ee84a460 eedf0000    60000 f8d527a8 nfs3 SETATTR  
>> act:status wq:xprt_pending
>>
>> -pid- flgs status -client- --rqstp- -timeout ---ops--
>> 60859 0001      0 ee84a460 eedf00d0        0 f8fa66c8 nfs3 READ  
>> act:status
>> 60860 0080      0 ee84a460 eedf0000        0 f8d527a8 nfs3 GETATTR  
>> act:status
>>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> include/linux/sunrpc/sched.h |    1 +
>> net/sunrpc/clnt.c            |   52 ++++++++++++++++++++++++++++++++ 
>> +++++++---
>> net/sunrpc/sched.c           |    2 +-
>> 3 files changed, 50 insertions(+), 5 deletions(-)
>>
>>
>> diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/ 
>> sched.h
>> index d1a5c8c..108342e 100644
>> --- a/include/linux/sunrpc/sched.h
>> +++ b/include/linux/sunrpc/sched.h
>> @@ -212,6 +212,7 @@ struct rpc_wait_queue {
>> struct rpc_task *rpc_new_task(const struct rpc_task_setup *);
>> struct rpc_task *rpc_run_task(const struct rpc_task_setup *);
>> void		rpc_put_task(struct rpc_task *);
>> +void		rpc_prepare_task(struct rpc_task *);
>> void		rpc_exit_task(struct rpc_task *);
>> void		rpc_release_calldata(const struct rpc_call_ops *, void *);
>> void		rpc_killall_tasks(struct rpc_clnt *);
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index c33d727..ec9c072 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1510,24 +1510,68 @@ struct rpc_task *rpc_call_null(struct  
>> rpc_clnt *clnt, struct rpc_cred *cred, int
>> EXPORT_SYMBOL_GPL(rpc_call_null);
>>
>> #ifdef RPC_DEBUG
>> +/*
>> + * To make it easier to tell what action each running RPC task
>> + * is executing, use a table to map the content of tk_action to
>> + * a human-readable name.  This uses a little extra memory, but
>> + * causes no additional run-time overhead per RPC request.
>> + */
>> +typedef void   (*rpc_task_action)(struct rpc_task *);
>> +
>> +static struct {
>> +	rpc_task_action		action;
>> +	const char		*name;
>> +} rpc_action_table[] = {
>> +	{ rpc_prepare_task,		"prepare"		},
>> +	{ call_start,			"start"			},
>> +	{ call_reserve,			"reserve"		},
>> +	{ call_reserveresult,		"reserveresult"		},
>> +	{ call_allocate,		"allocate"		},
>> +	{ call_bind,			"bind"			},
>> +	{ call_bind_status,		"bindstatus"		},
>> +	{ call_connect,			"connect"		},
>> +	{ call_connect_status,		"connectstatus"		},
>> +	{ call_transmit,		"transmit"		},
>> +	{ call_transmit_status,		"transmitstatus"	},
>> +	{ call_status,			"status"		},
>> +	{ call_timeout,			"timeout"		},
>> +	{ call_decode,			"decode"		},
>> +	{ call_refresh,			"refresh"		},
>> +	{ call_refreshresult,		"refreshresult"		},
>> +	{ rpc_exit_task,		"exit"			},
>> +	{ NULL,				"null"			},
>> +};
>>
>> +
>> +static const char *rpc_show_action(rpc_task_action action)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i <= ARRAY_SIZE(rpc_action_table); i++)
>> +		if (rpc_action_table[i].action == action)
>> +			return rpc_action_table[i].name;
>> +
>> +	return "unknown";
>> +}
>
>
> NACK. Maintaining tables like the above in the long run is a  
> nightmare.

> What's more, by eliminating the printout of the actual pointer you are
> potentially replacing useful information that could otherwise easily  
> be
> looked up using /proc/kallsyms with "unknown".

> The above translation could easily be done using a simple script in
> userspace instead.

Except when you don't have access to the kernel image and modules  
where the dump was generated.  Why would I want to run a script to do  
this, when Oops output already displays symbolic function addresses in  
a callback trace?

We can drop this one for now.  Please let me know about the  
disposition of the other seven.

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

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Chuck Lever]

On May 20, 2008, at 5:21 PM, Trond Myklebust wrote:

> On Tue, 2008-05-20 at 16:29 -0400, Chuck Lever wrote:
>> Making debugging output a little cleaner in call_verify by  
>> displaying the
>> name of the RPC procedure instead of it's number.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>> net/sunrpc/clnt.c |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>>
>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>> index adbc85c..5aa32fa 100644
>> --- a/net/sunrpc/clnt.c
>> +++ b/net/sunrpc/clnt.c
>> @@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
>> 		error = -EPROTONOSUPPORT;
>> 		goto out_err;
>> 	case RPC_PROC_UNAVAIL:
>> -		dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
>> +		dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
>> 				"version %u on server %s\n",
>> 				task->tk_pid, __func__,
>> -				task->tk_msg.rpc_proc,
>> +				task->tk_msg.rpc_proc->p_name,
>> 				task->tk_client->cl_prog,
>> 				task->tk_client->cl_vers,
>> 				task->tk_client->cl_server);
>
> This will cause a crash if you ever call it while somebody is calling
> rpc_call_null. There may be other cases too.

How could you ever get an RPC_PROC_UNAVAIL with a NULL RPC?

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

Re: [PATCH 8/8] SUNRPC: Display symbolic function addresses in rpc_show_tasks

[Trond Myklebust]

On Tue, 2008-05-20 at 17:37 -0400, Chuck Lever wrote:

> Except when you don't have access to the kernel image and modules  
> where the dump was generated.  Why would I want to run a script to do  
> this, when Oops output already displays symbolic function addresses in  
> a callback trace?

If you can generate the names in the same way that the Oops output does
(i.e. without having to maintain a table of function names), then I
withdraw my opposition.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Trond Myklebust]

On Tue, 2008-05-20 at 17:39 -0400, Chuck Lever wrote:

> How could you ever get an RPC_PROC_UNAVAIL with a NULL RPC?

You shouldn't, given a well adjusted server, but that's not equivalent
to saying that it can't ever happen. 

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Peter Staubach]

Chuck Lever wrote:
>
> On May 20, 2008, at 5:21 PM, Trond Myklebust wrote:
>
>> On Tue, 2008-05-20 at 16:29 -0400, Chuck Lever wrote:
>>> Making debugging output a little cleaner in call_verify by 
>>> displaying the
>>> name of the RPC procedure instead of it's number.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>
>>> net/sunrpc/clnt.c |    4 ++--
>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>>
>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> index adbc85c..5aa32fa 100644
>>> --- a/net/sunrpc/clnt.c
>>> +++ b/net/sunrpc/clnt.c
>>> @@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
>>>         error = -EPROTONOSUPPORT;
>>>         goto out_err;
>>>     case RPC_PROC_UNAVAIL:
>>> -        dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
>>> +        dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
>>>                 "version %u on server %s\n",
>>>                 task->tk_pid, __func__,
>>> -                task->tk_msg.rpc_proc,
>>> +                task->tk_msg.rpc_proc->p_name,
>>>                 task->tk_client->cl_prog,
>>>                 task->tk_client->cl_vers,
>>>                 task->tk_client->cl_server);
>>
>> This will cause a crash if you ever call it while somebody is calling
>> rpc_call_null. There may be other cases too.
>
> How could you ever get an RPC_PROC_UNAVAIL with a NULL RPC? 

I have seen servers which didn't implement the NULL_PROC procedure
for protocols like NFS_ACL.  Beats me why.

It would be good if the client didn't crash just because someone
got lazy in the server implementation though.

       ps

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Talpey, Thomas]

At 08:14 AM 5/21/2008, Peter Staubach wrote:
>I have seen servers which didn't implement the NULL_PROC procedure
>for protocols like NFS_ACL.  Beats me why.

Names. We want names. :-)

Tom.

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Peter Staubach]

Talpey, Thomas wrote:
> At 08:14 AM 5/21/2008, Peter Staubach wrote:
>   
>> I have seen servers which didn't implement the NULL_PROC procedure
>> for protocols like NFS_ACL.  Beats me why.
>>     
>
> Names. We want names. :-)

:-)

Names withheld to prevent embarrassment of the guilty?

Actually, I don't remember anymore who they were, but I suspect
that I would have complained to them at the time.  It just isn't
that hard to implement a procedure which doesn't do anything but
respond to the client.

       ps

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Robert Gordon]

This being an accepted practice -- as opposed to a clearly
defined requirement in a spec someplace ??

On May 21, 2008, at 7:38 AM, Peter Staubach wrote:
> Talpey, Thomas wrote:
>> At 08:14 AM 5/21/2008, Peter Staubach wrote:
>>
>>> I have seen servers which didn't implement the NULL_PROC procedure
>>> for protocols like NFS_ACL.  Beats me why.
>>>
>>
>> Names. We want names. :-)
>
> :-)
>
> Names withheld to prevent embarrassment of the guilty?
>
> Actually, I don't remember anymore who they were, but I suspect
> that I would have complained to them at the time.  It just isn't
> that hard to implement a procedure which doesn't do anything but
> respond to the client.
>
>      ps
> --
> 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

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Peter Staubach]

Robert Gordon wrote:
>
> This being an accepted practice -- as opposed to a clearly
> defined requirement in a spec someplace ?? 

You know, I don't know whether the RPC specification clearly
indicates that procedure, 0, is the NULL procedure.  It just
seems to be accepted practice and assumptions have been made
that procedure, 0, is the NULL procedure and also does not
require any special authentication.

The NULL_PROC is used as a "ping" sort of procedure by things
like rpcinfo, the NFS mount command, etc.

       ps

Re: [PATCH 3/8] SUNRPC: Use RPC procedure name in call_verify

[Chuck Lever]

On May 21, 2008, at 8:14 AM, Peter Staubach wrote:
> Chuck Lever wrote:
>>
>> On May 20, 2008, at 5:21 PM, Trond Myklebust wrote:
>>
>>> On Tue, 2008-05-20 at 16:29 -0400, Chuck Lever wrote:
>>>> Making debugging output a little cleaner in call_verify by  
>>>> displaying the
>>>> name of the RPC procedure instead of it's number.
>>>>
>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>> ---
>>>>
>>>> net/sunrpc/clnt.c |    4 ++--
>>>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>>> index adbc85c..5aa32fa 100644
>>>> --- a/net/sunrpc/clnt.c
>>>> +++ b/net/sunrpc/clnt.c
>>>> @@ -1431,10 +1431,10 @@ call_verify(struct rpc_task *task)
>>>>        error = -EPROTONOSUPPORT;
>>>>        goto out_err;
>>>>    case RPC_PROC_UNAVAIL:
>>>> -        dprintk("RPC: %5u %s: proc %p unsupported by program %u, "
>>>> +        dprintk("RPC: %5u %s: proc %s unsupported by program %u, "
>>>>                "version %u on server %s\n",
>>>>                task->tk_pid, __func__,
>>>> -                task->tk_msg.rpc_proc,
>>>> +                task->tk_msg.rpc_proc->p_name,
>>>>                task->tk_client->cl_prog,
>>>>                task->tk_client->cl_vers,
>>>>                task->tk_client->cl_server);
>>>
>>> This will cause a crash if you ever call it while somebody is  
>>> calling
>>> rpc_call_null. There may be other cases too.
>>
>> How could you ever get an RPC_PROC_UNAVAIL with a NULL RPC?
>
> I have seen servers which didn't implement the NULL_PROC procedure
> for protocols like NFS_ACL.  Beats me why.
>
> It would be good if the client didn't crash just because someone
> got lazy in the server implementation though.

Agreed, and I have already implemented a fix.  I will post the new  
patch set later today after a little more testing.

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