[PATCH 0/2] sunrpc: more reliable detection of running gssd

[PATCH 0/2] sunrpc: more reliable detection of running gssd

[Jeff Layton]

We've gotten a lot of complaints recently about the 15s delay when
doing a sec=sys mount without gssd running.

A large part of the problem is that the kernel isn't able to reliably
detect when rpc.gssd is running. What we currently have is a
gssd_running flag that is initially set to 1. When an upcall times out,
that gets set to 0, and subsequent upcalls get a much shorter timeout
(1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.

The approach of using a flag like this is pretty inadequate. First, it
doesn't eliminate the long delay on the initial upcall attempt. Also,
if gssd spontaneously dies, then the flag will still be set to 1 until
the next upcall attempt times out. Finally, it currently requires that
the pipe be reopened in order to reset the flag back to true.

This patchset replaces that flag with a more reliable mechanism for
detecting when gssd is running. When rpc_pipefs is mounted, it creates a
new "dummy" pipe that gssd will naturally find and hold open. We'll
never send an upcall down this pipe, and writing to it always fails.
But, since we can detect when something is holding it open, we can use
that to determine whether gssd is running.

The current patch just uses this mechanism to replace the gssd_running
flag with this new mechanism. This shortens the long delay when mounting
without gssd running, but does not silence these warnings:

    RPC: AUTH_GSS upcall timed out.
    Please check user daemon is running.

I'm willing to add a patch to do that, but I'm a little unclear on the
best way to do so. Those messages are generated by the auth_gss code. We
probably do want to print them if someone mounted with sec=krb5, but
suppress them when mounting with sec=sys.

Do we need to somehow pass down that intent to auth_gss? Another idea
would be to call gssd_running() from the nfs mount code and use that to
determine whether to try and use krb5 at all...

Discuss!

Jeff Layton (2):
  sunrpc: create a new dummy pipe for gssd to hold open
  sunrpc: replace sunrpc_net->gssd_running flag with a better mechanism

 include/linux/sunrpc/rpc_pipe_fs.h |  4 ++
 net/sunrpc/auth_gss/auth_gss.c     | 13 ++++--
 net/sunrpc/netns.h                 |  3 +-
 net/sunrpc/rpc_pipe.c              | 95 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 106 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] sunrpc: create a new dummy pipe for gssd to hold open

[Jeff Layton]

rpc.gssd will naturally hold open any pipe named */clnt*/gssd that shows
up under rpc_pipefs. That behavior gives us a reliable mechanism to tell
whether it's actually running or not.

Create a new toplevel "dummy" directory in rpc_pipefs when it's mounted.
Under that directory create another directory called "clntXX", and then
within that a pipe called "gssd".

We'll never send an upcall along that pipe, and any downcall written to
it will just return -EINVAL.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 net/sunrpc/netns.h    |  1 +
 net/sunrpc/rpc_pipe.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
index 779742c..6b82968 100644
--- a/net/sunrpc/netns.h
+++ b/net/sunrpc/netns.h
@@ -15,6 +15,7 @@ struct sunrpc_net {
 
 	struct super_block *pipefs_sb;
 	struct mutex pipefs_sb_lock;
+	struct dentry *gssd_dummy;
 
 	struct list_head all_clients;
 	spinlock_t rpc_client_lock;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index f94567b..1c54de6 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -1168,6 +1168,7 @@ enum {
 	RPCAUTH_nfsd4_cb,
 	RPCAUTH_cache,
 	RPCAUTH_nfsd,
+	RPCAUTH_dummy,
 	RPCAUTH_RootEOF
 };
 
@@ -1204,6 +1205,10 @@ static const struct rpc_filelist files[] = {
 		.name = "nfsd",
 		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
 	},
+	[RPCAUTH_dummy] = {
+		.name = "dummy",
+		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
+	},
 };
 
 /*
@@ -1253,6 +1258,80 @@ void rpc_put_sb_net(const struct net *net)
 }
 EXPORT_SYMBOL_GPL(rpc_put_sb_net);
 
+static const struct rpc_filelist dummy_clnt_dir[] = {
+	[0] = {
+		.name = "clntXX",
+		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
+	},
+};
+
+static ssize_t
+dummy_downcall(struct file *filp, const char __user *src, size_t len)
+{
+	return -EINVAL;
+}
+
+static const struct rpc_pipe_ops dummy_pipe_ops = {
+	.upcall		= rpc_pipe_generic_upcall,
+	.downcall	= dummy_downcall,
+};
+
+/**
+ * rpc_gssd_dummy_populate - create a dummy gssd pipe
+ * @root: root of the rpc_pipefs filesystem
+ *
+ * Create a dummy set of directories and a pipe that gssd can hold open to
+ * indicate that it is up and running.
+ */
+static struct dentry *
+rpc_gssd_dummy_populate(struct dentry *root)
+{
+	int ret = 0;
+	struct dentry *gssd_dentry;
+	struct dentry *clnt_dentry = NULL;
+	struct dentry *pipe_dentry = NULL;
+	struct rpc_pipe *pipe_data = NULL;
+	struct qstr q = QSTR_INIT(files[RPCAUTH_dummy].name,
+				  strlen(files[RPCAUTH_dummy].name));
+
+	/* We should never get this far if "gssd" doesn't exist */
+	gssd_dentry = d_hash_and_lookup(root, &q);
+	if (!gssd_dentry)
+		return ERR_PTR(-ENOENT);
+
+	ret = rpc_populate(gssd_dentry, dummy_clnt_dir, 0, 1, NULL);
+	if (ret) {
+		pipe_dentry = ERR_PTR(ret);
+		goto out;
+	}
+
+	pipe_data = rpc_mkpipe_data(&dummy_pipe_ops, 0);
+	if (IS_ERR(pipe_data)) {
+		pipe_dentry = ERR_CAST(pipe_data);
+		pipe_data = NULL;
+		goto out;
+	}
+
+	q.name = dummy_clnt_dir[0].name;
+	q.len = strlen(dummy_clnt_dir[0].name);
+	clnt_dentry = d_hash_and_lookup(gssd_dentry, &q);
+	if (!clnt_dentry) {
+		pipe_dentry = ERR_PTR(-ENOENT);
+		goto out;
+	}
+
+	pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
+	if (IS_ERR(pipe_dentry))
+		goto out;
+
+	pipe_data = NULL;
+out:
+	rpc_destroy_pipe_data(pipe_data);
+	dput(clnt_dentry);
+	dput(gssd_dentry);
+	return pipe_dentry;
+}
+
 static int
 rpc_fill_super(struct super_block *sb, void *data, int silent)
 {
@@ -1275,6 +1354,11 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
 		return -ENOMEM;
 	if (rpc_populate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF, NULL))
 		return -ENOMEM;
