connectathon test failures

DISCUSSION J. Bruce Fields 2 participants 7 messages Oct 8, 2008 – Oct 9, 2008

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2008-10-08 22:55 UTC #0 lore raw

I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.

I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).

--b.

----- Forwarded message from [email protected] -----

Linux ying3 2.6.27-rc9-00096-gb6ae157 #77 PREEMPT Wed Oct 8 18:24:11 EDT 2008 i686 GNU/Linux
cthon: local filesystem
cthon: nfsv4

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.1 has 1 links after link (expect 2) : No such file or directory
basic tests failed
cthon: nfsv3

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.0 has 1 links after link (expect 2) : No such file or directory
basic tests failed
cthon: nfsv4/krb5

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.1 has 1 links after link (expect 2) : No such file or directory
basic tests failed
cthon: nfsv3/krb5

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.0 has 1 links after link (expect 2) : No such file or directory
basic tests failed
cthon: nfsv4/krb5i

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.1 has 1 links after link (expect 2) : No such file or directory
basic tests failed
cthon: nfsv4/krb5p

Starting BASIC tests: test directory /mnt/TMP (arg: -f)

./test1: File and directory creation test

	created 6 files 6 directories 2 levels deep
	./test1 ok.

./test2: File and directory removal test

	removed 6 files 6 directories 2 levels deep
	./test2 ok.

./test3: lookups across mount point

	2 getcwd and stat calls
	./test3 ok.

./test4: setattr, getattr, and lookup

	20 chmods and stats on 10 files
	./test4 ok.

./test5: read and write

	wrote 1048576 byte file 1 times
	read 1048576 byte file 1 times
	./test5 ok.

./test6: readdir

	202 entries read, 200 files
	./test6 ok.

./test7: link and rename
./test7: (/mnt/TMP) newfile.1 has 1 links after link (expect 2) : No such file or directory
basic tests failed

J. Bruce Fields

author

2008-10-09 02:03 UTC #1 lore raw

Quoted 7 lines · bfields

I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.
I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).

The failure first happens with d65f557f3 "NFS: Fix nfs_post_op_update_inode_force_wcc()".

--b.

J. Bruce Fields

author

2008-10-09 02:10 UTC #2 lore raw

Quoted 11 lines · bfields

Quoted 7 lines · bfields
I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.
I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).
The failure first happens with d65f557f3 "NFS: Fix nfs_post_op_update_inode_force_wcc()".

Err, sorry, no: that was the last *good* one! It's the following one ( (4dc05efb86 "NFS: Convert __nfs_revalidate_inode() to use nfs_refresh_inode()") where the failures start.

--b.

Trond Myklebust

2008-10-09 14:36 UTC #3 lore raw

Quoted 10 lines · J. Bruce Fields

Quoted 9 lines · bfields
Quoted 7 lines · bfields
I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.
I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).

Quoted 6 lines

Quoted 2 lines
The failure first happens with d65f557f3 "NFS: Fix nfs_post_op_update_inode_force_wcc()".
Err, sorry, no: that was the last *good* one! It's the following one ( (4dc05efb86 "NFS: Convert __nfs_revalidate_inode() to use nfs_refresh_inode()") where the failures start.

Hmm... That patch should be correct. I rather suspect a problem with the updating of nfsi->last_updated.

Could you see if something like the following patch helps?
---------------------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
NFS: Fix attribute updates

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/inode.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

fs/nfs/inode.c +6 -6

index e25009f..d45e7b5 100644
@@ -933,10 +933,10 @@ static int nfs_inode_attrs_need_update(const struct inode *inode, const struct n
 {
 	const struct nfs_inode *nfsi = NFS_I(inode);
 
-	return nfs_ctime_need_update(inode, fattr) ||
-			nfs_size_need_update(inode, fattr) ||
-			time_after(fattr->time_start, nfsi->last_updated) ||
-			time_after(nfsi->last_updated, jiffies);
+	return time_after(fattr->time_start, nfsi->last_updated) ||
+		nfs_ctime_need_update(inode, fattr) ||
+		nfs_size_need_update(inode, fattr) ||
+		time_after(nfsi->last_updated, jiffies);
 }
 
 static int nfs_refresh_inode_locked(struct inode *inode, struct nfs_fattr *fattr)

@@ -1170,8 +1170,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 		/*
 		 * Avoid jiffy wraparound issues with nfsi->last_updated
 		 */
-		if (!time_in_range(nfsi->last_updated, nfsi->read_cache_jiffies, now))
-			nfsi->last_updated = nfsi->read_cache_jiffies;
+		if (!time_in_range(nfsi->last_updated, now - NFS_MAXATTRTIMEO(inode), now))
+			nfsi->last_updated = now - NFS_MAXATTRTIMEO(inode);
 	}
 	invalid &= ~NFS_INO_INVALID_ATTR;
 	/* Don't invalidate the data if we were to blame */

