connectathon test failures

connectathon test failures

[J. Bruce Fields]

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 git-KezNCC8cqm5ZN1qrTdtDg5Vzexx5G7lz@public.gmane.org -----

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

Re: connectathon test failures

[J. Bruce Fields]

On Wed, Oct 08, 2008 at 06:55:43PM -0400, bfields wrote:
> 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.

Re: connectathon test failures

[J. Bruce Fields]

On Wed, Oct 08, 2008 at 10:03:00PM -0400, bfields wrote:
> On Wed, Oct 08, 2008 at 06:55:43PM -0400, bfields wrote:
> > 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.

Re: connectathon test failures

[Trond Myklebust]

On Wed, 2008-10-08 at 22:10 -0400, J. Bruce Fields wrote:
> On Wed, Oct 08, 2008 at 10:03:00PM -0400, bfields wrote:
> > On Wed, Oct 08, 2008 at 06:55:43PM -0400, bfields wrote:
> > > 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.

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 <Trond.Myklebust@netapp.com>
NFS: Fix attribute updates

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e25009f..d45e7b5 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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 */

-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: connectathon test failures

[J. Bruce Fields]

On Thu, Oct 09, 2008 at 10:36:25AM -0400, Trond Myklebust wrote:
> On Wed, 2008-10-08 at 22:10 -0400, J. Bruce Fields wrote:
> > On Wed, Oct 08, 2008 at 10:03:00PM -0400, bfields wrote:
> > > On Wed, Oct 08, 2008 at 06:55:43PM -0400, bfields wrote:
> > > > 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.
> 
> 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.

> ---------------------------------------------------------------------------------
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> NFS: Fix attribute updates
> 
> Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/inode.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index e25009f..d45e7b5 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -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 */
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com

Re: connectathon test failures

[Trond Myklebust]

On Thu, 2008-10-09 at 11:53 -0400, J. Bruce Fields wrote:
> On Thu, Oct 09, 2008 at 10:36:25AM -0400, Trond Myklebust wrote:
> > On Wed, 2008-10-08 at 22:10 -0400, J. Bruce Fields wrote:
> > > On Wed, Oct 08, 2008 at 10:03:00PM -0400, bfields wrote:
> > > > On Wed, Oct 08, 2008 at 06:55:43PM -0400, bfields wrote:
> > > > > 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.
> > 
> > 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.

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 <Trond.Myklebust@netapp.com>
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 <Trond.Myklebust@netapp.com>
---

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

diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
index e25009f..6554281 100644
--- a/fs/nfs/inode.c
+++ b/fs/nfs/inode.c
@@ -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 */


-- 
Trond Myklebust
Linux NFS client maintainer

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

Re: connectathon test failures

[J. Bruce Fields]

On Thu, Oct 09, 2008 at 01:36:13PM -0400, Trond Myklebust wrote:
> 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.

> 
> Cheers
>   Trond
> --------------------------------------------------------------------------
> From: Trond Myklebust <Trond.Myklebust@netapp.com>
> 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 <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/nfs/inode.c |   13 ++++---------
>  1 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index e25009f..6554281 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -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 */
> 
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer
> 
> NetApp
> Trond.Myklebust@netapp.com
> www.netapp.com
> --
> 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