Stephen McKay
2004-Apr-18 07:41 UTC
Should I merge fix for PR#64091 (NFS data corruption)?
Hi all!
This one only got fixed recently in -current, and I've only given it
a few hours of heavy testing, but the fix for PR#64091 looks to work
just as well on -stable, and I'd like to merge it.
The test programs in the PR provoke the bug in no more than a couple
of minutes in my tests (4.10-beta client, 4.9-release server). With
the attached patch (taken nearly literally from -current), it's been
happy for about 8 hours now.
I appreciate that nobody wants NFS destabilised so close to the (probably)
last ever 4.x release, so I'd like people to try to poke holes in this
before I pester re@ for permission to merge.
Give the test programs in http://www.freebsd.org/cgi/query-pr.cgi?pr=64091
a go on a 4.10-beta client, then try it with the attached patch.
I suppose I'm particularly looking for anything that gets worse after the
patch, but any result good or bad is of interest. In my tests, it fixes
the problem and doesn't noticeably increase network traffic or cause any
other problems. I'd just like to be sure. :-)
Stephen.
PS NWANTED (in nfsnode.h) is totally bogus. Luckily it is only used
in commented out code. Should really be cleaned up though.
------8<------ ------8<------ ------8<------ ------8<------
Index: nfs_bio.c
==================================================================RCS file:
/cvs/src/sys/nfs/Attic/nfs_bio.c,v
retrieving revision 1.83.2.4
diff -u -r1.83.2.4 nfs_bio.c
--- nfs_bio.c 29 Dec 2002 18:19:53 -0000 1.83.2.4
+++ nfs_bio.c 18 Apr 2004 11:50:03 -0000
@@ -401,13 +401,15 @@
error = VOP_GETATTR(vp, &vattr, cred, p);
if (error)
return (error);
- if (np->n_mtime != vattr.va_mtime.tv_sec) {
+ if ((np->n_flag & NSIZECHANGED)
+ || np->n_mtime != vattr.va_mtime.tv_sec) {
if (vp->v_type == VDIR)
nfs_invaldir(vp);
error = nfs_vinvalbuf(vp, V_SAVE, cred, p, 1);
if (error)
return (error);
np->n_mtime = vattr.va_mtime.tv_sec;
+ np->n_flag &= ~NSIZECHANGED;
}
}
}
Index: nfs_subs.c
==================================================================RCS file:
/cvs/src/sys/nfs/Attic/nfs_subs.c,v
retrieving revision 1.90.2.2
diff -u -r1.90.2.2 nfs_subs.c
--- nfs_subs.c 25 Oct 2001 19:18:53 -0000 1.90.2.2
+++ nfs_subs.c 18 Apr 2004 11:50:03 -0000
@@ -1335,12 +1335,19 @@
vap->va_size = np->n_size;
np->n_attrstamp = 0;
} else if (np->n_flag & NMODIFIED) {
- if (vap->va_size < np->n_size)
+ /*
+ * We've modified the file: Use the larger
+ * of our size, and the server's size.
+ */
+ if (vap->va_size < np->n_size) {
vap->va_size = np->n_size;
- else
+ } else {
np->n_size = vap->va_size;
+ np->n_flag |= NSIZECHANGED;
+ }
} else {
np->n_size = vap->va_size;
+ np->n_flag |= NSIZECHANGED;
}
vnode_pager_setsize(vp, np->n_size);
} else {
Index: nfsnode.h
==================================================================RCS file:
/cvs/src/sys/nfs/Attic/nfsnode.h,v
retrieving revision 1.32.2.1
diff -u -r1.32.2.1 nfsnode.h
--- nfsnode.h 26 Jun 2001 04:20:11 -0000 1.32.2.1
+++ nfsnode.h 18 Apr 2004 11:50:03 -0000
@@ -144,6 +144,7 @@
#define NCHG 0x0400 /* Special file times changed */
#define NLOCKED 0x0800 /* node is locked */
#define NWANTED 0x0100 /* someone wants to lock */
+#define NSIZECHANGED 0x2000 /* File size has changed: need cache inval */
/*
* Convert between nfsnode pointers and vnode pointers
------8<------ ------8<------ ------8<------ ------8<------
Eugene Grosbein
2004-Apr-18 08:09 UTC
Should I merge fix for PR#64091 (NFS data corruption)?
On Mon, Apr 19, 2004 at 12:38:47AM +1000, Stephen McKay wrote:> Give the test programs in http://www.freebsd.org/cgi/query-pr.cgi?pr=64091 > a go on a 4.10-beta client, then try it with the attached patch.Should one patch (and reboot in case of NFS compiled in a kernel) both of a client and a server? Eugene Grosbein
Andre Albsmeier
2004-Jun-22 06:51 UTC
Should I merge fix for PR#64091 (NFS data corruption)?
On Mon, 19-Apr-2004 at 00:38:47 +1000, Stephen McKay wrote:> Hi all! > > This one only got fixed recently in -current, and I've only given it > a few hours of heavy testing, but the fix for PR#64091 looks to work > just as well on -stable, and I'd like to merge it.Yes, please. I have been running the patch on -STABLE for a while now and haven't seen any bad effects so far. -Andre> > The test programs in the PR provoke the bug in no more than a couple > of minutes in my tests (4.10-beta client, 4.9-release server). With > the attached patch (taken nearly literally from -current), it's been > happy for about 8 hours now. > > I appreciate that nobody wants NFS destabilised so close to the (probably) > last ever 4.x release, so I'd like people to try to poke holes in this > before I pester re@ for permission to merge. > > Give the test programs in http://www.freebsd.org/cgi/query-pr.cgi?pr=64091 > a go on a 4.10-beta client, then try it with the attached patch. > > I suppose I'm particularly looking for anything that gets worse after the > patch, but any result good or bad is of interest. In my tests, it fixes > the problem and doesn't noticeably increase network traffic or cause any > other problems. I'd just like to be sure. :-) > > Stephen. > > PS NWANTED (in nfsnode.h) is totally bogus. Luckily it is only used > in commented out code. Should really be cleaned up though. > > ------8<------ ------8<------ ------8<------ ------8<------ > Index: nfs_bio.c > ==================================================================> RCS file: /cvs/src/sys/nfs/Attic/nfs_bio.c,v > retrieving revision 1.83.2.4 > diff -u -r1.83.2.4 nfs_bio.c > --- nfs_bio.c 29 Dec 2002 18:19:53 -0000 1.83.2.4 > +++ nfs_bio.c 18 Apr 2004 11:50:03 -0000 > @@ -401,13 +401,15 @@ > error = VOP_GETATTR(vp, &vattr, cred, p); > if (error) > return (error); > - if (np->n_mtime != vattr.va_mtime.tv_sec) { > + if ((np->n_flag & NSIZECHANGED) > + || np->n_mtime != vattr.va_mtime.tv_sec) { > if (vp->v_type == VDIR) > nfs_invaldir(vp); > error = nfs_vinvalbuf(vp, V_SAVE, cred, p, 1); > if (error) > return (error); > np->n_mtime = vattr.va_mtime.tv_sec; > + np->n_flag &= ~NSIZECHANGED; > } > } > } > Index: nfs_subs.c > ==================================================================> RCS file: /cvs/src/sys/nfs/Attic/nfs_subs.c,v > retrieving revision 1.90.2.2 > diff -u -r1.90.2.2 nfs_subs.c > --- nfs_subs.c 25 Oct 2001 19:18:53 -0000 1.90.2.2 > +++ nfs_subs.c 18 Apr 2004 11:50:03 -0000 > @@ -1335,12 +1335,19 @@ > vap->va_size = np->n_size; > np->n_attrstamp = 0; > } else if (np->n_flag & NMODIFIED) { > - if (vap->va_size < np->n_size) > + /* > + * We've modified the file: Use the larger > + * of our size, and the server's size. > + */ > + if (vap->va_size < np->n_size) { > vap->va_size = np->n_size; > - else > + } else { > np->n_size = vap->va_size; > + np->n_flag |= NSIZECHANGED; > + } > } else { > np->n_size = vap->va_size; > + np->n_flag |= NSIZECHANGED; > } > vnode_pager_setsize(vp, np->n_size); > } else { > Index: nfsnode.h > ==================================================================> RCS file: /cvs/src/sys/nfs/Attic/nfsnode.h,v > retrieving revision 1.32.2.1 > diff -u -r1.32.2.1 nfsnode.h > --- nfsnode.h 26 Jun 2001 04:20:11 -0000 1.32.2.1 > +++ nfsnode.h 18 Apr 2004 11:50:03 -0000 > @@ -144,6 +144,7 @@ > #define NCHG 0x0400 /* Special file times changed */ > #define NLOCKED 0x0800 /* node is locked */ > #define NWANTED 0x0100 /* someone wants to lock */ > +#define NSIZECHANGED 0x2000 /* File size has changed: need cache inval */ > > /* > * Convert between nfsnode pointers and vnode pointers > ------8<------ ------8<------ ------8<------ ------8<------ > _______________________________________________ > freebsd-stable@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-stable > To unsubscribe, send any mail to "freebsd-stable-unsubscribe@freebsd.org"