+
+	sn->gssd_dummy = rpc_gssd_dummy_populate(root);
+	if (IS_ERR(sn->gssd_dummy))
+		return PTR_ERR(sn->gssd_dummy);
+
 	dprintk("RPC:       sending pipefs MOUNT notification for net %p%s\n",
 		net, NET_NAME(net));
 	mutex_lock(&sn->pipefs_sb_lock);
-- 
1.8.3.1

[PATCH 2/2] sunrpc: replace sunrpc_net->gssd_running flag with a better mechanism

[Jeff Layton]

Now that we have a more reliable method to tell if gssd is running, we
can replace the sn->gssd_running flag with a function that will query to
see if it's up and running.

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/sunrpc/rpc_pipe_fs.h |  4 ++++
 net/sunrpc/auth_gss/auth_gss.c     | 13 ++++++++++---
 net/sunrpc/netns.h                 |  2 --
 net/sunrpc/rpc_pipe.c              | 11 +++++++----
 4 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/linux/sunrpc/rpc_pipe_fs.h b/include/linux/sunrpc/rpc_pipe_fs.h
index a353e03..0ba459f 100644
--- a/include/linux/sunrpc/rpc_pipe_fs.h
+++ b/include/linux/sunrpc/rpc_pipe_fs.h
@@ -5,6 +5,8 @@
 
 #include <linux/workqueue.h>
 
+struct sunrpc_net;
+
 struct rpc_pipe_dir_head {
 	struct list_head pdh_entries;
 	struct dentry *pdh_dentry;
@@ -130,5 +132,7 @@ extern int rpc_unlink(struct dentry *);
 extern int register_rpc_pipefs(void);
 extern void unregister_rpc_pipefs(void);
 
+extern bool rpc_pipe_is_open(struct rpc_pipe *pipe);
+
 #endif
 #endif
diff --git a/net/sunrpc/auth_gss/auth_gss.c b/net/sunrpc/auth_gss/auth_gss.c
index 97912b4..d324563 100644
--- a/net/sunrpc/auth_gss/auth_gss.c
+++ b/net/sunrpc/auth_gss/auth_gss.c
@@ -592,6 +592,15 @@ out:
 	return err;
 }
 
+static bool
+gssd_running(struct sunrpc_net *sn)
+{
+	struct inode *inode = sn->gssd_dummy->d_inode;
+	struct rpc_pipe *pipe = RPC_I(inode)->pipe;
+
+	return rpc_pipe_is_open(pipe);
+}
+
 static inline int
 gss_create_upcall(struct gss_auth *gss_auth, struct gss_cred *gss_cred)
 {
@@ -610,15 +619,13 @@ retry:
 	err = 0;
 	/* Default timeout is 15s unless we know that gssd is not running */
 	timeout = 15 * HZ;
-	if (!sn->gssd_running)
+	if (!gssd_running(sn))
 		timeout = HZ >> 2;
 	gss_msg = gss_setup_upcall(gss_auth, cred);
 	if (PTR_ERR(gss_msg) == -EAGAIN) {
 		err = wait_event_interruptible_timeout(pipe_version_waitqueue,
 				sn->pipe_version >= 0, timeout);
 		if (sn->pipe_version < 0) {
-			if (err == 0)
-				sn->gssd_running = 0;
 			warn_gssd();
 			err = -EACCES;
 		}
diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
index 6b82968..39ab6bc 100644
--- a/net/sunrpc/netns.h
+++ b/net/sunrpc/netns.h
@@ -33,8 +33,6 @@ struct sunrpc_net {
 	int pipe_version;
 	atomic_t pipe_users;
 	struct proc_dir_entry *use_gssp_proc;
-
-	unsigned int gssd_running;
 };
 
 extern int sunrpc_net_id;
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 1c54de6..497261f 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -216,14 +216,11 @@ rpc_destroy_inode(struct inode *inode)
 static int
 rpc_pipe_open(struct inode *inode, struct file *filp)
 {
-	struct net *net = inode->i_sb->s_fs_info;
-	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
 	struct rpc_pipe *pipe;
 	int first_open;
 	int res = -ENXIO;
 
 	mutex_lock(&inode->i_mutex);
-	sn->gssd_running = 1;
 	pipe = RPC_I(inode)->pipe;
 	if (pipe == NULL)
 		goto out;
@@ -1227,7 +1224,6 @@ void rpc_pipefs_init_net(struct net *net)
 	struct sunrpc_net *sn = net_generic(net, sunrpc_net_id);
 
 	mutex_init(&sn->pipefs_sb_lock);
-	sn->gssd_running = 1;
 	sn->pipe_version = -1;
 }
 
@@ -1382,6 +1378,13 @@ err_depopulate:
 	return err;
 }
 
+bool
+rpc_pipe_is_open(struct rpc_pipe *pipe)
+{
+	return (pipe->nreaders || pipe->nwriters);
+}
+EXPORT_SYMBOL_GPL(rpc_pipe_is_open);
+
 static struct dentry *
 rpc_mount(struct file_system_type *fs_type,
 		int flags, const char *dev_name, void *data)
-- 
1.8.3.1

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Steve Dickson]