Signature · 6 lines

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

J. Bruce Fields

author

2008-10-09 15:53 UTC #4 lore raw

Quoted 14 lines · Trond Myklebust

Quoted 13 lines · J. Bruce Fields
Quoted 11 lines · bfields
Quoted 7 lines · bfields
I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.
I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).
The failure first happens with d65f557f3 "NFS: Fix nfs_post_op_update_inode_force_wcc()".

Quoted 8 lines

Quoted 3 lines
Err, sorry, no: that was the last *good* one! It's the following one ( (4dc05efb86 "NFS: Convert __nfs_revalidate_inode() to use nfs_refresh_inode()") where the failures start.
Hmm... That patch should be correct. I rather suspect a problem with the updating of nfsi->last_updated.
Could you see if something like the following patch helps?

Yep, if I apply this on top of your linux-mm (19d771f3caccaf66ce2fb539319222139e5b4e88) then I can no longer see the failure.

--b.

Quoted 48 lines

---------------------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
NFS: Fix attribute updates

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/inode.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

fs/nfs/inode.c +6 -6

index e25009f..d45e7b5 100644
@@ -933,10 +933,10 @@ static int nfs_inode_attrs_need_update(const struct inode *inode, const struct n
 {
 	const struct nfs_inode *nfsi = NFS_I(inode);
 
-	return nfs_ctime_need_update(inode, fattr) ||
-			nfs_size_need_update(inode, fattr) ||
-			time_after(fattr->time_start, nfsi->last_updated) ||
-			time_after(nfsi->last_updated, jiffies);
+	return time_after(fattr->time_start, nfsi->last_updated) ||
+		nfs_ctime_need_update(inode, fattr) ||
+		nfs_size_need_update(inode, fattr) ||
+		time_after(nfsi->last_updated, jiffies);
 }
 
 static int nfs_refresh_inode_locked(struct inode *inode, struct nfs_fattr *fattr)

@@ -1170,8 +1170,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 		/*
 		 * Avoid jiffy wraparound issues with nfsi->last_updated
 		 */
-		if (!time_in_range(nfsi->last_updated, nfsi->read_cache_jiffies, now))
-			nfsi->last_updated = nfsi->read_cache_jiffies;
+		if (!time_in_range(nfsi->last_updated, now - NFS_MAXATTRTIMEO(inode), now))
+			nfsi->last_updated = now - NFS_MAXATTRTIMEO(inode);
 	}
 	invalid &= ~NFS_INO_INVALID_ATTR;
 	/* Don't invalidate the data if we were to blame */

Signature · 7 lines

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

Trond Myklebust

2008-10-09 17:36 UTC #5 lore raw

Quoted 22 lines · J. Bruce Fields

Quoted 21 lines · Trond Myklebust
Quoted 10 lines · J. Bruce Fields
Quoted 9 lines · bfields
Quoted 7 lines · bfields
I'm getting connectathon test failures with the latest merge of code from both our trees, linux-to-linux over v3 and v4.
I just got this a moment ago, so haven't tried to track it down at all yet--but I seem to recall in the past this sort of failure has been an attribute revalidation problem (usually with the ext3 ctime resolution at least partly to blame).
Quoted 6 lines
Quoted 2 lines
The failure first happens with d65f557f3 "NFS: Fix nfs_post_op_update_inode_force_wcc()".
Err, sorry, no: that was the last *good* one! It's the following one ( (4dc05efb86 "NFS: Convert __nfs_revalidate_inode() to use nfs_refresh_inode()") where the failures start.
Hmm... That patch should be correct. I rather suspect a problem with the updating of nfsi->last_updated.

Quoted 7 lines

Quoted 1 line
Could you see if something like the following patch helps?
Yep, if I apply this on top of your linux-mm (19d771f3caccaf66ce2fb539319222139e5b4e88) then I can no longer see the failure.
--b.

OK. Having thought a little bit more about this, I think we can just delete the wraparound protection in nfs_update_inode(). We already have similar protection in nfs_inode_attrs_need_update()...

Cheers
Trond
--------------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
Date: Thu, 9 Oct 2008 13:27:55 -0400
NFS: Fix attribute updates

This fixes a regression seen when running the Connectathon testsuite against an ext3 filesystem. The reason was that the inode was constantly being marked as 'just updated' by the jiffy wraparound test. This again meant that newer GETATTR calls were failing to pass the nfs_inode_attrs_need_update() test unless the changes caused a ctime update on the server, since they were perceived as having been started before the latest inode update.

