* connectathon test failures @ 2008-10-08 22:55 J. Bruce Fields 2008-10-09 2:03 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2008-10-08 22:55 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-08 22:55 connectathon test failures J. Bruce Fields @ 2008-10-09 2:03 ` J. Bruce Fields 2008-10-09 2:10 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2008-10-09 2:03 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-09 2:03 ` J. Bruce Fields @ 2008-10-09 2:10 ` J. Bruce Fields 2008-10-09 14:36 ` Trond Myklebust 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2008-10-09 2:10 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-09 2:10 ` J. Bruce Fields @ 2008-10-09 14:36 ` Trond Myklebust 2008-10-09 15:53 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Trond Myklebust @ 2008-10-09 14:36 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-09 14:36 ` Trond Myklebust @ 2008-10-09 15:53 ` J. Bruce Fields 2008-10-09 17:36 ` Trond Myklebust 0 siblings, 1 reply; 7+ messages in thread From: J. Bruce Fields @ 2008-10-09 15:53 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-09 15:53 ` J. Bruce Fields @ 2008-10-09 17:36 ` Trond Myklebust 2008-10-09 19:23 ` J. Bruce Fields 0 siblings, 1 reply; 7+ messages in thread From: Trond Myklebust @ 2008-10-09 17:36 UTC (permalink / raw) To: J. Bruce Fields; +Cc: linux-nfs 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: connectathon test failures 2008-10-09 17:36 ` Trond Myklebust @ 2008-10-09 19:23 ` J. Bruce Fields 0 siblings, 0 replies; 7+ messages in thread From: J. Bruce Fields @ 2008-10-09 19:23 UTC (permalink / raw) To: Trond Myklebust; +Cc: linux-nfs 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2008-10-09 19:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-10-08 22:55 connectathon test failures J. Bruce Fields 2008-10-09 2:03 ` J. Bruce Fields 2008-10-09 2:10 ` J. Bruce Fields 2008-10-09 14:36 ` Trond Myklebust 2008-10-09 15:53 ` J. Bruce Fields 2008-10-09 17:36 ` Trond Myklebust 2008-10-09 19:23 ` J. Bruce Fields
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.