On 12/11/13 08:00, Jeff Layton wrote:
> We've gotten a lot of complaints recently about the 15s delay when
> doing a sec=sys mount without gssd running.
> 
> A large part of the problem is that the kernel isn't able to reliably
> detect when rpc.gssd is running. What we currently have is a
> gssd_running flag that is initially set to 1. When an upcall times out,
> that gets set to 0, and subsequent upcalls get a much shorter timeout
> (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> 
> The approach of using a flag like this is pretty inadequate. First, it
> doesn't eliminate the long delay on the initial upcall attempt. Also,
> if gssd spontaneously dies, then the flag will still be set to 1 until
> the next upcall attempt times out. Finally, it currently requires that
> the pipe be reopened in order to reset the flag back to true.
> 
> This patchset replaces that flag with a more reliable mechanism for
> detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> new "dummy" pipe that gssd will naturally find and hold open. We'll
> never send an upcall down this pipe, and writing to it always fails.
> But, since we can detect when something is holding it open, we can use
> that to determine whether gssd is running.
> 
> The current patch just uses this mechanism to replace the gssd_running
> flag with this new mechanism. This shortens the long delay when mounting
> without gssd running, but does not silence these warnings:
> 
>     RPC: AUTH_GSS upcall timed out.
>     Please check user daemon is running.
> 
> I'm willing to add a patch to do that, but I'm a little unclear on the
> best way to do so. Those messages are generated by the auth_gss code. We
> probably do want to print them if someone mounted with sec=krb5, but
> suppress them when mounting with sec=sys.
> 
> Do we need to somehow pass down that intent to auth_gss? Another idea
> would be to call gssd_running() from the nfs mount code and use that to
> determine whether to try and use krb5 at all...
> 
> Discuss!
I've just verified that a mount, with these patches, takes about 
1.2 seconds when rpc.gssd is not running.... With rpc.gssd it 
take about .2 seconds.

Tested-by: Steve Dickson <steved@redhat.com>

steved.

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Chuck Lever]

On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:

> We've gotten a lot of complaints recently about the 15s delay when
> doing a sec=sys mount without gssd running.
> 
> A large part of the problem is that the kernel isn't able to reliably
> detect when rpc.gssd is running. What we currently have is a
> gssd_running flag that is initially set to 1. When an upcall times out,
> that gets set to 0, and subsequent upcalls get a much shorter timeout
> (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> 
> The approach of using a flag like this is pretty inadequate. First, it
> doesn't eliminate the long delay on the initial upcall attempt. Also,
> if gssd spontaneously dies, then the flag will still be set to 1 until
> the next upcall attempt times out. Finally, it currently requires that
> the pipe be reopened in order to reset the flag back to true.
> 
> This patchset replaces that flag with a more reliable mechanism for
> detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> new "dummy" pipe that gssd will naturally find and hold open. We'll
> never send an upcall down this pipe, and writing to it always fails.
> But, since we can detect when something is holding it open, we can use
> that to determine whether gssd is running.
> 
> The current patch just uses this mechanism to replace the gssd_running
> flag with this new mechanism. This shortens the long delay when mounting
> without gssd running, but does not silence these warnings:
> 
>    RPC: AUTH_GSS upcall timed out.
>    Please check user daemon is running.
> 
> I'm willing to add a patch to do that, but I'm a little unclear on the
> best way to do so. Those messages are generated by the auth_gss code. We
> probably do want to print them if someone mounted with sec=krb5, but
> suppress them when mounting with sec=sys.
> 
> Do we need to somehow pass down that intent to auth_gss? Another idea
> would be to call gssd_running() from the nfs mount code and use that to
> determine whether to try and use krb5 at all...
> 
> Discuss!

I'd like to pursue the module loading solution as well.


> 
> Jeff Layton (2):
>  sunrpc: create a new dummy pipe for gssd to hold open
>  sunrpc: replace sunrpc_net->gssd_running flag with a better mechanism
> 
> include/linux/sunrpc/rpc_pipe_fs.h |  4 ++
> net/sunrpc/auth_gss/auth_gss.c     | 13 ++++--
> net/sunrpc/netns.h                 |  3 +-
> net/sunrpc/rpc_pipe.c              | 95 ++++++++++++++++++++++++++++++++++++--
> 4 files changed, 106 insertions(+), 9 deletions(-)
> 
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Jeff Layton]

