[RFC] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

[RFC] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

[Boaz Harrosh]

Bruce hi

This is the patch I'm currently using to be able to run. It is
your patch but the cl_firststate = 1 is promoted to before any
failures.

Please Also consider adding and promoting some of these prints
to DMESGs so Admins can know they have a broken setup. Like
directories do not exist or permission not properly set.
Though I admit the messages should be properly worded.

Thanks
Boaz

---
From: "J. Bruce Fields" <bfields@redhat.com>
Date: Fri, 12 Aug 2011 17:18:04 -0700
Subject: [PATCH] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

J. Bruce Fields:
    Even if we fail to write a recovery record to stable storage, we should
    still mark the client as having acquired its first state.  Otherwise we
    leave 4.1 clients with indefinite ERR_GRACE returns.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Boaz Harrosh:

    I don't think this fix is enough what about the failure of nfs4_save_creds
    It can only fail with -ENOMEM do you hang the client in this case?

    I think Some of these prints should be delegated to a KERN_ERR since
    it is a possible setup problem.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/nfsd/nfs4recover.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 29d77f6..baedd89 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -129,9 +129,12 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	if (!rec_file || clp->cl_firststate)
 		return 0;
 
+	clp->cl_firststate = 1;
 	status = nfs4_save_creds(&original_cred);
-	if (status < 0)
+	if (unlikely(status < 0)) {
+		printk(KERN_ERR "!!!nfs4_save_creds Returned => %d\n", status);
 		return status;
+	}
 
 	dir = rec_file->f_path.dentry;
 	/* lock the parent */
@@ -140,26 +143,30 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	dentry = lookup_one_len(dname, dir, HEXDIR_LEN-1);
 	if (IS_ERR(dentry)) {
 		status = PTR_ERR(dentry);
+		printk(KERN_ERR "NFSD: lookup_one_len => %d\n", status);
 		goto out_unlock;
 	}
 	status = -EEXIST;
 	if (dentry->d_inode) {
-		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
+		printk(KERN_ERR "NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
 		goto out_put;
 	}
 	status = mnt_want_write(rec_file->f_path.mnt);
-	if (status)
+	if (unlikely(status)) {
+		printk(KERN_ERR "!!!mnt_want_write Returned => %d\n", status);
 		goto out_put;
+	}
 	status = vfs_mkdir(dir->d_inode, dentry, S_IRWXU);
+	if (unlikely(status))
+		printk(KERN_ERR "!!!vfs_mkdir Returned => %d\n", status);
+
 	mnt_drop_write(rec_file->f_path.mnt);
 out_put:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
-	if (status == 0) {
-		clp->cl_firststate = 1;
+	if (status == 0)
 		vfs_fsync(rec_file, 0);
-	}
 	nfs4_reset_creds(original_cred);
 	dprintk("NFSD: nfsd4_create_clid_dir returns %d\n", status);
 	return status;
-- 
1.7.6

Re: [RFC] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

[J. Bruce Fields]

On Fri, Aug 12, 2011 at 05:30:12PM -0700, Boaz Harrosh wrote:
> Bruce hi
> 
> This is the patch I'm currently using to be able to run. It is
> your patch but the cl_firststate = 1 is promoted to before any
> failures.

Makes sense, thanks.

> Please Also consider adding and promoting some of these prints
> to DMESGs so Admins can know they have a broken setup. Like
> directories do not exist or permission not properly set.

OK, I agree, though a printk in every failure case seems overkill; could
you live with the following?

--b.

commit 1f2c6beb157790cfb253e0c856e97c80e8b1bbfd
Author: Boaz Harrosh <bharrosh@panasas.com>
Date:   Fri Aug 12 17:30:12 2011 -0700

    nfsd4: fix failure to end nfsd4 grace period
    
    Even if we fail to write a recovery record, we should still mark the
    client as having acquired its first state.  Otherwise we leave 4.1
    clients with indefinite ERR_GRACE returns.
    
    However, an inability to write stable storage records may cause failures
    of reboot recovery, and the problem should still be brought to the
    server administrator's attention.
    
    So, make sure the error is logged.
    
    These errors shouldn't normally be triggered on a corectly functioning
    server--this isn't a case where a misconfigured client could spam the
    logs.
    
    Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 02eb38e..1ea241a 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -129,6 +129,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	if (!rec_file || clp->cl_firststate)
 		return 0;
 
+	clp->cl_firststate = 1;
 	status = nfs4_save_creds(&original_cred);
 	if (status < 0)
 		return status;
@@ -143,10 +144,8 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 		goto out_unlock;
 	}
 	status = -EEXIST;
