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

RFC In review Boaz Harrosh 2 participants 8 messages Aug 13, 2011 – Aug 27, 2011

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

Boaz Harrosh

author

2011-08-13 00:30 UTC #0 lore raw

RFC 1/1 nfsd4: This is: "fix failure to end nfsd4 grace period" ++

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" <[email protected]>
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 <[email protected]>

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 <[email protected]>

---
 fs/nfsd/nfs4recover.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

fs/nfsd/nfs4recover.c +13 -6

index 29d77f6..baedd89 100644
@@ -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;

Signature · 1 line

1.7.6

J. Bruce Fields

2011-08-26 20:19 UTC #1 lore raw

Quoted 5 lines · 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.

Makes sense, thanks.

Quoted 3 lines

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 <[email protected]>
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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

fs/nfsd/nfs4recover.c +8 -7

index 02eb38e..1ea241a 100644
@@ -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;
 }

Boaz Harrosh

author

2011-08-27 00:23 UTC #2 lore raw

Quoted 1 line · J. Bruce Fields

On Fri, Aug 12, 2011 at 05:30:12PM -0700, Boaz Harrosh wrote:

<snip>

Quoted 4 lines

fs/nfsd/nfs4recover.c +0 -0

@@ -156,12 +155,14 @@ out_put:
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&dir->d_inode->i_mutex);

fs/nfsd/nfs4recover.c out_put: · line 164

Quoted 10 lines

fs/nfsd/nfs4recover.c +6 -3

-	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

Quoted 5 lines

fs/nfsd/nfs4recover.c +0 -1

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

J. Bruce Fields

2011-08-27 00:46 UTC #3 lore raw

Quoted 4 lines · Boaz Harrosh

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.

J. Bruce Fields

2011-08-27 00:47 UTC #4 lore raw

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

From: J. Bruce Fields <[email protected]>

Move around some of this code, simplify a bit.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4recover.c |   36 ++++++++++++++++++++++++++++++++----
 fs/nfsd/nfs4state.c   |   41 +----------------------------------------
 fs/nfsd/state.h       |    2 +-
 3 files changed, 34 insertions(+), 45 deletions(-)

fs/nfsd/nfs4recover.c +32 -4

index 02eb38e..3e6ebcf 100644
@@ -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;
+}

fs/nfsd/nfs4state.c +1 ~~-40~~

56 lines not on this page · raw message

fs/nfsd/state.h +1 -1

index 1f90cab..66179ac 100644
@@ -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);

Signature · 1 line

1.7.4.1

J. Bruce Fields

2011-08-27 00:47 UTC #5 lore raw

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

From: Boaz Harrosh <[email protected]>

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 <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4recover.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

fs/nfsd/nfs4recover.c +8 -7

index 3e6ebcf..ed083b9 100644
@@ -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;
 }

Signature · 1 line

1.7.4.1

Boaz Harrosh

author

2011-08-27 02:07 UTC #6 lore raw

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

Quoted 50 lines · J. Bruce Fields

From: Boaz Harrosh <[email protected]>
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 <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4recover.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
fs/nfsd/nfs4recover.c +2 -3
index 3e6ebcf..ed083b9 100644
@@ -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);

fs/nfsd/nfs4recover.c out_put: · line 165

Quoted 10 lines

fs/nfsd/nfs4recover.c +6 -3

-	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

Quoted 5 lines

fs/nfsd/nfs4recover.c +0 -1

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

J. Bruce Fields

2011-08-27 02:13 UTC #7 lore raw

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

Quoted 57 lines · Boaz Harrosh

Quoted 56 lines · J. Bruce Fields
From: Boaz Harrosh <[email protected]>
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 <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4recover.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)
fs/nfsd/nfs4recover.c +4 -6
index 3e6ebcf..ed083b9 100644
@@ -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

Quoted 8 lines

fs/nfsd/nfs4recover.c out_put: · line 165
Quoted 4 lines
fs/nfsd/nfs4recover.c +4 -0
+		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.

Quoted 4 lines

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.