On Tue, 12 Nov 2013 11:02:42 -0500
Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > We've gotten a lot of complaints recently about the 15s delay when
> > doing a sec=sys mount without gssd running.
> > 
> > A large part of the problem is that the kernel isn't able to reliably
> > detect when rpc.gssd is running. What we currently have is a
> > gssd_running flag that is initially set to 1. When an upcall times out,
> > that gets set to 0, and subsequent upcalls get a much shorter timeout
> > (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> > 
> > The approach of using a flag like this is pretty inadequate. First, it
> > doesn't eliminate the long delay on the initial upcall attempt. Also,
> > if gssd spontaneously dies, then the flag will still be set to 1 until
> > the next upcall attempt times out. Finally, it currently requires that
> > the pipe be reopened in order to reset the flag back to true.
> > 
> > This patchset replaces that flag with a more reliable mechanism for
> > detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> > new "dummy" pipe that gssd will naturally find and hold open. We'll
> > never send an upcall down this pipe, and writing to it always fails.
> > But, since we can detect when something is holding it open, we can use
> > that to determine whether gssd is running.
> > 
> > The current patch just uses this mechanism to replace the gssd_running
> > flag with this new mechanism. This shortens the long delay when mounting
> > without gssd running, but does not silence these warnings:
> > 
> >    RPC: AUTH_GSS upcall timed out.
> >    Please check user daemon is running.
> > 
> > I'm willing to add a patch to do that, but I'm a little unclear on the
> > best way to do so. Those messages are generated by the auth_gss code. We
> > probably do want to print them if someone mounted with sec=krb5, but
> > suppress them when mounting with sec=sys.
> > 
> > Do we need to somehow pass down that intent to auth_gss? Another idea
> > would be to call gssd_running() from the nfs mount code and use that to
> > determine whether to try and use krb5 at all...
> > 
> > Discuss!
> 
> I'd like to pursue the module loading solution as well.
> 

Sorry, I missed that part of the discussion.

What's the module loading solution?

-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Chuck Lever]

On Nov 12, 2013, at 11:08 AM, Jeff Layton <jlayton@redhat.com> wrote:

> On Tue, 12 Nov 2013 11:02:42 -0500
> Chuck Lever <chuck.lever@oracle.com> wrote:
> 
>> 
>> On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> We've gotten a lot of complaints recently about the 15s delay when
>>> doing a sec=sys mount without gssd running.
>>> 
>>> A large part of the problem is that the kernel isn't able to reliably
>>> detect when rpc.gssd is running. What we currently have is a
>>> gssd_running flag that is initially set to 1. When an upcall times out,
>>> that gets set to 0, and subsequent upcalls get a much shorter timeout
>>> (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
>>> 
>>> The approach of using a flag like this is pretty inadequate. First, it
>>> doesn't eliminate the long delay on the initial upcall attempt. Also,
>>> if gssd spontaneously dies, then the flag will still be set to 1 until
>>> the next upcall attempt times out. Finally, it currently requires that
>>> the pipe be reopened in order to reset the flag back to true.
>>> 
>>> This patchset replaces that flag with a more reliable mechanism for
>>> detecting when gssd is running. When rpc_pipefs is mounted, it creates a
>>> new "dummy" pipe that gssd will naturally find and hold open. We'll
>>> never send an upcall down this pipe, and writing to it always fails.
>>> But, since we can detect when something is holding it open, we can use
>>> that to determine whether gssd is running.
>>> 
>>> The current patch just uses this mechanism to replace the gssd_running
>>> flag with this new mechanism. This shortens the long delay when mounting
>>> without gssd running, but does not silence these warnings:
>>> 
>>>   RPC: AUTH_GSS upcall timed out.
>>>   Please check user daemon is running.
>>> 
>>> I'm willing to add a patch to do that, but I'm a little unclear on the
>>> best way to do so. Those messages are generated by the auth_gss code. We
>>> probably do want to print them if someone mounted with sec=krb5, but
>>> suppress them when mounting with sec=sys.
>>> 
>>> Do we need to somehow pass down that intent to auth_gss? Another idea
>>> would be to call gssd_running() from the nfs mount code and use that to
>>> determine whether to try and use krb5 at all...
>>> 
>>> Discuss!
>> 
>> I'd like to pursue the module loading solution as well.
>> 
> 
> Sorry, I missed that part of the discussion.
> 
> What's the module loading solution?

Load auth_rpcgss.ko only when rpc.gssd has been started.  See the "[PATCH] Adding the nfs4_secure_mounts bool" thread... If auth_rpcgss.ko is not loaded, the kernel won't ever try to do an upcall.

Then, systemd can be used to restart rpc.gssd if it crashes, maybe?  Just a thought.

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

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Myklebust, Trond]

On Tue, 2013-11-12 at 11:15 -0500, Chuck Lever wrote:
> On Nov 12, 2013, at 11:08 AM, Jeff Layton <jlayton@redhat.com> wrote:
> 
> > On Tue, 12 Nov 2013 11:02:42 -0500
> > Chuck Lever <chuck.lever@oracle.com> wrote:
> > 
> >> 
> >> On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
> >> 
> > What's the module loading solution?
> 
> Load auth_rpcgss.ko only when rpc.gssd has been started.  See the "[PATCH] Adding the nfs4_secure_mounts bool" thread... If auth_rpcgss.ko is not loaded, the kernel won't ever try to do an upcall.
> 
> Then, systemd can be used to restart rpc.gssd if it crashes, maybe?  Just a thought.

You can trivially defeat that by compiling the gss code into the kernel.

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Chuck Lever]

On Nov 12, 2013, at 11:56 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-11-12 at 11:15 -0500, Chuck Lever wrote:
>> On Nov 12, 2013, at 11:08 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> On Tue, 12 Nov 2013 11:02:42 -0500
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>> 
>>>> 
>>>> On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
>>>> 
>>> What's the module loading solution?
>> 
>> Load auth_rpcgss.ko only when rpc.gssd has been started.  See the "[PATCH] Adding the nfs4_secure_mounts bool" thread... If auth_rpcgss.ko is not loaded, the kernel won't ever try to do an upcall.
>> 
>> Then, systemd can be used to restart rpc.gssd if it crashes, maybe?  Just a thought.
> 
> You can trivially defeat that by compiling the gss code into the kernel.

My solution is a workaround.  It works for distributions that do not compile auth_rpcgss into the kernel, since they are the ones who provide an appropriate init script architecture.