-	if (dentry->d_inode) {
-		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
+	if (dentry->d_inode)
 		goto out_put;
-	}
 	status = mnt_want_write(rec_file->f_path.mnt);
 	if (status)
 		goto out_put;
@@ -156,12 +155,14 @@ out_put:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
-	if (status == 0) {
-		clp->cl_firststate = 1;
+	if (status == 0)
 		vfs_fsync(rec_file, 0);
-	}
+	else
+		printk(KERN_ERR "NFSD: failed to write recovery record"
+				" (err %d); please check that recovery"
+				" directory exists and is writeable",
+				status);
 	nfs4_reset_creds(original_cred);
-	dprintk("NFSD: nfsd4_create_clid_dir returns %d\n", status);
 	return status;
 }
 

Re: [RFC] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

[Boaz Harrosh]

On 08/26/2011 01:19 PM, J. Bruce Fields wrote:
> On Fri, Aug 12, 2011 at 05:30:12PM -0700, Boaz Harrosh wrote:

<snip>

> @@ -156,12 +155,14 @@ out_put:
>  	dput(dentry);
>  out_unlock:
>  	mutex_unlock(&dir->d_inode->i_mutex);
> -	if (status == 0) {
> -		clp->cl_firststate = 1;
> +	if (status == 0)
>  		vfs_fsync(rec_file, 0);
> -	}
> +	else
> +		printk(KERN_ERR "NFSD: failed to write recovery record"
> +				" (err %d); please check that recovery"
> +				" directory exists and is writeable",
> +				status);

Bruce Hi

Which of the variables has the directory path?
Would we like to add that information in the print. From my experience
alot of times those type of problems is the mis-communication of
the path names, and it helps to see what name was actually attempted.

Other wise looks good
Thanks


>  	nfs4_reset_creds(original_cred);
> -	dprintk("NFSD: nfsd4_create_clid_dir returns %d\n", status);
>  	return status;
>  }
>  

Re: [RFC] nfsd4: This is: "fix failure to end nfsd4 grace period" ++

[J. Bruce Fields]

On Fri, Aug 26, 2011 at 05:22:56PM -0700, Boaz Harrosh wrote:
> Which of the variables has the directory path?
> Would we like to add that information in the print. From my experience
> alot of times those type of problems is the mis-communication of
> the path names, and it helps to see what name was actually attempted.

Yeah, I wanted to then realized I'd have to muck about with some other
stuff to get access to the directory name.

Curse these patch reviewers, catching me being lazy.

--b.

[PATCH 1/2] nfsd4: simplify recovery dir setting ++

[J. Bruce Fields]

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

Move around some of this code, simplify a bit.

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4recover.c |   36 ++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4state.c   |   41 +----------------------------------------
 fs/nfsd/state.h       |    2 +-
 3 files changed, 34 insertions(+), 45 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 02eb38e..3e6ebcf 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -45,6 +45,7 @@
 
 /* Globals */
 static struct file *rec_file;
+static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
 
 static int
 nfs4_save_creds(const struct cred **original_creds)