Given that nfs_inode_attrs_need_update() already checks for wraparound of nfsi->last_updated, we can drop the buggy "protection" in nfs_update_inode().

Also make a slight micro-optimisation of nfs_inode_attrs_need_update(): we are more often going to see time_after(fattr->time_start, nfsi->last_updated) be true, rather than seeing an update of ctime/size, so put that test first to ensure that we optimise away the ctime/size tests.

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/nfs/inode.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

fs/nfs/inode.c +4 -9

index e25009f..6554281 100644
@@ -933,10 +933,10 @@ static int nfs_inode_attrs_need_update(const struct inode *inode, const struct n
 {
 	const struct nfs_inode *nfsi = NFS_I(inode);
 
-	return nfs_ctime_need_update(inode, fattr) ||
-			nfs_size_need_update(inode, fattr) ||
-			time_after(fattr->time_start, nfsi->last_updated) ||
-			time_after(nfsi->last_updated, jiffies);
+	return time_after(fattr->time_start, nfsi->last_updated) ||
+		nfs_ctime_need_update(inode, fattr) ||
+		nfs_size_need_update(inode, fattr) ||
+		time_after(nfsi->last_updated, jiffies);
 }
 
 static int nfs_refresh_inode_locked(struct inode *inode, struct nfs_fattr *fattr)

@@ -1167,11 +1167,6 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 				nfsi->attrtimeo = NFS_MAXATTRTIMEO(inode);
 			nfsi->attrtimeo_timestamp = now;
 		}
-		/*
-		 * Avoid jiffy wraparound issues with nfsi->last_updated
-		 */
-		if (!time_in_range(nfsi->last_updated, nfsi->read_cache_jiffies, now))
-			nfsi->last_updated = nfsi->read_cache_jiffies;
 	}
 	invalid &= ~NFS_INO_INVALID_ATTR;
 	/* Don't invalidate the data if we were to blame */

Signature · 6 lines

Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com

J. Bruce Fields

author

2008-10-09 19:24 UTC #6 lore raw

Quoted 3 lines · Trond Myklebust

OK. Having thought a little bit more about this, I think we can just delete the wraparound protection in nfs_update_inode(). We already have similar protection in nfs_inode_attrs_need_update()...

I retested with this patch, and it passed. Thanks!

--b.

Quoted 75 lines

Cheers
Trond
--------------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
Date: Thu, 9 Oct 2008 13:27:55 -0400
NFS: Fix attribute updates
This fixes a regression seen when running the Connectathon testsuite against an ext3 filesystem. The reason was that the inode was constantly being marked as 'just updated' by the jiffy wraparound test. This again meant that newer GETATTR calls were failing to pass the nfs_inode_attrs_need_update() test unless the changes caused a ctime update on the server, since they were perceived as having been started before the latest inode update.
Given that nfs_inode_attrs_need_update() already checks for wraparound of nfsi->last_updated, we can drop the buggy "protection" in nfs_update_inode().
Also make a slight micro-optimisation of nfs_inode_attrs_need_update(): we are more often going to see time_after(fattr->time_start, nfsi->last_updated) be true, rather than seeing an update of ctime/size, so put that test first to ensure that we optimise away the ctime/size tests.
Signed-off-by Trond Myklebust <[email protected]>
---
 fs/nfs/inode.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)
fs/nfs/inode.c +4 -9
index e25009f..6554281 100644
@@ -933,10 +933,10 @@ static int nfs_inode_attrs_need_update(const struct inode *inode, const struct n
 {
 	const struct nfs_inode *nfsi = NFS_I(inode);
 
-	return nfs_ctime_need_update(inode, fattr) ||
-			nfs_size_need_update(inode, fattr) ||
-			time_after(fattr->time_start, nfsi->last_updated) ||
-			time_after(nfsi->last_updated, jiffies);
+	return time_after(fattr->time_start, nfsi->last_updated) ||
+		nfs_ctime_need_update(inode, fattr) ||
+		nfs_size_need_update(inode, fattr) ||
+		time_after(nfsi->last_updated, jiffies);
 }
 
 static int nfs_refresh_inode_locked(struct inode *inode, struct nfs_fattr *fattr)
@@ -1167,11 +1167,6 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
 				nfsi->attrtimeo = NFS_MAXATTRTIMEO(inode);
 			nfsi->attrtimeo_timestamp = now;
 		}
-		/*
-		 * Avoid jiffy wraparound issues with nfsi->last_updated
-		 */
-		if (!time_in_range(nfsi->last_updated, nfsi->read_cache_jiffies, now))
-			nfsi->last_updated = nfsi->read_cache_jiffies;
 	}
 	invalid &= ~NFS_INO_INVALID_ATTR;
 	/* Don't invalidate the data if we were to blame */
Signature · 11 lines
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html