But it does not require a kernel code change, and it applies to every distribution I'm aware of.  Since distributions typically do not use built-in GSS support, I'm not too concerned about whoever might be left over.

And I never said that we shouldn't continue to mitigate the upcall timeout.  Note that Jeff's solution didn't entirely eliminate the problem either.  With either or both of these solutions in place, we are better off than we were before.

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

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Chuck Lever]

On Nov 12, 2013, at 11:56 AM, "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-11-12 at 11:15 -0500, Chuck Lever wrote:
>> On Nov 12, 2013, at 11:08 AM, Jeff Layton <jlayton@redhat.com> wrote:
>> 
>>> On Tue, 12 Nov 2013 11:02:42 -0500
>>> Chuck Lever <chuck.lever@oracle.com> wrote:
>>> 
>>>> 
>>>> On Nov 12, 2013, at 8:00 AM, Jeff Layton <jlayton@redhat.com> wrote:
>>>> 
>>> What's the module loading solution?
>> 
>> Load auth_rpcgss.ko only when rpc.gssd has been started.  See the "[PATCH] Adding the nfs4_secure_mounts bool" thread... If auth_rpcgss.ko is not loaded, the kernel won't ever try to do an upcall.
>> 
>> Then, systemd can be used to restart rpc.gssd if it crashes, maybe?  Just a thought.
> 
> You can trivially defeat that by compiling the gss code into the kernel.

And also: my preferred solution is to run rpc.gssd unconditionally.  But some find that onerous.

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

Re: [PATCH 1/2] sunrpc: create a new dummy pipe for gssd to hold open

[Myklebust, Trond]

On Tue, 2013-11-12 at 08:00 -0500, Jeff Layton wrote:
> rpc.gssd will naturally hold open any pipe named */clnt*/gssd that shows
> up under rpc_pipefs. That behavior gives us a reliable mechanism to tell
> whether it's actually running or not.
> 
> Create a new toplevel "dummy" directory in rpc_pipefs when it's mounted.
> Under that directory create another directory called "clntXX", and then
> within that a pipe called "gssd".
> 
> We'll never send an upcall along that pipe, and any downcall written to
> it will just return -EINVAL.
> 
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  net/sunrpc/netns.h    |  1 +
>  net/sunrpc/rpc_pipe.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 85 insertions(+)
> 
> diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
> index 779742c..6b82968 100644
> --- a/net/sunrpc/netns.h
> +++ b/net/sunrpc/netns.h
> @@ -15,6 +15,7 @@ struct sunrpc_net {
>  
>  	struct super_block *pipefs_sb;
>  	struct mutex pipefs_sb_lock;
> +	struct dentry *gssd_dummy;
>  
>  	struct list_head all_clients;
>  	spinlock_t rpc_client_lock;
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index f94567b..1c54de6 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -1168,6 +1168,7 @@ enum {
>  	RPCAUTH_nfsd4_cb,
>  	RPCAUTH_cache,
>  	RPCAUTH_nfsd,
> +	RPCAUTH_dummy,
>  	RPCAUTH_RootEOF
>  };
>  
> @@ -1204,6 +1205,10 @@ static const struct rpc_filelist files[] = {
>  		.name = "nfsd",
>  		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
>  	},
> +	[RPCAUTH_dummy] = {
> +		.name = "dummy",

"gssd" would be nicer.

> +		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> +	},
>  };
>  
>  /*
> @@ -1253,6 +1258,80 @@ void rpc_put_sb_net(const struct net *net)
>  }
>  EXPORT_SYMBOL_GPL(rpc_put_sb_net);
>  
> +static const struct rpc_filelist dummy_clnt_dir[] = {
> +	[0] = {
> +		.name = "clntXX",
> +		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> +	},
> +};
> +
> +static ssize_t
> +dummy_downcall(struct file *filp, const char __user *src, size_t len)
> +{
> +	return -EINVAL;
> +}
> +
> +static const struct rpc_pipe_ops dummy_pipe_ops = {
> +	.upcall		= rpc_pipe_generic_upcall,
> +	.downcall	= dummy_downcall,
> +};
> +
> +/**
> + * rpc_gssd_dummy_populate - create a dummy gssd pipe
> + * @root: root of the rpc_pipefs filesystem
> + *
> + * Create a dummy set of directories and a pipe that gssd can hold open to
> + * indicate that it is up and running.
> + */
> +static struct dentry *
> +rpc_gssd_dummy_populate(struct dentry *root)
> +{
> +	int ret = 0;
> +	struct dentry *gssd_dentry;
> +	struct dentry *clnt_dentry = NULL;
> +	struct dentry *pipe_dentry = NULL;
> +	struct rpc_pipe *pipe_data = NULL;
> +	struct qstr q = QSTR_INIT(files[RPCAUTH_dummy].name,
> +				  strlen(files[RPCAUTH_dummy].name));
> +
> +	/* We should never get this far if "gssd" doesn't exist */
> +	gssd_dentry = d_hash_and_lookup(root, &q);
> +	if (!gssd_dentry)
> +		return ERR_PTR(-ENOENT);
> +
> +	ret = rpc_populate(gssd_dentry, dummy_clnt_dir, 0, 1, NULL);
> +	if (ret) {
> +		pipe_dentry = ERR_PTR(ret);
> +		goto out;
> +	}
> +
> +	pipe_data = rpc_mkpipe_data(&dummy_pipe_ops, 0);
> +	if (IS_ERR(pipe_data)) {
> +		pipe_dentry = ERR_CAST(pipe_data);
> +		pipe_data = NULL;
> +		goto out;
> +	}
> +
> +	q.name = dummy_clnt_dir[0].name;
> +	q.len = strlen(dummy_clnt_dir[0].name);
> +	clnt_dentry = d_hash_and_lookup(gssd_dentry, &q);
> +	if (!clnt_dentry) {
> +		pipe_dentry = ERR_PTR(-ENOENT);
> +		goto out;
> +	}
> +
> +	pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
> +	if (IS_ERR(pipe_dentry))
> +		goto out;
> +
> +	pipe_data = NULL;
> +out:
> +	rpc_destroy_pipe_data(pipe_data);
> +	dput(clnt_dentry);
> +	dput(gssd_dentry);
> +	return pipe_dentry;
> +}
> +
>  static int
>  rpc_fill_super(struct super_block *sb, void *data, int silent)
>  {
> @@ -1275,6 +1354,11 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
>  		return -ENOMEM;
>  	if (rpc_populate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF, NULL))
>  		return -ENOMEM;
> +
> +	sn->gssd_dummy = rpc_gssd_dummy_populate(root);
> +	if (IS_ERR(sn->gssd_dummy))
> +		return PTR_ERR(sn->gssd_dummy);
> +
>  	dprintk("RPC:       sending pipefs MOUNT notification for net %p%s\n",
>  		net, NET_NAME(net));
>  	mutex_lock(&sn->pipefs_sb_lock);

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 1/2] sunrpc: create a new dummy pipe for gssd to hold open