@@ -354,13 +355,13 @@ nfsd4_recdir_load(void) {
  */
 
 void
-nfsd4_init_recdir(char *rec_dirname)
+nfsd4_init_recdir()
 {
 	const struct cred *original_cred;
 	int status;
 
 	printk("NFSD: Using %s as the NFSv4 state recovery directory\n",
-			rec_dirname);
+			user_recovery_dirname);
 
 	BUG_ON(rec_file);
 
@@ -372,10 +373,10 @@ nfsd4_init_recdir(char *rec_dirname)
 		return;
 	}
 
-	rec_file = filp_open(rec_dirname, O_RDONLY | O_DIRECTORY, 0);
+	rec_file = filp_open(user_recovery_dirname, O_RDONLY | O_DIRECTORY, 0);
 	if (IS_ERR(rec_file)) {
 		printk("NFSD: unable to find recovery directory %s\n",
-				rec_dirname);
+				user_recovery_dirname);
 		rec_file = NULL;
 	}
 
@@ -390,3 +391,30 @@ nfsd4_shutdown_recdir(void)
 	fput(rec_file);
 	rec_file = NULL;
 }
+
+/*
+ * Change the NFSv4 recovery directory to recdir.
+ */
+int
+nfs4_reset_recoverydir(char *recdir)
+{
+	int status;
+	struct path path;
+
+	status = kern_path(recdir, LOOKUP_FOLLOW, &path);
+	if (status)
+		return status;
+	status = -ENOTDIR;
+	if (S_ISDIR(path.dentry->d_inode->i_mode)) {
+		strcpy(user_recovery_dirname, recdir);
+		status = 0;
+	}
+	path_put(&path);
+	return status;
+}
+
+char *
+nfs4_recoverydir(void)
+{
+	return user_recovery_dirname;
+}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f71d73c..05e3f85 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -63,8 +63,6 @@ static u64 current_sessionid = 1;
 static struct nfs4_stateid * find_stateid(stateid_t *stid, int flags);
 static struct nfs4_delegation * search_for_delegation(stateid_t *stid);
 static struct nfs4_delegation * find_delegation_stateid(struct inode *ino, stateid_t *stid);
-static char user_recovery_dirname[PATH_MAX] = "/var/lib/nfs/v4recovery";
-static void nfs4_set_recdir(char *recdir);
 static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
 
 /* Locking: */
@@ -4475,7 +4473,7 @@ nfsd4_load_reboot_recovery_data(void)
 	int status;
 
 	nfs4_lock_state();
-	nfsd4_init_recdir(user_recovery_dirname);
+	nfsd4_init_recdir();
 	status = nfsd4_recdir_load();
 	nfs4_unlock_state();
 	if (status)
@@ -4584,40 +4582,3 @@ nfs4_state_shutdown(void)
 	nfs4_unlock_state();
 	nfsd4_destroy_callback_queue();
 }
-
-/*
- * user_recovery_dirname is protected by the nfsd_mutex since it's only
- * accessed when nfsd is starting.
- */
-static void
-nfs4_set_recdir(char *recdir)
-{
-	strcpy(user_recovery_dirname, recdir);
-}
-
-/*
- * Change the NFSv4 recovery directory to recdir.
- */
-int
-nfs4_reset_recoverydir(char *recdir)
-{
-	int status;
-	struct path path;
-
-	status = kern_path(recdir, LOOKUP_FOLLOW, &path);
-	if (status)
-		return status;
-	status = -ENOTDIR;
-	if (S_ISDIR(path.dentry->d_inode->i_mode)) {
-		nfs4_set_recdir(recdir);
-		status = 0;
-	}
-	path_put(&path);
-	return status;
-}
-
-char *
-nfs4_recoverydir(void)
-{
-	return user_recovery_dirname;
-}
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 1f90cab..66179ac 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -493,7 +493,7 @@ extern void nfsd4_destroy_callback_queue(void);
 extern void nfsd4_shutdown_callback(struct nfs4_client *);
 extern void nfs4_put_delegation(struct nfs4_delegation *dp);
 extern __be32 nfs4_make_rec_clidname(char *clidname, struct xdr_netobj *clname);