[Jeff Layton]

On Tue, 12 Nov 2013 17:36:15 +0000
"Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:

> On Tue, 2013-11-12 at 08:00 -0500, Jeff Layton wrote:
> > rpc.gssd will naturally hold open any pipe named */clnt*/gssd that shows
> > up under rpc_pipefs. That behavior gives us a reliable mechanism to tell
> > whether it's actually running or not.
> > 
> > Create a new toplevel "dummy" directory in rpc_pipefs when it's mounted.
> > Under that directory create another directory called "clntXX", and then
> > within that a pipe called "gssd".
> > 
> > We'll never send an upcall along that pipe, and any downcall written to
> > it will just return -EINVAL.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  net/sunrpc/netns.h    |  1 +
> >  net/sunrpc/rpc_pipe.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 85 insertions(+)
> > 
> > diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
> > index 779742c..6b82968 100644
> > --- a/net/sunrpc/netns.h
> > +++ b/net/sunrpc/netns.h
> > @@ -15,6 +15,7 @@ struct sunrpc_net {
> >  
> >  	struct super_block *pipefs_sb;
> >  	struct mutex pipefs_sb_lock;
> > +	struct dentry *gssd_dummy;
> >  
> >  	struct list_head all_clients;
> >  	spinlock_t rpc_client_lock;
> > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> > index f94567b..1c54de6 100644
> > --- a/net/sunrpc/rpc_pipe.c
> > +++ b/net/sunrpc/rpc_pipe.c
> > @@ -1168,6 +1168,7 @@ enum {
> >  	RPCAUTH_nfsd4_cb,
> >  	RPCAUTH_cache,
> >  	RPCAUTH_nfsd,
> > +	RPCAUTH_dummy,
> >  	RPCAUTH_RootEOF
> >  };
> >  
> > @@ -1204,6 +1205,10 @@ static const struct rpc_filelist files[] = {
> >  		.name = "nfsd",
> >  		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> >  	},
> > +	[RPCAUTH_dummy] = {
> > +		.name = "dummy",
> 
> "gssd" would be nicer.
> 

Ha! That's what my original patch had, but I thought that made it look
too legit. I can respin and change it though if you like...

> > +		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> > +	},
> >  };
> >  
> >  /*
> > @@ -1253,6 +1258,80 @@ void rpc_put_sb_net(const struct net *net)
> >  }
> >  EXPORT_SYMBOL_GPL(rpc_put_sb_net);
> >  
> > +static const struct rpc_filelist dummy_clnt_dir[] = {
> > +	[0] = {
> > +		.name = "clntXX",
> > +		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> > +	},
> > +};
> > +
> > +static ssize_t
> > +dummy_downcall(struct file *filp, const char __user *src, size_t len)
> > +{
> > +	return -EINVAL;
> > +}
> > +
> > +static const struct rpc_pipe_ops dummy_pipe_ops = {
> > +	.upcall		= rpc_pipe_generic_upcall,
> > +	.downcall	= dummy_downcall,
> > +};
> > +
> > +/**
> > + * rpc_gssd_dummy_populate - create a dummy gssd pipe
> > + * @root: root of the rpc_pipefs filesystem
> > + *
> > + * Create a dummy set of directories and a pipe that gssd can hold open to
> > + * indicate that it is up and running.
> > + */
> > +static struct dentry *
> > +rpc_gssd_dummy_populate(struct dentry *root)
> > +{
> > +	int ret = 0;
> > +	struct dentry *gssd_dentry;
> > +	struct dentry *clnt_dentry = NULL;
> > +	struct dentry *pipe_dentry = NULL;
> > +	struct rpc_pipe *pipe_data = NULL;
> > +	struct qstr q = QSTR_INIT(files[RPCAUTH_dummy].name,
> > +				  strlen(files[RPCAUTH_dummy].name));
> > +
> > +	/* We should never get this far if "gssd" doesn't exist */
> > +	gssd_dentry = d_hash_and_lookup(root, &q);
> > +	if (!gssd_dentry)
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	ret = rpc_populate(gssd_dentry, dummy_clnt_dir, 0, 1, NULL);
> > +	if (ret) {
> > +		pipe_dentry = ERR_PTR(ret);
> > +		goto out;
> > +	}
> > +
> > +	pipe_data = rpc_mkpipe_data(&dummy_pipe_ops, 0);
> > +	if (IS_ERR(pipe_data)) {
> > +		pipe_dentry = ERR_CAST(pipe_data);
> > +		pipe_data = NULL;
> > +		goto out;
> > +	}
> > +
> > +	q.name = dummy_clnt_dir[0].name;
> > +	q.len = strlen(dummy_clnt_dir[0].name);
> > +	clnt_dentry = d_hash_and_lookup(gssd_dentry, &q);
> > +	if (!clnt_dentry) {
> > +		pipe_dentry = ERR_PTR(-ENOENT);
> > +		goto out;
> > +	}
> > +
> > +	pipe_dentry = rpc_mkpipe_dentry(clnt_dentry, "gssd", NULL, pipe_data);
> > +	if (IS_ERR(pipe_dentry))
> > +		goto out;
> > +
> > +	pipe_data = NULL;
> > +out:
> > +	rpc_destroy_pipe_data(pipe_data);
> > +	dput(clnt_dentry);
> > +	dput(gssd_dentry);
> > +	return pipe_dentry;
> > +}
> > +
> >  static int
> >  rpc_fill_super(struct super_block *sb, void *data, int silent)
> >  {
> > @@ -1275,6 +1354,11 @@ rpc_fill_super(struct super_block *sb, void *data, int silent)
> >  		return -ENOMEM;
> >  	if (rpc_populate(root, files, RPCAUTH_lockd, RPCAUTH_RootEOF, NULL))
> >  		return -ENOMEM;
> > +
> > +	sn->gssd_dummy = rpc_gssd_dummy_populate(root);
> > +	if (IS_ERR(sn->gssd_dummy))
> > +		return PTR_ERR(sn->gssd_dummy);
> > +
> >  	dprintk("RPC:       sending pipefs MOUNT notification for net %p%s\n",
> >  		net, NET_NAME(net));
> >  	mutex_lock(&sn->pipefs_sb_lock);
> 


-- 
Jeff Layton <jlayton@redhat.com>

Re: [PATCH 1/2] sunrpc: create a new dummy pipe for gssd to hold open

[Myklebust, Trond]

On Tue, 2013-11-12 at 12:44 -0500, Jeff Layton wrote:
> On Tue, 12 Nov 2013 17:36:15 +0000
> "Myklebust, Trond" <Trond.Myklebust@netapp.com> wrote:
> 
> > On Tue, 2013-11-12 at 08:00 -0500, Jeff Layton wrote:
> > > rpc.gssd will naturally hold open any pipe named */clnt*/gssd that shows
> > > up under rpc_pipefs. That behavior gives us a reliable mechanism to tell
> > > whether it's actually running or not.
> > > 
> > > Create a new toplevel "dummy" directory in rpc_pipefs when it's mounted.
> > > Under that directory create another directory called "clntXX", and then
> > > within that a pipe called "gssd".
> > > 
> > > We'll never send an upcall along that pipe, and any downcall written to
> > > it will just return -EINVAL.
> > > 
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  net/sunrpc/netns.h    |  1 +
> > >  net/sunrpc/rpc_pipe.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 85 insertions(+)
> > > 
> > > diff --git a/net/sunrpc/netns.h b/net/sunrpc/netns.h
> > > index 779742c..6b82968 100644
> > > --- a/net/sunrpc/netns.h
> > > +++ b/net/sunrpc/netns.h
> > > @@ -15,6 +15,7 @@ struct sunrpc_net {
> > >  
> > >  	struct super_block *pipefs_sb;
> > >  	struct mutex pipefs_sb_lock;
> > > +	struct dentry *gssd_dummy;
> > >  
> > >  	struct list_head all_clients;
> > >  	spinlock_t rpc_client_lock;
> > > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> > > index f94567b..1c54de6 100644
> > > --- a/net/sunrpc/rpc_pipe.c
> > > +++ b/net/sunrpc/rpc_pipe.c
> > > @@ -1168,6 +1168,7 @@ enum {
> > >  	RPCAUTH_nfsd4_cb,
> > >  	RPCAUTH_cache,
> > >  	RPCAUTH_nfsd,
> > > +	RPCAUTH_dummy,
> > >  	RPCAUTH_RootEOF
> > >  };
> > >  
> > > @@ -1204,6 +1205,10 @@ static const struct rpc_filelist files[] = {
> > >  		.name = "nfsd",
> > >  		.mode = S_IFDIR | S_IRUGO | S_IXUGO,
> > >  	},
> > > +	[RPCAUTH_dummy] = {
> > > +		.name = "dummy",
> > 
> > "gssd" would be nicer.
> > 
> 
> Ha! That's what my original patch had, but I thought that made it look
> too legit. I can respin and change it though if you like...

I'd prefer that. At least 'gssd' makes it clear that this is a directory
that we want gssd to be snooping.

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2908 bytes --]

On Tue, 12 Nov 2013 10:21:40 -0500 Steve Dickson <SteveD@redhat.com> wrote:

> On 12/11/13 08:00, Jeff Layton wrote:
> > We've gotten a lot of complaints recently about the 15s delay when
> > doing a sec=sys mount without gssd running.
> > 
> > A large part of the problem is that the kernel isn't able to reliably
> > detect when rpc.gssd is running. What we currently have is a
> > gssd_running flag that is initially set to 1. When an upcall times out,
> > that gets set to 0, and subsequent upcalls get a much shorter timeout
> > (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> > 
> > The approach of using a flag like this is pretty inadequate. First, it
> > doesn't eliminate the long delay on the initial upcall attempt. Also,
> > if gssd spontaneously dies, then the flag will still be set to 1 until
> > the next upcall attempt times out. Finally, it currently requires that
> > the pipe be reopened in order to reset the flag back to true.
> > 
> > This patchset replaces that flag with a more reliable mechanism for
> > detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> > new "dummy" pipe that gssd will naturally find and hold open. We'll
> > never send an upcall down this pipe, and writing to it always fails.
> > But, since we can detect when something is holding it open, we can use
> > that to determine whether gssd is running.
> > 
> > The current patch just uses this mechanism to replace the gssd_running
> > flag with this new mechanism. This shortens the long delay when mounting
> > without gssd running, but does not silence these warnings:
> > 
> >     RPC: AUTH_GSS upcall timed out.
> >     Please check user daemon is running.
> > 
> > I'm willing to add a patch to do that, but I'm a little unclear on the
> > best way to do so. Those messages are generated by the auth_gss code. We
> > probably do want to print them if someone mounted with sec=krb5, but
> > suppress them when mounting with sec=sys.
> > 
> > Do we need to somehow pass down that intent to auth_gss? Another idea
> > would be to call gssd_running() from the nfs mount code and use that to
> > determine whether to try and use krb5 at all...
> > 
> > Discuss!
> I've just verified that a mount, with these patches, takes about 
> 1.2 seconds when rpc.gssd is not running.... With rpc.gssd it 
> take about .2 seconds.
> 
> Tested-by: Steve Dickson <steved@redhat.com>
>

Still sounds like about one second too long.

In that patch I see:

 	timeout = 15 * HZ;
-	if (!sn->gssd_running)
+	if (!gssd_running(sn))
 		timeout = HZ >> 2;

Given that "!gssd_running(sn)" is now certain knowledge rather than a hint,
can't we just skip the upcall and any timeout?
i.e.
 	timeout = 15 * HZ;
-	if (!sn->gssd_running)
+	if (!gssd_running(sn))
- 		timeout = HZ >> 2;
+		return -EACCES;

??

NeilBrown


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 0/2] sunrpc: more reliable detection of running gssd

[Jeff Layton]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

On Wed, 13 Nov 2013 09:15:37 +1100
NeilBrown <neilb@suse.de> wrote:

> On Tue, 12 Nov 2013 10:21:40 -0500 Steve Dickson <SteveD@redhat.com> wrote:
> 
> > On 12/11/13 08:00, Jeff Layton wrote:
> > > We've gotten a lot of complaints recently about the 15s delay when
> > > doing a sec=sys mount without gssd running.
> > > 
> > > A large part of the problem is that the kernel isn't able to reliably
> > > detect when rpc.gssd is running. What we currently have is a
> > > gssd_running flag that is initially set to 1. When an upcall times out,
> > > that gets set to 0, and subsequent upcalls get a much shorter timeout
> > > (1/4s instead of 15s). It's reset back to '1' when a pipe is reopened.
> > > 
> > > The approach of using a flag like this is pretty inadequate. First, it
> > > doesn't eliminate the long delay on the initial upcall attempt. Also,
> > > if gssd spontaneously dies, then the flag will still be set to 1 until
> > > the next upcall attempt times out. Finally, it currently requires that
> > > the pipe be reopened in order to reset the flag back to true.
> > > 
> > > This patchset replaces that flag with a more reliable mechanism for
> > > detecting when gssd is running. When rpc_pipefs is mounted, it creates a
> > > new "dummy" pipe that gssd will naturally find and hold open. We'll
> > > never send an upcall down this pipe, and writing to it always fails.
> > > But, since we can detect when something is holding it open, we can use
> > > that to determine whether gssd is running.
> > > 
> > > The current patch just uses this mechanism to replace the gssd_running
> > > flag with this new mechanism. This shortens the long delay when mounting
> > > without gssd running, but does not silence these warnings:
> > > 
> > >     RPC: AUTH_GSS upcall timed out.
> > >     Please check user daemon is running.
> > > 
> > > I'm willing to add a patch to do that, but I'm a little unclear on the
> > > best way to do so. Those messages are generated by the auth_gss code. We
> > > probably do want to print them if someone mounted with sec=krb5, but
> > > suppress them when mounting with sec=sys.
> > > 
> > > Do we need to somehow pass down that intent to auth_gss? Another idea
> > > would be to call gssd_running() from the nfs mount code and use that to
> > > determine whether to try and use krb5 at all...
> > > 
> > > Discuss!
> > I've just verified that a mount, with these patches, takes about 
> > 1.2 seconds when rpc.gssd is not running.... With rpc.gssd it 
> > take about .2 seconds.
> > 
> > Tested-by: Steve Dickson <steved@redhat.com>
> >
> 
> Still sounds like about one second too long.
> 
> In that patch I see:
> 
>  	timeout = 15 * HZ;
> -	if (!sn->gssd_running)
> +	if (!gssd_running(sn))
>  		timeout = HZ >> 2;
> 

Yeah, it's not clear to me where the extra delay there comes from
either. I was sort of hoping Steve would track that down... ;)

> Given that "!gssd_running(sn)" is now certain knowledge rather than a hint,
> can't we just skip the upcall and any timeout?
> i.e.
>  	timeout = 15 * HZ;
> -	if (!sn->gssd_running)
> +	if (!gssd_running(sn))
> - 		timeout = HZ >> 2;
> +		return -EACCES;
> 

Good point...I was trying to keep the semantic changes to a minimum,
but that does make sense. One minor nit...with the above you'll never
hit warn_gss(), so it probably makes sense to put that in there too.

I've got a v2 of the patchset that I'm working on that fixes a couple
of bugs, makes the dir name change that Trond wants, and also has a
patch that makes nfs4_init_client skip trying krb5i if gssd isn't up.
I'll probably post that tomorrow...

-- 
Jeff Layton <jlayton@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]