-extern void nfsd4_init_recdir(char *recdir_name);
+extern void nfsd4_init_recdir(void);
 extern int nfsd4_recdir_load(void);
 extern void nfsd4_shutdown_recdir(void);
 extern int nfs4_client_to_reclaim(const char *name);
-- 
1.7.4.1

[PATCH 2/2] nfsd4: fix failure to end nfsd4 grace period

[J. Bruce Fields]

From: Boaz Harrosh <bharrosh@panasas.com>

Even if we fail to write a recovery record, we should still mark the
client as having acquired its first state.  Otherwise we leave 4.1
clients with indefinite ERR_GRACE returns.

However, an inability to write stable storage records may cause failures
of reboot recovery, and the problem should still be brought to the
server administrator's attention.

So, make sure the error is logged.

These errors shouldn't normally be triggered on a corectly functioning
server--this isn't a case where a misconfigured client could spam the
logs.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/nfsd/nfs4recover.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 3e6ebcf..ed083b9 100644
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -130,6 +130,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 	if (!rec_file || clp->cl_firststate)
 		return 0;
 
+	clp->cl_firststate = 1;
 	status = nfs4_save_creds(&original_cred);
 	if (status < 0)
 		return status;
@@ -144,10 +145,8 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
 		goto out_unlock;
 	}
 	status = -EEXIST;
-	if (dentry->d_inode) {
-		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
+	if (dentry->d_inode)
 		goto out_put;
-	}
 	status = mnt_want_write(rec_file->f_path.mnt);
 	if (status)
 		goto out_put;
@@ -157,12 +156,14 @@ out_put:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);
-	if (status == 0) {
-		clp->cl_firststate = 1;
+	if (status == 0)
 		vfs_fsync(rec_file, 0);
-	}
+	else
+		printk(KERN_ERR "NFSD: failed to write recovery record"
+				" (err %d); please check that %s exists"
+				" and is writeable", status,
+				user_recovery_dirname);
 	nfs4_reset_creds(original_cred);
-	dprintk("NFSD: nfsd4_create_clid_dir returns %d\n", status);
 	return status;
 }
 
-- 
1.7.4.1

Re: [PATCH 2/2] nfsd4: fix failure to end nfsd4 grace period

[Boaz Harrosh]

On 08/26/2011 05:47 PM, J. Bruce Fields wrote:
> From: Boaz Harrosh <bharrosh@panasas.com>
> 
> Even if we fail to write a recovery record, we should still mark the
> client as having acquired its first state.  Otherwise we leave 4.1
> clients with indefinite ERR_GRACE returns.
> 
> However, an inability to write stable storage records may cause failures
> of reboot recovery, and the problem should still be brought to the
> server administrator's attention.
> 
> So, make sure the error is logged.
> 
> These errors shouldn't normally be triggered on a corectly functioning
> server--this isn't a case where a misconfigured client could spam the
> logs.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs4recover.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> index 3e6ebcf..ed083b9 100644
> --- a/fs/nfsd/nfs4recover.c
> +++ b/fs/nfsd/nfs4recover.c
> @@ -130,6 +130,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  	if (!rec_file || clp->cl_firststate)
>  		return 0;
>  
> +	clp->cl_firststate = 1;
>  	status = nfs4_save_creds(&original_cred);
>  	if (status < 0)
>  		return status;
> @@ -144,10 +145,8 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
>  		goto out_unlock;
>  	}
>  	status = -EEXIST;
> -	if (dentry->d_inode) {
> -		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
> +	if (dentry->d_inode)
>  		goto out_put;
> -	}
>  	status = mnt_want_write(rec_file->f_path.mnt);
>  	if (status)
>  		goto out_put;
> @@ -157,12 +156,14 @@ out_put:
>  	dput(dentry);
>  out_unlock:
>  	mutex_unlock(&dir->d_inode->i_mutex);
> -	if (status == 0) {
> -		clp->cl_firststate = 1;
> +	if (status == 0)
>  		vfs_fsync(rec_file, 0);
> -	}
> +	else
> +		printk(KERN_ERR "NFSD: failed to write recovery record"
> +				" (err %d); please check that %s exists"
> +				" and is writeable", status,
> +				user_recovery_dirname);

Whooff, hey thank you bruce, alot. I'm so sorry I assumed the name was
readily available only I have hard time finding it in 30 seconds of trying.
(So it was me, that's lazy). I did not mean for this to be so much work.

But the up side is that I'm sure some future admin's lives will be relieved
and that's Karma points ;-)

Thanks very much and ACK on both patches. 
Boaz

>  	nfs4_reset_creds(original_cred);
> -	dprintk("NFSD: nfsd4_create_clid_dir returns %d\n", status);
>  	return status;
>  }
>  

Re: [PATCH 2/2] nfsd4: fix failure to end nfsd4 grace period

[J. Bruce Fields]

On Fri, Aug 26, 2011 at 07:07:31PM -0700, Boaz Harrosh wrote:
> On 08/26/2011 05:47 PM, J. Bruce Fields wrote:
> > From: Boaz Harrosh <bharrosh@panasas.com>
> > 
> > Even if we fail to write a recovery record, we should still mark the
> > client as having acquired its first state.  Otherwise we leave 4.1
> > clients with indefinite ERR_GRACE returns.
> > 
> > However, an inability to write stable storage records may cause failures
> > of reboot recovery, and the problem should still be brought to the
> > server administrator's attention.
> > 
> > So, make sure the error is logged.
> > 
> > These errors shouldn't normally be triggered on a corectly functioning
> > server--this isn't a case where a misconfigured client could spam the
> > logs.
> > 
> > Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/nfsd/nfs4recover.c |   15 ++++++++-------
> >  1 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
> > index 3e6ebcf..ed083b9 100644
> > --- a/fs/nfsd/nfs4recover.c
> > +++ b/fs/nfsd/nfs4recover.c
> > @@ -130,6 +130,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
> >  	if (!rec_file || clp->cl_firststate)
> >  		return 0;
> >  
> > +	clp->cl_firststate = 1;
> >  	status = nfs4_save_creds(&original_cred);
> >  	if (status < 0)
> >  		return status;
> > @@ -144,10 +145,8 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
> >  		goto out_unlock;
> >  	}
> >  	status = -EEXIST;
> > -	if (dentry->d_inode) {
> > -		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
> > +	if (dentry->d_inode)
> >  		goto out_put;
> > -	}
> >  	status = mnt_want_write(rec_file->f_path.mnt);
> >  	if (status)
> >  		goto out_put;
> > @@ -157,12 +156,14 @@ out_put:
> >  	dput(dentry);
> >  out_unlock:
> >  	mutex_unlock(&dir->d_inode->i_mutex);
> > -	if (status == 0) {
> > -		clp->cl_firststate = 1;
> > +	if (status == 0)
> >  		vfs_fsync(rec_file, 0);
> > -	}
> > +	else
> > +		printk(KERN_ERR "NFSD: failed to write recovery record"
> > +				" (err %d); please check that %s exists"
> > +				" and is writeable", status,
> > +				user_recovery_dirname);
> 
> Whooff, hey thank you bruce, alot. I'm so sorry I assumed the name was
> readily available only I have hard time finding it in 30 seconds of trying.
> (So it was me, that's lazy). I did not mean for this to be so much work.

Well, I was exaggerating.

> But the up side is that I'm sure some future admin's lives will be relieved
> and that's Karma points ;-)
> 
> Thanks very much and ACK on both patches. 

OK thanks.

--b.